Re: [PATCH 7/8] counter/ti-eqep: add support for edge capture unit

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 16 Oct 2021 20:33:42 -0500
David Lechner <david@lechnology.com> wrote:

> This adds support for the Edge Capture Unit to the TI eQEP counter
> driver. This just adds the minimum required features to measure speed
> using the Unit Timer and the Edge Capture unit. Additional features can
> be added in the future if needed.
> 
> This adds 4 new device-level attributes:
> - edge_capture_unit_prescaler: selects a prescalar for the Counter count
> 	coming into the Edge Capture Unit
> - edge_capture_unit_max_period: selects the max time period that can be
> 	measured by the Edge Capture Unit
> - edge_capture_unit_latched_period: gets the period that was measured
> 	when the event selected by the latch_mode attribute is triggered
> - edge_capture_unit_enable: enables or disables the Edge Capture Unit
> 
> Signed-off-by: David Lechner <david@lechnology.com>

A general comment on using FIELD_GET and FIELD_PREP to avoid having to
define shifts twice.


> ---
>  drivers/counter/ti-eqep.c | 150 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index ef899655ad1d..fb1f4d0b4cde 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -116,6 +116,12 @@
>  #define QEPSTS_FIMF		BIT(1)
>  #define QEPSTS_PCEF		BIT(0)
>  
> +#define QCAPCTL_CEN		BIT(15)
> +#define QCAPCTL_CCPS_SHIFT	4

As mentioned below, using FIELD_GET() etc can avoid need to separately
define the shifts.

> +#define QCAPCTL_CCPS		GENMASK(6, 4)
> +#define QCAPCTL_UPPS_SHIFT	0
> +#define QCAPCTL_UPPS		GENMASK(3, 0)
> +
>  /* EQEP Inputs */
>  enum {
>  	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
> @@ -479,6 +485,137 @@ static struct counter_count ti_eqep_counts[] = {
>  	},
>  };
>  
> +static int ti_eqep_edge_capture_unit_enable_read(struct counter_device *counter,
> +						 u8 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcapctl;
> +
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	*value = !!(qcapctl & QCAPCTL_CEN);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_edge_capture_unit_enable_write(struct counter_device *counter,
> +						  u8 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	if (value)
> +		regmap_set_bits(priv->regmap16, QCAPCTL, QCAPCTL_CEN);
> +	else
> +		regmap_clear_bits(priv->regmap16, QCAPCTL, QCAPCTL_CEN);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_latched_period_read(struct counter_device *counter,
> +					      u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcprdlat, qcapctl;
> +	u8 ccps;
> +
> +	regmap_read(priv->regmap16, QCPRDLAT, &qcprdlat);
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	ccps = (qcapctl & QCAPCTL_CCPS) >> QCAPCTL_CCPS_SHIFT;

FIELD_PREP / FIELD_GET and friends allow this sort of action to be expressed using
just one define of the mask rather than a mask and shift.

> +
> +	/* convert timer ticks to nanoseconds */
> +	*value = mul_u64_u32_div(qcprdlat << ccps, NSEC_PER_SEC, priv->sysclkout_rate);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_max_period_read(struct counter_device *counter,
> +					  u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcapctl;
> +	u8 ccps;
> +
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	ccps = (qcapctl & QCAPCTL_CCPS) >> QCAPCTL_CCPS_SHIFT;
> +
> +	/* convert timer ticks to nanoseconds */
> +	*value = mul_u64_u32_div(USHRT_MAX << ccps, NSEC_PER_SEC,
> +				 priv->sysclkout_rate);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_max_period_write(struct counter_device *counter,
> +					   u64 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 period;
> +	u8 ccps;
> +
> +	/* convert nanoseconds to timer ticks */
> +	period = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> +	if (period != value)
> +		return -ERANGE;
> +
> +	/* find the smallest divider that will fit the requested period */
> +	for (ccps = 0; ccps <= 7; ccps++)
> +		if (USHRT_MAX << ccps >= period)
> +			break;
> +
> +	if (ccps > 7)
> +		return -EINVAL;
> +
> +	regmap_write_bits(priv->regmap16, QCAPCTL, QCAPCTL_CCPS,
> +			  ccps << QCAPCTL_CCPS_SHIFT);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_prescaler_read(struct counter_device *counter,
> +					 u32 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcapctl;
> +
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	*value = (qcapctl & QCAPCTL_UPPS) >> QCAPCTL_UPPS_SHIFT;
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_prescaler_write(struct counter_device *counter,
> +					  u32 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	regmap_write_bits(priv->regmap16, QCAPCTL, QCAPCTL_UPPS,
> +			  value << QCAPCTL_UPPS_SHIFT);
> +
> +	return 0;
> +}
> +
> +static const char *const ti_eqep_edge_capture_unit_prescaler_values[] = {
> +	"1",
> +	"2",
> +	"4",
> +	"8",
> +	"16",
> +	"32",
> +	"64",
> +	"128",
> +	"256",
> +	"512",
> +	"1024",
> +	"2048",
> +};
> +
> +static DEFINE_COUNTER_ENUM(ti_eqep_edge_capture_unit_prescaler_available,
> +			   ti_eqep_edge_capture_unit_prescaler_values);
> +
>  static int ti_eqep_latch_mode_read(struct counter_device *counter,
>  					    u32 *value)
>  {
> @@ -601,6 +738,19 @@ static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
>  }
>  
>  static struct counter_comp ti_eqep_device_ext[] = {
> +	COUNTER_COMP_DEVICE_BOOL("edge_capture_unit_enable",
> +				 ti_eqep_edge_capture_unit_enable_read,
> +				 ti_eqep_edge_capture_unit_enable_write),
> +	COUNTER_COMP_DEVICE_U64("edge_capture_unit_latched_period",
> +				ti_eqep_edge_capture_unit_latched_period_read,
> +				NULL),
> +	COUNTER_COMP_DEVICE_U64("edge_capture_unit_max_period",
> +				ti_eqep_edge_capture_unit_max_period_read,
> +				ti_eqep_edge_capture_unit_max_period_write),
> +	COUNTER_COMP_DEVICE_ENUM("edge_capture_unit_prescaler",
> +				 ti_eqep_edge_capture_unit_prescaler_read,
> +				 ti_eqep_edge_capture_unit_prescaler_write,
> +				 ti_eqep_edge_capture_unit_prescaler_available),
>  	COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read,
>  				ti_eqep_latch_mode_write, ti_eqep_latch_modes),
>  	COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,