Re: [PATCH v3] hisi_lpc: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Tue, Jul 5, 2022 at 12:16 PM John Garry <john.garry@xxxxxxxxxx> wrote:
>
> On 05/07/2022 10:39, Andy Shevchenko wrote:
> > On Tue, Jul 5, 2022 at 11:38 AM Andy Shevchenko
> > <andy.shevchenko@xxxxxxxxx>  wrote:
> >> On Tue, Jul 5, 2022 at 10:37 AM John Garry<john.garry@xxxxxxxxxx>  wrote:
> >>> On 04/07/2022 20:02, Rafael J. Wysocki wrote:
> >> ...
> >>
> >>> I gave these a quick test on my board and they look fine.
> >>>
> >>> Acked-by: John Garry<john.garry@xxxxxxxxxx>
> >> John, I believe now you may send a formal clean up to convert to platform_device
> > Hit Enter too early:-)
> >
> > ...to platform_device_register_full().
>
> Sure, I can look at that now. But I just found where we previously
> mentioned the possibility of factoring out some of the ACPI platform
> device creation code:
>
> https://lore.kernel.org/linux-acpi/CAHp75VfOa5pN4MKT-aQmWBwPGWsOaQupyfrN-weWwfR3vMLtuA@xxxxxxxxxxxxxx/
>
> There is actually still a comment in the hisi_lpc driver - I should have
> checked there first :)
>
> So my impression is that the hisi_lpc code is almost the same in
> acpi_create_platform_device(), apart from we need do the resource fixup
> in hisi_lpc_acpi_set_io_res().
>
> So we could factor out by dividing acpi_create_platform_device() into 2x
> parts: resource get and then platform dev create. But that does not seem
> wise as we have 2x parts which don't make sense on their own. Or else
> pass a fixup callback into acpi_create_platform_device(). Any other
> ideas if we want to go this way?

Personally, I would do the cleanup that can be done without
refactoring the library function as the first step, just to reduce the
amount of changes made in one go if nothing else.

Next, I'd look at introducing something like

acpi_create_platform_device_ops(struct acpi_device *adev, const struct
property_entry *properties, const struct *platform_device_create_ops
*ops);

where ops would be a set of callbacks to invoke as a matter of customization.

Then, acpi_create_platform_device() can be defined as a wrapper around
the above.