Re: [PATCH v3 10/10] counter: stm32-timer-cnt: add support for capture events

[Fabrice Gasnier]

On 1/8/24 23:07, William Breathitt Gray wrote:
> On Wed, Dec 20, 2023 at 03:57:26PM +0100, Fabrice Gasnier wrote:
>> +	/*
>> +	 * configure channel in input capture mode, map channel 1 on TI1, channel2 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].

Hi William,

Thanks for suggesting.
Done.

> 
>> +	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().

Done,

> 
>> +			sr = ~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 counter_device *counter)
>>  				regmap_write(priv->regmap, TIM_SR, (u32)~TIM_SR_UIF);
>>  			dier |= TIM_DIER_UIE;
>>  			break;
>> +		case COUNTER_EVENT_CAPTURE:
>> +			ret = stm32_count_capture_configure(counter, event_node->channel, true);
>> +			if (ret)
>> +				return ret;
>> +			dier |= 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 counter_device *counter)
>>  
>>  	regmap_write(priv->regmap, TIM_DIER, dier);
>>  
>> +	/* check for disabled capture events */
>> +	for (i = 0 ; i < priv->nchannels; i++) {
>> +		if (!(dier & TIM_DIER_CC_IE(i))) {
>> +			ret = stm32_count_capture_configure(counter, i, false);
>> +			if (ret)
>> +				return ret;
>> +		}
> 
> Would for_each_clear_bitrange() in linux/find.h work for this loop?

I had a look, but it requires to add some variables, for start and stop
bit in the bitmap. For now, I've kept the simple BIT macro and bit ops.

> 
>> @@ -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. Only take care of
>>  	 * the possibly enabled bits in DIER (that matches in between SR and DIER).
>>  	 */
>> -	dier &= TIM_DIER_UIE;
>> +	dier &= (TIM_DIER_UIE | TIM_DIER_CC1IE | TIM_DIER_CC2IE | TIM_DIER_CC3IE | 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 &= ~TIM_SR_UIF;
>>  	}
>>  
>> +	/* Check capture events */
>> +	for (i = 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?

same.

> 
>> +			counter_push_event(counter, COUNTER_EVENT_CAPTURE, i);
>> +			clr &= ~TIM_SR_CC_IF(i);
> 
> Perhaps u32p_replace_bits(&clr, 0, TIM_SR_CC_IF(i)) is clearer here.

I've hit some build issue with TIM_SR_CC_IF(i) macro, e.g.:
/include/linux/bitfield.h:165:17: error: call to ‘__bad_mask’ declared
with attribute error: bad bitfield mask
  165 |                 __bad_mask();

So I've kept the simple bit operation here.

Thanks & Best Regards,
Fabrice

> 
>> @@ -627,8 +752,11 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
>>  		}
>>  	} else {
>>  		for (i = 0; i < priv->nr_irqs; i++) {
>> -			/* Only take care of update IRQ for overflow events */
>> -			if (i != STM32_TIMERS_IRQ_UP)
>> +			/*
>> +			 * Only take care of update IRQ for overflow events, and cc for
>> +			 * capture events.
>> +			 */
>> +			if (i != STM32_TIMERS_IRQ_UP && i != 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