* [PATCH v5 5/7] i2c: designware: add SLAVE mode functions
From: Luis Oliveira @ 2016-12-28 14:43 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <cover.1482934380.git.lolivei@synopsys.com>
- Changes in Kconfig to enable I2C_SLAVE support
- Slave functions added to core library file
- Slave abort sources added to common source file
- New driver: i2c-designware-slave added
- Changes in the Makefile to compile it all
All the SLAVE flow is added but it is not enabled via platform
driver.
Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V4->V5: (Andy Shevchenko)
- This patch adds the SLAVE support functions but it still not enable it
via platform module. I think it is more readable now
drivers/i2c/busses/Kconfig | 3 +-
drivers/i2c/busses/Makefile | 2 +-
drivers/i2c/busses/i2c-designware-common.c | 10 +-
drivers/i2c/busses/i2c-designware-core.h | 6 +
drivers/i2c/busses/i2c-designware-slave.c | 434 +++++++++++++++++++++++++++++
5 files changed, 452 insertions(+), 3 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-designware-slave.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 0cdc8443deab..b58fdcde93a7 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -469,11 +469,12 @@ config I2C_DESIGNWARE_CORE
config I2C_DESIGNWARE_PLATFORM
tristate "Synopsys DesignWare Platform"
+ select I2C_SLAVE
select I2C_DESIGNWARE_CORE
depends on (ACPI && COMMON_CLK) || !ACPI
help
If you say yes to this option, support will be included for the
- Synopsys DesignWare I2C adapter. Only master mode is supported.
+ Synopsys DesignWare I2C adapter.
This driver can also be built as a module. If so, the module
will be called i2c-designware-platform.
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 4f8f6a2b9346..c2ed84a86f49 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -40,7 +40,7 @@ obj-$(CONFIG_I2C_CBUS_GPIO) += i2c-cbus-gpio.o
obj-$(CONFIG_I2C_CPM) += i2c-cpm.o
obj-$(CONFIG_I2C_DAVINCI) += i2c-davinci.o
obj-$(CONFIG_I2C_DESIGNWARE_CORE) += i2c-designware-core.o
-i2c-designware-core-objs := i2c-designware-common.o i2c-designware-master.o
+i2c-designware-core-objs := i2c-designware-common.o i2c-designware-slave.o i2c-designware-master.o
obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM) += i2c-designware-platform.o
i2c-designware-platform-objs := i2c-designware-platdrv.o
i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-baytrail.o
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 012b8f9dec15..b3beba639e98 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -30,6 +30,7 @@
#include <linux/pm_runtime.h>
#include <linux/delay.h>
#include <linux/module.h>
+
#include "i2c-designware-core.h"
static char *abort_sources[] = {
@@ -42,7 +43,7 @@ static char *abort_sources[] = {
[ABRT_TXDATA_NOACK] =
"data not acknowledged",
[ABRT_GCALL_NOACK] =
- "no acknowledgement for a general call",
+ "no acknowledgment for a general call",
[ABRT_GCALL_READ] =
"read after general call",
[ABRT_SBYTE_ACKDET] =
@@ -55,6 +56,13 @@ static char *abort_sources[] = {
"trying to use disabled adapter",
[ARB_LOST] =
"lost arbitration",
+ [ABRT_SLAVE_FLUSH_TXFIFO] =
+ "read command so flush old data in the TX FIFO",
+ [ABRT_SLAVE_ARBLOST] =
+ "slave lost the bus while transmitting data to a remote master",
+ [ABRT_SLAVE_RD_INTX] =
+ "slave request for data to be transmitted and there is a 1 in "
+ "bit 8 of IC_DATA_CMD",
};
u32 dw_readl(struct dw_i2c_dev *dev, int offset)
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 5080f1d2d2ec..1729b6bb5239 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -271,6 +271,7 @@ struct dw_i2c_dev {
void (*release_lock)(struct dw_i2c_dev *dev);
bool pm_runtime_disabled;
bool dynamic_tar_update_enabled;
+ bool mode;
};
#define ACCESS_SWAP 0x00000001
@@ -295,6 +296,11 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
extern int i2c_dw_probe(struct dw_i2c_dev *dev);
+extern int i2c_dw_init_slave(struct dw_i2c_dev *dev);
+extern void i2c_dw_disable_slave(struct dw_i2c_dev *dev);
+extern void i2c_dw_disable_int_slave(struct dw_i2c_dev *dev);
+extern u32 i2c_dw_read_comp_param_slave(struct dw_i2c_dev *dev);
+extern int i2c_dw_probe_slave(struct dw_i2c_dev *dev);
#if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
new file mode 100644
index 000000000000..5afc88d9e6b7
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -0,0 +1,434 @@
+/*
+ * Synopsys DesignWare I2C adapter driver (slave only).
+ *
+ * Based on the Synopsys DesignWare I2C adapter driver (master).
+ *
+ * Copyright (C) 2016 Synopsys Inc.
+ *
+ * ----------------------------------------------------------------------------
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ * ----------------------------------------------------------------------------
+ *
+ */
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+
+#include "i2c-designware-core.h"
+
+static void i2c_dw_configure_fifo_slave(struct dw_i2c_dev *dev)
+{
+ /* Configure Tx/Rx FIFO threshold levels. */
+ dw_writel(dev, 0, DW_IC_TX_TL);
+ dw_writel(dev, 0, DW_IC_RX_TL);
+
+ /* Configure the I2C slave. */
+ dw_writel(dev, dev->slave_cfg, DW_IC_CON);
+ dw_writel(dev, DW_IC_INTR_SLAVE_MASK, DW_IC_INTR_MASK);
+}
+
+/**
+ * i2c_dw_init_slave() - Initialize the designware i2c slave hardware
+ * @dev: device private data
+ *
+ * This functions configures and enables the I2C.
+ * This function is called during I2C init function, and in case of timeout at
+ * run time.
+ */
+int i2c_dw_init_slave(struct dw_i2c_dev *dev)
+{
+ u32 sda_falling_time, scl_falling_time;
+ u32 reg, comp_param1;
+ u32 hcnt, lcnt;
+ int ret;
+
+ ret = i2c_dw_acquire_lock(dev);
+ if (ret)
+ return ret;
+
+ reg = dw_readl(dev, DW_IC_COMP_TYPE);
+ if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
+ /* Configure register endianness access. */
+ dev->accessor_flags |= ACCESS_SWAP;
+ } else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
+ /* Configure register access mode 16bit. */
+ dev->accessor_flags |= ACCESS_16BIT;
+ } else if (reg != DW_IC_COMP_TYPE_VALUE) {
+ dev_err(dev->dev,
+ "Unknown Synopsys component type: 0x%08x\n", reg);
+ i2c_dw_release_lock(dev);
+ return -ENODEV;
+ }
+
+ comp_param1 = dw_readl(dev, DW_IC_COMP_PARAM_1);
+
+ /* Disable the adapter. */
+ __i2c_dw_enable_and_wait(dev, false);
+
+ /* Set standard and fast speed deviders for high/low periods. */
+ sda_falling_time = dev->sda_falling_time ?: 300; /* ns */
+ scl_falling_time = dev->scl_falling_time ?: 300; /* ns */
+
+ /* Set SCL timing parameters for standard-mode. */
+ if (dev->ss_hcnt && dev->ss_lcnt) {
+ hcnt = dev->ss_hcnt;
+ lcnt = dev->ss_lcnt;
+ } else {
+ hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
+ 4000, /* tHD;STA = tHIGH = 4.0 us */
+ sda_falling_time,
+ 0, /* 0: DW default, 1: Ideal */
+ 0); /* No offset */
+ lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+ 4700, /* tLOW = 4.7 us */
+ scl_falling_time,
+ 0); /* No offset */
+ }
+ dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
+ dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
+ dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
+
+ /* Set SCL timing parameters for fast-mode or fast-mode plus. */
+ if ((dev->clk_freq == 1000000) && dev->fp_hcnt && dev->fp_lcnt) {
+ hcnt = dev->fp_hcnt;
+ lcnt = dev->fp_lcnt;
+ } else if (dev->fs_hcnt && dev->fs_lcnt) {
+ hcnt = dev->fs_hcnt;
+ lcnt = dev->fs_lcnt;
+ } else {
+ hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
+ 600, /* tHD;STA = tHIGH = 0.6 us */
+ sda_falling_time,
+ 0, /* 0: DW default, 1: Ideal */
+ 0); /* No offset */
+ lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+ 1300, /* tLOW = 1.3 us */
+ scl_falling_time,
+ 0); /* No offset */
+ }
+ dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT);
+ dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
+ dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
+
+ if ((dev->slave_cfg & DW_IC_CON_SPEED_MASK) ==
+ DW_IC_CON_SPEED_HIGH) {
+ if ((comp_param1 & DW_IC_COMP_PARAM_1_SPEED_MODE_MASK)
+ != DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH) {
+ dev_err(dev->dev, "High Speed not supported!\n");
+ dev->slave_cfg &= ~DW_IC_CON_SPEED_MASK;
+ dev->slave_cfg |= DW_IC_CON_SPEED_FAST;
+ } else if (dev->hs_hcnt && dev->hs_lcnt) {
+ hcnt = dev->hs_hcnt;
+ lcnt = dev->hs_lcnt;
+ dw_writel(dev, hcnt, DW_IC_HS_SCL_HCNT);
+ dw_writel(dev, lcnt, DW_IC_HS_SCL_LCNT);
+ dev_dbg(dev->dev, "HighSpeed-mode HCNT:LCNT = %d:%d\n",
+ hcnt, lcnt);
+ }
+ }
+
+ /* Configure SDA Hold Time if required. */
+ reg = dw_readl(dev, DW_IC_COMP_VERSION);
+ if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
+ if (!dev->sda_hold_time) {
+ /* Keep previous hold time setting if no one set it. */
+ dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
+ }
+ /*
+ * Workaround for avoiding TX arbitration lost in case I2C
+ * slave pulls SDA down "too quickly" after falling egde of
+ * SCL by enabling non-zero SDA RX hold. Specification says it
+ * extends incoming SDA low to high transition while SCL is
+ * high but it apprears to help also above issue.
+ */
+ if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
+ dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT;
+ dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+ } else {
+ dev_warn(dev->dev,
+ "Hardware too old to adjust SDA hold time.\n");
+ }
+
+ i2c_dw_configure_fifo_slave(dev);
+ i2c_dw_release_lock(dev);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_init_slave);
+
+int i2c_dw_reg_slave(struct i2c_client *slave)
+{
+ struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
+
+ if (dev->slave)
+ return -EBUSY;
+ if (slave->flags & I2C_CLIENT_TEN)
+ return -EAFNOSUPPORT;
+ /*
+ * Set slave address in the IC_SAR register,
+ * the address to which the DW_apb_i2c responds.
+ */
+
+ __i2c_dw_enable(dev, false);
+ dw_writel(dev, slave->addr, DW_IC_SAR);
+ dev->slave = slave;
+
+ __i2c_dw_enable(dev, true);
+
+ dev->cmd_err = 0;
+ dev->msg_write_idx = 0;
+ dev->msg_read_idx = 0;
+ dev->msg_err = 0;
+ dev->status = STATUS_IDLE;
+ dev->abort_source = 0;
+ dev->rx_outstanding = 0;
+
+ return 0;
+}
+
+static int i2c_dw_unreg_slave(struct i2c_client *slave)
+{
+ struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
+
+ i2c_dw_disable_int_slave(dev);
+ i2c_dw_disable_slave(dev);
+ dev->slave = NULL;
+
+ return 0;
+}
+
+static u32 i2c_dw_read_clear_intrbits_slave(struct dw_i2c_dev *dev)
+{
+ u32 stat;
+
+ /*
+ * The IC_INTR_STAT register just indicates "enabled" interrupts.
+ * Ths unmasked raw version of interrupt status bits are available
+ * in the IC_RAW_INTR_STAT register.
+ *
+ * That is,
+ * stat = dw_readl(IC_INTR_STAT);
+ * equals to,
+ * stat = dw_readl(IC_RAW_INTR_STAT) & dw_readl(IC_INTR_MASK);
+ *
+ * The raw version might be useful for debugging purposes.
+ */
+ stat = dw_readl(dev, DW_IC_INTR_STAT);
+
+ /*
+ * Do not use the IC_CLR_INTR register to clear interrupts, or
+ * you'll miss some interrupts, triggered during the period from
+ * dw_readl(IC_INTR_STAT) to dw_readl(IC_CLR_INTR).
+ *
+ * Instead, use the separately-prepared IC_CLR_* registers.
+ */
+ if (stat & DW_IC_INTR_TX_ABRT)
+ dw_readl(dev, DW_IC_CLR_TX_ABRT);
+ if (stat & DW_IC_INTR_RX_UNDER)
+ dw_readl(dev, DW_IC_CLR_RX_UNDER);
+ if (stat & DW_IC_INTR_RX_OVER)
+ dw_readl(dev, DW_IC_CLR_RX_OVER);
+ if (stat & DW_IC_INTR_TX_OVER)
+ dw_readl(dev, DW_IC_CLR_TX_OVER);
+ if (stat & DW_IC_INTR_RX_DONE)
+ dw_readl(dev, DW_IC_CLR_RX_DONE);
+ if (stat & DW_IC_INTR_ACTIVITY)
+ dw_readl(dev, DW_IC_CLR_ACTIVITY);
+ if (stat & DW_IC_INTR_STOP_DET)
+ dw_readl(dev, DW_IC_CLR_STOP_DET);
+ if (stat & DW_IC_INTR_START_DET)
+ dw_readl(dev, DW_IC_CLR_START_DET);
+ if (stat & DW_IC_INTR_GEN_CALL)
+ dw_readl(dev, DW_IC_CLR_GEN_CALL);
+
+ return stat;
+}
+
+/*
+ * Interrupt service routine. This gets called whenever an I2C slave interrupt
+ * occurs.
+ */
+
+static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
+{
+ u32 raw_stat, stat, enabled;
+ u8 val, slave_activity;
+
+ stat = dw_readl(dev, DW_IC_INTR_STAT);
+ enabled = dw_readl(dev, DW_IC_ENABLE);
+ raw_stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
+ slave_activity = ((dw_readl(dev, DW_IC_STATUS) &
+ DW_IC_STATUS_SLAVE_ACTIVITY) >> 6);
+
+ if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY))
+ return 0;
+
+ dev_dbg(dev->dev,
+ "%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x : INTR_STAT=%#x\n",
+ __func__, enabled, slave_activity, raw_stat, stat);
+
+ if (stat & DW_IC_INTR_RESTART_DET)
+ dw_readl(dev, DW_IC_CLR_RESTART_DET);
+ if (stat & DW_IC_INTR_START_DET)
+ dw_readl(dev, DW_IC_CLR_START_DET);
+ if (stat & DW_IC_INTR_ACTIVITY)
+ dw_readl(dev, DW_IC_CLR_ACTIVITY);
+ if (stat & DW_IC_INTR_RX_OVER)
+ dw_readl(dev, DW_IC_CLR_RX_OVER);
+ if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))
+ i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
+
+ if (slave_activity) {
+ if (stat & DW_IC_INTR_RD_REQ) {
+ if (stat & DW_IC_INTR_RX_FULL) {
+ val = dw_readl(dev, DW_IC_DATA_CMD);
+ if (!i2c_slave_event(dev->slave,
+ I2C_SLAVE_WRITE_RECEIVED, &val)) {
+ dev_dbg(dev->dev, "Byte %X acked!",
+ val);
+ }
+ dw_readl(dev, DW_IC_CLR_RD_REQ);
+ stat = i2c_dw_read_clear_intrbits_slave(dev);
+ } else {
+ dw_readl(dev, DW_IC_CLR_RD_REQ);
+ dw_readl(dev, DW_IC_CLR_RX_UNDER);
+ stat = i2c_dw_read_clear_intrbits_slave(dev);
+ }
+ if (!i2c_slave_event(dev->slave,
+ I2C_SLAVE_READ_REQUESTED, &val))
+ dw_writel(dev, val, DW_IC_DATA_CMD);
+ }
+ }
+
+ if (stat & DW_IC_INTR_RX_DONE) {
+ if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
+ &val))
+ dw_readl(dev, DW_IC_CLR_RX_DONE);
+
+ i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
+ stat = i2c_dw_read_clear_intrbits_slave(dev);
+ return true;
+ }
+
+ if (stat & DW_IC_INTR_RX_FULL) {
+ val = dw_readl(dev, DW_IC_DATA_CMD);
+ if (!i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_RECEIVED,
+ &val))
+ dev_dbg(dev->dev, "Byte %X acked!", val);
+ } else {
+ i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
+ stat = i2c_dw_read_clear_intrbits_slave(dev);
+ }
+
+ if (stat & DW_IC_INTR_TX_OVER)
+ dw_readl(dev, DW_IC_CLR_TX_OVER);
+
+ return 1;
+}
+
+static irqreturn_t i2c_dw_isr_slave(int this_irq, void *dev_id)
+{
+ struct dw_i2c_dev *dev = dev_id;
+ int ret;
+
+ i2c_dw_read_clear_intrbits_slave(dev);
+ ret = i2c_dw_irq_handler_slave(dev);
+
+ if (ret > 0)
+ complete(&dev->cmd_complete);
+
+ return IRQ_RETVAL(ret);
+}
+
+static struct i2c_algorithm i2c_dw_algo = {
+ .functionality = i2c_dw_func,
+ .reg_slave = i2c_dw_reg_slave,
+ .unreg_slave = i2c_dw_unreg_slave,
+};
+
+void i2c_dw_disable_slave(struct dw_i2c_dev *dev)
+{
+ /* Disable controller. */
+ __i2c_dw_enable_and_wait(dev, false);
+
+ /* Disable all interupts. */
+ dw_writel(dev, 0, DW_IC_INTR_MASK);
+ dw_readl(dev, DW_IC_CLR_INTR);
+}
+EXPORT_SYMBOL_GPL(i2c_dw_disable_slave);
+
+void i2c_dw_disable_int_slave(struct dw_i2c_dev *dev)
+{
+ dw_writel(dev, 0, DW_IC_INTR_MASK);
+}
+EXPORT_SYMBOL_GPL(i2c_dw_disable_int_slave);
+
+u32 i2c_dw_read_comp_param_slave(struct dw_i2c_dev *dev)
+{
+ return dw_readl(dev, DW_IC_COMP_PARAM_1);
+}
+EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param_slave);
+
+int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
+{
+ struct i2c_adapter *adap = &dev->adapter;
+ int ret;
+
+ init_completion(&dev->cmd_complete);
+
+ ret = i2c_dw_init_slave(dev);
+ if (ret)
+ return ret;
+
+ ret = i2c_dw_acquire_lock(dev);
+ if (ret)
+ return ret;
+
+ i2c_dw_release_lock(dev);
+ snprintf(adap->name, sizeof(adap->name),
+ "Synopsys DesignWare I2C Slave adapter");
+ adap->retries = 3;
+ adap->algo = &i2c_dw_algo;
+ adap->dev.parent = dev->dev;
+ i2c_set_adapdata(adap, dev);
+
+ ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr_slave,
+ IRQF_SHARED, dev_name(dev->dev), dev);
+ if (ret) {
+ dev_err(dev->dev, "failure requesting irq %i: %d\n",
+ dev->irq, ret);
+ return ret;
+ }
+ /*
+ * Increment PM usage count during adapter registration in order to
+ * avoid possible spurious runtime suspend when adapter device is
+ * registered to the device core and immediate resume in case bus has
+ * registered I2C slaves that do I2C transfers in their probe.
+ */
+ pm_runtime_get_noresume(dev->dev);
+ ret = i2c_add_numbered_adapter(adap);
+ if (ret)
+ dev_err(dev->dev, "failure adding adapter: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_probe_slave);
+
+MODULE_AUTHOR("Luis Oliveira <lolivei@synopsys.com>");
+MODULE_DESCRIPTION("Synopsys DesignWare I2C bus slave adapter");
+MODULE_LICENSE("GPL v2");
--
2.11.0
^ permalink raw reply related
* [PATCH v5 3/7] i2c: designware: MASTER mode as separated driver
From: Luis Oliveira @ 2016-12-28 14:43 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <cover.1482934380.git.lolivei@synopsys.com>
- The functions related to I2C master mode of operation were transformed
in a single driver.
- Common definitions were moved to i2c-designware-core.h
- The i2c-designware-core is now only a library file, the functions
associated are in a source file called i2c-designware-common and
are used by both i2c-designware-master and i2c-designware-slave.
Almost all of the "core" source is now part of the "master" source. The
difference is the functions used by both modes and they are in the
"common" source file.
Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V4->V5: (ACK by Andy)
- No changes
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-designware-common.c | 253 +++++++++++++++
drivers/i2c/busses/i2c-designware-core.h | 131 ++++++++
...c-designware-core.c => i2c-designware-master.c} | 352 +--------------------
4 files changed, 394 insertions(+), 343 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-designware-common.c
rename drivers/i2c/busses/{i2c-designware-core.c => i2c-designware-master.c} (66%)
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 1c1bac87a9db..4f8f6a2b9346 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_I2C_CBUS_GPIO) += i2c-cbus-gpio.o
obj-$(CONFIG_I2C_CPM) += i2c-cpm.o
obj-$(CONFIG_I2C_DAVINCI) += i2c-davinci.o
obj-$(CONFIG_I2C_DESIGNWARE_CORE) += i2c-designware-core.o
+i2c-designware-core-objs := i2c-designware-common.o i2c-designware-master.o
obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM) += i2c-designware-platform.o
i2c-designware-platform-objs := i2c-designware-platdrv.o
i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-baytrail.o
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
new file mode 100644
index 000000000000..012b8f9dec15
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -0,0 +1,253 @@
+/*
+ * Synopsys DesignWare I2C adapter driver.
+ *
+ * Based on the TI DAVINCI I2C adapter driver.
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ * Copyright (C) 2007 MontaVista Software Inc.
+ * Copyright (C) 2009 Provigent Ltd.
+ *
+ * ----------------------------------------------------------------------------
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ * ----------------------------------------------------------------------------
+ *
+ */
+#include <linux/export.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include "i2c-designware-core.h"
+
+static char *abort_sources[] = {
+ [ABRT_7B_ADDR_NOACK] =
+ "slave address not acknowledged (7bit mode)",
+ [ABRT_10ADDR1_NOACK] =
+ "first address byte not acknowledged (10bit mode)",
+ [ABRT_10ADDR2_NOACK] =
+ "second address byte not acknowledged (10bit mode)",
+ [ABRT_TXDATA_NOACK] =
+ "data not acknowledged",
+ [ABRT_GCALL_NOACK] =
+ "no acknowledgement for a general call",
+ [ABRT_GCALL_READ] =
+ "read after general call",
+ [ABRT_SBYTE_ACKDET] =
+ "start byte acknowledged",
+ [ABRT_SBYTE_NORSTRT] =
+ "trying to send start byte when restart is disabled",
+ [ABRT_10B_RD_NORSTRT] =
+ "trying to read when restart is disabled (10bit mode)",
+ [ABRT_MASTER_DIS] =
+ "trying to use disabled adapter",
+ [ARB_LOST] =
+ "lost arbitration",
+};
+
+u32 dw_readl(struct dw_i2c_dev *dev, int offset)
+{
+ u32 value;
+
+ if (dev->accessor_flags & ACCESS_16BIT)
+ value = readw_relaxed(dev->base + offset) |
+ (readw_relaxed(dev->base + offset + 2) << 16);
+ else
+ value = readl_relaxed(dev->base + offset);
+
+ if (dev->accessor_flags & ACCESS_SWAP)
+ return swab32(value);
+ else
+ return value;
+}
+
+void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
+{
+ if (dev->accessor_flags & ACCESS_SWAP)
+ b = swab32(b);
+
+ if (dev->accessor_flags & ACCESS_16BIT) {
+ writew_relaxed((u16)b, dev->base + offset);
+ writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
+ } else {
+ writel_relaxed(b, dev->base + offset);
+ }
+}
+
+u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
+{
+ /*
+ * DesignWare I2C core doesn't seem to have solid strategy to meet
+ * the tHD;STA timing spec. Configuring _HCNT based on tHIGH spec
+ * will result in violation of the tHD;STA spec.
+ */
+ if (cond)
+ /*
+ * Conditional expression:
+ *
+ * IC_[FS]S_SCL_HCNT + (1+4+3) >= IC_CLK * tHIGH
+ *
+ * This is based on the DW manuals, and represents an ideal
+ * configuration. The resulting I2C bus speed will be
+ * faster than any of the others.
+ *
+ * If your hardware is free from tHD;STA issue, try this one.
+ */
+ return (ic_clk * tSYMBOL + 500000) / 1000000 - 8 + offset;
+ else
+ /*
+ * Conditional expression:
+ *
+ * IC_[FS]S_SCL_HCNT + 3 >= IC_CLK * (tHD;STA + tf)
+ *
+ * This is just experimental rule; the tHD;STA period turned
+ * out to be proportinal to (_HCNT + 3). With this setting,
+ * we could meet both tHIGH and tHD;STA timing specs.
+ *
+ * If unsure, you'd better to take this alternative.
+ *
+ * The reason why we need to take into account "tf" here,
+ * is the same as described in i2c_dw_scl_lcnt().
+ */
+ return (ic_clk * (tSYMBOL + tf) + 500000) / 1000000
+ - 3 + offset;
+}
+
+u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
+{
+ /*
+ * Conditional expression:
+ *
+ * IC_[FS]S_SCL_LCNT + 1 >= IC_CLK * (tLOW + tf)
+ *
+ * DW I2C core starts counting the SCL CNTs for the LOW period
+ * of the SCL clock (tLOW) as soon as it pulls the SCL line.
+ * In order to meet the tLOW timing spec, we need to take into
+ * account the fall time of SCL signal (tf). Default tf value
+ * should be 0.3 us, for safety.
+ */
+ return ((ic_clk * (tLOW + tf) + 500000) / 1000000) - 1 + offset;
+}
+
+void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
+{
+ dw_writel(dev, enable, DW_IC_ENABLE);
+}
+
+void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable)
+{
+ int timeout = 100;
+
+ do {
+ __i2c_dw_enable(dev, enable);
+ if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
+ return;
+
+ /*
+ * Wait 10 times the signaling period of the highest I2C
+ * transfer supported by the driver (for 400KHz this is
+ * 25us) as described in the DesignWare I2C databook.
+ */
+ usleep_range(25, 250);
+ } while (timeout--);
+
+ dev_warn(dev->dev, "timeout in %sabling adapter\n",
+ enable ? "en" : "dis");
+}
+
+unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
+{
+ /*
+ * Clock is not necessary if we got LCNT/HCNT values directly from
+ * the platform code.
+ */
+ if (WARN_ON_ONCE(!dev->get_clk_rate_khz))
+ return 0;
+ return dev->get_clk_rate_khz(dev);
+}
+
+int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
+{
+ int ret;
+
+ if (!dev->acquire_lock)
+ return 0;
+
+ ret = dev->acquire_lock(dev);
+ if (!ret)
+ return 0;
+
+ dev_err(dev->dev, "couldn't acquire bus ownership\n");
+
+ return ret;
+}
+
+void i2c_dw_release_lock(struct dw_i2c_dev *dev)
+{
+ if (dev->release_lock)
+ dev->release_lock(dev);
+}
+
+/*
+ * Waiting for bus not busy
+ */
+int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
+{
+ int timeout = TIMEOUT;
+
+ while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
+ if (timeout <= 0) {
+ dev_warn(dev->dev, "timeout waiting for bus ready\n");
+ return -ETIMEDOUT;
+ }
+ timeout--;
+ usleep_range(1000, 1100);
+ }
+
+ return 0;
+}
+
+int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
+{
+ unsigned long abort_source = dev->abort_source;
+ int i;
+
+ if (abort_source & DW_IC_TX_ABRT_NOACK) {
+ for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
+ dev_dbg(dev->dev,
+ "%s: %s\n", __func__, abort_sources[i]);
+ return -EREMOTEIO;
+ }
+
+ for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
+ dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
+
+ if (abort_source & DW_IC_TX_ARB_LOST)
+ return -EAGAIN;
+ else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
+ return -EINVAL; /* wrong msgs[] data */
+ else
+ return -EIO;
+}
+
+u32 i2c_dw_func(struct i2c_adapter *adap)
+{
+ struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+
+ return dev->functionality;
+}
+
+MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
+MODULE_LICENSE("GPL");
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 26250b425e2f..8bba7a37c3ce 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -40,6 +40,124 @@
#define DW_IC_CON_RESTART_EN 0x20
#define DW_IC_CON_SLAVE_DISABLE 0x40
+/*
+ * Registers offset
+ */
+#define DW_IC_CON 0x0
+#define DW_IC_TAR 0x4
+#define DW_IC_DATA_CMD 0x10
+#define DW_IC_SS_SCL_HCNT 0x14
+#define DW_IC_SS_SCL_LCNT 0x18
+#define DW_IC_FS_SCL_HCNT 0x1c
+#define DW_IC_FS_SCL_LCNT 0x20
+#define DW_IC_HS_SCL_HCNT 0x24
+#define DW_IC_HS_SCL_LCNT 0x28
+#define DW_IC_INTR_STAT 0x2c
+#define DW_IC_INTR_MASK 0x30
+#define DW_IC_RAW_INTR_STAT 0x34
+#define DW_IC_RX_TL 0x38
+#define DW_IC_TX_TL 0x3c
+#define DW_IC_CLR_INTR 0x40
+#define DW_IC_CLR_RX_UNDER 0x44
+#define DW_IC_CLR_RX_OVER 0x48
+#define DW_IC_CLR_TX_OVER 0x4c
+#define DW_IC_CLR_RD_REQ 0x50
+#define DW_IC_CLR_TX_ABRT 0x54
+#define DW_IC_CLR_RX_DONE 0x58
+#define DW_IC_CLR_ACTIVITY 0x5c
+#define DW_IC_CLR_STOP_DET 0x60
+#define DW_IC_CLR_START_DET 0x64
+#define DW_IC_CLR_GEN_CALL 0x68
+#define DW_IC_ENABLE 0x6c
+#define DW_IC_STATUS 0x70
+#define DW_IC_TXFLR 0x74
+#define DW_IC_RXFLR 0x78
+#define DW_IC_SDA_HOLD 0x7c
+#define DW_IC_TX_ABRT_SOURCE 0x80
+#define DW_IC_ENABLE_STATUS 0x9c
+#define DW_IC_COMP_PARAM_1 0xf4
+#define DW_IC_COMP_VERSION 0xf8
+#define DW_IC_SDA_HOLD_MIN_VERS 0x3131312A
+#define DW_IC_COMP_TYPE 0xfc
+#define DW_IC_COMP_TYPE_VALUE 0x44570140
+
+#define DW_IC_INTR_RX_UNDER 0x001
+#define DW_IC_INTR_RX_OVER 0x002
+#define DW_IC_INTR_RX_FULL 0x004
+#define DW_IC_INTR_TX_OVER 0x008
+#define DW_IC_INTR_TX_EMPTY 0x010
+#define DW_IC_INTR_RD_REQ 0x020
+#define DW_IC_INTR_TX_ABRT 0x040
+#define DW_IC_INTR_RX_DONE 0x080
+#define DW_IC_INTR_ACTIVITY 0x100
+#define DW_IC_INTR_STOP_DET 0x200
+#define DW_IC_INTR_START_DET 0x400
+#define DW_IC_INTR_GEN_CALL 0x800
+
+#define DW_IC_INTR_DEFAULT_MASK (DW_IC_INTR_RX_FULL | \
+ DW_IC_INTR_TX_ABRT | \
+ DW_IC_INTR_STOP_DET)
+#define DW_IC_INTR_MASTER_MASK (DW_IC_INTR_DEFAULT_MASK | \
+ DW_IC_INTR_TX_EMPTY)
+#define DW_IC_STATUS_ACTIVITY 0x1
+#define DW_IC_STATUS_TFE BIT(2)
+#define DW_IC_STATUS_MASTER_ACTIVITY BIT(5)
+
+#define DW_IC_SDA_HOLD_RX_SHIFT 16
+#define DW_IC_SDA_HOLD_RX_MASK GENMASK(23, DW_IC_SDA_HOLD_RX_SHIFT)
+
+#define DW_IC_ERR_TX_ABRT 0x1
+
+#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
+
+#define DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH (BIT(2) | BIT(3))
+#define DW_IC_COMP_PARAM_1_SPEED_MODE_MASK GENMASK(3, 2)
+
+/*
+ * status codes
+ */
+#define STATUS_IDLE 0x0
+#define STATUS_WRITE_IN_PROGRESS 0x1
+#define STATUS_READ_IN_PROGRESS 0x2
+
+#define TIMEOUT 20 /* ms */
+
+/*
+ * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
+ *
+ * only expected abort codes are listed here
+ * refer to the datasheet for the full list
+ */
+#define ABRT_7B_ADDR_NOACK 0
+#define ABRT_10ADDR1_NOACK 1
+#define ABRT_10ADDR2_NOACK 2
+#define ABRT_TXDATA_NOACK 3
+#define ABRT_GCALL_NOACK 4
+#define ABRT_GCALL_READ 5
+#define ABRT_SBYTE_ACKDET 7
+#define ABRT_SBYTE_NORSTRT 9
+#define ABRT_10B_RD_NORSTRT 10
+#define ABRT_MASTER_DIS 11
+#define ARB_LOST 12
+
+#define DW_IC_TX_ABRT_7B_ADDR_NOACK (1UL << ABRT_7B_ADDR_NOACK)
+#define DW_IC_TX_ABRT_10ADDR1_NOACK (1UL << ABRT_10ADDR1_NOACK)
+#define DW_IC_TX_ABRT_10ADDR2_NOACK (1UL << ABRT_10ADDR2_NOACK)
+#define DW_IC_TX_ABRT_TXDATA_NOACK (1UL << ABRT_TXDATA_NOACK)
+#define DW_IC_TX_ABRT_GCALL_NOACK (1UL << ABRT_GCALL_NOACK)
+#define DW_IC_TX_ABRT_GCALL_READ (1UL << ABRT_GCALL_READ)
+#define DW_IC_TX_ABRT_SBYTE_ACKDET (1UL << ABRT_SBYTE_ACKDET)
+#define DW_IC_TX_ABRT_SBYTE_NORSTRT (1UL << ABRT_SBYTE_NORSTRT)
+#define DW_IC_TX_ABRT_10B_RD_NORSTRT (1UL << ABRT_10B_RD_NORSTRT)
+#define DW_IC_TX_ABRT_MASTER_DIS (1UL << ABRT_MASTER_DIS)
+#define DW_IC_TX_ARB_LOST (1UL << ARB_LOST)
+
+#define DW_IC_TX_ABRT_NOACK (DW_IC_TX_ABRT_7B_ADDR_NOACK | \
+ DW_IC_TX_ABRT_10ADDR1_NOACK | \
+ DW_IC_TX_ABRT_10ADDR2_NOACK | \
+ DW_IC_TX_ABRT_TXDATA_NOACK | \
+ DW_IC_TX_ABRT_GCALL_NOACK)
+
/**
* struct dw_i2c_dev - private i2c-designware data
@@ -132,6 +250,19 @@ struct dw_i2c_dev {
#define ACCESS_16BIT 0x00000002
#define ACCESS_INTR_MASK 0x00000004
+u32 dw_readl(struct dw_i2c_dev *dev, int offset);
+void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset);
+u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset);
+u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset);
+void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable);
+void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable);
+unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev);
+int i2c_dw_acquire_lock(struct dw_i2c_dev *dev);
+void i2c_dw_release_lock(struct dw_i2c_dev *dev);
+int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev);
+int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev);
+u32 i2c_dw_func(struct i2c_adapter *adap);
+
extern int i2c_dw_init(struct dw_i2c_dev *dev);
extern void i2c_dw_disable(struct dw_i2c_dev *dev);
extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-master.c
similarity index 66%
rename from drivers/i2c/busses/i2c-designware-core.c
rename to drivers/i2c/busses/i2c-designware-master.c
index 951ababbd9a3..b55a7f4c5149 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -32,177 +32,6 @@
#include <linux/pm_runtime.h>
#include "i2c-designware-core.h"
-/*
- * Registers offset
- */
-#define DW_IC_CON 0x0
-#define DW_IC_TAR 0x4
-#define DW_IC_DATA_CMD 0x10
-#define DW_IC_SS_SCL_HCNT 0x14
-#define DW_IC_SS_SCL_LCNT 0x18
-#define DW_IC_FS_SCL_HCNT 0x1c
-#define DW_IC_FS_SCL_LCNT 0x20
-#define DW_IC_HS_SCL_HCNT 0x24
-#define DW_IC_HS_SCL_LCNT 0x28
-#define DW_IC_INTR_STAT 0x2c
-#define DW_IC_INTR_MASK 0x30
-#define DW_IC_RAW_INTR_STAT 0x34
-#define DW_IC_RX_TL 0x38
-#define DW_IC_TX_TL 0x3c
-#define DW_IC_CLR_INTR 0x40
-#define DW_IC_CLR_RX_UNDER 0x44
-#define DW_IC_CLR_RX_OVER 0x48
-#define DW_IC_CLR_TX_OVER 0x4c
-#define DW_IC_CLR_RD_REQ 0x50
-#define DW_IC_CLR_TX_ABRT 0x54
-#define DW_IC_CLR_RX_DONE 0x58
-#define DW_IC_CLR_ACTIVITY 0x5c
-#define DW_IC_CLR_STOP_DET 0x60
-#define DW_IC_CLR_START_DET 0x64
-#define DW_IC_CLR_GEN_CALL 0x68
-#define DW_IC_ENABLE 0x6c
-#define DW_IC_STATUS 0x70
-#define DW_IC_TXFLR 0x74
-#define DW_IC_RXFLR 0x78
-#define DW_IC_SDA_HOLD 0x7c
-#define DW_IC_TX_ABRT_SOURCE 0x80
-#define DW_IC_ENABLE_STATUS 0x9c
-#define DW_IC_COMP_PARAM_1 0xf4
-#define DW_IC_COMP_VERSION 0xf8
-#define DW_IC_SDA_HOLD_MIN_VERS 0x3131312A
-#define DW_IC_COMP_TYPE 0xfc
-#define DW_IC_COMP_TYPE_VALUE 0x44570140
-
-#define DW_IC_INTR_RX_UNDER 0x001
-#define DW_IC_INTR_RX_OVER 0x002
-#define DW_IC_INTR_RX_FULL 0x004
-#define DW_IC_INTR_TX_OVER 0x008
-#define DW_IC_INTR_TX_EMPTY 0x010
-#define DW_IC_INTR_RD_REQ 0x020
-#define DW_IC_INTR_TX_ABRT 0x040
-#define DW_IC_INTR_RX_DONE 0x080
-#define DW_IC_INTR_ACTIVITY 0x100
-#define DW_IC_INTR_STOP_DET 0x200
-#define DW_IC_INTR_START_DET 0x400
-#define DW_IC_INTR_GEN_CALL 0x800
-
-#define DW_IC_INTR_DEFAULT_MASK (DW_IC_INTR_RX_FULL | \
- DW_IC_INTR_TX_ABRT | \
- DW_IC_INTR_STOP_DET)
-
-#define DW_IC_INTR_MASTER_MASK (DW_IC_INTR_DEFAULT_MASK | \
- DW_IC_INTR_TX_EMPTY)
-
-#define DW_IC_STATUS_ACTIVITY 0x1
-
-#define DW_IC_SDA_HOLD_RX_SHIFT 16
-#define DW_IC_SDA_HOLD_RX_MASK GENMASK(23, DW_IC_SDA_HOLD_RX_SHIFT)
-
-#define DW_IC_ERR_TX_ABRT 0x1
-
-#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
-
-#define DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH (BIT(2) | BIT(3))
-#define DW_IC_COMP_PARAM_1_SPEED_MODE_MASK GENMASK(3, 2)
-
-/*
- * status codes
- */
-#define STATUS_IDLE 0x0
-#define STATUS_WRITE_IN_PROGRESS 0x1
-#define STATUS_READ_IN_PROGRESS 0x2
-
-#define TIMEOUT 20 /* ms */
-
-/*
- * Hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
- *
- * Only expected abort codes are listed here,
- * refer to the datasheet for the full list.
- */
-#define ABRT_7B_ADDR_NOACK 0
-#define ABRT_10ADDR1_NOACK 1
-#define ABRT_10ADDR2_NOACK 2
-#define ABRT_TXDATA_NOACK 3
-#define ABRT_GCALL_NOACK 4
-#define ABRT_GCALL_READ 5
-#define ABRT_SBYTE_ACKDET 7
-#define ABRT_SBYTE_NORSTRT 9
-#define ABRT_10B_RD_NORSTRT 10
-#define ABRT_MASTER_DIS 11
-#define ARB_LOST 12
-
-#define DW_IC_TX_ABRT_7B_ADDR_NOACK (1UL << ABRT_7B_ADDR_NOACK)
-#define DW_IC_TX_ABRT_10ADDR1_NOACK (1UL << ABRT_10ADDR1_NOACK)
-#define DW_IC_TX_ABRT_10ADDR2_NOACK (1UL << ABRT_10ADDR2_NOACK)
-#define DW_IC_TX_ABRT_TXDATA_NOACK (1UL << ABRT_TXDATA_NOACK)
-#define DW_IC_TX_ABRT_GCALL_NOACK (1UL << ABRT_GCALL_NOACK)
-#define DW_IC_TX_ABRT_GCALL_READ (1UL << ABRT_GCALL_READ)
-#define DW_IC_TX_ABRT_SBYTE_ACKDET (1UL << ABRT_SBYTE_ACKDET)
-#define DW_IC_TX_ABRT_SBYTE_NORSTRT (1UL << ABRT_SBYTE_NORSTRT)
-#define DW_IC_TX_ABRT_10B_RD_NORSTRT (1UL << ABRT_10B_RD_NORSTRT)
-#define DW_IC_TX_ABRT_MASTER_DIS (1UL << ABRT_MASTER_DIS)
-#define DW_IC_TX_ARB_LOST (1UL << ARB_LOST)
-
-#define DW_IC_TX_ABRT_NOACK (DW_IC_TX_ABRT_7B_ADDR_NOACK | \
- DW_IC_TX_ABRT_10ADDR1_NOACK | \
- DW_IC_TX_ABRT_10ADDR2_NOACK | \
- DW_IC_TX_ABRT_TXDATA_NOACK | \
- DW_IC_TX_ABRT_GCALL_NOACK)
-
-static char *abort_sources[] = {
- [ABRT_7B_ADDR_NOACK] =
- "slave address not acknowledged (7bit mode)",
- [ABRT_10ADDR1_NOACK] =
- "first address byte not acknowledged (10bit mode)",
- [ABRT_10ADDR2_NOACK] =
- "second address byte not acknowledged (10bit mode)",
- [ABRT_TXDATA_NOACK] =
- "data not acknowledged",
- [ABRT_GCALL_NOACK] =
- "no acknowledgement for a general call",
- [ABRT_GCALL_READ] =
- "read after general call",
- [ABRT_SBYTE_ACKDET] =
- "start byte acknowledged",
- [ABRT_SBYTE_NORSTRT] =
- "trying to send start byte when restart is disabled",
- [ABRT_10B_RD_NORSTRT] =
- "trying to read when restart is disabled (10bit mode)",
- [ABRT_MASTER_DIS] =
- "trying to use disabled adapter",
- [ARB_LOST] =
- "lost arbitration",
-};
-
-static u32 dw_readl(struct dw_i2c_dev *dev, int offset)
-{
- u32 value;
-
- if (dev->accessor_flags & ACCESS_16BIT)
- value = readw_relaxed(dev->base + offset) |
- (readw_relaxed(dev->base + offset + 2) << 16);
- else
- value = readl_relaxed(dev->base + offset);
-
- if (dev->accessor_flags & ACCESS_SWAP)
- return swab32(value);
- else
- return value;
-}
-
-static void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
-{
- if (dev->accessor_flags & ACCESS_SWAP)
- b = swab32(b);
-
- if (dev->accessor_flags & ACCESS_16BIT) {
- writew_relaxed((u16)b, dev->base + offset);
- writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
- } else {
- writel_relaxed(b, dev->base + offset);
- }
-}
static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
{
@@ -210,127 +39,12 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
- /* configure the i2c master */
+ /* configure the I2C master */
dw_writel(dev, dev->master_cfg, DW_IC_CON);
}
-static u32
-i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
-{
- /*
- * DesignWare I2C core doesn't seem to have solid strategy to meet
- * the tHD;STA timing spec. Configuring _HCNT based on tHIGH spec
- * will result in violation of the tHD;STA spec.
- */
- if (cond)
- /*
- * Conditional expression:
- *
- * IC_[FS]S_SCL_HCNT + (1+4+3) >= IC_CLK * tHIGH
- *
- * This is based on the DW manuals, and represents an ideal
- * configuration. The resulting I2C bus speed will be
- * faster than any of the others.
- *
- * If your hardware is free from tHD;STA issue, try this one.
- */
- return (ic_clk * tSYMBOL + 500000) / 1000000 - 8 + offset;
- else
- /*
- * Conditional expression:
- *
- * IC_[FS]S_SCL_HCNT + 3 >= IC_CLK * (tHD;STA + tf)
- *
- * This is just experimental rule; the tHD;STA period turned
- * out to be proportinal to (_HCNT + 3). With this setting,
- * we could meet both tHIGH and tHD;STA timing specs.
- *
- * If unsure, you'd better to take this alternative.
- *
- * The reason why we need to take into account "tf" here,
- * is the same as described in i2c_dw_scl_lcnt().
- */
- return (ic_clk * (tSYMBOL + tf) + 500000) / 1000000
- - 3 + offset;
-}
-
-static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
-{
- /*
- * Conditional expression:
- *
- * IC_[FS]S_SCL_LCNT + 1 >= IC_CLK * (tLOW + tf)
- *
- * DW I2C core starts counting the SCL CNTs for the LOW period
- * of the SCL clock (tLOW) as soon as it pulls the SCL line.
- * In order to meet the tLOW timing spec, we need to take into
- * account the fall time of SCL signal (tf). Default tf value
- * should be 0.3 us, for safety.
- */
- return ((ic_clk * (tLOW + tf) + 500000) / 1000000) - 1 + offset;
-}
-
-static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
-{
- dw_writel(dev, enable, DW_IC_ENABLE);
-}
-
-static void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable)
-{
- int timeout = 100;
-
- do {
- __i2c_dw_enable(dev, enable);
- if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
- return;
-
- /*
- * Wait 10 times the signaling period of the highest I2C
- * transfer supported by the driver (for 400KHz this is
- * 25us) as described in the DesignWare I2C databook.
- */
- usleep_range(25, 250);
- } while (timeout--);
-
- dev_warn(dev->dev, "timeout in %sabling adapter\n",
- enable ? "en" : "dis");
-}
-
-static unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
-{
- /*
- * Clock is not necessary if we got LCNT/HCNT values directly from
- * the platform code.
- */
- if (WARN_ON_ONCE(!dev->get_clk_rate_khz))
- return 0;
- return dev->get_clk_rate_khz(dev);
-}
-
-static int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
-{
- int ret;
-
- if (!dev->acquire_lock)
- return 0;
-
- ret = dev->acquire_lock(dev);
- if (!ret)
- return 0;
-
- dev_err(dev->dev, "couldn't acquire bus ownership\n");
-
- return ret;
-}
-
-static void i2c_dw_release_lock(struct dw_i2c_dev *dev)
-{
- if (dev->release_lock)
- dev->release_lock(dev);
-}
-
/**
- * i2c_dw_init() - initialize the designware i2c hardware
+ * i2c_dw_init() - Initialize the designware I2C master hardware
* @dev: device private data
*
* This functions configures and enables the I2C.
@@ -460,25 +174,6 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
}
EXPORT_SYMBOL_GPL(i2c_dw_init);
-/*
- * Waiting for bus not busy
- */
-static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
-{
- int timeout = TIMEOUT;
-
- while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
- if (timeout <= 0) {
- dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return -ETIMEDOUT;
- }
- timeout--;
- usleep_range(1000, 1100);
- }
-
- return 0;
-}
-
static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
{
struct i2c_msg *msgs = dev->msgs;
@@ -548,7 +243,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
/*
* If target address has changed, we need to
* reprogram the target address in the i2c
- * adapter when we are done with this transfer
+ * adapter when we are done with this transfer.
*/
if (msgs[dev->msg_write_idx].addr != addr) {
dev_err(dev->dev,
@@ -713,29 +408,6 @@ i2c_dw_read(struct dw_i2c_dev *dev)
}
}
-static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
-{
- unsigned long abort_source = dev->abort_source;
- int i;
-
- if (abort_source & DW_IC_TX_ABRT_NOACK) {
- for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
- dev_dbg(dev->dev,
- "%s: %s\n", __func__, abort_sources[i]);
- return -EREMOTEIO;
- }
-
- for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
- dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
-
- if (abort_source & DW_IC_TX_ARB_LOST)
- return -EAGAIN;
- else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
- return -EINVAL; /* wrong msgs[] data */
- else
- return -EIO;
-}
-
/*
* Prepare controller for a transaction and call i2c_dw_xfer_msg
*/
@@ -823,15 +495,9 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
return ret;
}
-static u32 i2c_dw_func(struct i2c_adapter *adap)
-{
- struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
- return dev->functionality;
-}
-
static struct i2c_algorithm i2c_dw_algo = {
- .master_xfer = i2c_dw_xfer,
- .functionality = i2c_dw_func,
+ .master_xfer = i2c_dw_xfer,
+ .functionality = i2c_dw_func,
};
static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
@@ -890,10 +556,10 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
}
/*
- * Interrupt service routine. This gets called whenever an I2C interrupt
+ * Interrupt service routine. This gets called whenever an I2C master interrupt
* occurs.
*/
-int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
+static int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
{
u32 stat;
@@ -994,7 +660,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
/*
* Test if dynamic TAR update is enabled in this controller by writing
* to IC_10BITADDR_MASTER field in IC_CON: when it is enabled this
- * field is read-only so it should not succeed
+ * field is read-only so it should not succeed.
*/
reg = dw_readl(dev, DW_IC_CON);
dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER, DW_IC_CON);
@@ -1040,5 +706,5 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
}
EXPORT_SYMBOL_GPL(i2c_dw_probe);
-MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
+MODULE_DESCRIPTION("Synopsys DesignWare I2C bus master adapter");
MODULE_LICENSE("GPL");
--
2.11.0
^ permalink raw reply related
* [PATCH v5 2/7] i2c: designware: refactoring of the i2c-designware
From: Luis Oliveira @ 2016-12-28 14:43 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <cover.1482934380.git.lolivei@synopsys.com>
- Factor out all _master() part of code from i2c-designware-core
and i2c-designware-platdrv to separate functions.
- Standardize all code related with MASTER mode.
- I have to take off DW_IC_INTR_TX_EMPTY from DW_IC_INTR_DEFAULT_MASK
because it is master specific.
The purpose of this is to prepare the controller to have is I2C MASTER
flow in a separate driver. To do this first all the
functions/definitions related to the MASTER flow were identified.
Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V4->V5: (ACK by Andy)
Fixed the bellow issues:
- Removed a part belonging to slave patch
- Removed unused variable "mode"
- Changed dev_info() to dev_dbg()
drivers/i2c/busses/i2c-designware-core.c | 56 ++++++++++++++++++-----------
drivers/i2c/busses/i2c-designware-platdrv.c | 35 +++++++++++-------
2 files changed, 58 insertions(+), 33 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 9d724a6a7ec8..951ababbd9a3 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -87,10 +87,12 @@
#define DW_IC_INTR_GEN_CALL 0x800
#define DW_IC_INTR_DEFAULT_MASK (DW_IC_INTR_RX_FULL | \
- DW_IC_INTR_TX_EMPTY | \
DW_IC_INTR_TX_ABRT | \
DW_IC_INTR_STOP_DET)
+#define DW_IC_INTR_MASTER_MASK (DW_IC_INTR_DEFAULT_MASK | \
+ DW_IC_INTR_TX_EMPTY)
+
#define DW_IC_STATUS_ACTIVITY 0x1
#define DW_IC_SDA_HOLD_RX_SHIFT 16
@@ -202,6 +204,16 @@ static void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
}
}
+static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
+{
+ /* Configure Tx/Rx FIFO threshold levels */
+ dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
+ dw_writel(dev, 0, DW_IC_RX_TL);
+
+ /* configure the i2c master */
+ dw_writel(dev, dev->master_cfg, DW_IC_CON);
+}
+
static u32
i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
{
@@ -318,10 +330,10 @@ static void i2c_dw_release_lock(struct dw_i2c_dev *dev)
}
/**
- * i2c_dw_init() - initialize the designware i2c master hardware
+ * i2c_dw_init() - initialize the designware i2c hardware
* @dev: device private data
*
- * This functions configures and enables the I2C master.
+ * This functions configures and enables the I2C.
* This function is called during I2C init function, and in case of timeout at
* run time.
*/
@@ -440,12 +452,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
"Hardware too old to adjust SDA hold time.\n");
}
- /* Configure Tx/Rx FIFO threshold levels */
- dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
- dw_writel(dev, 0, DW_IC_RX_TL);
-
- /* Configure the I2C master */
- dw_writel(dev, dev->master_cfg , DW_IC_CON);
+ i2c_dw_configure_fifo_master(dev);
i2c_dw_release_lock(dev);
@@ -513,7 +520,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
/* Clear and enable interrupts */
dw_readl(dev, DW_IC_CLR_INTR);
- dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
+ dw_writel(dev, DW_IC_INTR_MASTER_MASK, DW_IC_INTR_MASK);
}
/*
@@ -533,7 +540,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
u8 *buf = dev->tx_buf;
bool need_restart = false;
- intr_mask = DW_IC_INTR_DEFAULT_MASK;
+ intr_mask = DW_IC_INTR_MASTER_MASK;
for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
u32 flags = msgs[dev->msg_write_idx].flags;
@@ -886,16 +893,9 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
* Interrupt service routine. This gets called whenever an I2C interrupt
* occurs.
*/
-static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
+int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
{
- struct dw_i2c_dev *dev = dev_id;
- u32 stat, enabled;
-
- enabled = dw_readl(dev, DW_IC_ENABLE);
- stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
- dev_dbg(dev->dev, "%s: enabled=%#x stat=%#x\n", __func__, enabled, stat);
- if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY))
- return IRQ_NONE;
+ u32 stat;
stat = i2c_dw_read_clear_intrbits(dev);
@@ -932,6 +932,22 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
i2c_dw_disable_int(dev);
dw_writel(dev, stat, DW_IC_INTR_MASK);
}
+ return 0;
+}
+
+static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
+{
+ struct dw_i2c_dev *dev = dev_id;
+ u32 stat, enabled;
+
+ enabled = dw_readl(dev, DW_IC_ENABLE);
+ stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
+ dev_dbg(dev->dev,
+ "%s: enabled=%#x stat=%#x\n", __func__, enabled, stat);
+ if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY))
+ return IRQ_NONE;
+
+ i2c_dw_irq_handler_master(dev);
return IRQ_HANDLED;
}
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 4070baea4fb9..5cf4df63dbe8 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -139,6 +139,27 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
}
#endif
+static void i2c_dw_configure_master(struct platform_device *pdev)
+{
+ struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+
+ dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
+ DW_IC_CON_RESTART_EN;
+
+ dev_dbg(&pdev->dev, "I am registed as a I2C Master!\n");
+
+ switch (dev->clk_freq) {
+ case 100000:
+ dev->master_cfg |= DW_IC_CON_SPEED_STD;
+ break;
+ case 3400000:
+ dev->master_cfg |= DW_IC_CON_SPEED_HIGH;
+ break;
+ default:
+ dev->master_cfg |= DW_IC_CON_SPEED_FAST;
+ }
+}
+
static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
{
if (IS_ERR(i_dev->clk))
@@ -245,19 +266,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
- dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
- DW_IC_CON_RESTART_EN;
-
- switch (dev->clk_freq) {
- case 100000:
- dev->master_cfg |= DW_IC_CON_SPEED_STD;
- break;
- case 3400000:
- dev->master_cfg |= DW_IC_CON_SPEED_HIGH;
- break;
- default:
- dev->master_cfg |= DW_IC_CON_SPEED_FAST;
- }
+ i2c_dw_configure_master(pdev);
dev->clk = devm_clk_get(&pdev->dev, NULL);
if (!i2c_dw_plat_prepare_clk(dev, true)) {
--
2.11.0
^ permalink raw reply related
* [PATCH v5 0/7] i2c: designware: add I2C SLAVE support
From: Luis Oliveira @ 2016-12-28 14:43 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
The purpose of this patch is to enable Linux to be a I2C slave by enabling the
slave functionality in the designware I2C controller. The patch refactors the
original i2c-designware-core and extracts all master functions to a
i2c-designware-master source file as suggested by Andy Shevchenko. It also
creates a i2c-designware-slave source file and keeps the common functions in the
i2c-designware-src source file. For that changes also had to be made in the
Makefile and Kconfig.
The driver instantiates in slave or master mode by checking the reg address that
must be defined in the DT and evaluating if is a I2C_OWN_SLAVE_ADDRESS".
ACPI is not supported in this driver.
The functionality was tested using the hardware independent software backend
slave-eeprom driver.
Luis Oliveira (7):
i2c: designware: Cleaning and comment style fixes.
i2c: designware: refactoring of the i2c-designware
i2c: designware: MASTER mode as separated driver
i2c: designware: introducing I2C_SLAVE definitions
i2c: designware: add SLAVE mode functions
i2c: designware: enable SLAVE in platform module
i2c: designware: style changes in existing code
drivers/i2c/busses/Kconfig | 3 +-
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-designware-common.c | 261 ++++++++++++
drivers/i2c/busses/i2c-designware-core.h | 164 ++++++++
...c-designware-core.c => i2c-designware-master.c} | 444 +++------------------
drivers/i2c/busses/i2c-designware-platdrv.c | 144 +++++--
drivers/i2c/busses/i2c-designware-slave.c | 434 ++++++++++++++++++++
7 files changed, 1029 insertions(+), 422 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-designware-common.c
rename drivers/i2c/busses/{i2c-designware-core.c => i2c-designware-master.c} (62%)
create mode 100644 drivers/i2c/busses/i2c-designware-slave.c
--
2.11.0
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Wolfram Sang @ 2016-12-28 14:03 UTC (permalink / raw)
To: Pali Rohár
Cc: Andy Shevchenko, valdis.kletnieks, Mika Westerberg, Jean Delvare,
Steven Honeyman, Jochen Eisinger, Gabriele Mazzotta,
Andy Lutomirski, Mario Limonciello, Alex Hung,
Michał Kępień,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <201612281005.22088@pali>
> I have absolutely no idea how to you want to achieve calling that
> i2c_new_device() registration
> without kernel patches.
Documentation/i2c/instantiating-devices lists all supported methods.
Method 4 is userspace instantiation.
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Wolfram Sang @ 2016-12-28 14:02 UTC (permalink / raw)
To: Pali Rohár
Cc: Jean Delvare, Steven Honeyman, valdis.kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, mario_limonciello, Alex Hung,
Michał Kępień, Takashi Iwai,
linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <201612271451.01505@pali>
On Tue, Dec 27, 2016 at 02:51:01PM +0100, Pali Rohár wrote:
> On Tuesday 27 December 2016 14:43:49 Wolfram Sang wrote:
> > > Dell platform team told us that some (DMI whitelisted) Dell
> > > Latitude machines have ST microelectronics accelerometer at i2c
> > > address 0x29. That
> > > i2c address is not specified in DMI or ACPI, so runtime detection
> > > without
> > > whitelist which is below is not possible.
> >
> > I'd think this should rather live somewhere in
> > drivers/platform/x86/dell*.c?
>
> i2c_new_device() with lis3lv02d for i801 i2c bus needs to be called
> after initializing i2c-i801 bus driver.
>
> I have no idea how to do it (properly) outside of i2c-i801.c file.
I once used bus_notifiers to achieve something similar. You could check
arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c to see an action
triggered once a client device got added, but you could act on another
action like BUS_NOTIFY_BOUND_DRIVER. I used exactly that, too, somewhen
somewhere. Haven't checked if that helps here, too. And since we have
a
precedence (Fujitsu case), I'll leave it to Jean who is the maintainer
of this driver.
Thanks,
Wolfram
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2016-12-28 9:05 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Valdis.Kletnieks, Mika Westerberg, Wolfram Sang, Jean Delvare,
Steven Honeyman, Jochen Eisinger, Gabriele Mazzotta,
Andy Lutomirski, Mario Limonciello, Alex Hung,
Michał Kępień, Takashi Iwai,
linux-i2c, linux-kernel@vger.kernel.org, platform-driver-x86
In-Reply-To: <CAHp75Vc4WaGu1mkMvXY3B=YSdd-Pv9YD7gJFxubDWu0Vow8=BQ@mail.gmail.com>
[-- Attachment #1: Type: Text/Plain, Size: 1813 bytes --]
On Wednesday 28 December 2016 08:55:18 Andy Shevchenko wrote:
> On Wed, Dec 28, 2016 at 12:41 AM, <Valdis.Kletnieks@vt.edu> wrote:
> > On Wed, 28 Dec 2016 00:15:30 +0200, Andy Shevchenko said:
> >> On Tue, Dec 27, 2016 at 3:51 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> >> > I have no idea how to do it (properly) outside of i2c-i801.c file.
> >>
> >> I doubt we need a single line of code for this. See [1] and perhaps
> >> create an EFI variable with necessary upgrade device node.
> >>
> >> > Same thing is done for Fujitsu machines, see function
> >> > i801_probe_optional_slaves() in i2c-i801.c file. So I did similar
> >> > approach for Dell machines.
> >>
> >> Perhaps, this also needs to be converted to use EFI variable.
> >>
> >> [1] https://lwn.net/Articles/693212/
> >
> > There's no guarantee that the laptops in question are booted with UEFI,
> > as Dell still supports legacy boot. So assuming the presence of EFI variables
> > is somewhat problematic.
>
>
> > In addition, it requires the user (or something in userspace) to set the UEFI
> > variable or configfs tweak, rather than Just Working Out Of The Box.
>
> I have no strong opinion, though I don't support the idea to put all
> hacks in the world to the kernel. For example, we have user space tool
> to switch USB modem from storage to actual communication device and
> that is just working out of the box.
>
> Mika, Darren, what are your opinions?
I have absolutely no idea how to you want to achieve calling that i2c_new_device() registration
without kernel patches.
So before starting discussion which option to use (EFI, kernel patch, userspace script, etc...)
please describe how would you implement such logic with different options.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2016-12-28 8:38 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Wolfram Sang, Jean Delvare, Steven Honeyman, valdis.kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario Limonciello, Alex Hung,
Michał Kępień, Takashi Iwai,
linux-i2c, linux-kernel@vger.kernel.org, platform-driver-x86
In-Reply-To: <CAHp75VeWBaPNmT0mb+Gs_ZhFEHFx_H-Ro0UQ9u1VvLhNVjCqYQ@mail.gmail.com>
[-- Attachment #1: Type: Text/Plain, Size: 1089 bytes --]
On Tuesday 27 December 2016 23:15:30 Andy Shevchenko wrote:
> On Tue, Dec 27, 2016 at 3:51 PM, Pali Rohár <pali.rohar@gmail.com>
> wrote:
> > On Tuesday 27 December 2016 14:43:49 Wolfram Sang wrote:
> >> > Dell platform team told us that some (DMI whitelisted) Dell
> >> > Latitude machines have ST microelectronics accelerometer at i2c
> >> > address 0x29. That
> >> > i2c address is not specified in DMI or ACPI, so runtime
> >> > detection without
> >> > whitelist which is below is not possible.
> >>
> >> I'd think this should rather live somewhere in
> >> drivers/platform/x86/dell*.c?
> >
> > i2c_new_device() with lis3lv02d for i801 i2c bus needs to be called
> > after initializing i2c-i801 bus driver.
> >
> > I have no idea how to do it (properly) outside of i2c-i801.c file.
>
> I doubt we need a single line of code for this. See [1] and perhaps
> create an EFI variable with necessary upgrade device node.
Sorry, but EFI variable is not accessible from BIOS booted kernel. So
such thing will not work.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Andy Shevchenko @ 2016-12-28 7:55 UTC (permalink / raw)
To: Valdis.Kletnieks, Mika Westerberg
Cc: Pali Rohár, Wolfram Sang, Jean Delvare, Steven Honeyman,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario Limonciello, Alex Hung,
Michał Kępień, Takashi Iwai,
linux-i2c, linux-kernel@vger.kernel.org, platform-driver-x86
In-Reply-To: <52345.1482878491@turing-police.cc.vt.edu>
On Wed, Dec 28, 2016 at 12:41 AM, <Valdis.Kletnieks@vt.edu> wrote:
> On Wed, 28 Dec 2016 00:15:30 +0200, Andy Shevchenko said:
>> On Tue, Dec 27, 2016 at 3:51 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
>
>> > I have no idea how to do it (properly) outside of i2c-i801.c file.
>>
>> I doubt we need a single line of code for this. See [1] and perhaps
>> create an EFI variable with necessary upgrade device node.
>>
>> > Same thing is done for Fujitsu machines, see function
>> > i801_probe_optional_slaves() in i2c-i801.c file. So I did similar
>> > approach for Dell machines.
>>
>> Perhaps, this also needs to be converted to use EFI variable.
>>
>> [1] https://lwn.net/Articles/693212/
>
> There's no guarantee that the laptops in question are booted with UEFI,
> as Dell still supports legacy boot. So assuming the presence of EFI variables
> is somewhat problematic.
> In addition, it requires the user (or something in userspace) to set the UEFI
> variable or configfs tweak, rather than Just Working Out Of The Box.
I have no strong opinion, though I don't support the idea to put all
hacks in the world to the kernel. For example, we have user space tool
to switch USB modem from storage to actual communication device and
that is just working out of the box.
Mika, Darren, what are your opinions?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Valdis.Kletnieks @ 2016-12-27 22:41 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Pali Rohár, Wolfram Sang, Jean Delvare, Steven Honeyman,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario Limonciello, Alex Hung,
Michał Kępień, Takashi Iwai,
linux-i2c, linux-kernel@vger.kernel.org, platform-driver-x86
In-Reply-To: <CAHp75VeWBaPNmT0mb+Gs_ZhFEHFx_H-Ro0UQ9u1VvLhNVjCqYQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 956 bytes --]
On Wed, 28 Dec 2016 00:15:30 +0200, Andy Shevchenko said:
> On Tue, Dec 27, 2016 at 3:51 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > I have no idea how to do it (properly) outside of i2c-i801.c file.
>
> I doubt we need a single line of code for this. See [1] and perhaps
> create an EFI variable with necessary upgrade device node.
>
> > Same thing is done for Fujitsu machines, see function
> > i801_probe_optional_slaves() in i2c-i801.c file. So I did similar
> > approach for Dell machines.
>
> Perhaps, this also needs to be converted to use EFI variable.
>
> [1] https://lwn.net/Articles/693212/
There's no guarantee that the laptops in question are booted with UEFI,
as Dell still supports legacy boot. So assuming the presence of EFI variables
is somewhat problematic.
In addition, it requires the user (or something in userspace) to set the UEFI
variable or configfs tweak, rather than Just Working Out Of The Box.
[-- Attachment #2: Type: application/pgp-signature, Size: 484 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Andy Shevchenko @ 2016-12-27 22:15 UTC (permalink / raw)
To: Pali Rohár
Cc: Wolfram Sang, Jean Delvare, Steven Honeyman, valdis.kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario Limonciello, Alex Hung,
Michał Kępień, Takashi Iwai,
linux-i2c, linux-kernel@vger.kernel.org, platform-driver-x86
In-Reply-To: <201612271451.01505@pali>
On Tue, Dec 27, 2016 at 3:51 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Tuesday 27 December 2016 14:43:49 Wolfram Sang wrote:
>> > Dell platform team told us that some (DMI whitelisted) Dell
>> > Latitude machines have ST microelectronics accelerometer at i2c
>> > address 0x29. That
>> > i2c address is not specified in DMI or ACPI, so runtime detection
>> > without
>> > whitelist which is below is not possible.
>>
>> I'd think this should rather live somewhere in
>> drivers/platform/x86/dell*.c?
>
> i2c_new_device() with lis3lv02d for i801 i2c bus needs to be called
> after initializing i2c-i801 bus driver.
>
> I have no idea how to do it (properly) outside of i2c-i801.c file.
I doubt we need a single line of code for this. See [1] and perhaps
create an EFI variable with necessary upgrade device node.
> Same thing is done for Fujitsu machines, see function
> i801_probe_optional_slaves() in i2c-i801.c file. So I did similar
> approach for Dell machines.
Perhaps, this also needs to be converted to use EFI variable.
[1] https://lwn.net/Articles/693212/
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 resend] i2c: designware: add reset interface
From: Jarkko Nikula @ 2016-12-27 14:41 UTC (permalink / raw)
To: Zhangfei Gao, Wolfram Sang, andriy.shevchenko, mika.westerberg,
p.zabel
Cc: linux-arm-kernel, linux-i2c
In-Reply-To: <1482848560-3752-1-git-send-email-zhangfei.gao@linaro.org>
On 12/27/2016 04:22 PM, Zhangfei Gao wrote:
> Some platforms like hi3660 need do reset first to allow accessing registers
>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Ramiro Oliveira <ramiro.oliveira@synopsys.com>
> ---
> rebase to 4.10-rc1
>
> drivers/i2c/busses/i2c-designware-core.h | 1 +
> drivers/i2c/busses/i2c-designware-platdrv.c | 28 ++++++++++++++++++++++++----
> 2 files changed, 25 insertions(+), 4 deletions(-)
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
^ permalink raw reply
* [PATCH v3 resend] i2c: designware: add reset interface
From: Zhangfei Gao @ 2016-12-27 14:22 UTC (permalink / raw)
To: Wolfram Sang, andriy.shevchenko, mika.westerberg, jarkko.nikula,
p.zabel
Cc: linux-arm-kernel, linux-i2c, Zhangfei Gao
Some platforms like hi3660 need do reset first to allow accessing registers
Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Ramiro Oliveira <ramiro.oliveira@synopsys.com>
---
rebase to 4.10-rc1
drivers/i2c/busses/i2c-designware-core.h | 1 +
drivers/i2c/busses/i2c-designware-platdrv.c | 28 ++++++++++++++++++++++++----
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 26250b4..302807c 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,7 @@ struct dw_i2c_dev {
void __iomem *base;
struct completion cmd_complete;
struct clk *clk;
+ struct reset_control *rst;
u32 (*get_clk_rate_khz) (struct dw_i2c_dev *dev);
struct dw_pci_controller *controller;
int cmd_err;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 6ce4313..79c4b4e 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -38,6 +38,7 @@
#include <linux/pm_runtime.h>
#include <linux/property.h>
#include <linux/io.h>
+#include <linux/reset.h>
#include <linux/slab.h>
#include <linux/acpi.h>
#include <linux/platform_data/i2c-designware.h>
@@ -199,6 +200,14 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
dev->irq = irq;
platform_set_drvdata(pdev, dev);
+ dev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+ if (IS_ERR(dev->rst)) {
+ if (PTR_ERR(dev->rst) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ } else {
+ reset_control_deassert(dev->rst);
+ }
+
if (pdata) {
dev->clk_freq = pdata->i2c_scl_freq;
} else {
@@ -235,12 +244,13 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
&& dev->clk_freq != 1000000 && dev->clk_freq != 3400000) {
dev_err(&pdev->dev,
"Only 100kHz, 400kHz, 1MHz and 3.4MHz supported");
- return -EINVAL;
+ r = -EINVAL;
+ goto exit_reset;
}
r = i2c_dw_eval_lock_support(dev);
if (r)
- return r;
+ goto exit_reset;
dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
@@ -286,10 +296,18 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
}
r = i2c_dw_probe(dev);
- if (r && !dev->pm_runtime_disabled)
- pm_runtime_disable(&pdev->dev);
+ if (r)
+ goto exit_probe;
return r;
+
+exit_probe:
+ if (!dev->pm_runtime_disabled)
+ pm_runtime_disable(&pdev->dev);
+exit_reset:
+ if (!IS_ERR_OR_NULL(dev->rst))
+ reset_control_assert(dev->rst);
+ return r;
}
static int dw_i2c_plat_remove(struct platform_device *pdev)
@@ -306,6 +324,8 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
pm_runtime_put_sync(&pdev->dev);
if (!dev->pm_runtime_disabled)
pm_runtime_disable(&pdev->dev);
+ if (!IS_ERR_OR_NULL(dev->rst))
+ reset_control_assert(dev->rst);
return 0;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2016-12-27 13:51 UTC (permalink / raw)
To: Wolfram Sang
Cc: Jean Delvare, Steven Honeyman, valdis.kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, mario_limonciello, Alex Hung,
Michał Kępień, Takashi Iwai,
linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <21e8453f9ebd2955b2c5e499dc51efb0@the-dreams.de>
[-- Attachment #1: Type: Text/Plain, Size: 814 bytes --]
On Tuesday 27 December 2016 14:43:49 Wolfram Sang wrote:
> > Dell platform team told us that some (DMI whitelisted) Dell
> > Latitude machines have ST microelectronics accelerometer at i2c
> > address 0x29. That
> > i2c address is not specified in DMI or ACPI, so runtime detection
> > without
> > whitelist which is below is not possible.
>
> I'd think this should rather live somewhere in
> drivers/platform/x86/dell*.c?
i2c_new_device() with lis3lv02d for i801 i2c bus needs to be called
after initializing i2c-i801 bus driver.
I have no idea how to do it (properly) outside of i2c-i801.c file.
Same thing is done for Fujitsu machines, see function
i801_probe_optional_slaves() in i2c-i801.c file. So I did similar
approach for Dell machines.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Wolfram Sang @ 2016-12-27 13:43 UTC (permalink / raw)
To: Pali Rohár
Cc: Jean Delvare, Steven Honeyman, valdis.kletnieks, Jochen Eisinger,
Gabriele Mazzotta, Andy Lutomirski, mario_limonciello, Alex Hung,
Michał Kępień, Takashi Iwai,
linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <1482843136-12838-1-git-send-email-pali.rohar@gmail.com>
> Dell platform team told us that some (DMI whitelisted) Dell Latitude
> machines have ST microelectronics accelerometer at i2c address 0x29.
> That
> i2c address is not specified in DMI or ACPI, so runtime detection
> without
> whitelist which is below is not possible.
I'd think this should rather live somewhere in
drivers/platform/x86/dell*.c?
^ permalink raw reply
* [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2016-12-27 12:52 UTC (permalink / raw)
To: Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis.Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario_Limonciello, Alex Hung, Michał Kępień,
Takashi Iwai
Cc: linux-i2c, linux-kernel, platform-driver-x86, Pali Rohár
Dell platform team told us that some (DMI whitelisted) Dell Latitude
machines have ST microelectronics accelerometer at i2c address 0x29. That
i2c address is not specified in DMI or ACPI, so runtime detection without
whitelist which is below is not possible.
Presence of that ST microelectronics accelerometer is verified by existence
of SMO88xx ACPI device which represent that accelerometer. Unfortunately
without i2c address.
This patch registers lis3lv02d device at i2c address 0x29 if is detected.
Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to
conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so
lis3lv02d correctly initialize accelerometer.
Tested on Dell Latitude E6440.
Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
drivers/i2c/busses/i2c-i801.c | 98 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 98 insertions(+)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index eb3627f..188cfd4 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1118,6 +1118,101 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
}
}
+static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
+ u32 nesting_level,
+ void *context,
+ void **return_value)
+{
+ struct acpi_device_info *info;
+ acpi_status status;
+ char *hid;
+
+ status = acpi_get_object_info(obj_handle, &info);
+ if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
+ return AE_OK;
+
+ hid = info->hardware_id.string;
+ if (!hid)
+ return AE_OK;
+
+ if (strlen(hid) < 7)
+ return AE_OK;
+
+ if (memcmp(hid, "SMO88", 5) != 0)
+ return AE_OK;
+
+ *((bool *)return_value) = true;
+ return AE_CTRL_TERMINATE;
+}
+
+static bool is_dell_system_with_lis3lv02d(void)
+{
+ bool found;
+ acpi_status status;
+ const char *vendor;
+
+ vendor = dmi_get_system_info(DMI_SYS_VENDOR);
+ if (strcmp(vendor, "Dell Inc.") != 0)
+ return false;
+
+ /*
+ * Check if ACPI device SMO88xx exists and if is enabled. That ACPI
+ * device represent our ST microelectronics lis3lv02d accelerometer but
+ * unfortunately without any other additional information.
+ */
+ found = false;
+ status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
+ (void **)&found);
+ if (!ACPI_SUCCESS(status) || !found)
+ return false;
+
+ return true;
+}
+
+/*
+ * Dell platform team told us that these Latitude devices have
+ * ST microelectronics accelerometer at i2c address 0x29.
+ * That i2c address is not specified in DMI or ACPI, so runtime
+ * detection without whitelist which is below is not possible.
+ */
+static const char * const dmi_dell_product_names[] = {
+ "Latitude E5250",
+ "Latitude E5450",
+ "Latitude E5550",
+ "Latitude E6440",
+ "Latitude E6440 ATG",
+ "Latitude E6540",
+};
+
+static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
+{
+ struct i2c_board_info info;
+ const char *product_name;
+ bool known_i2c_address;
+ int i;
+
+ known_i2c_address = false;
+ product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
+ for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
+ if (strcmp(product_name, dmi_dell_product_names[i]) == 0) {
+ known_i2c_address = true;
+ break;
+ }
+ }
+
+ if (!known_i2c_address) {
+ dev_warn(&priv->pci_dev->dev,
+ "Accelerometer lis3lv02d i2c device is present "
+ "but its i2c address is unknown, skipping ...\n");
+ return;
+ }
+
+ memset(&info, 0, sizeof(struct i2c_board_info));
+ info.addr = 0x29;
+ strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+ i2c_new_device(&priv->adapter, &info);
+}
+
/* Register optional slaves */
static void i801_probe_optional_slaves(struct i801_priv *priv)
{
@@ -1136,6 +1231,9 @@ static void i801_probe_optional_slaves(struct i801_priv *priv)
if (dmi_name_in_vendors("FUJITSU"))
dmi_walk(dmi_check_onboard_devices, &priv->adapter);
+
+ if (is_dell_system_with_lis3lv02d())
+ register_dell_lis3lv02d_i2c_device(priv);
}
#else
static void __init input_apanel_init(void) {}
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore
From: Hans de Goede @ 2016-12-26 11:07 UTC (permalink / raw)
To: Len Brown
Cc: Andy Shevchenko, Jarkko Nikula, Wolfram Sang, Mika Westerberg,
Takashi Iwai, russianneuromancer @ ya . ru, Vincent Gerris,
linux-i2c
In-Reply-To: <CAJvTdKnqdhYo1hTp33z-dUCO3H5sROZQtDdSaGiL_EwW3DbbqQ@mail.gmail.com>
Hi,
On 25-12-16 19:31, Len Brown wrote:
> Is there a simple way to run a test to keep deep C-states
> and instead disable part or all of i2c on this platform,
> to see how much stability separating the two will buy us?
This should do the trick to completely disable i2c from the kernel
to the pmic, leaving the bus fully free for the punit:
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 1590ad0..fe73271 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -140,6 +140,7 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
if (shared_host) {
dev_info(dev->dev, "I2C bus managed by PUNIT\n");
+ return -ENODEV;
dev->acquire_lock = baytrail_i2c_acquire;
dev->release_lock = baytrail_i2c_release;
dev->pm_runtime_disabled = true;
Note that my patch only disabled deep C-states while the kernel
is accessing the pmic i2c bus, not all the time as most other
workarounds are doing.
> A lot of people are struggling w/ the stability of this platform,
> and it would be great to make some progress on that.
I know that many people are seeing these instabilities related
to deep C-states on Baytrail, but the platform I wrote this patch
on is Cherrytrail, which is actually reasonably stable.
Currently on Cherrytrail we effectively do the above -ENODEV,
because the punit semaphore code is using a wrong register offset
on cherrytrail, causing any attempts by the kernel to acquire
the semaphore to always fail. Once the wrong register offset is
fixed I can very reliably freeze my cherrytrail tablet in
seconds by reading *and only reading* from the pmic, e.g. doing:
i2cdump -y -f 6 0x34
Will usually freeze the system on the second attempt, sometimes
on the first / third and that is repeating it manually, so with
long (100-s ms) pauses between runs.
Debugging that lead me to:
https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/i2c-dw_i2c-Ported-punit-locking-patch-from-MCG-kerne.patch
Which does the same pm_qos cpu latency tricks as my patch is
doing, any that makes the problem completely go away.
TL;DR: yes there still is a lot to investigate wrt stability
issues on Baytrail, but this patch is needed for Cherrytrail
too, fixes a 100% reproducable problem there and the same
workaround is used in Android x86 too, so I believe it would
be good to merge this regardless of the ongoing Baytrail
investigation.
Regardsm
Hans
> thanks,
> -Len
>
>
>
> On Sat, Dec 10, 2016 at 2:59 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 10-12-16 20:33, Hans de Goede wrote:
>>>
>>> Hi,
>>>
>>> On 10-12-16 15:53, Andy Shevchenko wrote:
>>>>
>>>> +Cc: Len
>>>> Len, I think you would be interested by this.
>>>>
>>>> Hans, thanks for the change!
>>>
>>>
>>> You're welcome I ended up comparing the code in
>>> i2c-dw_i2c-Ported-punit-locking-patch-from-MCG-kerne.patch from:
>>>
>>>
>>> https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches
>>>
>>> against the mainline code while I was trying to fix the maddening
>>> problem of the entire SoC hanging more or less as soon
>>> as I tried to use the pmic i2c bus and there I found
>>> some fiddling with pm_qos which let to this patch.
>>>
>>>> Most probably we will anticipate Len's ACK
>>>> on this one.
>>>>
>>>> On Sat, 2016-12-10 at 15:19 +0100, Hans de Goede wrote:
>>>>>
>>>>> On my cherrytrail tablet with axp288 pmic, just doing a bunch of
>>>>> repeated
>>>>> reads from the pmic, e.g. "i2cdump -y 14 0x34" would lookup the tablet
>>>>> in
>>>>> 1 - 3 runs guaranteed.
>>>>>
>>>>> This seems to be causes by the cpuidle / intel_idle driver trying to
>>>>> change the C-state while we hold the punit bus semaphore, at which
>>>>> point
>>>>> everything just hangs.
>>>>>
>>>>> Avoid this by forcing the CPU to C1 before acquiring the punit bus
>>>>> semaphore.
>>>>
>>>>
>>>> Isn't it C0? C1 as far as I remember is halted state.
>>>
>>>
>>> You're right, I will fix it.
>>
>>
>> Correction, upon closer reading of the docs, we cannot disallow
>> the CPU to enter C1 / force it to either C0 or C1, what we can
>> disallow is for it to enter C6/C7. Which also makes sense wrt
>> this bug, since entering C6/C7 involves turning of the
>> CPU-core power-plane, which requires the punit to access the pmic.
>>
>> So I've changes the text in both the commit msg and the comment
>> to: "Disallow the CPU to enter C6 or C7"
>>
>> I still need to re-test (just to make sure I did not cause
>> any regressions) and then I'll send a v3.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>>>> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32
>>>>> *sem)
>>>>> u32 data;
>>>>> int ret;
>>>>>
>>>>> + /*
>>>>> + * Force CPU to C1 state, otherwise if the cpuidle /
>>>>> intel_idle
>>>>> + * driver tries to change the C state while we're holding the
>>>>> + * semaphore, the SoC hangs.
>>>>
>>>>
>>>> C0?
>>>>
>>>>> + */
>>>>> + pm_qos_update_request(&dev->pm_qos, 0);
>>>>
>>>>
>>>> C1 is when you set 1 here, right?
>>>
>>>
>>> I believe so, yes.
>>>
>>>>
>>>>> platform_device *pdev)
>>>>> if (!dev->pm_runtime_disabled)
>>>>> pm_runtime_disable(&pdev->dev);
>>>>
>>>>
>>>>> + if (dev->acquire_lock)
>>>>> + pm_qos_remove_request(&dev->pm_qos);
>>>>> +
>>>>
>>>>
>>>> Perhaps you need to do this in -core.c. Otherwise you missed PCI case.
>>>> (Even with PCI enumerated host with ACPI-enabled firmware you may get
>>>> _SEM object present)
>>>
>>>
>>> Currently only i2c-designware-plardrv.c calls i2c_dw_eval_lock_support()
>>> which does the pm_qos_add_request, so I put it here to keep things
>>> balanced.
>>>
>>> Regards,
>>>
>>> Hans
>
>
>
^ permalink raw reply related
* Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore
From: Len Brown @ 2016-12-25 18:31 UTC (permalink / raw)
To: Hans de Goede
Cc: Andy Shevchenko, Jarkko Nikula, Wolfram Sang, Mika Westerberg,
Takashi Iwai, russianneuromancer @ ya . ru, Vincent Gerris,
linux-i2c
In-Reply-To: <5425712f-550e-cc30-3b52-5bd25eabc5d9@redhat.com>
Is there a simple way to run a test to keep deep C-states
and instead disable part or all of i2c on this platform,
to see how much stability separating the two will buy us?
A lot of people are struggling w/ the stability of this platform,
and it would be great to make some progress on that.
thanks,
-Len
On Sat, Dec 10, 2016 at 2:59 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 10-12-16 20:33, Hans de Goede wrote:
>>
>> Hi,
>>
>> On 10-12-16 15:53, Andy Shevchenko wrote:
>>>
>>> +Cc: Len
>>> Len, I think you would be interested by this.
>>>
>>> Hans, thanks for the change!
>>
>>
>> You're welcome I ended up comparing the code in
>> i2c-dw_i2c-Ported-punit-locking-patch-from-MCG-kerne.patch from:
>>
>>
>> https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches
>>
>> against the mainline code while I was trying to fix the maddening
>> problem of the entire SoC hanging more or less as soon
>> as I tried to use the pmic i2c bus and there I found
>> some fiddling with pm_qos which let to this patch.
>>
>>> Most probably we will anticipate Len's ACK
>>> on this one.
>>>
>>> On Sat, 2016-12-10 at 15:19 +0100, Hans de Goede wrote:
>>>>
>>>> On my cherrytrail tablet with axp288 pmic, just doing a bunch of
>>>> repeated
>>>> reads from the pmic, e.g. "i2cdump -y 14 0x34" would lookup the tablet
>>>> in
>>>> 1 - 3 runs guaranteed.
>>>>
>>>> This seems to be causes by the cpuidle / intel_idle driver trying to
>>>> change the C-state while we hold the punit bus semaphore, at which
>>>> point
>>>> everything just hangs.
>>>>
>>>> Avoid this by forcing the CPU to C1 before acquiring the punit bus
>>>> semaphore.
>>>
>>>
>>> Isn't it C0? C1 as far as I remember is halted state.
>>
>>
>> You're right, I will fix it.
>
>
> Correction, upon closer reading of the docs, we cannot disallow
> the CPU to enter C1 / force it to either C0 or C1, what we can
> disallow is for it to enter C6/C7. Which also makes sense wrt
> this bug, since entering C6/C7 involves turning of the
> CPU-core power-plane, which requires the punit to access the pmic.
>
> So I've changes the text in both the commit msg and the comment
> to: "Disallow the CPU to enter C6 or C7"
>
> I still need to re-test (just to make sure I did not cause
> any regressions) and then I'll send a v3.
>
> Regards,
>
> Hans
>
>
>
>
>>>> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32
>>>> *sem)
>>>> u32 data;
>>>> int ret;
>>>>
>>>> + /*
>>>> + * Force CPU to C1 state, otherwise if the cpuidle /
>>>> intel_idle
>>>> + * driver tries to change the C state while we're holding the
>>>> + * semaphore, the SoC hangs.
>>>
>>>
>>> C0?
>>>
>>>> + */
>>>> + pm_qos_update_request(&dev->pm_qos, 0);
>>>
>>>
>>> C1 is when you set 1 here, right?
>>
>>
>> I believe so, yes.
>>
>>>
>>>> platform_device *pdev)
>>>> if (!dev->pm_runtime_disabled)
>>>> pm_runtime_disable(&pdev->dev);
>>>
>>>
>>>> + if (dev->acquire_lock)
>>>> + pm_qos_remove_request(&dev->pm_qos);
>>>> +
>>>
>>>
>>> Perhaps you need to do this in -core.c. Otherwise you missed PCI case.
>>> (Even with PCI enumerated host with ACPI-enabled firmware you may get
>>> _SEM object present)
>>
>>
>> Currently only i2c-designware-plardrv.c calls i2c_dw_eval_lock_support()
>> which does the pm_qos_add_request, so I put it here to keep things
>> balanced.
>>
>> Regards,
>>
>> Hans
--
Len Brown, Intel Open Source Technology Center
^ permalink raw reply
* [PATCH v3] i2c: designware: add reset interface
From: Zhangfei Gao @ 2016-12-23 13:40 UTC (permalink / raw)
To: Wolfram Sang, andriy.shevchenko, mika.westerberg, jarkko.nikula,
p.zabel
Cc: linux-arm-kernel, linux-i2c, Zhangfei Gao
Some platforms like hi3660 need do reset first to allow accessing registers
Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Ramiro Oliveira <ramiro.oliveira@synopsys.com>
---
drivers/i2c/busses/i2c-designware-core.h | 1 +
drivers/i2c/busses/i2c-designware-platdrv.c | 28 ++++++++++++++++++++++++----
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 0d44d2a..94b14fa 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -80,6 +80,7 @@ struct dw_i2c_dev {
void __iomem *base;
struct completion cmd_complete;
struct clk *clk;
+ struct reset_control *rst;
u32 (*get_clk_rate_khz) (struct dw_i2c_dev *dev);
struct dw_pci_controller *controller;
int cmd_err;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0b42a12..1fbe66f 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -38,6 +38,7 @@
#include <linux/pm_runtime.h>
#include <linux/property.h>
#include <linux/io.h>
+#include <linux/reset.h>
#include <linux/slab.h>
#include <linux/acpi.h>
#include <linux/platform_data/i2c-designware.h>
@@ -176,6 +177,14 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
dev->irq = irq;
platform_set_drvdata(pdev, dev);
+ dev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+ if (IS_ERR(dev->rst)) {
+ if (PTR_ERR(dev->rst) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ } else {
+ reset_control_deassert(dev->rst);
+ }
+
/* fast mode by default because of legacy reasons */
dev->clk_freq = 400000;
@@ -207,12 +216,13 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
&& dev->clk_freq != 1000000 && dev->clk_freq != 3400000) {
dev_err(&pdev->dev,
"Only 100kHz, 400kHz, 1MHz and 3.4MHz supported");
- return -EINVAL;
+ r = -EINVAL;
+ goto exit_reset;
}
r = i2c_dw_eval_lock_support(dev);
if (r)
- return r;
+ goto exit_reset;
dev->functionality =
I2C_FUNC_I2C |
@@ -270,10 +280,18 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
}
r = i2c_dw_probe(dev);
- if (r && !dev->pm_runtime_disabled)
- pm_runtime_disable(&pdev->dev);
+ if (r)
+ goto exit_probe;
return r;
+
+exit_probe:
+ if (!dev->pm_runtime_disabled)
+ pm_runtime_disable(&pdev->dev);
+exit_reset:
+ if (!IS_ERR_OR_NULL(dev->rst))
+ reset_control_assert(dev->rst);
+ return r;
}
static int dw_i2c_plat_remove(struct platform_device *pdev)
@@ -290,6 +308,8 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
pm_runtime_put_sync(&pdev->dev);
if (!dev->pm_runtime_disabled)
pm_runtime_disable(&pdev->dev);
+ if (!IS_ERR_OR_NULL(dev->rst))
+ reset_control_assert(dev->rst);
return 0;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2] i2c: designware: add reset interface
From: zhangfei @ 2016-12-23 13:39 UTC (permalink / raw)
To: Philipp Zabel
Cc: Ramiro Oliveira, Andy Shevchenko, Wolfram Sang,
mika.westerberg@linux.intel.com, jarkko.nikula@linux.intel.com,
linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org
In-Reply-To: <1482488792.2394.21.camel@pengutronix.de>
On 2016年12月23日 18:26, Philipp Zabel wrote:
> Hi Zhangfei,
>
> Am Freitag, den 16.12.2016, 11:01 +0800 schrieb zhangfei:
>> Hi, Philipp
>>
>> On 2016年12月16日 10:45, zhangfei wrote:
> [...]
>> Sorry, a bit confused.
>> Is that mean we should not use devm_reset_control_get_optional()
>> While driver should decide whether use
>> devm_reset_control_get_optional_exclusive() or
>> devm_reset_control_get_optional_shared()
>> What if different platform has different requirement?
>>
>> Looks the difference between _exclusive and _shared is refcount,
>> How about handle this inside, and not decided by interface?
> Because there is a significant difference in how the actual reset line
> behaves. The shared resets are clock-like, and should only be used if
> the device needs them to be deasserted to be enabled, but is fine if
> they have been deasserted earlier or if they are not immediately
> asserted after the device is disabled, but some time later.
> For the shared / non-exclusive resets calling reset_control_assert and
> then reset_control_deassert is not guaranteed to do anything at all,
> because another user of the reset line could keep it deasserted.
>
> If the device needs a reset to be issued at a certain point in time on
> the other hand, for example to reset its state machine or registers to a
> known good state, calling assert must guarantee to actually assert the
> reset. This can only be done if the reset is exclusive.
>
> Whether guaranteed asserts (exclusive reset lines) are necessary depends
> on the device, so this decision has to be made in the driver.
Thanks Philipp for the kind explanation.
Will use devm_reset_control_get_optional_exclusive here.
Thanks
^ permalink raw reply
* Re: [PATCH v7 3/5] ARM: dts: stm32: Add I2C1 support for STM32F429 SoC
From: M'boumba Cedric Madianga @ 2016-12-23 13:09 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: devicetree, Alexandre Torgue, Wolfram Sang, linux-kernel,
Linus Walleij, Patrice Chotard, linux, Rob Herring, linux-i2c,
Maxime Coquelin, linux-arm-kernel
In-Reply-To: <20161222191103.vzmfhnrtrzw2ivwa@pengutronix.de>
Hi,
2016-12-22 20:11 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Thu, Dec 22, 2016 at 02:35:02PM +0100, M'boumba Cedric Madianga wrote:
>> @@ -337,6 +350,16 @@
>> slew-rate = <2>;
>> };
>> };
>> +
>> + i2c1_pins_b: i2c1@0 {
>> + pins1 {
>> + pinmux = <STM32F429_PB9_FUNC_I2C1_SDA>;
>> + drive-open-drain;
>> + };
>> + pins2 {
>> + pinmux = <STM32F429_PB6_FUNC_I2C1_SCL>;
>> + };
>
> the second doesn't need the open-drain property? Why?
I thought that open-drain was only needed for SDA line.
But after double-checking I2C specification, it seems that SDA and SCL
lines need open-drain or open-collector to perform the wired-AND
function.
I will do some trials with this config and will fix it in the V8.
Thanks
>
>> + };
>> };
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2016-12-23 12:41 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Wolfram Sang, Rob Herring, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Patrice Chotard, linux, linux-i2c, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20161223090019.a3jkhpb3abgjqi55@pengutronix.de>
Hi Uwe,
Thanks for your comments.
Please see below my answers and one question regarding duty cycle:
2016-12-23 10:00 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Thu, Dec 22, 2016 at 02:35:01PM +0100, M'boumba Cedric Madianga wrote:
>> This patch adds support for the STM32F4 I2C controller.
>>
>> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
>> ---
>> drivers/i2c/busses/Kconfig | 10 +
>> drivers/i2c/busses/Makefile | 1 +
>> drivers/i2c/busses/i2c-stm32f4.c | 896 +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 907 insertions(+)
>> create mode 100644 drivers/i2c/busses/i2c-stm32f4.c
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 0cdc844..2719208 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -886,6 +886,16 @@ config I2C_ST
>> This driver can also be built as module. If so, the module
>> will be called i2c-st.
>>
>> +config I2C_STM32F4
>> + tristate "STMicroelectronics STM32F4 I2C support"
>> + depends on ARCH_STM32 || COMPILE_TEST
>> + help
>> + Enable this option to add support for STM32 I2C controller embedded
>> + in STM32F4 SoCs.
>> +
>> + This driver can also be built as module. If so, the module
>> + will be called i2c-stm32f4.
>> +
>> config I2C_STU300
>> tristate "ST Microelectronics DDC I2C interface"
>> depends on MACH_U300
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 1c1bac8..a2c6ff5 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -85,6 +85,7 @@ obj-$(CONFIG_I2C_SH_MOBILE) += i2c-sh_mobile.o
>> obj-$(CONFIG_I2C_SIMTEC) += i2c-simtec.o
>> obj-$(CONFIG_I2C_SIRF) += i2c-sirf.o
>> obj-$(CONFIG_I2C_ST) += i2c-st.o
>> +obj-$(CONFIG_I2C_STM32F4) += i2c-stm32f4.o
>> obj-$(CONFIG_I2C_STU300) += i2c-stu300.o
>> obj-$(CONFIG_I2C_SUN6I_P2WI) += i2c-sun6i-p2wi.o
>> obj-$(CONFIG_I2C_TEGRA) += i2c-tegra.o
>> diff --git a/drivers/i2c/busses/i2c-stm32f4.c b/drivers/i2c/busses/i2c-stm32f4.c
>> new file mode 100644
>> index 0000000..ca11dee
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-stm32f4.c
>> @@ -0,0 +1,896 @@
>> +/*
>> + * Driver for STMicroelectronics STM32 I2C controller
>> + *
>> + * This I2C controller is described in the STM32F429/439 Soc reference manual.
>> + * Please see below a link to the documentation:
>> + * http://www.st.com/resource/en/reference_manual/DM00031020.pdf
>> + *
>> + * Copyright (C) M'boumba Cedric Madianga 2016
>> + * Author: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
>> + *
>> + * This driver is based on i2c-st.c
>> + *
>> + * License terms: GNU General Public License (GPL), version 2
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +
>> +/* STM32F4 I2C offset registers */
>> +#define STM32F4_I2C_CR1 0x00
>> +#define STM32F4_I2C_CR2 0x04
>> +#define STM32F4_I2C_DR 0x10
>> +#define STM32F4_I2C_SR1 0x14
>> +#define STM32F4_I2C_SR2 0x18
>> +#define STM32F4_I2C_CCR 0x1C
>> +#define STM32F4_I2C_TRISE 0x20
>> +#define STM32F4_I2C_FLTR 0x24
>> +
>> +/* STM32F4 I2C control 1*/
>> +#define STM32F4_I2C_CR1_SWRST BIT(15)
>> +#define STM32F4_I2C_CR1_POS BIT(11)
>> +#define STM32F4_I2C_CR1_ACK BIT(10)
>> +#define STM32F4_I2C_CR1_STOP BIT(9)
>> +#define STM32F4_I2C_CR1_START BIT(8)
>> +#define STM32F4_I2C_CR1_PE BIT(0)
>> +
>> +/* STM32F4 I2C control 2 */
>> +#define STM32F4_I2C_CR2_FREQ_MASK GENMASK(5, 0)
>> +#define STM32F4_I2C_CR2_FREQ(n) (((n) & STM32F4_I2C_CR2_FREQ_MASK))
>
> ((n) & STM32F4_I2C_CR2_FREQ_MASK)
>
> should be enough.
You are right. I will fix it in the V8.
>
>> +#define STM32F4_I2C_CR2_ITBUFEN BIT(10)
>> +#define STM32F4_I2C_CR2_ITEVTEN BIT(9)
>> +#define STM32F4_I2C_CR2_ITERREN BIT(8)
>> +#define STM32F4_I2C_CR2_IRQ_MASK (STM32F4_I2C_CR2_ITBUFEN | \
>> + STM32F4_I2C_CR2_ITEVTEN | \
>> + STM32F4_I2C_CR2_ITERREN)
>> +
>> +/* STM32F4 I2C Status 1 */
>> +#define STM32F4_I2C_SR1_AF BIT(10)
>> +#define STM32F4_I2C_SR1_ARLO BIT(9)
>> +#define STM32F4_I2C_SR1_BERR BIT(8)
>> +#define STM32F4_I2C_SR1_TXE BIT(7)
>> +#define STM32F4_I2C_SR1_RXNE BIT(6)
>> +#define STM32F4_I2C_SR1_BTF BIT(2)
>> +#define STM32F4_I2C_SR1_ADDR BIT(1)
>> +#define STM32F4_I2C_SR1_SB BIT(0)
>> +#define STM32F4_I2C_SR1_ITEVTEN_MASK (STM32F4_I2C_SR1_BTF | \
>> + STM32F4_I2C_SR1_ADDR | \
>> + STM32F4_I2C_SR1_SB)
>> +#define STM32F4_I2C_SR1_ITBUFEN_MASK (STM32F4_I2C_SR1_TXE | \
>> + STM32F4_I2C_SR1_RXNE)
>> +#define STM32F4_I2C_SR1_ITERREN_MASK (STM32F4_I2C_SR1_AF | \
>> + STM32F4_I2C_SR1_ARLO | \
>> + STM32F4_I2C_SR1_BERR)
>> +
>> +/* STM32F4 I2C Status 2 */
>> +#define STM32F4_I2C_SR2_BUSY BIT(1)
>> +
>> +/* STM32F4 I2C Control Clock */
>> +#define STM32F4_I2C_CCR_CCR_MASK GENMASK(11, 0)
>> +#define STM32F4_I2C_CCR_CCR(n) (((n) & STM32F4_I2C_CCR_CCR_MASK))
>
> ditto
ok
>
>> +#define STM32F4_I2C_CCR_FS BIT(15)
>> +#define STM32F4_I2C_CCR_DUTY BIT(14)
>> +
>> +/* STM32F4 I2C Trise */
>> +#define STM32F4_I2C_TRISE_VALUE_MASK GENMASK(5, 0)
>> +#define STM32F4_I2C_TRISE_VALUE(n) (((n) & STM32F4_I2C_TRISE_VALUE_MASK))
>> +
>> +/* STM32F4 I2C Filter */
>> +#define STM32F4_I2C_FLTR_DNF_MASK GENMASK(3, 0)
>> +#define STM32F4_I2C_FLTR_DNF(n) (((n) & STM32F4_I2C_FLTR_DNF_MASK))
>> +#define STM32F4_I2C_FLTR_ANOFF BIT(4)
>> +
>> +#define STM32F4_I2C_MIN_FREQ 2U
>> +#define STM32F4_I2C_MAX_FREQ 42U
>> +#define HZ_TO_MHZ 1000000
>> +
>> +enum stm32f4_i2c_speed {
>> + STM32F4_I2C_SPEED_STANDARD, /* 100 kHz */
>> + STM32F4_I2C_SPEED_FAST, /* 400 kHz */
>> + STM32F4_I2C_SPEED_END,
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_timings - per-Mode tuning parameters
>> + * @duty: Fast mode duty cycle
>> + * @scl_period: SCL low/high period in microsecond
>> + * @mul_ccr: Value to be multiplied to CCR to reach 100Khz/400Khz SCL frequency
>> + * @min_ccr: Minimum clock ctrl reg value to reach 100Khz/400Khz SCL frequency
>
> s/Khz/ kHz/
Good point. Thanks
>
>> + */
>> +struct stm32f4_i2c_timings {
>> + u32 duty;
>> + u32 scl_period;
>> + u32 mul_ccr;
>> + u32 min_ccr;
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_msg - client specific data
>> + * @addr: 8-bit slave addr, including r/w bit
>> + * @count: number of bytes to be transferred
>> + * @buf: data buffer
>> + * @result: result of the transfer
>> + * @stop: last I2C msg to be sent, i.e. STOP to be generated
>> + */
>> +struct stm32f4_i2c_msg {
>> + u8 addr;
>> + u32 count;
>> + u8 *buf;
>> + int result;
>> + bool stop;
>
> You bought the argument about alignment of = in stm32f4_i2c_driver. The
> same logic applies here.
ok
>
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_dev - private data of the controller
>> + * @adap: I2C adapter for this controller
>> + * @dev: device for this controller
>> + * @base: virtual memory area
>> + * @complete: completion of I2C message
>> + * @clk: hw i2c clock
>> + * speed: I2C clock frequency of the controller. Standard or Fast only supported
>> + * @msg: I2C transfer information
>> + */
>> +struct stm32f4_i2c_dev {
>> + struct i2c_adapter adap;
>> + struct device *dev;
>> + void __iomem *base;
>> + struct completion complete;
>> + struct clk *clk;
>> + int speed;
>> + struct stm32f4_i2c_msg msg;
>> +};
>
> ditto
ok
>
>> +
>> +/*
>> + * In standard mode:
>> + * SCL high period = SCL low period = CCR * I2C CLK period
>> + * So, CCR = SCL period * I2C CLK frequency
>
> is "SCL period" the same as "SCL low period"? I2C CLK frequency is the
> input clk? If so, it's confusing that it has I2C in its name. The
> reference manual calls it PCLK1 (parent clock?).
SCL high period = SCL low period
I2C CLK frequency = I2C parent clock frequency so I will add this
precision in the V8
>
>> + * In fast mode:
>> + * DUTY = 0: Fast mode tlow/thigh = 2
>> + * DUTY = 1: Fast mode tlow/thigh = 16/9
>> + * If Duty = 0; SCL high period = 1 * CCR * I2C CLK period
>> + * SCL low period = 2 * CCR * I2C CLK period
>> + * If Duty = 1; SCL high period = 9 * CCR * I2C CLK period
>> + * SCL low period = 16 * CCR * I2C CLK period
>
> I'd drop the first two lines about the proportions
>
>> + *
>> + * Note that Duty has to bet set to reach 400khz in Fast mode
>
> s/khz/ kHz/
Ok. Thanks.
>
> I don't understand why DUTY is required to reach 400 kHz. Given a parent
> freq of 30 MHz, with CCR = 25 and DUTY = 0 we have:
>
> t_high = 25 * 33.333 ns = 833.333 ns
> t_low = 2 * 25 * 33.333 ns = 1666.667 ns
>
> then t_high and t_low satisfy the i2c bus specification
> (t_low > 1300 ns, t_high > 600 ns) and we have t_low + t_high = 2500 ns
> = 1 / 400 kHz.
>
> Where is the error?
Hum ok you are right. I was a bad interpretation of the datasheet.
So now it is clearer. Thanks for that.
I will correct and improve my comments in the V8.
>
>> + * So, in order to cover both SCL high/low with Duty = 1,
>> + * CCR = 16 * SCL period * I2C CLK frequency
>
> I don't get that. Actually you need to use low + high, so
> CCR = parentrate / (25 * 400 kHz), right?
With your new inputs above, I think I could use a simpler implementation:
CCR = scl_high_period * parent_rate
with scl_high_period = 5 µs in standard mode to reach 100khz
and scl_high_period = 1 µs in fast mode to reach 400khz with 1/2 or
16/9 duty cycle.
So, I am wondering if I have to let the customer setting the duty
cycle in the DT for example with "st,duty=0" or "st,duty=1" property
(0 for 1/2 and 1 for 16/9).
Or perhaps the best option it to use a default value. What is your
feeling regarding this point ?
>
>> + *
>> + * Please note that the minimum allowed value is 0x04, except in FAST DUTY mode
>> + * where the minimum allowed value is 0x01
>> + */
>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> + [STM32F4_I2C_SPEED_STANDARD] = {
>> + .mul_ccr = 1,
>> + .min_ccr = 4,
>> + .duty = 0,
>> + .scl_period = 5,
>> + },
>> + [STM32F4_I2C_SPEED_FAST] = {
>> + .mul_ccr = 16,
>> + .min_ccr = 1,
>> + .duty = 1,
>> + .scl_period = 2,
>> + },
>> +};
>> +
>> +static inline void stm32f4_i2c_set_bits(void __iomem *reg, u32 mask)
>> +{
>> + writel_relaxed(readl_relaxed(reg) | mask, reg);
>> +}
>> +
>> +static inline void stm32f4_i2c_clr_bits(void __iomem *reg, u32 mask)
>> +{
>> + writel_relaxed(readl_relaxed(reg) & ~mask, reg);
>> +}
>> +
>> +static void stm32f4_i2c_soft_reset(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + u32 val;
>> +
>> + val = readl_relaxed(reg);
>> + writel_relaxed(val | STM32F4_I2C_CR1_SWRST, reg);
>> + writel_relaxed(val, reg);
>> +}
>> +
>> +static void stm32f4_i2c_disable_irq(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_IRQ_MASK);
>> +}
>> +
>> +static void stm32f4_i2c_set_periph_clk_freq(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 clk_rate, cr2, freq;
>> +
>> + /*
>> + * The minimum allowed frequency is 2 MHz, the maximum frequency is
>> + * limited by the maximum APB frequency 42 MHz
>> + */
>> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
>> + clk_rate = clk_get_rate(i2c_dev->clk);
>> + freq = DIV_ROUND_UP(clk_rate, HZ_TO_MHZ);
>> + freq = clamp(freq, STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ);
>> + cr2 |= STM32F4_I2C_CR2_FREQ(freq);
>> + writel_relaxed(cr2, i2c_dev->base + STM32F4_I2C_CR2);
>
> Last round I suggested error checking here instead of silent clamping.
Ok
>
>> +}
>> +
>> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 trise, freq, cr2;
>> +
>> + /*
>> + * These bits must be programmed with the maximum SCL rise time given in
>> + * the I2C bus specification, incremented by 1.
>> + *
>> + * In standard mode, the maximum allowed SCL rise time is 1000 ns.
>> + * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
>> + * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
>> + * programmed with 09h.(1000 ns / 125 ns = 8 + 1)
>> + * So, for I2C standard mode TRISE = FREQ[5:0] + 1
>> + *
>> + * In fast mode, the maximum allowed SCL rise time is 300 ns.
>> + * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
>> + * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
>> + * programmed with 03h.(300 ns / 125 ns = 2 + 1)
>> + * So, for I2C fast mode TRISE = FREQ[5:0] * 300 / 1000 + 1
>> + */
>> +
>> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
>> +
>> + if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
>> + trise = freq + 1;
>> + else
>> + trise = freq * 300 / 1000 + 1;
>
> if freq is big such that freq * 300 overflows does this result in a
> wrong result, or does the compiler optimize correctly?
For sure the compiler will never exceeds u32 max value
>
>> + writel_relaxed(STM32F4_I2C_TRISE_VALUE(trise),
>> + i2c_dev->base + STM32F4_I2C_TRISE);
>> +}
>> +
>> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_timings *t = &i2c_timings[i2c_dev->speed];
>> + u32 cr2, ccr, freq, val;
>> +
>> + ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
>> + ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
>> + STM32F4_I2C_CCR_CCR_MASK);
>> +
>> + /*
>> + * Please see the comments above regarding i2c_timings[] declaration
>> + * to understand the below calculation
>> + */
>> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
>> + val = freq * t->scl_period * t->mul_ccr;
>> + if (val < t->min_ccr)
>> + val = t->min_ccr;
>> + ccr |= STM32F4_I2C_CCR_CCR(val);
>> +
>> + if (t->duty)
>> + ccr |= STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY;
>> +
>> + writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
>> +}
>> +
>> +static void stm32f4_i2c_set_filter(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 filter;
>> +
>> + /* Enable analog noise filter and disable digital noise filter */
>> + filter = readl_relaxed(i2c_dev->base + STM32F4_I2C_FLTR);
>> + filter &= ~(STM32F4_I2C_FLTR_ANOFF | STM32F4_I2C_FLTR_DNF_MASK);
>> + writel_relaxed(filter, i2c_dev->base + STM32F4_I2C_FLTR);
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_hw_config() - Prepare I2C block
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +
>> + /* Disable I2C */
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>> +
>> + stm32f4_i2c_set_periph_clk_freq(i2c_dev);
>> +
>> + stm32f4_i2c_set_rise_time(i2c_dev);
>> +
>> + stm32f4_i2c_set_speed_mode(i2c_dev);
>> +
>> + stm32f4_i2c_set_filter(i2c_dev);
>> +
>> + /* Enable I2C */
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
>
> This is the first write to STM32F4_I2C_CR1, right? So the state from the
> bootloader leaks here. This probably works most of the time, but if it
> makes problems later, that's hard to debug. Also, what if the bootloader
> already did some i2c transfers and kept the PE bit 1? I read in the
> manual that PE must be 0 for some things. So this only works most of the
> time.
The first thing I do before configuring I2C device is to clear PE bit
in order to disable I2C.
In that way, all previous config will be erased by the new one.
>
>> +}
>> +
>> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 status;
>> + int ret;
>> +
>> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> + status,
>> + !(status & STM32F4_I2C_SR2_BUSY),
>> + 10, 1000);
>> + if (ret) {
>> + dev_err(i2c_dev->dev, "bus not free\n");
>
> drop error message please or degrade to dev_debug
ok I will use a dev_dbg message
>
>> + ret = -EBUSY;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
>> + * @i2c_dev: Controller's private data
>> + * @byte: Data to write in the register
>> + */
>> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
>> +{
>> + writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
>> + * @i2c_dev: Controller's private data
>> + *
>> + * This function fills the data register with I2C transfer buffer
>> + */
>> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +
>> + stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
>> + msg->count--;
>> +}
>> +
>> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + u32 rbuf;
>> +
>> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>> + *msg->buf++ = rbuf & 0xff;
>> + msg->count--;
>> +}
>> +
>> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + stm32f4_i2c_disable_irq(i2c_dev);
>> +
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +
>> + complete(&i2c_dev->complete);
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + if (msg->count) {
>> + stm32f4_i2c_write_msg(i2c_dev);
>> + if (!msg->count) {
>> + /* Disable buffer interrupts for RXNE/TXE events */
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> + }
>> + } else {
>> + stm32f4_i2c_terminate_xfer(i2c_dev);
>> + }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + switch (msg->count) {
>> + case 1:
>> + stm32f4_i2c_disable_irq(i2c_dev);
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + complete(&i2c_dev->complete);
>> + break;
>> + case 2:
>> + case 3:
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> + break;
>> + default:
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
>> + * in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> + u32 mask;
>> + int i;
>> +
>> + switch (msg->count) {
>> + case 2:
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + /* Generate STOP or repeated Start */
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +
>> + /* Read two last data bytes */
>> + for (i = 2; i > 0; i--)
>> + stm32f4_i2c_read_msg(i2c_dev);
>> +
>> + /* Disable events and error interrupts */
>> + reg = i2c_dev->base + STM32F4_I2C_CR2;
>> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>> + stm32f4_i2c_clr_bits(reg, mask);
>> +
>> + complete(&i2c_dev->complete);
>> + break;
>> + case 3:
>> + /* Enable ACK and read data */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + break;
>> + default:
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
>> + * master receiver
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> +
>> + switch (msg->count) {
>> + case 0:
>> + stm32f4_i2c_terminate_xfer(i2c_dev);
>> + /* Clear ADDR flag */
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + break;
>> + case 1:
>> + /*
>> + * Single byte reception:
>> + * Enable NACK, clear ADDR flag and generate STOP or RepSTART
>> + */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> + break;
>> + case 2:
>> + /*
>> + * 2-byte reception:
>> + * Enable NACK and set POS
>> + */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + break;
>> +
>> + default:
>> + /* N-byte reception: Enable ACK */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + break;
>> + }
>> +}
>
> This is still not really understandable.
I have already added some comments from datasheet to explain the
different cases.
I don't see how I could be more understandable as it is clearly the
hardware way of working...
>
>> +
>> +/**
>> + * stm32f4_i2c_isr_event() - Interrupt routine for I2C bus event
>> + * @irq: interrupt number
>> + * @data: Controller's private data
>> + */
>> +static irqreturn_t stm32f4_i2c_isr_event(int irq, void *data)
>> +{
>> + struct stm32f4_i2c_dev *i2c_dev = data;
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> + u32 status, possible_status, ien;
>> + int flag;
>> +
>> + ien = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + ien &= STM32F4_I2C_CR2_IRQ_MASK;
>> + possible_status = 0;
>
> This can already be done when declaring possible_status.
Ok.
>
>> +
>> + /* Check possible status combinations */
>> + if (ien & STM32F4_I2C_CR2_ITEVTEN) {
>> + possible_status = STM32F4_I2C_SR1_ITEVTEN_MASK;
>> + if (ien & STM32F4_I2C_CR2_ITBUFEN)
>> + possible_status |= STM32F4_I2C_SR1_ITBUFEN_MASK;
>> + }
>> +
>> + status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
>> +
>> + if (!(status & possible_status)) {
>> + dev_dbg(i2c_dev->dev,
>> + "spurious evt irq (status=0x%08x, ien=0x%08x)\n",
>> + status, ien);
>> + return IRQ_NONE;
>> + }
>> +
>> + while (status & possible_status) {
>> + /* Use __fls() to check error bits first */
>> + flag = __fls(status & possible_status);
>> +
>> + switch (1 << flag) {
>> + case STM32F4_I2C_SR1_SB:
>> + stm32f4_i2c_write_byte(i2c_dev, msg->addr);
>> + break;
>> +
>> + case STM32F4_I2C_SR1_ADDR:
>> + if (msg->addr & I2C_M_RD)
>> + stm32f4_i2c_handle_rx_addr(i2c_dev);
>> + else
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> +
>> + /* Enable buffer interrupts for RXNE/TXE events */
>> + reg = i2c_dev->base + STM32F4_I2C_CR2;
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> + possible_status |= STM32F4_I2C_SR1_ITBUFEN_MASK;
>> + break;
>> +
>> + case STM32F4_I2C_SR1_BTF:
>> + if (msg->addr & I2C_M_RD)
>> + stm32f4_i2c_handle_rx_btf(i2c_dev);
>> + else
>> + stm32f4_i2c_handle_write(i2c_dev);
>> + break;
>> +
>> + case STM32F4_I2C_SR1_TXE:
>> + stm32f4_i2c_handle_write(i2c_dev);
>> + break;
>> +
>> + case STM32F4_I2C_SR1_RXNE:
>> + stm32f4_i2c_handle_read(i2c_dev);
>> + break;
>> +
>> + default:
>> + dev_err(i2c_dev->dev,
>> + "evt irq unhandled: status=0x%08x)\n",
>> + status);
>> + return IRQ_NONE;
>> + }
>> + status &= ~(1 << flag);
>> + }
>
> I wouldn't do this in a loop. Just do:
>
> if (status & STM32F4_I2C_SR1_SB) {
> ...
> }
>
> if (status & ...) {
>
> }
ok but I would prefer something like that:
flag = status & possible_status
if (flag & STM32F4_I2C_SR1_SB) {
...
}
if (flag & ...) {
}
>
> Then it's obvious by reading the code in which order they are handled
> without the need to check the definitions. Do you really need to jugle
> with possible_status?
I think I have to use possible_status as some events could occur
whereas the corresponding interrupt is disabled.
For example, for a 2 byte-reception, we don't have to take into accout
RXNE event so the corresponding interrupt is disabled.
If I don't check possible_status, I will call
stm32f4_i2c_handle_read() for nothing and generates unneeded registers
accesses.
>
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_isr_error() - Interrupt routine for I2C bus error
>> + * @irq: interrupt number
>> + * @data: Controller's private data
>> + */
>> +static irqreturn_t stm32f4_i2c_isr_error(int irq, void *data)
>> +{
>> + struct stm32f4_i2c_dev *i2c_dev = data;
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> + u32 status, possible_status, ien;
>> + int flag;
>> +
>> + ien = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + ien &= STM32F4_I2C_CR2_IRQ_MASK;
>> + possible_status = 0;
>> +
>> + /* Check possible status combinations */
>> + if (ien & STM32F4_I2C_CR2_ITERREN)
>> + possible_status = STM32F4_I2C_SR1_ITERREN_MASK;
>> +
>> + status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
>> +
>> + if (!(status & possible_status)) {
>> + dev_dbg(i2c_dev->dev,
>> + "spurious err it (status=0x%08x, ien=0x%08x)\n",
>> + status, ien);
>> + return IRQ_NONE;
>> + }
>> +
>> + /* Use __fls() to check error bits first */
>> + flag = __fls(status & possible_status);
>> +
>> + switch (1 << flag) {
>> + case STM32F4_I2C_SR1_BERR:
>> + reg = i2c_dev->base + STM32F4_I2C_SR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_BERR);
>> + msg->result = -EIO;
>> + break;
>> +
>> + case STM32F4_I2C_SR1_ARLO:
>> + reg = i2c_dev->base + STM32F4_I2C_SR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_ARLO);
>> + msg->result = -EAGAIN;
>> + break;
>> +
>> + case STM32F4_I2C_SR1_AF:
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + msg->result = -EIO;
>> + break;
>> +
>> + default:
>> + dev_err(i2c_dev->dev,
>> + "err it unhandled: status=0x%08x)\n", status);
>> + return IRQ_NONE;
>> + }
>
> You only check a single irq flag here.
Yes only the first error could be reported to the i2c clients via
msg->result that's why I don't check all errors.
Moreover, as soon as an error occurs, the I2C device is reset.
>
>> +
>> + stm32f4_i2c_soft_reset(i2c_dev);
>> + stm32f4_i2c_disable_irq(i2c_dev);
>> + complete(&i2c_dev->complete);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_xfer_msg() - Transfer a single I2C message
>> + * @i2c_dev: Controller's private data
>> + * @msg: I2C message to transfer
>> + * @is_first: first message of the sequence
>> + * @is_last: last message of the sequence
>> + */
>> +static int stm32f4_i2c_xfer_msg(struct stm32f4_i2c_dev *i2c_dev,
>> + struct i2c_msg *msg, bool is_first,
>> + bool is_last)
>> +{
>> + struct stm32f4_i2c_msg *f4_msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + unsigned long timeout;
>> + u32 mask;
>> + int ret;
>> +
>> + f4_msg->addr = i2c_8bit_addr_from_msg(msg);
>> + f4_msg->buf = msg->buf;
>> + f4_msg->count = msg->len;
>> + f4_msg->result = 0;
>> + f4_msg->stop = is_last;
>> +
>> + reinit_completion(&i2c_dev->complete);
>> +
>> + /* Enable events and errors interrupts */
>> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>> + stm32f4_i2c_set_bits(i2c_dev->base + STM32F4_I2C_CR2, mask);
>> +
>> + if (is_first) {
>> + ret = stm32f4_i2c_wait_free_bus(i2c_dev);
>> + if (ret)
>> + return ret;
>> +
>> + /* START generation */
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> + }
>> +
>> + timeout = wait_for_completion_timeout(&i2c_dev->complete,
>> + i2c_dev->adap.timeout);
>> + ret = f4_msg->result;
>> +
>> + /* Disable PEC position Ack */
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_POS);
>
> This is the only place mentioning PEC. Should this be about POS instead?
Yes you are right. Thanks
>
>> +
>> + if (!timeout)
>> + ret = -ETIMEDOUT;
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_xfer() - Transfer combined I2C message
>> + * @i2c_adap: Adapter pointer to the controller
>> + * @msgs: Pointer to data to be written.
>> + * @num: Number of messages to be executed
>> + */
>> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
>> + int num)
>> +{
>> + struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>> + int ret, i;
>> +
>> + ret = clk_enable(i2c_dev->clk);
>> + if (ret) {
>> + dev_err(i2c_dev->dev, "Failed to enable clock\n");
>> + return ret;
>> + }
>> +
>> + stm32f4_i2c_hw_config(i2c_dev);
>> +
>> + for (i = 0; i < num && !ret; i++)
>> + ret = stm32f4_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0,
>> + i == num - 1);
>> +
>> + clk_disable(i2c_dev->clk);
>> +
>> + return (ret < 0) ? ret : num;
>> +}
>> +
>> +static u32 stm32f4_i2c_func(struct i2c_adapter *adap)
>> +{
>> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>> +}
>> +
>> +static struct i2c_algorithm stm32f4_i2c_algo = {
>> + .master_xfer = stm32f4_i2c_xfer,
>> + .functionality = stm32f4_i2c_func,
>> +};
>> +
>> +static int stm32f4_i2c_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct stm32f4_i2c_dev *i2c_dev;
>> + struct resource *res;
>> + u32 irq_event, irq_error, clk_rate;
>> + struct i2c_adapter *adap;
>> + struct reset_control *rst;
>> + int ret;
>> +
>> + i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
>> + if (!i2c_dev)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + i2c_dev->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(i2c_dev->base))
>> + return PTR_ERR(i2c_dev->base);
>> +
>> + irq_event = irq_of_parse_and_map(np, 0);
>> + if (!irq_event) {
>> + dev_err(&pdev->dev, "IRQ event missing or invalid\n");
>> + return -EINVAL;
>> + }
>> +
>> + irq_error = irq_of_parse_and_map(np, 1);
>> + if (!irq_error) {
>> + dev_err(&pdev->dev, "IRQ error missing or invalid\n");
>> + return -EINVAL;
>> + }
>> +
>> + i2c_dev->clk = devm_clk_get(&pdev->dev, NULL);
>> + if (IS_ERR(i2c_dev->clk)) {
>> + dev_err(&pdev->dev, "Error: Missing controller clock\n");
>> + return PTR_ERR(i2c_dev->clk);
>> + }
>> + ret = clk_prepare(i2c_dev->clk);
>> + if (ret) {
>> + dev_err(i2c_dev->dev, "Failed to prepare clock\n");
>> + return ret;
>> + }
>> +
>> + rst = devm_reset_control_get(&pdev->dev, NULL);
>> + if (IS_ERR(rst)) {
>> + dev_err(&pdev->dev, "Error: Missing controller reset\n");
>> + ret = PTR_ERR(rst);
>> + goto clk_free;
>> + }
>> + reset_control_assert(rst);
>> + udelay(2);
>> + reset_control_deassert(rst);
>> +
>> + i2c_dev->speed = STM32F4_I2C_SPEED_STANDARD;
>> + ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
>> + if (!ret && clk_rate >= 40000)
>> + i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
>> +
>> + i2c_dev->dev = &pdev->dev;
>> +
>> + ret = devm_request_irq(&pdev->dev, irq_event, stm32f4_i2c_isr_event, 0,
>> + pdev->name, i2c_dev);
>
> Starting here this irq might trigger. Can this happen? If so,
> stm32f4_i2c_isr_event is called without the adapter being registered.
> Probably not an issue as the controller was just reset.
No irq could be triggered as after resett, the I2C is not enable as PE bit = 0.
>
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to request irq event %i\n",
>> + irq_event);
>> + goto clk_free;
>> + }
>> +
>> + ret = devm_request_irq(&pdev->dev, irq_error, stm32f4_i2c_isr_error, 0,
>> + pdev->name, i2c_dev);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to request irq error %i\n",
>> + irq_error);
>> + goto clk_free;
>> + }
>> +
>> + adap = &i2c_dev->adap;
>> + i2c_set_adapdata(adap, i2c_dev);
>> + snprintf(adap->name, sizeof(adap->name), "STM32 I2C(%pa)", &res->start);
>> + adap->owner = THIS_MODULE;
>> + adap->timeout = 2 * HZ;
>> + adap->retries = 0;
>> + adap->algo = &stm32f4_i2c_algo;
>> + adap->dev.parent = &pdev->dev;
>> + adap->dev.of_node = pdev->dev.of_node;
>> +
>> + init_completion(&i2c_dev->complete);
>> +
>> + ret = i2c_add_adapter(adap);
>> + if (ret)
>> + goto clk_free;
>> +
>> + platform_set_drvdata(pdev, i2c_dev);
>> +
>> + dev_info(i2c_dev->dev, "STM32F4 I2C driver registered\n");
>> +
>> + return 0;
>> +
>> +clk_free:
>> + clk_unprepare(i2c_dev->clk);
>> + return ret;
>> +}
>> +
>> +static int stm32f4_i2c_remove(struct platform_device *pdev)
>> +{
>> + struct stm32f4_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
>> +
>> + i2c_del_adapter(&i2c_dev->adap);
>> +
>> + clk_unprepare(i2c_dev->clk);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id stm32f4_i2c_match[] = {
>> + { .compatible = "st,stm32f4-i2c", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32f4_i2c_match);
>> +
>> +static struct platform_driver stm32f4_i2c_driver = {
>> + .driver = {
>> + .name = "stm32f4-i2c",
>> + .of_match_table = stm32f4_i2c_match,
>> + },
>> + .probe = stm32f4_i2c_probe,
>> + .remove = stm32f4_i2c_remove,
>> +};
>> +
>> +module_platform_driver(stm32f4_i2c_driver);
>> +
>> +MODULE_AUTHOR("M'boumba Cedric Madianga <cedric.madianga@gmail.com>");
>> +MODULE_DESCRIPTION("STMicroelectronics STM32F4 I2C driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [v1] i2c: busses: i2c-designware-pcidrv:- Unmap region obtained by pcim_iomap_regions
From: Andy Shevchenko @ 2016-12-23 11:55 UTC (permalink / raw)
To: Arvind Yadav, jarkko.nikula, wsa, mika.westerberg; +Cc: linux-i2c, linux-kernel
In-Reply-To: <1482487893-17589-1-git-send-email-arvind.yadav.cs@gmail.com>
On Fri, 2016-12-23 at 15:41 +0530, Arvind Yadav wrote:
> Release IO memory mapping, if i2c_dw_pci_probe is not successful.
>
Second NAK, and seems (due to kbot message) you didn't even bother to
compile test.
What are you doing?
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
> drivers/i2c/busses/i2c-designware-pcidrv.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
> b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index d6423cf..75e6e27 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -228,8 +228,10 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
> }
>
> dev = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev),
> GFP_KERNEL);
> - if (!dev)
> - return -ENOMEM;
> + if (!dev) {
> + r = -ENOMEM;
> + goto error;
> + }
>
> dev->clk = NULL;
> dev->controller = controller;
> @@ -241,7 +243,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
> if (controller->setup) {
> r = controller->setup(pdev, controller);
> if (r)
> - return r;
> + goto error;
> }
>
> dev->functionality = controller->functionality |
> @@ -270,7 +272,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
>
> r = i2c_dw_probe(dev);
> if (r)
> - return r;
> + goto error;
>
> pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> pm_runtime_use_autosuspend(&pdev->dev);
> @@ -278,6 +280,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
> pm_runtime_allow(&pdev->dev);
>
> return 0;
> +error:
> + pcim_iounmap_regions(pdev, 1 << 0);
> }
>
> static void i2c_dw_pci_remove(struct pci_dev *pdev)
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [v1] i2c: busses: i2c-designware-pcidrv:- Unmap region obtained by pcim_iomap_regions
From: Jarkko Nikula @ 2016-12-23 11:27 UTC (permalink / raw)
To: Mika Westerberg, Arvind Yadav
Cc: wsa, andriy.shevchenko, linux-i2c, linux-kernel
In-Reply-To: <20161223104449.GV1460@lahna.fi.intel.com>
On 12/23/2016 12:44 PM, Mika Westerberg wrote:
> On Fri, Dec 23, 2016 at 03:41:33PM +0530, Arvind Yadav wrote:
>> Release IO memory mapping, if i2c_dw_pci_probe is not successful.
>
> Point of pcim_iomap_regions() is that the regions are released
> automatically. So there is no need for explicit release.
>
Please see commit 76cf3fc844a4 ("i2c-designware-pci: use managed
functions pcim_* and devm_*") how it simplified error handling here.
--
Jarkko
^ permalink raw reply
* Re: [v1] i2c: busses: i2c-designware-pcidrv:- Unmap region obtained by pcim_iomap_regions
From: kbuild test robot @ 2016-12-23 11:15 UTC (permalink / raw)
To: Arvind Yadav
Cc: kbuild-all, jarkko.nikula, wsa, mika.westerberg,
andriy.shevchenko, linux-i2c, linux-kernel
In-Reply-To: <1482487893-17589-1-git-send-email-arvind.yadav.cs@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3016 bytes --]
Hi Arvind,
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v4.9 next-20161223]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Arvind-Yadav/i2c-busses-i2c-designware-pcidrv-Unmap-region-obtained-by-pcim_iomap_regions/20161223-185314
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: x86_64-randconfig-x007-201651 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
drivers/i2c/busses/i2c-designware-pcidrv.c: In function 'i2c_dw_pci_probe':
>> drivers/i2c/busses/i2c-designware-pcidrv.c:285:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
vim +285 drivers/i2c/busses/i2c-designware-pcidrv.c
fe20ff5c Dirk Brandewie 2011-10-06 269 adap->class = 0;
8eb5c87a Dustin Byford 2015-10-23 270 ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
fe20ff5c Dirk Brandewie 2011-10-06 271 adap->nr = controller->bus_num;
089c729a Mika Westerberg 2014-02-19 272
d80d1341 Jarkko Nikula 2015-10-12 273 r = i2c_dw_probe(dev);
d80d1341 Jarkko Nikula 2015-10-12 274 if (r)
5c99d5d6 Arvind Yadav 2016-12-23 275 goto error;
fe20ff5c Dirk Brandewie 2011-10-06 276
43452335 Mika Westerberg 2013-04-10 277 pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
43452335 Mika Westerberg 2013-04-10 278 pm_runtime_use_autosuspend(&pdev->dev);
be58eda7 Mika Westerberg 2014-02-04 279 pm_runtime_put_autosuspend(&pdev->dev);
18dbdda8 Dirk Brandewie 2011-10-06 280 pm_runtime_allow(&pdev->dev);
18dbdda8 Dirk Brandewie 2011-10-06 281
fe20ff5c Dirk Brandewie 2011-10-06 282 return 0;
5c99d5d6 Arvind Yadav 2016-12-23 283 error:
5c99d5d6 Arvind Yadav 2016-12-23 284 pcim_iounmap_regions(pdev, 1 << 0);
fe20ff5c Dirk Brandewie 2011-10-06 @285 }
fe20ff5c Dirk Brandewie 2011-10-06 286
0b255e92 Bill Pemberton 2012-11-27 287 static void i2c_dw_pci_remove(struct pci_dev *pdev)
fe20ff5c Dirk Brandewie 2011-10-06 288 {
fe20ff5c Dirk Brandewie 2011-10-06 289 struct dw_i2c_dev *dev = pci_get_drvdata(pdev);
fe20ff5c Dirk Brandewie 2011-10-06 290
18dbdda8 Dirk Brandewie 2011-10-06 291 i2c_dw_disable(dev);
18dbdda8 Dirk Brandewie 2011-10-06 292 pm_runtime_forbid(&pdev->dev);
18dbdda8 Dirk Brandewie 2011-10-06 293 pm_runtime_get_noresume(&pdev->dev);
:::::: The code at line 285 was first introduced by commit
:::::: fe20ff5c7e9ca7f5369aacc7d7ca3efeda3b90fe i2c-designware: Add support for Designware core behind PCI devices.
:::::: TO: Dirk Brandewie <dirk.brandewie@gmail.com>
:::::: CC: Ben Dooks <ben-linux@fluff.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27138 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox