lkml.org 
[lkml]   [2015]   [Jun]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] gpio: add ETRAXFS GPIO driver
    From
    On Thu, May 21, 2015 at 8:48 PM, Rabin Vincent <rabin@rab.in> 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


    \
     
     \ /
      Last update: 2015-06-01 16:01    [W:3.617 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site