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