Caglar Akyuz <caglarak...@gmail.com> writes:
>> Sugumar Natarajan wrote:
>>On Fri, Sep 10, 2010 at 23:12:36, Caglar Akyuz wrote:
>>> On Thursday 09 September 2010 01:40:49 pm Sugumar Natarajan wrote:
>>> > This patch adds generic PWM support where it maintains the list of PWM
>>> > control devices that can be added or removed.
>>> >
>>> > The interface provides a list of functions that can be accessed by the
>>> > PWM control driver module and the generic PWM driver.
>>> >
>>> > The PWM control driver module such as eCAP uses the interface to
>>> > register and add itself to the list as a PWM control device.
>>> > The generic PWM driver uses the interface to search for a PWM control
>>> > device and if present, uses the device for PWM control.
>>> >
>>> > Signed-off-by: Sugumar Natarajan <sugumar at ti.com>
>>> > ---
>>>
>>> Hi,
>>>
>>> [...]
>>>
>>> > +
>>> > +int pwm_enable(struct pwm_device *pwm) {
>>> > + if (WARN_ON(!pwm))
>>> > + return -EINVAL;
>>> > +
>>> > + return clk_enable(pwm->clk);
>>> > +}
>>> > +EXPORT_SYMBOL(pwm_enable);
>>> > +
>>>
>>> I think 'clk_enable' should be controlled for un-matched pwm_enable/disable
>>operation. Otherwise, disabling PWM won't shut down
>>peripheral clock. If I understand inner workings of DaVinci clock
>>implementation clock use_count is incremented with every
>>clk_enable.
>>>
>>> Regards,
>>> Caglar
>>>
>>
>>Caglar,
>> I could not understand what you mean by "un-match" .could you please
>>explain?
>
>>Regards,
>>N.sugumar
>
>
> List server didn't deliver this mail to me, I saw it from the archives. I
> mean
> something like this:
>
> int pwm_enable(struct pwm_device *pwm)
> {
> int rc = 0;
>
> if (WARN_ON(!pwm))
> return -EINVAL;
>
> if (!pwm->clk_enabled) {
> rc = clk_enable(pwm->clk);
> if (!rc)
> pwm->clk_enabled = 1;
> }
> return rc;
> }
>
>
> The reason is 'pwm_enable' can be called over and over without 'pwm_disable'
> and vice versa. For instance, every time backlight value is changed,
> pwm_enable is called by the backlight framework.
Implementing these extra checks is usually a safe way to go, however,
this could also be considered a bug in the backlight driver.
IOW, why is it repeatedly calling pwm_enable() when the value is
changed? Either it should call disable, change the value, call enable,
or if it leaves it enabled, it should not call pwm_enable() again.
Kevin
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
