Re: pwm: Introduce the pwm_args concept regression

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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