Re: Subject: [PATCH 00/22] gpio: xilinx: update driver to match official xilinx version

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Sun, Aug 5, 2018 at 10:06 PM Alexander Hedges <ahedges@xxxxxxx> wrote:
> On So, Jul 22, 2018 at 9:41 PM, Linus Walleij
> <linus.walleij@xxxxxxxxxx> wrote:
> > After looking at the patch set again I think the following is going to
> > be simplest
> > to proceed:
> >
> > 1. Look carefully at drivers like gpio-ftgpio010.c or gpio-pl061.c
> > 2. Rewrite the driver to use GPIO_GENERIC instead of the "mm"
> >     stuff.
> > 3. Use GPIOLIB_IRQCHIP and implement IRQ support.
> > 4. Think about how to handle the dual GPIO.
> >
> > Yours,
> > Linus Walleij
>
> Ok, having looked at some current GPIO implementations, it is still not
> quite clear to me how to best support dual GPIOs.
>
> Right now, I see the following two options:
> * Using two gpio_chip structs (one for each channel). This would be the
> most  straigt forward solution but I don't see how it should work to
> register two gpio_chips for a given device.

The relationship is one-to-many so you can register a hundred
GPIO chips to the same Linux device if you like.
Most subsystems are structured that way, the fact that most
of them only register one Linux framework thing per device
is just reflecting that this is the most usual case.

If there is reason to register two different chips to the same
device, by all means register two chips.

> Also I don't know where address
> translation
>   happens (where I could choose which of the gpio_chips to pass in).

I don't understand this. What address translation?

If you use GPIO_GENERIC what you pass in to bgpio_init()
is just a bunch of __iomem addresses. The translation from
physical to virtual address is done in probe() if that is what
you ask.

> * Rolling your own solution. This would essentially be not using
> bgpio_init()
>   and instead storing enough information in the struct to be able to
> identify
>   the channel based on the gpio number. This is what is currently done,
> but
>   as you said, it should be rewritten to *reuse* already implemented
> stuff.

Yes please.

>   Sadly, the gpio-xilinx is currently one out of three devices where
>   #gpio-cells is allowed to be larger than 2, so rather an exception.

The bindings says
Documentation/devicetree/bindings/gpio/gpio-xilinx.txt:

Required properties:
- compatible : Should be "xlnx,xps-gpio-1.00.a"
- reg : Address and length of the register set for the device
- #gpio-cells : Should be two. The first cell is the pin number and the
  second cell is used to specify optional parameters (currently unused).

I do not see it being other than two, which is standard.

> In the long term, adding multi-channel support to gpio-mmio.c would be
> nice.

I don't know what multi-channel would be, but please be more
specific, it sounds interesting.

BTW this:

- xlnx,tri-default : if n-th bit is 1, GPIO-n is in tristate mode
- xlnx,tri-default-2 : as above but for the second channel

This indicates that the driver should implement .set_config()
and enable the open drain setting, that seems to be what it means.
see gpio-lp873x.c etc for examples. This was the consumer
phandles can specify GPIO_OPEN_SOURCE as flag in the
device tree.

These two custom properties that are instead hacking it
staticallt into the producer should be deprecated.

I did merge these bindings but at the time (2013) we had no
way to specify open drain on the consumer side, but now
we do!

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html