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@xxxxxxxx> > Acked-by: Rob Herring <robh@xxxxxxxxxx> 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
