Re: [PATCH v4] mfd: tqmx86: IO controller with i2c, wachdog and gpio

[Andrew Lunn]

> > > For platform data, it should be "*_platform_data" or "*_pdata".
> > 
> > It would of been useful if you had said this in the first review,
> > rather than s/data/ddata/, which is rather ambiguous.
> 
> How is that ambiguous?  I guess it would be confusing if you didn't
> know the syntax, in which case you should have asked.
> 
> s/this/that/ simply means, replace 'this' with 'that'.

It is ambiguous if you mean just that one line, one variable, or the
whole file.

> For platform data, it should be "*_platform_data" or "*_pdata".

This is very unambiguous.

> > > > +static uint gpio_irq;
> > > > +module_param(gpio_irq, uint, 0);
> > > > +MODULE_PARM_DESC(gpio_irq, "GPIO IRQ number (7, 9, 12)");
> > > 
> > > What is the purpose of providing the IRQ number via a module param?
> > > 
> > > These seems like a very bad idea.
> > 
> > I explained this in my reply to your review comments for version 2.
> > 
> > > Can this driver be built-in?
> > 
> > Yes it can. But you can pass module parameters on the command line, so
> > that is not an issue. This is something i actually use.
> 
> What is connected to these IRQs?

MODULE_PARM_DESC says:

"GPIO IRQ number (7, 9, 12)"

One of the devices this MFD instantiates is a GPIO controller. Linus
Walleij accepted it a few days ago:

https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=for-next&id=b868db94a6a704755a33a362cfcf4625abdda115

It can generate interrupts on its GPI's. Unfortunately, the interrupt
the GPIO device uses needs to be configured in the io_ext_int register
which is shared by all devices in the MFD, and it has to unique across
all devices in the MFD. So the module parameter is here, and it then
gets passed to the GPIO driver as a resource in the usual way.

     Andrew