[PATCH v4 03/12] arm64: dts: rockchip: fix property for pwm-fan for Radxa ROCK 5C

[Dragan Simic]

Hello Alexey,

On 2024-12-10 01:40, Alexey Charkov wrote:
> On Tue, Dec 10, 2024 at 3:30 AM FUKAUMI Naoki <naoki at radxa.com> 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.