Re: [PATCH v2 3/3] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Linus Walleij <linus.walleij@linaro.org>]

Hi Joey!

over all this driver is much improved and using a lot of stock functions
in the pin control core and getting really clean and compact.
I have one major nit below:

On Fri, Oct 1, 2021 at 9:12 PM Joey Gouly <joey.gouly@arm.com> wrote:

> +struct apple_gpio_pinctrl {
> +       // Shadow the pin values, the REG_GPIOx_DATA bit can read back stale values.
> +       u32 *pin_shadow;
(...)
> +// No locking needed to mask/unmask IRQs as the interrupt mode is per pin-register.
> +static void apple_gpio_set_reg(struct apple_gpio_pinctrl *pctl,
> +                              unsigned int pin, uint32_t clr, uint32_t set)
> +{
> +       void __iomem *ppin = pctl->base + REG_GPIO(pin);
> +       uint32_t prev, cfg;
> +
> +       prev = pctl->pin_shadow[pin];
> +       cfg = (prev & ~clr) | set;
> +
> +       if (!(prev & REG_GPIOx_CFG_DONE))
> +               writel_relaxed(cfg & ~REG_GPIOx_CFG_DONE, ppin);
> +       writel_relaxed(cfg, ppin);
> +       pctl->pin_shadow[pin] = cfg;
> +}

Are you not simply reinventing regmap-mmio here?

Keeping shadows of registers including write-only registers
is exactly what regmap does.

Check it out, if in doubt consult Mark Brown, I'm pretty
sure we can add what you need to regmap if it is missing.

> +static uint32_t apple_gpio_get_reg(struct apple_gpio_pinctrl *pctl,
> +                                  unsigned int pin)
> +{
> +       return readl_relaxed(pctl->base + REG_GPIO(pin));
> +}

If you use regmap-mmio I am pretty sure this will also
return the right value for the shadowed registers which it
currently does not IIUC.

> +               apple_gpio_set_reg(pctl, group, 0,
> +                                  REG_GPIOx_PERIPH | REG_GPIOx_CFG_DONE);
> +       else
> +               apple_gpio_set_reg(pctl, group, REG_GPIOx_PERIPH,
> +                                  REG_GPIOx_CFG_DONE);

I think all calls to apple_gpio_set_reg() could be replaced with
regmap_update_bits() or similar.

Yours,
Linus Walleij