| Message ID | YXU2i0FtAGDRCMSu@fedora |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] media: rc: pwm-ir-tx: Switch to atomic PWM API | expand |
Hello, On Sun, Oct 24, 2021 at 07:33:47AM -0300, Maíra Canal 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> > --- > V1 -> V2: Assign variables directly and simplify conditional statement > V2 -> V3: Fix declaration of undeclared variable > --- > drivers/media/rc/pwm-ir-tx.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c > index 4bc28d2c9cc9..e1f348a962e8 100644 > --- a/drivers/media/rc/pwm-ir-tx.c > +++ b/drivers/media/rc/pwm-ir-tx.c > @@ -53,22 +53,21 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, > { > struct pwm_ir *pwm_ir = dev->priv; > struct pwm_device *pwm = pwm_ir->pwm; > - int i, duty, period; > + struct pwm_state state; > + int i; > ktime_t edge; > long delta; > > - period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier); > - duty = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * period, 100); > + pwm_init_state(pwm, &state); > > - pwm_config(pwm, duty, period); > + state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier); > + state.duty_cycle = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * state.period, 100); > > edge = ktime_get(); > > for (i = 0; i < count; i++) { > - if (i % 2) // space > - pwm_disable(pwm); > - else > - pwm_enable(pwm); > + state.enabled = !(i % 2); > + pwm_apply_state(pwm, &state); > > edge = ktime_add_us(edge, txbuf[i]); > delta = ktime_us_delta(edge, ktime_get()); > @@ -76,7 +75,8 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, > usleep_range(delta, delta + 10); > } > > - pwm_disable(pwm); > + state.enabled = false; > + pwm_apply_state(pwm, &state); I would have added a struct pwm_state to struct pwm_ir and then would call pwm_init_state() only once in .probe(). But that's subjective if you like it better or not, so do what you prefer. The other changes look fine, so: Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
Hi "Maíra,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on media-tree/master]
[also build test ERROR on v5.15-rc7 next-20211026]
[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/media-rc-pwm-ir-tx-Switch-to-atomic-PWM-API/20211024-183502
base: git://linuxtv.org/media_tree.git master
config: riscv-randconfig-r004-20211027 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
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
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/0day-ci/linux/commit/abea850df0b6436083fcaa097ad3029a27aa62bb
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Ma-ra-Canal/media-rc-pwm-ir-tx-Switch-to-atomic-PWM-API/20211024-183502
git checkout abea850df0b6436083fcaa097ad3029a27aa62bb
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv
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 >>, old ones prefixed by <<):
>> ERROR: modpost: "__udivdi3" [drivers/media/rc/pwm-ir-tx.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Oct 27, 2021 at 02:07:19PM +0800, kernel test robot wrote: > Hi "Maíra, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on media-tree/master] > [also build test ERROR on v5.15-rc7 next-20211026] > [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/media-rc-pwm-ir-tx-Switch-to-atomic-PWM-API/20211024-183502 > base: git://linuxtv.org/media_tree.git master > config: riscv-randconfig-r004-20211027 (attached as .config) > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72) > 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 > # install riscv cross compiling tool for clang build > # apt-get install binutils-riscv64-linux-gnu > # https://github.com/0day-ci/linux/commit/abea850df0b6436083fcaa097ad3029a27aa62bb > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Ma-ra-Canal/media-rc-pwm-ir-tx-Switch-to-atomic-PWM-API/20211024-183502 > git checkout abea850df0b6436083fcaa097ad3029a27aa62bb > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv > > 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 >>, old ones prefixed by <<): > > >> ERROR: modpost: "__udivdi3" [drivers/media/rc/pwm-ir-tx.ko] undefined! This comes from the line: state.duty_cycle = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * state.period, 100); where DIV_ROUND_CLOSEST expands to a normal division but state.period is a u64. So this should use DIV64_U64_ROUND_CLOSEST I guess. Best regards Uwe
On Wed, Oct 27, 2021 at 08:15:52AM +0200, Uwe Kleine-König wrote: > On Wed, Oct 27, 2021 at 02:07:19PM +0800, kernel test robot wrote: > > 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 >>, old ones prefixed by <<): > > > > >> ERROR: modpost: "__udivdi3" [drivers/media/rc/pwm-ir-tx.ko] undefined! > > This comes from the line: > > state.duty_cycle = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * state.period, 100); > > where DIV_ROUND_CLOSEST expands to a normal division but state.period is > a u64. So this should use DIV64_U64_ROUND_CLOSEST I guess. DIV64_U64_ROUND_CLOSEST is for dividing a u64 with a u64. We're dividing by 100 here so this is not necessary. It should use DIV_ROUND_CLOSEST_ULL, however it might be nicer to use: pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100); Thanks Sean
[resend it in Plain Text] Thank you for the feedback! I appreciate that! I'm new at the kernel and I got a little confused about how to send the new patch. Should I send a v4 of this patch or just send a new patch fixing this issue? I'm sorry about the question and thank you for your attention. > Em qua., 27 de out. de 2021 às 04:32, Sean Young <sean@mess.org> escreveu: >> >> On Wed, Oct 27, 2021 at 08:15:52AM +0200, Uwe Kleine-König wrote: >> > On Wed, Oct 27, 2021 at 02:07:19PM +0800, kernel test robot wrote: >> > > 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 >>, old ones prefixed by <<): >> > > >> > > >> ERROR: modpost: "__udivdi3" [drivers/media/rc/pwm-ir-tx.ko] undefined! >> > >> > This comes from the line: >> > >> > state.duty_cycle = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * state.period, 100); >> > >> > where DIV_ROUND_CLOSEST expands to a normal division but state.period is >> > a u64. So this should use DIV64_U64_ROUND_CLOSEST I guess. >> >> DIV64_U64_ROUND_CLOSEST is for dividing a u64 with a u64. We're dividing >> by 100 here so this is not necessary. >> >> It should use DIV_ROUND_CLOSEST_ULL, however it might be nicer to use: >> >> pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100); >> >> Thanks >> >> Sean
Hi Maíra, On Wed, Oct 27, 2021 at 09:43:47AM -0300, Maíra Canal wrote: > [resend it in Plain Text] > Thank you for the feedback! I appreciate that! I'm new at the kernel > and I got a little confused about how to send the new patch. Should I > send a v4 of this patch or just send a new patch fixing this issue? > I'm sorry about the question and thank you for your attention. Please send out a v4 with the problem fixed. Also top-posting is deprecated on linux mailing lists. Thanks, Sean > > Em qua., 27 de out. de 2021 às 04:32, Sean Young <sean@mess.org> escreveu: > >> > >> On Wed, Oct 27, 2021 at 08:15:52AM +0200, Uwe Kleine-König wrote: > >> > On Wed, Oct 27, 2021 at 02:07:19PM +0800, kernel test robot wrote: > >> > > 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 >>, old ones prefixed by <<): > >> > > > >> > > >> ERROR: modpost: "__udivdi3" [drivers/media/rc/pwm-ir-tx.ko] undefined! > >> > > >> > This comes from the line: > >> > > >> > state.duty_cycle = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * state.period, 100); > >> > > >> > where DIV_ROUND_CLOSEST expands to a normal division but state.period is > >> > a u64. So this should use DIV64_U64_ROUND_CLOSEST I guess. > >> > >> DIV64_U64_ROUND_CLOSEST is for dividing a u64 with a u64. We're dividing > >> by 100 here so this is not necessary. > >> > >> It should use DIV_ROUND_CLOSEST_ULL, however it might be nicer to use: > >> > >> pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100); > >> > >> Thanks > >> > >> Sean
diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c index 4bc28d2c9cc9..e1f348a962e8 100644 --- a/drivers/media/rc/pwm-ir-tx.c +++ b/drivers/media/rc/pwm-ir-tx.c @@ -53,22 +53,21 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, { struct pwm_ir *pwm_ir = dev->priv; struct pwm_device *pwm = pwm_ir->pwm; - int i, duty, period; + struct pwm_state state; + int i; ktime_t edge; long delta; - period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier); - duty = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * period, 100); + pwm_init_state(pwm, &state); - pwm_config(pwm, duty, period); + state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier); + state.duty_cycle = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * state.period, 100); edge = ktime_get(); for (i = 0; i < count; i++) { - if (i % 2) // space - pwm_disable(pwm); - else - pwm_enable(pwm); + state.enabled = !(i % 2); + pwm_apply_state(pwm, &state); edge = ktime_add_us(edge, txbuf[i]); delta = ktime_us_delta(edge, ktime_get()); @@ -76,7 +75,8 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, usleep_range(delta, delta + 10); } - pwm_disable(pwm); + state.enabled = false; + pwm_apply_state(pwm, &state); return count; }
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: Assign variables directly and simplify conditional statement V2 -> V3: Fix declaration of undeclared variable --- drivers/media/rc/pwm-ir-tx.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)