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

[Ludovic Desroches <ludovic.desroches@xxxxxxxxx>]

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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html