Hi,
Linus Walleij <linus.walleij@xxxxxxxxxx> writes:
> On Wed, Nov 29, 2017 at 2:56 PM, Felipe Balbi
> <felipe.balbi@xxxxxxxxxxxxxxx> wrote:
>> Me:
>>> For the other thing: timestamping of GPIO events, we already
>>> support timestamps for userspace GPIOs, but all it does is use
>>> the kernel time, see gpiolib.c:
>>>
>>> static irqreturn_t lineevent_irq_thread(int irq, void *p)
>>> {
>>> struct lineevent_state *le = p;
>>> struct gpioevent_data ge;
>>> int ret, level;
>>>
>>> ge.timestamp = ktime_get_real_ns();
>>> level = gpiod_get_value_cansleep(le->desc);
>>
>> this is running as a thread with interrupts enabled, AFAICT. This means
>> this thread can be preempted at least on PREEMPT_RT kernels, so your
>> timestamp can be wrong, right?
>
> Yes, it can be off. What we should do to get i better is
> something like what I did in:
> drivers/iio/gyro/mpu3050-core.c
>
> Here I have both a hard and a soft IRQ handler (fast/slow if
> you like) and take the timestamp in the hard IRQ, then use
> it in the thread.
>
> This should be done identically in gpiolib to increase precision
> in the general case.
>
> I was thinking about it already when implementing it but it fell
> out of my mind. I'm putting in on my TODO. (CC to bartosz
> who might be interested, he's using these ABIs quite a bit.)
fair enough. In that case, it'll probably be easier to implement
HW-based timestamping with something like below:
modified drivers/gpio/gpiolib.c
@@ -731,7 +731,11 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
struct gpioevent_data ge;
int ret, level;
- ge.timestamp = ktime_get_real_ns();
+ if (le->desc->flags & FLAG_HAS_HW_TIMESTAMP)
+ ge.timestamp = gpio_get_hw_timestamp(le->desc);
+ else
+ ge.timestamp = ktime_get_real_ns();
+
level = gpiod_get_value_cansleep(le->desc);
if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
modified drivers/gpio/gpiolib.h
@@ -206,6 +206,7 @@ struct gpio_desc {
#define FLAG_USED_AS_IRQ 9 /* GPIO is connected to an IRQ */
#define FLAG_IS_HOGGED 11 /* GPIO is hogged */
#define FLAG_SLEEP_MAY_LOSE_VALUE 12 /* GPIO may lose value in sleep */
+#define FLAG_HAS_HW_TIMESTAMP 13 /* GPIO has HW-based timestamping */
/* Connection label */
const char *label;
We may even extract ktime_get_real_ns() to gpio_get_timestamp() and do
the branching in that function, like:
modified drivers/gpio/gpiolib.c
@@ -731,7 +731,7 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
struct gpioevent_data ge;
int ret, level;
- ge.timestamp = ktime_get_real_ns();
+ ge.timestamp = gpiod_get_timestamp(le->desc);
level = gpiod_get_value_cansleep(le->desc);
if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
@@ -3155,6 +3155,14 @@ int gpiod_get_value_cansleep(const struct gpio_desc *desc)
}
EXPORT_SYMBOL_GPL(gpiod_get_value_cansleep);
+u64 gpiod_get_timestamp(const struct gpio_desc *desc)
+{
+ if (desc->flags & FLAG_HAS_HW_TIMESTAMP)
+ return gpiod_get_raw_timestamp(desc);
+ else
+ return ktime_get_real_ns();
+}
+
/**
* gpiod_get_raw_array_value_cansleep() - read raw values from an array of GPIOs
* @array_size: number of elements in the descriptor / value arrays
modified drivers/gpio/gpiolib.h
@@ -206,6 +206,7 @@ struct gpio_desc {
#define FLAG_USED_AS_IRQ 9 /* GPIO is connected to an IRQ */
#define FLAG_IS_HOGGED 11 /* GPIO is hogged */
#define FLAG_SLEEP_MAY_LOSE_VALUE 12 /* GPIO may lose value in sleep */
+#define FLAG_HAS_HW_TIMESTAMP 13 /* GPIO has HW-based timestamping */
/* Connection label */
const char *label;
--
balbi
Attachment:
signature.asc
Description: PGP signature
