On Wed, Aug 24, 2022 at 6:50 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > > 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 > Got it, thanks for your reply !! Another question, If we don't need to change the mode from relative to absolute mode, could we just change the PMBus core to determine vout mode with only bit 5 and 6 ? And clearing the bit 7 in the driver (tps546d24.c) probe function would not be needed, right ? Thanks Duke > > 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 > > > >
