Messages in this thread | | | Date | Fri, 23 Sep 2022 09:23:26 +0200 | Subject | Re: [PATCH v8 3/4] counter: ti-ecap-capture: capture driver support for ECAP | From | 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@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, ®val); >> + 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
| |