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:

quoted

On 11.08.2021 17:45, Andy Shevchenko wrote:  

quoted

On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote:  

quoted

Replace the ugly cast of the return_value pointer with proper usage.
In addition use dmi_match() instead of open-coding it.  

...
  

quoted

-	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