lkml.org 
[lkml]   [2016]   [Feb]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] pinctrl: sirf/atlas7: stop poking around in GPIO internals
    From
    On Fri, Feb 19, 2016 at 4:21 AM, Barry Song <baohua@kernel.org> wrote:
    > 2016-02-19 4:42 GMT+08:00 Linus Walleij <linus.walleij@linaro.org>:
    >> On Thu, Feb 18, 2016 at 3:50 AM, Barry Song <baohua@kernel.org> wrote:
    >>
    >>> gpiochip_add(chip will call this automatically since range is set in dtsi:
    >>> gpio_0: gpio_mediam@17040000 {
    >>> #gpio-cells = <2>;
    >>> #interrupt-cells = <2>;
    >>> compatible = "sirf,atlas7-gpio";
    >>> reg = <0x17040000 0x1000>;
    >>> interrupts = <0 13 0>, <0 14 0>;
    >>> clocks = <&car 98>;
    >>> clock-names = "gpio0_io";
    >>> gpio-controller;
    >>> interrupt-controller;
    >>>
    >>> gpio-banks = <2>;
    >>> gpio-ranges = <&pinctrl 0 0 0>,
    >>> <&pinctrl 32 0 0>;
    >>> gpio-ranges-group-names = "lvds_gpio_grp",
    >>> "jtag_uart_nand_gpio_grp";
    >>
    >> Aha I see.
    >>
    >>>> -
    >>>> - /* Records gpio_pin_range to a7gc */
    >>>> - list_for_each_entry(pin_range, &chip->pin_ranges, node) {
    >>>> - struct pinctrl_gpio_range *range;
    >>>> -
    >>>> - range = &pin_range->range;
    >>>> - if (range->id == NGPIO_OF_BANK * idx) {
    >>>> - bank->gpio_offset = range->id;
    >>>> - bank->ngpio = range->npins;
    >>>> - bank->gpio_pins = range->pins;
    >>>> - bank->pctldev = pin_range->pctldev;
    >>>> - break;
    >>>> - }
    >>>> - }
    >>>> -
    >>>> - BUG_ON(!bank->pctldev);
    >>>
    >>> this doesn't work. my pin range is not continuous and linear, so we
    >>> need the "if (range->id == NGPIO_OF_BANK * idx)" to calculate the
    >>> gpio_offset.
    >>> and gpio_offset is used in many places:
    >>
    >> Can't you use:
    >>
    >> struct pinctrl_gpio_range *range =
    >> pinctrl_find_gpio_range_from_pin(pctldev, pin);
    >>
    >> In these places instead?
    >
    > this is why gpio_offset was involved to avoid doing complex
    > pinctrl_find_gpio_range_from_pin() everytime.
    > since the range mapping is not linear, gpio_offset is a cache to do that.

    OK I think I know what the problem is: you do this in your device
    tree:

    gpio-ranges = <&pinctrl 0 0 0>,
    <&pinctrl 32 0 0>,
    <&pinctrl 64 0 0>,
    <&pinctrl 96 0 0>;
    gpio-ranges-group-names = "gnss_gpio_grp",
    "lcd_vip_gpio_grp",
    "sdio_i2s_gpio_grp",
    "sp_rgmii_gpio_grp";
    };

    So these gpio-ranges will populate the ranges using the pins from
    these groups.

    The problem is: these ranges are a lie. They are incorrect, as you
    say yourself "pin range is not continous".

    But the definition of a pin range is that it is continous!

    So you have put erroneous data into the device tree and then
    you use code in the driver to fix up that erroneous data.

    So instead: fix up the ranges in the GPIO device tree node.

    You don't have to use group names to define the ranges,
    noone is forcing you to do that. Cut the whole
    gpio-ranges-group-names property and use just
    <&pinctrl 0 1 12>, <&pinctrl 12 14 4> etc as is needed
    to proper register the *real* ranges, instead of going in
    and augmenting the ranges in the kernel.

    Yours,
    Linus Walleij

    \
     
     \ /
      Last update: 2016-02-19 09:41    [W:4.470 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site