Re: [PATCH v4 3/6] commands: add i2c commands

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2569 bytes --]

Hey,

Eric Bénard wrote:
> Le 25/05/2010 11:02, Marc Kleine-Budde a écrit :
>>> +        case 'b':
>>> +            adapter = i2c_get_adapter(simple_strtoul(optarg, NULL, 0));
>>
>> I'd just save the optarg in a variable...
>>
>>> +            break;
>>> +        case 'v':
>>> +            verbose = 1;
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    count = argc - optind;
>>> +
>>> +    if ((addr<  0) || (reg<  0) || (count == 0) || (addr>  0x7F))
>>> +        return COMMAND_ERROR_USAGE;
>>> +
>>> +    if (!adapter)
>>> +        adapter = i2c_get_adapter(0);
>>
>> and use it here. Because if you specify an invalid adapter number,
>> adapter 0 will be used silently (if it exists).
>>
> you're right, in fact I should exit in the case 'b' handling if the
> adapter doesn't exist, that will be simpler.

ACK, I'd do that here.....

int adapter_number = 0;

...

	case 'b':
		adapter_number = simple_strtoul(optarg, NULL, 0);
		break;
...

adapter = i2c_get_adapter(adapter_number);
if (!adapter) {
	printf("adapter %d not found\n", adapter_number);
	return -ENODEV
}

>>> +    if (!adapter)
>>> +        return -ENODEV;
>>> +
>>> +    client.adapter = adapter;
>>> +    client.addr = addr;
>>> +
>>> +    buf = xmalloc(count);
>>> +    for (i = 0; i<  count; i++)
>>> +        *(buf + i) = (char) simple_strtol(argv[optind+i], NULL, 16);

codingstyle...no space after the cast, but before and after the "+"

(char)simple_strtol(argv[optind + i], NULL, 16);

>>> +
>>> +    ret = i2c_write_reg(&client, reg, buf, count);
>>> +    if (ret != count)
>>> +        goto out;
>>
>> better set a return value indicating an error here...
> ret should already be set by i2c_write_reg so it there is no error, I
> can simply set ret to 0.

ACK

>>
>>> +
>>> +    if (verbose) {
>>> +        printf("wrote %i bytes starting at reg 0x%02x to i2cdev
>>> 0x%02x on bus %i\n",
>>> +            count, reg, addr, adapter->nr);
>>> +        for (i = 0; i<  count; i++)
>>> +            printf("0x%02x ", *(buf + i));
>>> +        printf("\n");
>>> +    }
>>> +
>>> +out:
>>> +    free(buf);
>>> +    return 0;
>>
>> ... and return it here.
> 
> yes, that should be return ret
> 
> Eric

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox