Received-SPF: pass (google.com: domain of linux-kernel+bounces-197278-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240531141152.327592-1-kikuchan98@gmail.com> <20240531141152.327592-6-kikuchan98@gmail.com>
 <851280ad-ac0e-47d1-99e2-4f3b5ea29f2f@kernel.org>
In-Reply-To: <851280ad-ac0e-47d1-99e2-4f3b5ea29f2f@kernel.org>
From: Hironori KIKUCHI <kikuchan98@gmail.com>
Date: Sat, 1 Jun 2024 02:57:15 +0900
Message-ID: <CAG40kxEbMQc-ni0HDVR7rtj48aFu-jz8sYUAO+fdmZSmXWrizw@mail.gmail.com>
Subject: Re: [PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a
 clock source and DIV_M
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: linux-kernel@vger.kernel.org, =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= <ukleinek@kernel.org>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>, 
	Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, 
	Samuel Holland <samuel@sholland.org>, Aleksandr Shubin <privatesub2@gmail.com>, 
	Cheo Fusi <fusibrandon13@gmail.com>, linux-pwm@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
	linux-sunxi@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"

Hello,

> > This patch adds new options to select a clock source and DIV_M register
> > value for each coupled PWM channels.
>
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
> Bindings are before their users. This should not be last patch, because
> this implies there is no user.

I'm sorry, I'll fix them.

> This applies to all variants? Or the one you add? Confused...

Apologies for confusing you. This applies to all variants.

>
> >
> > Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> > ---
> >  .../bindings/pwm/allwinner,sun20i-pwm.yaml    | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > index b9b6d7e7c87..436a1d344ab 100644
> > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
> > @@ -45,6 +45,25 @@ properties:
> >      description: The number of PWM channels configured for this instance
> >      enum: [6, 9]
> >
> > +  allwinner,pwm-pair-clock-sources:
> > +    description: The clock source names for each PWM pair
> > +    items:
> > +      enum: [hosc, apb]
> > +    minItems: 1
> > +    maxItems: 8
>
> Missing type... and add 8 of such items to your example to make it complete.

Thank you. I'll fix it.

>
> > +
> > +  allwinner,pwm-pair-clock-prescales:
> > +    description: The prescale (DIV_M register) values for each PWM pair
> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    items:
> > +      items:
> > +        minimum: 0
> > +        maximum: 8
> > +      minItems: 1
> > +      maxItems: 1
> > +    minItems: 1
> > +    maxItems: 8
>
> This does not look like matrix but array.

I wanted to specify values like this:

    allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>;
    allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc":

These should correspond to each PWM pair.
This way, I thought we might be able to visually understand the relationship
between prescalers and sources, like clock-names and clocks.

Is this notation uncommon, perhaps?

>
> Why clock DIV cannot be deduced from typical PWM attributes + clock
> frequency?

This SoC's PWM system has one shared prescaler and clock source for each pair
of PWM channels. I should have noted this earlier, sorry.

Actually, the original v9 patch automatically deduced the DIV value
from the frequency.
However, because the two channels share a single prescaler, once one channel is
enabled, it affects and restricts the DIV value for the other channel
in the pair.
This introduces a problem of determining which channel should set the shared DIV
value. The original behavior was that the first channel enabled would win.

Instead, this patch try to resolve the issue by specifying these
values for each PWM
pairs deterministically.
That's why it requires the new options.

>
> Best regards,
> Krzysztof

Best regards,
kikuchan.