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

[Florian Fainelli]

On 2/14/22 10:34 AM, Uwe Kleine-König wrote:
> On Mon, Feb 14, 2022 at 09:18:49AM -0800, Florian Fainelli wrote:
>> On 2/14/22 12:23 AM, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> here are a few cleanups for the brcmstb PWM driver. There are a few
>>> issues left with it, that I'm not addressing for now. Just mention it in
>>> case someone wants to work on this driver:
>>>
>>>  - There is no .get_state() callback
>>>    (That needs to be implemented by some with hardware and
>>>    documentation)
> 
> Assuming you have access to documentation, can you confirm, that the
> registers that define the PWM's behaviour are readable? If I knew that I
> could come up with an implementation for .get_state().

Yes, the registers are read/write with no side effects, so this is going
to be working.

> 
>>>  - There are a few places where an overflow can happen in
>>>    brcmstb_pwm_config() that are not handled
>>>
>>>  - The loop in brcmstb_pwm_config() to calculate cword is ineffective,
>>>    cword could be calculated ad hoc.
>>>
>>>  - I don't understand
>>>
>>>                 /*
>>>                  * We can be called with separate duty and period updates,
>>>                  * so do not reject dc == 0 right away
>>>                  */
>>>                 if (pc == PWM_PERIOD_MIN || (dc < PWM_ON_MIN && duty_ns))
>>>                         return -EINVAL;
>>>
>>>    The usual policy is "With the selected period, pick the biggest
>>>    possible duty_cycle that isn't bigger thatn the requested duty_cycle.
>>>    So should this case be handled using dc = 0 instead?
>>>    But as I don't understand the real issue here (is this about changing
>>>    period and duty at the same time?), I don't want to touch that.
>>
>> IIRC, I was testing using a shell script that would exercise corner
>> cases by modifying the /sys/class/pwm/*/{period,duty_cycle} separately
>> was able to run into that. Let me see if I can dig up that script.
> 
> When you find it, it would be great to document the problem in a way
> that it's still understandable some time later.
> 
>> Can you give me a day or two to make sure your changes work properly? I
>> need to locate a board with an exposed PWM header so I can put a scope
>> on it. Thanks!
> 
> Sure, in my experience it takes longer than two days on average until
> Thierry picks up PWM patches. Thanks for your willingness to test!

The least I can do, short of doing the conversion myself ;) With that
said, I just tested your two patches and things still worked just fine:

Tested-by: Florian Fainelli <f.fainelli at gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli at gmail.com>

Thanks a lot Uwe!
-- 
Florian