Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Arnd Bergmann <arnd@arndb.de>, Tim Harvey <tharvey@gateworks.com>,
	Krzysztof Halasa <khalasa@piap.pl>,
	Olof Johansson <olof@lixom.net>, Imre Kaloz <kaloz@openwrt.org>,
	arm-soc <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 05/17 v1] gpio: ixp4xx: Add driver for the IXP4xx GPIO
Date: Thu, 21 Feb 2019 09:50:45 +0100
Message-ID: <CACRpkdYiLPCeLGmynPeroGD9mKeRKKbA5u-1AH0+-yOyQxZ8hQ@mail.gmail.com> (raw)
In-Reply-To: <CAMpxmJWRH9+fbwyhjHe-ERjQfMCXTGzOLWeq1HJbbYXsZ14nXA@mail.gmail.com>

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

  reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-03 21:41 [PATCH 00/17 v1] ARM: ixp4xx: Modernize and DT support Linus Walleij
2019-02-03 21:41 ` [PATCH 01/17 v1] ARM: ixp4xx: Convert to MULTI_IRQ_HANDLER Linus Walleij
2019-02-03 21:41 ` [PATCH 02/17 v1] ARM: ixp4xx: Pass IRQ resource to beeper Linus Walleij
2019-02-03 21:41 ` [PATCH 03/17 v1] ARM: ixp4xx: Convert to SPARSE_IRQ Linus Walleij
2019-02-03 21:41 ` [PATCH 04/17 v1] irqchip: Add driver for IXP4xx Linus Walleij
2019-02-11 15:30   ` Marc Zyngier
2019-02-11 20:58     ` Linus Walleij
2019-02-11 22:11       ` Marc Zyngier
2019-02-18  7:06         ` khalasa
2019-02-18  7:16           ` Linus Walleij
2019-02-18  7:35             ` khalasa
2019-02-18  9:40             ` Arnd Bergmann
2019-02-18 12:03               ` khalasa
2019-02-18 12:44                 ` Arnd Bergmann
2019-02-19  6:51                   ` khalasa
2019-02-19  9:46                     ` Arnd Bergmann
2019-02-20  7:35                       ` khalasa
2019-02-18  9:18         ` Arnd Bergmann
2019-02-03 21:41 ` [PATCH 05/17 v1] gpio: ixp4xx: Add driver for the IXP4xx GPIO Linus Walleij
2019-02-06 16:03   ` Bartosz Golaszewski
2019-02-21  8:50     ` Linus Walleij [this message]
2019-02-03 21:41 ` [PATCH 06/17 v1] ARM: ixp4xx: Switch to use new IRQ+GPIO drivers Linus Walleij
2019-02-03 21:41 ` [PATCH 07/17 v1] clocksource/drivers/ixp4xx: Add driver Linus Walleij
2019-02-03 21:41 ` [PATCH 08/17 v1] ARM: ixp4xx: Switch to use new timer driver Linus Walleij
2019-02-03 21:41 ` [PATCH 09/17 v1] irqchip: ixp4xx: Add DT bindings Linus Walleij
2019-02-18 21:25   ` Rob Herring
2019-02-03 21:41 ` [PATCH 10/17 v1] irqchip: ixp4xx: Add OF initialization support Linus Walleij
2019-02-03 21:41 ` [PATCH 11/17 v1] clocksource/drivers/ixp4xx: Add DT bindings Linus Walleij
2019-02-18 21:26   ` Rob Herring
2019-02-18 22:10     ` Daniel Lezcano
2019-02-03 21:42 ` [PATCH 12/17 v1] clocksource/drivers/ixp4xx: Add OF initialization support Linus Walleij
2019-02-11 11:26   ` Daniel Lezcano
2019-02-03 21:42 ` [PATCH 13/17 v1] gpio: ixp4xx: Add DT bindings Linus Walleij
2019-02-06 16:05   ` Bartosz Golaszewski
2019-02-18 21:27   ` Rob Herring
2019-02-03 21:42 ` [PATCH 14/17 v1] gpio: ixp4xx: Add OF probing support Linus Walleij
2019-02-06 16:13   ` Bartosz Golaszewski
2019-02-21  8:55     ` Linus Walleij
2019-02-03 21:42 ` [PATCH 15/17 v1] ARM: ixp4xx: Add DT bindings Linus Walleij
2019-02-04 15:16   ` Rob Herring
2019-02-08 19:37     ` Linus Walleij
2019-02-03 21:42 ` [PATCH 16/17 v1] ARM: ixp4xx: Add device tree boot support Linus Walleij
2019-02-03 21:42 ` [PATCH 17/17 v1] RFC: ARM: dts: Add some initial IXP4xx device trees Linus Walleij

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACRpkdYiLPCeLGmynPeroGD9mKeRKKbA5u-1AH0+-yOyQxZ8hQ@mail.gmail.com \
    --to=linus.walleij@linaro.org \
    --cc=arnd@arndb.de \
    --cc=bgolaszewski@baylibre.com \
    --cc=kaloz@openwrt.org \
    --cc=khalasa@piap.pl \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=olof@lixom.net \
    --cc=tharvey@gateworks.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git