Re: [PATCH v3 2/2] hwmon:(pmbus/xdp720) Add support for efuse xdp730

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

On Mon, Jun 01, 2026 at 02:39:25PM +0530, ASHISH YADAV wrote:
> From: Ashish Yadav <ashish.yadav@infineon.com>
> 
> Adds support for the Infineon XDP730 Digital eFuse Controller by
> updating the existing XDP720 driver.
> 
> Signed-off-by: Ashish Yadav <ashish.yadav@infineon.com>
> ---
> XDP720/XDP730 Digital eFuse Controllers provides accurate system telemetry
> (V, I, P, T) and reports analog current at the IMON pin for post-processing.
> 
> Both parts share the same PMBus register map and direct-format
> coefficients; they differ in the GIMON gain step exposed via the
> TELEMETRY_AVG register (bit 10) and in the VDD_VIN pin number
> (XDP720: pin 9, XDP730: pin 20).
> 
> The Current and Power measurement depends on the RIMON and GIMON values.
> The GIMON (microA/A) depends on the 10th bit of TELEMETRY_AVG PMBUS Register.
> The value of RIMON (kohm) can be provided by the user through device tree using
> infineon,rimon-micro-ohms  property.
> ---
>  drivers/hwmon/pmbus/Kconfig  |   2 +-
>  drivers/hwmon/pmbus/xdp720.c | 167 +++++++++++++++++++++--------------
>  2 files changed, 100 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 8f4bff375ecb..a9e86d92b044 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -715,7 +715,7 @@ config SENSORS_XDP720
>  	tristate "Infineon XDP720 family"
>  	help
>  	  If you say yes here you get hardware monitoring support for Infineon
> -	  XDP720.
> +	  XDP720 and XDP730 Digital eFuse Controllers.
>  
>  	  This driver can also be built as a module. If so, the module will
>  	  be called xdp720.
> diff --git a/drivers/hwmon/pmbus/xdp720.c b/drivers/hwmon/pmbus/xdp720.c
> index 8729a771f216..1a5eab09f2fd 100644
> --- a/drivers/hwmon/pmbus/xdp720.c
> +++ b/drivers/hwmon/pmbus/xdp720.c
> @@ -1,128 +1,159 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
> - * Hardware monitoring driver for Infineon XDP720 Digital eFuse Controller
> + * Hardware monitoring driver for Infineon XDP720 / XDP730 Digital
> + * eFuse Controllers.
> + *
> + * Both parts share the same PMBus register map and direct-format
> + * coefficients; they differ in the GIMON gain step exposed via
> + * the TELEMETRY_AVG register and in the VDD_VIN pin number.
>   *
>   * Copyright (c) 2026 Infineon Technologies. All rights reserved.
>   */
>  
> +#include <linux/bitops.h>
>  #include <linux/i2c.h>
> -#include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> -#include <linux/of_device.h>
> -#include <linux/bitops.h>
>  #include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
>  #include "pmbus.h"
>  
>  /*
> - * The IMON resistor required to generate the system overcurrent protection.
> - * Arbitrary default Rimon value: 2k Ohm
> + * The IMON resistor required to generate the system overcurrent
> + * protection.  Arbitrary default Rimon value: 2 kOhm (in micro-ohms).
>   */
> -#define XDP720_DEFAULT_RIMON 2000000000 /* 2k ohm */
> -#define XDP720_TELEMETRY_AVG 0xE9
> +#define XDP720_DEFAULT_RIMON		2000000000U	/* 2 kohm */
> +#define XDP720_TELEMETRY_AVG		0xE9
> +#define XDP720_TELEMETRY_AVG_GIMON	BIT(10)		/* high/low GIMON select */
> +
> +/* Chip identifiers carried in OF match-data and i2c_device_id->driver_data. */
> +enum xdp720_chip_id {
> +	CHIP_XDP720 = 0,
> +	CHIP_XDP730,
> +};
>  
> -static struct pmbus_driver_info xdp720_info = {
> +struct xdp720_data {
> +	enum xdp720_chip_id	 id;
> +	struct pmbus_driver_info info;
> +};
> +
> +static const struct pmbus_driver_info xdp720_info = {
>  	.pages = 1,
> -	.format[PSC_VOLTAGE_IN] = direct,
> -	.format[PSC_VOLTAGE_OUT] = direct,
> -	.format[PSC_CURRENT_OUT] = direct,
> -	.format[PSC_POWER] = direct,
> -	.format[PSC_TEMPERATURE] = direct,
> -
> -	.m[PSC_VOLTAGE_IN] = 4653,
> -	.b[PSC_VOLTAGE_IN] = 0,
> -	.R[PSC_VOLTAGE_IN] = -2,
> -	.m[PSC_VOLTAGE_OUT] = 4653,
> -	.b[PSC_VOLTAGE_OUT] = 0,
> -	.R[PSC_VOLTAGE_OUT] = -2,
> +	.format[PSC_VOLTAGE_IN]		= direct,
> +	.format[PSC_VOLTAGE_OUT]	= direct,
> +	.format[PSC_CURRENT_OUT]	= direct,
> +	.format[PSC_POWER]		= direct,
> +	.format[PSC_TEMPERATURE]	= direct,
> +
> +	.m[PSC_VOLTAGE_IN]	= 4653,
> +	.b[PSC_VOLTAGE_IN]	= 0,
> +	.R[PSC_VOLTAGE_IN]	= -2,
> +	.m[PSC_VOLTAGE_OUT]	= 4653,
> +	.b[PSC_VOLTAGE_OUT]	= 0,
> +	.R[PSC_VOLTAGE_OUT]	= -2,

Do not change patch formatting just because you prefer your own
format. This hides the actual changes for no purpose.

>  	/*
> -	 * Current and Power measurement depends on the RIMON (kOhm) and
> -	 * GIMON(microA/A) values.
> +	 * Current and Power measurement depend on the RIMON (micro-ohm)
> +	 * and GIMON (microA/A) values; scaled per-instance in probe().

Also, if you make changes like this, do it in a separate patch
or patch series. Again, this hides functional changes.

I am not going to review this patch any further.

Guenter

>  	 */
> -	.m[PSC_CURRENT_OUT] = 24668,
> -	.b[PSC_CURRENT_OUT] = 0,
> -	.R[PSC_CURRENT_OUT] = -4,
> -	.m[PSC_POWER] = 4486,
> -	.b[PSC_POWER] = 0,
> -	.R[PSC_POWER] = -1,
> -	.m[PSC_TEMPERATURE] = 54,
> -	.b[PSC_TEMPERATURE] = 22521,
> -	.R[PSC_TEMPERATURE] = -1,
> +	.m[PSC_CURRENT_OUT]	= 24668,
> +	.b[PSC_CURRENT_OUT]	= 0,
> +	.R[PSC_CURRENT_OUT]	= -4,
> +	.m[PSC_POWER]		= 4486,
> +	.b[PSC_POWER]		= 0,
> +	.R[PSC_POWER]		= -1,
> +	.m[PSC_TEMPERATURE]	= 54,
> +	.b[PSC_TEMPERATURE]	= 22521,
> +	.R[PSC_TEMPERATURE]	= -1,
>  
>  	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_PIN |
> -		   PMBUS_HAVE_TEMP | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_INPUT |
> -		   PMBUS_HAVE_STATUS_TEMP,
> +		   PMBUS_HAVE_TEMP | PMBUS_HAVE_IOUT |
> +		   PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP,
>  };
>  
>  static int xdp720_probe(struct i2c_client *client)
>  {
> -	struct pmbus_driver_info *info;
> +	struct xdp720_data *data;
>  	int ret;
> -	u32 rimon;
>  	int gimon;
> +	u32 rimon;
>  
> -	info = devm_kmemdup(&client->dev, &xdp720_info, sizeof(*info),
> -			    GFP_KERNEL);
> -	if (!info)
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
>  		return -ENOMEM;
>  
> +	data->id = (enum xdp720_chip_id)(uintptr_t)i2c_get_match_data(client);
> +	data->info = xdp720_info;
> +
>  	ret = devm_regulator_get_enable(&client->dev, "vdd-vin");
>  	if (ret)
>  		return dev_err_probe(&client->dev, ret,
> -			"failed to enable vdd-vin supply\n");
> +				     "failed to enable vdd-vin supply\n");
>  
>  	ret = i2c_smbus_read_word_data(client, XDP720_TELEMETRY_AVG);
> -	if (ret < 0) {
> -		dev_err(&client->dev, "Can't get TELEMETRY_AVG\n");
> -		return ret;
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to read TELEMETRY_AVG\n");
> +
> +	/* Bit 10 of TELEMETRY_AVG selects the GIMON gain step in microA/A */
> +	switch (data->id) {
> +	case CHIP_XDP720:
> +		gimon = (ret & XDP720_TELEMETRY_AVG_GIMON) ? 18200 : 9100;
> +		dev_info(&client->dev, "Initialised XDP720 instance\n");
> +		break;
> +	case CHIP_XDP730:
> +		gimon = (ret & XDP720_TELEMETRY_AVG_GIMON) ? 20000 : 10000;
> +		dev_info(&client->dev, "Initialised XDP730 instance\n");
> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
> -	ret >>= 10; /* 10th bit of TELEMETRY_AVG REG for GIMON Value */
> -	ret &= GENMASK(0, 0);
> -	if (ret == 1)
> -		gimon = 18200; /* output gain 18.2 microA/A */
> -	else
> -		gimon = 9100; /* output gain 9.1 microA/A */
> -
> -	if (of_property_read_u32(client->dev.of_node,
> -				 "infineon,rimon-micro-ohms", &rimon))
> -		rimon = XDP720_DEFAULT_RIMON; /* Default if not set via DT */
> +	if (device_property_read_u32(&client->dev,
> +				     "infineon,rimon-micro-ohms", &rimon))
> +		rimon = XDP720_DEFAULT_RIMON;	/* Default if not in FW */
>  	if (rimon == 0)
>  		return -EINVAL;
>  
> -	/* Adapt the current and power scale for each instance */
> -	info->m[PSC_CURRENT_OUT] = DIV64_U64_ROUND_CLOSEST((u64)
> -		info->m[PSC_CURRENT_OUT] * rimon * gimon, 1000000000000ULL);
> -	info->m[PSC_POWER] = DIV64_U64_ROUND_CLOSEST((u64)
> -		info->m[PSC_POWER] * rimon * gimon, 1000000000000000ULL);
> +	/* Adapt the current and power scale for each instance. */
> +	data->info.m[PSC_CURRENT_OUT] = DIV64_U64_ROUND_CLOSEST((u64)
> +		data->info.m[PSC_CURRENT_OUT] * rimon * gimon,
> +		1000000000000ULL);
> +	data->info.m[PSC_POWER] = DIV64_U64_ROUND_CLOSEST((u64)
> +		data->info.m[PSC_POWER] * rimon * gimon,
> +		1000000000000000ULL);
>  
> -	return pmbus_do_probe(client, info);
> +	return pmbus_do_probe(client, &data->info);
>  }
>  
>  static const struct of_device_id xdp720_of_match[] = {
> -	{ .compatible = "infineon,xdp720" },
> -	{}
> +	{ .compatible = "infineon,xdp720", .data = (void *)CHIP_XDP720 },
> +	{ .compatible = "infineon,xdp730", .data = (void *)CHIP_XDP730 },
> +	{ }
>  };
>  MODULE_DEVICE_TABLE(of, xdp720_of_match);
>  
>  static const struct i2c_device_id xdp720_id[] = {
> -	{ "xdp720" },
> -	{}
> +	{ "xdp720", CHIP_XDP720 },
> +	{ "xdp730", CHIP_XDP730 },
> +	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, xdp720_id);
>  
>  static struct i2c_driver xdp720_driver = {
>  	.driver = {
> -		   .name = "xdp720",
> -		   .of_match_table = xdp720_of_match,
> +		.name		= "xdp720",
> +		.of_match_table	= xdp720_of_match,
>  	},
> -	.probe = xdp720_probe,
> -	.id_table = xdp720_id,
> +	.probe		= xdp720_probe,
> +	.id_table	= xdp720_id,
>  };
>  
>  module_i2c_driver(xdp720_driver);
>  
>  MODULE_AUTHOR("Ashish Yadav <ashish.yadav@infineon.com>");
> -MODULE_DESCRIPTION("PMBus driver for Infineon XDP720 Digital eFuse Controller");
> +MODULE_DESCRIPTION("PMBus driver for Infineon XDP720/XDP730 Digital eFuse Controllers");
>  MODULE_LICENSE("GPL");
>  MODULE_IMPORT_NS("PMBUS");