Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Sep 2022 22:07:54 -0400
From:   William Breathitt Gray <william.gray@linaro.org>
To:     Julien Panis <jpanis@baylibre.com>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, mranostay@ti.com
Subject: Re: [PATCH v7 3/4] counter: ti-ecap-capture: capture driver support
 for ECAP
Message-ID: <YyvDeuHgWPmcrqPR@fedora>
References: <20220921100627.124085-1-jpanis@baylibre.com>
 <20220921100627.124085-4-jpanis@baylibre.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="CIfG5Izydz6MDoAL"
Content-Disposition: inline
In-Reply-To: <20220921100627.124085-4-jpanis@baylibre.com>
Precedence: bulk


--CIfG5Izydz6MDoAL
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Sep 21, 2022 at 12:06:26PM +0200, Julien Panis wrote:
> ECAP hardware on TI AM62x SoC supports capture feature. It can be used
> to timestamp events (falling/rising edges) detected on input signal.
>=20
> This commit adds capture driver support for ECAP hardware on AM62x SoC.
>=20
> In the ECAP hardware, capture pin can also be configured to be in
> PWM mode. Current implementation only supports capture operating mode.
> Hardware also supports timebase sync between multiple instances, but
> this driver supports simple independent capture functionality.
>=20
> Signed-off-by: Julien Panis <jpanis@baylibre.com>

Hi Julien,

This driver is almost there; some comments follow below.

> +static u8 ecap_cnt_capture_get_evmode(struct counter_device *counter)
> +{
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter);
> +	u8 ev_mode =3D 0;
> +	unsigned int regval;
> +	int i;
> +
> +	pm_runtime_get_sync(counter->parent);
> +	regmap_read(ecap_dev->regmap, ECAP_ECCTL_REG, &regval);
> +	pm_runtime_put_sync(counter->parent);
> +
> +	for (i =3D 0 ; i < ECAP_NB_CEVT ; i++) {
> +		if (regval & ECAP_CAPPOL_BIT(i))
> +			ev_mode |=3D ECAP_EV_MODE_BIT(i);
> +	}

Looks like this for loop is just remapping the set bits in regval to
ev_mode. You can use bitmap_remap() here instead to simplify this
section of code.

> +static void ecap_cnt_capture_set_evmode(struct counter_device *counter, =
u8 ev_mode)
> +{
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter);
> +	unsigned int regval =3D 0;
> +	int i;
> +
> +	for (i =3D 0 ; i < ECAP_NB_CEVT ; i++) {
> +		if (ev_mode & ECAP_EV_MODE_BIT(i))
> +			regval |=3D ECAP_CAPPOL_BIT(i);
> +	}

Use bitmap_remap() here as well (just in the reverse direction).

> +static int ecap_cnt_count_write(struct counter_device *counter,
> +				struct counter_count *count, u64 val)
> +{
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter);
> +
> +	if (ecap_dev->enabled)
> +		return -EBUSY;
> +	if (val > 0)
> +		return -EINVAL;

The ECAP_TSCNT_REG can be set to an arbitrary count value so there's no
need to restrict val here to 0. Instead, check that val is within the
ceiling value (<=3D U32_MAX) and return -ERANGE if it is not.

> +static int ecap_cnt_watch_validate(struct counter_device *counter,
> +				   const struct counter_watch *watch)
> +{
> +	if ((watch->channel <=3D ECAP_CEVT_LAST && watch->event =3D=3D COUNTER_=
EVENT_CAPTURE) ||
> +	    (watch->channel =3D=3D ECAP_CNTOVF && watch->event =3D=3D COUNTER_E=
VENT_OVERFLOW))
> +		return 0;
> +
> +	return -EINVAL;

COUNTER_EVENT_OVERFLOW shouldn't be on a separate channel; I'll explain
why later below in the interrupt handler review.

For this callback, you can separate the channel and event type checks to
their own blocks:

    if (watch->channel > ECAP_CEVT_LAST)
            return -EINVAL;
   =20
    switch (watch->event) {
    case COUNTER_EVENT_CAPTURE:
    case COUNTER_EVENT_OVERFLOW:
            return 0;
    default:
            return -EINVAL;
    }

> +static int ecap_cnt_pol_read(struct counter_device *counter,
> +			     struct counter_signal *signal,
> +			     size_t idx, enum counter_signal_polarity *pol)
> +{
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter);
> +
> +	pm_runtime_get_sync(counter->parent);
> +	*pol =3D regmap_test_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL=
_BIT(idx)) ?
> +	       COUNTER_SIGNAL_POLARITY_NEGATIVE :
> +	       COUNTER_SIGNAL_POLARITY_POSITIVE;

This single line is doing a lot of things so I would rather see it
broken down. Save the regmap_test_bits() to a temporary local variable
first before evaluating to set *pol. This allows you to move the *pol
set operation to outside of the pm runtime syncs, possibly giving you a
marginal improvement in latency as well.

> +static inline int ecap_cnt_cap_read(struct counter_device *counter,
> +				    struct counter_count *count,
> +				    size_t idx, u64 *cap)
> +{
> +	return ecap_cnt_count_get_val(counter, ECAP_CAP_REG(idx), cap);
> +}

I don't remember if we've discussed this before, but if these capture
registers can be set then it'll be useful to provide a corresponding
ecap_cnt_cap_write() function for them as well.

> +static int ecap_cnt_nb_ovf_write(struct counter_device *counter,
> +				 struct counter_count *count, u64 val)
> +{
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter);
> +
> +	if (ecap_dev->enabled)
> +		return -EBUSY;
> +	if (val > 0)
> +		return -EINVAL;

Similar to the count_write() callback, check that val is <=3D U32_MAX and
return -ERANGE otherwise.

> +static DEFINE_COUNTER_ARRAY_U64(ecap_cnt_cap_array, ECAP_NB_CEVT);
> +
> +static struct counter_comp ecap_cnt_count_ext[] =3D {
> +	COUNTER_COMP_COUNT_ARRAY_U64("capture", ecap_cnt_cap_read, NULL, ecap_c=
nt_cap_array),

Use the DEFINE_COUNTER_ARRAY_CAPTURE() and COUNTER_COMP_ARRAY_CAPTURE()
macros.

> +static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *counter_dev =3D dev_id;
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter_dev);
> +	unsigned int clr =3D 0;
> +	unsigned int flg;
> +	int i;
> +
> +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
> +
> +	/* Check capture events */
> +	for (i =3D 0 ; i < ECAP_NB_CEVT ; i++) {
> +		if (flg & ECAP_EVT_FLG_BIT(i)) {
> +			counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, i);
> +			clr |=3D ECAP_EVT_CLR_BIT(i);
> +		}
> +	}
> +
> +	/* Check counter overflow */
> +	if (flg & ECAP_EVT_FLG_BIT(ECAP_CNTOVF)) {
> +		atomic_inc(&ecap_dev->nb_ovf);
> +		counter_push_event(counter_dev, COUNTER_EVENT_OVERFLOW, ECAP_CNTOVF);

COUNTER_EVENT_OVERFLOW doesn't conflict with COUNTER_EVENT_CAPTURE
(they're different event types) so they can both be pushed on the same
channel; in general, events of different type can share the same event
channels. In this case, you should push COUNTER_EVENT_OVERFLOW to all
four channels whenever you detect an overflow, so that users can receive
those events regardless of which channels they are watching:

    counter_push_event(counter_dev, COUNTER_EVENT_OVERFLOW, 0);
    counter_push_event(counter_dev, COUNTER_EVENT_OVERFLOW, 1);
    counter_push_event(counter_dev, COUNTER_EVENT_OVERFLOW, 2);
    counter_push_event(counter_dev, COUNTER_EVENT_OVERFLOW, 3);

There's no additional cost to pushing to these channels because events
get dropped by the Counter chrdev code if the user is not watching the
particular event on a channel.

> +static int ecap_cnt_suspend(struct device *dev)
> +{
> +	struct counter_device *counter_dev =3D dev_get_drvdata(dev);
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter_dev);
> +
> +	/* If eCAP is running, stop capture then save timestamp counter */
> +	if (ecap_dev->enabled) {
> +		/*
> +		 * Disabling capture has the following effects:
> +		 * - interrupts are disabled
> +		 * - loading of capture registers is disabled
> +		 * - timebase counter is stopped
> +		 */
> +		ecap_cnt_capture_disable(counter_dev);
> +		ecap_cnt_count_get_val(counter_dev, ECAP_TSCNT_REG, &ecap_dev->pm_ctx.=
time_cntr);

I see time_cntr define as u64, but if count value is always <=3D U32_MAX
you could redefine both ecap_cnt_count_get_val()'s val and time_cntr to
u32 instead.

> +static int ecap_cnt_resume(struct device *dev)
> +{
> +	struct counter_device *counter_dev =3D dev_get_drvdata(dev);
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter_dev);
> +
> +	clk_enable(ecap_dev->clk);
> +
> +	ecap_cnt_capture_set_evmode(counter_dev, ecap_dev->pm_ctx.ev_mode);
> +
> +	/* If eCAP was running, restore timestamp counter then run capture */
> +	if (ecap_dev->enabled) {
> +		ecap_cnt_count_set_val(counter_dev, ECAP_TSCNT_REG, ecap_dev->pm_ctx.t=
ime_cntr);

Same as above would apply for ecap_cnt_count_set_val() too I think.

William Breathitt Gray

--CIfG5Izydz6MDoAL
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQSNN83d4NIlKPjon7a1SFbKvhIjKwUCYyvDegAKCRC1SFbKvhIj
K9G+AP4xMrC13fuL8/1x33pIHZ2bXzKzLdoRWNe6HFvtfQEt6gD8CIWTic8ORaRY
GpuhxVimnt2gc/D8fuln+Ozm4I4Mcw8=
=pXZN
-----END PGP SIGNATURE-----

--CIfG5Izydz6MDoAL--