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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130626115051.GC7095@ab42.lan>
Date:	Wed, 26 Jun 2013 13:50:52 +0200
From:	Christian Ruppert <christian.ruppert@...lis.com>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Linus Walleij <linus.walleij@...aro.org>,
	Patrice CHOTARD <patrice.chotard@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Rob Landley <rob@...dley.net>,
	Sascha Leuenberger <sascha.leuenberger@...lis.com>,
	Pierrick Hascoet <pierrick.hascoet@...lis.com>,
	devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
	Alexandre Courbot <acourbot@...dia.com>
Subject: Re: [PATCH 2/4] pinmux: Add TB10x pinmux driver

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@...0601c {
> > +	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@...lis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/