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

[Ray Jui]

On 1/13/2015 12:25 AM, Linus Walleij wrote:
> On Fri, Jan 9, 2015 at 7:38 PM, Ray Jui <rjui@xxxxxxxxxxxx> wrote:
>> On 1/9/2015 3:03 AM, Linus Walleij wrote:
>>> On Fri, Nov 28, 2014 at 12:46 AM, Ray Jui <rjui@xxxxxxxxxxxx> 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...
> 
Okay will check the coh901 driver.

>>>> +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.
> 
Thanks.

>>>> +       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.
> 
Okay.

> Yours,
> Linus Walleij
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/