* Re: pwm: Introduce the pwm_args concept regression [not found] ` <20160517121632.102f7bbd@bbrezillon> @ 2016-05-17 12:05 ` Thierry Reding 2016-05-17 13:15 ` Mika Penttilä 0 siblings, 1 reply; 7+ messages in thread From: Thierry Reding @ 2016-05-17 12:05 UTC (permalink / raw) To: Boris Brezillon; +Cc: Mika Penttilä, jingoohan1, linux-pwm [-- Attachment #1: Type: text/plain, Size: 6180 bytes --] On Tue, May 17, 2016 at 12:16:32PM +0200, Boris Brezillon wrote: > On Tue, 17 May 2016 12:48:19 +0300 > Mika Penttilä <mika.penttila@nextfour.com> 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ä <mika.penttila@nextfour.com> wrote: > > > > > >> 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. > > > > > > Hm, just tried using a PWM backlight on an atmel board (at91sama5d4ek), > > > 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 account). > > > I doubt you have the same problem here since your PWM device is not > > > supporting polarity configuration. > > > > > > 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)? > > > > > > 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 platform_device *pdev) > > > chip->chip.of_pwm_n_cells = 3; > > > chip->chip.can_sleep = 1; > > > > > > - ret = pwmchip_add(&chip->chip); > > > + ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED); > > > if (ret) { > > > clk_disable_unprepare(hlcdc->periph_clk); > > > return ret; > > > > > > > Symptom is entirely dark screen. > > The board is not mainlined but the backlight part is defined like : > > > > backlight: backlight { > > compatible = "pwm-backlight"; > > pwms = <&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 = < 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>; > > default-brightness-level = <50>; > > }; > > > > > > so the polarity is in there defined. > > The call call graphs is like : devm_pwm_get() -> pwm_get() -> of_pwm_get() > > -> of_pwm_xlate_with_flags which used to call pwm_set_period and pwm_set_polarity but now > > 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 '=.*of_pwm_xlate_with_flags' -- drivers/pwm/ drivers/pwm/pwm-atmel-hlcdc.c:271: chip->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-atmel-tcb.c:397: tcbpwm->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-atmel.c:378: atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-bcm-kona.c:277: kp->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-berlin.c:181: pwm->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-fsl-ftm.c:445: fpc->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-lpc18xx-sct.c:383: lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-omap-dmtimer.c:318: omap->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-renesas-tpu.c:421: tpu->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-rockchip.c:269: pc->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-samsung.c:517: chip->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-sun4i.c:335: pwm->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-tiecap.c:229: pc->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-tiehrpwm.c:460: pc->chip.of_xlate = of_pwm_xlate_with_flags; drivers/pwm/pwm-vt8500.c:219: chip->chip.of_xlate = 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 = <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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pwm: Introduce the pwm_args concept regression 2016-05-17 12:05 ` pwm: Introduce the pwm_args concept regression Thierry Reding @ 2016-05-17 13:15 ` Mika Penttilä 2016-05-17 13:44 ` Mika Penttilä 0 siblings, 1 reply; 7+ messages in thread From: Mika Penttilä @ 2016-05-17 13:15 UTC (permalink / raw) To: Thierry Reding, Boris Brezillon; +Cc: jingoohan1, linux-pwm 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ä <mika.penttila@nextfour.com> 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ä <mika.penttila@nextfour.com> wrote: >>>> >>>>> 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. >>>> Hm, just tried using a PWM backlight on an atmel board (at91sama5d4ek), >>>> 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 account). >>>> I doubt you have the same problem here since your PWM device is not >>>> supporting polarity configuration. >>>> >>>> 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)? >>>> >>>> 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 platform_device *pdev) >>>> chip->chip.of_pwm_n_cells = 3; >>>> chip->chip.can_sleep = 1; >>>> >>>> - ret = pwmchip_add(&chip->chip); >>>> + ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED); >>>> if (ret) { >>>> clk_disable_unprepare(hlcdc->periph_clk); >>>> return ret; >>>> >>> Symptom is entirely dark screen. >>> The board is not mainlined but the backlight part is defined like : >>> >>> backlight: backlight { >>> compatible = "pwm-backlight"; >>> pwms = <&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 = < 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>; >>> default-brightness-level = <50>; >>> }; >>> >>> >>> so the polarity is in there defined. >>> The call call graphs is like : devm_pwm_get() -> pwm_get() -> of_pwm_get() >>> -> of_pwm_xlate_with_flags which used to call pwm_set_period and pwm_set_polarity but now >>> 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 '=.*of_pwm_xlate_with_flags' -- drivers/pwm/ > drivers/pwm/pwm-atmel-hlcdc.c:271: chip->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-atmel-tcb.c:397: tcbpwm->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-atmel.c:378: atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-bcm-kona.c:277: kp->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-berlin.c:181: pwm->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-fsl-ftm.c:445: fpc->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-lpc18xx-sct.c:383: lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-omap-dmtimer.c:318: omap->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-renesas-tpu.c:421: tpu->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-rockchip.c:269: pc->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-samsung.c:517: chip->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-sun4i.c:335: pwm->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-tiecap.c:229: pc->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-tiehrpwm.c:460: pc->chip.of_xlate = of_pwm_xlate_with_flags; > drivers/pwm/pwm-vt8500.c:219: chip->chip.of_xlate = 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 = <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 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 broke 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pwm: Introduce the pwm_args concept regression 2016-05-17 13:15 ` Mika Penttilä @ 2016-05-17 13:44 ` Mika Penttilä 2016-05-17 14:17 ` Boris Brezillon 0 siblings, 1 reply; 7+ messages in thread From: Mika Penttilä @ 2016-05-17 13:44 UTC (permalink / raw) To: Thierry Reding, Boris Brezillon; +Cc: jingoohan1, linux-pwm On 17.05.2016 16:15, Mika Penttilä 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ä <mika.penttila@nextfour.com> 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ä <mika.penttila@nextfour.com> wrote: >>>>> >>>>>> 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. >>>>> Hm, just tried using a PWM backlight on an atmel board (at91sama5d4ek), >>>>> 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 account). >>>>> I doubt you have the same problem here since your PWM device is not >>>>> supporting polarity configuration. >>>>> >>>>> 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)? >>>>> >>>>> 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 platform_device *pdev) >>>>> chip->chip.of_pwm_n_cells = 3; >>>>> chip->chip.can_sleep = 1; >>>>> >>>>> - ret = pwmchip_add(&chip->chip); >>>>> + ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED); >>>>> if (ret) { >>>>> clk_disable_unprepare(hlcdc->periph_clk); >>>>> return ret; >>>>> >>>> Symptom is entirely dark screen. >>>> The board is not mainlined but the backlight part is defined like : >>>> >>>> backlight: backlight { >>>> compatible = "pwm-backlight"; >>>> pwms = <&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 = < 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>; >>>> default-brightness-level = <50>; >>>> }; >>>> >>>> >>>> so the polarity is in there defined. >>>> The call call graphs is like : devm_pwm_get() -> pwm_get() -> of_pwm_get() >>>> -> of_pwm_xlate_with_flags which used to call pwm_set_period and pwm_set_polarity but now >>>> 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 '=.*of_pwm_xlate_with_flags' -- drivers/pwm/ >> drivers/pwm/pwm-atmel-hlcdc.c:271: chip->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-atmel-tcb.c:397: tcbpwm->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-atmel.c:378: atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-bcm-kona.c:277: kp->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-berlin.c:181: pwm->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-fsl-ftm.c:445: fpc->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-lpc18xx-sct.c:383: lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-omap-dmtimer.c:318: omap->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-renesas-tpu.c:421: tpu->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-rockchip.c:269: pc->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-samsung.c:517: chip->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-sun4i.c:335: pwm->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-tiecap.c:229: pc->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-tiehrpwm.c:460: pc->chip.of_xlate = of_pwm_xlate_with_flags; >> drivers/pwm/pwm-vt8500.c:219: chip->chip.of_xlate = 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 = <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 > 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 broke 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pwm: Introduce the pwm_args concept regression 2016-05-17 13:44 ` Mika Penttilä @ 2016-05-17 14:17 ` Boris Brezillon 2016-05-17 14:55 ` Mika Penttilä 0 siblings, 1 reply; 7+ messages in thread From: Boris Brezillon @ 2016-05-17 14:17 UTC (permalink / raw) To: Mika Penttilä; +Cc: Thierry Reding, jingoohan1, linux-pwm On Tue, 17 May 2016 16:44:05 +0300 Mika Penttilä <mika.penttila@nextfour.com> wrote: > On 17.05.2016 16:15, Mika Penttilä 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ä <mika.penttila@nextfour.com> 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ä <mika.penttila@nextfour.com> wrote: > >>>>> > >>>>>> 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. > >>>>> Hm, just tried using a PWM backlight on an atmel board (at91sama5d4ek), > >>>>> 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 account). > >>>>> I doubt you have the same problem here since your PWM device is not > >>>>> supporting polarity configuration. > >>>>> > >>>>> 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)? > >>>>> > >>>>> 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 platform_device *pdev) > >>>>> chip->chip.of_pwm_n_cells = 3; > >>>>> chip->chip.can_sleep = 1; > >>>>> > >>>>> - ret = pwmchip_add(&chip->chip); > >>>>> + ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED); > >>>>> if (ret) { > >>>>> clk_disable_unprepare(hlcdc->periph_clk); > >>>>> return ret; > >>>>> > >>>> Symptom is entirely dark screen. > >>>> The board is not mainlined but the backlight part is defined like : > >>>> > >>>> backlight: backlight { > >>>> compatible = "pwm-backlight"; > >>>> pwms = <&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 = < 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>; > >>>> default-brightness-level = <50>; > >>>> }; > >>>> > >>>> > >>>> so the polarity is in there defined. > >>>> The call call graphs is like : devm_pwm_get() -> pwm_get() -> of_pwm_get() > >>>> -> of_pwm_xlate_with_flags which used to call pwm_set_period and pwm_set_polarity but now > >>>> 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 '=.*of_pwm_xlate_with_flags' -- drivers/pwm/ > >> drivers/pwm/pwm-atmel-hlcdc.c:271: chip->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-atmel-tcb.c:397: tcbpwm->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-atmel.c:378: atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-bcm-kona.c:277: kp->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-berlin.c:181: pwm->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-fsl-ftm.c:445: fpc->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-lpc18xx-sct.c:383: lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-omap-dmtimer.c:318: omap->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-renesas-tpu.c:421: tpu->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-rockchip.c:269: pc->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-samsung.c:517: chip->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-sun4i.c:335: pwm->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-tiecap.c:229: pc->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-tiehrpwm.c:460: pc->chip.of_xlate = of_pwm_xlate_with_flags; > >> drivers/pwm/pwm-vt8500.c:219: chip->chip.of_xlate = 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 = <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 > > 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 broke 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. Are you still testing on next or did you revert a few patches (like this one 3a0cfa4 "backlight: pwm_bl: Use pwm_get_args() where appropriate")? Maybe that would be easier if you could share the code you're testing with, can you push a branch somewhere? -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pwm: Introduce the pwm_args concept regression 2016-05-17 14:17 ` Boris Brezillon @ 2016-05-17 14:55 ` Mika Penttilä 2016-05-17 15:17 ` Boris Brezillon 0 siblings, 1 reply; 7+ messages in thread From: Mika Penttilä @ 2016-05-17 14:55 UTC (permalink / raw) To: Boris Brezillon; +Cc: Thierry Reding, jingoohan1, linux-pwm On 17.05.2016 17:17, Boris Brezillon wrote: > On Tue, 17 May 2016 16:44:05 +0300 > Mika Penttilä <mika.penttila@nextfour.com> wrote: > >> On 17.05.2016 16:15, Mika Penttilä 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ä <mika.penttila@nextfour.com> 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ä <mika.penttila@nextfour.com> wrote: >>>>>>> >>>>>>>> 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. >>>>>>> Hm, just tried using a PWM backlight on an atmel board (at91sama5d4ek), >>>>>>> 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 account). >>>>>>> I doubt you have the same problem here since your PWM device is not >>>>>>> supporting polarity configuration. >>>>>>> >>>>>>> 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)? >>>>>>> >>>>>>> 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 platform_device *pdev) >>>>>>> chip->chip.of_pwm_n_cells = 3; >>>>>>> chip->chip.can_sleep = 1; >>>>>>> >>>>>>> - ret = pwmchip_add(&chip->chip); >>>>>>> + ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED); >>>>>>> if (ret) { >>>>>>> clk_disable_unprepare(hlcdc->periph_clk); >>>>>>> return ret; >>>>>>> >>>>>> Symptom is entirely dark screen. >>>>>> The board is not mainlined but the backlight part is defined like : >>>>>> >>>>>> backlight: backlight { >>>>>> compatible = "pwm-backlight"; >>>>>> pwms = <&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 = < 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>; >>>>>> default-brightness-level = <50>; >>>>>> }; >>>>>> >>>>>> >>>>>> so the polarity is in there defined. >>>>>> The call call graphs is like : devm_pwm_get() -> pwm_get() -> of_pwm_get() >>>>>> -> of_pwm_xlate_with_flags which used to call pwm_set_period and pwm_set_polarity but now >>>>>> 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 '=.*of_pwm_xlate_with_flags' -- drivers/pwm/ >>>> drivers/pwm/pwm-atmel-hlcdc.c:271: chip->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-atmel-tcb.c:397: tcbpwm->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-atmel.c:378: atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-bcm-kona.c:277: kp->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-berlin.c:181: pwm->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-fsl-ftm.c:445: fpc->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-lpc18xx-sct.c:383: lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-omap-dmtimer.c:318: omap->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-renesas-tpu.c:421: tpu->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-rockchip.c:269: pc->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-samsung.c:517: chip->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-sun4i.c:335: pwm->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-tiecap.c:229: pc->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-tiehrpwm.c:460: pc->chip.of_xlate = of_pwm_xlate_with_flags; >>>> drivers/pwm/pwm-vt8500.c:219: chip->chip.of_xlate = 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 = <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 >>> 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 broke 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. > Are you still testing on next or did you revert a few patches (like > this one 3a0cfa4 "backlight: pwm_bl: Use pwm_get_args() where > appropriate")? > Maybe that would be easier if you could share the code you're testing > with, can you push a branch somewhere? > Hmm I am testing against linus git (4.6.0+) and it doesnt have the "backlight: pwm_bl: Use pwm_get_args()" yet pulled ? Maybe that explains... It has "pwm: Introduce the pwm_args concept" so the tree is just broken atm without the other parts? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pwm: Introduce the pwm_args concept regression 2016-05-17 14:55 ` Mika Penttilä @ 2016-05-17 15:17 ` Boris Brezillon 2016-05-18 5:40 ` Mika Penttilä 0 siblings, 1 reply; 7+ messages in thread From: Boris Brezillon @ 2016-05-17 15:17 UTC (permalink / raw) To: Mika Penttilä, Thierry Reding, jingoohan1, linux-pwm On Tue, 17 May 2016 17:55:59 +0300 Mika Penttilä <mika.penttila@nextfour.com> wrote: > On 17.05.2016 17:17, Boris Brezillon wrote: > > On Tue, 17 May 2016 16:44:05 +0300 > > Mika Penttilä <mika.penttila@nextfour.com> wrote: > > > >> On 17.05.2016 16:15, Mika Penttilä 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ä <mika.penttila@nextfour.com> 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ä <mika.penttila@nextfour.com> wrote: > >>>>>>> > >>>>>>>> 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. > >>>>>>> Hm, just tried using a PWM backlight on an atmel board (at91sama5d4ek), > >>>>>>> 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 account). > >>>>>>> I doubt you have the same problem here since your PWM device is not > >>>>>>> supporting polarity configuration. > >>>>>>> > >>>>>>> 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)? > >>>>>>> > >>>>>>> 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 platform_device *pdev) > >>>>>>> chip->chip.of_pwm_n_cells = 3; > >>>>>>> chip->chip.can_sleep = 1; > >>>>>>> > >>>>>>> - ret = pwmchip_add(&chip->chip); > >>>>>>> + ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED); > >>>>>>> if (ret) { > >>>>>>> clk_disable_unprepare(hlcdc->periph_clk); > >>>>>>> return ret; > >>>>>>> > >>>>>> Symptom is entirely dark screen. > >>>>>> The board is not mainlined but the backlight part is defined like : > >>>>>> > >>>>>> backlight: backlight { > >>>>>> compatible = "pwm-backlight"; > >>>>>> pwms = <&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 = < 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>; > >>>>>> default-brightness-level = <50>; > >>>>>> }; > >>>>>> > >>>>>> > >>>>>> so the polarity is in there defined. > >>>>>> The call call graphs is like : devm_pwm_get() -> pwm_get() -> of_pwm_get() > >>>>>> -> of_pwm_xlate_with_flags which used to call pwm_set_period and pwm_set_polarity but now > >>>>>> 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 '=.*of_pwm_xlate_with_flags' -- drivers/pwm/ > >>>> drivers/pwm/pwm-atmel-hlcdc.c:271: chip->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-atmel-tcb.c:397: tcbpwm->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-atmel.c:378: atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-bcm-kona.c:277: kp->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-berlin.c:181: pwm->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-fsl-ftm.c:445: fpc->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-lpc18xx-sct.c:383: lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-omap-dmtimer.c:318: omap->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-renesas-tpu.c:421: tpu->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-rockchip.c:269: pc->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-samsung.c:517: chip->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-sun4i.c:335: pwm->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-tiecap.c:229: pc->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-tiehrpwm.c:460: pc->chip.of_xlate = of_pwm_xlate_with_flags; > >>>> drivers/pwm/pwm-vt8500.c:219: chip->chip.of_xlate = 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 = <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 > >>> 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 broke 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. > > Are you still testing on next or did you revert a few patches (like > > this one 3a0cfa4 "backlight: pwm_bl: Use pwm_get_args() where > > appropriate")? > > Maybe that would be easier if you could share the code you're testing > > with, can you push a branch somewhere? > > > Hmm I am testing against linus git (4.6.0+) and it doesnt have the "backlight: pwm_bl: Use pwm_get_args()" yet pulled ? Maybe that explains... > It has "pwm: Introduce the pwm_args concept" so the tree is just broken atm without the other parts? Yep, so that might explain. Today discovered that this commit was introducing a bug that was later fixed by other commits, so when you test the whole patchset it work. Can you try to manually apply this fix [1]? [1]http://www.spinics.net/lists/arm-kernel/msg504856.html -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pwm: Introduce the pwm_args concept regression 2016-05-17 15:17 ` Boris Brezillon @ 2016-05-18 5:40 ` Mika Penttilä 0 siblings, 0 replies; 7+ messages in thread From: Mika Penttilä @ 2016-05-18 5:40 UTC (permalink / raw) To: Boris Brezillon, Thierry Reding, jingoohan1, linux-pwm On 05/17/2016 06:17 PM, Boris Brezillon wrote: > On Tue, 17 May 2016 17:55:59 +0300 > Mika Penttilä <mika.penttila@nextfour.com> wrote: > >> On 17.05.2016 17:17, Boris Brezillon wrote: >>> On Tue, 17 May 2016 16:44:05 +0300 >>> Mika Penttilä <mika.penttila@nextfour.com> wrote: >>> >>>> On 17.05.2016 16:15, Mika Penttilä 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ä <mika.penttila@nextfour.com> 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ä <mika.penttila@nextfour.com> wrote: >>>>>>>>> >>>>>>>>>> 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. >>>>>>>>> Hm, just tried using a PWM backlight on an atmel board (at91sama5d4ek), >>>>>>>>> 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 account). >>>>>>>>> I doubt you have the same problem here since your PWM device is not >>>>>>>>> supporting polarity configuration. >>>>>>>>> >>>>>>>>> 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)? >>>>>>>>> >>>>>>>>> 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 platform_device *pdev) >>>>>>>>> chip->chip.of_pwm_n_cells = 3; >>>>>>>>> chip->chip.can_sleep = 1; >>>>>>>>> >>>>>>>>> - ret = pwmchip_add(&chip->chip); >>>>>>>>> + ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED); >>>>>>>>> if (ret) { >>>>>>>>> clk_disable_unprepare(hlcdc->periph_clk); >>>>>>>>> return ret; >>>>>>>>> >>>>>>>> Symptom is entirely dark screen. >>>>>>>> The board is not mainlined but the backlight part is defined like : >>>>>>>> >>>>>>>> backlight: backlight { >>>>>>>> compatible = "pwm-backlight"; >>>>>>>> pwms = <&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 = < 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>; >>>>>>>> default-brightness-level = <50>; >>>>>>>> }; >>>>>>>> >>>>>>>> >>>>>>>> so the polarity is in there defined. >>>>>>>> The call call graphs is like : devm_pwm_get() -> pwm_get() -> of_pwm_get() >>>>>>>> -> of_pwm_xlate_with_flags which used to call pwm_set_period and pwm_set_polarity but now >>>>>>>> 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 '=.*of_pwm_xlate_with_flags' -- drivers/pwm/ >>>>>> drivers/pwm/pwm-atmel-hlcdc.c:271: chip->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-atmel-tcb.c:397: tcbpwm->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-atmel.c:378: atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-bcm-kona.c:277: kp->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-berlin.c:181: pwm->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-fsl-ftm.c:445: fpc->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-lpc18xx-sct.c:383: lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-omap-dmtimer.c:318: omap->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-renesas-tpu.c:421: tpu->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-rockchip.c:269: pc->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-samsung.c:517: chip->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-sun4i.c:335: pwm->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-tiecap.c:229: pc->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-tiehrpwm.c:460: pc->chip.of_xlate = of_pwm_xlate_with_flags; >>>>>> drivers/pwm/pwm-vt8500.c:219: chip->chip.of_xlate = 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 = <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 >>>>> 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 broke 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. >>> Are you still testing on next or did you revert a few patches (like >>> this one 3a0cfa4 "backlight: pwm_bl: Use pwm_get_args() where >>> appropriate")? >>> Maybe that would be easier if you could share the code you're testing >>> with, can you push a branch somewhere? >>> >> Hmm I am testing against linus git (4.6.0+) and it doesnt have the "backlight: pwm_bl: Use pwm_get_args()" yet pulled ? Maybe that explains... >> It has "pwm: Introduce the pwm_args concept" so the tree is just broken atm without the other parts? > > Yep, so that might explain. Today discovered that this commit was > introducing a bug that was later fixed by other commits, so when you > test the whole patchset it work. > > Can you try to manually apply this fix [1]? > > [1]http://www.spinics.net/lists/arm-kernel/msg504856.html > I can confirm that after manually applying the "backlight: pwm_bl: Use pwm_get_args()" backlight works ok now. So the linux-next should be ok as of today. Thanks, Mika ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-05-18 5:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <573AAEAF.5040603@nextfour.com>
[not found] ` <20160517103151.54d1b413@bbrezillon>
[not found] ` <573AE8E3.2070500@nextfour.com>
[not found] ` <20160517121632.102f7bbd@bbrezillon>
2016-05-17 12:05 ` pwm: Introduce the pwm_args concept regression Thierry Reding
2016-05-17 13:15 ` Mika Penttilä
2016-05-17 13:44 ` Mika Penttilä
2016-05-17 14:17 ` Boris Brezillon
2016-05-17 14:55 ` Mika Penttilä
2016-05-17 15:17 ` Boris Brezillon
2016-05-18 5:40 ` Mika Penttilä
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).