| Message ID | 20211017013343.3385923-2-david@lechnology.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | counter: ti-eqep: implement features for speed measurement | expand |
On Sat, 16 Oct 2021 20:33:36 -0500 David Lechner <david@lechnology.com> wrote: > This adds support to the TI eQEP counter driver for subscribing to > overflow and underflow events using the counter chrdev interface. > > Since this is the first event added, this involved adding an irq > handler. Also, additional range checks had to be added to the ceiling > attribute to avoid infinite interrupts. > > Signed-off-by: David Lechner <david@lechnology.com> Hi David, A few minor things inline. I've not commented on the new counter interface stuff though as it's still a bit vague in my head, so over to William for that :) Jonathan > --- > drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 117 insertions(+), 2 deletions(-) > > diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c > index 09817c953f9a..b7c79435e127 100644 > --- a/drivers/counter/ti-eqep.c > +++ b/drivers/counter/ti-eqep.c > @@ -7,6 +7,7 @@ > > #include <linux/bitops.h> > #include <linux/counter.h> > +#include <linux/interrupt.h> > #include <linux/kernel.h> > #include <linux/mod_devicetable.h> > #include <linux/module.h> > @@ -67,6 +68,44 @@ > #define QEPCTL_UTE BIT(1) > #define QEPCTL_WDE BIT(0) > > +#define QEINT_UTO BIT(11) > +#define QEINT_IEL BIT(10) > +#define QEINT_SEL BIT(9) > +#define QEINT_PCM BIT(8) > +#define QEINT_PCR BIT(7) > +#define QEINT_PCO BIT(6) > +#define QEINT_PCU BIT(5) > +#define QEINT_WTO BIT(4) > +#define QEINT_QDC BIT(3) > +#define QEINT_PHE BIT(2) > +#define QEINT_PCE BIT(1) > + > +#define QFLG_UTO BIT(11) > +#define QFLG_IEL BIT(10) > +#define QFLG_SEL BIT(9) > +#define QFLG_PCM BIT(8) > +#define QFLG_PCR BIT(7) > +#define QFLG_PCO BIT(6) > +#define QFLG_PCU BIT(5) > +#define QFLG_WTO BIT(4) > +#define QFLG_QDC BIT(3) > +#define QFLG_PHE BIT(2) > +#define QFLG_PCE BIT(1) > +#define QFLG_INT BIT(0) > + > +#define QCLR_UTO BIT(11) > +#define QCLR_IEL BIT(10) > +#define QCLR_SEL BIT(9) > +#define QCLR_PCM BIT(8) > +#define QCLR_PCR BIT(7) > +#define QCLR_PCO BIT(6) > +#define QCLR_PCU BIT(5) > +#define QCLR_WTO BIT(4) > +#define QCLR_QDC BIT(3) > +#define QCLR_PHE BIT(2) > +#define QCLR_PCE BIT(1) > +#define QCLR_INT BIT(0) > + > /* EQEP Inputs */ > enum { > TI_EQEP_SIGNAL_QEPA, /* QEPA/XCLK */ > @@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter, > } > } > > +static int ti_eqep_events_configure(struct counter_device *counter) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + struct counter_event_node *event_node; > + u32 qeint = 0; > + > + list_for_each_entry(event_node, &counter->events_list, l) { > + switch (event_node->event) { > + case COUNTER_EVENT_OVERFLOW: > + qeint |= QEINT_PCO; > + break; > + case COUNTER_EVENT_UNDERFLOW: > + qeint |= QEINT_PCU; > + break; > + } > + } > + > + return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint); regmap_write() given mask is all bits. > +} > + > +static int ti_eqep_watch_validate(struct counter_device *counter, > + const struct counter_watch *watch) > +{ > + switch (watch->event) { > + case COUNTER_EVENT_OVERFLOW: > + case COUNTER_EVENT_UNDERFLOW: > + return 0; > + default: > + return -EINVAL; > + } > +} > + > static const struct counter_ops ti_eqep_counter_ops = { > .count_read = ti_eqep_count_read, > .count_write = ti_eqep_count_write, > .function_read = ti_eqep_function_read, > .function_write = ti_eqep_function_write, > .action_read = ti_eqep_action_read, > + .events_configure = ti_eqep_events_configure, > + .watch_validate = ti_eqep_watch_validate, > }; > > static int ti_eqep_position_ceiling_read(struct counter_device *counter, > @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter, > u64 ceiling) > { > struct ti_eqep_cnt *priv = counter->priv; > + u32 qposmax = ceiling; > > - if (ceiling != (u32)ceiling) > + /* ensure that value fits in 32-bit register */ > + if (qposmax != ceiling) > return -ERANGE; > > - regmap_write(priv->regmap32, QPOSMAX, ceiling); > + /* protect against infinite overflow interrupts */ > + if (qposmax == 0) > + return -EINVAL; > + > + regmap_write(priv->regmap32, QPOSMAX, qposmax); > > return 0; > } > @@ -349,6 +428,25 @@ static struct counter_count ti_eqep_counts[] = { > }, > }; > > +static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id) > +{ > + struct ti_eqep_cnt *priv = dev_id; > + struct counter_device *counter = &priv->counter; > + u32 qflg; > + > + regmap_read(priv->regmap16, QFLG, &qflg); > + > + if (qflg & QFLG_PCO) > + counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0); > + > + if (qflg & QFLG_PCU) > + counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0); > + > + regmap_set_bits(priv->regmap16, QCLR, ~0); Generally avoid clearing bits reflecting interrupt events you haven't handled. I'm guessing there is a potential race in here where we read qflg and additional events then occur before we clear. Those events we never see because they are unconditionally cleared by this write. We are better off interrupting again if that race happens. > + > + return IRQ_HANDLED; > +} > + > static const struct regmap_config ti_eqep_regmap32_config = { > .name = "32-bit", > .reg_bits = 32, > @@ -371,6 +469,7 @@ static int ti_eqep_probe(struct platform_device *pdev) > struct ti_eqep_cnt *priv; > void __iomem *base; > int err; > + int irq; > > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > if (!priv) > @@ -390,6 +489,15 @@ static int ti_eqep_probe(struct platform_device *pdev) > if (IS_ERR(priv->regmap16)) > return PTR_ERR(priv->regmap16); > > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + > + err = devm_request_threaded_irq(dev, irq, NULL, ti_eqep_irq_handler, > + IRQF_ONESHOT, dev_name(dev), priv); > + if (err < 0) > + return err; > + > priv->counter.name = dev_name(dev); > priv->counter.parent = dev; > priv->counter.ops = &ti_eqep_counter_ops; > @@ -409,6 +517,13 @@ static int ti_eqep_probe(struct platform_device *pdev) > pm_runtime_enable(dev); > pm_runtime_get_sync(dev); > > + /* > + * We can end up with an interupt infinite loop (interrupts triggered > + * as soon as they are cleared) if we leave this at the default value > + * of 0 and events are enabled. > + */ > + regmap_write(priv->regmap32, QPOSMAX, UINT_MAX); > + > err = counter_register(&priv->counter); > if (err < 0) { > pm_runtime_put_sync(dev);
On Sat, Oct 16, 2021 at 08:33:36PM -0500, David Lechner wrote: > This adds support to the TI eQEP counter driver for subscribing to > overflow and underflow events using the counter chrdev interface. > > Since this is the first event added, this involved adding an irq > handler. Also, additional range checks had to be added to the ceiling > attribute to avoid infinite interrupts. > > Signed-off-by: David Lechner <david@lechnology.com> Hi David, This looks functionally okay, but I have a couple minor comments inline. > --- > drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 117 insertions(+), 2 deletions(-) > > diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c > index 09817c953f9a..b7c79435e127 100644 > --- a/drivers/counter/ti-eqep.c > +++ b/drivers/counter/ti-eqep.c > @@ -7,6 +7,7 @@ > > #include <linux/bitops.h> > #include <linux/counter.h> > +#include <linux/interrupt.h> > #include <linux/kernel.h> > #include <linux/mod_devicetable.h> > #include <linux/module.h> > @@ -67,6 +68,44 @@ > #define QEPCTL_UTE BIT(1) > #define QEPCTL_WDE BIT(0) > > +#define QEINT_UTO BIT(11) > +#define QEINT_IEL BIT(10) > +#define QEINT_SEL BIT(9) > +#define QEINT_PCM BIT(8) > +#define QEINT_PCR BIT(7) > +#define QEINT_PCO BIT(6) > +#define QEINT_PCU BIT(5) > +#define QEINT_WTO BIT(4) > +#define QEINT_QDC BIT(3) > +#define QEINT_PHE BIT(2) > +#define QEINT_PCE BIT(1) > + > +#define QFLG_UTO BIT(11) > +#define QFLG_IEL BIT(10) > +#define QFLG_SEL BIT(9) > +#define QFLG_PCM BIT(8) > +#define QFLG_PCR BIT(7) > +#define QFLG_PCO BIT(6) > +#define QFLG_PCU BIT(5) > +#define QFLG_WTO BIT(4) > +#define QFLG_QDC BIT(3) > +#define QFLG_PHE BIT(2) > +#define QFLG_PCE BIT(1) > +#define QFLG_INT BIT(0) > + > +#define QCLR_UTO BIT(11) > +#define QCLR_IEL BIT(10) > +#define QCLR_SEL BIT(9) > +#define QCLR_PCM BIT(8) > +#define QCLR_PCR BIT(7) > +#define QCLR_PCO BIT(6) > +#define QCLR_PCU BIT(5) > +#define QCLR_WTO BIT(4) > +#define QCLR_QDC BIT(3) > +#define QCLR_PHE BIT(2) > +#define QCLR_PCE BIT(1) > +#define QCLR_INT BIT(0) > + > /* EQEP Inputs */ > enum { > TI_EQEP_SIGNAL_QEPA, /* QEPA/XCLK */ > @@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter, > } > } > > +static int ti_eqep_events_configure(struct counter_device *counter) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + struct counter_event_node *event_node; > + u32 qeint = 0; > + > + list_for_each_entry(event_node, &counter->events_list, l) { > + switch (event_node->event) { > + case COUNTER_EVENT_OVERFLOW: > + qeint |= QEINT_PCO; > + break; > + case COUNTER_EVENT_UNDERFLOW: > + qeint |= QEINT_PCU; > + break; > + } > + } > + > + return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint); > +} > + > +static int ti_eqep_watch_validate(struct counter_device *counter, > + const struct counter_watch *watch) > +{ > + switch (watch->event) { > + case COUNTER_EVENT_OVERFLOW: > + case COUNTER_EVENT_UNDERFLOW: > + return 0; > + default: > + return -EINVAL; > + } > +} > + > static const struct counter_ops ti_eqep_counter_ops = { > .count_read = ti_eqep_count_read, > .count_write = ti_eqep_count_write, > .function_read = ti_eqep_function_read, > .function_write = ti_eqep_function_write, > .action_read = ti_eqep_action_read, > + .events_configure = ti_eqep_events_configure, > + .watch_validate = ti_eqep_watch_validate, > }; > > static int ti_eqep_position_ceiling_read(struct counter_device *counter, > @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter, > u64 ceiling) > { > struct ti_eqep_cnt *priv = counter->priv; > + u32 qposmax = ceiling; > > - if (ceiling != (u32)ceiling) > + /* ensure that value fits in 32-bit register */ > + if (qposmax != ceiling) > return -ERANGE; > > - regmap_write(priv->regmap32, QPOSMAX, ceiling); > + /* protect against infinite overflow interrupts */ > + if (qposmax == 0) > + return -EINVAL; Would you be able to explain this scenario a bit further? My expectation would be that an overflow event would only occur if the position increased past the ceiling (i.e. increased to greater than 0). Of course, running the device with a ceiling of 0 effectively guarantees overflow eventss with every movement, but I would expect a stationary device to sit with a position of 0 and thus no overflow events. > + > + regmap_write(priv->regmap32, QPOSMAX, qposmax); > > return 0; > } > @@ -349,6 +428,25 @@ static struct counter_count ti_eqep_counts[] = { > }, > }; > > +static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id) > +{ > + struct ti_eqep_cnt *priv = dev_id; > + struct counter_device *counter = &priv->counter; > + u32 qflg; > + > + regmap_read(priv->regmap16, QFLG, &qflg); > + > + if (qflg & QFLG_PCO) > + counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0); > + > + if (qflg & QFLG_PCU) > + counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0); > + > + regmap_set_bits(priv->regmap16, QCLR, ~0); I agree with Jonathan here, it looks like this is unconditionally clearing all interrupt flags but it's possible new ones could have appear between the time after you read QFLG to here. Is it possible to clear just the interrupt flags you've handled here instead of all? William Breathitt Gray > + > + return IRQ_HANDLED; > +} > + > static const struct regmap_config ti_eqep_regmap32_config = { > .name = "32-bit", > .reg_bits = 32, > @@ -371,6 +469,7 @@ static int ti_eqep_probe(struct platform_device *pdev) > struct ti_eqep_cnt *priv; > void __iomem *base; > int err; > + int irq; > > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > if (!priv) > @@ -390,6 +489,15 @@ static int ti_eqep_probe(struct platform_device *pdev) > if (IS_ERR(priv->regmap16)) > return PTR_ERR(priv->regmap16); > > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + > + err = devm_request_threaded_irq(dev, irq, NULL, ti_eqep_irq_handler, > + IRQF_ONESHOT, dev_name(dev), priv); > + if (err < 0) > + return err; > + > priv->counter.name = dev_name(dev); > priv->counter.parent = dev; > priv->counter.ops = &ti_eqep_counter_ops; > @@ -409,6 +517,13 @@ static int ti_eqep_probe(struct platform_device *pdev) > pm_runtime_enable(dev); > pm_runtime_get_sync(dev); > > + /* > + * We can end up with an interupt infinite loop (interrupts triggered > + * as soon as they are cleared) if we leave this at the default value > + * of 0 and events are enabled. > + */ > + regmap_write(priv->regmap32, QPOSMAX, UINT_MAX); > + > err = counter_register(&priv->counter); > if (err < 0) { > pm_runtime_put_sync(dev); > -- > 2.25.1 >
On 10/25/21 2:13 AM, William Breathitt Gray wrote: > On Sat, Oct 16, 2021 at 08:33:36PM -0500, David Lechner wrote: >> This adds support to the TI eQEP counter driver for subscribing to >> overflow and underflow events using the counter chrdev interface. >> >> Since this is the first event added, this involved adding an irq >> handler. Also, additional range checks had to be added to the ceiling >> attribute to avoid infinite interrupts. >> >> Signed-off-by: David Lechner <david@lechnology.com> > > Hi David, > > This looks functionally okay, but I have a couple minor comments inline. > >> --- >> drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++- >> 1 file changed, 117 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c >> index 09817c953f9a..b7c79435e127 100644 >> --- a/drivers/counter/ti-eqep.c >> +++ b/drivers/counter/ti-eqep.c >> @@ -7,6 +7,7 @@ >> >> #include <linux/bitops.h> >> #include <linux/counter.h> >> +#include <linux/interrupt.h> >> #include <linux/kernel.h> >> #include <linux/mod_devicetable.h> >> #include <linux/module.h> >> @@ -67,6 +68,44 @@ >> #define QEPCTL_UTE BIT(1) >> #define QEPCTL_WDE BIT(0) >> >> +#define QEINT_UTO BIT(11) >> +#define QEINT_IEL BIT(10) >> +#define QEINT_SEL BIT(9) >> +#define QEINT_PCM BIT(8) >> +#define QEINT_PCR BIT(7) >> +#define QEINT_PCO BIT(6) >> +#define QEINT_PCU BIT(5) >> +#define QEINT_WTO BIT(4) >> +#define QEINT_QDC BIT(3) >> +#define QEINT_PHE BIT(2) >> +#define QEINT_PCE BIT(1) >> + >> +#define QFLG_UTO BIT(11) >> +#define QFLG_IEL BIT(10) >> +#define QFLG_SEL BIT(9) >> +#define QFLG_PCM BIT(8) >> +#define QFLG_PCR BIT(7) >> +#define QFLG_PCO BIT(6) >> +#define QFLG_PCU BIT(5) >> +#define QFLG_WTO BIT(4) >> +#define QFLG_QDC BIT(3) >> +#define QFLG_PHE BIT(2) >> +#define QFLG_PCE BIT(1) >> +#define QFLG_INT BIT(0) >> + >> +#define QCLR_UTO BIT(11) >> +#define QCLR_IEL BIT(10) >> +#define QCLR_SEL BIT(9) >> +#define QCLR_PCM BIT(8) >> +#define QCLR_PCR BIT(7) >> +#define QCLR_PCO BIT(6) >> +#define QCLR_PCU BIT(5) >> +#define QCLR_WTO BIT(4) >> +#define QCLR_QDC BIT(3) >> +#define QCLR_PHE BIT(2) >> +#define QCLR_PCE BIT(1) >> +#define QCLR_INT BIT(0) >> + >> /* EQEP Inputs */ >> enum { >> TI_EQEP_SIGNAL_QEPA, /* QEPA/XCLK */ >> @@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter, >> } >> } >> >> +static int ti_eqep_events_configure(struct counter_device *counter) >> +{ >> + struct ti_eqep_cnt *priv = counter->priv; >> + struct counter_event_node *event_node; >> + u32 qeint = 0; >> + >> + list_for_each_entry(event_node, &counter->events_list, l) { >> + switch (event_node->event) { >> + case COUNTER_EVENT_OVERFLOW: >> + qeint |= QEINT_PCO; >> + break; >> + case COUNTER_EVENT_UNDERFLOW: >> + qeint |= QEINT_PCU; >> + break; >> + } >> + } >> + >> + return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint); >> +} >> + >> +static int ti_eqep_watch_validate(struct counter_device *counter, >> + const struct counter_watch *watch) >> +{ >> + switch (watch->event) { >> + case COUNTER_EVENT_OVERFLOW: >> + case COUNTER_EVENT_UNDERFLOW: >> + return 0; >> + default: >> + return -EINVAL; >> + } >> +} >> + >> static const struct counter_ops ti_eqep_counter_ops = { >> .count_read = ti_eqep_count_read, >> .count_write = ti_eqep_count_write, >> .function_read = ti_eqep_function_read, >> .function_write = ti_eqep_function_write, >> .action_read = ti_eqep_action_read, >> + .events_configure = ti_eqep_events_configure, >> + .watch_validate = ti_eqep_watch_validate, >> }; >> >> static int ti_eqep_position_ceiling_read(struct counter_device *counter, >> @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter, >> u64 ceiling) >> { >> struct ti_eqep_cnt *priv = counter->priv; >> + u32 qposmax = ceiling; >> >> - if (ceiling != (u32)ceiling) >> + /* ensure that value fits in 32-bit register */ >> + if (qposmax != ceiling) >> return -ERANGE; >> >> - regmap_write(priv->regmap32, QPOSMAX, ceiling); >> + /* protect against infinite overflow interrupts */ >> + if (qposmax == 0) >> + return -EINVAL; > > Would you be able to explain this scenario a bit further? My expectation > would be that an overflow event would only occur if the position > increased past the ceiling (i.e. increased to greater than 0). Of > course, running the device with a ceiling of 0 effectively guarantees > overflow eventss with every movement, but I would expect a stationary > device to sit with a position of 0 and thus no overflow events. > This is just the way the hardware works. I discovered this the first time I enabled interrupts. Even if you clear the interrupt, it is triggered again immediately when QPOSMAX == 0.
On Wed, Oct 27, 2021 at 10:23:13AM -0500, David Lechner wrote: > On 10/25/21 2:13 AM, William Breathitt Gray wrote: > > On Sat, Oct 16, 2021 at 08:33:36PM -0500, David Lechner wrote: > >> @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter, > >> u64 ceiling) > >> { > >> struct ti_eqep_cnt *priv = counter->priv; > >> + u32 qposmax = ceiling; > >> > >> - if (ceiling != (u32)ceiling) > >> + /* ensure that value fits in 32-bit register */ > >> + if (qposmax != ceiling) > >> return -ERANGE; > >> > >> - regmap_write(priv->regmap32, QPOSMAX, ceiling); > >> + /* protect against infinite overflow interrupts */ > >> + if (qposmax == 0) > >> + return -EINVAL; > > > > Would you be able to explain this scenario a bit further? My expectation > > would be that an overflow event would only occur if the position > > increased past the ceiling (i.e. increased to greater than 0). Of > > course, running the device with a ceiling of 0 effectively guarantees > > overflow eventss with every movement, but I would expect a stationary > > device to sit with a position of 0 and thus no overflow events. > > > > This is just the way the hardware works. I discovered this the first > time I enabled interrupts. Even if you clear the interrupt, it is > triggered again immediately when QPOSMAX == 0. For this device, does an overflow event occur once the count value increases to equal the ceiling value, or once the count value increases past the ceiling value? The Counter interface defines ceiling as an inclusive upper limit (count value is capable of reaching ceiling) and defines COUNTER_EVENT_OVERFLOW as occuring when the count value increases past ceiling. I want to make sure the ceiling extension and COUNTER_EVENT_OVERFLOW events for this driver are behaving as expected of the Counter interface. Let's use a non-zero example to be clear. Suppose we set ceiling equal to 10. If count is currently at 9 and increases by 1, count should become 10 and no COUNTER_EVENT_OVERFLOW event is expected to trigger; if count is 10 and further increases, count should _not_ become 11 (staying at 10 or starting over at the floor) but a COUNTER_EVENT_OVERFLOW event does trigger. Does the driver behave like this currently? William Breathitt Gray
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c index 09817c953f9a..b7c79435e127 100644 --- a/drivers/counter/ti-eqep.c +++ b/drivers/counter/ti-eqep.c @@ -7,6 +7,7 @@ #include <linux/bitops.h> #include <linux/counter.h> +#include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/mod_devicetable.h> #include <linux/module.h> @@ -67,6 +68,44 @@ #define QEPCTL_UTE BIT(1) #define QEPCTL_WDE BIT(0) +#define QEINT_UTO BIT(11) +#define QEINT_IEL BIT(10) +#define QEINT_SEL BIT(9) +#define QEINT_PCM BIT(8) +#define QEINT_PCR BIT(7) +#define QEINT_PCO BIT(6) +#define QEINT_PCU BIT(5) +#define QEINT_WTO BIT(4) +#define QEINT_QDC BIT(3) +#define QEINT_PHE BIT(2) +#define QEINT_PCE BIT(1) + +#define QFLG_UTO BIT(11) +#define QFLG_IEL BIT(10) +#define QFLG_SEL BIT(9) +#define QFLG_PCM BIT(8) +#define QFLG_PCR BIT(7) +#define QFLG_PCO BIT(6) +#define QFLG_PCU BIT(5) +#define QFLG_WTO BIT(4) +#define QFLG_QDC BIT(3) +#define QFLG_PHE BIT(2) +#define QFLG_PCE BIT(1) +#define QFLG_INT BIT(0) + +#define QCLR_UTO BIT(11) +#define QCLR_IEL BIT(10) +#define QCLR_SEL BIT(9) +#define QCLR_PCM BIT(8) +#define QCLR_PCR BIT(7) +#define QCLR_PCO BIT(6) +#define QCLR_PCU BIT(5) +#define QCLR_WTO BIT(4) +#define QCLR_QDC BIT(3) +#define QCLR_PHE BIT(2) +#define QCLR_PCE BIT(1) +#define QCLR_INT BIT(0) + /* EQEP Inputs */ enum { TI_EQEP_SIGNAL_QEPA, /* QEPA/XCLK */ @@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter, } } +static int ti_eqep_events_configure(struct counter_device *counter) +{ + struct ti_eqep_cnt *priv = counter->priv; + struct counter_event_node *event_node; + u32 qeint = 0; + + list_for_each_entry(event_node, &counter->events_list, l) { + switch (event_node->event) { + case COUNTER_EVENT_OVERFLOW: + qeint |= QEINT_PCO; + break; + case COUNTER_EVENT_UNDERFLOW: + qeint |= QEINT_PCU; + break; + } + } + + return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint); +} + +static int ti_eqep_watch_validate(struct counter_device *counter, + const struct counter_watch *watch) +{ + switch (watch->event) { + case COUNTER_EVENT_OVERFLOW: + case COUNTER_EVENT_UNDERFLOW: + return 0; + default: + return -EINVAL; + } +} + static const struct counter_ops ti_eqep_counter_ops = { .count_read = ti_eqep_count_read, .count_write = ti_eqep_count_write, .function_read = ti_eqep_function_read, .function_write = ti_eqep_function_write, .action_read = ti_eqep_action_read, + .events_configure = ti_eqep_events_configure, + .watch_validate = ti_eqep_watch_validate, }; static int ti_eqep_position_ceiling_read(struct counter_device *counter, @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter, u64 ceiling) { struct ti_eqep_cnt *priv = counter->priv; + u32 qposmax = ceiling; - if (ceiling != (u32)ceiling) + /* ensure that value fits in 32-bit register */ + if (qposmax != ceiling) return -ERANGE; - regmap_write(priv->regmap32, QPOSMAX, ceiling); + /* protect against infinite overflow interrupts */ + if (qposmax == 0) + return -EINVAL; + + regmap_write(priv->regmap32, QPOSMAX, qposmax); return 0; } @@ -349,6 +428,25 @@ static struct counter_count ti_eqep_counts[] = { }, }; +static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id) +{ + struct ti_eqep_cnt *priv = dev_id; + struct counter_device *counter = &priv->counter; + u32 qflg; + + regmap_read(priv->regmap16, QFLG, &qflg); + + if (qflg & QFLG_PCO) + counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0); + + if (qflg & QFLG_PCU) + counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0); + + regmap_set_bits(priv->regmap16, QCLR, ~0); + + return IRQ_HANDLED; +} + static const struct regmap_config ti_eqep_regmap32_config = { .name = "32-bit", .reg_bits = 32, @@ -371,6 +469,7 @@ static int ti_eqep_probe(struct platform_device *pdev) struct ti_eqep_cnt *priv; void __iomem *base; int err; + int irq; priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) @@ -390,6 +489,15 @@ static int ti_eqep_probe(struct platform_device *pdev) if (IS_ERR(priv->regmap16)) return PTR_ERR(priv->regmap16); + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + err = devm_request_threaded_irq(dev, irq, NULL, ti_eqep_irq_handler, + IRQF_ONESHOT, dev_name(dev), priv); + if (err < 0) + return err; + priv->counter.name = dev_name(dev); priv->counter.parent = dev; priv->counter.ops = &ti_eqep_counter_ops; @@ -409,6 +517,13 @@ static int ti_eqep_probe(struct platform_device *pdev) pm_runtime_enable(dev); pm_runtime_get_sync(dev); + /* + * We can end up with an interupt infinite loop (interrupts triggered + * as soon as they are cleared) if we leave this at the default value + * of 0 and events are enabled. + */ + regmap_write(priv->regmap32, QPOSMAX, UINT_MAX); + err = counter_register(&priv->counter); if (err < 0) { pm_runtime_put_sync(dev);
This adds support to the TI eQEP counter driver for subscribing to overflow and underflow events using the counter chrdev interface. Since this is the first event added, this involved adding an irq handler. Also, additional range checks had to be added to the ceiling attribute to avoid infinite interrupts. Signed-off-by: David Lechner <david@lechnology.com> --- drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++- 1 file changed, 117 insertions(+), 2 deletions(-)