Re: [PATCH 1/6 v9] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's

[Alexey Charkov <alchark@gmail.com>]

2010/12/21 Alexey Charkov <alchark@gmail.com>:
> 2010/12/21 Ryan Mallon <ryan@bluewatersys.com>:
>> On 12/21/2010 12:00 PM, Alexey Charkov wrote:
>>> 2010/12/21 Ryan Mallon <ryan@bluewatersys.com>:
>>>> On 12/21/2010 10:48 AM, Alexey Charkov wrote:
>>>>> 2010/12/20 Ryan Mallon <ryan@bluewatersys.com>:
>>>>>> On 12/21/2010 08:54 AM, Alexey Charkov wrote:
>>>>>>> +}
>>>>>>> +
>>>>>>> +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
>>>>>>> +{
>>>>>>> +     unsigned long long c;
>>>>>>> +     unsigned long period_cycles, prescale, pv, dc;
>>>>>>> +
>>>>>>> +     if (pwm == NULL || period_ns == 0 || duty_ns > period_ns)
>>>>>>> +             return -EINVAL;
>>>>>>> +
>>>>>>> +     c = 25000000/2; /* wild guess --- need to implement clocks */
>>>>>>> +     c = c * period_ns;
>>>>>>> +     do_div(c, 1000000000);
>>>>>>> +     period_cycles = c;
>>>>>>
>>>>>> This looks like it could be reworked to remove the do_div call.
>>>>>>
>>>>>
>>>>> I just followed PXA implementation in this regard. Are there any
>>>>> specific suggestions? Note that c should not be a complie-time
>>>>> constant eventually, as this is the guessed PWM base frequency (should
>>>>> be read from the hardware, but the code for clocks is not yet in).
>>>>
>>>> I didn't have a particular solution in mind, but often by changing the
>>>> units used and rearranging the math a bit you can often avoid having to
>>>> do horrible multiplies and divides.
>>>>
>>>> For now at least you could avoid the do_div by assigning period_cycles
>>>> directly.
>>>>
>>>
>>> It depends on period_ns, which is passed in as an argument from
>>> whatever uses PWM, so I'm not sure it can be assigned directly.
>>
>> Ah. How big a number is period_ns? Can you do something like this instead?
>>
>>        period_cycles = ((250 / 2) * period_ns) / 10000;
>>
>> and still safely avoid overflows?
>>
>
> The only current in-kernel user of PWM is the backlight, and that
> currently uses period_ns = 250000. At this value it does not overflow.
> However, in a general case the base frequency will also be returned as
> a large number (like 12500000) as per CLK infrastructure conventions
> (once that part is implemented). Further, I can't see any built-in
> reasons for period_ns to be bounded by anything below sizeof(int). The
> hardware will work with up to 4096*1024*1000000000/base_frequency (see
> the code for constraints), so it can in principle overflow with 32 bit
> arithmetics.
>

This discussion led me to look closer at the duty counter calculation:
if period_ns and duty_ns are both large and close to each other (about
0.3 seconds, rare but possible use case for PWM), then (pv * duty_ns)
can overflow in 32 bit multiplication for permissible argument values.
Should I use do_div((u64)pv * (u64)duty_ns, period_ns) here?

Best regards,
Alexey