Message-ID: <YdWqFC4fub1ztFd0@latitude>
Date:   Wed, 5 Jan 2022 15:24:20 +0100
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Neuschäfer <j.neuschaefer@....net>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        Tomer Maimon <tmaimon77@...il.com>,
        Joel Stanley <joel@....id.au>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Avi Fishman <avifishman70@...il.com>,
        Tali Perry <tali.perry1@...il.com>,
        Patrick Venture <venture@...gle.com>,
        Nancy Yuen <yuenn@...gle.com>,
        Benjamin Fair <benjaminfair@...gle.com>
Subject: Re: [PATCH v3 5/9] pinctrl: nuvoton: Add driver for WPCM450

Hello and happy (belated) new year,

On Fri, Dec 24, 2021 at 11:15:04PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 24, 2021 at 10:10 PM Jonathan Neuschäfer
> <j.neuschaefer@....net> wrote:
> >
> > This driver is based on the one for NPCM7xx, because the WPCM450 is a
> > predecessor of those SoCs. Notable differences:
> >
> > - WPCM450, the GPIO registers are not organized in multiple banks, but
> >   rather placed continually into the same register block. This affects
> >   how register offsets are computed.
> > - Pinmux nodes can explicitly select GPIO mode, whereas, in the npcm7xx
> >   driver, this happens automatically when a GPIO is requested.
> >
> > Some functionality implemented in the hardware was (for now) left unused
> > in the driver, specifically blinking and pull-up/down.
> 
> Overall looks good. Some cosmetic stuff is required, but there are no
> show stoppers.

Good to hear!

> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@....net>
> > ---
> >
> > This patch now depends on gpio/for-next, specifically these patches:
> > - gpiolib: improve coding style for local variables
> > - gpiolib: allow to specify the firmware node in struct gpio_chip
> > - gpiolib: of: make fwnode take precedence in struct gpio_chip
[...]
> > +/* GCR registers */
> > +#define WPCM450_GCR_MFSEL1     0x0C
> 
> Be consistent with capitalization

Oops, will fix.

> > +struct wpcm450_bank {
> > +       /* Range of GPIOs in this port */
> > +       u8 base;
> > +       u8 length;
> > +
> > +       /* Register offsets (0 = register doesn't exist in this port) */
> > +       u8 cfg0, cfg1, cfg2;
> > +       u8 blink;
> > +       u8 dataout, datain;
> > +
> > +       /* Interrupt bit mapping */
> > +       u8 first_irq_bit;
> > +       u8 num_irqs;
> > +       u8 first_irq_gpio;
> 
> These three are a bit undocumented.

I'll add descriptions.

> > +static const struct wpcm450_bank wpcm450_banks[WPCM450_NUM_BANKS] = {
> > +       /*  range   cfg0  cfg1  cfg2 blink  out   in     IRQ map */
> > +       {   0, 16,  0x14, 0x18,    0,    0, 0x1c, 0x20,  0, 16, 0 },
> > +       {  16, 16,  0x24, 0x28, 0x2c, 0x30, 0x34, 0x38, 16,  2, 8 },
> 
> So, the first_irq_gpio is used only here and as far as I understood it
> has only two IRQ capable GPIOs starting from offset 8 in this bank.
> What I didn't get is the relation on all these three. And could you
> confirm that hardware indeed doesn't support full range of IRQs (to me
> these settings look weird a bit)?

The GPIO controller indeed only has 18 interrupt-capable GPIOs,
16 in the first bank, and 2 in the second.

The full mapping is as follows:

  IRQ*    | int. bits**  | GPIO bank  | offsets
----------|--------------|------------|-----------
  2       |  0-3         |  0         |  0-3
  3       |  4-11        |  0         |  4-11
  4       |  12-15       |  0         |  12-15
  5       |  16-17       |  1         |  8-9

*) At the central interrupt controller
**) In the GPIO controller's interrupt registers such as GPEVST

The hardware is indeed a bit weird.

> > +       {  32, 16,  0x3c, 0x40, 0x44,    0, 0x48, 0x4c,  0,  0, 0 },
> > +       {  48, 16,  0x50, 0x54, 0x58,    0, 0x5c, 0x60,  0,  0, 0 },
> > +       {  64, 16,  0x64, 0x68, 0x6c,    0, 0x70, 0x74,  0,  0, 0 },
> > +       {  80, 16,  0x78, 0x7c, 0x80,    0, 0x84, 0x88,  0,  0, 0 },
> > +       {  96, 18,     0,    0,    0,    0,    0, 0x8c,  0,  0, 0 },
> > +       { 114, 14,  0x90, 0x94, 0x98,    0, 0x9c, 0xa0,  0,  0, 0 },
> > +};


> > +static void wpcm450_gpio_irq_ack(struct irq_data *d)
> > +{
> > +       struct wpcm450_gpio *gpio = gpiochip_get_data(irq_data_get_irq_chip_data(d));
> > +       struct wpcm450_pinctrl *pctrl = gpio->pctrl;
> 
> 
> > +       unsigned long flags;
> 
> Is it in IRQ context or not?

I think ->irq_ack should run in IRQ context, I'm less sure about the
other irq_chip methods. Unfortunately, linux/irq.h doesn't document
these details.

To avoid confusing myself and introducing bugs, I think I'll stay with
spin_lock_irqsave/spin_unlock_irqrestore.

> 
> > +       int bit;
> > +
> > +       bit = wpcm450_gpio_irq_bitnum(gpio, d);
> > +       if (bit < 0)
> > +               return;
> > +
> > +       spin_lock_irqsave(&pctrl->lock, flags);
> > +       iowrite32(BIT(bit), pctrl->gpio_base + WPCM450_GPEVST);
> > +       spin_unlock_irqrestore(&pctrl->lock, flags);
> > +}


> > +/*
> > + * Since the GPIO controller does not support dual-edge triggered interrupts
> > + * (IRQ_TYPE_EDGE_BOTH), they are emulated using rising/falling edge triggered
> > + * interrupts. wpcm450_gpio_fix_evpol sets the interrupt polarity for the
> > + * specified emulated dual-edge triggered interrupts, so that the next edge can
> > + * be detected.
> > + */
> > +static void wpcm450_gpio_fix_evpol(struct wpcm450_gpio *gpio, unsigned long all)
> > +{
> > +       struct wpcm450_pinctrl *pctrl = gpio->pctrl;
> > +       unsigned long flags;
> > +       unsigned int bit;
> > +
> > +       for_each_set_bit(bit, &all, 32) {
> > +               int offset = wpcm450_irq_bitnum_to_gpio(gpio, bit);
> > +               unsigned long evpol;
> > +               int level;
> > +
> > +               spin_lock_irqsave(&gpio->gc.bgpio_lock, flags);
> > +               do {
> > +                       evpol = ioread32(pctrl->gpio_base + WPCM450_GPEVPOL);
> 
> > +                       level = gpio->gc.get(&gpio->gc, offset);
> 
> I'm not sure why here and below you are using a method via GPIO chip.
> Why can't you simply call a method directly?

The ->get method is defined through bgpio_init, it's probably bgpio_get. The
bgpio_* methods are private to gpio-mmio.c, so I can't call them directly.

I could theoretically reimplement the functionality here, but I found it
easier to call the existing ->get function.

> 
> > +                       /* Switch event polarity to the opposite of the current level */
> > +                       __assign_bit(bit, &evpol, !level);
> > +
> > +                       iowrite32(evpol, pctrl->gpio_base + WPCM450_GPEVPOL);
> > +               } while (gpio->gc.get(&gpio->gc, offset) != level);
> > +               spin_unlock_irqrestore(&gpio->gc.bgpio_lock, flags);
> > +       }
> > +}


> > +static void wpcm450_gpio_irqhandler(struct irq_desc *desc)
> > +{
> > +       struct wpcm450_gpio *gpio = gpiochip_get_data(irq_desc_get_handler_data(desc));
> > +       struct wpcm450_pinctrl *pctrl = gpio->pctrl;
> > +       struct irq_chip *chip = irq_desc_get_chip(desc);
> > +       unsigned long pending;
> > +       unsigned long flags;
> > +       unsigned long ours;
> > +       unsigned int bit;
> 
> > +       ours = GENMASK(gpio->bank->first_irq_bit + gpio->bank->num_irqs - 1,
> > +                      gpio->bank->first_irq_bit);
> 
>        ours = GENMASK(gpio->bank->num_irqs - 1, 0) << gpio->bank->first_irq_bit;
> 
> is better to read and understand. I think I commented on this.

Fair; I'll change it.


> > +static int wpcm450_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
> > +                             unsigned long *config)
> > +{
> > +       struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > +       enum pin_config_param param = pinconf_to_config_param(*config);
> > +       unsigned long flags;
> > +       int bit;
> > +       u32 reg;
> > +
> > +       switch (param) {
> > +       case PIN_CONFIG_INPUT_DEBOUNCE:
> > +               bit = debounce_bitnum(pin);
> > +               if (bit < 0)
> > +                       return bit;
> > +
> > +               spin_lock_irqsave(&pctrl->lock, flags);
> > +               reg = ioread32(pctrl->gpio_base + WPCM450_GPEVDBNC);
> > +               spin_unlock_irqrestore(&pctrl->lock, flags);
> > +
> > +               *config = pinconf_to_config_packed(param, !!(reg & BIT(bit)));
> > +               break;
> > +       default:
> > +               return -ENOTSUPP;
> > +       }
> 
> > +
> > +       return 0;
> 
> Why not to return from the case?
> Ditto for the rest.

I'll change it.

> > +static int wpcm450_gpio_register(struct platform_device *pdev,
> > +                                struct wpcm450_pinctrl *pctrl)
> > +{
> > +       int ret = 0;
> 
> Redundant assignment.

Indeed, I'll fix it.

> 
> > +       struct fwnode_handle *child;
> > +
> > +       pctrl->gpio_base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (!pctrl->gpio_base)
> > +               return dev_err_probe(pctrl->dev, -ENOMEM, "Resource fail for GPIO controller\n");
> > +
> > +       device_for_each_child_node(pctrl->dev, child)  {
> 
> Please, be consistent with the device pointer you are using here and
> there, see below as well.

Will do as below.


> > +               gpio->gc.fwnode = child;
> 
> Thanks, but this will make it a material for v5.18 (starting from). I
> think since you send the v3 just at the holidays season followed by
> release, it's an intention and we have a few weeks to handle this
> series.

I don't particularly care about one release or another, so 5.18 is soon
enough for me.


> > +static int wpcm450_pinctrl_probe(struct platform_device *pdev)
> > +{
> > +       struct wpcm450_pinctrl *pctrl;
> > +       int ret;
> > +
> > +       pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
> > +       if (!pctrl)
> > +               return -ENOMEM;
> > +
> > +       pctrl->dev = &pdev->dev;
> > +       spin_lock_init(&pctrl->lock);
> > +       dev_set_drvdata(&pdev->dev, pctrl);
> > +
> > +       pctrl->gcr_regmap =
> > +               syscon_regmap_lookup_by_compatible("nuvoton,wpcm450-gcr");
> > +       if (IS_ERR(pctrl->gcr_regmap))
> > +               return dev_err_probe(pctrl->dev, PTR_ERR(pctrl->gcr_regmap),
> 
> Please, use the original device pointer in the ->probe().
> Sometimes it's good to have
> 
> struct device *dev = &pdev->dev;

Will do.


Thanks,
Jonathan Neuschäfer

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists