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@xxxxxxx> 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@xxxxxxx>
> > ---
> >
> > 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
Attachment:
signature.asc
Description: PGP signature
