ARM: tegra: use clrsetbits_le32 in pinmux driver

PATCH Superseded Stephen Warren 3 participants 10 messages Mar 25, 2014

Subsystems (2)

Arm (32-bit) Tegra architecture support

lore

Share & tools

lore

Stephen Warren

author

2014-03-25 16:27 UTC #0 lore raw

PATCH v3 9/13 ARM: tegra: use clrsetbits_le32 in pinmux driver

From: Stephen Warren <[email protected]>

This removes a bunch of open-coded register IO, masking, and shifting. I would have squashed this into "ARM: tegra: pinctrl: remove duplication" except that keeping it a separate commit allows easier bisection of any issues that are introduced by this patch. I also wrote this patch on top of the series, and pushing it any lower in the series results in some conflicts I didn't feel like fixing.

Signed-off-by Stephen Warren <[email protected]>

Acked-by Simon Glass <[email protected]>

---
V3: Rename update_field() to update_reg_mask_shift_val() to make the
parameter order more obvious.
V2: New patch.

(I'm only reposting V3 of this one patch in order to avoid spamming the list with the other huge table replacements in the series. Hopefully this isn't too painful when applying them).

---
 arch/arm/cpu/tegra-common/pinmux-common.c | 140 ++++++------------------------
 1 file changed, 28 insertions(+), 112 deletions(-)

arch/arm/cpu/tegra-common/pinmux-common.c +28 ~~-112~~

index 32a46d53f068..7d5c74055644 100644
@@ -87,11 +87,16 @@
 #define IO_RESET_SHIFT	8
 #define RCV_SEL_SHIFT	9
 
+static inline void update_reg_mask_shift_val(u32 *reg, u32 mask, u32 shift,
+					     u32 val)
+{
+	clrsetbits_le32(reg, mask << shift, val << shift);
+}
+
 void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func func)
 {
 	u32 *reg = MUX_REG(pin);
 	int i, mux = -1;
-	u32 val;
 
 	/* Error check on pin and func */
 	assert(pmux_pingrp_isvalid(pin));

@@ -110,42 +115,30 @@ void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func func)
 	}
 	assert(mux != -1);
 
-	val = readl(reg);
-	val &= ~(3 << MUX_SHIFT(pin));
-	val |= (mux << MUX_SHIFT(pin));
-	writel(val, reg);
+	update_reg_mask_shift_val(reg, 3, MUX_SHIFT(pin), mux);
 }
 
 void pinmux_set_pullupdown(enum pmux_pingrp pin, enum pmux_pull pupd)
 {
 	u32 *reg = PULL_REG(pin);
-	u32 val;
 
 	/* Error check on pin and pupd */
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_pupd_isvalid(pupd));
 
-	val = readl(reg);
-	val &= ~(3 << PULL_SHIFT(pin));
-	val |= (pupd << PULL_SHIFT(pin));
-	writel(val, reg);
+	update_reg_mask_shift_val(reg, 3, PULL_SHIFT(pin), pupd);
 }
 
 static void pinmux_set_tristate(enum pmux_pingrp pin, int tri)
 {
 	u32 *reg = TRI_REG(pin);
-	u32 val;
 
 	/* Error check on pin */
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_tristate_isvalid(tri));
 
-	val = readl(reg);
-	if (tri == PMUX_TRI_TRISTATE)
-		val |= (1 << TRI_SHIFT(pin));
-	else
-		val &= ~(1 << TRI_SHIFT(pin));
-	writel(val, reg);
+	update_reg_mask_shift_val(reg, 1, TRI_SHIFT(pin),
+				  tri == PMUX_TRI_TRISTATE);
 }
 
 void pinmux_tristate_enable(enum pmux_pingrp pin)

@@ -162,7 +155,6 @@ void pinmux_tristate_disable(enum pmux_pingrp pin)
 void pinmux_set_io(enum pmux_pingrp pin, enum pmux_pin_io io)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (io == PMUX_PIN_NONE)
 		return;

@@ -171,18 +163,12 @@ void pinmux_set_io(enum pmux_pingrp pin, enum pmux_pin_io io)
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_io_isvalid(io));
 
-	val = readl(reg);
-	if (io == PMUX_PIN_INPUT)
-		val |= (io & 1) << IO_SHIFT;
-	else
-		val &= ~(1 << IO_SHIFT);
-	writel(val, reg);
+	update_reg_mask_shift_val(reg, 1, IO_SHIFT, io == PMUX_PIN_INPUT);
 }
 
 static void pinmux_set_lock(enum pmux_pingrp pin, enum pmux_pin_lock lock)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (lock == PMUX_PIN_LOCK_DEFAULT)
 		return;

@@ -191,23 +177,19 @@ static void pinmux_set_lock(enum pmux_pingrp pin, enum pmux_pin_lock lock)
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_lock_isvalid(lock));
 
-	val = readl(reg);
-	if (lock == PMUX_PIN_LOCK_ENABLE) {
-		val |= (1 << LOCK_SHIFT);
-	} else {
+	if (lock == PMUX_PIN_LOCK_DISABLE) {
+		u32 val = readl(reg);
 		if (val & (1 << LOCK_SHIFT))
 			printf("%s: Cannot clear LOCK bit!\n", __func__);
-		val &= ~(1 << LOCK_SHIFT);
 	}
-	writel(val, reg);
 
-	return;
+	update_reg_mask_shift_val(reg, 1, LOCK_SHIFT,
+				  lock == PMUX_PIN_LOCK_ENABLE);
 }
 
 static void pinmux_set_od(enum pmux_pingrp pin, enum pmux_pin_od od)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (od == PMUX_PIN_OD_DEFAULT)
 		return;

@@ -216,21 +198,13 @@ static void pinmux_set_od(enum pmux_pingrp pin, enum pmux_pin_od od)
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_od_isvalid(od));
 
-	val = readl(reg);
-	if (od == PMUX_PIN_OD_ENABLE)
-		val |= (1 << OD_SHIFT);
-	else
-		val &= ~(1 << OD_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, 1, OD_SHIFT, od == PMUX_PIN_OD_ENABLE);
 }
 
 static void pinmux_set_ioreset(enum pmux_pingrp pin,
 				enum pmux_pin_ioreset ioreset)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (ioreset == PMUX_PIN_IO_RESET_DEFAULT)
 		return;

@@ -239,14 +213,8 @@ static void pinmux_set_ioreset(enum pmux_pingrp pin,
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_ioreset_isvalid(ioreset));
 
-	val = readl(reg);
-	if (ioreset == PMUX_PIN_IO_RESET_ENABLE)
-		val |= (1 << IO_RESET_SHIFT);
-	else
-		val &= ~(1 << IO_RESET_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, 1, IO_RESET_SHIFT,
+				  ioreset == PMUX_PIN_IO_RESET_ENABLE);
 }
 
 #ifdef TEGRA_PMX_HAS_RCV_SEL

@@ -254,7 +222,6 @@ static void pinmux_set_rcv_sel(enum pmux_pingrp pin,
 				enum pmux_pin_rcv_sel rcv_sel)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (rcv_sel == PMUX_PIN_RCV_SEL_DEFAULT)
 		return;

@@ -263,14 +230,8 @@ static void pinmux_set_rcv_sel(enum pmux_pingrp pin,
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_rcv_sel_isvalid(rcv_sel));
 
-	val = readl(reg);
-	if (rcv_sel == PMUX_PIN_RCV_SEL_HIGH)
-		val |= (1 << RCV_SEL_SHIFT);
-	else
-		val &= ~(1 << RCV_SEL_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, 1, RCV_SEL_SHIFT,
+				  rcv_sel == PMUX_PIN_RCV_SEL_HIGH);
 }
 #endif /* TEGRA_PMX_HAS_RCV_SEL */
 #endif /* TEGRA_PMX_HAS_PIN_IO_BIT_ETC */

@@ -337,7 +298,6 @@ void pinmux_config_pingrp_table(const struct pmux_pingrp_config *config,
 static void pinmux_set_drvup_slwf(enum pmux_drvgrp grp, int slwf)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (slwf == PMUX_SLWF_NONE)

@@ -347,18 +307,12 @@ static void pinmux_set_drvup_slwf(enum pmux_drvgrp grp, int slwf)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_slw_isvalid(slwf));
 
-	val = readl(reg);
-	val &= ~SLWF_MASK;
-	val |= (slwf << SLWF_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, SLWF_MASK, SLWF_SHIFT, slwf);
 }
 
 static void pinmux_set_drvdn_slwr(enum pmux_drvgrp grp, int slwr)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (slwr == PMUX_SLWR_NONE)

@@ -368,18 +322,12 @@ static void pinmux_set_drvdn_slwr(enum pmux_drvgrp grp, int slwr)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_slw_isvalid(slwr));
 
-	val = readl(reg);
-	val &= ~SLWR_MASK;
-	val |= (slwr << SLWR_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, SLWR_MASK, SLWR_SHIFT, slwr);
 }
 
 static void pinmux_set_drvup(enum pmux_drvgrp grp, int drvup)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (drvup == PMUX_DRVUP_NONE)

@@ -389,18 +337,12 @@ static void pinmux_set_drvup(enum pmux_drvgrp grp, int drvup)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_drv_isvalid(drvup));
 
-	val = readl(reg);
-	val &= ~DRVUP_MASK;
-	val |= (drvup << DRVUP_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, DRVUP_MASK, DRVUP_SHIFT, drvup);
 }
 
 static void pinmux_set_drvdn(enum pmux_drvgrp grp, int drvdn)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (drvdn == PMUX_DRVDN_NONE)

@@ -410,18 +352,12 @@ static void pinmux_set_drvdn(enum pmux_drvgrp grp, int drvdn)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_drv_isvalid(drvdn));
 
-	val = readl(reg);
-	val &= ~DRVDN_MASK;
-	val |= (drvdn << DRVDN_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, DRVDN_MASK, DRVDN_SHIFT, drvdn);
 }
 
 static void pinmux_set_lpmd(enum pmux_drvgrp grp, enum pmux_lpmd lpmd)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (lpmd == PMUX_LPMD_NONE)

@@ -431,18 +367,12 @@ static void pinmux_set_lpmd(enum pmux_drvgrp grp, enum pmux_lpmd lpmd)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_lpmd_isvalid(lpmd));
 
-	val = readl(reg);
-	val &= ~LPMD_MASK;
-	val |= (lpmd << LPMD_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, LPMD_MASK, LPMD_SHIFT, lpmd);
 }
 
 static void pinmux_set_schmt(enum pmux_drvgrp grp, enum pmux_schmt schmt)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (schmt == PMUX_SCHMT_NONE)

@@ -452,20 +382,13 @@ static void pinmux_set_schmt(enum pmux_drvgrp grp, enum pmux_schmt schmt)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_schmt_isvalid(schmt));
 
-	val = readl(reg);
-	if (schmt == PMUX_SCHMT_ENABLE)
-		val |= (1 << SCHMT_SHIFT);
-	else
-		val &= ~(1 << SCHMT_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, 1, SCHMT_SHIFT,
+				  schmt == PMUX_SCHMT_ENABLE);
 }
 
 static void pinmux_set_hsm(enum pmux_drvgrp grp, enum pmux_hsm hsm)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (hsm == PMUX_HSM_NONE)

@@ -475,14 +398,7 @@ static void pinmux_set_hsm(enum pmux_drvgrp grp, enum pmux_hsm hsm)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_hsm_isvalid(hsm));
 
-	val = readl(reg);
-	if (hsm == PMUX_HSM_ENABLE)
-		val |= (1 << HSM_SHIFT);
-	else
-		val &= ~(1 << HSM_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, 1, HSM_SHIFT, hsm == PMUX_HSM_ENABLE);
 }
 
 static void pinmux_config_drvgrp(const struct pmux_drvgrp_config *config)

Signature · 1 line

1.8.1.5

Wolfgang Denk

2014-03-25 16:54 UTC #1 lore raw

PATCH v3 9/13 ARM: tegra: use clrsetbits_le32 in pinmux driver

Dear Stephen Warren,

arch/arm/cpu/tegra-common/pinmux-common.c line 94

Quoted 6 lines · In message <[email protected]> you wrote:

arch/arm/cpu/tegra-common/pinmux-common.c +5 -0

+static inline void update_reg_mask_shift_val(u32 *reg, u32 mask, u32 shift,
+					     u32 val)
+{
+	clrsetbits_le32(reg, mask << shift, val << shift);
+}

No, please do not do that. Please use plain clrsetbits_le32() as is. All these hidden shifts are (a) mostly unreadable and (b) sometimes dangerous.

Thanks.

Wolfgang Denk

Signature · 6 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The use of COBOL cripples the mind; its teaching  should,  therefore,
be regarded as a criminal offence.
          -- Edsger W. Dijkstra, SIGPLAN Notices, Volume 17, Number 5

Stephen Warren

author

2014-03-25 16:56 UTC #2 lore raw

PATCH v3 9/13 ARM: tegra: use clrsetbits_le32 in pinmux driver

Quoted 4 lines · Wolfgang Denk

Dear Stephen Warren,
Quoted 1 line · In message <[email protected]> you wrote:

Quoted 9 lines

arch/arm/cpu/tegra-common/pinmux-common.c line 94
Quoted 5 lines
arch/arm/cpu/tegra-common/pinmux-common.c +5 -0
+static inline void update_reg_mask_shift_val(u32 *reg, u32 mask, u32 shift,
+					     u32 val)
+{
+	clrsetbits_le32(reg, mask << shift, val << shift);
+}
No, please do not do that. Please use plain clrsetbits_le32() as is. All these hidden shifts are (a) mostly unreadable and (b) sometimes dangerous.

Seriously, are you joking now?????

If I was to write out the clrsetbits_le32() at each call site, I'd be writing out this supposedly dangerous shift N times instead of once. If the shift is somehow dangerous (BTW, it isn't!) then surely isolating it in one place, so that mistakes aren't made when writing the duplicate copies, is the right thing to do.

Wolfgang Denk

2014-03-25 17:06 UTC #3 lore raw

PATCH v3 9/13 ARM: tegra: use clrsetbits_le32 in pinmux driver

Dear Stephen Warren,

Quoted 6 lines · In message <[email protected]> you wrote:

Quoted 4 lines
No, please do not do that. Please use plain clrsetbits_le32() as is. All these hidden shifts are (a) mostly unreadable and (b) sometimes dangerous.
Seriously, are you joking now?????

No, I am not.

Quoted 2 lines

If I was to write out the clrsetbits_le32() at each call site, I'd be writing out this supposedly dangerous shift N times instead of once. If

N = 2, to be precise. And you have to type it only once, but to maintain that code for a long, long time.

Quoted 3 lines

the shift is somehow dangerous (BTW, it isn't!) then surely isolating it in one place, so that mistakes aren't made when writing the duplicate copies, is the right thing to do.

Well, I've just fixed a number of places were such code _was_ dangerous, but well hidden under a nice wrapper function like yours.

Best regards,

Wolfgang Denk

Signature · 4 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Sometimes, too long is too long.                          - Joe Crowe

Tom Rini

2014-03-25 20:04 UTC #4 lore raw

PATCH v3 9/13 ARM: tegra: use clrsetbits_le32 in pinmux driver

In reply to Wolfgang Denk

Quoted 4 lines · Wolfgang Denk

Dear Stephen Warren,
Quoted 1 line · In message <[email protected]> you wrote:

Quoted 9 lines

arch/arm/cpu/tegra-common/pinmux-common.c line 94
Quoted 5 lines
arch/arm/cpu/tegra-common/pinmux-common.c +5 -0
+static inline void update_reg_mask_shift_val(u32 *reg, u32 mask, u32 shift,
+					     u32 val)
+{
+	clrsetbits_le32(reg, mask << shift, val << shift);
+}
No, please do not do that. Please use plain clrsetbits_le32() as is. All these hidden shifts are (a) mostly unreadable and (b) sometimes dangerous.

No, this is why the lack of comments hurts things. This isn't sr32 from OMAP-land (which was on my todo list somewhere, thanks). sr32 was an incorrect generic function. This is a specific-use function that should say something like:
/*
* Set the correct pinmux value for a given part. We need to clear out
* M bits worth of the field and then set possibly less than M bits
* worth of value.
*/

With respect to danger / readability, no, either way is just as dangerous (or not dangerous) and it's still fairly dense code either way and fixing a problem with an incorrect shift value is the same effort.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140325/95b3eb9b/attachment.pgp>

Wolfgang Denk

2014-03-25 22:19 UTC #5 lore raw

PATCH v3 9/13 ARM: tegra: use clrsetbits_le32 in pinmux driver

Dear Tom,

Quoted 5 lines · In message <20140325200420.GU16360@bill-the-cat> you wrote:

With respect to danger / readability, no, either way is just as dangerous (or not dangerous) and it's still fairly dense code either way and fixing a problem with an incorrect shift value is the same effort.

The key problem which I detected with sr32() was that it was in several places called with a width of 32 - which looked perfectly fine when the intention was to clear / set the whole 32 bit variable. This went unnoticed becuase it was just a normally looking argument. If the shift operation that resulted from that had been visible, the problem would have been much easier to detect. Seeing an expression "value << 32" on u32 data types rings some alarms.

Best regards,

Wolfgang Denk

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [email protected]
If I had to live my life again,  I'd  make  the  same  mistakes, only
sooner.                                          -- Tallulah Bankhead

Wolfgang Denk

2014-03-25 19:09 UTC #6 lore raw

PATCH v4 9/13 ARM: tegra: use clrsetbits_le32 in pinmux driver

In reply to Stephen Warren

From: Stephen Warren <[email protected]>

Signed-off-by Stephen Warren <[email protected]>

Signed-off-by Wolfgang Denk <[email protected]>

---
V4: [wd] Drop update_reg_mask_shift_val() and use clrsetbits_le32()
directly.
V3: Rename update_field() to update_reg_mask_shift_val() to make the
parameter order more obvious.
V2: New patch.

 arch/arm/cpu/tegra-common/pinmux-common.c | 137 ++++++------------------------
 1 file changed, 25 insertions(+), 112 deletions(-)

arch/arm/cpu/tegra-common/pinmux-common.c +25 ~~-112~~

index 32a46d5..fec5e37 100644
@@ -91,7 +91,6 @@ void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func func)
 {
 	u32 *reg = MUX_REG(pin);
 	int i, mux = -1;
-	u32 val;
 
 	/* Error check on pin and func */
 	assert(pmux_pingrp_isvalid(pin));

@@ -110,42 +109,30 @@ void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func func)
 	}
 	assert(mux != -1);
 
-	val = readl(reg);
-	val &= ~(3 << MUX_SHIFT(pin));
-	val |= (mux << MUX_SHIFT(pin));
-	writel(val, reg);
+	clrsetbits_le32(reg, 3 << MUX_SHIFT(pin), mux << MUX_SHIFT(pin));
 }
 
 void pinmux_set_pullupdown(enum pmux_pingrp pin, enum pmux_pull pupd)
 {
 	u32 *reg = PULL_REG(pin);
-	u32 val;
 
 	/* Error check on pin and pupd */
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_pupd_isvalid(pupd));
 
-	val = readl(reg);
-	val &= ~(3 << PULL_SHIFT(pin));
-	val |= (pupd << PULL_SHIFT(pin));
-	writel(val, reg);
+	clrsetbits_le32(reg, 3 << PULL_SHIFT(pin), pupd << PULL_SHIFT(pin));
 }
 
 static void pinmux_set_tristate(enum pmux_pingrp pin, int tri)
 {
 	u32 *reg = TRI_REG(pin);
-	u32 val;
 
 	/* Error check on pin */
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_tristate_isvalid(tri));
 
-	val = readl(reg);
-	if (tri == PMUX_TRI_TRISTATE)
-		val |= (1 << TRI_SHIFT(pin));
-	else
-		val &= ~(1 << TRI_SHIFT(pin));
-	writel(val, reg);
+	clrsetbits_le32(reg, 1 << TRI_SHIFT(pin),
+			(tri == PMUX_TRI_TRISTATE) << TRI_SHIFT(pin));
 }
 
 void pinmux_tristate_enable(enum pmux_pingrp pin)

@@ -162,7 +149,6 @@ void pinmux_tristate_disable(enum pmux_pingrp pin)
 void pinmux_set_io(enum pmux_pingrp pin, enum pmux_pin_io io)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (io == PMUX_PIN_NONE)
 		return;

@@ -171,18 +157,13 @@ void pinmux_set_io(enum pmux_pingrp pin, enum pmux_pin_io io)
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_io_isvalid(io));
 
-	val = readl(reg);
-	if (io == PMUX_PIN_INPUT)
-		val |= (io & 1) << IO_SHIFT;
-	else
-		val &= ~(1 << IO_SHIFT);
-	writel(val, reg);
+	clrsetbits_le32(reg, 1 << IO_SHIFT,
+			(io == PMUX_PIN_INPUT) << IO_SHIFT);
 }
 
 static void pinmux_set_lock(enum pmux_pingrp pin, enum pmux_pin_lock lock)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (lock == PMUX_PIN_LOCK_DEFAULT)
 		return;

@@ -191,23 +172,19 @@ static void pinmux_set_lock(enum pmux_pingrp pin, enum pmux_pin_lock lock)
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_lock_isvalid(lock));
 
-	val = readl(reg);
-	if (lock == PMUX_PIN_LOCK_ENABLE) {
-		val |= (1 << LOCK_SHIFT);
-	} else {
+	if (lock == PMUX_PIN_LOCK_DISABLE) {
+		u32 val = readl(reg);
 		if (val & (1 << LOCK_SHIFT))
 			printf("%s: Cannot clear LOCK bit!\n", __func__);
-		val &= ~(1 << LOCK_SHIFT);
 	}
-	writel(val, reg);
 
-	return;
+	clrsetbits_le32(reg, 1 << LOCK_SHIFT,
+			(lock == PMUX_PIN_LOCK_ENABLE) << LOCK_SHIFT);
 }
 
 static void pinmux_set_od(enum pmux_pingrp pin, enum pmux_pin_od od)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (od == PMUX_PIN_OD_DEFAULT)
 		return;

@@ -216,21 +193,14 @@ static void pinmux_set_od(enum pmux_pingrp pin, enum pmux_pin_od od)
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_od_isvalid(od));
 
-	val = readl(reg);
-	if (od == PMUX_PIN_OD_ENABLE)
-		val |= (1 << OD_SHIFT);
-	else
-		val &= ~(1 << OD_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, 1 << OD_SHIFT,
+			(od == PMUX_PIN_OD_ENABLE) << OD_SHIFT);
 }
 
 static void pinmux_set_ioreset(enum pmux_pingrp pin,
 				enum pmux_pin_ioreset ioreset)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (ioreset == PMUX_PIN_IO_RESET_DEFAULT)
 		return;

@@ -239,14 +209,8 @@ static void pinmux_set_ioreset(enum pmux_pingrp pin,
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_ioreset_isvalid(ioreset));
 
-	val = readl(reg);
-	if (ioreset == PMUX_PIN_IO_RESET_ENABLE)
-		val |= (1 << IO_RESET_SHIFT);
-	else
-		val &= ~(1 << IO_RESET_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, 1 << IO_RESET_SHIFT,
+		(ioreset == PMUX_PIN_IO_RESET_ENABLE) << IO_RESET_SHIFT);
 }
 
 #ifdef TEGRA_PMX_HAS_RCV_SEL

@@ -254,7 +218,6 @@ static void pinmux_set_rcv_sel(enum pmux_pingrp pin,
 				enum pmux_pin_rcv_sel rcv_sel)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (rcv_sel == PMUX_PIN_RCV_SEL_DEFAULT)
 		return;

@@ -263,14 +226,8 @@ static void pinmux_set_rcv_sel(enum pmux_pingrp pin,
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_rcv_sel_isvalid(rcv_sel));
 
-	val = readl(reg);
-	if (rcv_sel == PMUX_PIN_RCV_SEL_HIGH)
-		val |= (1 << RCV_SEL_SHIFT);
-	else
-		val &= ~(1 << RCV_SEL_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, 1 << RCV_SEL_SHIFT,
+		(rcv_sel == PMUX_PIN_RCV_SEL_HIGH) << RCV_SEL_SHIFT);
 }
 #endif /* TEGRA_PMX_HAS_RCV_SEL */
 #endif /* TEGRA_PMX_HAS_PIN_IO_BIT_ETC */

@@ -337,7 +294,6 @@ void pinmux_config_pingrp_table(const struct pmux_pingrp_config *config,
 static void pinmux_set_drvup_slwf(enum pmux_drvgrp grp, int slwf)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (slwf == PMUX_SLWF_NONE)

@@ -347,18 +303,12 @@ static void pinmux_set_drvup_slwf(enum pmux_drvgrp grp, int slwf)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_slw_isvalid(slwf));
 
-	val = readl(reg);
-	val &= ~SLWF_MASK;
-	val |= (slwf << SLWF_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, SLWF_MASK << SLWF_SHIFT, slwf << SLWF_SHIFT);
 }
 
 static void pinmux_set_drvdn_slwr(enum pmux_drvgrp grp, int slwr)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (slwr == PMUX_SLWR_NONE)

@@ -368,18 +318,12 @@ static void pinmux_set_drvdn_slwr(enum pmux_drvgrp grp, int slwr)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_slw_isvalid(slwr));
 
-	val = readl(reg);
-	val &= ~SLWR_MASK;
-	val |= (slwr << SLWR_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, SLWR_MASK << SLWR_SHIFT, slwr << SLWR_SHIFT);
 }
 
 static void pinmux_set_drvup(enum pmux_drvgrp grp, int drvup)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (drvup == PMUX_DRVUP_NONE)

@@ -389,18 +333,12 @@ static void pinmux_set_drvup(enum pmux_drvgrp grp, int drvup)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_drv_isvalid(drvup));
 
-	val = readl(reg);
-	val &= ~DRVUP_MASK;
-	val |= (drvup << DRVUP_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, DRVUP_MASK << DRVUP_SHIFT, drvup << DRVUP_SHIFT);
 }
 
 static void pinmux_set_drvdn(enum pmux_drvgrp grp, int drvdn)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (drvdn == PMUX_DRVDN_NONE)

@@ -410,18 +348,12 @@ static void pinmux_set_drvdn(enum pmux_drvgrp grp, int drvdn)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_drv_isvalid(drvdn));
 
-	val = readl(reg);
-	val &= ~DRVDN_MASK;
-	val |= (drvdn << DRVDN_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, DRVDN_MASK << DRVDN_SHIFT, drvdn << DRVDN_SHIFT);
 }
 
 static void pinmux_set_lpmd(enum pmux_drvgrp grp, enum pmux_lpmd lpmd)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (lpmd == PMUX_LPMD_NONE)

@@ -431,18 +363,12 @@ static void pinmux_set_lpmd(enum pmux_drvgrp grp, enum pmux_lpmd lpmd)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_lpmd_isvalid(lpmd));
 
-	val = readl(reg);
-	val &= ~LPMD_MASK;
-	val |= (lpmd << LPMD_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, LPMD_MASK << LPMD_SHIFT, lpmd << LPMD_SHIFT);
 }
 
 static void pinmux_set_schmt(enum pmux_drvgrp grp, enum pmux_schmt schmt)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (schmt == PMUX_SCHMT_NONE)

@@ -452,20 +378,13 @@ static void pinmux_set_schmt(enum pmux_drvgrp grp, enum pmux_schmt schmt)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_schmt_isvalid(schmt));
 
-	val = readl(reg);
-	if (schmt == PMUX_SCHMT_ENABLE)
-		val |= (1 << SCHMT_SHIFT);
-	else
-		val &= ~(1 << SCHMT_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, 1 << SCHMT_SHIFT,
+			(schmt == PMUX_SCHMT_ENABLE) << SCHMT_SHIFT);
 }
 
 static void pinmux_set_hsm(enum pmux_drvgrp grp, enum pmux_hsm hsm)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (hsm == PMUX_HSM_NONE)

@@ -475,14 +394,8 @@ static void pinmux_set_hsm(enum pmux_drvgrp grp, enum pmux_hsm hsm)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_hsm_isvalid(hsm));
 
-	val = readl(reg);
-	if (hsm == PMUX_HSM_ENABLE)
-		val |= (1 << HSM_SHIFT);
-	else
-		val &= ~(1 << HSM_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, 1 << HSM_SHIFT,
+			(hsm == PMUX_HSM_ENABLE) << HSM_SHIFT);
 }
 
 static void pinmux_config_drvgrp(const struct pmux_drvgrp_config *config)

Signature · 1 line

1.8.5.3

Stephen Warren

author

2014-03-25 20:00 UTC #7 lore raw

PATCH v4 9/13 ARM: tegra: use clrsetbits_le32 in pinmux driver

Quoted 9 lines · Wolfgang Denk

From: Stephen Warren <[email protected]>
This removes a bunch of open-coded register IO, masking, and shifting. I would have squashed this into "ARM: tegra: pinctrl: remove duplication" except that keeping it a separate commit allows easier bisection of any issues that are introduced by this patch. I also wrote this patch on top of the series, and pushing it any lower in the series results in some conflicts I didn't feel like fixing.

Quoted 5 lines

Signed-off-by Stephen Warren <[email protected]>
Signed-off-by Wolfgang Denk <[email protected]>
---
V4: [wd] Drop update_reg_mask_shift_val() and use clrsetbits_le32()
directly.

I believe your own argument compels you to NAK this patch yourself.

In both V3 and V4:

1) Some very simple and obviously understandable open-coded bit manipulation is replaced with a function call which hides the details of the bit manipulation.

2) In both cases, the order of parameters to the function does actually appear to be obvious from the function name: clrsetbits_le32(clr, set), update_reg_mask_shift_val(reg, mask, shift, val)

3) I've seen plenty of examples where the function name doesn't correctly describe what the function does, or the parameter order doesn't match what would appear to be logical. Hence, in neither case is point (2) relevant; one must /always/ check or remember the prototype of a function in order to validate that the parameters at a call-site are correct. Not checking means making an assumption, and assumptions can be incorrect.

Now, I believe you argued that it was unsafe to hide the details of the bit manipulation behind a function precisely because it was then harder to see what the code was doing, and easier to pass the wrong values to the function parameters, and this wouldn't be obvious at the call site.

I believe the /exact/ same argument applies no matter whether the function is clrsetbits_le32() or update_reg_mask_shift_val().

Hence, if V3 which used update_reg_mask_shift_val() was unacceptable, then so is this.

So NAK.

Tom Rini

2014-03-25 20:04 UTC #8 lore raw

PATCH v3 9/13 ARM: tegra: use clrsetbits_le32 in pinmux driver

In reply to Stephen Warren

Quoted 8 lines · Stephen Warren

From: Stephen Warren <[email protected]>
This removes a bunch of open-coded register IO, masking, and shifting. I would have squashed this into "ARM: tegra: pinctrl: remove duplication" except that keeping it a separate commit allows easier bisection of any issues that are introduced by this patch. I also wrote this patch on top of the series, and pushing it any lower in the series results in some conflicts I didn't feel like fixing.

Since things got a bit heated here while I was reading some other stuff...

[snip]

arch/arm/cpu/tegra-common/pinmux-common.c line 94

Quoted 5 lines

arch/arm/cpu/tegra-common/pinmux-common.c +5 -0

+static inline void update_reg_mask_shift_val(u32 *reg, u32 mask, u32 shift,
+					     u32 val)
+{
+	clrsetbits_le32(reg, mask << shift, val << shift);
+}

So, lack of comments bad. Intention, good. We have a bitfield of size M (that's all cleared in the mask) and value that may be less than M bits wide. The name is a mouthful (but I see where Simon was coming from, had I caught in time I might have suggested a comment instead.

But as Wolfgang's v4 shows, it's also not hard to just call clrsetbits_le32 directly. Arguably the cases where mask==1 we should just call setbits_le32 but that's not a big deal.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140325/f5fbda1d/attachment.pgp>

Wolfgang Denk

2014-03-25 22:26 UTC #9 lore raw

PATCH v3 9/13 ARM: tegra: use clrsetbits_le32 in pinmux driver

Dear Tom,

Quoted 4 lines · In message <20140325200435.GV16360@bill-the-cat> you wrote:

But as Wolfgang's v4 shows, it's also not hard to just call clrsetbits_le32 directly. Arguably the cases where mask==1 we should just call setbits_le32 but that's not a big deal.

We would have to call setbits_le32() or clrbits_le32() depending on the arguments...

I was already about to rewrite the code more in the style previously used, i. e. turn for example

134 clrsetbits_le32(reg, 1 << TRI_SHIFT(pin),
135 (tri == PMUX_TRI_TRISTATE) << TRI_SHIFT(pin));

back into

	if (tri == PMUX_TRI_TRISTATE)
		setbits_le32(reg, 1 << TRI_SHIFT(pin));
	else
		clrbits_le32(reg, 1 << TRI_SHIFT(pin));

but then I decided to keep the changes copmpared to the update_reg_mask_shift_val() version minimal.

If the if/then version should be preferred, I can easily redo that patch.

Best regards,

Wolfgang Denk

Signature · 6 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
No, I'm not going to explain it. If you  can't  figure  it  out,  you
didn't want to know anyway... :-)
                   - Larry Wall in <[email protected]>