Hi,

On Wed, Dec 08, 2021 at 01:24:18PM +0200, Andy Shevchenko wrote:

On Tuesday, December 7, 2021, Jonathan Neuschäfer [off-list ref]
wrote:

quoted

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>
---

[...]

quoted

+
+#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.

quoted

+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.

quoted

+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.

quoted

+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.

quoted

+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...

quoted

+       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.

quoted

+       }
+       chained_irq_exit(chip, desc);

quoted

+               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.

quoted

+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.

quoted

+
+       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.

quoted

+       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.

quoted

+               gpio->gc.of_node = to_of_node(np);

I hope we will soon have fwnode in gpio_chip.

Yes, that would be good.

quoted

+               gpio->gc.parent = pctrl->dev;

Set by bgpio_init(), also check for other potential duplications.

Good catch, I'll check the assignments again.

quoted

+               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?

quoted

+               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