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).
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?
> > 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
