[RFC] pinmux: group and function definitions in the device tree

[RFC] pinmux: group and function definitions in the device tree

[Ludovic Desroches]

Hi,

I would like to start a discussion about pinmuxing and device tree bindings.

I am currently writing a new pinmuxing driver using the generic pinconf.
My main concern is about defining functions and which pins belong to a
group.

At the moment, it seems that most drivers using the generic pinconf
define this stuff in a static way. The pinctrl-at91 driver covers many
devices, the new one should do the same for new Atmel devices. Having
the group and function definitions in the driver could involve a huge
file...
I am not sure it is a good thing to embed all these information into a
single zImage...

How can we achieved this? I was thinking about something like this:

pinctrl at fc06a000 {

	[...]

	pinctrl_defs {
		mci0 {
			mci0_ioset0_1bit_grp {
				at91,pins = <68 69 70>;
				at91,mux = <2>;
			};

			mci0_ioset0_4bit_grp {
				at91,pins = <68 69 70 71 72 73>;
				at91,mux = <2>;
			};

			mci0_ioset0_8bit_grp {
				at91,pins = <68 69 70 71 72 73 74 75 76 77>;
				at91,mux = <2>;
			};
		};
	};

	pinctrl_mci0_default: mci0_default {
		mux {
			function = "mci0";
			groups = &mci0_ioset0_8bit_grp;
		};

		conf {
			groups = &mci0_ioset0_8bit_grp;
			bias-pullup;
		};
	};
};

- A subnode for these definitions in order to not parse the whole
  pinctrl node to retrieve groups and functions.
- Using node names as function and group names.
- Can we get generic properties to define the groups? Of course a 'pins'
  property is mandatory. In my case I will need an extra one to tell the
  controller how to mux the pins (a same pin can have up to 7 muxing
  possibilities).


Thanks for your advices.

Ludovic

[RFC] pinmux: group and function definitions in the device tree

[Sascha Hauer]

On Thu, Mar 19, 2015 at 04:39:50PM +0100, Ludovic Desroches wrote:
> Hi,
> 
> I would like to start a discussion about pinmuxing and device tree bindings.
> 
> I am currently writing a new pinmuxing driver using the generic pinconf.
> My main concern is about defining functions and which pins belong to a
> group.
> 
> At the moment, it seems that most drivers using the generic pinconf
> define this stuff in a static way. The pinctrl-at91 driver covers many
> devices, the new one should do the same for new Atmel devices. Having
> the group and function definitions in the driver could involve a huge
> file...
> I am not sure it is a good thing to embed all these information into a
> single zImage...
> 
> How can we achieved this? I was thinking about something like this:
> 
> pinctrl at fc06a000 {
> 
> 	[...]
> 
> 	pinctrl_defs {
> 		mci0 {
> 			mci0_ioset0_1bit_grp {
> 				at91,pins = <68 69 70>;
> 				at91,mux = <2>;
> 			};
> 
> 			mci0_ioset0_4bit_grp {
> 				at91,pins = <68 69 70 71 72 73>;
> 				at91,mux = <2>;
> 			};
> 
> 			mci0_ioset0_8bit_grp {
> 				at91,pins = <68 69 70 71 72 73 74 75 76 77>;
> 				at91,mux = <2>;
> 			};
> 		};
> 	};

Why are different groups here? Do you want to put them into the dtsi?
This would mean you have to carry a lot of groups in each dtsi from
which only a small fraction is used. We did that on i.MX but no longer
do this since the dtbs get very big.

> 
> 	pinctrl_mci0_default: mci0_default {
> 		mux {
> 			function = "mci0";
> 			groups = &mci0_ioset0_8bit_grp;
> 		};
> 
> 		conf {
> 			groups = &mci0_ioset0_8bit_grp;
> 			bias-pullup;
> 		};
> 	};
> };
> 
> - A subnode for these definitions in order to not parse the whole
>   pinctrl node to retrieve groups and functions.
> - Using node names as function and group names.
> - Can we get generic properties to define the groups? Of course a 'pins'
>   property is mandatory. In my case I will need an extra one to tell the
>   controller how to mux the pins (a same pin can have up to 7 muxing
>   possibilities).

Did you have a look at the RFC I sent for these kind of controllers [1] and
the final result for the Mediatek driver currently in Linux-next [2]?.

The binding has both the config and the pins in a single node and thus
is very compact.

Sascha

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/296491.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318452.html

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC] pinmux: group and function definitions in the device tree

[Ludovic Desroches]

On Thu, Mar 19, 2015 at 07:56:37PM +0100, Sascha Hauer wrote:
> On Thu, Mar 19, 2015 at 04:39:50PM +0100, Ludovic Desroches wrote:
> > Hi,
> > 
> > I would like to start a discussion about pinmuxing and device tree bindings.
> > 
> > I am currently writing a new pinmuxing driver using the generic pinconf.
> > My main concern is about defining functions and which pins belong to a
> > group.
> > 
> > At the moment, it seems that most drivers using the generic pinconf
> > define this stuff in a static way. The pinctrl-at91 driver covers many
> > devices, the new one should do the same for new Atmel devices. Having
> > the group and function definitions in the driver could involve a huge
> > file...
> > I am not sure it is a good thing to embed all these information into a
> > single zImage...
> > 
> > How can we achieved this? I was thinking about something like this:
> > 
> > pinctrl at fc06a000 {
> > 
> > 	[...]
> > 
> > 	pinctrl_defs {
> > 		mci0 {
> > 			mci0_ioset0_1bit_grp {
> > 				at91,pins = <68 69 70>;
> > 				at91,mux = <2>;
> > 			};
> > 
> > 			mci0_ioset0_4bit_grp {
> > 				at91,pins = <68 69 70 71 72 73>;
> > 				at91,mux = <2>;
> > 			};
> > 
> > 			mci0_ioset0_8bit_grp {
> > 				at91,pins = <68 69 70 71 72 73 74 75 76 77>;
> > 				at91,mux = <2>;
> > 			};
> > 		};
> > 	};
> 
> Why are different groups here? Do you want to put them into the dtsi?

We used to have a configuration per pin in our products. On next ones we
will have some constraints ie. on the controller side we still have a
configuration per pin but we will introduced the notion of iosets. This
notion involves that timings are guaranteed only in one ioset. That's
why we can't mix signals from several iosets because. On the controller side
we can do all we want so I would like to use groups as a software protection.

I have several groups because I can only choose the 4bit group and use other
pins for an other device.

Example:

mci0 {
	mci0_ioset0_1bit_grp {
		pins = <PINMUX_PIN(68, 2),
			PINMUX_PIN(69, 2),
			PINMUX_PIN(70, 2)>;
	};
	mci0_ioset0_4bit_grp {
		pins = <PINMUX_PIN(68, 2),
			PINMUX_PIN(69, 2),
			PINMUX_PIN(70, 2),
			PINMUX_PIN(71, 2),
			PINMUX_PIN(72, 2),
			PINMUX_PIN(73, 2)>;
	};
	mci0_ioset0_8bit_grp {
		pins = <PINMUX_PIN(68, 2),
			PINMUX_PIN(69, 2),
			PINMUX_PIN(70, 2),
			PINMUX_PIN(71, 2),
			PINMUX_PIN(72, 2),
			PINMUX_PIN(73, 2),
			PINMUX_PIN(74, 2),
			PINMUX_PIN(75, 2),
			PINMUX_PIN(76, 2),
			PINMUX_PIN(77, 2)>;
	};
	mci0_ioset1_1bit_grp {
		pins = <PINMUX_PIN(153, 1),
			PINMUX_PIN(154, 1),
			PINMUX_PIN(155, 1)>;
	};
};

i2c0 {
	i2c0_ioset0_grp {
		pins  = <PINMUX_PIN(71, 4),
			 PINMUX_PIN(72, 4)>;
	};
	i2c0_ioset1_grp {
		pins  = <PINMUX_PIN(91, 6),
			 PINMUX_PIN(92, 6)>;
	};
	i2c0_ioset2_grp {
		pins  = <PINMUX_PIN(108, 1),
			 PINMUX_PIN(109, 1)>;
	};
};


> This would mean you have to carry a lot of groups in each dtsi from
> which only a small fraction is used. We did that on i.MX but no longer
> do this since the dtbs get very big.

Yes I would like to put it in the dtsi file. For sure we will have big
dtbs. If we put that in the driver we will increase the size of our
kernel. If every one does this, we will have a lot of unused code for a
multiplatform kernel. Where is the best place to put this?

> 
> > 
> > 	pinctrl_mci0_default: mci0_default {
> > 		mux {
> > 			function = "mci0";
> > 			groups = &mci0_ioset0_8bit_grp;
> > 		};
> > 
> > 		conf {
> > 			groups = &mci0_ioset0_8bit_grp;
> > 			bias-pullup;
> > 		};
> > 	};
> > };
> > 
> > - A subnode for these definitions in order to not parse the whole
> >   pinctrl node to retrieve groups and functions.
> > - Using node names as function and group names.
> > - Can we get generic properties to define the groups? Of course a 'pins'
> >   property is mandatory. In my case I will need an extra one to tell the
> >   controller how to mux the pins (a same pin can have up to 7 muxing
> >   possibilities).
> 
> Did you have a look at the RFC I sent for these kind of controllers [1] and
> the final result for the Mediatek driver currently in Linux-next [2]?.
> 
> The binding has both the config and the pins in a single node and thus
> is very compact.
> 

Thanks for the links. Well I had a look to them and now I am a bit
lost...

I agree with this binding but it involves to get rid of
pinconf_generic_dt_node_to_map_all, isn't it? What do group and function
become? It seems these concepts have disappeared.

I am disappointed because I have the feeling that the only remaining
part of the generic pinconf are the configuration properties.

> Sascha
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/296491.html
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318452.html
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] pinmux: group and function definitions in the device tree

[Sascha Hauer]

On Fri, Mar 20, 2015 at 04:06:09PM +0100, Ludovic Desroches wrote:
> On Thu, Mar 19, 2015 at 07:56:37PM +0100, Sascha Hauer wrote:
> > On Thu, Mar 19, 2015 at 04:39:50PM +0100, Ludovic Desroches wrote:
> > > 	[...]
> > > 
> > > 	pinctrl_defs {
> > > 		mci0 {
> > > 			mci0_ioset0_1bit_grp {
> > > 				at91,pins = <68 69 70>;
> > > 				at91,mux = <2>;
> > > 			};
> > > 
> > > 			mci0_ioset0_4bit_grp {
> > > 				at91,pins = <68 69 70 71 72 73>;
> > > 				at91,mux = <2>;
> > > 			};
> > > 
> > > 			mci0_ioset0_8bit_grp {
> > > 				at91,pins = <68 69 70 71 72 73 74 75 76 77>;
> > > 				at91,mux = <2>;
> > > 			};
> > > 		};
> > > 	};
> > 
> > Why are different groups here? Do you want to put them into the dtsi?
> 
> We used to have a configuration per pin in our products. On next ones we
> will have some constraints ie. on the controller side we still have a
> configuration per pin but we will introduced the notion of iosets. This
> notion involves that timings are guaranteed only in one ioset. That's
> why we can't mix signals from several iosets because. On the controller side
> we can do all we want so I would like to use groups as a software protection.

What does happen when you mix signals of different iosets? It won't work
so the developer will change it. What do you need the software
protection for?

> i2c0 {
> 	i2c0_ioset0_grp {
> 		pins  = <PINMUX_PIN(71, 4),
> 			 PINMUX_PIN(72, 4)>;
> 	};
> 	i2c0_ioset1_grp {
> 		pins  = <PINMUX_PIN(91, 6),
> 			 PINMUX_PIN(92, 6)>;
> 	};
> 	i2c0_ioset2_grp {
> 		pins  = <PINMUX_PIN(108, 1),
> 			 PINMUX_PIN(109, 1)>;
> 	};
> };
> 
> 
> > This would mean you have to carry a lot of groups in each dtsi from
> > which only a small fraction is used. We did that on i.MX but no longer
> > do this since the dtbs get very big.
> 
> Yes I would like to put it in the dtsi file. For sure we will have big
> dtbs. If we put that in the driver we will increase the size of our
> kernel. If every one does this, we will have a lot of unused code for a
> multiplatform kernel. Where is the best place to put this?

If you put only the needed ones in each dts then you won't have to carry
all unused groups in all dtbs.

I think the idea you follow here is that you want to put all possible
combinations (mci0_ioset0_4bit_grp, mci0_ioset1_1bit_grp) into the dtsi
and let the board dts just pick one. For i.MX we realized that this does
not scale because the number of possibilities is too high. Very often
new boards had to introduce new pingroups in the dtsi because they did
not yet exist or even did exist in a slightly different configuration.
Out of tree dts files were painful because they often caused merge
conflicts in the dtsi files. Also there there was the risk that two new
groups were introduced with the same name but different meanings because
someone else was faster (and chose the next free number in the 'ioset'
counter).
We removed the groups in the dtsi files altogether and put them into
each board dts individually. This also made it easy to add the
additional pins (Write protect, card detect) to the same groups.

> > > - A subnode for these definitions in order to not parse the whole
> > >   pinctrl node to retrieve groups and functions.
> > > - Using node names as function and group names.
> > > - Can we get generic properties to define the groups? Of course a 'pins'
> > >   property is mandatory. In my case I will need an extra one to tell the
> > >   controller how to mux the pins (a same pin can have up to 7 muxing
> > >   possibilities).
> > 
> > Did you have a look at the RFC I sent for these kind of controllers [1] and
> > the final result for the Mediatek driver currently in Linux-next [2]?.
> > 
> > The binding has both the config and the pins in a single node and thus
> > is very compact.
> > 
> 
> Thanks for the links. Well I had a look to them and now I am a bit
> lost...
> 
> I agree with this binding but it involves to get rid of
> pinconf_generic_dt_node_to_map_all, isn't it? What do group and function
> become? It seems these concepts have disappeared.

The binding I suggested changes nothing with pinconf, only the pinmux
information is added to the same node. You can still call
pinconf_generic_dt_node_to_map_all() on the nodes, it will simply ignore
the pinmux information. You would have to handle them separately (or
write some generic helper if you like)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC] pinmux: group and function definitions in the device tree

[Ludovic Desroches]

On Mon, Mar 23, 2015 at 07:44:24AM +0100, Sascha Hauer wrote:
> On Fri, Mar 20, 2015 at 04:06:09PM +0100, Ludovic Desroches wrote:
> > On Thu, Mar 19, 2015 at 07:56:37PM +0100, Sascha Hauer wrote:
> > > On Thu, Mar 19, 2015 at 04:39:50PM +0100, Ludovic Desroches wrote:
> > > > 	[...]
> > > > 
> > > > 	pinctrl_defs {
> > > > 		mci0 {
> > > > 			mci0_ioset0_1bit_grp {
> > > > 				at91,pins = <68 69 70>;
> > > > 				at91,mux = <2>;
> > > > 			};
> > > > 
> > > > 			mci0_ioset0_4bit_grp {
> > > > 				at91,pins = <68 69 70 71 72 73>;
> > > > 				at91,mux = <2>;
> > > > 			};
> > > > 
> > > > 			mci0_ioset0_8bit_grp {
> > > > 				at91,pins = <68 69 70 71 72 73 74 75 76 77>;
> > > > 				at91,mux = <2>;
> > > > 			};
> > > > 		};
> > > > 	};
> > > 
> > > Why are different groups here? Do you want to put them into the dtsi?
> > 
> > We used to have a configuration per pin in our products. On next ones we
> > will have some constraints ie. on the controller side we still have a
> > configuration per pin but we will introduced the notion of iosets. This
> > notion involves that timings are guaranteed only in one ioset. That's
> > why we can't mix signals from several iosets because. On the controller side
> > we can do all we want so I would like to use groups as a software protection.
> 
> What does happen when you mix signals of different iosets? It won't work
> so the developer will change it. What do you need the software
> protection for?
> 

I can't say it won't work, it could work in some cases. My fear is to
have some support cases because of this. It seems easy to spot this kind
of issue but experience tell us that we can loose time for this kind of
"stupid" error.

> > i2c0 {
> > 	i2c0_ioset0_grp {
> > 		pins  = <PINMUX_PIN(71, 4),
> > 			 PINMUX_PIN(72, 4)>;
> > 	};
> > 	i2c0_ioset1_grp {
> > 		pins  = <PINMUX_PIN(91, 6),
> > 			 PINMUX_PIN(92, 6)>;
> > 	};
> > 	i2c0_ioset2_grp {
> > 		pins  = <PINMUX_PIN(108, 1),
> > 			 PINMUX_PIN(109, 1)>;
> > 	};
> > };
> > 
> > 
> > > This would mean you have to carry a lot of groups in each dtsi from
> > > which only a small fraction is used. We did that on i.MX but no longer
> > > do this since the dtbs get very big.
> > 
> > Yes I would like to put it in the dtsi file. For sure we will have big
> > dtbs. If we put that in the driver we will increase the size of our
> > kernel. If every one does this, we will have a lot of unused code for a
> > multiplatform kernel. Where is the best place to put this?
> 
> If you put only the needed ones in each dts then you won't have to carry
> all unused groups in all dtbs.
> 
> I think the idea you follow here is that you want to put all possible
> combinations (mci0_ioset0_4bit_grp, mci0_ioset1_1bit_grp) into the dtsi
> and let the board dts just pick one. For i.MX we realized that this does
> not scale because the number of possibilities is too high. Very often
> new boards had to introduce new pingroups in the dtsi because they did
> not yet exist or even did exist in a slightly different configuration.
> Out of tree dts files were painful because they often caused merge
> conflicts in the dtsi files. Also there there was the risk that two new
> groups were introduced with the same name but different meanings because
> someone else was faster (and chose the next free number in the 'ioset'
> counter).
> We removed the groups in the dtsi files altogether and put them into
> each board dts individually. This also made it easy to add the
> additional pins (Write protect, card detect) to the same groups.
>

It validates the decision to not put group declaration in the
driver. If we put all the configurations, it will add a big amount of
code. If we put only the configurations we use, the user will have to
compile a new kernel to add the ones he needs.

Well, maybe you're right about putting the groups in the dts files. I've
to think more about this solution.

> > > > - A subnode for these definitions in order to not parse the whole
> > > >   pinctrl node to retrieve groups and functions.
> > > > - Using node names as function and group names.
> > > > - Can we get generic properties to define the groups? Of course a 'pins'
> > > >   property is mandatory. In my case I will need an extra one to tell the
> > > >   controller how to mux the pins (a same pin can have up to 7 muxing
> > > >   possibilities).
> > > 
> > > Did you have a look at the RFC I sent for these kind of controllers [1] and
> > > the final result for the Mediatek driver currently in Linux-next [2]?.
> > > 
> > > The binding has both the config and the pins in a single node and thus
> > > is very compact.
> > > 
> > 
> > Thanks for the links. Well I had a look to them and now I am a bit
> > lost...
> > 
> > I agree with this binding but it involves to get rid of
> > pinconf_generic_dt_node_to_map_all, isn't it? What do group and function
> > become? It seems these concepts have disappeared.
> 
> The binding I suggested changes nothing with pinconf, only the pinmux
> information is added to the same node. You can still call
> pinconf_generic_dt_node_to_map_all() on the nodes, it will simply ignore
> the pinmux information. You would have to handle them separately (or
> write some generic helper if you like)

Yes, I can still use it. What I mean is there is no generic helper at the
moment to get both pinmux (excepting using the function property) and
pinconf information. Is it planned to have something generic for the
pinmux property? I see MTK_GET_PIN_NO and MTK_GET_PIN_FUNC macros, on my side,
I think it will feet my needs. Maybe we only need to remove the MTK
prefix and put these macros in another header.

In the mediatek driver, I have also noticed that we have a group for
each pin. I have the feeling that the concept of groups disappear, isn't
it?


Ludovic

[RFC] pinmux: group and function definitions in the device tree

[Sascha Hauer]

On Mon, Mar 23, 2015 at 10:08:27AM +0100, Ludovic Desroches wrote:
> On Mon, Mar 23, 2015 at 07:44:24AM +0100, Sascha Hauer wrote:
> > On Fri, Mar 20, 2015 at 04:06:09PM +0100, Ludovic Desroches wrote:
> > > On Thu, Mar 19, 2015 at 07:56:37PM +0100, Sascha Hauer wrote:
> > > > On Thu, Mar 19, 2015 at 04:39:50PM +0100, Ludovic Desroches wrote:
> > > > > 	[...]
> > > > > 
> > > > > 	pinctrl_defs {
> > > > > 		mci0 {
> > > > > 			mci0_ioset0_1bit_grp {
> > > > > 				at91,pins = <68 69 70>;
> > > > > 				at91,mux = <2>;
> > > > > 			};
> > > > > 
> > > > > 			mci0_ioset0_4bit_grp {
> > > > > 				at91,pins = <68 69 70 71 72 73>;
> > > > > 				at91,mux = <2>;
> > > > > 			};
> > > > > 
> > > > > 			mci0_ioset0_8bit_grp {
> > > > > 				at91,pins = <68 69 70 71 72 73 74 75 76 77>;
> > > > > 				at91,mux = <2>;
> > > > > 			};
> > > > > 		};
> > > > > 	};
> > > > 
> > > > Why are different groups here? Do you want to put them into the dtsi?
> > > 
> > > We used to have a configuration per pin in our products. On next ones we
> > > will have some constraints ie. on the controller side we still have a
> > > configuration per pin but we will introduced the notion of iosets. This
> > > notion involves that timings are guaranteed only in one ioset. That's
> > > why we can't mix signals from several iosets because. On the controller side
> > > we can do all we want so I would like to use groups as a software protection.
> > 
> > What does happen when you mix signals of different iosets? It won't work
> > so the developer will change it. What do you need the software
> > protection for?
> > 
> 
> I can't say it won't work, it could work in some cases. My fear is to
> have some support cases because of this. It seems easy to spot this kind
> of issue but experience tell us that we can loose time for this kind of
> "stupid" error.

Hm, the software (dts in this case) developer will only mix signals of
different iosets when he is forced to by the board designer. It's the
board designer that has made this mistake, the software developer will
only try to make it work anyway. I doubt that the board designer will
design the board based on the possibilities shown in the dts files.

> > > > > - A subnode for these definitions in order to not parse the whole
> > > > >   pinctrl node to retrieve groups and functions.
> > > > > - Using node names as function and group names.
> > > > > - Can we get generic properties to define the groups? Of course a 'pins'
> > > > >   property is mandatory. In my case I will need an extra one to tell the
> > > > >   controller how to mux the pins (a same pin can have up to 7 muxing
> > > > >   possibilities).
> > > > 
> > > > Did you have a look at the RFC I sent for these kind of controllers [1] and
> > > > the final result for the Mediatek driver currently in Linux-next [2]?.
> > > > 
> > > > The binding has both the config and the pins in a single node and thus
> > > > is very compact.
> > > > 
> > > 
> > > Thanks for the links. Well I had a look to them and now I am a bit
> > > lost...
> > > 
> > > I agree with this binding but it involves to get rid of
> > > pinconf_generic_dt_node_to_map_all, isn't it? What do group and function
> > > become? It seems these concepts have disappeared.
> > 
> > The binding I suggested changes nothing with pinconf, only the pinmux
> > information is added to the same node. You can still call
> > pinconf_generic_dt_node_to_map_all() on the nodes, it will simply ignore
> > the pinmux information. You would have to handle them separately (or
> > write some generic helper if you like)
> 
> Yes, I can still use it. What I mean is there is no generic helper at the
> moment to get both pinmux (excepting using the function property) and
> pinconf information. Is it planned to have something generic for the
> pinmux property?

Not yet, but it would be a good idea to add something generic.

> I see MTK_GET_PIN_NO and MTK_GET_PIN_FUNC macros, on my side,
> I think it will feet my needs. Maybe we only need to remove the MTK
> prefix and put these macros in another header.
> 
> In the mediatek driver, I have also noticed that we have a group for
> each pin. I have the feeling that the concept of groups disappear, isn't
> it?

This may be because the concept of groups doesn't most hardware.
There really is hardware out there which can only handle the pins in
groups (that is, a single mux switches multiple pins), but this hardware
is not very common. Most hardware can indeed control every pin
indivually. In this situation some drivers are consequent and make a
group out of each pin which renders the group concept moot. Other
drivers just interpret each device node as pin group which creates
artificial groups which do not exist in hardware.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC] pinmux: group and function definitions in the device tree

[Jean-Christophe PLAGNIOL-VILLARD]

> On Mar 23, 2015, at 6:09 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> On Mon, Mar 23, 2015 at 10:08:27AM +0100, Ludovic Desroches wrote:
>> On Mon, Mar 23, 2015 at 07:44:24AM +0100, Sascha Hauer wrote:
>>> On Fri, Mar 20, 2015 at 04:06:09PM +0100, Ludovic Desroches wrote:
>>>> On Thu, Mar 19, 2015 at 07:56:37PM +0100, Sascha Hauer wrote:
>>>>> On Thu, Mar 19, 2015 at 04:39:50PM +0100, Ludovic Desroches wrote:
>>>>>> 	[...]
>>>>>> 
>>>>>> 	pinctrl_defs {
>>>>>> 		mci0 {
>>>>>> 			mci0_ioset0_1bit_grp {
>>>>>> 				at91,pins = <68 69 70>;
>>>>>> 				at91,mux = <2>;
>>>>>> 			};
>>>>>> 
>>>>>> 			mci0_ioset0_4bit_grp {
>>>>>> 				at91,pins = <68 69 70 71 72 73>;
>>>>>> 				at91,mux = <2>;
>>>>>> 			};
>>>>>> 
>>>>>> 			mci0_ioset0_8bit_grp {
>>>>>> 				at91,pins = <68 69 70 71 72 73 74 75 76 77>;
>>>>>> 				at91,mux = <2>;
>>>>>> 			};
>>>>>> 		};
>>>>>> 	};
>>>>> 
>>>>> Why are different groups here? Do you want to put them into the dtsi?
>>>> 
>>>> We used to have a configuration per pin in our products. On next ones we
>>>> will have some constraints ie. on the controller side we still have a
>>>> configuration per pin but we will introduced the notion of iosets. This
>>>> notion involves that timings are guaranteed only in one ioset. That's
>>>> why we can't mix signals from several iosets because. On the controller side
>>>> we can do all we want so I would like to use groups as a software protection.
>>> 
>>> What does happen when you mix signals of different iosets? It won't work
>>> so the developer will change it. What do you need the software
>>> protection for?
>>> 
>> 
>> I can't say it won't work, it could work in some cases. My fear is to
>> have some support cases because of this. It seems easy to spot this kind
>> of issue but experience tell us that we can loose time for this kind of
>> "stupid" error.
> 
> Hm, the software (dts in this case) developer will only mix signals of
> different iosets when he is forced to by the board designer. It's the
> board designer that has made this mistake, the software developer will
> only try to make it work anyway. I doubt that the board designer will
> design the board based on the possibilities shown in the dts files.
> 
>>>>>> - A subnode for these definitions in order to not parse the whole
>>>>>>  pinctrl node to retrieve groups and functions.
>>>>>> - Using node names as function and group names.
>>>>>> - Can we get generic properties to define the groups? Of course a 'pins'
>>>>>>  property is mandatory. In my case I will need an extra one to tell the
>>>>>>  controller how to mux the pins (a same pin can have up to 7 muxing
>>>>>>  possibilities).
>>>>> 
>>>>> Did you have a look at the RFC I sent for these kind of controllers [1] and
>>>>> the final result for the Mediatek driver currently in Linux-next [2]?.
>>>>> 
>>>>> The binding has both the config and the pins in a single node and thus
>>>>> is very compact.
>>>>> 
>>>> 
>>>> Thanks for the links. Well I had a look to them and now I am a bit
>>>> lost...
>>>> 
>>>> I agree with this binding but it involves to get rid of
>>>> pinconf_generic_dt_node_to_map_all, isn't it? What do group and function
>>>> become? It seems these concepts have disappeared.
>>> 
>>> The binding I suggested changes nothing with pinconf, only the pinmux
>>> information is added to the same node. You can still call
>>> pinconf_generic_dt_node_to_map_all() on the nodes, it will simply ignore
>>> the pinmux information. You would have to handle them separately (or
>>> write some generic helper if you like)
>> 
>> Yes, I can still use it. What I mean is there is no generic helper at the
>> moment to get both pinmux (excepting using the function property) and
>> pinconf information. Is it planned to have something generic for the
>> pinmux property?
> 
> Not yet, but it would be a good idea to add something generic.
> 
>> I see MTK_GET_PIN_NO and MTK_GET_PIN_FUNC macros, on my side,
>> I think it will feet my needs. Maybe we only need to remove the MTK
>> prefix and put these macros in another header.
>> 
>> In the mediatek driver, I have also noticed that we have a group for
>> each pin. I have the feeling that the concept of groups disappear, isn't
>> it?
> 
> This may be because the concept of groups doesn't most hardware.
> There really is hardware out there which can only handle the pins in
> groups (that is, a single mux switches multiple pins), but this hardware
> is not very common. Most hardware can indeed control every pin
> indivually. In this situation some drivers are consequent and make a
> group out of each pin which renders the group concept moot. Other
> drivers just interpret each device node as pin group which creates
> artificial groups which do not exist in hardware.

This is what we do on the current pinctrl-at91

and as we do not see the specific of this new IP for at91 it?s difficult to see
if we can use generic or not.

Personally I do prefer when the number of possibility are not high to have a big
soc dtsi.

But in the case of I.MX yes it?s impossible to manage

Best Regards,
J.

[RFC] pinmux: group and function definitions in the device tree

[Ludovic Desroches]

On Mon, Mar 23, 2015 at 06:29:11PM +0800, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> > On Mar 23, 2015, at 6:09 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > 
> > On Mon, Mar 23, 2015 at 10:08:27AM +0100, Ludovic Desroches wrote:
> >> On Mon, Mar 23, 2015 at 07:44:24AM +0100, Sascha Hauer wrote:
> >>> On Fri, Mar 20, 2015 at 04:06:09PM +0100, Ludovic Desroches wrote:
> >>>> On Thu, Mar 19, 2015 at 07:56:37PM +0100, Sascha Hauer wrote:
> >>>>> On Thu, Mar 19, 2015 at 04:39:50PM +0100, Ludovic Desroches wrote:
> >>>>>> 	[...]
> >>>>>> 
> >>>>>> 	pinctrl_defs {
> >>>>>> 		mci0 {
> >>>>>> 			mci0_ioset0_1bit_grp {
> >>>>>> 				at91,pins = <68 69 70>;
> >>>>>> 				at91,mux = <2>;
> >>>>>> 			};
> >>>>>> 
> >>>>>> 			mci0_ioset0_4bit_grp {
> >>>>>> 				at91,pins = <68 69 70 71 72 73>;
> >>>>>> 				at91,mux = <2>;
> >>>>>> 			};
> >>>>>> 
> >>>>>> 			mci0_ioset0_8bit_grp {
> >>>>>> 				at91,pins = <68 69 70 71 72 73 74 75 76 77>;
> >>>>>> 				at91,mux = <2>;
> >>>>>> 			};
> >>>>>> 		};
> >>>>>> 	};
> >>>>> 
> >>>>> Why are different groups here? Do you want to put them into the dtsi?
> >>>> 
> >>>> We used to have a configuration per pin in our products. On next ones we
> >>>> will have some constraints ie. on the controller side we still have a
> >>>> configuration per pin but we will introduced the notion of iosets. This
> >>>> notion involves that timings are guaranteed only in one ioset. That's
> >>>> why we can't mix signals from several iosets because. On the controller side
> >>>> we can do all we want so I would like to use groups as a software protection.
> >>> 
> >>> What does happen when you mix signals of different iosets? It won't work
> >>> so the developer will change it. What do you need the software
> >>> protection for?
> >>> 
> >> 
> >> I can't say it won't work, it could work in some cases. My fear is to
> >> have some support cases because of this. It seems easy to spot this kind
> >> of issue but experience tell us that we can loose time for this kind of
> >> "stupid" error.
> > 
> > Hm, the software (dts in this case) developer will only mix signals of
> > different iosets when he is forced to by the board designer. It's the
> > board designer that has made this mistake, the software developer will
> > only try to make it work anyway. I doubt that the board designer will
> > design the board based on the possibilities shown in the dts files.
> > 
> >>>>>> - A subnode for these definitions in order to not parse the whole
> >>>>>>  pinctrl node to retrieve groups and functions.
> >>>>>> - Using node names as function and group names.
> >>>>>> - Can we get generic properties to define the groups? Of course a 'pins'
> >>>>>>  property is mandatory. In my case I will need an extra one to tell the
> >>>>>>  controller how to mux the pins (a same pin can have up to 7 muxing
> >>>>>>  possibilities).
> >>>>> 
> >>>>> Did you have a look at the RFC I sent for these kind of controllers [1] and
> >>>>> the final result for the Mediatek driver currently in Linux-next [2]?.
> >>>>> 
> >>>>> The binding has both the config and the pins in a single node and thus
> >>>>> is very compact.
> >>>>> 
> >>>> 
> >>>> Thanks for the links. Well I had a look to them and now I am a bit
> >>>> lost...
> >>>> 
> >>>> I agree with this binding but it involves to get rid of
> >>>> pinconf_generic_dt_node_to_map_all, isn't it? What do group and function
> >>>> become? It seems these concepts have disappeared.
> >>> 
> >>> The binding I suggested changes nothing with pinconf, only the pinmux
> >>> information is added to the same node. You can still call
> >>> pinconf_generic_dt_node_to_map_all() on the nodes, it will simply ignore
> >>> the pinmux information. You would have to handle them separately (or
> >>> write some generic helper if you like)
> >> 
> >> Yes, I can still use it. What I mean is there is no generic helper at the
> >> moment to get both pinmux (excepting using the function property) and
> >> pinconf information. Is it planned to have something generic for the
> >> pinmux property?
> > 
> > Not yet, but it would be a good idea to add something generic.
> > 
> >> I see MTK_GET_PIN_NO and MTK_GET_PIN_FUNC macros, on my side,
> >> I think it will feet my needs. Maybe we only need to remove the MTK
> >> prefix and put these macros in another header.
> >> 
> >> In the mediatek driver, I have also noticed that we have a group for
> >> each pin. I have the feeling that the concept of groups disappear, isn't
> >> it?
> > 
> > This may be because the concept of groups doesn't most hardware.
> > There really is hardware out there which can only handle the pins in
> > groups (that is, a single mux switches multiple pins), but this hardware
> > is not very common. Most hardware can indeed control every pin
> > indivually. In this situation some drivers are consequent and make a
> > group out of each pin which renders the group concept moot. Other
> > drivers just interpret each device node as pin group which creates
> > artificial groups which do not exist in hardware.
> 
> This is what we do on the current pinctrl-at91
> 

It is a bit different with pinctrl-at91, we don't have npins groups.
Even if there is no hardware reality behind we have groups of pins.

> and as we do not see the specific of this new IP for at91 it?s difficult to see
> if we can use generic or not.
> 

We can use the generic pinconf for sure. My main concern were the
concepts of groups and functions. But it seems, now, we can't get rid of these
concepts.

I am trying to figure out what we loose (or not) if using a mapping such as
'a pin = a group'. For example I am thinking about sysfs, it is probably
more human readable to have some groups with a name related to the
device they are connected as done in pinctrl-at91.

> Personally I do prefer when the number of possibility are not high to have a big
> soc dtsi.
> 
> But in the case of I.MX yes it?s impossible to manage
> 
> Best Regards,
> J.
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC] pinmux: group and function definitions in the device tree

[Sascha Hauer]

On Mon, Mar 23, 2015 at 11:09:13AM +0100, Sascha Hauer wrote:
> 
> This may be because the concept of groups doesn't most hardware.

I meant "doesn't match most hardware"

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC] pinmux: group and function definitions in the device tree

[Ludovic Desroches]

On Mon, Mar 23, 2015 at 11:09:13AM +0100, Sascha Hauer wrote:
> On Mon, Mar 23, 2015 at 10:08:27AM +0100, Ludovic Desroches wrote:
> > On Mon, Mar 23, 2015 at 07:44:24AM +0100, Sascha Hauer wrote:
> > > On Fri, Mar 20, 2015 at 04:06:09PM +0100, Ludovic Desroches wrote:
> > > > On Thu, Mar 19, 2015 at 07:56:37PM +0100, Sascha Hauer wrote:
> > > > > On Thu, Mar 19, 2015 at 04:39:50PM +0100, Ludovic Desroches wrote:
> > > > > > 	[...]
> > > > > > 
> > > > > > 	pinctrl_defs {
> > > > > > 		mci0 {
> > > > > > 			mci0_ioset0_1bit_grp {
> > > > > > 				at91,pins = <68 69 70>;
> > > > > > 				at91,mux = <2>;
> > > > > > 			};
> > > > > > 
> > > > > > 			mci0_ioset0_4bit_grp {
> > > > > > 				at91,pins = <68 69 70 71 72 73>;
> > > > > > 				at91,mux = <2>;
> > > > > > 			};
> > > > > > 
> > > > > > 			mci0_ioset0_8bit_grp {
> > > > > > 				at91,pins = <68 69 70 71 72 73 74 75 76 77>;
> > > > > > 				at91,mux = <2>;
> > > > > > 			};
> > > > > > 		};
> > > > > > 	};
> > > > > 
> > > > > Why are different groups here? Do you want to put them into the dtsi?
> > > > 
> > > > We used to have a configuration per pin in our products. On next ones we
> > > > will have some constraints ie. on the controller side we still have a
> > > > configuration per pin but we will introduced the notion of iosets. This
> > > > notion involves that timings are guaranteed only in one ioset. That's
> > > > why we can't mix signals from several iosets because. On the controller side
> > > > we can do all we want so I would like to use groups as a software protection.
> > > 
> > > What does happen when you mix signals of different iosets? It won't work
> > > so the developer will change it. What do you need the software
> > > protection for?
> > > 
> > 
> > I can't say it won't work, it could work in some cases. My fear is to
> > have some support cases because of this. It seems easy to spot this kind
> > of issue but experience tell us that we can loose time for this kind of
> > "stupid" error.
> 
> Hm, the software (dts in this case) developer will only mix signals of
> different iosets when he is forced to by the board designer. It's the
> board designer that has made this mistake, the software developer will
> only try to make it work anyway. I doubt that the board designer will
> design the board based on the possibilities shown in the dts files.
> 

I think we still have cases were the choice has to be done by the user.
For example, the board designer offers several GPIOs, some can be muxed
to the ISI device (there is two possible configurations). Among these pins, 
we can use some of them for I2C devices.

The user will see that he could get two I2C devices by mixing ISI
signals. Why not doing it. That's why I would like to keep some
protection, a 'soft' protection such as group.

> > > > > > - A subnode for these definitions in order to not parse the whole
> > > > > >   pinctrl node to retrieve groups and functions.
> > > > > > - Using node names as function and group names.
> > > > > > - Can we get generic properties to define the groups? Of course a 'pins'
> > > > > >   property is mandatory. In my case I will need an extra one to tell the
> > > > > >   controller how to mux the pins (a same pin can have up to 7 muxing
> > > > > >   possibilities).
> > > > > 
> > > > > Did you have a look at the RFC I sent for these kind of controllers [1] and
> > > > > the final result for the Mediatek driver currently in Linux-next [2]?.
> > > > > 
> > > > > The binding has both the config and the pins in a single node and thus
> > > > > is very compact.
> > > > > 
> > > > 
> > > > Thanks for the links. Well I had a look to them and now I am a bit
> > > > lost...
> > > > 
> > > > I agree with this binding but it involves to get rid of
> > > > pinconf_generic_dt_node_to_map_all, isn't it? What do group and function
> > > > become? It seems these concepts have disappeared.
> > > 
> > > The binding I suggested changes nothing with pinconf, only the pinmux
> > > information is added to the same node. You can still call
> > > pinconf_generic_dt_node_to_map_all() on the nodes, it will simply ignore
> > > the pinmux information. You would have to handle them separately (or
> > > write some generic helper if you like)
> > 
> > Yes, I can still use it. What I mean is there is no generic helper at the
> > moment to get both pinmux (excepting using the function property) and
> > pinconf information. Is it planned to have something generic for the
> > pinmux property?
> 
> Not yet, but it would be a good idea to add something generic.
> 
> > I see MTK_GET_PIN_NO and MTK_GET_PIN_FUNC macros, on my side,
> > I think it will feet my needs. Maybe we only need to remove the MTK
> > prefix and put these macros in another header.
> > 
> > In the mediatek driver, I have also noticed that we have a group for
> > each pin. I have the feeling that the concept of groups disappear, isn't
> > it?
> 
> This may be because the concept of groups doesn't most hardware.
> There really is hardware out there which can only handle the pins in
> groups (that is, a single mux switches multiple pins), but this hardware
> is not very common. Most hardware can indeed control every pin
> indivually. In this situation some drivers are consequent and make a
> group out of each pin which renders the group concept moot. Other
> drivers just interpret each device node as pin group which creates
> artificial groups which do not exist in hardware.
>

Ok it was the feeling I have, but I am not sure about the pros and cons
to create artificial groups. Any point of view about this?


Ludovic

[RFC] pinmux: group and function definitions in the device tree

[Sascha Hauer]

On Mon, Mar 23, 2015 at 03:29:54PM +0100, Ludovic Desroches wrote:
> On Mon, Mar 23, 2015 at 11:09:13AM +0100, Sascha Hauer wrote:
> > On Mon, Mar 23, 2015 at 10:08:27AM +0100, Ludovic Desroches wrote:
> > > On Mon, Mar 23, 2015 at 07:44:24AM +0100, Sascha Hauer wrote:
> > > > On Fri, Mar 20, 2015 at 04:06:09PM +0100, Ludovic Desroches wrote:
> > > > > On Thu, Mar 19, 2015 at 07:56:37PM +0100, Sascha Hauer wrote:
> > > > > We used to have a configuration per pin in our products. On next ones we
> > > > > will have some constraints ie. on the controller side we still have a
> > > > > configuration per pin but we will introduced the notion of iosets. This
> > > > > notion involves that timings are guaranteed only in one ioset. That's
> > > > > why we can't mix signals from several iosets because. On the controller side
> > > > > we can do all we want so I would like to use groups as a software protection.
> > > > 
> > > > What does happen when you mix signals of different iosets? It won't work
> > > > so the developer will change it. What do you need the software
> > > > protection for?
> > > > 
> > > 
> > > I can't say it won't work, it could work in some cases. My fear is to
> > > have some support cases because of this. It seems easy to spot this kind
> > > of issue but experience tell us that we can loose time for this kind of
> > > "stupid" error.
> > 
> > Hm, the software (dts in this case) developer will only mix signals of
> > different iosets when he is forced to by the board designer. It's the
> > board designer that has made this mistake, the software developer will
> > only try to make it work anyway. I doubt that the board designer will
> > design the board based on the possibilities shown in the dts files.
> > 
> 
> I think we still have cases were the choice has to be done by the user.
> For example, the board designer offers several GPIOs, some can be muxed
> to the ISI device (there is two possible configurations). Among these pins, 
> we can use some of them for I2C devices.
> 
> The user will see that he could get two I2C devices by mixing ISI
> signals. Why not doing it. That's why I would like to keep some
> protection, a 'soft' protection such as group.

Even if you put groups in the dtsi, this won't prevent me from creating my own
groups in the board dts and thus still violating the ioset rule.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |