lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 28 Nov 2014 19:19:51 -0800 From: Tim Kryger <tim.kryger@...il.com> To: Arun Ramamurthy <arun.ramamurthy@...adcom.com> Cc: 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 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. > >>>> >>>> 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/
Powered by blists - more mailing lists