From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Mika_Penttil=c3=a4?= Subject: Re: pwm: Introduce the pwm_args concept regression Date: Tue, 17 May 2016 16:44:05 +0300 Message-ID: <573B2025.3070408@nextfour.com> References: <573AAEAF.5040603@nextfour.com> <20160517103151.54d1b413@bbrezillon> <573AE8E3.2070500@nextfour.com> <20160517121632.102f7bbd@bbrezillon> <20160517120523.GC26166@ulmo.ba.sec> <573B1985.6010006@nextfour.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-am1on0078.outbound.protection.outlook.com ([157.56.112.78]:47184 "EHLO emea01-am1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754484AbcEQOAN (ORCPT ); Tue, 17 May 2016 10:00:13 -0400 In-Reply-To: <573B1985.6010006@nextfour.com> Sender: linux-pwm-owner@vger.kernel.org List-Id: linux-pwm@vger.kernel.org To: Thierry Reding , Boris Brezillon Cc: jingoohan1@gmail.com, linux-pwm@vger.kernel.org On 17.05.2016 16:15, Mika Penttil=C3=A4 wrote: > > On 17.05.2016 15:05, Thierry Reding wrote: >> On Tue, May 17, 2016 at 12:16:32PM +0200, Boris Brezillon wrote: >>> On Tue, 17 May 2016 12:48:19 +0300 >>> Mika Penttil=C3=A4 wrote: >>> >>>> On 05/17/2016 11:31 AM, Boris Brezillon wrote: >>>>> +Thierry >>>>> >>>>> Hi Mika, >>>>> >>>>> On Tue, 17 May 2016 08:39:59 +0300 >>>>> Mika Penttil=C3=A4 wrote: >>>>> =20 >>>>>> Hi, >>>>>> >>>>>> commit e39c0df1be5a97e0910b09af1530bdf3de057a06, pwm: Introduce = the pwm_args concept >>>>>> causes pwm backlight to fail on our imx6 based board. I suspect = it affects all pwm backlight setups. >>>>>> With this reverted, everything works as normal. =20 >>>>> Hm, just tried using a PWM backlight on an atmel board (at91sama5= d4ek), >>>>> and after applying the following patch, it worked correctly (that= 's actually >>>>> a bug in the PWM driver: the HLCDC PWM device is configured with = inversed >>>>> polarity by default, and the driver was not taking that into acco= unt). >>>>> I doubt you have the same problem here since your PWM device is n= ot >>>>> supporting polarity configuration. >>>>> >>>>> Could you tell me more about this bug? What are the symptoms? Whi= ch >>>>> board are you testing on (if it's not mainlined, can you share yo= ur >>>>> DT)? >>>>> >>>>> Thanks, >>>>> >>>>> Boris >>>>> >>>>> --- >>>>> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atme= l-hlcdc.c >>>>> index f994c7e..14fc011 100644 >>>>> --- a/drivers/pwm/pwm-atmel-hlcdc.c >>>>> +++ b/drivers/pwm/pwm-atmel-hlcdc.c >>>>> @@ -272,7 +272,7 @@ static int atmel_hlcdc_pwm_probe(struct platf= orm_device *pdev) >>>>> chip->chip.of_pwm_n_cells =3D 3; >>>>> chip->chip.can_sleep =3D 1; >>>>> =20 >>>>> - ret =3D pwmchip_add(&chip->chip); >>>>> + ret =3D pwmchip_add_with_polarity(&chip->chip, PWM_POLARI= TY_INVERSED); >>>>> if (ret) { >>>>> clk_disable_unprepare(hlcdc->periph_clk); >>>>> return ret; >>>>> =20 >>>> Symptom is entirely dark screen. >>>> The board is not mainlined but the backlight part is defined like = :=20 >>>> >>>> backlight: backlight { >>>> compatible =3D "pwm-backlight"; >>>> pwms =3D <&pwm2 0 1000000 PWM_POLARITY_INVERTED>; >>>> /* >>>> * a poor man's way to create a 1:1 relationship b= etween >>>> * the PWM value and the actual duty cycle >>>> */ >>>> brightness-levels =3D < 0 1 2 3 4 5 6 7 8 = 9 >>>> 10 11 12 13 14 15 16 17 18 19 >>>> 20 21 22 23 24 25 26 27 28 29 >>>> 30 31 32 33 34 35 36 37 38 39 >>>> 40 41 42 43 44 45 46 47 48 49 >>>> 50 51 52 53 54 55 56 57 58 59 >>>> 60 61 62 63 64 65 66 67 68 69 >>>> 70 71 72 73 74 75 76 77 78 79 >>>> 80 81 82 83 84 85 86 87 88 89 >>>> 90 91 92 93 94 95 96 97 98 99 >>>> 100>; =20 >>>> default-brightness-level =3D <50>; >>>> }; >>>> >>>> >>>> so the polarity is in there defined. >>>> The call call graphs is like : devm_pwm_get() -> pwm_get() -> of_p= wm_get()=20 >>>> -> of_pwm_xlate_with_flags which used to call pwm_set_period and p= wm_set_polarity but now =20 >>>> just update the struct values. >> Adding the linux-pwm mailing list on Cc for archiving. >> >> Do you have any local modifications? of_pwm_xlate_with_flags() is on= ly >> used if explicitly set up by the PWM driver and those that explicitl= y do >> so are: >> >> $ git grep -n '=3D.*of_pwm_xlate_with_flags' -- drivers/pwm/ >> drivers/pwm/pwm-atmel-hlcdc.c:271: chip->chip.of_xlate =3D of_= pwm_xlate_with_flags; >> drivers/pwm/pwm-atmel-tcb.c:397: tcbpwm->chip.of_xlate =3D o= f_pwm_xlate_with_flags; >> drivers/pwm/pwm-atmel.c:378: atmel_pwm->chip.of_xlate =3D= of_pwm_xlate_with_flags; >> drivers/pwm/pwm-bcm-kona.c:277: kp->chip.of_xlate =3D of_pwm_xlate_= with_flags; >> drivers/pwm/pwm-berlin.c:181: pwm->chip.of_xlate =3D of_pwm_xlate= _with_flags; >> drivers/pwm/pwm-fsl-ftm.c:445: fpc->chip.of_xlate =3D of_pwm_xlate= _with_flags; >> drivers/pwm/pwm-lpc18xx-sct.c:383: lpc18xx_pwm->chip.of_xlate = =3D of_pwm_xlate_with_flags; >> drivers/pwm/pwm-omap-dmtimer.c:318: omap->chip.of_xlate =3D of_= pwm_xlate_with_flags; >> drivers/pwm/pwm-renesas-tpu.c:421: tpu->chip.of_xlate =3D of_p= wm_xlate_with_flags; >> drivers/pwm/pwm-rockchip.c:269: pc->chip.of_xlate =3D of_pw= m_xlate_with_flags; >> drivers/pwm/pwm-samsung.c:517: chip->chip.of_xlate =3D of_= pwm_xlate_with_flags; >> drivers/pwm/pwm-sun4i.c:335: pwm->chip.of_xlate =3D of_pwm_xlate= _with_flags; >> drivers/pwm/pwm-tiecap.c:229: pc->chip.of_xlate =3D of_pwm_xlate_= with_flags; >> drivers/pwm/pwm-tiehrpwm.c:460: pc->chip.of_xlate =3D of_pwm_xlate_= with_flags; >> drivers/pwm/pwm-vt8500.c:219: chip->chip.of_xlate =3D of_pwm_xlat= e_with_flags; >> >> pwm-imx is not among them and since it doesn't set anything else it >> should be using of_pwm_simple_xlate(). Also, the pwm-backlight drive= r >> now calls pwm_apply_args() so it should still end up calling >> pwm_set_polarity(). >> >> Can you paste the DT node that has the pwm2 label? I suspect it's th= e >> one in arch/arm/boot/dts/imx6qdl.dtsi (or something similar). That n= ode >> has #pwm-cells =3D <2>, so providing the polarity in a third cell is= not >> correct. The PWM core /should/ be able to deal with it correctly, bu= t I >> think it might be worth correcting the DTS. There are a bunch of oth= er >> DTS files that contain the same mistake. >> >> If that doesn't help, perhaps you can attach dmesg output to help de= bug >> this further? The PWM core is fairly silent, though, so you might ne= ed >> to add some debugging messages in order to be able to see anything. = I'm >> thinking a first step would be to make sure all of the driver callba= cks >> get called in the sequence that they should (and with the expected >> parameters). >> >> Let me know if you need help with that. >> >> Thierry > Yes you're right the pwm-cells is <2> and polarity in dts has no effe= ct, and yes it > uses pwm_simple_xlate of course. The polarity is ok, but something br= oke with this patch, > looks like the pwm hw state is stuck to zero even when modified thru = /sys/class/backlight/backlight/brightness > (the values in sysfs do change). I can debug this further tomorrow. > > --Mika > > it's polarity issue, we have it right in hw and it all worked before = this patch. > is not polarity issue, had to say. --Mika