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

[Sean Anderson <sean.anderson@seco.com>]

On 6/29/21 5:23 PM, Luca Ceresoli wrote:
> 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.

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

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

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