Re: [PATCH] gpio: Emma Mobile GPIO driver

[Arnd Bergmann]

On Friday 11 May 2012, Magnus Damm wrote:
> On Fri, May 11, 2012 at 10:10 PM, Linus Walleij
> <linus.walleij@xxxxxxxxxx> wrote:
> > On Fri, May 11, 2012 at 11:41 AM, Magnus Damm <magnus.damm@xxxxxxxxx> wrote:
> >
> >> +config GPIO_EM
> >> +       tristate "Emma Mobile GPIO"
> >> +       depends on ARM
> >
> > ARM really? Why should all ARM systems have the option of configuring in an Emma
> > controller?
> 
> Well, first of all, the Emma Mobile line of SoCs are ARM based. You
> may think of MIPS based Emma devices. I don' t know so much about
> them, but if you happen to know where I can find docs then I'd be very
> interested in looking at them to see if I can share any of my recently
> developed drivers.
> 
> Not sure if your point is that this has nothing to do with the CPU
> core itself - at least I usually prefer to omit CPU dependencies for
> I/O devices and have as few dependencies as possible to get as much as
> compile time testing done. In this particular case I've added ARM as
> dependency because I was asked so for the UART driver 8250_em. Either
> way is fine with me, so it's up to you GPIO maintainers to decide what
> you want. As long as I can enable it on the ARM based SoC family I'm
> happy. =)

My preference is generally to define the dependencies rather broadly
so you can build each driver on as many platforms as possible, and
just leave them out of defconfig. This seems to be the common way to
do things. Limiting by architecture makes some sense if you don't
want to expose the driver to all architectures that might not be
able to build it (e.g. s390 would not be able to build this one).

> >> +static inline unsigned long em_gio_read(struct em_gio_priv *p, int offs)
> >> +{
> >> +       if (offs < GIO_IDT0)
> >> +               return ioread32(p->base0 + offs);
> >> +       else
> >> +               return ioread32(p->base1 + (offs - GIO_IDT0));
> >> +}
> >> +
> >> +static inline void em_gio_write(struct em_gio_priv *p, int offs,
> >> +                               unsigned long value)
> >> +{
> >> +       if (offs < GIO_IDT0)
> >> +               iowrite32(value, p->base0 + offs);
> >> +       else
> >> +               iowrite32(value, p->base1 + (offs - GIO_IDT0));
> >> +}
> >
> > Isn't ioread()/iowrite() some ISA I/O-space operations? What's wrong with
> > readl_[relaxed] and writel_[relaxed]()?
> 
> Nothing wrong with readl/writel, I've just have a habit of using the
> ioread/iowrite ones. I find the code that allows platforms to select
> PORT or IOMEM in lib/iomap.c rather neat.

For reference: The ioread family is defined to be a superset of the
readl family and the inl family and to be compatible with the calling
convention of readl, so it's correct to use it here, but it may be
slower on architectures that require the lib/iomap.c wrappers. ARM
does not use those wrappers on modern systems because the PIO space
is memory mapped.

For performance sensitive drivers that do not use DMA, it makes
sense to use the readl_relaxed() family instead, because those
omit the expensive barriers that protect us from overlapping with
DMA.

The main advantage of using readl/writel is that it's more common
on ARM so reviewers don't have to wonder why this driver does things
differently. Any driver shared with powerpc, mips or sh would likely
use a different style. E.g. on powerpc, using readl/writel is considered
wrong because it's only well-defined for PCI there.

> >> +static struct em_gio_priv *irq_to_priv(unsigned int irq)
> >> +{
> >> +       struct irq_chip *chip = irq_get_chip(irq);
> >> +       return container_of(chip, struct em_gio_priv, irq_chip);
> >> +}
> >
> > Should this be inline maybe?
> 
> I have not checked if they actually turn out inline with my ARM
> compiler, but gcc for SH was always rather good at deciding when to
> inline by itself. Its a bit like likely()/unlikely() in my mind, it
> may be good to give hints for the compiler but in the majority of the
> cases the compiler will be fine by itself. I guess this particular
> function may end up as just a few instructions.

Right.

> >> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
> >> +{
> >> +       struct em_gio_priv *p = dev_id;
> >> +       unsigned long tmp, status;
> >> +       unsigned int offset;
> >> +
> >> +       status = tmp = em_gio_read(p, GIO_MST);
> >> +       if (!status)
> >> +               return IRQ_NONE;
> >> +
> >> +       while (status) {
> >> +               offset = __ffs(status);
> >> +               generic_handle_irq(p->irq_base + offset);
> >
> > Pleas use irqdomain to translate the IRQ numbers.
> 
> Is this mandatory for regular platform devices not using DT?
> 
> I don't object to your idea, but I was planning on adding that in my
> DT feature patch.

There is little value in delaying this if you already know how
to do it. irq domains are useful for other reasons as well, e.g.
for allowing to build with sparse IRQ, which is needed when we get
to multiplatform kernels.

> >> +static struct em_gio_priv *gpio_to_priv(struct gpio_chip *chip)
> >> +{
> >> +       return container_of(chip, struct em_gio_priv, gpio_chip);
> >> +}
> >
> > inline?
> 
> Sure, if you think that is absolutely necessary. May I ask, is it
> important for you, or is it ok if I skip and keep this driver in line
> with my other ones? =)

I don't think it makes any difference in practice, other for people
reading the code. Adding the 'inline' tells the reader that this is
meant to be a trivial function that doesn't add much object code,
and that the author was aware of that. I don't think it matters much
either way.

> > Isn't there a new nice inline that will both request and
> > remap a piece of memory?
> 
> If so then that would be excellent. Especially together with
> ioread/iowrite so the code can work both for IOMEM and PORT
> transparently.

We have a bunch of helpers, but I think none that does
everything yet. io_iomap() locates and remaps the resource.
devm_request_and_ioremap() does the request and the map, but
doesn't pull it out of the device.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/