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