Re: [PATCH v2 3/4] pwm: kona: Fix enable, disable and config procedures

[Tim Kryger]

On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <sbranden@xxxxxxxxxxxx> wrote:
> From: Arun Ramamurthy <arunrama@xxxxxxxxxxxx>
>
> - Added helper functions to set and clear smooth and trigger bits
> - Added 400ns delays when clearing and setting trigger bit as requied
>   by spec
> - Added helper function to write prescale and other settings
> - Updated config procedure to match spec
> - Added code to handle pwn config when channel is disabled
> - Updated disable procedure to match spec
>
> Signed-off-by: Arun Ramamurthy <arunrama@xxxxxxxxxxxx>
> Reviewed-by: Ray Jui <rjui@xxxxxxxxxxxx>
> Signed-off-by: Scott Branden <sbranden@xxxxxxxxxxxx>
> ---
>  drivers/pwm/pwm-bcm-kona.c | 100 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 78 insertions(+), 22 deletions(-)

The driver is fairly small and this change rewrites a considerable amount of it.

Is there a actually specific deficiency that this change is intended to address?

I'm not sure all the extra helper functions improve readability.

>
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index fa0b5bf..06fa983 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -65,6 +65,10 @@
>  #define DUTY_CYCLE_HIGH_MIN                    (0x00000000)
>  #define DUTY_CYCLE_HIGH_MAX                    (0x00ffffff)
>
> +/* The delay required after clearing or setting
> +   PWMOUT_ENABLE*/
> +#define PWMOUT_ENABLE_HOLD_DELAY 400
> +
>  struct kona_pwmc {
>         struct pwm_chip chip;
>         void __iomem *base;
> @@ -76,28 +80,70 @@ static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip)
>         return container_of(_chip, struct kona_pwmc, chip);
>  }
>
> -static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> +static inline void kona_pwmc_set_trigger(struct kona_pwmc *kp,
> +                                        unsigned int chan)
>  {
>         unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>
> -       /* Clear trigger bit but set smooth bit to maintain old output */
> -       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
> +       /* set trigger bit to enable channel */
> +       value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
> +       writel(value, kp->base + PWM_CONTROL_OFFSET);
> +       ndelay(PWMOUT_ENABLE_HOLD_DELAY);
> +}
> +static inline void kona_pwmc_clear_trigger(struct kona_pwmc *kp,
> +                                          unsigned int chan)
> +{
> +       unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> +
> +       /* Clear trigger bit */
>         value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>         writel(value, kp->base + PWM_CONTROL_OFFSET);
> +       ndelay(PWMOUT_ENABLE_HOLD_DELAY);
> +}
>
> -       /* Set trigger bit and clear smooth bit to apply new settings */
> +static inline void kona_pwmc_clear_smooth(struct kona_pwmc *kp,
> +                                         unsigned int chan)
> +{
> +       unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> +
> +       /* Clear smooth bit */
>         value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
> -       value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
>         writel(value, kp->base + PWM_CONTROL_OFFSET);
>  }
>
> +static inline void kona_pwmc_set_smooth(struct kona_pwmc *kp, unsigned int chan)
> +{
> +       unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> +
> +       /*  set smooth bit to maintain old output */
> +       value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
> +       writel(value, kp->base + PWM_CONTROL_OFFSET);
> +}
> +
> +static void kona_pwmc_write_settings(struct kona_pwmc *kp, unsigned int chan,
> +                                    unsigned long prescale, unsigned long pc,
> +                                    unsigned long dc)
> +{
> +       unsigned int value;
> +
> +       value = readl(kp->base + PRESCALE_OFFSET);
> +       value &= ~PRESCALE_MASK(chan);
> +       value |= prescale << PRESCALE_SHIFT(chan);
> +       writel(value, kp->base + PRESCALE_OFFSET);
> +
> +       writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
> +
> +       writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> +
> +}
> +
>  static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>                             int duty_ns, int period_ns)
>  {
>         struct kona_pwmc *kp = to_kona_pwmc(chip);
>         u64 val, div, rate;
>         unsigned long prescale = PRESCALE_MIN, pc, dc;
> -       unsigned int value, chan = pwm->hwpwm;
> +       unsigned int ret, chan = pwm->hwpwm;
>
>         /*
>          * Find period count, duty count and prescale to suit duty_ns and
> @@ -133,19 +179,30 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>                         return -EINVAL;
>         }
>
> -       /* If the PWM channel is enabled, write the settings to the HW */
> -       if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> -               value = readl(kp->base + PRESCALE_OFFSET);
> -               value &= ~PRESCALE_MASK(chan);
> -               value |= prescale << PRESCALE_SHIFT(chan);
> -               writel(value, kp->base + PRESCALE_OFFSET);
>
> -               writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
> +       /* If the PWM channel is not enabled, enable the clock */
> +       if (!test_bit(PWMF_ENABLED, &pwm->flags)) {
> +               ret = clk_prepare_enable(kp->clk);
> +               if (ret < 0) {
> +                       dev_err(chip->dev, "failed to enable clock: %d\n", ret);
> +                       return ret;
> +               }
> +       }
>
> -               writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> +       /* Set smooth bit to maintain old output */
> +       kona_pwmc_set_smooth(kp, chan);
> +       kona_pwmc_clear_trigger(kp, chan);
> +
> +       /* apply new settings */
> +       kona_pwmc_write_settings(kp, chan, prescale, pc, dc);
> +
> +       /*If the PWM is enabled, enable the channel with the new settings
> +        and if not  disable the clock*/
> +       if (test_bit(PWMF_ENABLED, &pwm->flags))
> +               kona_pwmc_set_trigger(kp, chan);
> +       else
> +               clk_disable_unprepare(kp->clk);
>
> -               kona_pwmc_apply_settings(kp, chan);
> -       }
>
>         return 0;
>  }
> @@ -188,7 +245,6 @@ static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>                 dev_err(chip->dev, "failed to enable clock: %d\n", ret);
>                 return ret;
>         }
> -
>         ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
>         if (ret < 0) {
>                 clk_disable_unprepare(kp->clk);
> @@ -203,12 +259,12 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>         struct kona_pwmc *kp = to_kona_pwmc(chip);
>         unsigned int chan = pwm->hwpwm;
>
> +       kona_pwmc_clear_smooth(kp, chan);
> +       kona_pwmc_clear_trigger(kp, chan);

I believe the output will spike high here.  Likely not what you want...

>         /* Simulate a disable by configuring for zero duty */
> -       writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> -       kona_pwmc_apply_settings(kp, chan);
> -
> -       /* Wait for waveform to settle before gating off the clock */
> -       ndelay(400);
> +       kona_pwmc_write_settings(kp, chan, 0, 0, 0);
> +       kona_pwmc_set_polarity(chip, pwm, PWM_POLARITY_NORMAL);

This is wrong.  You shouldn't change the polarity when the PWM is disabled.

The original polarity isn't even restored when it is re-enabled...

> +       kona_pwmc_set_trigger(kp, chan);
>
>         clk_disable_unprepare(kp->clk);
>  }
> --
> 2.1.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/