On Wed, Aug 24, 2022 at 01:58:33PM +0800, Duke Du wrote: > Hi Guenter, > Thanks for your reply ! > But I have some confusion as following, please help to give me some feedback. > Thanks a lot ! > > On Fri, Aug 19, 2022 at 9:35 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > > > > On Fri, Aug 19, 2022 at 11:04:43AM +0800, Duke Du wrote: > > > Add the pmbus driver for TEXAS tps546d24 Buck Converter. The > > > tps546d24 PMBUS_VOUT_MODE is 0x97 (i.e. the bit 5~7 are 100) > > > which could not meet the standard pmbus spec, the standard mode > > > of PMBUS_VOUT_MODE must be 000 (linear foramt) or 001 (vid format). > > > Make the tps546d24 PMBUS_VOUT_MODE return value 0x17 (i.e. the > > > bit5~7 are 000), VOUT returned value is linear11. > > > > > > > First of all, the above should be documented as comment in the > > implementation. > > > > Second, this is actually problematic. In PMBus version 1.3.1, bit > > 7 of PMBUS_VOUT_MODE no longer describes the mode (linear, vid, direct, > > IEEE) but Absolute vs/ Relative VOUT voltages. This affects vout fault > > and warning limits. If the relative mode bit is set, those limits > > are supposed to reflect percentages, not absolute voltages. > > > > This means that the driver interprets vout voltage limits wrongly, > > at least if the chip works as described in the datasheet. Changing > > the reported value of PMBUS_VOUT_MODE is actually counter-productive. > > > > This means we'll need a number of changes. At the very least, the > > PMBus core needs to be be modified to only use bit 5 and 6 to determine > > the mode. On top of that, the driver probe function should update > > VOUT_MODE and clear bit 7. It might also make sense to warn in the > > PMBus core if mode bit 7 is set. > > > > When the vout mode bit 7 is set, we update vout mode and clear bit 7 > in the driver probe function, this operation is the same as changing > the reported value of PMBUS_VOUT_MODE ? Absolutely not. When changing the bit in the register, the chip operation mode changes, and the associated values (VOUT*) change from relative to absolute mode. When changing the value reported by the chip, nothing changes from the chip side, it still operates in relative mode, and all VOUT* registers are set to relative mode. Guenter > Maybe I misunderstood, could you please help to explain clearly about this ? > Thanks again for your help :) > > Thanks, > Duke > > > An alternative to the second change would be to implement relative > > vout support in the PMBus core, but that would be much more complex. > > We could also clear the relative bit in the PMBus core, but that might > > lead to unexpected side effects (we don't know how various chips > > respond to that) and thus probably not be a good idea. > > > > > Signed-off-by: Duke Du <Duke.Du@xxxxxxxxxxxx> > > > > This e-mail address does not match the e-mail address used to send > > the patch, resulting in a checkpatch warning. Please fix. > > > > More comments inline. > > > > Thanks, > > Guenter > > > > > --- > > > Change in v1: > > > Initial patchset. > > > Change in v2: > > > Correct the tps546d24.rst format. > > > Change in v3: > > > 1. Modify the patch description. > > > 2. Put the change log between the dashes and diffstat. > > > --- > > > --- > > > Documentation/hwmon/index.rst | 1 + > > > Documentation/hwmon/tps546d24.rst | 35 +++++++++++++++++++ > > > MAINTAINERS | 7 ++++ > > > drivers/hwmon/pmbus/Kconfig | 9 +++++ > > > drivers/hwmon/pmbus/Makefile | 1 + > > > drivers/hwmon/pmbus/tps546d24.c | 73 +++++++++++++++++++++++++++++++++++++++ > > > 6 files changed, 126 insertions(+) > > > create mode 100644 Documentation/hwmon/tps546d24.rst > > > create mode 100644 drivers/hwmon/pmbus/tps546d24.c > > > > > > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > > > index f7113b0..d3eede4 100644 > > > --- a/Documentation/hwmon/index.rst > > > +++ b/Documentation/hwmon/index.rst > > > @@ -205,6 +205,7 @@ Hardware Monitoring Kernel Drivers > > > tps23861 > > > tps40422 > > > tps53679 > > > + tps546d24 > > > twl4030-madc-hwmon > > > ucd9000 > > > ucd9200 > > > diff --git a/Documentation/hwmon/tps546d24.rst b/Documentation/hwmon/tps546d24.rst > > > new file mode 100644 > > > index 0000000..3061fd8 > > > --- /dev/null > > > +++ b/Documentation/hwmon/tps546d24.rst > > > @@ -0,0 +1,35 @@ > > > +.. SPDX-License-Identifier: GPL-2.0-only > > > + > > > +Kernel driver tps546d24 > > > +====================== > > > + > > > +Supported chips: > > > + > > > + * TI TPS546D24 > > > + > > > + Prefix: 'tps546d24' > > > + > > > + Addresses scanned: - > > > + > > > + Datasheet: https://www.ti.com/lit/gpn/tps546d24 > > > + > > > +Author: Duke Du <dukedu83@xxxxxxxxx> > > > > This needs to match Author and Signed-off-by:. This applies to all > > e-mail addresses. > > > > > + > > > + > > > +Description > > > +----------- > > > + > > > +The TPS546D24A is a highly integrated, non-isolated DC/DC converter capable > > > +of high frequency operation and 40-A current output from a 7-mm x 5-mm > > > +package. > > > + > > > +Two, three, and four TPS546D24A devices can be interconnected > > > +to provide up to 160 A on a single output. The device has an option to > > > +overdrive the internal 5-V LDO with an external 5-V supply via the VDD5 > > > +pin to improve efficiency and reduce power dissipation of the converter. > > > + > > > + > > > +Platform data support > > > +--------------------- > > > + > > > +The driver supports standard PMBus driver platform data. > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index 8a5012b..fa2d4fb 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -20583,6 +20583,13 @@ Q: https://patchwork.kernel.org/project/linux-integrity/list/ > > > T: git git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git > > > F: drivers/char/tpm/ > > > > > > +TPS546D24 DRIVER > > > +M: Duke Du <dukedu83@xxxxxxxxx> > > > +L: linux-hwmon@xxxxxxxxxxxxxxx > > > +S: Maintained > > > +F: Documentation/hwmon/tps546d24.rst > > > +F: drivers/hwmon/pmbus/tps546d24.c > > > + > > > TRACING > > > M: Steven Rostedt <rostedt@xxxxxxxxxxx> > > > M: Ingo Molnar <mingo@xxxxxxxxxx> > > > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > > > index 951e4a9..89668af 100644 > > > --- a/drivers/hwmon/pmbus/Kconfig > > > +++ b/drivers/hwmon/pmbus/Kconfig > > > @@ -397,6 +397,15 @@ config SENSORS_TPS53679 > > > This driver can also be built as a module. If so, the module will > > > be called tps53679. > > > > > > +config SENSORS_TPS546D24 > > > + tristate "TPS546D24" > > > + help > > > + If you say yes here you get hardware monitoring support for TEXAS > > > + TPS546D24. > > > + > > > + This driver can also be built as a module. If so, the module will > > > + be called tps546d24 > > > + > > > config SENSORS_UCD9000 > > > tristate "TI UCD90120, UCD90124, UCD90160, UCD90320, UCD9090, UCD90910" > > > help > > > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > > > index e2fe86f..0002dbe 100644 > > > --- a/drivers/hwmon/pmbus/Makefile > > > +++ b/drivers/hwmon/pmbus/Makefile > > > @@ -41,6 +41,7 @@ obj-$(CONFIG_SENSORS_Q54SJ108A2) += q54sj108a2.o > > > obj-$(CONFIG_SENSORS_STPDDC60) += stpddc60.o > > > obj-$(CONFIG_SENSORS_TPS40422) += tps40422.o > > > obj-$(CONFIG_SENSORS_TPS53679) += tps53679.o > > > +obj-$(CONFIG_SENSORS_TPS546D24) += tps546d24.o > > > obj-$(CONFIG_SENSORS_UCD9000) += ucd9000.o > > > obj-$(CONFIG_SENSORS_UCD9200) += ucd9200.o > > > obj-$(CONFIG_SENSORS_XDPE122) += xdpe12284.o > > > diff --git a/drivers/hwmon/pmbus/tps546d24.c b/drivers/hwmon/pmbus/tps546d24.c > > > new file mode 100644 > > > index 0000000..f6f79d3 > > > --- /dev/null > > > +++ b/drivers/hwmon/pmbus/tps546d24.c > > > @@ -0,0 +1,73 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * Hardware monitoring driver for TEXAS TPS546D24 buck converter > > > + */ > > > + > > > +#include <linux/err.h> > > > +#include <linux/i2c.h> > > > +#include <linux/init.h> > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/pmbus.h> > > > +#include "pmbus.h" > > > + > > > +static int tps546d24_read_byte_data(struct i2c_client *client, int page, int reg) > > > +{ > > > + int ret; > > > + > > > + switch (reg) { > > > + case PMBUS_VOUT_MODE: > > > + ret = 0x17; > > > + break; > > > + default: > > > + ret = -ENODATA; > > > + break; > > > + } > > > + return ret; > > > +} > > > + > > > +static struct pmbus_driver_info tps546d24_info = { > > > + .pages = 1, > > > + .format[PSC_VOLTAGE_IN] = linear, > > > + .format[PSC_VOLTAGE_OUT] = linear, > > > + .format[PSC_TEMPERATURE] = linear, > > > + .format[PSC_CURRENT_OUT] = linear, > > > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | > > > + PMBUS_HAVE_IOUT | PMBUS_HAVE_VOUT | > > > + PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_VOUT | > > > + PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP, > > > + .read_byte_data = tps546d24_read_byte_data, > > > > The chip supports multiple phases, and per-phase telemetry. > > Have you considered supporting it ? > > > > > +}; > > > + > > > +static int tps546d24_probe(struct i2c_client *client) > > > +{ > > > + return pmbus_do_probe(client, &tps546d24_info); > > > +} > > > + > > > +static const struct i2c_device_id tps546d24_id[] = { > > > + {"tps546d24", 0}, > > > + {} > > > +}; > > > +MODULE_DEVICE_TABLE(i2c, tps546d24_id); > > > + > > > +static const struct of_device_id __maybe_unused tps546d24_of_match[] = { > > > + {.compatible = "tps546d24"}, > > > > This needs a vendor prefix. > > > > > + {} > > > +}; > > > + > > > +/* This is the driver that will be inserted */ > > > +static struct i2c_driver tps546d24_driver = { > > > + .driver = { > > > + .name = "tps546d24", > > > + .of_match_table = of_match_ptr(tps546d24_of_match), > > > + }, > > > + .probe_new = tps546d24_probe, > > > + .id_table = tps546d24_id, > > > +}; > > > + > > > +module_i2c_driver(tps546d24_driver); > > > + > > > +MODULE_AUTHOR("Duke Du <dukedu83@xxxxxxxxx>"); > > > +MODULE_DESCRIPTION("PMBus driver for TI tps546d24"); > > > +MODULE_LICENSE("GPL"); > > > +MODULE_IMPORT_NS(PMBUS); > > > -- > > > 2.7.4 > > >
