[U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C

[Masahiro Yamada]

Hi Simon,



On Tue, 11 Nov 2014 10:46:24 -0700
Simon Glass <sjg at chromium.org> wrote:

> The uclass implements the same operations as the current I2C framework but
> makes some changes to make it fit driver model better:
> 
> - Remove the chip address from API calls
> - Remove the address length from API calls
> - Remove concept of 'current' I2C bus
> - Drop all existing init functions
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> 
> Changes in v2:
> - Fix cihp typo
> - Implement generic I2C devices to allow 'i2c probe' on unknown devices
> - Return the probed device from i2c_probe()
> - Set the bus speed after the bus is probed
> - Add some debugging for generic I2C device binding
> - Add a 'deblock' method to recover an I2C bus stuck in mid-transaction
> - Add a helper function to find a chip on a particular bus number
[snip]
> +
> +int i2c_read(struct udevice *dev, uint addr, 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);
> +
> +	if (!ops->read)
> +		return -ENOSYS;
> +
> +	return ops->read(bus, chip->chip_addr, addr, chip->addr_len, buffer,
> +			 len);
> +}
> +
> +int i2c_write(struct udevice *dev, uint addr, 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);
> +
> +	if (!ops->write)
> +		return -ENOSYS;
> +
> +	return ops->write(bus, chip->chip_addr, addr, chip->addr_len, buffer,
> +			  len);
> +}

This seems inconsistent with "struct dm_i2c_ops".
In this function, the address length(chip->addr_len) is passed to the forth argument of ops->write().

You should compare it with "struct dm_i2c_ops" in include/i2c.h
It says that the third argument is alen.



BTW, address_offset within the chip and data are treated in the same way in I2C bus.
Should we pass them separately to each driver?

I mean, can we put the offset address and data in the buffer?





> +
> +int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice **devp)
> +{
> +	struct udevice *dev;
> +
> +	debug("%s: Searching bus '%s' for address %02x: ", __func__,
> +	      bus->name, chip_addr);
> +	for (device_find_first_child(bus, &dev); dev;
> +			device_find_next_child(&dev)) {
> +		struct dm_i2c_chip store;
> +		struct dm_i2c_chip *chip = dev_get_parentdata(dev);
> +		int ret;
> +
> +		if (!chip) {
> +			chip = &store;
> +			i2c_chip_ofdata_to_platdata(gd->fdt_blob,
> +						    dev->of_offset, chip);
> +		}
> +		if (chip->chip_addr == chip_addr) {
> +			ret = device_probe(dev);
> +			debug("found, ret=%d\n", ret);
> +			if (ret)
> +				return ret;
> +			*devp = dev;
> +			return 0;
> +		}


If "chip" is "NULL", i2c_chip_ofdata_to_platdata() is called to create
struct dm_i2c_chip, but it is not thrown away soon.  It is not efficient.

If we use device_probe_child() instead of device_probe(), I think we can
re-use it.

I mean,

                if (chip->chip_addr == chip_addr) {
                        ret = device_probe_child(dev, chip);


Perhaps, we can remove sandbox_i2c_child_pre_probe().





> +	}
> +	debug("not found\n");
> +	return i2c_bind_driver(bus, chip_addr, devp);
> +}



If no chip-device is found at the specified chip_addr,
the last line calls i2c_bind_driver().  Why?

The i2c_bind_driver() tries to create a "generic" chip.
What is this "generic" chip?

Besides, i2c_bind_driver() tries to probe the created generic chip,
but it always fails in i2c_chip_ofdata_to_platdata()
because the generic chip does not have "reg" property

I could not understand at all what this code wants to do.






> +}
> +
> +int i2c_probe(struct udevice *bus, uint chip_addr, struct udevice **devp)
> +{
> +	struct dm_i2c_ops *ops = i2c_get_ops(bus);
> +	int ret;
> +
> +	*devp = NULL;
> +	if (!ops->probe)
> +		return -ENODEV;
> +
> +	/* First probe that chip */
> +	ret = ops->probe(bus, chip_addr);
> +	debug("%s: bus='%s', address %02x, ret=%d\n", __func__, bus->name,
> +	      chip_addr, ret);
> +	if (ret)
> +		return ret;


Is the "ops->probe" responsible to probe child devices?
(At least, sandbox_i2c probes its children)



> +	/* The chip was found, see if we have a driver, and probe it */
> +	ret = i2c_get_chip(bus, chip_addr, devp);
> +	debug("%s:  i2c_get_chip: ret=%d\n", __func__, ret);
> +	if (!ret || ret != -ENODEV)
> +		return ret;
> +
> +	return i2c_bind_driver(bus, chip_addr, devp);
> +}


If so, why do we need to call i2c_get_chip() and probe it again?






> +int i2c_set_bus_speed(struct udevice *bus, unsigned int speed)
> +{
> +	struct dm_i2c_ops *ops = i2c_get_ops(bus);
> +	struct dm_i2c_bus *i2c = bus->uclass_priv;
> +	int ret;
> +
> +	if (ops->set_bus_speed) {
> +		ret = ops->set_bus_speed(bus, speed);
> +		if (ret)
> +			return ret;
> +	}
> +	i2c->speed_hz = speed;
> +
> +	return 0;
> +}
> +
> +/*
> + * i2c_get_bus_speed:
> + *
> + *  Returns speed of selected I2C bus in Hz
> + */
> +int i2c_get_bus_speed(struct udevice *bus)
> +{
> +	struct dm_i2c_ops *ops = i2c_get_ops(bus);
> +	struct dm_i2c_bus *i2c = bus->uclass_priv;
> +
> +	if (!ops->set_bus_speed)
> +		return i2c->speed_hz;
> +
> +	return ops->get_bus_speed(bus);
> +}
> +
> +int i2c_set_addr_len(struct udevice *dev, uint addr_len)
> +{
> +	struct udevice *bus = dev->parent;
> +	struct dm_i2c_chip *chip = dev_get_parentdata(dev);
> +	struct dm_i2c_ops *ops = i2c_get_ops(bus);
> +	int ret;
> +
> +	if (addr_len > 3)
> +		return -EINVAL;
> +	if (ops->set_addr_len) {
> +		ret = ops->set_addr_len(dev, addr_len);
> +		if (ret)
> +			return ret;
> +	}
> +	chip->addr_len = addr_len;
> +
> +	return 0;
> +}


I am not 100% sure, but "addr_len" is a user-configurable parameter?

I think each device has its own fixed address size.












> +UCLASS_DRIVER(i2c_generic) = {
> +	.id		= UCLASS_I2C_GENERIC,
> +	.name		= "i2c_generic",
> +};
> +
> +U_BOOT_DRIVER(i2c_generic_drv) = {
> +	.name		= "i2c_generic_drv",
> +	.id		= UCLASS_I2C_GENERIC,
> +};


Could you explain how "i2c_generic" is used.



> +
> +/**
> + * 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 {
> +	/**
> +	 * read() - read from a chip
> +	 *
> +	 * @bus:	Bus to read from
> +	 * @chip_addr:	Chip address to read from
> +	 * @alen:	Length of chip address in bytes
> +	 * @offset:	Offset within chip to start reading
> +	 * @buffer:	Place to put data
> +	 * @len:	Number of bytes to read
> +	 */
> +	int (*read)(struct udevice *bus, uint chip_addr, uint alen,
> +		    uint offset, uint8_t *buffer, int len);
> +
> +	/**
> +	 * write() - write bytes to a chip
> +	 *
> +	 * @dev:	Device to write to
> +	 * @chip_addr:	Chip address to read from
> +	 * @alen:	Length of chip address in bytes
> +	 * @offset:	Offset within chip to start writing
> +	 * @buffer:	Buffer containing data to write
> +	 * @len:	Number of bytes to write
> +	 *
> +	 * @return 0 on success, -ve on failure
> +	 */
> +	int (*write)(struct udevice *bus, uint chip_addr, uint alen,
> +		     uint offset, const uint8_t *buffer, int len);


See. The third argument is address length.



Best Regards
Masahiro Yamada