Re: [PATCH v3 4/5] counter: Add rockchip-pwm-capture driver

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

Hi,

finally got an opportunity to work on this again.

I'll respond to some things in-line. If a review isn't directly
addressed, you can assume I acknowledge it and will address it
in the next revision with no further comment needed.

On Saturday, 6 December 2025 10:34:17 Central European Summer Time William Breathitt Gray wrote:
> > +struct rockchip_pwm_capture {
> > +	struct rockchip_mfpwm_func *pwmf;
> > +	struct counter_device *counter;
> 
> Is this structure member used at all? If not, you should just remove it.

The counter member is used in the interrupt handler. I actually
noticed that I request the interrupt before pc->counter is set,
so if an interrupt fires before the probe function finishes then
I think the handler would run with a NULL counter member. Oops,
I'll rectify that.

> > +	bool is_enabled;
> 
> Does this device offer some way to probe whether PWM capture mode is
> enabled? I suspect so, because I see PWM_ENABLE.pwm_en enables the
> channel and PWM_CTRL.pwm_mode selects capture mode, so perhaps we're
> able to read the current state of those registers. If you're able to
> read those registers to determine the enable state, I'd suggest wrapping
> that into a helper function and calling it when you need to determine
> whether the capture mode is currently enabled.

I'm going to read the hardware state in the next revision, you're right
that this is generally a better idea.

> 
> If we're not able to probe the enable state, is it safe to assume we're
> in a disabled state when the module loads, or should we ensure it by
> unconditionally disabling PWM capture mode during
> rockchip_pwm_capture_probe()?

In my next revision, I've now modified it to mfpwm_acquire if the hardware
state has the counter enabled during probe. This sounds niche but I'm also
doing this on the PWM output side, where Uwe rightfully pointed out that
a bootloader may have enabled PWM output in hardware and Linux needs to
recognise that state without any heavy-handed actions. For the counter
PWM capture side, resetting it to a known state wouldn't be disruptive in
the same way as it would be for PWM output, but I think it's a good idea
to keep the state as-is since we can read it.

> [... snip ...]

> > +static int rkpwmc_count_read(struct counter_device *counter,
> > +			     struct counter_count *count, u64 *value)
> > +{
> > +	struct rockchip_pwm_capture *pc = counter_priv(counter);
> > +
> > +	guard(spinlock)(&pc->enable_lock);
> > +
> > +	if (!pc->is_enabled) {
> > +		*value = 0;
> > +		return 0;
> > +	}
> 
> I don't think there's a need to check whether capture mode is disabled.
> The user should be aware of the enable state of the Count by checking
> the respective "enable" attribute; and if they ignore that, a value of
> "0" doesn't inherently tell them that the Count is disabled which makes
> it moot to do so. I'd suggest just removing this check entirely and
> returning the register values unconditionally.

I see what you're going for, but if the counter isn't enabled, we can't
rely in the counter having an mfpwm_acquire, and consequently, we can't
rely on the PWM core clock being on, which is required for reading the
registers.

In my next revision, I'll still be returning 0 if the counter is disabled,
but the is_enabled member is gone, so there's a new function called
rkpwmc_acquire_if_enabled to still make this correct.

I could of course also extend the core driver to let me poke at these
non-shared registers without exclusive control over the hardware, but
that may be more trouble than it's worth.

I'll also no longer return 0 on bogus count IDs when the counter is
disabled.

> > +
> > +	switch (count->id) {
> > +	case COUNT_LPC:
> > +		*value = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_LPC);
> > +		return 0;
> > +	case COUNT_HPC:
> > +		*value = mfpwm_reg_read(pc->pwmf->base, PWMV4_REG_HPC);
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct counter_ops rkpwmc_ops = {
> > +	.count_read = rkpwmc_count_read,
> > +};
> 
> You should implement a signal_read() callback if its possible to probe
> the current state of PWM Clock. You should implement action_read() if
> its possible to probe the current polarity of "pwm_in" in order to set
> which Synapse is currently active.

Unfortunately, it doesn't seem like the hardware allows direct access to
read the signal. "pwm_in" as it appears in the block diagram seems to be
an entirely internal signal that's not accessible through MMIO.

Thank you for the reviews!

Kind regards,
Nicolas Frattaroli

> 
> William Breathitt Gray
> 
> [^1] https://opensource.rock-chips.com/images/3/36/Rockchip_RK3506_TRM_Part_1_V1.2-20250811.pdf
>