* Re: [PATCH] Add support for on-board i2c device on NXP PNX833x SOC
[not found] ` <64660ef00806060502n369d495l79c68351c3799d60-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2008-06-16 0:21 ` Ben Dooks
0 siblings, 0 replies; 3+ messages in thread
From: Ben Dooks @ 2008-06-16 0:21 UTC (permalink / raw)
To: Daniel Laird; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA
On Fri, Jun 06, 2008 at 01:02:43PM +0100, Daniel Laird wrote:
> The following patch add support for the NXP PNX833x SOC i2c device.
> The SOC code is pending with linux-mips.
>
> drivers/i2c/busses/Kconfig | 12 +
> drivers/i2c/busses/Makefile | 1
> drivers/i2c/busses/i2c-pnx0105.c | 328 +++++++++++++++++++++++++++++++++++++++
> include/linux/i2c-id.h | 1
> include/linux/i2c-pnx0105.h | 58 ++++++
> 5 files changed, 400 insertions(+)
>
> Signed-off-by: daniel.j.laird <daniel.j.laird-3arQi8VN3Tc@public.gmane.org>
>
> diff -urN --exclude=.svn
> linux-2.6.26-rc4.orig/drivers/i2c/busses/i2c-pnx0105.c
> linux-2.6.26-rc4/drivers/i2c/busses/i2c-pnx0105.c
> --- linux-2.6.26-rc4.orig/drivers/i2c/busses/i2c-pnx0105.c 1970-01-01
> 01:00:00.000000000 +0100
> +++ linux-2.6.26-rc4/drivers/i2c/busses/i2c-pnx0105.c 2008-06-05
> 11:25:57.000000000 +0100
> @@ -0,0 +1,328 @@
> +/*
> + * i2c-pnx0105.c: driver for PNX833X I2C (IP0105 Block)
> + *
> + * Copyright 2008 NXP Semiconductors
> + * Daniel Laird <daniel.j.laird-3arQi8VN3Tc@public.gmane.org>
> + *
> + * Copyright (C) 2006 Nikita Youshchenko <yoush-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
Please ensure that this authour is copied in. If the original authour
isn't interested, then it would be useful to find this out, otherwise
please CC: and attempt to get a Signed-off-by: line for this.
> + *
> + * Partially based on i2c-pca-isa driver, Copyright (C) 2004 Arcom Control
> + * Systems.
> + *
> + * 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.
> + *
> + * 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.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c-id.h>
> +#include <linux/i2c-pnx0105.h>
> +#include <linux/i2c-algo-pca.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +
> +static inline unsigned long i2c_pnx0105_in(struct i2c_pnx0105_dev
> *dev, int offset)
> +{
> + return readl((unsigned long *)(dev->base + offset));
> +}
Firstly, 'unsigned long *' is missing the __iomem attribute, which
sparse should moan about if you've not tried running sparse during
the compile process.
Secondly, if you define 'dev->base' as 'void __iomem *', then readl()
should just accept that (and if not, then your architecture's readl
is, IMHO, broken). This should also remove the necessity to cast
dev->base to readl.
> +static inline void i2c_pnx0105_out(struct i2c_pnx0105_dev *dev, int
> offset, unsigned long value)
> +{
> + writel(value, (unsigned long *)(dev->base + offset));
> +}
See above comments for i2c_pnx0105_in.
> +static void i2c_pnx0105_writebyte(void *pa, int reg, int val)
> +{
> + struct i2c_algo_pca_data *algo_data = container_of(pa, struct
> i2c_algo_pca_data, data);
> + struct i2c_pnx0105_dev *dev = container_of(algo_data, struct
> i2c_pnx0105_dev, algo_data);
> + int old_si;
> +
> +#ifdef DEBUG
> + static char *names[] = { "T/O", "DAT", "ADR", "CON"};
> + printk(KERN_DEBUG "i2c_pnx0105(0x%08lx): write %s <= %#04x\n",
> dev->base, names[reg], val);
> +#endif
One, i'd suggest making a single area of debug which is then inlined
Two, keep a pointer to the device your driver bound to and use
dev_dbg() to output debug information.
> +
> + switch (reg) {
> +
> + case I2C_PCA_DAT:
> + i2c_pnx0105_out(dev, I2C_PNX0105_DAT, val & 255);
> + break;
> +
> + case I2C_PCA_ADR:
> + i2c_pnx0105_out(dev, I2C_PNX0105_ADDRESS, val & 255);
> + break;
out of interest, is it possible to avoid the '& 255' on these?
> +
> + case I2C_PCA_CON:
> + /* Possible RACE: just after init, or after stop,
> + SI bit is zero. That means that when STA bit
> + is written, hardware starts to process it
> + immediately. It could complete very fast (or
> + perhaps thread may get preempted), so when code
> + several lines below is executed, SI could already
> + be set to indicate that STA processing is complete.
> + In this case, SI must NOT be cleared here, so
> + hardware won't continue and send slave address
> + before it was written to register.
> + However, if SI bit is currently set, hardware
> + won't process command immediately, and SI should
> + be cleared at the bottom, to enable processing.
> + Solution: just check SI here, and clear it only
> + if it was set before any new value was written
> + to command register.
> + */
> + old_si = i2c_pnx0105_in(dev, I2C_PNX0105_INT_STATUS) & 1;
> +
> + i2c_pnx0105_out(dev, I2C_PNX0105_CONTROL, val & 255);
> +
> + /* We have to process STO bit separately */
> + if (val & I2C_PCA_CON_STO)
> + i2c_pnx0105_out(dev, I2C_PNX0105_STOP, 1);
> +
> + /* And also SI bit ... */
> + if (old_si && !(val & I2C_PCA_CON_SI)) {
> + i2c_pnx0105_out(dev, I2C_PNX0105_INT_CLEAR, 1);
> + if (dev->irq > -1 && !(val & I2C_PCA_CON_STO))
> + i2c_pnx0105_out(dev, I2C_PNX0105_INT_ENABLE, 1);
> + }
> +
> + break;
> +
> + default:
> + BUG();
> + }
> +}
> +
> +static int i2c_pnx0105_readbyte(void *pa, int reg)
> +{
> + struct i2c_algo_pca_data *algo_data = container_of(pa, struct
> i2c_algo_pca_data, data);
> + struct i2c_pnx0105_dev *dev = container_of(algo_data, struct
> i2c_pnx0105_dev, algo_data);
these wrap, it would be nicer if you had a pair of inline functions, such
as to_pca() and to_pnxdev() to deal with this.
> + int res = 0;
> +
> + switch (reg) {
> +
> + case I2C_PCA_STA:
> + if (dev->timeout) {
> + res = 0xff;
> + dev->timeout = 0;
> + } else
> + res = i2c_pnx0105_in(dev, I2C_PNX0105_STATUS) & 255;
whopping great space between 'res' and i2c_pnx0105_in().
> + break;
> +
> + case I2C_PCA_DAT:
> + res = i2c_pnx0105_in(dev, I2C_PNX0105_DAT) & 255;
> + break;
> +
> + case I2C_PCA_CON:
> + res = i2c_pnx0105_in(dev, I2C_PNX0105_CONTROL) & 255;
> +
> + /* Read SI bit from elsewhere */
> + if (i2c_pnx0105_in(dev, I2C_PNX0105_INT_STATUS))
> + res |= I2C_PCA_CON_SI;
> + else
> + res &= ~I2C_PCA_CON_SI;
oops, big space again?
> +
> + break;
> +
> + default:
> + BUG();
> + }
> +
> +#ifdef DEBUG
> + {
> + static char *names[] = { "STA", "DAT", "ADR", "CON"};
> + printk(KERN_DEBUG "i2c_pnx0105(0x%08lx): read %s => %#04x\n",
> dev->base, names[reg], res);
> + }
> +#endif
could you turn these bits of debug into inline functions and group
them at the top of the file together.
> + return res;
> +}
> +
> +static inline void i2c_pnx0105_reset(struct i2c_pnx0105_dev *dev)
> +{
> + unsigned long val = i2c_pnx0105_in(dev, I2C_PNX0105_CONTROL) & 0x47;
blank line in here would be nice. What is 0x47? and add to that, can
we have constants for some of these values you are using?
> + i2c_pnx0105_out(dev, I2C_PNX0105_CONTROL, val | 0x40);
> + i2c_pnx0105_out(dev, I2C_PNX0105_STOP, 1);
> + i2c_pnx0105_out(dev, I2C_PNX0105_INT_CLEAR, 1);
> + udelay(200);
> + i2c_pnx0105_out(dev, I2C_PNX0105_CONTROL, val);
> +}
> +
> +static inline int i2c_pnx0105_intr_condition(struct i2c_pnx0105_dev *dev)
> +{
> + return i2c_pnx0105_in(dev, I2C_PNX0105_INT_STATUS) & 1;
> +}
> +
> +static int i2c_pnx0105_waitforcompletion(void *pa)
> +{
> + struct i2c_algo_pca_data *algo_data = container_of(pa, struct
> i2c_algo_pca_data, data);
> + struct i2c_pnx0105_dev *dev = container_of(algo_data, struct
> i2c_pnx0105_dev, algo_data);
> +
> + /* Set some timeout */
> +#define JIFFIES_TO_WAIT ((HZ / 100) + 1) /* attempt to model 10 milliseconds */
there are valid calls for jiffies=>msecs.
> +
> + if (dev->irq > -1) {
> + wait_event_timeout(dev->wait,
> + i2c_pnx0105_intr_condition(dev), JIFFIES_TO_WAIT);
hmm, is that indeting proprely or is my mail client doing silliness?
> + } else {
> + unsigned long end = jiffies + JIFFIES_TO_WAIT;
> + while (!i2c_pnx0105_intr_condition(dev) &&
> + time_before(jiffies, end)) {
> + if (in_atomic())
> + udelay(100);
> + else
> + schedule();
> + }
is there a better way to do this other than udelay/schedule? I'm also
not entirely sure if the in_atomic() bit is a good diea, why would it
be called in an atomic context?
> + }
> +
> + if (i2c_pnx0105_intr_condition(dev))
> + return 0;
> +
> + /* Timeout. Reset device and make next status read to return 0xff */
> + i2c_pnx0105_reset(dev);
> + dev->timeout = 1;
> + return -EIO; /* Ignored anyway */
> +}
> +
> +static irqreturn_t i2c_pnx0105_interrupt(int this_irq, void *dev_id)
> +{
> + struct i2c_pnx0105_dev *dev = (struct i2c_pnx0105_dev *)dev_id;
> +
> + /* Disable interrupt for a while (until it's actually handled) */
> + i2c_pnx0105_out(dev, I2C_PNX0105_INT_ENABLE, 0);
> +
> + /* Wake up any process waiting for this interrupt */
> + wake_up_interruptible(&dev->wait);
> +
> + return IRQ_HANDLED;
> +}
An explanation of why you need to use a process, would it be possible
to deal with some of the interrupt conditions iwhtin the interrupt
handler?
> +
> +static int __devinit i2c_pnx0105_probe(struct platform_device *pdev)
> +{
> + struct i2c_pnx0105_dev *dev = (struct i2c_pnx0105_dev *)
> pdev->dev.platform_data;
you don't need a cast here.
> + struct i2c_algo_pca_data *algo_data = &dev->algo_data;
> + struct i2c_adapter *adap = &dev->adap;
> + int res;
> +
> + algo_data->write_byte = i2c_pnx0105_writebyte;
> + algo_data->read_byte = i2c_pnx0105_readbyte;
> + algo_data->wait_for_completion = i2c_pnx0105_waitforcompletion;
> +
> + adap->owner = THIS_MODULE;
> + adap->id = I2C_HW_A_PNX0105;
> + adap->algo_data = algo_data;
> + strncpy(adap->name, pdev->name, I2C_NAME_SIZE);
> +
> + dev->timeout = 0;
> + init_waitqueue_head(&dev->wait);
> +
> + if (request_region(dev->base, I2C_PNX0105_IO_SIZE, "i2c-pnx") == 0) {
Passing in an IO-address like this isn't standard, usually the physical
address of the device is passed in via the resource structure. You could
also pass in the IRQ number like this.
> + printk(KERN_ERR "i2c-pnx0105: request_region(0x%08lx) failed\n",
> + dev->base);
> + return -EBUSY;
> + }
you've got a platform device, use dev_err() here.
> +
> + /* Disable interrupt - just to be sure ... */
> + i2c_pnx0105_out(dev, I2C_PNX0105_INT_ENABLE, 0);
> +
> + if (dev->irq > -1) {
> + res = request_irq(dev->irq, i2c_pnx0105_interrupt, 0, "i2c-pnx", dev);
> + if (res < 0) {
> + printk(KERN_ERR "i2c-pnx0105: request_irq() failed\n");
> + goto err_region;
> + }
> + }
add dev_err() is the write thing to do here ttoo.
> + /* Rude attempt to probe hardware, to avoid future hangups if it is
> + not responding */
is this to ensure the hardware is there and working? why was it added to
the platform bus if you're not sure if it is there?
> + i2c_pnx0105_out(dev, I2C_PNX0105_CONTROL, 0x60);
> + udelay(200);
> + res = i2c_pnx0105_intr_condition(dev) ? 0 : -ENODEV;
> + i2c_pnx0105_reset(dev);
> +
> + if (res < 0) {
> + printk(KERN_ERR "i2c-pnx0105: device at 0x%08lx is not responding\n",
> + dev->base);
> + goto err_irq;
> + }
and here, dev_err is also your friend, ie:
dev_err(&pdev->dev, "device at 0x%08lx is not responding\n");
PS, %08lx is not good enough for a resource type as there is the possibility
of building a kernel with 64bit resources, so:
dev_err(&pdev->dev, "device at 0x%llx is not responding\n".
(unsigned long long)dev->base).
Alternatively, drop the dev base, the platform device has a unique id.
> +
> + res = i2c_pca_add_bus(adap);
> + if (res < 0) {
> + printk(KERN_ERR "i2c-pnx0105: i2c_pca_add_bus() failed\n");
> + goto err_irq;
> + }
> +
> + printk(KERN_INFO "i2c-pnx0105: registered device at 0x%08lx", dev->base);
> + if (dev->irq > -1)
> + printk(KERN_ERR ", irq %d", dev->irq);
> + printk(KERN_INFO "\n");
> +
> + return 0;
> +
> +err_irq:
> + if (dev->irq > -1)
> + free_irq(dev->irq, dev);
> +
> +err_region:
> + release_region(dev->base, I2C_PNX0105_IO_SIZE);
> +
> + return res;
> +}
> +
> +static int __devexit i2c_pnx0105_remove(struct platform_device *pdev)
> +{
> + struct i2c_pnx0105_dev *dev = (struct i2c_pnx0105_dev *)
> pdev->dev.platform_data;
cast is not needed here.
> + struct i2c_adapter *adap = &dev->adap;
> + int res;
> +
> + res = i2c_del_adapter(adap);
> + if (res < 0)
> + return res;
print an error, this is seroues.
> +
> + if (dev->irq > -1)
> + free_irq(dev->irq, dev);
> +
> + release_region(dev->base, I2C_PNX0105_IO_SIZE);
> +
> + return 0;
> +}
> +
> +static struct platform_driver i2c_pnx0105_driver = {
> + .probe = i2c_pnx0105_probe,
> + .remove = __devexit_p(i2c_pnx0105_remove),
> + .driver = {
over-indented?
> + .owner = THIS_MODULE,
> + .name = "i2c-pnx0105",
> + },
> +};
out of interest, no suspend/resume?
> +
> +static int __init i2c_pnx0105_init(void)
> +{
> + return platform_driver_register(&i2c_pnx0105_driver);
> +}
> +
> +static void __exit i2c_pnx0105_cleanup(void)
> +{
> + platform_driver_unregister(&i2c_pnx0105_driver);
> +}
> +
> +module_init(i2c_pnx0105_init);
> +module_exit(i2c_pnx0105_cleanup);
> +
> +MODULE_AUTHOR("Nikita Youshchenko <yoush-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>");
> +MODULE_DESCRIPTION("PNX833X I2C driver");
> +MODULE_LICENSE("GPL");
One, you're after MODULE_LICENSE("GPL v2"); here
Two, you've missed the MODULE_ALIAS("platform:i2c-pnx0105");
> +
> +
> diff -urN --exclude=.svn
> linux-2.6.26-rc4.orig/drivers/i2c/busses/Kconfig
> linux-2.6.26-rc4/drivers/i2c/busses/Kconfig
> --- linux-2.6.26-rc4.orig/drivers/i2c/busses/Kconfig 2008-06-03
> 10:56:53.000000000 +0100
> +++ linux-2.6.26-rc4/drivers/i2c/busses/Kconfig 2008-06-04
> 09:29:35.000000000 +0100
> @@ -677,6 +677,18 @@
> This driver can also be built as a module. If so, the module
> will be called i2c-pnx.
>
> +config I2C_PNX0105
> + tristate "I2C bus support for Philips PNX8XXX targets"
> + depends on I2C && SOC_PNX833X
> + select I2C_ALGOPCA
> + default y
> + help
> + Support for NXP PNX SoC internal I2C (IP0105).
> + Say y or m if you want to use PNX I2C interfaces.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-pnx0105.
> +
> config I2C_PMCMSP
> tristate "PMC MSP I2C TWI Controller"
> depends on PMC_MSP
> diff -urN --exclude=.svn
> linux-2.6.26-rc4.orig/drivers/i2c/busses/Makefile
> linux-2.6.26-rc4/drivers/i2c/busses/Makefile
> --- linux-2.6.26-rc4.orig/drivers/i2c/busses/Makefile 2008-06-03
> 10:56:53.000000000 +0100
> +++ linux-2.6.26-rc4/drivers/i2c/busses/Makefile 2008-06-04
> 09:29:40.000000000 +0100
> @@ -34,6 +34,7 @@
> obj-$(CONFIG_I2C_PIIX4) += i2c-piix4.o
> obj-$(CONFIG_I2C_PMCMSP) += i2c-pmcmsp.o
> obj-$(CONFIG_I2C_PNX) += i2c-pnx.o
> +obj-$(CONFIG_I2C_PNX0105) += i2c-pnx0105.o
> obj-$(CONFIG_I2C_PROSAVAGE) += i2c-prosavage.o
> obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
> obj-$(CONFIG_I2C_S3C2410) += i2c-s3c2410.o
> diff -urN --exclude=.svn linux-2.6.26-rc4.orig/include/linux/i2c-id.h
> linux-2.6.26-rc4/include/linux/i2c-id.h
> --- linux-2.6.26-rc4.orig/include/linux/i2c-id.h 2008-06-05
> 11:41:44.000000000 +0100
> +++ linux-2.6.26-rc4/include/linux/i2c-id.h 2008-06-04 09:33:51.000000000 +0100
> @@ -129,6 +129,7 @@
>
> /* --- PCA 9564 based algorithms */
> #define I2C_HW_A_ISA 0x1a0000 /* generic ISA Bus interface card */
> +#define I2C_HW_A_PNX0105 0x1a0001 /* NXP PNX833X SoC I2C */
>
> /* --- PowerPC on-chip adapters */
> #define I2C_HW_OCP 0x120000 /* IBM on-chip I2C adapter */
> diff -urN --exclude=.svn
> linux-2.6.26-rc4.orig/include/linux/i2c-pnx0105.h
> linux-2.6.26-rc4/include/linux/i2c-pnx0105.h
> --- linux-2.6.26-rc4.orig/include/linux/i2c-pnx0105.h 1970-01-01
> 01:00:00.000000000 +0100
> +++ linux-2.6.26-rc4/include/linux/i2c-pnx0105.h 2008-06-05
> 09:32:31.000000000 +0100
> @@ -0,0 +1,58 @@
> +/*
> + * i2c-pnx0105.h: driver for PNX833X I2C (IP0105 Block)
> + * Copyright (C) 2006 Nikita Youshchenko <yoush-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
> + *
> + * 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.
> + *
> + * 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.
> + */
> +#ifndef _LINUX_I2C_PNX0105_H
> +#define _LINUX_I2C_PNX0105_H
> +
> +#include <linux/i2c.h>
> +#include <linux/i2c-algo-pca.h>
> +#include <linux/wait.h>
> +
> +struct i2c_pnx0105_dev {
> + unsigned long base;
> + int irq;
> + unsigned char clock; /* value to write to freq bits of control reg */
> + unsigned char bus_addr; /* bus address for slave mode; currently not
> supported */
> +
> + int timeout; /* non-zero when timeout was detected */
> + wait_queue_head_t wait;
> +
> + struct i2c_algo_pca_data algo_data;
> + struct i2c_adapter adap;
> +};
I'd move the 'struct i2c_pnx0105_dev' into the driver, where it is
easier to see and you don't end up having to include three files
for a head.
> +
> +/* Register area size */
> +#define I2C_PNX0105_IO_SIZE 0x1000
> +
> +/* Register offsets */
> +#define I2C_PNX0105_CONTROL 0x0000
> +#define I2C_PNX0105_DAT 0x0004
> +#define I2C_PNX0105_STATUS 0x0008
> +#define I2C_PNX0105_ADDRESS 0x000C
> +#define I2C_PNX0105_STOP 0x0010
> +#define I2C_PNX0105_PD 0x0014
> +#define I2C_PNX0105_SET_PINS 0x0018
> +#define I2C_PNX0105_OBS_PINS 0x001C
> +#define I2C_PNX0105_INT_STATUS 0x0FE0
> +#define I2C_PNX0105_INT_ENABLE 0x0FE4
> +#define I2C_PNX0105_INT_CLEAR 0x0FE8
> +#define I2C_PNX0105_INT_SET 0x0FEC
> +#define I2C_PNX0105_POWER_DOWN 0x0FF4
> +#define I2C_PNX0105_MODULE_ID 0x0FFC
> +
> +#endif
As a note, have you run either of sparse or kernel/scripts/checkpatch.pl
over this?
--
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
'a smiley only costs 4 bytes'
_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
^ permalink raw reply [flat|nested] 3+ messages in thread