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

[John Garry]

On 05/07/2022 14:54, Rafael J. Wysocki wrote:
> > 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.
> .
JFYI, I'm trying out this change and it is looking quite disruptive. The 
problems are specifically related to the LPC UART support. Firstly, it 
looks like we require this patch (which was never applied):
https://lore.kernel.org/linux-acpi/1524218846-169934-2-git-send-email-john.garry@xxxxxxxxxx/#t

Secondly this code in the hisi lpc driver causes an issue:

static int hisi_lpc_acpi_add_child(struct acpi_device *child, void *data)
{
	const char *hid = acpi_device_hid(child);
	struct device *hostdev = data;
	const struct hisi_lpc_acpi_cell *cell;
	struct platform_device *pdev;
	const struct resource *res;
	bool found = false;
	int num_res;
	int ret;

	ret = hisi_lpc_acpi_set_io_res(child, hostdev, &res, &num_res);
	if (ret) {
		dev_warn(hostdev, "set resource fail (%d)\n", ret);
		return ret;
	}

	cell = (struct hisi_lpc_acpi_cell []){
...
		/* 8250-compatible uart */
		{
			.hid = "HISI1031",
			.name = "serial8250",
			.pdata = (struct plat_serial8250_port []) {
				{
***					.iobase = res->start,
					.uartclk = 1843200,
					.iotype = UPIO_PORT,
					.flags = UPF_BOOT_AUTOCONF,
				},
				{}
			},
			.pdata_size = 2 *
				sizeof(struct plat_serial8250_port),
		},
		{}
	};
...

	pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);

At ***, above, we need to set the platform data plat_serial8250_port 
iobase at the translated address, but this can only be done after we 
read and translate the resources, which is now all done in the acpi 
platform code - so we have an ordering problem.

Anyway, I'll try to get it working and then send out the patches. We may 
decide it's just not worth it.

Thanks,
John