From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Tue, 02 Dec 2014 07:29:54 +0100 Subject: [U-Boot] [PATCH v3 01/10] dm: i2c: Add a uclass for I2C In-Reply-To: References: <1416855444-32016-1-git-send-email-sjg@chromium.org> <1416855444-32016-2-git-send-email-sjg@chromium.org> <20141201204700.67C8.AA925319@jp.panasonic.com> Message-ID: <547D5C62.4000008@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, Am 02.12.2014 05:31, schrieb Simon Glass: > +Heiko - are you OK with the new msg-based approach? Yes, you can add my acked-by to the hole series. bye, Heiko > > > Hi Masahiro, > > On 1 December 2014 at 04:47, Masahiro Yamada wrote: >> Hi Simon, >> >> >> My review is still under way, >> but I have some comments below: >> >> >> >> >> On Mon, 24 Nov 2014 11:57:15 -0700 >> Simon Glass wrote: >> >>> +static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset, >>> + uint8_t offset_buf[], struct i2c_msg *msg) >>> +{ >>> + if (!chip->offset_len) >>> + return false; >>> + msg->addr = chip->chip_addr; >>> + msg->flags = chip->flags; >>> + msg->len = chip->offset_len; >>> + msg->buf = offset_buf; >> >> You directly copy >> from (struct dm_i2c_chip *)->flags >> to (struct i2c_msg *)->flags. >> >> But you define completely different flags for them: >> DM_I2C_CHIP_10BIT is defined as 0x1. >> I2C_M_TEN is defined as 0x10. >> >> It would not work. >> >> >> >>> + >>> +static int i2c_read_bytewise(struct udevice *dev, uint offset, >>> + const uint8_t *buffer, int len) >>> +{ >>> + struct dm_i2c_chip *chip = dev_get_parentdata(dev); >>> + struct udevice *bus = dev_get_parent(dev); >>> + struct dm_i2c_ops *ops = i2c_get_ops(bus); >>> + struct i2c_msg msg[1]; >>> + uint8_t buf[5]; >>> + int ret; >>> + int i; >>> + >>> + for (i = 0; i < len; i++) { >>> + i2c_setup_offset(chip, offset, buf, msg); >>> + msg->len++; >>> + buf[chip->offset_len] = buffer[i]; >>> + >>> + ret = ops->xfer(bus, msg, 1); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >> >> I could not understand how this works. >> It seems to send only write transactions. >> >> >> >>> + >>> +static int i2c_bind_driver(struct udevice *bus, uint chip_addr, >>> + struct udevice **devp) >>> +{ >>> + struct dm_i2c_chip *chip; >>> + char name[30], *str; >>> + struct udevice *dev; >>> + int ret; >>> + >>> + snprintf(name, sizeof(name), "generic_%x", chip_addr); >>> + str = strdup(name); >>> + ret = device_bind_driver(bus, "i2c_generic_drv", str, &dev); >>> + debug("%s: device_bind_driver: ret=%d\n", __func__, ret); >>> + if (ret) >>> + goto err_bind; >>> + >>> + /* Tell the device what we know about it */ >>> + chip = calloc(1, sizeof(struct dm_i2c_chip)); >>> + if (!chip) { >>> + ret = -ENOMEM; >>> + goto err_mem; >>> + } >>> + chip->chip_addr = chip_addr; >>> + chip->offset_len = 1; /* we assume */ >>> + ret = device_probe_child(dev, chip); >>> + debug("%s: device_probe_child: ret=%d\n", __func__, ret); >>> + free(chip); >> >> >> Why do you need calloc() & free() here? >> I think you can use the stack area for "struct dm_i2c_chip chip;" >> >> >> >> >> >> >> >> >>> + >>> +UCLASS_DRIVER(i2c) = { >>> + .id = UCLASS_I2C, >>> + .name = "i2c", >>> + .per_device_auto_alloc_size = sizeof(struct dm_i2c_bus), >>> + .post_bind = i2c_post_bind, >>> + .post_probe = i2c_post_probe, >>> +}; >>> + >>> +UCLASS_DRIVER(i2c_generic) = { >>> + .id = UCLASS_I2C_GENERIC, >>> + .name = "i2c_generic", >>> +}; >>> + >>> +U_BOOT_DRIVER(i2c_generic_drv) = { >> >> Perhaps isn't "i2c_generic_chip" clearer than "i2c_generic_drv"? >> >> >> >>> + .name = "i2c_generic_drv", >>> + .id = UCLASS_I2C_GENERIC, >>> +}; >> >> >> Can we move "i2c_generic" to a different file? >> maybe, drivers/i2c/i2c-generic.c or drivers/i2c/i2c-generic-chip.c ? >> >> UCLASS_DRIVER(i2c) is a bus, whereas UCLASS_DRIVER(i2c_generic) is a chip. >> >> Mixing up a bus and a chip-device together in the same file >> looks confusing to me. >> >> >> >> >>> >>> /* >>> + * For now there are essentially two parts to this file - driver model >>> + * here at the top, and the older code below (with CONFIG_SYS_I2C being >>> + * most recent). The plan is to migrate everything to driver model. >>> + * The driver model structures and API are separate as they are different >>> + * enough as to be incompatible for compilation purposes. >>> + */ >>> + >>> +#ifdef CONFIG_DM_I2C >>> + >>> +enum dm_i2c_chip_flags { >>> + DM_I2C_CHIP_10BIT = 1 << 0, /* Use 10-bit addressing */ >>> + DM_I2C_CHIP_RE_ADDRESS = 1 << 1, /* Send address for every byte */ >>> +}; >> >> >> As I mentioned above, you define DM_I2C_CHIP_10BIT as 0x1 >> whereas you define I2C_M_TEN as 0x0010. >> >> These flags should be shared with struct i2c_msg. >> >> >> >>> +/* >>> + * Not all of these flags are implemented in the U-Boot API >>> + */ >>> +enum dm_i2c_msg_flags { >>> + I2C_M_TEN = 0x0010, /* ten-bit chip address */ >>> + I2C_M_RD = 0x0001, /* read data, from slave to master */ >>> + I2C_M_STOP = 0x8000, /* send stop after this message */ >>> + I2C_M_NOSTART = 0x4000, /* no start before this message */ >>> + I2C_M_REV_DIR_ADDR = 0x2000, /* invert polarity of R/W bit */ >>> + I2C_M_IGNORE_NAK = 0x1000, /* continue after NAK */ >>> + I2C_M_NO_RD_ACK = 0x0800, /* skip the Ack bit on reads */ >>> + I2C_M_RECV_LEN = 0x0400, /* length is first received byte */ >>> +}; >> >> I think this enum usage is odd. >> >> If you want to allocate specific values such as 0x8000, 0x4000, etc. >> you should use #define instead of enum. >> >> If you do not care which value is assigned, you can use enum. >> arch/arm/include/asm/spl.h is a good example of usage of enum. >> >> >> >> >> >> >>> +}; >>> + >>> +/** >>> + * struct dm_i2c_ops - driver operations for I2C uclass >>> + * >>> + * Drivers should support these operations unless otherwise noted. These >>> + * operations are intended to be used by uclass code, not directly from >>> + * other code. >>> + */ >>> +struct dm_i2c_ops { >>> + /** >>> + * xfer() - transfer a list of I2C messages >>> + * >>> + * @bus: Bus to read from >>> + * @chip_addr: Chip address to read from >>> + * @offset: Offset within chip to start reading >>> + * @olen: Length of chip offset in bytes >>> + * @buffer: Place to put data >>> + * @len: Number of bytes to read >>> + * @return 0 if OK, -EREMOTEIO if the slave did not ACK a byte, >>> + * other -ve value on some other error >>> + */ >>> + int (*xfer)(struct udevice *bus, struct i2c_msg *msg, int nmsgs); >> >> >> This comment block does not reflect the actual prototype; >> chip_addr, offset, ... etc. do not exist any more. > > Thanks for these comments, I will work on another version soon. > > Regards, > Simon > -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany