Re: [PATCH 2/4] pinmux: Add TB10x pinmux driver

[Christian Ruppert <christian.ruppert@abilis.com>]

On Wed, Jun 19, 2013 at 04:35:14PM -0600, Stephen Warren wrote:
> On 06/18/2013 03:29 AM, Christian Ruppert wrote:
> > The pinmux driver of the Abilis Systems TB10x platform based on ARC700 CPUs.
> > Used to control the pinmux and is a prerequisite for the GPIO driver.
> 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/abilis,tb10x-iomux.txt b/Documentation/devicetree/bindings/pinctrl/abilis,tb10x-iomux.txt
> 
> > +Port definitions
> > +----------------
> > +
> > +Ports are defined (and referenced) by sub-nodes of the pin controller. Every
> > +sub-node defines exactly one port (i.e. a set of pins). Ports are predefined
> > +as named pin groups inside the pin controller driver and these names are used
> > +to associate pin group predefinitions to pin controller sub-nodes.
> > +
> > +Required port definition subnode properties:
> > +  - pingrp: should be set to the name of the port's pin group.
> 
> This seems odd.... More on that where I comment on the example.
> 
> > +The following pin groups are available:
> > +  - GPIO ports: gpioa_pins, gpiob_pins, gpioc_pins, gpiod_pins, gpioe_pins,
> > +                gpiof_pins, gpiog_pins, gpioh_pins, gpioi_pins, gpioj_pins,
> > +                gpiok_pins, gpiol_pins, gpiom_pins, gpion_pins
> ...
> > +  - JTAG: jtag_pins
> 
> I'd suggest removing "_pins" from all those names, since it's the same
> in all names and hence isn't necessary.
> 
> > +GPIO ranges definition
> > +----------------------
> > +
> > +The named pin groups of GPIO ports can be used to define GPIO ranges as
> > +explained in Documentation/devicetree/bindings/gpio/gpio.txt.
> 
> I wouldn't mention that here; the GPIO node contains the gpio-ranges
> property, not the pin controller node. Hence, the binding for the GPIO
> DT node should describe the property, not the binding for this node.
> 
> > +Example
> > +-------
> > +
> > +iomux: iomux@FF10601c {
> > +	compatible = "abilis,tb10x-iomux";
> > +	reg = <0xFF10601c 0x4>;
> > +	pctl_gpio_a: pctl-gpio-a {
> > +		pingrp = "gpioa_pins";
> > +	};
> > +	pctl_uart0: pctl-uart0 {
> > +		pingrp = "uart0_pins";
> > +	};
> > +};
> 
> The two nodes pctl-gpio-a and pctl-uart0 seem to be missing data. The
> idea here is that you define nodes that says:
> 
> * This node applies to these pin(s)/group(s).
> * Select mux function X on those pins/groups and/or apply these pin
> configuration options to those pins/groups.
> 
> The examples above don't include any mux/config options, nor does the
> binding say how to do specify them.
> 
> The set of pin groups defined by this binding should correspond directly
> to the set of pin groups that actually exist in HW. So, if you have 3
> pin groups (A, B, C) in HW each of which has two mux functions (X, Y),
> your DT binding should define just 3 pin groups (A, B, C), not 6 (A_X,
> A_Y, B_X, B_Y, C_X, C_Y). In other words, the pin group name shouldn't
> imply the mux function.

Can we consider it as agreed now that this implementation is acceptable
for the TB10x pin controller?

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates