* [[RFC V3] 0/1] i2c: imx: add slave support
From: Joshua Frkuska @ 2016-12-14 6:01 UTC (permalink / raw)
To: linux-i2c
Cc: joshua_frkuska, wsa, syrchin, peda, vladimir_zapolskiy,
Jiada_Wang
This patch was tested on i.MX6q sabresd and sabreai development boards. It is a continuation of the work started in the following thread https://www.spinics.net/lists/linux-i2c/msg27340.html
Its purpose is to introduce i2c slave and multimaster capabilities to the imx i2c controller driver. Slave mode can be tested using the i2c test EEPROM i2c slave device available in tree. Multimaster can be tested by creating a bus with 1 or more slave devices with 2 masters. For the purpose of this test, a sabreai and sabresd board were hooked together to form a multimaster bus. In this configuration, slave and multimaster configurations can alternatively be stress tested.
Some points to note:
The patch considers when I2C_SLAVE support is not enabled but introduces unused data structs and members even in this case. This can still probably be cleaned up further but it is left this way to reduce the number of conditional code in the driver.
Furthermore the state machine introduced to handle the slave states does not handle the master mode behavior. This is also done in order to allow an evolutionary change to the driver. A future update can integrate the master entirely into the statemachine.
Any constructive comments would be greatly appreciated.
Thank you
Joshua Frkuska (1):
i2c: imx: add slave support
drivers/i2c/busses/i2c-imx.c | 724 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 694 insertions(+), 30 deletions(-)
--
2.5.5
^ permalink raw reply
* [[RFC V3] 1/1] i2c: imx: add slave support
From: Joshua Frkuska @ 2016-12-14 6:01 UTC (permalink / raw)
To: linux-i2c
Cc: joshua_frkuska, wsa, syrchin, peda, vladimir_zapolskiy,
Jiada_Wang
In-Reply-To: <1481695291-11444-1-git-send-email-joshua_frkuska@mentor.com>
Add I2C slave provider using the generic slave interface.
It also supports master transactions when the slave in the idle mode.
Signed-off-by: Maxim Syrchin <msyrchin@dev.rtsoft.ru>
Signed-off-by: Joshua Frkuska <joshua_frkuska@mentor.com>
---
drivers/i2c/busses/i2c-imx.c | 724 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 694 insertions(+), 30 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 47fc1f1..7b2aeb8 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -53,6 +53,7 @@
#include <linux/pm_runtime.h>
#include <linux/sched.h>
#include <linux/slab.h>
+#include <linux/kthread.h>
/* This will be the driver name the kernel reports */
#define DRIVER_NAME "imx-i2c"
@@ -60,6 +61,13 @@
/* Default value */
#define IMX_I2C_BIT_RATE 100000 /* 100kHz */
+/* Wait delays */
+#define STATE_MIN_WAIT_TIME 1 /* 1 jiffy */
+#define STATE_WAIT_TIME (HZ / 10)
+
+#define MAX_EVENTS (1<<4)
+#define EUNDEFINED 4000
+
/*
* Enable DMA if transfer byte size is bigger than this threshold.
* As the hardware request, it must bigger than 4 bytes.\
@@ -171,6 +179,30 @@ enum imx_i2c_type {
VF610_I2C,
};
+enum i2c_imx_state {
+ STATE_IDLE = 0,
+ STATE_SLAVE,
+ STATE_MASTER,
+ STATE_SP
+};
+
+enum i2c_imx_events {
+ EVT_AL = 0,
+ EVT_SI,
+ EVT_START,
+ EVT_STOP,
+ EVT_POLL,
+ EVT_INVALID,
+ EVT_ENTRY
+};
+
+struct evt_queue {
+ atomic_t count;
+ atomic_t ir;
+ atomic_t iw;
+ unsigned int evt[MAX_EVENTS];
+};
+
struct imx_i2c_hwdata {
enum imx_i2c_type devtype;
unsigned regshift;
@@ -193,6 +225,7 @@ struct imx_i2c_dma {
struct imx_i2c_struct {
struct i2c_adapter adapter;
+ struct i2c_client *slave;
struct clk *clk;
void __iomem *base;
wait_queue_head_t queue;
@@ -210,6 +243,18 @@ struct imx_i2c_struct {
struct pinctrl_state *pinctrl_pins_gpio;
struct imx_i2c_dma *dma;
+
+ unsigned int state;
+ struct evt_queue events;
+ atomic_t last_error;
+
+ int master_interrupt;
+ int start_retry_cnt;
+ int slave_poll_cnt;
+
+ struct task_struct *slave_task;
+ wait_queue_head_t state_queue;
+
};
static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
@@ -414,17 +459,29 @@ static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
{
unsigned long orig_jiffies = jiffies;
- unsigned int temp;
+ unsigned int temp, ctl;
+
dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
while (1) {
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+ ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+
+ /*
+ * Check for arbitration lost. Datasheet recommends to
+ * clean IAL bit in interrupt handler before any other
+ * action.
+ *
+ * We can detect if controller resets MSTA bit, because
+ * hardware is switched to slave mode if arbitration was
+ * lost.
+ */
- /* check for arbitration lost */
- if (temp & I2SR_IAL) {
- temp &= ~I2SR_IAL;
- imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
+ if (for_busy && !(ctl & I2CR_MSTA)) {
+ dev_dbg(&i2c_imx->adapter.dev,
+ "<%s> Lost arbitration (SR = %02x, CR = %02x)\n",
+ __func__, temp, ctl);
return -EAGAIN;
}
@@ -443,16 +500,501 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
return 0;
}
+#ifdef CONFIG_I2C_SLAVE
+
+static void i2c_imx_enable_i2c_controller(struct imx_i2c_struct *i2c_imx);
+static void set_state(struct imx_i2c_struct *i2c_imx, unsigned int new);
+static int i2c_imx_hw_start(struct imx_i2c_struct *i2c_imx);
+static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx);
+
+static inline int evt_find_next_idx(atomic_t *v)
+{
+ return atomic_inc_return(v) & (MAX_EVENTS - 1);
+}
+
+static unsigned int evt_put(struct evt_queue *stk, unsigned int evt)
+{
+ int count = atomic_inc_return(&stk->count);
+ int idx;
+
+ if (count < MAX_EVENTS) {
+ idx = evt_find_next_idx(&stk->iw);
+ stk->evt[idx] = evt;
+ } else {
+ atomic_dec(&stk->count);
+ return EVT_INVALID;
+ }
+
+ return 0;
+}
+
+static unsigned int evt_get(struct evt_queue *stk)
+{
+ int count = atomic_dec_return(&stk->count);
+ int idx;
+
+ if (count >= 0) {
+ idx = evt_find_next_idx(&stk->ir);
+ } else {
+ atomic_inc(&stk->count);
+ return EVT_INVALID;
+ }
+
+ return stk->evt[idx];
+}
+
+static unsigned int evt_is_empty(struct evt_queue *stk)
+{
+ int ret = atomic_read(&stk->count);
+
+ return (ret <= 0);
+}
+
+static void evt_init(struct evt_queue *stk)
+{
+ atomic_set(&stk->count, 0);
+ atomic_set(&stk->iw, 0);
+ atomic_set(&stk->ir, 0);
+}
+
+
+static void i2c_imx_clear_ial_bit(struct imx_i2c_struct *i2c_imx)
+{
+ unsigned int status;
+
+ status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+ status &= ~I2SR_IAL;
+ imx_i2c_write_reg(status, i2c_imx, IMX_I2C_I2SR);
+}
+
+static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
+{
+ unsigned int temp;
+
+ dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
+
+ i2c_imx_enable_i2c_controller(i2c_imx);
+
+ /* Set Slave mode with interrupt enable */
+ temp = i2c_imx->hwdata->i2cr_ien_opcode | I2CR_IIEN;
+ imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+}
+
+
+static void i2c_imx_slave_process_irq(struct imx_i2c_struct *i2c_imx)
+{
+ unsigned int status, ctl;
+ u8 data;
+
+ status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+ ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+
+ if (status & I2SR_IAAS) {
+ if (status & I2SR_SRW) {
+ /* master wants to read from us */
+ i2c_slave_event(i2c_imx->slave,
+ I2C_SLAVE_READ_REQUESTED, &data);
+ ctl |= I2CR_MTX;
+ imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
+
+ /*send data */
+ imx_i2c_write_reg(data, i2c_imx, IMX_I2C_I2DR);
+ } else {
+ dev_dbg(&i2c_imx->adapter.dev, "write requested");
+ i2c_slave_event(i2c_imx->slave,
+ I2C_SLAVE_WRITE_REQUESTED, &data);
+ /*slave receive */
+ ctl &= ~I2CR_MTX;
+ imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
+
+ /*dummy read */
+ data = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+ }
+ } else {
+ /* slave send */
+ if (ctl & I2CR_MTX) {
+ if (!(status & I2SR_RXAK)) { /*ACK received */
+ i2c_slave_event(i2c_imx->slave,
+ I2C_SLAVE_READ_PROCESSED, &data);
+ ctl |= I2CR_MTX;
+ imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
+ /*send data */
+ imx_i2c_write_reg(data, i2c_imx, IMX_I2C_I2DR);
+ } else {
+ /*no ACK. */
+ /*dummy read */
+ dev_dbg(&i2c_imx->adapter.dev, "read requested");
+ i2c_slave_event(i2c_imx->slave,
+ I2C_SLAVE_READ_REQUESTED, &data);
+
+ ctl &= ~I2CR_MTX;
+ imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
+ imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+ }
+ } else { /*read */
+ ctl &= ~I2CR_MTX;
+ imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
+
+ /*read */
+ data = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+ dev_dbg(&i2c_imx->adapter.dev, "received %x",
+ (unsigned int) data);
+ i2c_slave_event(i2c_imx->slave,
+ I2C_SLAVE_WRITE_RECEIVED, &data);
+ }
+ }
+}
+
+
+static int idle_evt_handler(struct imx_i2c_struct *i2c_imx, unsigned int event)
+{
+ u8 reg;
+
+ switch (event) {
+ case EVT_ENTRY:
+ imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
+ i2c_imx, IMX_I2C_I2CR);
+ i2c_imx_enable_i2c_controller(i2c_imx);
+ imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode | I2CR_IIEN,
+ i2c_imx, IMX_I2C_I2CR);
+ if (atomic_read(&i2c_imx->last_error) == -EUNDEFINED) {
+ dev_dbg(&i2c_imx->adapter.dev, "Reset lost START event\n");
+ atomic_set(&i2c_imx->last_error, -EBUSY);
+ }
+ i2c_imx->start_retry_cnt = 0;
+ return 0;
+ case EVT_AL:
+ i2c_imx_clear_ial_bit(i2c_imx);
+ break;
+ case EVT_SI:
+ set_state(i2c_imx, STATE_SLAVE);
+ i2c_imx_slave_process_irq(i2c_imx);
+ break;
+ case EVT_START:
+ reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+ if ((reg & I2SR_IBB) != 0) {
+ atomic_set(&i2c_imx->last_error, -EBUSY);
+ break;
+ }
+
+ reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+ reg |= I2CR_MSTA;
+ imx_i2c_write_reg(reg, i2c_imx, IMX_I2C_I2CR);
+ set_state(i2c_imx, STATE_SP);
+ i2c_imx->start_retry_cnt = 0;
+ return 0;
+ case EVT_STOP:
+ case EVT_POLL:
+ default:
+ break;
+ }
+
+ return STATE_WAIT_TIME;
+}
+
+static int master_evt_handler(struct imx_i2c_struct *i2c_imx,
+ unsigned int event)
+{
+ switch (event) {
+ case EVT_ENTRY:
+ i2c_imx->start_retry_cnt = 0;
+ return 0;
+ case EVT_AL:
+ set_state(i2c_imx, STATE_IDLE);
+ break;
+ case EVT_SI:
+ break;
+ case EVT_START:
+ atomic_set(&i2c_imx->last_error, -EBUSY);
+ break;
+ case EVT_STOP:
+ imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx,
+ IMX_I2C_I2SR);
+ imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode | I2CR_IIEN,
+ i2c_imx, IMX_I2C_I2CR);
+
+ i2c_imx->stopped = 1;
+ udelay(50);
+ set_state(i2c_imx, STATE_IDLE);
+ return 0;
+ case EVT_POLL:
+ default:
+ break;
+ }
+
+ return STATE_WAIT_TIME;
+}
+
+static int slave_evt_handler (struct imx_i2c_struct *i2c_imx,
+ unsigned int event)
+{
+ u8 reg, data;
+
+ switch (event) {
+ case EVT_ENTRY:
+ if (atomic_read(&i2c_imx->last_error) == -EUNDEFINED) {
+ dev_dbg(&i2c_imx->adapter.dev, "Reset lost START event\n");
+ atomic_set(&i2c_imx->last_error, -EBUSY);
+ }
+ i2c_imx->start_retry_cnt = 0;
+ i2c_imx->slave_poll_cnt = 0;
+ return 0;
+ case EVT_AL:
+ set_state(i2c_imx, STATE_IDLE);
+ break;
+ case EVT_START:
+ reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+ atomic_set(&i2c_imx->last_error, -EBUSY);
+ break;
+ case EVT_STOP:
+ break;
+ case EVT_SI:
+ i2c_imx_slave_process_irq(i2c_imx);
+ reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+ if ((reg & I2SR_IBB) == 0) {
+ data = 0;
+ set_state(i2c_imx, STATE_IDLE);
+ dev_dbg(&i2c_imx->adapter.dev, "end of package");
+ i2c_slave_event(i2c_imx->slave, I2C_SLAVE_STOP, &data);
+ }
+ if (i2c_imx->slave_poll_cnt > 10) {
+ dev_err(&i2c_imx->adapter.dev,
+ "Too much slave loops (%i)\n",
+ i2c_imx->slave_poll_cnt);
+ }
+
+ i2c_imx->slave_poll_cnt = 0;
+ break;
+ case EVT_POLL:
+ reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+ if ((reg & I2SR_IBB) == 0) {
+ data = 0;
+ set_state(i2c_imx, STATE_IDLE);
+ dev_dbg(&i2c_imx->adapter.dev, "end of package");
+ i2c_slave_event(i2c_imx->slave, I2C_SLAVE_STOP, &data);
+ if (i2c_imx->slave_poll_cnt > 10) {
+ dev_err(&i2c_imx->adapter.dev,
+ "Too much slave loops (%i)\n",
+ i2c_imx->slave_poll_cnt);
+ }
+
+ i2c_imx->slave_poll_cnt = 0;
+ }
+
+ /*
+ * TODO: do "dummy read" if no interrupts or stop condition
+ * for more then 10 wait loops
+ */
+ i2c_imx->slave_poll_cnt += 1;
+ if (i2c_imx->slave_poll_cnt % 10 == 0)
+ imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+ break;
+ default:
+ break;
+ }
+
+ return STATE_MIN_WAIT_TIME;
+}
+
+static int sp_evt_handler (struct imx_i2c_struct *i2c_imx, unsigned int event)
+{
+ u8 reg;
+
+ switch (event) {
+ case EVT_AL:
+ dev_dbg(&i2c_imx->adapter.dev, "Lost arbitration on START");
+ atomic_set(&i2c_imx->last_error, -EAGAIN);
+ set_state(i2c_imx, STATE_IDLE);
+ return 0;
+ case EVT_SI:
+ set_state(i2c_imx, STATE_IDLE);
+ evt_put(&i2c_imx->events, EVT_SI);
+ case EVT_START:
+ atomic_set(&i2c_imx->last_error, -EBUSY);
+ break;
+ case EVT_STOP:
+ break;
+ case EVT_ENTRY:
+ return 0;
+ case EVT_POLL:
+ reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+ if ((reg & I2SR_IBB) && !(reg & I2SR_IAL)) {
+
+ set_state(i2c_imx, STATE_MASTER);
+ reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+
+ i2c_imx->stopped = 0;
+ reg |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
+ reg &= ~I2CR_DMAEN;
+ imx_i2c_write_reg(reg, i2c_imx, IMX_I2C_I2CR);
+ atomic_set(&i2c_imx->last_error, 0);
+ i2c_imx->start_retry_cnt = 0;
+ return 0;
+ }
+ break;
+ default:
+ break;
+
+ }
+
+ if (i2c_imx->start_retry_cnt++ < 100) {
+ dev_dbg(&i2c_imx->adapter.dev,
+ "wait for busy cnt = %i evt = %i",
+ i2c_imx->start_retry_cnt, event);
+ } else {
+
+ dev_dbg(&i2c_imx->adapter.dev, "start timeout");
+ i2c_imx->start_retry_cnt = 0;
+ atomic_set(&i2c_imx->last_error, -ETIMEDOUT);
+ set_state(i2c_imx, STATE_IDLE);
+ return STATE_WAIT_TIME;
+ }
+
+ return STATE_MIN_WAIT_TIME;
+}
+
+static void set_state(struct imx_i2c_struct *i2c_imx, unsigned int new)
+{
+ i2c_imx->state = new;
+
+ switch (new) {
+ case STATE_IDLE:
+ idle_evt_handler(i2c_imx, EVT_ENTRY);
+ break;
+ case STATE_SLAVE:
+ slave_evt_handler(i2c_imx, EVT_ENTRY);
+ break;
+ case STATE_SP:
+ sp_evt_handler(i2c_imx, EVT_ENTRY);
+ break;
+ case STATE_MASTER:
+ master_evt_handler(i2c_imx, EVT_ENTRY);
+ break;
+ }
+}
+
+
+static int i2c_imx_slave_threadfn(void *pdata)
+{
+ unsigned int event, delay;
+ struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *) pdata;
+
+ do {
+
+ event = evt_get(&i2c_imx->events);
+ if (event == EVT_INVALID)
+ event = EVT_POLL;
+
+ switch (i2c_imx->state) {
+ case STATE_IDLE:
+ delay = idle_evt_handler(i2c_imx, event);
+ break;
+ case STATE_SLAVE:
+ delay = slave_evt_handler(i2c_imx, event);
+ break;
+ case STATE_SP:
+ delay = sp_evt_handler(i2c_imx, event);
+ break;
+ case STATE_MASTER:
+ delay = master_evt_handler(i2c_imx, event);
+ break;
+ default:
+ delay = 0;
+ break;
+ }
+
+ if (delay)
+ wait_event_interruptible_timeout(i2c_imx->state_queue,
+ (evt_is_empty(&i2c_imx->events) == 0),
+ delay);
+
+ } while (kthread_should_stop() == 0);
+
+ return 0;
+}
+
+static int i2c_imx_reg_slave(struct i2c_client *slave)
+{
+ struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(slave->adapter);
+ int result;
+ struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
+
+ dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
+
+ if (i2c_imx->slave)
+ return -EBUSY;
+
+ if (slave->flags & I2C_CLIENT_TEN)
+ return -EAFNOSUPPORT;
+
+ i2c_imx->slave = slave;
+
+ /* Set the Slave address */
+ imx_i2c_write_reg((i2c_imx->slave->addr << 1), i2c_imx, IMX_I2C_IADR);
+
+ result = i2c_imx_hw_start(i2c_imx);
+ if (result != 0)
+ return result;
+
+ i2c_imx->slave_task = kthread_run(i2c_imx_slave_threadfn,
+ (void *)i2c_imx, "i2c-slave-%s", i2c_imx->adapter.name);
+
+ sched_setscheduler(i2c_imx->slave_task, SCHED_FIFO, ¶m);
+
+ if (IS_ERR(i2c_imx->slave_task))
+ return PTR_ERR(i2c_imx->slave_task);
+
+ i2c_imx_slave_init(i2c_imx);
+
+ return 0;
+
+}
+
+static int i2c_imx_unreg_slave(struct i2c_client *slave)
+{
+ struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(slave->adapter);
+
+ dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
+ if (i2c_imx->slave_task != NULL)
+ kthread_stop(i2c_imx->slave_task);
+
+ wake_up(&i2c_imx->state_queue);
+
+ i2c_imx->slave_task = NULL;
+
+ i2c_imx->slave = NULL;
+
+ i2c_imx_stop(i2c_imx);
+
+ return 0;
+}
+
+
+#else
+
+static void evt_init(struct evt_queue *stk)
+{
+ return;
+}
+
+static unsigned int evt_put(struct evt_queue *stk, unsigned int evt)
+{
+ return 0;
+}
+
+#endif
+
static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
{
- wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF, HZ / 10);
+ wait_event_timeout(i2c_imx->queue, i2c_imx->master_interrupt,
+ STATE_WAIT_TIME);
- if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
+ if (unlikely(!(i2c_imx->master_interrupt))) {
dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
return -ETIMEDOUT;
}
dev_dbg(&i2c_imx->adapter.dev, "<%s> TRX complete\n", __func__);
- i2c_imx->i2csr = 0;
+ i2c_imx->master_interrupt = 0;
return 0;
}
@@ -510,27 +1052,92 @@ static void i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx)
#endif
}
+static int i2c_imx_configure_clock(struct imx_i2c_struct *i2c_imx)
+{
+ int result;
+
+ i2c_imx_set_clk(i2c_imx);
+
+ result = clk_prepare_enable(i2c_imx->clk);
+ if (result == 0)
+ imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR);
+
+ return result;
+}
+
+static void i2c_imx_enable_i2c_controller(struct imx_i2c_struct *i2c_imx)
+{
+ imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx,
+ IMX_I2C_I2SR);
+ imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode, i2c_imx,
+ IMX_I2C_I2CR);
+
+ /* Wait controller to be stable */
+ udelay(50);
+}
+
+static int i2c_imx_hw_start(struct imx_i2c_struct *i2c_imx)
+{
+ int result;
+
+ dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
+
+ result = i2c_imx_configure_clock(i2c_imx);
+ if (result != 0)
+ return result;
+ i2c_imx_enable_i2c_controller(i2c_imx);
+ return 0;
+}
+
+
static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
{
unsigned int temp = 0;
- int result;
+ int result, cnt = 0;
dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
- i2c_imx_set_clk(i2c_imx);
+ if (i2c_imx->slave_task != NULL) {
- imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR);
- /* Enable I2C controller */
- imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, IMX_I2C_I2SR);
- imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode, i2c_imx, IMX_I2C_I2CR);
+ atomic_set(&i2c_imx->last_error, -EUNDEFINED);
+ if (evt_put(&i2c_imx->events, EVT_START) != 0) {
+ dev_err(&i2c_imx->adapter.dev,
+ "Event buffer overflow\n");
+ return -EBUSY;
+ }
- /* Wait controller to be stable */
- usleep_range(50, 150);
+ wake_up(&i2c_imx->state_queue);
+
+ while ((result = atomic_read(&i2c_imx->last_error)) == -EUNDEFINED) {
+ schedule();
+
+ /* TODO: debug workaround - start hung monitoring */
+ cnt++;
+ if (cnt == 500000) {
+ dev_err(&i2c_imx->adapter.dev,
+ "Too many start loops\n");
+ imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
+ i2c_imx, IMX_I2C_I2CR);
+ i2c_imx_enable_i2c_controller(i2c_imx);
+ imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode | I2CR_IIEN,
+ i2c_imx, IMX_I2C_I2CR);
+
+ return -ETIMEDOUT;
+ }
+
+ };
+ return result;
+ }
+
+ result = i2c_imx_hw_start(i2c_imx);
+ if (result != 0)
+ return result;
/* Start I2C transaction */
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
temp |= I2CR_MSTA;
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
result = i2c_imx_bus_busy(i2c_imx, 1);
if (result)
return result;
@@ -542,10 +1149,9 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
return result;
}
-static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
+static void i2c_imx_hw_stop(struct imx_i2c_struct *i2c_imx)
{
unsigned int temp = 0;
-
if (!i2c_imx->stopped) {
/* Stop I2C transaction */
dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
@@ -555,6 +1161,7 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
temp &= ~I2CR_DMAEN;
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
}
+
if (is_imx1_i2c(i2c_imx)) {
/*
* This delay caused by an i.MXL hardware bug.
@@ -568,24 +1175,58 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
i2c_imx->stopped = 1;
}
- /* Disable I2C controller */
- temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
+ temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN;
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+ clk_disable_unprepare(i2c_imx->clk);
+
+}
+
+static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
+{
+ if (i2c_imx->slave == NULL) {
+ i2c_imx_hw_stop(i2c_imx);
+ } else {
+
+ dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
+
+ evt_put(&i2c_imx->events, EVT_STOP);
+ wake_up(&i2c_imx->state_queue);
+ }
+}
+
+static void i2c_imx_clear_isr_bit(struct imx_i2c_struct *i2c_imx,
+ unsigned int status)
+{
+ status &= ~I2SR_IIF;
+ status |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
+ imx_i2c_write_reg(status, i2c_imx, IMX_I2C_I2SR);
}
+
+
static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
{
struct imx_i2c_struct *i2c_imx = dev_id;
- unsigned int temp;
+ unsigned int status, ctl;
+
+ status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+ ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+ if (status & I2SR_IIF) {
+ i2c_imx_clear_isr_bit(i2c_imx, status);
+
+ if (ctl & I2CR_MSTA) {
+ dev_dbg(&i2c_imx->adapter.dev, "master interrupt");
+ i2c_imx->master_interrupt = 1;
+ wake_up(&i2c_imx->queue);
+ } else if (status & I2SR_IAL) {
+ evt_put(&i2c_imx->events, EVT_AL);
+ } else {
+ dev_dbg(&i2c_imx->adapter.dev, "slave interrupt");
+ evt_put(&i2c_imx->events, EVT_SI);
+ wake_up(&i2c_imx->state_queue);
+ }
- temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
- if (temp & I2SR_IIF) {
- /* save status register */
- i2c_imx->i2csr = temp;
- temp &= ~I2SR_IIF;
- temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
- imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
- wake_up(&i2c_imx->queue);
return IRQ_HANDLED;
}
@@ -895,7 +1536,13 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
/* Start I2C transfer */
result = i2c_imx_start(i2c_imx);
- if (result) {
+ if (result == -ETIMEDOUT) {
+ /*
+ * Recovery is not started on arbitration lost, since it can
+ * break slave transfer. But in case of "bus timeout" recovery
+ * it could be useful to bring controller out of "strange state".
+ */
+ dev_dbg(&i2c_imx->adapter.dev, "call bus recovery");
if (i2c_imx->adapter.bus_recovery_info) {
i2c_recover_bus(&i2c_imx->adapter);
result = i2c_imx_start(i2c_imx);
@@ -1040,6 +1687,10 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter)
static struct i2c_algorithm i2c_imx_algo = {
.master_xfer = i2c_imx_xfer,
.functionality = i2c_imx_func,
+#ifdef CONFIG_I2C_SLAVE
+ .reg_slave = i2c_imx_reg_slave,
+ .unreg_slave = i2c_imx_unreg_slave,
+#endif
};
static int i2c_imx_probe(struct platform_device *pdev)
@@ -1099,6 +1750,12 @@ static int i2c_imx_probe(struct platform_device *pdev)
return ret;
}
+ i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
+ if (IS_ERR(i2c_imx->pinctrl)) {
+ ret = PTR_ERR(i2c_imx->pinctrl);
+ goto clk_disable;
+ }
+
/* Request IRQ */
ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
pdev->name, i2c_imx);
@@ -1109,6 +1766,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
/* Init queue */
init_waitqueue_head(&i2c_imx->queue);
+ init_waitqueue_head(&i2c_imx->state_queue);
/* Set up adapter data */
i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
@@ -1160,6 +1818,9 @@ static int i2c_imx_probe(struct platform_device *pdev)
/* Init DMA config if supported */
i2c_imx_dma_request(i2c_imx, phy_addr);
+ /* init slave_state to IDLE */
+ i2c_imx->state = STATE_IDLE;
+ evt_init(&i2c_imx->events);
return 0; /* Return OK */
rpm_disable:
@@ -1186,6 +1847,9 @@ static int i2c_imx_remove(struct platform_device *pdev)
dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
i2c_del_adapter(&i2c_imx->adapter);
+ if (i2c_imx->slave_task != NULL)
+ kthread_stop(i2c_imx->slave_task);
+
if (i2c_imx->dma)
i2c_imx_dma_free(i2c_imx);
--
2.5.5
^ permalink raw reply related
* Re: [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
From: Tin Huynh @ 2016-12-14 3:20 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jarkko Nikula, Mika Westerberg, Wolfram Sang, linux-i2c,
linux-kernel, linux-acpi, Loc Ho, Thang Nguyen, Phong Vo, patches
In-Reply-To: <1481628337.7188.63.camel@linux.intel.com>
On Tue, Dec 13, 2016 at 6:25 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2016-12-13 at 17:03 +0700, Tin Huynh wrote:
>> ACPI always sets Tx/Rx FIFO to 32. This configuration will
>> cause problem if the IP core supports a FIFO size of less than 32.
>> The driver should read the FIFO size from the IP and select the
>> smaller
>> one of the two.
>>
>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
>>
>
> One comment below, after addressing it
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
>> ---
>> drivers/i2c/busses/i2c-designware-platdrv.c | 27
>> ++++++++++++++++++++-------
>> 1 files changed, 20 insertions(+), 7 deletions(-)
>>
>> Change from V3:
>> -Use uppercase of FIFO instead of lowercase.
>> -Fix the problem when IP core return 0 of FIFO.
>>
>> Change from V2:
>> -Add a helper function to set FIFO size.
>>
>> Change from V1:
>> -Revert the default 32 for FIFO, read parameter from IP core
>> and pick the smaller one of the two.
>> -Correct the title to describe new approach.
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 0b42a12..24032d6 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -150,6 +150,25 @@ static int i2c_dw_plat_prepare_clk(struct
>> dw_i2c_dev *i_dev, bool prepare)
>> return 0;
>> }
>>
>> +static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
>> +{
>> + u32 param, tx_fifo_depth, rx_fifo_depth;
>> +
>>
>
> /*
> * Try to detect the FIFO depth if not
> set by interface driver,
> * the depth could be from 2 to 256 from
> HW spec.
> */
>
I will add your comment to the driver.
>> + param = i2c_dw_read_comp_param(dev);
>> + tx_fifo_depth = ((param >> 16) & 0xff) + 1;
>> + rx_fifo_depth = ((param >> 8) & 0xff) + 1;
>> + if (!dev->tx_fifo_depth) {
>> + dev->tx_fifo_depth = tx_fifo_depth;
>> + dev->rx_fifo_depth = rx_fifo_depth;
>> + dev->adapter.nr = id;
>> + } else if (tx_fifo_depth > 1) {
>
> This makes sense now, though I would add a comment here and use >= 2 to
> reflect datasheet.
>
> /*
> * Choose minimum values between HW and interface
> * driver provided.
> */
>
I will implement as your comment. However , because adding 1 to the
value , can i use > 2 or >=3 ?
>> + dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
>> + tx_fifo_depth);
>> + dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
>> + rx_fifo_depth);
>> + }
>> +}
>> +
>> static int dw_i2c_plat_probe(struct platform_device *pdev)
>> {
>> struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev-
>> >dev);
>> @@ -246,13 +265,7 @@ static int dw_i2c_plat_probe(struct
>> platform_device *pdev)
>> 1000000);
>> }
>>
>> - if (!dev->tx_fifo_depth) {
>> - u32 param1 = i2c_dw_read_comp_param(dev);
>> -
>> - dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
>> - dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1;
>> - dev->adapter.nr = pdev->id;
>> - }
>> + dw_i2c_set_fifo_size(dev, pdev->id);
>>
>> adap = &dev->adapter;
>> adap->owner = THIS_MODULE;
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
Thank you and regards,
Tin
^ permalink raw reply
* Re: [PATCH] i2c: designware: add reset interface
From: Wolfram Sang @ 2016-12-13 20:34 UTC (permalink / raw)
To: Zhangfei Gao
Cc: linux-arm-kernel, linux-i2c, Jarkko Nikula, Andy Shevchenko,
Mika Westerberg
In-Reply-To: <1479789700-19532-1-git-send-email-zhangfei.gao@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]
On Tue, Nov 22, 2016 at 12:41:40PM +0800, Zhangfei Gao wrote:
> Some platforms like hi3660 need do reset first to allow accessing registers
>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Adding designware maintainers to CC...
> ---
> drivers/i2c/busses/i2c-designware-core.h | 1 +
> drivers/i2c/busses/i2c-designware-platdrv.c | 5 +++++
> 2 files changed, 6 insertions(+)
>
> 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..fd80e58 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,10 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> dev->irq = irq;
> platform_set_drvdata(pdev, dev);
>
> + dev->rst = devm_reset_control_get(&pdev->dev, NULL);
> + if (!IS_ERR(dev->rst))
> + reset_control_reset(dev->rst);
> +
> /* fast mode by default because of legacy reasons */
> dev->clk_freq = 400000;
>
> --
> 2.7.4
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 3/4] i2c: octeon: thunderx: Limit register access retries
From: Wolfram Sang @ 2016-12-13 20:32 UTC (permalink / raw)
To: Jan Glauber
Cc: Wolfram Sang, Paul Burton, Steven J . Hill, linux-i2c, linux-mips,
David Daney
In-Reply-To: <20161212160711.GA2766@hardcore>
[-- Attachment #1: Type: text/plain, Size: 347 bytes --]
> >
> > I can't apply this one?
> >
>
> Strange. Applies for me on top of 4.9 and also next-20161212.
> I can also apply if back from the mail. Is the mail messed up on your
> side?
My fault, sorry for the noise. My branch missed the latest revert for
4.9. I'll apply this patch for a second pull request during the merge
window.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: designware: Cleaning and comment style fixes.
From: Joe Perches @ 2016-12-13 18:34 UTC (permalink / raw)
To: Luis Oliveira, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <32604e59cde19a8981681ad2f0ee447af847bf60.1481646098.git.lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
On Tue, 2016-12-13 at 16:34 +0000, Luis Oliveira wrote:
> - Misspelling of some words
> - Comment format fix
Most all adding periods to comments.
I think this is generally a value free change as these are
frequently sentence fragments, but, hey, it's not my code.
> /*
> - * Registers offset
> + * Registers offset.
> */
Like this.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Re: [PATCH] i2c: designware: Cleaning and comment style fixes.
From: Luis Oliveira @ 2016-12-13 18:05 UTC (permalink / raw)
To: Andy Shevchenko, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <1481648428.8991.4.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On 13-Dec-16 17:00, Andy Shevchenko wrote:
> On Tue, 2016-12-13 at 16:34 +0000, Luis Oliveira wrote:
>> - Misspelling of some words
>
> So, do you use codespell tool for that? I would suggest to use that tool
> and propose any fix there first. At least that tool has a good history
> of changes back in forward until some native speaker(s) settle some
> cases.
>
I just fixed the words "acknowledgment" and "endianness" because checkpatch.pl
says it should be fixed.
>> - Comment format fix
>
> No need to use dot at the end of one-liner comments
>
> /* One line */
>
> /*
> * Multi-line comments. Might include several lines or
> * even paragraphs.
> */
>
> Also I would leave
> /*
> * One line
> */
>
> Without dots here. AFAIU they are used to better distinguish group of
> definitions inside headers (that's why 3 lines).
>
Ok thank you, I will go redo that.
>> - Minor fix in coding style
>>
>
> I'm almost fine with the change itself, but people may be concerned in:
>
> 1) what is the value of the change (would be nice to put this in commit
> message);
>
The value of this, besides the Linux kernel rules of coding style, is because I
will use this code after and it will make my future patch a lot bigger and
complicated to review if I have to do it all together.
> 2) how much additional work this change may bring for backported fixes
> if any;
>
I don't think there is any.
> 3) native speakers are the best who can re-read this and Ack.
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] i2c: designware: Cleaning and comment style fixes.
From: Andy Shevchenko @ 2016-12-13 17:00 UTC (permalink / raw)
To: Luis Oliveira, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <32604e59cde19a8981681ad2f0ee447af847bf60.1481646098.git.lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
On Tue, 2016-12-13 at 16:34 +0000, Luis Oliveira wrote:
> - Misspelling of some words
So, do you use codespell tool for that? I would suggest to use that tool
and propose any fix there first. At least that tool has a good history
of changes back in forward until some native speaker(s) settle some
cases.
> - Comment format fix
No need to use dot at the end of one-liner comments
/* One line */
/*
* Multi-line comments. Might include several lines or
* even paragraphs.
*/
Also I would leave
/*
* One line
*/
Without dots here. AFAIU they are used to better distinguish group of
definitions inside headers (that's why 3 lines).
> - Minor fix in coding style
>
I'm almost fine with the change itself, but people may be concerned in:
1) what is the value of the change (would be nice to put this in commit
message);
2) how much additional work this change may bring for backported fixes
if any;
3) native speakers are the best who can re-read this and Ack.
--
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] i2c: designware: Cleaning and comment style fixes.
From: Luis Oliveira @ 2016-12-13 16:34 UTC (permalink / raw)
To: wsa-z923LK4zBo2bacvFa/9K2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Luis.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
- Misspelling of some words
- Comment format fix
- Minor fix in coding style
Signed-off-by: Luis Oliveira <lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
---
The purpose of this patch is to fix some comments and styling issues in the
existing code. What is being made here is:
- Sorted the headers files
- Corrected the commentary format
- Reverse tree style in the variables declaration as long as possible
- Add/remove empty lines if needed (style)
- Fix of two misspelled words
drivers/i2c/busses/i2c-designware-core.c | 106 ++++++++++++++--------------
drivers/i2c/busses/i2c-designware-core.h | 3 +-
drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++----
3 files changed, 67 insertions(+), 67 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 6d81c56184d3..565f4e761edd 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -21,19 +21,20 @@
* ----------------------------------------------------------------------------
*
*/
+#include <linux/delay.h>
#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 <linux/pm_runtime.h>
+
#include "i2c-designware-core.h"
/*
- * Registers offset
+ * Registers offset.
*/
#define DW_IC_CON 0x0
#define DW_IC_TAR 0x4
@@ -98,13 +99,13 @@
#define DW_IC_ERR_TX_ABRT 0x1
-#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
+#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
+ * Status codes.
*/
#define STATUS_IDLE 0x0
#define STATUS_WRITE_IN_PROGRESS 0x1
@@ -113,10 +114,10 @@
#define TIMEOUT 20 /* ms */
/*
- * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
+ * 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
+ * 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
@@ -158,7 +159,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] =
@@ -207,7 +208,7 @@ 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
+ * the tHD;STA timing spec. Configuring _HCNT based on tHIGH spec
* will result in violation of the tHD;STA spec.
*/
if (cond)
@@ -327,9 +328,9 @@ static void i2c_dw_release_lock(struct dw_i2c_dev *dev)
*/
int i2c_dw_init(struct dw_i2c_dev *dev)
{
- u32 hcnt, lcnt;
- u32 reg, comp_param1;
u32 sda_falling_time, scl_falling_time;
+ u32 reg, comp_param1;
+ u32 hcnt, lcnt;
int ret;
ret = i2c_dw_acquire_lock(dev);
@@ -338,29 +339,28 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
reg = dw_readl(dev, DW_IC_COMP_TYPE);
if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
- /* Configure register endianess access */
+ /* Configure register endianness access. */
dev->accessor_flags |= ACCESS_SWAP;
} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
- /* Configure register access mode 16bit */
+ /* 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);
+ 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 */
+ /* Disable the adapter. */
__i2c_dw_enable_and_wait(dev, false);
- /* set standard and fast speed deviders for high/low periods */
-
+ /* 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 */
+ /* Set SCL timing parameters for standard-mode. */
if (dev->ss_hcnt && dev->ss_lcnt) {
hcnt = dev->ss_hcnt;
lcnt = dev->ss_lcnt;
@@ -379,7 +379,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
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 */
+ /* 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;
@@ -418,11 +418,11 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
}
}
- /* Configure SDA Hold Time if required */
+ /* 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 */
+ /* Keep previous hold time setting if no one set it. */
dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
}
/*
@@ -440,11 +440,11 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
"Hardware too old to adjust SDA hold time.\n");
}
- /* Configure Tx/Rx FIFO threshold levels */
+ /* 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 */
+ /* Configure the I2C master. */
dw_writel(dev, dev->master_cfg , DW_IC_CON);
i2c_dw_release_lock(dev);
@@ -454,7 +454,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
EXPORT_SYMBOL_GPL(i2c_dw_init);
/*
- * Waiting for bus not busy
+ * Waiting for bus not busy.
*/
static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
{
@@ -477,10 +477,10 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
struct i2c_msg *msgs = dev->msgs;
u32 ic_tar = 0;
- /* Disable the adapter */
+ /* Disable the adapter. */
__i2c_dw_enable_and_wait(dev, false);
- /* if the slave address is ten bit address, enable 10BITADDR */
+ /* If the slave address is ten bit address, enable 10BITADDR. */
if (dev->dynamic_tar_update_enabled) {
/*
* If I2C_DYNAMIC_TAR_UPDATE is set, the 10-bit addressing
@@ -505,13 +505,13 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
*/
dw_writel(dev, msgs[dev->msg_write_idx].addr | ic_tar, DW_IC_TAR);
- /* enforce disabled interrupts (due to HW issues) */
+ /* Enforce disabled interrupts (due to HW issues). */
i2c_dw_disable_int(dev);
- /* Enable the adapter */
+ /* Enable the adapter. */
__i2c_dw_enable(dev, true);
- /* Clear and enable interrupts */
+ /* Clear and enable interrupts. */
dw_readl(dev, DW_IC_CLR_INTR);
dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
}
@@ -526,12 +526,12 @@ static void
i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
{
struct i2c_msg *msgs = dev->msgs;
- u32 intr_mask;
- int tx_limit, rx_limit;
u32 addr = msgs[dev->msg_write_idx].addr;
u32 buf_len = dev->tx_buf_len;
- u8 *buf = dev->tx_buf;
bool need_restart = false;
+ int tx_limit, rx_limit;
+ u8 *buf = dev->tx_buf;
+ u32 intr_mask;
intr_mask = DW_IC_INTR_DEFAULT_MASK;
@@ -539,9 +539,9 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
u32 flags = msgs[dev->msg_write_idx].flags;
/*
- * if target address has changed, we need to
- * reprogram the target address in the i2c
- * adapter when we are done with this transfer
+ * If target address has changed, we need to
+ * reprogram the target address in the I2C
+ * adapter when we are done with this transfer.
*/
if (msgs[dev->msg_write_idx].addr != addr) {
dev_err(dev->dev,
@@ -558,7 +558,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
}
if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
- /* new i2c_msg */
+ /* New i2c_msg */
buf = msgs[dev->msg_write_idx].buf;
buf_len = msgs[dev->msg_write_idx].len;
@@ -601,7 +601,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
- /* avoid rx buffer overrun */
+ /* Avoid rx buffer overrun. */
if (dev->rx_outstanding >= dev->rx_fifo_depth)
break;
@@ -622,7 +622,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
* the transaction here.
*/
if (buf_len > 0 || flags & I2C_M_RECV_LEN) {
- /* more bytes to be written */
+ /* More bytes to be written. */
dev->status |= STATUS_WRITE_IN_PROGRESS;
break;
} else
@@ -687,7 +687,7 @@ i2c_dw_read(struct dw_i2c_dev *dev)
u32 flags = msgs[dev->msg_read_idx].flags;
*buf = dw_readl(dev, DW_IC_DATA_CMD);
- /* Ensure length byte is a valid value */
+ /* Ensure length byte is a valid value. */
if (flags & I2C_M_RECV_LEN &&
*buf <= I2C_SMBUS_BLOCK_MAX && *buf > 0) {
len = i2c_dw_recv_len(dev, *buf);
@@ -724,13 +724,13 @@ static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
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 */
+ return -EINVAL; /* Wrong msgs[] data. */
else
return -EIO;
}
/*
- * Prepare controller for a transaction and call i2c_dw_xfer_msg
+ * Prepare controller for a transaction and call i2c_dw_xfer_msg.
*/
static int
i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
@@ -761,13 +761,13 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
if (ret < 0)
goto done;
- /* start the transfers */
+ /* Start the transfers. */
i2c_dw_xfer_init(dev);
- /* wait for tx to complete */
+ /* Wait for tx to complete. */
if (!wait_for_completion_timeout(&dev->cmd_complete, adap->timeout)) {
dev_err(dev->dev, "controller timed out\n");
- /* i2c_dw_init implicitly disables the adapter */
+ /* i2c_dw_init implicitly disables the adapter. */
i2c_dw_init(dev);
ret = -ETIMEDOUT;
goto done;
@@ -788,13 +788,13 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
goto done;
}
- /* no error */
+ /* No error. */
if (likely(!dev->cmd_err && !dev->status)) {
ret = num;
goto done;
}
- /* We have an error */
+ /* We have an error. */
if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
ret = i2c_dw_handle_tx_abort(dev);
goto done;
@@ -905,7 +905,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
/*
* Anytime TX_ABRT is set, the contents of the tx/rx
- * buffers are flushed. Make sure to skip them.
+ * buffers are flushed. Make sure to skip them.
*/
dw_writel(dev, 0, DW_IC_INTR_MASK);
goto tx_aborted;
@@ -927,7 +927,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err)
complete(&dev->cmd_complete);
else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
- /* workaround to trigger pending interrupt */
+ /* Workaround to trigger pending interrupt. */
stat = dw_readl(dev, DW_IC_INTR_MASK);
i2c_dw_disable_int(dev);
dw_writel(dev, stat, DW_IC_INTR_MASK);
@@ -938,10 +938,10 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
void i2c_dw_disable(struct dw_i2c_dev *dev)
{
- /* Disable controller */
+ /* Disable controller. */
__i2c_dw_enable_and_wait(dev, false);
- /* Disable all interupts */
+ /* Disable all interupts. */
dw_writel(dev, 0, DW_IC_INTR_MASK);
dw_readl(dev, DW_IC_CLR_INTR);
}
@@ -978,7 +978,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);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 26250b425e2f..3d13a9f0bb7c 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -36,11 +36,10 @@
#define DW_IC_CON_SPEED_FAST 0x4
#define DW_IC_CON_SPEED_HIGH 0x6
#define DW_IC_CON_SPEED_MASK 0x6
-#define DW_IC_CON_10BITADDR_MASTER 0x10
+#define DW_IC_CON_10BITADDR_MASTER 0x10
#define DW_IC_CON_RESTART_EN 0x20
#define DW_IC_CON_SLAVE_DISABLE 0x40
-
/**
* struct dw_i2c_dev - private i2c-designware data
* @dev: driver model device node
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 08153ea4d848..989ae0299cd0 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -21,26 +21,27 @@
* ----------------------------------------------------------------------------
*
*/
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/delay.h>
-#include <linux/dmi.h>
-#include <linux/i2c.h>
+#include <linux/acpi.h>
#include <linux/clk.h>
#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/dmi.h>
#include <linux/errno.h>
-#include <linux/sched.h>
#include <linux/err.h>
+#include <linux/i2c.h>
#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
#include <linux/of.h>
+#include <linux/platform_data/i2c-designware.h>
#include <linux/platform_device.h>
#include <linux/pm.h>
#include <linux/pm_runtime.h>
#include <linux/property.h>
-#include <linux/io.h>
+#include <linux/sched.h>
#include <linux/slab.h>
-#include <linux/acpi.h>
-#include <linux/platform_data/i2c-designware.h>
+
#include "i2c-designware-core.h"
static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
@@ -153,11 +154,11 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
static int dw_i2c_plat_probe(struct platform_device *pdev)
{
struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
- struct dw_i2c_dev *dev;
struct i2c_adapter *adap;
+ struct dw_i2c_dev *dev;
+ u32 acpi_speed, ht = 0;
struct resource *mem;
int irq, r;
- u32 acpi_speed, ht = 0;
irq = platform_get_irq(pdev, 0);
if (irq < 0)
@@ -354,7 +355,7 @@ static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
#define DW_I2C_DEV_PMOPS NULL
#endif
-/* work with hotplug and coldplug */
+/* Work with hotplug and coldplug. */
MODULE_ALIAS("platform:i2c_designware");
static struct platform_driver dw_i2c_driver = {
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v4 5/5] i2c: designware: Cleaning comments and formatation
From: Jarkko Nikula @ 2016-12-13 15:08 UTC (permalink / raw)
To: Luis Oliveira, wsa, robh+dt, mark.rutland, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <02856fdb6ce3230a4ac1ba0958938ad51e763205.1481131072.git.lolivei@synopsys.com>
On 12/07/2016 07:55 PM, Luis Oliveira wrote:
> - Missspelling, comment formatation and fix a string of
> the existing code
>
> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
> ---
> Changes V3->V4: (Andy Shevchenko)
> - created a commit message
>
> drivers/i2c/busses/i2c-designware-common.c | 2 +-
> drivers/i2c/busses/i2c-designware-slave.c | 10 ++++++----
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index 41b38d8b8732..838ef662d2c8 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -42,7 +42,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",
I'm not a native speaker but are both acknowledgement and acknowledgment
ok? I.e. is there need to change?
> diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
> index 1c7f82bb2513..442bc5ce6d47 100644
> --- a/drivers/i2c/busses/i2c-designware-slave.c
> +++ b/drivers/i2c/busses/i2c-designware-slave.c
> @@ -70,8 +70,8 @@ int i2c_dw_init_slave(struct dw_i2c_dev *dev)
> /* 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);
> + dev_err(dev->dev,
> + "Unknown Synopsys component type: 0x%08x\n", reg);
> i2c_dw_release_lock(dev);
> return -ENODEV;
> }
> @@ -181,8 +181,10 @@ int i2c_dw_reg_slave(struct i2c_client *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 */
> + /*
> + * set slave address in the IC_SAR register,
> + * the address to which the DW_apb_i2c responds
> + */
These two should be done already in the patch 4/5 since it introduced
the lines that you are changing here.
--
Jarkko
^ permalink raw reply
* Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
From: Rob Herring @ 2016-12-13 14:11 UTC (permalink / raw)
To: Luis de Oliveira
Cc: wsa@the-dreams.de, mark.rutland@arm.com,
jarkko.nikula@linux.intel.com, andriy.shevchenko@linux.intel.com,
mika.westerberg@linux.intel.com, linux-i2c@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Ramiro.Oliveira@synopsys.com, Joao.Pinto@synopsys.com,
CARLOS.PALMINHA@synopsys.com
In-Reply-To: <BCF0D4927F9C694C9AEA8827D4A9C7C897CEDB@de02wembxa.internal.synopsys.com>
Again, please don't top post. And your line wrapping is messed up.
IOW, you can't use Outlook.
On Tue, Dec 13, 2016 at 4:50 AM, Luis de Oliveira
<Luis.Oliveira@synopsys.com> wrote:
> The controller for i2c-designware cannot be slave/master at the same time and it has to be enabled knowing beforehand if we want it to be slave or master by something outside of the controller itself.
>
> I as looking and I see the use of this I2C_OWN_SLAVE_ADDRESS with the "linux,slave-24c02" slave interface but I am not seeing how it will help me identify a selected i2c-designware block as a "slave" device before instantiation. I'm sorry if I'm not understanding properly.
> I use the "linux,slave-24c02" to instantiate the i2c-designware as a slave with an address so I can do write/read operations, it is how I tested it.
Something like this:
of_for_each_child_node(child) {
of_property_read_u32(child, "reg", ®);
if (reg & I2C_OWN_SLAVE_ADDRESS))
im_a_slave = true;
}
...rather than testing "mode" is equal to "slave".
Rob
>
> Luis
>
> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Monday, December 12, 2016 23:16
> To: Luis de Oliveira <Luis.Oliveira@synopsys.com>
> Cc: wsa@the-dreams.de; mark.rutland@arm.com; jarkko.nikula@linux.intel.com; andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Ramiro.Oliveira@synopsys.com; Joao.Pinto@synopsys.com; CARLOS.PALMINHA@synopsys.com
> Subject: Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
>
> On Mon, Dec 12, 2016 at 12:35 PM, Luis de Oliveira <Luis.Oliveira@synopsys.com> wrote:
>> Hi all,
>
> Please don't top post.
>
>>
>> The slave address could be set by the I2C slave backend so I can't use it to setup the controller.
>> A boolean property was my initial approach then Andy and Wolfram Sang suggested the use of compatible strings and later It was suggested to use a property to select mode but I can do it again if it's the best way.
>> Can you please tell me where should it be documented?
>
> bindings/i2c/i2c.txt.
>
> Actually, looking at this some more, we already have a way to describe the controller being a slave device with the I2C_OWN_SLAVE_ADDRESS flag in the reg property. We should just need a helper to read reg property of each child and check for the bit set.
>
> Rob
^ permalink raw reply
* Re: [PATCH v4 5/5] i2c: designware-baytrail: Add support for cherrytrail
From: Jarkko Nikula @ 2016-12-13 13:45 UTC (permalink / raw)
To: Hans de Goede, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <20161212215612.9262-5-hdegoede@redhat.com>
On 12/12/2016 11:56 PM, Hans de Goede wrote:
> The cherrytrail punit has the pmic i2c bus access semaphore at a
> different register address.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Takashi Iwai <tiwai@suse.de>
> Tested-by: Takashi Iwai <tiwai@suse.de>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> Changes in v2:
> -Adjust for accessor_flags -> flags rename
> -Add flags field to struct dw_pci_controller
> -Add get_sem_addr() helper replacing MODEL_CHERRYTRAIL flag checking in
> PUNIT_SEMAPHORE macro
> Changes in v3:
> -Add a gap between ACCESS_* and MODEL_* flags as reserved space for
> future ACCESS_* flags
> Changes in v4:
> -s/CHV/CHT/
> ---
> drivers/i2c/busses/i2c-designware-baytrail.c | 22 +++++++++++++++++-----
> drivers/i2c/busses/i2c-designware-core.h | 2 ++
> drivers/i2c/busses/i2c-designware-pcidrv.c | 26 +++++++++++++++++++-------
> drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
> 4 files changed, 39 insertions(+), 13 deletions(-)
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v4 4/5] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore
From: Jarkko Nikula @ 2016-12-13 13:42 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, Wolfram Sang, Len Brown
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <1481622976.7188.60.camel@linux.intel.com>
On 12/13/2016 11:56 AM, Andy Shevchenko wrote:
> On Mon, 2016-12-12 at 22:56 +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 cpu trying to enter C6 or C7 while we
>> hold
>> the punit bus semaphore, at which point everything just hangs.
>>
>> Avoid this by disallowing the CPU to enter C6 or C7 before acquiring
>> the
>> punit bus semaphore.
>>
>
> Just a nitpick for abbreviations: pmic -> PMIC, punit -> P-Unit, but I'm
> okay with the contents which is more important.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
No need for v5 from my side either (Andy agreed it later in the thread)
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051
>
> What would be good is to have comments / tags from Len and Ville.
>
We can have also follow up patch if some other PM QoS acrobatics are
required than what's implemented here. Getting real bug fixed is quite
big benefit.
^ permalink raw reply
* Re: [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
From: Jarkko Nikula @ 2016-12-13 13:35 UTC (permalink / raw)
To: Andy Shevchenko, Tin Huynh, Mika Westerberg, Wolfram Sang
Cc: linux-i2c, linux-kernel, linux-acpi, Loc Ho, Thang Nguyen,
Phong Vo, patches
In-Reply-To: <1481628337.7188.63.camel@linux.intel.com>
On 12/13/2016 01:25 PM, Andy Shevchenko wrote:
> On Tue, 2016-12-13 at 17:03 +0700, Tin Huynh wrote:
>> ACPI always sets Tx/Rx FIFO to 32. This configuration will
>> cause problem if the IP core supports a FIFO size of less than 32.
>> The driver should read the FIFO size from the IP and select the
>> smaller
>> one of the two.
>>
>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
>>
>
> One comment below, after addressing it
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
Feel free to add my
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v4 4/5] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore
From: Andy Shevchenko @ 2016-12-13 12:34 UTC (permalink / raw)
To: Hans de Goede, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <63a1993a-7df8-db09-01e7-7b82ddcc85b0@redhat.com>
On Tue, 2016-12-13 at 13:21 +0100, Hans de Goede wrote:
> Hi,
>
> On 13-12-16 10:56, Andy Shevchenko wrote:
> > On Mon, 2016-12-12 at 22:56 +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 cpu trying to enter C6 or C7 while
> > > we
> > > hold
> > > the punit bus semaphore, at which point everything just hangs.
> > >
> > > Avoid this by disallowing the CPU to enter C6 or C7 before
> > > acquiring
> > > the
> > > punit bus semaphore.
> > >
> >
> > Just a nitpick for abbreviations: pmic -> PMIC, punit -> P-Unit, but
> > I'm
> > okay with the contents which is more important.
>
> Erm, the rest of the code, including dev_info and dev_err messages
> also uses punit without the - in there, anyways not planning to
> send a v5 for now.
Yes, no need in v5 until something more serious comes up.
>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Thank you.
>
> > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051
> >
> > What would be good is to have comments / tags from Len and Ville.
>
> About this patch vs bug bko109051, yesterday I've spend time reading
> that entire bug. It seems it is a combination of at least 3 bugs
> combined, 2 i915 related with commits which seem to trigger
> the problem (2 different groups of users with a different problem
> it seems) which causes a hang every few hours. And one other
> bug where the system freezes in minutes, that one sounds like
> what I was seeing without this patch (but may well be yet
> another issue).
There also a dw_dmac bug which I fixed, but people are still referring
to it in that bug report.
>
> As for the 2 i915 bugs, there have been git bisects for both of
> them, it would be good if someone could take a look at these, just
> search for bisect in that huge bug.
Agreed.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v4 4/5] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore
From: Hans de Goede @ 2016-12-13 12:21 UTC (permalink / raw)
To: Andy Shevchenko, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <1481622976.7188.60.camel@linux.intel.com>
Hi,
On 13-12-16 10:56, Andy Shevchenko wrote:
> On Mon, 2016-12-12 at 22:56 +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 cpu trying to enter C6 or C7 while we
>> hold
>> the punit bus semaphore, at which point everything just hangs.
>>
>> Avoid this by disallowing the CPU to enter C6 or C7 before acquiring
>> the
>> punit bus semaphore.
>>
>
> Just a nitpick for abbreviations: pmic -> PMIC, punit -> P-Unit, but I'm
> okay with the contents which is more important.
Erm, the rest of the code, including dev_info and dev_err messages
also uses punit without the - in there, anyways not planning to
send a v5 for now.
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Thank you.
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051
>
> What would be good is to have comments / tags from Len and Ville.
About this patch vs bug bko109051, yesterday I've spend time reading
that entire bug. It seems it is a combination of at least 3 bugs
combined, 2 i915 related with commits which seem to trigger
the problem (2 different groups of users with a different problem
it seems) which causes a hang every few hours. And one other
bug where the system freezes in minutes, that one sounds like
what I was seeing without this patch (but may well be yet
another issue).
As for the 2 i915 bugs, there have been git bisects for both of
them, it would be good if someone could take a look at these, just
search for bisect in that huge bug.
Regards,
Hans
>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: Takashi Iwai <tiwai@suse.de>
>> ---
>> Changes in v2:
>> -New patch in v2 of this set
>> Changes in v3:
>> -Change commit message and comment in the code from "force the CPU to
>> C1"
>> to "Disallow the CPU to enter C6 or C7", as the CPU may still be in
>> either
>> C0 or C1 with the request pm_qos
>> Changes in v4:
>> -Rename i2c_dw_eval_lock_support to i2c_dw_probe_lock_support so that
>> we can
>> add a matching i2c_dw_remove_lock_support cleanup function
>> -Move qm_pos removal to new i2c_dw_remove_lock_support function
>> -Move pm_qos_add_request to the end of i2c_dw_probe_lock_support
>> ---
>> drivers/i2c/busses/i2c-designware-baytrail.c | 21
>> ++++++++++++++++++++-
>> drivers/i2c/busses/i2c-designware-core.h | 9 +++++++--
>> drivers/i2c/busses/i2c-designware-platdrv.c | 4 +++-
>> 3 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
>> b/drivers/i2c/busses/i2c-designware-baytrail.c
>> index cf02222..95d7013 100644
>> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
>> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
>> @@ -16,6 +16,7 @@
>> #include <linux/acpi.h>
>> #include <linux/i2c.h>
>> #include <linux/interrupt.h>
>> +#include <linux/pm_qos.h>
>>
>> #include <asm/iosf_mbi.h>
>>
>> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32
>> *sem)
>> u32 data;
>> int ret;
>>
>> + /*
>> + * Disallow the CPU to enter C6 or C7 state, entering these
>> states
>> + * requires the punit to talk to the pmic and if this happens
>> while
>> + * we're holding the semaphore, the SoC hangs.
>> + */
>> + pm_qos_update_request(&dev->pm_qos, 0);
>> +
>> ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
>> PUNIT_SEMAPHORE, &data);
>> if (ret) {
>> dev_err(dev->dev, "iosf failed to read punit
>> semaphore\n");
>> @@ -56,6 +64,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
>> data &= ~PUNIT_SEMAPHORE_BIT;
>> if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
>> PUNIT_SEMAPHORE, data))
>> dev_err(dev->dev, "iosf failed to reset punit
>> semaphore during write\n");
>> +
>> + pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
>> }
>>
>> static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
>> @@ -121,7 +131,7 @@ static void baytrail_i2c_release(struct dw_i2c_dev
>> *dev)
>> jiffies_to_msecs(jiffies - acquired));
>> }
>>
>> -int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
>> +int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
>> {
>> acpi_status status;
>> unsigned long long shared_host = 0;
>> @@ -149,5 +159,14 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev
>> *dev)
>> dev->release_lock = baytrail_i2c_release;
>> dev->pm_runtime_disabled = true;
>>
>> + pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY,
>> + PM_QOS_DEFAULT_VALUE);
>> +
>> return 0;
>> }
>> +
>> +void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
>> +{
>> + if (dev->acquire_lock)
>> + pm_qos_remove_request(&dev->pm_qos);
>> +}
>> diff --git a/drivers/i2c/busses/i2c-designware-core.h
>> b/drivers/i2c/busses/i2c-designware-core.h
>> index fb143f5..871970e 100644
>> --- a/drivers/i2c/busses/i2c-designware-core.h
>> +++ b/drivers/i2c/busses/i2c-designware-core.h
>> @@ -22,6 +22,7 @@
>> *
>> */
>>
>> +#include <linux/pm_qos.h>
>>
>> #define DW_IC_CON_MASTER 0x1
>> #define DW_IC_CON_SPEED_STD 0x2
>> @@ -67,6 +68,7 @@
>> * @fp_lcnt: fast plus LCNT value
>> * @hs_hcnt: high speed HCNT value
>> * @hs_lcnt: high speed LCNT value
>> + * @pm_qos: pm_qos_request used while holding a hardware lock on the
>> bus
>> * @acquire_lock: function to acquire a hardware lock on the bus
>> * @release_lock: function to release a hardware lock on the bus
>> * @pm_runtime_disabled: true if pm runtime is disabled
>> @@ -114,6 +116,7 @@ struct dw_i2c_dev {
>> u16 fp_lcnt;
>> u16 hs_hcnt;
>> u16 hs_lcnt;
>> + struct pm_qos_request pm_qos;
>> int (*acquire_lock)(struct dw_i2c_dev
>> *dev);
>> void (*release_lock)(struct dw_i2c_dev
>> *dev);
>> bool pm_runtime_disabled;
>> @@ -131,7 +134,9 @@ extern u32 i2c_dw_read_comp_param(struct
>> dw_i2c_dev *dev);
>> extern int i2c_dw_probe(struct dw_i2c_dev *dev);
>>
>> #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
>> -extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev);
>> +extern int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev);
>> +extern void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev);
>> #else
>> -static inline int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev) {
>> return 0; }
>> +static inline int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev) {
>> return 0; }
>> +static inline void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
>> {}
>> #endif
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 97a2ca1..b0a64a2 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -210,7 +210,7 @@ static int dw_i2c_plat_probe(struct
>> platform_device *pdev)
>> return -EINVAL;
>> }
>>
>> - r = i2c_dw_eval_lock_support(dev);
>> + r = i2c_dw_probe_lock_support(dev);
>> if (r)
>> return r;
>>
>> @@ -291,6 +291,8 @@ static int dw_i2c_plat_remove(struct
>> platform_device *pdev)
>> if (!dev->pm_runtime_disabled)
>> pm_runtime_disable(&pdev->dev);
>>
>> + i2c_dw_remove_lock_support(dev);
>> +
>> return 0;
>> }
>>
>
^ permalink raw reply
* Re: [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
From: Andy Shevchenko @ 2016-12-13 11:25 UTC (permalink / raw)
To: Tin Huynh, Jarkko Nikula, Mika Westerberg, Wolfram Sang
Cc: linux-i2c, linux-kernel, linux-acpi, Loc Ho, Thang Nguyen,
Phong Vo, patches
In-Reply-To: <1481623414-19220-1-git-send-email-tnhuynh@apm.com>
On Tue, 2016-12-13 at 17:03 +0700, Tin Huynh wrote:
> ACPI always sets Tx/Rx FIFO to 32. This configuration will
> cause problem if the IP core supports a FIFO size of less than 32.
> The driver should read the FIFO size from the IP and select the
> smaller
> one of the two.
>
> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
>
One comment below, after addressing it
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-designware-platdrv.c | 27
> ++++++++++++++++++++-------
> 1 files changed, 20 insertions(+), 7 deletions(-)
>
> Change from V3:
> -Use uppercase of FIFO instead of lowercase.
> -Fix the problem when IP core return 0 of FIFO.
>
> Change from V2:
> -Add a helper function to set FIFO size.
>
> Change from V1:
> -Revert the default 32 for FIFO, read parameter from IP core
> and pick the smaller one of the two.
> -Correct the title to describe new approach.
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0b42a12..24032d6 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -150,6 +150,25 @@ static int i2c_dw_plat_prepare_clk(struct
> dw_i2c_dev *i_dev, bool prepare)
> return 0;
> }
>
> +static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
> +{
> + u32 param, tx_fifo_depth, rx_fifo_depth;
> +
>
/*
* Try to detect the FIFO depth if not
set by interface driver,
* the depth could be from 2 to 256 from
HW spec.
*/
> + param = i2c_dw_read_comp_param(dev);
> + tx_fifo_depth = ((param >> 16) & 0xff) + 1;
> + rx_fifo_depth = ((param >> 8) & 0xff) + 1;
> + if (!dev->tx_fifo_depth) {
> + dev->tx_fifo_depth = tx_fifo_depth;
> + dev->rx_fifo_depth = rx_fifo_depth;
> + dev->adapter.nr = id;
> + } else if (tx_fifo_depth > 1) {
This makes sense now, though I would add a comment here and use >= 2 to
reflect datasheet.
/*
* Choose minimum values between HW and interface
* driver provided.
*/
> + dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
> + tx_fifo_depth);
> + dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
> + rx_fifo_depth);
> + }
> +}
> +
> static int dw_i2c_plat_probe(struct platform_device *pdev)
> {
> struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev-
> >dev);
> @@ -246,13 +265,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> 1000000);
> }
>
> - if (!dev->tx_fifo_depth) {
> - u32 param1 = i2c_dw_read_comp_param(dev);
> -
> - dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> - dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1;
> - dev->adapter.nr = pdev->id;
> - }
> + dw_i2c_set_fifo_size(dev, pdev->id);
>
> adap = &dev->adapter;
> adap->owner = THIS_MODULE;
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* RE: [PATCH v4 3/5] i2c: designware: Add slave definitions
From: Luis de Oliveira @ 2016-12-13 10:50 UTC (permalink / raw)
To: Rob Herring, Luis de Oliveira
Cc: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org,
andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w@public.gmane.org,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w@public.gmane.org,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w@public.gmane.org
In-Reply-To: <CAL_JsqJwjX1Y=bDD-hKcXqa1aBKjVhXjEZm6j2tMAQszyv-QhA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
The controller for i2c-designware cannot be slave/master at the same time and it has to be enabled knowing beforehand if we want it to be slave or master by something outside of the controller itself.
I as looking and I see the use of this I2C_OWN_SLAVE_ADDRESS with the "linux,slave-24c02" slave interface but I am not seeing how it will help me identify a selected i2c-designware block as a "slave" device before instantiation. I'm sorry if I'm not understanding properly.
I use the "linux,slave-24c02" to instantiate the i2c-designware as a slave with an address so I can do write/read operations, it is how I tested it.
Luis
-----Original Message-----
From: Rob Herring [mailto:robh@kernel.org]
Sent: Monday, December 12, 2016 23:16
To: Luis de Oliveira <Luis.Oliveira@synopsys.com>
Cc: wsa@the-dreams.de; mark.rutland@arm.com; jarkko.nikula@linux.intel.com; andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Ramiro.Oliveira@synopsys.com; Joao.Pinto@synopsys.com; CARLOS.PALMINHA@synopsys.com
Subject: Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
On Mon, Dec 12, 2016 at 12:35 PM, Luis de Oliveira <Luis.Oliveira@synopsys.com> wrote:
> Hi all,
Please don't top post.
>
> The slave address could be set by the I2C slave backend so I can't use it to setup the controller.
> A boolean property was my initial approach then Andy and Wolfram Sang suggested the use of compatible strings and later It was suggested to use a property to select mode but I can do it again if it's the best way.
> Can you please tell me where should it be documented?
bindings/i2c/i2c.txt.
Actually, looking at this some more, we already have a way to describe the controller being a slave device with the I2C_OWN_SLAVE_ADDRESS flag in the reg property. We should just need a helper to read reg property of each child and check for the bit set.
Rob
^ permalink raw reply
* Re: [PATCH V3] i2c: designware: fix wrong tx/rx fifo for ACPI
From: Mika Westerberg @ 2016-12-13 10:19 UTC (permalink / raw)
To: Joe Perches
Cc: Andy Shevchenko, Tin Huynh, Jarkko Nikula, Wolfram Sang,
linux-i2c, linux-kernel, linux-acpi, Loc Ho, Thang Nguyen,
Phong Vo, patches
In-Reply-To: <1481571319.1764.32.camel@perches.com>
On Mon, Dec 12, 2016 at 11:35:19AM -0800, Joe Perches wrote:
> On Mon, 2016-12-12 at 21:21 +0200, Mika Westerberg wrote:
> > On Mon, Dec 12, 2016 at 09:02:53PM +0200, Andy Shevchenko wrote:
> > > > + tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> > > > + rx_fifo_depth = ((param1 >> 8) & 0xff) + 1;
> > > > + if (!dev->tx_fifo_depth) {
> > > > + dev->tx_fifo_depth = tx_fifo_depth;
> > > > + dev->rx_fifo_depth = rx_fifo_depth;
> > > > + } else if (tx_fifo_depth) {
> > > > + dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
> > > > + tx_fifo_depth);
> > > > + dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
> > > > + rx_fifo_depth);
> > > > + }
> > >
> > > So, let's clarify here:
> > > Is it possible to have an IP without parameter block enabled? I mean to
> > > read something arbitrary (or zeroes, or all-ones) from param1.
> >
> > Yes and it is Intel IP. Haswell IIRC and it returned zeroes.
>
> The "+ 1" in the first set of tx_fifo_depth
> makes the "else if" check unnecessary.
Good point. I did not notice that change at all.
The designware I2C databook I have here says that 0 is reserved value
and FIFO sizes start from 2 so the above is wrong either way.
^ permalink raw reply
* [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
From: Tin Huynh @ 2016-12-13 10:03 UTC (permalink / raw)
To: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, Wolfram Sang
Cc: linux-i2c, linux-kernel, linux-acpi, Loc Ho, Thang Nguyen,
Phong Vo, patches, Tin Huynh
ACPI always sets Tx/Rx FIFO to 32. This configuration will
cause problem if the IP core supports a FIFO size of less than 32.
The driver should read the FIFO size from the IP and select the smaller
one of the two.
Signed-off-by: Tin Huynh <tnhuynh@apm.com>
---
drivers/i2c/busses/i2c-designware-platdrv.c | 27 ++++++++++++++++++++-------
1 files changed, 20 insertions(+), 7 deletions(-)
Change from V3:
-Use uppercase of FIFO instead of lowercase.
-Fix the problem when IP core return 0 of FIFO.
Change from V2:
-Add a helper function to set FIFO size.
Change from V1:
-Revert the default 32 for FIFO, read parameter from IP core
and pick the smaller one of the two.
-Correct the title to describe new approach.
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0b42a12..24032d6 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -150,6 +150,25 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
return 0;
}
+static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
+{
+ u32 param, tx_fifo_depth, rx_fifo_depth;
+
+ param = i2c_dw_read_comp_param(dev);
+ tx_fifo_depth = ((param >> 16) & 0xff) + 1;
+ rx_fifo_depth = ((param >> 8) & 0xff) + 1;
+ if (!dev->tx_fifo_depth) {
+ dev->tx_fifo_depth = tx_fifo_depth;
+ dev->rx_fifo_depth = rx_fifo_depth;
+ dev->adapter.nr = id;
+ } else if (tx_fifo_depth > 1) {
+ dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
+ tx_fifo_depth);
+ dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
+ rx_fifo_depth);
+ }
+}
+
static int dw_i2c_plat_probe(struct platform_device *pdev)
{
struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -246,13 +265,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
1000000);
}
- if (!dev->tx_fifo_depth) {
- u32 param1 = i2c_dw_read_comp_param(dev);
-
- dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
- dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1;
- dev->adapter.nr = pdev->id;
- }
+ dw_i2c_set_fifo_size(dev, pdev->id);
adap = &dev->adapter;
adap->owner = THIS_MODULE;
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v6 5/5] ARM: configs: stm32: Add I2C support for STM32 defconfig
From: Alexandre Torgue @ 2016-12-13 10:03 UTC (permalink / raw)
To: M'boumba Cedric Madianga, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A, patrice.chotard-qxv4g6HH51o,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1481559342-6106-6-git-send-email-cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Cedric,
On 12/12/2016 05:15 PM, M'boumba Cedric Madianga wrote:
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Can you please add a commit message.
Thx in advance
Alex
> ---
> arch/arm/configs/stm32_defconfig | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm/configs/stm32_defconfig b/arch/arm/configs/stm32_defconfig
> index e7b56d4..9494eaf 100644
> --- a/arch/arm/configs/stm32_defconfig
> +++ b/arch/arm/configs/stm32_defconfig
> @@ -52,6 +52,9 @@ CONFIG_SERIAL_NONSTANDARD=y
> CONFIG_SERIAL_STM32=y
> CONFIG_SERIAL_STM32_CONSOLE=y
> # CONFIG_HW_RANDOM is not set
> +CONFIG_I2C=y
> +CONFIG_I2C_CHARDEV=y
> +CONFIG_I2C_STM32F4=y
> # CONFIG_HWMON is not set
> # CONFIG_USB_SUPPORT is not set
> CONFIG_NEW_LEDS=y
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 4/5] ARM: dts: stm32: Add I2C1 support for STM32429 eval board
From: Alexandre Torgue @ 2016-12-13 10:02 UTC (permalink / raw)
To: M'boumba Cedric Madianga, wsa, robh+dt, mcoquelin.stm32,
linus.walleij, patrice.chotard, linux, linux-i2c, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <1481559342-6106-5-git-send-email-cedric.madianga@gmail.com>
Hi Cedric,
On 12/12/2016 05:15 PM, M'boumba Cedric Madianga wrote:
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
Can you please add a commit message.
> ---
> arch/arm/boot/dts/stm32429i-eval.dts | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stm32429i-eval.dts b/arch/arm/boot/dts/stm32429i-eval.dts
> index afb90bc..74e0045 100644
> --- a/arch/arm/boot/dts/stm32429i-eval.dts
> +++ b/arch/arm/boot/dts/stm32429i-eval.dts
> @@ -141,3 +141,9 @@
> pinctrl-names = "default";
> status = "okay";
> };
> +
> +&i2c1 {
> + pinctrl-0 = <&i2c1_pins_b>;
> + pinctrl-names = "default";
> + status = "okay";
> +};
>
^ permalink raw reply
* Re: [PATCH v6 3/5] ARM: dts: stm32: Add I2C1 support for STM32F429 SoC
From: Alexandre Torgue @ 2016-12-13 10:01 UTC (permalink / raw)
To: M'boumba Cedric Madianga, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A, patrice.chotard-qxv4g6HH51o,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1481559342-6106-4-git-send-email-cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Cedric,
On 12/12/2016 05:15 PM, M'boumba Cedric Madianga wrote:
> Signed-off-by: Patrice Chotard <patrice.chotard-qxv4g6HH51o@public.gmane.org>
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Please Add a commit message.
> ---
> arch/arm/boot/dts/stm32f429.dtsi | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
> index 7de52ee..cbdece7 100644
> --- a/arch/arm/boot/dts/stm32f429.dtsi
> +++ b/arch/arm/boot/dts/stm32f429.dtsi
> @@ -48,6 +48,7 @@
> #include "skeleton.dtsi"
> #include "armv7-m.dtsi"
> #include <dt-bindings/pinctrl/stm32f429-pinfunc.h>
> +#include <dt-bindings/mfd/stm32f4-rcc.h>
>
> / {
> clocks {
> @@ -337,6 +338,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>;
> + };
> + };
> };
>
> rcc: rcc@40023810 {
> @@ -409,6 +420,18 @@
> interrupts = <80>;
> clocks = <&rcc 0 38>;
> };
> +
> + i2c1: i2c@40005400 {
Can you check the order on device node please ? (should follow base@)
> + compatible = "st,stm32f4-i2c";
> + reg = <0x40005400 0x400>;
> + interrupts = <31>,
> + <32>;
> + resets = <&rcc STM32F4_APB1_RESET(I2C1)>;
> + clocks = <&rcc 0 STM32F4_APB1_CLOCK(I2C1)>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> };
> };
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 4/5] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore
From: Andy Shevchenko @ 2016-12-13 9:56 UTC (permalink / raw)
To: Hans de Goede, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: Takashi Iwai, russianneuromancer @ ya . ru, intel-gfx, linux-i2c,
Mika Westerberg
In-Reply-To: <20161212215612.9262-4-hdegoede@redhat.com>
On Mon, 2016-12-12 at 22:56 +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 cpu trying to enter C6 or C7 while we
> hold
> the punit bus semaphore, at which point everything just hangs.
>
> Avoid this by disallowing the CPU to enter C6 or C7 before acquiring
> the
> punit bus semaphore.
>
Just a nitpick for abbreviations: pmic -> PMIC, punit -> P-Unit, but I'm
okay with the contents which is more important.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051
What would be good is to have comments / tags from Len and Ville.
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Takashi Iwai <tiwai@suse.de>
> ---
> Changes in v2:
> -New patch in v2 of this set
> Changes in v3:
> -Change commit message and comment in the code from "force the CPU to
> C1"
> to "Disallow the CPU to enter C6 or C7", as the CPU may still be in
> either
> C0 or C1 with the request pm_qos
> Changes in v4:
> -Rename i2c_dw_eval_lock_support to i2c_dw_probe_lock_support so that
> we can
> add a matching i2c_dw_remove_lock_support cleanup function
> -Move qm_pos removal to new i2c_dw_remove_lock_support function
> -Move pm_qos_add_request to the end of i2c_dw_probe_lock_support
> ---
> drivers/i2c/busses/i2c-designware-baytrail.c | 21
> ++++++++++++++++++++-
> drivers/i2c/busses/i2c-designware-core.h | 9 +++++++--
> drivers/i2c/busses/i2c-designware-platdrv.c | 4 +++-
> 3 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> b/drivers/i2c/busses/i2c-designware-baytrail.c
> index cf02222..95d7013 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -16,6 +16,7 @@
> #include <linux/acpi.h>
> #include <linux/i2c.h>
> #include <linux/interrupt.h>
> +#include <linux/pm_qos.h>
>
> #include <asm/iosf_mbi.h>
>
> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32
> *sem)
> u32 data;
> int ret;
>
> + /*
> + * Disallow the CPU to enter C6 or C7 state, entering these
> states
> + * requires the punit to talk to the pmic and if this happens
> while
> + * we're holding the semaphore, the SoC hangs.
> + */
> + pm_qos_update_request(&dev->pm_qos, 0);
> +
> ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> PUNIT_SEMAPHORE, &data);
> if (ret) {
> dev_err(dev->dev, "iosf failed to read punit
> semaphore\n");
> @@ -56,6 +64,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
> data &= ~PUNIT_SEMAPHORE_BIT;
> if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> PUNIT_SEMAPHORE, data))
> dev_err(dev->dev, "iosf failed to reset punit
> semaphore during write\n");
> +
> + pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
> }
>
> static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
> @@ -121,7 +131,7 @@ static void baytrail_i2c_release(struct dw_i2c_dev
> *dev)
> jiffies_to_msecs(jiffies - acquired));
> }
>
> -int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
> +int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
> {
> acpi_status status;
> unsigned long long shared_host = 0;
> @@ -149,5 +159,14 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev
> *dev)
> dev->release_lock = baytrail_i2c_release;
> dev->pm_runtime_disabled = true;
>
> + pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY,
> + PM_QOS_DEFAULT_VALUE);
> +
> return 0;
> }
> +
> +void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
> +{
> + if (dev->acquire_lock)
> + pm_qos_remove_request(&dev->pm_qos);
> +}
> diff --git a/drivers/i2c/busses/i2c-designware-core.h
> b/drivers/i2c/busses/i2c-designware-core.h
> index fb143f5..871970e 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -22,6 +22,7 @@
> *
> */
>
> +#include <linux/pm_qos.h>
>
> #define DW_IC_CON_MASTER 0x1
> #define DW_IC_CON_SPEED_STD 0x2
> @@ -67,6 +68,7 @@
> * @fp_lcnt: fast plus LCNT value
> * @hs_hcnt: high speed HCNT value
> * @hs_lcnt: high speed LCNT value
> + * @pm_qos: pm_qos_request used while holding a hardware lock on the
> bus
> * @acquire_lock: function to acquire a hardware lock on the bus
> * @release_lock: function to release a hardware lock on the bus
> * @pm_runtime_disabled: true if pm runtime is disabled
> @@ -114,6 +116,7 @@ struct dw_i2c_dev {
> u16 fp_lcnt;
> u16 hs_hcnt;
> u16 hs_lcnt;
> + struct pm_qos_request pm_qos;
> int (*acquire_lock)(struct dw_i2c_dev
> *dev);
> void (*release_lock)(struct dw_i2c_dev
> *dev);
> bool pm_runtime_disabled;
> @@ -131,7 +134,9 @@ extern u32 i2c_dw_read_comp_param(struct
> dw_i2c_dev *dev);
> extern int i2c_dw_probe(struct dw_i2c_dev *dev);
>
> #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
> -extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev);
> +extern int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev);
> +extern void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev);
> #else
> -static inline int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev) {
> return 0; }
> +static inline int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev) {
> return 0; }
> +static inline void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
> {}
> #endif
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 97a2ca1..b0a64a2 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -210,7 +210,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> return -EINVAL;
> }
>
> - r = i2c_dw_eval_lock_support(dev);
> + r = i2c_dw_probe_lock_support(dev);
> if (r)
> return r;
>
> @@ -291,6 +291,8 @@ static int dw_i2c_plat_remove(struct
> platform_device *pdev)
> if (!dev->pm_runtime_disabled)
> pm_runtime_disable(&pdev->dev);
>
> + i2c_dw_remove_lock_support(dev);
> +
> return 0;
> }
>
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH v6 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2016-12-13 9:20 UTC (permalink / raw)
To: M'boumba Cedric Madianga
Cc: wsa, robh+dt, mcoquelin.stm32, alexandre.torgue, linus.walleij,
patrice.chotard, linux, linux-i2c, devicetree, linux-arm-kernel,
linux-kernel
In-Reply-To: <1481559342-6106-3-git-send-email-cedric.madianga@gmail.com>
Hello,
On Mon, Dec 12, 2016 at 05:15:39PM +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 | 849 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 860 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..89ad579
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-stm32f4.c
> @@ -0,0 +1,849 @@
> +/*
> + * Driver for STMicroelectronics STM32 I2C controller
> + *
> + * Copyright (C) M'boumba Cedric Madianga 2015
> + * 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
> + */
If there is a public description available for the device, a link here
would be great.
> +
> +#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))
This should better be ((n) & STM32F4_I2C_CR2_FREQ_MASK). There a few
more constants that need the same fix.
> +#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)
I'd layout this like:
#define STM32F4_I2C_CR2_IRQ_MASK (STM32F4_I2C_CR2_ITBUFEN | \
STM32F4_I2C_CR2_ITEVTEN | \
STM32F4_I2C_CR2_ITERREN)
which is more usual I think.
> +/* 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))
> +#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 FAST_MODE_MAX_RISE_TIME 1000
> +#define STD_MODE_MAX_RISE_TIME 300
Are these supposed to be the values "rise time of both SDA and SCL
signals" from the i2c specification? If so, you got it wrong, fast mode
has the smaller value.
Maybe these constants could get a home in a more central place?
Also I'd add /* ns */ to the definition.
> +#define MHZ_TO_HZ 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
> + * @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
> + */
> +struct stm32f4_i2c_timings {
> + u32 rate;
rate is undocumented and unused.
> + u32 duty;
> + 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;
> +};
> +
> +/**
> + * 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
> + * @irq_event: interrupt event line for the controller
> + * @irq_error: interrupt error line for the controller
> + * @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;
> + int irq_event;
> + int irq_error;
You only use irq_event in the probe function. So there is no need to
remember this one and you could use a local variable instead.
> + struct clk *clk;
> + int speed;
> + struct stm32f4_i2c_msg msg;
> +};
> +
> +static struct stm32f4_i2c_timings i2c_timings[] = {
> + [STM32F4_I2C_SPEED_STANDARD] = {
> + .mul_ccr = 1,
> + .min_ccr = 4,
> + .duty = 0,
> + },
> + [STM32F4_I2C_SPEED_FAST] = {
> + .mul_ccr = 16,
> + .min_ccr = 1,
> + .duty = 1,
> + },
Are these values from the datasheet?
> +};
> +
> +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;
> +
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_SWRST);
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_SWRST);
Not very critical, but you're doing an unneeded register access here
because the register is read twice.
Also I think readability would improve if you dropped
stm32f4_i2c_{set,clr}_bits and do their logic explicitly in the callers.
stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_SWRST);
stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_SWRST);
vs
val = readl_relaxed(reg);
writel_relaxed(val | STM32F4_I2C_CR1_SWRST, reg);
writel_relaxed(val, reg);
> +}
> +
> +static void stm32f4_i2c_disable_it(struct stm32f4_i2c_dev *i2c_dev)
What is "it"? If it stands for "interrupt" the more usual abbrev is
"irq".
> +{
> + 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;
> +
> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> + cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
> + clk_rate = clk_get_rate(i2c_dev->clk);
> + freq = clk_rate / MHZ_TO_HZ;
> + 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);
Can you quote the data sheet enough in a comment here to make it obvious
that your calculation is right?
Would it be more sensible to error out if clk_rate / MHZ_TO_HZ isn't in
the interval [STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ]?
Usually I would expect that you need to use
DIV_ROUND_UP(clk_rate, MHZ_TO_HZ) instead of a plain division.
> +}
> +
> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + u32 trise, freq, cr2, val;
> +
> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> +
> + trise = readl_relaxed(i2c_dev->base + STM32F4_I2C_TRISE);
> + trise &= ~STM32F4_I2C_TRISE_VALUE_MASK;
Are you required to use rmw for STM32F4_I2C_TRISE? I'd prefer
writel_relaxed(STM32F4_I2C_TRISE_VALUE(..), i2c_dev->base + STM32F4_I2C_TRISE);
unless the datasheet requires rmw.
> + /* Maximum rise time computation */
> + if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD) {
> + trise |= STM32F4_I2C_TRISE_VALUE((freq + 1));
A single pair of parenthesis is enough when you fix
STM32F4_I2C_TRISE_VALUE as suggested above.
> + } else {
> + val = freq * FAST_MODE_MAX_RISE_TIME / STD_MODE_MAX_RISE_TIME;
> + trise |= STM32F4_I2C_TRISE_VALUE((val + 1));
val could be local to this branch.
Or make it shorter using:
freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
if (i2c_dev->speed == STM32F4_I2C_SPEED_FAST)
freq = freq * FAST_MODE_MAX_RISE_TIME / STD_MODE_MAX_RISE_TIME;
writel_relaxed(STM32F4_I2C_TRISE_VALUE(freq + 1), ...);
A quote from the data sheet about the algorithm would be good here, too.
> + }
> +
> + writel_relaxed(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 ccr, clk_rate;
> + int val;
> +
> + ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
> + ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
> + STM32F4_I2C_CCR_CCR_MASK);
> +
> + clk_rate = clk_get_rate(i2c_dev->clk);
> + val = clk_rate / MHZ_TO_HZ * t->mul_ccr;
Is the rounding done right? Again please describe the hardware in a
comment.
> + 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 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");
> + ret = -EBUSY;
I'm not sure if "bus not free" deserves an error message. Wolfram?
> + }
> +
> + return ret;
> +}
> +
> +[...]
> +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++ = (u8)rbuf & 0xff;
unneeded cast (or unneeded & 0xff).
> + msg->count--;
> +}
> +
> +[...]
> +/**
> + * 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_it(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);
> + }
It looks wrong that you don't call stm32f4_i2c_read_msg if msg->count is
2 or 3. I guess that's because these cases are handled in
stm32f4_i2c_handle_rx_btf? Maybe you can simplify the logic a bit?
> +}
> +
> +/**
> + * 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) {
I don't understand why the handling depends on the number of messages.
> + case 2:
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + /* Generate STOP or REPSTART */
I stumbled about "REPSTART" and would spell it out as "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 EVT and ERR interrupt */
> + 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);
> + 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 PEC Position Ack and clear ADDR flag
What is PEC?
> + */
> + 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 and clear ADDR flag */
> + 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;
> + }
> +}
> +
> +/**
> + * 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)
> +{
> +[...]
> + real_status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
s/real_status/status/ ?
> +
> + if (!(real_status & possible_status)) {
> + dev_dbg(i2c_dev->dev,
> + "spurious evt it (status=0x%08x, ien=0x%08x)\n",
> + real_status, ien);
s/it/irq/
> + return IRQ_NONE;
> + }
> +
> + /* Use __fls() to check error bits first */
> + flag = __fls(real_status & possible_status);
If you get several events reported you only handle a single one. Is this
effective?
> + 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 ITBUF interrupts */
What is ITBUF?
> + reg = i2c_dev->base + STM32F4_I2C_CR2;
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> + 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 it unhandled: status=0x%08x)\n", real_status);
s/it/irq/
> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +[...]
> +static int stm32f4_i2c_xfer_msg(struct stm32f4_i2c_dev *i2c_dev,
> + struct i2c_msg *msg, bool is_first,
> + bool is_last)
> +{
> +[...]
> + /* Enable ITEVT and ITERR interrupts */
This comment isn't helpful. Mentioning their meaning would be great
instead.
> +[...]
> +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 : i;
using num instead of i would be a bit more obvious.
> +static int stm32f4_i2c_probe(struct platform_device *pdev)
> +{
> +[...]
> + i2c_dev->speed = STM32F4_I2C_SPEED_STANDARD;
> + ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
> + if ((!ret) && (clk_rate == 400000))
> + i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
I'd use
if (!ret && clk_rate >= 400000)
i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
. That's less parenthesis and a more robust selection of the bus
frequency.
> +
> + i2c_dev->dev = &pdev->dev;
> +
> + ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_event,
> + NULL, stm32f4_i2c_isr_event,
> + IRQF_ONESHOT, pdev->name, i2c_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request irq %i\n",
> + i2c_dev->irq_error);
That's wrong. Requesting irq_event failed.
> + goto clk_free;
> + }
> +
> + ret = devm_request_threaded_irq(&pdev->dev, i2c_dev->irq_error,
> + NULL, stm32f4_i2c_isr_error,
> + IRQF_ONESHOT, pdev->name, i2c_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request irq %i\n",
> + i2c_dev->irq_error);
> + goto clk_free;
It would also be nice to know for which type of irq this failed. I.e.
please point out if this is the error irq or the event irq in the
message. Ditto for checking the return type of irq_of_parse_and_map.
> + }
> +
> + 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 initialized\n");
This is wrong. The driver is bound now to a device, not initialized.
> +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,
Is this needed?
> + },
> + .probe = stm32f4_i2c_probe,
> + .remove = stm32f4_i2c_remove,
> +};
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ 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