Re: [PATCH 1/2] dt-bindings: clk: vc5: Add property for SD polarity

[Luca Ceresoli <luca@lucaceresoli.net>]

Hi Sean,

On 07/06/21 17:49, Sean Anderson wrote:
> This property allows setting the SD/OE pin's polarity to active-high,
> instead of the default of active-low.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>

Thanks.

> +  idt,sd-active-high:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: SD/OE pin polarity is active-high

I think the name "sd" is misleading.

In the Renesas docs (which are very confusing on their own about this
topic) this bit is called "SP" -- *S*D/OE *P*olarity. But actually it
controls polarity of the SD/OE pin only if the pin is configured for
"OE" function:

> SP bit = “SD/OE pin Polarity Bit”: Set the polarity of the SD/OE
> pin where outputs enable or disable. Only works with OE, not with SD.
(VC6E register and programming guide [0])

As such I suggest you use either "sp" to keep the naming used in the
Renesas docs or "oe" as it actually controls OE polarity only. I do
prefer "sp" as it helps matching with the datasheets, but maybe adding a
little more detail in bindings docs to clarify, as in:

 idt,sp-active-high:
   $ref: /schemas/types.yaml#/definitions/flag
   description: SD/OE pin polarity is active-high
                (only works when SD/OE pin is configured as OE)

BTW is it only me finding the "Shutdown Function" of [0] completely
confusing? Also, Table 24 has contradictory lines and missing lines. I'm
sending a request to Renesas support to ask them to clarify it all.

[0]
https://www.renesas.com/eu/en/document/mau/versaclock-6e-family-register-descriptions-and-programming-guide

-- 
Luca