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

[Conor Dooley <conor@kernel.org>]

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

On Mon, Jun 08, 2026 at 10:03:48AM -0700, Guenter Roeck wrote:
> On 6/3/26 06:19, Conor Dooley wrote:
> > From: Lars Randers <lranders@mail.dk>
> > 
> > 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.
> > 
> > The hardware supports alarms in theory, but there is an erratum that
> > prevents clearing them once triggered, so no support is added for them.
> > 
> > The hardware measures voltage with 16 bits, of which 1 is a sign bit and
> > the remainder holds the voltage as a fixed point integer value. It's
> > improbable that the hardware will work if the voltages are negative, so
> > the driver ignores the sign bits.
> > 
> > There's no dt support etc here because this is the child of a simple-mfd
> > syscon.
> > 
> > Signed-off-by: Lars Randers <lranders@mail.dk>
> > Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Comments inline.

Cheers.

> > v2:
> > - Fix some minor things pointed out by Sashiko including inaccurate
> >    comments, bounds checking of values read from sysfs and Kconfig
> >    dependencies.
> > - Make update_interval use milliseconds instead of microseconds
> >    (I'll add update_interval_us support when that lands, there's a
> >    proposed workaround for the erratum circulating internally, so it'll
> >    probably come alongside alarm support).
> > 
> > CC: Guenter Roeck <linux@roeck-us.net>
> > CC: Jonathan Corbet <corbet@lwn.net>
> > CC: Shuah Khan <skhan@linuxfoundation.org>
> > CC: Conor Dooley <conor.dooley@microchip.com>
> > CC: Daire McNamara <daire.mcnamara@microchip.com>
> > CC: linux-hwmon@vger.kernel.org
> > CC: linux-doc@vger.kernel.org
> > CC: linux-kernel@vger.kernel.org
> > CC: linux-riscv@lists.infradead.org
> > CC: Valentina.FernandezAlanis@microchip.com

> > +Usage Notes
> > +-----------
> > +
> > +update_interval has a permitted range of 0 to 8.
> > +
> > +
> 
> It might make sense to document what "0" means.

Sure. The interval governs how much of a delay there is between the end
of one measurement and the start of the next one. Zero means no delay,
both here and in the register. Think that answers your question below
too?

> > +static int mpfs_tvs_temp_read(struct mpfs_tvs *data, u32 attr, long *val)
> > +{
> > +	u32 tmp, control;
> > +
> > +	if (attr != hwmon_temp_input && attr != hwmon_temp_enable)
> > +		return -EOPNOTSUPP;
> > +
> > +	regmap_read(data->regmap, MPFS_TVS_CTRL, &control);
> > +
> > +	if (attr == hwmon_temp_enable) {
> > +		*val = FIELD_GET(MPFS_TVS_CTRL_TEMP_ENABLE, control);
> > +		return 0;
> > +	}
> > +
> > +	if (!(control & MPFS_TVS_CTRL_TEMP_VALID))
> > +		return -EINVAL;
> > +
> "Invalid argument" can not be correct for data read from the chip.
> I don't know what this means. It should be either -ENODATA (no data available)
> if this is transient or -EIO (I/O error) if it is a permanent problem.
> The same applies to other validation checks.

-ENODATA then. It's realistically only possible to hit this when the
channel is disabled, although in you can also hit it in the gap
between the channel being enabled and the first measurement becoming
available.


> > +	regmap_read(data->regmap, MPFS_TVS_OUTPUT1, &tmp);
> > +	*val = FIELD_GET(MPFS_OUTPUT1_TEMP_MASK, tmp);
> > +	*val -= MPFS_TVS_K_TO_C;
> > +	*val = (1000 * *val) >> 4; /* fixed point (11.4) to millidegrees */
> > +
> > +	return 0;
> > +}

> > +static int mpfs_tvs_interval_write(struct mpfs_tvs *data, u32 attr, long val)
> > +{
> > +	unsigned long temp = val;
> > +
> > +	if (attr != hwmon_chip_update_interval)
> > +		return -EOPNOTSUPP;
> > +
> > +	temp *= 1000;
> 
> This is likely to result in overflow issues (for example if val == LONG_MAX).
> 
> > +	temp /= MPFS_TVS_INTERVAL_SCALE;
> > +
> > +	/*
> > +	 * The value is 8 bits wide, but 255 is described as
> > +	 * "255= Do single set of transfers when scoverride set"
> > +	 * but there's no scoverride bit in the tvs register region.
> > +	 * Ban using 255 since its behaviour is suspect.
> > +	 */
> > +	if (temp > 254)
> > +		return -EINVAL;
> 
> Hardware monitoring drivers should use clamp() and not return -EINVAL
> for ranges such as this. Since the valid range (in ms) is 0..8, I would
> suggest to clamp val to (0, 8) before any calculations to also avoid

Sure, I'll do that.

> the overflow issue mentioned above. That makes me wonder: What does "0"
> stand for ? 32 us or 0 us ? It does not make a difference here, but it
> may be relevant when microsecond intervals are implemented.

I think I answered this above, but 0 means 0 us between the end of a
measurement/conversion and the start of the next one. 

> > +
> > +	temp <<= MPFS_TVS_INTERVAL_OFFSET;
> > +	regmap_update_bits(data->regmap, MPFS_TVS_CTRL,
> > +			   MPFS_TVS_INTERVAL_MASK, temp);
> 
> If regmap never returns errors this needs to be documented in the driver.

It's an mmio regmap via a syscon, it evaluates to readl()/writel() so
there's nothing that can fail /and/ return an error.
I mean, I can add if (ret) return ret, there's not a clean place to put
a comment about it I don't think.


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