Re: [PATCH v2 1/4] pwm: kona: Remove setting default smooth type and polarity for all channels

[Tim Kryger <tim.kryger@gmail.com>]

On Fri, Nov 28, 2014 at 5:19 PM, Arun Ramamurthy
<arun.ramamurthy@broadcom.com> wrote:
>
>
> On 14-11-28 05:08 PM, Tim Kryger wrote:
>>
>> On Fri, Nov 28, 2014 at 3:47 PM, Arun Ramamurthy
>> <arun.ramamurthy@broadcom.com> wrote:
>>>
>>>
>>>
>>> On 14-11-25 09:51 PM, Tim Kryger wrote:
>>>>
>>>>
>>>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <sbranden@broadcom.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> From: Arun Ramamurthy <arunrama@broadcom.com>
>>>>>
>>>>> The probe routine unnecessarily sets the smooth type and polarity for
>>>>> all channels. This causes the channel for the speaker to click at the
>>>>> same
>>>>> time the backlight turns on. The smooth type and polarity should be set
>>>>> individually
>>>>> for each channel as required and no defaults need to be set.
>>>>
>>>>
>>>>
>>>> I am guessing you are talking about a PWM controlled beeper/buzzer.
>>>>
>>> This change is more so to remove setting smooth type and polarity for all
>>> channels during probe and to leave them as their default values. Infact,
>>> setting the PWM_CONTROL_TYPE_SHIT is also redundant cause the default
>>> value
>>> is already 1 for all channels. We can remove that loop entirely and this
>>> will be done in the next patch set. The smooth type and polarity are only
>>> changed when the particular pwm channel is enabled or polarity is
>>> changed.
>>>
>>>> Can you mention what board you are observing this issue on?
>>>>
>>>> Also please explain why setting these bits result in an audible click.
>>>>
>>> We observe this on the bcm958300K board where one of the
>>> PWM channels is connected to the buzzer and changing the
>>> smooth type and polarity from its default values causes a click
>>>
>>
>> Which of these two bits is causing the click?
>>
>> I've already said that I'm open to removing the smooth bit here if that
>> helps.
>>
> Thank you for your quick reply Tim. It is setting the polarity bit that
> causes the click. I am planning on removing this entire loop in the next
> patch set, are you okay with that?

Does it cause a click at this moment or at a later point in time?

Is the click your sole motivation for this series?  If so, can you
propose a more targeted fix?

I would be amenable to deferring polarity changes until subsequent
enable operations:

- kona_pwmc_probe doesn't do any hardware writes
- kona_pwmc_set_polarity only updates a polarity variable
- kona_pwmc_enable sets hardware polarity according to the variable

Would this be sufficient to satisfy your needs?

I am still worried that deferring polarity changes may negatively
impact some PWM users who set polarity but don't immediately enable
the PWM.

>>>>>
>>>>> Signed-off-by: Arun Ramamurthy <arunrama@broadcom.com>
>>>>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>>>>> Signed-off-by: Scott Branden <sbranden@broadcom.com>
>>>>> ---
>>>>>    drivers/pwm/pwm-bcm-kona.c | 7 ++-----
>>>>>    1 file changed, 2 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>>>>> index 02bc048..29eef9e 100644
>>>>> --- a/drivers/pwm/pwm-bcm-kona.c
>>>>> +++ b/drivers/pwm/pwm-bcm-kona.c
>>>>> @@ -266,12 +266,9 @@ static int kona_pwmc_probe(struct platform_device
>>>>> *pdev)
>>>>>                   return ret;
>>>>>           }
>>>>>
>>>>> -       /* Set smooth mode, push/pull, and normal polarity for all
>>>>> channels */
>>>>> -       for (chan = 0; chan < kp->chip.npwm; chan++) {
>>>>> -               value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>>>>> +       /* Set push/pull for all channels */
>>>>> +       for (chan = 0; chan < kp->chip.npwm; chan++)
>>>>>                   value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
>>>>> -               value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
>>>>> -       }
>>>>>
>>>>>           writel(value, kp->base + PWM_CONTROL_OFFSET);
>>>>
>>>>
>>>>
>>>> While the smooth bit need not be set here, it is important that the
>>>> polarity bit be set.
>>>>
>>> The default value for polarity is 0 which is normal polarity, so setting
>>> it
>>> to 1 here in the probe function without a sysfs call is
>>> when the software will report the polarity as normal when it is actually
>>> inversed.
>>
>>
>> Please double check the meaning of the polarity bits for the revision
>> of PWM IP in your chip.  I suspect you are mistaken here.
>>
>> This driver is for PWM blocks compatible those found in bcm28145,
>> bcm28155, bcm21664, and other mobile chips of that generation.
>>
>> Apparently in contrast to the chip you are using, a set polarity bit
>> in the control register means normal polarity for the chips I
>> mentioned.
>>
>> A default of zero for these bits means they must be set to meet the
>> PWM framework's expectation that channels begin with normal polarity.
>>
> Tim, this is from the RDB of our new chip which is supposed to have the same
> IP as the mobile chip sets you mentioned:
>
> When set to 1 the output polarity for the PWM Output signal will be active
> hight; When set to 0, the output polarity for the PWM Output signal will be
> active low. Default State is 0.
>
> My understanding is that the frameworks normal polarity means active low, am
> I mistaken in that?

That is not how I would interpret things.

Perhaps this paragraph from Documentation/pwm.txt will help you:

When implementing polarity support in a PWM driver, make sure to respect the
signal conventions in the PWM framework. By definition, normal polarity
characterizes a signal starts high for the duration of the duty cycle and
goes low for the remainder of the period. Conversely, a signal with inversed
polarity starts low for the duration of the duty cycle and goes high for the
remainder of the period.

>
>>>>
>>>> Otherwise software will report the polarity as normal when it it is
>>>> actually inversed.
>>>>
>>>> Consider the case where a userspace process is controlling the PWM via
>>>> sysfs.
>>>>
>>> I agree with you about the sysfs case Tim, but since this is the probe
>>> function and not a sysfs callback, should we not leave it as the default
>>> value?