Re: [PATCH v5 4/6] counter: Add rockchip-pwm-capture driver

[William Breathitt Gray <wbg@kernel.org>]

On Mon, Apr 20, 2026 at 03:52:41PM +0200, Nicolas Frattaroli wrote:
> Among many other things, Rockchip's new PWMv4 IP in the RK3576 supports
> PWM capture functionality.
> 
> Add a basic driver for this that works to expose HPC/LPC counts and
> state change events to userspace through the counter framework. It's
> quite basic, but works well enough to demonstrate the device function
> exclusion stuff that mfpwm does, in order to eventually support all the
> functions of this device in drivers within their appropriate subsystems,
> without them interfering with each other.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Hi Nicolas,

Forgive me if I asked this before, but I'm having trouble finding it
online: do you have a link to a publicly available RK357 technical
reference manual? I think that will help me better understand how this
PWMv4 IP works.

Regardless, some comments inline below from my current understanding.

> +static struct counter_signal rkpwmc_signals[] = {
> +	{
> +		.id = 0,
> +		.name = "PWM Clock"
> +	},
> +};

If the capture mode is used to measure the duty cycle of the PWM input,
then we actually have two Signals to define here: "PWM Clock" and "PWM
Input".

I imagine the capture mode waveforms look something like this:

               __    __    __    __    __    __    __
    clk     __|  |__|  |__|  |__|  |__|  |__|  |__|  |__
    
                      __________________ LPC: 2 edges ____
    pwm_in  _________|   HPC: 3 edges   |____________|

So the level of the pwm_in signal is counted each rising edge of clk;
the number of clk edges while pwm_in is high is the HPC count, whereas
the number of clk edges while pwm_in is low is the LPC count.

In the Generic Counter paradigm, this would look like the following:

    Signals             Synapses              Counts
    =======             ========              ======
    +-----------+                               _______________________
    |           | <---- "Rising Edge" ---+---> /                       \
    |"PWM Clock"|                        |     |"High Polarity Capture"|
    |           |                    +---|---> \                       /
    +-----------+                    |   |      -----------------------
                                     |   |
    +-----------+                    |   |      _______________________
    |           |                    |   +---> /                       \
    |"PWM Input"|                    |         | "Low Polarity Capture |
    |           | <---- "None" ------+-------> \                       /
    +-----------+                               -----------------------

The key idea is that the clock and PWM input Signals are associated to
both Counts (HPC and LPC) through their respective Synapses. However,
while the clock Synapse ("Rising Edge") indicates the clock Signal
state triggers updates in the Counts, the PWM input Synapse ("None")
indicates the PWM input Signal state does not trigger but is simply
evaluated to determine the new Count value.

> +static const enum counter_synapse_action rkpwmc_hpc_lpc_actions[] = {
> +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +	COUNTER_SYNAPSE_ACTION_NONE,
> +};

To simplify my above example, I assumed that the HPC and LPC counts are
only updated on rising edges of the clock Signal. If the Count values
actually update on both edges, then you don't need to make a change
here. Otherwise, change the "both edges" enum constant to "rising edge"
or "falling edge" as required.

> +static struct counter_synapse rkpwmc_pwm_synapses[] = {
> +	{
> +		.actions_list = rkpwmc_hpc_lpc_actions,
> +		.num_actions = ARRAY_SIZE(rkpwmc_hpc_lpc_actions),
> +		.signal = &rkpwmc_signals[0]
> +	},
> +};

Add a Synapse here for the "PWM Input" Signal.

You should also implement an action_read() callback. Check the value of
synapse->signal->id to determine which Signal is associated to the
Synapse and set the respective action; i.e. for the PWM clock set
COUNTER_SYNAPSE_ACTION_BOTH_EDGES, while for the PWM input set
COUNTER_SYNAPSE_ACTION_NONE.

> +static const enum counter_function rkpwmc_functions[] = {
> +	COUNTER_FUNCTION_INCREASE,
> +};

I wonder if we need a new enum counter_function constant to express
what's happening in this driver. In theory, the
COUNTER_FUNCTION_INCREASE represent a Count whose value only increases,
but what we're describing here is more of a duty cycle sample, right?

I haven't made up my mind on this, so for now you can stick with
COUNTER_FUNCTION_INCREASE. I'll reconsider it in the next revision.

> +static int rkpwmc_enable_write(struct counter_device *counter,
> +			       struct counter_count *count,
> +			       u8 enable)
> +{
> +	struct rockchip_pwm_capture *pc = counter_priv(counter);
> +	int ret;
> +
> +	ret = mfpwm_acquire(pc->pwmf);
> +	if (ret)
> +		return ret;
> +
> +	if (!!enable != rkpwmc_is_enabled(pc->pwmf)) {

The Counter subsystem gurantees enable is a boolean value so there's no
need for the double negation here in the conditional.

Also, instead of checking if the enable value is different from
rkpwm_is_enabled(), check if it is the same and exit early. That will
avoid the large conditional block as what you have inside can now move
outside after the conditional check.

> +		if (enable) {
> +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
> +					 PWMV4_EN(false));
> +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL,
> +					 PWMV4_CTRL_CAP_FLAGS);
> +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_INT_EN,
> +					 PWMV4_INT_LPC_W(true) |
> +					 PWMV4_INT_HPC_W(true));
> +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
> +					 PWMV4_EN(true) | PWMV4_CLK_EN(true));
> +
> +			ret = clk_enable(pc->pwmf->core);
> +			if (ret)
> +				goto err_release;
> +
> +			ret = clk_rate_exclusive_get(pc->pwmf->core);
> +			if (ret)
> +				goto err_disable_pwm_clk;
> +
> +			ret = mfpwm_acquire(pc->pwmf);
> +			if (ret)
> +				goto err_unprotect_pwm_clk;
> +		} else {
> +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_INT_EN,
> +					 PWMV4_INT_LPC_W(false) |
> +					 PWMV4_INT_HPC_W(false));
> +			mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_ENABLE,
> +					 PWMV4_EN(false) | PWMV4_CLK_EN(false));
> +			clk_rate_exclusive_put(pc->pwmf->core);
> +			clk_disable(pc->pwmf->core);
> +			mfpwm_release(pc->pwmf);
> +		}
> +	}
> +
> +	mfpwm_release(pc->pwmf);

The call to mfpwm_release() in the else block is redundant because it is
called immediately again here.

William Breathitt Gray

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip