[RFC PATCH 0/2] HISI LPC: Add PNP device support

[RFC PATCH 0/2] HISI LPC: Add PNP device support

[John Garry]

This patchset adds support for creating PNP devices
for devices attached to the HiSilicon LPC host bus.

Currently an mfd-cell (platform device) is created per
device on the bus for using ACPI-based firmware. As
such, we need require a platform driver for these
devices.

However for PNP-compatible devices, it would be better
to create a PNP device so that we may use the existing
PNP driver.

The PNP-compatible device we are interested in is the
8250-compatible UART on the Huawei D03 development
board. This uart has the following profile:
- IO space iotype
- no interrupt, so polling mode required
- 16550 compatible

Currently no platform driver exists for this. I did
send an RFC to add support to the 8250_dw driver, but
Andy Shevchenko suggested adding PNP support to the
host driver instead so we may use the appropriate PNP
driver (8250_pnp) without modification, see here:
https://lkml.org/lkml/2018/2/26/354

As for the implementation in this patchset, to avoid
the PNP scan from creating and adding the device,
we reuse the "enumeration_by_parent" flag to hold off
adding the PNP device, so the parent driver can modify
the IO resources to translate from the bus address
to the logical PIO address, and then add the device
for probing.

This solution is not good - hence the RFC. But I am
looking for suggestions on how to solve this.

Any opinions or ideas?

Thanks!

John Garry (2):
  ACPI / PNP: Don't add "enumeration_by_parent" devices
  HISI LPC: Add PNP device support

 drivers/bus/hisi_lpc.c     | 38 +++++++++++++++++++++++++++++++++++++-
 drivers/pnp/pnpacpi/core.c | 12 ++++++++----
 2 files changed, 45 insertions(+), 5 deletions(-)

-- 
1.9.1

[RFC PATCH 0/2] HISI LPC: Add PNP device support

[John Garry]

This patchset adds support for creating PNP devices
for devices attached to the HiSilicon LPC host bus.

Currently an mfd-cell (platform device) is created per
device on the bus for using ACPI-based firmware. As
such, we need require a platform driver for these
devices.

However for PNP-compatible devices, it would be better
to create a PNP device so that we may use the existing
PNP driver.

The PNP-compatible device we are interested in is the
8250-compatible UART on the Huawei D03 development
board. This uart has the following profile:
- IO space iotype
- no interrupt, so polling mode required
- 16550 compatible

Currently no platform driver exists for this. I did
send an RFC to add support to the 8250_dw driver, but
Andy Shevchenko suggested adding PNP support to the
host driver instead so we may use the appropriate PNP
driver (8250_pnp) without modification, see here:
https://lkml.org/lkml/2018/2/26/354

As for the implementation in this patchset, to avoid
the PNP scan from creating and adding the device,
we reuse the "enumeration_by_parent" flag to hold off
adding the PNP device, so the parent driver can modify
the IO resources to translate from the bus address
to the logical PIO address, and then add the device
for probing.

This solution is not good - hence the RFC. But I am
looking for suggestions on how to solve this.

Any opinions or ideas?

Thanks!

John Garry (2):
  ACPI / PNP: Don't add "enumeration_by_parent" devices
  HISI LPC: Add PNP device support

 drivers/bus/hisi_lpc.c     | 38 +++++++++++++++++++++++++++++++++++++-
 drivers/pnp/pnpacpi/core.c | 12 ++++++++----
 2 files changed, 45 insertions(+), 5 deletions(-)

-- 
1.9.1

[RFC PATCH 1/2] 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.

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

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 3a4c1aa..92f9d6f 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -285,10 +285,14 @@ 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) {
+		error = pnp_add_device(dev);
+		if (error) {
+			put_device(&dev->dev);
+			return error;
+		}
+	} else {
+		device->driver_data = dev;
 	}
 
 	num++;
-- 
1.9.1

[RFC PATCH 1/2] 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.

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

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 3a4c1aa..92f9d6f 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -285,10 +285,14 @@ 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) {
+		error = pnp_add_device(dev);
+		if (error) {
+			put_device(&dev->dev);
+			return error;
+		}
+	} else {
+		device->driver_data = dev;
 	}
 
 	num++;
-- 
1.9.1

[RFC PATCH 2/2] HISI LPC: Add PNP device support

[John Garry]

Currently the driver creates an per-ACPI device mfd_cell
for child devices. This does not suit devices which are
PNP-compatible, as we expect PNP-compatible devices to
derive PNP devices.

To add PNP device support, we continue to allow the PNP
scan code to create the PNP device (which have the
enumeration_by_parent flag set), but expect the PNP
scan to defer adding the device to allow the host probe
code to do this. In addition, no longer do we create an
mfd_cell (platform_device) for PNP-compatible devices.

We take this approach so that host probe code can
translate the IO resources of the PNP device prior
to adding the device.

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

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 2d4611e..d228bc5 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -17,8 +17,11 @@
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/pci.h>
+#include <linux/pnp.h>
 #include <linux/slab.h>
 
+#include "../pnp/base.h"
+
 #define DRV_NAME "hisi-lpc"
 
 /*
@@ -469,8 +472,11 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 	struct acpi_device *child;
 	int size, ret, count = 0, cell_num = 0;
 
-	list_for_each_entry(child, &adev->children, node)
+	list_for_each_entry(child, &adev->children, node) {
+		if (acpi_is_pnp_device(child))
+			continue;
 		cell_num++;
+	}
 
 	/* allocate the mfd cell and companion ACPI info, one per child */
 	size = sizeof(*mfd_cells) + sizeof(*hisi_lpc_mfd_cells);
@@ -492,6 +498,9 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 			.pnpid = pnpid,
 		};
 
+		if (acpi_is_pnp_device(child))
+			continue;
+
 		/*
 		 * For any instances of this host controller (Hip06 and Hip07
 		 * are the only chipsets), we would not have multiple slaves
@@ -523,6 +532,33 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 		return ret;
 	}
 
+	list_for_each_entry(child, &adev->children, node) {
+		struct pnp_resource *pnp_res;
+		struct pnp_dev *pnp_dev;
+		int rc;
+
+		if (!acpi_is_pnp_device(child))
+			continue;
+
+		pnp_dev = child->driver_data;
+
+		/*
+		 * Prior to adding the device, we need to translate the
+		 * resources to logical PIO addresses.
+		 */
+		list_for_each_entry(pnp_res, &pnp_dev->resources, list) {
+			struct resource *res = &pnp_res->res;
+
+			if (res->flags | IORESOURCE_IO)
+				hisi_lpc_acpi_xlat_io_res(child, adev, res);
+		}
+		rc = pnp_add_device(pnp_dev);
+		if (rc) {
+			put_device(&pnp_dev->dev);
+			return rc;
+		}
+	}
+
 	return 0;
 }
 
-- 
1.9.1

[RFC PATCH 2/2] HISI LPC: Add PNP device support

[John Garry]

Currently the driver creates an per-ACPI device mfd_cell
for child devices. This does not suit devices which are
PNP-compatible, as we expect PNP-compatible devices to
derive PNP devices.

To add PNP device support, we continue to allow the PNP
scan code to create the PNP device (which have the
enumeration_by_parent flag set), but expect the PNP
scan to defer adding the device to allow the host probe
code to do this. In addition, no longer do we create an
mfd_cell (platform_device) for PNP-compatible devices.

We take this approach so that host probe code can
translate the IO resources of the PNP device prior
to adding the device.

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

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 2d4611e..d228bc5 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -17,8 +17,11 @@
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/pci.h>
+#include <linux/pnp.h>
 #include <linux/slab.h>
 
+#include "../pnp/base.h"
+
 #define DRV_NAME "hisi-lpc"
 
 /*
@@ -469,8 +472,11 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 	struct acpi_device *child;
 	int size, ret, count = 0, cell_num = 0;
 
-	list_for_each_entry(child, &adev->children, node)
+	list_for_each_entry(child, &adev->children, node) {
+		if (acpi_is_pnp_device(child))
+			continue;
 		cell_num++;
+	}
 
 	/* allocate the mfd cell and companion ACPI info, one per child */
 	size = sizeof(*mfd_cells) + sizeof(*hisi_lpc_mfd_cells);
@@ -492,6 +498,9 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 			.pnpid = pnpid,
 		};
 
+		if (acpi_is_pnp_device(child))
+			continue;
+
 		/*
 		 * For any instances of this host controller (Hip06 and Hip07
 		 * are the only chipsets), we would not have multiple slaves
@@ -523,6 +532,33 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 		return ret;
 	}
 
+	list_for_each_entry(child, &adev->children, node) {
+		struct pnp_resource *pnp_res;
+		struct pnp_dev *pnp_dev;
+		int rc;
+
+		if (!acpi_is_pnp_device(child))
+			continue;
+
+		pnp_dev = child->driver_data;
+
+		/*
+		 * Prior to adding the device, we need to translate the
+		 * resources to logical PIO addresses.
+		 */
+		list_for_each_entry(pnp_res, &pnp_dev->resources, list) {
+			struct resource *res = &pnp_res->res;
+
+			if (res->flags | IORESOURCE_IO)
+				hisi_lpc_acpi_xlat_io_res(child, adev, res);
+		}
+		rc = pnp_add_device(pnp_dev);
+		if (rc) {
+			put_device(&pnp_dev->dev);
+			return rc;
+		}
+	}
+
 	return 0;
 }
 
-- 
1.9.1

Re: [RFC PATCH 2/2] HISI LPC: Add PNP device support

[Andy Shevchenko]

On Fri, 2018-04-20 at 18:07 +0800, John Garry wrote:
> Currently the driver creates an per-ACPI device mfd_cell
> for child devices. This does not suit devices which are
> PNP-compatible, as we expect PNP-compatible devices to
> derive PNP devices.
> 
> To add PNP device support, we continue to allow the PNP
> scan code to create the PNP device (which have the
> enumeration_by_parent flag set), but expect the PNP
> scan to defer adding the device to allow the host probe
> code to do this. In addition, no longer do we create an
> mfd_cell (platform_device) for PNP-compatible devices.
> 
> We take this approach so that host probe code can
> translate the IO resources of the PNP device prior
> to adding the device.

> +	list_for_each_entry(child, &adev->children, node) {
> +		if (acpi_is_pnp_device(child))
> +			continue;

This is good candidate for a separate helper macro

#define for_each_acpi_non_pnp_device(child, adev) \
...

(see, for example, for_each_pci_bridge() implementation as an example)

 
> +	list_for_each_entry(child, &adev->children, node) {

> +		if (!acpi_is_pnp_device(child))
> +			continue;

Ditto.

> +		/*
> +		 * Prior to adding the device, we need to translate
> the
> +		 * resources to logical PIO addresses.
> +		 */
> +		list_for_each_entry(pnp_res, &pnp_dev->resources,
> list) {
> +			struct resource *res = &pnp_res->res;
> +

> +			if (res->flags | IORESOURCE_IO)

What does this mean?

> +				hisi_lpc_acpi_xlat_io_res(child,
> adev, res);
> +		}

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[Mika Westerberg]

On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
> +	} else {
> +		device->driver_data = dev;

I think this deserves a comment explaining why we (ab)use driver_data
like this.

Re: [RFC PATCH 2/2] HISI LPC: Add PNP device support

[John Garry]

On 20/04/2018 13:50, Andy Shevchenko wrote:
> On Fri, 2018-04-20 at 18:07 +0800, John Garry wrote:
>> Currently the driver creates an per-ACPI device mfd_cell
>> for child devices. This does not suit devices which are
>> PNP-compatible, as we expect PNP-compatible devices to
>> derive PNP devices.
>>
>> To add PNP device support, we continue to allow the PNP
>> scan code to create the PNP device (which have the
>> enumeration_by_parent flag set), but expect the PNP
>> scan to defer adding the device to allow the host probe
>> code to do this. In addition, no longer do we create an
>> mfd_cell (platform_device) for PNP-compatible devices.
>>
>> We take this approach so that host probe code can
>> translate the IO resources of the PNP device prior
>> to adding the device.
>

Hi Andy,

Thanks for checking this.

>> +	list_for_each_entry(child, &adev->children, node) {
>> +		if (acpi_is_pnp_device(child))
>> +			continue;
>
> This is good candidate for a separate helper macro
>
> #define for_each_acpi_non_pnp_device(child, adev) \
> ...

Right, I did consider this, but was holding off refining until I get 
past RFC stage. In fact, we could also process each child entry in one 
loop, like this:

list_for_each_entry(child, &adev->children, node) {
	if (acpi_is_pnp_device(child)) {
		/* Do PNP compatible device work */

	} else {
		/* otherwise, make an MFD cell ... */
	}

>
> (see, for example, for_each_pci_bridge() implementation as an example)
>
>
>> +	list_for_each_entry(child, &adev->children, node) {
>
>> +		if (!acpi_is_pnp_device(child))
>> +			continue;
>
> Ditto.
>

ok

>> +		/*
>> +		 * Prior to adding the device, we need to translate
>> the
>> +		 * resources to logical PIO addresses.
>> +		 */
>> +		list_for_each_entry(pnp_res, &pnp_dev->resources,
>> list) {
>> +			struct resource *res = &pnp_res->res;
>> +
>
>> +			if (res->flags | IORESOURCE_IO)
>
> What does this mean?

Here we check the resource flag for each resource for this PNP device - 
for IO resources we must translate the resource value from the bus 
address to the logical PIO address.

>
>> +				hisi_lpc_acpi_xlat_io_res(child,
>> adev, res);
>> +		}
>

Regards,
John

Re: [RFC PATCH 2/2] HISI LPC: Add PNP device support

[John Garry]

On 20/04/2018 13:50, Andy Shevchenko wrote:
> On Fri, 2018-04-20 at 18:07 +0800, John Garry wrote:
>> Currently the driver creates an per-ACPI device mfd_cell
>> for child devices. This does not suit devices which are
>> PNP-compatible, as we expect PNP-compatible devices to
>> derive PNP devices.
>>
>> To add PNP device support, we continue to allow the PNP
>> scan code to create the PNP device (which have the
>> enumeration_by_parent flag set), but expect the PNP
>> scan to defer adding the device to allow the host probe
>> code to do this. In addition, no longer do we create an
>> mfd_cell (platform_device) for PNP-compatible devices.
>>
>> We take this approach so that host probe code can
>> translate the IO resources of the PNP device prior
>> to adding the device.
>

Hi Andy,

Thanks for checking this.

>> +	list_for_each_entry(child, &adev->children, node) {
>> +		if (acpi_is_pnp_device(child))
>> +			continue;
>
> This is good candidate for a separate helper macro
>
> #define for_each_acpi_non_pnp_device(child, adev) \
> ...

Right, I did consider this, but was holding off refining until I get 
past RFC stage. In fact, we could also process each child entry in one 
loop, like this:

list_for_each_entry(child, &adev->children, node) {
	if (acpi_is_pnp_device(child)) {
		/* Do PNP compatible device work */

	} else {
		/* otherwise, make an MFD cell ... */
	}

>
> (see, for example, for_each_pci_bridge() implementation as an example)
>
>
>> +	list_for_each_entry(child, &adev->children, node) {
>
>> +		if (!acpi_is_pnp_device(child))
>> +			continue;
>
> Ditto.
>

ok

>> +		/*
>> +		 * Prior to adding the device, we need to translate
>> the
>> +		 * resources to logical PIO addresses.
>> +		 */
>> +		list_for_each_entry(pnp_res, &pnp_dev->resources,
>> list) {
>> +			struct resource *res = &pnp_res->res;
>> +
>
>> +			if (res->flags | IORESOURCE_IO)
>
> What does this mean?

Here we check the resource flag for each resource for this PNP device - 
for IO resources we must translate the resource value from the bus 
address to the logical PIO address.

>
>> +				hisi_lpc_acpi_xlat_io_res(child,
>> adev, res);
>> +		}
>

Regards,
John

Re: [RFC PATCH 2/2] HISI LPC: Add PNP device support

[Mika Westerberg]

On Fri, Apr 20, 2018 at 06:07:26PM +0800, John Garry wrote:
> Currently the driver creates an per-ACPI device mfd_cell
> for child devices. This does not suit devices which are
> PNP-compatible, as we expect PNP-compatible devices to
> derive PNP devices.
> 
> To add PNP device support, we continue to allow the PNP
> scan code to create the PNP device (which have the
> enumeration_by_parent flag set), but expect the PNP
> scan to defer adding the device to allow the host probe
> code to do this. In addition, no longer do we create an
> mfd_cell (platform_device) for PNP-compatible devices.
> 
> We take this approach so that host probe code can
> translate the IO resources of the PNP device prior
> to adding the device.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/bus/hisi_lpc.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> index 2d4611e..d228bc5 100644
> --- a/drivers/bus/hisi_lpc.c
> +++ b/drivers/bus/hisi_lpc.c
> @@ -17,8 +17,11 @@
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
>  #include <linux/pci.h>
> +#include <linux/pnp.h>
>  #include <linux/slab.h>
>  
> +#include "../pnp/base.h"
> +
>  #define DRV_NAME "hisi-lpc"
>  
>  /*
> @@ -469,8 +472,11 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>  	struct acpi_device *child;
>  	int size, ret, count = 0, cell_num = 0;
>  
> -	list_for_each_entry(child, &adev->children, node)
> +	list_for_each_entry(child, &adev->children, node) {
> +		if (acpi_is_pnp_device(child))
> +			continue;
>  		cell_num++;
> +	}
>  
>  	/* allocate the mfd cell and companion ACPI info, one per child */
>  	size = sizeof(*mfd_cells) + sizeof(*hisi_lpc_mfd_cells);
> @@ -492,6 +498,9 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>  			.pnpid = pnpid,
>  		};
>  
> +		if (acpi_is_pnp_device(child))
> +			continue;
> +
>  		/*
>  		 * For any instances of this host controller (Hip06 and Hip07
>  		 * are the only chipsets), we would not have multiple slaves
> @@ -523,6 +532,33 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>  		return ret;
>  	}
>  
> +	list_for_each_entry(child, &adev->children, node) {
> +		struct pnp_resource *pnp_res;
> +		struct pnp_dev *pnp_dev;
> +		int rc;
> +
> +		if (!acpi_is_pnp_device(child))
> +			continue;
> +
> +		pnp_dev = child->driver_data;

...or better yet a PNP helper function that makes this more
understandable.

> +
> +		/*
> +		 * Prior to adding the device, we need to translate the
> +		 * resources to logical PIO addresses.
> +		 */
> +		list_for_each_entry(pnp_res, &pnp_dev->resources, list) {
> +			struct resource *res = &pnp_res->res;
> +
> +			if (res->flags | IORESOURCE_IO)

I think you should use

	if (resource_type(res) == IORESOURCE_IO)

instead.

> +				hisi_lpc_acpi_xlat_io_res(child, adev, res);
> +		}
> +		rc = pnp_add_device(pnp_dev);
> +		if (rc) {
> +			put_device(&pnp_dev->dev);
> +			return rc;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 1.9.1

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

Hi Mika,

On 20/04/2018 14:07, Mika Westerberg wrote:
> On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
>> +	} else {
>> +		device->driver_data = dev;
>
> I think this deserves a comment explaining why we (ab)use driver_data
> like this.

Sure, could add. I didn't see any other way for the acpi_device 
structure to reference the derived PNP device.

TBH, This overall approach is not good since we are creating the PNP 
device in the scan, and then leaving the device in limbo, waiting for 
the parent to add it, if at all. There's no rule for this.

So I'm looking for ideas on how to improve this.

Thanks,
John

>
> .
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

Hi Mika,

On 20/04/2018 14:07, Mika Westerberg wrote:
> On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
>> +	} else {
>> +		device->driver_data = dev;
>
> I think this deserves a comment explaining why we (ab)use driver_data
> like this.

Sure, could add. I didn't see any other way for the acpi_device 
structure to reference the derived PNP device.

TBH, This overall approach is not good since we are creating the PNP 
device in the scan, and then leaving the device in limbo, waiting for 
the parent to add it, if at all. There's no rule for this.

So I'm looking for ideas on how to improve this.

Thanks,
John

>
> .
>

Re: [RFC PATCH 2/2] HISI LPC: Add PNP device support

[Andy Shevchenko]

On Fri, 2018-04-20 at 14:09 +0100, John Garry wrote:
> On 20/04/2018 13:50, Andy Shevchenko wrote:
> > On Fri, 2018-04-20 at 18:07 +0800, John Garry wrote:

> > > +			if (res->flags | IORESOURCE_IO)
> > 
> > What does this mean?
> 
> Here we check the resource flag for each resource for this PNP device
> - 
> for IO resources we must translate the resource value from the bus 
> address to the logical PIO address.

Please, re-read your code again.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC PATCH 2/2] HISI LPC: Add PNP device support

[John Garry]

On 20/04/2018 14:28, Andy Shevchenko wrote:
> On Fri, 2018-04-20 at 14:09 +0100, John Garry wrote:
>> On 20/04/2018 13:50, Andy Shevchenko wrote:
>>> On Fri, 2018-04-20 at 18:07 +0800, John Garry wrote:
>
>>>> +			if (res->flags | IORESOURCE_IO)
>>>
>>> What does this mean?
>>
>> Here we check the resource flag for each resource for this PNP device
>> -
>> for IO resources we must translate the resource value from the bus
>> address to the logical PIO address.
>
> Please, re-read your code again.
>

Got it. But then again there is a helper for this, as Mika pointed out... :)

Cheers

Re: [RFC PATCH 2/2] HISI LPC: Add PNP device support

[John Garry]

On 20/04/2018 14:28, Andy Shevchenko wrote:
> On Fri, 2018-04-20 at 14:09 +0100, John Garry wrote:
>> On 20/04/2018 13:50, Andy Shevchenko wrote:
>>> On Fri, 2018-04-20 at 18:07 +0800, John Garry wrote:
>
>>>> +			if (res->flags | IORESOURCE_IO)
>>>
>>> What does this mean?
>>
>> Here we check the resource flag for each resource for this PNP device
>> -
>> for IO resources we must translate the resource value from the bus
>> address to the logical PIO address.
>
> Please, re-read your code again.
>

Got it. But then again there is a helper for this, as Mika pointed out... :)

Cheers

Re: [RFC PATCH 2/2] HISI LPC: Add PNP device support

[John Garry]

Hi Mika,

>>  /*
>> @@ -469,8 +472,11 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>>  	struct acpi_device *child;
>>  	int size, ret, count = 0, cell_num = 0;
>>
>> -	list_for_each_entry(child, &adev->children, node)
>> +	list_for_each_entry(child, &adev->children, node) {
>> +		if (acpi_is_pnp_device(child))
>> +			continue;
>>  		cell_num++;
>> +	}
>>
>>  	/* allocate the mfd cell and companion ACPI info, one per child */
>>  	size = sizeof(*mfd_cells) + sizeof(*hisi_lpc_mfd_cells);
>> @@ -492,6 +498,9 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>>  			.pnpid = pnpid,
>>  		};
>>
>> +		if (acpi_is_pnp_device(child))
>> +			continue;
>> +
>>  		/*
>>  		 * For any instances of this host controller (Hip06 and Hip07
>>  		 * are the only chipsets), we would not have multiple slaves
>> @@ -523,6 +532,33 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>>  		return ret;
>>  	}
>>
>> +	list_for_each_entry(child, &adev->children, node) {
>> +		struct pnp_resource *pnp_res;
>> +		struct pnp_dev *pnp_dev;
>> +		int rc;
>> +
>> +		if (!acpi_is_pnp_device(child))
>> +			continue;
>> +
>> +		pnp_dev = child->driver_data;
>
> ...or better yet a PNP helper function that makes this more
> understandable.

Sure, but I would not say the helper function would do the same, due to 
to (ab)use of driver_data element. As I mentioned in patch 1/2, I 
couldn't see a current method for the acpi_device to reference the PNP 
device.

>
>> +
>> +		/*
>> +		 * Prior to adding the device, we need to translate the
>> +		 * resources to logical PIO addresses.
>> +		 */
>> +		list_for_each_entry(pnp_res, &pnp_dev->resources, list) {
>> +			struct resource *res = &pnp_res->res;
>> +
>> +			if (res->flags | IORESOURCE_IO)
>
> I think you should use
>
> 	if (resource_type(res) == IORESOURCE_IO)
>
> instead.

Yes, since I don't know the difference between logical OR and logical AND :)

>
>> +				hisi_lpc_acpi_xlat_io_res(child, adev, res);
>> +		}
>> +		rc = pnp_add_device(pnp_dev);
>> +		if (rc) {
>> +			put_device(&pnp_dev->dev);
>> +			return rc;
>> +		}
>> +	}
>> +

Cheers,
John

>>  	return 0;
>>  }
>>
>> --
>> 1.9.1
>
> .
>

Re: [RFC PATCH 2/2] HISI LPC: Add PNP device support

[John Garry]

Hi Mika,

>>  /*
>> @@ -469,8 +472,11 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>>  	struct acpi_device *child;
>>  	int size, ret, count = 0, cell_num = 0;
>>
>> -	list_for_each_entry(child, &adev->children, node)
>> +	list_for_each_entry(child, &adev->children, node) {
>> +		if (acpi_is_pnp_device(child))
>> +			continue;
>>  		cell_num++;
>> +	}
>>
>>  	/* allocate the mfd cell and companion ACPI info, one per child */
>>  	size = sizeof(*mfd_cells) + sizeof(*hisi_lpc_mfd_cells);
>> @@ -492,6 +498,9 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>>  			.pnpid = pnpid,
>>  		};
>>
>> +		if (acpi_is_pnp_device(child))
>> +			continue;
>> +
>>  		/*
>>  		 * For any instances of this host controller (Hip06 and Hip07
>>  		 * are the only chipsets), we would not have multiple slaves
>> @@ -523,6 +532,33 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>>  		return ret;
>>  	}
>>
>> +	list_for_each_entry(child, &adev->children, node) {
>> +		struct pnp_resource *pnp_res;
>> +		struct pnp_dev *pnp_dev;
>> +		int rc;
>> +
>> +		if (!acpi_is_pnp_device(child))
>> +			continue;
>> +
>> +		pnp_dev = child->driver_data;
>
> ...or better yet a PNP helper function that makes this more
> understandable.

Sure, but I would not say the helper function would do the same, due to 
to (ab)use of driver_data element. As I mentioned in patch 1/2, I 
couldn't see a current method for the acpi_device to reference the PNP 
device.

>
>> +
>> +		/*
>> +		 * Prior to adding the device, we need to translate the
>> +		 * resources to logical PIO addresses.
>> +		 */
>> +		list_for_each_entry(pnp_res, &pnp_dev->resources, list) {
>> +			struct resource *res = &pnp_res->res;
>> +
>> +			if (res->flags | IORESOURCE_IO)
>
> I think you should use
>
> 	if (resource_type(res) == IORESOURCE_IO)
>
> instead.

Yes, since I don't know the difference between logical OR and logical AND :)

>
>> +				hisi_lpc_acpi_xlat_io_res(child, adev, res);
>> +		}
>> +		rc = pnp_add_device(pnp_dev);
>> +		if (rc) {
>> +			put_device(&pnp_dev->dev);
>> +			return rc;
>> +		}
>> +	}
>> +

Cheers,
John

>>  	return 0;
>>  }
>>
>> --
>> 1.9.1
>
> .
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[Mika Westerberg]

On Fri, Apr 20, 2018 at 02:24:18PM +0100, John Garry wrote:
> Hi Mika,
> 
> On 20/04/2018 14:07, Mika Westerberg wrote:
> > On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
> > > +	} else {
> > > +		device->driver_data = dev;
> > 
> > I think this deserves a comment explaining why we (ab)use driver_data
> > like this.
> 
> Sure, could add. I didn't see any other way for the acpi_device structure to
> reference the derived PNP device.
> 
> TBH, This overall approach is not good since we are creating the PNP device
> in the scan, and then leaving the device in limbo, waiting for the parent to
> add it, if at all. There's no rule for this.
> 
> So I'm looking for ideas on how to improve this.

One idea is to make pnpacpi_add_device() available outside of PNP and
call it directly (or some variation) in hisi_lpc.c when it walks over
its children.

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 20/04/2018 14:52, Mika Westerberg wrote:
> On Fri, Apr 20, 2018 at 02:24:18PM +0100, John Garry wrote:
>> Hi Mika,
>>
>> On 20/04/2018 14:07, Mika Westerberg wrote:
>>> On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
>>>> +	} else {
>>>> +		device->driver_data = dev;
>>>
>>> I think this deserves a comment explaining why we (ab)use driver_data
>>> like this.
>>
>> Sure, could add. I didn't see any other way for the acpi_device structure to
>> reference the derived PNP device.
>>
>> TBH, This overall approach is not good since we are creating the PNP device
>> in the scan, and then leaving the device in limbo, waiting for the parent to
>> add it, if at all. There's no rule for this.
>>
>> So I'm looking for ideas on how to improve this.
>

Hi Mika,

> One idea is to make pnpacpi_add_device() available outside of PNP and
> call it directly (or some variation) in hisi_lpc.c when it walks over
> its children.
>

I did consider this initially and it seems quite straightforward.

However I think the problem is that we would need to modify the 
acpi_device child resources from FW with kernel-specific resources, 
which does not seem right (I think that is what you meant). As I see, 
this is one reason that we went in the direction of modelling the host 
as an MFD, as we could set the resources of the mfd_cells.

So adding a variant of pnpacpi_add_device() could work, or modifying 
pnpacpi_add_device() to accept a callback to translate the resources. 
But this feature is specific to our very special requirement...

Thanks,
John

> .
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 20/04/2018 14:52, Mika Westerberg wrote:
> On Fri, Apr 20, 2018 at 02:24:18PM +0100, John Garry wrote:
>> Hi Mika,
>>
>> On 20/04/2018 14:07, Mika Westerberg wrote:
>>> On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
>>>> +	} else {
>>>> +		device->driver_data = dev;
>>>
>>> I think this deserves a comment explaining why we (ab)use driver_data
>>> like this.
>>
>> Sure, could add. I didn't see any other way for the acpi_device structure to
>> reference the derived PNP device.
>>
>> TBH, This overall approach is not good since we are creating the PNP device
>> in the scan, and then leaving the device in limbo, waiting for the parent to
>> add it, if at all. There's no rule for this.
>>
>> So I'm looking for ideas on how to improve this.
>

Hi Mika,

> One idea is to make pnpacpi_add_device() available outside of PNP and
> call it directly (or some variation) in hisi_lpc.c when it walks over
> its children.
>

I did consider this initially and it seems quite straightforward.

However I think the problem is that we would need to modify the 
acpi_device child resources from FW with kernel-specific resources, 
which does not seem right (I think that is what you meant). As I see, 
this is one reason that we went in the direction of modelling the host 
as an MFD, as we could set the resources of the mfd_cells.

So adding a variant of pnpacpi_add_device() could work, or modifying 
pnpacpi_add_device() to accept a callback to translate the resources. 
But this feature is specific to our very special requirement...

Thanks,
John

> .
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 20/04/2018 15:09, John Garry wrote:
> On 20/04/2018 14:52, Mika Westerberg wrote:
>> On Fri, Apr 20, 2018 at 02:24:18PM +0100, John Garry wrote:
>>> Hi Mika,
>>>
>>> On 20/04/2018 14:07, Mika Westerberg wrote:
>>>> On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
>>>>> +    } else {
>>>>> +        device->driver_data = dev;
>>>>
>>>> I think this deserves a comment explaining why we (ab)use driver_data
>>>> like this.
>>>
>>> Sure, could add. I didn't see any other way for the acpi_device
>>> structure to
>>> reference the derived PNP device.
>>>
>>> TBH, This overall approach is not good since we are creating the PNP
>>> device
>>> in the scan, and then leaving the device in limbo, waiting for the
>>> parent to
>>> add it, if at all. There's no rule for this.
>>>
>>> So I'm looking for ideas on how to improve this.
>>
>
> Hi Mika,
>
>> One idea is to make pnpacpi_add_device() available outside of PNP and
>> call it directly (or some variation) in hisi_lpc.c when it walks over
>> its children.
>>
>
> I did consider this initially and it seems quite straightforward.
>
> However I think the problem is that we would need to modify the
> acpi_device child resources from FW with kernel-specific resources,
> which does not seem right (I think that is what you meant). As I see,
> this is one reason that we went in the direction of modelling the host
> as an MFD, as we could set the resources of the mfd_cells.
>
> So adding a variant of pnpacpi_add_device() could work, or modifying
> pnpacpi_add_device() to accept a callback to translate the resources.
> But this feature is specific to our very special requirement...
>

Hi Andy, Mika,

I have spent a bit of time looking at this PNP support issue, and I 
still can't find a good solution.

So - as  discussed - I could add the call to pnpacpi_add_device(), but I 
would need a method to defer the pnp dev probe before resources fixup. 
As a alternative solution, I could add a callback pointer to 
pnpacpi_add_device(), for the caller to do the fixup, but this is quite 
arbitrary in the PNP code.

As an alternative, I am strongly considering this patch instead of 
adding PNP support:

-->8

Subject: [PATCH] HISI LPC: Add special handling for 8250-compatible UART

For APCI support, for each each child device on the host
LPC bus we create an mfd_cell, and, as such, we create a
platform device per ACPI child. This creates a problem
in 8250-compatible device support.

Currently the kernel does not support an suitable 8250-
compatible driver for the UART device on the LPC bus on
the Huawei D03 board, which has the following profile/
requirements:
- 16550 device
- platform_driver for ACPI device
- IO space iotype
- polling mode

In principle we should use the 8250_pnp driver for 8250-
compatible devices with ACPI firmware. However the host
driver does not support PNP devices, and the work is not
worth the effort to rework the host driver and PNP code
to support such a device.

As a alternate solution, add special UART handling to use
the 8250 isa generic driver for this one-off device.

Signed-off-by: John Garry <john.garry@huawei.com>

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 2d4611e..b04425b 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -18,6 +18,8 @@
  #include <linux/of_platform.h>
  #include <linux/pci.h>
  #include <linux/slab.h>
+#include <linux/serial_8250.h>
+#include "../tty/serial/8250/8250.h"

  #define DRV_NAME "hisi-lpc"

@@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, 
unsigned long pio,
  #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + 
sizeof(MFD_CHILD_NAME_PREFIX) - 1)

  struct hisi_lpc_mfd_cell {
+	struct plat_serial8250_port serial8250_port;
  	struct mfd_cell_acpi_match acpi_match;
  	char name[MFD_CHILD_NAME_LEN];
  	char pnpid[ACPI_ID_LEN];
@@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
  		dev_warn(&child->dev, "set resource fail (%d)\n", ret);
  			return ret;
  		}
+	if (!strcmp(acpi_device_hid(child), "HISI1031")) {
+		/*
+		 * Special handling for HISI1031 8250-compatible UART:
+		 * Since currently no platform driver exists for this
+		 * ACPI device, the generic 8250 isa driver instead.
+		 * For this, change cell name and add associated
+		 * serial port data.
+		 */
+		const struct resource * const io_base_resource =
+			mfd_cell->resources;
+		struct plat_serial8250_port ref =
+		SERIAL8250_PORT(io_base_resource->start, 0);
+
+		memcpy(&hisi_lpc_mfd_cell->serial8250_port,
+		       &ref, sizeof(ref));
+
+		mfd_cell->name = "serial8250";
+		mfd_cell->platform_data =
+			&hisi_lpc_mfd_cell->serial8250_port;
+		mfd_cell->pdata_size = sizeof(ref);
+	}
  	count++;
  	}

-	ret = mfd_add_devices(hostdev, PLATFORM_DEVID_NONE,
+	ret = mfd_add_devices(hostdev, PLATFORM_DEVID_AUTO,
  			      mfd_cells, cell_num, NULL, 0, NULL);
  	if (ret) {
  		dev_err(hostdev, "failed to add mfd cells (%d)\n", ret);

Any issue?

Thanks,
John

> Thanks,
> John
>
>> .
>>
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 20/04/2018 15:09, John Garry wrote:
> On 20/04/2018 14:52, Mika Westerberg wrote:
>> On Fri, Apr 20, 2018 at 02:24:18PM +0100, John Garry wrote:
>>> Hi Mika,
>>>
>>> On 20/04/2018 14:07, Mika Westerberg wrote:
>>>> On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
>>>>> +    } else {
>>>>> +        device->driver_data = dev;
>>>>
>>>> I think this deserves a comment explaining why we (ab)use driver_data
>>>> like this.
>>>
>>> Sure, could add. I didn't see any other way for the acpi_device
>>> structure to
>>> reference the derived PNP device.
>>>
>>> TBH, This overall approach is not good since we are creating the PNP
>>> device
>>> in the scan, and then leaving the device in limbo, waiting for the
>>> parent to
>>> add it, if at all. There's no rule for this.
>>>
>>> So I'm looking for ideas on how to improve this.
>>
>
> Hi Mika,
>
>> One idea is to make pnpacpi_add_device() available outside of PNP and
>> call it directly (or some variation) in hisi_lpc.c when it walks over
>> its children.
>>
>
> I did consider this initially and it seems quite straightforward.
>
> However I think the problem is that we would need to modify the
> acpi_device child resources from FW with kernel-specific resources,
> which does not seem right (I think that is what you meant). As I see,
> this is one reason that we went in the direction of modelling the host
> as an MFD, as we could set the resources of the mfd_cells.
>
> So adding a variant of pnpacpi_add_device() could work, or modifying
> pnpacpi_add_device() to accept a callback to translate the resources.
> But this feature is specific to our very special requirement...
>

Hi Andy, Mika,

I have spent a bit of time looking at this PNP support issue, and I 
still can't find a good solution.

So - as  discussed - I could add the call to pnpacpi_add_device(), but I 
would need a method to defer the pnp dev probe before resources fixup. 
As a alternative solution, I could add a callback pointer to 
pnpacpi_add_device(), for the caller to do the fixup, but this is quite 
arbitrary in the PNP code.

As an alternative, I am strongly considering this patch instead of 
adding PNP support:

-->8

Subject: [PATCH] HISI LPC: Add special handling for 8250-compatible UART

For APCI support, for each each child device on the host
LPC bus we create an mfd_cell, and, as such, we create a
platform device per ACPI child. This creates a problem
in 8250-compatible device support.

Currently the kernel does not support an suitable 8250-
compatible driver for the UART device on the LPC bus on
the Huawei D03 board, which has the following profile/
requirements:
- 16550 device
- platform_driver for ACPI device
- IO space iotype
- polling mode

In principle we should use the 8250_pnp driver for 8250-
compatible devices with ACPI firmware. However the host
driver does not support PNP devices, and the work is not
worth the effort to rework the host driver and PNP code
to support such a device.

As a alternate solution, add special UART handling to use
the 8250 isa generic driver for this one-off device.

Signed-off-by: John Garry <john.garry@huawei.com>

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 2d4611e..b04425b 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -18,6 +18,8 @@
  #include <linux/of_platform.h>
  #include <linux/pci.h>
  #include <linux/slab.h>
+#include <linux/serial_8250.h>
+#include "../tty/serial/8250/8250.h"

  #define DRV_NAME "hisi-lpc"

@@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, 
unsigned long pio,
  #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + 
sizeof(MFD_CHILD_NAME_PREFIX) - 1)

  struct hisi_lpc_mfd_cell {
+	struct plat_serial8250_port serial8250_port;
  	struct mfd_cell_acpi_match acpi_match;
  	char name[MFD_CHILD_NAME_LEN];
  	char pnpid[ACPI_ID_LEN];
@@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
  		dev_warn(&child->dev, "set resource fail (%d)\n", ret);
  			return ret;
  		}
+	if (!strcmp(acpi_device_hid(child), "HISI1031")) {
+		/*
+		 * Special handling for HISI1031 8250-compatible UART:
+		 * Since currently no platform driver exists for this
+		 * ACPI device, the generic 8250 isa driver instead.
+		 * For this, change cell name and add associated
+		 * serial port data.
+		 */
+		const struct resource * const io_base_resource =
+			mfd_cell->resources;
+		struct plat_serial8250_port ref =
+		SERIAL8250_PORT(io_base_resource->start, 0);
+
+		memcpy(&hisi_lpc_mfd_cell->serial8250_port,
+		       &ref, sizeof(ref));
+
+		mfd_cell->name = "serial8250";
+		mfd_cell->platform_data =
+			&hisi_lpc_mfd_cell->serial8250_port;
+		mfd_cell->pdata_size = sizeof(ref);
+	}
  	count++;
  	}

-	ret = mfd_add_devices(hostdev, PLATFORM_DEVID_NONE,
+	ret = mfd_add_devices(hostdev, PLATFORM_DEVID_AUTO,
  			      mfd_cells, cell_num, NULL, 0, NULL);
  	if (ret) {
  		dev_err(hostdev, "failed to add mfd cells (%d)\n", ret);

Any issue?

Thanks,
John

> Thanks,
> John
>
>> .
>>
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[Mika Westerberg]

On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote:
> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> index 2d4611e..b04425b 100644
> --- a/drivers/bus/hisi_lpc.c
> +++ b/drivers/bus/hisi_lpc.c
> @@ -18,6 +18,8 @@
>  #include <linux/of_platform.h>
>  #include <linux/pci.h>
>  #include <linux/slab.h>
> +#include <linux/serial_8250.h>
> +#include "../tty/serial/8250/8250.h"
> 
>  #define DRV_NAME "hisi-lpc"
> 
> @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, unsigned
> long pio,
>  #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + sizeof(MFD_CHILD_NAME_PREFIX) -
> 1)
> 
>  struct hisi_lpc_mfd_cell {
> +	struct plat_serial8250_port serial8250_port;
>  	struct mfd_cell_acpi_match acpi_match;
>  	char name[MFD_CHILD_NAME_LEN];
>  	char pnpid[ACPI_ID_LEN];
> @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>  		dev_warn(&child->dev, "set resource fail (%d)\n", ret);
>  			return ret;
>  		}
> +	if (!strcmp(acpi_device_hid(child), "HISI1031")) {

Hmm, there is a way in struct mfd_cell to match child devices using _HID
so is there something preventing you from using that?

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 26/04/2018 15:08, Mika Westerberg wrote:
> On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote:
>> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
>> index 2d4611e..b04425b 100644
>> --- a/drivers/bus/hisi_lpc.c
>> +++ b/drivers/bus/hisi_lpc.c
>> @@ -18,6 +18,8 @@
>>  #include <linux/of_platform.h>
>>  #include <linux/pci.h>
>>  #include <linux/slab.h>
>> +#include <linux/serial_8250.h>
>> +#include "../tty/serial/8250/8250.h"
>>
>>  #define DRV_NAME "hisi-lpc"
>>
>> @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, unsigned
>> long pio,
>>  #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + sizeof(MFD_CHILD_NAME_PREFIX) -
>> 1)
>>
>>  struct hisi_lpc_mfd_cell {
>> +	struct plat_serial8250_port serial8250_port;
>>  	struct mfd_cell_acpi_match acpi_match;
>>  	char name[MFD_CHILD_NAME_LEN];
>>  	char pnpid[ACPI_ID_LEN];
>> @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>>  		dev_warn(&child->dev, "set resource fail (%d)\n", ret);
>>  			return ret;
>>  		}
>> +	if (!strcmp(acpi_device_hid(child), "HISI1031")) {
>

Hi Mika,

> Hmm, there is a way in struct mfd_cell to match child devices using _HID
> so is there something preventing you from using that?

Not that I know about. Can you describe this method? I guess I also 
don't need to set the mfd_cell pnpid either for this special case device.

Thanks,
John


>
> .
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 26/04/2018 15:08, Mika Westerberg wrote:
> On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote:
>> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
>> index 2d4611e..b04425b 100644
>> --- a/drivers/bus/hisi_lpc.c
>> +++ b/drivers/bus/hisi_lpc.c
>> @@ -18,6 +18,8 @@
>>  #include <linux/of_platform.h>
>>  #include <linux/pci.h>
>>  #include <linux/slab.h>
>> +#include <linux/serial_8250.h>
>> +#include "../tty/serial/8250/8250.h"
>>
>>  #define DRV_NAME "hisi-lpc"
>>
>> @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, unsigned
>> long pio,
>>  #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + sizeof(MFD_CHILD_NAME_PREFIX) -
>> 1)
>>
>>  struct hisi_lpc_mfd_cell {
>> +	struct plat_serial8250_port serial8250_port;
>>  	struct mfd_cell_acpi_match acpi_match;
>>  	char name[MFD_CHILD_NAME_LEN];
>>  	char pnpid[ACPI_ID_LEN];
>> @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>>  		dev_warn(&child->dev, "set resource fail (%d)\n", ret);
>>  			return ret;
>>  		}
>> +	if (!strcmp(acpi_device_hid(child), "HISI1031")) {
>

Hi Mika,

> Hmm, there is a way in struct mfd_cell to match child devices using _HID
> so is there something preventing you from using that?

Not that I know about. Can you describe this method? I guess I also 
don't need to set the mfd_cell pnpid either for this special case device.

Thanks,
John


>
> .
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[Mika Westerberg]

On Thu, Apr 26, 2018 at 03:23:17PM +0100, John Garry wrote:
> Not that I know about. Can you describe this method? I guess I also don't
> need to set the mfd_cell pnpid either for this special case device.

There is some documentation in "MFD devices" chapter of
Documentation/acpi/enumeration.txt at least.

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 26/04/2018 15:23, John Garry wrote:

+

> On 26/04/2018 15:08, Mika Westerberg wrote:
>> On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote:
>>> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
>>> index 2d4611e..b04425b 100644
>>> --- a/drivers/bus/hisi_lpc.c
>>> +++ b/drivers/bus/hisi_lpc.c
>>> @@ -18,6 +18,8 @@
>>>  #include <linux/of_platform.h>
>>>  #include <linux/pci.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/serial_8250.h>
>>> +#include "../tty/serial/8250/8250.h"
>>>
>>>  #define DRV_NAME "hisi-lpc"
>>>
>>> @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata,
>>> unsigned
>>> long pio,
>>>  #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN +
>>> sizeof(MFD_CHILD_NAME_PREFIX) -
>>> 1)
>>>
>>>  struct hisi_lpc_mfd_cell {
>>> +    struct plat_serial8250_port serial8250_port;
>>>      struct mfd_cell_acpi_match acpi_match;
>>>      char name[MFD_CHILD_NAME_LEN];
>>>      char pnpid[ACPI_ID_LEN];
>>> @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device
>>> *hostdev)
>>>          dev_warn(&child->dev, "set resource fail (%d)\n", ret);
>>>              return ret;
>>>          }
>>> +    if (!strcmp(acpi_device_hid(child), "HISI1031")) {
>>
>
> Hi Mika,
>
>> Hmm, there is a way in struct mfd_cell to match child devices using _HID
>> so is there something preventing you from using that?
>
> Not that I know about. Can you describe this method? I guess I also
> don't need to set the mfd_cell pnpid either for this special case device.
>

So we using the mfd_cell to match child devices using _HID. At a glance, 
I don't actually see other drivers to use mfd_cell_acpi_match.pnpid .

Anyway we don't use static tables as we need to update the resources of 
the cell dynamically. However I do look at a driver like 
intel_quark_i2c_gpio.c, and this dynamically modifies the value of 
global mfd_cell array here:
https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266

I know the cell array is only used at probe time, but this did not look 
to be good standard practice to me.

Thanks,
John

> Thanks,
> John
>
>
>>
>> .
>>
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 26/04/2018 15:23, John Garry wrote:

+

> On 26/04/2018 15:08, Mika Westerberg wrote:
>> On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote:
>>> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
>>> index 2d4611e..b04425b 100644
>>> --- a/drivers/bus/hisi_lpc.c
>>> +++ b/drivers/bus/hisi_lpc.c
>>> @@ -18,6 +18,8 @@
>>>  #include <linux/of_platform.h>
>>>  #include <linux/pci.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/serial_8250.h>
>>> +#include "../tty/serial/8250/8250.h"
>>>
>>>  #define DRV_NAME "hisi-lpc"
>>>
>>> @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata,
>>> unsigned
>>> long pio,
>>>  #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN +
>>> sizeof(MFD_CHILD_NAME_PREFIX) -
>>> 1)
>>>
>>>  struct hisi_lpc_mfd_cell {
>>> +    struct plat_serial8250_port serial8250_port;
>>>      struct mfd_cell_acpi_match acpi_match;
>>>      char name[MFD_CHILD_NAME_LEN];
>>>      char pnpid[ACPI_ID_LEN];
>>> @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device
>>> *hostdev)
>>>          dev_warn(&child->dev, "set resource fail (%d)\n", ret);
>>>              return ret;
>>>          }
>>> +    if (!strcmp(acpi_device_hid(child), "HISI1031")) {
>>
>
> Hi Mika,
>
>> Hmm, there is a way in struct mfd_cell to match child devices using _HID
>> so is there something preventing you from using that?
>
> Not that I know about. Can you describe this method? I guess I also
> don't need to set the mfd_cell pnpid either for this special case device.
>

So we using the mfd_cell to match child devices using _HID. At a glance, 
I don't actually see other drivers to use mfd_cell_acpi_match.pnpid .

Anyway we don't use static tables as we need to update the resources of 
the cell dynamically. However I do look at a driver like 
intel_quark_i2c_gpio.c, and this dynamically modifies the value of 
global mfd_cell array here:
https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266

I know the cell array is only used at probe time, but this did not look 
to be good standard practice to me.

Thanks,
John

> Thanks,
> John
>
>
>>
>> .
>>
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[Lee Jones]

On Fri, 27 Apr 2018, John Garry wrote:
> On 26/04/2018 15:23, John Garry wrote:
> > On 26/04/2018 15:08, Mika Westerberg wrote:
> > > On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote:
> > > > diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> > > > index 2d4611e..b04425b 100644
> > > > --- a/drivers/bus/hisi_lpc.c
> > > > +++ b/drivers/bus/hisi_lpc.c
> > > > @@ -18,6 +18,8 @@
> > > >  #include <linux/of_platform.h>
> > > >  #include <linux/pci.h>
> > > >  #include <linux/slab.h>
> > > > +#include <linux/serial_8250.h>
> > > > +#include "../tty/serial/8250/8250.h"
> > > > 
> > > >  #define DRV_NAME "hisi-lpc"
> > > > 
> > > > @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata,
> > > > unsigned
> > > > long pio,
> > > >  #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN +
> > > > sizeof(MFD_CHILD_NAME_PREFIX) -
> > > > 1)
> > > > 
> > > >  struct hisi_lpc_mfd_cell {
> > > > +    struct plat_serial8250_port serial8250_port;
> > > >      struct mfd_cell_acpi_match acpi_match;
> > > >      char name[MFD_CHILD_NAME_LEN];
> > > >      char pnpid[ACPI_ID_LEN];
> > > > @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device
> > > > *hostdev)
> > > >          dev_warn(&child->dev, "set resource fail (%d)\n", ret);
> > > >              return ret;
> > > >          }
> > > > +    if (!strcmp(acpi_device_hid(child), "HISI1031")) {
> > > 
> > 
> > Hi Mika,
> > 
> > > Hmm, there is a way in struct mfd_cell to match child devices using _HID
> > > so is there something preventing you from using that?
> > 
> > Not that I know about. Can you describe this method? I guess I also
> > don't need to set the mfd_cell pnpid either for this special case device.
> > 
> 
> So we using the mfd_cell to match child devices using _HID. At a glance, I
> don't actually see other drivers to use mfd_cell_acpi_match.pnpid .
> 
> Anyway we don't use static tables as we need to update the resources of the
> cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c,
> and this dynamically modifies the value of global mfd_cell array here:
> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266
> 
> I know the cell array is only used at probe time, but this did not look to
> be good standard practice to me.

Lots of drivers do this to supply dynamic data.  If there is no other
sane way of providing such data, it's fine to do.  Although each
situation should be dealt with on a case-by-case basis.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 30/04/2018 06:36, Lee Jones wrote:
> On Fri, 27 Apr 2018, John Garry wrote:
>> On 26/04/2018 15:23, John Garry wrote:
>>> On 26/04/2018 15:08, Mika Westerberg wrote:
>>>> On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote:
>>>>> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
>>>>> index 2d4611e..b04425b 100644
>>>>> --- a/drivers/bus/hisi_lpc.c
>>>>> +++ b/drivers/bus/hisi_lpc.c
>>>>> @@ -18,6 +18,8 @@
>>>>>  #include <linux/of_platform.h>
>>>>>  #include <linux/pci.h>
>>>>>  #include <linux/slab.h>
>>>>> +#include <linux/serial_8250.h>
>>>>> +#include "../tty/serial/8250/8250.h"
>>>>>
>>>>>  #define DRV_NAME "hisi-lpc"
>>>>>
>>>>> @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata,
>>>>> unsigned
>>>>> long pio,
>>>>>  #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN +
>>>>> sizeof(MFD_CHILD_NAME_PREFIX) -
>>>>> 1)
>>>>>
>>>>>  struct hisi_lpc_mfd_cell {
>>>>> +    struct plat_serial8250_port serial8250_port;
>>>>>      struct mfd_cell_acpi_match acpi_match;
>>>>>      char name[MFD_CHILD_NAME_LEN];
>>>>>      char pnpid[ACPI_ID_LEN];
>>>>> @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device
>>>>> *hostdev)
>>>>>          dev_warn(&child->dev, "set resource fail (%d)\n", ret);
>>>>>              return ret;
>>>>>          }
>>>>> +    if (!strcmp(acpi_device_hid(child), "HISI1031")) {
>>>>
>>>
>>> Hi Mika,
>>>
>>>> Hmm, there is a way in struct mfd_cell to match child devices using _HID
>>>> so is there something preventing you from using that?
>>>
>>> Not that I know about. Can you describe this method? I guess I also
>>> don't need to set the mfd_cell pnpid either for this special case device.
>>>
>>
>> So we using the mfd_cell to match child devices using _HID. At a glance, I
>> don't actually see other drivers to use mfd_cell_acpi_match.pnpid .
>>
>> Anyway we don't use static tables as we need to update the resources of the
>> cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c,
>> and this dynamically modifies the value of global mfd_cell array here:
>> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266
>>
>> I know the cell array is only used at probe time, but this did not look to
>> be good standard practice to me.
>
> Lots of drivers do this to supply dynamic data.  If there is no other
> sane way of providing such data, it's fine to do.  Although each
> situation should be dealt with on a case-by-case basis.
>

Hi Lee,

Thanks for your input.

I do see others drivers which use dynamic mem for the mfd_cells (like 
cros_ec_dev.c), so what we're doing in this driver already is not 
totally unchartered territory. But creating the MFD cells from the ACPI 
table could be ...

Anyway, I'll cc you in my next patchset and maybe you can kindly check it.

Cheers,
John

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 30/04/2018 06:36, Lee Jones wrote:
> On Fri, 27 Apr 2018, John Garry wrote:
>> On 26/04/2018 15:23, John Garry wrote:
>>> On 26/04/2018 15:08, Mika Westerberg wrote:
>>>> On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote:
>>>>> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
>>>>> index 2d4611e..b04425b 100644
>>>>> --- a/drivers/bus/hisi_lpc.c
>>>>> +++ b/drivers/bus/hisi_lpc.c
>>>>> @@ -18,6 +18,8 @@
>>>>>  #include <linux/of_platform.h>
>>>>>  #include <linux/pci.h>
>>>>>  #include <linux/slab.h>
>>>>> +#include <linux/serial_8250.h>
>>>>> +#include "../tty/serial/8250/8250.h"
>>>>>
>>>>>  #define DRV_NAME "hisi-lpc"
>>>>>
>>>>> @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata,
>>>>> unsigned
>>>>> long pio,
>>>>>  #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN +
>>>>> sizeof(MFD_CHILD_NAME_PREFIX) -
>>>>> 1)
>>>>>
>>>>>  struct hisi_lpc_mfd_cell {
>>>>> +    struct plat_serial8250_port serial8250_port;
>>>>>      struct mfd_cell_acpi_match acpi_match;
>>>>>      char name[MFD_CHILD_NAME_LEN];
>>>>>      char pnpid[ACPI_ID_LEN];
>>>>> @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device
>>>>> *hostdev)
>>>>>          dev_warn(&child->dev, "set resource fail (%d)\n", ret);
>>>>>              return ret;
>>>>>          }
>>>>> +    if (!strcmp(acpi_device_hid(child), "HISI1031")) {
>>>>
>>>
>>> Hi Mika,
>>>
>>>> Hmm, there is a way in struct mfd_cell to match child devices using _HID
>>>> so is there something preventing you from using that?
>>>
>>> Not that I know about. Can you describe this method? I guess I also
>>> don't need to set the mfd_cell pnpid either for this special case device.
>>>
>>
>> So we using the mfd_cell to match child devices using _HID. At a glance, I
>> don't actually see other drivers to use mfd_cell_acpi_match.pnpid .
>>
>> Anyway we don't use static tables as we need to update the resources of the
>> cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c,
>> and this dynamically modifies the value of global mfd_cell array here:
>> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266
>>
>> I know the cell array is only used at probe time, but this did not look to
>> be good standard practice to me.
>
> Lots of drivers do this to supply dynamic data.  If there is no other
> sane way of providing such data, it's fine to do.  Although each
> situation should be dealt with on a case-by-case basis.
>

Hi Lee,

Thanks for your input.

I do see others drivers which use dynamic mem for the mfd_cells (like 
cros_ec_dev.c), so what we're doing in this driver already is not 
totally unchartered territory. But creating the MFD cells from the ACPI 
table could be ...

Anyway, I'll cc you in my next patchset and maybe you can kindly check it.

Cheers,
John

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[Lee Jones]

On Mon, 30 Apr 2018, John Garry wrote:

> On 30/04/2018 06:36, Lee Jones wrote:
> > On Fri, 27 Apr 2018, John Garry wrote:
> > > On 26/04/2018 15:23, John Garry wrote:
> > > > On 26/04/2018 15:08, Mika Westerberg wrote:
> > > > > On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote:
> > > > > > diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> > > > > > index 2d4611e..b04425b 100644
> > > > > > --- a/drivers/bus/hisi_lpc.c
> > > > > > +++ b/drivers/bus/hisi_lpc.c
> > > > > > @@ -18,6 +18,8 @@
> > > > > >  #include <linux/of_platform.h>
> > > > > >  #include <linux/pci.h>
> > > > > >  #include <linux/slab.h>
> > > > > > +#include <linux/serial_8250.h>
> > > > > > +#include "../tty/serial/8250/8250.h"
> > > > > > 
> > > > > >  #define DRV_NAME "hisi-lpc"
> > > > > > 
> > > > > > @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata,
> > > > > > unsigned
> > > > > > long pio,
> > > > > >  #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN +
> > > > > > sizeof(MFD_CHILD_NAME_PREFIX) -
> > > > > > 1)
> > > > > > 
> > > > > >  struct hisi_lpc_mfd_cell {
> > > > > > +    struct plat_serial8250_port serial8250_port;
> > > > > >      struct mfd_cell_acpi_match acpi_match;
> > > > > >      char name[MFD_CHILD_NAME_LEN];
> > > > > >      char pnpid[ACPI_ID_LEN];
> > > > > > @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device
> > > > > > *hostdev)
> > > > > >          dev_warn(&child->dev, "set resource fail (%d)\n", ret);
> > > > > >              return ret;
> > > > > >          }
> > > > > > +    if (!strcmp(acpi_device_hid(child), "HISI1031")) {
> > > > > 
> > > > 
> > > > Hi Mika,
> > > > 
> > > > > Hmm, there is a way in struct mfd_cell to match child devices using _HID
> > > > > so is there something preventing you from using that?
> > > > 
> > > > Not that I know about. Can you describe this method? I guess I also
> > > > don't need to set the mfd_cell pnpid either for this special case device.
> > > > 
> > > 
> > > So we using the mfd_cell to match child devices using _HID. At a glance, I
> > > don't actually see other drivers to use mfd_cell_acpi_match.pnpid .
> > > 
> > > Anyway we don't use static tables as we need to update the resources of the
> > > cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c,
> > > and this dynamically modifies the value of global mfd_cell array here:
> > > https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266
> > > 
> > > I know the cell array is only used at probe time, but this did not look to
> > > be good standard practice to me.
> > 
> > Lots of drivers do this to supply dynamic data.  If there is no other
> > sane way of providing such data, it's fine to do.  Although each
> > situation should be dealt with on a case-by-case basis.
> > 
> 
> Hi Lee,
> 
> Thanks for your input.
> 
> I do see others drivers which use dynamic mem for the mfd_cells (like
> cros_ec_dev.c), so what we're doing in this driver already is not totally
> unchartered territory. But creating the MFD cells from the ACPI table could
> be ...

Right.  I don't normally like mixing platform data technologies (MFD,
ACPI and DT).  I normally NACK patches which take information from
Device Tree and populate MFD cells with it.  ACPI would be the same I
guess.

> Anyway, I'll cc you in my next patchset and maybe you can kindly check it.
> 

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

>>>> So we using the mfd_cell to match child devices using _HID. At a glance, I
>>>> don't actually see other drivers to use mfd_cell_acpi_match.pnpid .
>>>>
>>>> Anyway we don't use static tables as we need to update the resources of the
>>>> cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c,
>>>> and this dynamically modifies the value of global mfd_cell array here:
>>>> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266
>>>>
>>>> I know the cell array is only used at probe time, but this did not look to
>>>> be good standard practice to me.
>>>
>>> Lots of drivers do this to supply dynamic data.  If there is no other
>>> sane way of providing such data, it's fine to do.  Although each
>>> situation should be dealt with on a case-by-case basis.
>>>
>>
>> Hi Lee,
>>
>> Thanks for your input.
>>
>> I do see others drivers which use dynamic mem for the mfd_cells (like
>> cros_ec_dev.c), so what we're doing in this driver already is not totally
>> unchartered territory. But creating the MFD cells from the ACPI table could
>> be ...
>
> Right.  I don't normally like mixing platform data technologies (MFD,
> ACPI and DT).  I normally NACK patches which take information from
> Device Tree and populate MFD cells with it.  ACPI would be the same I
> guess.

Oh, well that is what we have in this driver. So what's the preferred 
approach? Just not use MFD model at all if ACPI/DT needs to be scanned 
for data to create the cells?

Thanks,
John

>
>> Anyway, I'll cc you in my next patchset and maybe you can kindly check it.
>>
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

>>>> So we using the mfd_cell to match child devices using _HID. At a glance, I
>>>> don't actually see other drivers to use mfd_cell_acpi_match.pnpid .
>>>>
>>>> Anyway we don't use static tables as we need to update the resources of the
>>>> cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c,
>>>> and this dynamically modifies the value of global mfd_cell array here:
>>>> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266
>>>>
>>>> I know the cell array is only used at probe time, but this did not look to
>>>> be good standard practice to me.
>>>
>>> Lots of drivers do this to supply dynamic data.  If there is no other
>>> sane way of providing such data, it's fine to do.  Although each
>>> situation should be dealt with on a case-by-case basis.
>>>
>>
>> Hi Lee,
>>
>> Thanks for your input.
>>
>> I do see others drivers which use dynamic mem for the mfd_cells (like
>> cros_ec_dev.c), so what we're doing in this driver already is not totally
>> unchartered territory. But creating the MFD cells from the ACPI table could
>> be ...
>
> Right.  I don't normally like mixing platform data technologies (MFD,
> ACPI and DT).  I normally NACK patches which take information from
> Device Tree and populate MFD cells with it.  ACPI would be the same I
> guess.

Oh, well that is what we have in this driver. So what's the preferred 
approach? Just not use MFD model at all if ACPI/DT needs to be scanned 
for data to create the cells?

Thanks,
John

>
>> Anyway, I'll cc you in my next patchset and maybe you can kindly check it.
>>
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[Lee Jones]

On Mon, 30 Apr 2018, John Garry wrote:

> > > > > So we using the mfd_cell to match child devices using _HID. At a glance, I
> > > > > don't actually see other drivers to use mfd_cell_acpi_match.pnpid .
> > > > > 
> > > > > Anyway we don't use static tables as we need to update the resources of the
> > > > > cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c,
> > > > > and this dynamically modifies the value of global mfd_cell array here:
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266
> > > > > 
> > > > > I know the cell array is only used at probe time, but this did not look to
> > > > > be good standard practice to me.
> > > > 
> > > > Lots of drivers do this to supply dynamic data.  If there is no other
> > > > sane way of providing such data, it's fine to do.  Although each
> > > > situation should be dealt with on a case-by-case basis.
> > > > 
> > > 
> > > Hi Lee,
> > > 
> > > Thanks for your input.
> > > 
> > > I do see others drivers which use dynamic mem for the mfd_cells (like
> > > cros_ec_dev.c), so what we're doing in this driver already is not totally
> > > unchartered territory. But creating the MFD cells from the ACPI table could
> > > be ...
> > 
> > Right.  I don't normally like mixing platform data technologies (MFD,
> > ACPI and DT).  I normally NACK patches which take information from
> > Device Tree and populate MFD cells with it.  ACPI would be the same I
> > guess.
> 
> Oh, well that is what we have in this driver. So what's the preferred
> approach? Just not use MFD model at all if ACPI/DT needs to be scanned for
> data to create the cells?

I've just seen the driver - yuk!

Why are you using the MFD API outside of MFD anyway?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 30/04/2018 11:46, Lee Jones wrote:
> On Mon, 30 Apr 2018, John Garry wrote:
>
>>>>>> So we using the mfd_cell to match child devices using _HID. At a glance, I
>>>>>> don't actually see other drivers to use mfd_cell_acpi_match.pnpid .
>>>>>>
>>>>>> Anyway we don't use static tables as we need to update the resources of the
>>>>>> cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c,
>>>>>> and this dynamically modifies the value of global mfd_cell array here:
>>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266
>>>>>>
>>>>>> I know the cell array is only used at probe time, but this did not look to
>>>>>> be good standard practice to me.
>>>>>
>>>>> Lots of drivers do this to supply dynamic data.  If there is no other
>>>>> sane way of providing such data, it's fine to do.  Although each
>>>>> situation should be dealt with on a case-by-case basis.
>>>>>
>>>>
>>>> Hi Lee,
>>>>
>>>> Thanks for your input.
>>>>
>>>> I do see others drivers which use dynamic mem for the mfd_cells (like
>>>> cros_ec_dev.c), so what we're doing in this driver already is not totally
>>>> unchartered territory. But creating the MFD cells from the ACPI table could
>>>> be ...
>>>
>>> Right.  I don't normally like mixing platform data technologies (MFD,
>>> ACPI and DT).  I normally NACK patches which take information from
>>> Device Tree and populate MFD cells with it.  ACPI would be the same I
>>> guess.
>>
>> Oh, well that is what we have in this driver. So what's the preferred
>> approach? Just not use MFD model at all if ACPI/DT needs to be scanned for
>> data to create the cells?
>
> I've just seen the driver - yuk!
>
> Why are you using the MFD API outside of MFD anyway?

Hi Lee,

This goes back a bit. The original thread was here:
https://lkml.org/lkml/2017/6/13/581

Essentially a method was required to model this host to support platform 
device children for ACPI FW, and this did the job. In hindsight I think 
that there was a misunderstanding in recommending MFD since the devices 
attached are not fixed - hence the dynamic part.

Cheers,
John
>

Re: [RFC PATCH 1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

[John Garry]

On 30/04/2018 11:46, Lee Jones wrote:
> On Mon, 30 Apr 2018, John Garry wrote:
>
>>>>>> So we using the mfd_cell to match child devices using _HID. At a glance, I
>>>>>> don't actually see other drivers to use mfd_cell_acpi_match.pnpid .
>>>>>>
>>>>>> Anyway we don't use static tables as we need to update the resources of the
>>>>>> cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c,
>>>>>> and this dynamically modifies the value of global mfd_cell array here:
>>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266
>>>>>>
>>>>>> I know the cell array is only used at probe time, but this did not look to
>>>>>> be good standard practice to me.
>>>>>
>>>>> Lots of drivers do this to supply dynamic data.  If there is no other
>>>>> sane way of providing such data, it's fine to do.  Although each
>>>>> situation should be dealt with on a case-by-case basis.
>>>>>
>>>>
>>>> Hi Lee,
>>>>
>>>> Thanks for your input.
>>>>
>>>> I do see others drivers which use dynamic mem for the mfd_cells (like
>>>> cros_ec_dev.c), so what we're doing in this driver already is not totally
>>>> unchartered territory. But creating the MFD cells from the ACPI table could
>>>> be ...
>>>
>>> Right.  I don't normally like mixing platform data technologies (MFD,
>>> ACPI and DT).  I normally NACK patches which take information from
>>> Device Tree and populate MFD cells with it.  ACPI would be the same I
>>> guess.
>>
>> Oh, well that is what we have in this driver. So what's the preferred
>> approach? Just not use MFD model at all if ACPI/DT needs to be scanned for
>> data to create the cells?
>
> I've just seen the driver - yuk!
>
> Why are you using the MFD API outside of MFD anyway?

Hi Lee,

This goes back a bit. The original thread was here:
https://lkml.org/lkml/2017/6/13/581

Essentially a method was required to model this host to support platform 
device children for ACPI FW, and this did the job. In hindsight I think 
that there was a misunderstanding in recommending MFD since the devices 
attached are not fixed - hence the dynamic part.

Cheers,
John
>