Re: [PATCH] drivers/i2c: add support for tile architecture "bus" for I2C devices

[Ben Dooks <ben-i2c-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>]

On Tue, Nov 02, 2010 at 01:05:34PM -0400, Chris Metcalf wrote:
> This change enables the Linux driver to access i2c devices via
> the Tilera hypervisor's virtualized API.
> 
> Note that the arch/tile/include/hv/ headers are "upstream" headers
> that likely benefit less from LKML review.
> 
> Signed-off-by: Chris Metcalf <cmetcalf-kv+TWInifGbQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/tile/include/hv/drv_i2cm_intf.h |   68 +++++++
>  drivers/i2c/busses/Kconfig           |   10 +
>  drivers/i2c/busses/Makefile          |    1 +
>  drivers/i2c/busses/i2c-tile.c        |  324 ++++++++++++++++++++++++++++++++++
>  4 files changed, 403 insertions(+), 0 deletions(-)
>  create mode 100644 arch/tile/include/hv/drv_i2cm_intf.h
>  create mode 100644 drivers/i2c/busses/i2c-tile.c
> 
> diff --git a/arch/tile/include/hv/drv_i2cm_intf.h b/arch/tile/include/hv/drv_i2cm_intf.h
> new file mode 100644
> index 0000000..6584a72
> --- /dev/null
> +++ b/arch/tile/include/hv/drv_i2cm_intf.h
> @@ -0,0 +1,68 @@
> +/*
> + * Copyright 2010 Tilera Corporation. All Rights Reserved.
> + *
> + *   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, version 2.
> + *
> + *   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, GOOD TITLE or
> + *   NON INFRINGEMENT.  See the GNU General Public License for
> + *   more details.
> + */
> +
> +/**
> + * @file drv_i2cm_intf.h
> + * Interface definitions for the I2CM driver.
> + *
> + * The I2C Master interface driver provides a manner for supervisors to
> + * access the I2C devices on the I2C bus.
> + *
> + * When the supervisor issues an I2C data transaction, it stores the i2c
> + * device slave address and the data offset within the device in the offset
> + * of the HV I2C device handle. The low half-word contains the slave address
> + * while the data offset is stored in byte 2 and 3. For the write access,
> + * the first 1 or 2 bytes of the write data contain the device data address
> + * if the data offset field of the HV device handle offset is 0; otherwise,
> + * the write data are puse data payload. For the read access, it is always
> + * preceded by a dummy write access which should contain an either 1-byte or
> + * 2-byte device data address while the read message holds no addressing
> + * information.
> + */
> +
> +#ifndef _SYS_HV_INCLUDE_DRV_I2CM_INTF_H
> +#define _SYS_HV_INCLUDE_DRV_I2CM_INTF_H
> +
> +/** Maximum size of an HV I2C transfer. */
> +#define HV_I2CM_CHUNK_SIZE 128
> +
> +/** Length of the i2c device name. */
> +#define I2C_DEV_NAME_SIZE   20
> +
> +/** I2C device descriptor, to be exported to the client OS. */
> +typedef struct
> +{
> +  char name[I2C_DEV_NAME_SIZE];   /**< Device name, e.g. "24c512". */
> +  uint32_t addr;                  /**< I2C device slave address */
> +}
> +tile_i2c_desc_t;

I'd rather you'd not typedef these things and just used named structures.
> +#define I2C_GET_DEV_INFO_OFF   0xF0000004
> +
> +/** This structure is used to encode the I2C device slave address
> + *  and the chip data offset and is passed to the HV in the offset
> + *  of the i2cm HV device file.
> + */
> +typedef struct
> +{
> +  uint32_t addr:8;               /**< I2C device slave address */
> +  uint32_t data_offset:24;       /**< I2C device data offset */
> +}
> +tile_i2c_addr_desc_t;

You'll be better off just having this as a uint32 and assembling it
in code, gcc isn't guaranteed to pack these as you'd think it should.

Go for something like 

ADDR_DESC(addr, data) (((data) << 8) | (addr))

> +#endif /* _SYS_HV_INCLUDE_DRV_I2CM_INTF_H */
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 3a6321c..5d8d8ab 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -607,6 +607,16 @@ config I2C_STU300
>  	  This driver can also be built as a module. If so, the module
>  	  will be called i2c-stu300.
>  
> +config I2C_TILE
> +	tristate "Tilera I2C hypervisor interface"
> +	depends on TILE
> +	help
> +	  This supports the Tilera hypervisor interface for
> +	  communicating with I2C devices attached to the chip.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called i2c-tile.
> +
>  config I2C_VERSATILE
>  	tristate "ARM Versatile/Realview I2C bus support"
>  	depends on ARCH_VERSATILE || ARCH_REALVIEW || ARCH_VEXPRESS
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 84cb16a..0a739eb 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
>  obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
>  obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
>  obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
> +obj-$(CONFIG_I2C_TILE)		+= i2c-tile.o
>  obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
>  obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
>  obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
> diff --git a/drivers/i2c/busses/i2c-tile.c b/drivers/i2c/busses/i2c-tile.c
> new file mode 100644
> index 0000000..b4e8988
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-tile.c
> @@ -0,0 +1,324 @@
> +/*
> + * Copyright 2010 Tilera Corporation. All Rights Reserved.
> + *
> + *   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, version 2.
> + *
> + *   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, GOOD TITLE or
> + *   NON INFRINGEMENT.  See the GNU General Public License for
> + *   more details.
> + *
> + * Tilera-specific I2C driver.
> + *
> + * This source code is derived from the following driver:
> + *
> + * i2c Support for Atmel's AT91 Two-Wire Interface (TWI)
> + *
> + * Copyright (C) 2004 Rick Bronson
> + * Converted to 2.6 by Andrew Victor <andrew-eS41wJS13H5l57MIdRCFDg@public.gmane.org>
> + *
> + * Borrowed heavily from original work by:
> + * Copyright (C) 2000 Philip Edelbrock <phil-LT64U7CwzWEZMC/lefXSXFaTQe2KTcn/@public.gmane.org>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/errno.h>
> +#include <linux/platform_device.h>
> +#include <linux/uaccess.h>
> +#include <hv/hypervisor.h>
> +#include <hv/drv_i2cm_intf.h>
> +
> +#define DRV_NAME	"i2c-tile"
> +
> +static char i2cm_device[16] = "i2cm/0";
> +
> +/* Handle for hypervisor device. */
> +static int i2cm_hv_devhdl;
> +
> +/*
> + * The I2C platform device.
> + */
> +static struct platform_device *i2c_platform_device;
> +
> +static int xfer_msg(struct i2c_adapter *adap, struct i2c_msg *pmsg)
> +{
> +	int retval = 0;
> +	int data_offset = 0;
> +	int addr = pmsg->addr;
> +	int length = pmsg->len;
> +	char *buf = pmsg->buf;
> +
> +	/* HV uses 8-bit slave addresses. */
> +	addr <<= 1;
> +
> +	while (length) {
> +		int hv_retval;
> +		tile_i2c_addr_desc_t hv_offset = {
> +			.addr = addr,
> +			.data_offset = data_offset,
> +		}

hmm, you could have init-ed the addr outside of the while loop
and just set the data-offset each time around the loop.

Also, see notes on just making this a single unsigned int.

could you could even ignore the offset and just increment the buffer
pointer?

> +
> +		int bytes_this_pass = length;
> +		if (bytes_this_pass > HV_I2CM_CHUNK_SIZE)
> +			bytes_this_pass = HV_I2CM_CHUNK_SIZE;
> +
> +		if (pmsg->flags & I2C_M_RD)
> +			hv_retval = hv_dev_pread(i2cm_hv_devhdl, 0,
> +					(HV_VirtAddr) buf,
> +					bytes_this_pass,
> +					*(int *) &hv_offset);

please, just create hv_offset in a uint32 and just pass it in.

> +		else
> +			hv_retval = hv_dev_pwrite(i2cm_hv_devhdl, 0,
> +					(HV_VirtAddr) buf,
> +					bytes_this_pass,
> +					*(int *) &hv_offset);

really, is HV_VirtAddr not compatible with 'void *'?

> +		if (hv_retval < 0) {
> +			pr_err(DRV_NAME ": %s failed, error %d\n",
> +				(pmsg->flags & I2C_M_RD) ? "hv_dev_pread" :
> +				"hv_dev_pwrite", hv_retval);
> +			if (hv_retval == HV_ENODEV)
> +				retval = -ENODEV;
> +			else
> +				retval = -EIO;
> +			break;

see (a) comments about retval conversion, and (b), can you really lose a
device in the middle of a transfer?

note, what happens if you run i2c-detect, do you get lots of console output?

> +		}
> +
> +		buf += hv_retval;
> +		data_offset += hv_retval;
> +		length -= hv_retval;
> +	}
> +
> +	return retval;
> +}
> +
> +/*
> + * Generic I2C master transfer routine.
> + */
> +static int tile_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *pmsg,
> +			 int num)
> +{
> +	int ret, i;
> +
> +	/* We don't support ten bit chip address. */
> +	if (pmsg->flags & I2C_M_TEN)
> +		return -EINVAL;

check this for each message.

> +	for (i = 0; i < num; i++) {
> +		if (pmsg->len && pmsg->buf) {

pmsg->len == 0 would still mean sending an address to the other end and
getting an ack from it.

> +			ret = xfer_msg(adap, pmsg);
> +			if (ret)
> +				return ret;
> +
> +			pmsg++;
> +		} else
> +			return -EINVAL;
> +	}
> +
> +	return i;
> +}

> +/*
> + * Return list of supported functionality.
> + */
> +static u32 tile_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C;
> +}
> +
> +static const struct i2c_algorithm tile_i2c_algorithm = {
> +	.master_xfer	= tile_i2c_xfer,
> +	.functionality	= tile_i2c_functionality,
> +};
> +
> +/*
> + * This routine is called to register all I2C devices that are connected to
> + * the I2C bus. This should be done at arch_initcall time, before declaring
> + * the I2C adapter. This function does the following:
> + *
> + * 1. Retrieve the I2C device list from the HV which selectively grants the
> + *    access permission of individual I2C devices, and build an array of struct
> + *    i2c_board_info.
> + * 2. Statically declare these I2C devices by calling
> + *    i2c_register_board_info().
> + */
> +static int __init tile_i2c_dev_init(void)
> +{
> +	int ret;
> +	int i;
> +	int i2c_devs = 0;
> +	struct i2c_board_info *tile_i2c_devices;
> +	int i2c_desc_size;
> +	tile_i2c_desc_t *tile_i2c_desc;

i'd like to see this with the larger items ordered towards the top of the
function, but that's just my preference.

> +	/* Open the HV i2cm device. */
> +	i2cm_hv_devhdl = hv_dev_open((HV_VirtAddr)i2cm_device, 0);

yeurk, why not either have it sa a HV_VirtAddr in the first place or
make the api take a 'char *' instead.

> +	if (i2cm_hv_devhdl < 0) {
> +		switch (i2cm_hv_devhdl) {
> +		case HV_ENODEV:
> +			return -ENODEV;

returning -ENODEV means there's no device here, not that something went
wrong when we knew a device was meant to be here. You'll lose the error
in the upper layer.

> +		default:
> +			return (ssize_t)i2cm_hv_devhdl;

not an ssize_t return.

maybe the hv should either just return a proper error or have a conversion
function.

> +		}
> +	}
> +
> +	ret = hv_dev_pread(i2cm_hv_devhdl, 0, (HV_VirtAddr)&i2c_devs,
> +			sizeof(i2c_devs), I2C_GET_NUM_DEVS_OFF);

not liking this hv_dev api at-all. do we really need to keep doing these
cats to HV_VirtAddr around here? it could end up masking problems later on.

> +	if (ret <= 0) {
> +		pr_err(DRV_NAME ": hv_dev_pread(I2C_GET_NUM_DEVS_OFF)"
> +		       " failed, error %d\n", ret);
> +		return -EIO;
> +	}
> +
> +	if (i2c_devs == 0)
> +		return 0;
> +
> +	pr_info(DRV_NAME ": detected %d I2C devices.\n", i2c_devs);
> +
> +	i2c_desc_size = i2c_devs * sizeof(tile_i2c_desc_t);
> +	tile_i2c_desc = kzalloc(i2c_desc_size, GFP_KERNEL);
> +	if (!tile_i2c_desc)
> +		return -ENOMEM;

no error print here?

> +	ret = hv_dev_pread(i2cm_hv_devhdl, 0, (HV_VirtAddr)tile_i2c_desc,
> +			i2c_desc_size, I2C_GET_DEV_INFO_OFF);
> +	if (ret <= 0) {
> +		pr_err(DRV_NAME ": hv_dev_pread(I2C_GET_DEV_INFO_OFF)"
> +		       " failed, error %d\n", ret);
> +		return -EIO;
> +	}
> +
> +	i2c_desc_size = i2c_devs * sizeof(struct i2c_board_info);
> +	tile_i2c_devices = kzalloc(i2c_desc_size, GFP_KERNEL);
> +	if (!tile_i2c_devices)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < i2c_devs; i++) {
> +		strncpy(tile_i2c_devices[i].type, tile_i2c_desc[i].name,
> +			I2C_NAME_SIZE);
> +		/* HV uses 8-bit slave addresses, convert to 7bit for Linux. */
> +		tile_i2c_devices[i].addr = tile_i2c_desc[i].addr >> 1;
> +	}
> +
> +	ret = i2c_register_board_info(0, tile_i2c_devices, i2c_devs);

are you sure you're registered as bus 0? what happens if there's >1 bus?

> +	kfree(tile_i2c_desc);
> +	kfree(tile_i2c_devices);

why not do this stuff in the platform device probe in case you're handling
multiple instances?

> +
> +	return ret;
> +}
> +arch_initcall(tile_i2c_dev_init);
> +
> +/*
> + * I2C adapter probe routine which registers the I2C adapter with the I2C core.
> + */
> +static int __devinit tile_i2c_probe(struct platform_device *dev)
> +{
> +	struct i2c_adapter *adapter;
> +	int ret;
> +
> +	adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> +	if (adapter == NULL) {
> +		ret = -ENOMEM;
> +		goto malloc_err;

no error print?

> +	}
> +
> +	adapter->owner   = THIS_MODULE;
> +
> +	/*
> +	 * If "dev->id" is negative we consider it as zero.
> +	 * The reason to do so is to avoid sysfs names that only make
> +	 * sense when there are multiple adapters.
> +	 */
> +	adapter->nr = dev->id != -1 ? dev->id : 0;
> +	snprintf(adapter->name, sizeof(adapter->name), "tile_i2c-i2c.%u",
> +		 adapter->nr);

you could just use the dev_name() of the platform device here.

> +	adapter->algo = &tile_i2c_algorithm;
> +	adapter->class = I2C_CLASS_HWMON;
> +	adapter->dev.parent = &dev->dev;
> +
> +	ret = i2c_add_numbered_adapter(adapter);
> +	if (ret < 0) {
> +		pr_info(DRV_NAME ": %s registration failed\n",
> +			adapter->name);
> +		goto add_adapter_err;

dev_err() would have been better.

> +	}
> +
> +	platform_set_drvdata(dev, adapter);
> +
> +	return 0;
> +
> +add_adapter_err:
> +	kfree(adapter);
> +malloc_err:
> +	return ret;
> +}
> +
> +/*
> + * I2C adapter cleanup routine.
> + */
> +static int __devexit tile_i2c_remove(struct platform_device *dev)
> +{
> +	struct i2c_adapter *adapter = platform_get_drvdata(dev);
> +	int rc;
> +
> +	rc = i2c_del_adapter(adapter);
> +	platform_set_drvdata(dev, NULL);
> +
> +	kfree(adapter);
> +
> +	return rc;
> +}
> +
> +static struct platform_driver tile_i2c_driver = {
> +	.driver		= {
> +		.name	= DRV_NAME,
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= tile_i2c_probe,
> +	.remove		= __devexit_p(tile_i2c_remove),
> +};
> +
> +/*
> + * Driver init routine.
> + */
> +static int __init tile_i2c_init(void)
> +{
> +	int err;
> +
> +	err = platform_driver_register(&tile_i2c_driver);
> +	if (err)
> +		return err;
> +
> +	i2c_platform_device =
> +		platform_device_register_simple(DRV_NAME, -1, NULL, 0);
> +	if (IS_ERR(i2c_platform_device)) {
> +		err = PTR_ERR(i2c_platform_device);
> +		goto unreg_platform_driver;

you probably wanted to print an error here.

ps, why keep it around if you don't then use it again? why not have the
dev as a local pointer.

> +	}
> +
> +	return 0;
> +
> +unreg_platform_driver:
> +	platform_driver_unregister(&tile_i2c_driver);
> +	return err;
> +}
> +
> +/*
> + * Driver cleanup routine.
> + */
> +static void __exit tile_i2c_exit(void)
> +{
> +	platform_driver_unregister(&tile_i2c_driver);
> +}
> +
> +module_init(tile_i2c_init);
> +module_exit(tile_i2c_exit);
> -- 
> 1.6.5.2
> 

-- 
-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.