Re: [PATCH 2/8] counter/ti-eqep: add support for direction

[William Breathitt Gray]

On Sat, Oct 16, 2021 at 08:33:37PM -0500, David Lechner wrote:
> This adds support for direction to the TI eQEP counter driver. It adds
> both a direction attribute to sysfs and a direction change event to
> the chrdev. The direction change event type is new public API.
> 
> Signed-off-by: David Lechner <david@xxxxxxxxxxxxxx>

Just one minor comment below regarding the IRQ handler; the rest of the
patch is fine.

> ---
>  drivers/counter/ti-eqep.c    | 33 +++++++++++++++++++++++++++++++++
>  include/uapi/linux/counter.h |  2 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index b7c79435e127..9881e5115da6 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -106,6 +106,15 @@
>  #define QCLR_PCE		BIT(1)
>  #define QCLR_INT		BIT(0)
>  
> +#define QEPSTS_UPEVNT		BIT(7)
> +#define QEPSTS_FDF		BIT(6)
> +#define QEPSTS_QDF		BIT(5)
> +#define QEPSTS_QDLF		BIT(4)
> +#define QEPSTS_COEF		BIT(3)
> +#define QEPSTS_CDEF		BIT(2)
> +#define QEPSTS_FIMF		BIT(1)
> +#define QEPSTS_PCEF		BIT(0)
> +
>  /* EQEP Inputs */
>  enum {
>  	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
> @@ -286,6 +295,9 @@ static int ti_eqep_events_configure(struct counter_device *counter)
>  		case COUNTER_EVENT_UNDERFLOW:
>  			qeint |= QEINT_PCU;
>  			break;
> +		case COUNTER_EVENT_DIRECTION_CHANGE:
> +			qeint |= QEINT_QDC;
> +			break;
>  		}
>  	}
>  
> @@ -298,6 +310,7 @@ static int ti_eqep_watch_validate(struct counter_device *counter,
>  	switch (watch->event) {
>  	case COUNTER_EVENT_OVERFLOW:
>  	case COUNTER_EVENT_UNDERFLOW:
> +	case COUNTER_EVENT_DIRECTION_CHANGE:
>  		return 0;
>  	default:
>  		return -EINVAL;
> @@ -371,11 +384,27 @@ static int ti_eqep_position_enable_write(struct counter_device *counter,
>  	return 0;
>  }
>  
> +static int ti_eqep_direction_read(struct counter_device *counter,
> +				  struct counter_count *count,
> +				  enum counter_count_direction *direction)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qepsts;
> +
> +	regmap_read(priv->regmap16, QEPSTS, &qepsts);
> +
> +	*direction = (qepsts & QEPSTS_QDF) ? COUNTER_COUNT_DIRECTION_FORWARD
> +					   : COUNTER_COUNT_DIRECTION_BACKWARD;
> +
> +	return 0;
> +}
> +
>  static struct counter_comp ti_eqep_position_ext[] = {
>  	COUNTER_COMP_CEILING(ti_eqep_position_ceiling_read,
>  			     ti_eqep_position_ceiling_write),
>  	COUNTER_COMP_ENABLE(ti_eqep_position_enable_read,
>  			    ti_eqep_position_enable_write),
> +	COUNTER_COMP_DIRECTION(ti_eqep_direction_read),
>  };
>  
>  static struct counter_signal ti_eqep_signals[] = {
> @@ -442,6 +471,10 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
>  	if (qflg & QFLG_PCU)
>  		counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0);
>  
> +	if (qflg & QFLG_QDC)
> +		counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0);
> +
> +
>  	regmap_set_bits(priv->regmap16, QCLR, ~0);

As mentioned in the previous patch comments, you should try if possible
to clear only the interrupt flags for the events that you're actually
handling here.

William Breathitt Gray

>  
>  	return IRQ_HANDLED;
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> index d0aa95aeff7b..36dd3b474d09 100644
> --- a/include/uapi/linux/counter.h
> +++ b/include/uapi/linux/counter.h
> @@ -61,6 +61,8 @@ enum counter_event_type {
>  	COUNTER_EVENT_THRESHOLD,
>  	/* Index signal detected */
>  	COUNTER_EVENT_INDEX,
> +	/* Direction change detected */
> +	COUNTER_EVENT_DIRECTION_CHANGE,
>  };
>  
>  /**
> -- 
> 2.25.1
>

Attachment: signature.asc
Description: PGP signature