Caglar Akyuz <caglarak...@gmail.com> writes:
> On Thursday 16 September 2010 08:24:50 pm Kevin Hilman wrote:
>> 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?
>
> My guess is that because there is not any state variable it can rely on to?
Exactly. Either the backlight code or the PWM driver needs to keep
state. Looks like Sugumar decided to add it to his PWM layer in v5.
> Backlight update operation can be due to either user writing to sysfs node or
> fb blanking.
>
>> Either it should call disable, change the value, call enable,
>> or if it leaves it enabled, it should not call pwm_enable() again.
>>
>
> Code is attached for your reference, I don't see a way of fixing it. This
> code
> is called un-conditionally when anything is written to sysfs node(no check
> there either).
It could be fixed as easily as Sugumar fixed his patch, simply by adding
state.
Anyways, it's a minor concern.
Kevin
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
