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