Re: [PATCH v2 09/21] dt-bindings: Document canaan,k210-fpioa bindings

[Damien Le Moal <Damien.LeMoal@xxxxxxx>]

On 2020/11/24 18:49, Geert Uytterhoeven wrote:
> Hi Damien,
> 
> On Tue, Nov 24, 2020 at 5:40 AM Damien Le Moal <damien.lemoal@xxxxxxx> wrote:
>> Document the device tree bindings for the Canaan Kendryte K210 SoC
>> Fully Programmable IO Array (FPIOA) pinctrl driver in
>> Documentation/devicetree/bindings/pinctrl/canaan,k210-fpioa.yaml. The
>> new header file include/dt-bindings/pinctrl/k210-fpioa.h is added to
>> define all 256 possible pin functions of the SoC IO pins, as well as
>> macros simplifying the definition of pin functions in a device tree.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxx>
> 
> Thanks for your patch!
> 
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/canaan,k210-fpioa.yaml
> 
>> +  canaan,k210-sysctl-power:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description: |
>> +      phandle of the K210 system controller node and offset of the its
> 
> of its
> 
>> +      power domain control register.
> 
> Your k210-sysctl-v15 branch has a bogus trailing space here.

Oops. Forgot to push the fixed up patches. Just did it now (forced update
k210-sysctl-v15).

> 
>> +
>> +patternProperties:
> 
>> +  '-pins$':
>> +    type: object
>> +    $ref: /schemas/pinctrl/pincfg-node.yaml
>> +    description:
>> +      FPIOA client devices use sub-nodes to define the desired
>> +      configuration of pins. Client device sub-nodes use the
>> +      properties below.
>> +
>> +    properties:
> 
>> +      input-schmitt: true
>> +
>> +      input-schmitt-enable: true
> 
> Do you need both?
> Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml documents
> input-schmitt-enable and input-schmitt-disable, but not input-schmitt.

Ah. Yes. My bad. Will correct that.

> 
>> +
>> +      intput-polarity-invert:
> 
> input-polarity-invert
> 
>> +        description:
>> +          Enable or disable pin input polarity inversion.
> 
> Still, this is a non-standard property.  Do you need it, or can this be
> handled by the software GPIO_ACTIVE_LOW flag?

For GPIO, yes, that would work. But not all pin functions are GPIOs on this SoC,
and the hardware supports this. Strictly speaking, this is not really necessary
as pins that needs inversion have that coded in the function code given by the
pinmux node. So we could remove this. But that is not a lot of code at all in
the driver to support it and the hardware allows that to be set per pin,
regardless of the pin function assigned in the pinmux node.

> 
>> +      output-polarity-invert:
>> +        description:
>> +          Enable or disable pin output polarity inversion.
> 
> Same comment as for input.

Same answer :)

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 


-- 
Damien Le Moal
Western Digital Research