linux-kernel - Re: [PATCH 05/10] dt-bindings: mfd: cirrus,cs42l43: Add initial DT binding

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: <015c5208-ac85-8a14-3455-c70781fd92f8@linaro.org>
Date:   Sat, 13 May 2023 20:05:34 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc:     broonie@...nel.org, lee@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        tglx@...utronix.de, maz@...nel.org, linus.walleij@...aro.org,
        vkoul@...nel.org, lgirdwood@...il.com,
        yung-chuan.liao@...ux.intel.com, sanyog.r.kale@...el.com,
        pierre-louis.bossart@...ux.intel.com, alsa-devel@...a-project.org,
        patches@...nsource.cirrus.com, devicetree@...r.kernel.org,
        linux-gpio@...r.kernel.org, linux-spi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/10] dt-bindings: mfd: cirrus,cs42l43: Add initial DT
 binding

On 12/05/2023 18:15, Charles Keepax wrote:
> On Fri, May 12, 2023 at 05:23:22PM +0200, Krzysztof Kozlowski wrote:
>> On 12/05/2023 14:28, Charles Keepax wrote:
>>> +$id: http://devicetree.org/schemas/mfd/cirrus,cs42l43.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Cirrus Logic CS42L43 Audio CODEC
>>
>> That's audio codec, so it should be in sound, not MFD.
>>
> 
> Is this true even despite the device being implemented as an MFD?

Implementation in Linux almost does not matter here. Bindings location
match the device main purpose. We indeed stuff into MFD bindings things
like PMIC, because PMIC is a bit more than just regulators, and we do
not have here subsystem for PMICs. If you call it Audio Codec, then I
vote for sound directory for bindings.

> I am happy to move it, and will do so unless I hear otherwise.
> 
>>> +  - VDD_P-supply
>>> +  - VDD_A-supply
>>> +  - VDD_D-supply
>>> +  - VDD_IO-supply
>>> +  - VDD_CP-supply
>>
>> lowercase, no underscores in all property names.
> 
> I guess we can rename all the regulators to lower case.
> 
>>> +additionalProperties: false
>>
>> This order is quite unexpected... please do not invent your own layout.
>> Use example-schema as your starting point. I suspect there will be many
>> things to fix, so limited review follows (not complete).
>>
>>
>> Missing ref to dai-common
> 
> Apologies for that I was a little hesitant about this but this
> order did make the binding document much more readable, the
> intentation got really hard to follow in the traditional order. I
> guess since I have things working now I can put it back, again I
> will do so unless I hear otherwise.

The additional/unevaluatedProperties from child nodes are indeed moved
then up - following the property:
   pinctrl:
     type: object
     additionalProperties: false

but that's exception and for the rest I don't see any troubles with
indentation. That would be the only binding... so what's here so special?

> 
>>> +  pinctrl:
>>> +    type: object
>>
>> additionalProperties: false
>>
> 
> Can do.
> 
>>> +
>>> +    allOf:
>>> +      - $ref: "../pinctrl/pinctrl.yaml#"
>>
>> No quotes, absolute path, so /schemas/pinctrl/....
>>
> 
> Can do.
> 
>>> +
>>> +    properties:
>>> +      pin-settings:
>>
>> What's this node about? pinctrl/pinctrl/pins? One level too much.
>>
> 
> codec/pinctrl/pins
> 
> The device is a codec, so the main node should be called codec,
> then it has a subnode called pinctrl to satisfy the pinctrl DT
> binding.

Sure, but then you do not need pin-settings. Look at Qualcomm bindings
for example:

Documentation/devicetree/bindings/pinctrl/qcom,sm8550-tlmm.yaml

Best regards,
Krzysztof