Re: [PATCH] usb: typec: tcpm: Report back negotiated PPS voltage and current

[Guenter Roeck]

On Fri, Sep 28, 2018 at 04:33:49PM +0100, Adam Thomson wrote:
> Currently when requesting a specific voltage or current through
> the psy interface, for PPS, when reading back from that interface
> the values will always be the same as previously given, if the
> request was successful. However PPS only allows for 20mV voltage
> steps and 50mA current steps, and the psy class expects microvolt
> and micro amp requests, so inbetween values can be provided through
> this interface. Really when reading back the true values negotiated
> should be given, and not the ones originally asked for.
> 
> To report the actual values negotiated with the Source, the values
> stored are now rounded down to the relevant step units prior to
> building the PPS request, so that those values are later correctly
> reported through the psy interface. In addition this improves the
> adjustments made to meet the operating power requirements of the
> platform, which previously could have been slightly out due to not
> using valid PPS units of voltage and current.
> 
> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@xxxxxxxxxxx>
> ---
> Changes are based on usb-testing (ae8a2ca8a221)
> 
>  drivers/usb/typec/tcpm/tcpm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 4f1f421..33b3981 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2433,6 +2433,10 @@ static int tcpm_pd_build_pps_request(struct tcpm_port *port, u32 *rdo)
>  
>  	flags = RDO_USB_COMM | RDO_NO_SUSPEND;
>  
> +	/* Round down output voltage and current to align with PPS valid steps */
> +	out_mv = out_mv - (out_mv % RDO_PROG_VOLT_MV_STEP);
> +	op_ma = op_ma - (op_ma % RDO_PROG_CURR_MA_STEP);
> +

The original intent was to have out_mv (ie out_volt) set to valid values.
Some remnants of that are still in the code.

	out_mv += RDO_PROG_VOLT_MV_STEP - (out_mv % RDO_PROG_VOLT_MV_STEP);

	port->pps_data.out_volt =
		min(pdo_pps_apdo_max_voltage(pdo), port->pps_data.out_volt);

Somehow that got lost over time. Personally I think it would be better to set 
port->pps_data.out_vol and port->pps_data.op_curr to valid step sizes instead
of post-adjusting the values when used.

Thanks,
Guenter

>  	op_mw = (op_ma * out_mv) / 1000;
>  	if (op_mw < port->operating_snk_mw) {
>  		/*
> -- 
> 1.9.1
>