i2c: brcmstb: Add Broadcom settop SoC i2c controller driver
Subsystems (4)
PATCH v2 1/2 i2c: brcmstb: Add Broadcom settop SoC i2c controller driver
Adding support for i2c controller driver for Broadcom settop SoCs.
---
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
drivers/i2c/busses/Kconfig +10 -0
@@ -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 + 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
drivers/i2c/busses/Makefile +1 -0
@@ -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
drivers/i2c/busses/i2c-brcmstb.c new file +703 -0
703 lines not on this page · raw message
Signature · 1 line
2.3.2
A message that is not in the archive (sent off-list or not yet received): 1428004866-13543-1-git-send-email-kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
PATCH v2 2/2 dt-bindings: i2c: Add i2c-brcmstb dt bindings
i2c Device tree binding documentation for brcmstb
SoCs.
--- .../devicetree/bindings/i2c/i2c-brcmstb.txt | 28 ++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-brcmstb.txt
Documentation/devicetree/bindings/i2c/i2c-brcmstb.txt new file +28 -0
@@ -0,0 +1,28 @@ +Broadcom stb bsc iic master controller + +Required properties: + +- compatible: should be "brcm,brcmstb-i2c" +- clock-frequency: 32-bit decimal value of iic master clock freqency in Hz + valid values are 375000, 390000, 187500, 200000 + 93750, 97500, 46875 and 50000 +- reg: specifies the base physical address and size of the registers + +Optional properties : + +- interrupt-parent: specifies the phandle to the parent interrupt controller + this one is cascaded from +- interrupts: specifies the interrupt number, the irq line to be used +- interrupt-names: Interrupt name string + +Example: + +bsca: i2c@f0406200 { + clock-frequency = <390000>; + compatible = "brcm,brcmstb-i2c"; + interrupt-parent = <&irq0_intc>; + reg = <0xf0406200 0x58>; + interrupts = <0x18>; + interrupt-names = "upg_bsca"; +}; +
Signature · 1 line
2.3.2
Quoted 4 lines · Kamal Dasu
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
Quoted 756 lines
---
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.cdrivers/i2c/busses/Kconfig +10
-0@@ -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 + 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 BLACKFINdrivers/i2c/busses/Makefile +1
-0@@ -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.odrivers/i2c/busses/i2c-brcmstb.c new file +703
-0@@ -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> + +#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 +#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[] = { + [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 ; +barrier (); +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); +} + +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); + +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 ; + } + } +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 ) { +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); +brcmstb_i2c_disable_irq (dev); + } + +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 ; +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); + +/* 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); + +/* 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 ) { + +/* 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) { + 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 ); +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); +if (rc <0 ) + int_name =NULL ; + +/* Get the interrupt number */ + dev->irq =platform_get_irq (pdev,0 ); + +/* 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 ; +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); +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); + 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 <kdasu-dY08KVG/lbpWk0Htik3J/[email protected]>" ); +MODULE_DESCRIPTION ("Broadcom Settop I2C Driver" ); +MODULE_LICENSE ("GPL v2" );Signature · 1 line
2.3.2
Hi Kamal,
+ bcm-kernel-feedback-list <bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/[email protected]>
Quoted 31 lines · Wolfram Sang
Quoted 4 lines · Kamal Dasu
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
Quoted 17 lines
---
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.cdrivers/i2c/busses/Kconfig +0
-0@@ -392,6 +392,16 @@ config I2C_BCM_KONA
Quoted 5 lines
drivers/i2c/busses/Kconfig config I2C_BCM_KONA · line 397
Quoted 5 lines
drivers/i2c/busses/Kconfig +3
-0If 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?
Quoted 46 lines
Quoted 46 lines
drivers/i2c/busses/Kconfig +7
-0+ 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 BLACKFINdrivers/i2c/busses/Makefile +1
-0@@ -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.odrivers/i2c/busses/i2c-brcmstb.c new file +18
-0@@ -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>
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 26
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+#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
Quoted 30 lines
Quoted 30 lines
drivers/i2c/busses/i2c-brcmstb.c new file +30
-0+ +#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
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 64
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+#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?
Quoted 46 lines
Quoted 46 lines
drivers/i2c/busses/i2c-brcmstb.c new file +46
-0+#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 ,
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 118
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+SPD_200K , +SPD_93K , +SPD_97K , +SPD_46K , +SPD_50K +}; + +static const struct bsc_clk_param bsc_clk[] = {
two spaces before bsc_clk
Quoted 67 lines
Quoted 67 lines
drivers/i2c/busses/i2c-brcmstb.c new file +67
-0+ [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)
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 193
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+{ +__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?
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c line 194
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c new file +1
-0+barrier ();
What is the purpose of the above barrier? Same question applies to all other places
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c line 204
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+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.
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c line 210
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+ +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.
Quoted 17 lines
Quoted 17 lines
drivers/i2c/busses/i2c-brcmstb.c new file +17
-0+ +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) +{
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 235
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+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.
Quoted 11 lines
drivers/i2c/busses/i2c-brcmstb.c line 246
Quoted 11 lines
drivers/i2c/busses/i2c-brcmstb.c new file +11
-0+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?
Quoted 8 lines
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+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 {
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 262
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+ 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.
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c line 263
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c new file +1
-0+brcmstb_i2c_disable_irq (dev);
Why is irq disabled called in the case of polling mode but not irq mode?
Quoted 11 lines
Quoted 11 lines
drivers/i2c/busses/i2c-brcmstb.c new file +11
-0+ } + +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; +
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 282
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+ 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
Quoted 27 lines
Quoted 27 lines
drivers/i2c/busses/i2c-brcmstb.c new file +27
-0+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 ;
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 317
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+ +/* 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.
Quoted 22 lines
Quoted 22 lines
drivers/i2c/busses/i2c-brcmstb.c new file +22
-0+/* 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,
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 347
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+ 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 <wsa-z923LK4zBo2bacvFa/[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.
Quoted 47 lines
Quoted 47 lines
drivers/i2c/busses/i2c-brcmstb.c new file +47
-0+ +/* 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); +} +
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 402
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+/* 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
Quoted 119 lines
Quoted 119 lines
drivers/i2c/busses/i2c-brcmstb.c new file +119
-0+/* 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, +};
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 529
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+ +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?
Quoted 34 lines
Quoted 34 lines
drivers/i2c/busses/i2c-brcmstb.c new file +34
-0+ 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;
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 571
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+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?
Quoted 9 lines
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c new file +9
-0+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 );
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 588
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-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?
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c line 593
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+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.
Quoted 11 lines
Quoted 11 lines
drivers/i2c/busses/i2c-brcmstb.c new file +11
-0+ +/* 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) {
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 612
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+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...
Quoted 13 lines
Quoted 13 lines
drivers/i2c/busses/i2c-brcmstb.c new file +13
-0+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,
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 633
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+ "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?
Quoted 31 lines
Quoted 31 lines
drivers/i2c/busses/i2c-brcmstb.c new file +31
-0+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 ;
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 672
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-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?
Quoted 25 lines
Quoted 25 lines
drivers/i2c/busses/i2c-brcmstb.c new file +25
-0+ 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,
Quoted 8 lines
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+}; +module_platform_driver (brcmstb_i2c_driver); + +MODULE_AUTHOR ("Kamal Dasu <kdasu-dY08KVG/lbpWk0Htik3J/[email protected]>" ); +MODULE_DESCRIPTION ("Broadcom Settop I2C Driver" ); +MODULE_LICENSE ("GPL v2" );Signature · 1 line
2.3.2
Thanks,
Ray
A message that is not in the archive (sent off-list or not yet received): 552D7AF5.7040005-dY08KVG/lbpWk0Htik3J/[email protected]
Hi Ray,
thanks for the review! I agree to all points except one minor thing:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/i2c/busses/i2c-brcmstb.c new file +7
-0+#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>
Quoted 8 lines
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c line 26
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+#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
Sidenote: And one can see why here: clk.h is included twice.
Quoted 3 lines
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c line 612
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c new file +1
-0+ adap->owner =THIS_MODULE ;Not needed...
This is needed. It is not needed to set it with the platform_driver struct, but for the adapter created, this is needed.
Thanks again,
Wolfram
Quoted 20 lines · Wolfram Sang
Hi Ray,
thanks for the review! I agree to all points except one minor thing:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/i2c/busses/i2c-brcmstb.c new file +7
-0+#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> Quoted 8 lines
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c line 26
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+#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
Quoted 8 lines
Sidenote: And one can see why here:
clk.his included twice.Quoted 3 lines
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c line 612
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c new file +1
-0+ adap->owner =THIS_MODULE ;Not needed...
This is needed. It is not needed to set it with the
platform_driverstruct, but for the adapter created, this is needed.
Okay that's really good to know. Thanks, Wolfram!
Quoted 5 lines
Thanks again,
Wolfram
Quoted 269 lines · Ray Jui
Hi Kamal,
+ bcm-kernel-feedback-list <bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/[email protected]>
Quoted 31 lines · Wolfram Sang
Quoted 4 lines · Kamal Dasu
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
Quoted 17 lines
---
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.cdrivers/i2c/busses/Kconfig +0
-0@@ -392,6 +392,16 @@ config I2C_BCM_KONAQuoted 5 lines
drivers/i2c/busses/Kconfig config I2C_BCM_KONA · line 397
Quoted 5 lines
drivers/i2c/busses/Kconfig +3
-0If you do not need KONA I2C interface, say N. +config I2C_BRCMSTB + tristate "BRCM Settop I2C adapter" + depends on ARCH_BRCMSTBWould be nice to add dependency on
COMPILE_TESTif you don't mind?Quoted 46 lines
Quoted 46 lines
drivers/i2c/busses/Kconfig +7
-0+ 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 BLACKFINdrivers/i2c/busses/Makefile +1
-0@@ -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.odrivers/i2c/busses/i2c-brcmstb.c new file +18
-0@@ -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> Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 26
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+#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
Quoted 30 lines
Quoted 30 lines
drivers/i2c/busses/i2c-brcmstb.c new file +30
-0+ +#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 0x00000002Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 64
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+#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_MASKIs the tab above expected?
Quoted 46 lines
Quoted 46 lines
drivers/i2c/busses/i2c-brcmstb.c new file +46
-0+#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 ,Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 118
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+SPD_200K , +SPD_93K , +SPD_97K , +SPD_46K , +SPD_50K +}; + +static const struct bsc_clk_param bsc_clk[] = {two spaces before
bsc_clkQuoted 70 lines
Quoted 70 lines
drivers/i2c/busses/i2c-brcmstb.c new file +70
-0+ [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); +}
Quoted 9 lines
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c line 193
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+ +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?
Yes thats right, this is to optimize access to registeres.
Quoted 5 lines
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c line 194
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c new file +1
-0+barrier ();What is the purpose of the above barrier? Same question applies to all other places
Some architectures like ARM v6 or v7 processor could optimize order of instruction execution and data accesses. So this just protects agaist that.
Quoted 14 lines
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c line 204
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+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_irqandbrcmstb_i2c_enable_irqsince they access the same register.
Quoted 10 lines
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c line 210
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+ +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.
This interrupt bit (read only) is set when the BSC master completes transferring data to or receiving data from the I2C slave device and reset when enable bit (iic_enable) is deasserted.
This when level 2 interrupt controller is not used (no IRQ number returned) and we switch to polling mode.
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
drivers/i2c/busses/i2c-brcmstb.c new file +20
-0+ +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 )) {
Quoted 9 lines
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c line 235
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+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_relaxcall to make it more efficient.
Will do that.
Quoted 31 lines
Quoted 11 lines
drivers/i2c/busses/i2c-brcmstb.c line 246
Quoted 11 lines
drivers/i2c/busses/i2c-brcmstb.c new file +11
-0+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?
Quoted 8 lines
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+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 {Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 262
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+ 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);
Quoted 7 lines
Again, with the use of udelay, this is still busy waiting for 100 msec in the worst case.
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c line 263
Quoted 1 line
drivers/i2c/busses/i2c-brcmstb.c new file +1
-0+brcmstb_i2c_disable_irq (dev);Why is irq disabled called in the case of polling mode but not irq mode?
Will revisit this.
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/i2c/busses/i2c-brcmstb.c new file +13
-0+ } + +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; +}
Quoted 8 lines
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c line 282
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+ +/* 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_ackshould be a bool
Will change this.
Quoted 40 lines
Quoted 28 lines
Quoted 28 lines
drivers/i2c/busses/i2c-brcmstb.c new file +27
-0+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 ;Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 317
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+ +/* 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.
Quoted 4 lines
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.
Will make this consistent.
Quoted 37 lines
Quoted 22 lines
Quoted 22 lines
drivers/i2c/busses/i2c-brcmstb.c new file +22
-0+/* 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,Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 347
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+ 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:
Quoted 9 lines
commit
b7f625840267b18ef1011cba0085bb7e237d76f7Author: Wolfram Sang <wsa-z923LK4zBo2bacvFa/[email protected]> Date: Mon Jan 5 23:45:59 2015 +0100i2c: add quirk checks to core
Let the core do the checks if HW quirks prevent a transfer. Saves code from drivers and adds consistency.
Will have a look at this.
Quoted 300 lines
Quoted 48 lines
Quoted 48 lines
drivers/i2c/busses/i2c-brcmstb.c new file +47
-0+ +/* 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); +} +Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 402
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+/* 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
Quoted 119 lines
Quoted 119 lines
drivers/i2c/busses/i2c-brcmstb.c new file +119
-0+/* 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, +};Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 529
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+ +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?
Quoted 34 lines
Quoted 34 lines
drivers/i2c/busses/i2c-brcmstb.c new file +34
-0+ 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;Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 571
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+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_kzallochere?Quoted 9 lines
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c new file +9
-0+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 );Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 588
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-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?Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c line 593
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+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_irqbelow to do it? That's probably fine.Quoted 11 lines
Quoted 11 lines
drivers/i2c/busses/i2c-brcmstb.c new file +11
-0+ +/* 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) {Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 612
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+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...
Quoted 15 lines
Quoted 15 lines
drivers/i2c/busses/i2c-brcmstb.c new file +15
-0+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" );
Quoted 9 lines
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c line 633
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+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?
Will make the change.
Quoted 33 lines
Quoted 33 lines
Quoted 33 lines
drivers/i2c/busses/i2c-brcmstb.c new file +33
-0+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 ; +} +
Quoted 8 lines
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c line 672
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-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?
Yes thats right.
Quoted 37 lines
Quoted 25 lines
Quoted 25 lines
drivers/i2c/busses/i2c-brcmstb.c new file +25
-0+ 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,Quoted 8 lines
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-1+}; +module_platform_driver (brcmstb_i2c_driver); + +MODULE_AUTHOR ("Kamal Dasu <kdasu-dY08KVG/lbpWk0Htik3J/[email protected]>" ); +MODULE_DESCRIPTION ("Broadcom Settop I2C Driver" ); +MODULE_LICENSE ("GPL v2" ); --2.3.2
Thanks,
Ray
Quoted 71 lines · Kamal Dasu
Adding support for i2c controller driver for Broadcom settop SoCs.
Signed-off-by Kamal Dasu <[email protected]>---
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.cdrivers/i2c/busses/Kconfig +10
-0@@ -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 + 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 BLACKFINdrivers/i2c/busses/Makefile +1
-0@@ -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.odrivers/i2c/busses/i2c-brcmstb.c new file +17
-0@@ -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>
drivers/i2c/busses/i2c-brcmstb.c line 27
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+#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> +
includes to be sorted in alphabetic order.
Quoted 50 lines
drivers/i2c/busses/i2c-brcmstb.c new file +49
-0+#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 +#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 {
drivers/i2c/busses/i2c-brcmstb.c line 86
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+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; +}; +
Could you please add a single line description for few members like ctrl high register
Quoted 21 lines
drivers/i2c/busses/i2c-brcmstb.c new file +21
-0+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 {
drivers/i2c/busses/i2c-brcmstb.c line 117
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+SPD_375K , +SPD_390K , +SPD_187K , +SPD_200K , +SPD_93K , +SPD_97K , +SPD_46K , + SPD_50K +}; +
Is there any possibility to make it in increasing/decreasing order of speed like
enum bus_speeds {
SPD_390K,
SPD_375K,
SPD_200K,
SPD_187K,
SPD_97K,
SPD_93K,
SPD_50K,
SPD_46K
};Quoted 93 lines
drivers/i2c/busses/i2c-brcmstb.c new file +93
-0+static const struct bsc_clk_param 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 ; +barrier (); +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); +} + +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);
drivers/i2c/busses/i2c-brcmstb.c line 220
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+ +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); +
you can not do complete_all at this place.
Is there no need to enable interrupt before going out ?
Quoted 80 lines
drivers/i2c/busses/i2c-brcmstb.c new file +80
-0+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 ; + } + } +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 ) { +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); +brcmstb_i2c_disable_irq (dev); + } + +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 ; +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); +
drivers/i2c/busses/i2c-brcmstb.c line 310
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+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 ; +
Place direction setting in xfer function after checking pmsg->flags for read/write.
drivers/i2c/busses/i2c-brcmstb.c line 313
Quoted 3 lines
drivers/i2c/busses/i2c-brcmstb.c new file +3
-0+/* enable BSC CTL interrupt line */ +brcmstb_i2c_enable_irq (dev); +
Is it right place to enable interrupt at this place ?
drivers/i2c/busses/i2c-brcmstb.c line 319
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+/* initiate transfer by setting iic_enable */ + pi2creg->iic_enable |=BSC_IIC_EN_ENABLE_MASK ; +bsc_writel (dev, pi2creg->iic_enable, iic_enable); + +/* Wait for transaction to finish or timeout */ + rc =brcmstb_i2c_waitforcompletion (dev);
can you make brcmstb_i2c_waitforcompletion to brcmstb_i2c_wait_for_completion,
Quoted 15 lines
drivers/i2c/busses/i2c-brcmstb.c new file +15
-0+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);
drivers/i2c/busses/i2c-brcmstb.c line 344
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+ +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; +
Use variable with proper name.
Quoted 185 lines
drivers/i2c/busses/i2c-brcmstb.c new file +185
-0+/* set the read/write length */ +bsc_writel (dev,BSC_CNT_REG1_MASK & (len <<BSC_CNT_REG1_SHIFT ), + cnt_reg); + +/* 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 ) { + +/* 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) {
drivers/i2c/busses/i2c-brcmstb.c line 539
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+ 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) {
This check is not required. In normal condition you are not going to get num_speeds = 0
drivers/i2c/busses/i2c-brcmstb.c line 545
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+ 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);
Make it in single dev_warn.
Quoted 87 lines
drivers/i2c/busses/i2c-brcmstb.c new file +87
-0+ } +} + +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 ); +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); +if (rc <0 ) + int_name =NULL ; + +/* Get the interrupt number */ + dev->irq =platform_get_irq (pdev,0 ); + +/* 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 ; +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; + } +
drivers/i2c/busses/i2c-brcmstb.c line 642
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+brcmstb_i2c_set_bsc_reg_defaults (dev); +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;
Oops you may return wrong 'rc' at this place.
drivers/i2c/busses/i2c-brcmstb.c line 651
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c new file +9
-0+} + +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); +
No issue, but it better if you delete adapter first and then do kfree.
~Rajeev
Quoted 59 lines
drivers/i2c/busses/i2c-brcmstb.c new file +52
-1+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); + 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 <kdasu-dY08KVG/lbpWk0Htik3J/[email protected]>" ); +MODULE_DESCRIPTION ("Broadcom Settop I2C Driver" ); +MODULE_LICENSE ("GPL v2" ); --2.3.2
--
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
A message that is not in the archive (sent off-list or not yet received): CAA7nrtjWUZOBQiTr0YrZFmLOoAttyJ5gMxb+hR7jnz1HC51Buw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Hi Rajeev,
thanks for the review. Much appreciated!
Wolfram
Rajeev,
Thanks for the review.
Answers inline.
Kamal
Quoted 76 lines · rajeev kumar
Quoted 75 lines · Kamal Dasu
Adding support for i2c controller driver for Broadcom settop SoCs.
Signed-off-by Kamal Dasu <[email protected]>---
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.cdrivers/i2c/busses/Kconfig +10
-0@@ -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 + 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 BLACKFINdrivers/i2c/busses/Makefile +1
-0@@ -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.odrivers/i2c/busses/i2c-brcmstb.c new file +21
-0@@ -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>
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 27
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+#include <linux/io.h> +#include <linux/clk.h> +#include <linux/slab.h> +#include <linux/version.h> +#include <linux/delay.h> +includes to be sorted in alphabetic order.
Will make this change.
Quoted 55 lines
Quoted 55 lines
drivers/i2c/busses/i2c-brcmstb.c new file +54
-0+#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 +#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;
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 86
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+u32 data_out[N_DATA_REGS ]; +u32 ctlhi_reg; +u32 scl_param; +}; +Could you please add a single line description for few members like ctrl high register
sure can do that.
Quoted 33 lines
Quoted 21 lines
drivers/i2c/busses/i2c-brcmstb.c new file +21
-0+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 {drivers/i2c/busses/i2c-brcmstb.c line 117
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+SPD_375K , +SPD_390K , +SPD_187K , +SPD_200K , +SPD_93K , +SPD_97K , +SPD_46K , + SPD_50K +}; +Is there any possibility to make it in increasing/decreasing order of speed like
Quoted 12 lines
enum bus_speeds {
SPD_390K,
SPD_375K,
SPD_200K,
SPD_187K,
SPD_97K,
SPD_93K,
SPD_50K,
SPD_46K
};I kept it in the this order to match the mask used in table below. Maybe I can rename to be clear.
Quoted 98 lines
Quoted 98 lines
drivers/i2c/busses/i2c-brcmstb.c new file +98
-0+static const struct bsc_clk_param 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 ; +barrier (); +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); +} + +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); + +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 ))
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 220
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+return IRQ_NONE ; + +brcmstb_i2c_disable_irq (dev); +complete_all (&dev->done); +you can not do
complete_allat this place.
Is there no need to enable interrupt before going out ?
This interrupt is for a i2c master xfer completion, so when the next xfer is initiated by the user the interrupt is enabled in the brcmstb_send_i2c_cmd().
Quoted 86 lines
Quoted 86 lines
drivers/i2c/busses/i2c-brcmstb.c new file +85
-0+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 ; + } + } +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 ) { +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); +brcmstb_i2c_disable_irq (dev); + } + +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 ; +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 ;
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 310
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+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 ; +Place direction setting in xfer function after checking
pmsg->flagsfor read/write.
will try to do that.
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c line 313
Quoted 3 lines
drivers/i2c/busses/i2c-brcmstb.c new file +3
-0+/* enable BSC CTL interrupt line */ +brcmstb_i2c_enable_irq (dev); +Is it right place to enable interrupt at this place ?
Yes this is the place where everything is setup and we just enable the completion interrupt and the iic_enable to intiate transfer.
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 319
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+/* initiate transfer by setting iic_enable */ + pi2creg->iic_enable |=BSC_IIC_EN_ENABLE_MASK ; +bsc_writel (dev, pi2creg->iic_enable, iic_enable); + +/* Wait for transaction to finish or timeout */ + rc =brcmstb_i2c_waitforcompletion (dev);can you make
brcmstb_i2c_waitforcompletiontobrcmstb_i2c_wait_for_completion,
Will do that.
Quoted 19 lines
Quoted 19 lines
drivers/i2c/busses/i2c-brcmstb.c new file +19
-0+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; +} +
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 344
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+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; +Use variable with proper name.
Ok.
Quoted 190 lines
Quoted 190 lines
drivers/i2c/busses/i2c-brcmstb.c new file +190
-0+/* set the read/write length */ +bsc_writel (dev,BSC_CNT_REG1_MASK & (len <<BSC_CNT_REG1_SHIFT ), + cnt_reg); + +/* 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 ) { + +/* 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) { + 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);
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 539
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+break ; + } + } + +if (i == num_speeds) {This check is not required. In normal condition you are not going to get
num_speeds= 0
Actually this is checking if a proper speed setting was passed, else uses the defaults. i = num_speeds in that case as it iterated through all the available speeds.
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 545
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+ 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);Make it in single
dev_warn.
Will do that.
Quoted 91 lines
Quoted 91 lines
drivers/i2c/busses/i2c-brcmstb.c new file +91
-0+ } +} + +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 ); +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); +if (rc <0 ) + int_name =NULL ; + +/* Get the interrupt number */ + dev->irq =platform_get_irq (pdev,0 ); + +/* 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 ; +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); +dev_info (dev->device,"%s@%dhz registered in %s mode\n" , + int_name ? int_name :" " , dev->clk_freq_hz, + (dev->irq >=0 ) ?"interrupt" :"polling" );
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 642
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+ +return 0 ; + +probe_errorout: +kfree (dev->bsc_regmap); +return rc;Oops you may return wrong 'rc' at this place.
Which code path are you referring to. Can you please point it out.
Quoted 74 lines
drivers/i2c/busses/i2c-brcmstb.c line 651
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +9
-0+} + +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); +No issue, but it better if you delete adapter first and then do kfree.
~Rajeev
Quoted 59 lines
drivers/i2c/busses/i2c-brcmstb.c new file +52
-1+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); + 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 <kdasu-dY08KVG/lbpWk0Htik3J/[email protected]>" ); +MODULE_DESCRIPTION ("Broadcom Settop I2C Driver" ); +MODULE_LICENSE ("GPL v2" ); --2.3.2
--
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
A message that is not in the archive (sent off-list or not yet received): CAC=U0a3OXKBSUeRnbsXZ2J7snhRzhGAFctvzjwg=o90w59oNHg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 263 lines · Kamal Dasu
Rajeev,
Thanks for the review.
Answers inline.Kamal
Quoted 76 lines · rajeev kumar
Quoted 75 lines · Kamal Dasu
Adding support for i2c controller driver for Broadcom settop SoCs.
Signed-off-by Kamal Dasu <[email protected]>---
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.cdrivers/i2c/busses/Kconfig +10
-0@@ -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 + 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 BLACKFINdrivers/i2c/busses/Makefile +1
-0@@ -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.odrivers/i2c/busses/i2c-brcmstb.c new file +21
-0@@ -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> Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 27
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+#include <linux/io.h> +#include <linux/clk.h> +#include <linux/slab.h> +#include <linux/version.h> +#include <linux/delay.h> +includes to be sorted in alphabetic order.
Will make this change.
Quoted 55 lines
Quoted 55 lines
drivers/i2c/busses/i2c-brcmstb.c new file +54
-0+#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 +#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;Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 86
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+u32 data_out[N_DATA_REGS ]; +u32 ctlhi_reg; +u32 scl_param; +}; +Could you please add a single line description for few members like ctrl high register
sure can do that.
Quoted 33 lines
Quoted 21 lines
drivers/i2c/busses/i2c-brcmstb.c new file +21
-0+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 {drivers/i2c/busses/i2c-brcmstb.c line 117
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+SPD_375K , +SPD_390K , +SPD_187K , +SPD_200K , +SPD_93K , +SPD_97K , +SPD_46K , + SPD_50K +}; +Is there any possibility to make it in increasing/decreasing order of speed like
Quoted 12 lines
enum bus_speeds { SPD_390K, SPD_375K, SPD_200K, SPD_187K, SPD_97K, SPD_93K, SPD_50K, SPD_46K };I kept it in the this order to match the mask used in table below. Maybe I can rename to be clear.
Quoted 50 lines
Quoted 50 lines
drivers/i2c/busses/i2c-brcmstb.c new file +50
-0+static const struct bsc_clk_param 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;
Quoted 8 lines
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c line 175
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+struct completion done; +bool is_suspended; +u32 clk_freq_hz; +}; + +#define bsc_readl (_dev, reg) \ + __bsc_readl(_dev, offsetof(struct bsc_regs, reg)) +
use readl/writel
Quoted 50 lines
Quoted 40 lines
Quoted 40 lines
drivers/i2c/busses/i2c-brcmstb.c new file +40
-0+#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 ; +barrier (); +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); +} + +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); + +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 ))Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 220
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+return IRQ_NONE ; + +brcmstb_i2c_disable_irq (dev); +complete_all (&dev->done); +you can not do
complete_allat this place.
Is there no need to enable interrupt before going out ?
Quoted 4 lines
This interrupt is for a i2c master xfer completion, so when the next xfer is initiated by the user the interrupt is enabled in the
brcmstb_send_i2c_cmd().
I would say check for complete_all. This is not the right place to perform this. This is interrupt handler.
Quoted 102 lines
Quoted 86 lines
Quoted 86 lines
drivers/i2c/busses/i2c-brcmstb.c new file +85
-0+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 ; + } + } +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 ) { +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); +brcmstb_i2c_disable_irq (dev); + } + +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 ; +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 ;Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 310
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+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 ; +Place direction setting in xfer function after checking
pmsg->flagsfor read/write.will try to do that.
Quoted 4 lines
Quoted 3 lines
drivers/i2c/busses/i2c-brcmstb.c new file +3
-0+/* enable BSC CTL interrupt line */ +brcmstb_i2c_enable_irq (dev); +
Quoted 6 lines
Quoted 2 lines
Is it right place to enable interrupt at this place ?
Yes this is the place where everything is setup and we just enable the completion interrupt and the
iic_enableto intiate transfer.
Your driver starts in polling mode ??
~Rajeev
Quoted 436 lines
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 319
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+/* initiate transfer by setting iic_enable */ + pi2creg->iic_enable |=BSC_IIC_EN_ENABLE_MASK ; +bsc_writel (dev, pi2creg->iic_enable, iic_enable); + +/* Wait for transaction to finish or timeout */ + rc =brcmstb_i2c_waitforcompletion (dev);can you make
brcmstb_i2c_waitforcompletiontobrcmstb_i2c_wait_for_completion,Will do that.
Quoted 19 lines
Quoted 19 lines
drivers/i2c/busses/i2c-brcmstb.c new file +19
-0+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; +} +Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 344
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+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; +Use variable with proper name.
Ok.
Quoted 190 lines
Quoted 190 lines
drivers/i2c/busses/i2c-brcmstb.c new file +190
-0+/* set the read/write length */ +bsc_writel (dev,BSC_CNT_REG1_MASK & (len <<BSC_CNT_REG1_SHIFT ), + cnt_reg); + +/* 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 ) { + +/* 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) { + 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);Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 539
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+break ; + } + } + +if (i == num_speeds) {This check is not required. In normal condition you are not going to get
num_speeds= 0Actually this is checking if a proper speed setting was passed, else uses the defaults. i =
num_speedsin that case as it iterated through all the available speeds.Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 545
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+ 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);Make it in single
dev_warn.Will do that.
Quoted 91 lines
Quoted 91 lines
drivers/i2c/busses/i2c-brcmstb.c new file +91
-0+ } +} + +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 ); +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); +if (rc <0 ) + int_name =NULL ; + +/* Get the interrupt number */ + dev->irq =platform_get_irq (pdev,0 ); + +/* 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 ; +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); +dev_info (dev->device,"%s@%dhz registered in %s mode\n" , + int_name ? int_name :" " , dev->clk_freq_hz, + (dev->irq >=0 ) ?"interrupt" :"polling" );Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 642
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+ +return 0 ; + +probe_errorout: +kfree (dev->bsc_regmap); +return rc;Oops you may return wrong 'rc' at this place.
Which code path are you referring to. Can you please point it out.
Quoted 74 lines
drivers/i2c/busses/i2c-brcmstb.c line 651
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +9
-0+} + +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); +No issue, but it better if you delete adapter first and then do kfree.
~Rajeev
Quoted 59 lines
drivers/i2c/busses/i2c-brcmstb.c new file +52
-1+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); + 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 <kdasu-dY08KVG/lbpWk0Htik3J/[email protected]>" ); +MODULE_DESCRIPTION ("Broadcom Settop I2C Driver" ); +MODULE_LICENSE ("GPL v2" ); --2.3.2
--
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
A message that is not in the archive (sent off-list or not yet received): CAA7nrtjyVXkMYTL8AWMMp1peZ0TGMwYtYhRxrTrBH3kyZzi1tw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 326 lines · rajeev kumar
Quoted 263 lines · Kamal Dasu
Rajeev,
Thanks for the review.
Answers inline.Kamal
Quoted 76 lines · rajeev kumar
Quoted 75 lines · Kamal Dasu
Adding support for i2c controller driver for Broadcom settop SoCs.
Signed-off-by Kamal Dasu <[email protected]>---
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.cdrivers/i2c/busses/Kconfig +10
-0@@ -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 + 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 BLACKFINdrivers/i2c/busses/Makefile +1
-0@@ -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.odrivers/i2c/busses/i2c-brcmstb.c new file +21
-0@@ -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> Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c line 27
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+#include <linux/io.h> +#include <linux/clk.h> +#include <linux/slab.h> +#include <linux/version.h> +#include <linux/delay.h> +includes to be sorted in alphabetic order.
Will make this change.
Quoted 55 lines
Quoted 55 lines
drivers/i2c/busses/i2c-brcmstb.c new file +54
-0+#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 +#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;Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+u32 data_out[N_DATA_REGS ]; +u32 ctlhi_reg; +u32 scl_param; +}; +Could you please add a single line description for few members like ctrl high register
sure can do that.
Quoted 33 lines
Quoted 21 lines
drivers/i2c/busses/i2c-brcmstb.c new file +21
-0+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 {drivers/i2c/busses/i2c-brcmstb.c
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+SPD_375K , +SPD_390K , +SPD_187K , +SPD_200K , +SPD_93K , +SPD_97K , +SPD_46K , + SPD_50K +}; +Is there any possibility to make it in increasing/decreasing order of speed like
Quoted 12 lines
enum bus_speeds { SPD_390K, SPD_375K, SPD_200K, SPD_187K, SPD_97K, SPD_93K, SPD_50K, SPD_46K };I kept it in the this order to match the mask used in table below. Maybe I can rename to be clear.
Quoted 50 lines
Quoted 50 lines
drivers/i2c/busses/i2c-brcmstb.c new file +50
-0+static const struct bsc_clk_param 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;Quoted 8 lines
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c
Quoted 8 lines
drivers/i2c/busses/i2c-brcmstb.c new file +8
-0+struct completion done; +bool is_suspended; +u32 clk_freq_hz; +}; + +#define bsc_readl (_dev, reg) \ + __bsc_readl(_dev, offsetof(struct bsc_regs, reg)) +use readl/writel
Quoted 50 lines
Quoted 49 lines
Quoted 35 lines
drivers/i2c/busses/i2c-brcmstb.c new file +35
-0+#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 ; +barrier (); +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); +} + +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);drivers/i2c/busses/i2c-brcmstb.c
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +10
-0+ +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); +you can not do
complete_allat this place.
Is there no need to enable interrupt before going out ?
Quoted 8 lines
Quoted 4 lines
This interrupt is for a i2c master xfer completion, so when the next xfer is initiated by the user the interrupt is enabled in the
brcmstb_send_i2c_cmd().I would say check for
complete_all. This is not the right place to perform this. This is interrupt handler.
Ok with this. Can ignore this comment..
~Rajeev
Quoted 549 lines
Quoted 102 lines
Quoted 86 lines
Quoted 86 lines
drivers/i2c/busses/i2c-brcmstb.c new file +85
-0+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 ; + } + } +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 ) { +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); +brcmstb_i2c_disable_irq (dev); + } + +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 ; +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 ;Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+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 ; +Place direction setting in xfer function after checking
pmsg->flagsfor read/write.will try to do that.
Quoted 4 lines
Quoted 3 lines
drivers/i2c/busses/i2c-brcmstb.c new file +3
-0+/* enable BSC CTL interrupt line */ +brcmstb_i2c_enable_irq (dev); +Quoted 6 lines
Quoted 2 lines
Is it right place to enable interrupt at this place ?
Yes this is the place where everything is setup and we just enable the completion interrupt and the
iic_enableto intiate transfer.Your driver starts in polling mode ??
~Rajeev
Quoted 436 lines
Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+/* initiate transfer by setting iic_enable */ + pi2creg->iic_enable |=BSC_IIC_EN_ENABLE_MASK ; +bsc_writel (dev, pi2creg->iic_enable, iic_enable); + +/* Wait for transaction to finish or timeout */ + rc =brcmstb_i2c_waitforcompletion (dev);can you make
brcmstb_i2c_waitforcompletiontobrcmstb_i2c_wait_for_completion,Will do that.
Quoted 19 lines
Quoted 19 lines
drivers/i2c/busses/i2c-brcmstb.c new file +19
-0+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; +} +Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+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; +Use variable with proper name.
Ok.
Quoted 190 lines
Quoted 190 lines
drivers/i2c/busses/i2c-brcmstb.c new file +190
-0+/* set the read/write length */ +bsc_writel (dev,BSC_CNT_REG1_MASK & (len <<BSC_CNT_REG1_SHIFT ), + cnt_reg); + +/* 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 ) { + +/* 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) { + 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);Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c
Quoted 5 lines
drivers/i2c/busses/i2c-brcmstb.c new file +5
-0+break ; + } + } + +if (i == num_speeds) {This check is not required. In normal condition you are not going to get
num_speeds= 0Actually this is checking if a proper speed setting was passed, else uses the defaults. i =
num_speedsin that case as it iterated through all the available speeds.Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+ 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);Make it in single
dev_warn.Will do that.
Quoted 91 lines
Quoted 91 lines
drivers/i2c/busses/i2c-brcmstb.c new file +91
-0+ } +} + +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 ); +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); +if (rc <0 ) + int_name =NULL ; + +/* Get the interrupt number */ + dev->irq =platform_get_irq (pdev,0 ); + +/* 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 ; +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); +dev_info (dev->device,"%s@%dhz registered in %s mode\n" , + int_name ? int_name :" " , dev->clk_freq_hz, + (dev->irq >=0 ) ?"interrupt" :"polling" );Quoted 9 lines
drivers/i2c/busses/i2c-brcmstb.c
Quoted 6 lines
drivers/i2c/busses/i2c-brcmstb.c new file +6
-0+ +return 0 ; + +probe_errorout: +kfree (dev->bsc_regmap); +return rc;Oops you may return wrong 'rc' at this place.
Which code path are you referring to. Can you please point it out.
Quoted 74 lines
drivers/i2c/busses/i2c-brcmstb.c
Quoted 10 lines
drivers/i2c/busses/i2c-brcmstb.c new file +9
-0+} + +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); +No issue, but it better if you delete adapter first and then do kfree.
~Rajeev
Quoted 59 lines
drivers/i2c/busses/i2c-brcmstb.c new file +52
-1+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); + 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 <kdasu-dY08KVG/lbpWk0Htik3J/[email protected]>" ); +MODULE_DESCRIPTION ("Broadcom Settop I2C Driver" ); +MODULE_LICENSE ("GPL v2" ); --2.3.2
--
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
Quoted 5 lines · rajeev kumar
Quoted 3 lines
Quoted 3 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+#define bsc_readl(_dev, reg) \ + __bsc_readl(_dev, offsetof(struct bsc_regs, reg)) +use readl/writel
This peripheral is used on chips that can run in either little or big-endian, and in both cases, this is a board-level strap that dictates the entire endianess of the system, such that on-chip peripherals should always be accessed in the host (native) endianess.
readl/writel introduce an implicit endian conversion for kernels compiled in big-endian, that we do not want here. Depending on the architecture, there is also an additional barrier that is not needed, since the underlying bus serving this peripheral (GISB) does not re-order transactions, and writes are not posted.
A better abstraction however could be something like this:
#ifdef CONFIG_CPU_BIG_ENDIAN
#define __bsc_readl(_dev, reg) ioread32be(...)
#else
#define __bsc_readl(_dev, reg) ioread32(...)
#endif
Such that we preserve the host endianess access, but we clearly identify the differences in running in a big or little endian configuration?
Signature · 1 line
Florian