[PATCH v2 0/2] pwm: Introduce pwm driver for the Loongson family chips

[PATCH v2 0/2] pwm: Introduce pwm driver for the Loongson family chips

[Binbin Zhou]

Hi all:

This patchset introduce a generic PWM framework driver for Loongson
family

Each PWM has one pulse width output signal and one pulse input signal to
be measured. It can be found on Loongson-2K series cpus and Loongson
LS7A bridge chips.

Thanks.

-------
v2:
- Remove the dts-related patches and update dts at once after all
relevant drivers are complete.
patch (1/2):
 - The dt-binding filename should match compatible, rename it as
   loongson,ls7a-pwm.yaml;
 - Update binding description;
 - Add description for each pwm cell;
 - Drop '#pwm-cells' from required, for pwm.yaml makes it required already.

Link to v1:
https://lore.kernel.org/linux-pwm/cover.1711953223.git.zhoubinbin@loongson.cn/

Binbin Zhou (2):
  dt-bindings: pwm: Add Loongson PWM controller
  pwm: Add Loongson PWM controller support

 .../bindings/pwm/loongson,ls7a-pwm.yaml       |  66 ++++
 MAINTAINERS                                   |   7 +
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-loongson.c                    | 300 ++++++++++++++++++
 5 files changed, 384 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
 create mode 100644 drivers/pwm/pwm-loongson.c

-- 
2.43.0

[PATCH v2 1/2] dt-bindings: pwm: Add Loongson PWM controller

[Binbin Zhou]

Add Loongson PWM controller binding with DT schema format using
json-schema.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../bindings/pwm/loongson,ls7a-pwm.yaml       | 66 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml b/Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
new file mode 100644
index 000000000000..d7b6a34a8e09
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/loongson,ls7a-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson PWM Controller
+
+maintainers:
+  - Binbin Zhou <zhoubinbin@loongson.cn>
+
+description:
+  The Loongson PWM has one pulse width output signal and one pulse input
+  signal to be measured.
+  It can be found on Loongson-2K series cpus and Loongson LS7A bridge chips.
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - const: loongson,ls7a-pwm
+      - items:
+          - enum:
+              - loongson,ls2k0500-pwm
+              - loongson,ls2k1000-pwm
+              - loongson,ls2k2000-pwm
+          - const: loongson,ls7a-pwm
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  '#pwm-cells':
+    description:
+      The first cell must have a value of 0, which specifies the PWM output signal;
+      The second cell is the period in nanoseconds;
+      The third cell flag supported by this binding is PWM_POLARITY_INVERTED.
+    const: 3
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/loongson,ls2k-clk.h>
+    pwm@1fe22000 {
+        compatible = "loongson,ls2k1000-pwm", "loongson,ls7a-pwm";
+        reg = <0x1fe22000 0x10>;
+        interrupt-parent = <&liointc0>;
+        interrupts = <24 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clk LOONGSON2_APB_CLK>;
+        #pwm-cells = <3>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index aea47e04c3a5..cd0d4e2d02ff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12757,6 +12757,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
 F:	drivers/i2c/busses/i2c-ls2x.c
 
+LOONGSON PWM DRIVER
+M:	Binbin Zhou <zhoubinbin@loongson.cn>
+L:	linux-pwm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
+
 LOONGSON-2 SOC SERIES CLOCK DRIVER
 M:	Yinbo Zhu <zhuyinbo@loongson.cn>
 L:	linux-clk@vger.kernel.org
-- 
2.43.0

Re: [PATCH v2 1/2] dt-bindings: pwm: Add Loongson PWM controller

[Krzysztof Kozlowski]

On 11/04/2024 11:16, Binbin Zhou wrote:
> Add Loongson PWM controller binding with DT schema format using
> json-schema.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>


> +properties:
> +  compatible:
> +    oneOf:
> +      - const: loongson,ls7a-pwm
> +      - items:
> +          - enum:
> +              - loongson,ls2k0500-pwm
> +              - loongson,ls2k1000-pwm
> +              - loongson,ls2k2000-pwm
> +          - const: loongson,ls7a-pwm
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  '#pwm-cells':
> +    description:
> +      The first cell must have a value of 0, which specifies the PWM output signal;

If you have always the same value in PWM phandle, why encoding it in the
phandle in the first place? What's the benefit of passing 0?

> +      The second cell is the period in nanoseconds;
> +      The third cell flag supported by this binding is PWM_POLARITY_INVERTED.
> +    const: 3
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +additionalProperties: false
> +


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: pwm: Add Loongson PWM controller

[Binbin Zhou]

On Thu, Apr 11, 2024 at 4:26 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/04/2024 11:16, Binbin Zhou wrote:
> > Add Loongson PWM controller binding with DT schema format using
> > json-schema.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
>
>
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - const: loongson,ls7a-pwm
> > +      - items:
> > +          - enum:
> > +              - loongson,ls2k0500-pwm
> > +              - loongson,ls2k1000-pwm
> > +              - loongson,ls2k2000-pwm
> > +          - const: loongson,ls7a-pwm
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  '#pwm-cells':
> > +    description:
> > +      The first cell must have a value of 0, which specifies the PWM output signal;
>
> If you have always the same value in PWM phandle, why encoding it in the
> phandle in the first place? What's the benefit of passing 0?

Hi Krzysztof:

My thoughts are:
First of all, our pwm has only one output signal, so it can only be 0.
Also, as you know from the pwm xlate function, the first cell is the
pwm index, so I fixed it to be 0 here.

The xlate function:
https://elixir.bootlin.com/linux/v6.8/source/drivers/pwm/core.c#L106

Thanks.
Binbin
>
> > +      The second cell is the period in nanoseconds;
> > +      The third cell flag supported by this binding is PWM_POLARITY_INVERTED.
> > +    const: 3
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +
> > +additionalProperties: false
> > +
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 1/2] dt-bindings: pwm: Add Loongson PWM controller

[Krzysztof Kozlowski]

On 11/04/2024 13:01, Binbin Zhou wrote:
> On Thu, Apr 11, 2024 at 4:26 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/04/2024 11:16, Binbin Zhou wrote:
>>> Add Loongson PWM controller binding with DT schema format using
>>> json-schema.
>>>
>>> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
>>
>>
>>> +properties:
>>> +  compatible:
>>> +    oneOf:
>>> +      - const: loongson,ls7a-pwm
>>> +      - items:
>>> +          - enum:
>>> +              - loongson,ls2k0500-pwm
>>> +              - loongson,ls2k1000-pwm
>>> +              - loongson,ls2k2000-pwm
>>> +          - const: loongson,ls7a-pwm
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  '#pwm-cells':
>>> +    description:
>>> +      The first cell must have a value of 0, which specifies the PWM output signal;
>>
>> If you have always the same value in PWM phandle, why encoding it in the
>> phandle in the first place? What's the benefit of passing 0?
> 
> Hi Krzysztof:
> 
> My thoughts are:
> First of all, our pwm has only one output signal, so it can only be 0.
> Also, as you know from the pwm xlate function, the first cell is the
> pwm index, so I fixed it to be 0 here.
> 
> The xlate function:
> https://elixir.bootlin.com/linux/v6.8/source/drivers/pwm/core.c#L106

You refer for xlate for PWM with three cells. You do not have three
cells, as you have only on signal, so why insisting on using other
xlate? Do you do the same for clocks? Or resets?

I don't think you use appropriate argument in this discussion. We talk
about hardware and your argument "I don't want to use my own xlate in
the driver" is about driver.


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: pwm: Add Loongson PWM controller

[Binbin Zhou]

On Thu, Apr 11, 2024 at 5:07 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/04/2024 13:01, Binbin Zhou wrote:
> > On Thu, Apr 11, 2024 at 4:26 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 11/04/2024 11:16, Binbin Zhou wrote:
> >>> Add Loongson PWM controller binding with DT schema format using
> >>> json-schema.
> >>>
> >>> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> >>
> >>
> >>> +properties:
> >>> +  compatible:
> >>> +    oneOf:
> >>> +      - const: loongson,ls7a-pwm
> >>> +      - items:
> >>> +          - enum:
> >>> +              - loongson,ls2k0500-pwm
> >>> +              - loongson,ls2k1000-pwm
> >>> +              - loongson,ls2k2000-pwm
> >>> +          - const: loongson,ls7a-pwm
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  clocks:
> >>> +    maxItems: 1
> >>> +
> >>> +  '#pwm-cells':
> >>> +    description:
> >>> +      The first cell must have a value of 0, which specifies the PWM output signal;
> >>
> >> If you have always the same value in PWM phandle, why encoding it in the
> >> phandle in the first place? What's the benefit of passing 0?
> >
> > Hi Krzysztof:
> >
> > My thoughts are:
> > First of all, our pwm has only one output signal, so it can only be 0.
> > Also, as you know from the pwm xlate function, the first cell is the
> > pwm index, so I fixed it to be 0 here.
> >
> > The xlate function:
> > https://elixir.bootlin.com/linux/v6.8/source/drivers/pwm/core.c#L106
>
> You refer for xlate for PWM with three cells. You do not have three
> cells, as you have only on signal, so why insisting on using other
> xlate? Do you do the same for clocks? Or resets?
>
> I don't think you use appropriate argument in this discussion. We talk
> about hardware and your argument "I don't want to use my own xlate in
> the driver" is about driver.
>
Hi Krzysztof:

Thanks for your comments.

Emm... Indeed, I used to think about it from the driver's perspective.
From the binding perspective, two cells really should be more appropriate.
I try to make the following changes in the next version patchset:

  '#pwm-cells':
    description:
      The first cell is the period in nanoseconds;
      The second cell flag supported by this binding is PWM_POLARITY_INVERTED.
    const: 2

Accordingly, the custom xlate function will be used in the driver.

Thanks.
Binbin
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 1/2] dt-bindings: pwm: Add Loongson PWM controller

[Krzysztof Kozlowski]

On 11/04/2024 16:35, Binbin Zhou wrote:
> On Thu, Apr 11, 2024 at 5:07 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/04/2024 13:01, Binbin Zhou wrote:
>>> On Thu, Apr 11, 2024 at 4:26 PM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 11/04/2024 11:16, Binbin Zhou wrote:
>>>>> Add Loongson PWM controller binding with DT schema format using
>>>>> json-schema.
>>>>>
>>>>> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
>>>>
>>>>
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    oneOf:
>>>>> +      - const: loongson,ls7a-pwm
>>>>> +      - items:
>>>>> +          - enum:
>>>>> +              - loongson,ls2k0500-pwm
>>>>> +              - loongson,ls2k1000-pwm
>>>>> +              - loongson,ls2k2000-pwm
>>>>> +          - const: loongson,ls7a-pwm
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  interrupts:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clocks:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  '#pwm-cells':
>>>>> +    description:
>>>>> +      The first cell must have a value of 0, which specifies the PWM output signal;
>>>>
>>>> If you have always the same value in PWM phandle, why encoding it in the
>>>> phandle in the first place? What's the benefit of passing 0?
>>>
>>> Hi Krzysztof:
>>>
>>> My thoughts are:
>>> First of all, our pwm has only one output signal, so it can only be 0.
>>> Also, as you know from the pwm xlate function, the first cell is the
>>> pwm index, so I fixed it to be 0 here.
>>>
>>> The xlate function:
>>> https://elixir.bootlin.com/linux/v6.8/source/drivers/pwm/core.c#L106
>>
>> You refer for xlate for PWM with three cells. You do not have three
>> cells, as you have only on signal, so why insisting on using other
>> xlate? Do you do the same for clocks? Or resets?
>>
>> I don't think you use appropriate argument in this discussion. We talk
>> about hardware and your argument "I don't want to use my own xlate in
>> the driver" is about driver.
>>
> Hi Krzysztof:
> 
> Thanks for your comments.
> 
> Emm... Indeed, I used to think about it from the driver's perspective.
> From the binding perspective, two cells really should be more appropriate.
> I try to make the following changes in the next version patchset:
> 
>   '#pwm-cells':
>     description:
>       The first cell is the period in nanoseconds;
>       The second cell flag supported by this binding is PWM_POLARITY_INVERTED.
>     const: 2
> 
> Accordingly, the custom xlate function will be used in the driver.

If your other, upcoming variants had more PWM outputs, then I would find
reasonable keeping cells=3 to have one approach for all of them. But I
guess that's not the case here.


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: pwm: Add Loongson PWM controller

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4086 bytes --]

Hello,

On Thu, Apr 11, 2024 at 04:44:37PM +0200, Krzysztof Kozlowski wrote:
> On 11/04/2024 16:35, Binbin Zhou wrote:
> > On Thu, Apr 11, 2024 at 5:07 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 11/04/2024 13:01, Binbin Zhou wrote:
> >>> On Thu, Apr 11, 2024 at 4:26 PM Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@linaro.org> wrote:
> >>>>
> >>>> On 11/04/2024 11:16, Binbin Zhou wrote:
> >>>>> Add Loongson PWM controller binding with DT schema format using
> >>>>> json-schema.
> >>>>>
> >>>>> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> >>>>
> >>>>
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    oneOf:
> >>>>> +      - const: loongson,ls7a-pwm
> >>>>> +      - items:
> >>>>> +          - enum:
> >>>>> +              - loongson,ls2k0500-pwm
> >>>>> +              - loongson,ls2k1000-pwm
> >>>>> +              - loongson,ls2k2000-pwm
> >>>>> +          - const: loongson,ls7a-pwm
> >>>>> +
> >>>>> +  reg:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  interrupts:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  clocks:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  '#pwm-cells':
> >>>>> +    description:
> >>>>> +      The first cell must have a value of 0, which specifies the PWM output signal;
> >>>>
> >>>> If you have always the same value in PWM phandle, why encoding it in the
> >>>> phandle in the first place? What's the benefit of passing 0?
> >>>
> >>> Hi Krzysztof:
> >>>
> >>> My thoughts are:
> >>> First of all, our pwm has only one output signal, so it can only be 0.
> >>> Also, as you know from the pwm xlate function, the first cell is the
> >>> pwm index, so I fixed it to be 0 here.
> >>>
> >>> The xlate function:
> >>> https://elixir.bootlin.com/linux/v6.8/source/drivers/pwm/core.c#L106
> >>
> >> You refer for xlate for PWM with three cells. You do not have three
> >> cells, as you have only on signal, so why insisting on using other
> >> xlate? Do you do the same for clocks? Or resets?
> >>
> >> I don't think you use appropriate argument in this discussion. We talk
> >> about hardware and your argument "I don't want to use my own xlate in
> >> the driver" is about driver.
> >>
> > Hi Krzysztof:
> > 
> > Thanks for your comments.
> > 
> > Emm... Indeed, I used to think about it from the driver's perspective.
> > From the binding perspective, two cells really should be more appropriate.
> > I try to make the following changes in the next version patchset:
> > 
> >   '#pwm-cells':
> >     description:
> >       The first cell is the period in nanoseconds;
> >       The second cell flag supported by this binding is PWM_POLARITY_INVERTED.
> >     const: 2
> > 
> > Accordingly, the custom xlate function will be used in the driver.
> 
> If your other, upcoming variants had more PWM outputs, then I would find
> reasonable keeping cells=3 to have one approach for all of them. But I
> guess that's not the case here.

There is an easy way to get rid of the 0, just use chip->of_xlate =
of_pwm_single_xlate; Having said that, I don't particularily like that.
If it wasn't for dt bindings being stable I'd argue for all PWM chips to
use #pwm-cells = <3>; and accept that the 2nd field is zero for
consistency.

Some statistics:

There are only two drivers that use of_pwm_single_xlate:
 - drivers/pwm/pwm-pxa.c
 - drivers/gpu/drm/bridge/ti-sn65dsi86.c

There is one driver that uses a completely custom xlate function (and
#pwm-cells = <1>, to specify the pwmid; the period is fixed):
 - drivers/pwm/pwm-cros-ec.c

All 65 other drivers use #pwm-cells = <3> with the "usual" semantic. At
least 21 among them only have a single output and so always use 0 in the
2nd cell.

So I'm all in favour to stick to the approach used in this binding for
the sake of consistency among the drivers^Wbindings.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] dt-bindings: pwm: Add Loongson PWM controller

[Krzysztof Kozlowski]

On 11/04/2024 17:21, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Apr 11, 2024 at 04:44:37PM +0200, Krzysztof Kozlowski wrote:
>> On 11/04/2024 16:35, Binbin Zhou wrote:
>>> On Thu, Apr 11, 2024 at 5:07 PM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 11/04/2024 13:01, Binbin Zhou wrote:
>>>>> On Thu, Apr 11, 2024 at 4:26 PM Krzysztof Kozlowski
>>>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>>>
>>>>>> On 11/04/2024 11:16, Binbin Zhou wrote:
>>>>>>> Add Loongson PWM controller binding with DT schema format using
>>>>>>> json-schema.
>>>>>>>
>>>>>>> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
>>>>>>
>>>>>>
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    oneOf:
>>>>>>> +      - const: loongson,ls7a-pwm
>>>>>>> +      - items:
>>>>>>> +          - enum:
>>>>>>> +              - loongson,ls2k0500-pwm
>>>>>>> +              - loongson,ls2k1000-pwm
>>>>>>> +              - loongson,ls2k2000-pwm
>>>>>>> +          - const: loongson,ls7a-pwm
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  interrupts:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  clocks:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  '#pwm-cells':
>>>>>>> +    description:
>>>>>>> +      The first cell must have a value of 0, which specifies the PWM output signal;
>>>>>>
>>>>>> If you have always the same value in PWM phandle, why encoding it in the
>>>>>> phandle in the first place? What's the benefit of passing 0?
>>>>>
>>>>> Hi Krzysztof:
>>>>>
>>>>> My thoughts are:
>>>>> First of all, our pwm has only one output signal, so it can only be 0.
>>>>> Also, as you know from the pwm xlate function, the first cell is the
>>>>> pwm index, so I fixed it to be 0 here.
>>>>>
>>>>> The xlate function:
>>>>> https://elixir.bootlin.com/linux/v6.8/source/drivers/pwm/core.c#L106
>>>>
>>>> You refer for xlate for PWM with three cells. You do not have three
>>>> cells, as you have only on signal, so why insisting on using other
>>>> xlate? Do you do the same for clocks? Or resets?
>>>>
>>>> I don't think you use appropriate argument in this discussion. We talk
>>>> about hardware and your argument "I don't want to use my own xlate in
>>>> the driver" is about driver.
>>>>
>>> Hi Krzysztof:
>>>
>>> Thanks for your comments.
>>>
>>> Emm... Indeed, I used to think about it from the driver's perspective.
>>> From the binding perspective, two cells really should be more appropriate.
>>> I try to make the following changes in the next version patchset:
>>>
>>>   '#pwm-cells':
>>>     description:
>>>       The first cell is the period in nanoseconds;
>>>       The second cell flag supported by this binding is PWM_POLARITY_INVERTED.
>>>     const: 2
>>>
>>> Accordingly, the custom xlate function will be used in the driver.
>>
>> If your other, upcoming variants had more PWM outputs, then I would find
>> reasonable keeping cells=3 to have one approach for all of them. But I
>> guess that's not the case here.
> 
> There is an easy way to get rid of the 0, just use chip->of_xlate =
> of_pwm_single_xlate; Having said that, I don't particularily like that.
> If it wasn't for dt bindings being stable I'd argue for all PWM chips to
> use #pwm-cells = <3>; and accept that the 2nd field is zero for
> consistency.
> 
> Some statistics:
> 
> There are only two drivers that use of_pwm_single_xlate:
>  - drivers/pwm/pwm-pxa.c
>  - drivers/gpu/drm/bridge/ti-sn65dsi86.c
> 
> There is one driver that uses a completely custom xlate function (and
> #pwm-cells = <1>, to specify the pwmid; the period is fixed):
>  - drivers/pwm/pwm-cros-ec.c
> 
> All 65 other drivers use #pwm-cells = <3> with the "usual" semantic. At
> least 21 among them only have a single output and so always use 0 in the
> 2nd cell.
> 
> So I'm all in favour to stick to the approach used in this binding for
> the sake of consistency among the drivers^Wbindings.

Seems fine, thanks for the explanation.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: pwm: Add Loongson PWM controller

[Krzysztof Kozlowski]

On 11/04/2024 11:16, Binbin Zhou wrote:
> Add Loongson PWM controller binding with DT schema format using
> json-schema.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 2/2] pwm: Add Loongson PWM controller support

[Binbin Zhou]

This commit adds a generic PWM framework driver for the PWM controller
found on Loongson family chips.

Co-developed-by: Juxin Gao <gaojuxin@loongson.cn>
Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 MAINTAINERS                |   1 +
 drivers/pwm/Kconfig        |  10 ++
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-loongson.c | 300 +++++++++++++++++++++++++++++++++++++
 4 files changed, 312 insertions(+)
 create mode 100644 drivers/pwm/pwm-loongson.c

diff --git a/MAINTAINERS b/MAINTAINERS
index cd0d4e2d02ff..fef232ae8cca 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12762,6 +12762,7 @@ M:	Binbin Zhou <zhoubinbin@loongson.cn>
 L:	linux-pwm@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
+F:	drivers/pwm/pwm-loongson.c
 
 LOONGSON-2 SOC SERIES CLOCK DRIVER
 M:	Yinbo Zhu <zhuyinbo@loongson.cn>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..bb163c65e5ae 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -324,6 +324,16 @@ config PWM_KEEMBAY
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-keembay.
 
+config PWM_LOONGSON
+	tristate "Loongson PWM support"
+	depends on MACH_LOONGSON64
+	help
+	  Generic PWM framework driver for Loongson family.
+	  It can be found on Loongson-2K series cpu and Loongson LS7A bridge chips.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-loongson.
+
 config PWM_LP3943
 	tristate "TI/National Semiconductor LP3943 PWM support"
 	depends on MFD_LP3943
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c5ec9e168ee7..bffa49500277 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
 obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
+obj-$(CONFIG_PWM_LOONGSON)	+= pwm-loongson.o
 obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
 obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
new file mode 100644
index 000000000000..0afae42113a5
--- /dev/null
+++ b/drivers/pwm/pwm-loongson.c
@@ -0,0 +1,300 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Loongson PWM driver
+ *
+ * Author: Juxin Gao <gaojuxin@loongson.cn>
+ * Further cleanup and restructuring by:
+ *         Binbin Zhou <zhoubinbin@loongson.cn>
+ *
+ * Copyright (C) 2017-2024 Loongson Technology Corporation Limited.
+ */
+
+#include <linux/acpi.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/units.h>
+
+/* Loongson PWM registers */
+#define PWM_DUTY	0x4 /* Low Pulse Buffer Register */
+#define PWM_PERIOD	0x8 /* Pulse Period Buffer Register */
+#define PWM_CTRL	0xc /* Control Register */
+
+/* Control register bits */
+#define PWM_CTRL_EN	BIT(0)  /* Counter Enable Bit */
+#define PWM_CTRL_OE	BIT(3)  /* Pulse Output Enable Control Bit, Valid Low */
+#define PWM_CTRL_SINGLE	BIT(4)  /* Single Pulse Control Bit */
+#define PWM_CTRL_INTE	BIT(5)  /* Interrupt Enable Bit */
+#define PWM_CTRL_INT	BIT(6)  /* Interrupt Bit */
+#define PWM_CTRL_RST	BIT(7)  /* Counter Reset Bit */
+#define PWM_CTRL_CAPTE	BIT(8)  /* Measurement Pulse Enable Bit */
+#define PWM_CTRL_INVERT	BIT(9)  /* Output flip-flop Enable Bit */
+#define PWM_CTRL_DZONE	BIT(10) /* Anti-dead Zone Enable Bit */
+
+#define PWM_FREQ_STD       (50 * HZ_PER_KHZ)
+
+struct pwm_loongson_ddata {
+	struct pwm_chip	chip;
+	struct clk	*clk;
+	void __iomem	*base;
+	/* The following for PM */
+	u32		ctrl;
+	u32		duty;
+	u32		period;
+};
+
+static inline struct pwm_loongson_ddata *to_pwm_loongson_ddata(struct pwm_chip *chip)
+{
+	return container_of(chip, struct pwm_loongson_ddata, chip);
+}
+
+static inline u32 pwm_loongson_readl(struct pwm_loongson_ddata *ddata, u64 offset)
+{
+	return readl(ddata->base + offset);
+}
+
+static inline void pwm_loongson_writel(struct pwm_loongson_ddata *ddata,
+				       u32 val, u64 offset)
+{
+	writel(val, ddata->base + offset);
+}
+
+static int pwm_loongson_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+				     enum pwm_polarity polarity)
+{
+	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
+	u16 val;
+
+	val = pwm_loongson_readl(ddata, PWM_CTRL);
+
+	if (polarity == PWM_POLARITY_INVERSED)
+		/* Duty cycle defines LOW period of PWM */
+		val |= PWM_CTRL_INVERT;
+	else
+		/* Duty cycle defines HIGH period of PWM */
+		val &= ~PWM_CTRL_INVERT;
+
+	pwm_loongson_writel(ddata, val, PWM_CTRL);
+	return 0;
+}
+
+static void pwm_loongson_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
+	u32 val;
+
+	if (pwm->state.polarity == PWM_POLARITY_NORMAL)
+		pwm_loongson_writel(ddata, ddata->period, PWM_DUTY);
+	else if (pwm->state.polarity == PWM_POLARITY_INVERSED)
+		pwm_loongson_writel(ddata, 0, PWM_DUTY);
+
+	val = pwm_loongson_readl(ddata, PWM_CTRL);
+	val &= ~PWM_CTRL_EN;
+	pwm_loongson_writel(ddata, val, PWM_CTRL);
+}
+
+static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
+	u32 val;
+
+	pwm_loongson_writel(ddata, ddata->duty, PWM_DUTY);
+	pwm_loongson_writel(ddata, ddata->period, PWM_PERIOD);
+
+	val = pwm_loongson_readl(ddata, PWM_CTRL);
+	val |= PWM_CTRL_EN;
+	pwm_loongson_writel(ddata, val, PWM_CTRL);
+
+	return 0;
+}
+
+static u32 pwm_loongson_set_config(struct pwm_loongson_ddata *ddata, int ns,
+				   u64 clk_rate, u64 offset)
+{
+	u32 val;
+	u64 c;
+
+	c = clk_rate * ns;
+	do_div(c, NSEC_PER_SEC);
+	val = c < 1 ? 1 : c;
+
+	pwm_loongson_writel(ddata, val, offset);
+
+	return val;
+}
+
+static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			       int duty_ns, int period_ns)
+{
+	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
+	struct device *dev = chip->dev;
+	u64 clk_rate;
+
+	if (period_ns > NANOHZ_PER_HZ || duty_ns > NANOHZ_PER_HZ)
+		return -ERANGE;
+
+	clk_rate = has_acpi_companion(dev) ? PWM_FREQ_STD
+		 : clk_get_rate(ddata->clk);
+
+	ddata->period = pwm_loongson_set_config(ddata, period_ns,
+						clk_rate, PWM_PERIOD);
+	ddata->duty = pwm_loongson_set_config(ddata, duty_ns, clk_rate, PWM_DUTY);
+
+	return 0;
+}
+
+static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			      const struct pwm_state *state)
+{
+	int err;
+	bool enabled = pwm->state.enabled;
+
+	if (state->polarity != pwm->state.polarity) {
+		if (enabled) {
+			pwm_loongson_disable(chip, pwm);
+			enabled = false;
+		}
+
+		err = pwm_loongson_set_polarity(chip, pwm, state->polarity);
+		if (err)
+			return err;
+	}
+
+	if (!state->enabled) {
+		if (enabled)
+			pwm_loongson_disable(chip, pwm);
+		return 0;
+	}
+
+	err = pwm_loongson_config(chip, pwm, state->duty_cycle, state->period);
+	if (err)
+		return err;
+
+	if (!enabled)
+		err = pwm_loongson_enable(chip, pwm);
+
+	return err;
+}
+
+static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
+	u32 period, duty, ctrl;
+	u64 ns;
+
+	period = pwm_loongson_readl(ddata, PWM_PERIOD);
+	ns = period * NSEC_PER_SEC;
+	state->period = do_div(ns, period);
+
+	duty = pwm_loongson_readl(ddata, PWM_DUTY);
+	ns = duty * NSEC_PER_SEC;
+	state->duty_cycle = do_div(ns, duty);
+
+	ctrl = pwm_loongson_readl(ddata, PWM_CTRL);
+	state->polarity = (ctrl & PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED
+			: PWM_POLARITY_NORMAL;
+	state->enabled = (ctrl & PWM_CTRL_EN) ? true : false;
+
+	ddata->ctrl = ctrl;
+	ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
+	ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
+
+	return 0;
+}
+
+static const struct pwm_ops pwm_loongson_ops = {
+	.apply = pwm_loongson_apply,
+	.get_state = pwm_loongson_get_state,
+};
+
+static int pwm_loongson_probe(struct platform_device *pdev)
+{
+	struct pwm_loongson_ddata *ddata;
+	struct device *dev = &pdev->dev;
+
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	ddata->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ddata->base))
+		return PTR_ERR(ddata->base);
+
+	if (!has_acpi_companion(dev)) {
+		ddata->clk = devm_clk_get_enabled(dev, NULL);
+		if (IS_ERR(ddata->clk))
+			return PTR_ERR(ddata->clk);
+	}
+
+	ddata->chip.dev = dev;
+	ddata->chip.ops = &pwm_loongson_ops;
+	ddata->chip.npwm = 1;
+	platform_set_drvdata(pdev, ddata);
+
+	return devm_pwmchip_add(dev, &ddata->chip);
+}
+
+static int pwm_loongson_suspend(struct device *dev)
+{
+	struct pwm_loongson_ddata *ddata = dev_get_drvdata(dev);
+
+	ddata->ctrl = pwm_loongson_readl(ddata, PWM_CTRL);
+	ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
+	ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
+
+	clk_disable_unprepare(ddata->clk);
+
+	return 0;
+}
+
+static int pwm_loongson_resume(struct device *dev)
+{
+	struct pwm_loongson_ddata *ddata = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(ddata->clk);
+	if (ret)
+		return ret;
+
+	pwm_loongson_writel(ddata, ddata->ctrl, PWM_CTRL);
+	pwm_loongson_writel(ddata, ddata->duty, PWM_DUTY);
+	pwm_loongson_writel(ddata, ddata->period, PWM_PERIOD);
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(pwm_loongson_pm_ops, pwm_loongson_suspend,
+				pwm_loongson_resume);
+
+static const struct acpi_device_id pwm_loongson_acpi_ids[] = {
+	{ "LOON0006" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, pwm_loongson_acpi_ids);
+
+static const struct of_device_id pwm_loongson_of_ids[] = {
+	{ .compatible = "loongson,ls7a-pwm" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, pwm_loongson_of_ids);
+
+static struct platform_driver pwm_loongson_driver = {
+	.probe  = pwm_loongson_probe,
+	.driver = {
+		.name   = "loongson-pwm",
+		.pm	= pm_ptr(&pwm_loongson_pm_ops),
+		.of_match_table   = pwm_loongson_of_ids,
+		.acpi_match_table = pwm_loongson_acpi_ids,
+	},
+};
+module_platform_driver(pwm_loongson_driver);
+
+MODULE_DESCRIPTION("Loongson PWM driver");
+MODULE_AUTHOR("Loongson Technology Corporation Limited.");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v2 2/2] pwm: Add Loongson PWM controller support

[Huacai Chen]

Hi, Binbin,

On Thu, Apr 11, 2024 at 5:16 PM Binbin Zhou <zhoubinbin@loongson.cn> wrote:
>
> This commit adds a generic PWM framework driver for the PWM controller
> found on Loongson family chips.
>
> Co-developed-by: Juxin Gao <gaojuxin@loongson.cn>
> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  MAINTAINERS                |   1 +
>  drivers/pwm/Kconfig        |  10 ++
>  drivers/pwm/Makefile       |   1 +
>  drivers/pwm/pwm-loongson.c | 300 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 312 insertions(+)
>  create mode 100644 drivers/pwm/pwm-loongson.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cd0d4e2d02ff..fef232ae8cca 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12762,6 +12762,7 @@ M:      Binbin Zhou <zhoubinbin@loongson.cn>
>  L:     linux-pwm@vger.kernel.org
>  S:     Maintained
>  F:     Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
> +F:     drivers/pwm/pwm-loongson.c
>
>  LOONGSON-2 SOC SERIES CLOCK DRIVER
>  M:     Yinbo Zhu <zhuyinbo@loongson.cn>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4b956d661755..bb163c65e5ae 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -324,6 +324,16 @@ config PWM_KEEMBAY
>           To compile this driver as a module, choose M here: the module
>           will be called pwm-keembay.
>
> +config PWM_LOONGSON
> +       tristate "Loongson PWM support"
> +       depends on MACH_LOONGSON64
> +       help
> +         Generic PWM framework driver for Loongson family.
> +         It can be found on Loongson-2K series cpu and Loongson LS7A bridge chips.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called pwm-loongson.
> +
>  config PWM_LP3943
>         tristate "TI/National Semiconductor LP3943 PWM support"
>         depends on MFD_LP3943
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c5ec9e168ee7..bffa49500277 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_PWM_INTEL_LGM)   += pwm-intel-lgm.o
>  obj-$(CONFIG_PWM_IQS620A)      += pwm-iqs620a.o
>  obj-$(CONFIG_PWM_JZ4740)       += pwm-jz4740.o
>  obj-$(CONFIG_PWM_KEEMBAY)      += pwm-keembay.o
> +obj-$(CONFIG_PWM_LOONGSON)     += pwm-loongson.o
>  obj-$(CONFIG_PWM_LP3943)       += pwm-lp3943.o
>  obj-$(CONFIG_PWM_LPC18XX_SCT)  += pwm-lpc18xx-sct.o
>  obj-$(CONFIG_PWM_LPC32XX)      += pwm-lpc32xx.o
> diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> new file mode 100644
> index 000000000000..0afae42113a5
> --- /dev/null
> +++ b/drivers/pwm/pwm-loongson.c
> @@ -0,0 +1,300 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Loongson PWM driver
> + *
> + * Author: Juxin Gao <gaojuxin@loongson.cn>
> + * Further cleanup and restructuring by:
> + *         Binbin Zhou <zhoubinbin@loongson.cn>
> + *
> + * Copyright (C) 2017-2024 Loongson Technology Corporation Limited.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/units.h>
> +
> +/* Loongson PWM registers */
> +#define PWM_DUTY       0x4 /* Low Pulse Buffer Register */
> +#define PWM_PERIOD     0x8 /* Pulse Period Buffer Register */
> +#define PWM_CTRL       0xc /* Control Register */
> +
> +/* Control register bits */
> +#define PWM_CTRL_EN    BIT(0)  /* Counter Enable Bit */
> +#define PWM_CTRL_OE    BIT(3)  /* Pulse Output Enable Control Bit, Valid Low */
> +#define PWM_CTRL_SINGLE        BIT(4)  /* Single Pulse Control Bit */
> +#define PWM_CTRL_INTE  BIT(5)  /* Interrupt Enable Bit */
> +#define PWM_CTRL_INT   BIT(6)  /* Interrupt Bit */
> +#define PWM_CTRL_RST   BIT(7)  /* Counter Reset Bit */
> +#define PWM_CTRL_CAPTE BIT(8)  /* Measurement Pulse Enable Bit */
> +#define PWM_CTRL_INVERT        BIT(9)  /* Output flip-flop Enable Bit */
> +#define PWM_CTRL_DZONE BIT(10) /* Anti-dead Zone Enable Bit */
> +
> +#define PWM_FREQ_STD       (50 * HZ_PER_KHZ)
> +
> +struct pwm_loongson_ddata {
> +       struct pwm_chip chip;
> +       struct clk      *clk;
> +       void __iomem    *base;
> +       /* The following for PM */
> +       u32             ctrl;
> +       u32             duty;
> +       u32             period;
> +};
> +
> +static inline struct pwm_loongson_ddata *to_pwm_loongson_ddata(struct pwm_chip *chip)
> +{
> +       return container_of(chip, struct pwm_loongson_ddata, chip);
> +}
> +
> +static inline u32 pwm_loongson_readl(struct pwm_loongson_ddata *ddata, u64 offset)
> +{
> +       return readl(ddata->base + offset);
> +}
> +
> +static inline void pwm_loongson_writel(struct pwm_loongson_ddata *ddata,
> +                                      u32 val, u64 offset)
> +{
> +       writel(val, ddata->base + offset);
> +}
> +
> +static int pwm_loongson_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +                                    enum pwm_polarity polarity)
> +{
> +       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> +       u16 val;
> +
> +       val = pwm_loongson_readl(ddata, PWM_CTRL);
> +
> +       if (polarity == PWM_POLARITY_INVERSED)
> +               /* Duty cycle defines LOW period of PWM */
> +               val |= PWM_CTRL_INVERT;
> +       else
> +               /* Duty cycle defines HIGH period of PWM */
> +               val &= ~PWM_CTRL_INVERT;
> +
> +       pwm_loongson_writel(ddata, val, PWM_CTRL);
A new line is needed here.

> +       return 0;
> +}
> +
> +static void pwm_loongson_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> +       u32 val;
> +
> +       if (pwm->state.polarity == PWM_POLARITY_NORMAL)
> +               pwm_loongson_writel(ddata, ddata->period, PWM_DUTY);
> +       else if (pwm->state.polarity == PWM_POLARITY_INVERSED)
> +               pwm_loongson_writel(ddata, 0, PWM_DUTY);
> +
> +       val = pwm_loongson_readl(ddata, PWM_CTRL);
> +       val &= ~PWM_CTRL_EN;
> +       pwm_loongson_writel(ddata, val, PWM_CTRL);
> +}
> +
> +static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> +       u32 val;
> +
> +       pwm_loongson_writel(ddata, ddata->duty, PWM_DUTY);
> +       pwm_loongson_writel(ddata, ddata->period, PWM_PERIOD);
> +
> +       val = pwm_loongson_readl(ddata, PWM_CTRL);
> +       val |= PWM_CTRL_EN;
> +       pwm_loongson_writel(ddata, val, PWM_CTRL);
> +
> +       return 0;
> +}
> +
> +static u32 pwm_loongson_set_config(struct pwm_loongson_ddata *ddata, int ns,
> +                                  u64 clk_rate, u64 offset)
> +{
> +       u32 val;
> +       u64 c;
> +
> +       c = clk_rate * ns;
> +       do_div(c, NSEC_PER_SEC);
> +       val = c < 1 ? 1 : c;
> +
> +       pwm_loongson_writel(ddata, val, offset);
> +
> +       return val;
> +}
> +
> +static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +                              int duty_ns, int period_ns)
> +{
> +       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> +       struct device *dev = chip->dev;
> +       u64 clk_rate;
> +
> +       if (period_ns > NANOHZ_PER_HZ || duty_ns > NANOHZ_PER_HZ)
> +               return -ERANGE;
> +
> +       clk_rate = has_acpi_companion(dev) ? PWM_FREQ_STD
> +                : clk_get_rate(ddata->clk);
Long lines are acceptable because there is no 80 columns restriction now.

> +
> +       ddata->period = pwm_loongson_set_config(ddata, period_ns,
> +                                               clk_rate, PWM_PERIOD);
The same here. And moving the "period" sentence after "duty" is a
little better because the order will be the same as in
pwm_loongson_ddata.

> +       ddata->duty = pwm_loongson_set_config(ddata, duty_ns, clk_rate, PWM_DUTY);
> +
> +       return 0;
> +}
> +
> +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +                             const struct pwm_state *state)
> +{
> +       int err;
> +       bool enabled = pwm->state.enabled;
> +
> +       if (state->polarity != pwm->state.polarity) {
> +               if (enabled) {
> +                       pwm_loongson_disable(chip, pwm);
> +                       enabled = false;
> +               }
> +
> +               err = pwm_loongson_set_polarity(chip, pwm, state->polarity);
> +               if (err)
> +                       return err;
> +       }
> +
> +       if (!state->enabled) {
> +               if (enabled)
> +                       pwm_loongson_disable(chip, pwm);
> +               return 0;
> +       }
> +
> +       err = pwm_loongson_config(chip, pwm, state->duty_cycle, state->period);
> +       if (err)
> +               return err;
> +
> +       if (!enabled)
> +               err = pwm_loongson_enable(chip, pwm);
> +
> +       return err;
> +}
> +
> +static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +                                 struct pwm_state *state)
> +{
> +       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> +       u32 period, duty, ctrl;
> +       u64 ns;
> +
> +       period = pwm_loongson_readl(ddata, PWM_PERIOD);
> +       ns = period * NSEC_PER_SEC;
> +       state->period = do_div(ns, period);
> +
> +       duty = pwm_loongson_readl(ddata, PWM_DUTY);
> +       ns = duty * NSEC_PER_SEC;
> +       state->duty_cycle = do_div(ns, duty);
> +
> +       ctrl = pwm_loongson_readl(ddata, PWM_CTRL);
> +       state->polarity = (ctrl & PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED
> +                       : PWM_POLARITY_NORMAL;
> +       state->enabled = (ctrl & PWM_CTRL_EN) ? true : false;
> +
> +       ddata->ctrl = ctrl;
> +       ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
> +       ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
> +
> +       return 0;
> +}
> +
> +static const struct pwm_ops pwm_loongson_ops = {
> +       .apply = pwm_loongson_apply,
> +       .get_state = pwm_loongson_get_state,
> +};
> +
> +static int pwm_loongson_probe(struct platform_device *pdev)
> +{
> +       struct pwm_loongson_ddata *ddata;
> +       struct device *dev = &pdev->dev;
> +
> +       ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> +       if (!ddata)
> +               return -ENOMEM;
> +
> +       ddata->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(ddata->base))
> +               return PTR_ERR(ddata->base);
> +
> +       if (!has_acpi_companion(dev)) {
> +               ddata->clk = devm_clk_get_enabled(dev, NULL);
> +               if (IS_ERR(ddata->clk))
> +                       return PTR_ERR(ddata->clk);
> +       }
> +
> +       ddata->chip.dev = dev;
> +       ddata->chip.ops = &pwm_loongson_ops;
> +       ddata->chip.npwm = 1;
> +       platform_set_drvdata(pdev, ddata);
> +
> +       return devm_pwmchip_add(dev, &ddata->chip);
> +}
> +
> +static int pwm_loongson_suspend(struct device *dev)
> +{
> +       struct pwm_loongson_ddata *ddata = dev_get_drvdata(dev);
> +
> +       ddata->ctrl = pwm_loongson_readl(ddata, PWM_CTRL);
> +       ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
> +       ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
> +
> +       clk_disable_unprepare(ddata->clk);
> +
> +       return 0;
> +}
> +
> +static int pwm_loongson_resume(struct device *dev)
> +{
> +       struct pwm_loongson_ddata *ddata = dev_get_drvdata(dev);
> +       int ret;
> +
> +       ret = clk_prepare_enable(ddata->clk);
> +       if (ret)
> +               return ret;
> +
> +       pwm_loongson_writel(ddata, ddata->ctrl, PWM_CTRL);
> +       pwm_loongson_writel(ddata, ddata->duty, PWM_DUTY);
> +       pwm_loongson_writel(ddata, ddata->period, PWM_PERIOD);
> +
> +       return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(pwm_loongson_pm_ops, pwm_loongson_suspend,
> +                               pwm_loongson_resume);
> +
> +static const struct acpi_device_id pwm_loongson_acpi_ids[] = {
> +       { "LOON0006" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(acpi, pwm_loongson_acpi_ids);
Moving the acpi table after the dt table is a little better because
the order will be the same as in pwm_loongson_driver.

And others look good to me.

Huacai

> +
> +static const struct of_device_id pwm_loongson_of_ids[] = {
> +       { .compatible = "loongson,ls7a-pwm" },
> +       { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, pwm_loongson_of_ids);
> +
> +static struct platform_driver pwm_loongson_driver = {
> +       .probe  = pwm_loongson_probe,
> +       .driver = {
> +               .name   = "loongson-pwm",
> +               .pm     = pm_ptr(&pwm_loongson_pm_ops),
> +               .of_match_table   = pwm_loongson_of_ids,
> +               .acpi_match_table = pwm_loongson_acpi_ids,
> +       },
> +};
> +module_platform_driver(pwm_loongson_driver);
> +
> +MODULE_DESCRIPTION("Loongson PWM driver");
> +MODULE_AUTHOR("Loongson Technology Corporation Limited.");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
>
>

Re: [PATCH v2 2/2] pwm: Add Loongson PWM controller support

[Binbin Zhou]

On Fri, Apr 12, 2024 at 12:47 PM Huacai Chen <chenhuacai@kernel.org> wrote:
>
> Hi, Binbin,
>
> On Thu, Apr 11, 2024 at 5:16 PM Binbin Zhou <zhoubinbin@loongson.cn> wrote:
> >
> > This commit adds a generic PWM framework driver for the PWM controller
> > found on Loongson family chips.
> >
> > Co-developed-by: Juxin Gao <gaojuxin@loongson.cn>
> > Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  MAINTAINERS                |   1 +
> >  drivers/pwm/Kconfig        |  10 ++
> >  drivers/pwm/Makefile       |   1 +
> >  drivers/pwm/pwm-loongson.c | 300 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 312 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-loongson.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index cd0d4e2d02ff..fef232ae8cca 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12762,6 +12762,7 @@ M:      Binbin Zhou <zhoubinbin@loongson.cn>
> >  L:     linux-pwm@vger.kernel.org
> >  S:     Maintained
> >  F:     Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
> > +F:     drivers/pwm/pwm-loongson.c
> >
> >  LOONGSON-2 SOC SERIES CLOCK DRIVER
> >  M:     Yinbo Zhu <zhuyinbo@loongson.cn>
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 4b956d661755..bb163c65e5ae 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -324,6 +324,16 @@ config PWM_KEEMBAY
> >           To compile this driver as a module, choose M here: the module
> >           will be called pwm-keembay.
> >
> > +config PWM_LOONGSON
> > +       tristate "Loongson PWM support"
> > +       depends on MACH_LOONGSON64
> > +       help
> > +         Generic PWM framework driver for Loongson family.
> > +         It can be found on Loongson-2K series cpu and Loongson LS7A bridge chips.
> > +
> > +         To compile this driver as a module, choose M here: the module
> > +         will be called pwm-loongson.
> > +
> >  config PWM_LP3943
> >         tristate "TI/National Semiconductor LP3943 PWM support"
> >         depends on MFD_LP3943
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index c5ec9e168ee7..bffa49500277 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -28,6 +28,7 @@ obj-$(CONFIG_PWM_INTEL_LGM)   += pwm-intel-lgm.o
> >  obj-$(CONFIG_PWM_IQS620A)      += pwm-iqs620a.o
> >  obj-$(CONFIG_PWM_JZ4740)       += pwm-jz4740.o
> >  obj-$(CONFIG_PWM_KEEMBAY)      += pwm-keembay.o
> > +obj-$(CONFIG_PWM_LOONGSON)     += pwm-loongson.o
> >  obj-$(CONFIG_PWM_LP3943)       += pwm-lp3943.o
> >  obj-$(CONFIG_PWM_LPC18XX_SCT)  += pwm-lpc18xx-sct.o
> >  obj-$(CONFIG_PWM_LPC32XX)      += pwm-lpc32xx.o
> > diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> > new file mode 100644
> > index 000000000000..0afae42113a5
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-loongson.c
> > @@ -0,0 +1,300 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Loongson PWM driver
> > + *
> > + * Author: Juxin Gao <gaojuxin@loongson.cn>
> > + * Further cleanup and restructuring by:
> > + *         Binbin Zhou <zhoubinbin@loongson.cn>
> > + *
> > + * Copyright (C) 2017-2024 Loongson Technology Corporation Limited.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/units.h>
> > +
> > +/* Loongson PWM registers */
> > +#define PWM_DUTY       0x4 /* Low Pulse Buffer Register */
> > +#define PWM_PERIOD     0x8 /* Pulse Period Buffer Register */
> > +#define PWM_CTRL       0xc /* Control Register */
> > +
> > +/* Control register bits */
> > +#define PWM_CTRL_EN    BIT(0)  /* Counter Enable Bit */
> > +#define PWM_CTRL_OE    BIT(3)  /* Pulse Output Enable Control Bit, Valid Low */
> > +#define PWM_CTRL_SINGLE        BIT(4)  /* Single Pulse Control Bit */
> > +#define PWM_CTRL_INTE  BIT(5)  /* Interrupt Enable Bit */
> > +#define PWM_CTRL_INT   BIT(6)  /* Interrupt Bit */
> > +#define PWM_CTRL_RST   BIT(7)  /* Counter Reset Bit */
> > +#define PWM_CTRL_CAPTE BIT(8)  /* Measurement Pulse Enable Bit */
> > +#define PWM_CTRL_INVERT        BIT(9)  /* Output flip-flop Enable Bit */
> > +#define PWM_CTRL_DZONE BIT(10) /* Anti-dead Zone Enable Bit */
> > +
> > +#define PWM_FREQ_STD       (50 * HZ_PER_KHZ)
> > +
> > +struct pwm_loongson_ddata {
> > +       struct pwm_chip chip;
> > +       struct clk      *clk;
> > +       void __iomem    *base;
> > +       /* The following for PM */
> > +       u32             ctrl;
> > +       u32             duty;
> > +       u32             period;
> > +};
> > +
> > +static inline struct pwm_loongson_ddata *to_pwm_loongson_ddata(struct pwm_chip *chip)
> > +{
> > +       return container_of(chip, struct pwm_loongson_ddata, chip);
> > +}
> > +
> > +static inline u32 pwm_loongson_readl(struct pwm_loongson_ddata *ddata, u64 offset)
> > +{
> > +       return readl(ddata->base + offset);
> > +}
> > +
> > +static inline void pwm_loongson_writel(struct pwm_loongson_ddata *ddata,
> > +                                      u32 val, u64 offset)
> > +{
> > +       writel(val, ddata->base + offset);
> > +}
> > +
> > +static int pwm_loongson_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                                    enum pwm_polarity polarity)
> > +{
> > +       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > +       u16 val;
> > +
> > +       val = pwm_loongson_readl(ddata, PWM_CTRL);
> > +
> > +       if (polarity == PWM_POLARITY_INVERSED)
> > +               /* Duty cycle defines LOW period of PWM */
> > +               val |= PWM_CTRL_INVERT;
> > +       else
> > +               /* Duty cycle defines HIGH period of PWM */
> > +               val &= ~PWM_CTRL_INVERT;
> > +
> > +       pwm_loongson_writel(ddata, val, PWM_CTRL);
> A new line is needed here.
>
OK...

> > +       return 0;
> > +}
> > +
> > +static void pwm_loongson_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > +       u32 val;
> > +
> > +       if (pwm->state.polarity == PWM_POLARITY_NORMAL)
> > +               pwm_loongson_writel(ddata, ddata->period, PWM_DUTY);
> > +       else if (pwm->state.polarity == PWM_POLARITY_INVERSED)
> > +               pwm_loongson_writel(ddata, 0, PWM_DUTY);
> > +
> > +       val = pwm_loongson_readl(ddata, PWM_CTRL);
> > +       val &= ~PWM_CTRL_EN;
> > +       pwm_loongson_writel(ddata, val, PWM_CTRL);
> > +}
> > +
> > +static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > +       u32 val;
> > +
> > +       pwm_loongson_writel(ddata, ddata->duty, PWM_DUTY);
> > +       pwm_loongson_writel(ddata, ddata->period, PWM_PERIOD);
> > +
> > +       val = pwm_loongson_readl(ddata, PWM_CTRL);
> > +       val |= PWM_CTRL_EN;
> > +       pwm_loongson_writel(ddata, val, PWM_CTRL);
> > +
> > +       return 0;
> > +}
> > +
> > +static u32 pwm_loongson_set_config(struct pwm_loongson_ddata *ddata, int ns,
> > +                                  u64 clk_rate, u64 offset)
> > +{
> > +       u32 val;
> > +       u64 c;
> > +
> > +       c = clk_rate * ns;
> > +       do_div(c, NSEC_PER_SEC);
> > +       val = c < 1 ? 1 : c;
> > +
> > +       pwm_loongson_writel(ddata, val, offset);
> > +
> > +       return val;
> > +}
> > +
> > +static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                              int duty_ns, int period_ns)
> > +{
> > +       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > +       struct device *dev = chip->dev;
> > +       u64 clk_rate;
> > +
> > +       if (period_ns > NANOHZ_PER_HZ || duty_ns > NANOHZ_PER_HZ)
> > +               return -ERANGE;
> > +
> > +       clk_rate = has_acpi_companion(dev) ? PWM_FREQ_STD
> > +                : clk_get_rate(ddata->clk);
> Long lines are acceptable because there is no 80 columns restriction now.
>
> > +
> > +       ddata->period = pwm_loongson_set_config(ddata, period_ns,
> > +                                               clk_rate, PWM_PERIOD);
> The same here. And moving the "period" sentence after "duty" is a
> little better because the order will be the same as in
> pwm_loongson_ddata.
>
OK, I would check the entire code again.

> > +       ddata->duty = pwm_loongson_set_config(ddata, duty_ns, clk_rate, PWM_DUTY);
> > +
> > +       return 0;
> > +}
> > +
> > +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                             const struct pwm_state *state)
> > +{
> > +       int err;
> > +       bool enabled = pwm->state.enabled;
> > +
> > +       if (state->polarity != pwm->state.polarity) {
> > +               if (enabled) {
> > +                       pwm_loongson_disable(chip, pwm);
> > +                       enabled = false;
> > +               }
> > +
> > +               err = pwm_loongson_set_polarity(chip, pwm, state->polarity);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> > +       if (!state->enabled) {
> > +               if (enabled)
> > +                       pwm_loongson_disable(chip, pwm);
> > +               return 0;
> > +       }
> > +
> > +       err = pwm_loongson_config(chip, pwm, state->duty_cycle, state->period);
> > +       if (err)
> > +               return err;
> > +
> > +       if (!enabled)
> > +               err = pwm_loongson_enable(chip, pwm);
> > +
> > +       return err;
> > +}
> > +
> > +static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                                 struct pwm_state *state)
> > +{
> > +       struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > +       u32 period, duty, ctrl;
> > +       u64 ns;
> > +
> > +       period = pwm_loongson_readl(ddata, PWM_PERIOD);
> > +       ns = period * NSEC_PER_SEC;
> > +       state->period = do_div(ns, period);
> > +
> > +       duty = pwm_loongson_readl(ddata, PWM_DUTY);
> > +       ns = duty * NSEC_PER_SEC;
> > +       state->duty_cycle = do_div(ns, duty);
> > +
> > +       ctrl = pwm_loongson_readl(ddata, PWM_CTRL);
> > +       state->polarity = (ctrl & PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED
> > +                       : PWM_POLARITY_NORMAL;
> > +       state->enabled = (ctrl & PWM_CTRL_EN) ? true : false;
> > +
> > +       ddata->ctrl = ctrl;
> > +       ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
> > +       ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct pwm_ops pwm_loongson_ops = {
> > +       .apply = pwm_loongson_apply,
> > +       .get_state = pwm_loongson_get_state,
> > +};
> > +
> > +static int pwm_loongson_probe(struct platform_device *pdev)
> > +{
> > +       struct pwm_loongson_ddata *ddata;
> > +       struct device *dev = &pdev->dev;
> > +
> > +       ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > +       if (!ddata)
> > +               return -ENOMEM;
> > +
> > +       ddata->base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (IS_ERR(ddata->base))
> > +               return PTR_ERR(ddata->base);
> > +
> > +       if (!has_acpi_companion(dev)) {
> > +               ddata->clk = devm_clk_get_enabled(dev, NULL);
> > +               if (IS_ERR(ddata->clk))
> > +                       return PTR_ERR(ddata->clk);
> > +       }
> > +
> > +       ddata->chip.dev = dev;
> > +       ddata->chip.ops = &pwm_loongson_ops;
> > +       ddata->chip.npwm = 1;
> > +       platform_set_drvdata(pdev, ddata);
> > +
> > +       return devm_pwmchip_add(dev, &ddata->chip);
> > +}
> > +
> > +static int pwm_loongson_suspend(struct device *dev)
> > +{
> > +       struct pwm_loongson_ddata *ddata = dev_get_drvdata(dev);
> > +
> > +       ddata->ctrl = pwm_loongson_readl(ddata, PWM_CTRL);
> > +       ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
> > +       ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
> > +
> > +       clk_disable_unprepare(ddata->clk);
> > +
> > +       return 0;
> > +}
> > +
> > +static int pwm_loongson_resume(struct device *dev)
> > +{
> > +       struct pwm_loongson_ddata *ddata = dev_get_drvdata(dev);
> > +       int ret;
> > +
> > +       ret = clk_prepare_enable(ddata->clk);
> > +       if (ret)
> > +               return ret;
> > +
> > +       pwm_loongson_writel(ddata, ddata->ctrl, PWM_CTRL);
> > +       pwm_loongson_writel(ddata, ddata->duty, PWM_DUTY);
> > +       pwm_loongson_writel(ddata, ddata->period, PWM_PERIOD);
> > +
> > +       return 0;
> > +}
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(pwm_loongson_pm_ops, pwm_loongson_suspend,
> > +                               pwm_loongson_resume);
> > +
> > +static const struct acpi_device_id pwm_loongson_acpi_ids[] = {
> > +       { "LOON0006" },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, pwm_loongson_acpi_ids);
> Moving the acpi table after the dt table is a little better because
> the order will be the same as in pwm_loongson_driver.
>

OK, I will adjust the order in the next version.

Thanks.
Binbin

> And others look good to me.
>
> Huacai
>
> > +
> > +static const struct of_device_id pwm_loongson_of_ids[] = {
> > +       { .compatible = "loongson,ls7a-pwm" },
> > +       { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, pwm_loongson_of_ids);
> > +
> > +static struct platform_driver pwm_loongson_driver = {
> > +       .probe  = pwm_loongson_probe,
> > +       .driver = {
> > +               .name   = "loongson-pwm",
> > +               .pm     = pm_ptr(&pwm_loongson_pm_ops),
> > +               .of_match_table   = pwm_loongson_of_ids,
> > +               .acpi_match_table = pwm_loongson_acpi_ids,
> > +       },
> > +};
> > +module_platform_driver(pwm_loongson_driver);
> > +
> > +MODULE_DESCRIPTION("Loongson PWM driver");
> > +MODULE_AUTHOR("Loongson Technology Corporation Limited.");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.43.0
> >
> >