linux-kernel - Re: [PATCH 1/2] 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: <516EEAA0.10906@wwwdotorg.org>
Date:	Wed, 17 Apr 2013 12:32:00 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Christian Ruppert <christian.ruppert@...lis.com>
CC:	linux-kernel@...r.kernel.org,
	Linus Walleij <linus.walleij@...aro.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
Subject: Re: [PATCH 1/2] pinmux: Add TB10x pinmux driver

On 04/10/2013 09:45 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.

Linus already did a review of this, but I have a few extra comments:

> diff --git a/Documentation/devicetree/bindings/pinctrl/abilis,tb10x-iomux.txt b/Documentation/devicetree/bindings/pinctrl/abilis,tb10x-iomux.txt

> +Abilis Systems TB10x pin controller
> +===================================
> +
> +Required properties
> +-------------------
> +
> +- #address-cells: should be <1>.
> +- #size-cells: should be <1>;

What are those two used for? They would normally only be required if the
child nodes of the pinctrl node contained "reg" properties. But, they
don't in the examples later in this file.

> +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 defined by
> +their names in the pin controller driver.

I'm not really sure what the implication here is. Does this mean that
the driver isn't expected to contain tables which define which
pins/groups/functions exist, but rather gets those lists/tables from the
DT? I prefer not to do that, although it is acceptable to write the
binding/driver this way.

So there seems to be something huge missing here; the only property
defined here for the child nodes is "pingrp". In the examples, there's
nothing else used. I don't see how this binding allows the actual
desired mux functions to be specified. All other pinctrl DT bindings
have the child nodes contain something along the lines of:

* A list of pins/groups to apply the settings to.
* A property defining the mux function to select on those pins/groups.
* Other properties defining configuration for those pins/groups, such as
pull-up/down, drive-type, drive-strength, etc.

All that seems missing here. Surely it's required?

Perhaps this "abilis,simple-default" thing is intended to represent some
specific default configuration? If so, I don't think that's the right
way to go. Also, the DT binding should be as complete as possible from
the start, rather than planning to define/implement part of it now and
then keep adding to it later. This all implies that some extra
properties really should be defined here.

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