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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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