lkml.org 
[lkml]   [2021]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/8] counter/ti-eqep: implement over/underflow events
From
Date
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.

\
 
 \ /
  Last update: 2021-10-27 17:24    [W:0.122 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site