Re: [PATCH] i2c: Add support for Xilinx XPS IIC Bus Interface

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

On Mon, Sep 21, 2009 at 03:56:11PM +0200, Richard Röjfors wrote:
> This patch adds support for the Xilinx XPS IIC Bus Interface.
> 
> The driver uses the dynamic mode, supporting to put several
> I2C messages in the FIFO to reduce the number of interrupts.
> 
> It has the same feature as ocores, it can be passed a list
> of devices that will be added when the bus is probed.
> 
> Signed-off-by: Richard Röjfors <richard.rojfors-l7gf1WXxx3uGw+nKnLezzg@public.gmane.org>
> ---
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 8206442..6b291e8 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -433,6 +433,16 @@ config I2C_OCORES
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-ocores.
> 
> +config I2C_XILINX
> +	tristate "Xilinx I2C Controller"
> +	depends on EXPERIMENTAL && HAS_IOMEM
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  Xilinx I2C controller.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called xilinx_i2c.
> +
>  config I2C_OMAP
>  	tristate "OMAP I2C adapter"
>  	depends on ARCH_OMAP
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index e654263..a2ce5b8 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_I2C_IXP2000)	+= i2c-ixp2000.o
>  obj-$(CONFIG_I2C_MPC)		+= i2c-mpc.o
>  obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
>  obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
> +obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
>  obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
>  obj-$(CONFIG_I2C_PASEMI)	+= i2c-pasemi.o
>  obj-$(CONFIG_I2C_PNX)		+= i2c-pnx.o
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> new file mode 100644
> index 0000000..7b1e618
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -0,0 +1,800 @@
> +/*
> + * i2c-xiic.c
> + * Copyright (c) 2009 Intel Corporation

is this the right copyirhgt entity?

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +/* Supports:
> + * Xilinx IIC
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/wait.h>
> +#include <linux/i2c-xiic.h>
> +#include <linux/io.h>
> +
> +#define DRIVER_NAME "xiic-i2c"
> +
> +struct xiic_i2c {
> +	void __iomem *base;
> +	wait_queue_head_t wait;
> +	struct i2c_adapter adap;
> +	struct i2c_msg *tx_msg;
> +	spinlock_t	lock; /* mutual exclusion */
> +	unsigned int tx_pos;
> +	unsigned int nmsgs;
> +	int state; /* see STATE_ */
> +
> +	struct i2c_msg *rx_msg; /* current RX message */
> +	int		rx_pos;
> +};

It would be nice (but not esential) to see some documentation
on this structure.

> +#define STATE_DONE   0x00
> +#define STATE_ERROR  0x01
> +#define STATE_START  0x02

it would be great to see these as an enum (say, enum xilinx_i2c_state)
to represent the state.

> +#define xiic_irq_dis(i2c, mask)				\
> +	xiic_setreg32(i2c, XIIC_IIER_OFFSET,		\
> +	xiic_getreg32(i2c, XIIC_IIER_OFFSET) & ~(mask))

These should be 'static inline' functions, that would avoid the
whole \ continuation business.

> +static irqreturn_t xiic_isr(int irq, void *dev_id)
> +{
> +	struct xiic_i2c *i2c = dev_id;

blank sould be here.

> +static int __devinit xiic_i2c_probe(struct platform_device *pdev)
> +{
> +	struct xiic_i2c *i2c;
> +	struct xiic_i2c_platform_data *pdata;
> +	struct resource *res;
> +	int ret, irq;
> +	u8 i;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return -ENODEV;

-ENODEV is not a good error here, you won't get any report from the
driver core binding if you return this or (iirc, -EIO). Personally
I like returning -ENOENT, but this can be objectionable to others as
it is an file-system error.


> +	pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
> +	if (!pdata)
> +		return -ENODEV;

how about -EINVAL here.

> +	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
> +	if (!i2c)
> +		return -ENOMEM;
> +
> +	if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
> +		dev_err(&pdev->dev, "Memory region busy\n");
> +		ret = -EBUSY;
> +		goto request_mem_failed;
> +	}
> +
> +	i2c->base = ioremap(res->start, resource_size(res));
> +	if (!i2c->base) {
> +		dev_err(&pdev->dev, "Unable to map registers\n");
> +		ret = -EIO;
> +		goto map_failed;
> +	}
> +
> +	/* hook up driver to tree */
> +	platform_set_drvdata(pdev, i2c);
> +	i2c->adap = xiic_adapter;
> +	i2c_set_adapdata(&i2c->adap, i2c);
> +	i2c->adap.dev.parent = &pdev->dev;
> +
> +	xiic_reinit(i2c);
> +
> +	spin_lock_init(&i2c->lock);
> +	init_waitqueue_head(&i2c->wait);
> +	ret = request_irq(irq, xiic_isr, 0, pdev->name, i2c);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Cannot claim IRQ\n");
> +		goto request_irq_failed;
> +	}

Prints here but not for missing resources? at least you could
make a generic 'resource missing' error to tell users.

> +	/* add i2c adapter to i2c tree */
> +	ret = i2c_add_adapter(&i2c->adap);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to add adapter\n");
> +		goto add_adapter_failed;
> +	}
> +
> +	/* add in known devices to the bus */
> +	for (i = 0; i < pdata->num_devices; i++)
> +		i2c_new_device(&i2c->adap, pdata->devices + i);
> +
> +	return 0;
> +
> +add_adapter_failed:
> +	free_irq(irq, i2c);
> +request_irq_failed:
> +	xiic_deinit(i2c);
> +	iounmap(i2c->base);
> +map_failed:
> +	release_mem_region(res->start, resource_size(res));
> +request_mem_failed:
> +	kfree(i2c);
> +
> +	return ret;
> +}

> +++ b/include/linux/i2c-xiic.h
> @@ -0,0 +1,31 @@
> +/*
> + * i2c-xiic.h
> + * Copyright (c) 2009 Intel Corporation

is this the right copyright?

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +/* Supports:
> + * Xilinx IIC
> + */
> +
> +#ifndef _LINUX_I2C_XIIC_H
> +#define _LINUX_I2C_XIIC_H
> +
> +struct xiic_i2c_platform_data {
> +	u8 num_devices; /* number of devices in the devices list */
> +	struct i2c_board_info const *devices; /* devices connected to the bus */
> +};

kernel style documentation here people

> +#endif /* _LINUX_I2C_XIIC_H */

-- 
Ben

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