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

[PATCH v4 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 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       | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
index 28675b0b80f1..2631a175612b 100644
--- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
+++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
@@ -30,6 +30,21 @@ 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
+
+  One of idt,(en|dis)able-shutdown and one of
+  idt,output-enable-active-(high|low) should be specified.
+
 maintainers:
   - Luca Ceresoli <luca@lucaceresoli.net>
 
@@ -64,6 +79,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 functionality. The chip will be shut down if
+      the SD/OE pin is driven 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 functionality. The chip will never be shut
+      down based on the value of the SD/OE pin. This corresponds to
+      clearing the SH bit of the Primary Source and Shutdown Register.
+
+  idt,output-enable-active-high:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: |
+      This enables output when the SD/OE pin is high, and disables
+      output when the SD/OE pin is low. This corresponds to setting the
+      SP bit of the Primary Source and Shutdown Register.
+
+  idt,output-enable-active-low:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: |
+      This disables output when the SD/OE pin is high, and enables
+      output when the SD/OE pin is low. This corresponds to clearing the
+      SP bit of the Primary Source and Shutdown Register.
+
 patternProperties:
   "^OUT[1-4]$":
     type: object
@@ -109,6 +152,22 @@ allOf:
       required:
         - clock-names
         - clocks
+  - if:
+      true
+    then:
+      oneOf:
+        - required:
+            - idt,enable-shutdown
+        - required:
+            - idt,disable-shutdown
+  - if:
+      true
+    then:
+      oneOf:
+        - required:
+            - idt,output-enable-active-high
+        - required:
+            - idt,output-enable-active-low
 
 additionalProperties: false
 
@@ -138,6 +197,10 @@ examples:
             clocks = <&ref25m>;
             clock-names = "xin";
 
+            /* Set the SD/OE pin's settings */
+            idt,disable-shutdown;
+            idt,output-enable-active-low;
+
             OUT1 {
                 idt,drive-mode = <VC5_CMOSD>;
                 idt,voltage-microvolts = <1800000>;
-- 
2.25.1

[PATCH v4 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>
---

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

diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
index bfbb51191c8d..1fab4d7259a7 100644
--- a/drivers/clk/clk-versaclock5.c
+++ b/drivers/clk/clk-versaclock5.c
@@ -886,6 +886,8 @@ static const struct of_device_id clk_vc5_of_match[];
 
 static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
+	bool oe_high, oe_low, sh_enable, sh_disable;
+	unsigned int sp_mask, sh_mask, sp_val, sh_val;
 	struct vc5_driver_data *vc5;
 	struct clk_init_data init;
 	const char *parent_names[2];
@@ -913,6 +915,25 @@ 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");
 
+	oe_high = of_property_read_bool(client->dev.of_node,
+					"idt,output-enable-active-high");
+	oe_low = of_property_read_bool(client->dev.of_node,
+					"idt,output-enable-active-low");
+	sh_enable = of_property_read_bool(client->dev.of_node,
+					  "idt,enable-shutdown");
+	sh_disable = of_property_read_bool(client->dev.of_node,
+					   "idt,disable-shutdown");
+	if ((oe_high && oe_low) || (sh_enable && sh_disable))
+		return dev_err_probe(&client->dev, -EINVAL,
+				     "conflicting properties for SD/OE pin\n");
+
+	sp_mask = (oe_high || oe_low) ? VC5_PRIM_SRC_SHDN_SP : 0;
+	sh_mask = (sh_enable || sh_disable) ? VC5_PRIM_SRC_SHDN_EN_GBL_SHDN : 0;
+	sp_val = oe_high ? VC5_PRIM_SRC_SHDN_SP : 0;
+	sh_val = sh_enable ? VC5_PRIM_SRC_SHDN_EN_GBL_SHDN : 0;
+	regmap_update_bits(vc5->regmap, VC5_PRIM_SRC_SHDN, sp_mask | sh_mask,
+			   sp_val | sh_val);
+
 	/* Register clock input mux */
 	memset(&init, 0, sizeof(init));
 
-- 
2.25.1

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

[Geert Uytterhoeven]

Hi Sean,

On Thu, Jul 1, 2021 at 8:20 PM Sean Anderson <sean.anderson@seco.com> 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 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.

Thanks for the update!

> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml

> @@ -109,6 +152,22 @@ allOf:
>        required:
>          - clock-names
>          - clocks
> +  - if:
> +      true
> +    then:
> +      oneOf:
> +        - required:
> +            - idt,enable-shutdown
> +        - required:
> +            - idt,disable-shutdown
> +  - if:
> +      true
> +    then:
> +      oneOf:
> +        - required:
> +            - idt,output-enable-active-high
> +        - required:
> +            - idt,output-enable-active-low

Do you really need the "if: true then:"?
Just the "oneOf: ..." worked fine for me in another binding, but then I
didn't have a surrounding "allOf" to interfere...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 2/3] clk: vc5: Use dev_err_probe

[Geert Uytterhoeven]

On Thu, Jul 1, 2021 at 8:20 PM Sean Anderson <sean.anderson@seco.com> wrote:
> 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>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

 Hi Sean,

On Thu, Jul 1, 2021 at 8:20 PM Sean Anderson <sean.anderson@seco.com> 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>

Thanks for your patch!

> --- a/drivers/clk/clk-versaclock5.c
> +++ b/drivers/clk/clk-versaclock5.c
> @@ -886,6 +886,8 @@ static const struct of_device_id clk_vc5_of_match[];
>
>  static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
> +       bool oe_high, oe_low, sh_enable, sh_disable;
> +       unsigned int sp_mask, sh_mask, sp_val, sh_val;
>         struct vc5_driver_data *vc5;
>         struct clk_init_data init;
>         const char *parent_names[2];
> @@ -913,6 +915,25 @@ 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");
>
> +       oe_high = of_property_read_bool(client->dev.of_node,
> +                                       "idt,output-enable-active-high");
> +       oe_low = of_property_read_bool(client->dev.of_node,
> +                                       "idt,output-enable-active-low");
> +       sh_enable = of_property_read_bool(client->dev.of_node,
> +                                         "idt,enable-shutdown");
> +       sh_disable = of_property_read_bool(client->dev.of_node,
> +                                          "idt,disable-shutdown");
> +       if ((oe_high && oe_low) || (sh_enable && sh_disable))
> +               return dev_err_probe(&client->dev, -EINVAL,
> +                                    "conflicting properties for SD/OE pin\n");
> +
> +       sp_mask = (oe_high || oe_low) ? VC5_PRIM_SRC_SHDN_SP : 0;
> +       sh_mask = (sh_enable || sh_disable) ? VC5_PRIM_SRC_SHDN_EN_GBL_SHDN : 0;
> +       sp_val = oe_high ? VC5_PRIM_SRC_SHDN_SP : 0;
> +       sh_val = sh_enable ? VC5_PRIM_SRC_SHDN_EN_GBL_SHDN : 0;

I'm not so fond of ternary operators where one branch uses zero, so
I would write this (with pre-initialization of mask/val to zero):

    if (oe_high || oe_low)
            mask |= VC5_PRIM_SRC_SHDN_SP;
    if (oe_high)
            val |= VC5_PRIM_SRC_SHDN_SP;

which also has the benefit you need only one mask and val.
But that's purely my personal preference.

> +       regmap_update_bits(vc5->regmap, VC5_PRIM_SRC_SHDN, sp_mask | sh_mask,
> +                          sp_val | sh_val);
> +
>         /* Register clock input mux */
>         memset(&init, 0, sizeof(init));

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Luca Ceresoli]

Hi Sean,

On 01/07/21 20:20, 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 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       | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> index 28675b0b80f1..2631a175612b 100644
> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> @@ -30,6 +30,21 @@ 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

I just got an update from Renesas support, which confirms our table as
far as possible:

> I find that SH will only work when the Polarity bit is set to Active Low (SP=0).  When I program SP=1 together with SH=1, the outputs are always off. It simply won’t work.  So the Shutdown function can only be used with Active Low polarity for the SD/OE pin.  There is a pull-down resistor on the chip so when the pin is left open, outputs will be enabled with Active Low polarity.
>  
> [...] What is a more accurate description is that you can only use the Shutdown function (SH=1) together with the Active Low polarity (SP=0) setting.  The combination of SH=1 and SP=1 is an ‘illegal’ combination.
>  
> We actually find that very few customers use the Shutdown function. It is only partly useful because it turns off more circuits than the Output Enable function but there is still significant power consumption in Shutdown. The general expectation of a Shutdown function is that the power consumption will go down to a few microamperes of leakage current when in Shutdown but this device will not do that.

The combination SH=1 and SP=1 is not well specified, so I can't confirm
whether it should be Inactive/Shutdown or Shutdown/Shutdown or whatever.
Also I would not define the SH=1 and SP=1 combination as "illegal" but
rather as "useless".

Definitely (and obviously) the outputs are never driven when SH=1 and
SP=1. So I think we can stick to your table and I think this patch is
fine, more details are not needed in the DT bindings IMO. The only open
point now is the remark by Geert about the 'if: true' construct.

-- 
Luca

Re: [PATCH v4 2/3] clk: vc5: Use dev_err_probe

[Luca Ceresoli]

Hi Sean,

On 01/07/21 20:20, Sean Anderson wrote:
> Convert uses of dev_err + return to dev_err_probe.

The patch is also converting uses of dev_err() without return.

Not a big deal anyway. With or without this fixed:

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

Note: this patch can be applied independently from the other two.

-- 
Luca

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

[Luca Ceresoli]

Hi Sean,

On 02/07/21 09:29, Geert Uytterhoeven wrote:
>  Hi Sean,
> 
> On Thu, Jul 1, 2021 at 8:20 PM Sean Anderson <sean.anderson@seco.com> 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>
> 
> Thanks for your patch!
> 
>> --- a/drivers/clk/clk-versaclock5.c
>> +++ b/drivers/clk/clk-versaclock5.c
>> @@ -886,6 +886,8 @@ static const struct of_device_id clk_vc5_of_match[];
>>
>>  static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>  {
>> +       bool oe_high, oe_low, sh_enable, sh_disable;
>> +       unsigned int sp_mask, sh_mask, sp_val, sh_val;
>>         struct vc5_driver_data *vc5;
>>         struct clk_init_data init;
>>         const char *parent_names[2];
>> @@ -913,6 +915,25 @@ 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");
>>
>> +       oe_high = of_property_read_bool(client->dev.of_node,
>> +                                       "idt,output-enable-active-high");
>> +       oe_low = of_property_read_bool(client->dev.of_node,
>> +                                       "idt,output-enable-active-low");
>> +       sh_enable = of_property_read_bool(client->dev.of_node,
>> +                                         "idt,enable-shutdown");
>> +       sh_disable = of_property_read_bool(client->dev.of_node,
>> +                                          "idt,disable-shutdown");
>> +       if ((oe_high && oe_low) || (sh_enable && sh_disable))
>> +               return dev_err_probe(&client->dev, -EINVAL,
>> +                                    "conflicting properties for SD/OE pin\n");
>> +
>> +       sp_mask = (oe_high || oe_low) ? VC5_PRIM_SRC_SHDN_SP : 0;
>> +       sh_mask = (sh_enable || sh_disable) ? VC5_PRIM_SRC_SHDN_EN_GBL_SHDN : 0;
>> +       sp_val = oe_high ? VC5_PRIM_SRC_SHDN_SP : 0;
>> +       sh_val = sh_enable ? VC5_PRIM_SRC_SHDN_EN_GBL_SHDN : 0;
> 
> I'm not so fond of ternary operators where one branch uses zero, so
> I would write this (with pre-initialization of mask/val to zero):
> 
>     if (oe_high || oe_low)
>             mask |= VC5_PRIM_SRC_SHDN_SP;
>     if (oe_high)
>             val |= VC5_PRIM_SRC_SHDN_SP;
> 
> which also has the benefit you need only one mask and val.
> But that's purely my personal preference.

Both styles are fine for me, so with either version:

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

-- 
Luca

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

[Sean Anderson]

On 7/2/21 3:14 AM, Geert Uytterhoeven wrote:
 > Hi Sean,
 >
 > On Thu, Jul 1, 2021 at 8:20 PM Sean Anderson <sean.anderson@seco.com> 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 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.
 >
 > Thanks for the update!
 >
 >> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
 >> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
 >
 >> @@ -109,6 +152,22 @@ allOf:
 >>        required:
 >>          - clock-names
 >>          - clocks
 >> +  - if:
 >> +      true
 >> +    then:
 >> +      oneOf:
 >> +        - required:
 >> +            - idt,enable-shutdown
 >> +        - required:
 >> +            - idt,disable-shutdown
 >> +  - if:
 >> +      true
 >> +    then:
 >> +      oneOf:
 >> +        - required:
 >> +            - idt,output-enable-active-high
 >> +        - required:
 >> +            - idt,output-enable-active-low
 >
 > Do you really need the "if: true then:"?
 > Just the "oneOf: ..." worked fine for me in another binding, but then I
 > didn't have a surrounding "allOf" to interfere...

Yes. If you want to have multiple oneOfs, they have to go under an
allOf. And allOf *only* allows if statements. This is a pretty big
deficiency, IMO, but not something I can address here.

--Sean

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

[Rob Herring]

On Fri, Jul 02, 2021 at 11:07:57AM -0400, Sean Anderson wrote:
> 
> 
> On 7/2/21 3:14 AM, Geert Uytterhoeven wrote:
> > Hi Sean,
> >
> > On Thu, Jul 1, 2021 at 8:20 PM Sean Anderson <sean.anderson@seco.com> 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 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.
> >
> > Thanks for the update!
> >
> >> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> >> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> >
> >> @@ -109,6 +152,22 @@ allOf:
> >>        required:
> >>          - clock-names
> >>          - clocks
> >> +  - if:
> >> +      true
> >> +    then:
> >> +      oneOf:
> >> +        - required:
> >> +            - idt,enable-shutdown
> >> +        - required:
> >> +            - idt,disable-shutdown
> >> +  - if:
> >> +      true
> >> +    then:
> >> +      oneOf:
> >> +        - required:
> >> +            - idt,output-enable-active-high
> >> +        - required:
> >> +            - idt,output-enable-active-low
> >
> > Do you really need the "if: true then:"?
> > Just the "oneOf: ..." worked fine for me in another binding, but then I
> > didn't have a surrounding "allOf" to interfere...
> 
> Yes. If you want to have multiple oneOfs, they have to go under an
> allOf. And allOf *only* allows if statements. This is a pretty big
> deficiency, IMO, but not something I can address here.

Humm, we should relax that, not work around it.

Rob

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

[Rob Herring]

On Fri, Jul 2, 2021 at 9:18 AM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Jul 02, 2021 at 11:07:57AM -0400, Sean Anderson wrote:
> >
> >
> > On 7/2/21 3:14 AM, Geert Uytterhoeven wrote:
> > > Hi Sean,
> > >
> > > On Thu, Jul 1, 2021 at 8:20 PM Sean Anderson <sean.anderson@seco.com> 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 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.
> > >
> > > Thanks for the update!
> > >
> > >> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > >> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > >
> > >> @@ -109,6 +152,22 @@ allOf:
> > >>        required:
> > >>          - clock-names
> > >>          - clocks
> > >> +  - if:
> > >> +      true
> > >> +    then:
> > >> +      oneOf:
> > >> +        - required:
> > >> +            - idt,enable-shutdown
> > >> +        - required:
> > >> +            - idt,disable-shutdown
> > >> +  - if:
> > >> +      true
> > >> +    then:
> > >> +      oneOf:
> > >> +        - required:
> > >> +            - idt,output-enable-active-high
> > >> +        - required:
> > >> +            - idt,output-enable-active-low
> > >
> > > Do you really need the "if: true then:"?
> > > Just the "oneOf: ..." worked fine for me in another binding, but then I
> > > didn't have a surrounding "allOf" to interfere...
> >
> > Yes. If you want to have multiple oneOfs, they have to go under an
> > allOf. And allOf *only* allows if statements. This is a pretty big
> > deficiency, IMO, but not something I can address here.
>
> Humm, we should relax that, not work around it.

I've now relaxed this restriction in dt-schema master.

Rob

P.S. I probably broke something because it's Friday afternoon before
going on holiday for a week (so I'll do a tagged release when back).

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

[Sean Anderson]

On 7/2/21 5:15 PM, Rob Herring wrote:
> On Fri, Jul 2, 2021 at 9:18 AM Rob Herring <robh@kernel.org> wrote:
>>
>> On Fri, Jul 02, 2021 at 11:07:57AM -0400, Sean Anderson wrote:
>> >
>> >
>> > On 7/2/21 3:14 AM, Geert Uytterhoeven wrote:
>> > > Hi Sean,
>> > >
>> > > On Thu, Jul 1, 2021 at 8:20 PM Sean Anderson <sean.anderson@seco.com> 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 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.
>> > >
>> > > Thanks for the update!
>> > >
>> > >> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>> > >> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>> > >
>> > >> @@ -109,6 +152,22 @@ allOf:
>> > >>        required:
>> > >>          - clock-names
>> > >>          - clocks
>> > >> +  - if:
>> > >> +      true
>> > >> +    then:
>> > >> +      oneOf:
>> > >> +        - required:
>> > >> +            - idt,enable-shutdown
>> > >> +        - required:
>> > >> +            - idt,disable-shutdown
>> > >> +  - if:
>> > >> +      true
>> > >> +    then:
>> > >> +      oneOf:
>> > >> +        - required:
>> > >> +            - idt,output-enable-active-high
>> > >> +        - required:
>> > >> +            - idt,output-enable-active-low
>> > >
>> > > Do you really need the "if: true then:"?
>> > > Just the "oneOf: ..." worked fine for me in another binding, but then I
>> > > didn't have a surrounding "allOf" to interfere...
>> >
>> > Yes. If you want to have multiple oneOfs, they have to go under an
>> > allOf. And allOf *only* allows if statements. This is a pretty big
>> > deficiency, IMO, but not something I can address here.
>>
>> Humm, we should relax that, not work around it.
> 
> I've now relaxed this restriction in dt-schema master.
> 
> Rob
> 
> P.S. I probably broke something because it's Friday afternoon before
> going on holiday for a week (so I'll do a tagged release when back).

Will you be doing a release? I'm waiting on that before I send v5.

--Sean