Re: [PATCH v4 7/7] hwmon: (pmbus/tps25990): add initial support

[Guenter Roeck <linux@roeck-us.net>]

On Tue, Nov 05, 2024 at 06:58:44PM +0100, Jerome Brunet wrote:
> Add initial support for the Texas Instruments TPS25990 eFuse.
> This adds the basic PMBUS telemetry support for the device.
> 
> Tested-by: Vaishnav Achath <vaishnav.a@ti.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  Documentation/hwmon/index.rst    |   1 +
>  Documentation/hwmon/tps25990.rst | 148 ++++++++++++++
>  MAINTAINERS                      |   2 +
>  drivers/hwmon/pmbus/Kconfig      |  17 ++
>  drivers/hwmon/pmbus/Makefile     |   1 +
>  drivers/hwmon/pmbus/tps25990.c   | 428 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 597 insertions(+)
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 55f1111594b2e9ada4a881e5d4d8884f33256d1f..1a3cb0a59f7210b8a5e972a8015658b983834cd2 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -236,6 +236,7 @@ Hardware Monitoring Kernel Drivers
>     tmp464
>     tmp513
>     tps23861
> +   tps25990
>     tps40422
>     tps53679
>     tps546d24
> diff --git a/Documentation/hwmon/tps25990.rst b/Documentation/hwmon/tps25990.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..ed9e74d43e2c2f070d3abe987d93bcdfcf2162ec
> --- /dev/null
> +++ b/Documentation/hwmon/tps25990.rst
> @@ -0,0 +1,148 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver tps25990
> +======================
> +
> +Supported chips:
> +
> +  * TI TPS25990
> +
> +    Prefix: 'tps25990'
> +
> +  * Datasheet
> +
> +    Publicly available at Texas Instruments website: https://www.ti.com/lit/gpn/tps25990
> +
> +Author:
> +
> +	Jerome Brunet <jbrunet@baylibre.com>
> +
> +Description
> +-----------
> +
> +This driver implements support for TI TPS25990 eFuse.
> +This is an integrated, high-current circuit protection and power
> +management device with PMBUS interface
> +
> +Device compliant with:
> +
> +- PMBus rev 1.3 interface.
> +
> +Device supports direct format for reading input voltages,
> +output voltage, input current, input power and temperature.
> +
> +Due to the specificities of the chip, all history reset attributes
> +are tied together. Resetting the history of a sensor, resets the
> +history of all the sensors.
> +
> +The driver exports the following attributes via the 'sysfs' files
> +for input current:
> +
> +**curr1_average**
> +
> +**curr1_crit**
> +
> +**curr1_crit_alarm**
> +
> +**curr1_highest**
> +
> +**curr1_input**
> +
> +**curr1_label**
> +
> +**curr1_max**
> +
> +**curr1_max_alarm**
> +
> +**curr1_reset_history**
> +
> +The driver provides the following attributes for main input voltage:
> +
> +**in1_average**
> +
> +**in1_crit**
> +
> +**in1_crit_alarm**
> +
> +**in1_highest**
> +
> +**in1_input**
> +
> +**in1_label**
> +
> +**in1_lcrit**
> +
> +**in1_lcrit_alarm**
> +
> +**in1_lowest**
> +
> +**in1_max**
> +
> +**in1_max_alarm**
> +
> +**in1_min**
> +
> +**in1_min_alarm**
> +
> +**in1_reset_history**
> +
> +The driver provides the following attributes for auxiliary input voltage:
> +
> +**in2_input**
> +
> +**in2_label**
> +
> +The driver provides the following attributes for output voltage:
> +
> +**in3_average**
> +
> +**in3_input**
> +
> +**in3_label**
> +
> +**in3_lowest**
> +
> +**in3_min**
> +
> +**in3_min_alarm**
> +
> +**in3_reset_history**
> +
> +The driver provides the following attributes for input power:
> +
> +**power1_alarm**
> +
> +**power1_average**
> +
> +**power1_input**
> +
> +**power1_input_highest**
> +
> +**power1_label**
> +
> +**power1_max**
> +
> +**power1_reset_history**
> +
> +The driver provides the following attributes for temperature:
> +
> +**temp1_average**
> +
> +**temp1_crit**
> +
> +**temp1_crit_alarm**
> +
> +**temp1_highest**
> +
> +**temp1_input**
> +
> +**temp1_max**
> +
> +**temp1_max_alarm**
> +
> +**temp1_reset_history**
> +
> +The driver provides the following attributes for sampling:
> +
> +**samples**
> +
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4f21d7d2ce992f14d8c533f0c8742edb22a0db3f..10a65cd5c84e56cf876ee5eb06336b5bc8ff991c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22816,6 +22816,8 @@ M:	Jerome Brunet <jbrunet@baylibre.com>
>  L:	linux-hwmon@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/hwmon/pmbus/ti,tps25990.yaml
> +F:	Documentation/hwmon/tps25990.rst
> +F:	drivers/hwmon/pmbus/tps25990.c
>  
>  TEXAS INSTRUMENTS TPS23861 PoE PSE DRIVER
>  M:	Robert Marko <robert.marko@sartura.hr>
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index f6d3528419536a68011d67a4a239c0cba1bbf475..22418a05ced0c4d7025a243134f231c54c741371 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -510,6 +510,23 @@ config SENSORS_TDA38640_REGULATOR
>  	  If you say yes here you get regulator support for Infineon
>  	  TDA38640 as regulator.
>  
> +config SENSORS_TPS25990
> +	tristate "TI TPS25990"
> +	help
> +	  If you say yes here you get hardware monitoring support for TI
> +	  TPS25990.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called tps25990.
> +
> +config SENSORS_TPS25990_REGULATOR
> +	bool "Regulator support for TPS25990 and compatibles"
> +	depends on SENSORS_TPS25990 && REGULATOR
> +	default SENSORS_TPS25990
> +	help
> +	  If you say yes here you get regulator support for Texas Instruments
> +	  TPS25990.
> +
>  config SENSORS_TPS40422
>  	tristate "TI TPS40422"
>  	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index d00bcc758b97200b80158e33b0ac41e6e5ac3231..3d3183f8d2a7060eb513f54f4f0a78ba37c09393 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -51,6 +51,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_TPS25990)	+= tps25990.o
>  obj-$(CONFIG_SENSORS_TPS40422)	+= tps40422.o
>  obj-$(CONFIG_SENSORS_TPS53679)	+= tps53679.o
>  obj-$(CONFIG_SENSORS_TPS546D24)	+= tps546d24.o
> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..db59638f6c626f2467b859f023523276cfb6c3a5
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/tps25990.c
> @@ -0,0 +1,428 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2024 BayLibre, SAS.
> +// Author: Jerome Brunet <jbrunet@baylibre.com>
> +
> +#include <linux/bitfield.h>
> +#include <linux/debugfs.h>
> +#include <linux/err.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include "pmbus.h"
> +
> +#define TPS25990_READ_VAUX		0xd0
> +#define TPS25990_READ_VIN_MIN		0xd1
> +#define TPS25990_READ_VIN_PEAK		0xd2
> +#define TPS25990_READ_IIN_PEAK		0xd4
> +#define TPS25990_READ_PIN_PEAK		0xd5
> +#define TPS25990_READ_TEMP_AVG		0xd6
> +#define TPS25990_READ_TEMP_PEAK		0xd7
> +#define TPS25990_READ_VOUT_MIN		0xda
> +#define TPS25990_READ_VIN_AVG		0xdc
> +#define TPS25990_READ_VOUT_AVG		0xdd
> +#define TPS25990_READ_IIN_AVG		0xde
> +#define TPS25990_READ_PIN_AVG		0xdf
> +#define TPS25990_VIREF			0xe0
> +#define TPS25990_PK_MIN_AVG		0xea
> +#define  PK_MIN_AVG_RST_PEAK		BIT(7)
> +#define  PK_MIN_AVG_RST_AVG		BIT(6)
> +#define  PK_MIN_AVG_RST_MIN		BIT(5)
> +#define  PK_MIN_AVG_AVG_CNT		GENMASK(2, 0)
> +#define TPS25990_MFR_WRITE_PROTECT	0xf8
> +#define  TPS25990_UNLOCKED		BIT(7)
> +
> +#define TPS25990_8B_SHIFT		2
> +#define TPS25990_VIN_OVF_NUM		525100
> +#define TPS25990_VIN_OVF_DIV		10163
> +#define TPS25990_VIN_OVF_OFF		155
> +#define TPS25990_IIN_OCF_NUM		953800
> +#define TPS25990_IIN_OCF_DIV		129278
> +#define TPS25990_IIN_OCF_OFF		157
> +
> +#define PK_MIN_AVG_RST_MASK		(PK_MIN_AVG_RST_PEAK | \
> +					 PK_MIN_AVG_RST_AVG  | \
> +					 PK_MIN_AVG_RST_MIN)
> +
> +/*
> + * Arbitrary default Rimon value: 1kOhm
> + * This correspond to an overcurrent limit of 55A, close to the specified limit
> + * of un-stacked TPS25990 and makes further calculation easier to setup in
> + * sensor.conf, if necessary
> + */
> +#define TPS25990_DEFAULT_RIMON		1000000000
> +
> +static void tps25990_set_m(int *m, u32 rimon)
> +{
> +	u64 val = ((u64)*m) * rimon;
> +
> +	/* Make sure m fits the s32 type */
> +	*m = DIV_ROUND_CLOSEST_ULL(val, 1000000);
> +}
> +
> +static int tps25990_mfr_write_protect_set(struct i2c_client *client,
> +					  u8 protect)
> +{
> +	/*
> +	 * The chip has a single protection mode, set it regardless of
> +	 * the specific protection requested
> +	 */
> +	return pmbus_write_byte_data(client, -1, TPS25990_MFR_WRITE_PROTECT,
> +				     protect ? 0x0 : 0xa2);

After some thought, I think it would be better to reject all protect values
other than 0 (no write protection) and PB_WP_ALL because that is what the chip
supports. Something like

	if (protect & ~PB_WP_ALL)
		return -ENXIO;		// or -EINVAL ? Not really sure.

Thanks,
Guenter