diff mbox

[U-Boot,V4,09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

Message ID 1395774584-17305-1-git-send-email-wd@denx.de
State Deferred
Delegated to: Tom Warren
Headers show

Commit Message

Wolfgang Denk March 25, 2014, 7:09 p.m. UTC 
From: Stephen Warren <swarren@nvidia.com>

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 <swarren@nvidia.com>
Signed-off-by: Wolfgang Denk <wd@denx.de>
---
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(-)

Comments

Stephen Warren March 25, 2014, 8 p.m. UTC | #1 
On 03/25/2014 01:09 PM, Wolfgang Denk wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> 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 <swarren@nvidia.com>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> ---
> 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.
diff mbox

Patch

diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c b/arch/arm/cpu/tegra-common/pinmux-common.c
index 32a46d5..fec5e37 100644
--- a/arch/arm/cpu/tegra-common/pinmux-common.c
+++ b/arch/arm/cpu/tegra-common/pinmux-common.c
@@ -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)