[PATCH v3 0/4] pwm: meson: dt-bindings fixup

[PATCH v3 0/4] pwm: meson: dt-bindings fixup

[Jerome Brunet]

This patchset aims to:
* Fix the incorrect bindings for the s4 type of pwm that was introduced
  while converting the documentation from txt to yaml format.
* Introduce a new compatible for the existing PWMs to better describe the
  HW in DT, instead of describing driver settings.
* Make the introduction of a new pwm variant (s4) slightly easier.

Uwe,
Patches #3 and #4 conflict with patches #18 and #68 of your pwm rework [3]
I understand such wide rework are difficult to handle. I don't mind
handling the rebase of the amlogic patches you have proposed if this gets
in first. Just let me know :)

Changes since v2 [2]:
* Drop DTS changes. These will be re-submitted later on. Possibly after
  u-boot gets support for the new compatible to minimise conversion
  problems.
* Position deprecated property correctly in dt-bindings for the old
  meson8 type pwm bindings
* Reword commit description of patch #2 to make more obvious it does not
  introduce a new HW support but fixes a bad bindings.
* Dropped Rob's Reviewed-by on patch #2. It seemed appropriate considering
  the discussion on this change.

Changes since v1 [1]:
* Fix typo in the new binding compatible documentation
* Disallow clock-names for the new compatibles in the schema documenation

[1]: https://lore.kernel.org/linux-amlogic/20231106103259.703417-1-jbrunet@baylibre.com
[2]: https://lore.kernel.org/linux-amlogic/20231117125919.1696980-1-jbrunet@baylibre.com
[3]: https://lore.kernel.org/linux-amlogic/20231121134901.208535-1-u.kleine-koenig@pengutronix.de

Jerome Brunet (4):
  dt-bindings: pwm: amlogic: fix s4 bindings
  dt-bindings: pwm: amlogic: add new compatible for meson8 pwm type
  pwm: meson: prepare addition of new compatible types
  pwm: meson: add generic compatible for meson8 to sm1

 .../devicetree/bindings/pwm/pwm-amlogic.yaml  | 113 ++++++-
 drivers/pwm/pwm-meson.c                       | 312 +++++++++++-------
 2 files changed, 291 insertions(+), 134 deletions(-)

-- 
2.42.0

[PATCH v3 1/4] dt-bindings: pwm: amlogic: fix s4 bindings

[Jerome Brunet]

s4 has been added to the compatible list while converting the Amlogic PWM
binding documentation from txt to yaml.

However, on the s4, the clock bindings have different meaning compared to
the previous SoCs.

On the previous SoCs the clock bindings used to describe which input the
PWM channel multiplexer should pick among its possible parents.

This is very much tied to the driver implementation, instead of describing
the HW for what it is. When support for the Amlogic PWM was first added,
how to deal with clocks through DT was not as clear as it nowadays.
The Linux driver now ignores this DT setting, but still relies on the
hard-coded list of clock sources.

On the s4, the input multiplexer is gone. The clock bindings actually
describe the clock as it exists, not a setting. The property has a
different meaning, even if it is still 2 clocks and it would pass the check
when support is actually added.

Also the s4 cannot work if the clocks are not provided, so the property no
longer optional.

Finally, for once it makes sense to see the input as being numbered
somehow. No need to bother with clock-names on the s4 type of PWM.

Fixes: 43a1c4ff3977 ("dt-bindings: pwm: Convert Amlogic Meson PWM binding")
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 .../devicetree/bindings/pwm/pwm-amlogic.yaml  | 69 ++++++++++++++++---
 1 file changed, 60 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
index 527864a4d855..387976ed36d5 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
+++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
@@ -9,9 +9,6 @@ title: Amlogic PWM
 maintainers:
   - Heiner Kallweit <hkallweit1@gmail.com>
 
-allOf:
-  - $ref: pwm.yaml#
-
 properties:
   compatible:
     oneOf:
@@ -43,12 +40,8 @@ properties:
     maxItems: 2
 
   clock-names:
-    oneOf:
-      - items:
-          - enum: [clkin0, clkin1]
-      - items:
-          - const: clkin0
-          - const: clkin1
+    minItems: 1
+    maxItems: 2
 
   "#pwm-cells":
     const: 3
@@ -57,6 +50,57 @@ required:
   - compatible
   - reg
 
+allOf:
+  - $ref: pwm.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - amlogic,meson8-pwm
+              - amlogic,meson8b-pwm
+              - amlogic,meson-gxbb-pwm
+              - amlogic,meson-gxbb-ao-pwm
+              - amlogic,meson-axg-ee-pwm
+              - amlogic,meson-axg-ao-pwm
+              - amlogic,meson-g12a-ee-pwm
+              - amlogic,meson-g12a-ao-pwm-ab
+              - amlogic,meson-g12a-ao-pwm-cd
+              - amlogic,meson-gx-pwm
+              - amlogic,meson-gx-ao-pwm
+    then:
+      # 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.
+      properties:
+        clock-names:
+          oneOf:
+            - items:
+                - enum: [clkin0, clkin1]
+            - items:
+                - const: clkin0
+                - const: clkin1
+
+  # Newer IP block take a single input per channel, instead of 4 inputs
+  # for both channels
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - amlogic,meson-s4-pwm
+    then:
+      properties:
+        clocks:
+          items:
+            - description: input clock of PWM channel A
+            - description: input clock of PWM channel B
+        clock-names: false
+      required:
+        - clocks
+
 additionalProperties: false
 
 examples:
@@ -68,3 +112,10 @@ examples:
       clock-names = "clkin0", "clkin1";
       #pwm-cells = <3>;
     };
+  - |
+    pwm@1000 {
+      compatible = "amlogic,meson-s4-pwm";
+      reg = <0x1000 0x10>;
+      clocks = <&pwm_src_a>, <&pwm_src_b>;
+      #pwm-cells = <3>;
+    };
-- 
2.42.0

[PATCH v3 2/4] dt-bindings: pwm: amlogic: add new compatible for meson8 pwm type

[Jerome Brunet]

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  | 52 ++++++++++++++++---
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
index 387976ed36d5..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
@@ -67,14 +78,15 @@ allOf:
               - amlogic,meson-g12a-ee-pwm
               - amlogic,meson-g12a-ao-pwm-ab
               - amlogic,meson-g12a-ao-pwm-cd
-              - amlogic,meson-gx-pwm
-              - amlogic,meson-gx-ao-pwm
     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:
@@ -83,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:
@@ -112,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";
-- 
2.42.0

Re: [PATCH v3 2/4] dt-bindings: pwm: amlogic: add new compatible for meson8 pwm type

[Neil Armstrong]

Hi,

On 29/11/2023 14:39, 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  | 52 ++++++++++++++++---
>   1 file changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> index 387976ed36d5..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 deprecated should be moved in a third patch

> +      - 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
> @@ -67,14 +78,15 @@ allOf:
>                 - amlogic,meson-g12a-ee-pwm
>                 - amlogic,meson-g12a-ao-pwm-ab
>                 - amlogic,meson-g12a-ao-pwm-cd
> -              - amlogic,meson-gx-pwm
> -              - amlogic,meson-gx-ao-pwm

I don't understand why those entries are removed

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

Same here, this should go in a third patch

>         properties:
> +        clocks:
> +          maxItems: 2
>           clock-names:
>             oneOf:
>               - items:
> @@ -83,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:
> @@ -112,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";

Neil

Re: [PATCH v3 2/4] dt-bindings: pwm: amlogic: add new compatible for meson8 pwm type

[Jerome Brunet]

On Wed 29 Nov 2023 at 17:20, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> Hi,
>
> On 29/11/2023 14:39, 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  | 52 ++++++++++++++++---
>>   1 file changed, 46 insertions(+), 6 deletions(-)
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> index 387976ed36d5..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 deprecated should be moved in a third patch

The complain on v2 was that it was not clear the new binding was making
the old one obsolete. It looked to me that the deprecation old bindings
needed to go together with the introduction of the new.

I don't mind one way or the other

Is there a rule somewhere about this ?

>
>> +      - 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
>> @@ -67,14 +78,15 @@ allOf:
>>                 - amlogic,meson-g12a-ee-pwm
>>                 - amlogic,meson-g12a-ao-pwm-ab
>>                 - amlogic,meson-g12a-ao-pwm-cd
>> -              - amlogic,meson-gx-pwm
>> -              - amlogic,meson-gx-ao-pwm
>
> I don't understand why those entries are removed

It's a mistake. It should not have been added to begin with in
the first patch. "amlogic,meson-gx-*" must go along with
"amlogic,meson-gxbb-*" so it matches correctly without it.

I'll fix it

>
>>       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.
>
> Same here, this should go in a third patch
>
>>         properties:
>> +        clocks:
>> +          maxItems: 2
>>           clock-names:
>>             oneOf:
>>               - items:
>> @@ -83,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:
>> @@ -112,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";
>
> Neil


-- 
Jerome

Re: [PATCH v3 2/4] dt-bindings: pwm: amlogic: add new compatible for meson8 pwm type

[neil.armstrong]

Hi,

On 29/11/2023 17:26, Jerome Brunet wrote:
> 
> On Wed 29 Nov 2023 at 17:20, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> 
>> Hi,
>>
>> On 29/11/2023 14:39, 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  | 52 ++++++++++++++++---
>>>    1 file changed, 46 insertions(+), 6 deletions(-)
>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>> b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>> index 387976ed36d5..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 deprecated should be moved in a third patch
> 
> The complain on v2 was that it was not clear the new binding was making
> the old one obsolete. It looked to me that the deprecation old bindings
> needed to go together with the introduction of the new.
> 
> I don't mind one way or the other
> 
> Is there a rule somewhere about this ?

Not sure about that, I don't think it's a problem to have both valid
at the same time, setting them deprecated afterwards looks cleaner
to avoid mixing too much changes at the same time.

Neil

> 
>>
>>> +      - 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
>>> @@ -67,14 +78,15 @@ allOf:
>>>                  - amlogic,meson-g12a-ee-pwm
>>>                  - amlogic,meson-g12a-ao-pwm-ab
>>>                  - amlogic,meson-g12a-ao-pwm-cd
>>> -              - amlogic,meson-gx-pwm
>>> -              - amlogic,meson-gx-ao-pwm
>>
>> I don't understand why those entries are removed
> 
> It's a mistake. It should not have been added to begin with in
> the first patch. "amlogic,meson-gx-*" must go along with
> "amlogic,meson-gxbb-*" so it matches correctly without it.
> 
> I'll fix it
> 
>>
>>>        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.
>>
>> Same here, this should go in a third patch
>>
>>>          properties:
>>> +        clocks:
>>> +          maxItems: 2
>>>            clock-names:
>>>              oneOf:
>>>                - items:
>>> @@ -83,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:
>>> @@ -112,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";
>>
>> Neil
> 
> 

Re: [PATCH v3 2/4] dt-bindings: pwm: amlogic: add new compatible for meson8 pwm type

[Krzysztof Kozlowski]

On 29/11/2023 17:41, neil.armstrong@linaro.org wrote:
>>>>    .../devicetree/bindings/pwm/pwm-amlogic.yaml  | 52 ++++++++++++++++---
>>>>    1 file changed, 46 insertions(+), 6 deletions(-)
>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>>> b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>>> index 387976ed36d5..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 deprecated should be moved in a third patch
>>
>> The complain on v2 was that it was not clear the new binding was making
>> the old one obsolete. It looked to me that the deprecation old bindings
>> needed to go together with the introduction of the new.
>>
>> I don't mind one way or the other
>>
>> Is there a rule somewhere about this ?
> 
> Not sure about that, I don't think it's a problem to have both valid
> at the same time, setting them deprecated afterwards looks cleaner
> to avoid mixing too much changes at the same time.

For me current order is correct and intuitive: you add new binding,
because old binding was wrong, so the old binding should be deprecated.
Otherwise you have a state with both new and old binding and one could
question - why did we need new binding? For dtschema it does not matter,
but it matters how we read the code.

Best regards,
Krzysztof

Re: [PATCH v3 2/4] dt-bindings: pwm: amlogic: add new compatible for meson8 pwm type

[neil.armstrong]

On 30/11/2023 09:36, Krzysztof Kozlowski wrote:
> On 29/11/2023 17:41, neil.armstrong@linaro.org wrote:
>>>>>     .../devicetree/bindings/pwm/pwm-amlogic.yaml  | 52 ++++++++++++++++---
>>>>>     1 file changed, 46 insertions(+), 6 deletions(-)
>>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>>>> b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>>>> index 387976ed36d5..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 deprecated should be moved in a third patch
>>>
>>> The complain on v2 was that it was not clear the new binding was making
>>> the old one obsolete. It looked to me that the deprecation old bindings
>>> needed to go together with the introduction of the new.
>>>
>>> I don't mind one way or the other
>>>
>>> Is there a rule somewhere about this ?
>>
>> Not sure about that, I don't think it's a problem to have both valid
>> at the same time, setting them deprecated afterwards looks cleaner
>> to avoid mixing too much changes at the same time.
> 
> For me current order is correct and intuitive: you add new binding,
> because old binding was wrong, so the old binding should be deprecated.
> Otherwise you have a state with both new and old binding and one could
> question - why did we need new binding? For dtschema it does not matter,
> but it matters how we read the code.

Ack thx for the clarification

> 
> Best regards,
> Krzysztof
> 

Thanks,
Neil

[PATCH v3 3/4] pwm: meson: prepare addition of new compatible types

[Jerome Brunet]

Clean the amlogic pwm driver to prepare the addition of new pwm compatibles
* Generalize 4 inputs clock per channel.
  AO pwm may just get 2 extra NULL entries which actually better
  describes the reality of the HW.
* Use driver data to carry the device data and remove pwm_chip from it
* Stop carrying the internal clock elements with the device data.
  These are not needed past init.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/pwm/pwm-meson.c | 150 +++++++++++++++++++++++-----------------
 1 file changed, 87 insertions(+), 63 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 5bea53243ed2..5cbd65cae28a 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -60,7 +60,7 @@
 #define MISC_A_EN		BIT(0)
 
 #define MESON_NUM_PWMS		2
-#define MESON_MAX_MUX_PARENTS	4
+#define MESON_NUM_MUX_PARENTS	4
 
 static struct meson_pwm_channel_data {
 	u8		reg_offset;
@@ -90,19 +90,14 @@ struct meson_pwm_channel {
 	unsigned int hi;
 	unsigned int lo;
 
-	struct clk_mux mux;
-	struct clk_divider div;
-	struct clk_gate gate;
 	struct clk *clk;
 };
 
 struct meson_pwm_data {
 	const char * const *parent_names;
-	unsigned int num_parents;
 };
 
 struct meson_pwm {
-	struct pwm_chip chip;
 	const struct meson_pwm_data *data;
 	struct meson_pwm_channel channels[MESON_NUM_PWMS];
 	void __iomem *base;
@@ -115,7 +110,7 @@ struct meson_pwm {
 
 static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip)
 {
-	return container_of(chip, struct meson_pwm, chip);
+	return dev_get_drvdata(chip->dev);
 }
 
 static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -147,6 +142,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
 			  const struct pwm_state *state)
 {
 	struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm];
+	struct device *dev = pwm->chip->dev;
 	unsigned int cnt, duty_cnt;
 	unsigned long fin_freq;
 	u64 duty, period, freq;
@@ -169,19 +165,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
 
 	fin_freq = clk_round_rate(channel->clk, freq);
 	if (fin_freq == 0) {
-		dev_err(meson->chip.dev, "invalid source clock frequency\n");
+		dev_err(dev, "invalid source clock frequency\n");
 		return -EINVAL;
 	}
 
-	dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
+	dev_dbg(dev, "fin_freq: %lu Hz\n", fin_freq);
 
 	cnt = div_u64(fin_freq * period, NSEC_PER_SEC);
 	if (cnt > 0xffff) {
-		dev_err(meson->chip.dev, "unable to get period cnt\n");
+		dev_err(dev, "unable to get period cnt\n");
 		return -EINVAL;
 	}
 
-	dev_dbg(meson->chip.dev, "period=%llu cnt=%u\n", period, cnt);
+	dev_dbg(dev, "period=%llu cnt=%u\n", period, cnt);
 
 	if (duty == period) {
 		channel->hi = cnt;
@@ -192,7 +188,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
 	} else {
 		duty_cnt = div_u64(fin_freq * duty, NSEC_PER_SEC);
 
-		dev_dbg(meson->chip.dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
+		dev_dbg(dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
 
 		channel->hi = duty_cnt;
 		channel->lo = cnt - duty_cnt;
@@ -215,7 +211,7 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
 
 	err = clk_set_rate(channel->clk, channel->rate);
 	if (err)
-		dev_err(meson->chip.dev, "setting clock rate failed\n");
+		dev_err(pwm->chip->dev, "setting clock rate failed\n");
 
 	spin_lock_irqsave(&meson->lock, flags);
 
@@ -343,7 +339,6 @@ static const char * const pwm_meson8b_parent_names[] = {
 
 static const struct meson_pwm_data pwm_meson8b_data = {
 	.parent_names = pwm_meson8b_parent_names,
-	.num_parents = ARRAY_SIZE(pwm_meson8b_parent_names),
 };
 
 /*
@@ -351,12 +346,11 @@ static const struct meson_pwm_data pwm_meson8b_data = {
  * The last 2 are grounded
  */
 static const char * const pwm_gxbb_ao_parent_names[] = {
-	"xtal", "clk81"
+	"xtal", "clk81", NULL, NULL,
 };
 
 static const struct meson_pwm_data pwm_gxbb_ao_data = {
 	.parent_names = pwm_gxbb_ao_parent_names,
-	.num_parents = ARRAY_SIZE(pwm_gxbb_ao_parent_names),
 };
 
 static const char * const pwm_axg_ee_parent_names[] = {
@@ -365,7 +359,6 @@ static const char * const pwm_axg_ee_parent_names[] = {
 
 static const struct meson_pwm_data pwm_axg_ee_data = {
 	.parent_names = pwm_axg_ee_parent_names,
-	.num_parents = ARRAY_SIZE(pwm_axg_ee_parent_names),
 };
 
 static const char * const pwm_axg_ao_parent_names[] = {
@@ -374,7 +367,6 @@ static const char * const pwm_axg_ao_parent_names[] = {
 
 static const struct meson_pwm_data pwm_axg_ao_data = {
 	.parent_names = pwm_axg_ao_parent_names,
-	.num_parents = ARRAY_SIZE(pwm_axg_ao_parent_names),
 };
 
 static const char * const pwm_g12a_ao_ab_parent_names[] = {
@@ -383,16 +375,14 @@ static const char * const pwm_g12a_ao_ab_parent_names[] = {
 
 static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
 	.parent_names = pwm_g12a_ao_ab_parent_names,
-	.num_parents = ARRAY_SIZE(pwm_g12a_ao_ab_parent_names),
 };
 
 static const char * const pwm_g12a_ao_cd_parent_names[] = {
-	"xtal", "g12a_ao_clk81",
+	"xtal", "g12a_ao_clk81", NULL, NULL,
 };
 
 static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
 	.parent_names = pwm_g12a_ao_cd_parent_names,
-	.num_parents = ARRAY_SIZE(pwm_g12a_ao_cd_parent_names),
 };
 
 static const struct of_device_id meson_pwm_matches[] = {
@@ -432,23 +422,25 @@ static const struct of_device_id meson_pwm_matches[] = {
 };
 MODULE_DEVICE_TABLE(of, meson_pwm_matches);
 
-static int meson_pwm_init_channels(struct meson_pwm *meson)
+static int meson_pwm_init_clocks_legacy(struct device *dev,
+					struct clk_parent_data *mux_parent_data)
 {
-	struct clk_parent_data mux_parent_data[MESON_MAX_MUX_PARENTS] = {};
-	struct device *dev = meson->chip.dev;
+	struct meson_pwm *meson = dev_get_drvdata(dev);
 	unsigned int i;
 	char name[255];
 	int err;
 
-	for (i = 0; i < meson->data->num_parents; i++) {
-		mux_parent_data[i].index = -1;
-		mux_parent_data[i].name = meson->data->parent_names[i];
-	}
-
-	for (i = 0; i < meson->chip.npwm; i++) {
+	for (i = 0; i < MESON_NUM_PWMS; i++) {
 		struct meson_pwm_channel *channel = &meson->channels[i];
 		struct clk_parent_data div_parent = {}, gate_parent = {};
 		struct clk_init_data init = {};
+		struct clk_divider *div;
+		struct clk_gate *gate;
+		struct clk_mux *mux;
+
+		mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+		if (!mux)
+			return -ENOMEM;
 
 		snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i);
 
@@ -456,69 +448,76 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
 		init.ops = &clk_mux_ops;
 		init.flags = 0;
 		init.parent_data = mux_parent_data;
-		init.num_parents = meson->data->num_parents;
-
-		channel->mux.reg = meson->base + REG_MISC_AB;
-		channel->mux.shift =
-				meson_pwm_per_channel_data[i].clk_sel_shift;
-		channel->mux.mask = MISC_CLK_SEL_MASK;
-		channel->mux.flags = 0;
-		channel->mux.lock = &meson->lock;
-		channel->mux.table = NULL;
-		channel->mux.hw.init = &init;
-
-		err = devm_clk_hw_register(dev, &channel->mux.hw);
+		init.num_parents = MESON_NUM_MUX_PARENTS;
+
+		mux->reg = meson->base + REG_MISC_AB;
+		mux->shift = meson_pwm_per_channel_data[i].clk_sel_shift;
+		mux->mask = MISC_CLK_SEL_MASK;
+		mux->flags = 0;
+		mux->lock = &meson->lock;
+		mux->table = NULL;
+		mux->hw.init = &init;
+
+		err = devm_clk_hw_register(dev, &mux->hw);
 		if (err) {
 			dev_err(dev, "failed to register %s: %d\n", name, err);
 			return err;
 		}
 
+		div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
+		if (!div)
+			return -ENOMEM;
+
 		snprintf(name, sizeof(name), "%s#div%u", dev_name(dev), i);
 
 		init.name = name;
 		init.ops = &clk_divider_ops;
 		init.flags = CLK_SET_RATE_PARENT;
 		div_parent.index = -1;
-		div_parent.hw = &channel->mux.hw;
+		div_parent.hw = &mux->hw;
 		init.parent_data = &div_parent;
 		init.num_parents = 1;
 
-		channel->div.reg = meson->base + REG_MISC_AB;
-		channel->div.shift = meson_pwm_per_channel_data[i].clk_div_shift;
-		channel->div.width = MISC_CLK_DIV_WIDTH;
-		channel->div.hw.init = &init;
-		channel->div.flags = 0;
-		channel->div.lock = &meson->lock;
+		div->reg = meson->base + REG_MISC_AB;
+		div->shift = meson_pwm_per_channel_data[i].clk_div_shift;
+		div->width = MISC_CLK_DIV_WIDTH;
+		div->hw.init = &init;
+		div->flags = 0;
+		div->lock = &meson->lock;
 
-		err = devm_clk_hw_register(dev, &channel->div.hw);
+		err = devm_clk_hw_register(dev, &div->hw);
 		if (err) {
 			dev_err(dev, "failed to register %s: %d\n", name, err);
 			return err;
 		}
 
+		gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
+		if (!gate)
+			return -ENOMEM;
+
 		snprintf(name, sizeof(name), "%s#gate%u", dev_name(dev), i);
 
 		init.name = name;
 		init.ops = &clk_gate_ops;
 		init.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED;
 		gate_parent.index = -1;
-		gate_parent.hw = &channel->div.hw;
+		gate_parent.hw = &div->hw;
 		init.parent_data = &gate_parent;
 		init.num_parents = 1;
 
-		channel->gate.reg = meson->base + REG_MISC_AB;
-		channel->gate.bit_idx = meson_pwm_per_channel_data[i].clk_en_shift;
-		channel->gate.hw.init = &init;
-		channel->gate.flags = 0;
-		channel->gate.lock = &meson->lock;
+		gate->reg = meson->base + REG_MISC_AB;
+		gate->bit_idx = meson_pwm_per_channel_data[i].clk_en_shift;
+		gate->hw.init = &init;
+		gate->flags = 0;
+		gate->lock = &meson->lock;
 
-		err = devm_clk_hw_register(dev, &channel->gate.hw);
+		err = devm_clk_hw_register(dev, &gate->hw);
 		if (err) {
 			dev_err(dev, "failed to register %s: %d\n", name, err);
 			return err;
 		}
 
-		channel->clk = devm_clk_hw_get_clk(dev, &channel->gate.hw, NULL);
+		channel->clk = devm_clk_hw_get_clk(dev, &gate->hw, NULL);
 		if (IS_ERR(channel->clk)) {
 			err = PTR_ERR(channel->clk);
 			dev_err(dev, "failed to register %s: %d\n", name, err);
@@ -529,31 +528,56 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
 	return 0;
 }
 
+static int meson_pwm_init_channels(struct device *dev)
+{
+	struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
+	struct meson_pwm *meson = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < MESON_NUM_MUX_PARENTS; i++) {
+		mux_parent_data[i].index = -1;
+		mux_parent_data[i].name = meson->data->parent_names[i];
+	}
+
+	return meson_pwm_init_clocks_legacy(dev, mux_parent_data);
+}
+
 static int meson_pwm_probe(struct platform_device *pdev)
 {
 	struct meson_pwm *meson;
+	struct pwm_chip *chip;
 	int err;
 
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
 	meson = devm_kzalloc(&pdev->dev, sizeof(*meson), GFP_KERNEL);
 	if (!meson)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, meson);
+
 	meson->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(meson->base))
 		return PTR_ERR(meson->base);
 
 	spin_lock_init(&meson->lock);
-	meson->chip.dev = &pdev->dev;
-	meson->chip.ops = &meson_pwm_ops;
-	meson->chip.npwm = MESON_NUM_PWMS;
+	chip->dev = &pdev->dev;
+	chip->ops = &meson_pwm_ops;
+	chip->npwm = MESON_NUM_PWMS;
 
 	meson->data = of_device_get_match_data(&pdev->dev);
+	if (!meson->data) {
+		dev_err(&pdev->dev, "failed to match device\n");
+		return -ENODEV;
+	}
 
-	err = meson_pwm_init_channels(meson);
+	err = meson_pwm_init_channels(&pdev->dev);
 	if (err < 0)
 		return err;
 
-	err = devm_pwmchip_add(&pdev->dev, &meson->chip);
+	err = devm_pwmchip_add(&pdev->dev, chip);
 	if (err < 0) {
 		dev_err(&pdev->dev, "failed to register PWM chip: %d\n", err);
 		return err;
-- 
2.42.0

Re: [PATCH v3 3/4] pwm: meson: prepare addition of new compatible types

[Neil Armstrong]

Hi,

On 29/11/2023 14:39, Jerome Brunet wrote:
> Clean the amlogic pwm driver to prepare the addition of new pwm compatibles
> * Generalize 4 inputs clock per channel.
>    AO pwm may just get 2 extra NULL entries which actually better
>    describes the reality of the HW.
> * Use driver data to carry the device data and remove pwm_chip from it
> * Stop carrying the internal clock elements with the device data.
>    These are not needed past init.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/pwm/pwm-meson.c | 150 +++++++++++++++++++++++-----------------
>   1 file changed, 87 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 5bea53243ed2..5cbd65cae28a 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -60,7 +60,7 @@
>   #define MISC_A_EN		BIT(0)
>   
>   #define MESON_NUM_PWMS		2
> -#define MESON_MAX_MUX_PARENTS	4
> +#define MESON_NUM_MUX_PARENTS	4
>   
>   static struct meson_pwm_channel_data {
>   	u8		reg_offset;
> @@ -90,19 +90,14 @@ struct meson_pwm_channel {
>   	unsigned int hi;
>   	unsigned int lo;
>   
> -	struct clk_mux mux;
> -	struct clk_divider div;
> -	struct clk_gate gate;
>   	struct clk *clk;
>   };
>   
>   struct meson_pwm_data {
>   	const char * const *parent_names;
> -	unsigned int num_parents;
>   };
>   
>   struct meson_pwm {
> -	struct pwm_chip chip;

It seems you're mixing multiple things in the same patch,
you should split it to implement the pwm_chip migration,
and parent_names/clk init in 2 patches.

>   	const struct meson_pwm_data *data;
>   	struct meson_pwm_channel channels[MESON_NUM_PWMS];
>   	void __iomem *base;
> @@ -115,7 +110,7 @@ struct meson_pwm {
>   
>   static inline struct meson_pwm *to_meson_pwm(struct pwm_chip *chip)
>   {
> -	return container_of(chip, struct meson_pwm, chip);
> +	return dev_get_drvdata(chip->dev);
>   }
>   
>   static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -147,6 +142,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>   			  const struct pwm_state *state)
>   {
>   	struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm];
> +	struct device *dev = pwm->chip->dev;
>   	unsigned int cnt, duty_cnt;
>   	unsigned long fin_freq;
>   	u64 duty, period, freq;
> @@ -169,19 +165,19 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>   
>   	fin_freq = clk_round_rate(channel->clk, freq);
>   	if (fin_freq == 0) {
> -		dev_err(meson->chip.dev, "invalid source clock frequency\n");
> +		dev_err(dev, "invalid source clock frequency\n");
>   		return -EINVAL;
>   	}
>   
> -	dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
> +	dev_dbg(dev, "fin_freq: %lu Hz\n", fin_freq);
>   
>   	cnt = div_u64(fin_freq * period, NSEC_PER_SEC);
>   	if (cnt > 0xffff) {
> -		dev_err(meson->chip.dev, "unable to get period cnt\n");
> +		dev_err(dev, "unable to get period cnt\n");
>   		return -EINVAL;
>   	}
>   
> -	dev_dbg(meson->chip.dev, "period=%llu cnt=%u\n", period, cnt);
> +	dev_dbg(dev, "period=%llu cnt=%u\n", period, cnt);
>   
>   	if (duty == period) {
>   		channel->hi = cnt;
> @@ -192,7 +188,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>   	} else {
>   		duty_cnt = div_u64(fin_freq * duty, NSEC_PER_SEC);
>   
> -		dev_dbg(meson->chip.dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
> +		dev_dbg(dev, "duty=%llu duty_cnt=%u\n", duty, duty_cnt);
>   
>   		channel->hi = duty_cnt;
>   		channel->lo = cnt - duty_cnt;
> @@ -215,7 +211,7 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
>   
>   	err = clk_set_rate(channel->clk, channel->rate);
>   	if (err)
> -		dev_err(meson->chip.dev, "setting clock rate failed\n");
> +		dev_err(pwm->chip->dev, "setting clock rate failed\n");
>   
>   	spin_lock_irqsave(&meson->lock, flags);
>   
> @@ -343,7 +339,6 @@ static const char * const pwm_meson8b_parent_names[] = {
>   
>   static const struct meson_pwm_data pwm_meson8b_data = {
>   	.parent_names = pwm_meson8b_parent_names,
> -	.num_parents = ARRAY_SIZE(pwm_meson8b_parent_names),
>   };
>   
>   /*
> @@ -351,12 +346,11 @@ static const struct meson_pwm_data pwm_meson8b_data = {
>    * The last 2 are grounded
>    */
>   static const char * const pwm_gxbb_ao_parent_names[] = {
> -	"xtal", "clk81"
> +	"xtal", "clk81", NULL, NULL,
>   };
>   
>   static const struct meson_pwm_data pwm_gxbb_ao_data = {
>   	.parent_names = pwm_gxbb_ao_parent_names,
> -	.num_parents = ARRAY_SIZE(pwm_gxbb_ao_parent_names),
>   };
>   
>   static const char * const pwm_axg_ee_parent_names[] = {
> @@ -365,7 +359,6 @@ static const char * const pwm_axg_ee_parent_names[] = {
>   
>   static const struct meson_pwm_data pwm_axg_ee_data = {
>   	.parent_names = pwm_axg_ee_parent_names,
> -	.num_parents = ARRAY_SIZE(pwm_axg_ee_parent_names),
>   };
>   
>   static const char * const pwm_axg_ao_parent_names[] = {
> @@ -374,7 +367,6 @@ static const char * const pwm_axg_ao_parent_names[] = {
>   
>   static const struct meson_pwm_data pwm_axg_ao_data = {
>   	.parent_names = pwm_axg_ao_parent_names,
> -	.num_parents = ARRAY_SIZE(pwm_axg_ao_parent_names),
>   };
>   
>   static const char * const pwm_g12a_ao_ab_parent_names[] = {
> @@ -383,16 +375,14 @@ static const char * const pwm_g12a_ao_ab_parent_names[] = {
>   
>   static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
>   	.parent_names = pwm_g12a_ao_ab_parent_names,
> -	.num_parents = ARRAY_SIZE(pwm_g12a_ao_ab_parent_names),
>   };
>   
>   static const char * const pwm_g12a_ao_cd_parent_names[] = {
> -	"xtal", "g12a_ao_clk81",
> +	"xtal", "g12a_ao_clk81", NULL, NULL,
>   };
>   
>   static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
>   	.parent_names = pwm_g12a_ao_cd_parent_names,
> -	.num_parents = ARRAY_SIZE(pwm_g12a_ao_cd_parent_names),
>   };
>   
>   static const struct of_device_id meson_pwm_matches[] = {
> @@ -432,23 +422,25 @@ static const struct of_device_id meson_pwm_matches[] = {
>   };
>   MODULE_DEVICE_TABLE(of, meson_pwm_matches);
>   
> -static int meson_pwm_init_channels(struct meson_pwm *meson)
> +static int meson_pwm_init_clocks_legacy(struct device *dev,
> +					struct clk_parent_data *mux_parent_data)

It's not yet legacy, the rename should be in the next patch

>   {
> -	struct clk_parent_data mux_parent_data[MESON_MAX_MUX_PARENTS] = {};
> -	struct device *dev = meson->chip.dev;
> +	struct meson_pwm *meson = dev_get_drvdata(dev);
>   	unsigned int i;
>   	char name[255];
>   	int err;
>   
> -	for (i = 0; i < meson->data->num_parents; i++) {
> -		mux_parent_data[i].index = -1;
> -		mux_parent_data[i].name = meson->data->parent_names[i];
> -	}
> -
> -	for (i = 0; i < meson->chip.npwm; i++) {
> +	for (i = 0; i < MESON_NUM_PWMS; i++) {
>   		struct meson_pwm_channel *channel = &meson->channels[i];
>   		struct clk_parent_data div_parent = {}, gate_parent = {};
>   		struct clk_init_data init = {};
> +		struct clk_divider *div;
> +		struct clk_gate *gate;
> +		struct clk_mux *mux;
> +
> +		mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> +		if (!mux)
> +			return -ENOMEM;
>   
>   		snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i);
>   
> @@ -456,69 +448,76 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
>   		init.ops = &clk_mux_ops;
>   		init.flags = 0;
>   		init.parent_data = mux_parent_data;
> -		init.num_parents = meson->data->num_parents;
> -
> -		channel->mux.reg = meson->base + REG_MISC_AB;
> -		channel->mux.shift =
> -				meson_pwm_per_channel_data[i].clk_sel_shift;
> -		channel->mux.mask = MISC_CLK_SEL_MASK;
> -		channel->mux.flags = 0;
> -		channel->mux.lock = &meson->lock;
> -		channel->mux.table = NULL;
> -		channel->mux.hw.init = &init;
> -
> -		err = devm_clk_hw_register(dev, &channel->mux.hw);
> +		init.num_parents = MESON_NUM_MUX_PARENTS;
> +
> +		mux->reg = meson->base + REG_MISC_AB;
> +		mux->shift = meson_pwm_per_channel_data[i].clk_sel_shift;
> +		mux->mask = MISC_CLK_SEL_MASK;
> +		mux->flags = 0;
> +		mux->lock = &meson->lock;
> +		mux->table = NULL;
> +		mux->hw.init = &init;
> +
> +		err = devm_clk_hw_register(dev, &mux->hw);
>   		if (err) {
>   			dev_err(dev, "failed to register %s: %d\n", name, err);
>   			return err;
>   		}
>   
> +		div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
> +		if (!div)
> +			return -ENOMEM;
> +
>   		snprintf(name, sizeof(name), "%s#div%u", dev_name(dev), i);
>   
>   		init.name = name;
>   		init.ops = &clk_divider_ops;
>   		init.flags = CLK_SET_RATE_PARENT;
>   		div_parent.index = -1;
> -		div_parent.hw = &channel->mux.hw;
> +		div_parent.hw = &mux->hw;
>   		init.parent_data = &div_parent;
>   		init.num_parents = 1;
>   
> -		channel->div.reg = meson->base + REG_MISC_AB;
> -		channel->div.shift = meson_pwm_per_channel_data[i].clk_div_shift;
> -		channel->div.width = MISC_CLK_DIV_WIDTH;
> -		channel->div.hw.init = &init;
> -		channel->div.flags = 0;
> -		channel->div.lock = &meson->lock;
> +		div->reg = meson->base + REG_MISC_AB;
> +		div->shift = meson_pwm_per_channel_data[i].clk_div_shift;
> +		div->width = MISC_CLK_DIV_WIDTH;
> +		div->hw.init = &init;
> +		div->flags = 0;
> +		div->lock = &meson->lock;
>   
> -		err = devm_clk_hw_register(dev, &channel->div.hw);
> +		err = devm_clk_hw_register(dev, &div->hw);
>   		if (err) {
>   			dev_err(dev, "failed to register %s: %d\n", name, err);
>   			return err;
>   		}
>   
> +		gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
> +		if (!gate)
> +			return -ENOMEM;
> +
>   		snprintf(name, sizeof(name), "%s#gate%u", dev_name(dev), i);
>   
>   		init.name = name;
>   		init.ops = &clk_gate_ops;
>   		init.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED;
>   		gate_parent.index = -1;
> -		gate_parent.hw = &channel->div.hw;
> +		gate_parent.hw = &div->hw;
>   		init.parent_data = &gate_parent;
>   		init.num_parents = 1;
>   
> -		channel->gate.reg = meson->base + REG_MISC_AB;
> -		channel->gate.bit_idx = meson_pwm_per_channel_data[i].clk_en_shift;
> -		channel->gate.hw.init = &init;
> -		channel->gate.flags = 0;
> -		channel->gate.lock = &meson->lock;
> +		gate->reg = meson->base + REG_MISC_AB;
> +		gate->bit_idx = meson_pwm_per_channel_data[i].clk_en_shift;
> +		gate->hw.init = &init;
> +		gate->flags = 0;
> +		gate->lock = &meson->lock;
>   
> -		err = devm_clk_hw_register(dev, &channel->gate.hw);
> +		err = devm_clk_hw_register(dev, &gate->hw);
>   		if (err) {
>   			dev_err(dev, "failed to register %s: %d\n", name, err);
>   			return err;
>   		}
>   
> -		channel->clk = devm_clk_hw_get_clk(dev, &channel->gate.hw, NULL);
> +		channel->clk = devm_clk_hw_get_clk(dev, &gate->hw, NULL);
>   		if (IS_ERR(channel->clk)) {
>   			err = PTR_ERR(channel->clk);
>   			dev_err(dev, "failed to register %s: %d\n", name, err);
> @@ -529,31 +528,56 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
>   	return 0;
>   }
>   
> +static int meson_pwm_init_channels(struct device *dev)
> +{
> +	struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
> +	struct meson_pwm *meson = dev_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < MESON_NUM_MUX_PARENTS; i++) {
> +		mux_parent_data[i].index = -1;
> +		mux_parent_data[i].name = meson->data->parent_names[i];
> +	}
> +
> +	return meson_pwm_init_clocks_legacy(dev, mux_parent_data);
> +}
> +
>   static int meson_pwm_probe(struct platform_device *pdev)
>   {
>   	struct meson_pwm *meson;
> +	struct pwm_chip *chip;
>   	int err;
>   
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
>   	meson = devm_kzalloc(&pdev->dev, sizeof(*meson), GFP_KERNEL);
>   	if (!meson)
>   		return -ENOMEM;
>   
> +	platform_set_drvdata(pdev, meson);
> +
>   	meson->base = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(meson->base))
>   		return PTR_ERR(meson->base);
>   
>   	spin_lock_init(&meson->lock);
> -	meson->chip.dev = &pdev->dev;
> -	meson->chip.ops = &meson_pwm_ops;
> -	meson->chip.npwm = MESON_NUM_PWMS;
> +	chip->dev = &pdev->dev;
> +	chip->ops = &meson_pwm_ops;
> +	chip->npwm = MESON_NUM_PWMS;
>   
>   	meson->data = of_device_get_match_data(&pdev->dev);
> +	if (!meson->data) {
> +		dev_err(&pdev->dev, "failed to match device\n");
> +		return -ENODEV;
> +	}
>   
> -	err = meson_pwm_init_channels(meson);
> +	err = meson_pwm_init_channels(&pdev->dev);
>   	if (err < 0)
>   		return err;
>   
> -	err = devm_pwmchip_add(&pdev->dev, &meson->chip);
> +	err = devm_pwmchip_add(&pdev->dev, chip);
>   	if (err < 0) {
>   		dev_err(&pdev->dev, "failed to register PWM chip: %d\n", err);
>   		return err;

Thanks,
Neil

[PATCH v3 4/4] pwm: meson: add generic compatible for meson8 to sm1

[Jerome Brunet]

Introduce a new compatible support in the Amlogic PWM driver.

The PWM HW is actually the same for all SoCs supported so far.
A specific compatible is needed only because the clock sources
of the PWMs are hard-coded in the driver.

It is better to have the clock source described in DT but this
changes the bindings so a new compatible must be introduced.

When all supported platform have migrated to the new compatible,
support for the legacy ones may be removed from the driver.

Adding a callback to setup the clock will also make it easier
to add support for the new PWM HW found in a1, s4, c3 and t7 SoC
families.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/pwm/pwm-meson.c | 224 ++++++++++++++++++++++++----------------
 1 file changed, 133 insertions(+), 91 deletions(-)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 5cbd65cae28a..d5d745a651d3 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -95,6 +95,7 @@ struct meson_pwm_channel {
 
 struct meson_pwm_data {
 	const char * const *parent_names;
+	int (*channels_init)(struct device *dev);
 };
 
 struct meson_pwm {
@@ -333,95 +334,6 @@ static const struct pwm_ops meson_pwm_ops = {
 	.get_state = meson_pwm_get_state,
 };
 
-static const char * const pwm_meson8b_parent_names[] = {
-	"xtal", NULL, "fclk_div4", "fclk_div3"
-};
-
-static const struct meson_pwm_data pwm_meson8b_data = {
-	.parent_names = pwm_meson8b_parent_names,
-};
-
-/*
- * Only the 2 first inputs of the GXBB AO PWMs are valid
- * The last 2 are grounded
- */
-static const char * const pwm_gxbb_ao_parent_names[] = {
-	"xtal", "clk81", NULL, NULL,
-};
-
-static const struct meson_pwm_data pwm_gxbb_ao_data = {
-	.parent_names = pwm_gxbb_ao_parent_names,
-};
-
-static const char * const pwm_axg_ee_parent_names[] = {
-	"xtal", "fclk_div5", "fclk_div4", "fclk_div3"
-};
-
-static const struct meson_pwm_data pwm_axg_ee_data = {
-	.parent_names = pwm_axg_ee_parent_names,
-};
-
-static const char * const pwm_axg_ao_parent_names[] = {
-	"xtal", "axg_ao_clk81", "fclk_div4", "fclk_div5"
-};
-
-static const struct meson_pwm_data pwm_axg_ao_data = {
-	.parent_names = pwm_axg_ao_parent_names,
-};
-
-static const char * const pwm_g12a_ao_ab_parent_names[] = {
-	"xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5"
-};
-
-static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
-	.parent_names = pwm_g12a_ao_ab_parent_names,
-};
-
-static const char * const pwm_g12a_ao_cd_parent_names[] = {
-	"xtal", "g12a_ao_clk81", NULL, NULL,
-};
-
-static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
-	.parent_names = pwm_g12a_ao_cd_parent_names,
-};
-
-static const struct of_device_id meson_pwm_matches[] = {
-	{
-		.compatible = "amlogic,meson8b-pwm",
-		.data = &pwm_meson8b_data
-	},
-	{
-		.compatible = "amlogic,meson-gxbb-pwm",
-		.data = &pwm_meson8b_data
-	},
-	{
-		.compatible = "amlogic,meson-gxbb-ao-pwm",
-		.data = &pwm_gxbb_ao_data
-	},
-	{
-		.compatible = "amlogic,meson-axg-ee-pwm",
-		.data = &pwm_axg_ee_data
-	},
-	{
-		.compatible = "amlogic,meson-axg-ao-pwm",
-		.data = &pwm_axg_ao_data
-	},
-	{
-		.compatible = "amlogic,meson-g12a-ee-pwm",
-		.data = &pwm_meson8b_data
-	},
-	{
-		.compatible = "amlogic,meson-g12a-ao-pwm-ab",
-		.data = &pwm_g12a_ao_ab_data
-	},
-	{
-		.compatible = "amlogic,meson-g12a-ao-pwm-cd",
-		.data = &pwm_g12a_ao_cd_data
-	},
-	{},
-};
-MODULE_DEVICE_TABLE(of, meson_pwm_matches);
-
 static int meson_pwm_init_clocks_legacy(struct device *dev,
 					struct clk_parent_data *mux_parent_data)
 {
@@ -528,12 +440,15 @@ static int meson_pwm_init_clocks_legacy(struct device *dev,
 	return 0;
 }
 
-static int meson_pwm_init_channels(struct device *dev)
+static int meson_pwm_init_channels_legacy(struct device *dev)
 {
 	struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
 	struct meson_pwm *meson = dev_get_drvdata(dev);
 	int i;
 
+	dev_info(dev, "using obsolete compatible, please consider updating dt\n");
+
+
 	for (i = 0; i < MESON_NUM_MUX_PARENTS; i++) {
 		mux_parent_data[i].index = -1;
 		mux_parent_data[i].name = meson->data->parent_names[i];
@@ -542,6 +457,133 @@ static int meson_pwm_init_channels(struct device *dev)
 	return meson_pwm_init_clocks_legacy(dev, mux_parent_data);
 }
 
+static int meson_pwm_init_channels_meson8b_v2(struct device *dev)
+{
+	struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
+	int i;
+
+	/*
+	 * NOTE: Instead of relying on the hard coded names in the driver
+	 * as the legacy version, this relies on DT to provide the list of
+	 * clocks.
+	 * For once, using input numbers actually makes more sense than names.
+	 * Also DT requires clock-names to be explicitly ordered, so there is
+	 * no point bothering with clock names in this case.
+	 */
+	for (i = 0; i < MESON_NUM_MUX_PARENTS; i++)
+		mux_parent_data[i].index = i;
+
+	return meson_pwm_init_clocks_legacy(dev, mux_parent_data);
+}
+
+static const char * const pwm_meson8b_parent_names[] = {
+	"xtal", NULL, "fclk_div4", "fclk_div3"
+};
+
+static const struct meson_pwm_data pwm_meson8b_data = {
+	.parent_names = pwm_meson8b_parent_names,
+	.channels_init = meson_pwm_init_channels_legacy,
+};
+
+/*
+ * Only the 2 first inputs of the GXBB AO PWMs are valid
+ * The last 2 are grounded
+ */
+static const char * const pwm_gxbb_ao_parent_names[] = {
+	"xtal", "clk81", NULL, NULL,
+};
+
+static const struct meson_pwm_data pwm_gxbb_ao_data = {
+	.parent_names = pwm_gxbb_ao_parent_names,
+	.channels_init = meson_pwm_init_channels_legacy,
+};
+
+static const char * const pwm_axg_ee_parent_names[] = {
+	"xtal", "fclk_div5", "fclk_div4", "fclk_div3"
+};
+
+static const struct meson_pwm_data pwm_axg_ee_data = {
+	.parent_names = pwm_axg_ee_parent_names,
+	.channels_init = meson_pwm_init_channels_legacy,
+};
+
+static const char * const pwm_axg_ao_parent_names[] = {
+	"xtal", "axg_ao_clk81", "fclk_div4", "fclk_div5"
+};
+
+static const struct meson_pwm_data pwm_axg_ao_data = {
+	.parent_names = pwm_axg_ao_parent_names,
+	.channels_init = meson_pwm_init_channels_legacy,
+};
+
+static const char * const pwm_g12a_ao_ab_parent_names[] = {
+	"xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5"
+};
+
+static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
+	.parent_names = pwm_g12a_ao_ab_parent_names,
+	.channels_init = meson_pwm_init_channels_legacy,
+};
+
+static const char * const pwm_g12a_ao_cd_parent_names[] = {
+	"xtal", "g12a_ao_clk81", NULL, NULL,
+};
+
+static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
+	.parent_names = pwm_g12a_ao_cd_parent_names,
+	.channels_init = meson_pwm_init_channels_legacy,
+};
+
+static const struct meson_pwm_data pwm_meson8_v2_data = {
+	.channels_init = meson_pwm_init_channels_meson8b_v2,
+};
+
+static const struct of_device_id meson_pwm_matches[] = {
+	{
+		.compatible = "amlogic,meson8-pwm-v2",
+		.data = &pwm_meson8_v2_data
+	},
+	/*
+	 * The following compatibles are obsolete.
+	 * Support for these may be removed once the related
+	 * platforms have been updated
+	 */
+	{
+		.compatible = "amlogic,meson8b-pwm",
+		.data = &pwm_meson8b_data
+	},
+	{
+		.compatible = "amlogic,meson-gxbb-pwm",
+		.data = &pwm_meson8b_data
+	},
+	{
+		.compatible = "amlogic,meson-gxbb-ao-pwm",
+		.data = &pwm_gxbb_ao_data
+	},
+	{
+		.compatible = "amlogic,meson-axg-ee-pwm",
+		.data = &pwm_axg_ee_data
+	},
+	{
+		.compatible = "amlogic,meson-axg-ao-pwm",
+		.data = &pwm_axg_ao_data
+	},
+	{
+		.compatible = "amlogic,meson-g12a-ee-pwm",
+		.data = &pwm_meson8b_data
+	},
+	{
+		.compatible = "amlogic,meson-g12a-ao-pwm-ab",
+		.data = &pwm_g12a_ao_ab_data
+	},
+	{
+		.compatible = "amlogic,meson-g12a-ao-pwm-cd",
+		.data = &pwm_g12a_ao_cd_data
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, meson_pwm_matches);
+
 static int meson_pwm_probe(struct platform_device *pdev)
 {
 	struct meson_pwm *meson;
@@ -573,7 +615,7 @@ static int meson_pwm_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	err = meson_pwm_init_channels(&pdev->dev);
+	err = meson->data->channels_init(&pdev->dev);
 	if (err < 0)
 		return err;
 
-- 
2.42.0

Re: [PATCH v3 4/4] pwm: meson: add generic compatible for meson8 to sm1

[Neil Armstrong]

HI,

On 29/11/2023 14:40, Jerome Brunet wrote:
> Introduce a new compatible support in the Amlogic PWM driver.
> 
> The PWM HW is actually the same for all SoCs supported so far.
> A specific compatible is needed only because the clock sources
> of the PWMs are hard-coded in the driver.
> 
> It is better to have the clock source described in DT but this
> changes the bindings so a new compatible must be introduced.
> 
> When all supported platform have migrated to the new compatible,
> support for the legacy ones may be removed from the driver.
> 
> Adding a callback to setup the clock will also make it easier
> to add support for the new PWM HW found in a1, s4, c3 and t7 SoC
> families.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/pwm/pwm-meson.c | 224 ++++++++++++++++++++++++----------------
>   1 file changed, 133 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 5cbd65cae28a..d5d745a651d3 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -95,6 +95,7 @@ struct meson_pwm_channel {
>   
>   struct meson_pwm_data {
>   	const char * const *parent_names;
> +	int (*channels_init)(struct device *dev);
>   };
>   
>   struct meson_pwm {
> @@ -333,95 +334,6 @@ static const struct pwm_ops meson_pwm_ops = {
>   	.get_state = meson_pwm_get_state,
>   };
>   
> -static const char * const pwm_meson8b_parent_names[] = {
> -	"xtal", NULL, "fclk_div4", "fclk_div3"
> -};
> -
> -static const struct meson_pwm_data pwm_meson8b_data = {
> -	.parent_names = pwm_meson8b_parent_names,
> -};
> -
> -/*
> - * Only the 2 first inputs of the GXBB AO PWMs are valid
> - * The last 2 are grounded
> - */
> -static const char * const pwm_gxbb_ao_parent_names[] = {
> -	"xtal", "clk81", NULL, NULL,
> -};
> -
> -static const struct meson_pwm_data pwm_gxbb_ao_data = {
> -	.parent_names = pwm_gxbb_ao_parent_names,
> -};
> -
> -static const char * const pwm_axg_ee_parent_names[] = {
> -	"xtal", "fclk_div5", "fclk_div4", "fclk_div3"
> -};
> -
> -static const struct meson_pwm_data pwm_axg_ee_data = {
> -	.parent_names = pwm_axg_ee_parent_names,
> -};
> -
> -static const char * const pwm_axg_ao_parent_names[] = {
> -	"xtal", "axg_ao_clk81", "fclk_div4", "fclk_div5"
> -};
> -
> -static const struct meson_pwm_data pwm_axg_ao_data = {
> -	.parent_names = pwm_axg_ao_parent_names,
> -};
> -
> -static const char * const pwm_g12a_ao_ab_parent_names[] = {
> -	"xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5"
> -};
> -
> -static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
> -	.parent_names = pwm_g12a_ao_ab_parent_names,
> -};
> -
> -static const char * const pwm_g12a_ao_cd_parent_names[] = {
> -	"xtal", "g12a_ao_clk81", NULL, NULL,
> -};
> -
> -static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
> -	.parent_names = pwm_g12a_ao_cd_parent_names,
> -};
> -
> -static const struct of_device_id meson_pwm_matches[] = {
> -	{
> -		.compatible = "amlogic,meson8b-pwm",
> -		.data = &pwm_meson8b_data
> -	},
> -	{
> -		.compatible = "amlogic,meson-gxbb-pwm",
> -		.data = &pwm_meson8b_data
> -	},
> -	{
> -		.compatible = "amlogic,meson-gxbb-ao-pwm",
> -		.data = &pwm_gxbb_ao_data
> -	},
> -	{
> -		.compatible = "amlogic,meson-axg-ee-pwm",
> -		.data = &pwm_axg_ee_data
> -	},
> -	{
> -		.compatible = "amlogic,meson-axg-ao-pwm",
> -		.data = &pwm_axg_ao_data
> -	},
> -	{
> -		.compatible = "amlogic,meson-g12a-ee-pwm",
> -		.data = &pwm_meson8b_data
> -	},
> -	{
> -		.compatible = "amlogic,meson-g12a-ao-pwm-ab",
> -		.data = &pwm_g12a_ao_ab_data
> -	},
> -	{
> -		.compatible = "amlogic,meson-g12a-ao-pwm-cd",
> -		.data = &pwm_g12a_ao_cd_data
> -	},
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, meson_pwm_matches);
> -
>   static int meson_pwm_init_clocks_legacy(struct device *dev,
>   					struct clk_parent_data *mux_parent_data)
>   {
> @@ -528,12 +440,15 @@ static int meson_pwm_init_clocks_legacy(struct device *dev,
>   	return 0;
>   }
>   
> -static int meson_pwm_init_channels(struct device *dev)
> +static int meson_pwm_init_channels_legacy(struct device *dev)
>   {
>   	struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
>   	struct meson_pwm *meson = dev_get_drvdata(dev);
>   	int i;
>   
> +	dev_info(dev, "using obsolete compatible, please consider updating dt\n");

I think dev_warn_once would be more appropriate

> +
> +
>   	for (i = 0; i < MESON_NUM_MUX_PARENTS; i++) {
>   		mux_parent_data[i].index = -1;
>   		mux_parent_data[i].name = meson->data->parent_names[i];
> @@ -542,6 +457,133 @@ static int meson_pwm_init_channels(struct device *dev)
>   	return meson_pwm_init_clocks_legacy(dev, mux_parent_data);
>   }
>   
> +static int meson_pwm_init_channels_meson8b_v2(struct device *dev)
> +{
> +	struct clk_parent_data mux_parent_data[MESON_NUM_MUX_PARENTS] = {};
> +	int i;
> +
> +	/*
> +	 * NOTE: Instead of relying on the hard coded names in the driver
> +	 * as the legacy version, this relies on DT to provide the list of
> +	 * clocks.
> +	 * For once, using input numbers actually makes more sense than names.
> +	 * Also DT requires clock-names to be explicitly ordered, so there is
> +	 * no point bothering with clock names in this case.
> +	 */
> +	for (i = 0; i < MESON_NUM_MUX_PARENTS; i++)
> +		mux_parent_data[i].index = i;
> +
> +	return meson_pwm_init_clocks_legacy(dev, mux_parent_data);
> +}
> +
> +static const char * const pwm_meson8b_parent_names[] = {
> +	"xtal", NULL, "fclk_div4", "fclk_div3"
> +};
> +
> +static const struct meson_pwm_data pwm_meson8b_data = {
> +	.parent_names = pwm_meson8b_parent_names,
> +	.channels_init = meson_pwm_init_channels_legacy,
> +};
> +
> +/*
> + * Only the 2 first inputs of the GXBB AO PWMs are valid
> + * The last 2 are grounded
> + */
> +static const char * const pwm_gxbb_ao_parent_names[] = {
> +	"xtal", "clk81", NULL, NULL,
> +};
> +
> +static const struct meson_pwm_data pwm_gxbb_ao_data = {
> +	.parent_names = pwm_gxbb_ao_parent_names,
> +	.channels_init = meson_pwm_init_channels_legacy,
> +};
> +
> +static const char * const pwm_axg_ee_parent_names[] = {
> +	"xtal", "fclk_div5", "fclk_div4", "fclk_div3"
> +};
> +
> +static const struct meson_pwm_data pwm_axg_ee_data = {
> +	.parent_names = pwm_axg_ee_parent_names,
> +	.channels_init = meson_pwm_init_channels_legacy,
> +};
> +
> +static const char * const pwm_axg_ao_parent_names[] = {
> +	"xtal", "axg_ao_clk81", "fclk_div4", "fclk_div5"
> +};
> +
> +static const struct meson_pwm_data pwm_axg_ao_data = {
> +	.parent_names = pwm_axg_ao_parent_names,
> +	.channels_init = meson_pwm_init_channels_legacy,
> +};
> +
> +static const char * const pwm_g12a_ao_ab_parent_names[] = {
> +	"xtal", "g12a_ao_clk81", "fclk_div4", "fclk_div5"
> +};
> +
> +static const struct meson_pwm_data pwm_g12a_ao_ab_data = {
> +	.parent_names = pwm_g12a_ao_ab_parent_names,
> +	.channels_init = meson_pwm_init_channels_legacy,
> +};
> +
> +static const char * const pwm_g12a_ao_cd_parent_names[] = {
> +	"xtal", "g12a_ao_clk81", NULL, NULL,
> +};
> +
> +static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
> +	.parent_names = pwm_g12a_ao_cd_parent_names,
> +	.channels_init = meson_pwm_init_channels_legacy,
> +};
> +
> +static const struct meson_pwm_data pwm_meson8_v2_data = {
> +	.channels_init = meson_pwm_init_channels_meson8b_v2,
> +};
> +
> +static const struct of_device_id meson_pwm_matches[] = {
> +	{
> +		.compatible = "amlogic,meson8-pwm-v2",
> +		.data = &pwm_meson8_v2_data
> +	},
> +	/*
> +	 * The following compatibles are obsolete.
> +	 * Support for these may be removed once the related
> +	 * platforms have been updated
> +	 */

Not really, support will be needed until there's DT in the
wild with the old bindings, which is likely forever.
Drop the 2 last lines, only specify they are obsolete, and
perhaps note support for legacy bindings will be kept as
best effort but regressions may happen or something similar.

> +	{
> +		.compatible = "amlogic,meson8b-pwm",
> +		.data = &pwm_meson8b_data
> +	},
> +	{
> +		.compatible = "amlogic,meson-gxbb-pwm",
> +		.data = &pwm_meson8b_data
> +	},
> +	{
> +		.compatible = "amlogic,meson-gxbb-ao-pwm",
> +		.data = &pwm_gxbb_ao_data
> +	},
> +	{
> +		.compatible = "amlogic,meson-axg-ee-pwm",
> +		.data = &pwm_axg_ee_data
> +	},
> +	{
> +		.compatible = "amlogic,meson-axg-ao-pwm",
> +		.data = &pwm_axg_ao_data
> +	},
> +	{
> +		.compatible = "amlogic,meson-g12a-ee-pwm",
> +		.data = &pwm_meson8b_data
> +	},
> +	{
> +		.compatible = "amlogic,meson-g12a-ao-pwm-ab",
> +		.data = &pwm_g12a_ao_ab_data
> +	},
> +	{
> +		.compatible = "amlogic,meson-g12a-ao-pwm-cd",
> +		.data = &pwm_g12a_ao_cd_data
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, meson_pwm_matches);
> +
>   static int meson_pwm_probe(struct platform_device *pdev)
>   {
>   	struct meson_pwm *meson;
> @@ -573,7 +615,7 @@ static int meson_pwm_probe(struct platform_device *pdev)
>   		return -ENODEV;
>   	}
>   
> -	err = meson_pwm_init_channels(&pdev->dev);
> +	err = meson->data->channels_init(&pdev->dev);
>   	if (err < 0)
>   		return err;
>   

Apart the dev_info change and the meson_pwm_init_clocks_legacy rename, it looks fine.

Neil