Re: [PATCH] hwmon: (dell-smm-hwmon) Fix index values

From: Armin Wolf <W_Armin@gmx.de>
To: "Guenter Roeck" <linux@roeck-us.net>, "Pali Rohár" <pali@kernel.org>
Cc: linux-hwmon@vger.kernel.org, jdelvare@suse.com
Subject: Re: [PATCH] hwmon: (dell-smm-hwmon) Fix index values
Date: Thu, 13 May 2021 20:41:06 +0200	[thread overview]
Message-ID: <60d82aa4-3f44-d10d-8db5-3d6a30f25945@gmx.de> (raw)
In-Reply-To: <e333b7e2-4bb7-1be2-5d26-4b60e6463d7c@roeck-us.net>

On 13.05.21 18:53 Guenter Roeck wrote:
> On 5/13/21 9:48 AM, Pali Rohár wrote:
>> On Thursday 13 May 2021 17:45:46 W_Armin@gmx.de wrote:
>>> From: Armin Wolf <W_Armin@gmx.de>
>>>
>>> When support for up to 10 temp sensors and for disabling automatic BIOS
>>> fan control was added, noone updated the index values used for
>>> disallowing fan support and fan type calls.
>>> Fix those values.
>>
>> Do you mean this change, right?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bb46a20e73b0bb3364cff3839c9f716ed327770
>>
>>
>> Yes, it looks like that it should have been part of that change.
>>
>> Therefore I suggest to add Fixes tag:
>>
>> Fixes: 1bb46a20e73b ("hwmon: (dell-smm) Support up to 10 temp sensors")
>>
>> Otherwise looks good!
>>
>> Reviewed-by: Pali Rohár <pali@kernel.org>
>>
>> For future development I would suggest to rewrite/drop these magic
>> numbers as same problem can be easily repeated in future.
>>
>
> The best solution would be to rewrite the driver to use
> hwmon_device_register_with_info(), but that should be done
> by someone with access to the hardware.
>
> Guenter
Im currently doing exactly that, since i have an old dell notebook. But
that might take some time.
>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>> Tested on a Dell Latitude C600.
>>> ---
>>>   drivers/hwmon/dell-smm-hwmon.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c
>>> b/drivers/hwmon/dell-smm-hwmon.c
>>> index 2970892bed82..f2221ca0aa7b 100644
>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>> @@ -838,10 +838,10 @@ static struct attribute *i8k_attrs[] = {
>>>   static umode_t i8k_is_visible(struct kobject *kobj, struct
>>> attribute *attr,
>>>                     int index)
>>>   {
>>> -    if (disallow_fan_support && index >= 8)
>>> +    if (disallow_fan_support && index >= 20)
>>>           return 0;
>>>       if (disallow_fan_type_call &&
>>> -        (index == 9 || index == 12 || index == 15))
>>> +        (index == 21 || index == 25 || index == 28))
>>>           return 0;
>>>       if (index >= 0 && index <= 1 &&
>>>           !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
>>> --
>>> 2.20.1
>>>
>