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

Message ID 1524218846-169934-2-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.
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(-)

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

John Garry April 20, 2018, 1:24 p.m. | #1
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

>

> .

>



--
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, 2:09 p.m. | #2
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

> .

>



--
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 26, 2018, 1:49 p.m. | #3
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

>

>> .

>>

>



--
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 26, 2018, 2:08 p.m. | #4
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?
--
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 26, 2018, 2:23 p.m. | #5
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


>

> .

>



--
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 26, 2018, 2:40 p.m. | #6
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.
--
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 27, 2018, 9:17 a.m. | #7
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

>

>

>>

>> .

>>

>



--
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
Lee Jones April 30, 2018, 5:36 a.m. | #8
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
--
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 30, 2018, 9 a.m. | #9
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

--
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
Lee Jones April 30, 2018, 9:26 a.m. | #10
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
--
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 30, 2018, 9:35 a.m. | #11
>>>> 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.

>>

>



--
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 30, 2018, 10:57 a.m. | #12
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
>



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