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

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

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);

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?

Thanks!
- Nuno Sá