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

[Guenter Roeck <linux@roeck-us.net>]

On 2/19/26 02:56, ASHISH YADAV wrote:
> From: Ashish Yadav <ashish.yadav@infineon.com>
> 
> Add the pmbus driver for Infineon Digital Multi-phase XDPE1A2G5B and
> XDPE1A2G7B controllers.
> 
> Signed-off-by: Ashish Yadav <ashish.yadav@infineon.com>
> ---
> 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.

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