| Message ID | 38c234d548b4b9c334cda6e7664a803896b31f6f.1712732719.git.zhoubinbin@loongson.cn |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | pwm: Introduce pwm driver for the Loongson family chips | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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
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 >
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
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 >
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
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
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
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
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
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