diff mbox series

[v4,2/6] dt-bindings: pwm: amlogic: add new compatible for meson8 pwm type

Message ID 20231222111658.832167-3-jbrunet@baylibre.com (mailing list archive)
State New, archived
Headers show
Series pwm: meson: dt-bindings fixup | expand

Commit Message

Jerome Brunet Dec. 22, 2023, 11:16 a.m. UTC
Add a new compatible for the pwm found in the meson8 to sm1 Amlogic SoCs,
dealing with clocks differently. This does not enable new HW. It is meant
to fix a bad DT ABI for the currently supported HW.

The original clock bindings describe which input the PWM channel
multiplexer should pick among its possible parents, which are
hard-coded in the driver. As such, it is a setting tied to the driver
implementation and does not describe the HW.

The new bindings introduce here describe the clocks input of the PWM block
as they exist.

The old compatible is deprecated but kept to maintain ABI compatibility.

The SoC specific compatibles introduced match the SoC families supported
by the original bindings.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 .../devicetree/bindings/pwm/pwm-amlogic.yaml  | 50 +++++++++++++++++--
 1 file changed, 46 insertions(+), 4 deletions(-)

Comments

Krzysztof Kozlowski Dec. 23, 2023, 2:02 p.m. UTC | #1
On 22/12/2023 12:16, Jerome Brunet wrote:
> Add a new compatible for the pwm found in the meson8 to sm1 Amlogic SoCs,
> dealing with clocks differently. This does not enable new HW. It is meant
> to fix a bad DT ABI for the currently supported HW.
> 
> The original clock bindings describe which input the PWM channel
> multiplexer should pick among its possible parents, which are
> hard-coded in the driver. As such, it is a setting tied to the driver
> implementation and does not describe the HW.
> 
> The new bindings introduce here describe the clocks input of the PWM block
> as they exist.
> 
> The old compatible is deprecated but kept to maintain ABI compatibility.
> 
> The SoC specific compatibles introduced match the SoC families supported
> by the original bindings.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  .../devicetree/bindings/pwm/pwm-amlogic.yaml  | 50 +++++++++++++++++--
>  1 file changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> index a1d382aacb82..eece390114a3 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> @@ -21,23 +21,35 @@ properties:
>            - amlogic,meson-g12a-ee-pwm
>            - amlogic,meson-g12a-ao-pwm-ab
>            - amlogic,meson-g12a-ao-pwm-cd
> -          - amlogic,meson-s4-pwm
> +        deprecated: true
>        - items:
>            - const: amlogic,meson-gx-pwm
>            - const: amlogic,meson-gxbb-pwm
> +        deprecated: true
>        - items:
>            - const: amlogic,meson-gx-ao-pwm
>            - const: amlogic,meson-gxbb-ao-pwm
> +        deprecated: true
>        - items:
>            - const: amlogic,meson8-pwm
>            - const: amlogic,meson8b-pwm
> +        deprecated: true
> +      - const: amlogic,meson8-pwm-v2
> +      - items:
> +          - enum:
> +              - amlogic,meson8b-pwm-v2
> +              - amlogic,meson-gxbb-pwm-v2
> +              - amlogic,meson-axg-pwm-v2
> +              - amlogic,meson-g12-pwm-v2
> +          - const: amlogic,meson8-pwm-v2
> +      - const: amlogic,meson-s4-pwm

If there is going to be resend:
This and amlogic,meson8-pwm-v2 before items: should be just in enum,
it's easier to read.

>  
>    reg:
>      maxItems: 1
>  
>    clocks:
>      minItems: 1
> -    maxItems: 2
> +    maxItems: 4
>  
>    clock-names:
>      minItems: 1
> @@ -58,7 +70,6 @@ allOf:
>          compatible:
>            contains:
>              enum:
> -              - amlogic,meson8-pwm

I don't understand why removing it here.

The rest looks ok.
Best regards,
Krzysztof
Uwe Kleine-König Jan. 17, 2024, 9:58 a.m. UTC | #2
Hello,

On Fri, Dec 22, 2023 at 12:16:50PM +0100, Jerome Brunet wrote:
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> index a1d382aacb82..eece390114a3 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> @@ -21,23 +21,35 @@ properties:
>            - amlogic,meson-g12a-ee-pwm
>            - amlogic,meson-g12a-ao-pwm-ab
>            - amlogic,meson-g12a-ao-pwm-cd
> -          - amlogic,meson-s4-pwm
> +        deprecated: true
>        - items:
>            - const: amlogic,meson-gx-pwm
>            - const: amlogic,meson-gxbb-pwm
> +        deprecated: true
>        - items:
>            - const: amlogic,meson-gx-ao-pwm
>            - const: amlogic,meson-gxbb-ao-pwm
> +        deprecated: true
>        - items:
>            - const: amlogic,meson8-pwm
>            - const: amlogic,meson8b-pwm
> +        deprecated: true

I think deprecating the old binding and adding a new compatible should
be done in two commits.

> +      - const: amlogic,meson8-pwm-v2
> +      - items:
> +          - enum:
> +              - amlogic,meson8b-pwm-v2
> +              - amlogic,meson-gxbb-pwm-v2
> +              - amlogic,meson-axg-pwm-v2
> +              - amlogic,meson-g12-pwm-v2
> +          - const: amlogic,meson8-pwm-v2
> +      - const: amlogic,meson-s4-pwm

Best regards
Uwe
Jerome Brunet Jan. 17, 2024, 10:16 a.m. UTC | #3
On Wed 17 Jan 2024 at 10:58, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> [[PGP Signed Part:Undecided]]
> Hello,
>
> On Fri, Dec 22, 2023 at 12:16:50PM +0100, Jerome Brunet wrote:
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> index a1d382aacb82..eece390114a3 100644
>> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> @@ -21,23 +21,35 @@ properties:
>>            - amlogic,meson-g12a-ee-pwm
>>            - amlogic,meson-g12a-ao-pwm-ab
>>            - amlogic,meson-g12a-ao-pwm-cd
>> -          - amlogic,meson-s4-pwm
>> +        deprecated: true
>>        - items:
>>            - const: amlogic,meson-gx-pwm
>>            - const: amlogic,meson-gxbb-pwm
>> +        deprecated: true
>>        - items:
>>            - const: amlogic,meson-gx-ao-pwm
>>            - const: amlogic,meson-gxbb-ao-pwm
>> +        deprecated: true
>>        - items:
>>            - const: amlogic,meson8-pwm
>>            - const: amlogic,meson8b-pwm
>> +        deprecated: true
>
> I think deprecating the old binding and adding a new compatible should
> be done in two commits.

Hi Uwe,

There was the same comment on v3 and Krzysztof said it should be done
like this:

https://lore.kernel.org/linux-pwm/e127dcef-3149-443a-9a8c-d24ef4054f09@linaro.org

I tend to agree with Krzysztof on this but, as I previously said,
I don't really mind one way or the other. Just have to pick one.

>
>> +      - const: amlogic,meson8-pwm-v2
>> +      - items:
>> +          - enum:
>> +              - amlogic,meson8b-pwm-v2
>> +              - amlogic,meson-gxbb-pwm-v2
>> +              - amlogic,meson-axg-pwm-v2
>> +              - amlogic,meson-g12-pwm-v2
>> +          - const: amlogic,meson8-pwm-v2
>> +      - const: amlogic,meson-s4-pwm
>
> Best regards
> Uwe
Uwe Kleine-König Jan. 17, 2024, 10:51 a.m. UTC | #4
Hello Jerome,

On Wed, Jan 17, 2024 at 11:16:31AM +0100, Jerome Brunet wrote:
> On Wed 17 Jan 2024 at 10:58, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > [[PGP Signed Part:Undecided]]
> > Hello,
> >
> > On Fri, Dec 22, 2023 at 12:16:50PM +0100, Jerome Brunet wrote:
> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> >> index a1d382aacb82..eece390114a3 100644
> >> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> >> @@ -21,23 +21,35 @@ properties:
> >>            - amlogic,meson-g12a-ee-pwm
> >>            - amlogic,meson-g12a-ao-pwm-ab
> >>            - amlogic,meson-g12a-ao-pwm-cd
> >> -          - amlogic,meson-s4-pwm

Either I still didn't grasp all the details of this change, or removing
amlogic,meson-s4-pwm in this commit is wrong.

> >> +        deprecated: true
> >>        - items:
> >>            - const: amlogic,meson-gx-pwm
> >>            - const: amlogic,meson-gxbb-pwm
> >> +        deprecated: true
> >>        - items:
> >>            - const: amlogic,meson-gx-ao-pwm
> >>            - const: amlogic,meson-gxbb-ao-pwm
> >> +        deprecated: true
> >>        - items:
> >>            - const: amlogic,meson8-pwm
> >>            - const: amlogic,meson8b-pwm
> >> +        deprecated: true
> >
> > I think deprecating the old binding and adding a new compatible should
> > be done in two commits.
> 
> Hi Uwe,
> 
> There was the same comment on v3 and Krzysztof said it should be done
> like this:
> 
> https://lore.kernel.org/linux-pwm/e127dcef-3149-443a-9a8c-d24ef4054f09@linaro.org
> 
> I tend to agree with Krzysztof on this but, as I previously said,
> I don't really mind one way or the other. Just have to pick one.

Ah, so the machines that used amlogic,meson-g12a-ee-pwm before are
supposed to use amlogic,meson-g12-pwm-v2 now. With that understood I
agree to you and Krzysztof.

I wonder if me not understanding that is a sign that the commit log
isn't optimal (or if it's only that I didn't properly read it :-).
Now that I understood the change better, the commit log is
understandable, but maybe still make it a bit more explicit that it
introduces a new way to formalize already supported hardware. Something
like:

	dt-bindings: pwm: amlogic: Add a new binding for meson8 pwm types

	The binding that is used up to now describe which input the PWM
	channel multiplexer should pick among its possible parents,
	which are hardcoded in the driver. This isn't a good binding in
	the sense that it should describe hardware but not usage.

	Add a new binding deprecating the old one that uses clocks in a
	better way and how clocks are usually used today: The list of
	clocks describe the inputs of the PWM block as they are realised
	in hardware.

	So deprecate the old bindings and introduce a compatible per SoC
	family to replace these.

I think I'd understand that better, but that might be because I wrote
it and it's subjective?

Best regards
Uwe
Krzysztof Kozlowski Jan. 17, 2024, 5:29 p.m. UTC | #5
On 17/01/2024 10:58, Uwe Kleine-König wrote:
>>        - items:
>>            - const: amlogic,meson-gx-pwm
>>            - const: amlogic,meson-gxbb-pwm
>> +        deprecated: true
>>        - items:
>>            - const: amlogic,meson-gx-ao-pwm
>>            - const: amlogic,meson-gxbb-ao-pwm
>> +        deprecated: true
>>        - items:
>>            - const: amlogic,meson8-pwm
>>            - const: amlogic,meson8b-pwm
>> +        deprecated: true
> 
> I think deprecating the old binding and adding a new compatible should
> be done in two commits.

No, because you have a state with deprecated compatible and no correct
compatible. If deprecated property caused warnings, you would have
non-bisectable state.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
index a1d382aacb82..eece390114a3 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
+++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
@@ -21,23 +21,35 @@  properties:
           - amlogic,meson-g12a-ee-pwm
           - amlogic,meson-g12a-ao-pwm-ab
           - amlogic,meson-g12a-ao-pwm-cd
-          - amlogic,meson-s4-pwm
+        deprecated: true
       - items:
           - const: amlogic,meson-gx-pwm
           - const: amlogic,meson-gxbb-pwm
+        deprecated: true
       - items:
           - const: amlogic,meson-gx-ao-pwm
           - const: amlogic,meson-gxbb-ao-pwm
+        deprecated: true
       - items:
           - const: amlogic,meson8-pwm
           - const: amlogic,meson8b-pwm
+        deprecated: true
+      - const: amlogic,meson8-pwm-v2
+      - items:
+          - enum:
+              - amlogic,meson8b-pwm-v2
+              - amlogic,meson-gxbb-pwm-v2
+              - amlogic,meson-axg-pwm-v2
+              - amlogic,meson-g12-pwm-v2
+          - const: amlogic,meson8-pwm-v2
+      - const: amlogic,meson-s4-pwm
 
   reg:
     maxItems: 1
 
   clocks:
     minItems: 1
-    maxItems: 2
+    maxItems: 4
 
   clock-names:
     minItems: 1
@@ -58,7 +70,6 @@  allOf:
         compatible:
           contains:
             enum:
-              - amlogic,meson8-pwm
               - amlogic,meson8b-pwm
               - amlogic,meson-gxbb-pwm
               - amlogic,meson-gxbb-ao-pwm
@@ -68,11 +79,14 @@  allOf:
               - amlogic,meson-g12a-ao-pwm-ab
               - amlogic,meson-g12a-ao-pwm-cd
     then:
-      # Historic bindings tied to the driver implementation
+      # Obsolete historic bindings tied to the driver implementation
       # The clocks provided here are meant to be matched with the input
       # known (hard-coded) in the driver and used to select pwm clock
       # source. Currently, the linux driver ignores this.
+      # This is kept to maintain ABI backward compatibility.
       properties:
+        clocks:
+          maxItems: 2
         clock-names:
           oneOf:
             - items:
@@ -81,6 +95,27 @@  allOf:
                 - const: clkin0
                 - const: clkin1
 
+  # Newer binding where clock describe the actual clock inputs of the pwm
+  # block. These are necessary but some inputs may be grounded.
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - amlogic,meson8-pwm-v2
+    then:
+      properties:
+        clocks:
+          minItems: 1
+          items:
+            - description: input clock 0 of the pwm block
+            - description: input clock 1 of the pwm block
+            - description: input clock 2 of the pwm block
+            - description: input clock 3 of the pwm block
+        clock-names: false
+      required:
+        - clocks
+
   # Newer IP block take a single input per channel, instead of 4 inputs
   # for both channels
   - if:
@@ -110,6 +145,13 @@  examples:
       clock-names = "clkin0", "clkin1";
       #pwm-cells = <3>;
     };
+  - |
+    pwm@2000 {
+      compatible = "amlogic,meson8-pwm-v2";
+      reg = <0x1000 0x10>;
+      clocks = <&xtal>, <0>, <&fdiv4>, <&fdiv5>;
+      #pwm-cells = <3>;
+    };
   - |
     pwm@1000 {
       compatible = "amlogic,meson-s4-pwm";