> > Since we enable this module not only support OF devices, but also support
> MFD devices, so we remove OF_GPIO dependenc
> > For 'PCI', the original code is also not depended on PCI, and this patch
> > also
> not, do you think it is necessary?
>
> if not PCI then you should add something likwe
> "depends on OF_GPIO || MFD"
>
> looking further, you need also HAS_IOMEM for things like
> devm_ioremap_resource(). Linus, wouldn't it make sense to group this driver
> and make the block depend on it?
>
I can add "depends on OF_GPIO || INTEL_QUARK_I2C_GPIO_MFD", since now the MFD
path only support Intel Quark.
Andriy, how do you think?
> > > > struct dwapb_gpio {
> > > > struct device *dev;
> > > > void __iomem *regs;
> > > > struct dwapb_gpio_port *ports;
> > > > - unsigned int nr_ports;
> > > you could keep this the way it is
> > It has been moved to 'pdata'.
>
> I saw that. But there is no need keep a pointer to pdata across the whole
> driver
> since only need nr_ports in the driver removal part of the whole driver.
Got your idea. So can I just use a global static pdata pointer instead of
adding it as a member of ' struct dwapb_gpio'?
Since pdata is used in all 'probe' functions, and make pdata as global static
make programming more easy.
>
> > > > struct irq_domain *domain;
> > > > + const struct dwapb_gpio_platform_data *pdata;
> > >
> > > and not making this a member of this struct. I've been going up and
> > > down the source and I don't see the need to marry dwapb_gpio to
> > > dwapb_gpio_platform_data.
> > > That dwapb_gpio_port_property thing has a long name. Could you just
> > > set it up, pass it for registration and the free it? You can't free
> > > the pdata for the non-OF tree but for the OF case you keep that struct
> around for no reason.
> > > You could safe some memory and use pdata directly for setup.
> > Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from
> MFD.
> > For OF, 'pdata' is getting from device nodes properties. Why do we
> > have such design? Because it can make the handling more easy for
> > flowing routine. All necessary properties get from 'pdata', never care it is
> from OF or MFD. And someone are working on replacing OF interface with a
> firmware agnostic device_property* interface which will work with both OF and
> ACPI.
> > More information for this design, please contact Darren Hart
> <dvh...@linux.intel.com>. Darren Hart wrote to me:
> > "Generally speaking, rather than if/else blocks throughout the code
> > checking if it is enumerated via open firmware or as a platform device, a
> cleaner approach is to check if the pdata is null, and if so, populate the
> pdata
> from the open firmware description if present.
> > Then use the pdata throughout the driver.
>
> But isn't it enough to hold on to this pdata thing through the probe function
> only? After probe is done you could drop that memory in the OF-case, right?
OK. If it is OF-case, pdata can be freed in the end of probe.
> > > > + irq_set_handler_data(port->pp->irq, gpio);
> > >
> > > This does not look like it belongs here. It should only be used
> > > together with
> > > irq_set_chained_handler() or am I missing here something?
> > This patch reused ' dwapb_irq_handler', it needs the irq handler data. For '
> irq_set_handler_data', it just sets the irq data.
> > Why do you think it must be used together with ' irq_set_chained_handler'?
>
> because you do request_irq(…, driver_data). If you you look close to
> irq_set_chained_handler() it does not have such a member. Thus it uses
> irq_set_handler_data() for that same purpose.
>
OK. I can improve it.
