Re: [PATCH 05/10] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Heiner Kallweit <hkallweit1@gmail.com>]

On 05.08.2021 21:11, Pali Rohár wrote:
> Hello!
> 
> On Thursday 05 August 2021 11:51:56 Jean Delvare wrote:
>> Hi Heiner,
>>
>> On Sun, 01 Aug 2021 16:20:19 +0200, Heiner Kallweit wrote:
>>> Replace the ugly cast of the return_value pointer with proper usage.
>>> In addition use dmi_match() instead of open-coding it.
>>
>> Pali, would you be able to test this patch?
> 
> Tested now on Latitude E6440 and patch is working fine (no difference).
> 
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/i2c/busses/i2c-i801.c | 13 ++++---------
>>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>> index d971ee20c..a6287c520 100644
>>> --- a/drivers/i2c/busses/i2c-i801.c
>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>> @@ -1191,7 +1191,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
>>>  
>>>  	kfree(info);
>>>  
>>> -	*((bool *)return_value) = true;
>>> +	*return_value = obj_handle;
> 
> You are missing a cast here. "obj_handle" is of unknown typedef type
> acpi_handle and *return_value is of type void*. So this can generate a
> compile warning (either now or in future).
> 

acpi_handle is defined as:  typedef void *acpi_handle;
Therefore compiler is happy (as long as acpi_handle is any pointer type).

> So you need to write it something like this:
> 
>   *((acpi_handle *)return_value) = obj_handle;
> 
> But what is benefit of this change? Is not usage of explicit true and
> false values better than some acpi_handle type of undefined value stored
> in obj_handle?
> 
>From a logical perspective I agree. My motivation is that I see explicit
casts as a last resort and try to avoid them as far as possible.
The current code abuses a void* variable to store a bool. This makes the
implicit assumption that a pointer variable is always big enough to
store a bool.

With regard to "acpi_handle of undefined value": I'm just interested
in the information whether handle is NULL or not. That's the normal
implicit cast to bool like in every if(pointer) clause. 

>>>  	return AE_CTRL_TERMINATE;
>>>  
>>>  smo88xx_not_found:
>>> @@ -1201,13 +1201,10 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
>>>  
>>>  static bool is_dell_system_with_lis3lv02d(void)
>>>  {
>>> -	bool found;
>>> -	const char *vendor;
>>> +	acpi_handle found = NULL;
> 
> Anyway, it looks strange to use name "found" for object handle
> variable. I would expect that something named "found" is storing
> something which refers to 2-state logic and not some handle value.
> 
>>>  
>>> -	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
>>> -	if (!vendor || strcmp(vendor, "Dell Inc."))
>>> +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
>>>  		return false;
>>
>> Looks good to me.
>>
>>> -
>>
>> I see no reason to remove that blank line.
>>
>>>  	/*
>>>  	 * Check that ACPI device SMO88xx is present and is functioning.
>>>  	 * Function acpi_get_devices() already filters all ACPI devices
>>> @@ -1216,9 +1213,7 @@ static bool is_dell_system_with_lis3lv02d(void)
>>>  	 * accelerometer but unfortunately ACPI does not provide any other
>>>  	 * information (like I2C address).
>>>  	 */
>>> -	found = false;
>>> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
>>> -			 (void **)&found);
> 
> Just to explain my motivation: I originally assigned found to false
> value immediately before acpi_get_devices() function call to show that
> this is the place where variable is used due to to API of that function.
> 
>>> +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &found);
>>
>> The choice of return value by the acpi_get_devices() designer is very
>> unfortunate. It would have been much more convenient if the return
>> value was different whether a match has been found or not. Oh well.
> 
> I agree, it is very _original_ way...
> 
>> I agree that the proposed change is a nicer way to work around this
>> limitation. Unfortunately I can't test this as I do not own a Dell
>> laptop. Were you able to test it? If not, I hope Pali will.
>>
>>>  
>>>  	return found;
>>>  }
>>
>> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>>
>> -- 
>> Jean Delvare
>> SUSE L3 Support