Hi, thanks for the submission. > --- /dev/null > +++ b/drivers/i2c/busses/i2c-hix5hd2.c > @@ -0,0 +1,573 @@ > +/* > + * Copyright (c) 2014 Linaro Ltd. > + * Copyright (c) 2014 Hisilicon Limited. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * publishhed by the Free Software Foundation. "publishhed" > + * > + * Now only support 7 bit address. > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/io.h> > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> I think there should be at least of.h, too. > +struct hix5hd2_i2c { > + struct i2c_adapter adap; > + struct i2c_msg *msg; > + unsigned int msg_ptr; /* msg index */ Comment seems wrong. It looks to me as the msg buffer index. If so, the variable name is also confusing. > + unsigned int len; /* msg length */ > + int stop; > + struct completion msg_complete; > + > + unsigned int irq; That can be a local variable. > + void __iomem *regs; > + struct clk *clk; > + struct device *dev; > + spinlock_t lock; /* IRQ synchronization */ > + > + int err; > + enum hix5hd2_i2c_state state; > + enum hix5hd2_i2c_speed speed_mode; Where is this needed? > + > + /* Controller frequency */ > + unsigned int s_clock; > +}; > + > +static void hix5hd2_i2c_drv_setrate(struct hix5hd2_i2c *i2c) > +{ > + u32 rate, val; > + u32 sclh, scll, sysclock; > + > + /* close all i2c interrupt */ > + val = readl_relaxed(i2c->regs + HIX5I2C_CTRL); > + writel_relaxed(val & (~I2C_UNMASK_TOTAL), i2c->regs + HIX5I2C_CTRL); > + > + rate = i2c->s_clock; > + sysclock = clk_get_rate(i2c->clk); > + sclh = (sysclock / (rate * 2)) / 2 - 1; > + writel_relaxed(sclh, i2c->regs + HIX5I2C_SCL_H); > + scll = (sysclock / (rate * 2)) / 2 - 1; scll and sclh use the same formula? Have you measured the setup frequency with a scope? > + writel_relaxed(scll, i2c->regs + HIX5I2C_SCL_L); > + > + /* restore original interrupt*/ > + writel_relaxed(val, i2c->regs + HIX5I2C_CTRL); > + > + dev_dbg(i2c->dev, "%s: sysclock=%d, rate=%d, sclh=%d, scll=%d\n", > + __func__, sysclock, rate, sclh, scll); > +} > + > +static void hix5hd2_rw_over(struct hix5hd2_i2c *i2c) > +{ > + if (HIX5I2C_STAT_SND_STOP == i2c->state) To be honest, I don't like having the constant on the left side. It is far easier to read if they are on the right side. Plus, we have gcc warnings to prevent the "=" and "==" mistake. Please switch them around in this driver. > + dev_dbg(i2c->dev, "%s: rw and send stop over\n", __func__); > + else > + dev_dbg(i2c->dev, "%s: have not data to send\n", __func__); > + > + i2c->state = HIX5I2C_STAT_RW_SUCCESS; > + i2c->err = 0; > +} > + ... > + /* handle error */ > + if (int_status & I2C_ARBITRATE_INTR) { > + /*Bus error */ Space missing in front of "Bus". If this is an arbitration lost error, then please use -EAGAIN. Check Documentation/i2c/fault-codes for the convention. > + dev_dbg(i2c->dev, "ARB bus loss\n"); > + i2c->err = -ENXIO; > + i2c->state = HIX5I2C_STAT_RW_ERR; > + goto stop; > + } else if (int_status & I2C_ACK_INTR) { > + /* ack error */ > + dev_dbg(i2c->dev, "No ACK from device\n"); > + i2c->err = -ENXIO; > + i2c->state = HIX5I2C_STAT_RW_ERR; > + goto stop; > + } > + > +static void hix5hd2_i2c_message_start(struct hix5hd2_i2c *i2c, int stop) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&i2c->lock, flags); > + hix5hd2_i2c_clr_all_irq(i2c); > + hix5hd2_i2c_enable_irq(i2c); > + > + if (i2c->msg->flags & I2C_M_RD) > + writel_relaxed(i2c->msg->addr | HIX5I2C_READ_OPERATION, > + i2c->regs + HIX5I2C_TXR); > + else > + writel_relaxed(i2c->msg->addr & HIX5I2C_WRITE_OPERATION, > + i2c->regs + HIX5I2C_TXR); ??? Does this really work? i2c->msg->addr should be left shifted by 1? > + > + writel_relaxed(I2C_WRITE | I2C_START, i2c->regs + HIX5I2C_COM); > + spin_unlock_irqrestore(&i2c->lock, flags); > +} > + > +static int hix5hd2_i2c_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct hix5hd2_i2c *i2c; > + struct resource *mem; > + unsigned int op_clock; > + int ret; > + > + i2c = devm_kzalloc(&pdev->dev, sizeof(struct hix5hd2_i2c), GFP_KERNEL); > + if (!i2c) > + return -ENOMEM; > + > + if (of_property_read_u32(np, "clock-frequency", &op_clock)) { > + /* use default value */ > + i2c->speed_mode = HIX5I2C_HIG_SPD; > + i2c->s_clock = HIX5I2C_HS_TX_CLOCK; Please use 100kHz as the default. This is the speed devices should support. 400kHz is optional. And I think 100000 is easier to read than a define. > + } else { > + if (op_clock >= HIX5I2C_HS_TX_CLOCK) { > + i2c->speed_mode = HIX5I2C_HIG_SPD; > + i2c->s_clock = HIX5I2C_HS_TX_CLOCK; So, if the speed is bigger than 400kHz, it will be capped down to 400kHz? Is that true? Then, the user should be informed about that. > + } else { > + i2c->speed_mode = HIX5I2C_NORMAL_SPD; > + i2c->s_clock = op_clock; > + } > + } > + ... > + i2c->adap.owner = THIS_MODULE; > + i2c->adap.algo = &hix5hd2_i2c_algorithm; Single space as indentation. Thanks, Wolfram -- 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