Re: [PATCH v2 3/5] gpio: msc313: MStar MSC313 GPIO driver

[Daniel Palmer]

Hi Andy,

On Tue, 20 Oct 2020 at 20:59, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > +config GPIO_MSC313
> > +       bool "MStar MSC313 GPIO support"
>
> Why boolean?

Because it's a built in driver. I could change it to tristate/a module
but I didn't think it needed to be one.
The machines this is used on generally only have 64 or 128MB of RAM so
the kernel is usually built without modules and only the totally
required stuff built in.

> > +       default y if ARCH_MSTARV7
>
> Simply
>        default ARCH_MSTARV7
> should work as well.
>
> Are you planning to extend this to other boards?

I think I copy/pasted the block above there. I'll fix this up.

As for other boards. I think this GPIO controller is only present in
MStar's SoCs and some MediaTek SoCs that inherited parts from MStar
after MediaTek bought them. Like the MStar interrupt controller is
present in some MediaTek TV chips.

>
> > +       depends on ARCH_MSTARV7
> > +       select GPIOLIB_IRQCHIP
> > +       help
> > +         Say Y here to support GPIO on MStar MSC313 and later SoCs.
>
> Please, be more specific. Also it's recommended to have a module name
> to be included (but let's understand first why it's not a module)

Ok. I'll rework that. As for it not being a module. I can make it
possible to build it
as a module. I just didn't really think it needed to be one.

> > +#include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/gpio/driver.h>
>
> I believe this should be reworked.
> For example, it misses mod_devicetable.h, bits.h, io.h, types.h, etc, but has

I'll look at this again.

> > +/* These bits need to be saved to correctly restore the
> > + * gpio state when resuming from suspend to memory.
> > + */
>
> /*
>  * For this subsystem the comment style for multi-line
>  * like this.
>  */

Sorry, I'll fix these up.

> > +#ifdef CONFIG_MACH_INFINITY
>
> Does it make any sense?
>
> > +#endif

It doesn't make a lot of sense right now but it makes a bit more sense
when the support for the mercury chips is added. They have their own
set of offsets
for the used pins. I cut that out of this series because I haven't
fully reverse engineered all of the pins for those chips yet so the
support for them has a lot of guesses and notes in the code.

Anyhow, with only 64MB of RAM I thought it made sense to not compile
in the mercury tables when support for those machines isn't enabled.
I'll drop the if/defs for the next version.

> > +static struct irq_chip msc313_gpio_irqchip = {
> > +       .name = "GPIO",
>
> Is this name good enough?
>

There is only one GPIO block in the chips this is for so I think it's
unique at least.

> > +       gpiochip->label = DRIVER_NAME;
>
> Not good. When you use user space how do you distinguish if more than
> one chip appears in the system?

So far there is only ever one of these GPIO controller for the whole system.
There is another GPIO block in the system but it uses another driver
as the register layout is totally different.

Thank you for all of the comments. Sorry about the multiple style issues.
I thought checkpatch had my back on that.

Thanks,

Daniel