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

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

Hi Damon,

On Sunday, 26 April 2026 12:55:20 Central European Summer Time Damon Ding wrote:
> Hi Nicolas,
> 
> On 4/20/2026 9:52 PM, 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>
> > ---
> >   MAINTAINERS                            |   1 +
> >   drivers/counter/Kconfig                |  11 ++
> >   drivers/counter/Makefile               |   1 +
> >   drivers/counter/rockchip-pwm-capture.c | 307 +++++++++++++++++++++++++++++++++
> >   4 files changed, 320 insertions(+)
> > 
> 
> For functional validation, I connected PWM0/PWM1 (continuous output)
> to PWM2 (capture input) pairwise.
> 
> I enabled the counter via:
> /sys/bus/counter/devices/counter0/count0/enable
> 
> Then I verified the functionality by reading the count values from:
> /sys/bus/counter/devices/counter0/count0/count
> /sys/bus/counter/devices/counter0/count1/count
> 
> Tested-by: Damon Ding <damon.ding@rock-chips.com>

Thanks for testing! To make sure a bug on the pwm output driver won't
be cancelled out by an equivalent bug in the counter driver, I did
my counter driver testing by using an RK3588 as the source of the
PWM signal, with the two boards sharing a common ground. Maybe I
should get a proper function generator. :)

> BTW: Is there any user-space test tool similar to libpwm for the
> counter subsystem?

I've tried looking for this as well, and couldn't find anything. If
it exists then adding a mention of it to generic-counter.rst would
be in order I think.

> 
> ......
> > diff --git a/drivers/counter/rockchip-pwm-capture.c b/drivers/counter/rockchip-pwm-capture.c
> > new file mode 100644
> > index 000000000000..09a92f2bc409
> > --- /dev/null
> > +++ b/drivers/counter/rockchip-pwm-capture.c
> > @@ -0,0 +1,307 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2025 Collabora Ltd.
> > + *
> > + * A counter driver for the Pulse-Width-Modulation (PWM) hardware found on
> > + * Rockchip SoCs such as the RK3576, internally referred to as "PWM v4". It
> > + * allows for measuring the high cycles and low cycles of a PWM signal through
> > + * the generic counter framework, while guaranteeing exclusive use over the
> > + * MFPWM device while the counter is enabled.
> > + *
> > + * Authors:
> > + *     Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/counter.h>
> > +#include <linux/devm-helpers.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/rockchip-mfpwm.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spinlock.h>
> > +
> > +#define RKPWMC_INT_MASK			(PWMV4_INT_LPC | PWMV4_INT_HPC)
> > +
> > +struct rockchip_pwm_capture {
> > +	struct rockchip_mfpwm_func *pwmf;
> > +	struct counter_device *counter;
> > +};
> > +
> > +static struct counter_signal rkpwmc_signals[] = {
> > +	{
> > +		.id = 0,
> > +		.name = "PWM Clock"
> > +	},
> > +};
> > +
> > +static const enum counter_synapse_action rkpwmc_hpc_lpc_actions[] = {
> > +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> > +	COUNTER_SYNAPSE_ACTION_NONE,
> > +};
> 
> For the capture function, it uses the PWM's reference clock (dclk) as 
> the time base to measure how many reference cycles the high and low 
> levels of the input waveform last respectively.
> 
> I find it a bit strange to set COUNTER_SYNAPSE_ACTION_BOTH_EDGES for 
> counting. If we treat the input waveform as a sequence of square waves 
> sampled by dclk cycles, it feels like we should count on a single edge 
> (rising edge only) rather than both edges.
> 

Yeah, that's a good point. I think I've been struggling to wrap my head
around what the signal is and what the synapse should trigger on because
the signal isn't something exposed in this case.

Your explanation makes sense, and perhaps I should rename
rkpwmc_hpc_lpc_actions as well. Currently it makes it seem like HPC
and LPC are the signal, when actually the waveform is the signal, HPC
and LPC are the counts, and the synapse is a sample every dclk spotting
a transition if I understand this correctly.

> > +
> > +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]
> > +	},
> > +};
> > +
> 
> Best regards,
> Damon
> 

Kind regards,
Nicolas Frattaroli