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

[Daniel Palmer]

Hi Andy,

On Thu, 10 Dec 2020 at 23:22, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
>
> Perhaps ordered?

Ok. I did try to find some rules on includes, mainly what should be
included even though it's included in another include, but couldn't
find anything really.
If you have a link that would be helpful. So I could track what
includes I actually needed I went for order they are used in the code.

> > +       if (offset >= OFF_SPI0_CZ && offset <= OFF_SPI0_DO) {
>
> Why not traditional pattern, i.e.
>
> if (...)
>   return -EINVAL;
> ...

You mean check if the offset is not in the interrupt capable range,
returning -EINVAL if so, and then having the interrupt mapping code?

> > +       ret = devm_gpiochip_add_data(dev, gpiochip, gpio);
> > +       return ret;
>
> Purpose?

Sorry I think that is probably an artefact of splitting the driver
apart to extract just the msc313 support.
The current version of this driver supports more chips but those
aren't completely reverse engineered yet so I've been constantly
switching back and forth.

> return devm_...(...);
>
> ...
>
> > +static int msc313_gpio_remove(struct platform_device *pdev)
> > +{
> > +       return 0;
> > +}
>
> Purpose?
>

None that I can think of. I think I was under the impression that a
remove callback was needed even if it did nothing.

>
> > +static const struct of_device_id msc313_gpio_of_match[] = {
>
> > +#ifdef CONFIG_MACH_INFINITY
>
> What's the point? Are you expecting two drivers for the same IP?

This will make more sense when the support for CONFIG_MACH_MERCURY is added.
infinity and mercury are very very close but have slightly different
pinouts, slightly different tables for clks, pin mux etc.
These chips only have 64MB of DRAM and it's embedded into the chip so
you probably don't want to include all the baggage for the whole
family in your kernel if you possibly can. Also the kernel only has a
few megabytes to fit into on the SPI NOR it's loaded from. Something
similar is going on for the ingenic pinctrl and I thought maybe
wrapping of_device_ids in #ifdefs was a no no and asked [0].
Arguably this is "peeing into the ocean" for a driver like this
because the difference is going to be tiny but I think I'm probably
tens of kilobytes away from my kernel not fitting anymore :).

> > +       {
> > +               .compatible = "mstar,msc313-gpio",
> > +               .data = &msc313_data,
> > +       },
> > +#endif
> > +       { }
> > +};
>
> ...
>
> > +static struct platform_driver msc313_gpio_driver = {
> > +       .driver = {
> > +               .name = DRIVER_NAME,
> > +               .of_match_table = msc313_gpio_of_match,
> > +               .pm = &msc313_gpio_ops,
> > +       },
> > +       .probe = msc313_gpio_probe,
> > +       .remove = msc313_gpio_remove,
> > +};

For the fixes to the above should I send another series just to fix
these up or can it wait a little while?
I'm pretty close to having all of the registers mapped out for another
chip that'll go into this driver so I could send these small changes
as part of that series.

Thanks,

Daniel

0 - https://lore.kernel.org/linux-arm-kernel/CAFr9PX=EgQSXeATLn++DSHkkQar35rpLGh978J5Lnw9jS8XMrw@xxxxxxxxxxxxxx/