Re: [PATCH v3 1/2] dt-bindings: gpio: gpio-mvebu: convert txt binding to YAML

[Chris Packham <Chris.Packham@alliedtelesis.co.nz>]

On 15/05/22 08:28, Krzysztof Kozlowski wrote:
> On 14/05/2022 04:20, Chris Packham wrote:
>>>> +
>>>> +allOf:
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            const: marvell,armada-8k-gpio
>>>> +    then:
>>>> +      required:
>>>> +        - offset
>>>> +    else:
>>>> +      required:
>>>> +        - reg
>>> one blank line please
>>>
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            const: marvell,armadaxp-gpio
>>> Original bindings are saying that second reg is optional for
>>> marvell,armada-370-gpio. What about other cases, e.g. mv78200-gpio? Is
>>> it also allowed (and optional) there?
>> This is where things get interesting. The armadaxp (and only the
>> armadaxp) requires a second register value for some per-cpu registers.
>> All of the other SoCs can have an optional 2nd register value if they
>> want to use the PWM function. I guess that implies that the armadaxp
>> can't do PWM.
>>>> +    then:
>>>> +      properties:
>>>> +        reg:
>>>> +          minItems: 2
>>> Then you also should require two reg-names.
>> Simple enough to add. But currently we've said that the reg-names are
>> "gpio" and "pwm" but on the armadaxp the 2nd one is not "pwm" but
>> something else ("per-cpu" perhaps?)
> In such case they would be failing with current bindings, because they
> expect "pwm" as second name, right?

The driver is alright because it checks for the 
"marvell,armada-370-gpio" compatible before using 
platform_get_resource_byname("pwm"). The unused (at least in theory) 
armadaxp code just calls devm_platform_ioremap_resource(pdev, 1) so it 
doesn't care about the name.

I'm tempted to leave the regName: minItems: 2 out because the armadaxp 
code doesn't care about them and setting the 2nd regname to "pwm" when 
the compatible is armadaxp would be misleading.

>> On the other hand this is all completely moot because the
>> armada-xp-mv78*.dtsi actually use the "marvell,armada-370-gpio"
>> compatible so this appears to be documenting something that is no longer
>> used. Indeed it appears that the armadaxp specific usage was remove in
>> 5f79c651e81e ("arm: mvebu: use global interrupts for GPIOs on Armada XP").
>>
>> So perhaps the best course of action is to drop marvell,armadaxp-gpio
>> from the new binding (noting that we've done so in the commit message).
>
> That's fine, maybe in a separate patch (2nd one)?
I'll add another patch on top that marks the compatible as deprecated 
including some of the above info in the commit message.
>
> Best regards,
> Krzysztof