Re: [RFC v3 09/12] gpiolib: cdev: Add hardware timestamp clock type

[Kent Gibson <warthog618@gmail.com>]

On Tue, Dec 07, 2021 at 05:42:35PM -0800, Dipen Patel wrote:
> 
> On 12/1/21 4:53 PM, Kent Gibson wrote:
> > On Wed, Dec 01, 2021 at 10:01:46AM -0800, Dipen Patel wrote:
> >> Hi,
> >>
> >>
> >> On 12/1/21 9:16 AM, Kent Gibson wrote:
> >>> On Tue, Nov 30, 2021 at 07:29:20PM -0800, Dipen Patel wrote:
> >>>> Hi,
> >>>>
> >>>> On 11/25/21 5:31 PM, Kent Gibson wrote:
> >>>>> On Tue, Nov 23, 2021 at 11:30:36AM -0800, Dipen Patel wrote:
> >>>>>> This patch adds new clock type for the GPIO controller which can
> >>>>>> timestamp gpio lines in realtime 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, the HTE subsystem pushes timestamp data
> >>>>>> through callbacks.
> >>>>>>
> >>>>>> Signed-off-by: Dipen Patel <dipenp@nvidia.com>
> >>>>>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> >>>>>> ---
> >>>>>> Changes in v2:
> >>>>>> - Added hte_dir and static structure hte_ts_desc.
> >>>>>> - Added callbacks which get invoked by HTE when new data is available.
> >>>>>> - Better use of hte_dir and seq from hte_ts_desc.
> >>>>>> - Modified sw debounce function to accommodate hardware timestamping.
> >>>>>>
> >>>>>>  drivers/gpio/gpiolib-cdev.c | 161 ++++++++++++++++++++++++++++++++++--
> >>>>>>  include/uapi/linux/gpio.h   |   1 +
> >>>>>>  2 files changed, 153 insertions(+), 9 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> >>>>>> index c7b5446d01fd..1736ad54e3ec 100644
> >>>>>> --- a/drivers/gpio/gpiolib-cdev.c
> >>>>>> +++ b/drivers/gpio/gpiolib-cdev.c
> >>>>>> @@ -464,6 +464,12 @@ struct line {
> >>>>>>  	 * stale value.
> >>>>>>  	 */
> >>>>>>  	unsigned int level;
> >>>>>> +	/*
> >>>>>> +	 * dir will be touched in HTE callbacks hte_ts_cb_t and
> >>>>>> +	 * hte_ts_threaded_cb_t and they are mutually exclusive. This will be
> >>>>>> +	 * unused when HTE is not supported/disabled.
> >>>>>> +	 */
> >>>>>> +	enum hte_dir dir;
> >>>>>>  };
> >>>>>>  
> >>>>> Documentation should be in present tense, so 
> >>>>>
> >>>>> s/will be/is/g
> >>>>>
> >>>>> Same applies to other patches.
> >>>>>
> >>>>> Also
> >>>>>
> >>>>> s/touched/accessed/
> >>>>>
> >>>>> dir is a poor name for the field.  It is the hte edge direction and
> >>>>> effectively the line level, so call it hte_edge_dirn or
> >>>>> hte_edge_direction or hte_level.
> >>>>>
> >>>>> And it is placed in a section of the struct documented as "debouncer specific
> >>>>> fields", but it is not specfic to the debouncer.  Add a "hte specific
> >>>>> fields" section if nothing else is suitable.
> >>>>>
> >>>>>>  /**
> >>>>>> @@ -518,6 +524,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,
> >>>>>> @@ -546,6 +553,94 @@ static u64 line_event_timestamp(struct line *line)
> >>>>>>  	return ktime_get_ns();
> >>>>>>  }
> >>>>>>  
> >>>>>> +static hte_return_t process_hw_ts_thread(void *p)
> >>>>>> +{
> >>>>>> +	struct line *line = p;
> >>>>>> +	struct linereq *lr = line->req;
> >>>>>> +	struct gpio_v2_line_event le;
> >>>>>> +	u64 eflags;
> >>>>>> +
> >>>>>> +	memset(&le, 0, sizeof(le));
> >>>>>> +
> >>>>>> +	le.timestamp_ns = line->timestamp_ns;
> >>>>>> +	line->timestamp_ns = 0;
> >>>>>> +
> >>>>> What is the purpose of this zeroing?
> >>>>>
> >>>>>> +	if (line->dir >= HTE_DIR_NOSUPP) {
> >>>>>> +		eflags = READ_ONCE(line->eflags);
> >>>>>> +		if (eflags == GPIO_V2_LINE_FLAG_EDGE_BOTH) {
> >>>>>> +			int level = gpiod_get_value_cansleep(line->desc);
> >>>>>> +
> >>>>>> +			if (level)
> >>>>>> +				/* Emit low-to-high event */
> >>>>>> +				le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
> >>>>>> +			else
> >>>>>> +				/* Emit high-to-low event */
> >>>>>> +				le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
> >>>>>> +		} else if (eflags == GPIO_V2_LINE_FLAG_EDGE_RISING) {
> >>>>>> +			/* Emit low-to-high event */
> >>>>>> +			le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
> >>>>>> +		} else if (eflags == GPIO_V2_LINE_FLAG_EDGE_FALLING) {
> >>>>>> +			/* Emit high-to-low event */
> >>>>>> +			le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
> >>>>>> +		} else {
> >>>>>> +			return HTE_CB_ERROR;
> >>>>>> +		}
> >>>>>> +	} else {
> >>>>>> +		if (line->dir == HTE_RISING_EDGE_TS)
> >>>>>> +			le.id = GPIO_V2_LINE_EVENT_RISING_EDGE;
> >>>>>> +		else
> >>>>>> +			le.id = GPIO_V2_LINE_EVENT_FALLING_EDGE;
> >>>>>> +	}
> >>>>> The mapping from line->dir to le.id needs to take into account the active
> >>>>> low setting for the line.
> >>>>>
> >>>>> And it might be simpler if the hte_ts_data provided the level, equivalent
> >>>>> to gpiod_get_raw_value_cansleep(), rather than an edge direction, so you
> >>>>> can provide a common helper to determine the edge given the raw level.
> >>>> (So from the level determine the edge?) that sound right specially when
> >>>>
> >>>> HTE provider has capability to record the edge in that case why bother
> >>>>
> >>>> getting the level and determine edge?
> >>>>
> >>>> Calculating the edge from the level makes sense when hte provider does not
> >>>>
> >>>> have that feature and that is what if (line->dir >= HTE_DIR_NOSUPP) does.
> >>>>
> >>> As asked in the review of patch 02, do you have an example of hardware that
> >>> reports an edge direction rather than NOSUPP?
> >> No...
> > So you are adding an interface that nothing will currently use.
> > Are there plans for hardware that will report the edge, and you are
> > laying the groundwork here?
> 
> Adding here for the general case should there be provider
> 
> available with such feature.
> 

Then you are adding dead code, and you should remove that aspect of your
interface until you have hardware that does support it.

Cheers,
Kent.