linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thomas Abraham <thomas.abraham@linaro.org>
To: Stephen Warren <swarren@wwwdotorg.org>
Cc: linux-kernel@vger.kernel.org,
	devicetree-discuss@lists.ozlabs.org,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org, linus.walleij@linaro.org,
	grant.likely@secretlab.ca, rob.herring@calxeda.com,
	kgene.kim@samsung.com, dong.aisheng@linaro.org,
	patches@linaro.org
Subject: Re: [PATCH v3 1/4] pinctrl: add samsung pinctrl and gpiolib driver
Date: Fri, 24 Aug 2012 09:55:16 +0530	[thread overview]
Message-ID: <CAJuYYwQ2fGxVddWiaUGXQWfSUCw9LBBpYF=pKPFkU5RmZfpJRg@mail.gmail.com> (raw)
In-Reply-To: <5036B8EB.5050502@wwwdotorg.org>

On 24 August 2012 04:42, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/23/2012 05:15 AM, Thomas Abraham wrote:
>> Add a new device tree enabled pinctrl and gpiolib driver for Samsung
>> SoC's. This driver provides a common and extensible framework for all
>> Samsung SoC's to interface with the pinctrl and gpiolib subsystems. This
>> driver supports only device tree based instantiation and hence can be
>> used only on those Samsung platforms that have device tree enabled.
>>
>> This driver is split into two parts: the pinctrl interface and the gpiolib
>> interface. The pinctrl interface registers pinctrl devices with the pinctrl
>> subsystem and gpiolib interface registers gpio chips with the gpiolib
>> subsystem. The information about the pins, pin groups, pin functions and
>> gpio chips, which are SoC specific, are parsed from device tree node.
>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>
> BTW, this is a very nicely written and complete/precise binding
> document. Well done.

Thank you!

>
>> +Samsung GPIO and Pin Mux/Config controller
>> +
>> +Samsung's ARM based SoC's integrates a GPIO and Pin mux/config hardware
>> +controller. It controls the input/output settings on the available pads/pins
>> +and also provides ability to multiplex and configure the output of various
>> +on-chip controllers onto these pads.
>> +
>> +Required Properties:
>> +- compatible: should be one of the following.
>> +  - "samsung,pinctrl-exynos4210": for Exynos4210 compatible pin-controller.
>> +  - "samsung,pinctrl-exynos5250": for Exynos5250 compatible pin-controller.
>> +
>> +- reg: Base address of the pin controller hardware module and length of
>> +  the address space it occupies.
>> +
>> +- interrupts: interrupt specifier for the controller. The format and value of
>> +  the interrupt specifier depends on the interrupt parent for the controller.
>> +
>> +- Pin mux/config groups as child nodes: The pin mux (selecting pin function
>
> Direct child nodes of the pin-controller, not a second level?

The child nodes would be direct child nodes.

>
> While that's quite legal, it means that if you need a particular client
> module to use 4 pins, 2 of which need one samsung,pin-function value and
> 2 of which need a different pin-function value, then the client device's
> pinctrl-0 property has to have two entries.
>
> i.e. a completely hypothetical example roughly based on yours below:
>
>         pinctrl_1: pinctrl@11000000 {
>                 uart0_rxd: uart0-rxd {
>                         samsung,pins = "gpa0-0";
>                         samsung,pin-function = <2>;
>                         samsung,pin-pud = <0>;
>                         samsung,pin-drv = <0>;
>                 };
>
>                 uart0_txd: uart0-txd {
>                         samsung,pins = "gpa0-1";
>                         samsung,pin-function = <1>;
>                         samsung,pin-pud = <0>;
>                         samsung,pin-drv = <0>;
>                 };
>         };
>
>         uart@13800000 {
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&uart0_rxd &uart0_txd>;
>         };
>
> rather than:
>
>         pinctrl_1: pinctrl@11000000 {
>                 uart0_opt1: uart0-opt1 {
>                         uart0_rxd: uart0-rxd {
>                                 samsung,pins = "gpa0-0";
>                                 samsung,pin-function = <2>;
>                                 samsung,pin-pud = <0>;
>                                 samsung,pin-drv = <0>;
>                         };
>
>                         uart0_txd: uart0-txd {
>                                 samsung,pins = "gpa0-1";
>                                 samsung,pin-function = <1>;
>                                 samsung,pin-pud = <0>;
>                                 samsung,pin-drv = <0>;
>                         };
>                 };
>         };
>
>         uart@13800000 {
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&uart0_opt1;
>         };
>
> The latter layout simplifies writing the client nodes, since all the
> related settings can be grouped together by whoever writes the pinctrl
> node, rather than every client author having to work out all the entries
> to include in the list.
>
> That all said, the way you've defined the binding is perfectly
> legitimate, and I don't have any kind of issue with it; it's just
> something you might want to consider.

Thanks for suggesting this alternate method. I do agree with your
point. But, for now, I would prefer to stabilize this driver without
changing the dt parsing code and make it usable for client nodes. I
will revisit your suggested approach at a later point. I assume for
now that the author's of client nodes know which pin settings to
select.

>
> Irrespective of whether you choose to keep the binding as-is, or change
> it, please consider it:
>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>

Thanks.

>
>> +  The values specified by these config properties should be dervied from the
>
> s/dervied/derived/

Ok.

>
>> +External GPIO and Wakeup Interrupts:
>> +
>> +The controller supports two types of external interrupts over gpio. The first
>> +is the external gpio interrupt and second is the external wakeup interrupts.
>> +The difference between the two is that the external wakeup interrupts can be
>> +used as system wakeup events.
>> +
>> +A. External GPIO Interrupts: For supporting external gpio interrupts, the
>> +   properties should be specified in the pin-controller device node.
>
> s/the properties/the following properties/ ?

Ok.

>
>> +Aliases:
>> +
>> +All the pin controller nodes should be represented in the aliases node using
>> +the following format 'pinctrl{n}' where n is a unique number for the alias.
>
> There /should/ be an alias, or there /may/ be; I'm not sure why
> requiring or recommending an alias would be particularly important for
> this device?

The alias is required since the SoC data for a particular instance is
dependent on the instance number. And the instance number is derived
from the alias.

>
> I've only had time to review the binding document so far.

Ok. Thanks Stephen for your comments on this patch.

Regards,
Thomas.

  reply	other threads:[~2012-08-24  4:25 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-08-23 11:15 [PATCH v3 0/4] pinctrl: add support for samsung pinctrl driver Thomas Abraham
2012-08-23 11:15 ` [PATCH v3 1/4] pinctrl: add samsung pinctrl and gpiolib driver Thomas Abraham
2012-08-23 23:12   ` Stephen Warren
2012-08-24  4:25     ` Thomas Abraham [this message]
2012-09-03 11:14   ` Linus Walleij
2012-09-04 19:47     ` Thomas Abraham
2012-09-04 21:45       ` Kukjin Kim
2012-09-05  6:20         ` Thomas Abraham
2012-09-06 17:33           ` Stephen Warren
2012-09-06 22:03             ` Kukjin Kim
2012-09-05 13:50   ` Tomasz Figa
2012-09-05 15:19     ` Thomas Abraham
2012-08-23 11:15 ` [PATCH v3 2/4] pinctrl: add exynos4210 specific extensions for samsung pinctrl driver Thomas Abraham
2012-09-03 11:16   ` Linus Walleij
2012-08-23 11:15 ` [PATCH v3 3/4] gpio: exynos4: skip gpiolib registration if pinctrl driver is used Thomas Abraham
2012-08-23 11:15 ` [PATCH v3 4/4] ARM: EXYNOS: skip wakeup interrupt setup " Thomas Abraham
2012-09-03 11:17   ` Linus Walleij

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAJuYYwQ2fGxVddWiaUGXQWfSUCw9LBBpYF=pKPFkU5RmZfpJRg@mail.gmail.com' \
    --to=thomas.abraham@linaro.org \
    --cc=devicetree-discuss@lists.ozlabs.org \
    --cc=dong.aisheng@linaro.org \
    --cc=grant.likely@secretlab.ca \
    --cc=kgene.kim@samsung.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=patches@linaro.org \
    --cc=rob.herring@calxeda.com \
    --cc=swarren@wwwdotorg.org \
    --subject='Re: [PATCH v3 1/4] pinctrl: add samsung pinctrl and gpiolib driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).