| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJKbuCan+5AMuGuqKg4V1qs5HYZQ9zgS9S1rDDJ1usjJAjEGqw@mail.gmail.com>
Date: Thu, 5 Feb 2026 14:20:13 +0530
From: ashish yadav <ashishyadav78@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-hwmon@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Ashish Yadav <ashish.yadav@...ineon.com>
Subject: Re: [PATCH 2/2] hwmon:(pmbus/xdpe1a2g7b) Add support for
xdpe1a2g5b/7b controllers
Hi Guenter,
Thank you for taking the time to review and provide feedback.
I appreciate your input and insights.
Please find my comments inline below.
Best regards,
Ashish Yadav
On Mon, Feb 2, 2026 at 9:01 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 2/2/26 00:03, ASHISH YADAV wrote:
> > From: Ashish Yadav <ashish.yadav@...ineon.com>
> >
> > Add the pmbus driver for Infineon Digital Multi-phase XDPE1A2G5B and
> > XDPE1A2G7B controllers.
> >
> > XDPE1A2G5B controller supports Linear Data format for VOUT using VOUT_MODE
> > command.
> > XDPE1A2G7B controller supports Linear and VID Data format for VOUT using
> > VOUT_MODE command.
> >
> > In case of vid mode in XDPE1A2G7B controller, NVIDIA PWM VID vrm_version
> > is supported:
> > Vout = 5mV * (VID-1) + 195mV
> >
> > Signed-off-by: Ashish Yadav <ashish.yadav@...ineon.com>
> > ---
> > drivers/hwmon/pmbus/Kconfig | 9 +++
> > drivers/hwmon/pmbus/Makefile | 1 +
> > drivers/hwmon/pmbus/pmbus.h | 2 +-
> > drivers/hwmon/pmbus/pmbus_core.c | 4 ++
> > drivers/hwmon/pmbus/xdpe1a2g7b.c | 115 +++++++++++++++++++++++++++++++
>
> Driver documentation missing.
>
ACK, We will take care of this in the next release.
> > 5 files changed, 130 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/hwmon/pmbus/xdpe1a2g7b.c
> >
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index f3fb94cebf1a..c6750bce446d 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -684,6 +684,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 349a89b6d92e..620f24baa289 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -67,6 +67,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/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index d2e9bfb5320f..3ddcb742d289 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -416,7 +416,7 @@ enum pmbus_sensor_classes {
> > #define PMBUS_PAGE_VIRTUAL BIT(31) /* Page is virtual */
> >
> > enum pmbus_data_format { linear = 0, ieee754, direct, vid };
> > -enum vrm_version { vr11 = 0, vr12, vr13, imvp9, amd625mv };
> > +enum vrm_version { vr11 = 0, vr12, vr13, imvp9, amd625mv, nvidia195mv };
> >
> > /* PMBus revision identifiers */
> > #define PMBUS_REV_10 0x00 /* PMBus revision 1.0 */
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index be6d05def115..4d7634ee6148 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -885,6 +885,10 @@ static s64 pmbus_reg2data_vid(struct pmbus_data *data,
> > if (val >= 0x0 && val <= 0xd8)
> > rv = DIV_ROUND_CLOSEST(155000 - val * 625, 100);
> > break;
> > + case nvidia195mv:
> > + if (val >= 0x01)
> > + rv = 195 + (val - 1) * 5; /* VID step is 5mv */
> > + break;
> > }
> > return rv;
> > }
>
> The core change needs to be a separate patch.
>
ACK, We will take care of this in the next release.
> > diff --git a/drivers/hwmon/pmbus/xdpe1a2g7b.c b/drivers/hwmon/pmbus/xdpe1a2g7b.c
> > new file mode 100644
> > index 000000000000..79b12b56e7b6
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/xdpe1a2g7b.c
> > @@ -0,0 +1,115 @@
> > +// 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 i, ret, vout_mode;
> > +
> > + vout_mode = pmbus_read_byte_data(client, 0, PMBUS_VOUT_MODE);
> > + if (vout_mode >= 0 && vout_mode != 0xff) {
>
> What if vout_mode < 0 ? Also, what if the mode is different for page 1 ?
ACK, We will take care of this in the next release.
> Also, if I understand patch 0 correctly, executing this function is not needed
> for XDPE1A2G5B.
>
ACK, We will take care of this in the next release.
> > + switch (vout_mode >> 5) {
> > + case 0:
> > + info->format[PSC_VOLTAGE_OUT] = linear;
> > + return 0;
> > + case 1:
> > + info->format[PSC_VOLTAGE_OUT] = vid;
> > + break;
> > + default:
> > + return -ENODEV;
> > + }
> > + }
> > +
> > + for (i = 0; i < info->pages; i++) {
> > + /* Read the VOUT_MODE register for VID Code Type. */
> > + ret = pmbus_read_byte_data(client, i, PMBUS_VOUT_MODE);
>
> Given that there are only two pages, reading PMBUS_VOUT_MODE for
> page 0 twice is a bit of a waste. On top of that, the need for the loop
> suggests that the mode can be different across pages. That needs to be
> supported: Bailing out in that case is not acceptable. Worse:
> What if the mode is linear on page 0 but vid on page 1 ?
>
ACK, We will take care of this in the next release.
> > + if (ret < 0)
> > + return ret;
> > +
> > + vout_params = ret & GENMASK(4, 0);
> > + switch (vout_params) {
> > + case XDPE1A2G7B_NVIDIA_195MV:
> > + info->vrm_version[i] = nvidia195mv;
> > + break;
> > + default:
> > + return -EINVAL;
>
> This warrants an error message and an explanation (comment) why other modes
> are not supported by the driver. The detailed datasheet is not public, so
> you'll have to help out here. As mentioned above, bailing out because the
> mode on page 1 is linear is not acceptable.
>
ACK, we will provide comments in the next release.
Additionally, please note that the XDPE1A2G7B Controller only supports
the 'nvidia195mv' vrm_version in the VID Data format for VOUT.
We will ensure that this limitation is properly documented and
commented on in the next release.
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct pmbus_driver_info xdpe1a2g7b_info = {
> > + .pages = XDPE1A2G7B_PAGE_NUM,
> > + .identify = xdpe1a2g7b_identify,
> > + .format[PSC_VOLTAGE_IN] = linear,
> > + .format[PSC_TEMPERATURE] = linear,
> > + .format[PSC_CURRENT_IN] = linear,
> > + .format[PSC_CURRENT_OUT] = linear,
> > + .format[PSC_POWER] = linear,
> > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> > + PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_STATUS_TEMP |
> > + PMBUS_HAVE_POUT | PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT,
> > + .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> > + PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> > + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | PMBUS_HAVE_STATUS_INPUT,
> > +};
> > +
> > +static int xdpe1a2g7b_probe(struct i2c_client *client)
> > +{
> > + struct pmbus_driver_info *info;
> > +
> > + info = devm_kmemdup(&client->dev, &xdpe1a2g7b_info, sizeof(*info),
> > + GFP_KERNEL);
> > + if (!info)
> > + return -ENOMEM;
> > +
> > + return pmbus_do_probe(client, info);
> > +}
> > +
> > +static const struct i2c_device_id xdpe1a2g7b_id[] = { { "xdpe1a2g5b" },
> > + { "xdpe1a2g7b" },
> > + {} };
>
> Please use more lines and less indentation.
>
ACK, We will take care of this in the next release.
> > +
> > +MODULE_DEVICE_TABLE(i2c, xdpe1a2g7b_id);
> > +
> > +static const struct of_device_id __maybe_unused xdpe1a2g7b_of_match[] = {
> > + { .compatible = "infineon,xdpe1a2g5b" },
> > + { .compatible = "infineon,xdpe1a2g7b" },
> > + {}
>
> ... just like here.
>
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, xdpe1a2g7b_of_match);
> > +
> > +static struct i2c_driver xdpe1a2g7b_driver = {
> > + .driver = {
> > + .name = "xdpe1a2g7b",
> > + .of_match_table = of_match_ptr(xdpe1a2g7b_of_match),
> > + },
> > + .probe = xdpe1a2g7b_probe,
> > + .id_table = xdpe1a2g7b_id,
> > +};
> > +
> > +module_i2c_driver(xdpe1a2g7b_driver);
> > +
> > +MODULE_AUTHOR("Ashish Yadav <ashish.yadav@...ineon.com>");
> > +MODULE_DESCRIPTION("PMBus driver for Infineon XDPE1A2G5B/7B");
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS("PMBUS");
>
Powered by blists - more mailing lists