On 03/25/2014 10:04 AM, Simon Glass wrote:
> On 25 March 2014 08:54, Stephen Warren <swar...@wwwdotorg.org> wrote:
>> On 03/24/2014 08:27 PM, Simon Glass wrote:
>>> On 21 March 2014 11:28, Stephen Warren <swar...@wwwdotorg.org> wrote:
>>>> 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 <swar...@nvidia.com>
>>>
>>> Acked-by: Simon Glass <s...@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.
...
>>>> + 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.
Well, the wrapping didn't work out to badly, so I posted V3 including
this rename.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
