Re: [PATCH v8 2/3] hwmon: ltc4283: Add support for the LTC4283 Swap Controller

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, Mar 30, 2026 at 08:47:32AM -0700, Guenter Roeck wrote:
> On 3/30/26 02:28, Nuno Sá wrote:
> > Hi Guenter, Regarding AI review, I think most of the points were
> > discussed in previous revisions, but there are two valid.
> > 
> > On Fri, Mar 27, 2026 at 05:26:15PM +0000, Nuno Sá wrote:
> > > Support the LTC4283 Hot Swap Controller. 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: Nuno Sá <nuno.sa@analog.com>
> > > ---
> > >   Documentation/hwmon/index.rst   |    1 +
> > >   Documentation/hwmon/ltc4283.rst |  266 ++++++
> > >   MAINTAINERS                     |    1 +
> > >   drivers/hwmon/Kconfig           |   12 +
> > >   drivers/hwmon/Makefile          |    1 +
> > >   drivers/hwmon/ltc4283.c         | 1796 +++++++++++++++++++++++++++++++++++++++
> > >   6 files changed, 2077 insertions(+)
> > > 
> > 
> > ...
> > 
> > > +static int ltc4283_read_in_alarm(struct ltc4283_hwmon *st, u32 channel,
> > > +				 bool max_alm, long *val)
> > > +{
> > > +	if (channel == LTC4283_VPWR)
> > > +		return ltc4283_read_alarm(st, LTC4283_ADC_ALM_LOG_1,
> > > +					  BIT(2 + max_alm), val);
> > > +
> > > +	if (channel >= LTC4283_CHAN_ADI_1 && channel <= LTC4283_CHAN_ADI_4) {
> > > +		u32 bit = (channel - LTC4283_CHAN_ADI_1) * 2;
> > > +		/*
> > > +		 * Lower channels go to higher bits. We also want to go +1 down
> > > +		 * in the min_alarm case.
> > > +		 */
> > > +		return ltc4283_read_alarm(st, LTC4283_ADC_ALM_LOG_2,
> > > +					  BIT(7 - bit - !max_alm), val);
> > > +	}
> > > +
> > > +	if (channel >= LTC4283_CHAN_ADIO_1 && channel <= LTC4283_CHAN_ADIO_4) {
> > > +		u32 bit = (channel - LTC4283_CHAN_ADIO_1) * 2;
> > > +
> > > +		return ltc4283_read_alarm(st, LTC4283_ADC_ALM_LOG_3,
> > > +					  BIT(7 - bit - !max_alm), val);
> > > +	}
> > > +
> > > +	if (channel >= LTC4283_CHAN_ADIN12 && channel <= LTC4283_CHAN_ADIN34) {
> > > +		u32 bit = (channel - LTC4283_CHAN_ADIN12) * 2;
> > > +
> > > +		return ltc4283_read_alarm(st, LTC4283_ADC_ALM_LOG_5,
> > > +					  BIT(7 - bit - !max_alm), val);
> > > +	}
> > 
> > "Will this condition handle the ADIO12 and ADIO34 differential channels?
> > It looks like channels 14 and 15 fall through to the default return intended
> > for the DRAIN channel. Since reading the alarm implicitly clears the register
> > bits, could reading these ADIO alarms unintentionally clear actual DRAIN
> > alarms? Should the upper bound be LTC4283_CHAN_ADIO34?"
> > 
> > Good catch and should be:
> > 
> > -       if (channel >= LTC4283_CHAN_ADIN12 && channel <= LTC4283_CHAN_ADIN34) {
> > +       if (channel >= LTC4283_CHAN_ADIN12 && channel <= LTC4283_CHAN_ADIO34) {
> > 
> > > +
> > > +	if (channel == LTC4283_CHAN_DRNS)
> > > +		return ltc4283_read_alarm(st, LTC4283_ADC_ALM_LOG_4,
> > > +					  BIT(6 + max_alm), val);
> > > +
> > > +	return ltc4283_read_alarm(st, LTC4283_ADC_ALM_LOG_4, BIT(4 + max_alm),
> > > +				  val);
> > > +}
> > 
> > ...
> > 
> > > +
> > > +static int ltc4283_probe(struct i2c_client *client)
> > > +{
> > > +	struct device *dev = &client->dev, *hwmon;
> > > +	struct auxiliary_device *adev;
> > > +	struct ltc4283_hwmon *st;
> > > +	int ret;
> > > +
> > > +	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> > > +	if (!st)
> > > +		return -ENOMEM;
> > > +
> > > +	if (!i2c_check_functionality(client->adapter,
> > > +				     I2C_FUNC_SMBUS_BYTE_DATA |
> > > +				     I2C_FUNC_SMBUS_WORD_DATA |
> > > +				     I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	st->client = client;
> > > +	st->map = devm_regmap_init(dev, &ltc4283_regmap_bus, client,
> > > +				   &ltc4283_regmap_config);
> > > +	if (IS_ERR(st->map))
> > > +		return dev_err_probe(dev, PTR_ERR(st->map),
> > > +				     "Failed to create regmap\n");
> > > +
> > > +	ret = ltc4283_setup(st, dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	hwmon = devm_hwmon_device_register_with_info(dev, "ltc4283", st,
> > > +						     &ltc4283_chip_info, NULL);
> > > +
> > > +	if (IS_ERR(hwmon))
> > > +		return PTR_ERR(hwmon);
> > > +
> > > +	ltc4283_debugfs_init(st, client);
> > > +
> > > +	if (!st->gpio_mask)
> > > +		return 0;
> > > +
> > > +	adev = devm_auxiliary_device_create(dev, "gpio", &st->gpio_mask);
> > > +	if (!adev)
> > > +		return dev_err_probe(dev, -ENODEV, "Failed to add GPIO device\n");
> > 
> > "Does this allow multiple LTC4283 chips to probe successfully?
> > Without allocating a unique ID per I2C instance, it seems the first probed
> > chip takes the generic name. If a second chip is present, it might attempt
> > to register with the exact same name, resulting in a failure in device_add()
> > and aborting the probe."
> > 
> > Also looks valid and I suspect is one of those that a quick look will
> > find more "offenders". I would purpose:
> > 
> > -       adev = devm_auxiliary_device_create(dev, "gpio", &st->gpio_mask);
> > +       adev = __devm_auxiliary_device_create(dev, KBUILD_MODNAME, "gpio",
> > +                                             &st->gpio_mask, client->addr);
> > 
> 
> That would still fail if there are multiple chips at the same I2C address
> on multiple I2C busses. Check drivers/gpu/drm/bridge/ti-sn65dsi86.c which has
> the same problem.

I did looked at that one but totally forgot the multiple busses
scenario.

> 
> > If there's nothing else and you agree with the above, is this something
> > you can tweak while applying or should I spin a new version?
> > 
> 
> Please respin. Also, regarding the other concerns:
> 
>   Can BIT(8) * st->rsense wrap to zero on 32-bit architectures?
>   BIT(8) is a 32-bit unsigned long and st->rsense is a u32. If a user sets a
>   very large sense resistor value via the device tree, the multiplication could
>   wrap to 0, causing a division-by-zero kernel panic. Should the divisor use
>   BIT_ULL(8)?
> 
> Unless I am missing something, this _can_ overflow. Try to provide a sense
> resistor value of 1677721600. Yes, it is unreasonable to specify such large
> rsense values, but why not just limit it such that it does not overflow ?

Yes, that's pretty much my reasoning (regarding the unreasonable
rsense). I could just make BIT_ULL() and be done with it. I can also
also cap rsense to a max value but i'm not 100% what that value would
be. Maybe 1 ohm is already more than reasonable. I can also ask internally. Any
preference on this one?

> 
> Also, for the overflow concerns, if you are sure they can not happen, I'll
> really need to write the unit test code to make sure that this is indeed
> the case.
>

Hmm, for the val * MILLI case, well it should not happen but given it
depends on user input, better if I clamp it before passing the
value to ltc4283_write_in_byte(). Yes, we clamp again inside the
write_bytes() API but not a big deal.

For the st->power_max is again one of those cases where the values would
not make sense (I think - the combination of vsense_max and rsense). Just looking
at the code, it can overflow but this one I'm not really sure how we could handle it.
Maybe clamp power_max to U8_MAX and have a warning message in ltc4283_read_power_byte() if
we overflow long in which case we need a power64 attr?

But even clamping does not make much sense here. The power limit register
is 8 bits, so if our design (rsense + vsense_max) overflows that,
there's nothing we can do other that erroring out.

- Nuno Sá

> Thanks,
> Guenter
>