Re: [PATCH v5 3/3] hwmon: (pmbus/tps25990): Add TPS1689 support

On 7/27/26 18:58, Stoyan Bogdanov wrote:
> Extend the existing TPS25990 driver to support the TPS1689 eFuse,
> as both devices share the same command interface and functionality.
> 
> Update the documentation to include TPS1689 support.
> 
> Signed-off-by: Stoyan Bogdanov <[email protected]>
> ---
>   Documentation/hwmon/tps25990.rst | 15 +++--
>   drivers/hwmon/pmbus/tps25990.c   | 98 ++++++++++++++++++++++++++------
>   2 files changed, 92 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/hwmon/tps25990.rst b/Documentation/hwmon/tps25990.rst
> index 04faec780d26..e8bc9a550bda 100644
> --- a/Documentation/hwmon/tps25990.rst
> +++ b/Documentation/hwmon/tps25990.rst
> @@ -9,26 +9,31 @@ Supported chips:
>   
>       Prefix: 'tps25990'
>   
> -  * Datasheet
> +    Datasheet: Publicly available at Texas Instruments website: https://www.ti.com/lit/gpn/tps25990
>   
> -    Publicly available at Texas Instruments website: https://www.ti.com/lit/gpn/tps25990
> +  * TI TPS1689
> +
> +    Prefix: 'tps1689'
> +
> +    Datasheet: Publicly available at Texas Instruments website: https://www.ti.com/lit/gpn/tps1689
>   
>   Author:
>   
>   	Jerome Brunet <[email protected]>
> +	Stoyan Bogdanov <[email protected]>
>   
>   Description
>   -----------
>   
> -This driver implements support for TI TPS25990 eFuse.
> +This driver implements support for TI TPS25990 and TI TPS1689 eFuse chips.
>   This is an integrated, high-current circuit protection and power
>   management device with PMBUS interface
>   
> -Device compliant with:
> +Devices are compliant with:
>   
>   - PMBus rev 1.3 interface.
>   
> -Device supports direct format for reading input voltages,
> +Devices 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
> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
> index 490dcd2a5fa5..7e2991a85da1 100644
> --- a/drivers/hwmon/pmbus/tps25990.c
> +++ b/drivers/hwmon/pmbus/tps25990.c
> @@ -48,6 +48,7 @@
>   					 PK_MIN_AVG_RST_MIN)
>   
>   enum chips {
> +	tps1689,
>   	tps25990,
>   };
>   
> @@ -105,6 +106,8 @@ static int tps25990_mfr_write_protect_get(struct i2c_client *client)
>   static int tps25990_read_word_data(struct i2c_client *client,
>   				   int page, int phase, int reg)
>   {
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	struct tps25990_data *data = container_of(info, struct tps25990_data, info);
>   	int ret;
>   
>   	switch (reg) {
> @@ -193,9 +196,11 @@ static int tps25990_read_word_data(struct i2c_client *client,
>   		ret = pmbus_read_word_data(client, page, phase, reg);
>   		if (ret < 0)
>   			break;
> -		ret = DIV_ROUND_CLOSEST(ret * TPS25990_VIN_OVF_NUM,
> -					TPS25990_VIN_OVF_DIV);
> -		ret += TPS25990_VIN_OVF_OFF;
> +		if (data->chip_id == tps25990) {
> +			ret = DIV_ROUND_CLOSEST(ret * TPS25990_VIN_OVF_NUM,
> +						TPS25990_VIN_OVF_DIV);
> +			ret += TPS25990_VIN_OVF_OFF;
> +		}

Unless I am missing something, TPS1689 also has a voltage offset for this register.

>   		break;
>   
>   	case PMBUS_IIN_OC_FAULT_LIMIT:
> @@ -207,9 +212,11 @@ static int tps25990_read_word_data(struct i2c_client *client,
>   		ret = pmbus_read_byte_data(client, page, TPS25990_VIREF);
>   		if (ret < 0)
>   			break;
> -		ret = DIV_ROUND_CLOSEST(ret * TPS25990_IIN_OCF_NUM,
> -					TPS25990_IIN_OCF_DIV);
> -		ret += TPS25990_IIN_OCF_OFF;
> +		if (data->chip_id == tps25990) {
> +			ret = DIV_ROUND_CLOSEST(ret * TPS25990_IIN_OCF_NUM,
> +						TPS25990_IIN_OCF_DIV);
> +			ret += TPS25990_IIN_OCF_OFF;
> +		}

Looking at the datasheet, I see no difference regarding TPS25990_VIREF
and its use across the two chips. Also, it does not make sense to me to pass
the value of TPS25990_VIREF directoy as OC fault limit. Unless I am missing
something, I don't think this works as intended.

>   		break;
>   
>   	case PMBUS_VIRT_SAMPLES:
> @@ -238,6 +245,8 @@ static int tps25990_read_word_data(struct i2c_client *client,
>   static int tps25990_write_word_data(struct i2c_client *client,
>   				    int page, int reg, u16 value)
>   {
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	struct tps25990_data *data = container_of(info, struct tps25990_data, info);
>   	int ret;
>   
>   	switch (reg) {
> @@ -253,20 +262,23 @@ static int tps25990_write_word_data(struct i2c_client *client,
>   		value = clamp_val(value, 0, 0xff);
>   		ret = pmbus_write_word_data(client, page, reg, value);
>   		break;
> -
>   	case PMBUS_VIN_OV_FAULT_LIMIT:
> -		value -= TPS25990_VIN_OVF_OFF;
> -		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
> -					  TPS25990_VIN_OVF_NUM);
> -		value = clamp_val(value, 0, 0xf);
> +		if (data->chip_id == tps25990) {
> +			value -= TPS25990_VIN_OVF_OFF;
> +			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
> +						  TPS25990_VIN_OVF_NUM);
> +			value = clamp_val(value, 0, 0xf);
> +		}

As pointed out by Sashiko, a clamp is still needed here. Also, again,
according to the datasheet there is still a voltage offset.

>   		ret = pmbus_write_word_data(client, page, reg, value);
>   		break;
>   
>   	case PMBUS_IIN_OC_FAULT_LIMIT:
> -		value -= TPS25990_IIN_OCF_OFF;
> -		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
> -					  TPS25990_IIN_OCF_NUM);
> -		value = clamp_val(value, 0, 0x3f);
> +		if (data->chip_id == tps25990) {
> +			value -= TPS25990_IIN_OCF_OFF;
> +			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
> +						  TPS25990_IIN_OCF_NUM);
> +			value = clamp_val(value, 0, 0x3f);
> +		}

Again, per datasheets, there does not appear to be a difference between
the two chips regarding TPS25990_VIREF.
>   		ret = pmbus_write_byte_data(client, page, TPS25990_VIREF, value);
>   		break;
>   
> @@ -347,6 +359,59 @@ static const struct regulator_desc tps25990_reg_desc[] = {
>   #endif
>   
>   static struct pmbus_driver_info tps25990_base_info[] = {
> +	[tps1689] = {
> +		.pages = 1,
> +		.format[PSC_VOLTAGE_IN] = direct,
> +		.m[PSC_VOLTAGE_IN] = 1166,
> +		.b[PSC_VOLTAGE_IN] = 0,
> +		.R[PSC_VOLTAGE_IN] = -2,
> +		.format[PSC_VOLTAGE_OUT] = direct,
> +		.m[PSC_VOLTAGE_OUT] = 1166,
> +		.b[PSC_VOLTAGE_OUT] = 0,
> +		.R[PSC_VOLTAGE_OUT] = -2,
> +		.format[PSC_TEMPERATURE] = direct,
> +		.m[PSC_TEMPERATURE] = 140,
> +		.b[PSC_TEMPERATURE] = 32103,
> +		.R[PSC_TEMPERATURE] = -2,
> +		/*
> +		 * Current and Power measurement depends on the ohm value
> +		 * of Rimon. m is multiplied by 1000 below to have an integer
> +		 * and -3 is added to R to compensate.
> +		 */
> +		.format[PSC_CURRENT_IN] = direct,
> +		.m[PSC_CURRENT_IN] = 9548,
> +		.b[PSC_CURRENT_IN] = 0,
> +		.R[PSC_CURRENT_IN] = -6,
> +		.format[PSC_CURRENT_OUT] = direct,
> +		.m[PSC_CURRENT_OUT] = 24347,
> +		.b[PSC_CURRENT_OUT] = 0,
> +		.R[PSC_CURRENT_OUT] = -3,
> +		.format[PSC_POWER] = direct,
> +		.m[PSC_POWER] = 2775,
> +		.b[PSC_POWER] = 0,
> +		.R[PSC_POWER] = -4,
> +		.func[0] = (PMBUS_HAVE_VIN |
> +			    PMBUS_HAVE_VOUT |
> +			    PMBUS_HAVE_VMON |
> +			    PMBUS_HAVE_IIN |
> +			    PMBUS_HAVE_PIN |
> +			    PMBUS_HAVE_TEMP |
> +			    PMBUS_HAVE_STATUS_VOUT |
> +			    PMBUS_HAVE_STATUS_IOUT |
> +			    PMBUS_HAVE_STATUS_INPUT |
> +			    PMBUS_HAVE_STATUS_TEMP |
> +			    PMBUS_HAVE_SAMPLES),
> +
> +		.read_word_data = tps25990_read_word_data,
> +		.write_word_data = tps25990_write_word_data,
> +		.read_byte_data = tps25990_read_byte_data,
> +		.write_byte_data = tps25990_write_byte_data,
> +
> +#if IS_ENABLED(CONFIG_SENSORS_TPS25990_REGULATOR)
> +		.reg_desc = tps25990_reg_desc,
> +		.num_regulators = ARRAY_SIZE(tps25990_reg_desc),
> +#endif
> +		},
>   	[tps25990] = {
>   		.pages = 1,
>   		.format[PSC_VOLTAGE_IN] = direct,
> @@ -389,7 +454,6 @@ static struct pmbus_driver_info tps25990_base_info[] = {
>   		.write_word_data = tps25990_write_word_data,
>   		.read_byte_data = tps25990_read_byte_data,
>   		.write_byte_data = tps25990_write_byte_data,
> -
>   #if IS_ENABLED(CONFIG_SENSORS_TPS25990_REGULATOR)
>   		.reg_desc = tps25990_reg_desc,
>   		.num_regulators = ARRAY_SIZE(tps25990_reg_desc),
> @@ -398,12 +462,14 @@ static struct pmbus_driver_info tps25990_base_info[] = {
>   };
>   
>   static const struct i2c_device_id tps25990_i2c_id[] = {
> +	{ "tps1689", tps1689 },
>   	{ "tps25990", tps25990 },
>   	{}
>   };
>   MODULE_DEVICE_TABLE(i2c, tps25990_i2c_id);
>   
>   static const struct of_device_id tps25990_of_match[] = {
> +	{ .compatible = "ti,tps1689", .data = (void *)tps1689 },
>   	{ .compatible = "ti,tps25990", .data = (void *)tps25990 },
>   	{}
>   };