linux-kernel - Re: [PATCH v3 1/2] dt-bindings: i2c-mux: Add property for settle time

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: <087a49e0-30fe-7b09-b246-6d50e96299d1@axentia.se>
Date:   Wed, 3 Nov 2021 12:44:46 +0100
From:   Peter Rosin <peda@...ntia.se>
To:     Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Rob Herring <robh@...nel.org>
Cc:     linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] dt-bindings: i2c-mux: Add property for settle time

On 2021-11-02 23:27, Horatiu Vultur wrote:
> The 11/02/2021 13:37, Rob Herring wrote:
>> On Mon, Nov 01, 2021 at 10:32:01PM +0100, Horatiu Vultur wrote:
>>> The 11/01/2021 15:32, Peter Rosin wrote:

*snip*

>>>
>>> +required:
>>> +  - compatible
>>
>> compatible should not be required here.
>>
>>> +  - '#address-cells'
>>> +  - '#size-cells'
>>> +
>>>  examples:
>>>    - |
>>>      /*
>>> ---
>>>
>>> If I have this then my problem is with the required properties because then I
>>> start to get new warnings once I run:
>>>
>>> make ARCH=arm CROSS_COMPILE=arm-linux- dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/i2c/i2c-mux.yaml
>>>
>>> For example, one of new the warnings is this:
>>>
>>> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: 'compatible' is a required property
>>>       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
>>> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#address-cells' is a required property
>>>       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
>>> /home/hvultur/linux/arch/arm/boot/dts/am335x-icev2.dt.yaml: mux-mii-hog: '#size-cells' is a required property
>>>       From schema: /home/hvultur/linux/Documentation/devicetree/bindings/i2c/i2c-mux.yaml
>>
>> This is because of the $nodename pattern being pretty lax and matches
>> on mux-mii-hog by mistake. We have 2 options. Change the nodename
>> pattern to '^(i2c-?)?mux(@.*)?$' or add 'select: false'. The former
>> would still match on 'mux' or 'mux@.*' which might still have problems.
>> For the latter, we just need to make sure all the i2c-mux schemas have a
>> $ref to this schema. Also, with that change we'd stop checking 'i2c-mux'
>> nodes that don't yet have a specific schema. That said, I do lean toward
>> the latter option.
> 
> From what I can see there are only two i2c-mux schemas and both of them
> have a $ref to this schema [1][2]
> 
> [1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.yaml#L33
> [2] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml#L16

I'm a relative yaml bindings newbie, but I assume adding "select: false" will
have the side effect of not enforcing this i2c-mux schema on i2c-muxes that
have not yet been converted to yaml? E.g. i2c-mux-gpio.txt, i2c-mux-pinctrl.txt
etc etc. But there are not too many of those. Is it a prerequisite to update
those bindings to yaml before doing "select: false"? Looking further I think
there's a total of about 15-20 drivers doing i2c-muxing (or arbing/gating),
and some of those exist outside the "i2c umbrella".

I wonder if e.g. this one [1] should really reference i2c-controller.yaml as
it is currently doing, or if i2c-mux.yaml is correct?

[1] Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.yaml

Maybe i2c-mux.yaml didn't work in that case because the node names were
"wrong" and did not match the pattern and then someone stuck
i2c-controller.yaml in there simply because that was close enough, and
also happened to work?

Cheers,
Peter