Re: [PATCH 05/17 v1] gpio: ixp4xx: Add driver for the IXP4xx GPIO

[Linus Walleij <linus.walleij@linaro.org>]

Hi Bartosz,

thanks for the review!

On Wed, Feb 6, 2019 at 5:04 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> niedz., 3 lut 2019 o 22:42 Linus Walleij <linus.walleij@linaro.org> napisał(a):
(...)
> Just a nit, but I'd add a newline between the includes and the header.
> Also: C++ style comment for the header.

OK

> > +#include <linux/irqchip/irq-ixp4xx.h>
>
> I think a newline between linux/ and asm/ includes is a good rule.

OK

> > +#define IXP4XX_GPIO_GPOUTR     0x00
> > +#define IXP4XX_GPIO_GPOER      0x04
> > +#define IXP4XX_GPIO_GPINR      0x08
> > +#define IXP4XX_GPIO_GPISR      0x0C
> > +#define IXP4XX_GPIO_GPIT1R     0x10
> > +#define IXP4XX_GPIO_GPIT2R     0x14
> > +#define IXP4XX_GPIO_GPCLKR     0x18
> > +#define IXP4XX_GPIO_GPDBSELR   0x1C
>
> Since these are registers offsets - maybe the prefix could be
> IXP4XX_GPIO_REG_* or IXP4XX_GPIO_OFF_*? I think it's more readable.

OK I renamed them IXP4XX_REG_GPOUT etc.

> > +#define IXP4XX_GPIO_STYLE_MASK         0x7
>
> I'd use GENMASK(2, 0) here.

OK

> > +       /* Clear the style for the appropriate pin */
> > +       val = __raw_readl(g->base + int_reg);
> > +       val &= ~(IXP4XX_GPIO_STYLE_MASK << (line * IXP4XX_GPIO_STYLE_SIZE));
> > +       __raw_writel(val, g->base + int_reg);
>
> I know you're not using regmap, because this driver's based on
> gpio-mmio, but I'm wondering if you could maybe wrap those three
> operations in a helper wrapper e.g. ixp4xx_update_reg() or something
> to make it easier to read. Same below.

I'm uncertain about that, I feel that kind of stuff creates extra
runpaths that make the code hard to read, the compiler will
of course inline it, but I like the read-modify-write explicit instead
of wrapped in a function.

> > +static struct irq_chip ixp4xx_gpio_irqchip = {
> > +       .name = "IXP4GPIO",
> > +       .irq_ack = ixp4xx_gpio_irq_ack,
> > +       .irq_mask = irq_chip_mask_parent,
> > +       .irq_unmask = ixp4xx_gpio_irq_unmask,
> > +       .irq_set_type = ixp4xx_gpio_irq_set_type,
> > +};
>
> I assume this device cannot have multiple instances, so it's safe to
> have a static irqchip?

Yes there can be only one.

> > +static int ixp4xx_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +       struct ixp4xx_gpio *g = gpiochip_get_data(gc);
> > +       struct irq_fwspec fwspec;
> > +
> > +       fwspec.fwnode = g->fwnode;
> > +       fwspec.param_count = 2;
> > +       fwspec.param[0] = offset;
> > +       fwspec.param[1] = IRQ_TYPE_NONE;
> > +
> > +       return irq_create_fwspec_mapping(&fwspec);
> > +}
> > +
>
> This is were I start to struggle. I'm still not very well versed in
> irq domain hierarchies yet. You already explain the concept in the
> commit message, but it would be great if you could add a comment
> describing the technical details of implementation here and in other
> related callbacks. The whole fwspec thingy is not very obvious and it
> would serve as an example for the future.

I wrote a paragraph of explanations but I realized that I was just
writing documentation that should rather be in irqdomain.
My plan is to update the GPIO driver documentation with a
thorough description of hierarchical irqs instead.

> > +static int ixp4xx_gpio_irq_domain_translate(struct irq_domain *domain,
> > +                                           struct irq_fwspec *fwspec,
> > +                                           unsigned long *hwirq,
> > +                                           unsigned int *type)
> > +{
> > +
> > +       /* We support standard DT translation */
> > +       if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
> > +               *hwirq = fwspec->param[0];
> > +               *type = fwspec->param[1];
> > +               return 0;
> > +       }
>
> This logic is a bit non-intuitive - maybe you could first check for
> error conditions and then do any processing?

Brian has created a generic twocell translation helper in irqchip
for this, so I will refactor on top of that at some point and this
complexity goes away.

> Don't you need to dispose of the domain in the remove callback? They
> don't seem to have devm_ variants yet.

This is a bool Kconfig symbol so the driver is compiled into the kernel.
Once probed, the irqchip/domain never goes away. If I try to remove it
it would just take down the whole system with it, so even if I would
implement it it would make no sense. There is something of a
"hardness" in how tight an irqdomain/chip is coupled to a system,
and on this system it is (currently) used for a lot of static board files
and it just can't go away. (The opposite being e.g. a USB expander
gpiochip which should me a module and can come and go as it
please.)

We could reconsider once all is moved over to device tree and more
dynamic though.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel