On 02/23/2011 02:06 AM, Ben Dooks wrote: > On Wed, Feb 16, 2011 at 01:30:48PM +0100, Jan Andersson wrote: >> This patch adds support for the I2CMST core found on LEON/GRLIB SoCs. >> >> Signed-off-by: Jan Andersson <[email protected]> >> --- >> The I2CMST core is basically the OpenCores I2C master with an AMBA APB >> interface. This driver re-uses much of i2c-ocores.c. It is submitted as >> a separate driver since the register interfaces differ sligthly. Also the >> two IP cores are maintained separately so they may diverge further in >> the future. > > Hmm, some more of that should go above the "---" line. > > Going to do a quick review and then get back to this once I've had some > time to think on it. >
Thanks, I will wait a while for further comments and then send a V2. >> The driver is identical in terms of transfer handling and HW control. >> The original module author string has been kept. >> >> drivers/i2c/busses/Kconfig | 10 + >> drivers/i2c/busses/Makefile | 1 + >> drivers/i2c/busses/i2c-gr-i2cmst.c | 371 >> ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 382 insertions(+), 0 deletions(-) >> create mode 100644 drivers/i2c/busses/i2c-gr-i2cmst.c >> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index 113505a..06a3150 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -646,6 +646,16 @@ config I2C_EG20T >> is an IOH(Input/Output Hub) for x86 embedded processor. >> This driver can access PCH I2C bus device. >> >> +config I2C_GR_I2CMST >> + tristate "Aeroflex Gaisler I2CMST I2C Controller" >> + depends on SPARC_LEON >> + help >> + If you say yes to this option, support will be included for the >> + I2CMST I2C controller present on some LEON/GRLIB SoCs. >> + >> + This driver can also be built as a module. If so, the module >> + will be called i2c-gr-i2cmst. >> + >> comment "External I2C/SMBus adapter drivers" >> >> config I2C_PARPORT >> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile >> index 9d2d0ec..1e16e66 100644 >> --- a/drivers/i2c/busses/Makefile >> +++ b/drivers/i2c/busses/Makefile >> @@ -62,6 +62,7 @@ obj-$(CONFIG_I2C_VERSATILE) += i2c-versatile.o >> obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o >> obj-$(CONFIG_I2C_XILINX) += i2c-xiic.o >> obj-$(CONFIG_I2C_EG20T) += i2c-eg20t.o >> +obj-$(CONFIG_I2C_GR_I2CMST) += i2c-gr-i2cmst.o >> >> # External I2C/SMBus adapter drivers >> obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o >> diff --git a/drivers/i2c/busses/i2c-gr-i2cmst.c >> b/drivers/i2c/busses/i2c-gr-i2cmst.c >> new file mode 100644 >> index 0000000..90b23a8 >> --- /dev/null >> +++ b/drivers/i2c/busses/i2c-gr-i2cmst.c >> @@ -0,0 +1,371 @@ >> +/* >> + * i2c-gr-i2cmst.c I2C bus driver for GRLIB I2CMST core >> + * >> + * Main parts of this driver re-used from: >> + * >> + * i2c-ocores.c: I2C bus driver for OpenCores I2C controller >> + * by Peter Korsgaard <[email protected]> >> + * >> + * Adaption to GRLIB/I2CMST by Jan Andersson <[email protected]> >> + * >> + * This file is licensed under the terms of the GNU General Public License >> + * version 2. This program is licensed "as is" without any warranty of any >> + * kind, whether express or implied. >> + */ >> + >> +#include <linux/of_platform.h> >> +#include <linux/of_device.h> >> +#include <linux/interrupt.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/errno.h> >> +#include <linux/i2c.h> >> +#include <linux/wait.h> >> +#include <linux/slab.h> >> +#include <linux/io.h> >> + >> +/* I2CMST APB registers */ >> +struct i2cmst_regs { >> + u32 prescaler; >> + u32 control; >> + u32 data; >> + u32 cmdstat; /* Command (write), Status (read) */ >> +}; > > using structures for memory mapped registers makes me want to go > EURGH. the compiler is free to pack and order as it feels. > Ah, perhaps I should have added a __attribute__ ((packed)); after that(?). For V2 I can go back to using a pointer to the base address to be consistent with many of the other i2c drivers. >> +struct gr_i2cmst_i2c { >> + struct i2cmst_regs *regs; >> + wait_queue_head_t wait; >> + struct i2c_adapter adap; >> + struct i2c_msg *msg; >> + int pos; >> + int nmsgs; >> + int state; /* see STATE_ */ >> + int clock_khz; >> +}; >> + >> +/* register fields and commands */ >> +#define I2CMST_CTRL_IEN 0x40 >> +#define I2CMST_CTRL_EN 0x80 >> + >> +#define I2CMST_CMD_START 0x91 >> +#define I2CMST_CMD_STOP 0x41 >> +#define I2CMST_CMD_READ 0x21 >> +#define I2CMST_CMD_WRITE 0x11 >> +#define I2CMST_CMD_READ_ACK 0x21 >> +#define I2CMST_CMD_READ_NACK 0x29 >> +#define I2CMST_CMD_IACK 0x01 >> + >> +#define I2CMST_STAT_IF 0x01 >> +#define I2CMST_STAT_TIP 0x02 >> +#define I2CMST_STAT_ARBLOST 0x20 >> +#define I2CMST_STAT_BUSY 0x40 >> +#define I2CMST_STAT_NACK 0x80 >> + >> +#define STATE_DONE 0 >> +#define STATE_START 1 >> +#define STATE_WRITE 2 >> +#define STATE_READ 3 >> +#define STATE_ERROR 4 >> + >> +static inline void i2cmst_setreg(void __iomem *reg, u32 value) >> +{ >> + __raw_writel(cpu_to_be32(value), reg); >> +} >> + >> +static inline u32 i2cmst_getreg(void __iomem *reg) >> +{ >> + return be32_to_cpu(__raw_readl(reg)); >> +} > > do you really need to use the __raw versions of thees calls? With just readl() the register value would be byte swapped on my platform. After looking a bit closer at this it seems like it could be appropriate to replace the whole line with a call to ioread32be(..). > >> +static void gr_i2cmst_process(struct gr_i2cmst_i2c *i2c) >> +{ >> + struct i2c_msg *msg = i2c->msg; >> + u32 stat = i2cmst_getreg(&i2c->regs->cmdstat); >> + >> + if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) { >> + /* stop has been sent */ >> + i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_IACK); >> + wake_up(&i2c->wait); >> + return; >> + } >> + >> + /* error? */ >> + if (stat & I2CMST_STAT_ARBLOST) { >> + i2c->state = STATE_ERROR; >> + i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP); >> + return; >> + } >> + >> + if ((i2c->state == STATE_START) || (i2c->state == STATE_WRITE)) { >> + i2c->state = >> + (msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE; >> + >> + if (stat & I2CMST_STAT_NACK) { >> + i2c->state = STATE_ERROR; >> + i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP); >> + return; >> + } >> + } else >> + msg->buf[i2c->pos++] = i2cmst_getreg(&i2c->regs->data); >> + >> + /* end of msg? */ >> + if (i2c->pos == msg->len) { >> + i2c->nmsgs--; >> + i2c->msg++; >> + i2c->pos = 0; >> + msg = i2c->msg; >> + >> + if (i2c->nmsgs) { /* end? */ >> + /* send start? */ >> + if (!(msg->flags & I2C_M_NOSTART)) { >> + u8 addr = (msg->addr << 1); >> + >> + if (msg->flags & I2C_M_RD) >> + addr |= 1; >> + >> + i2c->state = STATE_START; >> + >> + i2cmst_setreg(&i2c->regs->data, addr); >> + i2cmst_setreg(&i2c->regs->cmdstat, >> + I2CMST_CMD_START); >> + return; >> + } else >> + i2c->state = (msg->flags & I2C_M_RD) >> + ? STATE_READ : STATE_WRITE; >> + } else { >> + i2c->state = STATE_DONE; >> + i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_STOP); >> + return; >> + } >> + } >> + >> + if (i2c->state == STATE_READ) { >> + i2cmst_setreg(&i2c->regs->cmdstat, i2c->pos == (msg->len-1) ? >> + I2CMST_CMD_READ_NACK : I2CMST_CMD_READ_ACK); >> + } else { >> + i2cmst_setreg(&i2c->regs->data, msg->buf[i2c->pos++]); >> + i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_WRITE); >> + } >> +} >> + >> +static irqreturn_t gr_i2cmst_isr(int irq, void *dev_id) >> +{ >> + struct gr_i2cmst_i2c *i2c = dev_id; >> + >> + gr_i2cmst_process(i2c); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int gr_i2cmst_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, >> int num) >> +{ >> + struct gr_i2cmst_i2c *i2c = i2c_get_adapdata(adap); >> + >> + i2c->msg = msgs; >> + i2c->pos = 0; >> + i2c->nmsgs = num; >> + i2c->state = STATE_START; >> + >> + i2cmst_setreg(&i2c->regs->data, (i2c->msg->addr << 1) | >> + ((i2c->msg->flags & I2C_M_RD) ? 1 : 0)); >> + >> + i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_START); >> + >> + if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) || >> + (i2c->state == STATE_DONE), HZ)) >> + return (i2c->state == STATE_DONE) ? num : -EIO; >> + else >> + return -ETIMEDOUT; >> +} >> + >> +static void gr_i2cmst_init(struct gr_i2cmst_i2c *i2c) >> +{ >> + int prescale; >> + u32 ctrl = i2cmst_getreg(&i2c->regs->control); >> + >> + /* make sure the device is disabled */ >> + i2cmst_setreg(&i2c->regs->control, >> + ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN)); >> + >> + /* Required prescaler = (AMBAfreq)/(5 * SCLfreq)-1 */ >> + prescale = (i2c->clock_khz / (5*100)) - 1; >> + /* Minimum value of precaler is 3 due to HW sync */ >> + prescale = prescale < 3 ? 3 : prescale; >> + i2cmst_setreg(&i2c->regs->prescaler, prescale); >> + >> + /* Init the device */ >> + i2cmst_setreg(&i2c->regs->cmdstat, I2CMST_CMD_IACK); >> + ctrl |= I2CMST_CTRL_IEN | I2CMST_CTRL_EN; >> + i2cmst_setreg(&i2c->regs->control, ctrl); >> +} >> + >> + >> +static u32 gr_i2cmst_func(struct i2c_adapter *adap) >> +{ >> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; >> +} >> + >> +static const struct i2c_algorithm gr_i2cmst_algorithm = { >> + .master_xfer = gr_i2cmst_xfer, >> + .functionality = gr_i2cmst_func, >> +}; >> + >> +static struct i2c_adapter gr_i2cmst_adapter = { >> + .owner = THIS_MODULE, >> + .name = "i2c-gr-i2cmst", >> + .class = I2C_CLASS_HWMON | I2C_CLASS_SPD, >> + .algo = &gr_i2cmst_algorithm, >> +}; >> + >> + >> +static int __devinit gr_i2cmst_of_probe(struct platform_device *ofdev, >> const struct of_device_id *match) > > please avoid line-wrap like this. OK > >> +{ >> + struct gr_i2cmst_i2c *i2c; >> + const unsigned int *busfreq; >> + int ret; >> + >> + i2c = kzalloc(sizeof(*i2c), GFP_KERNEL); >> + if (!i2c) >> + return -ENOMEM; > > would be nice to have an error print here. OK > >> + /* Get frequency of APB bus that core is attached to */ >> + busfreq = of_get_property(ofdev->dev.of_node, "freq", NULL); >> + if (!busfreq) { >> + dev_err(&ofdev->dev, "Missing parameter 'freq'\n"); >> + return -ENODEV; >> + } >> + i2c->clock_khz = *busfreq/1000; >> + >> + i2c->regs = of_ioremap(&ofdev->resource[0], 0, >> + resource_size(&ofdev->resource[0]), >> + "grlib-i2cmst regs"); >> + if (!i2c->regs) { >> + dev_err(&ofdev->dev, "Unable to map registers\n"); >> + ret = -EIO; >> + goto map_failed; >> + } >> + >> + gr_i2cmst_init(i2c); >> + >> + init_waitqueue_head(&i2c->wait); >> + ret = request_irq(ofdev->archdata.irqs[0], gr_i2cmst_isr, 0, >> + ofdev->name, i2c); >> + if (ret) { >> + dev_err(&ofdev->dev, "Cannot claim IRQ\n"); >> + goto request_irq_failed; >> + } >> + >> + /* hook up driver to tree */ >> + dev_set_drvdata(&ofdev->dev, i2c); >> + i2c->adap = gr_i2cmst_adapter; >> + i2c_set_adapdata(&i2c->adap, i2c); >> + i2c->adap.dev.parent = &ofdev->dev; >> + i2c->adap.dev.of_node = ofdev->dev.of_node; >> + >> + /* add i2c adapter to i2c tree */ >> + ret = i2c_add_adapter(&i2c->adap); >> + if (ret) { >> + dev_err(&ofdev->dev, "Failed to add adapter\n"); >> + goto add_adapter_failed; >> + } >> + >> + return 0; >> + >> +add_adapter_failed: >> + free_irq(ofdev->archdata.irqs[0], i2c); >> +request_irq_failed: >> + of_iounmap(&ofdev->resource[0], i2c->regs, >> + resource_size(&ofdev->resource[0])); >> +map_failed: >> + kfree(i2c); >> + >> + return ret; >> +} >> + >> +static int __devexit gr_i2cmst_of_remove(struct platform_device *ofdev) >> +{ >> + struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev); >> + u32 ctrl = i2cmst_getreg(&i2c->regs->control); >> + >> + /* disable i2c logic */ >> + i2cmst_setreg(&i2c->regs->control, >> + ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN)); >> + >> + /* remove adapter & data */ >> + i2c_del_adapter(&i2c->adap); >> + dev_set_drvdata(&ofdev->dev, NULL); >> + >> + free_irq(ofdev->archdata.irqs[0], i2c); >> + >> + of_iounmap(&ofdev->resource[0], i2c->regs, >> + resource_size(&ofdev->resource[0])); >> + >> + kfree(i2c); >> + >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM >> +static int gr_i2cmst_suspend(struct platform_device *ofdev, pm_message_t >> state) >> +{ >> + struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev); >> + u32 ctrl = gr_i2cmst_getreg(&i2c->regs->control); >> + >> + /* make sure the device is disabled */ >> + gr_i2cmst_setreg(&i2c->regs->control, >> + ctrl & ~(I2CMST_CTRL_EN|I2CMST_CTRL_IEN)); >> + >> + return 0; >> +} >> + >> +static int gr_i2cmst_resume(struct platform_device *ofdev) >> +{ >> + struct gr_i2cmst_i2c *i2c = dev_get_drvdata(&ofdev->dev); >> + >> + gr_i2cmst_init(i2c); >> + >> + return 0; >> +} >> +#else >> +#define gr_i2cmst_suspend NULL >> +#define gr_i2cmst_resume NULL >> +#endif >> + >> +static struct of_device_id gr_i2cmst_of_match[] = { >> + { .name = "GAISLER_I2CMST", }, >> + { .name = "01_028", }, >> + {}, >> +}; >> + >> +MODULE_DEVICE_TABLE(of, gr_i2cmst_of_match); >> + >> +static struct of_platform_driver gr_i2cmst_of_driver = { >> + .driver = { >> + .name = "grlib-i2cmst", >> + .owner = THIS_MODULE, >> + .of_match_table = gr_i2cmst_of_match, >> + }, >> + .probe = gr_i2cmst_of_probe, >> + .remove = __devexit_p(gr_i2cmst_of_remove), >> + .suspend = gr_i2cmst_suspend, >> + .resume = gr_i2cmst_resume, >> +}; >> + >> + >> +static int __init i2cmst_init(void) >> +{ >> + return of_register_platform_driver(&gr_i2cmst_of_driver); >> +} >> + >> +static void __exit i2cmst_exit(void) >> +{ >> + of_unregister_platform_driver(&gr_i2cmst_of_driver); >> +} >> + >> +module_init(i2cmst_init); >> +module_exit(i2cmst_exit); >> + >> +MODULE_AUTHOR("Peter Korsgaard <[email protected]>"); >> +MODULE_DESCRIPTION("GRLIB I2CMST I2C bus driver"); >> +MODULE_LICENSE("GPL"); > Thanks for reviewing! Best regards, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
