Date:   Thu, 9 Dec 2021 06:24:17 +0800
From:   Kent Gibson <warthog618@gmail.com>
To:     Dipen Patel <dipenp@nvidia.com>
Cc:     thierry.reding@gmail.com, jonathanh@nvidia.com,
        linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org,
        linux-gpio@vger.kernel.org, linus.walleij@linaro.org,
        brgl@bgdev.pl, devicetree@vger.kernel.org,
        linux-doc@vger.kernel.org, robh+dt@kernel.org
Subject: Re: [RFC v3 09/12] gpiolib: cdev: Add hardware timestamp clock type
Message-ID: <20211208222417.GA4654@sol>
References: <20211123193039.25154-1-dipenp@nvidia.com>
 <20211123193039.25154-10-dipenp@nvidia.com>
 <20211126013137.GC10380@sol>
 <9ad666ec-eedd-8075-73e6-1e47a1eb228b@nvidia.com>
 <20211201171638.GA31045@sol>
 <4c7c3db1-a1b3-1944-4278-cb37e8a4f373@nvidia.com>
 <20211202005349.GA7007@sol>
 <395ba111-d620-f302-d0e7-8f20f39e6485@nvidia.com>
 <9e53c30f-63e5-b2ca-a2ef-f85dab596b3c@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9e53c30f-63e5-b2ca-a2ef-f85dab596b3c@nvidia.com>
Precedence: bulk

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.