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:   Thu, 22 Sep 2022 21:08:05 -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 v8 3/4] counter: ti-ecap-capture: capture driver support
 for ECAP
Message-ID: <Yy0G9a5S3OzwyEwW@fedora>
References: <20220922170402.403683-1-jpanis@baylibre.com>
 <20220922170402.403683-4-jpanis@baylibre.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="8oIR1Oj1QlTXqeTr"
Content-Disposition: inline
In-Reply-To: <20220922170402.403683-4-jpanis@baylibre.com>
Precedence: bulk


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

On Thu, Sep 22, 2022 at 07:04:01PM +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>

Hello Julien,

Comments follow inline below.

> +/**
> + * struct ecap_cnt_dev - device private data structure
> + * @enabled: device state
> + * @clk:     device clock
> + * @regmap:  device register map
> + * @nb_ovf:  number of overflows since capture start
> + * @pm_ctx:  device context for PM operations
> + */
> +struct ecap_cnt_dev {
> +	bool enabled;
> +	struct clk *clk;
> +	struct regmap *regmap;
> +	atomic_t nb_ovf;
> +	struct {
> +		u8 ev_mode;
> +		u32 time_cntr;
> +	} pm_ctx;
> +};

Provide documentation for the ev_mode and time_cntr members. You
probably need a lock as well to protect access to this structure or
you'll end up with race problems.


> +static void ecap_cnt_capture_enable(struct counter_device *counter)
> +{
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter);
> +
> +	pm_runtime_get_sync(counter->parent);
> +
> +	/* Enable interrupts on events */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG,
> +			   ECAP_EVT_EN_MASK, ECAP_EVT_EN_MASK);
> +
> +	/* Run counter */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_CFG_MAS=
K,
> +			   ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK);
> +}
> +
> +static void ecap_cnt_capture_disable(struct counter_device *counter)
> +{
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter);
> +
> +	/* Disable interrupts on events */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, ECAP_EVT_EN=
_MASK, 0);
> +
> +	/* Stop counter */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_EN_MASK=
, 0);

Shouldn't the counter be stopped before stopping the interrupts?

> +static int ecap_cnt_count_get_val(struct counter_device *counter, unsign=
ed int reg, u32 *val)
> +{
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter);
> +	unsigned int regval;
> +
> +	pm_runtime_get_sync(counter->parent);
> +	regmap_read(ecap_dev->regmap, reg, &regval);
> +	pm_runtime_put_sync(counter->parent);
> +
> +	*val =3D regval;
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_count_set_val(struct counter_device *counter, unsign=
ed int reg, u32 val)
> +{
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter);
> +
> +	pm_runtime_get_sync(counter->parent);
> +	regmap_write(ecap_dev->regmap, reg, val);
> +	pm_runtime_put_sync(counter->parent);
> +
> +	return 0;
> +}

The ecap_cnt_count_get_val() and ecap_cnt_count_set_val() functions only
ever return 0. Redefine them as void functions and eliminate the
unnecessary returns.

> +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;

You should return -EBUSY when the requested operation cannot be
completed because the device currently performing a task -- i.e. the
requested operation would stall or otherwise fail if forced. In this
case, the count value actually can be set while the device is enabled,
if I'm not mistaken; the count just continues increasing from the new
written value (i.e. no stall/failure). Therefore, there's not need to
return -EBUSY here and this check can be eliminated.

> +static int ecap_cnt_pol_write(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);
> +
> +	if (ecap_dev->enabled)
> +		return -EBUSY;

I suspect this check can go away for the same reason as above.

> +static int ecap_cnt_cap_write(struct counter_device *counter,
> +			      struct counter_count *count,
> +			      size_t idx, u64 cap)
> +{
> +	struct ecap_cnt_dev *ecap_dev =3D counter_priv(counter);
> +
> +	if (ecap_dev->enabled)
> +		return -EBUSY;

Same comment as above.

> +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;

Same comment as above.

> +static struct counter_count ecap_cnt_counts[] =3D {
> +	{
> +		.id =3D 0,

The id member is for differentiating between multiple Counts. You only
have one Count in this driver so you don't need to set it because you
never use it.

William Breathitt Gray

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

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

iHUEARYKAB0WIQSNN83d4NIlKPjon7a1SFbKvhIjKwUCYy0G9QAKCRC1SFbKvhIj
KxouAQCf2abb4CAphtgGkhQHIiAbAAbJAjCL1/3/eLHnBeBuwgEAl3EJhEc2Xmrp
jmOjd5jNtDVQnvB+NsoZnflvSxpU+gQ=
=XTZc
-----END PGP SIGNATURE-----

--8oIR1Oj1QlTXqeTr--