| 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: <CAJKbuCZWjioOBFXQU+F7kA3dyjG5oQcnwW2WDcyi3Pfub7npUQ@mail.gmail.com>
Date: Wed, 7 Jan 2026 17:15:53 +0530
From: ashish yadav <ashishyadav78@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
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 Mon, Jan 5, 2026 at 9:10 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> 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.
ACK
>
> >
> > 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.
ACK, I will share the details in the next commit.
>
> More comments inline.
>
> > ---
> > drivers/hwmon/pmbus/Kconfig | 16 +++
> > drivers/hwmon/pmbus/Makefile | 1 +
> > drivers/hwmon/pmbus/tda38740a.c | 182 ++++++++++++++++++++++++++++++++
>
> Driver documentation missing.
ACK, I will submit another patch for device tree binding.
>
> > 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).
>
TDA38740/25 pin strap parts are available in two flavors of 1:1 & 1:2
vout scale loop.
For the 1:1 vout_scale_loop version, there is no need for any resistor
divider as output voltage sense pins are directly connected to
the output.
For a 1:2 scale loop version, it is recommended to use 499 ohms each for
top and bottom across the feedback path.
However, in some applications customers tend to use an intentional
resistor divider across the output with a different divider ratio other
than 1:1 or 1:2 to alter the actual output voltage.
For example, if pin strap part is set to Vboot of 0.7V,they use a
resistor divider to generate 0.75V using the equation provided in
Section 13.3 of the datasheet.In this case, as there are only two
vout_scale_loop options of 1:1 and 1:2 that the IC can identify,
Read_Vout would still read as 0.7V in the telemetry and the baseboard
management controllers would use this telemetry data to monitor the
rail parameters leading to false tripping of the system.
This multiplier is used to offset the telemetry output voltage Read_Vout
so that the telemetry data is reported correctly to the monitoring
controller,in this example the multiplier would be 0.75/0.7 = 1.071.
This multiplier is required only for any external monitoring of the rail
output voltage. All the other Vout related parameters are used
internally by the IC and there is only a slight impact on the fault
thresholds.The impact can be calculated using equations in Section 13.3
of the datasheet.
> > + break;
> > + default:
> > + ret = pmbus_read_word_data(client, page, phase, reg);
>
> Should return -ENODATA and let the calling code handle it.
>
ACK
> > + 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.
>
ACK
> > + 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.
ACK
>
> > + } 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.
>
ACK
> > + 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.
ACK
>
> > + }
> > +
> > + 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