diff mbox series

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

Message ID 1660649244-146842-2-git-send-email-john.garry@huawei.com
State New
Headers show
Series ACPI/PNP/HISI_LPC: Refactor ACPI platform code for reuse | expand

Commit Message

John Garry Aug. 16, 2022, 11:27 a.m. UTC
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(-)

Comments

Andy Shevchenko Aug. 18, 2022, 7:31 p.m. UTC | #1
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.
John Garry Aug. 19, 2022, 8:05 a.m. UTC | #2
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
Andy Shevchenko Aug. 19, 2022, 9:59 a.m. UTC | #3
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".
John Garry Aug. 19, 2022, 10:20 a.m. UTC | #4
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
Andy Shevchenko Aug. 19, 2022, 10:41 a.m. UTC | #5
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.
diff mbox series

Patch

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++;