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: <51C23222.2060906@wwwdotorg.org>
Date:	Wed, 19 Jun 2013 16:35:14 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Christian Ruppert <christian.ruppert@...lis.com>
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 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.

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