Re: [PATCH v5 5/8] pwm: axi-pwmgen: Implementation of the waveform callbacks

[Trevor Gamblin <tgamblin@baylibre.com>]

On 2024-09-20 04:58, Uwe Kleine-König wrote:
> Convert the axi-pwmgen driver to use the new callbacks for hardware
> programming.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Tested-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
>   drivers/pwm/pwm-axi-pwmgen.c | 154 ++++++++++++++++++++++++-----------
>   1 file changed, 108 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
> index b5477659ba18..39d184417c7c 100644
> --- a/drivers/pwm/pwm-axi-pwmgen.c
> +++ b/drivers/pwm/pwm-axi-pwmgen.c
> @@ -23,6 +23,7 @@
>   #include <linux/err.h>
>   #include <linux/fpga/adi-axi-common.h>
>   #include <linux/io.h>
> +#include <linux/minmax.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
>   #include <linux/pwm.h>
> @@ -53,81 +54,142 @@ static const struct regmap_config axi_pwmgen_regmap_config = {
>   	.max_register = 0xFC,
>   };
>   
> -static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> -			    const struct pwm_state *state)
> +/* This represents a hardware configuration for one channel */
> +struct axi_pwmgen_waveform {
> +	u32 period_cnt;
> +	u32 duty_cycle_cnt;
> +	u32 duty_offset_cnt;
> +};
> +
> +static int axi_pwmgen_round_waveform_tohw(struct pwm_chip *chip,
> +					  struct pwm_device *pwm,
> +					  const struct pwm_waveform *wf,
> +					  void *_wfhw)
>   {
> +	struct axi_pwmgen_waveform *wfhw = _wfhw;
> +	struct axi_pwmgen_ddata *ddata = pwmchip_get_drvdata(chip);
> +
> +	if (wf->period_length_ns == 0) {
> +		*wfhw = (struct axi_pwmgen_waveform){
> +			.period_cnt = 0,
> +			.duty_cycle_cnt = 0,
> +			.duty_offset_cnt = 0,
> +		};
> +	} else {
> +		/* With ddata->clk_rate_hz < NSEC_PER_SEC this won't overflow. */
> +		wfhw->period_cnt = min_t(u64,
> +					 mul_u64_u32_div(wf->period_length_ns, ddata->clk_rate_hz, NSEC_PER_SEC),
> +					 U32_MAX);
> +
> +		if (wfhw->period_cnt == 0) {
> +			/*
> +			 * The specified period is too short for the hardware.
> +			 * Let's round .duty_cycle down to 0 to get a (somewhat)
> +			 * valid result.
> +			 */
> +			wfhw->period_cnt = 1;
> +			wfhw->duty_cycle_cnt = 0;
> +			wfhw->duty_offset_cnt = 0;
> +		} else {
> +			wfhw->duty_cycle_cnt = min_t(u64,
> +						     mul_u64_u32_div(wf->duty_length_ns, ddata->clk_rate_hz, NSEC_PER_SEC),
> +						     U32_MAX);
> +			wfhw->duty_offset_cnt = min_t(u64,
> +						      mul_u64_u32_div(wf->duty_offset_ns, ddata->clk_rate_hz, NSEC_PER_SEC),
> +						      U32_MAX);
> +		}
> +	}
> +
> +	dev_dbg(&chip->dev, "pwm#%u: %lld/%lld [+%lld] @%lu -> PERIOD: %08x, DUTY: %08x, OFFSET: %08x\n",
> +		pwm->hwpwm, wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns,
> +		ddata->clk_rate_hz, wfhw->period_cnt, wfhw->duty_cycle_cnt, wfhw->duty_offset_cnt);
> +
> +	return 0;
> +}
> +
> +static int axi_pwmgen_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
> +					     const void *_wfhw, struct pwm_waveform *wf)
> +{
> +	const struct axi_pwmgen_waveform *wfhw = _wfhw;
> +	struct axi_pwmgen_ddata *ddata = pwmchip_get_drvdata(chip);
> +
> +	wf->period_length_ns = DIV64_U64_ROUND_UP((u64)wfhw->period_cnt * NSEC_PER_SEC,
> +					ddata->clk_rate_hz);
> +
> +	wf->duty_length_ns = DIV64_U64_ROUND_UP((u64)wfhw->duty_cycle_cnt * NSEC_PER_SEC,
> +					    ddata->clk_rate_hz);
> +
> +	wf->duty_offset_ns = DIV64_U64_ROUND_UP((u64)wfhw->duty_offset_cnt * NSEC_PER_SEC,
> +					     ddata->clk_rate_hz);
> +
> +	return 0;
> +}
> +
> +static int axi_pwmgen_write_waveform(struct pwm_chip *chip,
> +				     struct pwm_device *pwm,
> +				     const void *_wfhw)
> +{
> +	const struct axi_pwmgen_waveform *wfhw = _wfhw;
>   	struct axi_pwmgen_ddata *ddata = pwmchip_get_drvdata(chip);
> -	unsigned int ch = pwm->hwpwm;
>   	struct regmap *regmap = ddata->regmap;
> -	u64 period_cnt, duty_cnt;
> +	unsigned int ch = pwm->hwpwm;
>   	int ret;
>   
> -	if (state->polarity != PWM_POLARITY_NORMAL)
> -		return -EINVAL;
> +	ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), wfhw->period_cnt);
> +	if (ret)
> +		return ret;
>   
> -	if (state->enabled) {
> -		period_cnt = mul_u64_u64_div_u64(state->period, ddata->clk_rate_hz, NSEC_PER_SEC);
> -		if (period_cnt > UINT_MAX)
> -			period_cnt = UINT_MAX;
> +	ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), wfhw->duty_cycle_cnt);
> +	if (ret)
> +		return ret;
>   
> -		if (period_cnt == 0)
> -			return -EINVAL;
> -
> -		ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), period_cnt);
> -		if (ret)
> -			return ret;
> -
> -		duty_cnt = mul_u64_u64_div_u64(state->duty_cycle, ddata->clk_rate_hz, NSEC_PER_SEC);
> -		if (duty_cnt > UINT_MAX)
> -			duty_cnt = UINT_MAX;
> -
> -		ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), duty_cnt);
> -		if (ret)
> -			return ret;
> -	} else {
> -		ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), 0);
> -		if (ret)
> -			return ret;
> -
> -		ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), 0);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = regmap_write(regmap, AXI_PWMGEN_CHX_OFFSET(ch), wfhw->duty_offset_cnt);
> +	if (ret)
> +		return ret;
>   
>   	return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_LOAD_CONFIG);
>   }
>   
> -static int axi_pwmgen_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> -				struct pwm_state *state)
> +static int axi_pwmgen_read_waveform(struct pwm_chip *chip,
> +				    struct pwm_device *pwm,
> +				    void *_wfhw)
>   {
> +	struct axi_pwmgen_waveform *wfhw = _wfhw;
>   	struct axi_pwmgen_ddata *ddata = pwmchip_get_drvdata(chip);
>   	struct regmap *regmap = ddata->regmap;
>   	unsigned int ch = pwm->hwpwm;
> -	u32 cnt;
>   	int ret;
>   
> -	ret = regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &cnt);
> +	ret = regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &wfhw->period_cnt);
>   	if (ret)
>   		return ret;
>   
> -	state->enabled = cnt != 0;
> -
> -	state->period = DIV_ROUND_UP_ULL((u64)cnt * NSEC_PER_SEC, ddata->clk_rate_hz);
> -
> -	ret = regmap_read(regmap, AXI_PWMGEN_CHX_DUTY(ch), &cnt);
> +	ret = regmap_read(regmap, AXI_PWMGEN_CHX_DUTY(ch), &wfhw->duty_cycle_cnt);
>   	if (ret)
>   		return ret;
>   
> -	state->duty_cycle = DIV_ROUND_UP_ULL((u64)cnt * NSEC_PER_SEC, ddata->clk_rate_hz);
> +	ret = regmap_read(regmap, AXI_PWMGEN_CHX_OFFSET(ch), &wfhw->duty_offset_cnt);
> +	if (ret)
> +		return ret;
>   
> -	state->polarity = PWM_POLARITY_NORMAL;
> +	if (wfhw->duty_cycle_cnt > wfhw->period_cnt)
> +		wfhw->duty_cycle_cnt = wfhw->period_cnt;
> +
> +	/* XXX: is this the actual behaviour of the hardware? */
> +	if (wfhw->duty_offset_cnt >= wfhw->period_cnt) {
> +		wfhw->duty_cycle_cnt = 0;
> +		wfhw->duty_offset_cnt = 0;
> +	}
>   
>   	return 0;
>   }
>   
>   static const struct pwm_ops axi_pwmgen_pwm_ops = {
> -	.apply = axi_pwmgen_apply,
> -	.get_state = axi_pwmgen_get_state,
> +	.sizeof_wfhw = sizeof(struct axi_pwmgen_waveform),
> +	.round_waveform_tohw = axi_pwmgen_round_waveform_tohw,
> +	.round_waveform_fromhw = axi_pwmgen_round_waveform_fromhw,
> +	.read_waveform = axi_pwmgen_read_waveform,
> +	.write_waveform = axi_pwmgen_write_waveform,
>   };
>   
>   static int axi_pwmgen_setup(struct regmap *regmap, struct device *dev)