Messages in this thread | | | Subject | Re: [PATCH 1/8] counter/ti-eqep: implement over/underflow events | From | David Lechner <> | Date | Wed, 27 Oct 2021 10:23:13 -0500 |
| |
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.
| |