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@xxxxxxxxx>
>>> ---
>>> 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@xxxxxxx>
>>
>> --
>> Jean Delvare
>> SUSE L3 Support
