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

[Daniel Palmer]

Hi Arnd,

On Thu, 10 Dec 2020 at 06:58, Arnd Bergmann <arnd@xxxxxxxxxx> wrote:
> These seem to just be contiguous ranges, so I probably would have
> suggested describing them as separate gpio controllers to avoid
> all the complexity with the names. As Linus already merged the
> driver into the gpio tree, I won't complain too much about it.
>
> Maybe you can do that for the other chips though and have one
> implementation that works for all others, leaving only the msc313
> as the one with the extra complexity.

I'll have a think about that. The other chips I'm aiming to support
(the mercury5 ssc8336 and infinity2m ssd202) currently reuse most of
the msc313 bits with a few extras for the differences.
i.e. the ssc8336 reuses most of the tables for the msc313 with some
additions. Adding new chips hasn't been too bad so far.

> > +#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.

> > +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.

> > +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.

> > +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.

Thanks,

Daniel