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

[Arnd Bergmann]

On Sun, Nov 29, 2020 at 12:12 PM Daniel Palmer <daniel at 0x0f.com> wrote:
>
> This adds a driver that supports the GPIO block found in
> MStar/SigmaStar ARMv7 SoCs.
>
> The controller seems to have enough register for 128 lines
> but where they are wired up differs between chips and
> no currently known chip uses anywhere near 128 lines so there
> needs to be some per-chip data to collect together what lines
> actually have physical pins attached and map the right names to them.

I took a closer  look at the driver now, to figure out why this even
needs a dt-bindings header in the first place. I noticed a few other
things as well:

> +#define OFF_SR_IO3     0x8c
> +#define OFF_SR_IO4     0x90
> +#define OFF_SR_IO5     0x94
> +#define OFF_SR_IO6     0x98
> +#define OFF_SR_IO7     0x9c
> +#define OFF_SR_IO8     0xa0
> +#define OFF_SR_IO9     0xa4
> +#define OFF_SR_IO10    0xa8
> +#define OFF_SR_IO11    0xac
> +#define OFF_SR_IO12    0xb0
> +#define OFF_SR_IO13    0xb4
> +#define OFF_SR_IO14    0xb8
> +#define OFF_SR_IO15    0xbc
> +#define OFF_SR_IO16    0xc0
> +#define OFF_SR_IO17    0xc4
> +
> +#define SR_OFFSETS     \
> +       OFF_SR_IO2,     \
> +       OFF_SR_IO3,     \
> +       OFF_SR_IO4,     \
> +       OFF_SR_IO5,     \
> +       OFF_SR_IO6,     \
> +       OFF_SR_IO7,     \
> +       OFF_SR_IO8,     \
> +       OFF_SR_IO9,     \
> +       OFF_SR_IO10,    \
> +       OFF_SR_IO11,    \
> +       OFF_SR_IO12,    \
> +       OFF_SR_IO13,    \
> +       OFF_SR_IO14,    \
> +       OFF_SR_IO15,    \
> +       OFF_SR_IO16,    \
> +       OFF_SR_IO17
> +
> +#define SD_NAMES               \
> +       MSC313_PINNAME_SD_CLK,  \
> +       MSC313_PINNAME_SD_CMD,  \
> +       MSC313_PINNAME_SD_D0,   \
> +       MSC313_PINNAME_SD_D1,   \
> +       MSC313_PINNAME_SD_D2,   \
> +       MSC313_PINNAME_SD_D3
> +
> +#define OFF_SD_CLK     0x140
> +#define OFF_SD_CMD     0x144
> +#define OFF_SD_D0      0x148
> +#define OFF_SD_D1      0x14c
> +#define OFF_SD_D2      0x150
> +#define OFF_SD_D3      0x154

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.

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

Even better would be to completely avoid the lookup tables here,
and have one driver that is instantiated based on settings from
the DT.

> +struct msc313_gpio {
> +       void __iomem *base;
> +       const struct msc313_gpio_data *gpio_data;
> +       u8 *saved;
> +};
> +
> +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.

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

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

       Arnd