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

[Arnd Bergmann]

On Thu, Dec 10, 2020 at 11:29 AM Daniel Palmer <daniel at 0x0f.com> wrote:
> On Thu, 10 Dec 2020 at 06:58, Arnd Bergmann <arnd at kernel.org> wrote:
> > > +#define MSC313_GPIO_CHIPDATA(_chip) \
> > > static const struct msc313_gpio_data _chip##_data = { \
> > > +       .names = _chip##_names, \
> > > +       .offsets = _chip##_offsets, \
> > > +       .num = ARRAY_SIZE(_chip##_offsets), \
> > > +}
> >
> > > +#ifdef CONFIG_MACH_INFINITY
> > > +static const char * const msc313_names[] = {
> > > +       FUART_NAMES,
> > > +       SR_NAMES,
> >
> > I would try to avoid the #ifdefs and the macros here, don't overthink
> > it. The macro really hurts readability with the ## concatenation
> > making it impossible to grep for where things are defined, and
> > the #ifdef means you get worse compile test coverage compared
> > to an if(IS_ENABLED()) check in the place where the identifiers
> > are actually used.
>
> Ok. I was really just trying to enforce some sort of pattern there so
> that each chip that gets added follows the same convention.

> > Even better would be to completely avoid the lookup tables here,
> > and have one driver that is instantiated based on settings from
> > the DT.
>
> I did think about this and I did this with the clk mux driver I
> haven't pushed yet. In that case there is a random lump of registers
> with some muxes mixed into it so I decided to make the lump a syscon
> and then have a node for each clk mux in the lump and some properties
> for the muxes within. The driver is certainly less complex but the
> device tree is pretty unmanageable as there are probably 30 or more
> muxes.

Right, for clk drivers, the trade-off is often different, it's not
unusual that they are a bit of a mess and require a separate driver for
each cheap.

> > > +static void msc313_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
> > > +{
> > > +       struct msc313_gpio *gpio = gpiochip_get_data(chip);
> > > +       u8 gpioreg = readb_relaxed(gpio->base + gpio->gpio_data->offsets[offset]);
> > > +
> > > +       if (value)
> > > +               gpioreg |= MSC313_GPIO_OUT;
> > > +       else
> > > +               gpioreg &= ~MSC313_GPIO_OUT;
> > > +
> > > +       writeb_relaxed(gpioreg, gpio->base + gpio->gpio_data->offsets[offset]);
> > > +}
> >
> > It would be helpful here to replace all the readb_relaxed/writeb_relaxed()
> > with normal readb()/writeb(). Don't use _relaxed() unless there is a strong
> > reason why you have to do it, and if you do, explain it in a comment what
> > the reason is.
>
> The reason is that readb()/writeb() will invoke the heavy memory
> barrier even though it's not needed for peripheral registers.
> I guess it doesn't actually make all that much difference in reality.

Ah, I forgot you had that heavy barrier. It depends a bit on what you
use the GPIOs for then. For most uses I think the overhead does not
matter, but if there is any bit-banged I/O it might make a difference.

> > > +static int msc313_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value)
> > > +{
> > > +       struct msc313_gpio *gpio = gpiochip_get_data(chip);
> > > +       u8 gpioreg = readb_relaxed(gpio->base + gpio->gpio_data->offsets[offset]);
> > > +
> > > +       gpioreg &= ~MSC313_GPIO_OEN;
> > > +       if (value)
> > > +               gpioreg |= MSC313_GPIO_OUT;
> > > +       else
> > > +               gpioreg &= ~MSC313_GPIO_OUT;
> > > +       writeb_relaxed(gpioreg, gpio->base + gpio->gpio_data->offsets[offset]);
> >
> > These look like they also need a spinlock to avoid races between concurrent
> > read-modify-write cycles on the same register.
>
> Noted. I'll fix this and the readb and send a patch at some point.

Ok

> > > +builtin_platform_driver(msc313_gpio_driver);
> >
> > There is a trend to make all drivers optionally loadable modules these days.
> > Is there a reason this has to be built-in?
>
> This was discussed and I think Linus said it was ok.

Ok.

       Arnd