* [PATCH 0/3] pwm: stm32: Minor cleanups @ 2019-10-16 7:38 Thierry Reding 2019-10-16 7:38 ` [PATCH 1/3] pwm: stm32: Remove clutter from ternary operator Thierry Reding ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Thierry Reding @ 2019-10-16 7:38 UTC (permalink / raw) To: Thierry Reding; +Cc: Uwe Kleine-König, Fabrice Gasnier, linux-pwm Hi, Looking at Fabrice's STM32 patches I noticed that we're now passing the breakinput values (u32) into a function via int parameters. The easiest way to fix this inconsistency is by just passing a pointer to the break input structure. There's some preparatory work here that makes the code slightly more readable, in my opinion, but it's really marginal, so I'm not terribly thrilled by this series in retrospect. If nobody else thinks this is a big improvement I'll just scrap it. Thierry Thierry Reding (3): pwm: stm32: Remove clutter from ternary operator pwm: stm32: Remove confusing bitmask pwm: stm32: Pass breakinput instead of its values drivers/pwm/pwm-stm32.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) -- 2.23.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] pwm: stm32: Remove clutter from ternary operator 2019-10-16 7:38 [PATCH 0/3] pwm: stm32: Minor cleanups Thierry Reding @ 2019-10-16 7:38 ` Thierry Reding 2019-10-16 8:26 ` Uwe Kleine-König 2019-10-16 7:38 ` [PATCH 2/3] pwm: stm32: Remove confusing bitmask Thierry Reding ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Thierry Reding @ 2019-10-16 7:38 UTC (permalink / raw) To: Thierry Reding; +Cc: Uwe Kleine-König, Fabrice Gasnier, linux-pwm Remove usage of the ternary operator to assign values for register fields. This removes clutter and improves readability. Signed-off-by: Thierry Reding <thierry.reding@gmail.com> --- drivers/pwm/pwm-stm32.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c index 9430b4cd383f..b12fb11b7a55 100644 --- a/drivers/pwm/pwm-stm32.c +++ b/drivers/pwm/pwm-stm32.c @@ -493,11 +493,19 @@ static const struct pwm_ops stm32pwm_ops = { static int stm32_pwm_set_breakinput(struct stm32_pwm *priv, int index, int level, int filter) { - u32 bke = (index == 0) ? TIM_BDTR_BKE : TIM_BDTR_BK2E; - int shift = (index == 0) ? TIM_BDTR_BKF_SHIFT : TIM_BDTR_BK2F_SHIFT; - u32 mask = (index == 0) ? TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF - : TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F; - u32 bdtr = bke; + u32 bke, shift, mask, bdtr; + + if (index == 0) { + bke = TIM_BDTR_BKE; + shift = TIM_BDTR_BKF_SHIFT; + mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF; + } else { + bke = TIM_BDTR_BK2E; + shift = TIM_BDTR_BK2F_SHIFT; + mask = TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F; + } + + bdtr = bke; /* * The both bits could be set since only one will be wrote -- 2.23.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] pwm: stm32: Remove clutter from ternary operator 2019-10-16 7:38 ` [PATCH 1/3] pwm: stm32: Remove clutter from ternary operator Thierry Reding @ 2019-10-16 8:26 ` Uwe Kleine-König 2019-10-16 9:30 ` Thierry Reding 0 siblings, 1 reply; 11+ messages in thread From: Uwe Kleine-König @ 2019-10-16 8:26 UTC (permalink / raw) To: Thierry Reding; +Cc: Fabrice Gasnier, linux-pwm On Wed, Oct 16, 2019 at 09:38:40AM +0200, Thierry Reding wrote: > Remove usage of the ternary operator to assign values for register > fields. This removes clutter and improves readability. > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > --- > drivers/pwm/pwm-stm32.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c > index 9430b4cd383f..b12fb11b7a55 100644 > --- a/drivers/pwm/pwm-stm32.c > +++ b/drivers/pwm/pwm-stm32.c > @@ -493,11 +493,19 @@ static const struct pwm_ops stm32pwm_ops = { > static int stm32_pwm_set_breakinput(struct stm32_pwm *priv, > int index, int level, int filter) > { > - u32 bke = (index == 0) ? TIM_BDTR_BKE : TIM_BDTR_BK2E; > - int shift = (index == 0) ? TIM_BDTR_BKF_SHIFT : TIM_BDTR_BK2F_SHIFT; > - u32 mask = (index == 0) ? TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF > - : TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F; > - u32 bdtr = bke; > + u32 bke, shift, mask, bdtr; > + > + if (index == 0) { > + bke = TIM_BDTR_BKE; > + shift = TIM_BDTR_BKF_SHIFT; > + mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF; > + } else { > + bke = TIM_BDTR_BK2E; > + shift = TIM_BDTR_BK2F_SHIFT; > + mask = TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F; > + } > + > + bdtr = bke; Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Is index always in {0, 1}? Maybe a comment or a check about that would be helpful. (-> separate patch I think). Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] pwm: stm32: Remove clutter from ternary operator 2019-10-16 8:26 ` Uwe Kleine-König @ 2019-10-16 9:30 ` Thierry Reding 0 siblings, 0 replies; 11+ messages in thread From: Thierry Reding @ 2019-10-16 9:30 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Fabrice Gasnier, linux-pwm [-- Attachment #1: Type: text/plain, Size: 2000 bytes --] On Wed, Oct 16, 2019 at 10:26:12AM +0200, Uwe Kleine-König wrote: > On Wed, Oct 16, 2019 at 09:38:40AM +0200, Thierry Reding wrote: > > Remove usage of the ternary operator to assign values for register > > fields. This removes clutter and improves readability. > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > drivers/pwm/pwm-stm32.c | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c > > index 9430b4cd383f..b12fb11b7a55 100644 > > --- a/drivers/pwm/pwm-stm32.c > > +++ b/drivers/pwm/pwm-stm32.c > > @@ -493,11 +493,19 @@ static const struct pwm_ops stm32pwm_ops = { > > static int stm32_pwm_set_breakinput(struct stm32_pwm *priv, > > int index, int level, int filter) > > { > > - u32 bke = (index == 0) ? TIM_BDTR_BKE : TIM_BDTR_BK2E; > > - int shift = (index == 0) ? TIM_BDTR_BKF_SHIFT : TIM_BDTR_BK2F_SHIFT; > > - u32 mask = (index == 0) ? TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF > > - : TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F; > > - u32 bdtr = bke; > > + u32 bke, shift, mask, bdtr; > > + > > + if (index == 0) { > > + bke = TIM_BDTR_BKE; > > + shift = TIM_BDTR_BKF_SHIFT; > > + mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF; > > + } else { > > + bke = TIM_BDTR_BK2E; > > + shift = TIM_BDTR_BK2F_SHIFT; > > + mask = TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F; > > + } > > + > > + bdtr = bke; > > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Is index always in {0, 1}? Maybe a comment or a check about that would > be helpful. (-> separate patch I think). The bindings say that index can only be 0 or 1. I guess strictly it might actually depend on the number of break inputs, but given these register definitions, there's only ever two. But yeah, it might be a good idea to sanitize the values upon probe. I'll add another patch to do that. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] pwm: stm32: Remove confusing bitmask 2019-10-16 7:38 [PATCH 0/3] pwm: stm32: Minor cleanups Thierry Reding 2019-10-16 7:38 ` [PATCH 1/3] pwm: stm32: Remove clutter from ternary operator Thierry Reding @ 2019-10-16 7:38 ` Thierry Reding 2019-10-16 8:31 ` Uwe Kleine-König 2019-10-16 7:38 ` [PATCH 3/3] pwm: stm32: Pass breakinput instead of its values Thierry Reding 2019-10-16 8:33 ` [PATCH 0/3] pwm: stm32: Minor cleanups Uwe Kleine-König 3 siblings, 1 reply; 11+ messages in thread From: Thierry Reding @ 2019-10-16 7:38 UTC (permalink / raw) To: Thierry Reding; +Cc: Uwe Kleine-König, Fabrice Gasnier, linux-pwm Both BKP bits are set in the BDTR register and the code relies on the mask used during write to make sure only one of them is written. Since this isn't immediately obvious, a comment is needed to explain it. The same can be achieved by making explicit what happens, so add another temporary variable that contains only the one bit that is actually ORed into the register and get rid of the comment. Signed-off-by: Thierry Reding <thierry.reding@gmail.com> --- drivers/pwm/pwm-stm32.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c index b12fb11b7a55..8f1f3371e1dd 100644 --- a/drivers/pwm/pwm-stm32.c +++ b/drivers/pwm/pwm-stm32.c @@ -493,26 +493,24 @@ static const struct pwm_ops stm32pwm_ops = { static int stm32_pwm_set_breakinput(struct stm32_pwm *priv, int index, int level, int filter) { - u32 bke, shift, mask, bdtr; + u32 bke, bkp, shift, mask, bdtr; if (index == 0) { bke = TIM_BDTR_BKE; + bkp = TIM_BDTR_BKP; shift = TIM_BDTR_BKF_SHIFT; mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF; } else { bke = TIM_BDTR_BK2E; + bkp = TIM_BDTR_BK2P; shift = TIM_BDTR_BK2F_SHIFT; mask = TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F; } bdtr = bke; - /* - * The both bits could be set since only one will be wrote - * due to mask value. - */ if (level) - bdtr |= TIM_BDTR_BKP | TIM_BDTR_BK2P; + bdtr |= bkp; bdtr |= (filter & TIM_BDTR_BKF_MASK) << shift; -- 2.23.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] pwm: stm32: Remove confusing bitmask 2019-10-16 7:38 ` [PATCH 2/3] pwm: stm32: Remove confusing bitmask Thierry Reding @ 2019-10-16 8:31 ` Uwe Kleine-König 2019-10-16 9:50 ` Thierry Reding 0 siblings, 1 reply; 11+ messages in thread From: Uwe Kleine-König @ 2019-10-16 8:31 UTC (permalink / raw) To: Thierry Reding; +Cc: Fabrice Gasnier, linux-pwm On Wed, Oct 16, 2019 at 09:38:41AM +0200, Thierry Reding wrote: > Both BKP bits are set in the BDTR register and the code relies on the > mask used during write to make sure only one of them is written. Since > this isn't immediately obvious, a comment is needed to explain it. The > same can be achieved by making explicit what happens, so add another > temporary variable that contains only the one bit that is actually ORed > into the register and get rid of the comment. > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > --- > drivers/pwm/pwm-stm32.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c > index b12fb11b7a55..8f1f3371e1dd 100644 > --- a/drivers/pwm/pwm-stm32.c > +++ b/drivers/pwm/pwm-stm32.c > @@ -493,26 +493,24 @@ static const struct pwm_ops stm32pwm_ops = { > static int stm32_pwm_set_breakinput(struct stm32_pwm *priv, > int index, int level, int filter) > { > - u32 bke, shift, mask, bdtr; > + u32 bke, bkp, shift, mask, bdtr; > > if (index == 0) { > bke = TIM_BDTR_BKE; > + bkp = TIM_BDTR_BKP; > shift = TIM_BDTR_BKF_SHIFT; > mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF; > } else { > bke = TIM_BDTR_BK2E; > + bkp = TIM_BDTR_BK2P; > shift = TIM_BDTR_BK2F_SHIFT; > mask = TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F; Assuming in the else branch index is always 1, the following would be IMHO nicer: #define TIM_BDTR_BKE(i) BIT(12 + 12 * (i)) #define TIM_BDTR_BKP(i) BIT(13 + 12 * (i)) #define TIM_BDTR_BKF_SHIFT(i) (16 + 4 * (i)) .. bke = TIM_BDTR_BKE(index); bkp = TIM_BDTR_BKP(index); Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] pwm: stm32: Remove confusing bitmask 2019-10-16 8:31 ` Uwe Kleine-König @ 2019-10-16 9:50 ` Thierry Reding 2019-10-16 10:20 ` Fabrice Gasnier 0 siblings, 1 reply; 11+ messages in thread From: Thierry Reding @ 2019-10-16 9:50 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Fabrice Gasnier, linux-pwm [-- Attachment #1: Type: text/plain, Size: 2455 bytes --] On Wed, Oct 16, 2019 at 10:31:07AM +0200, Uwe Kleine-König wrote: > On Wed, Oct 16, 2019 at 09:38:41AM +0200, Thierry Reding wrote: > > Both BKP bits are set in the BDTR register and the code relies on the > > mask used during write to make sure only one of them is written. Since > > this isn't immediately obvious, a comment is needed to explain it. The > > same can be achieved by making explicit what happens, so add another > > temporary variable that contains only the one bit that is actually ORed > > into the register and get rid of the comment. > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > drivers/pwm/pwm-stm32.c | 10 ++++------ > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c > > index b12fb11b7a55..8f1f3371e1dd 100644 > > --- a/drivers/pwm/pwm-stm32.c > > +++ b/drivers/pwm/pwm-stm32.c > > @@ -493,26 +493,24 @@ static const struct pwm_ops stm32pwm_ops = { > > static int stm32_pwm_set_breakinput(struct stm32_pwm *priv, > > int index, int level, int filter) > > { > > - u32 bke, shift, mask, bdtr; > > + u32 bke, bkp, shift, mask, bdtr; > > > > if (index == 0) { > > bke = TIM_BDTR_BKE; > > + bkp = TIM_BDTR_BKP; > > shift = TIM_BDTR_BKF_SHIFT; > > mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF; > > } else { > > bke = TIM_BDTR_BK2E; > > + bkp = TIM_BDTR_BK2P; > > shift = TIM_BDTR_BK2F_SHIFT; > > mask = TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F; > > Assuming in the else branch index is always 1, the following would be > IMHO nicer: > > #define TIM_BDTR_BKE(i) BIT(12 + 12 * (i)) > #define TIM_BDTR_BKP(i) BIT(13 + 12 * (i)) > #define TIM_BDTR_BKF_SHIFT(i) (16 + 4 * (i)) > > .. > > bke = TIM_BDTR_BKE(index); > bkp = TIM_BDTR_BKP(index); I had thought about that, but ultimately decided against it because the original defines might match exactly what's in the datasheet, so there's some value to keep the originals. I suppose one other alternative would be to let the macros be and do the computations in the driver instead, something like: bke = TIM_BDTR_BKE << (index * 12); bkp = TIM_BDTR_BKP << (index * 12); bkf = TIM_BDTR_BKF << (index * 4); But yeah, I agree that having the parameters be part of the macros is even better. Fabrice, any objection to redefining the macros as above? Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] pwm: stm32: Remove confusing bitmask 2019-10-16 9:50 ` Thierry Reding @ 2019-10-16 10:20 ` Fabrice Gasnier 2019-10-16 10:52 ` Uwe Kleine-König 0 siblings, 1 reply; 11+ messages in thread From: Fabrice Gasnier @ 2019-10-16 10:20 UTC (permalink / raw) To: Thierry Reding, Uwe Kleine-König; +Cc: linux-pwm On 10/16/19 11:50 AM, Thierry Reding wrote: > On Wed, Oct 16, 2019 at 10:31:07AM +0200, Uwe Kleine-König wrote: >> On Wed, Oct 16, 2019 at 09:38:41AM +0200, Thierry Reding wrote: >>> Both BKP bits are set in the BDTR register and the code relies on the >>> mask used during write to make sure only one of them is written. Since >>> this isn't immediately obvious, a comment is needed to explain it. The >>> same can be achieved by making explicit what happens, so add another >>> temporary variable that contains only the one bit that is actually ORed >>> into the register and get rid of the comment. >>> >>> Signed-off-by: Thierry Reding <thierry.reding@gmail.com> >>> --- >>> drivers/pwm/pwm-stm32.c | 10 ++++------ >>> 1 file changed, 4 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c >>> index b12fb11b7a55..8f1f3371e1dd 100644 >>> --- a/drivers/pwm/pwm-stm32.c >>> +++ b/drivers/pwm/pwm-stm32.c >>> @@ -493,26 +493,24 @@ static const struct pwm_ops stm32pwm_ops = { >>> static int stm32_pwm_set_breakinput(struct stm32_pwm *priv, >>> int index, int level, int filter) >>> { >>> - u32 bke, shift, mask, bdtr; >>> + u32 bke, bkp, shift, mask, bdtr; >>> >>> if (index == 0) { >>> bke = TIM_BDTR_BKE; >>> + bkp = TIM_BDTR_BKP; >>> shift = TIM_BDTR_BKF_SHIFT; >>> mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF; >>> } else { >>> bke = TIM_BDTR_BK2E; >>> + bkp = TIM_BDTR_BK2P; >>> shift = TIM_BDTR_BK2F_SHIFT; >>> mask = TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F; >> >> Assuming in the else branch index is always 1, the following would be >> IMHO nicer: >> >> #define TIM_BDTR_BKE(i) BIT(12 + 12 * (i)) >> #define TIM_BDTR_BKP(i) BIT(13 + 12 * (i)) >> #define TIM_BDTR_BKF_SHIFT(i) (16 + 4 * (i)) >> >> .. >> >> bke = TIM_BDTR_BKE(index); >> bkp = TIM_BDTR_BKP(index); > > I had thought about that, but ultimately decided against it because > the original defines might match exactly what's in the datasheet, so > there's some value to keep the originals. > > I suppose one other alternative would be to let the macros be and do the > computations in the driver instead, something like: > > bke = TIM_BDTR_BKE << (index * 12); > bkp = TIM_BDTR_BKP << (index * 12); > bkf = TIM_BDTR_BKF << (index * 4); > > But yeah, I agree that having the parameters be part of the macros is > even better. > > Fabrice, any objection to redefining the macros as above? Hi Thierry, No objection from me, it will probably improve readability. I'd just suggest an alternative to this: maybe a simple struct array with two entries can improve readability ? E.g. keep the defines matching the datasheet, and get rid of the conditional code ? Dirty proposal: static const struct stm32_pwm_brk[] = { /* {bke, bkp, shift, mask} */ { TIM_BDTR_BKE, TIM_BDTR_BKP, ...}, { TIM_BDTR_BK2E, TIM_BDTR_BK2P, ...}, } and use "index" to index it ? But I'm fine with the macros as well: there's already similar things in this driver to deal with the channels for instance. Thanks, Fabrice > > Thierry > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] pwm: stm32: Remove confusing bitmask 2019-10-16 10:20 ` Fabrice Gasnier @ 2019-10-16 10:52 ` Uwe Kleine-König 0 siblings, 0 replies; 11+ messages in thread From: Uwe Kleine-König @ 2019-10-16 10:52 UTC (permalink / raw) To: Fabrice Gasnier; +Cc: Thierry Reding, linux-pwm On Wed, Oct 16, 2019 at 12:20:17PM +0200, Fabrice Gasnier wrote: > On 10/16/19 11:50 AM, Thierry Reding wrote: > > On Wed, Oct 16, 2019 at 10:31:07AM +0200, Uwe Kleine-König wrote: > >> On Wed, Oct 16, 2019 at 09:38:41AM +0200, Thierry Reding wrote: > >>> Both BKP bits are set in the BDTR register and the code relies on the > >>> mask used during write to make sure only one of them is written. Since > >>> this isn't immediately obvious, a comment is needed to explain it. The > >>> same can be achieved by making explicit what happens, so add another > >>> temporary variable that contains only the one bit that is actually ORed > >>> into the register and get rid of the comment. > >>> > >>> Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > >>> --- > >>> drivers/pwm/pwm-stm32.c | 10 ++++------ > >>> 1 file changed, 4 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c > >>> index b12fb11b7a55..8f1f3371e1dd 100644 > >>> --- a/drivers/pwm/pwm-stm32.c > >>> +++ b/drivers/pwm/pwm-stm32.c > >>> @@ -493,26 +493,24 @@ static const struct pwm_ops stm32pwm_ops = { > >>> static int stm32_pwm_set_breakinput(struct stm32_pwm *priv, > >>> int index, int level, int filter) > >>> { > >>> - u32 bke, shift, mask, bdtr; > >>> + u32 bke, bkp, shift, mask, bdtr; > >>> > >>> if (index == 0) { > >>> bke = TIM_BDTR_BKE; > >>> + bkp = TIM_BDTR_BKP; > >>> shift = TIM_BDTR_BKF_SHIFT; > >>> mask = TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF; > >>> } else { > >>> bke = TIM_BDTR_BK2E; > >>> + bkp = TIM_BDTR_BK2P; > >>> shift = TIM_BDTR_BK2F_SHIFT; > >>> mask = TIM_BDTR_BK2E | TIM_BDTR_BK2P | TIM_BDTR_BK2F; > >> > >> Assuming in the else branch index is always 1, the following would be > >> IMHO nicer: > >> > >> #define TIM_BDTR_BKE(i) BIT(12 + 12 * (i)) > >> #define TIM_BDTR_BKP(i) BIT(13 + 12 * (i)) > >> #define TIM_BDTR_BKF_SHIFT(i) (16 + 4 * (i)) > >> > >> .. > >> > >> bke = TIM_BDTR_BKE(index); > >> bkp = TIM_BDTR_BKP(index); > > > > I had thought about that, but ultimately decided against it because > > the original defines might match exactly what's in the datasheet, so > > there's some value to keep the originals. > > > > I suppose one other alternative would be to let the macros be and do the > > computations in the driver instead, something like: > > > > bke = TIM_BDTR_BKE << (index * 12); > > bkp = TIM_BDTR_BKP << (index * 12); > > bkf = TIM_BDTR_BKF << (index * 4); > > > > But yeah, I agree that having the parameters be part of the macros is > > even better. > > > > Fabrice, any objection to redefining the macros as above? > > Hi Thierry, > > No objection from me, it will probably improve readability. > > I'd just suggest an alternative to this: maybe a simple struct array > with two entries can improve readability ? E.g. keep the defines > matching the datasheet, and get rid of the conditional code ? > > Dirty proposal: > > static const struct stm32_pwm_brk[] = { > /* {bke, bkp, shift, mask} */ > { TIM_BDTR_BKE, TIM_BDTR_BKP, ...}, > { TIM_BDTR_BK2E, TIM_BDTR_BK2P, ...}, > } > > and use "index" to index it ? > > But I'm fine with the macros as well: there's already similar things in > this driver to deal with the channels for instance. I didn't test but I wouldn't be surprised if the compiler could better optimize the solution I suggested. Might be worth a test however. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] pwm: stm32: Pass breakinput instead of its values 2019-10-16 7:38 [PATCH 0/3] pwm: stm32: Minor cleanups Thierry Reding 2019-10-16 7:38 ` [PATCH 1/3] pwm: stm32: Remove clutter from ternary operator Thierry Reding 2019-10-16 7:38 ` [PATCH 2/3] pwm: stm32: Remove confusing bitmask Thierry Reding @ 2019-10-16 7:38 ` Thierry Reding 2019-10-16 8:33 ` [PATCH 0/3] pwm: stm32: Minor cleanups Uwe Kleine-König 3 siblings, 0 replies; 11+ messages in thread From: Thierry Reding @ 2019-10-16 7:38 UTC (permalink / raw) To: Thierry Reding; +Cc: Uwe Kleine-König, Fabrice Gasnier, linux-pwm Instead of passing the individual values of the breakpoint, pass a pointer to the breakpoint. Signed-off-by: Thierry Reding <thierry.reding@gmail.com> --- drivers/pwm/pwm-stm32.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c index 8f1f3371e1dd..9e500928c37d 100644 --- a/drivers/pwm/pwm-stm32.c +++ b/drivers/pwm/pwm-stm32.c @@ -491,11 +491,11 @@ static const struct pwm_ops stm32pwm_ops = { }; static int stm32_pwm_set_breakinput(struct stm32_pwm *priv, - int index, int level, int filter) + const struct stm32_breakinput *bi) { u32 bke, bkp, shift, mask, bdtr; - if (index == 0) { + if (bi->index == 0) { bke = TIM_BDTR_BKE; bkp = TIM_BDTR_BKP; shift = TIM_BDTR_BKF_SHIFT; @@ -509,10 +509,10 @@ static int stm32_pwm_set_breakinput(struct stm32_pwm *priv, bdtr = bke; - if (level) + if (bi->level) bdtr |= bkp; - bdtr |= (filter & TIM_BDTR_BKF_MASK) << shift; + bdtr |= (bi->filter & TIM_BDTR_BKF_MASK) << shift; regmap_update_bits(priv->regmap, TIM_BDTR, mask, bdtr); @@ -527,10 +527,7 @@ static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv) int ret; for (i = 0; i < priv->num_breakinputs; i++) { - ret = stm32_pwm_set_breakinput(priv, - priv->breakinputs[i].index, - priv->breakinputs[i].level, - priv->breakinputs[i].filter); + ret = stm32_pwm_set_breakinput(priv, &priv->breakinputs[i]); if (ret < 0) return ret; } -- 2.23.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] pwm: stm32: Minor cleanups 2019-10-16 7:38 [PATCH 0/3] pwm: stm32: Minor cleanups Thierry Reding ` (2 preceding siblings ...) 2019-10-16 7:38 ` [PATCH 3/3] pwm: stm32: Pass breakinput instead of its values Thierry Reding @ 2019-10-16 8:33 ` Uwe Kleine-König 3 siblings, 0 replies; 11+ messages in thread From: Uwe Kleine-König @ 2019-10-16 8:33 UTC (permalink / raw) To: Thierry Reding; +Cc: Fabrice Gasnier, linux-pwm On Wed, Oct 16, 2019 at 09:38:39AM +0200, Thierry Reding wrote: > Hi, > > Looking at Fabrice's STM32 patches I noticed that we're now passing the > breakinput values (u32) into a function via int parameters. The easiest > way to fix this inconsistency is by just passing a pointer to the break > input structure. There's some preparatory work here that makes the code > slightly more readable, in my opinion, but it's really marginal, so I'm > not terribly thrilled by this series in retrospect. > > If nobody else thinks this is a big improvement I'll just scrap it. I like it. Together with my suggestion to add parameters to the register offsets I think it's worth the effort. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-10-16 10:52 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-16 7:38 [PATCH 0/3] pwm: stm32: Minor cleanups Thierry Reding 2019-10-16 7:38 ` [PATCH 1/3] pwm: stm32: Remove clutter from ternary operator Thierry Reding 2019-10-16 8:26 ` Uwe Kleine-König 2019-10-16 9:30 ` Thierry Reding 2019-10-16 7:38 ` [PATCH 2/3] pwm: stm32: Remove confusing bitmask Thierry Reding 2019-10-16 8:31 ` Uwe Kleine-König 2019-10-16 9:50 ` Thierry Reding 2019-10-16 10:20 ` Fabrice Gasnier 2019-10-16 10:52 ` Uwe Kleine-König 2019-10-16 7:38 ` [PATCH 3/3] pwm: stm32: Pass breakinput instead of its values Thierry Reding 2019-10-16 8:33 ` [PATCH 0/3] pwm: stm32: Minor cleanups Uwe Kleine-König
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.