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. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
