| Message ID | 20211207210823.1975632-1-j.neuschaefer@gmx.net |
|---|---|
| Headers | show |
| Series | Nuvoton WPCM450 pinctrl and GPIO driver | expand |
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, ®, 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
On Wed, Dec 8, 2021 at 3:58 PM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > 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: ... > > > + 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. It is still better to use in my opinion. > 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... Maybe, I dunno since I haven't clearly got this code anyway, so up to you to check and see what to do about it. ... > > > + 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. Just a convention "de facto". ... > > > + 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? https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/driver.h#L400
On Wed, Dec 08, 2021 at 05:58:30AM PST, Jonathan Neuschäfer wrote: >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> > ><snip> > >> > +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... > There's GENMASK(), though it takes a high bit and low bit rather than a bit position and count, so it'd require a small bit of arithmetic, e.g. lastbit = gpio->first_irq_bit + gpio->num_irqs - 1; ours = GENMASK(lastbit, gpio->first_irq_bit); or a manual shift: ours = GENMASK(gpio->num_irqs - 1, 0) << gpio->first_irq_bit; (I don't have any terribly strong opinions on which of these is best, personally.) Zev
On Thu, Dec 9, 2021 at 9:26 AM Zev Weiss <zweiss@equinix.com> wrote: > On Wed, Dec 08, 2021 at 05:58:30AM PST, Jonathan Neuschäfer wrote: > >> 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... > > > > There's GENMASK(), though it takes a high bit and low bit rather than a > bit position and count, so it'd require a small bit of arithmetic, e.g. > > lastbit = gpio->first_irq_bit + gpio->num_irqs - 1; > ours = GENMASK(lastbit, gpio->first_irq_bit); > > or a manual shift: > > ours = GENMASK(gpio->num_irqs - 1, 0) << gpio->first_irq_bit; I think this can be handled with FIELD_PREP() from <linux/bitfield.h>? Some examples at the top of the header. Yours, Linus Walleij
On Tue, Dec 7, 2021 at 10:08 PM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > This patch adds the pin controller and GPIO banks to the devicetree for the > WPCM450 SoC. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> This looks good. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, Dec 08, 2021 at 04:14:38PM +0200, Andy Shevchenko wrote: > On Wed, Dec 8, 2021 at 3:58 PM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > > 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: > > ... > > > > > + 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. > > It is still better to use in my opinion. Ok. > > > 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... > > Maybe, I dunno since I haven't clearly got this code anyway, so up to > you to check and see what to do about it. Right, I'll evaluate my options and come up with something. > ... > > > > > + 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. > > Just a convention "de facto". Ok, I'll change it. > > > > + 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? > > https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/driver.h#L400 Thanks. Best regards, Jonathan Neuschäfer
On Fri, Dec 10, 2021 at 02:41:45AM +0100, Linus Walleij wrote: > On Thu, Dec 9, 2021 at 9:26 AM Zev Weiss <zweiss@equinix.com> wrote: > > On Wed, Dec 08, 2021 at 05:58:30AM PST, Jonathan Neuschäfer wrote: > > > >> 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... > > > > > > > There's GENMASK(), though it takes a high bit and low bit rather than a > > bit position and count, so it'd require a small bit of arithmetic, e.g. > > > > lastbit = gpio->first_irq_bit + gpio->num_irqs - 1; > > ours = GENMASK(lastbit, gpio->first_irq_bit); > > > > or a manual shift: > > > > ours = GENMASK(gpio->num_irqs - 1, 0) << gpio->first_irq_bit; > > I think this can be handled with FIELD_PREP() from > <linux/bitfield.h>? Some examples at the top of the > header. Thank you both! Best regards, Jonathan