linux-kernel - Re: [PATCH v6 8/9] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G mux

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]
Date:   Mon, 2 Jan 2017 21:47:58 +0100
From:   Peter Rosin <peda@...ntia.se>
To:     Jonathan Cameron <jic23@...nel.org>, linux-kernel@...r.kernel.org
Cc:     Wolfram Sang <wsa@...-dreams.de>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Jonathan Corbet <corbet@....net>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
        linux-iio@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v6 8/9] dt-bindings: mux-adg792a: document devicetree
 bindings for ADG792A/G mux

On 2017-01-02 19:05, Jonathan Cameron wrote:
> On 02/01/17 16:01, Peter Rosin wrote:
>> On 2017-01-01 12:00, Jonathan Cameron wrote:
>>> On 30/11/16 08:17, Peter Rosin wrote:
>>>> Analog Devices ADG792A/G is a triple 4:1 mux.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@...ntia.se>
>>> Few comments inline.  Worth adding anything about the gpio (output pins) to
>>> the binding at this stage as well?  Would certainly be nice to support
>>> them.
>>
>> I'll add optional properties "gpio-controller;" and "#gpio-cells = <2>;"
>> with the usual interpretation in v7 (but no implementation...) Is that
>> enough?
>>
>>> Jonathan
>>>> ---
>>>>  .../devicetree/bindings/misc/mux-adg792a.txt       | 64 ++++++++++++++++++++++
>>>>  1 file changed, 64 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/misc/mux-adg792a.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/misc/mux-adg792a.txt b/Documentation/devicetree/bindings/misc/mux-adg792a.txt
>>>> new file mode 100644
>>>> index 000000000000..4677f9ab1c55
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/misc/mux-adg792a.txt
>>>> @@ -0,0 +1,64 @@
>>>> +Bindings for Analog Devices ADG792A/G Triple 4:1 Multiplexers
>>>> +
>>>> +Required properties:
>>>> +- compatible : "adi,adg792a" or "adi,adg792g"
>>>> +- #mux-control-cells : <0> if parallel, or <1> if not.
>>>> +* Standard mux-controller bindings as decribed in mux-controller.txt
>>>> +
>>>> +Optional properties:
>>>> +- adi,parallel : if present, the three muxes are bound together with a single
>>>> +  mux controller, controlling all three muxes in parallel.
>>>> +- adi,idle-state : if present, array of states the three mux controllers will
>>>> +  have when idle (or, if parallel, a single idle-state).
>>> Hmm. These are actually a policy decision.  As only one policy will make
>>> sense for a given set of hardware probably fine to have it in here I guess.
>>> Might be worth adding a note to say this though.
>>
>> I don't really know what you want me to add, do you have a suggestion for the
>> wording?
>>
>>>> +
>>>> +Mux controller states 0 through 3 correspond to signals A through D in the
>>>> +datasheet. Mux controller states 4 and 5 are only available as possible idle
>>>> +states. State 4 represents that nothing is connected, and state 5 represents
>>>> +that the mux controller keeps the mux in its previously selected state during
>>>> +the idle period. State 5 is the default idle state.
>>> I'm never a great fan of magic numbers.  Can we represent this more cleanly by
>>> breaking it into multiple properties?
>>> Optional:
>>> adi,idle-switch-to-channel : switch to this channel when idle.
>>> adi,idle-high-impedance : <boolean> the nothing connected state?
>>>
>>> If neither present leaves it in previous state?
>>
>> It's not that easy. adi,idle-state is an array when there are three single
>> pole quadruple throw muxes, so there really needs to be a number for each
>> desired idle-behavior. Unless you have a better idea for how to describe
>> that?
> The above with arrays for each of the two parameters?
> Though then you need a priority documented - I'd say high impedance overrides
> the channel selection if both are present.

How would you specify that the first mux should idle in "state 5", the second
should idle in "state 4" and the third in "state 0"? (original state numbering)

You'd still need a magic number for the default idle state (state 5) so that
you can skip entries in the arrays. Or am I missing something?

Cheers,
peda

>>
>> Cheers,
>> peda
>>
>>>> +
>>>> +Example:
>>>> +
>>>> +	/* three independent mux controllers (of which one is used) */
>>>> +	&i2c0 {
>>>> +		mux: adg792a@50 {
>>>> +			compatible = "adi,adg792a";
>>>> +			reg = <0x50>;
>>>> +			#mux-control-cells = <1>;
>>>> +		};
>>>> +	};
>>>> +
>>>> +	adc-mux {
>>>> +		compatible = "iio-mux";
>>>> +		io-channels = <&adc 0>;
>>>> +		io-channel-names = "parent";
>>>> +
>>>> +		mux-controls = <&mux 1>;
>>>> +
>>>> +		channels = "sync-1", "", "out";
>>>> +	};
>>>> +
>>>> +
>>>> +	/*
>>>> +	 * Three parallel muxes with one mux controller, useful e.g. if
>>>> +	 * the adc is differential, thus needing two signals to be muxed
>>>> +	 * simultaneously for correct operation.
>>>> +	 */
>>>> +	&i2c0 {
>>>> +		pmux: adg792a@50 {
>>>> +			compatible = "adi,adg792a";
>>>> +			reg = <0x50>;
>>>> +			#mux-control-cells = <0>;
>>>> +			adi,parallel;
>>>> +		};
>>>> +	};
>>>> +
>>>> +	diff-adc-mux {
>>>> +		compatible = "iio-mux";
>>>> +		io-channels = <&adc 0>;
>>>> +		io-channel-names = "parent";
>>>> +
>>>> +		mux-controls = <&pmux>;
>>>> +
>>>> +		channels = "sync-1", "", "out";
>>>> +	};
>>>>
>>>
>>
>