From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: pwm: Introduce the pwm_args concept regression Date: Tue, 17 May 2016 14:05:23 +0200 Message-ID: <20160517120523.GC26166@ulmo.ba.sec> References: <573AAEAF.5040603@nextfour.com> <20160517103151.54d1b413@bbrezillon> <573AE8E3.2070500@nextfour.com> <20160517121632.102f7bbd@bbrezillon> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="QRj9sO5tAVLaXnSD" Return-path: Received: from mail-wm0-f65.google.com ([74.125.82.65]:34191 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932154AbcEQMF1 (ORCPT ); Tue, 17 May 2016 08:05:27 -0400 Received: by mail-wm0-f65.google.com with SMTP id n129so4227917wmn.1 for ; Tue, 17 May 2016 05:05:27 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20160517121632.102f7bbd@bbrezillon> Sender: linux-pwm-owner@vger.kernel.org List-Id: linux-pwm@vger.kernel.org To: Boris Brezillon Cc: Mika =?utf-8?B?UGVudHRpbMOk?= , jingoohan1@gmail.com, linux-pwm@vger.kernel.org --QRj9sO5tAVLaXnSD Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > > On 05/17/2016 11:31 AM, Boris Brezillon wrote: > > > +Thierry > > >=20 > > > Hi Mika, > > >=20 > > > 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 a= ffects all pwm backlight setups. > > >> With this reverted, everything works as normal. =20 > > >=20 > > > Hm, just tried using a PWM backlight on an atmel board (at91sama5d4ek= ), > > > and after applying the following patch, it worked correctly (that's a= ctually > > > a bug in the PWM driver: the HLCDC PWM device is configured with inve= rsed > > > polarity by default, and the driver was not taking that into account). > > > I doubt you have the same problem here since your PWM device is not > > > supporting polarity configuration. > > >=20 > > > Could you tell me more about this bug? What are the symptoms? Which > > > board are you testing on (if it's not mainlined, can you share your > > > DT)? > > >=20 > > > Thanks, > > >=20 > > > Boris > > >=20 > > > --- > > > diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hl= cdc.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 platform_= 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_POLARITY_I= NVERSED); > > > if (ret) { > > > clk_disable_unprepare(hlcdc->periph_clk); > > > return ret; > > > =20 > >=20 > > Symptom is entirely dark screen. > > The board is not mainlined but the backlight part is defined like :=20 > >=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 between > > * 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>; > > }; > >=20 > >=20 > > so the polarity is in there defined. > > The call call graphs is like : devm_pwm_get() -> pwm_get() -> of_pwm_ge= t()=20 > > -> of_pwm_xlate_with_flags which used to call pwm_set_period and pwm_se= t_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 only 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_pwm_xla= te_with_flags; drivers/pwm/pwm-atmel-tcb.c:397: tcbpwm->chip.of_xlate =3D of_pwm_x= late_with_flags; drivers/pwm/pwm-atmel.c:378: atmel_pwm->chip.of_xlate =3D of_pw= m_xlate_with_flags; drivers/pwm/pwm-bcm-kona.c:277: kp->chip.of_xlate =3D of_pwm_xlate_with_fl= ags; drivers/pwm/pwm-berlin.c:181: pwm->chip.of_xlate =3D of_pwm_xlate_with_f= lags; drivers/pwm/pwm-fsl-ftm.c:445: fpc->chip.of_xlate =3D of_pwm_xlate_with_f= lags; 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_xla= te_with_flags; drivers/pwm/pwm-renesas-tpu.c:421: tpu->chip.of_xlate =3D of_pwm_xlat= e_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_pwm_xla= te_with_flags; drivers/pwm/pwm-sun4i.c:335: pwm->chip.of_xlate =3D of_pwm_xlate_with_f= lags; drivers/pwm/pwm-tiecap.c:229: pc->chip.of_xlate =3D of_pwm_xlate_with_fl= ags; drivers/pwm/pwm-tiehrpwm.c:460: pc->chip.of_xlate =3D of_pwm_xlate_with_fl= ags; 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 node 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 other DTS files that contain the same mistake. If that doesn't help, perhaps you can attach dmesg output to help debug this further? The PWM core is fairly silent, though, so you might need 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 callbacks get called in the sequence that they should (and with the expected parameters). Let me know if you need help with that. Thierry --QRj9sO5tAVLaXnSD Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJXOwkBAAoJEN0jrNd/PrOh7DYP/juhuUoixgHDQbNidimT1Zl0 rh20AYos9MKavIZwpUjwFNDIydGQrrgNlWSklUl7uxjAcnWNVzC3ZSOQTF0OLldn yKVEga9uVIJr/erKIQyuV/5J7lp8J5Bi0JxSxv/maDA5XqqT8L56ai8505r4P1Bp kn083AfJmUacJ2Ko7zVbJvGlOLPAKpe5ZVW8BxGsBe9AZlpCi2VuZZA5C67eH56M b9Drdm51kRWtYLh9opnQrQRvctQzh/fvirIQdsyerNxfdLRhGCxRISPISf3z1rA4 WwrNMKT2qWdHoLXhytrZ5hm5yOUal1/GOxCyqJg3ue26U9W9vnhBGoEs7oWhr9e6 JNF0Xltjq5+qKM0J0ScI9sPnlEbZO8DPUeHfrr4U56LtroPVFdSNB2G5N7nFcCHp iawaKsk6sy4dNYkSUghItQZzoTPJDeqVlvPIlnY0CZzsUQHtc4N1QhDhHjW0ZQeB 3Hcuw6rknz16TvoZpzcLq571lIIKH5sXusvoYYvxJ57f/hhyVDj8AzkUnyb5OOVP TokXF20CADORiagcvMW30Vr56tV+8bcGWTbIjLcZVScjASBZvzFIQKLhG2wzjh9Q o87nwxQ+Ov14UJt9wp+oCSo0VMvbsncdSh2vgOuzse6EbHPws8Ay6LmlMFMxeOn2 mLKm36dNTJ0FXHgbgPyu =OoAw -----END PGP SIGNATURE----- --QRj9sO5tAVLaXnSD--