On Wed, Feb 8, 2017 at 11:16 AM, Baoyou Xie <baoyou.xie@xxxxxxxxxx> wrote:
> This patch adds i2c controller driver for ZTE's zx2967 family.
>
> Signed-off-by: Baoyou Xie <baoyou.xie@xxxxxxxxxx>
> Reviewed-by: Shawn Guo <shawnguo@xxxxxxxxxx>
> +static int zx2967_i2c_empty_rx_fifo(struct zx2967_i2c_info *zx_i2c, u32 size)
> +{
> + u8 val[I2C_FIFO_MAX] = {0};
> + int i;
> + zx2967_i2c_readsb(zx_i2c, val, REG_DATA, size);
> + for (i = 0; i < size; i++) {
> + *zx_i2c->buf++ = val[i];
Sorry, I just realized that it might be completely wrong.
Here we advance the buf pointer. If it's allocated the question is
where the beginning pointer is kept?
Or what I missed?
> + zx_i2c->residue--;
> + if (zx_i2c->residue <= 0)
How it could be < 0 ?
> +static int zx2967_i2c_xfer_msg(struct zx2967_i2c_info *zx_i2c,
> + struct i2c_msg *msg)
> +{
> + if (msg->len == 0)
> + return -EINVAL;
> +
> + zx2967_i2c_flush_fifos(zx_i2c);
> +
> + zx_i2c->buf = msg->buf;
> + zx_i2c->residue = msg->len;
> + zx_i2c->access_cnt = msg->len / I2C_FIFO_MAX;
> + zx_i2c->msg_rd = (msg->flags & I2C_M_RD);
Redundant parens.
> +
> + if (zx_i2c->msg_rd)
> + return zx2967_i2c_xfer_read(zx_i2c);
> +
> + return zx2967_i2c_xfer_write(zx_i2c);
> +}
> + zx2967_i2c_writel(zx_i2c, (msgs->addr & 0x7f), REG_DEVADDR_L);
Ditto
> + zx2967_i2c_writel(zx_i2c, (msgs->addr >> 7) & 0x7, REG_DEVADDR_H);
Though it might be written like this (depends on what style Wolfram prefers)
zx2967_i2c_writel(zx_i2c, (msgs->addr >> 0) & 0x7f, REG_DEVADDR_L);
zx2967_i2c_writel(zx_i2c, (msgs->addr >> 7) & 0x07, REG_DEVADDR_H);
> + return num;
> +}
> +static void
> +zx2967_smbus_xfer_prepare(struct zx2967_i2c_info *zx_i2c, u16 addr,
> + char read_write, u8 command, int size,
> + union i2c_smbus_data *data)
> +{
> + u32 val;
> +
> + val = zx2967_i2c_readl(zx_i2c, REG_RDCONF);
> + val |= I2C_RFIFO_RESET;
> + zx2967_i2c_writel(zx_i2c, val, REG_RDCONF);
> + zx2967_i2c_writel(zx_i2c, (addr & 0x7f), REG_DEVADDR_L);
Redundant parens.
> + case I2C_SMBUS_WORD_DATA:
> + zx2967_i2c_writel(zx_i2c, command, REG_DATA);
> + if (read_write == I2C_SMBUS_WRITE) {
> + zx2967_i2c_writel(zx_i2c, (data->word >> 8), REG_DATA);
> + zx2967_i2c_writel(zx_i2c, (data->word & 0xff),
Btw does hw support byte access?
If so, _writeb() / _readb() would help a lot in this driver.
> + REG_DATA);
> + }
> + break;
> + }
> +}
> +static int zx2967_i2c_probe(struct platform_device *pdev)
> +{
> + struct zx2967_i2c_info *zx_i2c;
> + void __iomem *reg_base;
> + ret = clk_prepare_enable(clk);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to enable i2c_clk\n");
> + return ret;
> + }
If Dmitry's patch gets upstream earlier than yours I would consider to
switch to devm_clk_prepare_enable().
> +
> + ret = platform_get_irq(pdev, 0);
> + if (ret < 0)
> + return ret;
> + zx_i2c->irq = ret;
I would group this with below assignments...
> +
> + ret = device_property_read_u32(&pdev->dev, "clock-frequency",
> + &zx_i2c->clk_freq);
> + if (ret) {
> + dev_err(&pdev->dev, "missing clock-frequency");
> + return ret;
> + }
> +
> + zx_i2c->reg_base = reg_base;
> + zx_i2c->clk = clk;
...^^^
--
With Best Regards,
Andy Shevchenko
--
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
