[PATCH v6 1/3] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[PATCH v6 1/3] dt-bindings: clk: vc5: Add properties for configuring the SD/OE pin

[Sean Anderson]

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

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

Changes in v6:
- Use tri-state properties

Changes in v5:
- Don't use dummy if's for oneOfs under allOfs

Changes in v4:
- Specify that bindings should specify these properties, but don't make
  any guarantees about the driver's behavior when they are not present.
- Clarify description of idt,(en|dis)able-shutdown properties.
- Make opposing properties mutually exclusive.
- Add these properties to the example.

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       | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
index 28675b0b80f1..e9fc781a21b5 100644
--- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
+++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
@@ -30,6 +30,20 @@ description: |
     3 -- OUT3
     4 -- OUT4
 
+  The idt,shutdown and idt,output-enable-active 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
+
+  The case where SH and SP are both 1 is likely not very interesting.
+
 maintainers:
   - Luca Ceresoli <luca@lucaceresoli.net>
 
@@ -64,6 +78,26 @@ properties:
     maximum: 22760
     description: Optional load capacitor for XTAL1 and XTAL2
 
+  idt,shutdown:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+    description: |
+      If 1, this enables the shutdown functionality: the chip will be
+      shut down if the SD/OE pin is driven high. If 0, this disables the
+      shutdown functionality: the chip will never be shut down based on
+      the value of the SD/OE pin. This property corresponds to the SH
+      bit of the Primary Source and Shutdown Register.
+
+  idt,output-enable-active:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+    description: |
+      If 1, this enables output when the SD/OE pin is high, and disables
+      output when the SD/OE pin is low. If 0, this disables output when
+      the SD/OE pin is high, and enables output when the SD/OE pin is
+      low. This corresponds to the SP bit of the Primary Source and
+      Shutdown Register.
+
 patternProperties:
   "^OUT[1-4]$":
     type: object
@@ -89,6 +123,8 @@ required:
   - compatible
   - reg
   - '#clock-cells'
+  - idt,shutdown
+  - idt,output-enable-active
 
 allOf:
   - if:
@@ -138,6 +174,10 @@ examples:
             clocks = <&ref25m>;
             clock-names = "xin";
 
+            /* Set the SD/OE pin's settings */
+            idt,shutdown = <0>;
+            idt,output-enable-active = <0>;
+
             OUT1 {
                 idt,drive-mode = <VC5_CMOSD>;
                 idt,voltage-microvolts = <1800000>;
-- 
2.25.1

[PATCH v6 2/3] clk: vc5: Use dev_err_probe

[Sean Anderson]

Convert uses of dev_err (+ return) to dev_err_probe.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
---

(no changes since v5)

Changes in v5:
- Note in commit message that not all dev_errs converted return the
  result of dev_err_probe

Changes in v4:
- New

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

diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
index 344cd6c61188..bfbb51191c8d 100644
--- a/drivers/clk/clk-versaclock5.c
+++ b/drivers/clk/clk-versaclock5.c
@@ -909,10 +909,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return -EPROBE_DEFER;
 
 	vc5->regmap = devm_regmap_init_i2c(client, &vc5_regmap_config);
-	if (IS_ERR(vc5->regmap)) {
-		dev_err(&client->dev, "failed to allocate register map\n");
-		return PTR_ERR(vc5->regmap);
-	}
+	if (IS_ERR(vc5->regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(vc5->regmap),
+				     "failed to allocate register map\n");
 
 	/* Register clock input mux */
 	memset(&init, 0, sizeof(init));
@@ -936,10 +935,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		    __clk_get_name(vc5->pin_clkin);
 	}
 
-	if (!init.num_parents) {
-		dev_err(&client->dev, "no input clock specified!\n");
-		return -EINVAL;
-	}
+	if (!init.num_parents)
+		return dev_err_probe(&client->dev, -EINVAL,
+				     "no input clock specified!\n");
 
 	/* Configure Optional Loading Capacitance for external XTAL */
 	if (!(vc5->chip_info->flags & VC5_HAS_INTERNAL_XTAL)) {
@@ -1078,14 +1076,16 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	ret = of_clk_add_hw_provider(client->dev.of_node, vc5_of_clk_get, vc5);
 	if (ret) {
-		dev_err(&client->dev, "unable to add clk provider\n");
+		dev_err_probe(&client->dev, ret,
+			      "unable to add clk provider\n");
 		goto err_clk;
 	}
 
 	return 0;
 
 err_clk_register:
-	dev_err(&client->dev, "unable to register %s\n", init.name);
+	dev_err_probe(&client->dev, ret,
+		      "unable to register %s\n", init.name);
 	kfree(init.name); /* clock framework made a copy of the name */
 err_clk:
 	if (vc5->chip_info->flags & VC5_HAS_INTERNAL_XTAL)
-- 
2.25.1

[PATCH v6 3/3] clk: vc5: Add properties for configuring SD/OE behavior

[Sean Anderson]

The SD/OE pin may be configured to enable output when high or low, and
to shutdown the device when high. This behavior is controller by the SH
and SP bits of the Primary Source and Shutdown Register (and to a lesser
extent the OS and OE bits). By default, both bits are 0 (unless set by
OTP memory), but they may need to be configured differently, depending
on the external circuitry controlling the SD/OE pin.

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

Changes in v6:
- Use tri-state properties
- Drop Reviewed-bys

Changes in v5:
- Use if (...) mask |= ...; instead of mask = ... ? ... : 0;

Changes in v4:
- Use dev_err_probe over dev_err
- Put new variables on their own line

Changes in v3:
- Default to not changing SH or SP unless there is a property affecting
  them.

Changes in v2:
- Set SH as well as SP

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

diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
index bfbb51191c8d..5b986894bd3b 100644
--- a/drivers/clk/clk-versaclock5.c
+++ b/drivers/clk/clk-versaclock5.c
@@ -886,6 +886,7 @@ static const struct of_device_id clk_vc5_of_match[];
 
 static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
+	unsigned int oe, sd, src_mask = 0, src_val = 0;
 	struct vc5_driver_data *vc5;
 	struct clk_init_data init;
 	const char *parent_names[2];
@@ -913,6 +914,29 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return dev_err_probe(&client->dev, PTR_ERR(vc5->regmap),
 				     "failed to allocate register map\n");
 
+	ret = of_property_read_u32(client->dev.of_node, "idt,shutdown", &sd);
+	if (ret > 0) {
+		src_mask |= VC5_PRIM_SRC_SHDN_EN_GBL_SHDN;
+		if (sd)
+			src_val |= VC5_PRIM_SRC_SHDN_EN_GBL_SHDN;
+	} else if (ret != -EINVAL) {
+		return dev_err_probe(&client->dev, ret,
+				     "could not read idt,shutdown\n");
+	}
+
+	ret = of_property_read_u32(client->dev.of_node,
+				   "idt,output-enable-active", &oe);
+	if (ret > 0) {
+		src_mask |= VC5_PRIM_SRC_SHDN_SP;
+		if (oe)
+			src_val |= VC5_PRIM_SRC_SHDN_SP;
+	} else if (ret != -EINVAL) {
+		return dev_err_probe(&client->dev, ret,
+				     "could not read idt,output-enable-active\n");
+	}
+
+	regmap_update_bits(vc5->regmap, VC5_PRIM_SRC_SHDN, src_mask, src_val);
+
 	/* Register clock input mux */
 	memset(&init, 0, sizeof(init));
 
-- 
2.25.1

Re: [PATCH v6 3/3] clk: vc5: Add properties for configuring SD/OE behavior

[Sean Anderson]

On 7/23/21 7:13 PM, Sean Anderson wrote:
> The SD/OE pin may be configured to enable output when high or low, and
> to shutdown the device when high. This behavior is controller by the SH
> and SP bits of the Primary Source and Shutdown Register (and to a lesser
> extent the OS and OE bits). By default, both bits are 0 (unless set by
> OTP memory), but they may need to be configured differently, depending
> on the external circuitry controlling the SD/OE pin.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v6:
> - Use tri-state properties
> - Drop Reviewed-bys
> 
> Changes in v5:
> - Use if (...) mask |= ...; instead of mask = ... ? ... : 0;
> 
> Changes in v4:
> - Use dev_err_probe over dev_err
> - Put new variables on their own line
> 
> Changes in v3:
> - Default to not changing SH or SP unless there is a property affecting
>    them.
> 
> Changes in v2:
> - Set SH as well as SP
> 
>   drivers/clk/clk-versaclock5.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
> index bfbb51191c8d..5b986894bd3b 100644
> --- a/drivers/clk/clk-versaclock5.c
> +++ b/drivers/clk/clk-versaclock5.c
> @@ -886,6 +886,7 @@ static const struct of_device_id clk_vc5_of_match[];
>   
>   static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
>   {
> +	unsigned int oe, sd, src_mask = 0, src_val = 0;
>   	struct vc5_driver_data *vc5;
>   	struct clk_init_data init;
>   	const char *parent_names[2];
> @@ -913,6 +914,29 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
>   		return dev_err_probe(&client->dev, PTR_ERR(vc5->regmap),
>   				     "failed to allocate register map\n");
>   
> +	ret = of_property_read_u32(client->dev.of_node, "idt,shutdown", &sd);
> +	if (ret > 0) {

Ugh, looks like this should be >=. I guess that's what I get for only compile-testing changes.

--Sean

> +		src_mask |= VC5_PRIM_SRC_SHDN_EN_GBL_SHDN;
> +		if (sd)
> +			src_val |= VC5_PRIM_SRC_SHDN_EN_GBL_SHDN;
> +	} else if (ret != -EINVAL) {
> +		return dev_err_probe(&client->dev, ret,
> +				     "could not read idt,shutdown\n");
> +	}
> +
> +	ret = of_property_read_u32(client->dev.of_node,
> +				   "idt,output-enable-active", &oe);
> +	if (ret > 0) {
> +		src_mask |= VC5_PRIM_SRC_SHDN_SP;
> +		if (oe)
> +			src_val |= VC5_PRIM_SRC_SHDN_SP;
> +	} else if (ret != -EINVAL) {
> +		return dev_err_probe(&client->dev, ret,
> +				     "could not read idt,output-enable-active\n");
> +	}
> +
> +	regmap_update_bits(vc5->regmap, VC5_PRIM_SRC_SHDN, src_mask, src_val);
> +
>   	/* Register clock input mux */
>   	memset(&init, 0, sizeof(init));
>   
> 

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

[Rob Herring]

On Fri, 23 Jul 2021 19:13:04 -0400, Sean Anderson wrote:
> These properties allow configuring the SD/OE pin as described in the
> datasheet.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v6:
> - Use tri-state properties
> 
> Changes in v5:
> - Don't use dummy if's for oneOfs under allOfs
> 
> Changes in v4:
> - Specify that bindings should specify these properties, but don't make
>   any guarantees about the driver's behavior when they are not present.
> - Clarify description of idt,(en|dis)able-shutdown properties.
> - Make opposing properties mutually exclusive.
> - Add these properties to the example.
> 
> 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       | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v6 3/3] clk: vc5: Add properties for configuring SD/OE behavior

[Luca Ceresoli]

Hi Sean,

On 24/07/21 01:13, Sean Anderson wrote:
> The SD/OE pin may be configured to enable output when high or low, and
> to shutdown the device when high. This behavior is controller by the SH
> and SP bits of the Primary Source and Shutdown Register (and to a lesser
> extent the OS and OE bits). By default, both bits are 0 (unless set by
> OTP memory), but they may need to be configured differently, depending
> on the external circuitry controlling the SD/OE pin.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v6:
> - Use tri-state properties
> - Drop Reviewed-bys
> 
> Changes in v5:
> - Use if (...) mask |= ...; instead of mask = ... ? ... : 0;
> 
> Changes in v4:
> - Use dev_err_probe over dev_err
> - Put new variables on their own line
> 
> Changes in v3:
> - Default to not changing SH or SP unless there is a property affecting
>   them.
> 
> Changes in v2:
> - Set SH as well as SP
> 
>  drivers/clk/clk-versaclock5.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
> index bfbb51191c8d..5b986894bd3b 100644
> --- a/drivers/clk/clk-versaclock5.c
> +++ b/drivers/clk/clk-versaclock5.c
> @@ -886,6 +886,7 @@ static const struct of_device_id clk_vc5_of_match[];
>  
>  static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
> +	unsigned int oe, sd, src_mask = 0, src_val = 0;
>  	struct vc5_driver_data *vc5;
>  	struct clk_init_data init;
>  	const char *parent_names[2];
> @@ -913,6 +914,29 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		return dev_err_probe(&client->dev, PTR_ERR(vc5->regmap),
>  				     "failed to allocate register map\n");
>  
> +	ret = of_property_read_u32(client->dev.of_node, "idt,shutdown", &sd);
> +	if (ret > 0) {
> +		src_mask |= VC5_PRIM_SRC_SHDN_EN_GBL_SHDN;
> +		if (sd)
> +			src_val |= VC5_PRIM_SRC_SHDN_EN_GBL_SHDN;
> +	} else if (ret != -EINVAL) {
> +		return dev_err_probe(&client->dev, ret,
> +				     "could not read idt,shutdown\n");

Minor nit: "could not read" sounds like "property not found", i.e. the
property is not present in DT, but if it's not present we do not enter
here (which is OK). For clarity I'd rather say "error reading idt,...".

> +	}
> +
> +	ret = of_property_read_u32(client->dev.of_node,
> +				   "idt,output-enable-active", &oe);
> +	if (ret > 0) {
> +		src_mask |= VC5_PRIM_SRC_SHDN_SP;
> +		if (oe)
> +			src_val |= VC5_PRIM_SRC_SHDN_SP;
> +	} else if (ret != -EINVAL) {
> +		return dev_err_probe(&client->dev, ret,
> +				     "could not read idt,output-enable-active\n");

Ditto.

Otherwise LGTM.

-- 
Luca

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

[Luca Ceresoli]

On 24/07/21 01:13, Sean Anderson wrote:
> These properties allow configuring the SD/OE pin as described in the
> datasheet.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

Re: [PATCH v6 3/3] clk: vc5: Add properties for configuring SD/OE behavior

[Sean Anderson]

On 7/31/21 10:34 AM, Luca Ceresoli wrote:
> Hi Sean,
> 
> On 24/07/21 01:13, Sean Anderson wrote:
>> The SD/OE pin may be configured to enable output when high or low, and
>> to shutdown the device when high. This behavior is controller by the SH
>> and SP bits of the Primary Source and Shutdown Register (and to a lesser
>> extent the OS and OE bits). By default, both bits are 0 (unless set by
>> OTP memory), but they may need to be configured differently, depending
>> on the external circuitry controlling the SD/OE pin.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v6:
>> - Use tri-state properties
>> - Drop Reviewed-bys
>> 
>> Changes in v5:
>> - Use if (...) mask |= ...; instead of mask = ... ? ... : 0;
>> 
>> Changes in v4:
>> - Use dev_err_probe over dev_err
>> - Put new variables on their own line
>> 
>> Changes in v3:
>> - Default to not changing SH or SP unless there is a property affecting
>>   them.
>> 
>> Changes in v2:
>> - Set SH as well as SP
>> 
>>  drivers/clk/clk-versaclock5.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>> 
>> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
>> index bfbb51191c8d..5b986894bd3b 100644
>> --- a/drivers/clk/clk-versaclock5.c
>> +++ b/drivers/clk/clk-versaclock5.c
>> @@ -886,6 +886,7 @@ static const struct of_device_id clk_vc5_of_match[];
>>  
>>  static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>  {
>> +	unsigned int oe, sd, src_mask = 0, src_val = 0;
>>  	struct vc5_driver_data *vc5;
>>  	struct clk_init_data init;
>>  	const char *parent_names[2];
>> @@ -913,6 +914,29 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>  		return dev_err_probe(&client->dev, PTR_ERR(vc5->regmap),
>>  				     "failed to allocate register map\n");
>>  
>> +	ret = of_property_read_u32(client->dev.of_node, "idt,shutdown", &sd);
>> +	if (ret > 0) {
>> +		src_mask |= VC5_PRIM_SRC_SHDN_EN_GBL_SHDN;
>> +		if (sd)
>> +			src_val |= VC5_PRIM_SRC_SHDN_EN_GBL_SHDN;
>> +	} else if (ret != -EINVAL) {
>> +		return dev_err_probe(&client->dev, ret,
>> +				     "could not read idt,shutdown\n");
> 
> Minor nit: "could not read" sounds like "property not found", i.e. the
> property is not present in DT, but if it's not present we do not enter
> here (which is OK). For clarity I'd rather say "error reading idt,...".

dev_err_probe adds to this message, so example output might be

	vc5 1-006a: error 61: could not read idt,shutdown

I think this makes this clearer that there was a problem doing the reading.

> 
>> +	}
>> +
>> +	ret = of_property_read_u32(client->dev.of_node,
>> +				   "idt,output-enable-active", &oe);
>> +	if (ret > 0) {
>> +		src_mask |= VC5_PRIM_SRC_SHDN_SP;
>> +		if (oe)
>> +			src_val |= VC5_PRIM_SRC_SHDN_SP;
>> +	} else if (ret != -EINVAL) {
>> +		return dev_err_probe(&client->dev, ret,
>> +				     "could not read idt,output-enable-active\n");
> 
> Ditto.
> 
> Otherwise LGTM.
>