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

Hi,

On 12/7/21 5:42 PM, 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

quoted

[snip]

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.

I have a doubt as below on how edge_irq_thread calculates le.id (Only for

gpiod_get_value_cansleep case), i believe clearing that doubt will help me properly

address this issue:

- Does it have potential to read level which might have changed by the time thread is run?

- Does it make sense to read it in edge_irq_handler instead at least of the chip which can

fetch the level without needing to sleep?

quoted