Re: [PATCH v2 3/3] hwmon:(pmbus/xdpe1a2g7b) Add support for xdpe1a2g5b/7b controllers

[ashish yadav]

Hi Guenter,

Thanks for your time and feedback.
Please find my response inline.

With Best Regards,
    Ashish Yadav


On Sun, Feb 22, 2026 at 4:52 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 2/19/26 02:56, ASHISH YADAV wrote:
> > From: Ashish Yadav <ashish.yadav@xxxxxxxxxxxx>
> >
> > Add the pmbus driver for Infineon Digital Multi-phase XDPE1A2G5B and
> > XDPE1A2G7B controllers.
> >
> > Signed-off-by: Ashish Yadav <ashish.yadav@xxxxxxxxxxxx>
> > ---
> > XDPE1A2G5B and XDPE1A2G7B support both Linear and NVIDIA PWM VID Data
> > formats for VOUT_MODE.
> > The configuring both loops/pages of the device independently is not
> > supported for VOUT_MODE.
> > In case of vid mode, NVIDIA PWM VID vrm_version is supported:
> > Vout = 5mV * (VID-1) + 195mV
> > ---
> >   drivers/hwmon/pmbus/Kconfig      |   9 +++
> >   drivers/hwmon/pmbus/Makefile     |   1 +
> >   drivers/hwmon/pmbus/xdpe1a2g7b.c | 120 +++++++++++++++++++++++++++++++
> >   3 files changed, 130 insertions(+)
> >   create mode 100644 drivers/hwmon/pmbus/xdpe1a2g7b.c
> >
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index fc1273abe357..a4513fc6bc26 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -711,6 +711,15 @@ config SENSORS_XDPE152
> >         This driver can also be built as a module. If so, the module will
> >         be called xdpe152c4.
> >
> > +config SENSORS_XDPE1A2G7B
> > +     tristate "Infineon XDPE1A2G7B"
> > +     help
> > +       If you say yes here you get hardware monitoring support for Infineon
> > +       XDPE1A2G5B and XDPE1A2G7B.
> > +
> > +       This driver can also be built as a module. If so, the module will
> > +       be called xdpe1a2g7b.
> > +
> >   config SENSORS_XDPE122
> >       tristate "Infineon XDPE122 family"
> >       help
> > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> > index d6c86924f887..d592d8c77bec 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -70,6 +70,7 @@ obj-$(CONFIG_SENSORS_UCD9200)       += ucd9200.o
> >   obj-$(CONFIG_SENSORS_XDP710)        += xdp710.o
> >   obj-$(CONFIG_SENSORS_XDPE122)       += xdpe12284.o
> >   obj-$(CONFIG_SENSORS_XDPE152)       += xdpe152c4.o
> > +obj-$(CONFIG_SENSORS_XDPE1A2G7B)     += xdpe1a2g7b.o
> >   obj-$(CONFIG_SENSORS_ZL6100)        += zl6100.o
> >   obj-$(CONFIG_SENSORS_PIM4328)       += pim4328.o
> >   obj-$(CONFIG_SENSORS_CRPS)  += crps.o
> > diff --git a/drivers/hwmon/pmbus/xdpe1a2g7b.c b/drivers/hwmon/pmbus/xdpe1a2g7b.c
> > new file mode 100644
> > index 000000000000..e10bafeb0984
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/xdpe1a2g7b.c
> > @@ -0,0 +1,120 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Hardware monitoring driver for Infineon Multi-phase Digital XDPE1A2G5B
> > + * and XDPE1A2G7B Controllers
> > + *
> > + * Copyright (c) 2026 Infineon Technologies. All rights reserved.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include "pmbus.h"
> > +
> > +#define XDPE1A2G7B_PAGE_NUM 2
> > +#define XDPE1A2G7B_NVIDIA_195MV 0x1E /* NVIDIA mode 1.95mV, VID step is 5mV */
> > +
> > +static int xdpe1a2g7b_identify(struct i2c_client *client,
> > +                            struct pmbus_driver_info *info)
> > +{
> > +     u8 vout_params;
> > +     int vout_mode;
> > +
> > +     /*
> > +      * XDPE1A2G5B and XDPE1A2G7B support both Linear and NVIDIA PWM VID data
> > +      * formats via VOUT_MODE. Note that the device pages/loops are not fully
> > +      * independent: configuration is shared, so programming each page/loop
> > +      * separately is not supported for VOUT_MODE.
> > +      */
> > +     vout_mode = pmbus_read_byte_data(client, 0, PMBUS_VOUT_MODE);
> > +     if (vout_mode >= 0 && vout_mode != 0xff) {
>
> "vout_mode != 0xff only makes sense in generic code, where it is possible
> that the command is not properly implemented. It does not make sense
> in a device specific driver: Either the command is implemented and returns
> a useful value, or it is not implemented. So this code should simply be
>
>         if (vout_mode < 0)
>                 return vout_mode;
>
>         switch (vout_mode >> 5) {
>         ...
>
> If the controller is really so unstable that it returns 0xff once
> in a while, a respective comment would be needed in the code. However,
> even then, I would suggest something like
>
>         if (vout_mode < 0)
>                 return vout_mode;
>
>         /* Explain instability */
>         if (vout_mode == 0xff)
>                 return -ENODEV;
>
>         switch (vout_mode >> 5) {
>         ...
>
> i.e., error handling first.
  The chip and command are stable, so I will proceed with making the
necessary changes as suggested by you.

> If the concern is that this isn't really one of the chips supported by the driver,
> I would suggest to implement chip detection code either in the identify
> function or better in the probe function. If that is the case, the return value
> from PMBUS_VOUT_MODE isn't useful anyway.
>
> Thanks,
> Guenter
>