On 23/09/2022 14:17, Julien Panis wrote:
On 23/09/2022 13:35, William Breathitt Gray wrote:
On Fri, Sep 23, 2022 at 09:23:26AM +0200, Julien Panis wrote:
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
I was thinking of the ecap_cnt_dev enabled member. The Counter callbacks
may execute in concurrent threads, so races can appear when you access
members of the ecap_cnt_dev structure in these callbacks.
Take for example this section of ecap_cnt_enable_write():
if (enable == ecap_dev->enabled)
return 0;
if (enable)
ecap_cnt_capture_enable(counter);
else
ecap_cnt_capture_disable(counter);
ecap_dev->enabled = enable
Suppose two threads try to enable the count capture. A race condition is
present where the two threads could see ecap_dev->enabled as false and
both proceed to call ecap_cnt_capture_enable(). This results in
pm_runtime_get_sync() bumping the usage count twice and we're left with
a mismatch the next time ecap_cnt_capture_disable() is called.
William Breathitt Gray
OK, If I understand well there's the same problem with IO access with
regmap ?
Julien
[ERRATUM] It seems that some spinlock is already used by regmap API.
So, only the 'enabled' member needs a lock.
