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
