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

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

> +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.

> +	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.

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()?

> +	spinlock_t enable_lock;
> +};
> +
> +/*
> + * Channel 0 receives a state changed notification whenever the LPC interrupt
> + * fires.
> + *
> + * Channel 1 receives a state changed notification whenever the HPC interrupt
> + * fires.
> + */
> +static struct counter_signal rkpwmc_signals[] = {
> +	{
> +		.id = 0,
> +		.name = "Channel 0"
> +	},
> +	{
> +		.id = 1,
> +		.name = "Channel 1"
> +	},
> +};

From "31.3.1 Capture Mode" of the Rockchip RK3506 Technical Reference
Manual[^1], it looks like "clk_pwm" is the only signal that is counted
for PWM_HPC AND PWM_LPC. So instead of two Signals, you would have just
one Signal named "PWM Clock" which sources your two Synapses defined below.

Technically, "pwm_in" is also a signal evaluated (its polarity is used
to determine whether we're counting for PWM_HPC or PWM_LPC) but the
respective Synapse would be COUNTER_SYNAPSE_ACTION_NONE because this
signal not actually triggering the change in the count value. You're
welcome to add the "pwm_in" signal here if you like, but I'd say it's
optional if you actually want to expose it here.

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

Add COUNTER_SYNAPSE_ACTION_NONE to this array. When the polarity is
high, the Synapse for PWM_HPC will be active with
COUNTER_SYNAPSE_ACTION_BOTH_EDGES, while the Synapse for PWM_LPC will be
inactive with COUNTER_SYNAPSE_ACTION_NONE; the inverse occurs when the
polarity switches to low.

> +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]
> +	},
> +	{
> +		.actions_list = rkpwmc_hpc_lpc_actions,
> +		.num_actions = ARRAY_SIZE(rkpwmc_hpc_lpc_actions),
> +		.signal = &rkpwmc_signals[1]
> +	},
> +};

Both Synapses are sourced by the same single Signal ("PWM Clock") so set
both "signal" members to &rkpwmc_signals[0].

> +enum rkpwmc_count_id {
> +	COUNT_LPC = 0,
> +	COUNT_HPC = 1,
> +};
> +
> +static struct counter_count rkpwmc_counts[] = {
> +	{
> +		.id = COUNT_LPC,
> +		.name = "PWM core clock cycles during which the signal was low",
> +		.functions_list = rkpwmc_functions,
> +		.num_functions = ARRAY_SIZE(rkpwmc_functions),
> +		.synapses = rkpwmc_pwm_synapses,
> +		.num_synapses = ARRAY_SIZE(rkpwmc_pwm_synapses),
> +		.ext = rkpwmc_ext,
> +		.num_ext = ARRAY_SIZE(rkpwmc_ext),
> +	},
> +	{
> +		.id = COUNT_HPC,
> +		.name = "PWM core clock cycles during which the signal was high",
> +		.functions_list = rkpwmc_functions,
> +		.num_functions = ARRAY_SIZE(rkpwmc_functions),
> +		.synapses = rkpwmc_pwm_synapses,
> +		.num_synapses = ARRAY_SIZE(rkpwmc_pwm_synapses),
> +		.ext = rkpwmc_ext,
> +		.num_ext = ARRAY_SIZE(rkpwmc_ext),
> +	},
> +};

Count names should be short and descriptive. I think the RK3506 manual
section "31.4.2 Registers Summary" description for the PWM_HPC and
PWM_LPC register do a pretty good job of that:

* Low Polarity Capture
* High Polarity Capture

How about changing the two Count names to those respectively?

> +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.

> +
> +	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.

William Breathitt Gray

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