On Thu, Jun 22, 2023 at 05:53:13PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 21, 2023 at 5:16 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> >
> > After switching i2c-scmi driver to be a plaform one, it stopped
>
> "platform"
>
> > being enumerated on number of Kontron platforms, because it's
> > listed in the forbidden_id_list.
> >
> > To resolve the situation, split the list to generic one and
> > another that holds devices that has to be skipped if and only
>
> "have"
>
> > if they have bogus resources attached (_CRS method returns some).
Thanks for the typo fixes!
...
> > +static const struct acpi_device_id forbidden_id_with_resourses[] = {
>
> I don't quite like this name and the driver_data field could be used
> to indicate the need to check the resources.
Okay, something like
/* Check if the device has resources provided by _CRS method */
#define ACPI_PLATFORM_CHECK_RES BIT(0)
?
> > + {"SMB0001", 0}, /* ACPI SMBUS virtual device */
> > + { }
> > +};
...
> > +static int acpi_platform_resource_count(struct acpi_resource *ares, void *data)
> > +{
> > + int *count = data;
> > +
> > + *count = *count + 1;
>
> Why not (*count)++?
Can be that way, I just copied'n'pasted from the existing code.
> > + return 1;
> > +}
...
> > INIT_LIST_HEAD(&resource_list);
> > + ret = acpi_dev_get_resources(adev, &resource_list, acpi_platform_resource_count, &count);
> > + if (ret < 0)
> > + return ERR_PTR(ret);
>
> Why not use acpi_walk_resources() directly here?
Can be done that way. Again, I just used a template (existing code in kernel)
for similar functionality.
> Also, this extra resources walk is only needed if the resources check
> is needed to decide whether or not to skip the device, so what's the
> benefit of doing it for every device that's not skipped?
Ah, indeed. Makes sense to have done it conditionally.
> > + acpi_dev_free_resource_list(&resource_list);
...
Thank you for the review!
--
With Best Regards,
Andy Shevchenko
