| 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: <a8c3586b-5baa-4cf8-996d-b99082bf2563@roeck-us.net>
Date: Mon, 5 Jan 2026 07:40:09 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: ASHISH YADAV <ashishyadav78@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
ASHISH YADAV <Ashish.Yadav@...ineon.com>
Subject: Re: [PATCH] hwmon:(pmbus/tda38740a) TDA38740A Voltage Regulator
Driver
On Sun, Jan 04, 2026 at 04:23:51PM +0530, ASHISH YADAV wrote:
> Add the pmbus driver for the Infineon TDA38740A/TDA38725A
> DC-DC voltage regulator.
No need for indentation.
>
> Signed-off-by: ASHISH YADAV <Ashish.Yadav@...ineon.com>
No change log, no versioning, the devicetree description was not separated
but dropped or submitted entirely separately without copying hwmon,
no explanation for the new property, review feedback not completely
addressed.
More comments inline.
> ---
> drivers/hwmon/pmbus/Kconfig | 16 +++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/tda38740a.c | 182 ++++++++++++++++++++++++++++++++
Driver documentation missing.
> 3 files changed, 199 insertions(+)
> create mode 100644 drivers/hwmon/pmbus/tda38740a.c
>
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index f3fb94cebf1a..e7d7ff1b57df 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -602,6 +602,22 @@ config SENSORS_TDA38640_REGULATOR
> If you say yes here you get regulator support for Infineon
> TDA38640 as regulator.
>
> +config SENSORS_TDA38740A
> + tristate "Infineon TDA38740A"
> + help
> + If you say yes here you get hardware monitoring support for Infineon
> + TDA38740A/25A.
> +
> + This driver can also be built as a module. If so, the module will
> + be called tda38740a.
> +
> +config SENSORS_TDA38740A_REGULATOR
> + bool "Regulator support for TDA38740A and compatibles"
> + depends on SENSORS_TDA38740A && REGULATOR
> + help
> + If you say yes here you get regulator support for Infineon
> + TDA38740A/25A as regulator.
> +
> config SENSORS_TPS25990
> tristate "TI TPS25990"
> help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 349a89b6d92e..f422c80cf3d8 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_SENSORS_PXE1610) += pxe1610.o
> obj-$(CONFIG_SENSORS_Q54SJ108A2) += q54sj108a2.o
> obj-$(CONFIG_SENSORS_STPDDC60) += stpddc60.o
> obj-$(CONFIG_SENSORS_TDA38640) += tda38640.o
> +obj-$(CONFIG_SENSORS_TDA38740A) += tda38740a.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/tda38740a.c b/drivers/hwmon/pmbus/tda38740a.c
> new file mode 100644
> index 000000000000..3402bbf2cc47
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/tda38740a.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/**
> + * Hardware monitoring driver for Infineon Integrated-pol-voltage-regulators
> + * Driver for TDA38725A and TDA38740A
> + *
> + * Copyright (c) 2025 Infineon Technologies
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regulator/driver.h>
> +#include "pmbus.h"
> +
> +#define TDA38725A_IC_DEVICE_ID "\xA9"
> +#define TDA38740A_IC_DEVICE_ID "\xA8"
> +
> +static const struct i2c_device_id tda38740a_id[];
> +
> +enum chips { tda38725a, tda38740a };
> +
> +struct tda38740a_data {
> + enum chips id;
> + struct pmbus_driver_info info;
> + u32 vout_multiplier[2];
> +};
> +
> +#define to_tda38740a_data(x) container_of(x, struct tda38740a_data, info)
> +
> +static const struct regulator_desc __maybe_unused tda38740a_reg_desc[] = {
> + PMBUS_REGULATOR("vout", 0),
> +};
> +
> +static int tda38740a_read_word_data(struct i2c_client *client, int page,
> + int phase, int reg)
> +{
> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> + const struct tda38740a_data *data = to_tda38740a_data(info);
> + int ret;
> +
> + /* Virtual PMBUS Command not supported */
> + if (reg >= PMBUS_VIRT_BASE)
> + return -ENXIO;
> +
> + switch (reg) {
> + case PMBUS_READ_VOUT:
> + ret = pmbus_read_word_data(client, page, phase, reg);
> + if (ret < 0)
> + return ret;
> + ret = ((ret * data->vout_multiplier[0]) /
> + data->vout_multiplier[1]);
Repeating me, but the rationale (use case) for the multiplier is still not
provided, much less why it would be needed for READ_VOUT but not for any
of the other VOUT related commands. The data sheet does not provide an
explanation (section 13.3 does not explain the need for it, or suggest
that READ_VOUT would return a bad value, much less why only READ_VOUT would
require scaling or why adjusting VOUT_SCALE_LOOP would not be sufficient).
> + break;
> + default:
> + ret = pmbus_read_word_data(client, page, phase, reg);
Should return -ENODATA and let the calling code handle it.
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static struct pmbus_driver_info tda38740a_info[] = {
> + [tda38740a] = {
> + .pages = 1,
> + .read_word_data = tda38740a_read_word_data,
> + .format[PSC_VOLTAGE_IN] = linear,
> + .format[PSC_VOLTAGE_OUT] = 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,
> +#if IS_ENABLED(CONFIG_SENSORS_TDA38740A_REGULATOR)
> + .num_regulators = 1,
> + .reg_desc = tda38740a_reg_desc,
> +#endif
> + },
> +};
> +
> +static int tda38740a_get_device_id(struct i2c_client *client)
> +{
> + u8 device_id[I2C_SMBUS_BLOCK_MAX + 1];
> + enum chips id;
> + int status;
> +
> + status = i2c_smbus_read_block_data(client, PMBUS_IC_DEVICE_ID,
> + device_id);
> + if (status < 0 || status > 1) {
> + dev_err(&client->dev, "Failed to read Device Id %x\n", status);
Not necessarily. If status > 1 it is just unexpected/unsupported.
> + return -ENODEV;
> + }
> +
> + if (!memcmp(TDA38725A_IC_DEVICE_ID, device_id, strlen(device_id))) {
> + id = tda38725a;
device_id[] is not initialized, meaning its contents are random. There is no
guarantee that the returned data is a string either, 0-terminated or not.
Thus, strlen() is wrong.
> + } else if (!memcmp(TDA38740A_IC_DEVICE_ID, device_id,
> + strlen(device_id))) {
> + id = tda38740a;
> + } else {
> + dev_err(&client->dev, "Unsupported device\n");
Consider telling the user the ID of the unsupported device.
> + return -ENODEV;
> + }
> +
> + return id;
> +}
> +
> +static int tda38740a_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct tda38740a_data *data;
> + int chip_id;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_BYTE |
> + I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_WORD_DATA |
> + I2C_FUNC_SMBUS_BLOCK_DATA))
> + return -ENODEV;
> +
> + chip_id = tda38740a_get_device_id(client);
> + if (chip_id < 0)
> + return chip_id;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> + data->id = chip_id;
> + memcpy(&data->info, &tda38740a_info[chip_id], sizeof(data->info));
> +
> + if (!of_property_read_u32_array(client->dev.of_node, "vout_multiplier",
> + data->vout_multiplier,
> + ARRAY_SIZE(data->vout_multiplier))) {
> + dev_info(&client->dev,
> + "vout_multiplier from Device Tree:%d %d\n",
> + data->vout_multiplier[0], data->vout_multiplier[1]);
> + } else {
> + dev_info(&client->dev,
> + "vout_multiplier not available from Device Tree");
> + data->vout_multiplier[0] = 0x01;
> + data->vout_multiplier[1] = 0x01;
> + dev_info(&client->dev, "vout_multiplier default value:%d %d\n",
> + data->vout_multiplier[0], data->vout_multiplier[1]);
Drop the noise.
> + }
> +
> + return pmbus_do_probe(client, &data->info);
> +}
> +
> +static const struct i2c_device_id tda38740a_id[] = { { "tda38725a", tda38725a },
> + { "tda38740a", tda38740a },
> + {} };
> +
> +MODULE_DEVICE_TABLE(i2c, tda38740a_id);
> +
> +static const struct of_device_id __maybe_unused tda38740a_of_match[] = {
> + { .compatible = "infineon,tda38725a", .data = (void *)tda38725a },
> + { .compatible = "infineon,tda38740a", .data = (void *)tda38740a },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(of, tda38740a_of_match);
> +
> +static struct i2c_driver tda38740a_driver = {
> + .driver = {
> + .name = "tda38740a",
> + .of_match_table = of_match_ptr(tda38740a_of_match),
> + },
> + .probe = tda38740a_probe,
> + .id_table = tda38740a_id,
> +};
> +
> +module_i2c_driver(tda38740a_driver);
> +
> +MODULE_AUTHOR("Ashish Yadav <Ashish.Yadav@...ineon.com>");
> +MODULE_DESCRIPTION("PMBus driver for Infineon IPOL");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("PMBUS");
> --
> 2.39.5
>
Powered by blists - more mailing lists