Re: [PATCH] gpio: add ETRAXFS GPIO driver

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Thu, May 21, 2015 at 8:48 PM, Rabin Vincent <rabin@xxxxxx> wrote:
> On Tue, May 19, 2015 at 11:39:01AM +0200, Linus Walleij wrote:

>> Three cells is rather unusual, is it the best arrangement?
>>
>> Usually it's just offset+flags (your flags are ununsed I see).
>> And then you could divide offset by num gpios per bank
>> (I guess 32) in the driver to get bank number.
>
> At least to me, this:
>
> +       i2c {
> +               compatible = "i2c-gpio";
> +               gpios = <&gio 0xD 5 0>, <&gio 0xD 6 0>;
> +               i2c-gpio,delay-us = <2>;
>
> which immediately shows that it's port D pins 5 and 6 which are being used, and
> which matches the naming in the schematics and the chip documentation is
> clearly preferable to this:
>
> +               gpios = <&gio 101 0>, <&gio 102 0>;
>
> which uses made up numbers with no relation to any documentation and which
> probably requires the use of a calculator to determine if the correct
> pins are being used.

Sure I buy this ... just need to push back some to not get too
many deviant DT bindings :/

There is also the code such as of_gpio_simple_xlate()
that can't be reused for this, so thus it needs its own xlate
function and adds some complexity to the code.
But the convention in of_gpio_simple_xlate() is that
cell 0 is offset, and cell 1 is flags, so what about
moving the bank number to the last argument so you
can still use of_gpio_simple_xlate()?

Like so:
gpios = <&gio 5 0 0xD>, <&gio 6 0 0xD>;

I.e. extra cells go at the end. I can see you have a check
hack to see it is hitting a valid GPIO chip by using the label,
but that doesn't seem totally necessary.

Maybe we should even document this as a preferred binding
for "one IP with many banks inside it" use cases so we can
use that going forward?

> (btw, the ports have varying numbers of GPIOs and none of them have 32).

How typical.

> The binding in the patch matches the hardware.  The hardware is
> described as one IP with several ports and not several instances of the
> same IP.  The registers are also just 3 per port in the same region.
> Creating one instance of the device for handling each port, seems
> like useless overhead at best and, because it doesn't even match how the
> hardware looks like, quite wrong anyway.

OK.

> Only port A has interrupt support; this is not implemented in the
> current driver.

OK.

> BTW, the documentation for the chip is available here (GIO starts at
> page 647 and its registers at page 895):
> http://www.axis.com/files/manuals/etrax_fs_des_ref-070821.pdf

Ah I see it has pin multiplexing in front of the GPIO controller
block too. The driver may need  pinctrl_request_gpio()/
pinctrl_free_gpio() etc the day there is a standard pin control
driver in the back end of it. But no hurry with that.

>> > +struct etraxfs_gpio_port {
>> > +       const char *label;
>> > +       unsigned int oe;
>> > +       unsigned int dout;
>> > +       unsigned int din;
>>
>> consider using u32 for these.
>
> Why?  These are just offsets to the base address so there's no reason
> they _have_ to be 32 bits so u32 seems semantically wrong.

Ah I thought it was register shadows or something,
offsets are OK. Sorry.

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