From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Tue, 25 Mar 2014 09:54:30 -0600
Subject: [U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in
	pinmux driver
In-Reply-To: <CAPnjgZ262kWYJEuwEL-+WYKVTd9XxWgT6mzH-XhLirpEKen59Q@mail.gmail.com>
References: <1395426541-18706-1-git-send-email-swarren@wwwdotorg.org>	<1395426541-18706-10-git-send-email-swarren@wwwdotorg.org>
	<CAPnjgZ262kWYJEuwEL-+WYKVTd9XxWgT6mzH-XhLirpEKen59Q@mail.gmail.com>
Message-ID: <5331A6B6.8090805@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 03/24/2014 08:27 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On 21 March 2014 11:28, Stephen Warren <swarren@wwwdotorg.org> 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>
> 
> Acked-by: Simon Glass <sjg@chromium.org>
> 
> But see comment below.

>> diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c b/arch/arm/cpu/tegra-common/pinmux-common.c

>> +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.

Well, for pretty much no function is it obvious from the function name
what the parameter order is; it's just one of those things you memorize
or look up. The exact same issue exists for clrsetbits_le32() itself IMHO.

> Or perhaps update_reg_mask_shift_val()?

Still, I can rename the function if you want; it certainly does make it
obvious. It's rather a long name though, but I guess wrapping the
parameters isn't too bad.