[U-Boot] [PATCH v4 02/12] omap4: cleanup pin mux data

[Aneesh V]

Dear Wolfgang,

On Friday 29 July 2011 02:26 PM, Wolfgang Denk wrote:
> Dear Aneesh V,
>
> In message<4E32722C.6020707 at ti.com>  you wrote:
>> Dear Wolfgang,
>>
>> On Friday 29 July 2011 12:56 AM, Wolfgang Denk wrote:
>>> Dear Aneesh V,
>>>
>>> In message<1311233298-17265-3-git-send-email-aneesh at ti.com>   you wrote:
>>>> - separate mux settings into essential and non essential parts
>>>> - essential part is board independent as of now(so move it
>>>>     to SoC directory). Will help in having single SPL for all
>>>>     boards.
>>>> - Non-essential part(the pins not essential for u-boot to function)
>>>>     need to be phased out eventually.
>>>> - Correct mux data by aligning to the latest settings in x-loader
>>> ...
>>>> +	{USBB1_ULPITLL_CLK, (IEN | OFF_EN | OFF_IN | M1)},		/* hsi1_cawake */
>>>> +	{USBB1_ULPITLL_STP, (IEN | OFF_EN | OFF_IN | M1)},		/* hsi1_cadata */
>>>> +	{USBB1_ULPITLL_DIR, (IEN | OFF_EN | OFF_IN | M1)},		/* hsi1_caflag */
>>>> +	{USBB1_ULPITLL_NXT, (OFF_EN | M1)},				/* hsi1_acready */
>>>> +	{USBB1_ULPITLL_DAT0, (OFF_EN | M1)},				/* hsi1_acwake */
>>>> +	{USBB1_ULPITLL_DAT1, (OFF_EN | M1)},				/* hsi1_acdata */
>>>> +	{USBB1_ULPITLL_DAT2, (OFF_EN | M1)},				/* hsi1_acflag */
>>>> +	{USBB1_ULPITLL_DAT3, (IEN | OFF_EN | OFF_IN | M1)},		/* hsi1_caready */
>>>
>>> Lines too long, please fix globally.
>>
>> This table looks better and readable if each row is a single line. I
>> had mentioned this in the commit log. Does it make sense to make an
>> exception for this?
>
> I agree that it does not make sense to split the lines.
>
> But it seems there is potential to reduce the line length: in may
> cases, you can just reduce the indentation of the comments.

Yes, for some lines there is potential to reduce the length. But I just
aligned the comments vertically for the entire table since more than
half of the rows were anyway overflowing.

>
> In many other places the commets are just useless and should be
> omitted.  See for example the (old, existing) lines:
>
>          {CAM_SHUTTER, (OFF_EN | OFF_PD | OFF_OUT_PTD | M0)},            /* cam_shutter */
>          {CAM_STROBE, (OFF_EN | OFF_PD | OFF_OUT_PTD | M0)},             /* cam_strobe */

The two are actually not same. The first column indicates the register
name. The documented name specifies the function selected which depends
on the last column, the mode(M0, M4 etc). The name of the register
happens to be the function selected by M0. So, in those cases, what you
said is correct. But IMHO, it's better to document the function
selected even in those cases, because when somebody changes the mode
here or for a new board, he/she is not likely to update the comment if
it is already absent.

> ...
>          {USBB1_ULPITLL_DAT4, (IEN | OFF_EN | OFF_PD | OFF_IN | M4)},    /* usbb1_ulpiphy_dat4 */
>          {USBB1_ULPITLL_DAT5, (IEN | OFF_EN | OFF_PD | OFF_IN | M4)},    /* usbb1_ulpiphy_dat5 */
>          {USBB1_ULPITLL_DAT6, (IEN | OFF_EN | OFF_PD | OFF_IN | M4)},    /* usbb1_ulpiphy_dat6 */
> ...
> etc.

For the above please note that first column and comments are indeed
different. Please note the difference between ULPI'TLL'_DATA4 and
ulpi'phy'_data4.

My patch is already in u-boot-arm. If I have to rework this patch
should I create a new patch to fix the problem or should I re-work the
original patch?

best regards,
Aneesh