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

[karthik gengan]

Hi Guenter,

Thanks for your response but I need some more valuable suggestions...

        In the pmbus_data structure, we are updating the vout_out mode
value based on the config ( linear, vid .direct) and this structure is declared
in pmbus_core.c .so the low-level driver could not able to access this
structure.

Guidance required,
     1. Move the pumbus_data structure declaration to pmbus.h, so that low-level
          the driver can check and update the vout_mode if the value
is not proper.   (or)
     2.  if the vout_mode attribute value is exposed in the sysfs, the
user-level application
          can modify the value. ( if the value is not proper).
                               (or)
      3. if an ioctl call is exposed for the vout_mode then the
user-level application can
           able to update the value. ( if the value is not proper).

(or)  guide me to a better approach to handle this issue.

For reference,
          UCD90xx vendor claims that vout_mode value should be present
if the chip
supports PMBUS_VOUT_MODE command. Hence the patch validated the vout_mode
value for PMBUS_VOUT_MODE supported chip.

Thanks & Regards,
Karthik.G

On Wed, Oct 12, 2022 at 7:05 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> 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
> >
>
>