Hello Biju,
On Thu, Aug 14, 2025 at 12:50:20PM +0100, Biju wrote:
> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Convert the rzg2l-gpt driver to use the new callbacks for hardware
> programming.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> v1->v2:
> * Dropped modifing hardware from .round_waveform_tohw() callback.
> ---
> drivers/pwm/pwm-rzg2l-gpt.c | 175 +++++++++++++++++++++---------------
> 1 file changed, 102 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
> index 360c8bf3b190..f0a8531457ca 100644
> --- a/drivers/pwm/pwm-rzg2l-gpt.c
> +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> @@ -86,6 +86,13 @@ struct rzg2l_gpt_chip {
> u32 channel_enable_count[RZG2L_MAX_HW_CHANNELS];
> };
>
> +/* This represents a hardware configuration for one channel */
> +struct rzg2l_gpt_waveform {
> + u32 gtpr;
> + u32 gtccr;
> + u8 prescale;
> +};
> +
> static inline struct rzg2l_gpt_chip *to_rzg2l_gpt_chip(struct pwm_chip *chip)
> {
> return pwmchip_get_drvdata(chip);
> @@ -190,8 +197,10 @@ static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *rzg2l_gpt,
> /* Stop count, Output low on GTIOCx pin when counting stops */
> rzg2l_gpt->channel_enable_count[ch]--;
>
> - if (!rzg2l_gpt->channel_enable_count[ch])
> + if (!rzg2l_gpt->channel_enable_count[ch]) {
> rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_CST, 0);
> + rzg2l_gpt->period_ticks[ch] = 0;
> + }
I wonder if this is a fix that is orthogonal to the waveform conversion.
Shouldn't that be a separate commit?
> /* Disable pin output */
> rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR(ch), RZG2L_GTIOR_OxE(sub_ch), 0);
> @@ -215,54 +224,37 @@ static u64 rzg2l_gpt_calculate_period_or_duty(struct rzg2l_gpt_chip *rzg2l_gpt,
> return DIV64_U64_ROUND_UP(tmp, rzg2l_gpt->rate_khz);
> }
>
> -static int rzg2l_gpt_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> - struct pwm_state *state)
> -{
> - struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> -
> - state->enabled = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm);
> - if (state->enabled) {
> - u32 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> - u32 ch = RZG2L_GET_CH(pwm->hwpwm);
> - u8 prescale;
> - u32 val;
> -
> - val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR(ch));
> - prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
> -
> - val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR(ch));
> - state->period = rzg2l_gpt_calculate_period_or_duty(rzg2l_gpt, val, prescale);
> -
> - val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch));
> - state->duty_cycle = rzg2l_gpt_calculate_period_or_duty(rzg2l_gpt, val, prescale);
> - if (state->duty_cycle > state->period)
> - state->duty_cycle = state->period;
> - }
> -
> - state->polarity = PWM_POLARITY_NORMAL;
> -
> - return 0;
> -}
> -
> static u32 rzg2l_gpt_calculate_pv_or_dc(u64 period_or_duty_cycle, u8 prescale)
> {
> return min_t(u64, DIV_ROUND_DOWN_ULL(period_or_duty_cycle, 1 << (2 * prescale)),
> U32_MAX);
> }
>
> -/* Caller holds the lock while calling rzg2l_gpt_config() */
> -static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> - const struct pwm_state *state)
> +static int rzg2l_gpt_round_waveform_tohw(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + const struct pwm_waveform *wf,
> + void *_wfhw)
> +
> {
> struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> - u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> + struct rzg2l_gpt_waveform *wfhw = _wfhw;
> u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> u64 period_ticks, duty_ticks;
> unsigned long pv, dc;
> - u8 prescale;
> +
> + guard(mutex)(&rzg2l_gpt->lock);
> + if (wf->period_length_ns == 0) {
> + *wfhw = (struct rzg2l_gpt_waveform){
> + .gtpr = 0,
> + .gtccr = 0,
> + .prescale = 0,
> + };
> +
> + return 0;
> + }
>
> /* Limit period/duty cycle to max value supported by the HW */
> - period_ticks = mul_u64_u64_div_u64(state->period, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> + period_ticks = mul_u64_u64_div_u64(wf->period_length_ns, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> if (period_ticks > RZG2L_MAX_TICKS)
> period_ticks = RZG2L_MAX_TICKS;
> /*
The code that follows here needs adaption. Other than .apply(),
.round_waveform_tohw() is supposed to not fail if the requested period
is too small but use the smallest possible value then (and return 1).
> @@ -277,13 +269,14 @@ static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> period_ticks = rzg2l_gpt->period_ticks[ch];
> }
>
> - prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_ticks);
> - pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, prescale);
> -
> - duty_ticks = mul_u64_u64_div_u64(state->duty_cycle, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> + wfhw->prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_ticks);
> + pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, wfhw->prescale);
> + wfhw->gtpr = pv;
> + duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> if (duty_ticks > period_ticks)
> duty_ticks = period_ticks;
> - dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, prescale);
> + dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, wfhw->prescale);
> + wfhw->gtccr = dc;
>
> /*
> * GPT counter is shared by multiple channels, we cache the period ticks
> @@ -292,6 +285,58 @@ static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> */
> rzg2l_gpt->period_ticks[ch] = period_ticks;
>
> + return 0;
> +}
> +
> +static int rzg2l_gpt_round_waveform_fromhw(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + const void *_wfhw,
> + struct pwm_waveform *wf)
> +{
> + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> + const struct rzg2l_gpt_waveform *wfhw = _wfhw;
> +
> + wf->period_length_ns = rzg2l_gpt_calculate_period_or_duty(rzg2l_gpt, wfhw->gtpr,
> + wfhw->prescale);
> + wf->duty_length_ns = rzg2l_gpt_calculate_period_or_duty(rzg2l_gpt, wfhw->gtccr,
> + wfhw->prescale);
> + wf->duty_offset_ns = 0;
> +
> + return 0;
> +}
> +
> +static int rzg2l_gpt_read_waveform(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + void *_wfhw)
> +{
> + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> + struct rzg2l_gpt_waveform *wfhw = _wfhw;
> + u32 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> + u32 ch = RZG2L_GET_CH(pwm->hwpwm);
> + u32 gtcr;
> +
Why does rzg2l_gpt_write_waveform() grab the lock but
rzg2l_gpt_read_waveform() doesn't?
> + if (rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm)) {
> + gtcr = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR(ch));
> + wfhw->prescale = FIELD_GET(RZG2L_GTCR_TPCS, gtcr);
> + wfhw->gtpr = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR(ch));
> + wfhw->gtccr = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch));
> + if (wfhw->gtccr > wfhw->gtpr)
> + wfhw->gtccr = wfhw->gtpr;
> + }
If rzg2l_gpt_is_ch_enabled() returns false wfhw must still be
initialized, I guess using:
*wfhw = (struct rzg2l_gpt_waveform) { };
Minor optimization: rzg2l_gpt_is_ch_enabled() reads RZG2L_GTCR(ch)
already, would be nice to only do it once.
> + return 0;
> +}
> +
> +static int rzg2l_gpt_write_waveform(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + const void *_wfhw)
> +{
> + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> + const struct rzg2l_gpt_waveform *wfhw = _wfhw;
> + u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> + u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> +
> + guard(mutex)(&rzg2l_gpt->lock);
> /*
> * Counter must be stopped before modifying mode, prescaler, timer
> * counter and buffer enable registers. These registers are shared
> @@ -310,14 +355,14 @@ static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
>
> /* Select count clock */
> rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_TPCS,
> - FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
> + FIELD_PREP(RZG2L_GTCR_TPCS, wfhw->prescale));
>
> /* Set period */
> - rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR(ch), pv);
> + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR(ch), wfhw->gtpr);
> }
>
> /* Set duty cycle */
> - rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch), dc);
> + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch), wfhw->gtccr);
>
> if (rzg2l_gpt->channel_enable_count[ch] <= 1) {
> /* Set initial value for counter */
> @@ -326,44 +371,28 @@ static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> /* Set no buffer operation */
> rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTBER(ch), 0);
>
> - /* Restart the counter after updating the registers */
> - rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch),
> - RZG2L_GTCR_CST, RZG2L_GTCR_CST);
> - }
> -
> - return 0;
> -}
.write_waveform() must make sure to not disturb the other channel. So
this is the place where you need to check if the other channel relies on
the current value of global period (and fail with an error if yes and
the period value to write is different).
> -
> -static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> - const struct pwm_state *state)
> -{
> - struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> - bool enabled = pwm->state.enabled;
> - int ret;
> -
> - if (state->polarity != PWM_POLARITY_NORMAL)
> - return -EINVAL;
> -
> - guard(mutex)(&rzg2l_gpt->lock);
> - if (!state->enabled) {
> - if (enabled)
> - rzg2l_gpt_disable(rzg2l_gpt, pwm);
> -
> - return 0;
> + if (wfhw->gtpr)
> + /* Restart the counter after updating the registers */
> + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch),
> + RZG2L_GTCR_CST, RZG2L_GTCR_CST);
> }
>
> - ret = rzg2l_gpt_config(chip, pwm, state);
> - if (!ret && !enabled)
> + if (wfhw->gtpr && !rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm))
> rzg2l_gpt_enable(rzg2l_gpt, pwm);
> + else if (!wfhw->gtpr && rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm))
> + rzg2l_gpt_disable(rzg2l_gpt, pwm);
>
> - return ret;
> + return 0;
> }
>
> static const struct pwm_ops rzg2l_gpt_ops = {
> .request = rzg2l_gpt_request,
> .free = rzg2l_gpt_free,
> - .get_state = rzg2l_gpt_get_state,
> - .apply = rzg2l_gpt_apply,
> + .sizeof_wfhw = sizeof(struct rzg2l_gpt_waveform),
> + .round_waveform_tohw = rzg2l_gpt_round_waveform_tohw,
> + .round_waveform_fromhw = rzg2l_gpt_round_waveform_fromhw,
> + .read_waveform = rzg2l_gpt_read_waveform,
> + .write_waveform = rzg2l_gpt_write_waveform,
> };
>
> static int rzg2l_gpt_probe(struct platform_device *pdev)
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
