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

Message ID 1524218846-169934-3-git-send-email-john.garry@huawei.com
State New
Headers show
Series
  • [RFC,1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices
Related show

Commit Message

John Garry April 20, 2018, 10:07 a.m.
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(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Andy Shevchenko April 20, 2018, 12:50 p.m. | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Garry April 20, 2018, 1:09 p.m. | #2
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


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg April 20, 2018, 1:12 p.m. | #3
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

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko April 20, 2018, 1:28 p.m. | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Garry April 20, 2018, 1:36 p.m. | #5
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

>

> .

>



--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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