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

[Simon Glass]

Hi Stephen,

On 21 March 2014 11:28, Stephen Warren <swarren at wwwdotorg.org> wrote:
> From: Stephen Warren <swarren at 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 at nvidia.com>

Acked-by: Simon Glass <sjg at chromium.org>

But see comment below.

> ---
> V2: New patch.
> ---
>  arch/arm/cpu/tegra-common/pinmux-common.c | 135 +++++-------------------------
>  1 file changed, 23 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..7f7d4a87a4aa 100644
> --- a/arch/arm/cpu/tegra-common/pinmux-common.c
> +++ b/arch/arm/cpu/tegra-common/pinmux-common.c
> @@ -87,11 +87,15 @@
>  #define IO_RESET_SHIFT 8
>  #define RCV_SEL_SHIFT  9
>
> +static inline void update_field(u32 *reg, u32 mask, u32 shift, u32 val)
> +{
> +       clrsetbits_le32(reg, mask << shift, val << shift);

I wonder if it would be better to write this out explicitly in each site.

> +}
> +
>  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 +114,29 @@ 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_field(reg, 3, MUX_SHIFT(pin), mux);

Because here you are obscuring the shift - the parameter order is by
no means obvious.

Or perhaps update_reg_mask_shift_val()?

Regards,
Simon