[U-Boot-Users] [PATCH 4/6 part 5] UCC slow mode support in QUICC Engine

Thu Aug 17 15:28:24 CEST 2006

Dear Tanya,

in message <5B3767CDCE2C2B46B2BCB72C4833E8882096B8 at zch01exm24.fsl.freescale.net> you wrote:
> Subject: [PATCH] UCC slow mode support in QUICC Engine

No. You Subject: is:

	[PATCH 4/6 part 8] PHY support for QE

May I ask what you expect when submitting such a puzzle  of  patches?
Part 8 of part 4 of 6? Do you really expect me to collect all your
patches from dozens of fragments? Sorry, I don't have so much time.

And event your fragments of the parts are still to big:

    List:    U-Boot-Users at lists.sourceforge.net
    From:    tanya.jiang at freescale.com
    Subject: [PATCH 4/6 part 6] header file for QE UCC geth feature
    Reason:  Message body is too big: 43058 bytes with a limit of 40 KB

Also, your patches have been corrupted by your mailer which wrapped
lines, for example (from "[PATCH 4/6 part 5]"):

> diff --git a/drivers/sysdev/qe_lib/ucc/ucc_slow.c
> b/drivers/sysdev/qe_lib/ucc/ucc_slow.c
> new file mode 100644
> index 0000000..8e060f8
> --- /dev/null
> +++ b/drivers/sysdev/qe_lib/ucc/ucc_slow.c
> @@ -0,0 +1,397 @@
> +/*
> + * drivers/sysdev/qe_libucc/ucc_slow.c
> + *
> + * QE UCC Slow API Set - UCC Slow specific routines implementations.
> + *
> + * (C) Copyright 2006 Freescale Semiconductor, Inc
> + * Author: Shlomi Gridish <gridish at freescale.com>
> + *
> + * History:
> + * 20060601 tanya jiang (tanya.jiang at freescale.com)
> + *	    Code style fixed; move from cpu/mpc83xx to drivers/sysdev
> + *
> + * This program is free software; you can redistribute  it and/or
> modify it
^^^^^^^^^^^
> + * under  the terms of  the GNU General  Public License as published by
> the
^^^^^^^^^^^
> + * Free Software Foundation;  either version 2 of the  License, or (at
> your
^^^^^^^^^^^
...
> +void ucc_slow_stop_tx(ucc_slow_private_t * uccs)
> +{
> +	ucc_slow_info_t *us_info = uccs->us_info;
> +	u32 id;
> +
> +	id = ucc_slow_get_qe_cr_subblock(us_info->ucc_num);
> +	qe_issue_cmd(QE_STOP_TX, id, (u8) QE_CR_PROTOCOL_UNSPECIFIED,
> 0);
^^^^^^^^^^^
> +}
> +
> +void ucc_slow_restart_tx(ucc_slow_private_t * uccs)
> +{
> +	ucc_slow_info_t *us_info = uccs->us_info;
> +	u32 id;
> +
> +	id = ucc_slow_get_qe_cr_subblock(us_info->ucc_num);
> +	qe_issue_cmd(QE_RESTART_TX, id, (u8) QE_CR_PROTOCOL_UNSPECIFIED,
> 0);
^^^^^^^^^^^

And so on.

Please also stick to the Coding Style requirements. A cursory scan of
your patches showed C++ comments, indentation by spaces instead of
TABs, trailing white space, etc. This has to be cleaned up.

Also, parts of the code look more complex than needed. For example:

> +u32 ucc_slow_get_qe_cr_subblock(int uccs_num)
> +{
> +	switch (uccs_num) {
> +	case (0):
> +		return (QE_CR_SUBBLOCK_UCCSLOW1);
> +	case (1):
> +		return (QE_CR_SUBBLOCK_UCCSLOW2);
> +	case (2):
> +		return (QE_CR_SUBBLOCK_UCCSLOW3);
> +	case (3):
> +		return (QE_CR_SUBBLOCK_UCCSLOW4);
> +	case (4):
> +		return (QE_CR_SUBBLOCK_UCCSLOW5);
> +	case (5):
> +		return (QE_CR_SUBBLOCK_UCCSLOW6);
> +	case (6):
> +		return (QE_CR_SUBBLOCK_UCCSLOW7);
> +	case (7):
> +		return (QE_CR_SUBBLOCK_UCCSLOW8);
> +	default:
> +		return QE_CR_SUBBLOCK_INVALID;
> +	}
> +}

Why don't you write (and probably inline!) this as

	if (uccs_num > 7)
		return QE_CR_SUBBLOCK_INVALID;
	return (uccs_num << 21);

?

In other places there is code like this:

> +	if (us_info->cdp)
> +		gumr |= UCC_SLOW_GUMR_H_CDP;
> +	if (us_info->ctsp)
> +		gumr |= UCC_SLOW_GUMR_H_CTSP;
> +	if (us_info->cds)
> +		gumr |= UCC_SLOW_GUMR_H_CDS;
> +	if (us_info->ctss)
> +		gumr |= UCC_SLOW_GUMR_H_CTSS;
> +	if (us_info->tfl)
> +		gumr |= UCC_SLOW_GUMR_H_TFL;
> +	if (us_info->rfw)
> +		gumr |= UCC_SLOW_GUMR_H_RFW;
> +	if (us_info->txsy)
> +		gumr |= UCC_SLOW_GUMR_H_TXSY;
> +	if (us_info->rtsm)
> +		gumr |= UCC_SLOW_GUMR_H_RTSM;
...
> +	if (us_info->tci)
> +		gumr |= UCC_SLOW_GUMR_L_TCI;
> +	if (us_info->rinv)
> +		gumr |= UCC_SLOW_GUMR_L_RINV;
> +	if (us_info->tinv)
> +		gumr |= UCC_SLOW_GUMR_L_TINV;
> +	if (us_info->tend)
> +		gumr |= UCC_SLOW_GUMR_L_TEND;

or this:

> +	snums[i++].num = 0x04; 
> +	snums[i++].num = 0x05; 
> +	snums[i++].num = 0x0C; 
> +	snums[i++].num = 0x0D; 
> +	snums[i++].num = 0x14; 
> +	snums[i++].num = 0x15; 
> +	snums[i++].num = 0x1C; 
> +	snums[i++].num = 0x1D; 
> +	snums[i++].num = 0x24; 
> +	snums[i++].num = 0x25; 
> +	snums[i++].num = 0x2C; 
> +	snums[i++].num = 0x2D; 
> +	snums[i++].num = 0x34; 
> +	snums[i++].num = 0x35; 
> +	snums[i++].num = 0x88; 
> +	snums[i++].num = 0x89; 
> +	snums[i++].num = 0x98; 
> +	snums[i++].num = 0x99; 
> +	snums[i++].num = 0xA8; 
> +	snums[i++].num = 0xA9; 
> +	snums[i++].num = 0xB8; 
> +	snums[i++].num = 0xB9; 
> +	snums[i++].num = 0xC8; 
> +	snums[i++].num = 0xC9; 
> +	snums[i++].num = 0xD8; 
> +	snums[i++].num = 0xD9; 
> +	snums[i++].num = 0xE8; 
> +	snums[i++].num = 0xE9; 

To me this looks either like poor coding and/or like a broken design.

Of course, the biggest problem and killing point is the corruption of
the patches by your mailer. Because of this I have to reject ALL  the
patches  you  submitted  lately. Sorry, but there is no other option.
You will not receive reject messages for the individual messages.

Please clean up before resubmitting. When resubmitting,  please  make
sure  the  patches  get through uncorrupted. Also make sure to follow
the patch submission rules. And don't fragment patches  like  this  -
this is not acceptable.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The connection between the language in which we think/program and the
problems and solutions we can imagine is very close. For this  reason
restricting  language  features  with  the intent of eliminating pro-
grammer errors is at best dangerous.
               - Bjarne Stroustrup in "The  C++ Programming Language"