Hello Alexey,
On 2024-12-10 01:40, Alexey Charkov wrote:
On Tue, Dec 10, 2024 at 3:30 AM FUKAUMI Naoki <naoki@xxxxxxxxx> wrote:
Thanks for your review!
On 12/10/24 01:32, Dragan Simic wrote:
> Hello Fukaumi,
>
> On 2024-12-09 13:51, FUKAUMI Naoki wrote:
>> fix pwm period to match with vendor kernel[1].
>
> Instead of simply referring to the downstream vendor kernel, in this
> specific case the reasons for adjusting the fan PWM parameters should
> be explained by referring to the actual fan setup you're using, the
> observed fan RPM behavior, etc.
original commit message is:
| arm64: dts: rockchip: modify fan pwm period to 60us
| Reduce pwm frequency to 16.6 KHz for a larger adjustable range of
AO3416 mosfet.
I have no knowledge about this kind of things. Is quoting this message
enough?
I think it would be better to expand a bit to make sure the commit
message explains the whole rationale without too much extra digging.
Something like this:
arm64: dts: rockchip: Use a longer PWM period for the fan on Radxa ROCK
5C
The fan on Radxa ROCK 5C is driven via an AO3416 MOSFET, which has a
total switch-on time of 0,6us and a total switch-off time of 6us [1],
meaning that the current PWM period of just 10us is too short for
fine-grained fan speed control. Increase the PWM period to 60us, so
that the switch-on and switch-off time of the MOSFET fall within a
more reasonable ~10% of the full period, thus making lower PWM duty
cycles meaningful.
[1] https://www.aosmd.com/pdfs/datasheet/AO3416.pdf
Well written, thanks. That's pretty much the same how I wanted
to explain it, but you were faster. :)
Additionally, it's quite strange that the AO3416 FET is used in
that place. Its large continuous current-carrying capability
(over 5 A at 70 oC!) is an absolute overkill for driving a small
fan, and its integrated ESD protection ends up basically useless
in this case. Radxa could've easily redirected a few pennies into
something else by using a much less beefy FET.
