From: <Conor.Dooley@microchip.com>
To: <dan.carpenter@oracle.com>, <kbuild@lists.01.org>,
<thierry.reding@gmail.com>, <u.kleine-koenig@pengutronix.de>,
<robh+dt@kernel.org>, <krzk@kernel.org>
Cc: <lkp@intel.com>, <kbuild-all@lists.01.org>,
<Daire.McNamara@microchip.com>, <devicetree@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-pwm@vger.kernel.org>,
<linux-riscv@lists.infradead.org>
Subject: Re: [PATCH v9 3/4] pwm: add microchip soft ip corePWM driver
Date: Mon, 22 Aug 2022 09:18:26 +0000 [thread overview]
Message-ID: <be336a32-e729-b357-7db0-bacc02776cb2@microchip.com> (raw)
In-Reply-To: <202208212329.XETz1mt0-lkp@intel.com>
On 22/08/2022 09:42, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi Conor,
>
> url: https://github.com/intel-lab-lkp/linux/commits/Conor-Dooley/Microchip-soft-ip-corePWM-driver/20220819-170106
> base: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
> config: arm64-randconfig-m031-20220821 (https://download.01.org/0day-ci/archive/20220821/202208212329.XETz1mt0-lkp@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'.
Totally correct, there's a missing unlock. I'll send a v10 at some stage this week.
Thanks Dan,
Conor.
>
> vim +295 drivers/pwm/pwm-microchip-core.c
>
> 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;
>
> mutex_unlock(&mchp_core_pwm->lock); before the retun?
>
> 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 }
>
> --
> 0-DAY CI Kernel Test Service
> https://01.org/lkp
>
WARNING: multiple messages have this Message-ID (diff)
From: <Conor.Dooley@microchip.com>
To: <dan.carpenter@oracle.com>, <kbuild@lists.01.org>,
<thierry.reding@gmail.com>, <u.kleine-koenig@pengutronix.de>,
<robh+dt@kernel.org>, <krzk@kernel.org>
Cc: <lkp@intel.com>, <kbuild-all@lists.01.org>,
<Daire.McNamara@microchip.com>, <devicetree@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-pwm@vger.kernel.org>,
<linux-riscv@lists.infradead.org>
Subject: Re: [PATCH v9 3/4] pwm: add microchip soft ip corePWM driver
Date: Mon, 22 Aug 2022 09:18:26 +0000 [thread overview]
Message-ID: <be336a32-e729-b357-7db0-bacc02776cb2@microchip.com> (raw)
In-Reply-To: <202208212329.XETz1mt0-lkp@intel.com>
On 22/08/2022 09:42, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi Conor,
>
> url: https://github.com/intel-lab-lkp/linux/commits/Conor-Dooley/Microchip-soft-ip-corePWM-driver/20220819-170106
> base: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
> config: arm64-randconfig-m031-20220821 (https://download.01.org/0day-ci/archive/20220821/202208212329.XETz1mt0-lkp@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'.
Totally correct, there's a missing unlock. I'll send a v10 at some stage this week.
Thanks Dan,
Conor.
>
> vim +295 drivers/pwm/pwm-microchip-core.c
>
> 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;
>
> mutex_unlock(&mchp_core_pwm->lock); before the retun?
>
> 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 }
>
> --
> 0-DAY CI Kernel Test Service
> https://01.org/lkp
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
WARNING: multiple messages have this Message-ID (diff)
From: Conor.Dooley@microchip.com
To: kbuild-all@lists.01.org
Subject: Re: [PATCH v9 3/4] pwm: add microchip soft ip corePWM driver
Date: Mon, 22 Aug 2022 09:18:26 +0000 [thread overview]
Message-ID: <be336a32-e729-b357-7db0-bacc02776cb2@microchip.com> (raw)
In-Reply-To: <202208212329.XETz1mt0-lkp@intel.com>
[-- Attachment #1: Type: text/plain, Size: 9506 bytes --]
On 22/08/2022 09:42, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi Conor,
>
> url: https://github.com/intel-lab-lkp/linux/commits/Conor-Dooley/Microchip-soft-ip-corePWM-driver/20220819-170106
> base: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
> 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'.
Totally correct, there's a missing unlock. I'll send a v10 at some stage this week.
Thanks Dan,
Conor.
>
> vim +295 drivers/pwm/pwm-microchip-core.c
>
> 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;
>
> mutex_unlock(&mchp_core_pwm->lock); before the retun?
>
> 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 }
>
> --
> 0-DAY CI Kernel Test Service
> https://01.org/lkp
>
next prev parent reply other threads:[~2022-08-22 9:19 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=be336a32-e729-b357-7db0-bacc02776cb2@microchip.com \
--to=conor.dooley@microchip.com \
--cc=Daire.McNamara@microchip.com \
--cc=dan.carpenter@oracle.com \
--cc=devicetree@vger.kernel.org \
--cc=kbuild-all@lists.01.org \
--cc=kbuild@lists.01.org \
--cc=krzk@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=lkp@intel.com \
--cc=robh+dt@kernel.org \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.