Re: [PATCH] hwmon: Add T2 Mac fan control support in applesmc driver

[Guenter Roeck <linux@roeck-us.net>]

On 1/11/25 05:32, Atharva Tiwari wrote:
> This patch adds support for fan control on T2 Macs in the applesmc driver.
> It introduces functions to handle floating-point fan speed values.
> The fan speed reading and writing are updated to
> 

drop empty line

> support both integer and floating-point values.
> The fan manual control is also updated to handle T2 Mac-specific keys.
> 
> Changes:
>   - Added support for floating-point fan speeds.
> 
>   - Updated fan speed functions to
>     handle both integer and floating-point values.
> 
>   - Modified fan control to support T2 Mac-specific keys.
> 
The "Changes" part duplicates the description.

> Signed-off-by: Atharva Tiwari <evepolonium@gmail.com>
> Co-developed-by: Aun-Ali Zaidi <admin@kodeit.net>
> Signed-off-by: Aun-Ali Zaidi <admin@kodeit.net>

How does this version differ from the previous version ? Or is it a resend ?
If it is a resend, why did you not tag it as resend ?

On a high and immediate level, the patch reports checkpatch issues,
and the subject line does not match subsystem expectations. On top of that,
it is complicated and difficult to review. All of that causes it to end up
at the end of my review queue.

Some comments below. This is not a complete review.

Guenter

> ---
>   drivers/hwmon/applesmc.c | 105 ++++++++++++++++++++++++++++++---------
>   1 file changed, 82 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index fc6d6a9053ce..9ce7c426a2f6 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -510,7 +510,28 @@ static int applesmc_read_s16(const char *key, s16 *value)
>   	*value = ((s16)buffer[0] << 8) | buffer[1];
>   	return 0;
>   }

blank line here

> +static inline u32 applesmc_float_to_u32(u32 d)
> +{
> +	u8 sign = (d >> 31) & 1;
> +	s32 exp = ((d >> 23) & 0xff) - 0x7F;
> +	u32 fr = d & ((1u << 23) - 1);
> +
> +	if (sign || exp < 0)
> +		return 0;
>   
> +	return (1u << exp) + (fr >> (23 - exp));
> +}

blank line here

> +static inline u32 applesmc_u32_to_float(u32 d) > +{
> +	u32 dc = d, bc = 0, exp;

blank line here

> +	if (!d)
> +		return 0;
> +	while (dc >>= 1)
> +		++bc;
> +	exp = 0x7F + bc;
> +	return (exp << 23) |
> +		 ((d << (23 - (exp - 0x7F))) & ((1u << 23) - 1));
> +}

blank line here

>   /*
>    * applesmc_device_init - initialize the accelerometer.  Can sleep.
>    */
> @@ -842,15 +863,23 @@ static ssize_t applesmc_show_fan_speed(struct device *dev,
>   	unsigned int speed = 0;
>   	char newkey[5];
>   	u8 buffer[2];
> -
> +	const struct applesmc_entry *entry;

blank line here

>   	scnprintf(newkey, sizeof(newkey), fan_speed_fmt[to_option(attr)],
>   		  to_index(attr));
>   
> -	ret = applesmc_read_key(newkey, buffer, 2);
> +	entry = applesmc_get_entry_by_key(newkey);
> +	if (IS_ERR(entry))
> +		return PTR_ERR(entry);
> +
> +	if (!strcmp(entry->type, "flt")) {
> +		ret = applesmc_read_entry(entry, (u8 *)buffer, 4);
> +		speed = applesmc_float_to_u32(*(u32 *)buffer);
> +	} else {
> +		ret = applesmc_read_entry(entry, (u8 *)buffer, 2);
> +		speed = ((buffer[0] << 8 | buffer[1]) >> 2);
> +	}
>   	if (ret)
>   		return ret;
> -
> -	speed = ((buffer[0] << 8 | buffer[1]) >> 2);
>   	return sysfs_emit(sysfsbuf, "%u\n", speed);
>   }
>   
> @@ -861,7 +890,8 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
>   	int ret;
>   	unsigned long speed;
>   	char newkey[5];
> -	u8 buffer[2];
> +	u8 buffer[4];
> +	const struct applesmc_entry *entry;
>   
>   	if (kstrtoul(sysfsbuf, 10, &speed) < 0 || speed >= 0x4000)
>   		return -EINVAL;		/* Bigger than a 14-bit value */
> @@ -869,9 +899,18 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
>   	scnprintf(newkey, sizeof(newkey), fan_speed_fmt[to_option(attr)],
>   		  to_index(attr));
>   
> -	buffer[0] = (speed >> 6) & 0xff;
> -	buffer[1] = (speed << 2) & 0xff;
> -	ret = applesmc_write_key(newkey, buffer, 2);
> +	entry = applesmc_get_entry_by_key(newkey);
> +	if (IS_ERR(entry))
> +		return PTR_ERR(entry);
> +
> +	if (!strcmp(entry->type, "flt")) {
> +		*(u32 *)buffer = applesmc_u32_to_float(speed);
> +		ret = applesmc_write_entry(entry, (u8 *)buffer, 4);
> +	} else {
> +		buffer[0] = (speed >> 6) & 0xff;
> +		buffer[1] = (speed << 2) & 0xff;
> +		ret = applesmc_write_key(newkey, (u8 *)buffer, 2);

Why use applesmc_write_key here ?

> +	}
>   
>   	if (ret)
>   		return ret;
> @@ -885,12 +924,23 @@ static ssize_t applesmc_show_fan_manual(struct device *dev,
>   	int ret;
>   	u16 manual = 0;
>   	u8 buffer[2];
> +	char newkey[5];
> +	bool has_newkey = false;
> +
> +	scnprintf(newkey, sizeof(newkey), "F%dMd", to_index(attr));
> +
> +	ret = applesmc_has_key(newkey, &has_newkey);
> +	if (has_newkey) {
> +		ret = applesmc_read_key(newkey, (u8 *)buffer, 1);

buffer is already a u8 array. Why the type cast ?

> +		manual = buffer[0];
> +	} else {
> +		ret = applesmc_read_key(FANS_MANUAL, (u8 *)buffer, 2);
> +		manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
> +	}
>   
> -	ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
>   	if (ret)
>   		return ret;
>   
> -	manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
>   	return sysfs_emit(sysfsbuf, "%d\n", manual);
>   }
>   
> @@ -900,28 +950,37 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
>   {
>   	int ret;
>   	u8 buffer[2];
> +	char newkey[5];
> +	bool has_newkey = false;
>   	unsigned long input;
>   	u16 val;
>   
>   	if (kstrtoul(sysfsbuf, 10, &input) < 0)
>   		return -EINVAL;
>   
> -	ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> -	if (ret)
> -		goto out;
> -
> -	val = (buffer[0] << 8 | buffer[1]);
> +	scnprintf(newkey, sizeof(newkey), "F%dMd", to_index(attr));
>   
> -	if (input)
> -		val = val | (0x01 << to_index(attr));
> -	else
> -		val = val & ~(0x01 << to_index(attr));
> -
> -	buffer[0] = (val >> 8) & 0xFF;
> -	buffer[1] = val & 0xFF;
> +	ret = applesmc_has_key(newkey, &has_newkey);
> +	if (ret)
> +		return ret;
> +	if (has_newkey) {
> +		buffer[0] = input & 1;
> +		ret = applesmc_write_key(newkey, (u8 *)buffer, 1);
> +	} else {
> +		ret = applesmc_read_key(FANS_MANUAL, (u8 *)buffer, 2);
> +		val = (buffer[0] << 8 | buffer[1]);
> +		if (ret)
> +			goto out;
> +		if (input)
> +			val = val | (0x01 << to_index(attr));
> +		else
> +			val = val & ~(0x01 << to_index(attr));
>   
> -	ret = applesmc_write_key(FANS_MANUAL, buffer, 2);
> +		buffer[0] = (val >> 8) & 0xFF;
> +		buffer[1] = val & 0xFF;
>   
> +		ret = applesmc_write_key(FANS_MANUAL, buffer, 2);
> +	}
>   out:
>   	if (ret)
>   		return ret;