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

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Eric,

On Tue, May 18, 2010 at 11:26:48AM +0200, Eric Bénard wrote:
> theses commands allow low level access to i2c bus and can be useful
> to setup an i2c device without having to add code, compile and flash
> barebox.

This is surely a nice thing to have sometimes. Comments inline.


> 
> Signed-off-by: Eric Bénard <eric@eukrea.com>
> ---
>  commands/Kconfig  |    8 ++
>  commands/Makefile |    1 +
>  commands/i2c.c    |  233 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 242 insertions(+), 0 deletions(-)
>  create mode 100644 commands/i2c.c
> 
> diff --git a/commands/Kconfig b/commands/Kconfig
> index 0c09f91..7115b18 100644
> --- a/commands/Kconfig
> +++ b/commands/Kconfig
> @@ -314,4 +314,12 @@ config CMD_UNLZO
>  	  Say yes here to get the unlzo command. lzo is a fast compression
>  	  algorithm by Markus Franz Xaver Johannes Oberhumer.
>  
> +config CMD_I2C
> +	bool
> +	depends on DRIVER_I2C_I2CDEV
> +	prompt "i2c commands"
> +	help
> +	  include i2c_probe, i2c_read and i2c_write commands to communicate
> +	  on i2c bus.
> +
>  endmenu
> diff --git a/commands/Makefile b/commands/Makefile
> index 74b0994..3eef5de 100644
> --- a/commands/Makefile
> +++ b/commands/Makefile
> @@ -48,3 +48,4 @@ obj-$(CONFIG_CMD_BMP)		+= bmp.o
>  obj-$(CONFIG_USB_GADGET_DFU)	+= dfu.o
>  obj-$(CONFIG_CMD_GPIO)		+= gpio.o
>  obj-$(CONFIG_CMD_UNLZO)		+= unlzo.o
> +obj-$(CONFIG_CMD_I2C)		+= i2c.o
> diff --git a/commands/i2c.c b/commands/i2c.c
> new file mode 100644
> index 0000000..b4152eb
> --- /dev/null
> +++ b/commands/i2c.c
> @@ -0,0 +1,233 @@
> +/*
> + * i2c.c - i2c commands
> + *
> + * Copyright (c) 2010 Eric Bénard <eric@eukrea.Com>, Eukréa Electromatique
> + *
> + * originals hex1_bin and hex2_bin are
> + * (C) Copyright 2000 Wolfgang Denk, DENX Software Engineering, wd@denx.de.
> + *
> + * 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 option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <errno.h>
> +#include <malloc.h>
> +#include <getopt.h>
> +#include <i2c/i2c.h>
> +
> +struct i2c_client	*client;
> +extern struct i2c_client *i2cdev_get_client(void);
> +
> +static int hex1_bin(char c)
> +{
> +	if (c >= '0' && c <= '9')
> +		return c - '0';
> +	if (c >= 'a' && c <= 'f')
> +		return c + 10 - 'a';
> +	if (c >= 'A' && c <= 'F')
> +		return c + 10 - 'A';
> +	return -1;
> +}
> +
> +static int hex2_bin(char *s)
> +{
> +	int i, j;
> +
> +	if (*s == '0' && (*(s+1) == 'x'))
> +		s += 2;
> +	i = hex1_bin(*s++);
> +	if (i < 0)
> +		return -1;
> +	j = hex1_bin(*s);
> +	if (j < 0)
> +		return -1;
> +	return (i<<4) + j;
> +}

Any reason not to use simple_strtoul here?

> +
> +static int do_i2c_probe(struct command *cmdtp, int argc, char *argv[])
> +{
> +	struct i2c_client *client;
> +	int startaddr = -1, stopaddr = -1, addr, ret;
> +	u8 reg;
> +
> +	if (argc < 3)
> +		return COMMAND_ERROR_USAGE;
> +
> +	startaddr = hex2_bin(argv[1]);
> +	stopaddr = hex2_bin(argv[2]);
> +	if ((startaddr == -1) || (stopaddr == -1))
> +		return COMMAND_ERROR_USAGE;
> +

You should also check for stopaddr >= startaddr, otherwise you can run
into a (nearly) infinite loop below. Or better check for addr <=
stopaddr below.

> +	client = i2cdev_get_client();
> +	if (!client)
> +		return -ENODEV;
> +
> +	printf("probing i2c range 0X%02x - 0x%02x :\n", startaddr, stopaddr);
> +	for (addr = startaddr; addr != stopaddr; addr++) {
> +		client->addr = addr;
> +		ret = i2c_write_reg(client, 0x00, &reg, 0);
> +		if (ret == 0)
> +			printf("0x%02x ", addr);
> +	}
> +	printf("\n");
> +	return 0;
> +}
> +
> +static const __maybe_unused char cmd_i2c_probe_help[] =
> +"Usage: i2c_probe 0xstartaddr 0xstopaddr\n"
> +"probe a range of i2c addresses.\n";
> +
> +BAREBOX_CMD_START(i2c_probe)
> +	.cmd		= do_i2c_probe,
> +	.usage		= "probe for an i2c device",
> +	BAREBOX_CMD_HELP(cmd_i2c_probe_help)
> +BAREBOX_CMD_END
> +
> +static int do_i2c_write(struct command *cmdtp, int argc, char *argv[])
> +{
> +	struct i2c_client *client;
> +	int addr = -1, reg = -1, count = -1, ret, opt;
> +	void *buf = NULL;

No need to set this to NULL.

> +
> +	if (argc < 5)
> +		return COMMAND_ERROR_USAGE;

You don't need this check.

> +
> +	while ((opt = getopt(argc, argv, "a:c:r:")) > 0) {
> +		switch (opt) {
> +		case 'a':
> +			addr = hex2_bin(optarg);
> +			if (addr < 0)
> +				return COMMAND_ERROR_USAGE;
> +			break;
> +		case 'c':
> +			count = simple_strtoul(optarg, NULL, 0);
> +			break;
> +		case 'r':
> +			reg = hex2_bin(optarg);
> +			if (reg < 0)
> +				return COMMAND_ERROR_USAGE;
> +			break;
> +		}
> +	}
> +	if ((argc - optind) != count)
> +		return COMMAND_ERROR_USAGE;

We do not need the count command line argument. Just do a
count = argc - optind.

You should move the addr < 0 and reg < 0 checks here. This also catches
the case when the user did not specify -a or -r.

> +
> +	client = i2cdev_get_client();
> +	if (!client)
> +		return -ENODEV;
> +	client->addr = addr;
> +
> +	buf = malloc(count);
> +	if (buf) {
> +		int i;
> +		u8 tmp;
> +		for (i = 0; i < count; i++) {
> +			tmp = hex2_bin(argv[optind+i]);
> +			if (tmp < 1) {

Shouldn't this be tmp < 0?
Anyway, simple_strtoul can do a better job here. You can explicitely
specify the base to 16 to get rid of the mandatory 0x prefix.

> +				free(buf);
> +				return COMMAND_ERROR_USAGE;
> +			}
> +			*(u8 *) (buf + i) = tmp;

u8 might a better type for buf.

> +		}
> +		ret = i2c_write_reg(client, reg, buf, count);
> +		if (ret != count) {
> +			free(buf);
> +			return ret;
> +		}
> +		free(buf);
> +		printf("wrote %i bytes starting at reg 0x%02x to i2cdev \
> +			0x%02x\n", count, reg, addr);
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +static const __maybe_unused char cmd_i2c_write_help[] =
> +"Usage: i2c_write [OPTION] ... hexdatas\n"
> +"write to i2c device.\n"
> +"  -a 0x<addr>   i2c device address\n"
> +"  -r 0x<reg>    start register\n"
> +"  -c <count>  byte number to write\n";
> +
> +BAREBOX_CMD_START(i2c_write)
> +	.cmd		= do_i2c_write,
> +	.usage		= "write to an i2c device",
> +	BAREBOX_CMD_HELP(cmd_i2c_write_help)
> +BAREBOX_CMD_END
> +
> +static int do_i2c_read(struct command *cmdtp, int argc, char *argv[])
> +{

Some comments above apply to this function aswell.

> +	struct i2c_client *client;
> +	void *buf = NULL;
> +	int count = -1, addr = -1, reg = -1, ret, opt;
> +
> +	if (argc < 4)
> +		return COMMAND_ERROR_USAGE;
> +
> +	while ((opt = getopt(argc, argv, "a:c:r:")) > 0) {
> +		switch (opt) {
> +		case 'a':
> +			addr = hex2_bin(optarg);
> +			if (addr < 0)
> +				return COMMAND_ERROR_USAGE;
> +			break;
> +		case 'c':
> +			count = simple_strtoul(optarg, NULL, 0);
> +			break;
> +		case 'r':
> +			reg = hex2_bin(optarg);
> +			if (reg < 0)
> +				return COMMAND_ERROR_USAGE;
> +			break;
> +		}
> +	}
> +
> +	client = i2cdev_get_client();
> +	if (!client)
> +		return -ENODEV;
> +	client->addr = addr;
> +
> +	buf = malloc(count);
> +	if (buf) {
> +		ret = i2c_read_reg(client, reg, buf, count);
> +		if (ret == count) {
> +			int i;
> +			printf("read %i bytes starting at reg 0x%02x from \
> +				i2cdev 0x%02x\n", count, reg, addr);
> +			for (i = 0; i < count; i++)
> +				printf("0x%02x ", *(u8 *) (buf + i));
> +			printf("\n");
> +			free(buf);
> +			return 0;
> +		}
> +		free(buf);
> +	}
> +	return 1;
> +}
> +
> +static const __maybe_unused char cmd_i2c_read_help[] =
> +"Usage: i2c_read [OPTION]\n"
> +"read i2c device.\n"
> +"  -a 0x<addr>   i2c device address\n"
> +"  -r 0x<reg>    start register\n"
> +"  -c <count>  byte count\n";
> +
> +BAREBOX_CMD_START(i2c_read)
> +	.cmd		= do_i2c_read,
> +	.usage		= "read from an i2c device",
> +	BAREBOX_CMD_HELP(cmd_i2c_read_help)
> +BAREBOX_CMD_END
> -- 
> 1.6.3.3
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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