[PATCH PoC 0/3] ACPI/PNP/HISI_LPC: Refactor ACPI platform code for reuse

[PATCH PoC 0/3] ACPI/PNP/HISI_LPC: Refactor ACPI platform code for reuse

[John Garry]

This series attempts to refactor the ACPI platform device creation code
such that it may reused in the HiSilicon LPC driver. The topic was
recently discussed at the following:
https://lore.kernel.org/lkml/CAJZ5v0hkiOqhWyOcd2vjj-tcyHWB-cK=Dae-3sifhRfcbKWWGg@mail.gmail.com/

So first off I'll say that I am not advocating pursing the changes in this
series, but just wanted to share to show others how it would look.

Apart from requiring PNP driver changes, the API in
acpi_create_platform_device_ops() ain't pretty. And also the PNP change
means that the LPC uart changes from ttyS1 -> ttyS0, which would be a
nuisance for anyone still using a board which uses the LPC UART.

I have a separate change to show how the HiSilicon LPC driver would look
by using platform_device_register_full() instead, and it is a lot neater:
https://github.com/hisilicon/kernel-dev/commits/private-topic-lpc-6.0-platform-full

Anyway, the commit messages should describe anything else needed to be
known.

Please share any comments, thanks.

John Garry (3):
  ACPI / PNP: Don't add enumeration_by_parent devices
  ACPI: platform: Refactor acpi_create_platform_device()
  bus: hisi_lpc: Use acpi_create_platform_device_ops()

 drivers/acpi/acpi_platform.c |  37 ++++++--
 drivers/bus/hisi_lpc.c       | 165 +++++++++++++++--------------------
 drivers/pnp/pnpacpi/core.c   |  13 ++-
 include/linux/acpi.h         |  17 +++-
 4 files changed, 122 insertions(+), 110 deletions(-)

-- 
2.35.3

[PATCH PoC 1/3] ACPI / PNP: Don't add enumeration_by_parent devices

[John Garry]

For ACPI devices with the enumeration_by_parent flag set, we expect the
parent device to enumerate the device after the ACPI scan.

This patch does partially the same for devices which are enumerated as PNP
devices.

We still want PNP scan code to create the per-ACPI device PNP device, but
hold off adding the device to allow the parent to do this optionally.

Flag acpi_device.driver_data is used as temp store as a reference to the
PNP device for the parent.

A note on impact of this change:

For the  hisi_lpc driver, for the UART ACPI node we have a binding like:

 Device (LPC0.CON0) {
    Name (_HID, "HISI1031")
    Name (_CID, "PNP0501")
    Name (LORS, ResourceTemplate() {
      QWordIO (

We have the compat and hid string. The ACPI/PNP code matches the compat
string first, and creates the PNP device. In doing so, the acpi_device
created has physical_node_count member set in acpi_bind_one().

The hisi_lpc driver also creates a platform device serial device for uart,
which is the actual uart which we want to use - see
hisi_lpc_acpi_add_child(). That function does not check
physical_node_count value, but acpi_create_platform_device() does check it.
So if we were to move hisi_lpc_acpi_add_child() across to use
acpi_create_platform_device(), then the change in this patch is required to
not create the PNP binding (so that physical_node_count is not set from
PNP probe).

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/pnp/pnpacpi/core.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 38928ff7472b..b751381b0429 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -272,10 +272,15 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	if (!dev->active)
 		pnp_init_resources(dev);
 
-	error = pnp_add_device(dev);
-	if (error) {
-		put_device(&dev->dev);
-		return error;
+
+	if (device->flags.enumeration_by_parent) {
+		device->driver_data = dev;
+	} else {
+		error = pnp_add_device(dev);
+		if (error) {
+			put_device(&dev->dev);
+			return error;
+		}
 	}
 
 	num++;
-- 
2.35.3

[PATCH PoC 2/3] ACPI: platform: Refactor acpi_create_platform_device()

[John Garry]

There is commonality between acpi_create_platform_device() and
hisi_lpc_acpi_add_child(), in that it covers 2x main steps:
- Read resources for the acpi_device
- Create platform device

Refactor acpi_create_platform_device() so that it may be reused by
hisi_lpc_acpi_add_child() to reduce duplication.

The following extended support is added:
- Allow custom platform device name be set
- Support platform data
- Support custom platform device id
- Support translating resources for Indirect PIO

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/acpi/acpi_platform.c | 37 ++++++++++++++++++++++++++++--------
 include/linux/acpi.h         | 17 +++++++++++++++--
 2 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index de3cbf152dee..f153372a0b11 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -84,9 +84,14 @@ static void acpi_platform_fill_resource(struct acpi_device *adev,
 }
 
 /**
- * acpi_create_platform_device - Create platform device for ACPI device node
+ * acpi_create_platform_device_ops - Create platform device for ACPI device node
  * @adev: ACPI device node to create a platform device for.
+ * @name: Name for platform device, may be unset
  * @properties: Optional collection of build-in properties.
+ * @data: platform data pointer
+ * @size_data: platform data size
+ * @xlat: callback translate function for resources
+ * @id: platform device id
  *
  * Check if the given @adev can be represented as a platform device and, if
  * that's the case, create and register a platform device, populate its common
@@ -94,8 +99,15 @@ static void acpi_platform_fill_resource(struct acpi_device *adev,
  *
  * Name of the platform device will be the same as @adev's.
  */
-struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
-						    const struct property_entry *properties)
+struct platform_device *acpi_create_platform_device_ops(
+					struct acpi_device *adev,
+					const char *name,
+					const struct property_entry *properties,
+					void *data, size_t size_data,
+					int (*xlat)(struct acpi_device *adev,
+						    struct resource *res,
+						    void *data, size_t size_data),
+					int id)
 {
 	struct platform_device *pdev = NULL;
 	struct platform_device_info pdevinfo;
@@ -124,9 +136,13 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
 			return ERR_PTR(-ENOMEM);
 		}
 		count = 0;
-		list_for_each_entry(rentry, &resource_list, node)
+		list_for_each_entry(rentry, &resource_list, node) {
 			acpi_platform_fill_resource(adev, rentry->res,
-						    &resources[count++]);
+						    &resources[count]);
+			if (xlat)
+				xlat(adev, &resources[count], data, size_data);
+			count++;
+		}
 
 		acpi_dev_free_resource_list(&resource_list);
 	}
@@ -139,12 +155,17 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
 	 */
 	pdevinfo.parent = adev->parent ?
 		acpi_get_first_physical_node(adev->parent) : NULL;
-	pdevinfo.name = dev_name(&adev->dev);
-	pdevinfo.id = -1;
+	if (name)
+		pdevinfo.name = name;
+	else
+		pdevinfo.name = dev_name(&adev->dev);
+	pdevinfo.id = id;
 	pdevinfo.res = resources;
 	pdevinfo.num_res = count;
 	pdevinfo.fwnode = acpi_fwnode_handle(adev);
 	pdevinfo.properties = properties;
+	pdevinfo.data = data;
+	pdevinfo.size_data = size_data;
 
 	if (acpi_dma_supported(adev))
 		pdevinfo.dma_mask = DMA_BIT_MASK(32);
@@ -165,7 +186,7 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
 
 	return pdev;
 }
-EXPORT_SYMBOL_GPL(acpi_create_platform_device);
+EXPORT_SYMBOL_GPL(acpi_create_platform_device_ops);
 
 void __init acpi_platform_init(void)
 {
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6f64b2f3dc54..d657731086fd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -28,6 +28,7 @@
 #include <linux/dynamic_debug.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/platform_device.h>
 
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -721,8 +722,20 @@ extern bool acpi_driver_match_device(struct device *dev,
 int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
 int acpi_device_modalias(struct device *, char *, int);
 
-struct platform_device *acpi_create_platform_device(struct acpi_device *,
-						    const struct property_entry *);
+struct platform_device *acpi_create_platform_device_ops(
+				struct acpi_device *adev,
+				const char *name,
+				const struct property_entry *properties,
+				void *data, size_t size_data,
+				int (*xlat)(struct acpi_device *adev,
+					    struct resource *res,
+					    void *data, size_t size_data),
+				int id);
+
+#define acpi_create_platform_device(adev, properties) \
+	acpi_create_platform_device_ops(adev, NULL, properties, NULL, 0, \
+					NULL, PLATFORM_DEVID_NONE)
+
 #define ACPI_PTR(_ptr)	(_ptr)
 
 static inline void acpi_device_set_enumerated(struct acpi_device *adev)
-- 
2.35.3

[PATCH PoC 3/3] bus: hisi_lpc: Use acpi_create_platform_device_ops()

[John Garry]

Function acpi_create_platform_device_ops() was added to factor out the
duplicated code to create platform device between ACPI platform device
code and this driver.

Most of the complication added here comes from LPC UART support. To
support the LPC UART, we create a serial platform device. For this we
need to create a structure of plat_serial8250_port data. The specific
complication comes from the fact that the UART iobase in
plat_serial8250_port needs to be fixed up after the resource translation.
As such, we need to provide a pointer to this plat_serial8250_port data
to do the fix up in the xlat callback.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/hisi_lpc.c | 165 +++++++++++++++++------------------------
 1 file changed, 69 insertions(+), 96 deletions(-)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 2e564803e786..ada1cea1affc 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -390,70 +390,49 @@ static void hisi_lpc_acpi_fixup_child_resource(struct device *hostdev,
  * host-relative address resource.  This function will return the translated
  * logical PIO addresses for each child devices resources.
  */
-static int hisi_lpc_acpi_set_io_res(struct acpi_device *adev,
-				    struct device *hostdev,
-				    const struct resource **res, int *num_res)
+struct hisi_lpc_acpi_cell_data;
+
+struct hisi_lpc_acpi_cell {
+	const char *hid;
+	const char *name;
+	struct hisi_lpc_acpi_cell_data *data;
+	size_t data_size;
+};
+
+struct hisi_lpc_acpi_cell_data {
+	const struct hisi_lpc_acpi_cell *owner;
+	union {
+		struct plat_serial8250_port serial[2];
+	};
+};
+
+int hisi_lpc_acpi_xlat_res(struct acpi_device *adev, struct resource *res,
+			   void *data, size_t data_len)
 {
 	struct acpi_device *host = to_acpi_device(adev->dev.parent);
-	struct resource_entry *rentry;
-	LIST_HEAD(resource_list);
-	struct resource *resources;
-	int count;
-	int i;
-
-	if (!adev->status.present) {
-		dev_dbg(&adev->dev, "device is not present\n");
-		return -EIO;
-	}
+	struct device *dev = &adev->dev;
+	struct device *hostdev = dev->parent;
+	int ret;
 
-	if (acpi_device_enumerated(adev)) {
-		dev_dbg(&adev->dev, "has been enumerated\n");
-		return -EIO;
-	}
+	hisi_lpc_acpi_fixup_child_resource(hostdev, res);
 
-	/*
-	 * The following code segment to retrieve the resources is common to
-	 * acpi_create_platform_device(), so consider a common helper function
-	 * in future.
-	 */
-	count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
-	if (count <= 0) {
-		dev_dbg(&adev->dev, "failed to get resources\n");
-		return count ? count : -EIO;
-	}
+	if (!(res->flags & IORESOURCE_IO))
+		return 0;
 
-	resources = devm_kcalloc(hostdev, count, sizeof(*resources),
-				 GFP_KERNEL);
-	if (!resources) {
-		dev_warn(hostdev, "could not allocate memory for %d resources\n",
-			 count);
-		acpi_dev_free_resource_list(&resource_list);
-		return -ENOMEM;
-	}
-	count = 0;
-	list_for_each_entry(rentry, &resource_list, node) {
-		resources[count] = *rentry->res;
-		hisi_lpc_acpi_fixup_child_resource(hostdev, &resources[count]);
-		count++;
-	}
+	ret = hisi_lpc_acpi_xlat_io_res(adev, host, res);
+	if (data_len) {
+		struct hisi_lpc_acpi_cell_data *cell_data = container_of(data,
+				struct hisi_lpc_acpi_cell_data,  serial);
+		const struct hisi_lpc_acpi_cell *cell;
 
-	acpi_dev_free_resource_list(&resource_list);
+		cell = cell_data->owner;
 
-	/* translate the I/O resources */
-	for (i = 0; i < count; i++) {
-		int ret;
+		if (!strcmp(cell->hid, "HISI1031")) {
+			struct plat_serial8250_port *serial = data;
 
-		if (!(resources[i].flags & IORESOURCE_IO))
-			continue;
-		ret = hisi_lpc_acpi_xlat_io_res(adev, host, &resources[i]);
-		if (ret) {
-			dev_err(&adev->dev, "translate IO range %pR failed (%d)\n",
-				&resources[i], ret);
-			return ret;
+			serial->iobase = res->start;
 		}
 	}
-	*res = resources;
-	*num_res = count;
 
 	return 0;
 }
@@ -470,13 +449,6 @@ static int hisi_lpc_acpi_clear_enumerated(struct acpi_device *adev, void *not_us
 	return 0;
 }
 
-struct hisi_lpc_acpi_cell {
-	const char *hid;
-	const char *name;
-	void *pdata;
-	size_t pdata_size;
-};
-
 static void hisi_lpc_acpi_remove(struct device *hostdev)
 {
 	device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev);
@@ -490,15 +462,16 @@ static int hisi_lpc_acpi_add_child(struct acpi_device *child, void *data)
 	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;
+	if (!child->status.present) {
+		dev_err(&child->dev, "device is not present\n");
+		return -EIO;
+	}
+
+	if (acpi_device_enumerated(child)) {
+		dev_err(&child->dev, "has been enumerated\n");
+		return -EIO;
 	}
 
 	cell = (struct hisi_lpc_acpi_cell []){
@@ -511,16 +484,21 @@ static int hisi_lpc_acpi_add_child(struct acpi_device *child, void *data)
 		{
 			.hid = "HISI1031",
 			.name = "serial8250",
-			.pdata = (struct plat_serial8250_port []) {
+			.data = (struct hisi_lpc_acpi_cell_data []) {
 				{
-					.iobase = res->start,
-					.uartclk = 1843200,
-					.iotype = UPIO_PORT,
-					.flags = UPF_BOOT_AUTOCONF,
+					.serial = {
+						{
+						//	.iobase to be set after xlat'ing resources
+							.uartclk = 1843200,
+							.iotype = UPIO_PORT,
+							.flags = UPF_BOOT_AUTOCONF,
+						},
+						{}
+					},
 				},
 				{}
 			},
-			.pdata_size = 2 *
+			.data_size = 2 *
 				sizeof(struct plat_serial8250_port),
 		},
 		{}
@@ -540,31 +518,26 @@ static int hisi_lpc_acpi_add_child(struct acpi_device *child, void *data)
 		return 0;
 	}
 
-	pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);
-	if (!pdev)
-		return -ENOMEM;
-
-	pdev->dev.parent = hostdev;
-	ACPI_COMPANION_SET(&pdev->dev, child);
-
-	ret = platform_device_add_resources(pdev, res, num_res);
-	if (ret)
-		goto fail;
-
-	ret = platform_device_add_data(pdev, cell->pdata, cell->pdata_size);
-	if (ret)
-		goto fail;
+	if (cell->data) {
+		struct hisi_lpc_acpi_cell_data *cell_data = cell->data;
+		struct plat_serial8250_port *data = cell_data->serial;
+
+		cell_data->owner = cell;
+		pdev = acpi_create_platform_device_ops(child, cell->name, NULL,
+						       data, cell->data_size,
+						       hisi_lpc_acpi_xlat_res,
+						       PLATFORM_DEVID_AUTO);
+	} else {
+		pdev = acpi_create_platform_device_ops(child, cell->name, NULL,
+						       NULL, 0,
+						       hisi_lpc_acpi_xlat_res,
+						       PLATFORM_DEVID_AUTO);
+	}
 
-	ret = platform_device_add(pdev);
-	if (ret)
-		goto fail;
+	if (IS_ERR_OR_NULL(pdev))
+		return -ENOMEM;
 
-	acpi_device_set_enumerated(child);
 	return 0;
-
-fail:
-	platform_device_put(pdev);
-	return ret;
 }
 
 /*
-- 
2.35.3

Re: [PATCH PoC 1/3] ACPI / PNP: Don't add enumeration_by_parent devices

[Andy Shevchenko]

On Tue, Aug 16, 2022 at 2:33 PM John Garry <john.garry@huawei.com> wrote:
>
> For ACPI devices with the enumeration_by_parent flag set, we expect the
> parent device to enumerate the device after the ACPI scan.
>
> This patch does partially the same for devices which are enumerated as PNP
> devices.
>
> We still want PNP scan code to create the per-ACPI device PNP device, but
> hold off adding the device to allow the parent to do this optionally.
>
> Flag acpi_device.driver_data is used as temp store as a reference to the
> PNP device for the parent.
>
> A note on impact of this change:
>
> For the  hisi_lpc driver, for the UART ACPI node we have a binding like:
>
>  Device (LPC0.CON0) {
>     Name (_HID, "HISI1031")
>     Name (_CID, "PNP0501")
>     Name (LORS, ResourceTemplate() {
>       QWordIO (
>
> We have the compat and hid string. The ACPI/PNP code matches the compat
> string first, and creates the PNP device. In doing so, the acpi_device
> created has physical_node_count member set in acpi_bind_one().
>
> The hisi_lpc driver also creates a platform device serial device for uart,
> which is the actual uart which we want to use - see
> hisi_lpc_acpi_add_child(). That function does not check
> physical_node_count value, but acpi_create_platform_device() does check it.
> So if we were to move hisi_lpc_acpi_add_child() across to use
> acpi_create_platform_device(), then the change in this patch is required to
> not create the PNP binding (so that physical_node_count is not set from
> PNP probe).

Hmm... The flag, as I interpret it, is equal to "the device in
question is a peripheral device to the non-discoverable bus, such as
SPI, I2C or UART". I.o.w. I do not see how PNP suits here. So, from my
point of view it seems like an abuse of the flag. Not sure the current
state of affairs in ACPI glue layer regarding this, though.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH PoC 2/3] ACPI: platform: Refactor acpi_create_platform_device()

[Andy Shevchenko]

On Tue, Aug 16, 2022 at 2:33 PM John Garry <john.garry@huawei.com> wrote:
>
> There is commonality between acpi_create_platform_device() and
> hisi_lpc_acpi_add_child(), in that it covers 2x main steps:
> - Read resources for the acpi_device
> - Create platform device
>
> Refactor acpi_create_platform_device() so that it may be reused by
> hisi_lpc_acpi_add_child() to reduce duplication.

...

> + * acpi_create_platform_device_ops - Create platform device for ACPI device node

Not sure I understand why _ops is a suffix for the function. I would
expect _ops to be a data struct where the ->xlate() and perhaps other
callbacks may be collected. It may be that I have missed that portion
in the previous discussion.

...

> +       if (name)
> +               pdevinfo.name = name;
> +       else
> +               pdevinfo.name = dev_name(&adev->dev);

> +       pdevinfo.data = data;
> +       pdevinfo.size_data = size_data;

It rather reminds me of platform device registration full with this
device info. May be what you need is
struct acpi_platfrom_device_info {
  properties;
  name;
  id;
  ->xlate();
  ...
};

?

...

> +struct platform_device *acpi_create_platform_device_ops(
> +                               struct acpi_device *adev,
> +                               const char *name,
> +                               const struct property_entry *properties,
> +                               void *data, size_t size_data,
> +                               int (*xlat)(struct acpi_device *adev,
> +                                           struct resource *res,
> +                                           void *data, size_t size_data),
> +                               int id);

...because this looks  a bit too much from the amount of parameters
point of view.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH PoC 1/3] ACPI / PNP: Don't add enumeration_by_parent devices

[John Garry]

On 18/08/2022 20:31, Andy Shevchenko wrote:
>> For the  hisi_lpc driver, for the UART ACPI node we have a binding like:
>>
>>   Device (LPC0.CON0) {
>>      Name (_HID, "HISI1031")
>>      Name (_CID, "PNP0501")
>>      Name (LORS, ResourceTemplate() {
>>        QWordIO (
>>
>> We have the compat and hid string. The ACPI/PNP code matches the compat
>> string first, and creates the PNP device. In doing so, the acpi_device
>> created has physical_node_count member set in acpi_bind_one().
>>
>> The hisi_lpc driver also creates a platform device serial device for uart,
>> which is the actual uart which we want to use - see
>> hisi_lpc_acpi_add_child(). That function does not check
>> physical_node_count value, but acpi_create_platform_device() does check it.
>> So if we were to move hisi_lpc_acpi_add_child() across to use
>> acpi_create_platform_device(), then the change in this patch is required to
>> not create the PNP binding (so that physical_node_count is not set from
>> PNP probe).
> Hmm... The flag, as I interpret it, is equal to "the device in
> question is a peripheral device to the non-discoverable bus, such as
> SPI, I2C or UART". I.o.w. I do not see how PNP suits here. So, from my
> point of view it seems like an abuse of the flag. Not sure the current
> state of affairs in ACPI glue layer regarding this, though.

Hi Andy,

Sorry, but I'm not following you here. Which flag are you talking about?

thanks,
John

Re: [PATCH PoC 2/3] ACPI: platform: Refactor acpi_create_platform_device()

[John Garry]

On 18/08/2022 20:41, Andy Shevchenko wrote:
> On Tue, Aug 16, 2022 at 2:33 PM John Garry <john.garry@huawei.com> wrote:
>>
>> There is commonality between acpi_create_platform_device() and
>> hisi_lpc_acpi_add_child(), in that it covers 2x main steps:
>> - Read resources for the acpi_device
>> - Create platform device
>>
>> Refactor acpi_create_platform_device() so that it may be reused by
>> hisi_lpc_acpi_add_child() to reduce duplication.
> 
> ...
> 
>> + * acpi_create_platform_device_ops - Create platform device for ACPI device node
> 
> Not sure I understand why _ops is a suffix for the function. I would
> expect _ops to be a data struct where the ->xlate() and perhaps other
> callbacks may be collected. It may be that I have missed that portion
> in the previous discussion.

ok, maybe I can put all the members into a struct, but I don't think 
that it improves the overall code too much.

> 
> ...
> 
>> +       if (name)
>> +               pdevinfo.name = name;
>> +       else
>> +               pdevinfo.name = dev_name(&adev->dev);
> 
>> +       pdevinfo.data = data;
>> +       pdevinfo.size_data = size_data;
> 
> It rather reminds me of platform device registration full with this
> device info. May be what you need is
> struct acpi_platfrom_device_info {
>    properties;
>    name;
>    id;
>    ->xlate();
>    ...
> };
> 
> ?
> 
> ...
> 
>> +struct platform_device *acpi_create_platform_device_ops(
>> +                               struct acpi_device *adev,
>> +                               const char *name,
>> +                               const struct property_entry *properties,
>> +                               void *data, size_t size_data,
>> +                               int (*xlat)(struct acpi_device *adev,
>> +                                           struct resource *res,
>> +                                           void *data, size_t size_data),
>> +                               int id);
> 
> ...because this looks  a bit too much from the amount of parameters
> point of view.
> 

ok, agreed.

But even if we improve this code, the hisi_lpc changes are quite large 
and unwieldly.

Thanks,
John

Re: [PATCH PoC 1/3] ACPI / PNP: Don't add enumeration_by_parent devices

[Andy Shevchenko]

On Fri, Aug 19, 2022 at 11:05 AM John Garry <john.garry@huawei.com> wrote:
>
> On 18/08/2022 20:31, Andy Shevchenko wrote:
> >> For the  hisi_lpc driver, for the UART ACPI node we have a binding like:
> >>
> >>   Device (LPC0.CON0) {
> >>      Name (_HID, "HISI1031")
> >>      Name (_CID, "PNP0501")
> >>      Name (LORS, ResourceTemplate() {
> >>        QWordIO (
> >>
> >> We have the compat and hid string. The ACPI/PNP code matches the compat
> >> string first, and creates the PNP device. In doing so, the acpi_device
> >> created has physical_node_count member set in acpi_bind_one().
> >>
> >> The hisi_lpc driver also creates a platform device serial device for uart,
> >> which is the actual uart which we want to use - see
> >> hisi_lpc_acpi_add_child(). That function does not check
> >> physical_node_count value, but acpi_create_platform_device() does check it.
> >> So if we were to move hisi_lpc_acpi_add_child() across to use
> >> acpi_create_platform_device(), then the change in this patch is required to
> >> not create the PNP binding (so that physical_node_count is not set from
> >> PNP probe).
> > Hmm... The flag, as I interpret it, is equal to "the device in
> > question is a peripheral device to the non-discoverable bus, such as
> > SPI, I2C or UART". I.o.w. I do not see how PNP suits here. So, from my
> > point of view it seems like an abuse of the flag. Not sure the current
> > state of affairs in ACPI glue layer regarding this, though.

> Sorry, but I'm not following you here. Which flag are you talking about?

"enumerated by parent".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH PoC 1/3] ACPI / PNP: Don't add enumeration_by_parent devices

[John Garry]

On 19/08/2022 10:59, Andy Shevchenko wrote:
>>>> The hisi_lpc driver also creates a platform device serial device for uart,
>>>> which is the actual uart which we want to use - see
>>>> hisi_lpc_acpi_add_child(). That function does not check
>>>> physical_node_count value, but acpi_create_platform_device() does check it.
>>>> So if we were to move hisi_lpc_acpi_add_child() across to use
>>>> acpi_create_platform_device(), then the change in this patch is required to
>>>> not create the PNP binding (so that physical_node_count is not set from
>>>> PNP probe).
>>> Hmm... The flag, as I interpret it, is equal to "the device in
>>> question is a peripheral device to the non-discoverable bus, such as
>>> SPI, I2C or UART". I.o.w. I do not see how PNP suits here. So, from my
>>> point of view it seems like an abuse of the flag. Not sure the current
>>> state of affairs in ACPI glue layer regarding this, though.
>> Sorry, but I'm not following you here. Which flag are you talking about?
> "enumerated by parent".

ok, right. So I thought that PNP0501 was a standard cid to describe 
16550-compat UART which may be on an LPC bus. And a LPC bus is 
non-discoverable, like SPI or I2C, which also use "enumerated by parent".

Indeed, using PNP0501 for hisi lpc UART cid would be without problem, 
apart from the fact that we can't do all the PIO translation (so need 
"enumerated by parent").

Thanks,
john

Re: [PATCH PoC 1/3] ACPI / PNP: Don't add enumeration_by_parent devices

[Andy Shevchenko]

On Fri, Aug 19, 2022 at 1:20 PM John Garry <john.garry@huawei.com> wrote:
> On 19/08/2022 10:59, Andy Shevchenko wrote:
> >>>> The hisi_lpc driver also creates a platform device serial device for uart,
> >>>> which is the actual uart which we want to use - see
> >>>> hisi_lpc_acpi_add_child(). That function does not check
> >>>> physical_node_count value, but acpi_create_platform_device() does check it.
> >>>> So if we were to move hisi_lpc_acpi_add_child() across to use
> >>>> acpi_create_platform_device(), then the change in this patch is required to
> >>>> not create the PNP binding (so that physical_node_count is not set from
> >>>> PNP probe).
> >>> Hmm... The flag, as I interpret it, is equal to "the device in
> >>> question is a peripheral device to the non-discoverable bus, such as
> >>> SPI, I2C or UART". I.o.w. I do not see how PNP suits here. So, from my
> >>> point of view it seems like an abuse of the flag. Not sure the current
> >>> state of affairs in ACPI glue layer regarding this, though.
> >> Sorry, but I'm not following you here. Which flag are you talking about?
> > "enumerated by parent".
>
> ok, right. So I thought that PNP0501 was a standard cid to describe
> 16550-compat UART which may be on an LPC bus. And a LPC bus is
> non-discoverable, like SPI or I2C, which also use "enumerated by parent".

While in most cases the above-mentioned UART is sitting behind LPC and
what you described is all true, there is no requirement to have
PNP0501 to be tightened to LPC bus, so it can be any 8250-compatible
UART.

> Indeed, using PNP0501 for hisi lpc UART cid would be without problem,
> apart from the fact that we can't do all the PIO translation (so need
> "enumerated by parent").

Yeah, what can use this flag is the UART peer that connects to the
UART host, but it's a bit grey, because UART is p2p and not
controller-peripheral type of connections.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH PoC 2/3] ACPI: platform: Refactor acpi_create_platform_device()

[Rafael J. Wysocki]

On Fri, Aug 19, 2022 at 10:10 AM John Garry <john.garry@huawei.com> wrote:
>
> On 18/08/2022 20:41, Andy Shevchenko wrote:
> > On Tue, Aug 16, 2022 at 2:33 PM John Garry <john.garry@huawei.com> wrote:
> >>
> >> There is commonality between acpi_create_platform_device() and
> >> hisi_lpc_acpi_add_child(), in that it covers 2x main steps:
> >> - Read resources for the acpi_device
> >> - Create platform device
> >>
> >> Refactor acpi_create_platform_device() so that it may be reused by
> >> hisi_lpc_acpi_add_child() to reduce duplication.
> >
> > ...
> >
> >> + * acpi_create_platform_device_ops - Create platform device for ACPI device node
> >
> > Not sure I understand why _ops is a suffix for the function. I would
> > expect _ops to be a data struct where the ->xlate() and perhaps other
> > callbacks may be collected. It may be that I have missed that portion
> > in the previous discussion.
>
> ok, maybe I can put all the members into a struct, but I don't think
> that it improves the overall code too much.
>
> >
> > ...
> >
> >> +       if (name)
> >> +               pdevinfo.name = name;
> >> +       else
> >> +               pdevinfo.name = dev_name(&adev->dev);
> >
> >> +       pdevinfo.data = data;
> >> +       pdevinfo.size_data = size_data;
> >
> > It rather reminds me of platform device registration full with this
> > device info. May be what you need is
> > struct acpi_platfrom_device_info {
> >    properties;
> >    name;
> >    id;
> >    ->xlate();
> >    ...
> > };
> >
> > ?
> >
> > ...
> >
> >> +struct platform_device *acpi_create_platform_device_ops(
> >> +                               struct acpi_device *adev,
> >> +                               const char *name,
> >> +                               const struct property_entry *properties,
> >> +                               void *data, size_t size_data,
> >> +                               int (*xlat)(struct acpi_device *adev,
> >> +                                           struct resource *res,
> >> +                                           void *data, size_t size_data),
> >> +                               int id);
> >
> > ...because this looks  a bit too much from the amount of parameters
> > point of view.
> >
>
> ok, agreed.
>
> But even if we improve this code, the hisi_lpc changes are quite large
> and unwieldly.

Well, they allow you to drop quite a few LOC ...

Re: [PATCH PoC 2/3] ACPI: platform: Refactor acpi_create_platform_device()

[John Garry]

On 23/08/2022 17:42, Rafael J. Wysocki wrote:
>>>
>>>> +struct platform_device *acpi_create_platform_device_ops(
>>>> +                               struct acpi_device *adev,
>>>> +                               const char *name,
>>>> +                               const struct property_entry *properties,
>>>> +                               void *data, size_t size_data,
>>>> +                               int (*xlat)(struct acpi_device *adev,
>>>> +                                           struct resource *res,
>>>> +                                           void *data, size_t size_data),
>>>> +                               int id);
>>> ...because this looks  a bit too much from the amount of parameters
>>> point of view.
>>>
>> ok, agreed.

Hi Rafael,

 >>
 >> But even if we improve this code, the hisi_lpc changes are quite large
 >> and unwieldly.
 > Well, they allow you to drop quite a few LOC ...

Sure, but the ACPI platform device code here is growing by about the 
same amount :) However maybe we can reduce that with Andy's idea to 
create a struct of function args. But first I will go with using 
platform_device_register_full() in hisi_lpc.

And you please also check the PNP patch? I am not so keen on pushing for 
that.

thanks,
John