Hi,
On Wed, Dec 08, 2021 at 01:24:18PM +0200, Andy Shevchenko wrote:
> On Tuesday, December 7, 2021, 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.
> >
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@xxxxxxx>
> > ---
[...]
> > +
> > +#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, ®, 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:
signature.asc
Description: PGP signature
