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

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

[Sean Anderson]

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

 Documentation/devicetree/bindings/clock/idt,versaclock5.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
index 28675b0b80f1..e6406c0db624 100644
--- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
+++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
@@ -64,6 +64,10 @@ properties:
     maximum: 22760
     description: Optional load capacitor for XTAL1 and XTAL2
 
+  idt,sd-active-high:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: SD/OE pin polarity is active-high
+
 patternProperties:
   "^OUT[1-4]$":
     type: object
-- 
2.25.1

[PATCH 2/2] clk: vc5: Add property to set SD/OE pin polarity

[Sean Anderson]

This pin has configurable polarity, defaulting to active-low. Add a
prioperty to allow setting it to active-high. This may be necessary if the
pin is driven high and there is no GPIO allocated for it.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/clk/clk-versaclock5.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
index 344cd6c61188..4e2461cb3950 100644
--- a/drivers/clk/clk-versaclock5.c
+++ b/drivers/clk/clk-versaclock5.c
@@ -914,6 +914,10 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return PTR_ERR(vc5->regmap);
 	}
 
+	if (of_property_read_bool(client->dev.of_node, "idt,sd-active-high"))
+		regmap_update_bits(vc5->regmap, VC5_PRIM_SRC_SHDN,
+				   VC5_PRIM_SRC_SHDN_SP, VC5_PRIM_SRC_SHDN_SP);
+
 	/* Register clock input mux */
 	memset(&init, 0, sizeof(init));
 
-- 
2.25.1

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

[Luca Ceresoli]

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

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

[Sean Anderson]

On 6/10/21 5:05 AM, Luca Ceresoli wrote:
 > 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.

I do as well. After sending this patch, I reviewed the documentation
again and discovered that the functionality was not as clear as I
initially thought.

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

I rearranged the table to highlight which bits cause the output to
become inactive:

SH	SP	OSn	OEn	SD/OE	OUT
x	x	1	0	x	Active
0	0	1	1	0	Active
0	0	1	1	1	Inactive
0	1	1	1	0	Inactive
0	1	1	1	1	Active
1	0	1	1	0	Active
1	0	x	x	1	Shutdown
1	1	1	1	0	Inactive
1	1	x	x	1	Shutdown
x	x	0	x	x	Inactive

This may be condensed to

SH	SP	SD/OE function for 0/1
0	0	Active/Inactive
0	1	Inactive/Active
1	0	Active/Shutdown
1	1	Inactive/Shutdown

According to the datasheet, the default settings are SH=0 and SP=0. So
perhaps a good set of properties would be

idt,enable-shutdown:
	Shutdown the device when the SD/OE pin is high. This would set
	SH=1.
idt,output-enable-active-high:
	Disable output when the SD/OE pin is low. This would set SP=1.

--Sean

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

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

[Luca Ceresoli]

Hi Sean,

On 10/06/21 17:43, Sean Anderson wrote:
> 
> 
> On 6/10/21 5:05 AM, Luca Ceresoli wrote:
>> 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.
> 
> I do as well. After sending this patch, I reviewed the documentation
> again and discovered that the functionality was not as clear as I
> initially thought.
> 
>> 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.
> 
> I rearranged the table to highlight which bits cause the output to
> become inactive:
> 
> SH    SP    OSn    OEn    SD/OE    OUT
> x    x    1    0    x    Active
> 0    0    1    1    0    Active
> 0    0    1    1    1    Inactive
> 0    1    1    1    0    Inactive
> 0    1    1    1    1    Active
> 1    0    1    1    0    Active
> 1    0    x    x    1    Shutdown
> 1    1    1    1    0    Inactive
> 1    1    x    x    1    Shutdown
> x    x    0    x    x    Inactive>
> This may be condensed to
> 
> SH    SP    SD/OE function for 0/1
> 0    0    Active/Inactive
> 0    1    Inactive/Active
> 1    0    Active/Shutdown
> 1    1    Inactive/Shutdown
> 
> According to the datasheet, the default settings are SH=0 and SP=0. So
> perhaps a good set of properties would be
> 
> idt,enable-shutdown:
>     Shutdown the device when the SD/OE pin is high. This would set
>     SH=1.
> idt,output-enable-active-high:
>     Disable output when the SD/OE pin is low. This would set SP=1.

Seems good.

-- 
Luca