On Fri, Apr 17, 2015 at 11:22 AM, rajeev kumar
<rajeevkumar.linux@xxxxxxxxx> wrote:
> On Thu, Apr 16, 2015 at 9:40 PM, Kamal Dasu <kdasu.kdev@xxxxxxxxx> wrote:
>> Rajeev,
>>
>> Thanks for the review.
>> Answers inline.
>>
>> Kamal
>>
>> On Wed, Apr 15, 2015 at 3:21 AM, rajeev kumar
>> <rajeevkumar.linux@xxxxxxxxx> wrote:
>>> On Fri, Apr 3, 2015 at 1:31 AM, Kamal Dasu <kdasu.kdev@xxxxxxxxx> wrote:
>>>> Adding support for i2c controller driver for Broadcom settop
>>>> SoCs.
>>>>
>>>> Signed-off-by: Kamal Dasu <kdasu.kdev@xxxxxxxxx>
>>>> ---
>>>> V2 changes
>>>> - Separate commits for device tree bindings and
>>>> initial i2c-brcmstb driver code
>>>> ---
>>>> drivers/i2c/busses/Kconfig | 10 +
>>>> drivers/i2c/busses/Makefile | 1 +
>>>> drivers/i2c/busses/i2c-brcmstb.c | 703 +++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 714 insertions(+)
>>>> create mode 100644 drivers/i2c/busses/i2c-brcmstb.c
>>>>
>>>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>>>> index 22da9c2..e3938f4 100644
>>>> --- a/drivers/i2c/busses/Kconfig
>>>> +++ b/drivers/i2c/busses/Kconfig
>>>> @@ -392,6 +392,16 @@ config I2C_BCM_KONA
>>>>
>>>> If you do not need KONA I2C interface, say N.
>>>>
>>>> +config I2C_BRCMSTB
>>>> + tristate "BRCM Settop I2C adapter"
>>>> + depends on ARCH_BRCMSTB
>>>> + default y
>>>> + help
>>>> + If you say yes to this option, support will be included for the
>>>> + I2C interface on the Broadcom Settop SoCs.
>>>> +
>>>> + If you do not need I2C interface, say N.
>>>> +
>>>> config I2C_BLACKFIN_TWI
>>>> tristate "Blackfin TWI I2C support"
>>>> depends on BLACKFIN
>>>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>>>> index 3638feb..91a0b5f 100644
>>>> --- a/drivers/i2c/busses/Makefile
>>>> +++ b/drivers/i2c/busses/Makefile
>>>> @@ -102,6 +102,7 @@ obj-$(CONFIG_I2C_VIPERBOARD) += i2c-viperboard.o
>>>> # Other I2C/SMBus bus drivers
>>>> obj-$(CONFIG_I2C_ACORN) += i2c-acorn.o
>>>> obj-$(CONFIG_I2C_BCM_KONA) += i2c-bcm-kona.o
>>>> +obj-$(CONFIG_I2C_BRCMSTB) += i2c-brcmstb.o
>>>> obj-$(CONFIG_I2C_CROS_EC_TUNNEL) += i2c-cros-ec-tunnel.o
>>>> obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o
>>>> obj-$(CONFIG_I2C_OPAL) += i2c-opal.o
>>>> diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
>>>> new file mode 100644
>>>> index 0000000..b69ab14
>>>> --- /dev/null
>>>> +++ b/drivers/i2c/busses/i2c-brcmstb.c
>>>> @@ -0,0 +1,703 @@
>>>> +/*
>>>> + * Copyright (C) 2014 Broadcom Corporation
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU General Public License as
>>>> + * published by the Free Software Foundation version 2.
>>>> + *
>>>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>>>> + * kind, whether express or implied; without even the implied warranty
>>>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/sched.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/clk.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/clk.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/version.h>
>>>> +#include <linux/delay.h>
>>>> +
>>>
>>> includes to be sorted in alphabetic order.
>>>
>>
>> Will make this change.
>>
>>>> +#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;
>>>> +};
>>>> +
>>>
>>> Could you please add a single line description for few members like
>>> ctrl high register
>>>
>>
>> sure can do that.
>>
>>>> +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
>>>> +};
>>>> +
>>>
>>> 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
>>> };
>>>
>>>
>>
>> I kept it in the this order to match the mask used in table below.
>> Maybe I can rename to be clear.
>>
>>>> +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))
>>>> +
>
> use readl/writel
>
>
>>>> +#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);
>>>> +
>>>
>>> you can not do complete_all at 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().
>>
>
> 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
>>>
>>>> + 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;
>>>> +
>>>
>>> Place direction setting in xfer function after checking pmsg->flags
>>> for read/write.
>>>
>>
>> will try to do that.
>>
>>>> + /* 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.
>>
>
> Your driver starts in polling mode ??
>
> ~Rajeev
>
>>>> + /* 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,
>>>
>>
>> Will do that.
>>
>>>> + 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;
>>>> +
>>>
>>> Use variable with proper name.
>>>
>>
>> Ok.
>>
>>>> + /* 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) {
>>>
>>> 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.
>>
>>>> + 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.
>>
>>>> + }
>>>> +}
>>>> +
>>>> +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;
>>>
>>> Oops you may return wrong 'rc' at this place.
>>>
>>
>> Which code path are you referring to. Can you please point it out.
>>
>>>
>>>> +}
>>>> +
>>>> +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
>>>
>>>> + 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@xxxxxxxxxxxx>");
>>>> +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 majordomo@xxxxxxxxxxxxxxx
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
