diff mbox series

[v3] backlight: lp855x: Switch to atomic PWM API

Message ID YX1KCclMB/HTZ22c@fedora (mailing list archive)
State New, archived
Headers show
Series [v3] backlight: lp855x: Switch to atomic PWM API | expand

Commit Message

Maíra Canal Oct. 30, 2021, 1:35 p.m. UTC 
Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
replace it for the atomic PWM API.

Signed-off-by: Maíra Canal <maira.canal@usp.br>
---
V1 -> V2: Initialize variable and simply conditional loop
V2 -> V3: Fix assignment of NULL variable
---
 drivers/video/backlight/lp855x_bl.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Comments

kernel test robot Oct. 30, 2021, 11:43 p.m. UTC | #1 
Hi "Maíra,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on lee-backlight/for-backlight-next]
[also build test ERROR on v5.15-rc7 next-20211029]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ma-ra-Canal/backlight-lp855x-Switch-to-atomic-PWM-API/20211030-213551
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
config: i386-randconfig-a002-20211031 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/f4d53f4d51c78636fc4fd34aecbdcbd1f83f656e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ma-ra-Canal/backlight-lp855x-Switch-to-atomic-PWM-API/20211030-213551
        git checkout f4d53f4d51c78636fc4fd34aecbdcbd1f83f656e
        # save the attached .config to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: drivers/video/backlight/lp855x_bl.o: in function `lp855x_pwm_ctrl':
>> drivers/video/backlight/lp855x_bl.c:253: undefined reference to `__udivdi3'


vim +253 drivers/video/backlight/lp855x_bl.c

   233	
   234	static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
   235	{
   236		struct pwm_device *pwm = NULL;
   237		struct pwm_state state;
   238	
   239		/* request pwm device with the consumer name */
   240		if (!lp->pwm) {
   241			pwm = devm_pwm_get(lp->dev, lp->chipname);
   242			if (IS_ERR(pwm))
   243				return;
   244	
   245			lp->pwm = pwm;
   246	
   247			pwm_init_state(lp->pwm, &state);
   248			state.period = lp->pdata->period_ns;
   249		}
   250	
   251		pwm_get_state(lp->pwm, &state);
   252	
 > 253		state.duty_cycle = br * state.period / max_br;
   254		state.enabled = state.duty_cycle;
   255	
   256		pwm_apply_state(lp->pwm, &state);
   257	}
   258	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Oct. 31, 2021, 3:20 a.m. UTC | #2 
Hi "Maíra,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on lee-backlight/for-backlight-next]
[also build test ERROR on v5.15-rc7 next-20211029]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ma-ra-Canal/backlight-lp855x-Switch-to-atomic-PWM-API/20211030-213551
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
config: m68k-randconfig-r005-20211028 (attached as .config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f4d53f4d51c78636fc4fd34aecbdcbd1f83f656e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ma-ra-Canal/backlight-lp855x-Switch-to-atomic-PWM-API/20211030-213551
        git checkout f4d53f4d51c78636fc4fd34aecbdcbd1f83f656e
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   m68k-linux-ld: arch/m68k/kernel/machine_kexec.o: in function `machine_kexec':
   machine_kexec.c:(.text+0x154): undefined reference to `m68k_mmutype'
   m68k-linux-ld: machine_kexec.c:(.text+0x15c): undefined reference to `m68k_cputype'
   m68k-linux-ld: arch/m68k/kernel/relocate_kernel.o:(.m68k_fixup+0x0): undefined reference to `M68K_FIXUP_MEMOFFSET'
   m68k-linux-ld: arch/m68k/kernel/relocate_kernel.o:(.m68k_fixup+0x8): undefined reference to `M68K_FIXUP_MEMOFFSET'
   m68k-linux-ld: arch/m68k/kernel/uboot.o: in function `process_uboot_commandline':
   uboot.c:(.init.text+0x2e): undefined reference to `_init_sp'
   m68k-linux-ld: drivers/video/backlight/lp855x_bl.o: in function `lp855x_bl_update_status':
>> lp855x_bl.c:(.text+0x15c): undefined reference to `__udivdi3'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Geert Uytterhoeven Nov. 2, 2021, 8:39 a.m. UTC | #3 
Hi Maíra,

On Sat, Oct 30, 2021 at 3:35 PM Maíra Canal <maira.canal@usp.br> wrote:
> Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
> replace it for the atomic PWM API.
>
> Signed-off-by: Maíra Canal <maira.canal@usp.br>

Thanks for your patch!

> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -233,9 +233,8 @@ static int lp855x_configure(struct lp855x *lp)
>
>  static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
>  {
> -       unsigned int period = lp->pdata->period_ns;
> -       unsigned int duty = br * period / max_br;
> -       struct pwm_device *pwm;
> +       struct pwm_device *pwm = NULL;
> +       struct pwm_state state;
>
>         /* request pwm device with the consumer name */
>         if (!lp->pwm) {
> @@ -245,18 +244,16 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
>
>                 lp->pwm = pwm;
>
> -               /*
> -                * FIXME: pwm_apply_args() should be removed when switching to
> -                * the atomic PWM API.
> -                */
> -               pwm_apply_args(pwm);
> +               pwm_init_state(lp->pwm, &state);
> +               state.period = lp->pdata->period_ns;
>         }
>
> -       pwm_config(lp->pwm, duty, period);
> -       if (duty)
> -               pwm_enable(lp->pwm);
> -       else
> -               pwm_disable(lp->pwm);
> +       pwm_get_state(lp->pwm, &state);
> +
> +       state.duty_cycle = br * state.period / max_br;

Above is a 64-by-32 division, which should not be open-coded, as
that will cause link failures on 32-bit platform (cfr. "undefined
reference to `__udivdi3'", as reported by the kernel test robot).
Please use div_u64() instead.

> +       state.enabled = state.duty_cycle;
> +
> +       pwm_apply_state(lp->pwm, &state);
>  }
>
>  static int lp855x_bl_update_status(struct backlight_device *bl)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Maíra Canal Nov. 3, 2021, 12:32 p.m. UTC | #4 
Em ter., 2 de nov. de 2021 às 05:39, Geert Uytterhoeven
<geert@linux-m68k.org> escreveu:
>
> Hi Maíra,
>
> On Sat, Oct 30, 2021 at 3:35 PM Maíra Canal <maira.canal@usp.br> wrote:
> > Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
> > replace it for the atomic PWM API.
> >
> > Signed-off-by: Maíra Canal <maira.canal@usp.br>
>
> Thanks for your patch!
>
> > --- a/drivers/video/backlight/lp855x_bl.c
> > +++ b/drivers/video/backlight/lp855x_bl.c
> > @@ -233,9 +233,8 @@ static int lp855x_configure(struct lp855x *lp)
> >
> >  static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
> >  {
> > -       unsigned int period = lp->pdata->period_ns;
> > -       unsigned int duty = br * period / max_br;
> > -       struct pwm_device *pwm;
> > +       struct pwm_device *pwm = NULL;
> > +       struct pwm_state state;
> >
> >         /* request pwm device with the consumer name */
> >         if (!lp->pwm) {
> > @@ -245,18 +244,16 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
> >
> >                 lp->pwm = pwm;
> >
> > -               /*
> > -                * FIXME: pwm_apply_args() should be removed when switching to
> > -                * the atomic PWM API.
> > -                */
> > -               pwm_apply_args(pwm);
> > +               pwm_init_state(lp->pwm, &state);
> > +               state.period = lp->pdata->period_ns;
> >         }
> >
> > -       pwm_config(lp->pwm, duty, period);
> > -       if (duty)
> > -               pwm_enable(lp->pwm);
> > -       else
> > -               pwm_disable(lp->pwm);
> > +       pwm_get_state(lp->pwm, &state);
> > +
> > +       state.duty_cycle = br * state.period / max_br;
>
> Above is a 64-by-32 division, which should not be open-coded, as
> that will cause link failures on 32-bit platform (cfr. "undefined
> reference to `__udivdi3'", as reported by the kernel test robot).
> Please use div_u64() instead.

Hi Geert,

Thank you for the suggestion! I fixed this problem a bit differently
and submitted the v4. I made use of the PWM API and applied the
pwm_set_relative_duty_cycle function, which solved this division
problem.

>
> > +       state.enabled = state.duty_cycle;
> > +
> > +       pwm_apply_state(lp->pwm, &state);
> >  }
> >
> >  static int lp855x_bl_update_status(struct backlight_device *bl)
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
diff mbox series

Patch

diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index e94932c69f54..627a31d05691 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -233,9 +233,8 @@  static int lp855x_configure(struct lp855x *lp)
 
 static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 {
-	unsigned int period = lp->pdata->period_ns;
-	unsigned int duty = br * period / max_br;
-	struct pwm_device *pwm;
+	struct pwm_device *pwm = NULL;
+	struct pwm_state state;
 
 	/* request pwm device with the consumer name */
 	if (!lp->pwm) {
@@ -245,18 +244,16 @@  static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 
 		lp->pwm = pwm;
 
-		/*
-		 * FIXME: pwm_apply_args() should be removed when switching to
-		 * the atomic PWM API.
-		 */
-		pwm_apply_args(pwm);
+		pwm_init_state(lp->pwm, &state);
+		state.period = lp->pdata->period_ns;
 	}
 
-	pwm_config(lp->pwm, duty, period);
-	if (duty)
-		pwm_enable(lp->pwm);
-	else
-		pwm_disable(lp->pwm);
+	pwm_get_state(lp->pwm, &state);
+
+	state.duty_cycle = br * state.period / max_br;
+	state.enabled = state.duty_cycle;
+
+	pwm_apply_state(lp->pwm, &state);
 }
 
 static int lp855x_bl_update_status(struct backlight_device *bl)