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.
next prev parent 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).