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@xxxxxxxxxx> wrote: >>>> >>>> On 11/04/2024 13:01, Binbin Zhou wrote: >>>>> On Thu, Apr 11, 2024 at 4:26 PM Krzysztof Kozlowski >>>>> <krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxxxx> >>>>>> >>>>>> >>>>>>> +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
