On Wed, Dec 08, 2021 at 12:14:36PM -0800, Dipen Patel wrote:

Hi,

On 12/7/21 5:42 PM, Dipen Patel wrote:

quoted

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?

Yes it does.

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

That would not make it any more valid.  There is an inherent race there
- that is the nature of the irq interface.
The existing code does the best it can in the circumstances - for the
more likely case that there isn't another edge between the interrupt
handler and thread.

The hte can do better - assumung it has hardware capable of providing the
edge as well as the timestamp.

Cheers,
Kent.