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

From: Dipen Patel <dipenp@nvidia.com>
To: Kent Gibson <warthog618@gmail.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
Date: Wed, 8 Dec 2021 12:14:36 -0800	[thread overview]
Message-ID: <9e53c30f-63e5-b2ca-a2ef-f85dab596b3c@nvidia.com> (raw)
In-Reply-To: <395ba111-d620-f302-d0e7-8f20f39e6485@nvidia.com>

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?

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

>
>>