[PATCH 2/4] pinctrl: cygnus: add initial pinctrl support

[Linus Walleij]

On Fri, Jan 9, 2015 at 7:38 PM, Ray Jui <rjui at broadcom.com> wrote:
> On 1/9/2015 3:03 AM, Linus Walleij wrote:
>> On Fri, Nov 28, 2014 at 12:46 AM, Ray Jui <rjui at broadcom.com> wrote:

>> I don't know if the hardware has any similarity though, so invite
>> the authors of the previous drivers to review this code.
>>
> They are completely different. The only similarity between Cygnus and
> bcm281xx pinctrl is that they use the same concept of alternation
> functions (1, 2, 3, 4) for mux configuration.

Then you can probably look at that driver for inspiration on how to handle
the situation you described earlier with collissions.

>>> +/*
>>> + * List of groups of pins
>>> + */
>>> +static const unsigned gpio0_pins[] = { 12 };
>>> +static const unsigned gpio1_pins[] = { 13 };
>>> +static const unsigned gpio2_pins[] = { 14 };
>>> +static const unsigned gpio3_pins[] = { 15 };
>>> +static const unsigned gpio4_pins[] = { 16 };
>>> +static const unsigned gpio5_pins[] = { 17 };
>>> +static const unsigned gpio6_pins[] = { 18 };
>>> +static const unsigned gpio7_pins[] = { 19 };
>>> +static const unsigned gpio8_pins[] = { 20 };
>>> +static const unsigned gpio9_pins[] = { 21 };
>>> +static const unsigned gpio10_pins[] = { 22 };
>>> +static const unsigned gpio11_pins[] = { 23 };
>>> +static const unsigned gpio12_pins[] = { 24 };
>>> +static const unsigned gpio13_pins[] = { 25 };
>>> +static const unsigned gpio14_pins[] = { 26 };
>>> +static const unsigned gpio15_pins[] = { 27 };
>>> +static const unsigned gpio16_pins[] = { 28 };
>>> +static const unsigned gpio17_pins[] = { 29 };
>>> +static const unsigned gpio18_pins[] = { 30 };
>>> +static const unsigned gpio19_pins[] = { 31 };
>>> +static const unsigned gpio20_pins[] = { 32 };
>>> +static const unsigned gpio21_pins[] = { 33 };
>>> +static const unsigned gpio22_pins[] = { 34 };
>>> +static const unsigned gpio23_pins[] = { 35 };
>>
>> Have you considered implementing .gpio_request_enable()
>> and .gpio_disable_free() to get around having to have one
>> group for each GPIO line?
>>
> Okay the Cygnus pin controller is really a mess. GPIO 0 ~ GPIO23 are
> really 23 distinct groups, each with one pin. Then the rest of GPIOs go
> under other groups. In general, when we set a group to alternate
> function 4, all pins become GPIO.

It will require some complicated code no matter how you
handle it I'm afraid. Rely on the pin control subsystem
to handle collisions though.

>>> +static const unsigned qspi_gpio_pins[] = { 108, 109 };
>>> +static const unsigned smart_card0_fcb_pins[] = { 45 };
>>> +static const unsigned smart_card1_fcb_pins[] = { 51 };
>>> +static const unsigned gpio0_3p3_pins[] = { 176 };
>>> +static const unsigned gpio1_3p3_pins[] = { 177 };
>>> +static const unsigned gpio2_3p3_pins[] = { 178 };
>>
>> Looks good...
>>
> Note these pins are definitions in the driver that help to describe the
> pad layout. We can't really configure any individual pins in Cygnus.

Yeah it's a groupwise controller then, that's similar to
e.g. the coh901 driver.

We should be able to accomodate this...

>>> +static int cygnus_dt_node_to_map(struct pinctrl_dev *pctrl_dev,
>>> +               struct device_node *np, struct pinctrl_map **map,
>>> +               unsigned *num_maps)
>>> +{
>>
>> After Sören Brinkmanns patches youy should be able to use core
>> functions for this and avoid this code altogether.
>
> Will that help to take care our case, based on the way we will use
> "function" and "group"?

groupS but yes it will work with your controller, though I think
.set_mux and the controller state will need some elaborate code
to handle what the framework requests.

>>> +       num_groups = of_property_count_strings(np, "brcm,groups");
>>
>> As mentioned, just "groups".
>>
> I guess I will use "group"?

No groups, as with the standard attribute "gpios", this may be
a single group too, it's just a standard binding.

Yours,
Linus Walleij