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

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

On Tue, Mar 31, 2026 at 06:31:59AM -0700, Guenter Roeck wrote:
> On 3/31/26 02:48, Nuno Sá wrote:
> > 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?
> > 
> 
> I'd suggest to reject large (unreasonable) values. In this case, rejecting rsense
> values >= 1677721600 should solve the problem.
> 
> > > 
> > > 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.
> > 
> 
> Again, why not just reject unreasonable values such that calculations
> can not overflow ?
> 
> In other drivers, the common approach is to reject unreeasonable values if
> provided through devicetree and to clamp them if provided through sysfs.
> I don't see why that would not work here.

Hi Guenter,

Just FYI, I intended to re-spin today but then I started to double check
the st->power_max logic. If I did not messed up 14.5uOhm is the minimum rsense
we can take so that we don't overflow long on 32bits systems. I'm not sure but
I think it's plausible to have values lower than that. So, bottom line, I
asked internally to some HW folks, who definitely know these systems
better than I do, about that 14,5 min value. I'm waiting for feedback
but it might be that we end up needing power64 attrs as you suggested
some revisions ago.

- Nuno Sá

> 
> Thanks,
> Guenter
>