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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <54B5532F.7050107@broadcom.com>
Date:	Tue, 13 Jan 2015 09:17:35 -0800
From:	Ray Jui <rjui@...adcom.com>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	Sherman Yin <syin@...adcom.com>, Simon Arlott <simon@...e.lp0.eu>,
	"Chris Boot" <bootc@...tc.net>,
	Stephen Warren <swarren@...dotorg.org>,
	Sören Brinkmann <soren.brinkmann@...inx.com>,
	"Grant Likely" <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>,
	"Scott Branden" <sbranden@...adcom.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Fengguang Wu <fengguang.wu@...el.com>
Subject: Re: [PATCH 2/4] pinctrl: cygnus: add initial pinctrl support



On 1/13/2015 12:25 AM, Linus Walleij wrote:
> On Fri, Jan 9, 2015 at 7:38 PM, Ray Jui <rjui@...adcom.com> wrote:
>> On 1/9/2015 3:03 AM, Linus Walleij wrote:
>>> On Fri, Nov 28, 2014 at 12:46 AM, Ray Jui <rjui@...adcom.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...
> 
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/