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

[Ludovic Desroches <ludovic.desroches@xxxxxxxxx>]

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
--
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