Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 203.18.50.13 as permitted sender) receiver=protection.outlook.com;
 client-ip=203.18.50.13; helo=mail.nvidia.com;
Subject: 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>
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>
Message-ID: <9e53c30f-63e5-b2ca-a2ef-f85dab596b3c@nvidia.com>
Date:   Wed, 8 Dec 2021 12:14:36 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <395ba111-d620-f302-d0e7-8f20f39e6485@nvidia.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Language: en-US
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2021 20:12:31.4956
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 816c92af-7ba2-44b4-cdbc-08d9ba87111a
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[203.18.50.13];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: BN8NAM11FT065.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4063
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?

>
>>