pinctrl-mxs fails to parse function definitions from different .dts files

pinctrl-mxs fails to parse function definitions from different .dts files

[Uwe Kleine-König]

Hello,

I hit a problem with pinctrl-mxs on an i.MX28 machine (based on v3.8,
but I'm sure it still applies to newer kernels). imx28.dtsi defines a
pinctrl function:

	i2c1_pins_a: i2c1 at 0 {
		reg = <0>;
		fsl,pinmux-ids = <
			0x3101 /* MX28_PAD_PWM0__I2C1_SCL */
			0x3111 /* MX28_PAD_PWM1__I2C1_SDA */
		>;
		fsl,drive-strength = <1>;
		fsl,voltage = <1>;
		fsl,pull-up = <1>;
	};

my machine however needs

	i2c1_pins_b: i2c1 at 1 {
			...
			0x30a1 /* MX28_PAD_AUART2_CTS__I2C1_SCL */
			0x30b1 /* MX28_PAD_AUART2_RTS__I2C1_SDA */
			...
	}

so I added a corresponding entry to my machine's dts. The problem with
that is the pinctrl-mxs driver fails to make use of the i2c1 at 1 node
because it goes through the device tree in the order
for_each_child_of_node uses and does (a bit simplified):

	fn = "";
	for_each_child_of_node(np, child) {
		...
		soc->ngroups++;
		...
		if (strcmp(fn, child->name)) {
			fn = child->name;
			soc->nfunctions++;
		}
	}

. Then it does some allocation and iterates a few times more using
for_each_child_of_node and creates the groups for all functions.

This results in i2c1 at 0 and i2c1 at 1 being counted as two different
functions as they are not defined in succession in the dtb. I didn't
look at how the code behaves during probe, but with i2c1 at 1 being defined
in my machine's dtb, i2c1 failes to probe with

	imx28-pinctrl 80018000.pinctrl: invalid group "i2c1.1" for function "i2c1"

and I get:

	# grep i2c1 /sys/kernel/debug/pinctrl/80018000.pinctrl/pinmux-functions
	function: i2c1, groups = [ i2c1.0 ]
	function: i2c1, groups = [ i2c1.1 ]

while when I move the definition of i2c1 at 1 into imx28.dtsi after i2c1 at 0
i2c1 probes just fine and also the pinmux-functions look right:
	
	# grep i2c1 /sys/kernel/debug/pinctrl/80018000.pinctrl/pinmux-functions
	function: i2c1, groups = [ i2c1.0 i2c1.1 ]

This is not intended, is it?

The obvious ways to fix that are either check all already found
functions when a new one is hit and not only the last or using a sorted
list. Any thoughts?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

pinctrl-mxs fails to parse function definitions from different .dts files

[Shawn Guo]

Hi Uwe,

On Fri, Aug 30, 2013 at 11:27:25AM +0200, Uwe Kleine-K?nig wrote:
> while when I move the definition of i2c1 at 1 into imx28.dtsi after i2c1 at 0
> i2c1 probes just fine and also the pinmux-functions look right:
> 	
> 	# grep i2c1 /sys/kernel/debug/pinctrl/80018000.pinctrl/pinmux-functions
> 	function: i2c1, groups = [ i2c1.0 i2c1.1 ]
> 
> This is not intended, is it?

Sorry.  I should have documented it somewhere, but this is intended.
The driver requires all the pin groups for given function be sorted
together in 'reg'.

Shawn

pinctrl-mxs fails to parse function definitions from different .dts files

[Uwe Kleine-König]

Hello Shawn,

On Fri, Aug 30, 2013 at 07:51:29PM +0800, Shawn Guo wrote:
> On Fri, Aug 30, 2013 at 11:27:25AM +0200, Uwe Kleine-K?nig wrote:
> > while when I move the definition of i2c1 at 1 into imx28.dtsi after i2c1 at 0
> > i2c1 probes just fine and also the pinmux-functions look right:
> > 	
> > 	# grep i2c1 /sys/kernel/debug/pinctrl/80018000.pinctrl/pinmux-functions
> > 	function: i2c1, groups = [ i2c1.0 i2c1.1 ]
> > 
> > This is not intended, is it?
> 
> Sorry.  I should have documented it somewhere, but this is intended.
> The driver requires all the pin groups for given function be sorted
> together in 'reg'.
The groups for the function i2c1 are sorted, i.e. i2c1.0 comes before
i2c1.1 :-) But honestly, it's very unconvenient to require this
grouping. It makes it impossible to define i2c1.1 in myimx28machine.dts
and so a nice advantage of being able to just include the vanilla
imx28.dtsi in my external dts file goes away :-(

So I would prefer a patch that gets rid of this restriction instead of a
patch that documents it.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

pinctrl-mxs fails to parse function definitions from different .dts files

[Shawn Guo]

On Tue, Sep 03, 2013 at 09:50:50AM +0200, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Fri, Aug 30, 2013 at 07:51:29PM +0800, Shawn Guo wrote:
> > On Fri, Aug 30, 2013 at 11:27:25AM +0200, Uwe Kleine-K?nig wrote:
> > > while when I move the definition of i2c1 at 1 into imx28.dtsi after i2c1 at 0
> > > i2c1 probes just fine and also the pinmux-functions look right:
> > > 	
> > > 	# grep i2c1 /sys/kernel/debug/pinctrl/80018000.pinctrl/pinmux-functions
> > > 	function: i2c1, groups = [ i2c1.0 i2c1.1 ]
> > > 
> > > This is not intended, is it?
> > 
> > Sorry.  I should have documented it somewhere, but this is intended.
> > The driver requires all the pin groups for given function be sorted
> > together in 'reg'.
> The groups for the function i2c1 are sorted, i.e. i2c1.0 comes before
> i2c1.1 :-) But honestly, it's very unconvenient to require this
> grouping. It makes it impossible to define i2c1.1 in myimx28machine.dts
> and so a nice advantage of being able to just include the vanilla
> imx28.dtsi in my external dts file goes away :-(
> 
> So I would prefer a patch that gets rid of this restriction instead of a
> patch that documents it.

Yeah, patch for the driver is welcomed.

Shawn