Hi Kamal, + bcm-kernel-feedback-list <[email protected]>
On 4/14/2015 6:47 AM, Wolfram Sang wrote: > On Thu, Apr 02, 2015 at 04:01:05PM -0400, Kamal Dasu wrote: >> Adding support for i2c controller driver for Broadcom settop >> SoCs. >> >> Signed-off-by: Kamal Dasu <[email protected]> > > Wow, this is the third I2C master driver coming from Broadcom in a > pretty short time. May I ask the authors of the previous Broadcom > drivers (CCed) to do the initial review? > > Thanks, > > Wolfram > >> --- >> V2 changes >> - Separate commits for device tree bindings and >> initial i2c-brcmstb driver code >> --- >> drivers/i2c/busses/Kconfig | 10 + >> drivers/i2c/busses/Makefile | 1 + >> drivers/i2c/busses/i2c-brcmstb.c | 703 >> +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 714 insertions(+) >> create mode 100644 drivers/i2c/busses/i2c-brcmstb.c >> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index 22da9c2..e3938f4 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -392,6 +392,16 @@ config I2C_BCM_KONA >> >> If you do not need KONA I2C interface, say N. >> >> +config I2C_BRCMSTB >> + tristate "BRCM Settop I2C adapter" >> + depends on ARCH_BRCMSTB Would be nice to add dependency on COMPILE_TEST if you don't mind? >> + default y >> + help >> + If you say yes to this option, support will be included for the >> + I2C interface on the Broadcom Settop SoCs. >> + >> + If you do not need I2C interface, say N. >> + >> config I2C_BLACKFIN_TWI >> tristate "Blackfin TWI I2C support" >> depends on BLACKFIN >> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile >> index 3638feb..91a0b5f 100644 >> --- a/drivers/i2c/busses/Makefile >> +++ b/drivers/i2c/busses/Makefile >> @@ -102,6 +102,7 @@ obj-$(CONFIG_I2C_VIPERBOARD) += i2c-viperboard.o >> # Other I2C/SMBus bus drivers >> obj-$(CONFIG_I2C_ACORN) += i2c-acorn.o >> obj-$(CONFIG_I2C_BCM_KONA) += i2c-bcm-kona.o >> +obj-$(CONFIG_I2C_BRCMSTB) += i2c-brcmstb.o >> obj-$(CONFIG_I2C_CROS_EC_TUNNEL) += i2c-cros-ec-tunnel.o >> obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o >> obj-$(CONFIG_I2C_OPAL) += i2c-opal.o >> diff --git a/drivers/i2c/busses/i2c-brcmstb.c >> b/drivers/i2c/busses/i2c-brcmstb.c >> new file mode 100644 >> index 0000000..b69ab14 >> --- /dev/null >> +++ b/drivers/i2c/busses/i2c-brcmstb.c >> @@ -0,0 +1,703 @@ >> +/* >> + * Copyright (C) 2014 Broadcom Corporation >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation version 2. >> + * >> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any >> + * kind, whether express or implied; without even the implied warranty >> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/sched.h> >> +#include <linux/i2c.h> >> +#include <linux/interrupt.h> >> +#include <linux/platform_device.h> >> +#include <linux/clk.h> >> +#include <linux/io.h> >> +#include <linux/clk.h> >> +#include <linux/slab.h> >> +#include <linux/version.h> >> +#include <linux/delay.h> Wolfram would prefer the includes to be sorted in alphabetic order >> + >> +#define N_DATA_REGS 8 >> +#define N_DATA_BYTES (N_DATA_REGS * >> 4) >> + >> +/* BSC count register field definitions */ >> +#define BSC_CNT_REG1_MASK 0x0000003f >> +#define BSC_CNT_REG1_SHIFT 0 >> +#define BSC_CNT_REG2_MASK 0x00000fc0 >> +#define BSC_CNT_REG2_SHIFT 6 >> + >> +/* BSC CTL register field definitions */ >> +#define BSC_CTL_REG_DTF_MASK 0x00000003 >> +#define BSC_CTL_REG_SCL_SEL_MASK 0x00000030 >> +#define BSC_CTL_REG_SCL_SEL_SHIFT 4 >> +#define BSC_CTL_REG_INT_EN_MASK 0x00000040 >> +#define BSC_CTL_REG_INT_EN_SHIFT 6 >> +#define BSC_CTL_REG_DIV_CLK_MASK 0x00000080 >> + >> +/* BSC_IIC_ENABLE r/w enable and interrupt field defintions */ >> +#define BSC_IIC_EN_RESTART_MASK 0x00000040 >> +#define BSC_IIC_EN_NOSTART_MASK 0x00000020 >> +#define BSC_IIC_EN_NOSTOP_MASK 0x00000010 >> +#define BSC_IIC_EN_NOACK_MASK 0x00000004 >> +#define BSC_IIC_EN_INTRP_MASK 0x00000002 >> +#define BSC_IIC_EN_ENABLE_MASK 0x00000001 >> + >> +/* BSC_CTLHI control register field definitions */ >> +#define BSC_CTLHI_REG_INPUT_SWITCHING_LEVEL_MASK 0x00000080 >> +#define BSC_CTLHI_REG_DATAREG_SIZE_MASK 0x00000040 >> +#define BSC_CTLHI_REG_IGNORE_ACK_MASK 0x00000002 >> +#define BSC_CTLHI_REG_WAIT_DIS_MASK 0x00000001 >> + >> +#define I2C_TIMEOUT 100 /* msecs */ >> + >> +/* Condition mask used for non combined transfer */ >> +#define COND_RESTART BSC_IIC_EN_RESTART_MASK >> +#define COND_NOSTART BSC_IIC_EN_NOSTART_MASK >> +#define COND_NOSTOP BSC_IIC_EN_NOSTOP_MASK Is the tab above expected? >> +#define COND_START_STOP (COND_RESTART | COND_NOSTART | >> COND_NOSTOP) >> + >> +/* BSC data transfer direction */ >> +#define DTF_WR_MASK 0x00000000 >> +#define DTF_RD_MASK 0x00000001 >> +/* BSC data transfer direction combined format */ >> +#define DTF_RD_WR_MASK 0x00000002 >> +#define DTF_WR_RD_MASK 0x00000003 >> +/* default bus speed */ >> +#define DEFAULT_BUS_SPEEDHZ 375000 >> + >> +struct bsc_regs { >> + u32 chip_address; >> + u32 data_in[N_DATA_REGS]; >> + u32 cnt_reg; >> + u32 ctl_reg; >> + u32 iic_enable; >> + u32 data_out[N_DATA_REGS]; >> + u32 ctlhi_reg; >> + u32 scl_param; >> +}; >> + >> +struct bsc_clk_param { >> + u32 hz; >> + u32 scl_mask; >> + u32 div_mask; >> +}; >> + >> +enum bsc_xfer_cmd { >> + CMD_WR, >> + CMD_RD, >> + CMD_WR_NOACK, >> + CMD_RD_NOACK, >> +}; >> + >> +static char const *cmd_string[] = { >> + [CMD_WR] = "WR", >> + [CMD_RD] = "RD", >> + [CMD_WR_NOACK] = "WR NOACK", >> + [CMD_RD_NOACK] = "RD NOACK", >> +}; >> + >> +enum bus_speeds { >> + SPD_375K, >> + SPD_390K, >> + SPD_187K, >> + SPD_200K, >> + SPD_93K, >> + SPD_97K, >> + SPD_46K, >> + SPD_50K >> +}; >> + >> +static const struct bsc_clk_param bsc_clk[] = { two spaces before bsc_clk >> + [SPD_375K] = { >> + .hz = 375000, >> + .scl_mask = SPD_375K << BSC_CTL_REG_SCL_SEL_SHIFT, >> + .div_mask = 0 >> + }, >> + [SPD_390K] = { >> + .hz = 390000, >> + .scl_mask = SPD_390K << BSC_CTL_REG_SCL_SEL_SHIFT, >> + .div_mask = 0 >> + }, >> + [SPD_187K] = { >> + .hz = 187500, >> + .scl_mask = SPD_187K << BSC_CTL_REG_SCL_SEL_SHIFT, >> + .div_mask = 0 >> + }, >> + [SPD_200K] = { >> + .hz = 200000, >> + .scl_mask = SPD_200K << BSC_CTL_REG_SCL_SEL_SHIFT, >> + .div_mask = 0 >> + }, >> + [SPD_93K] = { >> + .hz = 93750, >> + .scl_mask = SPD_375K << BSC_CTL_REG_SCL_SEL_SHIFT, >> + .div_mask = BSC_CTL_REG_DIV_CLK_MASK >> + }, >> + [SPD_97K] = { >> + .hz = 97500, >> + .scl_mask = SPD_390K << BSC_CTL_REG_SCL_SEL_SHIFT, >> + .div_mask = BSC_CTL_REG_DIV_CLK_MASK >> + }, >> + [SPD_46K] = { >> + .hz = 46875, >> + .scl_mask = SPD_187K << BSC_CTL_REG_SCL_SEL_SHIFT, >> + .div_mask = BSC_CTL_REG_DIV_CLK_MASK >> + }, >> + [SPD_50K] = { >> + .hz = 50000, >> + .scl_mask = SPD_200K << BSC_CTL_REG_SCL_SEL_SHIFT, >> + .div_mask = BSC_CTL_REG_DIV_CLK_MASK >> + } >> +}; >> + >> +struct brcmstb_i2c_dev { >> + struct device *device; >> + void __iomem *base; >> + void __iomem *irq_base; >> + int irq; >> + struct bsc_regs *bsc_regmap; >> + struct i2c_adapter adapter; >> + struct completion done; >> + bool is_suspended; >> + u32 clk_freq_hz; >> +}; >> + >> +#define bsc_readl(_dev, reg) >> \ >> + __bsc_readl(_dev, offsetof(struct bsc_regs, reg)) >> + >> +#define bsc_writel(_dev, val, reg) \ >> + __bsc_writel(_dev, val, offsetof(struct bsc_regs, reg)) >> + >> +static inline u32 __bsc_readl(struct brcmstb_i2c_dev *dev, u32 reg) >> +{ >> + return __raw_readl(dev->base + reg); >> +} >> + >> +static inline void __bsc_writel(struct brcmstb_i2c_dev *dev, u32 val, >> + u32 reg) >> +{ >> + __raw_writel(val, dev->base + reg); >> +} >> + >> +static void brcmstb_i2c_disable_irq(struct brcmstb_i2c_dev *dev) >> +{ >> + /* Disable BSC CTL interrupt line */ >> + dev->bsc_regmap->ctl_reg &= ~BSC_CTL_REG_INT_EN_MASK; What is the purpose of dev->bsc_regmap? For storing cached masked value of some registers? >> + barrier(); What is the purpose of the above barrier? Same question applies to all other places >> + bsc_writel(dev, dev->bsc_regmap->ctl_reg, ctl_reg); >> +} >> + >> +static void brcmstb_i2c_enable_irq(struct brcmstb_i2c_dev *dev) >> +{ >> + /* Enable BSC CTL interrupt line */ >> + dev->bsc_regmap->ctl_reg |= BSC_CTL_REG_INT_EN_MASK; >> + barrier(); >> + bsc_writel(dev, dev->bsc_regmap->ctl_reg, ctl_reg); >> +} You should consider combining brcmstb_i2c_disable_irq and brcmstb_i2c_enable_irq since they access the same register. >> + >> +static irqreturn_t brcmstb_i2c_isr(int irq, void *devid) >> +{ >> + struct brcmstb_i2c_dev *dev = devid; >> + u32 status_bsc_ctl = bsc_readl(dev, ctl_reg); >> + u32 status_iic_intrp = bsc_readl(dev, iic_enable); What is the purpose of status_iic_intrp? It's not used any where in the ISR except the debugging prints. >> + >> + dev_dbg(dev->device, "isr CTL_REG %x IIC_EN %x\n", >> + status_bsc_ctl, status_iic_intrp); >> + >> + if (!(status_bsc_ctl & BSC_CTL_REG_INT_EN_MASK)) >> + return IRQ_NONE; >> + >> + brcmstb_i2c_disable_irq(dev); >> + complete_all(&dev->done); >> + >> + dev_dbg(dev->device, "isr handled"); >> + return IRQ_HANDLED; >> +} >> + >> +/* Wait for device to be ready */ >> +static int brcmstb_i2c_wait_if_busy(struct brcmstb_i2c_dev *dev) >> +{ >> + unsigned long timeout = jiffies + msecs_to_jiffies(I2C_TIMEOUT); >> + >> + while ((bsc_readl(dev, iic_enable) & BSC_IIC_EN_INTRP_MASK)) { >> + if (time_after(jiffies, timeout)) { >> + dev_err(dev->device, "i2c device busy timeout\n"); >> + return -ETIMEDOUT; >> + } >> + } In the worst case, the above loop busy waiting for 100 msec? Consider adding a cpu_relax call to make it more efficient. >> + return 0; >> +} >> + >> +/* i2c xfer completion function, handles both irq and polling mode >> + */ >> +static int brcmstb_i2c_waitforcompletion(struct brcmstb_i2c_dev *dev) >> +{ >> + int ret = 0; >> + unsigned long timeout = msecs_to_jiffies(I2C_TIMEOUT); >> + >> + if (dev->irq >= 0) { irq = 0 is not a valid line? >> + if (!wait_for_completion_timeout(&dev->done, timeout)) >> + ret = -ETIMEDOUT; >> + } else { >> + /* we are in polling mode */ >> + u32 bsc_intrp; >> + unsigned long time_left = jiffies + timeout; >> + >> + do { >> + bsc_intrp = bsc_readl(dev, iic_enable) & >> + BSC_IIC_EN_INTRP_MASK; >> + if (time_after(jiffies, time_left)) { >> + ret = -ETIMEDOUT; >> + break; >> + } >> + udelay(100); >> + } while (!bsc_intrp); Again, with the use of udelay, this is still busy waiting for 100 msec in the worst case. >> + brcmstb_i2c_disable_irq(dev); Why is irq disabled called in the case of polling mode but not irq mode? >> + } >> + >> + return ret; >> +} >> + >> +/* Set xfer START/STOP conditions for subsequent transfer */ >> +static void brcmstb_set_i2c_start_stop(struct brcmstb_i2c_dev *dev, >> + u32 cond_flag) >> +{ >> + u32 regval = dev->bsc_regmap->iic_enable; >> + >> + dev->bsc_regmap->iic_enable = (regval & ~COND_START_STOP) | cond_flag; >> +} >> + >> +/* Send I2C request */ >> +static int brcmstb_send_i2c_cmd(struct brcmstb_i2c_dev *dev, >> + enum bsc_xfer_cmd cmd) >> +{ >> + int rc = 0, ignore_ack = 0; ignore_ack should be a bool >> + struct bsc_regs *pi2creg = dev->bsc_regmap; >> + u32 ctl_reg; >> + >> + /* Make sure the hardware is ready */ >> + rc = brcmstb_i2c_wait_if_busy(dev); >> + if (rc < 0) >> + return rc; >> + >> + dev_dbg(dev->device, "%s, %d\n", __func__, cmd); >> + >> + /* see if the transaction needs check NACK conditions */ >> + ignore_ack = (cmd == CMD_WR || cmd == CMD_RD) ? 0 : 1; >> + if (ignore_ack) >> + pi2creg->ctlhi_reg |= BSC_CTLHI_REG_IGNORE_ACK_MASK; >> + else >> + pi2creg->ctlhi_reg &= ~BSC_CTLHI_REG_IGNORE_ACK_MASK; >> + bsc_writel(dev, pi2creg->ctlhi_reg, ctlhi_reg); >> + >> + if (dev->irq >= 0) >> + reinit_completion(&dev->done); >> + >> + /* set data transfer direction */ >> + ctl_reg = pi2creg->ctl_reg & ~BSC_CTL_REG_DTF_MASK; >> + if (cmd == CMD_WR || cmd == CMD_WR_NOACK) >> + pi2creg->ctl_reg = ctl_reg | DTF_WR_MASK; >> + else >> + pi2creg->ctl_reg = ctl_reg | DTF_RD_MASK; >> + >> + /* enable BSC CTL interrupt line */ >> + brcmstb_i2c_enable_irq(dev); >> + >> + /* initiate transfer by setting iic_enable */ >> + pi2creg->iic_enable |= BSC_IIC_EN_ENABLE_MASK; >> + bsc_writel(dev, pi2creg->iic_enable, iic_enable); >> + So you only disable the interrupt in the ISR. If the ISR is never fired for whatever reason, then the interrupt is kept enabled. A better way to handle this would be to disable the interrupt after brcmstb_i2c_waitforcompletion (in either sucess or failure case), so it's consistent that the interrupt always gets disabled after you come out of this function. >> + /* Wait for transaction to finish or timeout */ >> + rc = brcmstb_i2c_waitforcompletion(dev); >> + if (rc) { >> + dev_err(dev->device, "intr timeout for cmd %s\n", >> + cmd_string[cmd]); >> + goto cmd_out; >> + } >> + >> + if (!ignore_ack && bsc_readl(dev, iic_enable) & BSC_IIC_EN_NOACK_MASK) { >> + rc = -EREMOTEIO; >> + dev_dbg(dev->device, "controller received NOACK intr for %s\n", >> + cmd_string[cmd]); >> + } >> + >> +cmd_out: >> + bsc_writel(dev, 0, cnt_reg); >> + bsc_writel(dev, 0, iic_enable); >> + >> + return rc; >> +} >> + >> +static int brcmstb_i2c_xfer_bsc_data(struct brcmstb_i2c_dev *dev, >> + u8 *buf, unsigned int len, >> + enum bsc_xfer_cmd cmd) >> +{ >> + int i, j, rc; >> + >> + /* set the read/write length */ >> + bsc_writel(dev, BSC_CNT_REG1_MASK & (len << BSC_CNT_REG1_SHIFT), >> + cnt_reg); What is the max len that can be supported? BSC_CNT_REG1_MASK = 63? Then you should check against that. Note Wolfram has introduced a way to validate the supported data length at the i2c-core, which you should adapt: commit b7f625840267b18ef1011cba0085bb7e237d76f7 Author: Wolfram Sang <[email protected]> Date: Mon Jan 5 23:45:59 2015 +0100 i2c: add quirk checks to core Let the core do the checks if HW quirks prevent a transfer. Saves code from drivers and adds consistency. >> + >> + /* Write data into data_in register */ >> + if (cmd == CMD_WR || cmd == CMD_WR_NOACK) { >> + for (i = 0; i < len; i += 4) { >> + u32 word = 0; >> + >> + for (j = 0; j < 4; j++) { >> + word >>= 8; >> + if ((i + j) < len) >> + word |= buf[i + j] << 24; >> + } >> + bsc_writel(dev, word, data_in[i >> 2]); >> + } >> + } >> + >> + /* Initiate xfer */ >> + rc = brcmstb_send_i2c_cmd(dev, cmd); >> + >> + if (rc != 0) >> + return rc; >> + >> + if (cmd == CMD_RD || cmd == CMD_RD_NOACK) { >> + for (i = 0; i < len; i += 4) { >> + u32 data = bsc_readl(dev, data_out[i >> 2]); >> + >> + for (j = 0; j < 4 && (j + i) < len; j++) { >> + buf[i + j] = data & 0xff; >> + data >>= 8; >> + } >> + } >> + } >> + >> + return 0; >> +} >> + >> +/* Write a single byte of data to the i2c bus */ >> +static int brcmstb_i2c_write_data_byte(struct brcmstb_i2c_dev *dev, >> + u8 *buf, unsigned int nak_expected) >> +{ >> + enum bsc_xfer_cmd cmd = nak_expected ? CMD_WR : CMD_WR_NOACK; >> + >> + bsc_writel(dev, 1, cnt_reg); >> + bsc_writel(dev, *buf, data_in); >> + >> + return brcmstb_send_i2c_cmd(dev, cmd); >> +} >> + >> +/* Send i2c address */ >> +static int brcmstb_i2c_do_addr(struct brcmstb_i2c_dev *dev, >> + struct i2c_msg *msg) >> +{ >> + unsigned char addr; >> + >> + if (msg->flags & I2C_M_TEN) { >> + redundant space >> + /* First byte is 11110XX0 where XX is upper 2 bits */ >> + addr = 0xF0 | ((msg->addr & 0x300) >> 7); >> + bsc_writel(dev, addr, chip_address); >> + >> + /* Second byte is the remaining 8 bits */ >> + addr = msg->addr & 0xFF; >> + if (brcmstb_i2c_write_data_byte(dev, &addr, 0) < 0) >> + return -EREMOTEIO; >> + >> + if (msg->flags & I2C_M_RD) { >> + /* For read, send restart without stop condition */ >> + brcmstb_set_i2c_start_stop(dev, COND_RESTART >> + | COND_NOSTOP); >> + /* Then re-send the first byte with the read bit set */ >> + addr = 0xF0 | ((msg->addr & 0x300) >> 7) | 0x01; >> + if (brcmstb_i2c_write_data_byte(dev, &addr, 0) < 0) >> + return -EREMOTEIO; >> + >> + } >> + } else { >> + addr = msg->addr << 1; >> + if (msg->flags & I2C_M_RD) >> + addr |= 1; >> + >> + bsc_writel(dev, addr, chip_address); >> + } >> + >> + return 0; >> +} >> + >> +/* Master transfer function */ >> +static int brcmstb_i2c_xfer(struct i2c_adapter *adapter, >> + struct i2c_msg msgs[], int num) >> +{ >> + struct brcmstb_i2c_dev *dev = i2c_get_adapdata(adapter); >> + struct i2c_msg *pmsg; >> + int rc = 0; >> + int i; >> + int bytes_to_xfer; >> + enum bsc_xfer_cmd cmd; >> + u8 *tmp_buf; >> + int len = 0; >> + int ignore_nack = 0; >> + >> + if (dev->is_suspended) >> + return -EBUSY; >> + >> + /* Loop through all messages */ >> + for (i = 0; i < num; i++) { >> + pmsg = &msgs[i]; >> + len = pmsg->len; >> + tmp_buf = pmsg->buf; >> + ignore_nack = pmsg->flags & I2C_M_IGNORE_NAK; >> + >> + dev_dbg(dev->device, >> + "msg# %d/%d flg %x buf %x len %d\n", i, >> + num - 1, pmsg->flags, >> + pmsg->buf ? pmsg->buf[0] : '0', pmsg->len); >> + >> + if (i < (num - 1) && (msgs[i + 1].flags & I2C_M_NOSTART)) >> + brcmstb_set_i2c_start_stop(dev, ~(COND_START_STOP)); >> + else >> + brcmstb_set_i2c_start_stop(dev, >> + COND_RESTART | COND_NOSTOP); >> + >> + /* Send slave address */ >> + if (!(pmsg->flags & I2C_M_NOSTART)) { >> + rc = brcmstb_i2c_do_addr(dev, pmsg); >> + if (rc < 0) { >> + dev_dbg(dev->device, >> + "NACK for addr %2.2x msg#%d rc = %d\n", >> + pmsg->addr, i, rc); >> + goto out; >> + } >> + } >> + >> + cmd = (pmsg->flags & I2C_M_RD) ? CMD_RD : CMD_WR; >> + >> + /* Perform data transfer */ >> + while (len) { >> + bytes_to_xfer = min(len, N_DATA_BYTES); >> + >> + if (ignore_nack || len <= N_DATA_BYTES) >> + cmd = (pmsg->flags & I2C_M_RD) ? CMD_RD_NOACK >> + : CMD_WR_NOACK; >> + >> + if (len <= N_DATA_BYTES && i == (num - 1)) >> + brcmstb_set_i2c_start_stop(dev, >> + ~(COND_START_STOP)); >> + >> + rc = brcmstb_i2c_xfer_bsc_data(dev, tmp_buf, >> + bytes_to_xfer, cmd); >> + if (rc < 0) { >> + dev_dbg(dev->device, "%s failure", >> + cmd_string[cmd]); >> + goto out; >> + } >> + >> + len -= bytes_to_xfer; >> + tmp_buf += bytes_to_xfer; >> + } >> + } >> + >> + rc = num; >> +out: >> + return rc; >> + >> +} >> + >> +static u32 brcmstb_i2c_functionality(struct i2c_adapter *adap) >> +{ >> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR >> + | I2C_FUNC_NOSTART | I2C_FUNC_PROTOCOL_MANGLING; >> +} >> + >> +static const struct i2c_algorithm brcmstb_i2c_algo = { >> + .master_xfer = brcmstb_i2c_xfer, >> + .functionality = brcmstb_i2c_functionality, >> +}; >> + >> +static void brcmstb_i2c_set_bus_speed(struct brcmstb_i2c_dev *dev) >> +{ >> + int i = 0, num_speeds = ARRAY_SIZE(bsc_clk); >> + u32 clk_freq_hz = dev->clk_freq_hz; >> + >> + for (i = 0; i < num_speeds; i++) { >> + if (bsc_clk[i].hz == clk_freq_hz) { So you only allow an exact rate match? I think a round down of the rate when not exactly matched might be sufficient? >> + dev->bsc_regmap->ctl_reg &= ~(BSC_CTL_REG_SCL_SEL_MASK >> + | BSC_CTL_REG_DIV_CLK_MASK); >> + dev->bsc_regmap->ctl_reg |= (bsc_clk[i].scl_mask | >> + bsc_clk[i].div_mask); >> + bsc_writel(dev, dev->bsc_regmap->ctl_reg, ctl_reg); >> + break; >> + } >> + } >> + >> + if (i == num_speeds) { >> + i = (bsc_readl(dev, ctl_reg) & BSC_CTL_REG_SCL_SEL_MASK) >> >> + BSC_CTL_REG_SCL_SEL_SHIFT; >> + dev_warn(dev->device, "invalid input clock-frequency %dHz\n", >> + clk_freq_hz); >> + dev_warn(dev->device, "leaving current clock-frequency @ >> %dHz\n", >> + bsc_clk[i].hz); >> + } >> +} >> + >> +static void brcmstb_i2c_set_bsc_reg_defaults(struct brcmstb_i2c_dev *dev) >> +{ >> + /* 4 byte data register */ >> + dev->bsc_regmap->ctlhi_reg = BSC_CTLHI_REG_DATAREG_SIZE_MASK; >> + bsc_writel(dev, dev->bsc_regmap->ctlhi_reg, ctlhi_reg); >> + /* set bus speed */ >> + brcmstb_i2c_set_bus_speed(dev); >> +} >> + >> +static int brcmstb_i2c_probe(struct platform_device *pdev) >> +{ >> + int rc = 0; >> + struct brcmstb_i2c_dev *dev; >> + struct i2c_adapter *adap; >> + struct resource *iomem; >> + const char *int_name; >> + >> + /* Allocate memory for private data structure */ >> + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL); >> + if (!dev) >> + return -ENOMEM; >> + >> + dev->bsc_regmap = kzalloc(sizeof(struct bsc_regs), GFP_KERNEL); why can't you use devm_kzalloc here? >> + if (!dev->bsc_regmap) >> + return -ENOMEM; >> + >> + platform_set_drvdata(pdev, dev); >> + dev->device = &pdev->dev; >> + init_completion(&dev->done); >> + >> + /* Map hardware registers */ >> + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + dev->base = devm_ioremap_resource(dev->device, iomem); >> + if (IS_ERR(dev->base)) { >> + rc = -ENOMEM; >> + goto probe_errorout; >> + } >> + >> + rc = of_property_read_string(dev->device->of_node, "interrupt-names", >> + &int_name); Why do you need a specific DT property to specify the interrupt name? Why can't you simply set it to pdev->name? >> + if (rc < 0) >> + int_name = NULL; >> + >> + /* Get the interrupt number */ >> + dev->irq = platform_get_irq(pdev, 0); No error checking? I guess you rely on devm_request_irq below to do it? That's probably fine. >> + >> + /* disable the bsc interrupt line */ >> + brcmstb_i2c_disable_irq(dev); >> + >> + /* register the ISR handler */ >> + rc = devm_request_irq(&pdev->dev, dev->irq, brcmstb_i2c_isr, >> + IRQF_SHARED, >> + int_name ? int_name : pdev->name, >> + dev); >> + >> + if (rc) { >> + dev_dbg(dev->device, "falling back to polling mode"); >> + dev->irq = -1; >> + } >> + >> + /* Add the i2c adapter */ >> + adap = &dev->adapter; >> + i2c_set_adapdata(adap, dev); >> + adap->owner = THIS_MODULE; Not needed... >> + strlcpy(adap->name, "Broadcom STB : ", sizeof(adap->name)); >> + if (int_name) >> + strlcat(adap->name, int_name, sizeof(adap->name)); >> + adap->algo = &brcmstb_i2c_algo; >> + adap->dev.parent = &pdev->dev; >> + adap->dev.of_node = pdev->dev.of_node; >> + rc = i2c_add_adapter(adap); >> + if (rc) { >> + dev_err(dev->device, "failed to add adapter\n"); >> + goto probe_errorout; >> + } >> + >> + if (of_property_read_u32(dev->device->of_node, >> + "clock-frequency", &dev->clk_freq_hz)) { >> + dev_warn(dev->device, "missing clock-frequency property\n"); >> + dev_warn(dev->device, "setting default clock-frequency %dHz\n", >> + bsc_clk[0].hz); >> + dev->clk_freq_hz = bsc_clk[0].hz; >> + } >> + >> + brcmstb_i2c_set_bsc_reg_defaults(dev); Don't you want to do this before calling i2c_add_adapter? >> + dev_info(dev->device, "%s@%dhz registered in %s mode\n", >> + int_name ? int_name : " ", dev->clk_freq_hz, >> + (dev->irq >= 0) ? "interrupt" : "polling"); >> + >> + return 0; >> + >> +probe_errorout: >> + kfree(dev->bsc_regmap); >> + return rc; >> +} >> + >> +static int brcmstb_i2c_remove(struct platform_device *pdev) >> +{ >> + struct brcmstb_i2c_dev *dev = platform_get_drvdata(pdev); >> + >> + kfree(dev->bsc_regmap); >> + i2c_del_adapter(&dev->adapter); >> + >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM_SLEEP >> +static int brcmstb_i2c_suspend(struct device *dev) >> +{ >> + struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev); >> + >> + i2c_lock_adapter(&i2c_dev->adapter); >> + i2c_dev->is_suspended = true; >> + i2c_unlock_adapter(&i2c_dev->adapter); >> + >> + return 0; >> +} >> + >> +static int brcmstb_i2c_resume(struct device *dev) >> +{ >> + struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev); >> + >> + i2c_lock_adapter(&i2c_dev->adapter); >> + brcmstb_i2c_set_bsc_reg_defaults(i2c_dev); The BSC registers might be reset coming out of deep sleep? >> + i2c_dev->is_suspended = false; >> + i2c_unlock_adapter(&i2c_dev->adapter); >> + >> + return 0; >> +} >> +#endif >> + >> +static SIMPLE_DEV_PM_OPS(brcmstb_i2c_pm, brcmstb_i2c_suspend, >> + brcmstb_i2c_resume); >> + >> +static const struct of_device_id brcmstb_i2c_of_match[] = { >> + {.compatible = "brcm,brcmstb-i2c"}, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, brcmstb_i2c_of_match); >> + >> +static struct platform_driver brcmstb_i2c_driver = { >> + .driver = { >> + .name = "brcmstb-i2c", >> + .owner = THIS_MODULE, >> + .of_match_table = brcmstb_i2c_of_match, >> + .pm = &brcmstb_i2c_pm, >> + }, >> + .probe = brcmstb_i2c_probe, >> + .remove = brcmstb_i2c_remove, >> +}; >> +module_platform_driver(brcmstb_i2c_driver); >> + >> +MODULE_AUTHOR("Kamal Dasu <[email protected]>"); >> +MODULE_DESCRIPTION("Broadcom Settop I2C Driver"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 2.3.2 Thanks, Ray -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
