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
