Re: [PATCH v9 3/4] pwm: add microchip soft ip corePWM driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx/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@xxxxxxxxx>
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> 
> 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
> 





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux