Re: [PATCH V4 2/3] pincntrl: add support for AMS AS3722 pin control driver

[Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On Friday 27 September 2013 06:10 PM, Linus Walleij wrote:
> On Tue, Sep 24, 2013 at 7:47 PM, Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
>> function which just return not supported.
>> - Nit cleanups.
> This is getting a lot better quickly.
>
> I'd like someone from devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org to say
> something about the DT bindings, can you convince one of
> the DT custodians to ACK this?

I will request Stephen to review this.

> Remaining issues though:
>
>> +This binding uses the following generic properties as defined in
>> +pinctrl-bindings.txt:
>> +
>> +Required: pins
> You're not explaining what this property does.
>
> Normally the set of pins are defined by the driver, not by the
> device tree, since it's a property of the hardware, i.e. the
> driver defines what hardware we have, and the DT defines
> only how to set it up to do what we want to do.

pins are defined in the pinctrl-bindings and hence not duplicated here.

But some where we need to document that what are valid pin names which 
driver entertain.
I have referred the other dt document and I derived this from there.


>
>> +Example:
>> +       as3722: as3722 {
>> +               ....
>> +               gpio-controller;
>> +               #gpio-cells = <2>;
>> +
>> +               pinctrl-names = "default";
>> +               pinctrl-0 = <&as3722_default>;
>> +
>> +               as3722_default: pinmux {
>> +                       gpio0 {
>> +                               pins = "gpio0";
>> +                               function = "gpio";
>> +                               bias-pull-down;
>> +                       };
>> +
>> +                       gpio1_2_4_7 {
>> +                               pins = "gpio1", "gpio2", "gpio4", "gpio7";
>> +                               function = "gpio";
>> +                               bias-pull-up;
>> +                       };
>> +
>> +                       gpio5 {
>> +                               pins = "gpio5";
>> +                               function = "clk32k_out";
>> +                       };
>> +               };
>
> As you see the name "gpio0" thru "gpio5" is quite confusing
> here, as it is referring to pins, not GPIO lines (which is something
> else in Linux).
>
> I guess you know for sure whatever it is, and if the hardware
> manual names the pins like this then there is not much we can
> do. On most chips the actual pins have better names, like
> chessboard coordinates, "D1" etc on BGAs or other good names.
>
> Can you just inspect your HW docs and verify that your pins
> really have these confusing names?
>
> So it's this:


The pin diagram in datasheet of the device names these pins as GPIO0..GPIO7.
So hardly we can do here.
For GPIO3 on BGA124, it is E4 but CSP108, it is C9 so the other names 
defined on package.
But both places, it is said as GPIO3.

>
> +static const char * const gpio_groups[] = {
> +       "gpio0",
> +       "gpio1",
> +       "gpio2",
> +       "gpio3",
> +       "gpio4",
> +       "gpio5",
> +       "gpio6",
> +       "gpio7",
> +};
> That is really, really confusing.
>
> And this one-pin-per-group:

Yaah, typical device, all io function available on all GPIO pins.

>> +static int as3722_gpio_direction_output(struct gpio_chip *chip,
>> +               unsigned offset, int value)
>> +{
>> +       as3722_gpio_set(chip, offset, value);
>> +       return pinctrl_gpio_direction_output(chip->base + offset);
>> +}
> Son't you want to set the direction first, then output the value?
> This order is OK with some hardware, I'm just asking...

Avoid glitches, we need to set output value first and the output mode.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html