[U-Boot] [PATCH V3 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver
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 Acked-by: Simon Glass sjg@chromium.org --- 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(-)
diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c b/arch/arm/cpu/tegra-common/pinmux-common.c index 32a46d53f068..7d5c74055644 100644 --- a/arch/arm/cpu/tegra-common/pinmux-common.c +++ b/arch/arm/cpu/tegra-common/pinmux-common.c @@ -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)
Dear Stephen Warren,
In message 1395764855-23377-1-git-send-email-swarren@wwwdotorg.org you wrote:
+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
On 03/25/2014 10:54 AM, Wolfgang Denk wrote:
Dear Stephen Warren,
In message 1395764855-23377-1-git-send-email-swarren@wwwdotorg.org you wrote:
+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.
Dear Stephen Warren,
In message 5331B55B.7080209@wwwdotorg.org you wrote:
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.
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.
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
On Tue, Mar 25, 2014 at 05:54:10PM +0100, Wolfgang Denk wrote:
Dear Stephen Warren,
In message 1395764855-23377-1-git-send-email-swarren@wwwdotorg.org you wrote:
+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.
Dear Tom,
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
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(-)
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)
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.
On Tue, Mar 25, 2014 at 10:27:35AM -0600, Stephen Warren 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.
Since things got a bit heated here while I was reading some other stuff...
[snip]
+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.
Dear Tom,
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
participants (3)
-
Stephen Warren -
Tom Rini -
Wolfgang Denk