On Tue, Dec 07, 2021 at 05:42:35PM -0800, Dipen Patel wrote:

On 12/1/21 4:53 PM, Kent Gibson wrote:

quoted

On Wed, Dec 01, 2021 at 10:01:46AM -0800, Dipen Patel wrote:

quoted

Hi,


On 12/1/21 9:16 AM, Kent Gibson wrote:

quoted

On Tue, Nov 30, 2021 at 07:29:20PM -0800, Dipen Patel wrote:

quoted

Hi,

On 11/25/21 5:31 PM, Kent Gibson wrote:

quoted

On Tue, Nov 23, 2021 at 11:30:36AM -0800, Dipen Patel wrote:

quoted

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 <redacted>
---
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.

quoted

 /**

@@ -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?

quoted

+	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.