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.
