Re: pwm: Introduce the pwm_args concept regression

[Thierry Reding <thierry.reding@gmail.com>]

[-- 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 --]