linux-kernel - Re: [PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a clock source and DIV

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e7dvk2g4ymz527tklywy2elka4ndzvftfujobnjmfdepgt3lm5@dzex45st2wya>
Date: Sat, 13 Jul 2024 14:58:35 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Hironori KIKUCHI <kikuchan98@...il.com>
Cc: Andre Przywara <andre.przywara@....com>, 
	Krzysztof Kozlowski <krzk@...nel.org>, linux-kernel@...r.kernel.org, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, 
	Aleksandr Shubin <privatesub2@...il.com>, Cheo Fusi <fusibrandon13@...il.com>, linux-pwm@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH 5/5] dt-bindings: pwm: sun20i: Add options to select a
 clock source and DIV_M

Hello,

On Mon, Jun 03, 2024 at 05:42:08PM +0900, Hironori KIKUCHI wrote:
> 2024年6月3日(月) 9:10 Andre Przywara <andre.przywara@....com>:
> >
> > On Sun, 2 Jun 2024 15:15:13 +0900
> > Hironori KIKUCHI <kikuchan98@...il.com> wrote:
> >
> > > > On 31/05/2024 19:57, Hironori KIKUCHI wrote:
> > > > >>> ---
> > > > >>>  .../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?
> > > >
> > > > It's still an array.
> > >
> > > Oh I understood and clear. Thank you.

For clocks there is already a binding to assign a working configuration.
assigned-clocks, assigned-clock-rates and assigned-clock-parents are the
relevant properties. If you create a clk from the parent clock selector
and mdiv, you can stick to the existing bindings.

> > > [...]
> >
> > So I understand the problem, but I don't think expressing this in the
> > devicetree is the right solution. It seems like a tempting pragmatical
> > approach, but it sounds like the wrong way: this is not a hardware
> > *description* of any kind, but rather a way to describe a certain user
> > intention or a configuration. So this looks like a rather embedded
> > approach to me, where you have a certain fixed register setup in mind,
> > and want to somehow force this to the hardware.
> > Another problem with this approach is that it doesn't really cover the
> > sysfs interface, which is very dynamic by nature.
> 
> ... Indeed. You're right.
> Now I've realized it was a bad idea.
> It should be done in sysfs or sysctl perhaps.

I don't think a driver specific knob somewhere is a practical solution.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)