On Tue, Dec 07, 2021 at 05:42:35PM -0800, 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:
> >>>> Hi,
> >>>>
> >>>> On 11/25/21 5:31 PM, Kent Gibson wrote:
> >>>>> On Tue, Nov 23, 2021 at 11:30:36AM -0800, Dipen Patel wrote:
> >>>>>> This patch adds new clock type for the GPIO controller which can
> >>>>>> timestamp gpio lines in realtime using hardware means. To expose such
> >>>>>> functionalities to the userspace, code has been added in this patch
> >>>>>> where during line create call, it checks for new clock type and if
> >>>>>> requested, calls hardware timestamp related API from gpiolib.c.
> >>>>>> During line change event, the HTE subsystem pushes timestamp data
> >>>>>> through callbacks.
> >>>>>>
> >>>>>> Signed-off-by: Dipen Patel <dipenp@xxxxxxxxxx>
> >>>>>> Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> >>>>>> ---
> >>>>>> Changes in v2:
> >>>>>> - Added hte_dir and static structure hte_ts_desc.
> >>>>>> - Added callbacks which get invoked by HTE when new data is available.
> >>>>>> - Better use of hte_dir and seq from hte_ts_desc.
> >>>>>> - Modified sw debounce function to accommodate hardware timestamping.
> >>>>>>
> >>>>>> drivers/gpio/gpiolib-cdev.c | 161 ++++++++++++++++++++++++++++++++++--
> >>>>>> include/uapi/linux/gpio.h | 1 +
> >>>>>> 2 files changed, 153 insertions(+), 9 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> >>>>>> index c7b5446d01fd..1736ad54e3ec 100644
> >>>>>> --- a/drivers/gpio/gpiolib-cdev.c
> >>>>>> +++ b/drivers/gpio/gpiolib-cdev.c
> >>>>>> @@ -464,6 +464,12 @@ struct line {
> >>>>>> * stale value.
> >>>>>> */
> >>>>>> unsigned int level;
> >>>>>> + /*
> >>>>>> + * dir will be touched in HTE callbacks hte_ts_cb_t and
> >>>>>> + * hte_ts_threaded_cb_t and they are mutually exclusive. This will be
> >>>>>> + * unused when HTE is not supported/disabled.
> >>>>>> + */
> >>>>>> + enum hte_dir dir;
> >>>>>> };
> >>>>>>
> >>>>> Documentation should be in present tense, so
> >>>>>
> >>>>> s/will be/is/g
> >>>>>
> >>>>> Same applies to other patches.
> >>>>>
> >>>>> Also
> >>>>>
> >>>>> s/touched/accessed/
> >>>>>
> >>>>> dir is a poor name for the field. It is the hte edge direction and
> >>>>> effectively the line level, so call it hte_edge_dirn or
> >>>>> hte_edge_direction or hte_level.
> >>>>>
> >>>>> And it is placed in a section of the struct documented as "debouncer specific
> >>>>> fields", but it is not specfic to the debouncer. Add a "hte specific
> >>>>> fields" section if nothing else is suitable.
> >>>>>
> >>>>>> /**
> >>>>>> @@ -518,6 +524,7 @@ struct linereq {
> >>>>>> GPIO_V2_LINE_DRIVE_FLAGS | \
> >>>>>> GPIO_V2_LINE_EDGE_FLAGS | \
> >>>>>> GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME | \
> >>>>>> + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \
> >>>>>> GPIO_V2_LINE_BIAS_FLAGS)
> >>>>>>
> >>>>>> static void linereq_put_event(struct linereq *lr,
> >>>>>> @@ -546,6 +553,94 @@ static u64 line_event_timestamp(struct line *line)
> >>>>>> return ktime_get_ns();
> >>>>>> }
> >>>>>>
> >>>>>> +static hte_return_t process_hw_ts_thread(void *p)
> >>>>>> +{
> >>>>>> + struct line *line = p;
> >>>>>> + struct linereq *lr = line->req;
> >>>>>> + struct gpio_v2_line_event le;
> >>>>>> + u64 eflags;
> >>>>>> +
> >>>>>> + memset(&le, 0, sizeof(le));
> >>>>>> +
> >>>>>> + le.timestamp_ns = line->timestamp_ns;
> >>>>>> + line->timestamp_ns = 0;
> >>>>>> +
> >>>>> What is the purpose of this zeroing?
> >>>>>
> >>>>>> + 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.
>
Then you are adding dead code, and you should remove that aspect of your
interface until you have hardware that does support it.
Cheers,
Kent.
