Re: [PATCH v2 5/8] pinctrl: nuvoton: Add driver for WPCM450

["Jonathan Neuschäfer" <j.neuschaefer@gmx.net>]

[-- Attachment #1: Type: text/plain, Size: 7225 bytes --]

Hi,

On Wed, Dec 08, 2021 at 01:24:18PM +0200, Andy Shevchenko wrote:
> On Tuesday, December 7, 2021, Jonathan Neuschäfer <j.neuschaefer@gmx.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.
> >
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
[...]
> > +
> > +#include <linux/device.h>
> > +#include <linux/fwnode.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> 
> 
> + blank line?

Sounds reasonable, I'll add it.


> > +struct wpcm450_gpio {
> > +       struct wpcm450_pinctrl  *pctrl;
> > +       struct gpio_chip        gc;
> 
> 
> Making this first speeds up pointer arithmetics by making it no-op at
> compile time.

Will do.


> > +static int wpcm450_gpio_irq_bitnum(struct wpcm450_gpio *gpio, struct
> > irq_data *d)
> > +{
> > +       int hwirq = irqd_to_hwirq(d);
> > +
> > +       if (hwirq < gpio->first_irq_gpio)
> > +               return -EINVAL;
> > +
> > +       if (hwirq - gpio->first_irq_gpio >= gpio->num_irqs)
> > +               return -EINVAL;
> > +
> > +       return hwirq - gpio->first_irq_gpio + gpio->first_irq_bit;
> > +}
> > +
> > +static int wpcm450_irq_bitnum_to_gpio(struct wpcm450_gpio *gpio, int
> > bitnum)
> > +{
> > +       if (bitnum < gpio->first_irq_bit)
> > +               return -EINVAL;
> > +
> > +       if (bitnum - gpio->first_irq_bit > gpio->num_irqs)
> > +               return -EINVAL;
> > +
> > +       return bitnum - gpio->first_irq_bit + gpio->first_irq_gpio;
> > +}
> >
> 
> 
> Have you chance to look at bitmap_remap() and bitmap_bitremap() APIs? I’m
> pretty sure you can make this all cleaner by switching to those calls and
> represent the GPIOs as continuous bitmap on the Linux side while on the
> hardware it will be sparse. Check gpio-Xilinx for the details of use.

I haven't looked at it yet in detail, but I'll consider it for the next
iteration.

> > +static void wpcm450_gpio_fix_evpol(struct wpcm450_gpio *gpio, unsigned
> > long all)
> > +{
> 
> 
> 
> What is this quirk (?) for? Please add a comment.

The hardware does not support triggering on both edges, so the trigger
edge polarity has to be adjusted before the next interrupt can work
properly.

I'll add a comment.


> > +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 = ((1UL << gpio->num_irqs) - 1) << gpio->first_irq_bit;
> 
> 
> BIT()

I'll use it, but in this case, I think it doesn't simplify much the
whole expression all that much. Is there perhaps a macro that
constructs a continuous bitmask of N bits, perhaps additionally
left-shifted by M bits?

Maybe somewhere in the bitmap_* API...


> > +       chained_irq_enter(chip, desc);
> > +       for_each_set_bit(bit, &pending, 32) {
> > +               int offset = wpcm450_irq_bitnum_to_gpio(gpio, bit);
> > +               int irq = irq_find_mapping(gpio->gc.irq.domain, offset);
> > +
> > +               generic_handle_irq(irq);
> 
> 
> Above two are now represented by another generic IRQ handle call, check
> relatively recently updated drivers around.

Will do.

> > +       }
> > +       chained_irq_exit(chip, desc);



> > +               spin_lock_irqsave(&pctrl->lock, flags);
> > +               reg = ioread32(pctrl->gpio_base + WPCM450_GPEVDBNC);
> > +               __assign_bit(bit, &reg, arg);
> 
> 
>  In all these cases are you really need to use __assign_bit() APIs? I don’t
> see that this goes any higher than 32-bit.
> 
> It’s not a big deal though.

Not really necessary, it just seemed short and good, because it saved
having to spent multiple lines setting/resetting the bit in the variable.


> > +static int wpcm450_gpio_register(struct platform_device *pdev,
> > +                                struct wpcm450_pinctrl *pctrl)
> > +{
> > +       int ret = 0;
> > +       struct fwnode_handle *np;
> 
> 
>  Either be fully OF, or don’t name ‘np' here. We usually use fwnode or
> ‘child’ in this case.

Ah, I thought "np" (= node pointer) was still appropriate because I'm
dealing with firmware _nodes_. My intention was indeed to switch fully
to the fwnode API.


> > +
> > +       pctrl->gpio_base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (!pctrl->gpio_base) {
> > +               dev_err(pctrl->dev, "Resource fail for GPIO controller\n");
> > +               return -ENOMEM;
> 
> 
> dev_err_probe(), ditto for the rest in ->probe().

Oh, I missed these error paths when I changed wpcm450_pinctrl_probe to
dev_err_probe(). I'll go through the rest of the dev_err calls.


> > +       fwnode_for_each_available_child_node(pctrl->dev->fwnode, np) {
> 
> 
> Please, do not dereference fwnode, instead use analogue from device_*()
> APIs. Hence, replace fwnode.h with property.h.

Ok, I'll use device_for_each_child_node() for iteration.


> > +               gpio->gc.of_node = to_of_node(np);
> 
> 
> I hope we will soon have fwnode in gpio_chip.

Yes, that would be good.



> > +               gpio->gc.parent = pctrl->dev;
> >
> >
> Set by bgpio_init(), also check for other potential duplications.

Good catch, I'll check the assignments again.


> > +               ret = gpiochip_add_pin_range(&gpio->gc,
> > dev_name(pctrl->dev),
> > +                                            0, bank->base, bank->length);
> > +               if (ret) {
> > +                       dev_err(pctrl->dev, "Failed to add pin range for
> > GPIO bank %u\n", reg);
> > +                       return ret;
> > +               }
> 
> 
> 
> Please move it to the corresponding callback.

What's the corresponding callback?


> > +               dev_err_probe(&pdev->dev, PTR_ERR(pctrl->pctldev),
> > +                             "Failed to register pinctrl device\n");
> > +               return PTR_ERR(pctrl->pctldev);
> 
> 
> You may combine those two in one return statement.

Good catch, will do.


Thanks for your review,
Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]