From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030401AbcJ0OMP (ORCPT ); Thu, 27 Oct 2016 10:12:15 -0400 Received: from muru.com ([72.249.23.125]:46222 "EHLO muru.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936559AbcJ0OLZ (ORCPT ); Thu, 27 Oct 2016 10:11:25 -0400 Date: Thu, 27 Oct 2016 07:11:20 -0700 From: Tony Lindgren To: Linus Walleij Cc: Rob Herring , "linux-kernel@vger.kernel.org" , Jon Hunter , Mark Rutland , Grygorii Strashko , Nishanth Menon , "linux-gpio@vger.kernel.org" , "devicetree@vger.kernel.org" , Linux-OMAP Subject: Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args Message-ID: <20161027141120.jqvul6q7iz5fjsmb@atomide.com> References: <20161025164538.453-1-tony@atomide.com> <20161025164538.453-2-tony@atomide.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.2-neo (2016-07-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Linus Walleij [161027 00:57]: > On Tue, Oct 25, 2016 at 6:45 PM, Tony Lindgren wrote: > > I need some DT person to take a look at this binding and ACK it. > > > +For pin controller hardware with a large number of identical registers naming > > +each bit both can be unmaintainable. Further there can be a large number of similar > > +pinctrl hardware using the same registers for different purposes depending on the > > +packaging. For cases like this, the pinctrl driver may use pinctrl-pin-array helper > > +binding using a hardware based index and a number of configuration values: > > Maybe we can reword it a bit so that it is clear that this is an > either-or approach > for the pin controller, either they use the pins/groups/functions scheme > or they use this scheme. Sure, this is just an optional helper. > > +pincontroller { > > + ... /* Standard DT properties for the device itself elided */ > > + #pinctrl-cells = <2>; > > + > > + state_0_node_a { > > + pinctrl-pin-array = < > > + 0 A_DELAY_PS(0) G_DELAY_PS(120) > > + 4 A_DELAY_PS(0) G_DELAY_PS(360) > > + ... > > + >; > > + }; > > + ... > > +}; > > Looks all right to me. Sad to add to the binding mess, but on the other > hand, in the overall picture this nicely consolidates and structure > pinctrl-single. > > > +The index for pinctrl-pin-array must relate to the hardware for the pinctrl > > +registers, and must not be a virtual index of pin instances. The reason for > > +this is to avoid mapping of the index in the dts files and the pin controller > > +driver as it can change. > > OK > > > And we want to avoid another case of interrupt > > +numbering with pinctrl numbering. > > Maybe this file is not a good place for making technical arguments, > more describing what we agreed on, so cut that sentence IMO. Sure :) > > +/* > > + * For pinctrl binding, typically #pinctrl-cells is for the pin controller > > + * device, so either parent or grandparent. See pinctrl-bindings.txt. > > + */ > > +static int pinctrl_find_cells_size(const struct device_node *np, > > + const char *cells_name) > > +{ > > + int cells_size, error; > > + > > + error = of_property_read_u32(np->parent, cells_name, &cells_size); > > + if (error) { > > + error = of_property_read_u32(np->parent->parent, > > + cells_name, &cells_size); > > + if (error) > > + return -ENOENT; > > + } > > + > > + return cells_size; > > +} > > Can't we just hardcode this to "#pinctrl-cells" and skip the cells_name > parameter? We can parametrize it the day we need it instead. Sure we can do that. > The rest of the helpers look nice and clean. OK cool thanks, Tony