On Sat, Mar 21, 2015 at 02:05:21PM +0800, Eddie Huang wrote:
> From: Xudong Chen <xudong.chen@xxxxxxxxxxxx>
>
> The mediatek SoCs have I2C controller that handle I2C transfer.
> This patch include common I2C bus driver.
> This driver is compatible with I2C controller on mt65xx/mt81xx.
>
> Signed-off-by: Xudong Chen <xudong.chen@xxxxxxxxxxxx>
> Signed-off-by: Liguo Zhang <liguo.zhang@xxxxxxxxxxxx>
> Signed-off-by: Eddie Huang <eddie.huang@xxxxxxxxxxxx>
> ---
> drivers/i2c/busses/Kconfig | 9 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-mt65xx.c | 705 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 715 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-mt65xx.c
>
> +
> +static void mtk_i2c_clock_disable(struct mtk_i2c *i2c)
> +{
> + if (i2c->have_pmic)
> + clk_disable_unprepare(i2c->clk_pmic);
> +
> + clk_disable_unprepare(i2c->clk_main);
> + clk_disable_unprepare(i2c->clk_dma);
> +}
> +
> +static inline void mtk_i2c_init_hw(struct mtk_i2c *i2c)
Please let the compiler decide whether to inline this or not.
> +{
> + mtk_i2c_writew(I2C_SOFT_RST, i2c, OFFSET_SOFTRESET);
> + /* Set ioconfig */
> + if (i2c->use_push_pull)
> + mtk_i2c_writew(I2C_IO_CONFIG_PUSH_PULL, i2c, OFFSET_IO_CONFIG);
> + else
> + mtk_i2c_writew(I2C_IO_CONFIG_OPEN_DRAIN, i2c, OFFSET_IO_CONFIG);
> +
> + if (i2c->platform_compat & COMPAT_MT6577)
> + mtk_i2c_writew(I2C_DCM_DISABLE, i2c, OFFSET_DCM_EN);
> +
> + mtk_i2c_writew(i2c->timing_reg, i2c, OFFSET_TIMING);
> + mtk_i2c_writew(i2c->high_speed_reg, i2c, OFFSET_HS);
> +}
> +
[...]
> + step_cnt = step_div;
> + sclk = hclk / (2 * sample_cnt * step_cnt);
> + if (sclk > khz) {
> + dev_dbg(i2c->dev, "%s mode: unsupported speed (%ldkhz)\n",
> + (mode == HS_MODE) ? "HS" : "ST/FT", (long int)khz);
> + return -ENOTSUPP;
> + }
> +
> + step_cnt--;
> + sample_cnt--;
> +
> + if (mode == HS_MODE) {
This is the only place where the HS_MODE is actually tested for.
Dropping this enum and using i2c->speed_hz > MAX_FS_MODE_SPEED directly
here would improve readability.
> + /* Set the hign speed mode register */
> + i2c->timing_reg = I2C_FS_TIME_INIT_VALUE;
> + i2c->high_speed_reg = I2C_TIME_DEFAULT_VALUE |
> + (sample_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 12 |
> + (step_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 8;
> + } else {
> + i2c->timing_reg =
> + (sample_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 8 |
> + (step_cnt & I2C_TIMING_STEP_DIV_MASK) << 0;
> + /* Disable the high speed transaction */
> + i2c->high_speed_reg = I2C_TIME_CLR_VALUE;
> + }
> +
> + return 0;
> +}
> +
[...]
> + if (i2c->speed_hz > 400000)
> + control_reg |= I2C_CONTROL_RS;
> + if (i2c->op == I2C_MASTER_WRRD)
> + control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
> + mtk_i2c_writew(control_reg, i2c, OFFSET_CONTROL);
> +
> + /* set start condition */
> + if (i2c->speed_hz <= 100000)
> + mtk_i2c_writew(I2C_ST_START_CON, i2c, OFFSET_EXT_CONF);
> + else
> + mtk_i2c_writew(I2C_FS_START_CON, i2c, OFFSET_EXT_CONF);
> +
> + if (~control_reg & I2C_CONTROL_RS)
> + mtk_i2c_writew(I2C_DELAY_LEN, i2c, OFFSET_DELAY_LEN);
speed <= 400000 here to make this more obvious?
> +
> + addr_reg = msgs->addr << 1;
> + if (i2c->op == I2C_MASTER_RD)
> + addr_reg |= 0x1;
> + mtk_i2c_writew(addr_reg, i2c, OFFSET_SLAVE_ADDR);
> +
> + /* Clear interrupt status */
> + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP,
> + i2c, OFFSET_INTR_STAT);
> + mtk_i2c_writew(I2C_FIFO_ADDR_CLR, i2c, OFFSET_FIFO_ADDR_CLR);
> +
> + /* Enable interrupt */
> + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP,
> + i2c, OFFSET_INTR_MASK);
Why do you enable/disable interrupts for each transfer? Enabling them
once and just acknowledge them in the interrupt handler should be
enough.
> + if (!i2c->trans_stop && tmo == 0) {
> + dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", msgs->addr);
> + mtk_i2c_init_hw(i2c);
> + return -ETIMEDOUT;
> + }
> +
> + spin_lock(&i2c->irqlock);
> + irqstat = i2c->irq_stat;
> + spin_unlock(&i2c->irqlock);
A plain spin_lock can't protect you against the interrupt handler, see
https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html.
You need at least spin_lock_irq().
Anyway, I think this spin_lock can be removed since it only protects the
irq_stat variable. This is written only in the interrupt handler and
then tested for in thread context. The thread waits for the interrupt
handler to be finished anyway.
> +static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id)
> +{
> + struct mtk_i2c *i2c = dev_id;
> +
> + /* Clear interrupt mask */
> + mtk_i2c_writew(~(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP),
> + i2c, OFFSET_INTR_MASK);
> +
> + spin_lock(&i2c->irqlock);
> + i2c->irq_stat = mtk_i2c_readw(i2c, OFFSET_INTR_STAT);
> + i2c->trans_stop = true;
A struct completion seems more what you want here. This makes the
trans_stop variable unnecessary (it contains no information anyway).
See the tegra driver as an example.
> + spin_unlock(&i2c->irqlock);
> + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR
> + | I2C_TRANSAC_COMP, i2c, OFFSET_INTR_STAT);
> + wake_up(&i2c->wait);
> +
> + return IRQ_HANDLED;
> +}
> +
[...]
> +static int mtk_i2c_probe(struct platform_device *pdev)
> +{
> + int ret = 0;
> + struct mtk_i2c *i2c;
> + struct clk *clk;
> + unsigned int clk_src_in_hz;
> + unsigned int clk_src_div;
> + struct resource *res;
> +
> + i2c = devm_kzalloc(&pdev->dev, sizeof(struct mtk_i2c), GFP_KERNEL);
sizeof(*i2c) instead. This will make it harder to allocate the memory
for a wrong struct size.
> + if (i2c == NULL)
> + return -ENOMEM;
> +
> + ret = mtk_i2c_parse_dt(pdev->dev.of_node, i2c, &clk_src_div);
> + if (ret)
> + return -EINVAL;
> +
> + i2c->platform_compat = mtk_get_device_prop(pdev);
> + if (i2c->have_pmic && (i2c->platform_compat & COMPAT_MT6577))
> + return -EINVAL;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + i2c->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(i2c->base))
> + return PTR_ERR(i2c->base);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +
> + i2c->pdmabase = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(i2c->pdmabase))
> + return PTR_ERR(i2c->pdmabase);
> +
> + i2c->irqnr = platform_get_irq(pdev, 0);
> + if (i2c->irqnr <= 0)
> + return -EINVAL;
i2c->irqnr is never used outside this function, so you can drop it from
struct mtk_i2c and make this a local variable.
Contrary to what Uwe said this variable should be a signed variable
because platform_get_irq() returns a signed integer which may contain a
negative error code. This will never be catched if you use an unsigned
variable.
Also you should forward the error instead of returning -EINVAL.
> +static int mtk_i2c_remove(struct platform_device *pdev)
> +{
> + struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&i2c->adap);
> + platform_set_drvdata(pdev, NULL);
This is unnecessary. This pointer is unused when no driver is bound to
the device and no driver will expect it to be valid in probe().
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
