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

[Julien Panis <jpanis@baylibre.com>]

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@baylibre.com>
> 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