backlight: lp855x: Switch to atomic PWM API

PATCH Superseded Maíra Canal 3 participants 5 messages Oct 30, 2021 – Nov 3, 2021

Subsystems (3)

Framebuffer layer PWM Backlight class/subsystem

lore

Share & tools

lore

Maíra Canal

author

2021-10-30 13:35 UTC #0 lore raw

PATCH v3 1/1 backlight: lp855x: Switch to atomic PWM API

Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and replace it for the atomic PWM API.

Signed-off-by Maíra Canal <[email protected]>

---
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(-)

drivers/video/backlight/lp855x_bl.c +10 ~~-13~~

index e94932c69f54..627a31d05691 100644
@@ -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)

Signature · 1 line

2.31.1

kernel test robot

build bot

2021-10-30 23:43 UTC #1 lore raw

Thank you for the patch! Yet something to improve:

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

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(a)lists.01.org

kernel test robot

build bot

2021-10-31 03:20 UTC #2 lore raw

In reply to Maíra Canal

Thank you for the patch! Yet something to improve:

Hi "Maíra,

Thank you for the patch! Yet something to improve:

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

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(a)lists.01.org

Geert Uytterhoeven

2021-11-02 08:39 UTC #3 lore raw

In reply to Maíra Canal

Hi Maíra,

Quoted 4 lines · Maíra Canal

Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and replace it for the atomic PWM API.
Signed-off-by Maíra Canal <[email protected]>

Thanks for your patch!

Quoted 25 lines

drivers/video/backlight/lp855x_bl.c +4 -8

@@ -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;

drivers/video/backlight/lp855x_bl.c static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br) · line 253

Quoted 10 lines

drivers/video/backlight/lp855x_bl.c +3 -5

        }
 
-       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.

drivers/video/backlight/lp855x_bl.c static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br) · line 259

Quoted 6 lines

drivers/video/backlight/lp855x_bl.c +3 -0

+       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 -- [email protected]

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

author

2021-11-03 12:32 UTC #4 lore raw

Em ter., 2 de nov. de 2021 às 05:39, Geert Uytterhoeven <[email protected]> escreveu:

Quoted 38 lines

Hi Maíra,

Quoted 4 lines · Maíra Canal

Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and replace it for the atomic PWM API.
Signed-off-by Maíra Canal <[email protected]>

Thanks for your patch!

Quoted 27 lines

drivers/video/backlight/lp855x_bl.c +4 -8

@@ -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;
        }

Quoted 13 lines

drivers/video/backlight/lp855x_bl.c static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br) · line 253
Quoted 8 lines
drivers/video/backlight/lp855x_bl.c +3 -5
-       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.

Quoted 18 lines

drivers/video/backlight/lp855x_bl.c static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br) · line 259
Quoted 7 lines
drivers/video/backlight/lp855x_bl.c +3 -0
+       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 -- [email protected]
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