Re: [PATCH v6 2/3] counter: microchip-tcb-capture: Add IRQ handling

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

[-- Attachment #1: Type: text/plain, Size: 2549 bytes --]

On Thu, Feb 27, 2025 at 03:40:19PM +0100, Bence Csókás wrote:
> Add interrupt servicing to allow userspace to wait for the following events:

Hi Bence,

This is a nitpick but keep the commit description with a maximum of 75
characters per line so we don't have formatting issues with how they
wrap.

> @@ -378,6 +444,15 @@ static int mchp_tc_probe(struct platform_device *pdev)
>  	counter->num_signals = ARRAY_SIZE(mchp_tc_count_signals);
>  	counter->signals = mchp_tc_count_signals;
>  
> +	priv->irq = of_irq_get(np->parent, 0);
> +	if (priv->irq == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

In theory, the error code could be something else if of_irq_get() failed
for any other reason. Handle all those error cases at once by checking
IS_ERR(priv->irq) rather than just -EPROBE_DEFER. Then you can just
return dev_err_probe() with priv->irq for the error code.

> diff --git a/include/uapi/linux/counter/microchip-tcb-capture.h b/include/uapi/linux/counter/microchip-tcb-capture.h
> index 7bda5fdef19b..ee72f1463594 100644
> --- a/include/uapi/linux/counter/microchip-tcb-capture.h
> +++ b/include/uapi/linux/counter/microchip-tcb-capture.h
> @@ -12,6 +12,17 @@
>   * Count 0
>   * \__  Synapse 0 -- Signal 0 (Channel A, i.e. TIOA)
>   * \__  Synapse 1 -- Signal 1 (Channel B, i.e. TIOB)
> + *
> + * It also supports the following events:
> + *
> + * Channel 0:
> + * - CV register changed
> + * - CV overflowed
> + * - RA captured
> + * Channel 1:
> + * - RB captured
> + * Channel 2:
> + * - RC compare triggered
>   */
>  
>  enum counter_mchp_signals {
> @@ -19,4 +30,11 @@ enum counter_mchp_signals {
>  	COUNTER_MCHP_SIG_TIOB,
>  };
>  
> +enum counter_mchp_event_channels {
> +	COUNTER_MCHP_EVCHN_CV = 0,
> +	COUNTER_MCHP_EVCHN_RA = 0,
> +	COUNTER_MCHP_EVCHN_RB,
> +	COUNTER_MCHP_EVCHN_RC,
> +};

These would be better as preprocessor defines in case we need to
introduce new events to channel 1 or 2 in the future. That would allow
us to insert new events easily to existing channels without having to
worry about its actual position in an enum list.

One additional benefit is if we do end up introducing more Counts for
the module. In that situation we would have multiple CV and RA/RB/RC per
Counter device, but we can easily define a preprocessor macro to
calculate the channel offset given the Count index. However, with enum
structure we would have to manually add and maintain redundant defines
for each Count, which is far less ideal.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]