From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 269C33ED5A7 for ; Tue, 31 Mar 2026 09:48:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.45 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774950482; cv=none; b=EaxXc2tC4GaPuEql0T9QOPkOxunMCm8NZA8HPzasglsZydn+3cIHx0YYSTm/QIFXOgSFiKB1En+P/B7VxYuU82iCvh0bc11y7UrZ3RwIGggKzuhpKdp9HTEyjvZrf5owtKNpKRAD3dWkXk39yhjYn/PV3bLks32q7xuyu7ZZZnc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774950482; c=relaxed/simple; bh=enjXq1IaosHLxnBgQhk2lHI8aL7+jqMUWCiR5kNvnlI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=WSq15439ZU5LEEOi+KM1l5nK9v47YJMBDdT9scxuKeileVRm8TzFJqNCVyZdhFu1aFNAWxLzCnxEqXomY9uyvtXjF6z0VAFN9knCHtEi5vFuuPl/JKq1SjBxJYyBquvjW9MP49n73lnThDCyHEAJBf7NW6eVHpxHfTxKbs8RU7M= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=adzEQltm; arc=none smtp.client-ip=209.85.128.45 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="adzEQltm" Received: by mail-wm1-f45.google.com with SMTP id 5b1f17b1804b1-4852a9c6309so46179975e9.0 for ; Tue, 31 Mar 2026 02:47:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1774950478; x=1775555278; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=HmEtPoEG1EUMKwQsf4OvvafgJCsYzJUriFfog6MNbws=; b=adzEQltmPRXJzxV64O8v8A+IFV8kFJoXexFbIIoSwvYutmRN9qZaOwWysUZkM+Hpa2 NFdcRYQvUnuySWJA5DAtHYaAdoMx5E7FCodw2oF8kgxAdPzzk29snueyyQ79FIARaPl4 EEvzQDnvDw8MKn8YhJkvQSVehu0Tkr8OslqT+iLoFWY9JAiIya9UYTaG/kZyE1qYVhJB cp6dLe9D3s6p83aCu/nqnsQ1MeZ2BTjLItlXL+FN7HUNm2NVc5r2V/CjDb3TyGzKf81q vNZNACzlI+r3W7FDtZgVfewB8jfTZRBP767CO+Hk1fOWwdEaFM0ev8oqY0N1P8JG4P70 eMRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774950478; x=1775555278; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=HmEtPoEG1EUMKwQsf4OvvafgJCsYzJUriFfog6MNbws=; b=oh6Vk+oQpRZbF8Iv/nsxoCX5kb+WUx6Re8M69zEvikJiqO4M8ZAhaJHLDjFeI/QJs+ HEG6t2SqEIY5aJQfYVaPDLg/r6tin6MssL7/j9IoWVKe5IZa/Twg/StBXuyrxQbHEiFd emn3jY/dZmO+/bP79uubYsdJRGPys3h+2eqDcID3qdzgQH8vMleDwPrd3AUJ6gcfJ+u3 ZIDnI9uwI1Bqph+oUyL+J0SkuOeed3c0Km7LJTT28C51XbEy5B6tKe+knk5DOhJCPodm 6W9+s8z8X2SLw0AZK6C42sNYpF1cpNyNIYJoRdy2xsbL1lfZGe+jD23uaRej25YLQPxZ 5qHQ== X-Forwarded-Encrypted: i=1; AJvYcCWGsFxt9DqtDEi6G1DDX4g8RpEuHz7VPDequtxfVimRzQEAn01n2auzR4Vh9Km1lk+mbbiPZ987Mm6q@vger.kernel.org X-Gm-Message-State: AOJu0YyXVEcAUA272qCbhUaqmWUkDA8OxukEnHpcHz8zZt5rWBpIScPF rPB1Hn38a4OMPIno0P+WfiuZ1HI/e7BIJkQqhp+1/G1PNbhHgFg9sxWmjq9MRw== X-Gm-Gg: ATEYQzx0Jn7CcO+Slf3Tw4gC7J7EAtfGXMdnn9d7ROAfcJbc1jGOrXu2IvXuH9mhGrF 4ZF7LzRjfePcuP6lurG0VY4rOeghdQB5mqVv301DyY8W2ZV/kTBIONJ3eG23tzz3pPA1D1y/XW9 pZpbCPx4cK3zZ+SWKlInEI2KAl6HLCmn8vhg+cOJGeg4VvoW/3keYdHaIjHNF7JMYzHGuaUbFYG e4e6a4idRSOQeIUHwCtqAaFsh1U3RlmxhovgOkq+jSSaI8vC2Be5JiW4MSCuNwaqR+N/NwNqe8t Ml1Lz2lqoTPlsTKC3kuZZ7ZsJ21lqMIq1akZERvKiQMgEw7ejjLKta4BhGXS3V2BEr9GC3DCXlG nHbhDg+6o5NuArTzTltEKCe7Yz/grv3ZyoS4zOjVTusbc9zL9Qwqnxzlc43zm75LboeAU1mt8/S Xpdz6UdE6h+NFf X-Received: by 2002:a05:600c:b95:b0:485:5c6e:8a38 with SMTP id 5b1f17b1804b1-48727f63664mr250648925e9.17.1774950478236; Tue, 31 Mar 2026 02:47:58 -0700 (PDT) Received: from nsa ([185.128.9.53]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4887aacb8d0sm14105285e9.3.2026.03.31.02.47.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 Mar 2026 02:47:57 -0700 (PDT) Date: Tue, 31 Mar 2026 10:48:43 +0100 From: Nuno =?utf-8?B?U8Oh?= To: Guenter Roeck Cc: Nuno =?utf-8?B?U8Oh?= , linux-gpio@vger.kernel.org, linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org, linux-doc@vger.kernel.org, Rob Herring , Krzysztof Kozlowski , Conor Dooley , Jonathan Corbet , Linus Walleij , Bartosz Golaszewski Subject: Re: [PATCH v8 2/3] hwmon: ltc4283: Add support for the LTC4283 Swap Controller Message-ID: References: <20260327-ltc4283-support-v8-0-471de255d728@analog.com> <20260327-ltc4283-support-v8-2-471de255d728@analog.com> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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á > > > --- > > > 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, <c4283_regmap_bus, client, > > > + <c4283_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, > > > + <c4283_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 >