On Tue, Nov 16, 2021 at 10:54:36AM +0100, Hans de Goede wrote:
> On 11/8/21 15:12, Andy Shevchenko wrote:
> > On Mon, Nov 08, 2021 at 02:12:38PM +0100, Hans de Goede wrote:
> >> On 11/2/21 00:43, Daniel Scally wrote:
...
> >> +int cio2_bridge_sensors_are_ready(void)
> >> +{
> >> + struct acpi_device *adev;
> >
> >> + bool ready = true;
> >
> > Redundant. See below.
> >
> >> + unsigned int i;
> >> +
> >> + for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
> >> + const struct cio2_sensor_config *cfg =
> >> + &cio2_supported_sensors[i];
> >> +
> >> + for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> >> + if (!adev->status.enabled)
> >> + continue;
> >
> >> + if (!acpi_dev_ready_for_enumeration(adev))
> >> + ready = false;
> >
> > You may put the adev here and return false.
> >
> >> + }
> >> + }
> >
> >> + return ready;
> >
> > So return true.
>
> I actually did it this way deliberately making use of
> for_each_acpi_dev_match() not "leaking" a ref when you let
> it run to the end.
>
> I find this clearer because this way all the ref handling
> is abstracted away in for_each_acpi_dev_match(), where as with
> a put in the middle of the loop a causal reader of the code
> is going to wonder there the put ref is coming from.
Here are pros and cons, currently you abstracted it away, but in case of
extending this piece of code (I don't believe it will happen, though) it may
play a trick if one forgets about this nuance of for_each_acpi_dev_match().
But it's fine for me, so you decide.
> >> +}
--
With Best Regards,
Andy Shevchenko
