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].
> + 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 = ~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?
> @@ -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?
> + 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.
> @@ -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
Attachment:
signature.asc
Description: PGP signature
