Re: [PATCH 2/3] hwmon: ltc4283: Add support for the LTC4284 swap controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Low] hwmon: ltc4283: positional initializers in ltc4283_i2c_id table entries
- [Low] hwmon: ltc4283: duplicated channel info table and chip-ID helpers
- [Low] hwmon: ltc4283: missing default resistor fallback in ltc4283_setup()
- [Low] hwmon: ltc4283: commit message omits functional differences for LTC4284
--

commit 07a1775aa285e31dccd905e19fc784e472406b97
Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

hwmon: ltc4283: Add support for the LTC4284 swap controller

This commit adds support for the Analog Devices LTC4284 negative voltage hot
swap controller to the ltc4283 hwmon driver, including dual current sense
channels and separate ADC power sense resistor handling.

>     hwmon: ltc4283: Add support for the LTC4284 swap controller
>     
>     Support the LTC4284 Hot Swap Controller. It is very Similar to LTC4283 but
>     for higher power. The device features programmable current limit with
>     foldback and independently adjustable inrush current to optimize the
>     MOSFET safe operating area (SOA). The SOA timer limits MOSFET temperature
>     rise for reliable protection against overstresses.
>     
>     An I2C interface and onboard ADC allow monitoring of board current,
>     voltage, power, energy, and fault status.
>     
>     Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

[Severity: Low]
This isn't a bug, but the commit message mostly repeats generic hot-swap
controller features already shared with the LTC4283.

Could the changelog also describe the functional differences added to the
driver for the LTC4284, specifically the two current sense channels (SENSE1
and SENSE2) with separate sense resistors (rsense and rsense2) and the
dedicated ADC sense resistor (radc) used for power and energy calculations?

> diff --git a/drivers/hwmon/ltc4283.c b/drivers/hwmon/ltc4283.c
> index 9ec409bc26482..d27c211146152 100644
> --- a/drivers/hwmon/ltc4283.c
> +++ b/drivers/hwmon/ltc4283.c
[ ... ]
> @@ -173,7 +192,19 @@ enum {
>  #define LTC4283_ADIO34_MAX \
>  	LTC4283_ADC_2_MAX_DIFF(LTC4283_CHAN_ADIO34 - LTC4283_CHAN_ADIN12)
>  
> +enum ltc4283_chip_id {
> +	ID_LTC4283,
> +	ID_LTC4284,
> +};
> +
> +struct ltc4283_chip_info {
> +	enum ltc4283_chip_id id;
> +	const char *name;
> +	const struct hwmon_chip_info *hwmon_info;
> +};
> +
>  struct ltc4283_hwmon {
> +	const struct ltc4283_chip_info *info;
>  	struct regmap *map;
>  	struct i2c_client *client;
>  	unsigned long gpio_mask;
> @@ -182,12 +213,118 @@ struct ltc4283_hwmon {
>  	unsigned long power_max;
>  	/* in millivolt */
>  	u32 vsense_max;
> -	/* in tenths of microohm*/
> +	/* in tenths of microohm - For LTC4283 SENSE or LTC4284 SENSE1 */
>  	u32 rsense;
> +	/* in tenths of microohm - For LTC4284 SENSE2 only */
> +	u32 rsense2;
> +	/* in tenths of microohm - For LTC4284 ADC power measurement */
> +	u32 radc;
>  	bool energy_en;
>  	bool ext_fault;
>  };
>  
> +static u32 ltc4283_get_rsense(const struct ltc4283_hwmon *st, u32 channel)
> +{
> +	if (channel)
> +		return st->rsense2;
> +
> +	return st->rsense;
> +}
> +
> +static u32 ltc4283_get_rpower(const struct ltc4283_hwmon *st)
> +{
> +	/* For LTC4284, ADC uses radc. For LTC4283, ADC uses rsense. */
> +	if (st->info->id == ID_LTC4284)
> +		return st->radc;
> +
> +	return st->rsense;
> +}
> +
> +static u8 ltc4283_get_sense_reg(const struct ltc4283_hwmon *st, u32 channel)
> +{
> +	if (st->info->id == ID_LTC4283)
> +		return LTC4283_SENSE;
> +
> +	if (!channel)
> +		return LTC4284_SENSE1;
> +
> +	return LTC4284_SENSE2;
> +}
> +
> +static u8 ltc4283_get_sense_min_reg(const struct ltc4283_hwmon *st, u32 channel)
> +{
> +	if (st->info->id == ID_LTC4283)
> +		return LTC4283_SENSE_MIN;
> +
> +	if (!channel)
> +		return LTC4284_SENSE1_MIN;
> +
> +	return LTC4284_SENSE2_MIN;
> +}
> +
> +static u8 ltc4283_get_sense_max_reg(const struct ltc4283_hwmon *st, u32 channel)
> +{
> +	if (st->info->id == ID_LTC4283)
> +		return LTC4283_SENSE_MAX;
> +
> +	if (!channel)
> +		return LTC4284_SENSE1_MAX;
> +
> +	return LTC4284_SENSE2_MAX;
> +}
> +
> +static u8 ltc4283_get_sense_min_th_reg(const struct ltc4283_hwmon *st, u32 channel)
> +{
> +	if (st->info->id == ID_LTC4283)
> +		return LTC4283_SENSE_MIN_TH;
> +
> +	if (!channel)
> +		return LTC4284_SENSE1_MIN_TH;
> +
> +	return LTC4284_SENSE2_MIN_TH;
> +}
> +
> +static u8 ltc4283_get_sense_max_th_reg(const struct ltc4283_hwmon *st, u32 channel)
> +{
> +	if (st->info->id == ID_LTC4283)
> +		return LTC4283_SENSE_MAX_TH;
> +
> +	if (!channel)
> +		return LTC4284_SENSE1_MAX_TH;
> +
> +	return LTC4284_SENSE2_MAX_TH;
> +}
> +
> +static u8 ltc4283_get_curr_alarm_reg(const struct ltc4283_hwmon *st)
> +{
> +	if (st->info->id == ID_LTC4284)
> +		return LTC4283_ADC_ALM_LOG_4;
> +
> +	return LTC4283_ADC_ALM_LOG_1;
> +}
> +
> +static u8 ltc4283_get_curr_alarm_high_mask(const struct ltc4283_hwmon *st, u32 channel)
> +{
> +	if (st->info->id == ID_LTC4283)
> +		return LTC4283_SENSE_HIGH_ALM;
> +
> +	if (!channel)
> +		return LTC4284_SENSE1_HIGH_ALM;
> +
> +	return LTC4284_SENSE2_HIGH_ALM;
> +}
> +
> +static u8 ltc4283_get_curr_alarm_low_mask(const struct ltc4283_hwmon *st, u32 channel)
> +{
> +	if (st->info->id == ID_LTC4283)
> +		return LTC4283_SENSE_LOW_ALM;
> +
> +	if (!channel)
> +		return LTC4284_SENSE1_LOW_ALM;
> +
> +	return LTC4284_SENSE2_LOW_ALM;
> +}

[ ... ]

> @@ -974,15 +1133,32 @@ static int ltc4283_read_labels(struct device *dev,
>  static int ltc4283_set_max_limits(struct ltc4283_hwmon *st, struct device *dev)
>  {
>  	u32 temp = st->vsense_max * DECA * MICRO;
> +	u32 resistor = ltc4283_get_rpower(st);
>  	int ret;
>  
> -	ret = ltc4283_write_in_byte(st, LTC4283_SENSE_MAX_TH, LTC4283_ADC1_FS_uV,
> -				    st->vsense_max * MILLI);
> -	if (ret)
> -		return ret;
> +	if (st->info->id == ID_LTC4283) {
> +		ret = ltc4283_write_in_byte(st, LTC4283_SENSE_MAX_TH, LTC4283_ADC1_FS_uV,
> +					    st->vsense_max * MILLI);
> +		if (ret)
> +			return ret;
> +	} else {
> +		/* LTC4284: Set max limits for SENSE1 and SENSE2 */
> +		ret = ltc4283_write_in_byte(st, LTC4284_SENSE1_MAX_TH,
> +					    LTC4283_ADC1_FS_uV,
> +					    st->vsense_max * MILLI);
> +		if (ret)
> +			return ret;
> +
> +		ret = ltc4283_write_in_byte(st, LTC4284_SENSE2_MAX_TH,
> +					    LTC4283_ADC1_FS_uV,
> +					    st->vsense_max * MILLI);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/* Power is given by ISENSE * Vout. */
> -	st->power_max = DIV_ROUND_CLOSEST(temp, st->rsense) * LTC4283_ADC2_FS_mV;
> +	st->power_max = DIV_ROUND_CLOSEST(temp, resistor) * LTC4283_ADC2_FS_mV;
> +
>  	return ltc4283_write_power_byte(st, LTC4283_POWER_MAX_TH, st->power_max);
>  }
>  
> @@ -1271,6 +1447,45 @@ static int ltc4283_setup(struct ltc4283_hwmon *st, struct device *dev)
>  	 */
>  	st->rsense /= CENTI;
>  
> +	/*
> +	 * For LTC4284, rsense is for SENSE1, rsense2 for SENSE2, and radc is
> +	 * the effective sense resistance for ADC+- pin.
> +	 */
> +	if (st->info->id == ID_LTC4284) {
> +		ret = device_property_read_u32(dev, "adi,rsense2-nano-ohms",
> +					       &st->rsense2);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to read adi,rsense2-nano-ohms\n");
> +
> +		if (st->rsense2 < LTC4283_MIN_RSENSE || st->rsense2 > LTC4283_MAX_RSENSE)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "adi,rsense2-nano-ohms(%u) too small or too large [%u %u]\n",
> +					     st->rsense2, LTC4283_MIN_RSENSE, LTC4283_MAX_RSENSE);
> +
> +		st->rsense2 /= CENTI;
> +
> +		ret = device_property_read_u32(dev, "adi,radc-nano-ohms",
> +					       &st->radc);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to read adi,radc-nano-ohms\n");
> +
> +		if (st->radc < LTC4283_MIN_RSENSE || st->radc > LTC4283_MAX_RSENSE)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "adi,radc-nano-ohms(%u) too small or too large [%u %u]\n",
> +					     st->radc, LTC4283_MIN_RSENSE, LTC4283_MAX_RSENSE);
> +
> +		st->radc /= CENTI;

[Severity: Low]
In ltc4283_setup(), st->rsense is initialized to LTC4283_MIN_RSENSE before
calling device_property_read_u32(dev, "adi,rsense-nano-ohms", &st->rsense) so
that the driver can probe without firmware properties:

	st->rsense = LTC4283_MIN_RSENSE;
	ret = device_property_read_u32(dev, "adi,rsense-nano-ohms",
				       &st->rsense);
	if (!ret) {
		if (st->rsense < LTC4283_MIN_RSENSE || st->rsense > LTC4283_MAX_RSENSE)
			return dev_err_probe(...);
	}

When an LTC4284 device is instantiated via ltc4283_i2c_id without firmware
properties, won't ltc4283_probe() -> ltc4283_setup() fail because
device_property_read_u32() is required to succeed for adi,rsense2-nano-ohms
and adi,radc-nano-ohms instead of defaulting to LTC4283_MIN_RSENSE?

> +
> +		/* Enable SENSE1 and SENSE2 */
> +		ret = regmap_set_bits(st->map, LTC4283_ADC_SELECT_2,
> +				      LTC4284_SENSE1_SELECT_MASK |
> +				      LTC4284_SENSE2_SELECT_MASK);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = device_property_read_u32(dev, "adi,current-limit-sense-microvolt",
>  				       &st->vsense_max);
>  	if (!ret) {
> @@ -1558,6 +1773,91 @@ static const struct hwmon_channel_info * const ltc4283_info[] = {
>  	NULL
>  };
>  
> +static const struct hwmon_channel_info * const ltc4284_info[] = {
[ ... ]
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT | HWMON_C_LOWEST | HWMON_C_HIGHEST |
> +			   HWMON_C_MAX | HWMON_C_MIN | HWMON_C_MIN_ALARM |
> +			   HWMON_C_MAX_ALARM | HWMON_C_CRIT_ALARM |
> +			   HWMON_C_RESET_HISTORY | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LOWEST | HWMON_C_HIGHEST |
> +			   HWMON_C_MAX | HWMON_C_MIN | HWMON_C_MIN_ALARM |
> +			   HWMON_C_MAX_ALARM | HWMON_C_CRIT_ALARM |
> +			   HWMON_C_RESET_HISTORY | HWMON_C_LABEL),
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_INPUT | HWMON_P_INPUT_LOWEST |
> +			   HWMON_P_INPUT_HIGHEST | HWMON_P_MAX | HWMON_P_MIN |
> +			   HWMON_P_MAX_ALARM | HWMON_P_MIN_ALARM |
> +			   HWMON_P_RESET_HISTORY | HWMON_P_LABEL),
> +	HWMON_CHANNEL_INFO(energy,
> +			   HWMON_E_ENABLE),
> +	HWMON_CHANNEL_INFO(energy64,
> +			   HWMON_E_INPUT),
> +	NULL
> +};

[Severity: Low]
This isn't a bug, but there is quite a bit of duplication here:

1. ltc4284_info[] copies all 16 voltage channels, the power channel, and the
energy channels from ltc4283_info[] just to add a second current channel.
Could both current channels be defined in ltc4283_info[] and channel 1 hidden
for ID_LTC4283 in ltc4283_is_visible() / ltc4283_curr_is_visible(), similar
to how ltc4283_in_is_visible() hides voltage channels?

2. Eight separate helper functions (ltc4283_get_sense_reg(),
ltc4283_get_sense_min_reg(), ltc4283_get_sense_max_reg(),
ltc4283_get_sense_min_th_reg(), ltc4283_get_sense_max_th_reg(),
ltc4283_get_curr_alarm_reg(), ltc4283_get_curr_alarm_high_mask(), and
ltc4283_get_curr_alarm_low_mask()) branch on st->info->id. Could these
register addresses and masks be stored in struct ltc4283_chip_info instead?

3. ltc4283_set_max_limits() branches on st->info->id == ID_LTC4283 to select
LTC4283_SENSE_MAX_TH vs LTC4284_SENSE1_MAX_TH instead of reusing
ltc4283_get_sense_max_th_reg(st, 0).

4. ltc4283_setup() repeats the property read, [LTC4283_MIN_RSENSE,
LTC4283_MAX_RSENSE] range check, and division by CENTI three times for
adi,rsense-nano-ohms, adi,rsense2-nano-ohms, and adi,radc-nano-ohms. Could
that be factored into a small helper?

[ ... ]

> @@ -1769,13 +2090,15 @@ static int ltc4283_probe(struct i2c_client *client)
>  }
>  
>  static const struct of_device_id ltc4283_of_match[] = {
> -	{ .compatible = "adi,ltc4283" },
> +	{ .compatible = "adi,ltc4283", .data = &chip_info_ltc4283 },
> +	{ .compatible = "adi,ltc4284", .data = &chip_info_ltc4284 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, ltc4283_of_match);
>  
>  static const struct i2c_device_id ltc4283_i2c_id[] = {
> -	{ "ltc4283" },
> +	{ "ltc4283", (kernel_ulong_t)&chip_info_ltc4283 },
> +	{ "ltc4284", (kernel_ulong_t)&chip_info_ltc4284 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, ltc4283_i2c_id);

[Severity: Low]
This isn't a bug, but should the modified and new entries in ltc4283_i2c_id[]
use named initializers (.name and .driver_data) to match I2C subsystem
conventions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-staging-ltc4284-v1-0-49fce67ebc48@analog.com?part=2