Message-ID: <5480C286.6000405@broadcom.com>
Date:	Thu, 4 Dec 2014 12:22:30 -0800
From:	Jonathan Richardson <jonathar@...adcom.com>
To:	Tim Kryger <tim.kryger@...il.com>
CC:	Arun Ramamurthy <arun.ramamurthy@...adcom.com>,
	Scott Branden <sbranden@...adcom.com>,
	Thierry Reding <thierry.reding@...il.com>,
	Ray Jui <rjui@...adcom.com>,
	Arun Ramamurthy <arunrama@...adcom.com>,
	<bcm-kernel-feedback-list@...adcom.com>,
	<linux-pwm@...r.kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/4] pwm: kona: Remove setting default smooth type
 and polarity for all channels

Hi Tim,

I'm going to take over this submission because I made the changes to the
driver. Arun was filling in for me while I was on leave. Now I'm back
and I think I can help clarify why these changes were made. A summary
for all the changes should help.

There were two problems. First was a problem caused by setting the
polarity in probe. It caused the speaker to click when the polarity was
set so we took that out as it didn't seem to serve any useful purpose
that I could see. The polarity changes should be made in the polarity
callback.

The second and bigger problem was the smooth type sequence. If it isn't
done according to the spec then one in ten or twenty times you won't
even get a signal when it's enabled. Following the correct sequence with
the 400 ns delays solves this problem.

Additionally, by not following the sequence you won't get a smooth
transition. You'll get a change in the settings (duty cycle, period) but
may get a non smooth transition. So it's important to follow the spec
here. We don't want non-smooth transitions.

More comments below.

On 14-11-28 07:19 PM, Tim Kryger wrote:
> On Fri, Nov 28, 2014 at 5:19 PM, Arun Ramamurthy
> <arun.ramamurthy@...adcom.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@...adcom.com> wrote:
>>>>
>>>>
>>>>
>>>> On 14-11-25 09:51 PM, Tim Kryger wrote:
>>>>>
>>>>>
>>>>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <sbranden@...adcom.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> From: Arun Ramamurthy <arunrama@...adcom.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@...adcom.com>
>>>>>> Reviewed-by: Ray Jui <rjui@...adcom.com>
>>>>>> Signed-off-by: Scott Branden <sbranden@...adcom.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.
> 

Agreed. When using DT, the polarity will be set properly. When using
sysfs there is a discrepancy and this needs to be addressed. Because the
hw default is active low, the polarity will be inversed when the sysfs
says polarity is "normal". I'll have to account for this in the driver.

>>
>>>>>
>>>>> 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?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists