Re: [PATCH v5 0/3] hwmon: Add support for the LTC4283 Hot Swap Controller

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

On Wed, 2026-01-28 at 08:55 -0800, Guenter Roeck wrote:
> On Wed, Jan 28, 2026 at 02:04:35AM -0800, Guenter Roeck wrote:
> > On 1/28/26 01:44, Nuno Sá wrote:
> > > Hi Guenter,
> > > 
> > > On Tue, 2026-01-27 at 09:39 -0800, Guenter Roeck wrote:
> > > > Hi Nuno,
> > > > 
> > > > On 12/23/25 04:21, Nuno Sá via B4 Relay wrote:
> > > > > This is v3 for the LTC4283 how swap controller. Main change is that I'm
> > > > > now using the auxiliary bus for adding the GPIO device (done depending
> > > > > on FW properties).
> > > > > 
> > > > > Similar to the LTC4282 device, we're clearing some fault logs in the
> > > > > reset_history attributes.
> > > > > 
> > > > 
> > > > I ran the patch series through an AI review. Results are below.
> > > > Please take a look.
> > > > 
> 
> Another run through AI below. Please let me know if there are further inaccuracies.
> 
> Thanks,
> Guenter
> 
> ---
> commit 82433332ce165903d56fae327ced975aeef30475
> Author:     Nuno Sá <nuno.sa@analog.com>
> 
> hwmon: ltc4283: Add support for the LTC4283 Swap Controller
> 
> Adds support for the LTC4283 Hot Swap Controller I2C interface.
> 
> > diff --git a/drivers/hwmon/ltc4283.c b/drivers/hwmon/ltc4283.c
> > new file mode 100644
> > index 000000000000..402b53f81548
> > --- /dev/null
> > +++ b/drivers/hwmon/ltc4283.c
> 
> [ ... ]
> 
> > +static int ltc4283_read_power_word(const struct ltc4283_hwmon *st,
> > +				   u32 reg, long *val)
> > +{
> > +	u64 temp = (u64)LTC4283_ADC1_FS_uV * LTC4283_ADC2_FS_mV * DECA * MILLI;
> > +	unsigned int __raw;
> > +	int ret;
> > +
> > +	ret = regmap_read(st->map_word, reg, &__raw);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Power is given by:
> > +	 *     P = CODE(16b) * 32.768mV * 2.048V / (2^16 * Rsense)
> > +	 */
> > +	*val = DIV64_U64_ROUND_CLOSEST(temp * __raw, BIT_ULL(16) * st->rsense);
> 
> Does this overflow *val on 32-bit systems if the calculated power exceeds
> 2000 Watts (approx 2^31 microWatts)?  The intermediate calculation is
> u64, but the result is assigned to a signed long.
> 

I'll have to double check this one but IIRC 2000W is not a realistic value for
these applications. One thing that could overflow is power_max given that the default
register value is 0xff but we do write the proper max value depending on
rsense (I actually need to check how will this behave in case we use a default
rsense value - ie with no DT property).

> [ ... ]
> 
> > +static bool ltc4283_word_reg(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case LTC4283_SENSE ... LTC4283_ADIO34_MAX:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> 
> LTC4283_SENSE is 0x41 and LTC4283_ADIO34_MAX is 0x40.  Does this invalid
> range (start > end) effectively disable access to the word registers
> via st->map_word?

The above is odd ad I'm pretty sure I ran sensors on this but could be some late edit
mistake. Anyways irrelevant for the next version as you prefer the custom bus regmap
approach.

> 
> > +static const struct regmap_config ltc4283_word_regmap_config = {
> > +	.name = "ltc4283-16",
> > +	.reg_bits = 8,
> > +	.val_bits = 16,
> > +	.max_register = LTC4283_ADIO34_MAX,
> > +	.val_format_endian = REGMAP_ENDIAN_BIG,
> > +	.writeable_reg = ltc4283_word_reg,
> > +	.readable_reg = ltc4283_word_reg,
> > +};
> 
> Does max_register = 0x40 prevent access to the sensor value registers
> (LTC4283_SENSE at 0x41 and above) which are intended to be read as
> words?

ditto

> 
> commit 5998c620fbd8915340a4b92635c9489a91fae567
> Author:     Nuno Sá <nuno.sa@analog.com>
> 
> gpio: gpio-ltc4283: Add support for the LTC4283 Swap Controller
> 
> Adds GPIO support for the LTC4283.
> 
> > diff --git a/drivers/gpio/gpio-ltc4283.c b/drivers/gpio/gpio-ltc4283.c
> > new file mode 100644
> > index 000000000000..fa0c106d4fac
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-ltc4283.c
> 
> [ ... ]
> 
> > +#define LTC4283_PINS_MAX			8
> > +#define LTC4283_PGIOX_START_NR			4
> > +#define LTC4283_INPUT_STATUS			0x02
> > +#define LTC4283_PGIO_CONFIG			0x10
> > +#define   LTC4283_PGIO_CFG_MASK(pin) 
> > +	GENMASK(((pin) - LTC4283_PGIOX_START_NR) * 2 + 1, (((pin) - LTC4283_PGIOX_START_NR) *
> > 2))
> > +#define LTC4283_PGIO_CONFIG_2			0x11
> > +
> > +#define LTC42823_ADIO_CONFIG			0x12
> 
> This isn't a bug, but should this be LTC4283_ADIO_CONFIG instead of
> LTC42823?

Indeed!

- Nuno Sá
		2