Re: [PATCH 5/8] counter/ti-eqep: add support for latched position

[William Breathitt Gray]

On Sat, Oct 16, 2021 at 08:33:40PM -0500, David Lechner wrote:
> This adds support to the TI eQEP counter driver for a latched position.
> This is a new extension that gets the counter count that was recorded
> when an event was triggered. A new device-level latch_mode attribute is
> added to select the trigger. Edge capture unit support will be needed
> to make full use of this, but "Unit timeout" mode can already be used
> to calculate high speeds.
> 
> The unit timer could also have attributes for latched_time and
> latched_period that use the same trigger. However this is not a use
> case at this time, so they can be added later if needed.

I see that "latched_count" holds the captured counter count; would this
"latched_time" hold the captured unit timer time? If so, does that mean
setting the latch mode to "Unit timeout" always results in a
"latched_time" equal to 0 (assuming that's when the timeout event
triggers)?

> 
> Signed-off-by: David Lechner <david@xxxxxxxxxxxxxx>
> ---
>  drivers/counter/ti-eqep.c | 50 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index 1ba7f3c7cb7e..ef899655ad1d 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -405,12 +405,28 @@ static int ti_eqep_direction_read(struct counter_device *counter,
>  	return 0;
>  }
>  
> +static int ti_eqep_position_latched_count_read(struct counter_device *counter,
> +					       struct counter_count *count,
> +					       u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qposlat;
> +
> +	regmap_read(priv->regmap32, QPOSLAT, &qposlat);
> +
> +	*value = qposlat;
> +
> +	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),
> +	COUNTER_COMP_COUNT_U64("latched_count",
> +			       ti_eqep_position_latched_count_read, NULL),
>  };
>  
>  static struct counter_signal ti_eqep_signals[] = {
> @@ -463,6 +479,38 @@ static struct counter_count ti_eqep_counts[] = {
>  	},
>  };
>  
> +static int ti_eqep_latch_mode_read(struct counter_device *counter,
> +					    u32 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qepctl;
> +
> +	regmap_read(priv->regmap16, QEPCTL, &qepctl);
> +	*value = !!(qepctl & QEPCTL_QCLM);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_latch_mode_write(struct counter_device *counter,
> +					     u32 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	if (value)
> +		regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_QCLM);
> +	else
> +		regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_QCLM);
> +
> +	return 0;
> +}
> +
> +static const char *const ti_eqep_latch_mode_names[] = {
> +	"Read count",
> +	"Unit timeout",
> +};
> +
> +static DEFINE_COUNTER_ENUM(ti_eqep_latch_modes, ti_eqep_latch_mode_names);
> +
>  static int ti_eqep_unit_timer_time_read(struct counter_device *counter,
>  				       u64 *value)
>  {
> @@ -553,6 +601,8 @@ static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
>  }
>  
>  static struct counter_comp ti_eqep_device_ext[] = {
> +	COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read,
> +				ti_eqep_latch_mode_write, ti_eqep_latch_modes),

It seems more appropriate to move this alongside "latched_count" as
Count extension because this is setting the trigger mode to latch the
respective Count's count. Or does this particular extension also affect
the "latched_time" capture for the unit timer?

William Breathitt Gray

>  	COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,
>  				ti_eqep_unit_timer_time_write),
>  	COUNTER_COMP_DEVICE_U64("unit_timer_period",
> -- 
> 2.25.1
>

Attachment: signature.asc
Description: PGP signature