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 <jan@xxxxxxxxxxx>
>> ---
>> 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 <jacmet@xxxxxxxxxx>
>> + *
>> + * Adaption to GRLIB/I2CMST by Jan Andersson <jan@xxxxxxxxxxx>
>> + *
>> + * 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 <jacmet@xxxxxxxxxx>");
>> +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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
