Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren <swarren@wwwdotorg.org>]

On 05/04/2012 03:57 PM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [120504 12:27]:
>> On 05/02/2012 11:24 AM, Tony Lindgren wrote:
>>
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt
...
>> On the other hand, I worry about whether using "pinctrl-simple" here as
>> the compatible value is going to cause issues:
>>
>> Certainly, this is a pretty simple driver, and most likely reasonably
>> generic an applicable to many SoCs. However, it doesn't cover a bunch of
>> cases that I'd still consider "simple". For example, what if each pin
>> has a separate mux and pinconf register? There are probably many such
>> small cases that would add up to something more complex. to cover those
>> cases, will we be able to extend pinctrl-simple to cover them, and
>> continue to be backwards compatible, or will we need to create a
>> binding/driver for pinctrl-simple-1, pinctrl-simple-2, pinctrl-simple-3
>> each of which covers some different, yet still simple, configuration?
> 
> Yes getting the binding right is the critical part here, everything else
> can be added as needed. I was thinking about using separate properties
> for auxilary registers, but now thinking about it a bit more, it may not
> be sufficient.
> 
> How about we make some of these properties into arrays? For example:
> 
> #pinctrl-cells = 6;
> pinctrl-simple,function-mask = <0x0000ffff 0x0000ffff 0xffff0000>;
> pinctrl-simple,function-off = <0x7 0x7 0x70000>;
> pinctrl-simple,pinconf-mask = <0xffff0000 0xffff0000 0x0000ffff>;

I'm not sure what the 3 entries in the array are meant to describe?

> Then for handling the set/clear bits case of registers, we could do that
> automatically if both masks are 0 for some registers. We actually have
> that for some omap1 where there are three registers per mux with bits
> to set or clear. And these bits are not the same across all registers..

>>> +				0x6c 0xf	/* CSI21_DX3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
>>> +				0x6e 0xf	/* CSI21_DY3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
>>> +				0x70 0xf	/* CSI21_DX4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
>>> +				0x72 0xf	/* CSI21_DY4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
>>> +			>;
>>> +		};
>>> +	};
>>> +
>>> +
>>> +	/* map all uart2 pins as a single function */
>>> +	uart2_pins: pinmux_uart2_pins {
>>> +		uart2_pins {
>>> +			pinctrl-simple,cells = <
>>> +				0xd8 0x118	/* UART2_CTS OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
>>> +				0xda 0		/* UART2_RTS OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
>>> +				0xdc 0x118	/* UART2_RX OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
>>> +				0xde 0		/* UART2_TX OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
>>> +			>;
>>> +		};
>>> +	};
>>> +
>>> +	/* map all uart3 pins as separate functions */
>>> +	uart3_pins: pinmux_uart3_pins {
>>
>> From a binding perspective, I don't see why you'd want to allow two cases:
>>
>> 1) One node with multiple entries in pinctrl-simple,cells
>> 2) Multiple nodes each with a single entry in pinctrl-simple,cells
>>
>> Why not only allow (1)?
> 
> Because we need to specify GPIO for some pins. There may be additional flags

What do you mean by "specify GPIO"?

Nothing in this pinctrl-simple binding seems to imply that it's also a
GPIO controller.

If "GPIO" is one of the functions that can be mux'd onto a pin, then I'd
expect that to be represented in exactly the same way as any other
function that could be mux'd onto the pin.

So, I'm not sure what GPIO-related information you want to represent.

> too, we do have external DMA request lines for few pins available.. I'm not
> saying pinctrl fwk should know about that, but it's a similar mapping of pins
> to GPIO lines.

Aren't DMA request lines also just another function that can be mux'd
onto a pin?