Re: [PATCH v2 1/4] pwm: kona: Remove setting default smooth type and polarity for all channels
From: Arun Ramamurthy
Date: Mon Dec 01 2014 - 14:52:58 EST
On 14-11-28 07:19 PM, Tim Kryger wrote:
On Fri, Nov 28, 2014 at 5:19 PM, Arun Ramamurthy
<arun.ramamurthy@xxxxxxxxxxxx> wrote:
On 14-11-28 05:08 PM, Tim Kryger wrote:
On Fri, Nov 28, 2014 at 3:47 PM, Arun Ramamurthy
<arun.ramamurthy@xxxxxxxxxxxx> wrote:
On 14-11-25 09:51 PM, Tim Kryger wrote:
On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <sbranden@xxxxxxxxxxxx>
wrote:
From: Arun Ramamurthy <arunrama@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
Reviewed-by: Ray Jui <rjui@xxxxxxxxxxxx>
Signed-off-by: Scott Branden <sbranden@xxxxxxxxxxxx>
---
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/