Date:   Fri, 26 Nov 2021 09:31:09 +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 06/12] gpiolib: Add HTE support
Message-ID: <20211126013109.GB10380@sol>
References: <20211123193039.25154-1-dipenp@nvidia.com>
 <20211123193039.25154-7-dipenp@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211123193039.25154-7-dipenp@nvidia.com>
Precedence: bulk

On Tue, Nov 23, 2021 at 11:30:33AM -0800, Dipen Patel wrote:
> Some GPIO chip can provide hardware timestamp support on its GPIO lines
> , in order to support that additional API needs to be added which
> can talk to both GPIO chip and HTE (hardware timestamping engine)
> subsystem. This patch introduces APIs which gpio consumer can use
> to request hardware assisted timestamping. Below is the list of the APIs
> that are added in gpiolib subsystem.
> 
> - gpiod_req_hw_timestamp_ns - Request HTE on specified GPIO line.
> - gpiod_rel_hw_timestamp_ns - Release HTE functionality on GPIO line.
> 
> Signed-off-by: Dipen Patel <dipenp@nvidia.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
> Changes in v2:
> - removed get timestamp and is timestamp enabled APIs
> 
>  drivers/gpio/gpiolib.c        | 73 +++++++++++++++++++++++++++++++++++
>  drivers/gpio/gpiolib.h        | 12 ++++++
>  include/linux/gpio/consumer.h | 19 ++++++++-
>  include/linux/gpio/driver.h   | 14 +++++++
>  4 files changed, 116 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index abfbf546d159..46cba75c80e8 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1976,6 +1976,10 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
>  			gc->free(gc, gpio_chip_hwgpio(desc));
>  			spin_lock_irqsave(&gpio_lock, flags);
>  		}
> +		spin_unlock_irqrestore(&gpio_lock, flags);
> +		gpiod_rel_hw_timestamp_ns(desc);
> +		spin_lock_irqsave(&gpio_lock, flags);
> +
>  		kfree_const(desc->label);
>  		desc_set_label(desc, NULL);
>  		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
> @@ -2388,6 +2392,75 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
>  }
>  EXPORT_SYMBOL_GPL(gpiod_direction_output);
>  
> +/**
> + * gpiod_req_hw_timestamp_ns - Enable the hardware assisted timestamp in
> + * nano second.
> + *

s/nano second/nanoseconds/g

> + * @desc: GPIO to enable
> + * @cb:	Callback, will be called when HTE pushes timestamp data.
> + * @tcb: Threaeded callback, it gets called from kernel thread context and when

s/Threaeded/Threaded/

> + * cb returns with HTE_RUN_THREADED_CB return value.
> + * @data: Client data, will be sent back with tcb and cb.
> + *
> + * Certain GPIO chip can rely on hardware assisted timestamp engines which can

Either drop the 'assisted' and refer to them as "hardware timestamping
engines" throughout, or rename your subsystem 'hate'?
Either way, be consistent.

> + * record timestamp at the occurance of the configured events
> + * i.e. rising/falling on specified GPIO lines. This is helper API to enable hw
> + * assisted timestamp in nano second.
> + *

Not sure this comment block adds anything.

> + * Return 0 in case of success, else an error code.
> + */
> +int gpiod_req_hw_timestamp_ns(struct gpio_desc *desc, hte_ts_cb_t cb,
> +			      hte_ts_threaded_cb_t tcb, void *data)
> +{
> +	struct gpio_chip *gc;
> +	int ret = 0;
> +
> +	VALIDATE_DESC(desc);
> +	gc = desc->gdev->chip;
> +
> +	if (!gc->req_hw_timestamp) {
> +		gpiod_warn(desc, "%s: hw ts not supported\n", __func__);
> +		return -ENOTSUPP;
> +	}
> +
> +	ret = gc->req_hw_timestamp(gc, gpio_chip_hwgpio(desc), cb, tcb,
> +				   &desc->hdesc, data);
> +	if (ret)
> +		gpiod_warn(desc, "%s: hw ts request failed\n", __func__);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gpiod_req_hw_timestamp_ns);
> +
> +/**
> + * gpiod_rel_hw_timestamp_ns - Release and disable the hardware assisted
> + * timestamp.
> + *

Are gpiod_req_hw_timestamp_ns() and gpiod_rel_hw_timestamp_ns()
request/release or enable/disable?
You are using both descriptions in the documentation.
request/release implies resource allocation, while enable/disable does
not.  Which is it?

> + * @desc: GPIO to disable
> + *
> + * Return 0 in case of success, else an error code.
> + */
> +int gpiod_rel_hw_timestamp_ns(struct gpio_desc *desc)

Cheers,
Kent.