Re: [PATCH v3] hwmon: Add driver for the TEXAS TPS546D24 Buck Converter.

[Guenter Roeck]

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.

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
>