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:15:49 +0300 Message-ID: <573B1985.6010006@nextfour.com> References: <573AAEAF.5040603@nextfour.com> <20160517103151.54d1b413@bbrezillon> <573AE8E3.2070500@nextfour.com> <20160517121632.102f7bbd@bbrezillon> <20160517120523.GC26166@ulmo.ba.sec> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-am1on0088.outbound.protection.outlook.com ([157.56.112.88]:18512 "EHLO emea01-am1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754868AbcEQNsY (ORCPT ); Tue, 17 May 2016 09:48:24 -0400 In-Reply-To: <20160517120523.GC26166@ulmo.ba.sec> 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 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 t= he pwm_args concept >>>>> causes pwm backlight to fail on our imx6 based board. I suspect i= t affects all pwm backlight setups. >>>>> With this reverted, everything works as normal. =20 >>>> Hm, just tried using a PWM backlight on an atmel board (at91sama5d= 4ek), >>>> 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 i= nversed >>>> polarity by default, and the driver was not taking that into accou= nt). >>>> I doubt you have the same problem here since your PWM device is no= t >>>> supporting polarity configuration. >>>> >>>> Could you tell me more about this bug? What are the symptoms? Whic= h >>>> board are you testing on (if it's not mainlined, can you share you= r >>>> DT)? >>>> >>>> Thanks, >>>> >>>> Boris >>>> >>>> --- >>>> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel= -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 platfo= rm_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_POLARIT= Y_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 be= tween >>> * 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_pw= m_get()=20 >>> -> of_pwm_xlate_with_flags which used to call pwm_set_period and pw= m_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 onl= y > used if explicitly set up by the PWM driver and those that explicitly= 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_p= wm_xlate_with_flags; > drivers/pwm/pwm-atmel-tcb.c:397: tcbpwm->chip.of_xlate =3D of= _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_w= ith_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_p= wm_xlate_with_flags; > drivers/pwm/pwm-renesas-tpu.c:421: tpu->chip.of_xlate =3D of_pw= m_xlate_with_flags; > drivers/pwm/pwm-rockchip.c:269: pc->chip.of_xlate =3D of_pwm= _xlate_with_flags; > drivers/pwm/pwm-samsung.c:517: chip->chip.of_xlate =3D of_p= wm_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_w= ith_flags; > drivers/pwm/pwm-tiehrpwm.c:460: pc->chip.of_xlate =3D of_pwm_xlate_w= ith_flags; > drivers/pwm/pwm-vt8500.c:219: chip->chip.of_xlate =3D of_pwm_xlate= _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 driver > 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 the > one in arch/arm/boot/dts/imx6qdl.dtsi (or something similar). That no= de > 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, but= I > think it might be worth correcting the DTS. There are a bunch of othe= r > DTS files that contain the same mistake. > > If that doesn't help, perhaps you can attach dmesg output to help deb= ug > this further? The PWM core is fairly silent, though, so you might nee= d > 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 callbac= ks > 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 effect= , and yes it uses pwm_simple_xlate of course. The polarity is ok, but something brok= e with this patch, looks like the pwm hw state is stuck to zero even when modified thru /s= ys/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 th= is patch.