Re: [PATCH v8 3/4] counter: ti-ecap-capture: capture driver support for ECAP

[Julien Panis]

On 23/09/2022 03:08, William Breathitt Gray wrote:
> On Thu, Sep 22, 2022 at 07:04:01PM +0200, Julien Panis wrote:
> > ECAP hardware on TI AM62x SoC supports capture feature. It can be used
> > to timestamp events (falling/rising edges) detected on input signal.
> >
> > This commit adds capture driver support for ECAP hardware on AM62x SoC.
> >
> > In the ECAP hardware, capture pin can also be configured to be in
> > PWM mode. Current implementation only supports capture operating mode.
> > Hardware also supports timebase sync between multiple instances, but
> > this driver supports simple independent capture functionality.
> >
> > Signed-off-by: Julien Panis <jpanis@xxxxxxxxxxxx>
> Hello Julien,
>
> Comments follow inline below.
>
> > +/**
> > + * struct ecap_cnt_dev - device private data structure
> > + * @enabled: device state
> > + * @clk:     device clock
> > + * @regmap:  device register map
> > + * @nb_ovf:  number of overflows since capture start
> > + * @pm_ctx:  device context for PM operations
> > + */
> > +struct ecap_cnt_dev {
> > +	bool enabled;
> > +	struct clk *clk;
> > +	struct regmap *regmap;
> > +	atomic_t nb_ovf;
> > +	struct {
> > +		u8 ev_mode;
> > +		u32 time_cntr;
> > +	} pm_ctx;
> > +};
> Provide documentation for the ev_mode and time_cntr members. You
> probably need a lock as well to protect access to this structure or
> you'll end up with race problems.

Hi William,

How can I end up with race problems ? pm_ctx members are only accessed at
suspend (after capture/IRQ are disabled) and resume (before capture/IRQ 
are re-enabled).
Is there any risk I did not identify ?

Julien

> > +static void ecap_cnt_capture_enable(struct counter_device *counter)
> > +{
> > +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> > +
> > +	pm_runtime_get_sync(counter->parent);
> > +
> > +	/* Enable interrupts on events */
> > +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG,
> > +			   ECAP_EVT_EN_MASK, ECAP_EVT_EN_MASK);
> > +
> > +	/* Run counter */
> > +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_CFG_MASK,
> > +			   ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK);
> > +}
> > +
> > +static void ecap_cnt_capture_disable(struct counter_device *counter)
> > +{
> > +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> > +
> > +	/* Disable interrupts on events */
> > +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, ECAP_EVT_EN_MASK, 0);
> > +
> > +	/* Stop counter */
> > +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_EN_MASK, 0);
> Shouldn't the counter be stopped before stopping the interrupts?
>
> > +static int ecap_cnt_count_get_val(struct counter_device *counter, unsigned int reg, u32 *val)
> > +{
> > +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> > +	unsigned int regval;
> > +
> > +	pm_runtime_get_sync(counter->parent);
> > +	regmap_read(ecap_dev->regmap, reg, &regval);
> > +	pm_runtime_put_sync(counter->parent);
> > +
> > +	*val = regval;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ecap_cnt_count_set_val(struct counter_device *counter, unsigned int reg, u32 val)
> > +{
> > +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> > +
> > +	pm_runtime_get_sync(counter->parent);
> > +	regmap_write(ecap_dev->regmap, reg, val);
> > +	pm_runtime_put_sync(counter->parent);
> > +
> > +	return 0;
> > +}
> The ecap_cnt_count_get_val() and ecap_cnt_count_set_val() functions only
> ever return 0. Redefine them as void functions and eliminate the
> unnecessary returns.
>
> > +static int ecap_cnt_count_write(struct counter_device *counter,
> > +				struct counter_count *count, u64 val)
> > +{
> > +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> > +
> > +	if (ecap_dev->enabled)
> > +		return -EBUSY;
> You should return -EBUSY when the requested operation cannot be
> completed because the device currently performing a task -- i.e. the
> requested operation would stall or otherwise fail if forced. In this
> case, the count value actually can be set while the device is enabled,
> if I'm not mistaken; the count just continues increasing from the new
> written value (i.e. no stall/failure). Therefore, there's not need to
> return -EBUSY here and this check can be eliminated.
>
> > +static int ecap_cnt_pol_write(struct counter_device *counter,
> > +			      struct counter_signal *signal,
> > +			      size_t idx, enum counter_signal_polarity pol)
> > +{
> > +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> > +
> > +	if (ecap_dev->enabled)
> > +		return -EBUSY;
> I suspect this check can go away for the same reason as above.
>
> > +static int ecap_cnt_cap_write(struct counter_device *counter,
> > +			      struct counter_count *count,
> > +			      size_t idx, u64 cap)
> > +{
> > +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> > +
> > +	if (ecap_dev->enabled)
> > +		return -EBUSY;
> Same comment as above.
>
> > +static int ecap_cnt_nb_ovf_write(struct counter_device *counter,
> > +				 struct counter_count *count, u64 val)
> > +{
> > +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> > +
> > +	if (ecap_dev->enabled)
> > +		return -EBUSY;
> Same comment as above.
>
> > +static struct counter_count ecap_cnt_counts[] = {
> > +	{
> > +		.id = 0,
> The id member is for differentiating between multiple Counts. You only
> have one Count in this driver so you don't need to set it because you
> never use it.
>
> William Breathitt Gray