| 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
| ||
|
Message-ID: <2cfed688-f401-e69e-1ff4-f775c6d90f64@linaro.org>
Date: Thu, 20 Oct 2022 08:21:43 -0400
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Yassine Oudjana <yassine.oudjana@...il.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Sean Wang <sean.wang@...nel.org>,
Andy Teng <andy.teng@...iatek.com>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>,
Yassine Oudjana <y.oudjana@...tonmail.com>,
linux-mediatek@...ts.infradead.org, linux-gpio@...r.kernel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 06/10] dt-bindings: pinctrl: mediatek,mt6779-pinctrl:
Add MT6795
On 20/10/2022 07:36, Yassine Oudjana wrote:
>
> On Mon, Oct 10 2022 at 07:24:59 -04:00:00, Krzysztof Kozlowski
> <krzysztof.kozlowski@...aro.org> wrote:
>> On 07/10/2022 08:59, Yassine Oudjana wrote:
>>> From: Yassine Oudjana <y.oudjana@...tonmail.com>
>>>
>>> Combine MT6795 pin controller document into MT6779 one. In the
>>> process, replace the current interrupts property description with
>>> the one from the MT6795 document since it makes more sense. Also
>>> amend property descriptions and examples with more detailed
>>> information that was available in the MT6795 document, and replace
>>> the current pinmux node name patterns with ones from it since they
>>> are more common across mediatek pin controller bindings.
>>>
>>> Signed-off-by: Yassine Oudjana <y.oudjana@...tonmail.com>
>>> ---
>>> .../pinctrl/mediatek,mt6779-pinctrl.yaml | 94 ++++++--
>>> .../pinctrl/mediatek,pinctrl-mt6795.yaml | 227
>>> ------------------
>>> 2 files changed, 77 insertions(+), 244 deletions(-)
>>> delete mode 100644
>>> Documentation/devicetree/bindings/pinctrl/mediatek,pinctrl-mt6795.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
>>> b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
>>> index a2141eb0854e..cada3530dd0a 100644
>>> ---
>>> a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
>>> +++
>>> b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
>>> @@ -8,6 +8,7 @@ title: Mediatek MT6779 Pin Controller
>>>
>>> maintainers:
>>> - Andy Teng <andy.teng@...iatek.com>
>>> + - AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@...labora.com>
>>> - Sean Wang <sean.wang@...nel.org>
>>>
>>> description:
>>> @@ -18,6 +19,7 @@ properties:
>>> compatible:
>>> enum:
>>> - mediatek,mt6779-pinctrl
>>> + - mediatek,mt6795-pinctrl
>>> - mediatek,mt6797-pinctrl
>>>
>>> reg:
>>> @@ -43,9 +45,10 @@ properties:
>>> interrupt-controller: true
>>>
>>> interrupts:
>>> - maxItems: 1
>>> + minItems: 1
>>> + maxItems: 2
>>> description: |
>>> - Specifies the summary IRQ.
>>> + The interrupt outputs to sysirq.
>>
>> I am not sure if description is relevant now for all variants... what
>> is
>> the sysirq? You have two interrupts so both go to one sysirq?
>
> It's the system interrupt controller and it has several inputs. Both
> interrupts go to it.
Then the naming is confusing because "sysirq" sounds like "system
interrupt".
>
>>
>>>
>>> "#interrupt-cells":
>>> const: 2
>>> @@ -81,6 +84,30 @@ allOf:
>>> - const: iocfg_lt
>>> - const: iocfg_tl
>>> - const: eint
>>> +
>>> + interrupts:
>>> + items:
>>> + - description: EINT interrupt
>>> +
>>> + - if:
>>> + properties:
>>> + compatible:
>>> + contains:
>>> + const: mediatek,mt6795-pinctrl
>>> + then:
>>> + properties:
>>> + reg:
>>> + minItems: 2
>>
>> What's the maxItems? You declared reg and reg-names in top-level
>> properties as accepting anything, therefore you cannot have loose
>> constraints here.
>
> That was an oversight. I'll fix it.
>>
>>> +
>>> + reg-names:
>>> + items:
>>> + - const: base
>>> + - const: eint
>>> +
>>> + interrupts:
>>> + items:
>>> + - description: EINT interrupt
>>> + - description: EINT event_b interrupt
>>
>> Blank line
>>
>>> - if:
>>> properties:
>>> compatible:
>>> @@ -111,32 +138,50 @@ allOf:
>>> - "#interrupt-cells"
>>>
>>> patternProperties:
>>> - '-[0-9]*$':
>>> + '-pins$':
>>> type: object
>>> additionalProperties: false
>>>
>>> patternProperties:
>>> - '-pins*$':
>>> + '^pins':
>>> type: object
>>> description: |
>>> A pinctrl node should contain at least one subnodes
>>> representing the
>>> pinctrl groups available on the machine. Each subnode
>>> will list the
>>> pins it needs, and how they should be configured, with
>>> regard to muxer
>>> - configuration, pullups, drive strength, input
>>> enable/disable and input schmitt.
>>> - $ref: "/schemas/pinctrl/pincfg-node.yaml"
>>> + configuration, pullups, drive strength, input
>>> enable/disable and
>>> + input schmitt.
>>> + $ref: "pinmux-node.yaml"
>>
>> Drop quotes
>>
>> Why this one is not pincfg-node anymore? All your properties are not
>> valid then? You mix here so many changes it is a bit difficult to
>> understand the concept.
>
> Seems like I didn't pay enough attention to that. This node actually
> takes both pinmux-node (pinmux specifically) and pincfg-node
> properties, so would it make sense to add ref for both?
Yes, and make changes in organized way, easier to read...
>
>>
>>>
>>> properties:
>>> pinmux:
>>> description:
>>> - integer array, represents gpio pin number and mux
>>> setting.
>>> - Supported pin number and mux varies for different
>>> SoCs, and are defined
>>> - as macros in boot/dts/<soc>-pinfunc.h directly.
>>> + Integer array, represents gpio pin number and mux
>>> setting.
>>> + Supported pin number and mux varies for different
>>> SoCs, and are
>>> + defined as macros in
>>> dt-bindings/pinctrl/<soc>-pinfunc.h
>>> + directly.
>>>
>>> bias-disable: true
>>>
>>> - bias-pull-up: true
>>> -
>>> - bias-pull-down: true
>>> + bias-pull-up:
>>> + oneOf:
>>> + - type: boolean
>>> + - enum: [100, 101, 102, 103]
>>
>> Missing ref
>>
>>> + description: Pull up PUPD/R0/R1 type define value.
>>> + description: |
>>> + For normal pull up type, it is not necessary to
>>> specify R1R0
>>> + values; When pull up type is PUPD/R0/R1, adding
>>> R1R0 defines
>>> + will set different resistance values.
>>> +
>>> + bias-pull-down:
>>> + oneOf:
>>> + - type: boolean
>>> + - enum: [100, 101, 102, 103]
>>
>> Missing ref
>>
>>> + description: Pull down PUPD/R0/R1 type define
>>> value.
>>> + description: |
>>> + For normal pull down type, it is not necessary to
>>> specify R1R0
>>> + values; When pull down type is PUPD/R0/R1, adding
>>> R1R0 defines
>>> + will set different resistance values.
>>>
>>> input-enable: true
>>>
>>> @@ -151,7 +196,7 @@ patternProperties:
>>> input-schmitt-disable: true
>>>
>>> drive-strength:
>>> - enum: [2, 4, 8, 12, 16]
>>> + enum: [2, 4, 6, 8, 10, 12, 14, 16]
>>
>> Now you are missing ref - you do not have a type now, because you
>> removed pincfg-node. Split the merging of different pinctrl bindings
>> and
>> reorganization.
>
> Will do.
>
>>
>> The drive strengths are also not valid for the other variant...
>
> Actually the supported drive strengths vary between pins of a single
> variant, so technically they have never been described completely
> accurately. The old drive strenghs are a superset of strengths
> supported by pins on the MT6779 pin controller, and this change expands
> the superset with values supported by some pins in MT6795. Would it be
> better to move this to the conditionals to have it defined per variant?
If they vary, then yes.
Best regards,
Krzysztof
Powered by blists - more mailing lists