linux-kernel - Re: [PATCH 3/4] ARM: pinctrl: Add Broadcom Capri pinctrl 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: <527A75A6.5010904@wwwdotorg.org>
Date:	Wed, 06 Nov 2013 10:00:22 -0700
From:	Stephen Warren <swarren@...dotorg.org>
To:	Sherman Yin <syin@...adcom.com>,
	Heiko Stübner <heiko@...ech.de>,
	Linus Walleij <linus.walleij@...aro.org>
CC:	Laxman Dewangan <ldewangan@...dia.com>,
	Mark Rutland <mark.rutland@....com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Christian Daudt <bcm@...thebug.org>,
	Russell King <linux@....linux.org.uk>,
	Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Rob Herring <rob.herring@...xeda.com>,
	bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
	Rob Landley <rob@...dley.net>,
	Grant Likely <grant.likely@...aro.org>,
	Matt Porter <matt.porter@...aro.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 3/4] ARM: pinctrl: Add Broadcom Capri pinctrl driver

On 11/05/2013 07:02 PM, Sherman Yin wrote:
> On 13-11-04 04:04 PM, Stephen Warren wrote:
>> On 11/04/2013 04:26 PM, Heiko Stübner wrote:
>>
>>> I remember we had a discussion about how things like bias-disable
>>> explicitly
>>> shouldn't have a value, when they are represented in the list-format:
>>>
>>>         pcfg_pull_none: pcfg_pull_none {
>>>             bias-disable;
>>>         };
>>>
>>> so a bias-disable = <1> was explicitly "forbidden" [for a lack of a
>>> better
>>> word]. And it was similar for other options, the parameter not meant
>>> to be
>>> indicating if they are active but really only setting the "strength"
>>> or so.
>>
>> Pure Boolean values should be represented as a valueless property. If
>> the property is present, the value is true, otherwise false.
>>
>> However, pinctrl bindings often don't represent Boolean values, but
>> rather tri-states, with the following values:
>>
>> * Don't touch this configuration option at all (missing)
>> * Enable the option (<1>)
>> * Disable the option (<0>)
>>
>> The reason for using tri-states being that you might want to write:
>>
>> xxx1 {
>>      pins = <PINA>, <PINB>, <PINC>;
>>      function = <...>;
>>      // this node doesn't affect pullup
>> }
>> xxx2 {
>>      pins = <PINA>, <PINB>;
>>      // this node doesn't affect function
>>      pull-up = <1>; // change, and enable
>> }
>> xxx3 {
>>      pins = <PINAC>;
>>      // this node doesn't affect function
>>      pull-up = <0>; // change, and disable
>> }
> 
> If I understand correctly, in Stephen's example, if a certain driver
> wants to configure PINA PINB and PINC, the pin configuration nodes
> "xxx1", "xxx2", and "xxx3" will all have to be selected for the
> particular pin state.

You probably don't want to reference the individual xxx1/2/3 nodes in
the client pinctrl properties, but instead wrap them in a higher-level
node that represents the whole pinctrl state. Then, the client pinctrl
properties can reference just that single parent node, instead of many
small nodes. In other words:

pinctrl@... {
	...
	sx: state_xxx {
		xxx1 { ... };
		xxx2 { ... };
		xxx3 { ... };
	};
	sy: state_yyy {
		yyy1 { ... };
		yyy2 { ... };
	};
}

some_client@... {
	...
	pinctrl-names = "default";
	pinctrl-0 = <&sx>;
};

other_client@... {
	...
	pinctrl-names = "default";
	pinctrl-0 = <&sy>;
};

rather than:

pinctrl@... {
	...
	sx1: xxx1 { ... };
	sx2: xxx2 { ... };
	sx3: xxx3 { ... };
	sy1: yyy1 { ... };
	sy2: yyy2 { ... };
}

some_client@... {
	...
	pinctrl-names = "default";
	pinctrl-0 = <&sx1 &sx2 &sx3>;
};

other_client@... {
	...
	pinctrl-names = "default";
	pinctrl-0 = <&sy1 &sy2>;
};

This is exactly how the Tegra pinctrl bindings work for example.


> This works fine.  However, I'm just thinking that
> it would have been easier if we could specify just one node:
> 
> xxx {
>     pins = <PINA>, <PINB>, <PINC>;
>     function = <...>;
>     pull-up = <1 1 0>;
> }
> 
> This "feature" seems a bit more concise to me and is what I did for my
> original pinctrl driver.  The only downside is that with this method,
> one cannot specify "don't touch this option for this pin" if the same
> property must provide values for other pins.

The other downside is that if the lists get even slightly long, it get
really hard to match up the entries in the t properties.

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