linux-kernel - Re: [PATCH v9 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs

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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 23 May 2024 13:04:11 +1000
From: John Watts <contact@...kia.org>
To: きくちゃんさん <kikuchan98@...il.com>
Cc: privatesub2@...il.com, aou@...s.berkeley.edu, bigunclemax@...il.com,
	conor+dt@...nel.org, devicetree@...r.kernel.org,
	fusibrandon13@...il.com, jernej.skrabec@...il.com,
	krzk+dt@...nel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
	linux-riscv@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
	mkl@...gutronix.de, p.zabel@...gutronix.de, palmer@...belt.com,
	paul.walmsley@...ive.com, robh@...nel.org, samuel@...lland.org,
	ukleinek@...nel.org, wens@...e.org
Subject: Re: [PATCH v9 0/3] Add support for Allwinner PWM on D1/T113s/R329
 SoCs

On Thu, May 23, 2024 at 11:26:07AM +0900, きくちゃんさん wrote:
> Hello Aleksandr,
> 
> I had coincidentally developed a PWM driver for the device.
> Based on my experience, I find that dynamically changing the coupled
> DIV_M value is quite complex.
> The current approach has limitations, especially with resolution
> changes, which can be unpredictable for users. For example:
> 
>   1. Enabling channel A automatically selects DIV_M.
>   2. Enabling coupled channel B with a specific period may result in
> poor resolution for channel B, as the DIV_M value depends on the
> period of channel A.
>   3. If channel B is enabled first, channel A may not be enabled if
> its period doesn't fit the DIV_M range selected by channel B.
> 
> Additionally, using APB as a clock source for the channels would
> further complicate the process.
> 
> To simplify this, I suggest (maybe for the future) specifying these
> values directly in the Device Tree like this:
> ```
> allwinner,pwm-coupled-channel-clock-sources="hosc", "apb", "hosc";
> allwinner,pwm-coupled-channel-clock-prescales=<0>, <3>, <8>;
> ```
> This would delegate the complexity to the DT, making the resolution
> predictable for users.
> As a bonus, it introduces a way to select clock sources for each
> coupled channels.
> 
> For the meantime, I think it is enough to use fixed "hosc" and <0> for
> regular use.
> 
> Looking forward to your thoughts.
> 
> Best regards,
> kikuchan.

I have a somewhat opposite opinion. I've developed a driver too and posted it
on the u-boot mailing list that is deterministic and handles both channels:

https://lore.kernel.org/all/20240518-pwm_d1-v1-0-311fc5fe2248@jookia.org/

It does this by remembering the settings for channels and disabling then
setting both channels at once whenever there's an update.

I think this is a decent enough solution to the problem and just works
automatically without people having to micromanage the controller.

John.