Re: [RFC 08/11] gpiolib: cdev: Add hardware timestamp clock type

[Kent Gibson]

On Tue, Aug 03, 2021 at 03:51:31PM -0700, Dipen Patel wrote:
> 
> On 8/3/21 9:42 AM, Jon Hunter wrote:
> >
> > On 30/07/2021 03:33, Dipen Patel wrote:
> >>
> >> On 7/9/21 1:30 AM, Jon Hunter wrote:
> >>> On 26/06/2021 00:55, Dipen Patel wrote:
> >>>> This patch adds new clock type for the GPIO controller which can
> >>>> timestamp gpio lines using hardware means. To expose such
> >>>> functionalities to the userspace, code has been added in this patch
> >>>> where during line create call, it checks for new clock type and if
> >>>> requested, calls hardware timestamp related API from gpiolib.c.
> >>>> During line change event, it retrieves timestamp in nano seconds by
> >>>> calling gpiod_get_hw_timestamp API from gpiolib.c. At the line release,
> >>>> it disables this functionality by calling gpiod_hw_timestamp_control.
> >>>>
> >>>> Signed-off-by: Dipen Patel <dipenp@xxxxxxxxxx>
> >>>> ---
> >>>>   drivers/gpio/gpiolib-cdev.c | 65 +++++++++++++++++++++++++++++++++++--
> >>>>   include/uapi/linux/gpio.h   |  1 +
> >>>>   2 files changed, 64 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> >>>> index 1631727bf0da..9f98c727e937 100644
> >>>> --- a/drivers/gpio/gpiolib-cdev.c
> >>>> +++ b/drivers/gpio/gpiolib-cdev.c
> >>>> @@ -518,6 +518,7 @@ struct linereq {
> >>>>        GPIO_V2_LINE_DRIVE_FLAGS | \
> >>>>        GPIO_V2_LINE_EDGE_FLAGS | \
> >>>>        GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME | \
> >>>> +     GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \
> >>>>        GPIO_V2_LINE_BIAS_FLAGS)
> >>>>     static void linereq_put_event(struct linereq *lr,
> >>>> @@ -540,9 +541,20 @@ static void linereq_put_event(struct linereq *lr,
> >>>>     static u64 line_event_timestamp(struct line *line)
> >>>>   {
> >>>> +    bool block;
> >>>> +
> >>>>       if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags))
> >>>>           return ktime_get_real_ns();
> >>>>   +    if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &line->desc->flags)) {
> >>>> +        if (irq_count())
> >>>> +            block = false;
> >>>> +        else
> >>>> +            block = true;
> >>>> +
> >>>> +        return gpiod_get_hw_timestamp(line->desc, block);
> >>>> +    }
> >>>> +
> >>>>       return ktime_get_ns();
> >>>>   }
> >>>
> >>> Looking at line_event_timestamp() and the callers of this function, it
> >>> appears that this should always return nanoseconds. Does
> >>> gpiod_get_hw_timestamp() return nanoseconds?
> >> Yes, it returns in ns to align with line_event_timestamp.
> >
> >
> > It might be worth updating the function name to gpiod_get_hw_timestamp_ns() so that this is clear.
> Wouldn't be sufficient to right into its description rather embed in API name?
> >

Adding a suffix identifying the timestamp resolution to variable and
function names is pretty standard in the kernel.
It makes the code easier to read as you don't have to keep checking the
documentation.

Cheers,
Kent.