[PATCH v14 07/16] dt-bindings: fix sifive gpio properties

[Damien Le Moal]

On 2021/02/04 9:47, Damien Le Moal wrote:
> On Wed, 2021-02-03 at 14:41 -0600, Rob Herring wrote:
>> On Wed, Feb 3, 2021 at 6:52 AM Damien Le Moal <Damien.LeMoal at wdc.com> wrote:
>>>
>>> On Tue, 2021-02-02 at 13:02 -0600, Rob Herring wrote:
>>>> On Tue, Feb 2, 2021 at 4:36 AM Damien Le Moal <damien.lemoal at wdc.com> wrote:
>>>>>
>>>>> The sifive gpio IP block supports up to 32 GPIOs. Reflect that in the
>>>>> interrupts property description and maxItems. Also add the standard
>>>>> ngpios property to describe the number of GPIOs available on the
>>>>> implementation.
>>>>>
>>>>> Also add the "canaan,k210-gpiohs" compatible string to indicate the use
>>>>> of this gpio controller in the Canaan Kendryte K210 SoC. If this
>>>>> compatible string is used, do not define the clocks property as
>>>>> required as the K210 SoC does not have a software controllable clock
>>>>> for the Sifive gpio IP block.
>>>>>
>>>>> Cc: Paul Walmsley <paul.walmsley at sifive.com>
>>>>> Cc: Rob Herring <robh at kernel.org>
>>>>> Cc: devicetree at vger.kernel.org
>>>>> Signed-off-by: Damien Le Moal <damien.lemoal at wdc.com>
>>>>> ---
>>>>>  .../devicetree/bindings/gpio/sifive,gpio.yaml | 21 ++++++++++++++++---
>>>>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
>>>>> index ab22056f8b44..2cef18ca737c 100644
>>>>> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
>>>>> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
>>>>> @@ -16,6 +16,7 @@ properties:
>>>>>        - enum:
>>>>>            - sifive,fu540-c000-gpio
>>>>>            - sifive,fu740-c000-gpio
>>>>> +          - canaan,k210-gpiohs
>>>>>        - const: sifive,gpio0
>>>>>
>>>>>    reg:
>>>>> @@ -23,9 +24,9 @@ properties:
>>>>>
>>>>>    interrupts:
>>>>>      description:
>>>>> -      interrupt mapping one per GPIO. Maximum 16 GPIOs.
>>>>> +      interrupt mapping one per GPIO. Maximum 32 GPIOs.
>>>>>      minItems: 1
>>>>> -    maxItems: 16
>>>>> +    maxItems: 32
>>>>>
>>>>>    interrupt-controller: true
>>>>>
>>>>> @@ -38,6 +39,10 @@ properties:
>>>>>    "#gpio-cells":
>>>>>      const: 2
>>>>>
>>>>> +  ngpios:
>>>>> +    minimum: 1
>>>>> +    maximum: 32
>>>>
>>>> What's the default as obviously drivers already assume something.
>>>>
>>>> Does a driver actually need to know this? For example, does the
>>>> register stride change or something?
>>>>
>>>> Please don't add it if the only purpose is error check your DT (IOW,
>>>> if it just checks the max cell value in gpios phandles).
>>>
>>> If I remove that, make dtbs_check complains. Looking at othe gpio controller
>>> bindings, they all have it. So isn't it better to be consistent, and avoid make
>>> dtbs_check errors ?
>>
>> That would mean you are already using 'ngpios' and it is undocumented
>> (for this binding). If already in use and possibly having users then
>> that changes things, but that's not what the commit msg says.
>>
>> Not *all* gpio controllers have ngpios. It's a good number, but
>> probably more than need it though. If we wanted it everywhere, there
>> would be a schema enforcing that.
> 
> If I remove the minimum and maximum lines, I get this error:
> 
> ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml:42:10: [error] empty
> value in block mapping (empty-values)
>   CHKDT   Documentation/devicetree/bindings/processed-schema-examples.json
> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive
> ,gpio.yaml: properties:ngpios: None is not of type 'object', 'boolean'
>   SCHEMA  Documentation/devicetree/bindings/processed-schema-examples.json
> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive
> ,gpio.yaml: ignoring, error in schema: properties: ngpios
> warning: no schema found in file:
> ./Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
> 
> If I remove the ngpios property entirely, then I get a hit on the device tree:
> 
>   CHECK   arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml
> /linux/arch/riscv/boot/dts/canaan/sipeed_maix_bit.dt.yaml:
> gpio-controller at 38001000: 'ngpios' does not match any of the regexes: 'pinctrl-
> [0-9]+'
> 	From schema:
> /home/damien/Projects/RISCV/linux/Documentation/devicetree/bindings/gpio/sifive
> ,gpio.yaml
> 
> Now, If I change the property definition to this:
> 
> diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
> b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
> index 2cef18ca737c..5c7865180383 100644
> --- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
> +++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
> @@ -40,8 +40,11 @@ properties:
>      const: 2
>  
>    ngpios:
> -    minimum: 1
> -    maximum: 32
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The number of GPIO pins implemented by the controller.
> +      It is 16 for the SiFive SoCs and 32 for the Canaan K210 SoC.
> +
>  
>    gpio-controller: true
> 
> Then all is OK.
> 
> Which option should I go for here ? If we want to avoid a dtbs_check error, as
> far as I can see, we can:
> 1) Remove the ngpios property and remove its use from the DTS, which is not
> nice in my opinion
> 2) Use the modification proposed above
> 
> Please advise. Thanks !
> 

Rob,

Thanks for the reviews and acks on the other patches. I would like to send v16
with your suggested fixes. But what should I do about ngpios ? Option (1) or (2)
above ? Any other option ?

Thanks !

-- 
Damien Le Moal
Western Digital Research