On 6/29/21 5:23 PM, Luca Ceresoli wrote:

Hi Sean,

On 29/06/21 17:47, Sean Anderson wrote:

quoted

These properties allow configuring the SD/OE pin as described in the
datasheet.

*Many* thanks for addressing this issue so quickly!

quoted

Signed-off-by: Sean Anderson <redacted>
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.

Sorry, I was unsure whether I should keep it or not.

quoted

---

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.

It of course *is* an implementation description. As Geert found out, it
is important to keep the defaults if none of these properties are
specified.

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.

Ok, so if I understand correctly, you would like to deprecate existing
bindings which do not specify any of these properties.

quoted

@@ -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...

The issue here is that the OE function is in some sense always enabled.
So perhaps a better wording would be

	Disable the shutdown functionality. The chip will never be
	shut down based on the value of the SD/OE pin.

And for enable-shutdown

	Enable the shutdown functionality. The chip will be shut down if
	the SD/OE pin is driven high.

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-*).

Ok, will fix in v4.

--Sean