Re: [PATCH v3] hwmon: add a driver for the temp/voltage sensor on PolarFire SoC

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3688 bytes --]

On Thu, Jun 11, 2026 at 09:41:49AM -0700, Guenter Roeck wrote:
> On Thu, Jun 11, 2026 at 05:21:41PM +0100, Conor Dooley wrote:
> > On Thu, Jun 11, 2026 at 03:13:16PM +0000, sashiko-bot@kernel.org wrote:
> > > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> > > - [Medium] Negative values written to `update_interval` are incorrectly clamped to the maximum interval instead of returning an error or clamping to 0.
> > > - [Medium] Reading `update_interval` inconsistently under-reports the configured value due to integer division truncation.
> > > - [Medium] Hardware sensors are enabled in `probe` but are not disabled on probe failure or driver unbind.
> > > --
> > > 
> > > commit 91a403f5c5eef9fbcfcab3857179dc2f03ad3c89
> > > Author: Lars Randers <lranders@mail.dk>
> > > 
> > > hwmon: add a driver for the temp/voltage sensor on PolarFire SoC
> > > 
> > > Add a driver for the temperature and voltage sensors on PolarFire SoC.
> > > The temperature reports how hot the die is, and the voltages are the
> > > SoC's 1.05, 1.8 and 2.5 volt rails respectively.
> > > 
> > > > diff --git a/drivers/hwmon/tvs-mpfs.c b/drivers/hwmon/tvs-mpfs.c
> > > > new file mode 100644
> > > > index 0000000000000..48de8d98b3444
> > > > --- /dev/null
> > > > +++ b/drivers/hwmon/tvs-mpfs.c
> > > 
> > > [ ... ]
> > > 
> > > > +static int mpfs_tvs_interval_read(struct mpfs_tvs *data, u32 attr, long *val)
> > > > +{
> > > > +	u32 tmp;
> > > > +
> > > > +	if (attr != hwmon_chip_update_interval)
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > > +	regmap_read(data->regmap, MPFS_TVS_CTRL, &tmp);
> > > > +	*val = FIELD_GET(MPFS_TVS_INTERVAL_MASK, tmp);
> > > > +	*val *= MPFS_TVS_INTERVAL_SCALE;
> > > > +	*val /= 1000;
> > > 
> > > [Severity: Medium]
> > > Will this return an incorrectly truncated value for certain intervals?
> > > 
> > > Since integer division is used both here and in mpfs_tvs_interval_write(), 
> > > the truncation can cause the readback value to be lower than what was written.
> > > For example, writing 7 ms calculates `(7 * 1000) / 32 = 218`. Reading it back
> > > computes `(218 * 32) / 1000 = 6` ms. 
> > > 
> > > Should DIV_ROUND_CLOSEST be used for these conversions to ensure the value
> > > survives the round trip?
> > 
> > Hmm, this is actually a good point that I overlooked.
> > What is the desired rounding behaviour here?
> > IOW, should asking for 7 ms produce the closest interval less than 7 ms or
> > the closest interval greater than 7 ms?
> > 
> > A consistent rounding is better than rounding to the closest IMO, if both
> > update_interval and update_interval_us are going to be supported
> > 
> 
> Not sure what you mean with "consistent rounding". Effectively the current

What I meant was that it would round to an interval that would either be
at least (always down) or at most (always up) as frequently as requested.
If, for example, the interval was set to 7400 or 7600 us via
update_interval_us, and then read back from update_interval, that the
rounding used for that would do the same thing as operating entirely via
update_interval and always display an interval that was at least as or
as most as frequent as the configured value.
Rounding closest would of course produce different results for 7400 and
7600, with one being more frequent and one less frequent than reality.

> code is rounding down. I personally like using DIV_ROUND_CLOSEST() for both
> read and write conversions because it gives the most accurate result. So
> (7 * 1000) / 32 = 218.75 would result in 219 and (219 * 32) / 1000 = 7.008
> would result in 7.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]