On Wed, Jul 15, 2015 at 12:05:25PM +0200, Sascha Hauer wrote:

On Wed, Jul 15, 2015 at 10:45:42AM +0200, Ludovic Desroches wrote:

quoted

On Tue, Jul 14, 2015 at 08:13:59AM +0200, Sascha Hauer wrote:

quoted

On Wed, Jun 10, 2015 at 05:04:57PM +0200, Ludovic Desroches wrote:

quoted

Using a string to describe a pin in the device tree can be not enough.
Some controllers may need extra information to fully describe a pin. It
concerns mainly controllers which have a per pin muxing approach which
don't fit well the notions of groups and functions.
Instead of using a pin name, a 32 bit value is used. The 16 least
significant bits are used for the pin number. Other 16 bits can be used to
store extra parameters.

In the Mediatek driver we use 'pinmux' as name for the property
containing the combined pin number / mux value defines. 'pinmux' better
describes what it is...

At the moment, I don't mix pin number and pin mux. I mix pin number and
ioset. It allows to check that all the pins belong to the same ioset.

As said previously, I didn't want to mix pin mux and pin conf in the
same node (but it is something I can do, it's not a problem on my side).
If I do it I will have to mux three values: pin number, pin mux value
and pin ioset.

So assuming I do this change, your advice is to add a 'pinmux' property in
addition of 'pins' instead of trying to use it?

My advise is to not enslave your code to this ioset concept. The only
effect of introducing this concept is one single warning in the log:

My code is not totally enslaved to the ioset concept. The group_defs
node is not only for ioset, it is also to have comprehensive group
names when consultind sysfs. I don't know how I can achieve this if I
remove group_defs... Maybe with the parent node name?

	dev_warn(&pdev->dev,
		"/!\\ pins from group %s are not using the same ioset /!\\\n",
		group->name);

There are *no* decisions made in the driver upon the ioset, only the
above warning is printed.

I can easily find examples in which a device needs some functional pins
and some additional GPIOs, be it for card detection or something else,
and this *will* work, regardless of which ioset the pins are in. Why
should a ioset concept limit me in this way that everything works except
I have to suppress or ignore the warning in the log or split the pinmux
node up to two different nodes?

The only thing that won't work (or at least you don't want to guarantee
that it works), is to mix functional pins from the upper left corner of
the SoC with pins from the lower right corner to form a single MMC/SD
controller. Yes, you shouldn't do that. I may or may not work, but that
is nothing this particular SoC introduces, it's like this on many other
SoCs, perhaps even including earlier Atmel SoCs. Still, me as the guy
writing board support have to support the way the hardware is designed,
and if a board designer chooses to use pins from different iosets for a
single device I'll support it that way, no matter if a warning is shown
or not. If the users of this board are annoyed by the above warning
you'll probably receive a patch dropping it soon. Then the ioset concept
has completely vanished, but still the device trees are written like
that.

We know that mixing pins from several iosets should work for most of our
devices but it won't be the case for some such as SDHCI, ISI. Yes,
it is only a warning which can be easily removed. The goal of this
warning was to ease support. We often have the kernel logs but no
knowledge about the hardware. We could easily spot this mistake.

So, yes, my advice is to drop the ioset concept completely. If you still
insist on it then you can encode the ioset number in the pinmux define.
Only the lower 16bit are defined as the pin number, you can use the
upper 16bit like you want. You can encode the muxing in bits 16-23 and the
ioset in bits 24-31.

It was exactly what I wanted to do. Every one could use the bits 31-16
to store data they need.

quoted

quoted

quoted

+	if (pctldesc->complex_pin_desc && pins_prop) {
+		of_property_for_each_u32(np, subnode_target_type, prop, cur, val) {
+			pin_id = val & PINCTRL_PIN_MASK;
+			items_name[i++] = pctldesc->pins[pin_id].name;
+		}

I don't like that even though pins have numbers here they are converted
to strings which the driver later has to search in a list just to
convert it back into the number. This is quite inefficient.

I guess this could be optimized later, but it would be nice to have the
pin number directly in the driver.

I know that is something you don't like but, at the moment, I need a string for
pinctrl_utils_add_map_mux and pinctrl_utils_add_map_configs.

Yeah, I am fine with it. This can be fixed later. I am more concerned
about the device tree bindings than about the actual implementation. The
implementation can far more easy be changed than the bindings.

Using mediatek bindings is not an issue. My remaining concern is about
groups, I would like to avoid 'a pin is a group'.


Regards

Ludovic