[PATCH V3 1/4] drivers/i2c/busses/i2c-at91.c: remove broken driver

[PATCH V3 1/4] drivers/i2c/busses/i2c-at91.c: remove broken driver

[Felipe Balbi]

Hi,

On Tue, Nov 08, 2011 at 11:49:24AM +0100, Nikolaus Voss wrote:
> Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
> ---
>  drivers/i2c/busses/i2c-at91.c |  327 -----------------------------------------
>  1 files changed, 0 insertions(+), 327 deletions(-)
>  delete mode 100644 drivers/i2c/busses/i2c-at91.c
> 
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> deleted file mode 100644
> index 305c075..0000000
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ /dev/null
> @@ -1,327 +0,0 @@
> -/*
> -    i2c Support for Atmel's AT91 Two-Wire Interface (TWI)
> -
> -    Copyright (C) 2004 Rick Bronson
> -    Converted to 2.6 by Andrew Victor <andrew@sanpeople.com>
> -
> -    Borrowed heavily from original work by:
> -    Copyright (C) 2000 Philip Edelbrock <phil@stimpy.netroedge.com>
> -
> -    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.
> -*/
> -
> -#include <linux/module.h>
> -#include <linux/kernel.h>
> -#include <linux/err.h>
> -#include <linux/slab.h>
> -#include <linux/types.h>
> -#include <linux/delay.h>
> -#include <linux/i2c.h>
> -#include <linux/init.h>
> -#include <linux/clk.h>
> -#include <linux/platform_device.h>
> -#include <linux/io.h>
> -
> -#include <mach/at91_twi.h>
> -#include <mach/board.h>
> -#include <mach/cpu.h>
> -
> -#define TWI_CLOCK		100000		/* Hz. max 400 Kbits/sec */
> -
> -
> -static struct clk *twi_clk;
> -static void __iomem *twi_base;
> -
> -#define at91_twi_read(reg)		__raw_readl(twi_base + (reg))
> -#define at91_twi_write(reg, val)	__raw_writel((val), twi_base + (reg))
> -
> -
> -/*
> - * Initialize the TWI hardware registers.
> - */
> -static void __devinit at91_twi_hwinit(void)
> -{
> -	unsigned long cdiv, ckdiv;
> -
> -	at91_twi_write(AT91_TWI_IDR, 0xffffffff);	/* Disable all interrupts */
> -	at91_twi_write(AT91_TWI_CR, AT91_TWI_SWRST);	/* Reset peripheral */
> -	at91_twi_write(AT91_TWI_CR, AT91_TWI_MSEN);	/* Set Master mode */
> -
> -	/* Calcuate clock dividers */
> -	cdiv = (clk_get_rate(twi_clk) / (2 * TWI_CLOCK)) - 3;
> -	cdiv = cdiv + 1;	/* round up */
> -	ckdiv = 0;
> -	while (cdiv > 255) {
> -		ckdiv++;
> -		cdiv = cdiv >> 1;
> -	}
> -
> -	if (cpu_is_at91rm9200()) {			/* AT91RM9200 Errata #22 */
> -		if (ckdiv > 5) {
> -			printk(KERN_ERR "AT91 I2C: Invalid TWI_CLOCK value!\n");
> -			ckdiv = 5;
> -		}
> -	}
> -
> -	at91_twi_write(AT91_TWI_CWGR, (ckdiv << 16) | (cdiv << 8) | cdiv);
> -}
> -
> -/*
> - * Poll the i2c status register until the specified bit is set.
> - * Returns 0 if timed out (100 msec).
> - */
> -static short at91_poll_status(unsigned long bit)
> -{
> -	int loop_cntr = 10000;
> -
> -	do {
> -		udelay(10);
> -	} while (!(at91_twi_read(AT91_TWI_SR) & bit) && (--loop_cntr > 0));
> -
> -	return (loop_cntr > 0);
> -}
> -
> -static int xfer_read(struct i2c_adapter *adap, unsigned char *buf, int length)
> -{
> -	/* Send Start */
> -	at91_twi_write(AT91_TWI_CR, AT91_TWI_START);
> -
> -	/* Read data */
> -	while (length--) {
> -		if (!length)	/* need to send Stop before reading last byte */
> -			at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
> -		if (!at91_poll_status(AT91_TWI_RXRDY)) {
> -			dev_dbg(&adap->dev, "RXRDY timeout\n");
> -			return -ETIMEDOUT;
> -		}
> -		*buf++ = (at91_twi_read(AT91_TWI_RHR) & 0xff);
> -	}
> -
> -	return 0;
> -}
> -
> -static int xfer_write(struct i2c_adapter *adap, unsigned char *buf, int length)
> -{
> -	/* Load first byte into transmitter */
> -	at91_twi_write(AT91_TWI_THR, *buf++);
> -
> -	/* Send Start */
> -	at91_twi_write(AT91_TWI_CR, AT91_TWI_START);
> -
> -	do {
> -		if (!at91_poll_status(AT91_TWI_TXRDY)) {
> -			dev_dbg(&adap->dev, "TXRDY timeout\n");
> -			return -ETIMEDOUT;
> -		}
> -
> -		length--;	/* byte was transmitted */
> -
> -		if (length > 0)		/* more data to send? */
> -			at91_twi_write(AT91_TWI_THR, *buf++);
> -	} while (length);
> -
> -	/* Send Stop */
> -	at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
> -
> -	return 0;
> -}
> -
> -/*
> - * Generic i2c master transfer entrypoint.
> - *
> - * Note: We do not use Atmel's feature of storing the "internal device address".
> - * Instead the "internal device address" has to be written using a separate
> - * i2c message.
> - * http://lists.arm.linux.org.uk/pipermail/linux-arm-kernel/2004-September/024411.html
> - */
> -static int at91_xfer(struct i2c_adapter *adap, struct i2c_msg *pmsg, int num)
> -{
> -	int i, ret;
> -
> -	dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
> -
> -	for (i = 0; i < num; i++) {
> -		dev_dbg(&adap->dev, " #%d: %sing %d byte%s %s 0x%02x\n", i,
> -			pmsg->flags & I2C_M_RD ? "read" : "writ",
> -			pmsg->len, pmsg->len > 1 ? "s" : "",
> -			pmsg->flags & I2C_M_RD ? "from" : "to",	pmsg->addr);
> -
> -		at91_twi_write(AT91_TWI_MMR, (pmsg->addr << 16)
> -			| ((pmsg->flags & I2C_M_RD) ? AT91_TWI_MREAD : 0));
> -
> -		if (pmsg->len && pmsg->buf) {	/* sanity check */
> -			if (pmsg->flags & I2C_M_RD)
> -				ret = xfer_read(adap, pmsg->buf, pmsg->len);
> -			else
> -				ret = xfer_write(adap, pmsg->buf, pmsg->len);
> -
> -			if (ret)
> -				return ret;
> -
> -			/* Wait until transfer is finished */
> -			if (!at91_poll_status(AT91_TWI_TXCOMP)) {
> -				dev_dbg(&adap->dev, "TXCOMP timeout\n");
> -				return -ETIMEDOUT;
> -			}
> -		}
> -		dev_dbg(&adap->dev, "transfer complete\n");
> -		pmsg++;		/* next message */
> -	}
> -	return i;
> -}
> -
> -/*
> - * Return list of supported functionality.
> - */
> -static u32 at91_func(struct i2c_adapter *adapter)
> -{
> -	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> -}
> -
> -static struct i2c_algorithm at91_algorithm = {
> -	.master_xfer	= at91_xfer,
> -	.functionality	= at91_func,
> -};
> -
> -/*
> - * Main initialization routine.
> - */
> -static int __devinit at91_i2c_probe(struct platform_device *pdev)
> -{
> -	struct i2c_adapter *adapter;
> -	struct resource *res;
> -	int rc;
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res)
> -		return -ENXIO;
> -
> -	if (!request_mem_region(res->start, resource_size(res), "at91_i2c"))
> -		return -EBUSY;
> -
> -	twi_base = ioremap(res->start, resource_size(res));
> -	if (!twi_base) {
> -		rc = -ENOMEM;
> -		goto fail0;
> -	}
> -
> -	twi_clk = clk_get(NULL, "twi_clk");
> -	if (IS_ERR(twi_clk)) {
> -		dev_err(&pdev->dev, "no clock defined\n");
> -		rc = -ENODEV;
> -		goto fail1;
> -	}
> -
> -	adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> -	if (adapter == NULL) {
> -		dev_err(&pdev->dev, "can't allocate inteface!\n");
> -		rc = -ENOMEM;
> -		goto fail2;
> -	}
> -	snprintf(adapter->name, sizeof(adapter->name), "AT91");
> -	adapter->algo = &at91_algorithm;
> -	adapter->class = I2C_CLASS_HWMON;
> -	adapter->dev.parent = &pdev->dev;
> -	/* adapter->id == 0 ... only one TWI controller for now */
> -
> -	platform_set_drvdata(pdev, adapter);
> -
> -	clk_enable(twi_clk);		/* enable peripheral clock */
> -	at91_twi_hwinit();		/* initialize TWI controller */
> -
> -	rc = i2c_add_numbered_adapter(adapter);
> -	if (rc) {
> -		dev_err(&pdev->dev, "Adapter %s registration failed\n",
> -				adapter->name);
> -		goto fail3;
> -	}
> -
> -	dev_info(&pdev->dev, "AT91 i2c bus driver.\n");
> -	return 0;
> -
> -fail3:
> -	platform_set_drvdata(pdev, NULL);
> -	kfree(adapter);
> -	clk_disable(twi_clk);
> -fail2:
> -	clk_put(twi_clk);
> -fail1:
> -	iounmap(twi_base);
> -fail0:
> -	release_mem_region(res->start, resource_size(res));
> -
> -	return rc;
> -}
> -
> -static int __devexit at91_i2c_remove(struct platform_device *pdev)
> -{
> -	struct i2c_adapter *adapter = platform_get_drvdata(pdev);
> -	struct resource *res;
> -	int rc;
> -
> -	rc = i2c_del_adapter(adapter);
> -	platform_set_drvdata(pdev, NULL);
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	iounmap(twi_base);
> -	release_mem_region(res->start, resource_size(res));
> -
> -	clk_disable(twi_clk);		/* disable peripheral clock */
> -	clk_put(twi_clk);
> -
> -	return rc;
> -}
> -
> -#ifdef CONFIG_PM
> -
> -/* NOTE: could save a few mA by keeping clock off outside of at91_xfer... */
> -
> -static int at91_i2c_suspend(struct platform_device *pdev, pm_message_t mesg)
> -{
> -	clk_disable(twi_clk);
> -	return 0;
> -}
> -
> -static int at91_i2c_resume(struct platform_device *pdev)
> -{
> -	return clk_enable(twi_clk);
> -}
> -
> -#else
> -#define at91_i2c_suspend	NULL
> -#define at91_i2c_resume		NULL
> -#endif
> -
> -/* work with "modprobe at91_i2c" from hotplugging or coldplugging */
> -MODULE_ALIAS("platform:at91_i2c");
> -
> -static struct platform_driver at91_i2c_driver = {
> -	.probe		= at91_i2c_probe,
> -	.remove		= __devexit_p(at91_i2c_remove),
> -	.suspend	= at91_i2c_suspend,
> -	.resume		= at91_i2c_resume,
> -	.driver		= {
> -		.name	= "at91_i2c",
> -		.owner	= THIS_MODULE,
> -	},
> -};
> -
> -static int __init at91_i2c_init(void)
> -{
> -	return platform_driver_register(&at91_i2c_driver);
> -}
> -
> -static void __exit at91_i2c_exit(void)
> -{
> -	platform_driver_unregister(&at91_i2c_driver);
> -}
> -
> -module_init(at91_i2c_init);
> -module_exit(at91_i2c_exit);
> -
> -MODULE_AUTHOR("Rick Bronson");
> -MODULE_DESCRIPTION("I2C (TWI) driver for Atmel AT91");
> -MODULE_LICENSE("GPL");

after this patch you could be breaking tree compilation.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111108/bf04fd96/attachment-0001.sig>

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Felipe Balbi]

Hi,

On Tue, Nov 08, 2011 at 11:49:46AM +0100, Nikolaus Voss wrote:
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> new file mode 100644
> index 0000000..e35479b
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -0,0 +1,442 @@
> +/*
> +
> +    i2c Support for Atmel's AT91 Two-Wire Interface (TWI)
> +
> +    Copyright (C) 2011 Nikolaus Voss <n.voss@weinmann.de>
> +
> +    Evolved from original work by:
> +    Copyright (C) 2004 Rick Bronson
> +    Converted to 2.6 by Andrew Victor <andrew@sanpeople.com>
> +
> +    Borrowed heavily from original work by:
> +    Copyright (C) 2000 Philip Edelbrock <phil@stimpy.netroedge.com>
> +
> +    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.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +
> +#include <mach/at91_twi.h>
> +#include <mach/board.h>
> +#include <mach/cpu.h>

avoid including <mach/*> on drivers.

> +static void at91_set_twi_clock(struct at91_twi_dev *dev)
> +{
> +	unsigned long cdiv, ckdiv;
> +
> +	/* Calcuate clock dividers */
> +	cdiv = (clk_get_rate(dev->clk) / (2 * TWI_CLOCK)) - 3;
> +	cdiv = cdiv + 1;	/* round up */
> +	ckdiv = 0;
> +	while (cdiv > 255) {
> +		ckdiv++;
> +		cdiv = cdiv >> 1;
> +	}
> +
> +	if (cpu_is_at91rm9200()) {			/* AT91RM9200 Errata #22 */

I don't think you should be using cpu_is_* on drivers.

> +static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> +{
> +	struct at91_twi_dev *dev = dev_id;
> +	const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
> +	const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> +
> +	if (irqstatus & AT91_TWI_TXCOMP) {
> +		at91_disable_twi_interrupts(dev);
> +		dev->transfer_status = status;
> +		complete(&dev->cmd_complete);
> +	}
> +	else if (irqstatus & AT91_TWI_RXRDY) {
> +		at91_twi_read_next_byte(dev);
> +	}
> +	else if (irqstatus & AT91_TWI_TXRDY) {
> +		at91_twi_write_next_byte(dev);
> +	}
> +	else {
> +		return IRQ_NONE;

coding style is wrong. Also, are those IRQ events really mutually
exclusive ??

> +static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
> +{
> +	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +	int ret;
> +	unsigned int_addr_flag = 0;
> +	struct i2c_msg *m_start = msg;
> +
> +	dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
> +
> +	/* the hardware can handle at most two messages concatenated by a
> +	 * repeated start via it's internal address feature.
> +	 */

wrong comment style.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111108/2bfcef61/attachment.sig>

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Nicolas Ferre]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/08/2011 03:41 PM, Felipe Balbi :

>> +	if (cpu_is_at91rm9200()) {			/* AT91RM9200 Errata #22 */
> 
> I don't think you should be using cpu_is_* on drivers.

It is a common pattern in at91 drivers and has worked for ages.
Do you think it is related to the need to be able to compile the
driver for any SoC in the case of multi-SoC zImage support?

Best regards,
- -- 
Nicolas Ferre
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJOuUd1AAoJEAf03oE53VmQs9UH/i+pKZlIN7lNb+sHvqHJhiUK
zqvPObtSo2Y78dODM4Qf/WrJP/jBW4FUAx60kBlEBzWAD2aef1D078POAOVhcVdH
Gj76Z+O5tF9H9YPcn/9HGyA42kL3NZu43ibywsvbUmX1O2LbtmX49bGOjArYSyXQ
sN4F8+QJnkMQfmMNjYgG0WfhMyWnp15W4QWq+frk2Kq0nXjiuozqr9goq8/LVRZc
aSToJUUJqsfk/bEGBAF6RmNxRIu2AMNpKQZS9a1gOZA/mAfSZrT9zQE6EZVXJDVr
VVck4KlhHBR3JU6AEh0CtoRgfZxsZyzS2RNUDyF7dq+8VsNiNpajSL8S2ILanZc=
=iqlm
-----END PGP SIGNATURE-----

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Felipe Balbi]

On Tue, Nov 08, 2011 at 04:15:10PM +0100, Nicolas Ferre wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 11/08/2011 03:41 PM, Felipe Balbi :
> 
> >> +	if (cpu_is_at91rm9200()) {			/* AT91RM9200 Errata #22 */
> > 
> > I don't think you should be using cpu_is_* on drivers.
> 
> It is a common pattern in at91 drivers and has worked for ages.
> Do you think it is related to the need to be able to compile the
> driver for any SoC in the case of multi-SoC zImage support?

we have drivers compiling on multiple OMAP versions without those hacks.
Generally, we check the IP revision for that. Don't you have a Revision
register of some sort ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111108/2d7f082a/attachment.sig>

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Voss, Nikolaus]

Hi,

> > +#include <mach/at91_twi.h>
> > +#include <mach/board.h>
> > +#include <mach/cpu.h>
> 
> avoid including <mach/*> on drivers.

Should I move at91_twi.h to include/linux (omap does it like this,
other use the mach-include)?


> > +	if (irqstatus & AT91_TWI_TXCOMP) {
> > +		at91_disable_twi_interrupts(dev);
> > +		dev->transfer_status = status;
> > +		complete(&dev->cmd_complete);
> > +	}
> > +	else if (irqstatus & AT91_TWI_RXRDY) {
> > +		at91_twi_read_next_byte(dev);
> > +	}
> > +	else if (irqstatus & AT91_TWI_TXRDY) {
> > +		at91_twi_write_next_byte(dev);
> > +	}
> > +	else {
> > +		return IRQ_NONE;
> 
> coding style is wrong. Also, are those IRQ events really mutually exclusive ??

These are indeed mutually exclusive (semantically).

Niko

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Felipe Balbi]

On Tue, Nov 08, 2011 at 04:35:21PM +0100, Voss, Nikolaus wrote:
> Hi,
> 
> > > +#include <mach/at91_twi.h>
> > > +#include <mach/board.h>
> > > +#include <mach/cpu.h>
> > 
> > avoid including <mach/*> on drivers.
> 
> Should I move at91_twi.h to include/linux (omap does it like this,
> other use the mach-include)?

maybe, is at91_twi.h some sort of platform_data ? there's
<linux/platform_data/...> for that.

> > > +	if (irqstatus & AT91_TWI_TXCOMP) {
> > > +		at91_disable_twi_interrupts(dev);
> > > +		dev->transfer_status = status;
> > > +		complete(&dev->cmd_complete);
> > > +	}
> > > +	else if (irqstatus & AT91_TWI_RXRDY) {
> > > +		at91_twi_read_next_byte(dev);
> > > +	}
> > > +	else if (irqstatus & AT91_TWI_TXRDY) {
> > > +		at91_twi_write_next_byte(dev);
> > > +	}
> > > +	else {
> > > +		return IRQ_NONE;
> > 
> > coding style is wrong. Also, are those IRQ events really mutually exclusive ??
> 
> These are indeed mutually exclusive (semantically).

so you couldn't have AT91_TWI_TXCOMP and AT91_TWI_RXRDY set when you
read irqstatus ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111108/620b9204/attachment.sig>

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Voss, Nikolaus]

> > > > +#include <mach/at91_twi.h>
> > > > +#include <mach/board.h>
> > > > +#include <mach/cpu.h>
> > >
> > > avoid including <mach/*> on drivers.
> >
> > Should I move at91_twi.h to include/linux (omap does it like this,
> > other use the mach-include)?
> 
> maybe, is at91_twi.h some sort of platform_data ? there's
> <linux/platform_data/...> for that.

It contains hardware register definitions, not really platform data.
So linux/i2c-at91.h (like linux/i2c-{omap,pxe,...}) would be the right place?


> 
> > > > +	if (irqstatus & AT91_TWI_TXCOMP) {
> > > > +		at91_disable_twi_interrupts(dev);
> > > > +		dev->transfer_status = status;
> > > > +		complete(&dev->cmd_complete);
> > > > +	}
> > > > +	else if (irqstatus & AT91_TWI_RXRDY) {
> > > > +		at91_twi_read_next_byte(dev);
> > > > +	}
> > > > +	else if (irqstatus & AT91_TWI_TXRDY) {
> > > > +		at91_twi_write_next_byte(dev);
> > > > +	}
> > > > +	else {
> > > > +		return IRQ_NONE;
> > >
> > > coding style is wrong. Also, are those IRQ events really mutually
> exclusive ??
> >
> > These are indeed mutually exclusive (semantically).
> 
> so you couldn't have AT91_TWI_TXCOMP and AT91_TWI_RXRDY set when you read
> irqstatus ?

Yes, I do have this, but in this constellation only TXCOMP is relevant and
all other flags can be ignored (because the transfer is finished).

Niko

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Felipe Balbi]

Hi,

On Tue, Nov 08, 2011 at 04:49:07PM +0100, Voss, Nikolaus wrote:
> > > > > +#include <mach/at91_twi.h>
> > > > > +#include <mach/board.h>
> > > > > +#include <mach/cpu.h>
> > > >
> > > > avoid including <mach/*> on drivers.
> > >
> > > Should I move at91_twi.h to include/linux (omap does it like this,
> > > other use the mach-include)?
> > 
> > maybe, is at91_twi.h some sort of platform_data ? there's
> > <linux/platform_data/...> for that.
> 
> It contains hardware register definitions, not really platform data.
> So linux/i2c-at91.h (like linux/i2c-{omap,pxe,...}) would be the right place?

if it's only register definitions, does it need to be in a header ? I
mean, is anyone outside of this driver trying to access those registers?
Otherwise they could sit on the C source file itself.

If there's anyone else which needs those register definitions then
<linux/i2c/at91.h> seems like a good place (??)

> > > > > +	if (irqstatus & AT91_TWI_TXCOMP) {
> > > > > +		at91_disable_twi_interrupts(dev);
> > > > > +		dev->transfer_status = status;
> > > > > +		complete(&dev->cmd_complete);
> > > > > +	}
> > > > > +	else if (irqstatus & AT91_TWI_RXRDY) {
> > > > > +		at91_twi_read_next_byte(dev);
> > > > > +	}
> > > > > +	else if (irqstatus & AT91_TWI_TXRDY) {
> > > > > +		at91_twi_write_next_byte(dev);
> > > > > +	}
> > > > > +	else {
> > > > > +		return IRQ_NONE;
> > > >
> > > > coding style is wrong. Also, are those IRQ events really mutually
> > exclusive ??
> > >
> > > These are indeed mutually exclusive (semantically).
> > 
> > so you couldn't have AT91_TWI_TXCOMP and AT91_TWI_RXRDY set when you read
> > irqstatus ?
> 
> Yes, I do have this, but in this constellation only TXCOMP is relevant and
> all other flags can be ignored (because the transfer is finished).

I asked about different directions exactly because of that. My question
was if you could have a TX Complete and RX Ready IRQs simultaneously.

The way you coded this IRQ handler is like a priority encoder, meaning
that you will ignore all other bits once you find the first set bit. I'm
wondering if you shouldn't drop the "else" as most of the IRQ handlers
do.

But it's your driver anyway, I don't know how this controller behaves.
Just found it a bit worrying that you ignore all other IRQ status bits.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111108/49980e10/attachment.sig>

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Russell King - ARM Linux]

On Tue, Nov 08, 2011 at 05:23:46PM +0200, Felipe Balbi wrote:
> On Tue, Nov 08, 2011 at 04:15:10PM +0100, Nicolas Ferre wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > On 11/08/2011 03:41 PM, Felipe Balbi :
> > 
> > >> +	if (cpu_is_at91rm9200()) {			/* AT91RM9200 Errata #22 */
> > > 
> > > I don't think you should be using cpu_is_* on drivers.
> > 
> > It is a common pattern in at91 drivers and has worked for ages.
> > Do you think it is related to the need to be able to compile the
> > driver for any SoC in the case of multi-SoC zImage support?
> 
> we have drivers compiling on multiple OMAP versions without those hacks.
> Generally, we check the IP revision for that. Don't you have a Revision
> register of some sort ?

I'm not sure what your objection is - this is no different from what
happens with OMAP when detecting OMAP1,2,3,4 etc.  E.g.:

drivers/mfd/twl-core.c:     if (cpu_is_omap2430())
drivers/media/video/omap3isp/isp.c: if (cpu_is_omap3630())
drivers/media/video/omap/omap_vout.c:               if (cpu_is_omap24xx() || cpu_is_omap34xx())
drivers/media/video/omap/omap_voutlib.c:    if (cpu_is_omap24xx()) {
drivers/media/video/omap/omap_voutlib.c:    } else if (cpu_is_omap34xx()) {
drivers/media/video/omap/omap_voutlib.c:    if (cpu_is_omap24xx()) {

for a small selection.

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Felipe Balbi]

hi,

On Tue, Nov 08, 2011 at 06:29:55PM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2011 at 05:23:46PM +0200, Felipe Balbi wrote:
> > On Tue, Nov 08, 2011 at 04:15:10PM +0100, Nicolas Ferre wrote:
> > > -----BEGIN PGP SIGNED MESSAGE-----
> > > Hash: SHA1
> > > 
> > > On 11/08/2011 03:41 PM, Felipe Balbi :
> > > 
> > > >> +	if (cpu_is_at91rm9200()) {			/* AT91RM9200 Errata #22 */
> > > > 
> > > > I don't think you should be using cpu_is_* on drivers.
> > > 
> > > It is a common pattern in at91 drivers and has worked for ages.
> > > Do you think it is related to the need to be able to compile the
> > > driver for any SoC in the case of multi-SoC zImage support?
> > 
> > we have drivers compiling on multiple OMAP versions without those hacks.
> > Generally, we check the IP revision for that. Don't you have a Revision
> > register of some sort ?
> 
> I'm not sure what your objection is - this is no different from what
> happens with OMAP when detecting OMAP1,2,3,4 etc.  E.g.:

so ? Instead of saying this to me, you should contact the
authors/maintainers of those drivers and ask them to clean that up.

Moreover, just because there are a bunch of drivers with such silly
mistakes, is it really enough argument to perpetuate the problem ?

You were (actually still is) one of the biggest source of complaints for
drivers using <mach/*> and <asm/*> includes and now you're changing your
mind ?

Also, instead of taking bad examples take the ones which are activelly
improving with regards to those details. I have been fighting against
those on the USB drivers, at least. And, except for
drivers/usb/musb/davinci.c you won't find those on MUSB driver or its
glue layers. Similarly, I have been asking all USB Peripheral Controller
drivers to make their driver buildable on all arches, which will get rid
of bad usage of <mach/*> and <asm/*> headers while also allowing for a
better coverage of compile tests on linux-next.

Now, if you really think those aren't problematic, refrain from
complaining next time someone adds a machine_is_* or cpu_is_* check on a
driver.

> drivers/mfd/twl-core.c:     if (cpu_is_omap2430())
> drivers/media/video/omap3isp/isp.c: if (cpu_is_omap3630())
> drivers/media/video/omap/omap_vout.c:               if (cpu_is_omap24xx() || cpu_is_omap34xx())
> drivers/media/video/omap/omap_voutlib.c:    if (cpu_is_omap24xx()) {
> drivers/media/video/omap/omap_voutlib.c:    } else if (cpu_is_omap34xx()) {
> drivers/media/video/omap/omap_voutlib.c:    if (cpu_is_omap24xx()) {

at least vout and voutlib haven't been touched since january.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111108/9676d69d/attachment.sig>

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Russell King - ARM Linux]

On Tue, Nov 08, 2011 at 08:44:48PM +0200, Felipe Balbi wrote:
> hi,
> 
> On Tue, Nov 08, 2011 at 06:29:55PM +0000, Russell King - ARM Linux wrote:
> > On Tue, Nov 08, 2011 at 05:23:46PM +0200, Felipe Balbi wrote:
> > > On Tue, Nov 08, 2011 at 04:15:10PM +0100, Nicolas Ferre wrote:
> > > > -----BEGIN PGP SIGNED MESSAGE-----
> > > > Hash: SHA1
> > > > 
> > > > On 11/08/2011 03:41 PM, Felipe Balbi :
> > > > 
> > > > >> +	if (cpu_is_at91rm9200()) {			/* AT91RM9200 Errata #22 */
> > > > > 
> > > > > I don't think you should be using cpu_is_* on drivers.
> > > > 
> > > > It is a common pattern in at91 drivers and has worked for ages.
> > > > Do you think it is related to the need to be able to compile the
> > > > driver for any SoC in the case of multi-SoC zImage support?
> > > 
> > > we have drivers compiling on multiple OMAP versions without those hacks.
> > > Generally, we check the IP revision for that. Don't you have a Revision
> > > register of some sort ?
> > 
> > I'm not sure what your objection is - this is no different from what
> > happens with OMAP when detecting OMAP1,2,3,4 etc.  E.g.:
> 
> so ? Instead of saying this to me, you should contact the
> authors/maintainers of those drivers and ask them to clean that up.

Oh for god sake, I was just asking you to clarify your statement in
light of what is currently being done.

Now, let me set something straight.  I've been saying that machine_is_xxx()
should not be used in drivers.  That's a platform thing and platform
specifics should not be in drivers - it should be passed in via DT or
platform data.  That's enforced by the way DT works (Grant's decision
not mine) - with DT you don't have any kind of testable machine ID for
machine_is_xxx() to use.

I've never said that cpu_is_xxx() should not - that's something *other*
people are saying (and quite rightly so) because if we're going to start
sharing drivers between different SoCs (or even architectures - eg, PXA
IP appearing on x86) then it doesn't make sense for the type of SoC to
be tested.  It makes more sense for the revision of the IP implementation
to be checked IFF such information is available.  If not, some other way
of controlling the 'features' needs to be sought.

As far as the use of asm/*.h includes, I've NEVER made any statement
about the use of those in drivers.  In fact, I don't see any reason to
avoid them _provided_ they're standard cross-arch includes.

As for mach/*.h includes, I don't think that I've made any statement
about those either, but at this point - given that we're working towards
a single zImage on ARM - it is _sensible_ to avoid such includes in
drivers.

So, I think your reaction to my statement is way off mark, and you're
attributing statements (that it seems you personally don't agree with)
to me.

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Felipe Balbi]

Hi,

On Tue, Nov 08, 2011 at 06:55:25PM +0000, Russell King - ARM Linux wrote:
> > so ? Instead of saying this to me, you should contact the
> > authors/maintainers of those drivers and ask them to clean that up.
> 
> Oh for god sake, I was just asking you to clarify your statement in
> light of what is currently being done.
> 
> Now, let me set something straight.  I've been saying that machine_is_xxx()
> should not be used in drivers.  That's a platform thing and platform
> specifics should not be in drivers - it should be passed in via DT or
> platform data.  That's enforced by the way DT works (Grant's decision
> not mine) - with DT you don't have any kind of testable machine ID for
> machine_is_xxx() to use.
> 
> I've never said that cpu_is_xxx() should not - that's something *other*
> people are saying (and quite rightly so) because if we're going to start
> sharing drivers between different SoCs (or even architectures - eg, PXA
> IP appearing on x86) then it doesn't make sense for the type of SoC to
> be tested.  It makes more sense for the revision of the IP implementation
> to be checked IFF such information is available.  If not, some other way
> of controlling the 'features' needs to be sought.
> 
> As far as the use of asm/*.h includes, I've NEVER made any statement
> about the use of those in drivers.  In fact, I don't see any reason to
> avoid them _provided_ they're standard cross-arch includes.
> 
> As for mach/*.h includes, I don't think that I've made any statement
> about those either, but at this point - given that we're working towards
> a single zImage on ARM - it is _sensible_ to avoid such includes in
> drivers.
> 
> So, I think your reaction to my statement is way off mark, and you're
> attributing statements (that it seems you personally don't agree with)
> to me.

If I did, then it's really my fault. But I _do_ remember you complaining
about uses of <asm/gpio.h> instead of <linux/gpio.h>, for example.

Now, all the other topics I agree and, in fact, have been pushing for
that as I can. Specially with regards to IP cores being shared among
several architectures (see drivers/usb/dwc3 where I have a core driver
shared between ARM and PCI/x86).

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111108/dec12d42/attachment.sig>

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Russell King - ARM Linux]

On Tue, Nov 08, 2011 at 09:02:02PM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Nov 08, 2011 at 06:55:25PM +0000, Russell King - ARM Linux wrote:
> > > so ? Instead of saying this to me, you should contact the
> > > authors/maintainers of those drivers and ask them to clean that up.
> > 
> > Oh for god sake, I was just asking you to clarify your statement in
> > light of what is currently being done.
> > 
> > Now, let me set something straight.  I've been saying that machine_is_xxx()
> > should not be used in drivers.  That's a platform thing and platform
> > specifics should not be in drivers - it should be passed in via DT or
> > platform data.  That's enforced by the way DT works (Grant's decision
> > not mine) - with DT you don't have any kind of testable machine ID for
> > machine_is_xxx() to use.
> > 
> > I've never said that cpu_is_xxx() should not - that's something *other*
> > people are saying (and quite rightly so) because if we're going to start
> > sharing drivers between different SoCs (or even architectures - eg, PXA
> > IP appearing on x86) then it doesn't make sense for the type of SoC to
> > be tested.  It makes more sense for the revision of the IP implementation
> > to be checked IFF such information is available.  If not, some other way
> > of controlling the 'features' needs to be sought.
> > 
> > As far as the use of asm/*.h includes, I've NEVER made any statement
> > about the use of those in drivers.  In fact, I don't see any reason to
> > avoid them _provided_ they're standard cross-arch includes.
> > 
> > As for mach/*.h includes, I don't think that I've made any statement
> > about those either, but at this point - given that we're working towards
> > a single zImage on ARM - it is _sensible_ to avoid such includes in
> > drivers.
> > 
> > So, I think your reaction to my statement is way off mark, and you're
> > attributing statements (that it seems you personally don't agree with)
> > to me.
> 
> If I did, then it's really my fault. But I _do_ remember you complaining
> about uses of <asm/gpio.h> instead of <linux/gpio.h>, for example.

Yes, I do complain about those with _good_ reason:
1. it's a strict checkpatch thing.
2. it's a correctness thing which matters as linux/gpio.h is the top-level
   include for gpio stuff, and at some point may have stuff pulled up
   from asm/gpio.h into it (or new stuff added to it.)

Same reasoning for linux/io.h - things get moved out of asm/io.h into
linux/io.h.  One such example is check_signature() which used to be
declared at the asm/io.h level but is now at the linux/io.h level.

And the overriding reason: if I can get them fixed _now_ they won't have
to be fixed in some massive patch in the future when we've got a few
thousand of them to deal with and tonnes of breakage because something
cross-arch changed there.

That point has been *well* proven by my recent clean ups to the mach/gpio.h
includes - which necessitated changing tonnes of mach/gpio.h includes
across the kernel tree.  Had people paid more attention to getting these
includes right, that cleanup would have been much easier.

But, that's something very different from your statement in your previous
message about my alleged stance on *all* asm/*.h includes in drivers,
which is FALSE.

> Now, all the other topics I agree and, in fact, have been pushing for
> that as I can. Specially with regards to IP cores being shared among
> several architectures (see drivers/usb/dwc3 where I have a core driver
> shared between ARM and PCI/x86).

Good, so you've just taken back most of what you said in your previous
message.

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Felipe Balbi]

Hi,

On Tue, Nov 08, 2011 at 07:39:30PM +0000, Russell King - ARM Linux wrote:
> But, that's something very different from your statement in your previous
> message about my alleged stance on *all* asm/*.h includes in drivers,
> which is FALSE.

http://marc.info/?l=linux-arm-kernel&m=132077795410718&w=2

I don't see me complaining about *all* headers anywhere in that message.
I did generalize, but I didn't stated you were complaining about *all*
headers. Go back and read it for yourself.

> > Now, all the other topics I agree and, in fact, have been pushing for
> > that as I can. Specially with regards to IP cores being shared among
> > several architectures (see drivers/usb/dwc3 where I have a core driver
> > shared between ARM and PCI/x86).
> 
> Good, so you've just taken back most of what you said in your previous
> message.

of course not... maybe you misunderstood me. My whole point was to avoid
using cpu_is_* exactly because it would prevent this driver from
compiling anywhere outside of ARM builds.

here's where you chiped in:

http://marc.info/?l=linux-kernel&m=132077706910296&w=2

And a small quote:

| On Tue, Nov 08, 2011 at 05:23:46PM +0200, Felipe Balbi wrote:
| > On Tue, Nov 08, 2011 at 04:15:10PM +0100, Nicolas Ferre wrote:
| > > -----BEGIN PGP SIGNED MESSAGE-----
| > > Hash: SHA1
| > > 
| > > On 11/08/2011 03:41 PM, Felipe Balbi :
| > > 
| > > >> +	if (cpu_is_at91rm9200()) {			/*  AT91RM9200 Errata #22 */
| > > > 
| > > > I don't think you should be using cpu_is_* on drivers.
| > > 
| > > It is a common pattern in at91 drivers and has worked for ages.
| > > Do you think it is related to the need to be able to compile the
| > > driver for any SoC in the case of multi-SoC zImage support?
| > 
| > we have drivers compiling on multiple OMAP versions without those
| > hacks.
| > Generally, we check the IP revision for that. Don't you have a
| > Revision register of some sort ?

You see ? I was asking $author to try and use some revision register in
order to apply erratas instead of using cpu_is_at91rm9200().

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111108/b4dc04e0/attachment.sig>

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Russell King - ARM Linux]

On Tue, Nov 08, 2011 at 09:58:55PM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Nov 08, 2011 at 07:39:30PM +0000, Russell King - ARM Linux wrote:
> > But, that's something very different from your statement in your previous
> > message about my alleged stance on *all* asm/*.h includes in drivers,
> > which is FALSE.
> 
> http://marc.info/?l=linux-arm-kernel&m=132077795410718&w=2
> 
> I don't see me complaining about *all* headers anywhere in that message.
> I did generalize, but I didn't stated you were complaining about *all*
> headers. Go back and read it for yourself.

I really can't believe what you're saying.

"You were (actually still is) one of the biggest source of complaints for
drivers using <mach/*> and <asm/*> includes and now you're changing your
mind ?"

That's an exact quote.

That statement is very clear in its meaning - and it doesn't match your
assertion that it doesn't mean "all" because it makes no distinction what
so ever between any of those header files.

> You see ? I was asking $author to try and use some revision register in
> order to apply erratas instead of using cpu_is_at91rm9200().

For fuck sake, - AND AGAIN - I was asking you to *qualify* your fucking
statement.  It was you who then launched an attack about irrelevant junk
like what includes and so forth.

You must be drunk this evening.  Please come back once you've sobered up.

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Ryan Mallon]

On 08/11/11 21:49, Nikolaus Voss wrote:

> This driver has the following properties compared to the old driver:
> 1. Support for multiple interfaces.
> 2. Interrupt driven I/O as opposed to polling/busy waiting.
> 3. Support for _one_ repeated start (Sr) condition, which is enough
>    for most real-world applications including all SMBus transfer types.
>    (The hardware does not support issuing arbitrary Sr conditions on the
>     bus.)
> 
> Tested on Atmel G45 with BQ20Z80 battery SMBus client.


Hi Nikolaus. Few more review comments below.

~Ryan

> 
> Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
> ---
>  drivers/i2c/busses/Kconfig    |   11 +-
>  drivers/i2c/busses/i2c-at91.c |  442 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 446 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-at91.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 646068e..c4b6bdc 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -286,18 +286,15 @@ comment "I2C system bus drivers (mostly embedded / system-on-chip)"
>  
>  config I2C_AT91
>  	tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
> -	depends on ARCH_AT91 && EXPERIMENTAL && BROKEN
> +	depends on ARCH_AT91 && EXPERIMENTAL
>  	help
>  	  This supports the use of the I2C interface on Atmel AT91
>  	  processors.
>  
> -	  This driver is BROKEN because the controller which it uses
> -	  will easily trigger RX overrun and TX underrun errors.  Using
> -	  low I2C clock rates may partially work around those issues
> -	  on some systems.  Another serious problem is that there is no
> -	  documented way to issue repeated START conditions, as needed
> +	  A serious problem is that there is no documented way to issue
> +	  repeated START conditions for more than two messages, as needed
>  	  to support combined I2C messages.  Use the i2c-gpio driver
> -	  unless your system can cope with those limitations.
> +	  unless your system can cope with this limitation.
>  
>  config I2C_AU1550
>  	tristate "Au1550/Au1200 SMBus interface"
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> new file mode 100644
> index 0000000..e35479b
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -0,0 +1,442 @@
> +/*
> +
> +    i2c Support for Atmel's AT91 Two-Wire Interface (TWI)
> +
> +    Copyright (C) 2011 Nikolaus Voss <n.voss@weinmann.de>
> +
> +    Evolved from original work by:
> +    Copyright (C) 2004 Rick Bronson
> +    Converted to 2.6 by Andrew Victor <andrew@sanpeople.com>
> +
> +    Borrowed heavily from original work by:
> +    Copyright (C) 2000 Philip Edelbrock <phil@stimpy.netroedge.com>
> +
> +    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.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +
> +#include <mach/at91_twi.h>


This include file looks like it only contains register definitions which
are used in this file. Would be good to either move them directly into
this driver or move the header file to this directory and make it a
local include.

> +#include <mach/board.h>
> +#include <mach/cpu.h>
> +
> +#define TWI_CLOCK		100000		/* Hz. max 400 Kbits/sec */
> +#define AT91_I2C_TIMEOUT	(HZ/100)	/* transfer timeout */


There is the msecs_to_jiffies function which can make timeouts more
clearly readable. YMMV.

> +
> +struct at91_twi_dev {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct completion cmd_complete;
> +	struct clk *clk;
> +	u8 *buf;
> +	size_t buf_len;
> +	int irq;
> +	unsigned transfer_status;
> +	struct i2c_adapter adapter;
> +};


Nitpick: Tab aligning struct fields makes them a bit more readable.

> +
> +static inline unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
> +{
> +	return __raw_readl(dev->base + (reg));
> +}


It probably doesn't matter here, but there is a general push to remove
inline from functions since the compiler is smart enough to inline
functions like this itself.

You also don't need the additional parens around reg now that this is a
proper function. Same in the other functions which have been converted
from macros.

> +
> +static inline void at91_twi_write(struct at91_twi_dev *dev, unsigned reg,
> +				  unsigned val)
> +{
> +	__raw_writel((val), dev->base + (reg));
> +}
> +
> +static inline void at91_disable_twi_interrupts(struct at91_twi_dev *dev)
> +{
> +	at91_twi_write(dev, AT91_TWI_IDR,
> +		       AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY);
> +}
> +
> +static void at91_init_twi_bus(struct at91_twi_dev *dev)
> +{
> +	at91_disable_twi_interrupts(dev);
> +	at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_SWRST);
> +	at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_MSEN);
> +	at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_SVDIS);
> +}
> +
> +static void at91_set_twi_clock(struct at91_twi_dev *dev)
> +{
> +	unsigned long cdiv, ckdiv;
> +
> +	/* Calcuate clock dividers */
> +	cdiv = (clk_get_rate(dev->clk) / (2 * TWI_CLOCK)) - 3;
> +	cdiv = cdiv + 1;	/* round up */


	/* Calculate clock dividers - rounded up */
	cdiv = ((clk_get_rate(dev->clk) / (2 * TWI_CLOCK)) - 3) + 1;

	?

> +	ckdiv = 0;
> +	while (cdiv > 255) {
> +		ckdiv++;
> +		cdiv = cdiv >> 1;
> +	}
> +
> +	if (cpu_is_at91rm9200()) {			/* AT91RM9200 Errata #22 */
> +		if (ckdiv > 5) {


	if (cpu_is_at91rm9200() && clkdiv > 5) {

> +			printk(KERN_ERR "AT91 I2C: Invalid TWI_CLOCK value!\n");


	dev_err(dev->dev, "AT91RM9200 Errata #22: Invalid clock value - using
cldiv = 5\n");

> +			ckdiv = 5;
> +		}
> +	}
> +
> +	at91_twi_write(dev, AT91_TWI_CWGR, (ckdiv << 16) | (cdiv << 8) | cdiv);
> +}
> +
> +static void __devinit at91_twi_hwinit(struct at91_twi_dev *dev)
> +{
> +	at91_init_twi_bus(dev);
> +	at91_set_twi_clock(dev);
> +}
> +
> +static void at91_twi_write_next_byte(struct at91_twi_dev *dev)
> +{
> +	if (dev->buf_len > 0) {
> +
> +		const u8 c = *(dev->buf++);
> +
> +		at91_twi_write(dev, AT91_TWI_THR, c);
> +


Don't need all the blank lines here. Also, is there are reason to use
the temporary variable c and not put the buf dereference into the the
at91_twi_write call?

> +		/* send stop when last byte has been written */
> +		if (--dev->buf_len == 0)
> +			at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
> +
> +		dev_dbg(dev->dev, "wrote 0x%x, to go %d\n", c, dev->buf_len);
> +	}
> +}
> +
> +static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
> +{
> +	const u8 c = at91_twi_read(dev, AT91_TWI_RHR) & 0xff;
> +
> +	*(dev->buf)++ = c;


Same here, the temporary variable c should be able to be removed and
just do it in place.

> +
> +	/* send stop if second but last byte has been read */
> +	if (--dev->buf_len == 1)
> +		at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
> +
> +	dev_dbg(dev->dev, "read 0x%x, to go %d\n", c, dev->buf_len);
> +}
> +
> +static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> +{
> +	struct at91_twi_dev *dev = dev_id;
> +	const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
> +	const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> +
> +	if (irqstatus & AT91_TWI_TXCOMP) {
> +		at91_disable_twi_interrupts(dev);
> +		dev->transfer_status = status;
> +		complete(&dev->cmd_complete);
> +	}
> +	else if (irqstatus & AT91_TWI_RXRDY) {


CodingStyle is:

	if (foo) {
		foo();
	} else if (bar) {
		bar();
	} else {
		foobar();
	}

> +		at91_twi_read_next_byte(dev);
> +	}
> +	else if (irqstatus & AT91_TWI_TXRDY) {
> +		at91_twi_write_next_byte(dev);
> +	}
> +	else {
> +		return IRQ_NONE;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +int at91_do_twi_transfer(struct at91_twi_dev *dev, bool is_read)


static?

> +{
> +	int ret = 0;
> +
> +	INIT_COMPLETION(dev->cmd_complete);
> +
> +	if (is_read) {
> +		if (!dev->buf_len)
> +			at91_twi_write(dev, AT91_TWI_CR,
> +				       AT91_TWI_START | AT91_TWI_STOP);
> +		else
> +			at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_START);
> +
> +		at91_twi_write(dev, AT91_TWI_IER,
> +			       AT91_TWI_TXCOMP | AT91_TWI_RXRDY);
> +
> +	} else {
> +
> +		at91_twi_write_next_byte(dev);
> +
> +		at91_twi_write(dev, AT91_TWI_IER,
> +			       AT91_TWI_TXCOMP | AT91_TWI_TXRDY);


Nitpick: Too many blank lines.

> +	}
> +
> +	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
> +							dev->adapter.timeout);
> +	if (ret == 0) {
> +		dev_err(dev->dev, "controller timed out\n");
> +		at91_init_twi_bus(dev);
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (dev->transfer_status & AT91_TWI_NACK) {
> +		dev_dbg(dev->dev, "received nack\n");
> +		return -ENODEV;
> +	}
> +
> +	if (dev->transfer_status & AT91_TWI_OVRE) {
> +		dev_err(dev->dev, "overrun while reading\n");
> +		return -EIO;
> +	}
> +
> +	dev_dbg(dev->dev, "transfer complete\n");
> +
> +	return 0;
> +}
> +
> +static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
> +{
> +	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +	int ret;
> +	unsigned int_addr_flag = 0;
> +	struct i2c_msg *m_start = msg;
> +
> +	dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
> +
> +	/* the hardware can handle at most two messages concatenated by a
> +	 * repeated start via it's internal address feature.
> +	 */


Your multiline comment style is still wrong. See Documentation/CodingStyle.

> +	if (num > 2) {
> +		dev_err(dev->dev,
> +			"cannot handle more than two concatenated messages.\n");
> +		return 0;


Should these not return error codes?

> +
> +	} else if (num == 2) {
> +
> +		int internal_address = 0;
> +		int i;
> +
> +		if (msg->flags & I2C_M_RD) {
> +			dev_err(dev->dev, "first transfer must be write.\n");
> +			return 0;
> +		}
> +
> +		if (msg->len > 3) {
> +			dev_err(dev->dev, "first message size must be <= 3.\n");
> +			return 0;
> +		}
> +
> +		/* 1st msg is put into the internal address, start with 2nd */
> +		m_start = &msg[1];
> +
> +		for (i = 0; i < msg->len; ++i) {
> +			internal_address |= ((unsigned)msg->buf[i]) << (8 * i);
> +			int_addr_flag += AT91_TWI_IADRSZ_1;
> +		}
> +
> +		at91_twi_write(dev, AT91_TWI_IADR, internal_address);
> +	}
> +
> +	at91_twi_write(dev, AT91_TWI_MMR, (m_start->addr << 16) | int_addr_flag
> +		       | ((m_start->flags & I2C_M_RD) ? AT91_TWI_MREAD : 0));
> +
> +	dev->buf_len = m_start->len;
> +	dev->buf = m_start->buf;
> +
> +	ret = at91_do_twi_transfer(dev, m_start->flags & I2C_M_RD);
> +
> +	if (ret < 0)


Nitpick: Remove the blank line.

> +		return ret;
> +
> +	return num;
> +}
> +
> +static u32 at91_twi_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm at91_twi_algorithm = {
> +	.master_xfer	= at91_twi_xfer,
> +	.functionality	= at91_twi_func,
> +};
> +
> +static int __devinit at91_twi_probe(struct platform_device *pdev)
> +{
> +	struct at91_twi_dev *dev;
> +	struct resource *mem, *irq, *ioarea;
> +	int rc;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!mem)
> +		return -ENODEV;
> +
> +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!irq)
> +		return -ENODEV;
> +
> +	ioarea = request_mem_region(mem->start, resource_size(mem), pdev->name);
> +	if (!ioarea)
> +		return -EBUSY;
> +
> +	dev = kzalloc(sizeof(struct at91_twi_dev), GFP_KERNEL);
> +	if (!dev) {
> +		rc = -ENOMEM;
> +		goto err_release_region;
> +	}
> +
> +	init_completion(&dev->cmd_complete);
> +
> +	dev->dev = get_device(&pdev->dev);


Is this (and the put_device(&pdev->dev) in the exit code) necessary? I
see that some other i2c drivers do this also, but I'm not familiar with
using it in other device drivers. Isn't the reference count for
pdev->dev already incremented by the fact we are probing the device?

> +	dev->irq = irq->start;
> +	platform_set_drvdata(pdev, dev);
> +
> +	dev->clk = clk_get(&pdev->dev, "twi_clk");


This didn't get answered. Can't you just do:

	dev->clk = clk_get(&pdev->dev, NULL);

and clkdev should figure out the correct clock based on the device pointer?

> +	if (IS_ERR(dev->clk)) {
> +		dev_err(&pdev->dev, "no clock defined\n");
> +		rc = -ENODEV;
> +		goto err_free_mem;
> +	}
> +	clk_enable(dev->clk);
> +
> +	dev->base = ioremap(mem->start, resource_size(mem));
> +	if (!dev->base) {
> +		rc = -EBUSY;
> +		goto err_mem_ioremap;
> +	}
> +
> +	at91_twi_hwinit(dev);
> +
> +	rc = request_irq(dev->irq, atmel_twi_interrupt, 0,
> +			 dev_name(&pdev->dev), dev);
> +	if (rc) {
> +		dev_err(&pdev->dev, "Cannot get irq %d: %d\n", dev->irq, rc);
> +		goto err_unuse_clocks;
> +	}
> +
> +	snprintf(dev->adapter.name, sizeof(dev->adapter.name), "AT91");
> +	i2c_set_adapdata(&dev->adapter, dev);
> +	dev->adapter.owner = THIS_MODULE;
> +	dev->adapter.class = I2C_CLASS_HWMON;
> +	dev->adapter.algo = &at91_twi_algorithm;
> +	dev->adapter.dev.parent = &pdev->dev;
> +	dev->adapter.nr = pdev->id;
> +	dev->adapter.timeout = AT91_I2C_TIMEOUT;
> +
> +	rc = i2c_add_numbered_adapter(&dev->adapter);
> +	if (rc) {
> +		dev_err(&pdev->dev, "Adapter %s registration failed\n",
> +			dev->adapter.name);
> +		goto err_free_irq;
> +	}
> +
> +	dev_info(&pdev->dev, "AT91 i2c bus driver.\n");
> +	return 0;
> +
> +err_free_irq:
> +	free_irq(dev->irq, dev);
> +err_unuse_clocks:
> +	iounmap(dev->base);
> +err_mem_ioremap:
> +	clk_disable(dev->clk);
> +	clk_put(dev->clk);
> +	dev->clk = NULL;


You don't need to assign dev->clk to NULL.

> +err_free_mem:
> +	platform_set_drvdata(pdev, NULL);
> +	put_device(&pdev->dev);

> +	kfree(dev);

> +err_release_region:
> +	release_mem_region(mem->start, resource_size(mem));
> +
> +	return rc;
> +}
> +
> +static int __devexit at91_twi_remove(struct platform_device *pdev)
> +{
> +	struct at91_twi_dev *dev = platform_get_drvdata(pdev);
> +	struct resource *mem;
> +	int rc;
> +
> +	platform_set_drvdata(pdev, NULL);


I don't think this is necessary.

> +	rc = i2c_del_adapter(&dev->adapter);


Most of the other i2c drivers don't check the return code for
i2c_del_adapter. If this fails then you will unload the module, but
potentially leak resources that i2c_del_adapter failed to free up. Not
sure what the correct answer is here.

> +	put_device(&pdev->dev);
> +
> +	clk_disable(dev->clk);
> +	clk_put(dev->clk);
> +	dev->clk = NULL;
> +
> +	free_irq(dev->irq, dev);
> +
> +	iounmap(dev->base);
> +	kfree(dev);
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	release_mem_region(mem->start, resource_size(mem));
> +
> +	return rc;
> +}
> +
> +#ifdef CONFIG_PM
> +
> +static int at91_twi_suspend(struct device *dev)
> +{
> +	struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
> +
> +	clk_disable(twi_dev->clk);
> +
> +	return 0;
> +}
> +
> +static int at91_twi_resume(struct device *dev)
> +{
> +	struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
> +
> +	return clk_enable(twi_dev->clk);
> +}
> +
> +static const struct dev_pm_ops at91_twi_pm = {
> +	.suspend	= at91_twi_suspend,
> +	.resume		= at91_twi_resume,
> +};
> +
> +#define at91_twi_pm_ops (&at91_twi_pm)
> +#else
> +#define at91_twi_pm_ops NULL
> +#endif
> +
> +MODULE_ALIAS("platform:at91_i2c");
> +
> +static struct platform_driver at91_twi_driver = {
> +	.probe		= at91_twi_probe,
> +	.remove		= __devexit_p(at91_twi_remove),
> +	.driver		= {
> +		.name	= "at91_i2c",
> +		.owner	= THIS_MODULE,
> +		.pm	= at91_twi_pm_ops,
> +	},
> +};
> +
> +static int __init at91_twi_init(void)
> +{
> +	return platform_driver_register(&at91_twi_driver);
> +}
> +
> +static void __exit at91_twi_exit(void)
> +{
> +	platform_driver_unregister(&at91_twi_driver);
> +}
> +
> +module_init(at91_twi_init);
> +module_exit(at91_twi_exit);
> +
> +MODULE_AUTHOR("Nikolaus Voss");
> +MODULE_DESCRIPTION("I2C (TWI) driver for Atmel AT91");
> +MODULE_LICENSE("GPL");

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Ryan Mallon]

On 08/11/11 21:49, Nikolaus Voss wrote:

> This driver has the following properties compared to the old driver:
> 1. Support for multiple interfaces.
> 2. Interrupt driven I/O as opposed to polling/busy waiting.
> 3. Support for _one_ repeated start (Sr) condition, which is enough
>    for most real-world applications including all SMBus transfer types.
>    (The hardware does not support issuing arbitrary Sr conditions on the
>     bus.)
> 
> Tested on Atmel G45 with BQ20Z80 battery SMBus client.
> 
> Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>

<snip>

> +	dev->clk = clk_get(&pdev->dev, "twi_clk");
> +	if (IS_ERR(dev->clk)) {
> +		dev_err(&pdev->dev, "no clock defined\n");
> +		rc = -ENODEV;
> +		goto err_free_mem;
> +	}
> +	clk_enable(dev->clk);


There are now dummy clk_prepare/unprepare functions in
include/linux/clk.h, so you should be using them in this driver to
future proof it.

~Ryan

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Voss, Nikolaus]

Hi Ryan,

> > +#include <mach/at91_twi.h>
> 
> 
> This include file looks like it only contains register definitions which
> are used in this file. Would be good to either move them directly into
> this driver or move the header file to this directory and make it a
> local include.

Ok.

> > +
> > +	dev->dev = get_device(&pdev->dev);
> 
> 
> Is this (and the put_device(&pdev->dev) in the exit code) necessary? I
> see that some other i2c drivers do this also, but I'm not familiar with
> using it in other device drivers. Isn't the reference count for
> pdev->dev already incremented by the fact we are probing the device?

It is incremented by the platform_device_register() which does a
device_add(). This seems to be enough, I removed the get-/put_device().


> 
> > +	dev->irq = irq->start;
> > +	platform_set_drvdata(pdev, dev);
> > +
> > +	dev->clk = clk_get(&pdev->dev, "twi_clk");
> 
> 
> This didn't get answered. Can't you just do:
> 
> 	dev->clk = clk_get(&pdev->dev, NULL);
> 
> and clkdev should figure out the correct clock based on the device pointer?

No, this doesn't work on at91 since the clocks have no dev_id property
but only con_id. I cannot see a reason for this, but that's the way it's
done. Multiple hardware interfaces are handled via a lookup table.


> > +	rc = i2c_del_adapter(&dev->adapter);
> 
> 
> Most of the other i2c drivers don't check the return code for
> i2c_del_adapter. If this fails then you will unload the module, but
> potentially leak resources that i2c_del_adapter failed to free up. Not
> sure what the correct answer is here.

I don't really check the value but use it has exit code for the remove()-
function to indicate something went wrong. Just ignoring it wouldn't heal
the resource leakage.

Thanks for reviewing,

Niko

[PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

[Russell King - ARM Linux]

On Wed, Nov 09, 2011 at 05:01:07PM +0100, Voss, Nikolaus wrote:
> Hi Ryan,
> > 
> > > +	dev->irq = irq->start;
> > > +	platform_set_drvdata(pdev, dev);
> > > +
> > > +	dev->clk = clk_get(&pdev->dev, "twi_clk");
> > 
> > 
> > This didn't get answered. Can't you just do:
> > 
> > 	dev->clk = clk_get(&pdev->dev, NULL);
> > 
> > and clkdev should figure out the correct clock based on the device pointer?
> 
> No, this doesn't work on at91 since the clocks have no dev_id property
> but only con_id. I cannot see a reason for this, but that's the way it's
> done. Multiple hardware interfaces are handled via a lookup table.

If you can, you could move over to doing this via the standard method
and start switching some of this stuff over to using dev_id.

We'll then have another platform starting to use this stuff correctly.