Re: [PATCH 4/4] pwm: omap-dmtimer: Implement .apply callback

[Uwe Kleine-König]

On Mon, Feb 24, 2020 at 10:51:35AM +0530, Lokesh Vutla wrote:
> Implement .apply callback and drop the legacy callbacks(enable, disable,
> config, set_polarity).
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@xxxxxx>
> ---
>  drivers/pwm/pwm-omap-dmtimer.c | 141 +++++++++++++++++++--------------
>  1 file changed, 80 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> index 58c61559e72f..aeda4ab12385 100644
> --- a/drivers/pwm/pwm-omap-dmtimer.c
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -31,8 +31,18 @@
>  #define DM_TIMER_LOAD_MIN 0xfffffffe
>  #define DM_TIMER_MAX      0xffffffff
>  
> +/**
> + * struct pwm_omap_dmtimer_chip - Structure representing a pwm chip
> + *				  corresponding to omap dmtimer.
> + * @chip:		PWM chip structure representing PWM controller
> + * @mutex:		Mutex to protect pwm apply state
> + * @dm_timer:		Pointer to omap dm timer.
> + * @pdata:		Pointer to omap dm timer ops.
> + * dm_timer_pdev:	Pointer to omap dm timer platform device
> + */
>  struct pwm_omap_dmtimer_chip {
>  	struct pwm_chip chip;
> +	/* Mutex to protect pwm apply state */
>  	struct mutex mutex;
>  	struct omap_dm_timer *dm_timer;
>  	const struct omap_dm_timer_ops *pdata;
> @@ -45,11 +55,22 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip)
>  	return container_of(chip, struct pwm_omap_dmtimer_chip, chip);
>  }
>  
> +/**
> + * pwm_omap_dmtimer_get_clock_cycles() - Get clock cycles in a time frame
> + * @clk_rate:	pwm timer clock rate
> + * @ns:		time frame in nano seconds.
> + *
> + * Return number of clock cycles in a given period(ins ns).
> + */
>  static u32 pwm_omap_dmtimer_get_clock_cycles(unsigned long clk_rate, int ns)
>  {
>  	return DIV_ROUND_CLOSEST_ULL((u64)clk_rate * ns, NSEC_PER_SEC);
>  }
>  
> +/**
> + * pwm_omap_dmtimer_start() - Start the pwm omap dm timer in pwm mode
> + * @omap:	Pointer to pwm omap dm timer chip
> + */
>  static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap)
>  {
>  	/*
> @@ -67,32 +88,16 @@ static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap)
>  	omap->pdata->start(omap->dm_timer);
>  }
>  
> -static int pwm_omap_dmtimer_enable(struct pwm_chip *chip,
> -				   struct pwm_device *pwm)
> -{
> -	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
> -
> -	mutex_lock(&omap->mutex);
> -	omap->pdata->set_pwm(omap->dm_timer,
> -			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
> -			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
> -
> -	pwm_omap_dmtimer_start(omap);
> -	mutex_unlock(&omap->mutex);
> -
> -	return 0;
> -}
> -
> -static void pwm_omap_dmtimer_disable(struct pwm_chip *chip,
> -				     struct pwm_device *pwm)
> -{
> -	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
> -
> -	mutex_lock(&omap->mutex);
> -	omap->pdata->stop(omap->dm_timer);
> -	mutex_unlock(&omap->mutex);
> -}
> -
> +/**
> + * pwm_omap_dmtimer_config() - Update the configuration of pwm omap dm timer
> + * @chip:	Pointer to PWM controller
> + * @pwm:	Pointer to PWM channel
> + * @duty_ns:	New duty cycle in nano seconds
> + * @period_ns:	New period in nano seconds
> + *
> + * Return 0 if successfully changed the period/duty_cycle else appropriate
> + * error.
> + */
>  static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  				   struct pwm_device *pwm,
>  				   int duty_ns, int period_ns)
> @@ -100,30 +105,26 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
>  	u32 period_cycles, duty_cycles;
>  	u32 load_value, match_value;
> -	struct clk *fclk;
>  	unsigned long clk_rate;
> +	struct clk *fclk;
>  
>  	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
>  		duty_ns, period_ns);
>  
> -	mutex_lock(&omap->mutex);
>  	if (duty_ns == pwm_get_duty_cycle(pwm) &&
> -	    period_ns == pwm_get_period(pwm)) {
> -		/* No change - don't cause any transients. */
> -		mutex_unlock(&omap->mutex);
> +	    period_ns == pwm_get_period(pwm))
>  		return 0;
> -	}
>  
>  	fclk = omap->pdata->get_fclk(omap->dm_timer);
>  	if (!fclk) {
>  		dev_err(chip->dev, "invalid pmtimer fclk\n");
> -		goto err_einval;
> +		return -EINVAL;
>  	}
>  
>  	clk_rate = clk_get_rate(fclk);
>  	if (!clk_rate) {
>  		dev_err(chip->dev, "invalid pmtimer fclk rate\n");
> -		goto err_einval;
> +		return -EINVAL;
>  	}
>  
>  	dev_dbg(chip->dev, "clk rate: %luHz\n", clk_rate);
> @@ -151,7 +152,7 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  		dev_info(chip->dev,
>  			 "period %d ns too short for clock rate %lu Hz\n",
>  			 period_ns, clk_rate);
> -		goto err_einval;
> +		return -EINVAL;
>  	}
>  
>  	if (duty_cycles < 1) {
> @@ -183,54 +184,72 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  	dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
>  		load_value, load_value,	match_value, match_value);
>  
> -	mutex_unlock(&omap->mutex);
> -
>  	return 0;
> -
> -err_einval:
> -	mutex_unlock(&omap->mutex);
> -
> -	return -EINVAL;
>  }
>  
> -static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip,
> -					 struct pwm_device *pwm,
> -					 enum pwm_polarity polarity)
> +/**
> + * pwm_omap_dmtimer_apply() - Changes the state of the pwm omap dm timer.
> + * @chip:	Pointer to PWM controller
> + * @pwm:	Pointer to PWM channel
> + * @state:	New sate to apply
> + *
> + * Return 0 if successfully changed the state else appropriate error.
> + */
> +static int pwm_omap_dmtimer_apply(struct pwm_chip *chip,
> +				  struct pwm_device *pwm,
> +				  const struct pwm_state *state)
>  {
>  	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
> +	int ret = 0;
>  
> -	/*
> -	 * PWM core will not call set_polarity while PWM is enabled so it's
> -	 * safe to reconfigure the timer here without stopping it first.
> -	 */
>  	mutex_lock(&omap->mutex);
> -	omap->pdata->set_pwm(omap->dm_timer,
> -			     polarity == PWM_POLARITY_INVERSED,
> -			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
> +
> +	if (pwm_is_enabled(pwm) && !state->enabled) {

In my book calling PWM API functions (designed for PWM consumers) is not
so nice. I would prefer you checking the hardware registers or cache the
state locally instead of relying on the core here.

It would be great to have a general description at the top of the driver
(like for example drivers/pwm/pwm-sifive.c) that answers things like:

 - Does calling .stop completes the currently running period (it
   should)?
 - Does changing polarity, duty_cycle and period complete the running
   period?
 - How does the hardware behave on disable? (i.e. does it output the
   state the pin is at in that moment? Does it go High-Z?)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |