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

[Lee Jones <lee.jones@linaro.org>]

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