Re: [PATCH v3 2/2] hwmon: (pmbus/tda38740) Add driver for Infineon TDA38740/TDA38725

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

On 9/9/26 23:18, Colin Huang wrote:
> From: Colin Huang <u8813345@gmail.com>
> 
> Add a PMBus driver for Infineon TDA38740 and TDA38725
> single-voltage synchronous buck regulators.
> 
> Signed-off-by: Colin Huang <u8813345@gmail.com>
> ---
>   drivers/hwmon/pmbus/Kconfig    |   9 +++
>   drivers/hwmon/pmbus/Makefile   |   1 +
>   drivers/hwmon/pmbus/tda38740.c | 150 +++++++++++++++++++++++++++++++++++++++++

Documentation is missing.

>   3 files changed, 160 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index bcfdc4ce4c10..e4ca80dd0574 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -763,6 +763,15 @@ config SENSORS_TDA38640_REGULATOR
>   	  If you say yes here you get regulator support for Infineon
>   	  TDA38640 as regulator.
>   
> +config SENSORS_TDA38740
> +	tristate "Infineon TDA38725/TDA38740"
> +	help
> +	  If you say yes here you get hardware monitoring support for Infineon
> +	  TDA38725 and TDA38740.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called tda38740.
> +
>   config SENSORS_TPS25990
>   	tristate "TI TPS25990"
>   	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index e288fe72a437..eb06d47816fd 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_SENSORS_STEF48H28)	+= stef48h28.o
>   obj-$(CONFIG_SENSORS_SQ24860)	+= sq24860.o
>   obj-$(CONFIG_SENSORS_STPDDC60)	+= stpddc60.o
>   obj-$(CONFIG_SENSORS_TDA38640)	+= tda38640.o
> +obj-$(CONFIG_SENSORS_TDA38740)	+= tda38740.o
>   obj-$(CONFIG_SENSORS_TPS25990)	+= tps25990.o
>   obj-$(CONFIG_SENSORS_TPS40422)	+= tps40422.o
>   obj-$(CONFIG_SENSORS_TPS53679)	+= tps53679.o
> diff --git a/drivers/hwmon/pmbus/tda38740.c b/drivers/hwmon/pmbus/tda38740.c
> new file mode 100644
> index 000000000000..df5173d8da0c
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/tda38740.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Hardware monitoring driver for Infineon TDA38725/TDA38740
> + *
> + * Copyright (c) 2023 9elements GmbH
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include "pmbus.h"
> +
> +#define TDA38740_VOUT_SCALE_DEFAULT_MICRO	1000000
> +#define TDA38740_VOUT_SCALE_MIN_MICRO		100000
> +#define TDA38740_VOUT_SCALE_MAX_MICRO		2000000
> +
> +struct tda38740_data {
> +	struct pmbus_driver_info info;
> +	u32 vout_scale_micro;
> +};
> +
> +static int tda38740_read_word_data(struct i2c_client *client, int page,
> +				   int phase, int reg)
> +{
> +	const struct tda38740_data *data;
> +	int ret;
> +	u64 scaled;
> +
> +	if (reg != PMBUS_READ_VOUT)
> +		return -ENODATA;
> +
> +	ret = pmbus_read_word_data(client, page, phase, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	data = container_of(pmbus_get_driver_info(client), struct tda38740_data,
> +			    info);
> +
> +	scaled = (u64)ret * data->vout_scale_micro;
> +	scaled = DIV_ROUND_CLOSEST_ULL(scaled,
> +				       TDA38740_VOUT_SCALE_DEFAULT_MICRO);
> +

The chip supports VOUT_SCALE_LOOP, which should be used for any VOUT scaling.
VOUT values should not be manipulated manually.

Also, Sashiko is correct in complaining about not scaling other VOUT
related commands, both on the read and write side. The chip _does_ support
limit commands.

> +	return clamp_val(scaled, 0, U16_MAX);
> +}
> +
> +/*
> + * TDA38725/TDA38740 only support Linear format for VOUT related commands,
> + * with exponents in the range of -8 to -12 (see datasheet VOUT_MODE
> + * description). Direct format is not supported by this device.
> + */
> +static int tda38740_identify(struct i2c_client *client,
> +			     struct pmbus_driver_info *info)
> +{
> +	int vout_mode;
> +
> +	vout_mode = pmbus_read_byte_data(client, 0, PMBUS_VOUT_MODE);
> +	if (vout_mode < 0 || vout_mode == 0xff)
> +		return vout_mode < 0 ? vout_mode : -ENODEV;
> +
> +	if ((vout_mode >> 5) != 0)
> +		return -ENODEV;
> +
> +	info->format[PSC_VOLTAGE_OUT] = linear;
> +
> +	return 0;
> +}
> +
> +static struct pmbus_driver_info tda38740_info = {
> +	.pages = 1,

The chips support the PAGE command, described as "Allows access
of each loop via paging". I don't know what exactly that refers to,
but it does look like it supports multiple pages.
> +	.format[PSC_VOLTAGE_IN] = linear,
> +	.format[PSC_CURRENT_OUT] = linear,
> +	.format[PSC_CURRENT_IN] = linear,
> +	.format[PSC_POWER] = linear,
> +	.format[PSC_TEMPERATURE] = linear,
> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
> +	    | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
> +	    | PMBUS_HAVE_IIN
> +	    | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
> +	    | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
> +	    | PMBUS_HAVE_POUT | PMBUS_HAVE_PIN,
> +	.identify = tda38740_identify,
> +};
> +
> +static int tda38740_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct tda38740_data *data;
> +	const char *propname;
> +	u32 vout_scale_micro;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	propname = "infineon,vout-scale-micro";
> +	if (device_property_present(dev, propname)) {
> +		ret = device_property_read_u32(dev, propname, &vout_scale_micro);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "%s property read fail.\n",
> +					     propname);
> +	} else {
> +		vout_scale_micro = TDA38740_VOUT_SCALE_DEFAULT_MICRO;
> +	}
> +
> +	if (vout_scale_micro < TDA38740_VOUT_SCALE_MIN_MICRO ||
> +	    vout_scale_micro > TDA38740_VOUT_SCALE_MAX_MICRO)
> +		return -EINVAL;
> +
> +	memcpy(&data->info, &tda38740_info, sizeof(tda38740_info));
> +	data->vout_scale_micro = vout_scale_micro;
> +	data->info.read_word_data = tda38740_read_word_data;
> +
> +	return pmbus_do_probe(client, &data->info);
> +}
> +
> +static const struct i2c_device_id tda38740_id[] = {
> +	{ .name = "tda38725"},
> +	{ .name = "tda38740"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, tda38740_id);
> +
> +static const struct of_device_id __maybe_unused tda38740_of_match[] = {
> +	{ .compatible = "infineon,tda38725"},
> +	{ .compatible = "infineon,tda38740"},
> +	{ },

No "," here.

> +};
> +MODULE_DEVICE_TABLE(of, tda38740_of_match);
> +
> +/* This is the driver that will be inserted */

Pointless comment.

> +static struct i2c_driver tda38740_driver = {
> +	.driver = {
> +		.name = "tda38740",
> +		.of_match_table = of_match_ptr(tda38740_of_match),
> +	},
> +	.probe = tda38740_probe,
> +	.id_table = tda38740_id,
> +};
> +
> +module_i2c_driver(tda38740_driver);
> +
> +MODULE_DESCRIPTION("PMBus driver for Infineon TDA38725/TDA38740");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("PMBUS");
>