| Message ID | 0735860960b1b38570bffa5b0de81a97f6e3230e.1718352022.git.u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | mfd: stm32-timers: Make register definition more flexible | expand |
On Fri, 14 Jun 2024, Uwe Kleine-König wrote: > These two defines have the same purpose and this change doesn't > introduce any differences in drivers/counter/stm32-timer-cnt.o. > > The only difference between the two is that > > TIM_DIER_CC_IE(1) == TIM_DIER_CC2IE > > while > > TIM_DIER_CCxIE(1) == TIM_DIER_CC1IE > > . That makes it necessary to have an explicit "+ 1" in the user code, > but IMHO this is a good thing as this is the code locatation that > "knows" that for software channel 1 you have to use TIM_DIER_CC2IE > (because software guys start counting at 0, while the relevant hardware > designer started at 1). > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > --- > drivers/counter/stm32-timer-cnt.c | 4 ++-- The subject should be renamed. > include/linux/mfd/stm32-timers.h | 1 - > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c > index 0664ef969f79..186e73d6ccb4 100644 > --- a/drivers/counter/stm32-timer-cnt.c > +++ b/drivers/counter/stm32-timer-cnt.c > @@ -465,7 +465,7 @@ static int stm32_count_events_configure(struct counter_device *counter) > ret = stm32_count_capture_configure(counter, event_node->channel, true); > if (ret) > return ret; > - dier |= TIM_DIER_CC_IE(event_node->channel); > + dier |= TIM_DIER_CCxIE(event_node->channel + 1); > break; > default: > /* should never reach this path */ > @@ -478,7 +478,7 @@ static int stm32_count_events_configure(struct counter_device *counter) > > /* check for disabled capture events */ > for (i = 0 ; i < priv->nchannels; i++) { > - if (!(dier & TIM_DIER_CC_IE(i))) { > + if (!(dier & TIM_DIER_CCxIE(i + 1))) { > ret = stm32_count_capture_configure(counter, i, false); > if (ret) > return ret; > diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h > index 92b45a559656..f09ba598c97a 100644 > --- a/include/linux/mfd/stm32-timers.h > +++ b/include/linux/mfd/stm32-timers.h > @@ -47,7 +47,6 @@ > #define TIM_DIER_CC2IE TIM_DIER_CCxIE(2) /* CC2 Interrupt Enable */ > #define TIM_DIER_CC3IE TIM_DIER_CCxIE(3) /* CC3 Interrupt Enable */ > #define TIM_DIER_CC4IE TIM_DIER_CCxIE(4) /* CC4 Interrupt Enable */ > -#define TIM_DIER_CC_IE(x) BIT((x) + 1) /* CC1, CC2, CC3, CC4 interrupt enable */ > #define TIM_DIER_UDE BIT(8) /* Update DMA request Enable */ > #define TIM_DIER_CCxDE(x) BIT(9 + ((x) - 1)) /* CCx DMA request Enable (x ∈ {1, .. 4}) */ > #define TIM_DIER_CC1DE TIM_DIER_CCxDE(1) /* CC1 DMA request Enable */ > -- > 2.43.0 >
On Fri, Jun 14, 2024 at 10:10:13AM +0200, Uwe Kleine-König wrote: > These two defines have the same purpose and this change doesn't > introduce any differences in drivers/counter/stm32-timer-cnt.o. > > The only difference between the two is that > > TIM_DIER_CC_IE(1) == TIM_DIER_CC2IE > > while > > TIM_DIER_CCxIE(1) == TIM_DIER_CC1IE > > . That makes it necessary to have an explicit "+ 1" in the user code, > but IMHO this is a good thing as this is the code locatation that > "knows" that for software channel 1 you have to use TIM_DIER_CC2IE > (because software guys start counting at 0, while the relevant hardware > designer started at 1). > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> I concur with Lee Jones that the subject should be renamed. Regardless, here's my ack for the code changes. Acked-by: William Breathitt Gray <wbg@kernel.org>
Hello Lee, On Fri, Jun 14, 2024 at 10:31:24AM +0100, Lee Jones wrote: > On Fri, 14 Jun 2024, Uwe Kleine-König wrote: > > > These two defines have the same purpose and this change doesn't > > introduce any differences in drivers/counter/stm32-timer-cnt.o. > > > > The only difference between the two is that > > > > TIM_DIER_CC_IE(1) == TIM_DIER_CC2IE > > > > while > > > > TIM_DIER_CCxIE(1) == TIM_DIER_CC1IE > > > > . That makes it necessary to have an explicit "+ 1" in the user code, > > but IMHO this is a good thing as this is the code locatation that > > "knows" that for software channel 1 you have to use TIM_DIER_CC2IE > > (because software guys start counting at 0, while the relevant hardware > > designer started at 1). > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > > --- > > drivers/counter/stm32-timer-cnt.c | 4 ++-- > > The subject should be renamed. I guess you mean it should be something like: counter: stm32-timer-cnt: Drop TIM_DIER_CC_IE(x) in favour of TIM_DIER_CCxIE(x + 1) ? Best regards Uwe
diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c index 0664ef969f79..186e73d6ccb4 100644 --- a/drivers/counter/stm32-timer-cnt.c +++ b/drivers/counter/stm32-timer-cnt.c @@ -465,7 +465,7 @@ static int stm32_count_events_configure(struct counter_device *counter) ret = stm32_count_capture_configure(counter, event_node->channel, true); if (ret) return ret; - dier |= TIM_DIER_CC_IE(event_node->channel); + dier |= TIM_DIER_CCxIE(event_node->channel + 1); break; default: /* should never reach this path */ @@ -478,7 +478,7 @@ static int stm32_count_events_configure(struct counter_device *counter) /* check for disabled capture events */ for (i = 0 ; i < priv->nchannels; i++) { - if (!(dier & TIM_DIER_CC_IE(i))) { + if (!(dier & TIM_DIER_CCxIE(i + 1))) { ret = stm32_count_capture_configure(counter, i, false); if (ret) return ret; diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h index 92b45a559656..f09ba598c97a 100644 --- a/include/linux/mfd/stm32-timers.h +++ b/include/linux/mfd/stm32-timers.h @@ -47,7 +47,6 @@ #define TIM_DIER_CC2IE TIM_DIER_CCxIE(2) /* CC2 Interrupt Enable */ #define TIM_DIER_CC3IE TIM_DIER_CCxIE(3) /* CC3 Interrupt Enable */ #define TIM_DIER_CC4IE TIM_DIER_CCxIE(4) /* CC4 Interrupt Enable */ -#define TIM_DIER_CC_IE(x) BIT((x) + 1) /* CC1, CC2, CC3, CC4 interrupt enable */ #define TIM_DIER_UDE BIT(8) /* Update DMA request Enable */ #define TIM_DIER_CCxDE(x) BIT(9 + ((x) - 1)) /* CCx DMA request Enable (x ∈ {1, .. 4}) */ #define TIM_DIER_CC1DE TIM_DIER_CCxDE(1) /* CC1 DMA request Enable */
These two defines have the same purpose and this change doesn't introduce any differences in drivers/counter/stm32-timer-cnt.o. The only difference between the two is that TIM_DIER_CC_IE(1) == TIM_DIER_CC2IE while TIM_DIER_CCxIE(1) == TIM_DIER_CC1IE . That makes it necessary to have an explicit "+ 1" in the user code, but IMHO this is a good thing as this is the code locatation that "knows" that for software channel 1 you have to use TIM_DIER_CC2IE (because software guys start counting at 0, while the relevant hardware designer started at 1). Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/counter/stm32-timer-cnt.c | 4 ++-- include/linux/mfd/stm32-timers.h | 1 - 2 files changed, 2 insertions(+), 3 deletions(-)