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

[Guenter Roeck]

On 8/22/25 20:33, James Calligeros wrote:
> 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

Odd (and wrong) answer. What do pointers have to do with 64-bit math ? Nothing.
val is a 64-bit variable, so "val /= -scale" is a 64-bit math operation.

> 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.

Right answer.

> > > +
> > > +	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.

dev_err -> it is an error. Don't ignore it. If some of the properties are
optional, they should be defined as such in the devicetree description,
there should be neither an error nor a warning message. Plus, the above
should be explained in a comment so future developers do not wonder and
don't add error handling.

Guenter