* 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).