Re: [PATCH 2/6] hwmon: (pmbus/max20830): add support for enable GPIO

[Guenter Roeck]

On 6/29/26 20:50, Guenter Roeck wrote:
> On 6/29/26 19:46, Alexis Czezar Torreno wrote:
> > Add support for the GPIO controlled EN pin. The EN pin is asserted high
> > for device to operate.
> >
> > Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@xxxxxxxxxx>
> > ---
> >   drivers/hwmon/pmbus/max20830.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
> > index cb2c23672166d641852199ca07eb716924f4f286..cb3a39d747edee3aefb0fb4051ef957436b3c15b 100644
> > --- a/drivers/hwmon/pmbus/max20830.c
> > +++ b/drivers/hwmon/pmbus/max20830.c
> > @@ -6,6 +6,7 @@
> >    */
> >   #include <linux/errno.h>
> > +#include <linux/gpio/consumer.h>
> >   #include <linux/i2c.h>
> >   #include <linux/mod_devicetable.h>
> >   #include <linux/module.h>
> > @@ -29,8 +30,14 @@ static struct pmbus_driver_info max20830_info = {
> >   static int max20830_probe(struct i2c_client *client)
> >   {
> >       u8 buf[I2C_SMBUS_BLOCK_MAX + 1] = {};
> > +    struct gpio_desc *enable_gpio;
> >       int ret;
> > +    enable_gpio = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
> > +    if (IS_ERR(enable_gpio))
> > +        return dev_err_probe(&client->dev, PTR_ERR(enable_gpio),
> > +                     "Failed to get enable GPIO\n");
> > +
>
> The above code gets the gpio reference, and then it doesn't do anything
> with it. What exactly is the point of this exercise ? Where is the
> chip actually enabled ?
>
> Do you have an actual customer with such a set-up or is this
> "just in case" ? Have you tested this code to ensure that the chip
> is actually enabled in this setup ?

Also, please explain the need in detail, especially in the context of
PMBus command 0x02 (ON_OFF_CONFIG) which can be used to configure
the pin functionality. Specifically, what would be the point of
trying to force-enable the chip if on-off-config happens to be set
to 0x1b (ignore EN pin and require OPERATION = 0x80) ?
And if ON_OFF_CONFIG happens to be set to 0x17 (Ignore OPERATION
command), why not just set it to 0x1b and override EN ?

In other words, I expect the use case to be explained in the context
of the ON_OFF_CONFIG and OPERATION commands. "The EN pin is asserted
high for device to operate" is misleading and only half-true since that
is only the case if the chip is configured to actually use it (which,
I notice, you are not doing here).

Thanks,
Guenter

> If there is indeed a use case where a customer indeed connects the
> enable pin to a gpio output, wouldn't that same customer also want
> to connect the "pgood" output to a gpio pin ? And what about
> the LDOIN pin ? Shouldn't that be connected to a power supply ?
>
> Guenter