On Thu, May 21, 2015 at 8:48 PM, Rabin Vincent [off-list ref] wrote:

On Tue, May 19, 2015 at 11:39:01AM +0200, Linus Walleij wrote:

quoted

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.

quoted

quoted

+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