* [PATCH v1 1/3] dt: bindings: add documentation for zx2967 family i2c controller @ 2017-01-20 8:43 Baoyou Xie 2017-01-20 8:43 ` [PATCH v1 2/3] MAINTAINERS: add zx2967 i2c controller driver to ARM ZTE architecture Baoyou Xie 2017-01-20 8:43 ` [PATCH v1 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family Baoyou Xie 0 siblings, 2 replies; 6+ messages in thread From: Baoyou Xie @ 2017-01-20 8:43 UTC (permalink / raw) To: jun.nie-QSEj5FYQhm4dnm+yROfE0A, wsa-z923LK4zBo2bacvFa/9K2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8 Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-i2c-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, shawnguo-DgEjT+Ai2ygdnm+yROfE0A, baoyou.xie-QSEj5FYQhm4dnm+yROfE0A, xie.baoyou-Th6q7B73Y6EnDS1+zs4M5A, chen.chaokai-Th6q7B73Y6EnDS1+zs4M5A, wang.qiang01-Th6q7B73Y6EnDS1+zs4M5A This patch adds dt-binding documentation for zx2967 family i2c controller. Signed-off-by: Baoyou Xie <baoyou.xie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> --- .../devicetree/bindings/i2c/i2c-zx2967.txt | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-zx2967.txt diff --git a/Documentation/devicetree/bindings/i2c/i2c-zx2967.txt b/Documentation/devicetree/bindings/i2c/i2c-zx2967.txt new file mode 100644 index 0000000..cb806d1 --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/i2c-zx2967.txt @@ -0,0 +1,22 @@ +ZTE zx2967 I2C controller + +Required properties: + - compatible: must be "zte,zx296718-i2c" + - reg: physical address and length of the device registers + - interrupts: a single interrupt specifier + - clocks: clock for the device + - #address-cells: should be <1> + - #size-cells: should be <0> + - clock-frequency: the desired I2C bus clock frequency. + +Examples: + + i2c@112000 { + compatible = "zte,zx296718-i2c"; + reg = <0x00112000 0x1000>; + interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&osc24m>; + #address-cells = <1> + #size-cells = <0>; + clock-frequency = <1600000>; + }; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v1 2/3] MAINTAINERS: add zx2967 i2c controller driver to ARM ZTE architecture 2017-01-20 8:43 [PATCH v1 1/3] dt: bindings: add documentation for zx2967 family i2c controller Baoyou Xie @ 2017-01-20 8:43 ` Baoyou Xie 2017-01-20 8:43 ` [PATCH v1 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family Baoyou Xie 1 sibling, 0 replies; 6+ messages in thread From: Baoyou Xie @ 2017-01-20 8:43 UTC (permalink / raw) To: jun.nie, wsa, robh+dt, mark.rutland Cc: devicetree, xie.baoyou, linux-kernel, chen.chaokai, linux-i2c, wang.qiang01, baoyou.xie, shawnguo, linux-arm-kernel Add the zx2967 i2c controller driver as maintained by ARM ZTE architecture maintainers, as they're parts of the core IP. Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org> --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 275c434..757c098 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1987,12 +1987,14 @@ L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) S: Maintained F: arch/arm/mach-zx/ F: drivers/clk/zte/ +F: drivers/i2c/busses/i2c-zx2967.c F: drivers/reset/reset-zx2967.c F: drivers/soc/zte/ F: drivers/thermal/zx* F: drivers/watchdog/zx2967_wdt.c F: Documentation/devicetree/bindings/arm/zte.txt F: Documentation/devicetree/bindings/clock/zx296702-clk.txt +F: Documentation/devicetree/bindings/i2c/i2c-zx2967.txt F: Documentation/devicetree/bindings/reset/zte,zx2967-reset.txt F: Documentation/devicetree/bindings/soc/zte/ F: Documentation/devicetree/bindings/thermal/zx* -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v1 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family 2017-01-20 8:43 [PATCH v1 1/3] dt: bindings: add documentation for zx2967 family i2c controller Baoyou Xie 2017-01-20 8:43 ` [PATCH v1 2/3] MAINTAINERS: add zx2967 i2c controller driver to ARM ZTE architecture Baoyou Xie @ 2017-01-20 8:43 ` Baoyou Xie 2017-01-20 21:45 ` Andy Shevchenko 2017-01-23 2:50 ` Shawn Guo 1 sibling, 2 replies; 6+ messages in thread From: Baoyou Xie @ 2017-01-20 8:43 UTC (permalink / raw) To: jun.nie, wsa, robh+dt, mark.rutland Cc: devicetree, xie.baoyou, linux-kernel, chen.chaokai, linux-i2c, wang.qiang01, baoyou.xie, shawnguo, linux-arm-kernel This patch adds i2c controller driver for ZTE's zx2967 family. Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org> --- drivers/i2c/busses/Kconfig | 8 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-zx2967.c | 645 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 654 insertions(+) create mode 100644 drivers/i2c/busses/i2c-zx2967.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index e4a603e..946d2b0 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -1246,4 +1246,12 @@ config I2C_OPAL This driver can also be built as a module. If so, the module will be called as i2c-opal. +config I2C_ZX2967 + bool "ZTE zx2967 I2C support" + depends on ARCH_ZX + default y + help + Selecting this option will add ZX I2C driver. + If unsure, say y. + endmenu diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index beb4809..16b2901 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -101,6 +101,7 @@ obj-$(CONFIG_I2C_XILINX) += i2c-xiic.o obj-$(CONFIG_I2C_XLR) += i2c-xlr.o obj-$(CONFIG_I2C_XLP9XX) += i2c-xlp9xx.o obj-$(CONFIG_I2C_RCAR) += i2c-rcar.o +obj-$(CONFIG_I2C_ZX2967) += i2c-zx2967.o # External I2C/SMBus adapter drivers obj-$(CONFIG_I2C_DIOLAN_U2C) += i2c-diolan-u2c.o diff --git a/drivers/i2c/busses/i2c-zx2967.c b/drivers/i2c/busses/i2c-zx2967.c new file mode 100644 index 0000000..6c2145b --- /dev/null +++ b/drivers/i2c/busses/i2c-zx2967.c @@ -0,0 +1,645 @@ +/* + * ZTE's zx2967 family i2c bus controller driver + * + * Copyright (C) 2017 ZTE Ltd. + * + * Author: Baoyou Xie <baoyou.xie@linaro.org> + * + * License terms: GNU General Public License (GPL) version 2 + */ + +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/pinctrl/consumer.h> +#include <linux/platform_device.h> + +#define BW1 (u32)1 + +#define REG_CMD 0x04 +#define REG_DEVADDR_H 0x0C +#define REG_DEVADDR_L 0x10 +#define REG_CLK_DIV_FS 0x14 +#define REG_CLK_DIV_HS 0x18 +#define REG_WRCONF 0x1C +#define REG_RDCONF 0x20 +#define REG_DATA 0x24 +#define REG_STAT 0x28 + +#define I2C_STOP 0 +#define I2C_MASTER (1 << 0) +#define I2C_ADDR_MODE_TEN (1 << 1) +#define I2C_IRQ_MSK_ENABLE (1 << 3) +#define I2C_RW_READ (1 << 4) +#define I2C_CMB_RW_EN (1 << 5) +#define I2C_START (1 << 6) +#define I2C_ADDR_MODE_TEN (1 << 1) + +#define I2C_WFIFO_RESET (1 << 7) +#define I2C_RFIFO_RESET (1 << 7) + +#define I2C_IRQ_ACK_CLEAR (1 << 7) +#define I2C_INT_MASK 0x3f + +#define I2C_TRANS_DONE (1 << 0) +#define I2C_ERROR_DEVICE (1 << 1) +#define I2C_ERROR_DATA (1 << 2) +#define I2C_ERROR_MASK (I2C_ERROR_DATA | I2C_ERROR_DEVICE) + +#define I2C_SR_BUSY (1 << 6) + +#define I2C_SR_EDEVICE (1 << 1) +#define I2C_SR_EDATA (1 << 2) + +#define I2C_FIFO_MAX 16 + +#define I2C_TIMEOUT msecs_to_jiffies(1000) + +struct zx2967_i2c_info { + spinlock_t lock; + struct device *dev; + struct i2c_adapter adap; + struct clk *clk; + struct completion complete; + u32 clk_freq; + struct pinctrl *pin_scl; + struct pinctrl *pin_sda; + void __iomem *reg_base; + size_t residue; + int id; + int irq; + int msg_rd; + u8 *buf; + u8 access_cnt; + bool is_suspended; +}; + +static void zx2967_i2c_writel(struct zx2967_i2c_info *zx_i2c, + u32 val, unsigned long reg) +{ + writel_relaxed(val, zx_i2c->reg_base + reg); +} + +static u32 zx2967_i2c_readl(struct zx2967_i2c_info *zx_i2c, unsigned long reg) +{ + return readl_relaxed(zx_i2c->reg_base + reg); +} + +static void zx2967_i2c_writesb(struct zx2967_i2c_info *zx_i2c, + void *data, unsigned long reg, int len) +{ + writesb(zx_i2c->reg_base + reg, data, len); +} + +static void zx2967_i2c_readsb(struct zx2967_i2c_info *zx_i2c, + void *data, unsigned long reg, int len) +{ + readsb(zx_i2c->reg_base + reg, data, len); +} + +static void zx2967_i2c_start_ctrl(struct zx2967_i2c_info *zx_i2c) +{ + u32 status; + u32 ctl; + + status = zx2967_i2c_readl(zx_i2c, REG_STAT); + status |= I2C_IRQ_ACK_CLEAR; + zx2967_i2c_writel(zx_i2c, status, REG_STAT); + + ctl = zx2967_i2c_readl(zx_i2c, REG_CMD); + if (zx_i2c->msg_rd) + ctl |= I2C_RW_READ; + else + ctl &= ~I2C_RW_READ; + ctl &= ~I2C_CMB_RW_EN; + ctl |= I2C_START; + zx2967_i2c_writel(zx_i2c, ctl, REG_CMD); +} + +static int zx2967_i2c_flush_fifos(struct zx2967_i2c_info *zx_i2c) +{ + u32 val = 0; + u32 offset = 0; + + if (zx_i2c->msg_rd) { + offset = REG_RDCONF; + val = I2C_RFIFO_RESET; + } else { + offset = REG_WRCONF; + val = I2C_WFIFO_RESET; + } + + val |= zx2967_i2c_readl(zx_i2c, offset); + zx2967_i2c_writel(zx_i2c, val, offset); + + return 0; +} + +static int zx2967_i2c_empty_rx_fifo(struct zx2967_i2c_info *zx_i2c, u32 size) +{ + u8 val[I2C_FIFO_MAX] = {0}; + int i = 0; + + if (size == 0) { + dev_err(zx_i2c->dev, "size is invalid\n"); + return -EINVAL; + } + + zx2967_i2c_readsb(zx_i2c, val, REG_DATA, size); + for (i = 0; i < size; i++) { + *(zx_i2c->buf++) = val[i]; + zx_i2c->residue--; + if (zx_i2c->residue <= 0) + break; + } + + barrier(); + + return 0; +} + +static int zx2967_i2c_fill_tx_fifo(struct zx2967_i2c_info *zx_i2c) +{ + u8 *buf = zx_i2c->buf; + size_t residue = zx_i2c->residue; + + if (residue == 0) { + dev_err(zx_i2c->dev, "residue is %d\n", (int)residue); + return -EINVAL; + } + + if (residue <= I2C_FIFO_MAX) { + zx2967_i2c_writesb(zx_i2c, buf, REG_DATA, residue); + + /* Again update before writing to FIFO to make sure isr sees. */ + zx_i2c->residue = 0; + zx_i2c->buf = NULL; + } else { + zx2967_i2c_writesb(zx_i2c, buf, REG_DATA, I2C_FIFO_MAX); + zx_i2c->residue -= I2C_FIFO_MAX; + zx_i2c->buf += I2C_FIFO_MAX; + } + + barrier(); + + return 0; +} + +static int zx2967_i2c_reset_hardware(struct zx2967_i2c_info *zx_i2c) +{ + u32 val; + u32 clk_div; + u32 status; + + val = I2C_MASTER | I2C_IRQ_MSK_ENABLE; + zx2967_i2c_writel(zx_i2c, val, REG_CMD); + + clk_div = clk_get_rate(zx_i2c->clk) / zx_i2c->clk_freq - 1; + zx2967_i2c_writel(zx_i2c, clk_div, REG_CLK_DIV_FS); + zx2967_i2c_writel(zx_i2c, clk_div, REG_CLK_DIV_HS); + + zx2967_i2c_writel(zx_i2c, I2C_FIFO_MAX - 1, REG_WRCONF); + zx2967_i2c_writel(zx_i2c, I2C_FIFO_MAX - 1, REG_RDCONF); + zx2967_i2c_writel(zx_i2c, 1, REG_RDCONF); + + if (zx2967_i2c_flush_fifos(zx_i2c)) + return -ETIMEDOUT; + + status = zx2967_i2c_readl(zx_i2c, REG_STAT); + if (status & (I2C_SR_BUSY)) + return -EBUSY; + if (status & (I2C_SR_EDEVICE | I2C_SR_EDATA)) + return -EIO; + + enable_irq(zx_i2c->irq); + + return 0; +} + +static void zx2967_i2c_isr_clr(struct zx2967_i2c_info *zx_i2c) +{ + u32 status; + + status = zx2967_i2c_readl(zx_i2c, REG_STAT); + status |= I2C_IRQ_ACK_CLEAR; + zx2967_i2c_writel(zx_i2c, status, REG_STAT); +} + +static irqreturn_t zx2967_i2c_isr(int irq, void *dev_id) +{ + u32 status; + struct zx2967_i2c_info *zx_i2c = (struct zx2967_i2c_info *)dev_id; + unsigned long flags; + + spin_lock_irqsave(&zx_i2c->lock, flags); + + status = zx2967_i2c_readl(zx_i2c, REG_STAT) & I2C_INT_MASK; + zx2967_i2c_isr_clr(zx_i2c); + + if (status & I2C_ERROR_MASK) { + spin_unlock_irqrestore(&zx_i2c->lock, flags); + return IRQ_HANDLED; + } + + if (status & I2C_TRANS_DONE) + complete(&zx_i2c->complete); + + spin_unlock_irqrestore(&zx_i2c->lock, flags); + + return IRQ_HANDLED; +} + +static void zx2967_enable_tenbit(struct zx2967_i2c_info *zx_i2c, __u16 addr) +{ + u16 val = ((addr >> 7) & 0x7); + + if (val > 0) { + zx2967_i2c_writel(zx_i2c, val, REG_DEVADDR_H); + val = (zx2967_i2c_readl(zx_i2c, REG_CMD)) | I2C_ADDR_MODE_TEN; + zx2967_i2c_writel(zx_i2c, val, REG_CMD); + } +} + +static int zx2967_i2c_xfer_msg(struct zx2967_i2c_info *zx_i2c, + struct i2c_msg *msg) +{ + unsigned long time_left = 0; + unsigned int i = 0; + + 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); + + if (zx_i2c->msg_rd) { + for (i = 0; i < zx_i2c->access_cnt; i++) { + reinit_completion(&zx_i2c->complete); + zx2967_i2c_writel(zx_i2c, I2C_FIFO_MAX - 1, + REG_RDCONF); + zx2967_i2c_start_ctrl(zx_i2c); + + time_left = wait_for_completion_timeout( + &zx_i2c->complete, I2C_TIMEOUT); + if (time_left == 0) { + dev_err(zx_i2c->dev, + "read i2c transfer timed out\n"); + disable_irq(zx_i2c->irq); + zx2967_i2c_reset_hardware(zx_i2c); + return -EIO; + } + + zx2967_i2c_empty_rx_fifo(zx_i2c, I2C_FIFO_MAX); + } + + if (zx_i2c->residue < I2C_FIFO_MAX) { + reinit_completion(&zx_i2c->complete); + zx2967_i2c_writel(zx_i2c, zx_i2c->residue - 1, + REG_RDCONF); + zx2967_i2c_start_ctrl(zx_i2c); + time_left = wait_for_completion_timeout( + &zx_i2c->complete, I2C_TIMEOUT); + if (time_left == 0) { + dev_err(zx_i2c->dev, + "read i2c transfer timed out\n"); + disable_irq(zx_i2c->irq); + zx2967_i2c_reset_hardware(zx_i2c); + return -EIO; + } + + zx2967_i2c_empty_rx_fifo(zx_i2c, zx_i2c->residue); + } + + zx_i2c->residue = 0; + zx_i2c->access_cnt = 0; + } else { + reinit_completion(&zx_i2c->complete); + zx2967_i2c_fill_tx_fifo(zx_i2c); + zx2967_i2c_start_ctrl(zx_i2c); + + for (i = 0; i < zx_i2c->access_cnt; i++) { + time_left = wait_for_completion_timeout( + &zx_i2c->complete, I2C_TIMEOUT); + if (time_left == 0) { + dev_err(zx_i2c->dev, + "write i2c transfer timed out\n"); + disable_irq(zx_i2c->irq); + zx2967_i2c_reset_hardware(zx_i2c); + return -EIO; + } + + if (zx_i2c->residue != 0) { + reinit_completion(&zx_i2c->complete); + zx2967_i2c_fill_tx_fifo(zx_i2c); + zx2967_i2c_start_ctrl(zx_i2c); + } + } + } + + return 0; +} + +static int zx2967_i2c_xfer(struct i2c_adapter *adap, + struct i2c_msg *msgs, int num) +{ + int i; + int ret = 0; + struct zx2967_i2c_info *zx_i2c = i2c_get_adapdata(adap); + + if (zx_i2c->is_suspended) + return -EBUSY; + + zx2967_i2c_writel(zx_i2c, (msgs->addr & 0x7f), REG_DEVADDR_L); + zx2967_i2c_writel(zx_i2c, (msgs->addr >> 7) & 0x7, REG_DEVADDR_H); + if (zx2967_i2c_readl(zx_i2c, REG_DEVADDR_H) > 0) + zx2967_enable_tenbit(zx_i2c, msgs->addr); + + for (i = 0; i < num; i++) { + ret = zx2967_i2c_xfer_msg(zx_i2c, &msgs[i]); + if (num > 1) + usleep_range(1000, 2000); + if (ret) + break; + } + + return ret ?: i; +} + +static int zx2967_smbus_xfer(struct i2c_adapter *adap, u16 addr, + unsigned short flags, char read_write, + u8 command, int size, union i2c_smbus_data *data) +{ + struct zx2967_i2c_info *zx_i2c = i2c_get_adapdata(adap); + u32 val; + u8 buf[2]; + unsigned long time_left; + + 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); + + zx2967_enable_tenbit(zx_i2c, addr); + val = zx2967_i2c_readl(zx_i2c, REG_CMD); + val &= ~I2C_RW_READ; + zx2967_i2c_writel(zx_i2c, val, REG_CMD); + + switch (size) { + case I2C_SMBUS_QUICK: + read_write = I2C_SMBUS_WRITE; + break; + case I2C_SMBUS_BYTE: + zx2967_i2c_writel(zx_i2c, command, REG_DATA); + break; + case I2C_SMBUS_BYTE_DATA: + zx2967_i2c_writel(zx_i2c, command, REG_DATA); + + if (read_write == I2C_SMBUS_WRITE) + zx2967_i2c_writel(zx_i2c, data->byte, REG_DATA); + + break; + case I2C_SMBUS_WORD_DATA: + zx2967_i2c_writel(zx_i2c, command, REG_DATA); + zx2967_i2c_writel(zx_i2c, (data->word & 0xff), 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), REG_DATA); + } + break; + + default: + dev_warn(&adap->dev, "Unsupported transaction %d\n", size); + return -EOPNOTSUPP; + } + + if (read_write == I2C_SMBUS_WRITE) { + reinit_completion(&zx_i2c->complete); + val = zx2967_i2c_readl(zx_i2c, REG_CMD); + val |= I2C_START; + zx2967_i2c_writel(zx_i2c, val, REG_CMD); + + time_left = wait_for_completion_timeout( + &zx_i2c->complete, I2C_TIMEOUT); + if (time_left == 0) { + dev_err(zx_i2c->dev, "i2c write transfer timed out\n"); + disable_irq(zx_i2c->irq); + zx2967_i2c_reset_hardware(zx_i2c); + return -EIO; + } + + return 0; + } + + reinit_completion(&zx_i2c->complete); + + val = zx2967_i2c_readl(zx_i2c, REG_CMD); + val |= I2C_CMB_RW_EN; + zx2967_i2c_writel(zx_i2c, val, REG_CMD); + + val = zx2967_i2c_readl(zx_i2c, REG_CMD); + val |= I2C_START; + zx2967_i2c_writel(zx_i2c, val, REG_CMD); + + time_left = wait_for_completion_timeout(&zx_i2c->complete, + I2C_TIMEOUT); + if (time_left == 0) { + dev_err(zx_i2c->dev, "i2c read transfer timed out\n"); + disable_irq(zx_i2c->irq); + zx2967_i2c_reset_hardware(zx_i2c); + return -EIO; + } + + usleep_range(1000, 2000); + switch (size) { + case I2C_SMBUS_BYTE: + case I2C_SMBUS_BYTE_DATA: + val = zx2967_i2c_readl(zx_i2c, REG_DATA); + data->byte = val; + break; + case I2C_SMBUS_WORD_DATA: + case I2C_SMBUS_PROC_CALL: + buf[0] = zx2967_i2c_readl(zx_i2c, REG_DATA); + buf[1] = zx2967_i2c_readl(zx_i2c, REG_DATA); + data->word = (buf[0] << 8) | buf[1]; + break; + default: + dev_warn(&adap->dev, "Unsupported transaction %d\n", size); + return -EOPNOTSUPP; + } + + return 0; +} + +static u32 zx2967_i2c_func(struct i2c_adapter *adap) +{ + return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | + I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | + I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_PROC_CALL | + I2C_FUNC_I2C | I2C_FUNC_SMBUS_I2C_BLOCK; +} + +#ifdef CONFIG_PM +static int zx2967_i2c_suspend(struct device *dev) +{ + struct zx2967_i2c_info *zx_i2c = dev_get_drvdata(dev); + + zx_i2c->is_suspended = true; + if (__clk_get_enable_count(zx_i2c->clk) == 1) + clk_disable_unprepare(zx_i2c->clk); + + return 0; +} + +static int zx2967_i2c_resume(struct device *dev) +{ + struct zx2967_i2c_info *zx_i2c = dev_get_drvdata(dev); + + zx_i2c->is_suspended = false; + clk_prepare_enable(zx_i2c->clk); + + return 0; +} +static const struct dev_pm_ops zx2967_i2c_dev_pm_ops = { + .suspend = zx2967_i2c_suspend, + .resume = zx2967_i2c_resume, +}; +#define ZX2967_I2C_DEV_PM_OPS (&zx2967_i2c_dev_pm_ops) +#else +#define ZX2967_I2C_DEV_PM_OPS NULL +#endif + +static const struct i2c_algorithm zx2967_i2c_algo = { + .master_xfer = zx2967_i2c_xfer, + .smbus_xfer = zx2967_smbus_xfer, + .functionality = zx2967_i2c_func, +}; + +static const struct of_device_id zx2967_i2c_of_match[] = { + { .compatible = "zte,zx296718-i2c", }, + {}, +}; +MODULE_DEVICE_TABLE(of, zx2967_i2c_of_match); + +static int zx2967_i2c_probe(struct platform_device *pdev) +{ + struct resource *res; + struct zx2967_i2c_info *zx_i2c = NULL; + struct clk *clk; + void __iomem *reg_base; + int ret = 0; + + zx_i2c = devm_kzalloc(&pdev->dev, sizeof(*zx_i2c), GFP_KERNEL); + if (!zx_i2c) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_err(&pdev->dev, "missing io resource\n"); + return -EINVAL; + } + + reg_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(reg_base)) + return PTR_ERR(reg_base); + + clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(clk)) { + dev_err(&pdev->dev, "missing controller clock"); + return PTR_ERR(clk); + } + + ret = clk_prepare_enable(clk); + if (ret) { + dev_err(&pdev->dev, "failed to enable i2c_clk\n"); + return ret; + } + + of_property_read_u32(pdev->dev.of_node, + "clock-frequency", &zx_i2c->clk_freq); + zx_i2c->reg_base = reg_base; + zx_i2c->clk = clk; + zx_i2c->irq = platform_get_irq(pdev, 0); + zx_i2c->id = pdev->id; + zx_i2c->dev = &pdev->dev; + + zx_i2c->pin_scl = devm_pinctrl_get_select(&pdev->dev, "scl"); + if (IS_ERR(zx_i2c->pin_scl)) + dev_info(&pdev->dev, "scl pin is not specified in dts\n"); + + zx_i2c->pin_sda = devm_pinctrl_get_select(&pdev->dev, "sda"); + if (IS_ERR(zx_i2c->pin_sda)) + dev_info(&pdev->dev, "sda pin is not specified in dts\n"); + + spin_lock_init(&zx_i2c->lock); + init_completion(&zx_i2c->complete); + platform_set_drvdata(pdev, zx_i2c); + + ret = zx2967_i2c_reset_hardware(zx_i2c); + if (ret) { + dev_err(&pdev->dev, "failed to initialize i2c controller\n"); + goto unprepare_clk; + } + + ret = devm_request_irq(&pdev->dev, zx_i2c->irq, + zx2967_i2c_isr, 0, dev_name(&pdev->dev), zx_i2c); + if (ret) { + dev_err(&pdev->dev, "failed to request irq %i\n", zx_i2c->irq); + goto unprepare_clk; + } + + i2c_set_adapdata(&zx_i2c->adap, zx_i2c); + zx_i2c->adap.owner = THIS_MODULE; + zx_i2c->adap.class = I2C_CLASS_DEPRECATED; + strlcpy(zx_i2c->adap.name, "zx2967 i2c adapter", + sizeof(zx_i2c->adap.name)); + zx_i2c->adap.algo = &zx2967_i2c_algo; + zx_i2c->adap.dev.parent = &pdev->dev; + zx_i2c->adap.nr = pdev->id; + zx_i2c->adap.dev.of_node = pdev->dev.of_node; + + ret = i2c_add_numbered_adapter(&zx_i2c->adap); + if (ret) { + dev_err(&pdev->dev, "failed to add zx2967 i2c adapter\n"); + goto unprepare_clk; + } + + return 0; + +unprepare_clk: + clk_unprepare(zx_i2c->clk); + return ret; +} + +static int zx2967_i2c_remove(struct platform_device *pdev) +{ + struct zx2967_i2c_info *zx_i2c = platform_get_drvdata(pdev); + + i2c_del_adapter(&zx_i2c->adap); + clk_unprepare(zx_i2c->clk); + + return 0; +} + +static struct platform_driver zx2967_i2c_driver = { + .probe = zx2967_i2c_probe, + .remove = zx2967_i2c_remove, + .driver = { + .name = "zx2967_i2c", + .of_match_table = zx2967_i2c_of_match, + .pm = ZX2967_I2C_DEV_PM_OPS, + }, +}; +module_platform_driver(zx2967_i2c_driver); + +MODULE_AUTHOR("Baoyou Xie <baoyou.xie@linaro.org>"); +MODULE_DESCRIPTION("ZTE zx2967 I2C Bus Controller driver"); +MODULE_LICENSE("GPL v2"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family 2017-01-20 8:43 ` [PATCH v1 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family Baoyou Xie @ 2017-01-20 21:45 ` Andy Shevchenko 2017-01-22 10:12 ` Baoyou Xie 2017-01-23 2:50 ` Shawn Guo 1 sibling, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2017-01-20 21:45 UTC (permalink / raw) To: Baoyou Xie Cc: jun.nie, Wolfram Sang, Rob Herring, Mark Rutland, linux-arm Mailing List, linux-i2c, devicetree, linux-kernel@vger.kernel.org, shawnguo, xie.baoyou, chen.chaokai, wang.qiang01 On Fri, Jan 20, 2017 at 10:43 AM, Baoyou Xie <baoyou.xie@linaro.org> wrote: > This patch adds i2c controller driver for ZTE's zx2967 family. > > +config I2C_ZX2967 > + bool "ZTE zx2967 I2C support" No module? > + depends on ARCH_ZX > + default y Would architecture selects it instead? > + help > + Selecting this option will add ZX I2C driver. > + If unsure, say y. Two sentences about the same in spite 'default y'. > +#define BW1 (u32)1 Do you really need casting? > +#define I2C_STOP 0 > +#define I2C_MASTER (1 << 0) BIT(0) ? And so further. > +#define I2C_INT_MASK 0x3f GENMASK(6, 0) ? > +#define I2C_ERROR_MASK (I2C_ERROR_DATA | I2C_ERROR_DEVICE) Ditto. GENMASK() with explicit numbers. > +static int zx2967_i2c_flush_fifos(struct zx2967_i2c_info *zx_i2c) > +{ > + u32 val = 0; > + u32 offset = 0; Useless assignments. > + > + if (zx_i2c->msg_rd) { > + offset = REG_RDCONF; > + val = I2C_RFIFO_RESET; > + } else { > + offset = REG_WRCONF; > + val = I2C_WFIFO_RESET; > + } > +static int zx2967_i2c_empty_rx_fifo(struct zx2967_i2c_info *zx_i2c, u32 size) > +{ > + u8 val[I2C_FIFO_MAX] = {0}; > + int i = 0; Ditto. > + > + if (size == 0) { Would it be the case? > + dev_err(zx_i2c->dev, "size is invalid\n"); > + return -EINVAL; > + } And what happens even in that case if you just remove this check? > + > + zx2967_i2c_readsb(zx_i2c, val, REG_DATA, size); > + for (i = 0; i < size; i++) { > + *(zx_i2c->buf++) = val[i]; > + zx_i2c->residue--; > + if (zx_i2c->residue <= 0) > + break; > +static int zx2967_i2c_fill_tx_fifo(struct zx2967_i2c_info *zx_i2c) > +{ > + u8 *buf = zx_i2c->buf; > + size_t residue = zx_i2c->residue; > + > + if (residue == 0) { > + dev_err(zx_i2c->dev, "residue is %d\n", (int)residue); > + return -EINVAL; > + } Ditto. > +static void zx2967_enable_tenbit(struct zx2967_i2c_info *zx_i2c, __u16 addr) > +{ > + u16 val = ((addr >> 7) & 0x7); Redundant parens. > + if (val > 0) { if (!val) return; > +static int zx2967_i2c_xfer_msg(struct zx2967_i2c_info *zx_i2c, > + struct i2c_msg *msg > +{ > + unsigned long time_left = 0; > + unsigned int i = 0; Redundant assignment. > + > + if (msg->len == 0) if (!msg...) > + return -EINVAL; > +static int zx2967_i2c_xfer(struct i2c_adapter *adap, > + struct i2c_msg *msgs, int num) > +{ > + int i; > + int ret = 0; Redundant. See below. > + struct zx2967_i2c_info *zx_i2c = i2c_get_adapdata(adap); Reverse tree, please. > + > + if (zx_i2c->is_suspended) > + return -EBUSY; > + > + zx2967_i2c_writel(zx_i2c, (msgs->addr & 0x7f), REG_DEVADDR_L); > + zx2967_i2c_writel(zx_i2c, (msgs->addr >> 7) & 0x7, REG_DEVADDR_H); > + if (zx2967_i2c_readl(zx_i2c, REG_DEVADDR_H) > 0) > + zx2967_enable_tenbit(zx_i2c, msgs->addr); > + > + for (i = 0; i < num; i++) { int ret; > + ret = zx2967_i2c_xfer_msg(zx_i2c, &msgs[i]); > + if (num > 1) > + usleep_range(1000, 2000); > + if (ret) > + break; return ret; > + } > + > + return ret ?: i; return i; And looking now, how i can't be num here? > +} > +{ > + struct zx2967_i2c_info *zx_i2c = i2c_get_adapdata(adap); > + u32 val; > + u8 buf[2]; > + unsigned long time_left; Reverse tree. > + usleep_range(1000, 2000); > + switch (size) { > + case I2C_SMBUS_BYTE: > + case I2C_SMBUS_BYTE_DATA: > + val = zx2967_i2c_readl(zx_i2c, REG_DATA); > + data->byte = val; > + break; > + case I2C_SMBUS_WORD_DATA: > + case I2C_SMBUS_PROC_CALL: > + buf[0] = zx2967_i2c_readl(zx_i2c, REG_DATA); > + buf[1] = zx2967_i2c_readl(zx_i2c, REG_DATA); > + data->word = (buf[0] << 8) | buf[1]; > + break; > + default: > + dev_warn(&adap->dev, "Unsupported transaction %d\n", size); > + return -EOPNOTSUPP; > + } > + > + return 0; > +} I would revisit those several loooong functions for splitting/refactoring. > + > +static u32 zx2967_i2c_func(struct i2c_adapter *adap) > +{ > + return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | > + I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | > + I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_PROC_CALL | > + I2C_FUNC_I2C | I2C_FUNC_SMBUS_I2C_BLOCK; > +} It's just #define XXX YY1 | YY2 | ... > +#ifdef CONFIG_PM > +static int zx2967_i2c_suspend(struct device *dev) Switch to __maybe_unuse. > +static int zx2967_i2c_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + struct zx2967_i2c_info *zx_i2c = NULL; Redundant assignment. > + struct clk *clk; > + void __iomem *reg_base; Reverse tree for all above until top of this function. > + int ret = 0; Redundant assignment. > + > + zx_i2c = devm_kzalloc(&pdev->dev, sizeof(*zx_i2c), GFP_KERNEL); > + if (!zx_i2c) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(&pdev->dev, "missing io resource\n"); > + return -EINVAL; > + } > + Completely redundant lines of code. > + reg_base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(reg_base)) > + return PTR_ERR(reg_base); > + > + clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(clk)) { > + dev_err(&pdev->dev, "missing controller clock"); > + return PTR_ERR(clk); > + } > + > + ret = clk_prepare_enable(clk); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable i2c_clk\n"); > + return ret; > + } > + > + of_property_read_u32(pdev->dev.of_node, > + "clock-frequency", &zx_i2c->clk_freq); devce_property_read_uXX(); > + zx_i2c->reg_base = reg_base; > + zx_i2c->clk = clk; > + zx_i2c->irq = platform_get_irq(pdev, 0); Is this optional? Otherwise has to be checked. > + zx_i2c->id = pdev->id; > + zx_i2c->dev = &pdev->dev; > + > + zx_i2c->pin_scl = devm_pinctrl_get_select(&pdev->dev, "scl"); > + if (IS_ERR(zx_i2c->pin_scl)) > + dev_info(&pdev->dev, "scl pin is not specified in dts\n"); > + > + zx_i2c->pin_sda = devm_pinctrl_get_select(&pdev->dev, "sda"); > + if (IS_ERR(zx_i2c->pin_sda)) > + dev_info(&pdev->dev, "sda pin is not specified in dts\n"); > + > + spin_lock_init(&zx_i2c->lock); > + init_completion(&zx_i2c->complete); > + platform_set_drvdata(pdev, zx_i2c); > + > + ret = zx2967_i2c_reset_hardware(zx_i2c); > + if (ret) { > + dev_err(&pdev->dev, "failed to initialize i2c controller\n"); > + goto unprepare_clk; > + } > + > + ret = devm_request_irq(&pdev->dev, zx_i2c->irq, > + zx2967_i2c_isr, 0, dev_name(&pdev->dev), zx_i2c); > + if (ret) { > + dev_err(&pdev->dev, "failed to request irq %i\n", zx_i2c->irq); > + goto unprepare_clk; > + } > + > + i2c_set_adapdata(&zx_i2c->adap, zx_i2c); > + zx_i2c->adap.owner = THIS_MODULE; Do you need this? > + zx_i2c->adap.class = I2C_CLASS_DEPRECATED; > + strlcpy(zx_i2c->adap.name, "zx2967 i2c adapter", > + sizeof(zx_i2c->adap.name)); > + zx_i2c->adap.algo = &zx2967_i2c_algo; > + zx_i2c->adap.dev.parent = &pdev->dev; > + zx_i2c->adap.nr = pdev->id; > + zx_i2c->adap.dev.of_node = pdev->dev.of_node; > + > + ret = i2c_add_numbered_adapter(&zx_i2c->adap); > + if (ret) { > + dev_err(&pdev->dev, "failed to add zx2967 i2c adapter\n"); > + goto unprepare_clk; > + } > + > + return 0; > + > +unprepare_clk: err_clk_unprepare: > + clk_unprepare(zx_i2c->clk); Perhaps clk_disable_unprepare(); > + return ret; > +} > + > +static int zx2967_i2c_remove(struct platform_device *pdev) > +{ > + struct zx2967_i2c_info *zx_i2c = platform_get_drvdata(pdev); > + > + i2c_del_adapter(&zx_i2c->adap); > + clk_unprepare(zx_i2c->clk); Ditto. > + > + return 0; > +} > +MODULE_AUTHOR("Baoyou Xie <baoyou.xie@linaro.org>"); > +MODULE_DESCRIPTION("ZTE zx2967 I2C Bus Controller driver"); > +MODULE_LICENSE("GPL v2"); When bool in Kconfig this all redundant. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family 2017-01-20 21:45 ` Andy Shevchenko @ 2017-01-22 10:12 ` Baoyou Xie 0 siblings, 0 replies; 6+ messages in thread From: Baoyou Xie @ 2017-01-22 10:12 UTC (permalink / raw) To: Andy Shevchenko Cc: Jun Nie, Wolfram Sang, Rob Herring, Mark Rutland, linux-arm Mailing List, linux-i2c, devicetree, linux-kernel@vger.kernel.org, Shawn Guo, xie.baoyou, chen.chaokai, wang.qiang01 [-- Attachment #1: Type: text/plain, Size: 10037 bytes --] On 21 January 2017 at 05:45, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jan 20, 2017 at 10:43 AM, Baoyou Xie <baoyou.xie@linaro.org> > wrote: > > This patch adds i2c controller driver for ZTE's zx2967 family. > > > > > +config I2C_ZX2967 > > + bool "ZTE zx2967 I2C support" > > No module? > > > + depends on ARCH_ZX > > + default y > > Would architecture selects it instead? > > of cause, It's ok that architecture selects it. but for subject to other i2c drivers, I suggest to remain it. > > + help > > + Selecting this option will add ZX I2C driver. > > + If unsure, say y. > > Two sentences about the same in spite 'default y'. > > > +#define BW1 (u32)1 > > Do you really need casting? > > > +#define I2C_STOP 0 > > +#define I2C_MASTER (1 << 0) > > BIT(0) ? > And so further. > > > +#define I2C_INT_MASK 0x3f > > GENMASK(6, 0) ? > > > > +#define I2C_ERROR_MASK (I2C_ERROR_DATA | > I2C_ERROR_DEVICE) > > Ditto. > GENMASK() with explicit numbers. > > > +static int zx2967_i2c_flush_fifos(struct zx2967_i2c_info *zx_i2c) > > +{ > > + u32 val = 0; > > + u32 offset = 0; > > Useless assignments. > > > + > > + if (zx_i2c->msg_rd) { > > + offset = REG_RDCONF; > > + val = I2C_RFIFO_RESET; > > + } else { > > + offset = REG_WRCONF; > > + val = I2C_WFIFO_RESET; > > + } > > > +static int zx2967_i2c_empty_rx_fifo(struct zx2967_i2c_info *zx_i2c, > u32 size) > > +{ > > > + u8 val[I2C_FIFO_MAX] = {0}; > > + int i = 0; > > Ditto. > > > + > > + if (size == 0) { > > Would it be the case? > > > + dev_err(zx_i2c->dev, "size is invalid\n"); > > + return -EINVAL; > > + } > > And what happens even in that case if you just remove this check? > > en, thanks for you find this big bug:) > > + > > + zx2967_i2c_readsb(zx_i2c, val, REG_DATA, size); > > + for (i = 0; i < size; i++) { > > + *(zx_i2c->buf++) = val[i]; > > + zx_i2c->residue--; > > + if (zx_i2c->residue <= 0) > > + break; > > > +static int zx2967_i2c_fill_tx_fifo(struct zx2967_i2c_info *zx_i2c) > > +{ > > + u8 *buf = zx_i2c->buf; > > + size_t residue = zx_i2c->residue; > > + > > + if (residue == 0) { > > + dev_err(zx_i2c->dev, "residue is %d\n", (int)residue); > > + return -EINVAL; > > + } > > Ditto. > > here, we need not worry the value of residue over I2C_FIFO_MAX. > > > +static void zx2967_enable_tenbit(struct zx2967_i2c_info *zx_i2c, __u16 > addr) > > +{ > > + u16 val = ((addr >> 7) & 0x7); > > Redundant parens. > > > + if (val > 0) { > > if (!val) > return; > > > +static int zx2967_i2c_xfer_msg(struct zx2967_i2c_info *zx_i2c, > > + struct i2c_msg *msg > > +{ > > + unsigned long time_left = 0; > > > + unsigned int i = 0; > > Redundant assignment. > > > + > > + if (msg->len == 0) > > if (!msg...) > > > + return -EINVAL; > > > +static int zx2967_i2c_xfer(struct i2c_adapter *adap, > > + struct i2c_msg *msgs, int num) > > +{ > > + int i; > > > + int ret = 0; > > Redundant. See below. > > > + struct zx2967_i2c_info *zx_i2c = i2c_get_adapdata(adap); > > Reverse tree, please. > > > + > > + if (zx_i2c->is_suspended) > > + return -EBUSY; > > + > > + zx2967_i2c_writel(zx_i2c, (msgs->addr & 0x7f), REG_DEVADDR_L); > > + zx2967_i2c_writel(zx_i2c, (msgs->addr >> 7) & 0x7, > REG_DEVADDR_H); > > + if (zx2967_i2c_readl(zx_i2c, REG_DEVADDR_H) > 0) > > + zx2967_enable_tenbit(zx_i2c, msgs->addr); > > + > > + for (i = 0; i < num; i++) { > > int ret; > > > + ret = zx2967_i2c_xfer_msg(zx_i2c, &msgs[i]); > > + if (num > 1) > > + usleep_range(1000, 2000); > > + if (ret) > > > + break; > > return ret; > > > + } > > + > > > + return ret ?: i; > > return i; > > And looking now, how i can't be num here? > > > +} > > > +{ > > + struct zx2967_i2c_info *zx_i2c = i2c_get_adapdata(adap); > > > + u32 val; > > + u8 buf[2]; > > + unsigned long time_left; > > Reverse tree. > > > + usleep_range(1000, 2000); > > + switch (size) { > > + case I2C_SMBUS_BYTE: > > + case I2C_SMBUS_BYTE_DATA: > > + val = zx2967_i2c_readl(zx_i2c, REG_DATA); > > + data->byte = val; > > + break; > > + case I2C_SMBUS_WORD_DATA: > > + case I2C_SMBUS_PROC_CALL: > > + buf[0] = zx2967_i2c_readl(zx_i2c, REG_DATA); > > + buf[1] = zx2967_i2c_readl(zx_i2c, REG_DATA); > > + data->word = (buf[0] << 8) | buf[1]; > > + break; > > + default: > > + dev_warn(&adap->dev, "Unsupported transaction %d\n", > size); > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > I would revisit those several loooong functions for splitting/refactoring. > > > + > > +static u32 zx2967_i2c_func(struct i2c_adapter *adap) > > +{ > > + return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | > > + I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | > > + I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_PROC_CALL | > > + I2C_FUNC_I2C | I2C_FUNC_SMBUS_I2C_BLOCK; > > +} > > It's just > #define XXX YY1 | YY2 | ... > > > +#ifdef CONFIG_PM > > +static int zx2967_i2c_suspend(struct device *dev) > > Switch to __maybe_unuse. > > > +static int zx2967_i2c_probe(struct platform_device *pdev) > > +{ > > > + struct resource *res; > > + struct zx2967_i2c_info *zx_i2c = NULL; > > Redundant assignment. > > > + struct clk *clk; > > + void __iomem *reg_base; > > Reverse tree for all above until top of this function. > > > + int ret = 0; > > Redundant assignment. > > > + > > + zx_i2c = devm_kzalloc(&pdev->dev, sizeof(*zx_i2c), GFP_KERNEL); > > + if (!zx_i2c) > > + return -ENOMEM; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + if (!res) { > > + dev_err(&pdev->dev, "missing io resource\n"); > > + return -EINVAL; > > + } > > + > > Completely redundant lines of code. > > > + reg_base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(reg_base)) > > + return PTR_ERR(reg_base); > > + > > + clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(clk)) { > > + dev_err(&pdev->dev, "missing controller clock"); > > + return PTR_ERR(clk); > > + } > > + > > + ret = clk_prepare_enable(clk); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to enable i2c_clk\n"); > > + return ret; > > + } > > + > > > + of_property_read_u32(pdev->dev.of_node, > > + "clock-frequency", &zx_i2c->clk_freq); > > devce_property_read_uXX(); > > > + zx_i2c->reg_base = reg_base; > > + zx_i2c->clk = clk; > > > + zx_i2c->irq = platform_get_irq(pdev, 0); > > Is this optional? > Otherwise has to be checked. > > > + zx_i2c->id = pdev->id; > > + zx_i2c->dev = &pdev->dev; > > + > > + zx_i2c->pin_scl = devm_pinctrl_get_select(&pdev->dev, "scl"); > > + if (IS_ERR(zx_i2c->pin_scl)) > > + dev_info(&pdev->dev, "scl pin is not specified in > dts\n"); > > + > > + zx_i2c->pin_sda = devm_pinctrl_get_select(&pdev->dev, "sda"); > > + if (IS_ERR(zx_i2c->pin_sda)) > > + dev_info(&pdev->dev, "sda pin is not specified in > dts\n"); > > + > > + spin_lock_init(&zx_i2c->lock); > > + init_completion(&zx_i2c->complete); > > + platform_set_drvdata(pdev, zx_i2c); > > + > > + ret = zx2967_i2c_reset_hardware(zx_i2c); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to initialize i2c > controller\n"); > > + goto unprepare_clk; > > + } > > + > > + ret = devm_request_irq(&pdev->dev, zx_i2c->irq, > > + zx2967_i2c_isr, 0, dev_name(&pdev->dev), zx_i2c); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to request irq %i\n", > zx_i2c->irq); > > + goto unprepare_clk; > > + } > > + > > + i2c_set_adapdata(&zx_i2c->adap, zx_i2c); > > > + zx_i2c->adap.owner = THIS_MODULE; > > Do you need this? > > > + zx_i2c->adap.class = I2C_CLASS_DEPRECATED; > > + strlcpy(zx_i2c->adap.name, "zx2967 i2c adapter", > > + sizeof(zx_i2c->adap.name)); > > + zx_i2c->adap.algo = &zx2967_i2c_algo; > > + zx_i2c->adap.dev.parent = &pdev->dev; > > + zx_i2c->adap.nr = pdev->id; > > + zx_i2c->adap.dev.of_node = pdev->dev.of_node; > > + > > + ret = i2c_add_numbered_adapter(&zx_i2c->adap); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to add zx2967 i2c > adapter\n"); > > + goto unprepare_clk; > > + } > > + > > + return 0; > > + > > +unprepare_clk: > > err_clk_unprepare: > > > + clk_unprepare(zx_i2c->clk); > > Perhaps clk_disable_unprepare(); > > > + return ret; > > +} > > + > > +static int zx2967_i2c_remove(struct platform_device *pdev) > > +{ > > + struct zx2967_i2c_info *zx_i2c = platform_get_drvdata(pdev); > > + > > + i2c_del_adapter(&zx_i2c->adap); > > > + clk_unprepare(zx_i2c->clk); > > Ditto. > > > + > > + return 0; > > +} > > > +MODULE_AUTHOR("Baoyou Xie <baoyou.xie@linaro.org>"); > > +MODULE_DESCRIPTION("ZTE zx2967 I2C Bus Controller driver"); > > +MODULE_LICENSE("GPL v2"); > > When bool in Kconfig this all redundant. > > -- > With Best Regards, > Andy Shevchenko > [-- Attachment #2: Type: text/html, Size: 15631 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family 2017-01-20 8:43 ` [PATCH v1 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family Baoyou Xie 2017-01-20 21:45 ` Andy Shevchenko @ 2017-01-23 2:50 ` Shawn Guo 1 sibling, 0 replies; 6+ messages in thread From: Shawn Guo @ 2017-01-23 2:50 UTC (permalink / raw) To: Baoyou Xie Cc: jun.nie, wsa, robh+dt, mark.rutland, linux-arm-kernel, linux-i2c, devicetree, linux-kernel, xie.baoyou, chen.chaokai, wang.qiang01 On Fri, Jan 20, 2017 at 04:43:47PM +0800, Baoyou Xie wrote: > +static int zx2967_i2c_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + struct zx2967_i2c_info *zx_i2c = NULL; > + struct clk *clk; > + void __iomem *reg_base; > + int ret = 0; > + > + zx_i2c = devm_kzalloc(&pdev->dev, sizeof(*zx_i2c), GFP_KERNEL); > + if (!zx_i2c) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(&pdev->dev, "missing io resource\n"); > + return -EINVAL; > + } > + > + reg_base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(reg_base)) > + return PTR_ERR(reg_base); > + > + clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(clk)) { > + dev_err(&pdev->dev, "missing controller clock"); > + return PTR_ERR(clk); > + } > + > + ret = clk_prepare_enable(clk); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable i2c_clk\n"); > + return ret; > + } > + > + of_property_read_u32(pdev->dev.of_node, > + "clock-frequency", &zx_i2c->clk_freq); > + zx_i2c->reg_base = reg_base; > + zx_i2c->clk = clk; > + zx_i2c->irq = platform_get_irq(pdev, 0); > + zx_i2c->id = pdev->id; > + zx_i2c->dev = &pdev->dev; > + > + zx_i2c->pin_scl = devm_pinctrl_get_select(&pdev->dev, "scl"); > + if (IS_ERR(zx_i2c->pin_scl)) > + dev_info(&pdev->dev, "scl pin is not specified in dts\n"); > + > + zx_i2c->pin_sda = devm_pinctrl_get_select(&pdev->dev, "sda"); > + if (IS_ERR(zx_i2c->pin_sda)) > + dev_info(&pdev->dev, "sda pin is not specified in dts\n"); I don't understand why this is necessary. For most cases, we only need to specify needed pins in property 'pinctrl-0' and 'default' in 'pinctrl-names', and pinctrl/driver core will just set up the pins for the device. Shawn ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-23 2:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-20 8:43 [PATCH v1 1/3] dt: bindings: add documentation for zx2967 family i2c controller Baoyou Xie 2017-01-20 8:43 ` [PATCH v1 2/3] MAINTAINERS: add zx2967 i2c controller driver to ARM ZTE architecture Baoyou Xie 2017-01-20 8:43 ` [PATCH v1 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family Baoyou Xie 2017-01-20 21:45 ` Andy Shevchenko 2017-01-22 10:12 ` Baoyou Xie 2017-01-23 2:50 ` Shawn Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).