Messages in this thread | | | Date | Mon, 1 Dec 2014 11:37:57 -0800 | From | Arun Ramamurthy <> | Subject | Re: [PATCH v2 1/4] pwm: kona: Remove setting default smooth type and polarity for all channels |
| |
On 14-11-28 07:19 PM, Tim Kryger wrote: > 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? > It clicks at time of boot up, I can agree to the above changes and will update the next patch set accordingly. > 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. > I had it mixed up, I will update the polarity to match the PWM framework. >> >>>>> >>>>> 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?
| |