Re: [PATCH v2 4/9] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Jean Delvare <jdelvare@suse.de>]

On Thu, 12 Aug 2021 12:55:20 +0300, Andy Shevchenko wrote:
> On Wed, Aug 11, 2021 at 10:28:25PM +0200, Heiner Kallweit wrote:
> > On 11.08.2021 17:45, Andy Shevchenko wrote:  
> > > On Fri, Aug 06, 2021 at 11:15:15PM +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.  
> > > 
> > > ...
> > >   
> > >> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > >> -			 (void **)&found);
> > >> +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
> > >>  
> > >> -	return found;
> > >> +	return !IS_ERR(err);  
> > > 
> > > Shouldn't you also check the status of acpi_get_device()?
> >
> > This shouldn't be needed because err isn't touched if function fails.  
> 
> For the sake of clearness of the code I would do it.

This brings us back to how awkward the API is. Most callers don't
bother checking the return value of acpi_get_devices() because it's
useless in practice. But I agree that in theory it could return with an
error and then it would be nicer to catch that.

> (...) But in any case what
> really hurt my eye is the last line here. To me sounds like
> 
> 	if (IS_ERR(err))
> 		return false;
> 	return true;
> 
> is much better to read (and I bet the compiler will generate the very same
> code for it).

Somehow the assembly code differs, but I'm unable to see the relation
between your proposed change and the assembly code changes. That's why
I hate modern compilers. They pretend to be smart, but what they are
essentially is unstable, and this ruins any attempt at such trivial
comparisons. Sad.

Personally I don't really care, Heiner's code did not strike me as
being hard to read in the first place. I tend to avoid conditionals
when possible.

-- 
Jean Delvare
SUSE L3 Support