[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