Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

[Guenter Roeck]

On Wed, Nov 15, 2023 at 08:42:22AM +0000, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> > Sent: Tuesday, November 7, 2023 11:30 AM
> > To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@xxxxxxxxxx>;
> > patrick@xxxxxxxxx; Jean Delvare <jdelvare@xxxxxxxx>; Jonathan Corbet
> > <corbet@xxxxxxx>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>;
> > linux-i2c@xxxxxxxxxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > linux-doc@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> > 
> >   Security Reminder: Please be aware that this email is sent by an external
> > sender.
> > 
> > On 11/6/23 19:08, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> > >> -----Original Message-----
> > >> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> > >> Sent: Tuesday, October 31, 2023 9:47 PM
> > >> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@xxxxxxxxxx>;
> > >> patrick@xxxxxxxxx; Jean Delvare <jdelvare@xxxxxxxx>; Jonathan Corbet
> > >> <corbet@xxxxxxx>
> > >> Cc: Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> > >> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley
> > >> <conor+dt@xxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx;
> > >> linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > >> linux-kernel@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx
> > >> Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> > >>
> > >>    Security Reminder: Please be aware that this email is sent by an
> > >> external sender.
> > >>
> > >> On 10/30/23 23:46, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> > >> [ ... ]
> > >>>>
> > >>>>> +
> > >>>>> +     ret = of_property_read_u32(client->dev.of_node,
> > >>>>> +                                "shunt-resistor-micro-ohms",
> > >>>> &rsense);
> > >>>>> +     if (ret < 0)
> > >>>>> +             return ret;
> > >>>>> +
> > >>>>> +     if (rsense == 0)
> > >>>>> +             return -EINVAL;
> > >>>>> +
> > >>>>> +     info = &ltc4286_info;
> > >>>>> +
> > >>>>> +     /* Default of VRANGE_SELECT = 1, 102.4V */
> > >>>>> +     if (device_property_read_bool(&client->dev,
> > >>>> "adi,vrange-select-25p6")) {
> > >>>>
> > >>>> What if the adi,vrange-select-25p6 property is not provided, but
> > >>>> the chip is programmed for this range ?
> > >>> The binding document tells programmers how to fill the dts.
> > >>> Thus, programmers must fill this property if their system is 25.6
> > >>> volts voltage
> > >> range.
> > >>>
> > >>
> > >> Sure, but there is no else case, meaning VRANGE_SELECT is unmodified
> > >> in that case. There is no guarantee that the chip is in its power-on state.
> > >
> > > The else case is in v2 ltc4286.c line 133 It means that the voltage
> > > range for programmer is 102.4 volts which is default value, so driver
> > > doesn't need to do any change for VRANGE_SELECT bit.
> > 
> > There is no guarantee that the value wasn't changed before the driver was
> > loaded.
> 
> We still can’t get your point.
> Could you tell us about your concern here?

I have repeated it several times. You are making assumptions about
register values when the driver is loaded. Those asumptions
are wrong since the state of the chip is unknown when the driver
is loaded. Any entty (BIOS, ROMMON, i2cset, some operating system
loaded earlier, or even some other driver or platform code) may
have changed those values.

On top of that, as I also have pointed out, LTC4287 supports
saving its configuration data in eeprom. That means that any chip
configuration set during production or anytime later will be
retained, meaning any assumption about chip configuration
when the driver is loaded is even more wrong.

Guenter