| Message ID | 20250227144023.64530-3-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | microchip-tcb-capture: Add Capture, Compare, Overflow etc. events | expand |
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
Hi, On 2025. 03. 04. 8:02, William Breathitt Gray wrote: > 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. Yes, `of_irq_get()` can return an error, for example if the IRQ is not defined in the DT. In these cases, we just don't do IRQ, but still allow the device to probe. -EPROBE_DEFER is special in this case, because it signifies that there *is* an IRQ to set up, just not now. >> +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. Okay. I personally like the enum interface more, as it is much cleaner, but if there's a good reason to use #define's, then so be it. > 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 Bence
On Tue, Mar 04, 2025 at 10:57:05AM +0100, Csókás Bence wrote: > On 2025. 03. 04. 8:02, William Breathitt Gray wrote: > > 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. > > Yes, `of_irq_get()` can return an error, for example if the IRQ is not > defined in the DT. In these cases, we just don't do IRQ, but still allow the > device to probe. -EPROBE_DEFER is special in this case, because it signifies > that there *is* an IRQ to set up, just not now. You're right, that makes sense. Thank you for explaining. William Breathitt Gray
diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c index 2f096a5b973d..cc12c2e2113a 100644 --- a/drivers/counter/microchip-tcb-capture.c +++ b/drivers/counter/microchip-tcb-capture.c @@ -6,18 +6,24 @@ */ #include <linux/clk.h> #include <linux/counter.h> +#include <linux/interrupt.h> #include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/mutex.h> #include <linux/of.h> +#include <linux/of_irq.h> #include <linux/platform_device.h> #include <linux/regmap.h> +#include <uapi/linux/counter/microchip-tcb-capture.h> #include <soc/at91/atmel_tcb.h> #define ATMEL_TC_CMR_MASK (ATMEL_TC_LDRA_RISING | ATMEL_TC_LDRB_FALLING | \ ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_LDBDIS | \ ATMEL_TC_LDBSTOP) +#define ATMEL_TC_DEF_IRQS (ATMEL_TC_ETRGS | ATMEL_TC_COVFS | \ + ATMEL_TC_LDRAS | ATMEL_TC_LDRBS | ATMEL_TC_CPCS) + #define ATMEL_TC_QDEN BIT(8) #define ATMEL_TC_POSEN BIT(9) @@ -27,6 +33,7 @@ struct mchp_tc_data { int qdec_mode; int num_channels; int channel[2]; + int irq; }; static const enum counter_function mchp_tc_count_functions[] = { @@ -294,6 +301,65 @@ static const struct of_device_id atmel_tc_of_match[] = { { /* sentinel */ } }; +static irqreturn_t mchp_tc_isr(int irq, void *dev_id) +{ + struct counter_device *const counter = dev_id; + struct mchp_tc_data *const priv = counter_priv(counter); + u32 sr, mask; + + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr); + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], IMR), &mask); + + sr &= mask; + if (!(sr & ATMEL_TC_ALL_IRQ)) + return IRQ_NONE; + + if (sr & ATMEL_TC_ETRGS) + counter_push_event(counter, COUNTER_EVENT_CHANGE_OF_STATE, + COUNTER_MCHP_EVCHN_CV); + if (sr & ATMEL_TC_LDRAS) + counter_push_event(counter, COUNTER_EVENT_CAPTURE, + COUNTER_MCHP_EVCHN_RA); + if (sr & ATMEL_TC_LDRBS) + counter_push_event(counter, COUNTER_EVENT_CAPTURE, + COUNTER_MCHP_EVCHN_RB); + if (sr & ATMEL_TC_CPCS) + counter_push_event(counter, COUNTER_EVENT_THRESHOLD, + COUNTER_MCHP_EVCHN_RC); + if (sr & ATMEL_TC_COVFS) + counter_push_event(counter, COUNTER_EVENT_OVERFLOW, + COUNTER_MCHP_EVCHN_CV); + + return IRQ_HANDLED; +} + +static void mchp_tc_irq_remove(void *ptr) +{ + struct mchp_tc_data *priv = ptr; + + regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], IDR), ATMEL_TC_DEF_IRQS); +} + +static int mchp_tc_irq_enable(struct counter_device *const counter) +{ + struct mchp_tc_data *const priv = counter_priv(counter); + int ret = devm_request_irq(counter->parent, priv->irq, mchp_tc_isr, 0, + dev_name(counter->parent), counter); + + if (ret < 0) + return ret; + + ret = regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], IER), ATMEL_TC_DEF_IRQS); + if (ret < 0) + return ret; + + ret = devm_add_action_or_reset(counter->parent, mchp_tc_irq_remove, priv); + if (ret < 0) + return ret; + + return 0; +} + static void mchp_tc_clk_remove(void *ptr) { clk_disable_unprepare((struct clk *)ptr); @@ -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; + if (priv->irq > 0) { + ret = mchp_tc_irq_enable(counter); + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, "Failed to set up IRQ"); + } + ret = devm_counter_add(&pdev->dev, counter); if (ret < 0) return dev_err_probe(&pdev->dev, ret, "Failed to add counter\n"); 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, +}; + #endif /* _UAPI_COUNTER_MCHP_TCB_H_ */
Add interrupt servicing to allow userspace to wait for the following events: * Change-of-state caused by external trigger * Capture of timer value into RA/RB * Compare to RC register * Overflow Signed-off-by: Bence Csókás <csokas.bence@prolan.hu> --- Notes: New in v2 Changes in v3: * Add IRQs for Capture events (from next patch) * Add IRQ for RC Compare * Add events as bullet points to commit msg Changes in v4: * Add uapi header, names for the event channels * Add check for -EPROBE_DEFER from `of_irq_get()` Changes in v5: * Split out UAPI header introduction drivers/counter/microchip-tcb-capture.c | 75 +++++++++++++++++++ .../linux/counter/microchip-tcb-capture.h | 18 +++++ 2 files changed, 93 insertions(+)