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

[Eric Bénard]

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.

>> +	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);
>> +
>> +	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.
>
>> +
>> +	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