diff mbox series

[v3,2/2] pwm: sifive: change the PWM controlled LED algorithm

Message ID 20230420093457.18936-3-nylon.chen@sifive.com (mailing list archive)
State Handled Elsewhere
Headers show
Series Change PWM-controlled LED pin active mode and algorithm | expand

Checks

Context Check Description
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be for-next at HEAD c070bcfdb2fa
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 1 and now 1
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig fail Errors and warnings before: 18 this patch: 20
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig fail Errors and warnings before: 18 this patch: 21
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 3 this patch: 3
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 29 lines checked
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Nylon Chen April 20, 2023, 9:34 a.m. UTC 
The `frac` variable represents the pulse inactive time, and the result of
this algorithm is the pulse active time. Therefore, we must reverse the
result.

The reference is SiFive FU740-C000 Manual[0]

Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
---
 drivers/pwm/pwm-sifive.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Emil Renner Berthing April 20, 2023, 10:04 a.m. UTC | #1 
On Thu, 20 Apr 2023 at 11:35, Nylon Chen <nylon.chen@sifive.com> wrote:
>
> The `frac` variable represents the pulse inactive time, and the result of
> this algorithm is the pulse active time. Therefore, we must reverse the
> result.
>
> The reference is SiFive FU740-C000 Manual[0]
>
> Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>  drivers/pwm/pwm-sifive.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> index 393a4b97fc19..d5d5f36da297 100644
> --- a/drivers/pwm/pwm-sifive.c
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  {
>         struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
>         struct pwm_state cur_state;
> -       unsigned int duty_cycle;
> +       unsigned int duty_cycle, period;
>         unsigned long long num;
>         bool enabled;
>         int ret = 0;
>         u32 frac;
>
> -       if (state->polarity != PWM_POLARITY_INVERSED)
> +       if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
>                 return -EINVAL;
>
>         cur_state = pwm->state;
> @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>          * calculating the register values first and then writing them
>          * consecutively
>          */
> +       period = max(state->period, ddata->approx_period);

Hi Nylon,

I don't understand this patch. You introduce this new variable,
period, and set it here but you never seem to use it. If you planned
to use it instead of state->period below, why should it be the max of
the old period and what is requested? What happens if the consumer
wants to lower the period?

Also above you now allow both PWM_POLARITY_NORMAL and
PWM_POLARITY_INVERSED but you treat both cases the same.

/Emil

>         num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
>         frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> -       /* The hardware cannot generate a 100% duty cycle */
>         frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> +       /* The hardware cannot generate a 100% duty cycle */
> +       frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> +
>
>         mutex_lock(&ddata->lock);
>         if (state->period != ddata->approx_period) {
> --
> 2.40.0
>
Conor Dooley April 20, 2023, 10:28 a.m. UTC | #2 
On Thu, Apr 20, 2023 at 05:34:57PM +0800, Nylon Chen wrote:
> The `frac` variable represents the pulse inactive time, and the result of
> this algorithm is the pulse active time. Therefore, we must reverse the
> result.
> 
> The reference is SiFive FU740-C000 Manual[0]
> 
> Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
> 
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Hmm, I don't recall reviewing or acking this patch. I do recalling doing
it for 1/2 though:
https://lore.kernel.org/linux-pwm/Y9len4GinXQ101xr@spud/

Please remove these from your next submission, I don't have any knowledge
of this driver nor do I maintain it, thanks.

> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>

This SoB is new too AFAICT and looks a bit odd.
Should there be a Co-developed-by for Vincent?

Thanks,
Conor.
Emil Renner Berthing April 20, 2023, 10:46 a.m. UTC | #3 
On Thu, 20 Apr 2023 at 12:41, Nylon Chen <nylon.chen@sifive.com> wrote:
>
> Hi, Emil
>
> Emil Renner Berthing <emil.renner.berthing@canonical.com> 於 2023年4月20日 週四 下午6:04寫道：
> >
> > On Thu, 20 Apr 2023 at 11:35, Nylon Chen <nylon.chen@sifive.com> wrote:
> > >
> > > The `frac` variable represents the pulse inactive time, and the result of
> > > this algorithm is the pulse active time. Therefore, we must reverse the
> > > result.
> > >
> > > The reference is SiFive FU740-C000 Manual[0]
> > >
> > > Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
> > >
> > > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > > ---
> > >  drivers/pwm/pwm-sifive.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > index 393a4b97fc19..d5d5f36da297 100644
> > > --- a/drivers/pwm/pwm-sifive.c
> > > +++ b/drivers/pwm/pwm-sifive.c
> > > @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  {
> > >         struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
> > >         struct pwm_state cur_state;
> > > -       unsigned int duty_cycle;
> > > +       unsigned int duty_cycle, period;
> > >         unsigned long long num;
> > >         bool enabled;
> > >         int ret = 0;
> > >         u32 frac;
> > >
> > > -       if (state->polarity != PWM_POLARITY_INVERSED)
> > > +       if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
> > >                 return -EINVAL;
> > >
> > >         cur_state = pwm->state;
> > > @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >          * calculating the register values first and then writing them
> > >          * consecutively
> > >          */
> > > +       period = max(state->period, ddata->approx_period);
> >
> > Hi Nylon,
> >
> > I don't understand this patch. You introduce this new variable,
> > period, and set it here but you never seem to use it. If you planned
> > to use it instead of state->period below, why should it be the max of
> > the old period and what is requested? What happens if the consumer
> > wants to lower the period?
> Sorry this was an oversight on my part, there was a line correction that didn't change to
> - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> + frac = DIV64_U64_ROUND_CLOSEST(num, period);

I see, so then my second question was why period needs to be the
larger of the previous period and the requested period.

What happens if the requested period, state->period, is lower than the
old period, ddata->approx_period? Then the period will be changed to
state->period below, but the calculations will be made using period =
ddata->approx_period, right?

> >
> > Also above you now allow both PWM_POLARITY_NORMAL and
> > PWM_POLARITY_INVERSED but you treat both cases the same.
> I may have misunderstood what Uwe means here, I will confirm again here
> >
> > /Emil
> >
> > >         num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> > >         frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > > -       /* The hardware cannot generate a 100% duty cycle */
> > >         frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > +       /* The hardware cannot generate a 100% duty cycle */
> > > +       frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> > > +
> > >
> > >         mutex_lock(&ddata->lock);
> > >         if (state->period != ddata->approx_period) {
> > > --
> > > 2.40.0
> > >
kernel test robot April 20, 2023, 2:13 p.m. UTC | #4 
Hi Nylon,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on thierry-reding-pwm/for-next rockchip/for-next linus/master v6.3-rc7 next-20230419]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230420093457.18936-3-nylon.chen%40sifive.com
patch subject: [PATCH v3 2/2] pwm: sifive: change the PWM controlled LED algorithm
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230420/202304202141.JYCKBVOQ-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
        git checkout f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304202141.JYCKBVOQ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:26,
                    from include/linux/clk.h:13,
                    from drivers/pwm/pwm-sifive.c:14:
   drivers/pwm/pwm-sifive.c: In function 'pwm_sifive_apply':
   include/linux/minmax.h:20:35: warning: comparison of distinct pointer types lacks a cast
      20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
         |                                   ^~
   include/linux/minmax.h:26:18: note: in expansion of macro '__typecheck'
      26 |                 (__typecheck(x, y) && __no_side_effects(x, y))
         |                  ^~~~~~~~~~~
   include/linux/minmax.h:36:31: note: in expansion of macro '__safe_cmp'
      36 |         __builtin_choose_expr(__safe_cmp(x, y), \
         |                               ^~~~~~~~~~
   include/linux/minmax.h:74:25: note: in expansion of macro '__careful_cmp'
      74 | #define max(x, y)       __careful_cmp(x, y, >)
         |                         ^~~~~~~~~~~~~
   drivers/pwm/pwm-sifive.c:157:18: note: in expansion of macro 'max'
     157 |         period = max(state->period, ddata->approx_period);
         |                  ^~~
>> drivers/pwm/pwm-sifive.c:135:34: warning: variable 'period' set but not used [-Wunused-but-set-variable]
     135 |         unsigned int duty_cycle, period;
         |                                  ^~~~~~


vim +/period +135 drivers/pwm/pwm-sifive.c

   129	
   130	static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
   131				    const struct pwm_state *state)
   132	{
   133		struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
   134		struct pwm_state cur_state;
 > 135		unsigned int duty_cycle, period;
   136		unsigned long long num;
   137		bool enabled;
   138		int ret = 0;
   139		u32 frac;
   140	
   141		if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
   142			return -EINVAL;
   143	
   144		cur_state = pwm->state;
   145		enabled = cur_state.enabled;
   146	
   147		duty_cycle = state->duty_cycle;
   148		if (!state->enabled)
   149			duty_cycle = 0;
   150	
   151		/*
   152		 * The problem of output producing mixed setting as mentioned at top,
   153		 * occurs here. To minimize the window for this problem, we are
   154		 * calculating the register values first and then writing them
   155		 * consecutively
   156		 */
   157		period = max(state->period, ddata->approx_period);
   158		num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
   159		frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
   160		frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
   161		/* The hardware cannot generate a 100% duty cycle */
   162		frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
   163	
   164	
   165		mutex_lock(&ddata->lock);
   166		if (state->period != ddata->approx_period) {
   167			/*
   168			 * Don't let a 2nd user change the period underneath the 1st user.
   169			 * However if ddate->approx_period == 0 this is the first time we set
   170			 * any period, so let whoever gets here first set the period so other
   171			 * users who agree on the period won't fail.
   172			 */
   173			if (ddata->user_count != 1 && ddata->approx_period) {
   174				mutex_unlock(&ddata->lock);
   175				return -EBUSY;
   176			}
   177			ddata->approx_period = state->period;
   178			pwm_sifive_update_clock(ddata, clk_get_rate(ddata->clk));
   179		}
   180		mutex_unlock(&ddata->lock);
   181	
   182		/*
   183		 * If the PWM is enabled the clk is already on. So only enable it
   184		 * conditionally to have it on exactly once afterwards independent of
   185		 * the PWM state.
   186		 */
   187		if (!enabled) {
   188			ret = clk_enable(ddata->clk);
   189			if (ret) {
   190				dev_err(ddata->chip.dev, "Enable clk failed\n");
   191				return ret;
   192			}
   193		}
   194	
   195		writel(frac, ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
   196	
   197		if (!state->enabled)
   198			clk_disable(ddata->clk);
   199	
   200		return 0;
   201	}
   202
Nylon Chen April 21, 2023, 6:16 a.m. UTC | #5 
Hi Emil,

Emil Renner Berthing <emil.renner.berthing@canonical.com> 於 2023年4月20日
週四 下午6:46寫道：
>
> On Thu, 20 Apr 2023 at 12:41, Nylon Chen <nylon.chen@sifive.com> wrote:
> >
> > Hi, Emil
> >
> > Emil Renner Berthing <emil.renner.berthing@canonical.com> 於 2023年4月20日 週四 下午6:04寫道：
> > >
> > > On Thu, 20 Apr 2023 at 11:35, Nylon Chen <nylon.chen@sifive.com> wrote:
> > > >
> > > > The `frac` variable represents the pulse inactive time, and the result of
> > > > this algorithm is the pulse active time. Therefore, we must reverse the
> > > > result.
> > > >
> > > > The reference is SiFive FU740-C000 Manual[0]
> > > >
> > > > Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
> > > >
> > > > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > > > ---
> > > >  drivers/pwm/pwm-sifive.c | 9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > > index 393a4b97fc19..d5d5f36da297 100644
> > > > --- a/drivers/pwm/pwm-sifive.c
> > > > +++ b/drivers/pwm/pwm-sifive.c
> > > > @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >  {
> > > >         struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
> > > >         struct pwm_state cur_state;
> > > > -       unsigned int duty_cycle;
> > > > +       unsigned int duty_cycle, period;
> > > >         unsigned long long num;
> > > >         bool enabled;
> > > >         int ret = 0;
> > > >         u32 frac;
> > > >
> > > > -       if (state->polarity != PWM_POLARITY_INVERSED)
> > > > +       if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
> > > >                 return -EINVAL;
> > > >
> > > >         cur_state = pwm->state;
> > > > @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >          * calculating the register values first and then writing them
> > > >          * consecutively
> > > >          */
> > > > +       period = max(state->period, ddata->approx_period);
> > >
> > > Hi Nylon,
> > >
> > > I don't understand this patch. You introduce this new variable,
> > > period, and set it here but you never seem to use it. If you planned
> > > to use it instead of state->period below, why should it be the max of
> > > the old period and what is requested? What happens if the consumer
> > > wants to lower the period?
> > Sorry this was an oversight on my part, there was a line correction that didn't change to
> > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > + frac = DIV64_U64_ROUND_CLOSEST(num, period);
>
> I see, so then my second question was why period needs to be the
> larger of the previous period and the requested period.
>
> What happens if the requested period, state->period, is lower than the
> old period, ddata->approx_period? Then the period will be changed to
> state->period below, but the calculations will be made using period =
> ddata->approx_period, right?

Your understanding is correct. According to the new algorithm proposed
by Uwe, the goal is to:
Pick the biggest period length possible that is not bigger than the
requested period.
>
> > >
> > > Also above you now allow both PWM_POLARITY_NORMAL and
> > > PWM_POLARITY_INVERSED but you treat both cases the same.
> > I may have misunderstood what Uwe means here, I will confirm again here
> > >
> > > /Emil
> > >
> > > >         num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> > > >         frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > > > -       /* The hardware cannot generate a 100% duty cycle */
> > > >         frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > > +       /* The hardware cannot generate a 100% duty cycle */
> > > > +       frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> > > > +
> > > >
> > > >         mutex_lock(&ddata->lock);
> > > >         if (state->period != ddata->approx_period) {
> > > > --
> > > > 2.40.0
> > > >
Emil Renner Berthing April 21, 2023, 10:09 a.m. UTC | #6 
On Fri, 21 Apr 2023 at 08:16, Nylon Chen <nylon.chen@sifive.com> wrote:
>
> Hi Emil,
>
> Emil Renner Berthing <emil.renner.berthing@canonical.com> 於 2023年4月20日
> 週四 下午6:46寫道：
> >
> > On Thu, 20 Apr 2023 at 12:41, Nylon Chen <nylon.chen@sifive.com> wrote:
> > >
> > > Hi, Emil
> > >
> > > Emil Renner Berthing <emil.renner.berthing@canonical.com> 於 2023年4月20日 週四 下午6:04寫道：
> > > >
> > > > On Thu, 20 Apr 2023 at 11:35, Nylon Chen <nylon.chen@sifive.com> wrote:
> > > > >
> > > > > The `frac` variable represents the pulse inactive time, and the result of
> > > > > this algorithm is the pulse active time. Therefore, we must reverse the
> > > > > result.
> > > > >
> > > > > The reference is SiFive FU740-C000 Manual[0]
> > > > >
> > > > > Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
> > > > >
> > > > > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > > > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> > > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > > > > ---
> > > > >  drivers/pwm/pwm-sifive.c | 9 ++++++---
> > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > > > index 393a4b97fc19..d5d5f36da297 100644
> > > > > --- a/drivers/pwm/pwm-sifive.c
> > > > > +++ b/drivers/pwm/pwm-sifive.c
> > > > > @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > >  {
> > > > >         struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
> > > > >         struct pwm_state cur_state;
> > > > > -       unsigned int duty_cycle;
> > > > > +       unsigned int duty_cycle, period;
> > > > >         unsigned long long num;
> > > > >         bool enabled;
> > > > >         int ret = 0;
> > > > >         u32 frac;
> > > > >
> > > > > -       if (state->polarity != PWM_POLARITY_INVERSED)
> > > > > +       if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
> > > > >                 return -EINVAL;
> > > > >
> > > > >         cur_state = pwm->state;
> > > > > @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > >          * calculating the register values first and then writing them
> > > > >          * consecutively
> > > > >          */
> > > > > +       period = max(state->period, ddata->approx_period);
> > > >
> > > > Hi Nylon,
> > > >
> > > > I don't understand this patch. You introduce this new variable,
> > > > period, and set it here but you never seem to use it. If you planned
> > > > to use it instead of state->period below, why should it be the max of
> > > > the old period and what is requested? What happens if the consumer
> > > > wants to lower the period?
> > > Sorry this was an oversight on my part, there was a line correction that didn't change to
> > > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > > + frac = DIV64_U64_ROUND_CLOSEST(num, period);
> >
> > I see, so then my second question was why period needs to be the
> > larger of the previous period and the requested period.
> >
> > What happens if the requested period, state->period, is lower than the
> > old period, ddata->approx_period? Then the period will be changed to
> > state->period below, but the calculations will be made using period =
> > ddata->approx_period, right?
>
> Your understanding is correct. According to the new algorithm proposed
> by Uwe, the goal is to:
> Pick the biggest period length possible that is not bigger than the
> requested period.

Right, and to be clear: this patch doesn't do that.

If the previous period in ddata->approx_period is bigger than the
requested period in state->period, it will do the frac calculations
with the old period, but still set the period to the shorter requested
period.

> > > >
> > > > Also above you now allow both PWM_POLARITY_NORMAL and
> > > > PWM_POLARITY_INVERSED but you treat both cases the same.
> > > I may have misunderstood what Uwe means here, I will confirm again here
> > > >
> > > > /Emil
> > > >
> > > > >         num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> > > > >         frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > > > > -       /* The hardware cannot generate a 100% duty cycle */
> > > > >         frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > > > +       /* The hardware cannot generate a 100% duty cycle */
> > > > > +       frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> > > > > +
> > > > >
> > > > >         mutex_lock(&ddata->lock);
> > > > >         if (state->period != ddata->approx_period) {
> > > > > --
> > > > > 2.40.0
> > > > >
kernel test robot April 22, 2023, 1:22 p.m. UTC | #7 
Hi Nylon,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on thierry-reding-pwm/for-next rockchip/for-next linus/master v6.3-rc7 next-20230421]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230420093457.18936-3-nylon.chen%40sifive.com
patch subject: [PATCH v3 2/2] pwm: sifive: change the PWM controlled LED algorithm
config: sparc64-randconfig-s031-20230421 (https://download.01.org/0day-ci/archive/20230422/202304222135.B9PoQ5w3-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
        git checkout f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 SHELL=/bin/bash drivers/pwm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304222135.B9PoQ5w3-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/pwm/pwm-sifive.c:157:18: sparse: sparse: incompatible types in comparison expression (different type sizes):
>> drivers/pwm/pwm-sifive.c:157:18: sparse:    unsigned long long const *
>> drivers/pwm/pwm-sifive.c:157:18: sparse:    unsigned int *

vim +157 drivers/pwm/pwm-sifive.c

   129	
   130	static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
   131				    const struct pwm_state *state)
   132	{
   133		struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
   134		struct pwm_state cur_state;
   135		unsigned int duty_cycle, period;
   136		unsigned long long num;
   137		bool enabled;
   138		int ret = 0;
   139		u32 frac;
   140	
   141		if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
   142			return -EINVAL;
   143	
   144		cur_state = pwm->state;
   145		enabled = cur_state.enabled;
   146	
   147		duty_cycle = state->duty_cycle;
   148		if (!state->enabled)
   149			duty_cycle = 0;
   150	
   151		/*
   152		 * The problem of output producing mixed setting as mentioned at top,
   153		 * occurs here. To minimize the window for this problem, we are
   154		 * calculating the register values first and then writing them
   155		 * consecutively
   156		 */
 > 157		period = max(state->period, ddata->approx_period);
   158		num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
   159		frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
   160		frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
   161		/* The hardware cannot generate a 100% duty cycle */
   162		frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
   163	
   164	
   165		mutex_lock(&ddata->lock);
   166		if (state->period != ddata->approx_period) {
   167			/*
   168			 * Don't let a 2nd user change the period underneath the 1st user.
   169			 * However if ddate->approx_period == 0 this is the first time we set
   170			 * any period, so let whoever gets here first set the period so other
   171			 * users who agree on the period won't fail.
   172			 */
   173			if (ddata->user_count != 1 && ddata->approx_period) {
   174				mutex_unlock(&ddata->lock);
   175				return -EBUSY;
   176			}
   177			ddata->approx_period = state->period;
   178			pwm_sifive_update_clock(ddata, clk_get_rate(ddata->clk));
   179		}
   180		mutex_unlock(&ddata->lock);
   181	
   182		/*
   183		 * If the PWM is enabled the clk is already on. So only enable it
   184		 * conditionally to have it on exactly once afterwards independent of
   185		 * the PWM state.
   186		 */
   187		if (!enabled) {
   188			ret = clk_enable(ddata->clk);
   189			if (ret) {
   190				dev_err(ddata->chip.dev, "Enable clk failed\n");
   191				return ret;
   192			}
   193		}
   194	
   195		writel(frac, ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
   196	
   197		if (!state->enabled)
   198			clk_disable(ddata->clk);
   199	
   200		return 0;
   201	}
   202
kernel test robot May 8, 2023, 9:48 a.m. UTC | #8 
Hi Nylon,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on thierry-reding-pwm/for-next rockchip/for-next linus/master v6.4-rc1 next-20230508]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230420093457.18936-3-nylon.chen%40sifive.com
patch subject: [PATCH v3 2/2] pwm: sifive: change the PWM controlled LED algorithm
config: powerpc-randconfig-s041-20230507 (https://download.01.org/0day-ci/archive/20230508/202305081759.wgN4Q80I-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
        git checkout f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=powerpc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/pwm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305081759.wgN4Q80I-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/pwm/pwm-sifive.c:157:18: sparse: sparse: incompatible types in comparison expression (different type sizes):
>> drivers/pwm/pwm-sifive.c:157:18: sparse:    unsigned long long const *
>> drivers/pwm/pwm-sifive.c:157:18: sparse:    unsigned int *

vim +157 drivers/pwm/pwm-sifive.c

   129	
   130	static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
   131				    const struct pwm_state *state)
   132	{
   133		struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
   134		struct pwm_state cur_state;
   135		unsigned int duty_cycle, period;
   136		unsigned long long num;
   137		bool enabled;
   138		int ret = 0;
   139		u32 frac;
   140	
   141		if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
   142			return -EINVAL;
   143	
   144		cur_state = pwm->state;
   145		enabled = cur_state.enabled;
   146	
   147		duty_cycle = state->duty_cycle;
   148		if (!state->enabled)
   149			duty_cycle = 0;
   150	
   151		/*
   152		 * The problem of output producing mixed setting as mentioned at top,
   153		 * occurs here. To minimize the window for this problem, we are
   154		 * calculating the register values first and then writing them
   155		 * consecutively
   156		 */
 > 157		period = max(state->period, ddata->approx_period);
   158		num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
   159		frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
   160		frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
   161		/* The hardware cannot generate a 100% duty cycle */
   162		frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
   163	
   164	
   165		mutex_lock(&ddata->lock);
   166		if (state->period != ddata->approx_period) {
   167			/*
   168			 * Don't let a 2nd user change the period underneath the 1st user.
   169			 * However if ddate->approx_period == 0 this is the first time we set
   170			 * any period, so let whoever gets here first set the period so other
   171			 * users who agree on the period won't fail.
   172			 */
   173			if (ddata->user_count != 1 && ddata->approx_period) {
   174				mutex_unlock(&ddata->lock);
   175				return -EBUSY;
   176			}
   177			ddata->approx_period = state->period;
   178			pwm_sifive_update_clock(ddata, clk_get_rate(ddata->clk));
   179		}
   180		mutex_unlock(&ddata->lock);
   181	
   182		/*
   183		 * If the PWM is enabled the clk is already on. So only enable it
   184		 * conditionally to have it on exactly once afterwards independent of
   185		 * the PWM state.
   186		 */
   187		if (!enabled) {
   188			ret = clk_enable(ddata->clk);
   189			if (ret) {
   190				dev_err(ddata->chip.dev, "Enable clk failed\n");
   191				return ret;
   192			}
   193		}
   194	
   195		writel(frac, ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
   196	
   197		if (!state->enabled)
   198			clk_disable(ddata->clk);
   199	
   200		return 0;
   201	}
   202
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
index 393a4b97fc19..d5d5f36da297 100644
--- a/drivers/pwm/pwm-sifive.c
+++ b/drivers/pwm/pwm-sifive.c
@@ -132,13 +132,13 @@  static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
 	struct pwm_state cur_state;
-	unsigned int duty_cycle;
+	unsigned int duty_cycle, period;
 	unsigned long long num;
 	bool enabled;
 	int ret = 0;
 	u32 frac;
 
-	if (state->polarity != PWM_POLARITY_INVERSED)
+	if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
 		return -EINVAL;
 
 	cur_state = pwm->state;
@@ -154,10 +154,13 @@  static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * calculating the register values first and then writing them
 	 * consecutively
 	 */
+	period = max(state->period, ddata->approx_period);
 	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
 	frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
-	/* The hardware cannot generate a 100% duty cycle */
 	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
+	/* The hardware cannot generate a 100% duty cycle */
+	frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
+
 
 	mutex_lock(&ddata->lock);
 	if (state->period != ddata->approx_period) {