Re: [PATCH] [v1 1/1]hwmon:(pmbus) Validate the data for chip supporting vout_mode (PMBUS_HAVE_VOUT) in the linear config.

[Guenter Roeck]

On 10/11/22 16:59, karthik gengan wrote:
> Linear mode config calculation is based on the exponent value
> derived from vout_mode. So vout_mode value should be valid
> for PMBUS_VOUT_MODE command-supported chips.

We can not do this. The operational word is "should". See comment
below "Not all chips support the VOUT_MODE command". It is what it is.
We can not just refuse to support such chips because they don't
support what we expect them to support.

Sure, those chips will (likely) report wrong values since the
exponent will default to 0. That can be adjusted in user space,
or whoever finds such a chip can provide a back-end driver
with the appropriate values configured (for example by providing
a dummy VOUT_MODE command response). That is better than just
rejecting the chip entirely.

From a practical perspective, if you know about an affected chip
one that would refuse to instantiate after your patch is applied,
I would suggest to submit (or improve) a back-end driver with
an explanation instead.

Thanks,
Guenter

> Signed-off-by: karthik.gengan <gengankarthik@xxxxxxxxx <mailto:gengankarthik@xxxxxxxxx>>
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 7ec04934747e..5f80c3b8f245 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2507,9 +2507,17 @@ static int pmbus_identify_common(struct i2c_client *client,
>   {
>      int vout_mode = -1;
>
> -   if (pmbus_check_byte_register(client, page, PMBUS_VOUT_MODE))
> +   if (pmbus_check_byte_register(client, page, PMBUS_VOUT_MODE)) {
>          vout_mode = _pmbus_read_byte_data(client, page,
>                            PMBUS_VOUT_MODE);
> +       /*
> +        * If the client page supports PMBUS_VOUT_MODE,
> +        * then the output of the VOUT_MODE command should
> +        * be a valid value for linear mode calculation.
> +        */
> +       if ((data->info->format[PSC_VOLTAGE_OUT] == linear) && (vout_mode < 0))
> +           return -ENODEV;
> +   }
>      if (vout_mode >= 0 && vout_mode != 0xff) {
>          /*
>           * Not all chips support the VOUT_MODE command,
> --
> 2.25.1