All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v9 3/4] pwm: add microchip soft ip corePWM driver
  2022-08-19  8:57   ` Conor Dooley
  (?)
  (?)
@ 2022-08-22  8:42 ` Dan Carpenter
  -1 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2022-08-21 15:53 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 9403 bytes --]

BCC: lkp(a)intel.com
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20220819085703.4161266-4-conor.dooley@microchip.com>
References: <20220819085703.4161266-4-conor.dooley@microchip.com>
TO: Conor Dooley <conor.dooley@microchip.com>
TO: Thierry Reding <thierry.reding@gmail.com>
TO: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
TO: Rob Herring <robh+dt@kernel.org>
TO: Krzysztof Kozlowski <krzk@kernel.org>
CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: devicetree(a)vger.kernel.org
CC: linux-kernel(a)vger.kernel.org
CC: linux-pwm(a)vger.kernel.org
CC: linux-riscv(a)lists.infradead.org
CC: Conor Dooley <conor.dooley@microchip.com>

Hi Conor,

I love your patch! Perhaps something to improve:

[auto build test WARNING on 568035b01cfb107af8d2e4bd2fb9aea22cf5b868]

url:    https://github.com/intel-lab-lkp/linux/commits/Conor-Dooley/Microchip-soft-ip-corePWM-driver/20220819-170106
base:   568035b01cfb107af8d2e4bd2fb9aea22cf5b868
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: arm64-randconfig-m031-20220821 (https://download.01.org/0day-ci/archive/20220821/202208212329.XETz1mt0-lkp(a)intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/pwm/pwm-microchip-core.c:295 mchp_core_pwm_apply() warn: inconsistent returns '&mchp_core_pwm->lock'.

vim +295 drivers/pwm/pwm-microchip-core.c

ae39414af22131 Conor Dooley 2022-08-19  199  
ae39414af22131 Conor Dooley 2022-08-19  200  static int mchp_core_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
ae39414af22131 Conor Dooley 2022-08-19  201  			       const struct pwm_state *state)
ae39414af22131 Conor Dooley 2022-08-19  202  {
ae39414af22131 Conor Dooley 2022-08-19  203  	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
ae39414af22131 Conor Dooley 2022-08-19  204  	struct pwm_state current_state = pwm->state;
ae39414af22131 Conor Dooley 2022-08-19  205  	bool period_locked;
ae39414af22131 Conor Dooley 2022-08-19  206  	u64 duty_steps;
ae39414af22131 Conor Dooley 2022-08-19  207  	u16 prescale;
ae39414af22131 Conor Dooley 2022-08-19  208  	u8 period_steps;
ae39414af22131 Conor Dooley 2022-08-19  209  	int ret;
ae39414af22131 Conor Dooley 2022-08-19  210  
ae39414af22131 Conor Dooley 2022-08-19  211  	mutex_lock(&mchp_core_pwm->lock);
ae39414af22131 Conor Dooley 2022-08-19  212  
ae39414af22131 Conor Dooley 2022-08-19  213  	if (!state->enabled) {
ae39414af22131 Conor Dooley 2022-08-19  214  		mchp_core_pwm_enable(chip, pwm, false, current_state.period);
ae39414af22131 Conor Dooley 2022-08-19  215  		mutex_unlock(&mchp_core_pwm->lock);
ae39414af22131 Conor Dooley 2022-08-19  216  		return 0;
ae39414af22131 Conor Dooley 2022-08-19  217  	}
ae39414af22131 Conor Dooley 2022-08-19  218  
ae39414af22131 Conor Dooley 2022-08-19  219  	/*
ae39414af22131 Conor Dooley 2022-08-19  220  	 * If the only thing that has changed is the duty cycle or the polarity,
ae39414af22131 Conor Dooley 2022-08-19  221  	 * we can shortcut the calculations and just compute/apply the new duty
ae39414af22131 Conor Dooley 2022-08-19  222  	 * cycle pos & neg edges
ae39414af22131 Conor Dooley 2022-08-19  223  	 * As all the channels share the same period, do not allow it to be
ae39414af22131 Conor Dooley 2022-08-19  224  	 * changed if any other channels are enabled.
ae39414af22131 Conor Dooley 2022-08-19  225  	 * If the period is locked, it may not be possible to use a period
ae39414af22131 Conor Dooley 2022-08-19  226  	 * less than that requested. In that case, we just abort.
ae39414af22131 Conor Dooley 2022-08-19  227  	 */
ae39414af22131 Conor Dooley 2022-08-19  228  	period_locked = mchp_core_pwm->channel_enabled & ~(1 << pwm->hwpwm);
ae39414af22131 Conor Dooley 2022-08-19  229  
ae39414af22131 Conor Dooley 2022-08-19  230  	if (period_locked) {
ae39414af22131 Conor Dooley 2022-08-19  231  		u16 hw_prescale;
ae39414af22131 Conor Dooley 2022-08-19  232  		u8 hw_period_steps;
ae39414af22131 Conor Dooley 2022-08-19  233  
ae39414af22131 Conor Dooley 2022-08-19  234  		mchp_core_pwm_calc_period(chip, state, (u8 *)&prescale, &period_steps);
ae39414af22131 Conor Dooley 2022-08-19  235  		hw_prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
ae39414af22131 Conor Dooley 2022-08-19  236  		hw_period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
ae39414af22131 Conor Dooley 2022-08-19  237  
ae39414af22131 Conor Dooley 2022-08-19  238  		if ((period_steps + 1) * (prescale + 1) <
ae39414af22131 Conor Dooley 2022-08-19  239  		    (hw_period_steps + 1) * (hw_prescale + 1)) {
ae39414af22131 Conor Dooley 2022-08-19  240  			mutex_unlock(&mchp_core_pwm->lock);
ae39414af22131 Conor Dooley 2022-08-19  241  			return -EINVAL;
ae39414af22131 Conor Dooley 2022-08-19  242  		}
ae39414af22131 Conor Dooley 2022-08-19  243  
ae39414af22131 Conor Dooley 2022-08-19  244  		/*
ae39414af22131 Conor Dooley 2022-08-19  245  		 * It is possible that something could have set the period_steps
ae39414af22131 Conor Dooley 2022-08-19  246  		 * register to 0xff, which would prevent us from setting a 100%
ae39414af22131 Conor Dooley 2022-08-19  247  		 * duty cycle, as explained in the mchp_core_pwm_calc_period()
ae39414af22131 Conor Dooley 2022-08-19  248  		 * above.
ae39414af22131 Conor Dooley 2022-08-19  249  		 * The period is locked and we cannot change this, so we abort.
ae39414af22131 Conor Dooley 2022-08-19  250  		 */
ae39414af22131 Conor Dooley 2022-08-19  251  		if (period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX)
ae39414af22131 Conor Dooley 2022-08-19  252  			return -EINVAL;
ae39414af22131 Conor Dooley 2022-08-19  253  
ae39414af22131 Conor Dooley 2022-08-19  254  		prescale = hw_prescale;
ae39414af22131 Conor Dooley 2022-08-19  255  		period_steps = hw_period_steps;
ae39414af22131 Conor Dooley 2022-08-19  256  	} else if (!current_state.enabled || current_state.period != state->period) {
ae39414af22131 Conor Dooley 2022-08-19  257  		ret = mchp_core_pwm_calc_period(chip, state, (u8 *)&prescale, &period_steps);
ae39414af22131 Conor Dooley 2022-08-19  258  		if (ret) {
ae39414af22131 Conor Dooley 2022-08-19  259  			mutex_unlock(&mchp_core_pwm->lock);
ae39414af22131 Conor Dooley 2022-08-19  260  			return ret;
ae39414af22131 Conor Dooley 2022-08-19  261  		}
ae39414af22131 Conor Dooley 2022-08-19  262  		mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps);
ae39414af22131 Conor Dooley 2022-08-19  263  	} else {
ae39414af22131 Conor Dooley 2022-08-19  264  		prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
ae39414af22131 Conor Dooley 2022-08-19  265  		period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
ae39414af22131 Conor Dooley 2022-08-19  266  
ae39414af22131 Conor Dooley 2022-08-19  267  		/*
ae39414af22131 Conor Dooley 2022-08-19  268  		 * As above, it is possible that something could have set the
ae39414af22131 Conor Dooley 2022-08-19  269  		 * period_steps register to 0xff, which would prevent us from
ae39414af22131 Conor Dooley 2022-08-19  270  		 * setting a 100% duty cycle, as explained above.
ae39414af22131 Conor Dooley 2022-08-19  271  		 * As the period is not locked, we are free to fix this.
ae39414af22131 Conor Dooley 2022-08-19  272  		 */
ae39414af22131 Conor Dooley 2022-08-19  273  		if (period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX) {
ae39414af22131 Conor Dooley 2022-08-19  274  			period_steps -= 1;
ae39414af22131 Conor Dooley 2022-08-19  275  			mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps);
ae39414af22131 Conor Dooley 2022-08-19  276  		}
ae39414af22131 Conor Dooley 2022-08-19  277  	}
ae39414af22131 Conor Dooley 2022-08-19  278  
ae39414af22131 Conor Dooley 2022-08-19  279  	duty_steps = mchp_core_pwm_calc_duty(chip, pwm, state, prescale, period_steps);
ae39414af22131 Conor Dooley 2022-08-19  280  
ae39414af22131 Conor Dooley 2022-08-19  281  	/*
ae39414af22131 Conor Dooley 2022-08-19  282  	 * Because the period is per channel, it is possible that the requested
ae39414af22131 Conor Dooley 2022-08-19  283  	 * duty cycle is longer than the period, in which case cap it to the
ae39414af22131 Conor Dooley 2022-08-19  284  	 * period, IOW a 100% duty cycle.
ae39414af22131 Conor Dooley 2022-08-19  285  	 */
ae39414af22131 Conor Dooley 2022-08-19  286  	if (duty_steps > period_steps)
ae39414af22131 Conor Dooley 2022-08-19  287  		duty_steps = period_steps + 1;
ae39414af22131 Conor Dooley 2022-08-19  288  
ae39414af22131 Conor Dooley 2022-08-19  289  	mchp_core_pwm_apply_duty(chip, pwm, state, duty_steps, period_steps);
ae39414af22131 Conor Dooley 2022-08-19  290  
ae39414af22131 Conor Dooley 2022-08-19  291  	mchp_core_pwm_enable(chip, pwm, true, state->period);
ae39414af22131 Conor Dooley 2022-08-19  292  
ae39414af22131 Conor Dooley 2022-08-19  293  	mutex_unlock(&mchp_core_pwm->lock);
ae39414af22131 Conor Dooley 2022-08-19  294  
ae39414af22131 Conor Dooley 2022-08-19 @295  	return 0;
ae39414af22131 Conor Dooley 2022-08-19  296  }
ae39414af22131 Conor Dooley 2022-08-19  297  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [PATCH v9 0/4] Microchip soft ip corePWM driver
@ 2022-08-19  8:57 ` Conor Dooley
  0 siblings, 0 replies; 17+ messages in thread
From: Conor Dooley @ 2022-08-19  8:57 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Rob Herring,
	Krzysztof Kozlowski
  Cc: Daire McNamara, devicetree, linux-kernel, linux-pwm, linux-riscv,
	Conor Dooley

Hey Uwe, all,

6.0-rc1 has rolled around so here is the promised ~v8~v9.
The pre 6.0-rc1 cover letter/series is here:
https://lore.kernel.org/linux-pwm/20220721172109.941900-1-mail@conchuod.ie
I'll take the dts change myself once the rest is merged.

There is one change here that is not directly from your feedback on v7,
Iadded a test for invalid PERIOD_STEPS values, in which case we abort if
the period is locked and cannot be fixed. Hopefully the rounding is not
ruined..

Thanks,
Conor.

Changes since v8:
- fixed a(nother) raw 64 bit division (& built it for riscv32!)
- added a check to make sure we don't try to sleep for 0 us

Changes since v7:
- rebased on 6.0-rc1
- reworded comments you highlighted in v7
- fixed the overkill sleeping
- removed the unused variables in calc_duty
- added some extra comments to explain behaviours you questioned in v7
- make the mutexes un-interruptible
- fixed added the 1s you suggested for the if(period_locked) logic
- added setup of the channel_enabled shadowing
- fixed the period reporting for the negedge == posedge case in
  get_state() I had to add the enabled check, as otherwise it broke
  setting the period for the first time out of reset.
- added a test for invalid PERIOD_STEPS values, in which case we abort
  if we cannot fix the period

Changes from v6:
- Dropped an unused variable that I'd missed
- Actually check the return values of the mutex lock()s
- Re-rebased on -next for the MAINTAINERS patch (again...)

Changes from v5:
- switched to a mutex b/c we must sleep with the lock taken
- simplified the locking in apply() and added locking to get_state()
- reworked apply() as requested
- removed the loop in the period calculation (thanks Uwe!)
- add a copy of the enable registers in the driver to save on reads.
- remove the second (useless) write to sync_update
- added some missing rounding in get_state()
- couple other minor cleanups as requested in:
https://lore.kernel.org/linux-riscv/20220709160206.cw5luo7kxdshoiua@pengutronix.de/

Changes from v4:
- dropped some accidentally added files

Conor Dooley (4):
  dt-bindings: pwm: fix microchip corePWM's pwm-cells
  riscv: dts: fix the icicle's #pwm-cells
  pwm: add microchip soft ip corePWM driver
  MAINTAINERS: add pwm to PolarFire SoC entry

 .../bindings/pwm/microchip,corepwm.yaml       |   4 +-
 MAINTAINERS                                   |   1 +
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |   2 +-
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-microchip-core.c              | 400 ++++++++++++++++++
 6 files changed, 416 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pwm/pwm-microchip-core.c


base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
-- 
2.36.1


^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-08-22  9:19 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-21 15:53 [PATCH v9 3/4] pwm: add microchip soft ip corePWM driver kernel test robot
2022-08-22  8:42 ` Dan Carpenter
2022-08-22  8:42 ` Dan Carpenter
2022-08-22  8:42 ` Dan Carpenter
2022-08-22  9:18 ` Conor.Dooley
2022-08-22  9:18   ` Conor.Dooley
2022-08-22  9:18   ` Conor.Dooley
  -- strict thread matches above, loose matches on Subject: below --
2022-08-19  8:57 [PATCH v9 0/4] Microchip " Conor Dooley
2022-08-19  8:57 ` Conor Dooley
2022-08-19  8:57 ` [PATCH v9 1/4] dt-bindings: pwm: fix microchip corePWM's pwm-cells Conor Dooley
2022-08-19  8:57   ` Conor Dooley
2022-08-19  8:57 ` [PATCH v9 2/4] riscv: dts: fix the icicle's #pwm-cells Conor Dooley
2022-08-19  8:57   ` Conor Dooley
2022-08-19  8:57 ` [PATCH v9 3/4] pwm: add microchip soft ip corePWM driver Conor Dooley
2022-08-19  8:57   ` Conor Dooley
2022-08-19  8:57 ` [PATCH v9 4/4] MAINTAINERS: add pwm to PolarFire SoC entry Conor Dooley
2022-08-19  8:57   ` Conor Dooley

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.