Re: [PATCH v3 1/2] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[Luca Ceresoli <luca@lucaceresoli.net>]

Hi Sean,

On 29/06/21 17:47, Sean Anderson wrote:
> These properties allow configuring the SD/OE pin as described in the
> datasheet.

*Many* thanks for addressing this issue so quickly!

> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Acked-by: Rob Herring <robh@kernel.org>

I don't think Rob's ack should be present, he hasn't approved _this_
version of the patch.

> ---
> 
> Changes in v3:
> - Add idt,disable-shutdown and idt,output-enable-active-low to allow for
>   a default of not changing the SP/SH bits at all.
> 
> Changes in v2:
> - Rename idt,sd-active-high to idt,output-enable-active-high
> - Add idt,enable-shutdown
> 
>  .../bindings/clock/idt,versaclock5.yaml       | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> index 28675b0b80f1..51f0f78cc3f4 100644
> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> @@ -30,6 +30,22 @@ description: |
>      3 -- OUT3
>      4 -- OUT4
>  
> +  The idt,(en|dis)able-shutdown and idt,output-enable-active-(high|low)
> +  properties control the SH (en_global_shutdown) and SP bits of the
> +  Primary Source and Shutdown Register, respectively. Their behavior is
> +  summarized by the following table:
> +
> +  SH SP Output when the SD/OE pin is Low/High
> +  == == =====================================
> +   0  0 Active/Inactive
> +   0  1 Inactive/Active
> +   1  0 Active/Shutdown
> +   1  1 Inactive/Shutdown
> +
> +  If no properties related to these bits are specified, then they will
> +  be left in their default state. This may be useful if the SH and SP
> +  bits are set to a default value using the OTP memory.

This paragraph looks more an implementation description than a hardware
description.

I suggest something like (possibly better rephrased):

It is recommended to specify the two properties that describe the
hardware. The lack of them leaves the value unspecified and thus opens
to the risk of future incompatibilities, depending on implementation
details.

> @@ -64,6 +80,34 @@ properties:
>      maximum: 22760
>      description: Optional load capacitor for XTAL1 and XTAL2
>  
> +  idt,enable-shutdown:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: |
> +      Enable the shutdown function when the SD/OE pin is high. This
> +      corresponds to setting the SH bit of the Primary Source and
> +      Shutdown Register.
> +
> +  idt,disable-shutdown:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: |
> +      Disable the shutdown function for the SD/OE pin. This corresponds
> +      to clearing the SH bit of the Primary Source and Shutdown
> +      Register.

Saying "Disable the shutdown function" leaves a hole, it is not telling
what gets enabled. I'd rephrase using positive logic:

  Enable the OE (output enable) function for the SD/OE pin. This...

But there are too many "enable" words in it now, it's confusing, so why not:

  Choose the OE (output enable) function for the SD/OE pin. This...

And change correspondingly the idt,enable-shutdown description:
s/^Enable/Choose/.

Also it would be nice to declare in DT that the two flags are mutually
exclusive (same for idt,output-enable-active-*).

-- 
Luca