| 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: <CAJKbuCYcRMrX5H5rWXWXOz4FCZi5iu8CCE2Oi3WEsWqEikqsYg@mail.gmail.com>
Date: Tue, 13 Jan 2026 12:54:10 +0530
From: ashish yadav <ashishyadav78@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-hwmon@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
ASHISH YADAV <Ashish.Yadav@...ineon.com>
Subject: Re: [PATCH v2 1/2] hwmon:(pmbus/tda38740a) TDA38740A Voltage
Regulator Driver
Hi Guenter,
Thanks for your time and review comments.
Please find my answer inline.
With Regards
Ashish
On Tue, Jan 13, 2026 at 3:21 AM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 1/7/26 06:45, ASHISH YADAV wrote:
> > Add the pmbus driver for the Infineon TDA38740A/TDA38725A
> > DC-DC voltage regulator.
> >
> > Signed-off-by: ASHISH YADAV <Ashish.Yadav@...ineon.com>
> > ---
> > Changes in v2:
> > - Review comments address.
>
> That is not a change log.
ACK, I will address it in the v3 release .
>
> > - Another Patch for Devicetree binding submitted for Driver
> > Documentation.
> > ---
> > drivers/hwmon/pmbus/Kconfig | 16 +++
> > drivers/hwmon/pmbus/Makefile | 1 +
> > drivers/hwmon/pmbus/tda38740a.c | 203 ++++++++++++++++++++++++++++++++
>
> Documentation is missing.
ACK, I will address it in the v3 release .
> > 3 files changed, 220 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..b31e1b5c6916
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/tda38740a.c
> > @@ -0,0 +1,203 @@
> > +// 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_voltage_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;
> > +
>
> Why is this needed (instead of just returning -ENODATA) ?
>
ACK, I will address it in the v3 release .
> > + switch (reg) {
> > + case PMBUS_READ_VOUT:
> > + ret = pmbus_read_word_data(client, page, phase, reg);
> > + if (ret < 0)
> > + return ret;
> > + ret = ((ret * data->vout_voltage_multiplier[0]) /
> > + data->vout_voltage_multiplier[1]);
>
> The need for this, especially why it would only be needed for PMBUS_READ_VOUT
> but not for any other VOUT related commands, is still insufficiently explained
> (and I failed to understand the rationale provided earlier).
>
It is specifically needed for READ_VOUT as it is being used by
external controller to monitor the rail health.
Other Vout related parameters are used internally in the IC to for
output voltage related protections and does not impact any external
decision making.
> > + break;
> > + case PMBUS_VOUT_COMMAND:
> > + case PMBUS_VOUT_MAX:
> > + case PMBUS_VOUT_MARGIN_HIGH:
> > + case PMBUS_VOUT_MARGIN_LOW:
> > + case PMBUS_VOUT_TRANSITION_RATE:
> > + case PMBUS_VOUT_DROOP:
> > + case PMBUS_VOUT_SCALE_LOOP:
> > + case PMBUS_VOUT_OV_FAULT_LIMIT:
> > + case PMBUS_VOUT_UV_FAULT_LIMIT:
> > + case PMBUS_IOUT_OC_FAULT_LIMIT:
> > + case PMBUS_OT_FAULT_LIMIT:
> > + case PMBUS_OT_WARN_LIMIT:
> > + case PMBUS_VIN_OV_FAULT_LIMIT:
> > + case PMBUS_STATUS_WORD:
> > + case PMBUS_READ_VIN:
> > + case PMBUS_READ_IIN:
> > + case PMBUS_READ_IOUT:
> > + case PMBUS_READ_TEMPERATURE_1:
> > + case PMBUS_READ_POUT:
> > + case PMBUS_READ_PIN:
> > + ret = pmbus_read_word_data(client, page, phase, reg);
>
> I fail to see why this would be necessary. Just return -ENODATA.
>
ACK, I will address it in the v3 release .
> > + break;
> > + default:
> > + ret = -ENODATA;
> > + 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 or unexpected/unsupported Device\n");
>
> How about printing the device ID here if it is unsupported ?
> It could be printed as hex string.
>
ACK, I will address it in the v3 release .
> > + return -ENODEV;
> > + }
> > +
> > + if (!memcmp(TDA38725A_IC_DEVICE_ID, device_id, 1)) {
> > + id = tda38725a;
> > + } else if (!memcmp(TDA38740A_IC_DEVICE_ID, device_id, 1)) {
> > + id = tda38740a;
> > + } else {
> > + dev_err(&client->dev, "Unsupported device with ID:%s\n",
> > + device_id);
>
> device_id is not terminated, and it is not a user readable string.
> It should be printed as hex string, or as hex byte (0xXX).
>
ACK, I will address it in the v3 release .
> > + 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, "infineon,vout-voltage-multiplier",
> > + data->vout_voltage_multiplier,
> > + ARRAY_SIZE(data->vout_voltage_multiplier))) {
> > + dev_info(&client->dev,
> > + "vout-voltage-multiplier from Device Tree:%d %d\n",
> > + data->vout_voltage_multiplier[0],
> > + data->vout_voltage_multiplier[1]);
> > + } else {
> > + dev_info(&client->dev,
> > + "vout-voltage-multiplier not available from Device Tree,using default values");
> > + data->vout_voltage_multiplier[0] = 0x01;
> > + data->vout_voltage_multiplier[1] = 0x01;
> > + }
> > +
> > + 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 TDA38725A/40A IPOL");
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS("PMBUS");
>
Powered by blists - more mailing lists