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

[John Garry]

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