Re: [PATCH 4/8] hwmon: Add Apple Silicon SMC hwmon driver

[James Calligeros <jcalligeros99@gmail.com>]

Hi Guenter,

On Wednesday, 20 August 2025 2:02:58 am Australian Eastern Standard Time Guenter Roeck wrote:
> On 8/19/25 04:47, James Calligeros wrote:
> > +/*
> > + * Many sensors report their data as IEEE-754 floats. No other SMC
> > function uses + * them.
> > + */
> > +static int macsmc_hwmon_read_f32_scaled(struct apple_smc *smc, smc_key
> > key, +					int *p, int scale)
> > +{
> > +	u32 fval;
> > +	u64 val;
> > +	int ret, exp;
> > +
> > +	ret = apple_smc_read_u32(smc, key, &fval);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	val = ((u64)((fval & FLT_MANT_MASK) | BIT(23)));
> > +	exp = ((fval >> 23) & 0xff) - FLT_EXP_BIAS - FLT_MANT_BIAS;
> > +	if (scale < 0) {
> > +		val <<= 32;
> > +		exp -= 32;
> > +		val /= -scale;
> 
> I am quiter sure that this doesn't compile on 32 bit builds.
> 
I don't see why not. We're not doing any 64-bit math on pointers, so we should
be safe here. Regardless, this driver depends on MFD_MACSMC, which depends on
ARCH_APPLE, which is an ARM64 platform, so this driver shouldn't be compiled
during a 32-bit build anyway.


> > +
> > +	ret = of_property_read_string(fan_node, "apple,key-id", &now);
> > +	if (ret) {
> > +		dev_err(dev, "apple,key-id not found in fan node!");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = macsmc_hwmon_parse_key(dev, smc, &fan->now, now);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!of_property_read_string(fan_node, "label", &label))
> > +		strscpy_pad(fan->label, label, sizeof(fan->label));
> > +	else
> > +		strscpy_pad(fan->label, now, sizeof(fan->label));
> > +
> > +	fan->attrs = HWMON_F_LABEL | HWMON_F_INPUT;
> > +
> > +	ret = of_property_read_string(fan_node, "apple,fan-minimum", &min);
> > +	if (ret)
> > +		dev_warn(dev, "No minimum fan speed key for %s", fan->label);
> > +	else {
> > +		if (!macsmc_hwmon_parse_key(dev, smc, &fan->min, min))
> > +			fan->attrs |= HWMON_F_MIN;
> 
> Above the error from macsmc_hwmon_parse_key() results in an abort,
> here the error is logged in the function and ignored.
> 
> Either it is an error or it isn't. Ignoring errors is not acceptable.
> Dumping error messages and ignoring the error is even less acceptable.
> 
The only strictly required key for fan speed monitoring is apple,key-id,
which is why it is the only one that causes an early return when parsing
it fails. If we don't have keys in the DT for min, max, target and mode,
then all that means is we can't enable manual fan speed control. I don't
see how making a failure to read these keys non-blocking is unacceptable
in this context. If this is about the dev_err print in parse_key, then
I can just get rid of that and have the parse_key callers do it when it's
actually a blocking error.

Regards,
James