From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tony Lindgren Subject: Re: [PATCH] pinctrl: Add one-register-per-pin type device tree based pinctrl driver Date: Fri, 22 Jun 2012 01:39:56 -0700 Message-ID: <20120622083955.GZ12766@atomide.com> References: <20120611135826.GB12766@atomide.com> <4FDA6FCD.1020605@wwwdotorg.org> <20120615094938.GQ12766@atomide.com> <4FDB6000.5080703@wwwdotorg.org> <20120618055036.GU12766@atomide.com> <20120619135600.GX12766@atomide.com> <4FE39C86.5070901@wwwdotorg.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <4FE39C86.5070901-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: "devicetree-discuss" To: Stephen Warren Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring , linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org Hi, * Stephen Warren [120621 15:17]: > On 06/19/2012 07:56 AM, Tony Lindgren wrote: > > + > > +- pinctrl-single,pinconf-mask : mask of allowed pinconf bits in the > > + pinmux register; this gets combined with pinconf mask but is a separate > > + mask to allow the option of setting pinconf separatately from the > > + function > > Given that this binding doesn't allow describing pin configuration at > present, I would simply remove all mention of that property in the > binding documentation. It can be added back if/when that feature is > added. Any future driver using this binding can refuse to allow pin > configuration if that property is missing. It might be better to just add support for pin_config_get/set to avoid changing the binding later: static int pcs_pinconf_get(struct pinctrl_dev *pctldev, unsigned pin, unsigned long *config) { - return -ENOTSUPP; + struct pcs_device *pcs; + void __iomem *reg; + int res; + + pcs = pinctrl_dev_get_drvdata(pctldev); + res = pcs_pin_to_reg(pcs, pin, ®); + if (res) + return res; + + return pcs->read(reg) & pcs->cmask; } A have not done that yet as currently the pcs_pin_to_reg() would need to be sorted out in somewhat clean manner. > > +- pinctrl-single,function-off : function off mode for disabled state if > > + available and same for all registers; if not, use a value larger than > > + function-mask to ignore disabling of registers > > Rather than requiring an invalid value in this property, shouldn't the > lack of a valid function-off value be represented by the property not > being present in the DT? Heh good point :) Will change. > > +This driver assumes that there is only one register for each pin, > > +and uses the common pinctrl bindings as specified in the pinctrl-bindings.txt > > +document in this directory. > > At this point in the file, I think you need to mention that you're > switching from describing the top-level device node to describing pin > configuration nodes. Will add. > > +The pinctrl register offsets and default values are specified as pairs > > I thought we were going to remove "default" here? Oops, looks like one was left, will remove. > > +using pinctrl-single,pins. For example, setting a pin for a device > > +could be done with: > > + > > + pinctrl-single,pins = <0xdc 0x118>; > > + > > +Where 0xdc is the offset from the pinctrl register base address for the > > +device pinctrl register, and 0x118 contains the desired value of the > > +pinctrl register. See the device example and static board pins example > > +below for more information. > > There should be some explanation only the portion of this value covered > by the pinctrl-single,function-mask value is updated in the register. OK > > +This driver tries to avoid understanding pin and function names because of > > +the extra bloat they would cause especially in the case of a large number > > +of pins. This driver just sets what is specified for the board in the .dts file. > > +Further user space debugging tools can be developed to decipher the pin and > > +function names using debugfs. > > There shouldn't be any discussion of a driver here; the binding is a HW > description. Will move that to the driver comments. > > +Example: > > I only reviewed the binding document, not the code. Thanks, Tony