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

[Simon Glass]

Hi Stephen,

On 25 March 2014 08:54, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 03/24/2014 08:27 PM, Simon Glass wrote:
>> 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.
>
>>> 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.

Well that at least indicates that the clr mask comes before set.

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

I'll leave it to you, it's just a thought.

Regards,
Simon