| Message ID | 1524218846-169934-3-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
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); > + }
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
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
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.
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 -- 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
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
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; }
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(-)