Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Florian Fainelli <f.fainelli@gmail.com>]

On 3/7/22 10:47 AM, Uwe Kleine-König wrote:
> Hello Florian,
> 
> I have a few questions here looking in more detail into the brcmstb
> driver:
> 
>  - What happens on PWM_ON(channel) = 0?
>    I guess it just emits a flat inactive line, and refusing a small
>    duty_cycle that results in PWM_ON(channel) = 0 is just artificial?
> 
>  - There is a line describing:
> 
>    	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword
> 
>    The driver only considers powers of two <= 2^15 for cword. Is the
>    implementation just lazy, or is the comment misleading?
>    At least s/</<=/ ?
>    There is no sense in using a value > 2^15 as for each such value
>    there is a smaller value with the same result, right?

This was copied from the specification which now that you mention it,
seems off by one in its formula, it should be <=. This is further
confirmed with:

pwm1_cword[15:0] must be less than or equal to 32768 when the
variable-frequency PWM is used as a clock for the constant-frequency PWM.
Reset value is 0x0.

so I believe that the comment is wrong and so is the specification of
the block that was used to write the driver.

> 
>  - clk_get_rate(p->clk) is expected to return 27 MHz, right?
>    (Just for my understanding, not about to hardcode this in the code)

That is right.

> 
>  - The explanation about period in the comment is:
> 
>    	The period is: (period + 1) / Fv
> 
>    so I would have expected:
> 
>    	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1
> 
>    (assuming no overflows). However the -1 isn't in the code.
> 
>  - Duty-cycle calculation is unclear, the docs say:
> 
>  	"on" time is on / (period + 1)
> 
>    I suspect on time is $on / Fv though?

Yes, that is also what the specification says, not sure why I wrote that
down TBH.

>    But even with that I don't understand the +1 in
> 
>  	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
> 
> Can you enlighten me?

I wish, this was 7 years ago, and I do not remember why there was a +1
being added here, it might have been that this should have been a
DIV_ROUND_UP().

I am slowly rebuilding some context here so if you want me to test
something in the meantime, I will do that.
-- 
Florian