linux-kernel - Re: [PATCH RFC v3 1/4] dt-bindings: mux: Increase the number of arguments in mux-controls

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: <5f455c4d-5edb-4382-1193-a519a7a227a5@ti.com>
Date:   Mon, 29 Nov 2021 10:06:30 +0530
From:   Aswath Govindraju <a-govindraju@...com>
To:     Peter Rosin <peda@...ntia.se>
CC:     Vignesh Raghavendra <vigneshr@...com>, Nishanth Menon <nm@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        Kishon Vijay Abraham I <kishon@...com>,
        Vinod Koul <vkoul@...nel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-can@...r.kernel.org>,
        <linux-phy@...ts.infradead.org>
Subject: Re: [PATCH RFC v3 1/4] dt-bindings: mux: Increase the number of
 arguments in mux-controls

Hi Peter,

On 25/11/21 7:05 pm, Peter Rosin wrote:
> Hi!
> 
> You need to have some description on how #mux-control-cells now work.
> The previous description is in mux-consumer.yaml and an update there
> is needed.
> 
> However, I have realized that the adg792a binding uses #mux-control-cells
> to indicate if it should expose its three muxes with one mux-control
> and operate the muxes in parallel, or if it should be expose three
> independent mux-controls. So, the approach in this series to always
> have the #mux-control-cells property fixed at <2> when indicating a
> state will not work for that binding. And I see no fix for that binding
> without adding a new property.
> 
> So, I would like a different approach. Since I dislike how mux-controls
> -after this series- is not (always) specifying a mux-control like the name
> says, but instead optionally a specific state, the new property I would
> like to add is #mux-state-cells such that it would always be one more
> than #mux-control-cells.
> 
> 	mux: mux-controller {
> 		compatible = "gpio-mux";
> 		#mux-control-cells = <0>;
> 		#mux-state-cells = <1>;
> 
> 		mux-gpios = <...>;
> 	};
> 
> 	can-phy {
> 		compatible = "ti,tcan1043";
> 		...
> 		mux-states = <&mux 1>;
> 	};
> 
> That solves the naming issue, the unused argument for mux-conrtrollers
> that previously had #mux-control-cells = <0>, and the binding for adg792a
> need no longer be inconsistent.
> 
> Or, how should this be solved? I'm sure there are other options...
> 


I feel that the new approach using mux-state-cells seems to be
overpopulating the device tree nodes, when the state can be represented
using the control cells. I understand that the definition for
mux-controls is to only specify the control line to be used in a given
mux. Can't it now be upgraded to also represent the state at which the
control line has to be set to?

With respect to adg792a, it is inline with the current implementation
and the only change I think would be required in the driver is,

diff --git a/drivers/mux/adg792a.c b/drivers/mux/adg792a.c
index e8fc2fc1ab09..2cd3bb8a40d4 100644
--- a/drivers/mux/adg792a.c
+++ b/drivers/mux/adg792a.c
@@ -73,8 +73,6 @@ static int adg792a_probe(struct i2c_client *i2c)
        ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
        if (ret < 0)
                return ret;
-       if (cells >= 2)
-               return -EINVAL;

        mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);
        if (IS_ERR(mux_chip))

And the following series should be compatible with it. If adg792a driver
is the only issue then would there be any issue with only changing it
and using this implementation?

Thanks,
Aswath




> Cheers,
> Peter
> 
> On 2021-11-23 09:12, Aswath Govindraju wrote:
>> Increase the allowed number of arguments in mux-controls to add support for
>> passing information regarding the state of the mux to be set, for a given
>> device.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@...com>
>> ---
>>  Documentation/devicetree/bindings/mux/gpio-mux.yaml       | 2 +-
>>  Documentation/devicetree/bindings/mux/mux-controller.yaml | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>> index 0a7c8d64981a..c810b7df39de 100644
>> --- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>> @@ -26,7 +26,7 @@ properties:
>>        List of gpios used to control the multiplexer, least significant bit first.
>>  
>>    '#mux-control-cells':
>> -    const: 0
>> +    enum: [ 0, 1, 2 ]
>>  
>>    idle-state:
>>      default: -1
>> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>> index 736a84c3b6a5..0b4b067a97bf 100644
>> --- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
>> +++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>> @@ -73,7 +73,7 @@ properties:
>>      pattern: '^mux-controller(@.*|-[0-9a-f]+)?$'
>>  
>>    '#mux-control-cells':
>> -    enum: [ 0, 1 ]
>> +    enum: [ 0, 1, 2 ]
>>  
>>    idle-state:
>>      $ref: /schemas/types.yaml#/definitions/int32
>>