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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Wed, Dec 08, 2021 at 12:14:36PM -0800, Dipen Patel wrote:
> Hi,
> 
> On 12/7/21 5:42 PM, 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:
> >>>>>
> >>>>>> [snip]
> >>>>>>> +	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.