[U-Boot] RFC: factor out common i2c code

[U-Boot] RFC: factor out common i2c code

[Frans Meulenbroeks]

In cmd_i2c the following snippet of code appears 6 times so it would
be a good candidate to factor out into a static helper function:

        /*
         * I2C chip address
         */
        chip = simple_strtoul(argv[1], NULL, 16);

        /*
         * I2C data address within the chip.  This can be 1 or
         * 2 bytes long.  Some day it might be 3 bytes long :-).
         */
        devaddr = simple_strtoul(argv[2], NULL, 16);
        alen = 1;
        for (j = 0; j < 8; j++) {
                if (argv[2][j] == '.') {
                        alen = argv[2][j+1] - '0';
                        if (alen > 3) {
                                cmd_usage(cmdtp);
                                return 1;
                        }
                        break;
                } else if (argv[2][j] == '\0')
                        break;
        }

Now before I actually start doing that a few questions.
Should I factor out all lines and create a helper function with three
pointer arguments to contain the return values chip, devaddr and alen
(apart from argv and cmdptp which are input parameters).

Or would it be better to only factor out only the alen code and have a
function like:
int get_alen(char *str, cmd_tbl_t *cmdtp)
{
    int alen, j;
        alen = 1;
        for (j = 0; j < 8; j++) {
                if str[j] == '.') {
                        alen = str[j+1] - '0';
                        if (alen > 3) {
                                cmd_usage(cmdtp);
                                return -1;
                        }
                        break;
                } else if (str[j] == '\0')
                        break;
       return alen;
}

and call it by something like

   alen = get_alen(argv[2]);
   if (alen < 0) return alen; /* or return -1 */

Your opinion on the preferred solution is appreciated (and ofc keeping
the current code is also an option).

Frans

[U-Boot] RFC: factor out common i2c code

[Wolfgang Denk]

Dear Frans Meulenbroeks,

In message <ac9c93b11003221418j391cbf66jf0cb0688da84dae1@mail.gmail.com> you wrote:
> In cmd_i2c the following snippet of code appears 6 times so it would
> be a good candidate to factor out into a static helper function:

Thanks for digging into this.

> Now before I actually start doing that a few questions.
> Should I factor out all lines and create a helper function with three
> pointer arguments to contain the return values chip, devaddr and alen
> (apart from argv and cmdptp which are input parameters).

A function with 5 arguments makes not much sense for for such a
relatively simple piece of code.

> Or would it be better to only factor out only the alen code and have a
> function like:
> int get_alen(char *str, cmd_tbl_t *cmdtp)
> {
>     int alen, j;
>         alen = 1;
>         for (j = 0; j < 8; j++) {
>                 if str[j] == '.') {
>                         alen = str[j+1] - '0';
>                         if (alen > 3) {
>                                 cmd_usage(cmdtp);
>                                 return -1;
>                         }

Eventually you move the cmd_usage() usage callout of the function so
you can get rid of the second argument.

> and call it by something like
> 
>    alen = get_alen(argv[2]);
>    if (alen < 0) return alen; /* or return -1 */

	if (alen < 0) {
		cmd_usage(cmdtp);
		return -1;
	}

or even use a common error exit like

	if (alen < 0)
		goto errout;

Viele Gr??e,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"Unix is simple, but it takes a genius to understand the simplicity."
					             - Dennis Ritchie

[U-Boot] RFC: factor out common i2c code

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> Dear Frans Meulenbroeks,
> 
> In message <ac9c93b11003221418j391cbf66jf0cb0688da84dae1@mail.gmail.com> you wrote:
>> In cmd_i2c the following snippet of code appears 6 times so it would
>> be a good candidate to factor out into a static helper function:
> 
> Thanks for digging into this.
> 
>> Now before I actually start doing that a few questions.
>> Should I factor out all lines and create a helper function with three
>> pointer arguments to contain the return values chip, devaddr and alen
>> (apart from argv and cmdptp which are input parameters).
> 
> A function with 5 arguments makes not much sense for for such a
> relatively simple piece of code.
> 
>> Or would it be better to only factor out only the alen code and have a
>> function like:
>> int get_alen(char *str, cmd_tbl_t *cmdtp)
>> {
>>     int alen, j;
>>         alen = 1;
>>         for (j = 0; j < 8; j++) {
>>                 if str[j] == '.') {
>>                         alen = str[j+1] - '0';
>>                         if (alen > 3) {
>>                                 cmd_usage(cmdtp);
>>                                 return -1;
>>                         }
> 
> Eventually you move the cmd_usage() usage callout of the function so
> you can get rid of the second argument.
> 
>> and call it by something like
>>
>>    alen = get_alen(argv[2]);
>>    if (alen < 0) return alen; /* or return -1 */
> 
> 	if (alen < 0) {
> 		cmd_usage(cmdtp);
> 		return -1;
> 	}
> 
> or even use a common error exit like
> 
> 	if (alen < 0)
> 		goto errout;

I vote for:

alen = get_alen(argv[2]);
if (alen < 0)
	goto errout;

bye
Heiko