Received-SPF: pass (google.com: domain of linux-kernel+bounces-20141-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 8 Jan 2024 22:07:09 +0000
From: William Breathitt Gray <william.gray@linaro.org>
To: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Cc: lee@kernel.org, alexandre.torgue@foss.st.com, linux-iio@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 10/10] counter: stm32-timer-cnt: add support for
 capture events
Message-ID: <ZZxyDbYC9oHNKcGF@ubuntu-server-vm-macos>
References: <20231220145726.640627-1-fabrice.gasnier@foss.st.com>
 <20231220145726.640627-11-fabrice.gasnier@foss.st.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="WiW47Lt2gdjHTFQC"
Content-Disposition: inline
In-Reply-To: <20231220145726.640627-11-fabrice.gasnier@foss.st.com>


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

On Wed, Dec 20, 2023 at 03:57:26PM +0100, Fabrice Gasnier wrote:
> +	/*
> +	 * configure channel in input capture mode, map channel 1 on TI1, chann=
el2 on TI2...
> +	 * Select both edges / non-inverted to trigger a capture.
> +	 */

I suggest defining a new local variable 'cc' to point to stm32_cc[ch]. I
think that's make the code look nicer here to avoid all the array index
syntax every time you access stm32_cc[ch].

> +	if (enable) {
> +		/* first clear possibly latched capture flag upon enabling */
> +		regmap_read(priv->regmap, TIM_CCER, &ccer);
> +		if (!(ccer & stm32_cc[ch].ccer_bits)) {

Try regmap_test_bits() here instead of using regmap_read().

> +			sr =3D ~TIM_SR_CC_IF(ch);
> +			regmap_write(priv->regmap, TIM_SR, sr);

Eliminate 'sr' by regmap_write(priv->regmap, TIM_SR, ~TIM_SR_CC_IF(ch)).

> @@ -366,6 +460,12 @@ static int stm32_count_events_configure(struct count=
er_device *counter)
>  				regmap_write(priv->regmap, TIM_SR, (u32)~TIM_SR_UIF);
>  			dier |=3D TIM_DIER_UIE;
>  			break;
> +		case COUNTER_EVENT_CAPTURE:
> +			ret =3D stm32_count_capture_configure(counter, event_node->channel, t=
rue);
> +			if (ret)
> +				return ret;
> +			dier |=3D TIM_DIER_CC_IE(event_node->channel);

Ah, now I understand why the previous patch OR'd TIM_DIER_UIE to dier.
Apologies for the noise.

> @@ -374,6 +474,15 @@ static int stm32_count_events_configure(struct count=
er_device *counter)
> =20
>  	regmap_write(priv->regmap, TIM_DIER, dier);
> =20
> +	/* check for disabled capture events */
> +	for (i =3D 0 ; i < priv->nchannels; i++) {
> +		if (!(dier & TIM_DIER_CC_IE(i))) {
> +			ret =3D stm32_count_capture_configure(counter, i, false);
> +			if (ret)
> +				return ret;
> +		}

Would for_each_clear_bitrange() in linux/find.h work for this loop?

> @@ -504,7 +620,7 @@ static irqreturn_t stm32_timer_cnt_isr(int irq, void =
*ptr)
>  	 * Some status bits in SR don't match with the enable bits in DIER. Onl=
y take care of
>  	 * the possibly enabled bits in DIER (that matches in between SR and DI=
ER).
>  	 */
> -	dier &=3D TIM_DIER_UIE;
> +	dier &=3D (TIM_DIER_UIE | TIM_DIER_CC1IE | TIM_DIER_CC2IE | TIM_DIER_CC=
3IE | TIM_DIER_CC4IE);

Again, sorry for the noise on the previous patch; this makes sense now.

> @@ -515,6 +631,15 @@ static irqreturn_t stm32_timer_cnt_isr(int irq, void=
 *ptr)
>  		clr &=3D ~TIM_SR_UIF;
>  	}
> =20
> +	/* Check capture events */
> +	for (i =3D 0 ; i < priv->nchannels; i++) {
> +		if (sr & TIM_SR_CC_IF(i)) {

Would for_each_set_bitrange() in linux/find.h work for this loop?

> +			counter_push_event(counter, COUNTER_EVENT_CAPTURE, i);
> +			clr &=3D ~TIM_SR_CC_IF(i);

Perhaps u32p_replace_bits(&clr, 0, TIM_SR_CC_IF(i)) is clearer here.

> @@ -627,8 +752,11 @@ static int stm32_timer_cnt_probe(struct platform_dev=
ice *pdev)
>  		}
>  	} else {
>  		for (i =3D 0; i < priv->nr_irqs; i++) {
> -			/* Only take care of update IRQ for overflow events */
> -			if (i !=3D STM32_TIMERS_IRQ_UP)
> +			/*
> +			 * Only take care of update IRQ for overflow events, and cc for
> +			 * capture events.
> +			 */
> +			if (i !=3D STM32_TIMERS_IRQ_UP && i !=3D STM32_TIMERS_IRQ_CC)
>  				continue;

Okay, I see now why you have this check. This should be fine as it'll
makes adding support in the future for the other IRQs a less invasive
change.

William Breathitt Gray

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

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

iHUEABYIAB0WIQSNN83d4NIlKPjon7a1SFbKvhIjKwUCZZxyDQAKCRC1SFbKvhIj
Ky5wAP4gGBh3+vNrHgCFcl/2xnX9onULDns/GxDSAl/SYUHaxQD/UlBRNYjuTlpn
mQy4bZaGfms5hT09QoJcBuaniegpLQg=
=2KS6
-----END PGP SIGNATURE-----

--WiW47Lt2gdjHTFQC--