* [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegra i2c.
@ 2015-03-30 20:00 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100
Cc: Mark Rutland, Alexandre Courbot, Russell King, Pawel Moll,
Wolfram Sang, Julian Andres Klode, Greg Kroah-Hartman,
Ian Campbell, Rob Herring, Marc Dietrich, Laxman Dewangan,
Thierry Reding, Kumar Gala, Stephen Warren, Andrey Danin
NVEC driver contains code to manage tegra i2c controller in slave mode.
I2C slave support was implemented in linux kernel. The goal of this
patch serie is to implement I2C slave mode in tegra drived and rework
NVEC driver to use it.
Patches are based on i2c for-next.
Changes for v2:
- rebased on top of new i2c slave framework.
- old code is removed in separate patch
- documentation patch is integrated to main nvec patch
Thanks in advance
Andrey Danin (4):
i2c: tegra: implement slave mode
staging/nvec: reimplement on top of tegra i2c driver
staging/nvec: remove old code
dt: paz00: define nvec as child of i2c bus
.../devicetree/bindings/nvec/nvidia,nvec.txt | 21 +-
arch/arm/boot/dts/tegra20-paz00.dts | 22 +-
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-tegra.c | 119 ++++++
drivers/staging/nvec/nvec.c | 411 +++++++--------------
drivers/staging/nvec/nvec.h | 10 -
6 files changed, 269 insertions(+), 315 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegra i2c.
@ 2015-03-30 20:00 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100
Cc: Andrey Danin, Laxman Dewangan, Wolfram Sang, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
NVEC driver contains code to manage tegra i2c controller in slave mode.
I2C slave support was implemented in linux kernel. The goal of this
patch serie is to implement I2C slave mode in tegra drived and rework
NVEC driver to use it.
Patches are based on i2c for-next.
Changes for v2:
- rebased on top of new i2c slave framework.
- old code is removed in separate patch
- documentation patch is integrated to main nvec patch
Thanks in advance
Andrey Danin (4):
i2c: tegra: implement slave mode
staging/nvec: reimplement on top of tegra i2c driver
staging/nvec: remove old code
dt: paz00: define nvec as child of i2c bus
.../devicetree/bindings/nvec/nvidia,nvec.txt | 21 +-
arch/arm/boot/dts/tegra20-paz00.dts | 22 +-
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-tegra.c | 119 ++++++
drivers/staging/nvec/nvec.c | 411 +++++++--------------
drivers/staging/nvec/nvec.h | 10 -
6 files changed, 269 insertions(+), 315 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegra i2c.
@ 2015-03-30 20:00 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: linux-arm-kernel
NVEC driver contains code to manage tegra i2c controller in slave mode.
I2C slave support was implemented in linux kernel. The goal of this
patch serie is to implement I2C slave mode in tegra drived and rework
NVEC driver to use it.
Patches are based on i2c for-next.
Changes for v2:
- rebased on top of new i2c slave framework.
- old code is removed in separate patch
- documentation patch is integrated to main nvec patch
Thanks in advance
Andrey Danin (4):
i2c: tegra: implement slave mode
staging/nvec: reimplement on top of tegra i2c driver
staging/nvec: remove old code
dt: paz00: define nvec as child of i2c bus
.../devicetree/bindings/nvec/nvidia,nvec.txt | 21 +-
arch/arm/boot/dts/tegra20-paz00.dts | 22 +-
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-tegra.c | 119 ++++++
drivers/staging/nvec/nvec.c | 411 +++++++--------------
drivers/staging/nvec/nvec.h | 10 -
6 files changed, 269 insertions(+), 315 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 1/4] i2c: tegra: implement slave mode
2015-03-30 20:00 ` Andrey Danin
(?)
@ 2015-03-30 20:00 ` Andrey Danin
-1 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100
Cc: Mark Rutland, Alexandre Courbot, Russell King, Pawel Moll,
Wolfram Sang, Julian Andres Klode, Greg Kroah-Hartman,
Ian Campbell, Rob Herring, Marc Dietrich, Laxman Dewangan,
Thierry Reding, Kumar Gala, Stephen Warren, Andrey Danin
Initialization code is based on NVEC driver.
There is a HW bug in AP20 that was also mentioned in kernel sources
for Toshiba AC100.
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- remove hack from tegra_i2c_clock_disable
- replace slave status helper functions with local variables
- add constant for default delay count value
- add 10-bit address support
- remove read_slave_start_delay init as zero
- don't reset controller during slave registration
- slave isr returns int instead of bool
- make status related variables in slave u32 instead of unsigned int
- enable i2c slave in Kconfig
- rebase on top of new i2c slave framework
- delay workaround was added from nvec
---
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-tegra.c | 119 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 120 insertions(+)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index db09881..a2b259b 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -852,6 +852,7 @@ config I2C_SUN6I_P2WI
config I2C_TEGRA
tristate "NVIDIA Tegra internal I2C controller"
depends on ARCH_TEGRA
+ select I2C_SLAVE
help
If you say yes to this option, support will be included for the
I2C controller embedded in NVIDIA Tegra SOCs
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 1bcd75e..e378827 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -42,8 +42,17 @@
#define I2C_SL_CNFG 0x020
#define I2C_SL_CNFG_NACK (1<<1)
#define I2C_SL_CNFG_NEWSL (1<<2)
+#define I2C_SL_RCVD 0x024
+#define I2C_SL_STATUS 0x028
+#define I2C_SL_ST_IRQ (1<<3)
+#define I2C_SL_ST_END_TRANS (1<<4)
+#define I2C_SL_ST_RCVD (1<<2)
+#define I2C_SL_ST_RNW (1<<1)
#define I2C_SL_ADDR1 0x02c
#define I2C_SL_ADDR2 0x030
+#define I2C_SL_ADDR2_TEN_BIT_MODE 1
+#define I2C_SL_DELAY_COUNT 0x03c
+#define I2C_SL_DELAY_COUNT_DEFAULT 0x1E
#define I2C_TX_FIFO 0x050
#define I2C_RX_FIFO 0x054
#define I2C_PACKET_TRANSFER_STATUS 0x058
@@ -125,6 +134,8 @@ enum msg_end_type {
* @clk_divisor_std_fast_mode: Clock divisor in standard/fast mode. It is
* applicable if there is no fast clock source i.e. single clock
* source.
+ * @slave_read_start_delay: Workaround for AP20 I2C Slave Controller bug. Delay
+ * before writing data byte into register I2C_SL_RCVD.
*/
struct tegra_i2c_hw_feature {
@@ -133,6 +144,7 @@ struct tegra_i2c_hw_feature {
bool has_single_clk_source;
int clk_divisor_hs_mode;
int clk_divisor_std_fast_mode;
+ int slave_read_start_delay;
};
/**
@@ -173,6 +185,7 @@ struct tegra_i2c_dev {
int msg_read;
u32 bus_clk_rate;
bool is_suspended;
+ struct i2c_client *slave;
};
static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
@@ -461,12 +474,78 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
return err;
}
+static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
+{
+ i2c_writel(i2c_dev, val, I2C_SL_RCVD);
+
+ /*
+ * TODO: A correct fix needs to be found for this.
+ *
+ * We experience less incomplete messages with this delay than without
+ * it, but we don't know why. Help is appreciated.
+ */
+ udelay(100);
+}
+
+static int tegra_i2c_slave_isr(int irq, struct tegra_i2c_dev *i2c_dev)
+{
+ u32 status;
+ u8 value;
+ u8 dummy;
+ u32 is_slave_irq, is_read, is_trans_start, is_trans_end;
+
+ if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
+ return -EINVAL;
+
+ status = i2c_readl(i2c_dev, I2C_SL_STATUS);
+
+ is_slave_irq = (status & I2C_SL_ST_IRQ);
+ is_read = (status & I2C_SL_ST_RNW);
+ is_trans_start = (status & I2C_SL_ST_RCVD);
+ is_trans_end = (status & I2C_SL_ST_END_TRANS);
+
+ if (!is_slave_irq)
+ return -EINVAL;
+
+ /* master sent stop */
+ if (is_trans_end) {
+ i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
+ if (!is_trans_start)
+ return 0;
+ }
+
+ if (is_read) {
+ /* i2c master reads data from us */
+ i2c_slave_event(i2c_dev->slave,
+ is_trans_start ? I2C_SLAVE_READ_REQUESTED
+ : I2C_SLAVE_READ_PROCESSED,
+ &value);
+ if (is_trans_start && i2c_dev->hw->slave_read_start_delay)
+ udelay(i2c_dev->hw->slave_read_start_delay);
+ tegra_i2c_slave_write(i2c_dev, value);
+ } else {
+ /* i2c master sends data to us */
+ value = i2c_readl(i2c_dev, I2C_SL_RCVD);
+ if (is_trans_start)
+ tegra_i2c_slave_write(i2c_dev, 0);
+ i2c_slave_event(i2c_dev->slave,
+ is_trans_start ? I2C_SLAVE_WRITE_REQUESTED
+ : I2C_SLAVE_WRITE_RECEIVED,
+ &value);
+ }
+
+ return 0;
+}
+
static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
{
u32 status;
const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
struct tegra_i2c_dev *i2c_dev = dev_id;
+ if (!tegra_i2c_slave_isr(irq, i2c_dev))
+ return IRQ_HANDLED;
+
status = i2c_readl(i2c_dev, I2C_INT_STATUS);
if (status == 0) {
@@ -664,9 +743,48 @@ static u32 tegra_i2c_func(struct i2c_adapter *adap)
return ret;
}
+static int tegra_reg_slave(struct i2c_client *slave)
+{
+ struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(slave->adapter);
+ int addr2 = 0;
+
+ if (i2c_dev->slave)
+ return -EBUSY;
+
+ i2c_dev->slave = slave;
+
+ tegra_i2c_clock_enable(i2c_dev);
+
+ i2c_writel(i2c_dev, I2C_SL_CNFG_NEWSL, I2C_SL_CNFG);
+ i2c_writel(i2c_dev, I2C_SL_DELAY_COUNT_DEFAULT, I2C_SL_DELAY_COUNT);
+
+ if (slave->addr > 0x7F)
+ addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
+
+ i2c_writel(i2c_dev, slave->addr, I2C_SL_ADDR1);
+ i2c_writel(i2c_dev, addr2, I2C_SL_ADDR2);
+
+ return 0;
+}
+
+static int tegra_unreg_slave(struct i2c_client *slave)
+{
+ struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(slave->adapter);
+
+ WARN_ON(!i2c_dev->slave);
+
+ i2c_writel(i2c_dev, 0, I2C_SL_CNFG);
+
+ i2c_dev->slave = NULL;
+
+ return 0;
+}
+
static const struct i2c_algorithm tegra_i2c_algo = {
.master_xfer = tegra_i2c_xfer,
.functionality = tegra_i2c_func,
+ .reg_slave = tegra_reg_slave,
+ .unreg_slave = tegra_unreg_slave,
};
static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
@@ -675,6 +793,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
.has_single_clk_source = false,
.clk_divisor_hs_mode = 3,
.clk_divisor_std_fast_mode = 0,
+ .slave_read_start_delay = 8,
};
static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH v2 1/4] i2c: tegra: implement slave mode
@ 2015-03-30 20:00 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100
Cc: Andrey Danin, Laxman Dewangan, Wolfram Sang, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
Initialization code is based on NVEC driver.
There is a HW bug in AP20 that was also mentioned in kernel sources
for Toshiba AC100.
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- remove hack from tegra_i2c_clock_disable
- replace slave status helper functions with local variables
- add constant for default delay count value
- add 10-bit address support
- remove read_slave_start_delay init as zero
- don't reset controller during slave registration
- slave isr returns int instead of bool
- make status related variables in slave u32 instead of unsigned int
- enable i2c slave in Kconfig
- rebase on top of new i2c slave framework
- delay workaround was added from nvec
---
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-tegra.c | 119 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 120 insertions(+)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index db09881..a2b259b 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -852,6 +852,7 @@ config I2C_SUN6I_P2WI
config I2C_TEGRA
tristate "NVIDIA Tegra internal I2C controller"
depends on ARCH_TEGRA
+ select I2C_SLAVE
help
If you say yes to this option, support will be included for the
I2C controller embedded in NVIDIA Tegra SOCs
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 1bcd75e..e378827 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -42,8 +42,17 @@
#define I2C_SL_CNFG 0x020
#define I2C_SL_CNFG_NACK (1<<1)
#define I2C_SL_CNFG_NEWSL (1<<2)
+#define I2C_SL_RCVD 0x024
+#define I2C_SL_STATUS 0x028
+#define I2C_SL_ST_IRQ (1<<3)
+#define I2C_SL_ST_END_TRANS (1<<4)
+#define I2C_SL_ST_RCVD (1<<2)
+#define I2C_SL_ST_RNW (1<<1)
#define I2C_SL_ADDR1 0x02c
#define I2C_SL_ADDR2 0x030
+#define I2C_SL_ADDR2_TEN_BIT_MODE 1
+#define I2C_SL_DELAY_COUNT 0x03c
+#define I2C_SL_DELAY_COUNT_DEFAULT 0x1E
#define I2C_TX_FIFO 0x050
#define I2C_RX_FIFO 0x054
#define I2C_PACKET_TRANSFER_STATUS 0x058
@@ -125,6 +134,8 @@ enum msg_end_type {
* @clk_divisor_std_fast_mode: Clock divisor in standard/fast mode. It is
* applicable if there is no fast clock source i.e. single clock
* source.
+ * @slave_read_start_delay: Workaround for AP20 I2C Slave Controller bug. Delay
+ * before writing data byte into register I2C_SL_RCVD.
*/
struct tegra_i2c_hw_feature {
@@ -133,6 +144,7 @@ struct tegra_i2c_hw_feature {
bool has_single_clk_source;
int clk_divisor_hs_mode;
int clk_divisor_std_fast_mode;
+ int slave_read_start_delay;
};
/**
@@ -173,6 +185,7 @@ struct tegra_i2c_dev {
int msg_read;
u32 bus_clk_rate;
bool is_suspended;
+ struct i2c_client *slave;
};
static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
@@ -461,12 +474,78 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
return err;
}
+static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
+{
+ i2c_writel(i2c_dev, val, I2C_SL_RCVD);
+
+ /*
+ * TODO: A correct fix needs to be found for this.
+ *
+ * We experience less incomplete messages with this delay than without
+ * it, but we don't know why. Help is appreciated.
+ */
+ udelay(100);
+}
+
+static int tegra_i2c_slave_isr(int irq, struct tegra_i2c_dev *i2c_dev)
+{
+ u32 status;
+ u8 value;
+ u8 dummy;
+ u32 is_slave_irq, is_read, is_trans_start, is_trans_end;
+
+ if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
+ return -EINVAL;
+
+ status = i2c_readl(i2c_dev, I2C_SL_STATUS);
+
+ is_slave_irq = (status & I2C_SL_ST_IRQ);
+ is_read = (status & I2C_SL_ST_RNW);
+ is_trans_start = (status & I2C_SL_ST_RCVD);
+ is_trans_end = (status & I2C_SL_ST_END_TRANS);
+
+ if (!is_slave_irq)
+ return -EINVAL;
+
+ /* master sent stop */
+ if (is_trans_end) {
+ i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
+ if (!is_trans_start)
+ return 0;
+ }
+
+ if (is_read) {
+ /* i2c master reads data from us */
+ i2c_slave_event(i2c_dev->slave,
+ is_trans_start ? I2C_SLAVE_READ_REQUESTED
+ : I2C_SLAVE_READ_PROCESSED,
+ &value);
+ if (is_trans_start && i2c_dev->hw->slave_read_start_delay)
+ udelay(i2c_dev->hw->slave_read_start_delay);
+ tegra_i2c_slave_write(i2c_dev, value);
+ } else {
+ /* i2c master sends data to us */
+ value = i2c_readl(i2c_dev, I2C_SL_RCVD);
+ if (is_trans_start)
+ tegra_i2c_slave_write(i2c_dev, 0);
+ i2c_slave_event(i2c_dev->slave,
+ is_trans_start ? I2C_SLAVE_WRITE_REQUESTED
+ : I2C_SLAVE_WRITE_RECEIVED,
+ &value);
+ }
+
+ return 0;
+}
+
static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
{
u32 status;
const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
struct tegra_i2c_dev *i2c_dev = dev_id;
+ if (!tegra_i2c_slave_isr(irq, i2c_dev))
+ return IRQ_HANDLED;
+
status = i2c_readl(i2c_dev, I2C_INT_STATUS);
if (status == 0) {
@@ -664,9 +743,48 @@ static u32 tegra_i2c_func(struct i2c_adapter *adap)
return ret;
}
+static int tegra_reg_slave(struct i2c_client *slave)
+{
+ struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(slave->adapter);
+ int addr2 = 0;
+
+ if (i2c_dev->slave)
+ return -EBUSY;
+
+ i2c_dev->slave = slave;
+
+ tegra_i2c_clock_enable(i2c_dev);
+
+ i2c_writel(i2c_dev, I2C_SL_CNFG_NEWSL, I2C_SL_CNFG);
+ i2c_writel(i2c_dev, I2C_SL_DELAY_COUNT_DEFAULT, I2C_SL_DELAY_COUNT);
+
+ if (slave->addr > 0x7F)
+ addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
+
+ i2c_writel(i2c_dev, slave->addr, I2C_SL_ADDR1);
+ i2c_writel(i2c_dev, addr2, I2C_SL_ADDR2);
+
+ return 0;
+}
+
+static int tegra_unreg_slave(struct i2c_client *slave)
+{
+ struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(slave->adapter);
+
+ WARN_ON(!i2c_dev->slave);
+
+ i2c_writel(i2c_dev, 0, I2C_SL_CNFG);
+
+ i2c_dev->slave = NULL;
+
+ return 0;
+}
+
static const struct i2c_algorithm tegra_i2c_algo = {
.master_xfer = tegra_i2c_xfer,
.functionality = tegra_i2c_func,
+ .reg_slave = tegra_reg_slave,
+ .unreg_slave = tegra_unreg_slave,
};
static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
@@ -675,6 +793,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
.has_single_clk_source = false,
.clk_divisor_hs_mode = 3,
.clk_divisor_std_fast_mode = 0,
+ .slave_read_start_delay = 8,
};
static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH v2 1/4] i2c: tegra: implement slave mode
@ 2015-03-30 20:00 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: linux-arm-kernel
Initialization code is based on NVEC driver.
There is a HW bug in AP20 that was also mentioned in kernel sources
for Toshiba AC100.
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- remove hack from tegra_i2c_clock_disable
- replace slave status helper functions with local variables
- add constant for default delay count value
- add 10-bit address support
- remove read_slave_start_delay init as zero
- don't reset controller during slave registration
- slave isr returns int instead of bool
- make status related variables in slave u32 instead of unsigned int
- enable i2c slave in Kconfig
- rebase on top of new i2c slave framework
- delay workaround was added from nvec
---
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-tegra.c | 119 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 120 insertions(+)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index db09881..a2b259b 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -852,6 +852,7 @@ config I2C_SUN6I_P2WI
config I2C_TEGRA
tristate "NVIDIA Tegra internal I2C controller"
depends on ARCH_TEGRA
+ select I2C_SLAVE
help
If you say yes to this option, support will be included for the
I2C controller embedded in NVIDIA Tegra SOCs
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 1bcd75e..e378827 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -42,8 +42,17 @@
#define I2C_SL_CNFG 0x020
#define I2C_SL_CNFG_NACK (1<<1)
#define I2C_SL_CNFG_NEWSL (1<<2)
+#define I2C_SL_RCVD 0x024
+#define I2C_SL_STATUS 0x028
+#define I2C_SL_ST_IRQ (1<<3)
+#define I2C_SL_ST_END_TRANS (1<<4)
+#define I2C_SL_ST_RCVD (1<<2)
+#define I2C_SL_ST_RNW (1<<1)
#define I2C_SL_ADDR1 0x02c
#define I2C_SL_ADDR2 0x030
+#define I2C_SL_ADDR2_TEN_BIT_MODE 1
+#define I2C_SL_DELAY_COUNT 0x03c
+#define I2C_SL_DELAY_COUNT_DEFAULT 0x1E
#define I2C_TX_FIFO 0x050
#define I2C_RX_FIFO 0x054
#define I2C_PACKET_TRANSFER_STATUS 0x058
@@ -125,6 +134,8 @@ enum msg_end_type {
* @clk_divisor_std_fast_mode: Clock divisor in standard/fast mode. It is
* applicable if there is no fast clock source i.e. single clock
* source.
+ * @slave_read_start_delay: Workaround for AP20 I2C Slave Controller bug. Delay
+ * before writing data byte into register I2C_SL_RCVD.
*/
struct tegra_i2c_hw_feature {
@@ -133,6 +144,7 @@ struct tegra_i2c_hw_feature {
bool has_single_clk_source;
int clk_divisor_hs_mode;
int clk_divisor_std_fast_mode;
+ int slave_read_start_delay;
};
/**
@@ -173,6 +185,7 @@ struct tegra_i2c_dev {
int msg_read;
u32 bus_clk_rate;
bool is_suspended;
+ struct i2c_client *slave;
};
static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
@@ -461,12 +474,78 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
return err;
}
+static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
+{
+ i2c_writel(i2c_dev, val, I2C_SL_RCVD);
+
+ /*
+ * TODO: A correct fix needs to be found for this.
+ *
+ * We experience less incomplete messages with this delay than without
+ * it, but we don't know why. Help is appreciated.
+ */
+ udelay(100);
+}
+
+static int tegra_i2c_slave_isr(int irq, struct tegra_i2c_dev *i2c_dev)
+{
+ u32 status;
+ u8 value;
+ u8 dummy;
+ u32 is_slave_irq, is_read, is_trans_start, is_trans_end;
+
+ if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
+ return -EINVAL;
+
+ status = i2c_readl(i2c_dev, I2C_SL_STATUS);
+
+ is_slave_irq = (status & I2C_SL_ST_IRQ);
+ is_read = (status & I2C_SL_ST_RNW);
+ is_trans_start = (status & I2C_SL_ST_RCVD);
+ is_trans_end = (status & I2C_SL_ST_END_TRANS);
+
+ if (!is_slave_irq)
+ return -EINVAL;
+
+ /* master sent stop */
+ if (is_trans_end) {
+ i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
+ if (!is_trans_start)
+ return 0;
+ }
+
+ if (is_read) {
+ /* i2c master reads data from us */
+ i2c_slave_event(i2c_dev->slave,
+ is_trans_start ? I2C_SLAVE_READ_REQUESTED
+ : I2C_SLAVE_READ_PROCESSED,
+ &value);
+ if (is_trans_start && i2c_dev->hw->slave_read_start_delay)
+ udelay(i2c_dev->hw->slave_read_start_delay);
+ tegra_i2c_slave_write(i2c_dev, value);
+ } else {
+ /* i2c master sends data to us */
+ value = i2c_readl(i2c_dev, I2C_SL_RCVD);
+ if (is_trans_start)
+ tegra_i2c_slave_write(i2c_dev, 0);
+ i2c_slave_event(i2c_dev->slave,
+ is_trans_start ? I2C_SLAVE_WRITE_REQUESTED
+ : I2C_SLAVE_WRITE_RECEIVED,
+ &value);
+ }
+
+ return 0;
+}
+
static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
{
u32 status;
const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
struct tegra_i2c_dev *i2c_dev = dev_id;
+ if (!tegra_i2c_slave_isr(irq, i2c_dev))
+ return IRQ_HANDLED;
+
status = i2c_readl(i2c_dev, I2C_INT_STATUS);
if (status == 0) {
@@ -664,9 +743,48 @@ static u32 tegra_i2c_func(struct i2c_adapter *adap)
return ret;
}
+static int tegra_reg_slave(struct i2c_client *slave)
+{
+ struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(slave->adapter);
+ int addr2 = 0;
+
+ if (i2c_dev->slave)
+ return -EBUSY;
+
+ i2c_dev->slave = slave;
+
+ tegra_i2c_clock_enable(i2c_dev);
+
+ i2c_writel(i2c_dev, I2C_SL_CNFG_NEWSL, I2C_SL_CNFG);
+ i2c_writel(i2c_dev, I2C_SL_DELAY_COUNT_DEFAULT, I2C_SL_DELAY_COUNT);
+
+ if (slave->addr > 0x7F)
+ addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
+
+ i2c_writel(i2c_dev, slave->addr, I2C_SL_ADDR1);
+ i2c_writel(i2c_dev, addr2, I2C_SL_ADDR2);
+
+ return 0;
+}
+
+static int tegra_unreg_slave(struct i2c_client *slave)
+{
+ struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(slave->adapter);
+
+ WARN_ON(!i2c_dev->slave);
+
+ i2c_writel(i2c_dev, 0, I2C_SL_CNFG);
+
+ i2c_dev->slave = NULL;
+
+ return 0;
+}
+
static const struct i2c_algorithm tegra_i2c_algo = {
.master_xfer = tegra_i2c_xfer,
.functionality = tegra_i2c_func,
+ .reg_slave = tegra_reg_slave,
+ .unreg_slave = tegra_unreg_slave,
};
static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
@@ -675,6 +793,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
.has_single_clk_source = false,
.clk_divisor_hs_mode = 3,
.clk_divisor_std_fast_mode = 0,
+ .slave_read_start_delay = 8,
};
static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH v2 2/4] staging/nvec: reimplement on top of tegra i2c driver
2015-03-30 20:00 ` Andrey Danin
(?)
@ 2015-03-30 20:00 ` Andrey Danin
-1 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100
Cc: Mark Rutland, Alexandre Courbot, Russell King, Pawel Moll,
Wolfram Sang, Julian Andres Klode, Greg Kroah-Hartman,
Ian Campbell, Rob Herring, Marc Dietrich, Laxman Dewangan,
Thierry Reding, Kumar Gala, Stephen Warren, Andrey Danin
Remove i2c controller related code and use tegra i2c driver in slave mode.
Update nvec documentation.
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- remove extra new line
- keep old functions to simplify review
- move nvec_state enum to nvec.c
- use nvec-slave instead of nvec in dts to keep ABI compatibility
- rebased on top of new i2c slave framework
- delay workaround moved to tegra-i2c
- documentation patch is integrated in this commit
- reverted a log message to minimize changes
---
.../devicetree/bindings/nvec/nvidia,nvec.txt | 21 +-
drivers/staging/nvec/nvec.c | 258 +++++++++++++--------
drivers/staging/nvec/nvec.h | 10 -
3 files changed, 169 insertions(+), 120 deletions(-)
diff --git a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt
index 5ae601e..aba34095 100644
--- a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt
+++ b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt
@@ -1,21 +1,6 @@
NVIDIA compliant embedded controller
Required properties:
-- compatible : should be "nvidia,nvec".
-- reg : the iomem of the i2c slave controller
-- interrupts : the interrupt line of the i2c slave controller
-- clock-frequency : the frequency of the i2c bus
-- gpios : the gpio used for ec request
-- slave-addr: the i2c address of the slave controller
-- clocks : Must contain an entry for each entry in clock-names.
- See ../clocks/clock-bindings.txt for details.
-- clock-names : Must include the following entries:
- Tegra20/Tegra30:
- - div-clk
- - fast-clk
- Tegra114:
- - div-clk
-- resets : Must contain an entry for each entry in reset-names.
- See ../reset/reset.txt for details.
-- reset-names : Must include the following entries:
- - i2c
+- compatible : should be "nvidia,nvec-slave".
+- reg: the i2c address of the slave controller
+- request-gpios : the gpio used for ec request
diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index 5868ebb..f4c5527 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -25,8 +25,8 @@
#include <linux/err.h>
#include <linux/gpio.h>
#include <linux/interrupt.h>
+#include <linux/i2c.h>
#include <linux/io.h>
-#include <linux/irq.h>
#include <linux/of.h>
#include <linux/of_gpio.h>
#include <linux/list.h>
@@ -39,25 +39,19 @@
#include "nvec.h"
-#define I2C_CNFG 0x00
-#define I2C_CNFG_PACKET_MODE_EN (1<<10)
-#define I2C_CNFG_NEW_MASTER_SFM (1<<11)
-#define I2C_CNFG_DEBOUNCE_CNT_SHIFT 12
-
-#define I2C_SL_CNFG 0x20
-#define I2C_SL_NEWSL (1<<2)
-#define I2C_SL_NACK (1<<1)
-#define I2C_SL_RESP (1<<0)
-#define I2C_SL_IRQ (1<<3)
-#define END_TRANS (1<<4)
-#define RCVD (1<<2)
-#define RNW (1<<1)
-
-#define I2C_SL_RCVD 0x24
-#define I2C_SL_STATUS 0x28
-#define I2C_SL_ADDR1 0x2c
-#define I2C_SL_ADDR2 0x30
-#define I2C_SL_DELAY_COUNT 0x3c
+
+#define I2C_SL_ST_END_TRANS (1<<4)
+#define I2C_SL_ST_IRQ (1<<3)
+#define I2C_SL_ST_RCVD (1<<2)
+#define I2C_SL_ST_RNW (1<<1)
+
+
+enum nvec_state {
+ ST_NONE,
+ ST_RX,
+ ST_TX,
+ ST_TRANS_START,
+};
/**
* enum nvec_msg_category - Message categories for nvec_msg_alloc()
@@ -479,7 +473,7 @@ static void nvec_tx_completed(struct nvec_chip *nvec)
nvec->tx->pos = 0;
nvec_gpio_set_value(nvec, 0);
} else {
- nvec->state = 0;
+ nvec->state = ST_NONE;
}
}
@@ -497,7 +491,7 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
(uint) nvec->rx->pos);
nvec_msg_free(nvec, nvec->rx);
- nvec->state = 0;
+ nvec->state = ST_NONE;
/* Battery quirk - Often incomplete, and likes to crash */
if (nvec->rx->data[0] == NVEC_BAT)
@@ -514,7 +508,7 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
spin_unlock(&nvec->rx_lock);
- nvec->state = 0;
+ nvec->state = ST_NONE;
if (!nvec_msg_is_event(nvec->rx))
complete(&nvec->ec_transfer);
@@ -522,6 +516,7 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
schedule_work(&nvec->rx_work);
}
+#if 0
/**
* nvec_invalid_flags - Send an error message about invalid flags and jump
* @nvec: The nvec device
@@ -536,6 +531,7 @@ static void nvec_invalid_flags(struct nvec_chip *nvec, unsigned int status,
if (reset)
nvec->state = 0;
}
+#endif /* FIXME: remove old code */
/**
* nvec_tx_set - Set the message to transfer (nvec->tx)
@@ -566,6 +562,8 @@ static void nvec_tx_set(struct nvec_chip *nvec)
(uint)nvec->tx->size, nvec->tx->data[1]);
}
+
+#if 0
/**
* nvec_interrupt - Interrupt handler
* @irq: The IRQ
@@ -755,6 +753,115 @@ static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
clk_disable_unprepare(nvec->i2c_clk);
}
#endif
+#endif /* FIXME: remove old code. */
+
+
+/**
+ * nvec_slave_cb - I2C slave callback
+ *
+ * This callback fills our RX buffers and empties our TX
+ * buffers. This uses a finite state machine.
+ */
+static int nvec_slave_cb(struct i2c_client *client,
+ enum i2c_slave_event event, u8 *val)
+{
+ struct nvec_chip *nvec = i2c_get_clientdata(client);
+
+ switch (event) {
+ case I2C_SLAVE_WRITE_REQUESTED:
+ /* Alloc new msg only if prev transaction finished */
+ if (nvec->state == ST_NONE)
+ nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
+
+ /* Should not happen in a normal world */
+ if (unlikely(nvec->rx == NULL)) {
+ nvec->state = ST_NONE;
+ return -1;
+ }
+ nvec->rx->pos = 0;
+
+ if (client->addr != ((*val) >> 1)) {
+ dev_err(&client->dev,
+ "received address 0x%02x, expected 0x%02x\n",
+ ((*val) >> 1), client->addr);
+ return -1;
+ }
+ nvec->state = ST_TRANS_START;
+ break;
+
+ case I2C_SLAVE_WRITE_RECEIVED:
+ if (nvec->state != ST_TRANS_START &&
+ nvec->state != ST_RX) {
+ dev_err(&client->dev,
+ "unexpected write: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ if (nvec->rx->pos >= NVEC_MSG_SIZE) {
+ dev_err(&client->dev,
+ "write buffer overflow: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ nvec->rx->data[nvec->rx->pos++] = *val;
+ nvec->state = ST_RX;
+ break;
+
+ case I2C_SLAVE_READ_REQUESTED:
+ if (nvec->state != ST_RX) {
+ dev_err(&client->dev,
+ "unexpected read request: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ nvec_msg_free(nvec, nvec->rx);
+ nvec_tx_set(nvec);
+ /* Should not happen in a normal world */
+ if (!nvec->tx) {
+ dev_err(&client->dev,
+ "can't alloc tx msg: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ nvec_gpio_set_value(nvec, 1);
+ /* fallthrough */
+ case I2C_SLAVE_READ_PROCESSED:
+ if (nvec->state != ST_RX &&
+ nvec->state != ST_TX) {
+ dev_err(&client->dev,
+ "unexpected read: state %d\n",
+ nvec->state);
+ return -1;
+ }
+
+ if (!nvec->tx || nvec->tx->pos >= nvec->tx->size) {
+ dev_err(nvec->dev,
+ "tx buffer underflow on %p (%u > %u)\n",
+ nvec->tx,
+ (uint) (nvec->tx ? nvec->tx->pos : 0),
+ (uint) (nvec->tx ? nvec->tx->size : 0));
+ nvec->state = ST_NONE;
+ break;
+ }
+
+ nvec->state = ST_TX;
+ *val = nvec->tx->data[nvec->tx->pos++];
+ break;
+
+ case I2C_SLAVE_STOP:
+ if (nvec->state == ST_TX)
+ nvec_tx_completed(nvec);
+ else if (nvec->state == ST_RX)
+ nvec_rx_completed(nvec);
+ nvec->state = ST_NONE;
+ break;
+
+ default:
+ return 0;
+ }
+
+ return 0;
+}
static void nvec_power_off(void)
{
@@ -767,7 +874,7 @@ static void nvec_power_off(void)
/*
* Parse common device tree data
*/
-static int nvec_i2c_parse_dt_pdata(struct nvec_chip *nvec)
+static int nvec_i2c_parse_dt(struct nvec_chip *nvec)
{
nvec->gpio = of_get_named_gpio(nvec->dev->of_node, "request-gpios", 0);
@@ -776,68 +883,39 @@ static int nvec_i2c_parse_dt_pdata(struct nvec_chip *nvec)
return -ENODEV;
}
- if (of_property_read_u32(nvec->dev->of_node, "slave-addr",
- &nvec->i2c_addr)) {
- dev_err(nvec->dev, "no i2c address specified");
- return -ENODEV;
- }
-
return 0;
}
-static int tegra_nvec_probe(struct platform_device *pdev)
+static int nvec_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
- int err, ret;
- struct clk *i2c_clk;
+ int ret;
struct nvec_chip *nvec;
struct nvec_msg *msg;
- struct resource *res;
- void __iomem *base;
- char get_firmware_version[] = { NVEC_CNTL, GET_FIRMWARE_VERSION },
+ char get_firmware_version[] = { NVEC_CNTL, GET_FIRMWARE_VERSION },
unmute_speakers[] = { NVEC_OEM0, 0x10, 0x59, 0x95 },
enable_event[7] = { NVEC_SYS, CNF_EVENT_REPORTING, true };
- if (!pdev->dev.of_node) {
- dev_err(&pdev->dev, "must be instantiated using device tree\n");
+
+ if (!client->dev.of_node) {
+ dev_err(&client->dev, "must be instantiated using device tree\n");
return -ENODEV;
}
- nvec = devm_kzalloc(&pdev->dev, sizeof(struct nvec_chip), GFP_KERNEL);
+ nvec = devm_kzalloc(&client->dev, sizeof(struct nvec_chip), GFP_KERNEL);
if (nvec == NULL)
return -ENOMEM;
- platform_set_drvdata(pdev, nvec);
- nvec->dev = &pdev->dev;
+ nvec->dev = &client->dev;
+ ret = nvec_i2c_parse_dt(nvec);
+ if (ret < 0)
+ return ret;
- err = nvec_i2c_parse_dt_pdata(nvec);
- if (err < 0)
- return err;
+ i2c_set_clientdata(client, nvec);
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(base))
- return PTR_ERR(base);
+ ret = i2c_slave_register(client, nvec_slave_cb);
+ if (ret < 0)
+ return ret;
- nvec->irq = platform_get_irq(pdev, 0);
- if (nvec->irq < 0) {
- dev_err(&pdev->dev, "no irq resource?\n");
- return -ENODEV;
- }
-
- i2c_clk = devm_clk_get(&pdev->dev, "div-clk");
- if (IS_ERR(i2c_clk)) {
- dev_err(nvec->dev, "failed to get controller clock\n");
- return -ENODEV;
- }
-
- nvec->rst = devm_reset_control_get(&pdev->dev, "i2c");
- if (IS_ERR(nvec->rst)) {
- dev_err(nvec->dev, "failed to get controller reset\n");
- return PTR_ERR(nvec->rst);
- }
-
- nvec->base = base;
- nvec->i2c_clk = i2c_clk;
nvec->rx = &nvec->msg_pool[0];
ATOMIC_INIT_NOTIFIER_HEAD(&nvec->notifier_list);
@@ -852,23 +930,14 @@ static int tegra_nvec_probe(struct platform_device *pdev)
INIT_WORK(&nvec->rx_work, nvec_dispatch);
INIT_WORK(&nvec->tx_work, nvec_request_master);
- err = devm_gpio_request_one(&pdev->dev, nvec->gpio, GPIOF_OUT_INIT_HIGH,
- "nvec gpio");
- if (err < 0) {
+ ret = devm_gpio_request_one(&client->dev, nvec->gpio,
+ GPIOF_OUT_INIT_HIGH,
+ "nvec gpio");
+ if (ret < 0) {
dev_err(nvec->dev, "couldn't request gpio\n");
return -ENODEV;
}
- err = devm_request_irq(&pdev->dev, nvec->irq, nvec_interrupt, 0,
- "nvec", nvec);
- if (err) {
- dev_err(nvec->dev, "couldn't request irq\n");
- return -ENODEV;
- }
- disable_irq(nvec->irq);
-
- tegra_init_i2c_slave(nvec);
-
/* enable event reporting */
nvec_toggle_global_events(nvec, true);
@@ -907,9 +976,10 @@ static int tegra_nvec_probe(struct platform_device *pdev)
return 0;
}
-static int tegra_nvec_remove(struct platform_device *pdev)
+
+static int nvec_remove(struct i2c_client *client)
{
- struct nvec_chip *nvec = platform_get_drvdata(pdev);
+ struct nvec_chip *nvec = i2c_get_clientdata(client);
nvec_toggle_global_events(nvec, false);
mfd_remove_devices(nvec->dev);
@@ -938,8 +1008,6 @@ static int nvec_suspend(struct device *dev)
msg = nvec_write_sync(nvec, ap_suspend, sizeof(ap_suspend));
nvec_msg_free(nvec, msg);
- nvec_disable_i2c_slave(nvec);
-
return 0;
}
@@ -949,7 +1017,6 @@ static int nvec_resume(struct device *dev)
struct nvec_chip *nvec = platform_get_drvdata(pdev);
dev_dbg(nvec->dev, "resuming\n");
- tegra_init_i2c_slave(nvec);
nvec_toggle_global_events(nvec, true);
return 0;
@@ -960,14 +1027,21 @@ static SIMPLE_DEV_PM_OPS(nvec_pm_ops, nvec_suspend, nvec_resume);
/* Match table for of_platform binding */
static const struct of_device_id nvidia_nvec_of_match[] = {
- { .compatible = "nvidia,nvec", },
+ { .compatible = "nvidia,nvec-slave", },
{},
};
MODULE_DEVICE_TABLE(of, nvidia_nvec_of_match);
-static struct platform_driver nvec_device_driver = {
- .probe = tegra_nvec_probe,
- .remove = tegra_nvec_remove,
+static const struct i2c_device_id nvidia_nvec_i2c_table[] = {
+ { "nvec-slave", 0 },
+ {},
+};
+MODULE_DEVICE_TABLE(i2c, nvidia_nvec_i2c_table);
+
+static struct i2c_driver i2c_nvec_device_driver = {
+ .probe = nvec_probe,
+ .remove = nvec_remove,
+ .id_table = nvidia_nvec_i2c_table,
.driver = {
.name = "nvec",
.pm = &nvec_pm_ops,
@@ -975,9 +1049,9 @@ static struct platform_driver nvec_device_driver = {
}
};
-module_platform_driver(nvec_device_driver);
+module_i2c_driver(i2c_nvec_device_driver);
-MODULE_ALIAS("platform:nvec");
+MODULE_ALIAS("i2c:nvec");
MODULE_DESCRIPTION("NVIDIA compliant embedded controller interface");
MODULE_AUTHOR("Marc Dietrich <marvin24@gmx.de>");
MODULE_LICENSE("GPL");
diff --git a/drivers/staging/nvec/nvec.h b/drivers/staging/nvec/nvec.h
index e271375..ad6a556 100644
--- a/drivers/staging/nvec/nvec.h
+++ b/drivers/staging/nvec/nvec.h
@@ -107,11 +107,6 @@ struct nvec_msg {
* struct nvec_chip - A single connection to an NVIDIA Embedded controller
* @dev: The device
* @gpio: The same as for &struct nvec_platform_data
- * @irq: The IRQ of the I2C device
- * @i2c_addr: The address of the I2C slave
- * @base: The base of the memory mapped region of the I2C device
- * @i2c_clk: The clock of the I2C device
- * @rst: The reset of the I2C device
* @notifier_list: Notifiers to be called on received messages, see
* nvec_register_notifier()
* @rx_data: Received messages that have to be processed
@@ -137,11 +132,6 @@ struct nvec_msg {
struct nvec_chip {
struct device *dev;
int gpio;
- int irq;
- int i2c_addr;
- void __iomem *base;
- struct clk *i2c_clk;
- struct reset_control *rst;
struct atomic_notifier_head notifier_list;
struct list_head rx_data, tx_data;
struct notifier_block nvec_status_notifier;
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH v2 2/4] staging/nvec: reimplement on top of tegra i2c driver
@ 2015-03-30 20:00 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100
Cc: Andrey Danin, Laxman Dewangan, Wolfram Sang, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
Remove i2c controller related code and use tegra i2c driver in slave mode.
Update nvec documentation.
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- remove extra new line
- keep old functions to simplify review
- move nvec_state enum to nvec.c
- use nvec-slave instead of nvec in dts to keep ABI compatibility
- rebased on top of new i2c slave framework
- delay workaround moved to tegra-i2c
- documentation patch is integrated in this commit
- reverted a log message to minimize changes
---
.../devicetree/bindings/nvec/nvidia,nvec.txt | 21 +-
drivers/staging/nvec/nvec.c | 258 +++++++++++++--------
drivers/staging/nvec/nvec.h | 10 -
3 files changed, 169 insertions(+), 120 deletions(-)
diff --git a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt
index 5ae601e..aba34095 100644
--- a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt
+++ b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt
@@ -1,21 +1,6 @@
NVIDIA compliant embedded controller
Required properties:
-- compatible : should be "nvidia,nvec".
-- reg : the iomem of the i2c slave controller
-- interrupts : the interrupt line of the i2c slave controller
-- clock-frequency : the frequency of the i2c bus
-- gpios : the gpio used for ec request
-- slave-addr: the i2c address of the slave controller
-- clocks : Must contain an entry for each entry in clock-names.
- See ../clocks/clock-bindings.txt for details.
-- clock-names : Must include the following entries:
- Tegra20/Tegra30:
- - div-clk
- - fast-clk
- Tegra114:
- - div-clk
-- resets : Must contain an entry for each entry in reset-names.
- See ../reset/reset.txt for details.
-- reset-names : Must include the following entries:
- - i2c
+- compatible : should be "nvidia,nvec-slave".
+- reg: the i2c address of the slave controller
+- request-gpios : the gpio used for ec request
diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index 5868ebb..f4c5527 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -25,8 +25,8 @@
#include <linux/err.h>
#include <linux/gpio.h>
#include <linux/interrupt.h>
+#include <linux/i2c.h>
#include <linux/io.h>
-#include <linux/irq.h>
#include <linux/of.h>
#include <linux/of_gpio.h>
#include <linux/list.h>
@@ -39,25 +39,19 @@
#include "nvec.h"
-#define I2C_CNFG 0x00
-#define I2C_CNFG_PACKET_MODE_EN (1<<10)
-#define I2C_CNFG_NEW_MASTER_SFM (1<<11)
-#define I2C_CNFG_DEBOUNCE_CNT_SHIFT 12
-
-#define I2C_SL_CNFG 0x20
-#define I2C_SL_NEWSL (1<<2)
-#define I2C_SL_NACK (1<<1)
-#define I2C_SL_RESP (1<<0)
-#define I2C_SL_IRQ (1<<3)
-#define END_TRANS (1<<4)
-#define RCVD (1<<2)
-#define RNW (1<<1)
-
-#define I2C_SL_RCVD 0x24
-#define I2C_SL_STATUS 0x28
-#define I2C_SL_ADDR1 0x2c
-#define I2C_SL_ADDR2 0x30
-#define I2C_SL_DELAY_COUNT 0x3c
+
+#define I2C_SL_ST_END_TRANS (1<<4)
+#define I2C_SL_ST_IRQ (1<<3)
+#define I2C_SL_ST_RCVD (1<<2)
+#define I2C_SL_ST_RNW (1<<1)
+
+
+enum nvec_state {
+ ST_NONE,
+ ST_RX,
+ ST_TX,
+ ST_TRANS_START,
+};
/**
* enum nvec_msg_category - Message categories for nvec_msg_alloc()
@@ -479,7 +473,7 @@ static void nvec_tx_completed(struct nvec_chip *nvec)
nvec->tx->pos = 0;
nvec_gpio_set_value(nvec, 0);
} else {
- nvec->state = 0;
+ nvec->state = ST_NONE;
}
}
@@ -497,7 +491,7 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
(uint) nvec->rx->pos);
nvec_msg_free(nvec, nvec->rx);
- nvec->state = 0;
+ nvec->state = ST_NONE;
/* Battery quirk - Often incomplete, and likes to crash */
if (nvec->rx->data[0] == NVEC_BAT)
@@ -514,7 +508,7 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
spin_unlock(&nvec->rx_lock);
- nvec->state = 0;
+ nvec->state = ST_NONE;
if (!nvec_msg_is_event(nvec->rx))
complete(&nvec->ec_transfer);
@@ -522,6 +516,7 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
schedule_work(&nvec->rx_work);
}
+#if 0
/**
* nvec_invalid_flags - Send an error message about invalid flags and jump
* @nvec: The nvec device
@@ -536,6 +531,7 @@ static void nvec_invalid_flags(struct nvec_chip *nvec, unsigned int status,
if (reset)
nvec->state = 0;
}
+#endif /* FIXME: remove old code */
/**
* nvec_tx_set - Set the message to transfer (nvec->tx)
@@ -566,6 +562,8 @@ static void nvec_tx_set(struct nvec_chip *nvec)
(uint)nvec->tx->size, nvec->tx->data[1]);
}
+
+#if 0
/**
* nvec_interrupt - Interrupt handler
* @irq: The IRQ
@@ -755,6 +753,115 @@ static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
clk_disable_unprepare(nvec->i2c_clk);
}
#endif
+#endif /* FIXME: remove old code. */
+
+
+/**
+ * nvec_slave_cb - I2C slave callback
+ *
+ * This callback fills our RX buffers and empties our TX
+ * buffers. This uses a finite state machine.
+ */
+static int nvec_slave_cb(struct i2c_client *client,
+ enum i2c_slave_event event, u8 *val)
+{
+ struct nvec_chip *nvec = i2c_get_clientdata(client);
+
+ switch (event) {
+ case I2C_SLAVE_WRITE_REQUESTED:
+ /* Alloc new msg only if prev transaction finished */
+ if (nvec->state == ST_NONE)
+ nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
+
+ /* Should not happen in a normal world */
+ if (unlikely(nvec->rx == NULL)) {
+ nvec->state = ST_NONE;
+ return -1;
+ }
+ nvec->rx->pos = 0;
+
+ if (client->addr != ((*val) >> 1)) {
+ dev_err(&client->dev,
+ "received address 0x%02x, expected 0x%02x\n",
+ ((*val) >> 1), client->addr);
+ return -1;
+ }
+ nvec->state = ST_TRANS_START;
+ break;
+
+ case I2C_SLAVE_WRITE_RECEIVED:
+ if (nvec->state != ST_TRANS_START &&
+ nvec->state != ST_RX) {
+ dev_err(&client->dev,
+ "unexpected write: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ if (nvec->rx->pos >= NVEC_MSG_SIZE) {
+ dev_err(&client->dev,
+ "write buffer overflow: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ nvec->rx->data[nvec->rx->pos++] = *val;
+ nvec->state = ST_RX;
+ break;
+
+ case I2C_SLAVE_READ_REQUESTED:
+ if (nvec->state != ST_RX) {
+ dev_err(&client->dev,
+ "unexpected read request: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ nvec_msg_free(nvec, nvec->rx);
+ nvec_tx_set(nvec);
+ /* Should not happen in a normal world */
+ if (!nvec->tx) {
+ dev_err(&client->dev,
+ "can't alloc tx msg: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ nvec_gpio_set_value(nvec, 1);
+ /* fallthrough */
+ case I2C_SLAVE_READ_PROCESSED:
+ if (nvec->state != ST_RX &&
+ nvec->state != ST_TX) {
+ dev_err(&client->dev,
+ "unexpected read: state %d\n",
+ nvec->state);
+ return -1;
+ }
+
+ if (!nvec->tx || nvec->tx->pos >= nvec->tx->size) {
+ dev_err(nvec->dev,
+ "tx buffer underflow on %p (%u > %u)\n",
+ nvec->tx,
+ (uint) (nvec->tx ? nvec->tx->pos : 0),
+ (uint) (nvec->tx ? nvec->tx->size : 0));
+ nvec->state = ST_NONE;
+ break;
+ }
+
+ nvec->state = ST_TX;
+ *val = nvec->tx->data[nvec->tx->pos++];
+ break;
+
+ case I2C_SLAVE_STOP:
+ if (nvec->state == ST_TX)
+ nvec_tx_completed(nvec);
+ else if (nvec->state == ST_RX)
+ nvec_rx_completed(nvec);
+ nvec->state = ST_NONE;
+ break;
+
+ default:
+ return 0;
+ }
+
+ return 0;
+}
static void nvec_power_off(void)
{
@@ -767,7 +874,7 @@ static void nvec_power_off(void)
/*
* Parse common device tree data
*/
-static int nvec_i2c_parse_dt_pdata(struct nvec_chip *nvec)
+static int nvec_i2c_parse_dt(struct nvec_chip *nvec)
{
nvec->gpio = of_get_named_gpio(nvec->dev->of_node, "request-gpios", 0);
@@ -776,68 +883,39 @@ static int nvec_i2c_parse_dt_pdata(struct nvec_chip *nvec)
return -ENODEV;
}
- if (of_property_read_u32(nvec->dev->of_node, "slave-addr",
- &nvec->i2c_addr)) {
- dev_err(nvec->dev, "no i2c address specified");
- return -ENODEV;
- }
-
return 0;
}
-static int tegra_nvec_probe(struct platform_device *pdev)
+static int nvec_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
- int err, ret;
- struct clk *i2c_clk;
+ int ret;
struct nvec_chip *nvec;
struct nvec_msg *msg;
- struct resource *res;
- void __iomem *base;
- char get_firmware_version[] = { NVEC_CNTL, GET_FIRMWARE_VERSION },
+ char get_firmware_version[] = { NVEC_CNTL, GET_FIRMWARE_VERSION },
unmute_speakers[] = { NVEC_OEM0, 0x10, 0x59, 0x95 },
enable_event[7] = { NVEC_SYS, CNF_EVENT_REPORTING, true };
- if (!pdev->dev.of_node) {
- dev_err(&pdev->dev, "must be instantiated using device tree\n");
+
+ if (!client->dev.of_node) {
+ dev_err(&client->dev, "must be instantiated using device tree\n");
return -ENODEV;
}
- nvec = devm_kzalloc(&pdev->dev, sizeof(struct nvec_chip), GFP_KERNEL);
+ nvec = devm_kzalloc(&client->dev, sizeof(struct nvec_chip), GFP_KERNEL);
if (nvec == NULL)
return -ENOMEM;
- platform_set_drvdata(pdev, nvec);
- nvec->dev = &pdev->dev;
+ nvec->dev = &client->dev;
+ ret = nvec_i2c_parse_dt(nvec);
+ if (ret < 0)
+ return ret;
- err = nvec_i2c_parse_dt_pdata(nvec);
- if (err < 0)
- return err;
+ i2c_set_clientdata(client, nvec);
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(base))
- return PTR_ERR(base);
+ ret = i2c_slave_register(client, nvec_slave_cb);
+ if (ret < 0)
+ return ret;
- nvec->irq = platform_get_irq(pdev, 0);
- if (nvec->irq < 0) {
- dev_err(&pdev->dev, "no irq resource?\n");
- return -ENODEV;
- }
-
- i2c_clk = devm_clk_get(&pdev->dev, "div-clk");
- if (IS_ERR(i2c_clk)) {
- dev_err(nvec->dev, "failed to get controller clock\n");
- return -ENODEV;
- }
-
- nvec->rst = devm_reset_control_get(&pdev->dev, "i2c");
- if (IS_ERR(nvec->rst)) {
- dev_err(nvec->dev, "failed to get controller reset\n");
- return PTR_ERR(nvec->rst);
- }
-
- nvec->base = base;
- nvec->i2c_clk = i2c_clk;
nvec->rx = &nvec->msg_pool[0];
ATOMIC_INIT_NOTIFIER_HEAD(&nvec->notifier_list);
@@ -852,23 +930,14 @@ static int tegra_nvec_probe(struct platform_device *pdev)
INIT_WORK(&nvec->rx_work, nvec_dispatch);
INIT_WORK(&nvec->tx_work, nvec_request_master);
- err = devm_gpio_request_one(&pdev->dev, nvec->gpio, GPIOF_OUT_INIT_HIGH,
- "nvec gpio");
- if (err < 0) {
+ ret = devm_gpio_request_one(&client->dev, nvec->gpio,
+ GPIOF_OUT_INIT_HIGH,
+ "nvec gpio");
+ if (ret < 0) {
dev_err(nvec->dev, "couldn't request gpio\n");
return -ENODEV;
}
- err = devm_request_irq(&pdev->dev, nvec->irq, nvec_interrupt, 0,
- "nvec", nvec);
- if (err) {
- dev_err(nvec->dev, "couldn't request irq\n");
- return -ENODEV;
- }
- disable_irq(nvec->irq);
-
- tegra_init_i2c_slave(nvec);
-
/* enable event reporting */
nvec_toggle_global_events(nvec, true);
@@ -907,9 +976,10 @@ static int tegra_nvec_probe(struct platform_device *pdev)
return 0;
}
-static int tegra_nvec_remove(struct platform_device *pdev)
+
+static int nvec_remove(struct i2c_client *client)
{
- struct nvec_chip *nvec = platform_get_drvdata(pdev);
+ struct nvec_chip *nvec = i2c_get_clientdata(client);
nvec_toggle_global_events(nvec, false);
mfd_remove_devices(nvec->dev);
@@ -938,8 +1008,6 @@ static int nvec_suspend(struct device *dev)
msg = nvec_write_sync(nvec, ap_suspend, sizeof(ap_suspend));
nvec_msg_free(nvec, msg);
- nvec_disable_i2c_slave(nvec);
-
return 0;
}
@@ -949,7 +1017,6 @@ static int nvec_resume(struct device *dev)
struct nvec_chip *nvec = platform_get_drvdata(pdev);
dev_dbg(nvec->dev, "resuming\n");
- tegra_init_i2c_slave(nvec);
nvec_toggle_global_events(nvec, true);
return 0;
@@ -960,14 +1027,21 @@ static SIMPLE_DEV_PM_OPS(nvec_pm_ops, nvec_suspend, nvec_resume);
/* Match table for of_platform binding */
static const struct of_device_id nvidia_nvec_of_match[] = {
- { .compatible = "nvidia,nvec", },
+ { .compatible = "nvidia,nvec-slave", },
{},
};
MODULE_DEVICE_TABLE(of, nvidia_nvec_of_match);
-static struct platform_driver nvec_device_driver = {
- .probe = tegra_nvec_probe,
- .remove = tegra_nvec_remove,
+static const struct i2c_device_id nvidia_nvec_i2c_table[] = {
+ { "nvec-slave", 0 },
+ {},
+};
+MODULE_DEVICE_TABLE(i2c, nvidia_nvec_i2c_table);
+
+static struct i2c_driver i2c_nvec_device_driver = {
+ .probe = nvec_probe,
+ .remove = nvec_remove,
+ .id_table = nvidia_nvec_i2c_table,
.driver = {
.name = "nvec",
.pm = &nvec_pm_ops,
@@ -975,9 +1049,9 @@ static struct platform_driver nvec_device_driver = {
}
};
-module_platform_driver(nvec_device_driver);
+module_i2c_driver(i2c_nvec_device_driver);
-MODULE_ALIAS("platform:nvec");
+MODULE_ALIAS("i2c:nvec");
MODULE_DESCRIPTION("NVIDIA compliant embedded controller interface");
MODULE_AUTHOR("Marc Dietrich <marvin24@gmx.de>");
MODULE_LICENSE("GPL");
diff --git a/drivers/staging/nvec/nvec.h b/drivers/staging/nvec/nvec.h
index e271375..ad6a556 100644
--- a/drivers/staging/nvec/nvec.h
+++ b/drivers/staging/nvec/nvec.h
@@ -107,11 +107,6 @@ struct nvec_msg {
* struct nvec_chip - A single connection to an NVIDIA Embedded controller
* @dev: The device
* @gpio: The same as for &struct nvec_platform_data
- * @irq: The IRQ of the I2C device
- * @i2c_addr: The address of the I2C slave
- * @base: The base of the memory mapped region of the I2C device
- * @i2c_clk: The clock of the I2C device
- * @rst: The reset of the I2C device
* @notifier_list: Notifiers to be called on received messages, see
* nvec_register_notifier()
* @rx_data: Received messages that have to be processed
@@ -137,11 +132,6 @@ struct nvec_msg {
struct nvec_chip {
struct device *dev;
int gpio;
- int irq;
- int i2c_addr;
- void __iomem *base;
- struct clk *i2c_clk;
- struct reset_control *rst;
struct atomic_notifier_head notifier_list;
struct list_head rx_data, tx_data;
struct notifier_block nvec_status_notifier;
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH v2 2/4] staging/nvec: reimplement on top of tegra i2c driver
@ 2015-03-30 20:00 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: linux-arm-kernel
Remove i2c controller related code and use tegra i2c driver in slave mode.
Update nvec documentation.
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- remove extra new line
- keep old functions to simplify review
- move nvec_state enum to nvec.c
- use nvec-slave instead of nvec in dts to keep ABI compatibility
- rebased on top of new i2c slave framework
- delay workaround moved to tegra-i2c
- documentation patch is integrated in this commit
- reverted a log message to minimize changes
---
.../devicetree/bindings/nvec/nvidia,nvec.txt | 21 +-
drivers/staging/nvec/nvec.c | 258 +++++++++++++--------
drivers/staging/nvec/nvec.h | 10 -
3 files changed, 169 insertions(+), 120 deletions(-)
diff --git a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt
index 5ae601e..aba34095 100644
--- a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt
+++ b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt
@@ -1,21 +1,6 @@
NVIDIA compliant embedded controller
Required properties:
-- compatible : should be "nvidia,nvec".
-- reg : the iomem of the i2c slave controller
-- interrupts : the interrupt line of the i2c slave controller
-- clock-frequency : the frequency of the i2c bus
-- gpios : the gpio used for ec request
-- slave-addr: the i2c address of the slave controller
-- clocks : Must contain an entry for each entry in clock-names.
- See ../clocks/clock-bindings.txt for details.
-- clock-names : Must include the following entries:
- Tegra20/Tegra30:
- - div-clk
- - fast-clk
- Tegra114:
- - div-clk
-- resets : Must contain an entry for each entry in reset-names.
- See ../reset/reset.txt for details.
-- reset-names : Must include the following entries:
- - i2c
+- compatible : should be "nvidia,nvec-slave".
+- reg: the i2c address of the slave controller
+- request-gpios : the gpio used for ec request
diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index 5868ebb..f4c5527 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -25,8 +25,8 @@
#include <linux/err.h>
#include <linux/gpio.h>
#include <linux/interrupt.h>
+#include <linux/i2c.h>
#include <linux/io.h>
-#include <linux/irq.h>
#include <linux/of.h>
#include <linux/of_gpio.h>
#include <linux/list.h>
@@ -39,25 +39,19 @@
#include "nvec.h"
-#define I2C_CNFG 0x00
-#define I2C_CNFG_PACKET_MODE_EN (1<<10)
-#define I2C_CNFG_NEW_MASTER_SFM (1<<11)
-#define I2C_CNFG_DEBOUNCE_CNT_SHIFT 12
-
-#define I2C_SL_CNFG 0x20
-#define I2C_SL_NEWSL (1<<2)
-#define I2C_SL_NACK (1<<1)
-#define I2C_SL_RESP (1<<0)
-#define I2C_SL_IRQ (1<<3)
-#define END_TRANS (1<<4)
-#define RCVD (1<<2)
-#define RNW (1<<1)
-
-#define I2C_SL_RCVD 0x24
-#define I2C_SL_STATUS 0x28
-#define I2C_SL_ADDR1 0x2c
-#define I2C_SL_ADDR2 0x30
-#define I2C_SL_DELAY_COUNT 0x3c
+
+#define I2C_SL_ST_END_TRANS (1<<4)
+#define I2C_SL_ST_IRQ (1<<3)
+#define I2C_SL_ST_RCVD (1<<2)
+#define I2C_SL_ST_RNW (1<<1)
+
+
+enum nvec_state {
+ ST_NONE,
+ ST_RX,
+ ST_TX,
+ ST_TRANS_START,
+};
/**
* enum nvec_msg_category - Message categories for nvec_msg_alloc()
@@ -479,7 +473,7 @@ static void nvec_tx_completed(struct nvec_chip *nvec)
nvec->tx->pos = 0;
nvec_gpio_set_value(nvec, 0);
} else {
- nvec->state = 0;
+ nvec->state = ST_NONE;
}
}
@@ -497,7 +491,7 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
(uint) nvec->rx->pos);
nvec_msg_free(nvec, nvec->rx);
- nvec->state = 0;
+ nvec->state = ST_NONE;
/* Battery quirk - Often incomplete, and likes to crash */
if (nvec->rx->data[0] == NVEC_BAT)
@@ -514,7 +508,7 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
spin_unlock(&nvec->rx_lock);
- nvec->state = 0;
+ nvec->state = ST_NONE;
if (!nvec_msg_is_event(nvec->rx))
complete(&nvec->ec_transfer);
@@ -522,6 +516,7 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
schedule_work(&nvec->rx_work);
}
+#if 0
/**
* nvec_invalid_flags - Send an error message about invalid flags and jump
* @nvec: The nvec device
@@ -536,6 +531,7 @@ static void nvec_invalid_flags(struct nvec_chip *nvec, unsigned int status,
if (reset)
nvec->state = 0;
}
+#endif /* FIXME: remove old code */
/**
* nvec_tx_set - Set the message to transfer (nvec->tx)
@@ -566,6 +562,8 @@ static void nvec_tx_set(struct nvec_chip *nvec)
(uint)nvec->tx->size, nvec->tx->data[1]);
}
+
+#if 0
/**
* nvec_interrupt - Interrupt handler
* @irq: The IRQ
@@ -755,6 +753,115 @@ static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
clk_disable_unprepare(nvec->i2c_clk);
}
#endif
+#endif /* FIXME: remove old code. */
+
+
+/**
+ * nvec_slave_cb - I2C slave callback
+ *
+ * This callback fills our RX buffers and empties our TX
+ * buffers. This uses a finite state machine.
+ */
+static int nvec_slave_cb(struct i2c_client *client,
+ enum i2c_slave_event event, u8 *val)
+{
+ struct nvec_chip *nvec = i2c_get_clientdata(client);
+
+ switch (event) {
+ case I2C_SLAVE_WRITE_REQUESTED:
+ /* Alloc new msg only if prev transaction finished */
+ if (nvec->state == ST_NONE)
+ nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
+
+ /* Should not happen in a normal world */
+ if (unlikely(nvec->rx == NULL)) {
+ nvec->state = ST_NONE;
+ return -1;
+ }
+ nvec->rx->pos = 0;
+
+ if (client->addr != ((*val) >> 1)) {
+ dev_err(&client->dev,
+ "received address 0x%02x, expected 0x%02x\n",
+ ((*val) >> 1), client->addr);
+ return -1;
+ }
+ nvec->state = ST_TRANS_START;
+ break;
+
+ case I2C_SLAVE_WRITE_RECEIVED:
+ if (nvec->state != ST_TRANS_START &&
+ nvec->state != ST_RX) {
+ dev_err(&client->dev,
+ "unexpected write: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ if (nvec->rx->pos >= NVEC_MSG_SIZE) {
+ dev_err(&client->dev,
+ "write buffer overflow: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ nvec->rx->data[nvec->rx->pos++] = *val;
+ nvec->state = ST_RX;
+ break;
+
+ case I2C_SLAVE_READ_REQUESTED:
+ if (nvec->state != ST_RX) {
+ dev_err(&client->dev,
+ "unexpected read request: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ nvec_msg_free(nvec, nvec->rx);
+ nvec_tx_set(nvec);
+ /* Should not happen in a normal world */
+ if (!nvec->tx) {
+ dev_err(&client->dev,
+ "can't alloc tx msg: state %d\n",
+ nvec->state);
+ return -1;
+ }
+ nvec_gpio_set_value(nvec, 1);
+ /* fallthrough */
+ case I2C_SLAVE_READ_PROCESSED:
+ if (nvec->state != ST_RX &&
+ nvec->state != ST_TX) {
+ dev_err(&client->dev,
+ "unexpected read: state %d\n",
+ nvec->state);
+ return -1;
+ }
+
+ if (!nvec->tx || nvec->tx->pos >= nvec->tx->size) {
+ dev_err(nvec->dev,
+ "tx buffer underflow on %p (%u > %u)\n",
+ nvec->tx,
+ (uint) (nvec->tx ? nvec->tx->pos : 0),
+ (uint) (nvec->tx ? nvec->tx->size : 0));
+ nvec->state = ST_NONE;
+ break;
+ }
+
+ nvec->state = ST_TX;
+ *val = nvec->tx->data[nvec->tx->pos++];
+ break;
+
+ case I2C_SLAVE_STOP:
+ if (nvec->state == ST_TX)
+ nvec_tx_completed(nvec);
+ else if (nvec->state == ST_RX)
+ nvec_rx_completed(nvec);
+ nvec->state = ST_NONE;
+ break;
+
+ default:
+ return 0;
+ }
+
+ return 0;
+}
static void nvec_power_off(void)
{
@@ -767,7 +874,7 @@ static void nvec_power_off(void)
/*
* Parse common device tree data
*/
-static int nvec_i2c_parse_dt_pdata(struct nvec_chip *nvec)
+static int nvec_i2c_parse_dt(struct nvec_chip *nvec)
{
nvec->gpio = of_get_named_gpio(nvec->dev->of_node, "request-gpios", 0);
@@ -776,68 +883,39 @@ static int nvec_i2c_parse_dt_pdata(struct nvec_chip *nvec)
return -ENODEV;
}
- if (of_property_read_u32(nvec->dev->of_node, "slave-addr",
- &nvec->i2c_addr)) {
- dev_err(nvec->dev, "no i2c address specified");
- return -ENODEV;
- }
-
return 0;
}
-static int tegra_nvec_probe(struct platform_device *pdev)
+static int nvec_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
- int err, ret;
- struct clk *i2c_clk;
+ int ret;
struct nvec_chip *nvec;
struct nvec_msg *msg;
- struct resource *res;
- void __iomem *base;
- char get_firmware_version[] = { NVEC_CNTL, GET_FIRMWARE_VERSION },
+ char get_firmware_version[] = { NVEC_CNTL, GET_FIRMWARE_VERSION },
unmute_speakers[] = { NVEC_OEM0, 0x10, 0x59, 0x95 },
enable_event[7] = { NVEC_SYS, CNF_EVENT_REPORTING, true };
- if (!pdev->dev.of_node) {
- dev_err(&pdev->dev, "must be instantiated using device tree\n");
+
+ if (!client->dev.of_node) {
+ dev_err(&client->dev, "must be instantiated using device tree\n");
return -ENODEV;
}
- nvec = devm_kzalloc(&pdev->dev, sizeof(struct nvec_chip), GFP_KERNEL);
+ nvec = devm_kzalloc(&client->dev, sizeof(struct nvec_chip), GFP_KERNEL);
if (nvec == NULL)
return -ENOMEM;
- platform_set_drvdata(pdev, nvec);
- nvec->dev = &pdev->dev;
+ nvec->dev = &client->dev;
+ ret = nvec_i2c_parse_dt(nvec);
+ if (ret < 0)
+ return ret;
- err = nvec_i2c_parse_dt_pdata(nvec);
- if (err < 0)
- return err;
+ i2c_set_clientdata(client, nvec);
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(base))
- return PTR_ERR(base);
+ ret = i2c_slave_register(client, nvec_slave_cb);
+ if (ret < 0)
+ return ret;
- nvec->irq = platform_get_irq(pdev, 0);
- if (nvec->irq < 0) {
- dev_err(&pdev->dev, "no irq resource?\n");
- return -ENODEV;
- }
-
- i2c_clk = devm_clk_get(&pdev->dev, "div-clk");
- if (IS_ERR(i2c_clk)) {
- dev_err(nvec->dev, "failed to get controller clock\n");
- return -ENODEV;
- }
-
- nvec->rst = devm_reset_control_get(&pdev->dev, "i2c");
- if (IS_ERR(nvec->rst)) {
- dev_err(nvec->dev, "failed to get controller reset\n");
- return PTR_ERR(nvec->rst);
- }
-
- nvec->base = base;
- nvec->i2c_clk = i2c_clk;
nvec->rx = &nvec->msg_pool[0];
ATOMIC_INIT_NOTIFIER_HEAD(&nvec->notifier_list);
@@ -852,23 +930,14 @@ static int tegra_nvec_probe(struct platform_device *pdev)
INIT_WORK(&nvec->rx_work, nvec_dispatch);
INIT_WORK(&nvec->tx_work, nvec_request_master);
- err = devm_gpio_request_one(&pdev->dev, nvec->gpio, GPIOF_OUT_INIT_HIGH,
- "nvec gpio");
- if (err < 0) {
+ ret = devm_gpio_request_one(&client->dev, nvec->gpio,
+ GPIOF_OUT_INIT_HIGH,
+ "nvec gpio");
+ if (ret < 0) {
dev_err(nvec->dev, "couldn't request gpio\n");
return -ENODEV;
}
- err = devm_request_irq(&pdev->dev, nvec->irq, nvec_interrupt, 0,
- "nvec", nvec);
- if (err) {
- dev_err(nvec->dev, "couldn't request irq\n");
- return -ENODEV;
- }
- disable_irq(nvec->irq);
-
- tegra_init_i2c_slave(nvec);
-
/* enable event reporting */
nvec_toggle_global_events(nvec, true);
@@ -907,9 +976,10 @@ static int tegra_nvec_probe(struct platform_device *pdev)
return 0;
}
-static int tegra_nvec_remove(struct platform_device *pdev)
+
+static int nvec_remove(struct i2c_client *client)
{
- struct nvec_chip *nvec = platform_get_drvdata(pdev);
+ struct nvec_chip *nvec = i2c_get_clientdata(client);
nvec_toggle_global_events(nvec, false);
mfd_remove_devices(nvec->dev);
@@ -938,8 +1008,6 @@ static int nvec_suspend(struct device *dev)
msg = nvec_write_sync(nvec, ap_suspend, sizeof(ap_suspend));
nvec_msg_free(nvec, msg);
- nvec_disable_i2c_slave(nvec);
-
return 0;
}
@@ -949,7 +1017,6 @@ static int nvec_resume(struct device *dev)
struct nvec_chip *nvec = platform_get_drvdata(pdev);
dev_dbg(nvec->dev, "resuming\n");
- tegra_init_i2c_slave(nvec);
nvec_toggle_global_events(nvec, true);
return 0;
@@ -960,14 +1027,21 @@ static SIMPLE_DEV_PM_OPS(nvec_pm_ops, nvec_suspend, nvec_resume);
/* Match table for of_platform binding */
static const struct of_device_id nvidia_nvec_of_match[] = {
- { .compatible = "nvidia,nvec", },
+ { .compatible = "nvidia,nvec-slave", },
{},
};
MODULE_DEVICE_TABLE(of, nvidia_nvec_of_match);
-static struct platform_driver nvec_device_driver = {
- .probe = tegra_nvec_probe,
- .remove = tegra_nvec_remove,
+static const struct i2c_device_id nvidia_nvec_i2c_table[] = {
+ { "nvec-slave", 0 },
+ {},
+};
+MODULE_DEVICE_TABLE(i2c, nvidia_nvec_i2c_table);
+
+static struct i2c_driver i2c_nvec_device_driver = {
+ .probe = nvec_probe,
+ .remove = nvec_remove,
+ .id_table = nvidia_nvec_i2c_table,
.driver = {
.name = "nvec",
.pm = &nvec_pm_ops,
@@ -975,9 +1049,9 @@ static struct platform_driver nvec_device_driver = {
}
};
-module_platform_driver(nvec_device_driver);
+module_i2c_driver(i2c_nvec_device_driver);
-MODULE_ALIAS("platform:nvec");
+MODULE_ALIAS("i2c:nvec");
MODULE_DESCRIPTION("NVIDIA compliant embedded controller interface");
MODULE_AUTHOR("Marc Dietrich <marvin24@gmx.de>");
MODULE_LICENSE("GPL");
diff --git a/drivers/staging/nvec/nvec.h b/drivers/staging/nvec/nvec.h
index e271375..ad6a556 100644
--- a/drivers/staging/nvec/nvec.h
+++ b/drivers/staging/nvec/nvec.h
@@ -107,11 +107,6 @@ struct nvec_msg {
* struct nvec_chip - A single connection to an NVIDIA Embedded controller
* @dev: The device
* @gpio: The same as for &struct nvec_platform_data
- * @irq: The IRQ of the I2C device
- * @i2c_addr: The address of the I2C slave
- * @base: The base of the memory mapped region of the I2C device
- * @i2c_clk: The clock of the I2C device
- * @rst: The reset of the I2C device
* @notifier_list: Notifiers to be called on received messages, see
* nvec_register_notifier()
* @rx_data: Received messages that have to be processed
@@ -137,11 +132,6 @@ struct nvec_msg {
struct nvec_chip {
struct device *dev;
int gpio;
- int irq;
- int i2c_addr;
- void __iomem *base;
- struct clk *i2c_clk;
- struct reset_control *rst;
struct atomic_notifier_head notifier_list;
struct list_head rx_data, tx_data;
struct notifier_block nvec_status_notifier;
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH v2 3/4] staging/nvec: remove old code
2015-03-30 20:00 ` Andrey Danin
(?)
@ 2015-03-30 20:00 ` Andrey Danin
-1 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100
Cc: Mark Rutland, Alexandre Courbot, Russell King, Pawel Moll,
Wolfram Sang, Julian Andres Klode, Greg Kroah-Hartman,
Ian Campbell, Rob Herring, Marc Dietrich, Laxman Dewangan,
Thierry Reding, Kumar Gala, Stephen Warren, Andrey Danin
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- it is initial verion
---
drivers/staging/nvec/nvec.c | 211 --------------------------------------------
1 file changed, 211 deletions(-)
diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index f4c5527..f39f516 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -516,23 +516,6 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
schedule_work(&nvec->rx_work);
}
-#if 0
-/**
- * nvec_invalid_flags - Send an error message about invalid flags and jump
- * @nvec: The nvec device
- * @status: The status flags
- * @reset: Whether we shall jump to state 0.
- */
-static void nvec_invalid_flags(struct nvec_chip *nvec, unsigned int status,
- bool reset)
-{
- dev_err(nvec->dev, "unexpected status flags 0x%02x during state %i\n",
- status, nvec->state);
- if (reset)
- nvec->state = 0;
-}
-#endif /* FIXME: remove old code */
-
/**
* nvec_tx_set - Set the message to transfer (nvec->tx)
* @nvec: A &struct nvec_chip
@@ -562,200 +545,6 @@ static void nvec_tx_set(struct nvec_chip *nvec)
(uint)nvec->tx->size, nvec->tx->data[1]);
}
-
-#if 0
-/**
- * nvec_interrupt - Interrupt handler
- * @irq: The IRQ
- * @dev: The nvec device
- *
- * Interrupt handler that fills our RX buffers and empties our TX
- * buffers. This uses a finite state machine with ridiculous amounts
- * of error checking, in order to be fairly reliable.
- */
-static irqreturn_t nvec_interrupt(int irq, void *dev)
-{
- unsigned long status;
- unsigned int received = 0;
- unsigned char to_send = 0xff;
- const unsigned long irq_mask = I2C_SL_IRQ | END_TRANS | RCVD | RNW;
- struct nvec_chip *nvec = dev;
- unsigned int state = nvec->state;
-
- status = readl(nvec->base + I2C_SL_STATUS);
-
- /* Filter out some errors */
- if ((status & irq_mask) == 0 && (status & ~irq_mask) != 0) {
- dev_err(nvec->dev, "unexpected irq mask %lx\n", status);
- return IRQ_HANDLED;
- }
- if ((status & I2C_SL_IRQ) == 0) {
- dev_err(nvec->dev, "Spurious IRQ\n");
- return IRQ_HANDLED;
- }
-
- /* The EC did not request a read, so it send us something, read it */
- if ((status & RNW) == 0) {
- received = readl(nvec->base + I2C_SL_RCVD);
- if (status & RCVD)
- writel(0, nvec->base + I2C_SL_RCVD);
- }
-
- if (status == (I2C_SL_IRQ | RCVD))
- nvec->state = 0;
-
- switch (nvec->state) {
- case 0: /* Verify that its a transfer start, the rest later */
- if (status != (I2C_SL_IRQ | RCVD))
- nvec_invalid_flags(nvec, status, false);
- break;
- case 1: /* command byte */
- if (status != I2C_SL_IRQ) {
- nvec_invalid_flags(nvec, status, true);
- } else {
- nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
- /* Should not happen in a normal world */
- if (unlikely(nvec->rx == NULL)) {
- nvec->state = 0;
- break;
- }
- nvec->rx->data[0] = received;
- nvec->rx->pos = 1;
- nvec->state = 2;
- }
- break;
- case 2: /* first byte after command */
- if (status == (I2C_SL_IRQ | RNW | RCVD)) {
- udelay(33);
- if (nvec->rx->data[0] != 0x01) {
- dev_err(nvec->dev,
- "Read without prior read command\n");
- nvec->state = 0;
- break;
- }
- nvec_msg_free(nvec, nvec->rx);
- nvec->state = 3;
- nvec_tx_set(nvec);
- BUG_ON(nvec->tx->size < 1);
- to_send = nvec->tx->data[0];
- nvec->tx->pos = 1;
- } else if (status == (I2C_SL_IRQ)) {
- BUG_ON(nvec->rx == NULL);
- nvec->rx->data[1] = received;
- nvec->rx->pos = 2;
- nvec->state = 4;
- } else {
- nvec_invalid_flags(nvec, status, true);
- }
- break;
- case 3: /* EC does a block read, we transmit data */
- if (status & END_TRANS) {
- nvec_tx_completed(nvec);
- } else if ((status & RNW) == 0 || (status & RCVD)) {
- nvec_invalid_flags(nvec, status, true);
- } else if (nvec->tx && nvec->tx->pos < nvec->tx->size) {
- to_send = nvec->tx->data[nvec->tx->pos++];
- } else {
- dev_err(nvec->dev, "tx buffer underflow on %p (%u > %u)\n",
- nvec->tx,
- (uint) (nvec->tx ? nvec->tx->pos : 0),
- (uint) (nvec->tx ? nvec->tx->size : 0));
- nvec->state = 0;
- }
- break;
- case 4: /* EC does some write, we read the data */
- if ((status & (END_TRANS | RNW)) == END_TRANS)
- nvec_rx_completed(nvec);
- else if (status & (RNW | RCVD))
- nvec_invalid_flags(nvec, status, true);
- else if (nvec->rx && nvec->rx->pos < NVEC_MSG_SIZE)
- nvec->rx->data[nvec->rx->pos++] = received;
- else
- dev_err(nvec->dev,
- "RX buffer overflow on %p: Trying to write byte %u of %u\n",
- nvec->rx, nvec->rx ? nvec->rx->pos : 0,
- NVEC_MSG_SIZE);
- break;
- default:
- nvec->state = 0;
- }
-
- /* If we are told that a new transfer starts, verify it */
- if ((status & (RCVD | RNW)) == RCVD) {
- if (received != nvec->i2c_addr)
- dev_err(nvec->dev,
- "received address 0x%02x, expected 0x%02x\n",
- received, nvec->i2c_addr);
- nvec->state = 1;
- }
-
- /* Send data if requested, but not on end of transmission */
- if ((status & (RNW | END_TRANS)) == RNW)
- writel(to_send, nvec->base + I2C_SL_RCVD);
-
- /* If we have send the first byte */
- if (status == (I2C_SL_IRQ | RNW | RCVD))
- nvec_gpio_set_value(nvec, 1);
-
- dev_dbg(nvec->dev,
- "Handled: %s 0x%02x, %s 0x%02x in state %u [%s%s%s]\n",
- (status & RNW) == 0 ? "received" : "R=",
- received,
- (status & (RNW | END_TRANS)) ? "sent" : "S=",
- to_send,
- state,
- status & END_TRANS ? " END_TRANS" : "",
- status & RCVD ? " RCVD" : "",
- status & RNW ? " RNW" : "");
-
-
- /*
- * TODO: A correct fix needs to be found for this.
- *
- * We experience less incomplete messages with this delay than without
- * it, but we don't know why. Help is appreciated.
- */
- udelay(100);
-
- return IRQ_HANDLED;
-}
-
-static void tegra_init_i2c_slave(struct nvec_chip *nvec)
-{
- u32 val;
-
- clk_prepare_enable(nvec->i2c_clk);
-
- reset_control_assert(nvec->rst);
- udelay(2);
- reset_control_deassert(nvec->rst);
-
- val = I2C_CNFG_NEW_MASTER_SFM | I2C_CNFG_PACKET_MODE_EN |
- (0x2 << I2C_CNFG_DEBOUNCE_CNT_SHIFT);
- writel(val, nvec->base + I2C_CNFG);
-
- clk_set_rate(nvec->i2c_clk, 8 * 80000);
-
- writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
- writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);
-
- writel(nvec->i2c_addr>>1, nvec->base + I2C_SL_ADDR1);
- writel(0, nvec->base + I2C_SL_ADDR2);
-
- enable_irq(nvec->irq);
-}
-
-#ifdef CONFIG_PM_SLEEP
-static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
-{
- disable_irq(nvec->irq);
- writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
- clk_disable_unprepare(nvec->i2c_clk);
-}
-#endif
-#endif /* FIXME: remove old code. */
-
-
/**
* nvec_slave_cb - I2C slave callback
*
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH v2 3/4] staging/nvec: remove old code
@ 2015-03-30 20:00 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100
Cc: Andrey Danin, Laxman Dewangan, Wolfram Sang, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- it is initial verion
---
drivers/staging/nvec/nvec.c | 211 --------------------------------------------
1 file changed, 211 deletions(-)
diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index f4c5527..f39f516 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -516,23 +516,6 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
schedule_work(&nvec->rx_work);
}
-#if 0
-/**
- * nvec_invalid_flags - Send an error message about invalid flags and jump
- * @nvec: The nvec device
- * @status: The status flags
- * @reset: Whether we shall jump to state 0.
- */
-static void nvec_invalid_flags(struct nvec_chip *nvec, unsigned int status,
- bool reset)
-{
- dev_err(nvec->dev, "unexpected status flags 0x%02x during state %i\n",
- status, nvec->state);
- if (reset)
- nvec->state = 0;
-}
-#endif /* FIXME: remove old code */
-
/**
* nvec_tx_set - Set the message to transfer (nvec->tx)
* @nvec: A &struct nvec_chip
@@ -562,200 +545,6 @@ static void nvec_tx_set(struct nvec_chip *nvec)
(uint)nvec->tx->size, nvec->tx->data[1]);
}
-
-#if 0
-/**
- * nvec_interrupt - Interrupt handler
- * @irq: The IRQ
- * @dev: The nvec device
- *
- * Interrupt handler that fills our RX buffers and empties our TX
- * buffers. This uses a finite state machine with ridiculous amounts
- * of error checking, in order to be fairly reliable.
- */
-static irqreturn_t nvec_interrupt(int irq, void *dev)
-{
- unsigned long status;
- unsigned int received = 0;
- unsigned char to_send = 0xff;
- const unsigned long irq_mask = I2C_SL_IRQ | END_TRANS | RCVD | RNW;
- struct nvec_chip *nvec = dev;
- unsigned int state = nvec->state;
-
- status = readl(nvec->base + I2C_SL_STATUS);
-
- /* Filter out some errors */
- if ((status & irq_mask) == 0 && (status & ~irq_mask) != 0) {
- dev_err(nvec->dev, "unexpected irq mask %lx\n", status);
- return IRQ_HANDLED;
- }
- if ((status & I2C_SL_IRQ) == 0) {
- dev_err(nvec->dev, "Spurious IRQ\n");
- return IRQ_HANDLED;
- }
-
- /* The EC did not request a read, so it send us something, read it */
- if ((status & RNW) == 0) {
- received = readl(nvec->base + I2C_SL_RCVD);
- if (status & RCVD)
- writel(0, nvec->base + I2C_SL_RCVD);
- }
-
- if (status == (I2C_SL_IRQ | RCVD))
- nvec->state = 0;
-
- switch (nvec->state) {
- case 0: /* Verify that its a transfer start, the rest later */
- if (status != (I2C_SL_IRQ | RCVD))
- nvec_invalid_flags(nvec, status, false);
- break;
- case 1: /* command byte */
- if (status != I2C_SL_IRQ) {
- nvec_invalid_flags(nvec, status, true);
- } else {
- nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
- /* Should not happen in a normal world */
- if (unlikely(nvec->rx == NULL)) {
- nvec->state = 0;
- break;
- }
- nvec->rx->data[0] = received;
- nvec->rx->pos = 1;
- nvec->state = 2;
- }
- break;
- case 2: /* first byte after command */
- if (status == (I2C_SL_IRQ | RNW | RCVD)) {
- udelay(33);
- if (nvec->rx->data[0] != 0x01) {
- dev_err(nvec->dev,
- "Read without prior read command\n");
- nvec->state = 0;
- break;
- }
- nvec_msg_free(nvec, nvec->rx);
- nvec->state = 3;
- nvec_tx_set(nvec);
- BUG_ON(nvec->tx->size < 1);
- to_send = nvec->tx->data[0];
- nvec->tx->pos = 1;
- } else if (status == (I2C_SL_IRQ)) {
- BUG_ON(nvec->rx == NULL);
- nvec->rx->data[1] = received;
- nvec->rx->pos = 2;
- nvec->state = 4;
- } else {
- nvec_invalid_flags(nvec, status, true);
- }
- break;
- case 3: /* EC does a block read, we transmit data */
- if (status & END_TRANS) {
- nvec_tx_completed(nvec);
- } else if ((status & RNW) == 0 || (status & RCVD)) {
- nvec_invalid_flags(nvec, status, true);
- } else if (nvec->tx && nvec->tx->pos < nvec->tx->size) {
- to_send = nvec->tx->data[nvec->tx->pos++];
- } else {
- dev_err(nvec->dev, "tx buffer underflow on %p (%u > %u)\n",
- nvec->tx,
- (uint) (nvec->tx ? nvec->tx->pos : 0),
- (uint) (nvec->tx ? nvec->tx->size : 0));
- nvec->state = 0;
- }
- break;
- case 4: /* EC does some write, we read the data */
- if ((status & (END_TRANS | RNW)) == END_TRANS)
- nvec_rx_completed(nvec);
- else if (status & (RNW | RCVD))
- nvec_invalid_flags(nvec, status, true);
- else if (nvec->rx && nvec->rx->pos < NVEC_MSG_SIZE)
- nvec->rx->data[nvec->rx->pos++] = received;
- else
- dev_err(nvec->dev,
- "RX buffer overflow on %p: Trying to write byte %u of %u\n",
- nvec->rx, nvec->rx ? nvec->rx->pos : 0,
- NVEC_MSG_SIZE);
- break;
- default:
- nvec->state = 0;
- }
-
- /* If we are told that a new transfer starts, verify it */
- if ((status & (RCVD | RNW)) == RCVD) {
- if (received != nvec->i2c_addr)
- dev_err(nvec->dev,
- "received address 0x%02x, expected 0x%02x\n",
- received, nvec->i2c_addr);
- nvec->state = 1;
- }
-
- /* Send data if requested, but not on end of transmission */
- if ((status & (RNW | END_TRANS)) == RNW)
- writel(to_send, nvec->base + I2C_SL_RCVD);
-
- /* If we have send the first byte */
- if (status == (I2C_SL_IRQ | RNW | RCVD))
- nvec_gpio_set_value(nvec, 1);
-
- dev_dbg(nvec->dev,
- "Handled: %s 0x%02x, %s 0x%02x in state %u [%s%s%s]\n",
- (status & RNW) == 0 ? "received" : "R=",
- received,
- (status & (RNW | END_TRANS)) ? "sent" : "S=",
- to_send,
- state,
- status & END_TRANS ? " END_TRANS" : "",
- status & RCVD ? " RCVD" : "",
- status & RNW ? " RNW" : "");
-
-
- /*
- * TODO: A correct fix needs to be found for this.
- *
- * We experience less incomplete messages with this delay than without
- * it, but we don't know why. Help is appreciated.
- */
- udelay(100);
-
- return IRQ_HANDLED;
-}
-
-static void tegra_init_i2c_slave(struct nvec_chip *nvec)
-{
- u32 val;
-
- clk_prepare_enable(nvec->i2c_clk);
-
- reset_control_assert(nvec->rst);
- udelay(2);
- reset_control_deassert(nvec->rst);
-
- val = I2C_CNFG_NEW_MASTER_SFM | I2C_CNFG_PACKET_MODE_EN |
- (0x2 << I2C_CNFG_DEBOUNCE_CNT_SHIFT);
- writel(val, nvec->base + I2C_CNFG);
-
- clk_set_rate(nvec->i2c_clk, 8 * 80000);
-
- writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
- writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);
-
- writel(nvec->i2c_addr>>1, nvec->base + I2C_SL_ADDR1);
- writel(0, nvec->base + I2C_SL_ADDR2);
-
- enable_irq(nvec->irq);
-}
-
-#ifdef CONFIG_PM_SLEEP
-static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
-{
- disable_irq(nvec->irq);
- writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
- clk_disable_unprepare(nvec->i2c_clk);
-}
-#endif
-#endif /* FIXME: remove old code. */
-
-
/**
* nvec_slave_cb - I2C slave callback
*
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH v2 3/4] staging/nvec: remove old code
@ 2015-03-30 20:00 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- it is initial verion
---
drivers/staging/nvec/nvec.c | 211 --------------------------------------------
1 file changed, 211 deletions(-)
diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index f4c5527..f39f516 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -516,23 +516,6 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
schedule_work(&nvec->rx_work);
}
-#if 0
-/**
- * nvec_invalid_flags - Send an error message about invalid flags and jump
- * @nvec: The nvec device
- * @status: The status flags
- * @reset: Whether we shall jump to state 0.
- */
-static void nvec_invalid_flags(struct nvec_chip *nvec, unsigned int status,
- bool reset)
-{
- dev_err(nvec->dev, "unexpected status flags 0x%02x during state %i\n",
- status, nvec->state);
- if (reset)
- nvec->state = 0;
-}
-#endif /* FIXME: remove old code */
-
/**
* nvec_tx_set - Set the message to transfer (nvec->tx)
* @nvec: A &struct nvec_chip
@@ -562,200 +545,6 @@ static void nvec_tx_set(struct nvec_chip *nvec)
(uint)nvec->tx->size, nvec->tx->data[1]);
}
-
-#if 0
-/**
- * nvec_interrupt - Interrupt handler
- * @irq: The IRQ
- * @dev: The nvec device
- *
- * Interrupt handler that fills our RX buffers and empties our TX
- * buffers. This uses a finite state machine with ridiculous amounts
- * of error checking, in order to be fairly reliable.
- */
-static irqreturn_t nvec_interrupt(int irq, void *dev)
-{
- unsigned long status;
- unsigned int received = 0;
- unsigned char to_send = 0xff;
- const unsigned long irq_mask = I2C_SL_IRQ | END_TRANS | RCVD | RNW;
- struct nvec_chip *nvec = dev;
- unsigned int state = nvec->state;
-
- status = readl(nvec->base + I2C_SL_STATUS);
-
- /* Filter out some errors */
- if ((status & irq_mask) == 0 && (status & ~irq_mask) != 0) {
- dev_err(nvec->dev, "unexpected irq mask %lx\n", status);
- return IRQ_HANDLED;
- }
- if ((status & I2C_SL_IRQ) == 0) {
- dev_err(nvec->dev, "Spurious IRQ\n");
- return IRQ_HANDLED;
- }
-
- /* The EC did not request a read, so it send us something, read it */
- if ((status & RNW) == 0) {
- received = readl(nvec->base + I2C_SL_RCVD);
- if (status & RCVD)
- writel(0, nvec->base + I2C_SL_RCVD);
- }
-
- if (status == (I2C_SL_IRQ | RCVD))
- nvec->state = 0;
-
- switch (nvec->state) {
- case 0: /* Verify that its a transfer start, the rest later */
- if (status != (I2C_SL_IRQ | RCVD))
- nvec_invalid_flags(nvec, status, false);
- break;
- case 1: /* command byte */
- if (status != I2C_SL_IRQ) {
- nvec_invalid_flags(nvec, status, true);
- } else {
- nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
- /* Should not happen in a normal world */
- if (unlikely(nvec->rx == NULL)) {
- nvec->state = 0;
- break;
- }
- nvec->rx->data[0] = received;
- nvec->rx->pos = 1;
- nvec->state = 2;
- }
- break;
- case 2: /* first byte after command */
- if (status == (I2C_SL_IRQ | RNW | RCVD)) {
- udelay(33);
- if (nvec->rx->data[0] != 0x01) {
- dev_err(nvec->dev,
- "Read without prior read command\n");
- nvec->state = 0;
- break;
- }
- nvec_msg_free(nvec, nvec->rx);
- nvec->state = 3;
- nvec_tx_set(nvec);
- BUG_ON(nvec->tx->size < 1);
- to_send = nvec->tx->data[0];
- nvec->tx->pos = 1;
- } else if (status == (I2C_SL_IRQ)) {
- BUG_ON(nvec->rx == NULL);
- nvec->rx->data[1] = received;
- nvec->rx->pos = 2;
- nvec->state = 4;
- } else {
- nvec_invalid_flags(nvec, status, true);
- }
- break;
- case 3: /* EC does a block read, we transmit data */
- if (status & END_TRANS) {
- nvec_tx_completed(nvec);
- } else if ((status & RNW) == 0 || (status & RCVD)) {
- nvec_invalid_flags(nvec, status, true);
- } else if (nvec->tx && nvec->tx->pos < nvec->tx->size) {
- to_send = nvec->tx->data[nvec->tx->pos++];
- } else {
- dev_err(nvec->dev, "tx buffer underflow on %p (%u > %u)\n",
- nvec->tx,
- (uint) (nvec->tx ? nvec->tx->pos : 0),
- (uint) (nvec->tx ? nvec->tx->size : 0));
- nvec->state = 0;
- }
- break;
- case 4: /* EC does some write, we read the data */
- if ((status & (END_TRANS | RNW)) == END_TRANS)
- nvec_rx_completed(nvec);
- else if (status & (RNW | RCVD))
- nvec_invalid_flags(nvec, status, true);
- else if (nvec->rx && nvec->rx->pos < NVEC_MSG_SIZE)
- nvec->rx->data[nvec->rx->pos++] = received;
- else
- dev_err(nvec->dev,
- "RX buffer overflow on %p: Trying to write byte %u of %u\n",
- nvec->rx, nvec->rx ? nvec->rx->pos : 0,
- NVEC_MSG_SIZE);
- break;
- default:
- nvec->state = 0;
- }
-
- /* If we are told that a new transfer starts, verify it */
- if ((status & (RCVD | RNW)) == RCVD) {
- if (received != nvec->i2c_addr)
- dev_err(nvec->dev,
- "received address 0x%02x, expected 0x%02x\n",
- received, nvec->i2c_addr);
- nvec->state = 1;
- }
-
- /* Send data if requested, but not on end of transmission */
- if ((status & (RNW | END_TRANS)) == RNW)
- writel(to_send, nvec->base + I2C_SL_RCVD);
-
- /* If we have send the first byte */
- if (status == (I2C_SL_IRQ | RNW | RCVD))
- nvec_gpio_set_value(nvec, 1);
-
- dev_dbg(nvec->dev,
- "Handled: %s 0x%02x, %s 0x%02x in state %u [%s%s%s]\n",
- (status & RNW) == 0 ? "received" : "R=",
- received,
- (status & (RNW | END_TRANS)) ? "sent" : "S=",
- to_send,
- state,
- status & END_TRANS ? " END_TRANS" : "",
- status & RCVD ? " RCVD" : "",
- status & RNW ? " RNW" : "");
-
-
- /*
- * TODO: A correct fix needs to be found for this.
- *
- * We experience less incomplete messages with this delay than without
- * it, but we don't know why. Help is appreciated.
- */
- udelay(100);
-
- return IRQ_HANDLED;
-}
-
-static void tegra_init_i2c_slave(struct nvec_chip *nvec)
-{
- u32 val;
-
- clk_prepare_enable(nvec->i2c_clk);
-
- reset_control_assert(nvec->rst);
- udelay(2);
- reset_control_deassert(nvec->rst);
-
- val = I2C_CNFG_NEW_MASTER_SFM | I2C_CNFG_PACKET_MODE_EN |
- (0x2 << I2C_CNFG_DEBOUNCE_CNT_SHIFT);
- writel(val, nvec->base + I2C_CNFG);
-
- clk_set_rate(nvec->i2c_clk, 8 * 80000);
-
- writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
- writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);
-
- writel(nvec->i2c_addr>>1, nvec->base + I2C_SL_ADDR1);
- writel(0, nvec->base + I2C_SL_ADDR2);
-
- enable_irq(nvec->irq);
-}
-
-#ifdef CONFIG_PM_SLEEP
-static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
-{
- disable_irq(nvec->irq);
- writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
- clk_disable_unprepare(nvec->i2c_clk);
-}
-#endif
-#endif /* FIXME: remove old code. */
-
-
/**
* nvec_slave_cb - I2C slave callback
*
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH v2 4/4] dt: paz00: define nvec as child of i2c bus
2015-03-30 20:00 ` Andrey Danin
(?)
@ 2015-03-30 20:00 ` Andrey Danin
-1 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100
Cc: Mark Rutland, Alexandre Courbot, Russell King, Pawel Moll,
Wolfram Sang, Julian Andres Klode, Greg Kroah-Hartman,
Ian Campbell, Rob Herring, Marc Dietrich, Laxman Dewangan,
Thierry Reding, Kumar Gala, Stephen Warren, Andrey Danin
NVEC driver was reimplemented to use tegra i2c. Use common i2c bindings
for NVEC node.
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- swap reg and request-gpios properties
- use nvec-slave instead of nvec to keep ABI compatibility
- place doc in separate patch
---
arch/arm/boot/dts/tegra20-paz00.dts | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/arch/arm/boot/dts/tegra20-paz00.dts b/arch/arm/boot/dts/tegra20-paz00.dts
index ed7e100..cd5e6ef 100644
--- a/arch/arm/boot/dts/tegra20-paz00.dts
+++ b/arch/arm/boot/dts/tegra20-paz00.dts
@@ -288,20 +288,16 @@
clock-frequency = <100000>;
};
- nvec@7000c500 {
- compatible = "nvidia,nvec";
- reg = <0x7000c500 0x100>;
- interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
- #address-cells = <1>;
- #size-cells = <0>;
+ i2c@7000c500 {
+ status = "okay";
clock-frequency = <80000>;
- request-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_HIGH>;
- slave-addr = <138>;
- clocks = <&tegra_car TEGRA20_CLK_I2C3>,
- <&tegra_car TEGRA20_CLK_PLL_P_OUT3>;
- clock-names = "div-clk", "fast-clk";
- resets = <&tegra_car 67>;
- reset-names = "i2c";
+
+ nvec: nvec@45 {
+ compatible = "nvidia,nvec-slave";
+ reg = <0x45>;
+ request-gpios = <&gpio TEGRA_GPIO(V, 2)
+ GPIO_ACTIVE_HIGH>;
+ };
};
i2c@7000d000 {
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH v2 4/4] dt: paz00: define nvec as child of i2c bus
@ 2015-03-30 20:00 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100
Cc: Andrey Danin, Laxman Dewangan, Wolfram Sang, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
NVEC driver was reimplemented to use tegra i2c. Use common i2c bindings
for NVEC node.
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- swap reg and request-gpios properties
- use nvec-slave instead of nvec to keep ABI compatibility
- place doc in separate patch
---
arch/arm/boot/dts/tegra20-paz00.dts | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/arch/arm/boot/dts/tegra20-paz00.dts b/arch/arm/boot/dts/tegra20-paz00.dts
index ed7e100..cd5e6ef 100644
--- a/arch/arm/boot/dts/tegra20-paz00.dts
+++ b/arch/arm/boot/dts/tegra20-paz00.dts
@@ -288,20 +288,16 @@
clock-frequency = <100000>;
};
- nvec@7000c500 {
- compatible = "nvidia,nvec";
- reg = <0x7000c500 0x100>;
- interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
- #address-cells = <1>;
- #size-cells = <0>;
+ i2c@7000c500 {
+ status = "okay";
clock-frequency = <80000>;
- request-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_HIGH>;
- slave-addr = <138>;
- clocks = <&tegra_car TEGRA20_CLK_I2C3>,
- <&tegra_car TEGRA20_CLK_PLL_P_OUT3>;
- clock-names = "div-clk", "fast-clk";
- resets = <&tegra_car 67>;
- reset-names = "i2c";
+
+ nvec: nvec@45 {
+ compatible = "nvidia,nvec-slave";
+ reg = <0x45>;
+ request-gpios = <&gpio TEGRA_GPIO(V, 2)
+ GPIO_ACTIVE_HIGH>;
+ };
};
i2c@7000d000 {
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH v2 4/4] dt: paz00: define nvec as child of i2c bus
@ 2015-03-30 20:00 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-03-30 20:00 UTC (permalink / raw)
To: linux-arm-kernel
NVEC driver was reimplemented to use tegra i2c. Use common i2c bindings
for NVEC node.
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
Changes for v2:
- swap reg and request-gpios properties
- use nvec-slave instead of nvec to keep ABI compatibility
- place doc in separate patch
---
arch/arm/boot/dts/tegra20-paz00.dts | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/arch/arm/boot/dts/tegra20-paz00.dts b/arch/arm/boot/dts/tegra20-paz00.dts
index ed7e100..cd5e6ef 100644
--- a/arch/arm/boot/dts/tegra20-paz00.dts
+++ b/arch/arm/boot/dts/tegra20-paz00.dts
@@ -288,20 +288,16 @@
clock-frequency = <100000>;
};
- nvec at 7000c500 {
- compatible = "nvidia,nvec";
- reg = <0x7000c500 0x100>;
- interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
- #address-cells = <1>;
- #size-cells = <0>;
+ i2c at 7000c500 {
+ status = "okay";
clock-frequency = <80000>;
- request-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_HIGH>;
- slave-addr = <138>;
- clocks = <&tegra_car TEGRA20_CLK_I2C3>,
- <&tegra_car TEGRA20_CLK_PLL_P_OUT3>;
- clock-names = "div-clk", "fast-clk";
- resets = <&tegra_car 67>;
- reset-names = "i2c";
+
+ nvec: nvec at 45 {
+ compatible = "nvidia,nvec-slave";
+ reg = <0x45>;
+ request-gpios = <&gpio TEGRA_GPIO(V, 2)
+ GPIO_ACTIVE_HIGH>;
+ };
};
i2c at 7000d000 {
--
1.9.1
^ permalink raw reply related [flat|nested] 57+ messages in thread
* Re: [PATCH v2 1/4] i2c: tegra: implement slave mode
2015-03-30 20:00 ` Andrey Danin
(?)
@ 2015-04-03 19:46 ` Wolfram Sang
-1 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-04-03 19:46 UTC (permalink / raw)
To: Andrey Danin
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
ac100-oU9gvf+ajcQ97yFScArB1dHuzzzSOjJt, Laxman Dewangan,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Russell King, Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
[-- Attachment #1: Type: text/plain, Size: 988 bytes --]
> +static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
> +{
> + i2c_writel(i2c_dev, val, I2C_SL_RCVD);
> +
> + /*
> + * TODO: A correct fix needs to be found for this.
> + *
> + * We experience less incomplete messages with this delay than without
> + * it, but we don't know why. Help is appreciated.
> + */
Uh oh. So I assume this is another reason for staging?
> + if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
> + return -EINVAL;
The core checks for slave_cb being valid.
> + i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
You could use value here, too, or?
> + if (!tegra_i2c_slave_isr(irq, i2c_dev))
> + return IRQ_HANDLED;
Minor nit: I'd prefer == 0 here, since it reads "if not slave_isr return
handled".
> + if (slave->addr > 0x7F)
> + addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you
need to check for the flag (slave->flags & I2C_CLIENT_TEN).
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2 1/4] i2c: tegra: implement slave mode
@ 2015-04-03 19:46 ` Wolfram Sang
0 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-04-03 19:46 UTC (permalink / raw)
To: Andrey Danin
Cc: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100, Laxman Dewangan, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
[-- Attachment #1: Type: text/plain, Size: 988 bytes --]
> +static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
> +{
> + i2c_writel(i2c_dev, val, I2C_SL_RCVD);
> +
> + /*
> + * TODO: A correct fix needs to be found for this.
> + *
> + * We experience less incomplete messages with this delay than without
> + * it, but we don't know why. Help is appreciated.
> + */
Uh oh. So I assume this is another reason for staging?
> + if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
> + return -EINVAL;
The core checks for slave_cb being valid.
> + i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
You could use value here, too, or?
> + if (!tegra_i2c_slave_isr(irq, i2c_dev))
> + return IRQ_HANDLED;
Minor nit: I'd prefer == 0 here, since it reads "if not slave_isr return
handled".
> + if (slave->addr > 0x7F)
> + addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you
need to check for the flag (slave->flags & I2C_CLIENT_TEN).
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 1/4] i2c: tegra: implement slave mode
@ 2015-04-03 19:46 ` Wolfram Sang
0 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-04-03 19:46 UTC (permalink / raw)
To: linux-arm-kernel
> +static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
> +{
> + i2c_writel(i2c_dev, val, I2C_SL_RCVD);
> +
> + /*
> + * TODO: A correct fix needs to be found for this.
> + *
> + * We experience less incomplete messages with this delay than without
> + * it, but we don't know why. Help is appreciated.
> + */
Uh oh. So I assume this is another reason for staging?
> + if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
> + return -EINVAL;
The core checks for slave_cb being valid.
> + i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
You could use value here, too, or?
> + if (!tegra_i2c_slave_isr(irq, i2c_dev))
> + return IRQ_HANDLED;
Minor nit: I'd prefer == 0 here, since it reads "if not slave_isr return
handled".
> + if (slave->addr > 0x7F)
> + addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you
need to check for the flag (slave->flags & I2C_CLIENT_TEN).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150403/560a4959/attachment.sig>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegra i2c.
2015-03-30 20:00 ` Andrey Danin
@ 2015-04-03 19:46 ` Wolfram Sang
-1 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-04-03 19:46 UTC (permalink / raw)
To: Andrey Danin
Cc: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100, Laxman Dewangan, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
[-- Attachment #1: Type: text/plain, Size: 465 bytes --]
On Mon, Mar 30, 2015 at 11:00:11PM +0300, Andrey Danin wrote:
> NVEC driver contains code to manage tegra i2c controller in slave mode.
> I2C slave support was implemented in linux kernel. The goal of this
> patch serie is to implement I2C slave mode in tegra drived and rework
> NVEC driver to use it.
Thanks. Looks pretty good already. If being I2C slave was the reason for
this driver being in staging, we soon need to think of a proper place
outside staging.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegra i2c.
@ 2015-04-03 19:46 ` Wolfram Sang
0 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-04-03 19:46 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Mar 30, 2015 at 11:00:11PM +0300, Andrey Danin wrote:
> NVEC driver contains code to manage tegra i2c controller in slave mode.
> I2C slave support was implemented in linux kernel. The goal of this
> patch serie is to implement I2C slave mode in tegra drived and rework
> NVEC driver to use it.
Thanks. Looks pretty good already. If being I2C slave was the reason for
this driver being in staging, we soon need to think of a proper place
outside staging.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150403/88f11028/attachment.sig>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2 2/4] staging/nvec: reimplement on top of tegra i2c driver
2015-03-30 20:00 ` Andrey Danin
(?)
@ 2015-04-03 19:57 ` Wolfram Sang
-1 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-04-03 19:57 UTC (permalink / raw)
To: Andrey Danin
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
ac100-oU9gvf+ajcQ97yFScArB1dHuzzzSOjJt, Laxman Dewangan,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Russell King, Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
[-- Attachment #1: Type: text/plain, Size: 1802 bytes --]
> +/**
> + * nvec_slave_cb - I2C slave callback
> + *
> + * This callback fills our RX buffers and empties our TX
> + * buffers. This uses a finite state machine.
> + */
> +static int nvec_slave_cb(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> +{
> + struct nvec_chip *nvec = i2c_get_clientdata(client);
> +
> + switch (event) {
> + case I2C_SLAVE_WRITE_REQUESTED:
> + /* Alloc new msg only if prev transaction finished */
> + if (nvec->state == ST_NONE)
> + nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
> +
> + /* Should not happen in a normal world */
> + if (unlikely(nvec->rx == NULL)) {
> + nvec->state = ST_NONE;
> + return -1;
> + }
> + nvec->rx->pos = 0;
> +
> + if (client->addr != ((*val) >> 1)) {
Uh, I2C_SLAVE_WRITE_REQUESTED should not use val.
> + dev_err(&client->dev,
> + "received address 0x%02x, expected 0x%02x\n",
> + ((*val) >> 1), client->addr);
> + return -1;
> + }
> + nvec->state = ST_TRANS_START;
> + break;
> +
...
> + case I2C_SLAVE_READ_PROCESSED:
> + if (nvec->state != ST_RX &&
> + nvec->state != ST_TX) {
> + dev_err(&client->dev,
> + "unexpected read: state %d\n",
> + nvec->state);
> + return -1;
> + }
> +
> + if (!nvec->tx || nvec->tx->pos >= nvec->tx->size) {
> + dev_err(nvec->dev,
> + "tx buffer underflow on %p (%u > %u)\n",
> + nvec->tx,
> + (uint) (nvec->tx ? nvec->tx->pos : 0),
> + (uint) (nvec->tx ? nvec->tx->size : 0));
> + nvec->state = ST_NONE;
> + break;
> + }
> +
> + nvec->state = ST_TX;
> + *val = nvec->tx->data[nvec->tx->pos++];
Are you sure you want to increase the pointer here? Remember that this
byte is requested but might not be sent out if the remote master stops
the transfer after the previous byte using NACK instead of ACK.
> + break;
> +
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2 2/4] staging/nvec: reimplement on top of tegra i2c driver
@ 2015-04-03 19:57 ` Wolfram Sang
0 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-04-03 19:57 UTC (permalink / raw)
To: Andrey Danin
Cc: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100, Laxman Dewangan, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
[-- Attachment #1: Type: text/plain, Size: 1802 bytes --]
> +/**
> + * nvec_slave_cb - I2C slave callback
> + *
> + * This callback fills our RX buffers and empties our TX
> + * buffers. This uses a finite state machine.
> + */
> +static int nvec_slave_cb(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> +{
> + struct nvec_chip *nvec = i2c_get_clientdata(client);
> +
> + switch (event) {
> + case I2C_SLAVE_WRITE_REQUESTED:
> + /* Alloc new msg only if prev transaction finished */
> + if (nvec->state == ST_NONE)
> + nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
> +
> + /* Should not happen in a normal world */
> + if (unlikely(nvec->rx == NULL)) {
> + nvec->state = ST_NONE;
> + return -1;
> + }
> + nvec->rx->pos = 0;
> +
> + if (client->addr != ((*val) >> 1)) {
Uh, I2C_SLAVE_WRITE_REQUESTED should not use val.
> + dev_err(&client->dev,
> + "received address 0x%02x, expected 0x%02x\n",
> + ((*val) >> 1), client->addr);
> + return -1;
> + }
> + nvec->state = ST_TRANS_START;
> + break;
> +
...
> + case I2C_SLAVE_READ_PROCESSED:
> + if (nvec->state != ST_RX &&
> + nvec->state != ST_TX) {
> + dev_err(&client->dev,
> + "unexpected read: state %d\n",
> + nvec->state);
> + return -1;
> + }
> +
> + if (!nvec->tx || nvec->tx->pos >= nvec->tx->size) {
> + dev_err(nvec->dev,
> + "tx buffer underflow on %p (%u > %u)\n",
> + nvec->tx,
> + (uint) (nvec->tx ? nvec->tx->pos : 0),
> + (uint) (nvec->tx ? nvec->tx->size : 0));
> + nvec->state = ST_NONE;
> + break;
> + }
> +
> + nvec->state = ST_TX;
> + *val = nvec->tx->data[nvec->tx->pos++];
Are you sure you want to increase the pointer here? Remember that this
byte is requested but might not be sent out if the remote master stops
the transfer after the previous byte using NACK instead of ACK.
> + break;
> +
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 2/4] staging/nvec: reimplement on top of tegra i2c driver
@ 2015-04-03 19:57 ` Wolfram Sang
0 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-04-03 19:57 UTC (permalink / raw)
To: linux-arm-kernel
> +/**
> + * nvec_slave_cb - I2C slave callback
> + *
> + * This callback fills our RX buffers and empties our TX
> + * buffers. This uses a finite state machine.
> + */
> +static int nvec_slave_cb(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> +{
> + struct nvec_chip *nvec = i2c_get_clientdata(client);
> +
> + switch (event) {
> + case I2C_SLAVE_WRITE_REQUESTED:
> + /* Alloc new msg only if prev transaction finished */
> + if (nvec->state == ST_NONE)
> + nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
> +
> + /* Should not happen in a normal world */
> + if (unlikely(nvec->rx == NULL)) {
> + nvec->state = ST_NONE;
> + return -1;
> + }
> + nvec->rx->pos = 0;
> +
> + if (client->addr != ((*val) >> 1)) {
Uh, I2C_SLAVE_WRITE_REQUESTED should not use val.
> + dev_err(&client->dev,
> + "received address 0x%02x, expected 0x%02x\n",
> + ((*val) >> 1), client->addr);
> + return -1;
> + }
> + nvec->state = ST_TRANS_START;
> + break;
> +
...
> + case I2C_SLAVE_READ_PROCESSED:
> + if (nvec->state != ST_RX &&
> + nvec->state != ST_TX) {
> + dev_err(&client->dev,
> + "unexpected read: state %d\n",
> + nvec->state);
> + return -1;
> + }
> +
> + if (!nvec->tx || nvec->tx->pos >= nvec->tx->size) {
> + dev_err(nvec->dev,
> + "tx buffer underflow on %p (%u > %u)\n",
> + nvec->tx,
> + (uint) (nvec->tx ? nvec->tx->pos : 0),
> + (uint) (nvec->tx ? nvec->tx->size : 0));
> + nvec->state = ST_NONE;
> + break;
> + }
> +
> + nvec->state = ST_TX;
> + *val = nvec->tx->data[nvec->tx->pos++];
Are you sure you want to increase the pointer here? Remember that this
byte is requested but might not be sent out if the remote master stops
the transfer after the previous byte using NACK instead of ACK.
> + break;
> +
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150403/69aaf73d/attachment.sig>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegrai2c.
2015-04-03 19:46 ` Wolfram Sang
@ 2015-04-07 11:37 ` Marc Dietrich
-1 siblings, 0 replies; 57+ messages in thread
From: Marc Dietrich @ 2015-04-07 11:37 UTC (permalink / raw)
To: Wolfram Sang
Cc: Andrey Danin, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Stephen Warren,
Greg Kroah-Hartman
[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]
Hi Wolfram,
Am Freitag, 3. April 2015, 21:46:35 schrieb Wolfram Sang:
> On Mon, Mar 30, 2015 at 11:00:11PM +0300, Andrey Danin wrote:
> > NVEC driver contains code to manage tegra i2c controller in slave mode.
> > I2C slave support was implemented in linux kernel. The goal of this
> > patch serie is to implement I2C slave mode in tegra drived and rework
> > NVEC driver to use it.
>
> Thanks. Looks pretty good already. If being I2C slave was the reason for
> this driver being in staging, we soon need to think of a proper place
> outside staging.
yes, moving the low level i2c functions out to the bus driver was our major
concern. But there are still some minor things left before before we can move
out of staging.
One thing where we need your help as a I2C maintainer is how to represent an
i2c slave device using device-tree. You may remember our discussion in the
past from here [1] where you suggested to just make a slave client by its
compatible name. Stephen Warren from NVIDIA raised some concerns about this
solution because it may not be appropriate in all possible future cases (which
is what a proper device-tree representation should take care off). He instead
suggested to mark a slave client by adding some flag to the reg property, to
be able to handle a situation where both master client and slave client have
the same i2c bus address forming a loopback (e.g. for testing purpose) on the
same bus. More details here [2].
I hope with this post I can join the different discussions somehow so we are
able to find a common sense which is acceptable for all.
Marc
[1] http://lkml.iu.edu/hypermail/linux/kernel/1409.1/03446.html
[2] https://lists.launchpad.net/ac100/msg01361.html
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegrai2c.
@ 2015-04-07 11:37 ` Marc Dietrich
0 siblings, 0 replies; 57+ messages in thread
From: Marc Dietrich @ 2015-04-07 11:37 UTC (permalink / raw)
To: linux-arm-kernel
Hi Wolfram,
Am Freitag, 3. April 2015, 21:46:35 schrieb Wolfram Sang:
> On Mon, Mar 30, 2015 at 11:00:11PM +0300, Andrey Danin wrote:
> > NVEC driver contains code to manage tegra i2c controller in slave mode.
> > I2C slave support was implemented in linux kernel. The goal of this
> > patch serie is to implement I2C slave mode in tegra drived and rework
> > NVEC driver to use it.
>
> Thanks. Looks pretty good already. If being I2C slave was the reason for
> this driver being in staging, we soon need to think of a proper place
> outside staging.
yes, moving the low level i2c functions out to the bus driver was our major
concern. But there are still some minor things left before before we can move
out of staging.
One thing where we need your help as a I2C maintainer is how to represent an
i2c slave device using device-tree. You may remember our discussion in the
past from here [1] where you suggested to just make a slave client by its
compatible name. Stephen Warren from NVIDIA raised some concerns about this
solution because it may not be appropriate in all possible future cases (which
is what a proper device-tree representation should take care off). He instead
suggested to mark a slave client by adding some flag to the reg property, to
be able to handle a situation where both master client and slave client have
the same i2c bus address forming a loopback (e.g. for testing purpose) on the
same bus. More details here [2].
I hope with this post I can join the different discussions somehow so we are
able to find a common sense which is acceptable for all.
Marc
[1] http://lkml.iu.edu/hypermail/linux/kernel/1409.1/03446.html
[2] https://lists.launchpad.net/ac100/msg01361.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150407/330e1c96/attachment.sig>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegrai2c.
2015-04-07 11:37 ` Marc Dietrich
@ 2015-04-10 21:35 ` Wolfram Sang
-1 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-04-10 21:35 UTC (permalink / raw)
To: Marc Dietrich
Cc: Andrey Danin, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Stephen Warren,
Greg Kroah-Hartman
[-- Attachment #1: Type: text/plain, Size: 947 bytes --]
> One thing where we need your help as a I2C maintainer is how to represent an
> i2c slave device using device-tree. You may remember our discussion in the
> past from here [1] where you suggested to just make a slave client by its
> compatible name. Stephen Warren from NVIDIA raised some concerns about this
> solution because it may not be appropriate in all possible future cases (which
> is what a proper device-tree representation should take care off). He instead
> suggested to mark a slave client by adding some flag to the reg property, to
> be able to handle a situation where both master client and slave client have
> the same i2c bus address forming a loopback (e.g. for testing purpose) on the
> same bus. More details here [2].
>
> I hope with this post I can join the different discussions somehow so we are
> able to find a common sense which is acceptable for all.
I'll have a look again for 4.2.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegrai2c.
@ 2015-04-10 21:35 ` Wolfram Sang
0 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-04-10 21:35 UTC (permalink / raw)
To: linux-arm-kernel
> One thing where we need your help as a I2C maintainer is how to represent an
> i2c slave device using device-tree. You may remember our discussion in the
> past from here [1] where you suggested to just make a slave client by its
> compatible name. Stephen Warren from NVIDIA raised some concerns about this
> solution because it may not be appropriate in all possible future cases (which
> is what a proper device-tree representation should take care off). He instead
> suggested to mark a slave client by adding some flag to the reg property, to
> be able to handle a situation where both master client and slave client have
> the same i2c bus address forming a loopback (e.g. for testing purpose) on the
> same bus. More details here [2].
>
> I hope with this post I can join the different discussions somehow so we are
> able to find a common sense which is acceptable for all.
I'll have a look again for 4.2.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150410/577d2d97/attachment.sig>
^ permalink raw reply [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
2015-04-07 11:37 ` Marc Dietrich
(?)
(?)
@ 2015-05-05 10:55 ` Wolfram Sang
2015-05-05 20:07 ` Stephen Warren
2015-05-06 6:59 ` Uwe Kleine-König
-1 siblings, 2 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-05-05 10:55 UTC (permalink / raw)
To: Marc Dietrich
Cc: Andrey Danin, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Stephen Warren,
Greg Kroah-Hartman
[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]
I'm back in town. Sorry for the delay...
> One thing where we need your help as a I2C maintainer is how to represent an
> i2c slave device using device-tree. You may remember our discussion in the
> past from here [1] where you suggested to just make a slave client by its
> compatible name. Stephen Warren from NVIDIA raised some concerns about this
> solution because it may not be appropriate in all possible future cases (which
> is what a proper device-tree representation should take care off). He instead
> suggested to mark a slave client by adding some flag to the reg property, to
> be able to handle a situation where both master client and slave client have
> the same i2c bus address forming a loopback (e.g. for testing purpose) on the
> same bus. More details here [2].
Well... I can agree that we shouldn't prevent a loopback from a DT point
of view. (Despite the fact that it is really for development and I
wonder how many I2C IP cores can do this flawlessly)
However, I am still against putting that information into the reg
property. I see devices coming which have multiple addresses, so people
somewhen want to encode this in DT as well. I'd like to have that a
simple array of addresses. Adding flags, rarely used, will create a mess
IMO.
So what about adding a new property "i2c-slave-reg"? This does not only
prevent the confusion above, but also makes it very clear that this node
is an I2C slave without the need to encode that somehow in the
compatible property (although it probably should be described there as
well, still).
> I hope with this post I can join the different discussions somehow so we are
> able to find a common sense which is acceptable for all.
Thanks for doing this! I changed the subject to maybe raise interest a
bit more.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-05 10:55 ` How to encode being an I2C slave in DT? Wolfram Sang
@ 2015-05-05 20:07 ` Stephen Warren
2015-05-06 6:59 ` Uwe Kleine-König
1 sibling, 0 replies; 57+ messages in thread
From: Stephen Warren @ 2015-05-05 20:07 UTC (permalink / raw)
To: Wolfram Sang, Marc Dietrich
Cc: Andrey Danin, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
On 05/05/2015 04:55 AM, Wolfram Sang wrote:
>
> I'm back in town. Sorry for the delay...
>
>> One thing where we need your help as a I2C maintainer is how to represent an
>> i2c slave device using device-tree. You may remember our discussion in the
>> past from here [1] where you suggested to just make a slave client by its
>> compatible name. Stephen Warren from NVIDIA raised some concerns about this
>> solution because it may not be appropriate in all possible future cases (which
>> is what a proper device-tree representation should take care off). He instead
>> suggested to mark a slave client by adding some flag to the reg property, to
>> be able to handle a situation where both master client and slave client have
>> the same i2c bus address forming a loopback (e.g. for testing purpose) on the
>> same bus. More details here [2].
>
> Well... I can agree that we shouldn't prevent a loopback from a DT point
> of view. (Despite the fact that it is really for development and I
> wonder how many I2C IP cores can do this flawlessly)
>
> However, I am still against putting that information into the reg
> property. I see devices coming which have multiple addresses, so people
> somewhen want to encode this in DT as well. I'd like to have that a
> simple array of addresses. Adding flags, rarely used, will create a mess
> IMO.
The container node has a #address-cells property for this very reason.
It's perfectly well-defined how to split up a property containing a
large number of cells into separate values, by using the value of
#address-cells. Plus, the canonical formatting (albeit not enforced by
the DT compiler) for a property that contains an array of entries, each
2 cells in size, would be:
reg = <0 0x1a>, <0 0x40>, <0 0x48>;
rather than:
reg = <0 0x1a 0 0x40 0 0x48>;
... so it's quite simple to make it very human-readable too.
> So what about adding a new property "i2c-slave-reg"? This does not only
> prevent the confusion above, but also makes it very clear that this node
> is an I2C slave without the need to encode that somehow in the
> compatible property (although it probably should be described there as
> well, still).
That doesn't sound like a good idea. reg is the DT-defined way of
identifying/numbering child nodes. We shouldn't invent other properties
that mean essentially the same thing, but simply encode flags.
It might be reasonable to split an I2C controller node up so that it has
n child nodes:
i2c-controller@1234 {
compatible = "foo";
reg = <0x1234 0x100>;
#address-cells = <1>;
#size-cells = <0>;
master {
eeprom@1a {
compatible = ...;
reg = <0x1a>;
};
...
};
slave {
nvec@NN {
compatible = ...;
reg = <0xNN>;
};
...
};
};
However, that seems a lot more complex and invasive than just adding an
extra address cell to reg, and putting everything into a single node.
>> I hope with this post I can join the different discussions somehow so we are
>> able to find a common sense which is acceptable for all.
>
> Thanks for doing this! I changed the subject to maybe raise interest a
> bit more.
^ permalink raw reply [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-05-05 20:07 ` Stephen Warren
0 siblings, 0 replies; 57+ messages in thread
From: Stephen Warren @ 2015-05-05 20:07 UTC (permalink / raw)
To: linux-arm-kernel
On 05/05/2015 04:55 AM, Wolfram Sang wrote:
>
> I'm back in town. Sorry for the delay...
>
>> One thing where we need your help as a I2C maintainer is how to represent an
>> i2c slave device using device-tree. You may remember our discussion in the
>> past from here [1] where you suggested to just make a slave client by its
>> compatible name. Stephen Warren from NVIDIA raised some concerns about this
>> solution because it may not be appropriate in all possible future cases (which
>> is what a proper device-tree representation should take care off). He instead
>> suggested to mark a slave client by adding some flag to the reg property, to
>> be able to handle a situation where both master client and slave client have
>> the same i2c bus address forming a loopback (e.g. for testing purpose) on the
>> same bus. More details here [2].
>
> Well... I can agree that we shouldn't prevent a loopback from a DT point
> of view. (Despite the fact that it is really for development and I
> wonder how many I2C IP cores can do this flawlessly)
>
> However, I am still against putting that information into the reg
> property. I see devices coming which have multiple addresses, so people
> somewhen want to encode this in DT as well. I'd like to have that a
> simple array of addresses. Adding flags, rarely used, will create a mess
> IMO.
The container node has a #address-cells property for this very reason.
It's perfectly well-defined how to split up a property containing a
large number of cells into separate values, by using the value of
#address-cells. Plus, the canonical formatting (albeit not enforced by
the DT compiler) for a property that contains an array of entries, each
2 cells in size, would be:
reg = <0 0x1a>, <0 0x40>, <0 0x48>;
rather than:
reg = <0 0x1a 0 0x40 0 0x48>;
... so it's quite simple to make it very human-readable too.
> So what about adding a new property "i2c-slave-reg"? This does not only
> prevent the confusion above, but also makes it very clear that this node
> is an I2C slave without the need to encode that somehow in the
> compatible property (although it probably should be described there as
> well, still).
That doesn't sound like a good idea. reg is the DT-defined way of
identifying/numbering child nodes. We shouldn't invent other properties
that mean essentially the same thing, but simply encode flags.
It might be reasonable to split an I2C controller node up so that it has
n child nodes:
i2c-controller at 1234 {
compatible = "foo";
reg = <0x1234 0x100>;
#address-cells = <1>;
#size-cells = <0>;
master {
eeprom at 1a {
compatible = ...;
reg = <0x1a>;
};
...
};
slave {
nvec at NN {
compatible = ...;
reg = <0xNN>;
};
...
};
};
However, that seems a lot more complex and invasive than just adding an
extra address cell to reg, and putting everything into a single node.
>> I hope with this post I can join the different discussions somehow so we are
>> able to find a common sense which is acceptable for all.
>
> Thanks for doing this! I changed the subject to maybe raise interest a
> bit more.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-05 10:55 ` How to encode being an I2C slave in DT? Wolfram Sang
@ 2015-05-06 6:59 ` Uwe Kleine-König
2015-05-06 6:59 ` Uwe Kleine-König
1 sibling, 0 replies; 57+ messages in thread
From: Uwe Kleine-König @ 2015-05-06 6:59 UTC (permalink / raw)
To: Wolfram Sang
Cc: Marc Dietrich, Andrey Danin, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Stephen Warren,
Greg Kroah-Hartman
Hello,
On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
> So what about adding a new property "i2c-slave-reg"? This does not only
> prevent the confusion above, but also makes it very clear that this node
> is an I2C slave without the need to encode that somehow in the
> compatible property (although it probably should be described there as
> well, still).
I admit I didn't follow the discussions referenced in the footnotes, but
I wonder if the slave part should be added to the device tree at all.
AFAICT it could (and so should) be completely userspace-defined which
slave driver is used on which address. I imagine that for most
controllers the bus addresses to use can be chosen more or less freely.
So what am I missing?
> > I hope with this post I can join the different discussions somehow so we are
> > able to find a common sense which is acceptable for all.
>
> Thanks for doing this! I changed the subject to maybe raise interest a
> bit more.
that worked fine :-)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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 [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-05-06 6:59 ` Uwe Kleine-König
0 siblings, 0 replies; 57+ messages in thread
From: Uwe Kleine-König @ 2015-05-06 6:59 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
> So what about adding a new property "i2c-slave-reg"? This does not only
> prevent the confusion above, but also makes it very clear that this node
> is an I2C slave without the need to encode that somehow in the
> compatible property (although it probably should be described there as
> well, still).
I admit I didn't follow the discussions referenced in the footnotes, but
I wonder if the slave part should be added to the device tree at all.
AFAICT it could (and so should) be completely userspace-defined which
slave driver is used on which address. I imagine that for most
controllers the bus addresses to use can be chosen more or less freely.
So what am I missing?
> > I hope with this post I can join the different discussions somehow so we are
> > able to find a common sense which is acceptable for all.
>
> Thanks for doing this! I changed the subject to maybe raise interest a
> bit more.
that worked fine :-)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-06 6:59 ` Uwe Kleine-König
@ 2015-05-06 7:53 ` Marc Dietrich
-1 siblings, 0 replies; 57+ messages in thread
From: Marc Dietrich @ 2015-05-06 7:53 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Wolfram Sang, Andrey Danin, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Stephen Warren,
Greg Kroah-Hartman
[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]
Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-König:
> Hello,
>
> On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
> > So what about adding a new property "i2c-slave-reg"? This does not only
> > prevent the confusion above, but also makes it very clear that this node
> > is an I2C slave without the need to encode that somehow in the
> > compatible property (although it probably should be described there as
> > well, still).
>
> I admit I didn't follow the discussions referenced in the footnotes, but
> I wonder if the slave part should be added to the device tree at all.
> AFAICT it could (and so should) be completely userspace-defined which
> slave driver is used on which address. I imagine that for most
> controllers the bus addresses to use can be chosen more or less freely.
> So what am I missing?
if you had read the footnotes you would know :-) Our usecase is connect an
embeedded controller via i2c to the host soc, similar to cros-ec, but here the
ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other
stuff, for which the drivers are best implemented in kernel code AFAIK.
Marc
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-05-06 7:53 ` Marc Dietrich
0 siblings, 0 replies; 57+ messages in thread
From: Marc Dietrich @ 2015-05-06 7:53 UTC (permalink / raw)
To: linux-arm-kernel
Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-K?nig:
> Hello,
>
> On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
> > So what about adding a new property "i2c-slave-reg"? This does not only
> > prevent the confusion above, but also makes it very clear that this node
> > is an I2C slave without the need to encode that somehow in the
> > compatible property (although it probably should be described there as
> > well, still).
>
> I admit I didn't follow the discussions referenced in the footnotes, but
> I wonder if the slave part should be added to the device tree at all.
> AFAICT it could (and so should) be completely userspace-defined which
> slave driver is used on which address. I imagine that for most
> controllers the bus addresses to use can be chosen more or less freely.
> So what am I missing?
if you had read the footnotes you would know :-) Our usecase is connect an
embeedded controller via i2c to the host soc, similar to cros-ec, but here the
ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other
stuff, for which the drivers are best implemented in kernel code AFAIK.
Marc
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150506/afbd62ae/attachment-0001.sig>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-06 7:53 ` Marc Dietrich
@ 2015-05-06 8:09 ` Uwe Kleine-König
-1 siblings, 0 replies; 57+ messages in thread
From: Uwe Kleine-König @ 2015-05-06 8:09 UTC (permalink / raw)
To: Marc Dietrich
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang,
Greg Kroah-Hartman, Stephen Warren,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Andrey Danin,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Hello Marc,
On Wed, May 06, 2015 at 09:53:55AM +0200, Marc Dietrich wrote:
> Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-König:
> > On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
> > > So what about adding a new property "i2c-slave-reg"? This does not only
> > > prevent the confusion above, but also makes it very clear that this node
> > > is an I2C slave without the need to encode that somehow in the
> > > compatible property (although it probably should be described there as
> > > well, still).
> >
> > I admit I didn't follow the discussions referenced in the footnotes, but
> > I wonder if the slave part should be added to the device tree at all.
> > AFAICT it could (and so should) be completely userspace-defined which
> > slave driver is used on which address. I imagine that for most
> > controllers the bus addresses to use can be chosen more or less freely.
> > So what am I missing?
>
> if you had read the footnotes you would know :-) Our usecase is connect an
> embeedded controller via i2c to the host soc, similar to cros-ec, but here the
> ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other
> stuff, for which the drivers are best implemented in kernel code AFAIK.
Right, the driver might sensibly be implemented in kernel space. But I'd
vote that you still need to do the binding of these drivers to your
slave controller from userspace. Then there is no need to specify
anything in your dtb.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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 [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-05-06 8:09 ` Uwe Kleine-König
0 siblings, 0 replies; 57+ messages in thread
From: Uwe Kleine-König @ 2015-05-06 8:09 UTC (permalink / raw)
To: linux-arm-kernel
Hello Marc,
On Wed, May 06, 2015 at 09:53:55AM +0200, Marc Dietrich wrote:
> Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-K?nig:
> > On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
> > > So what about adding a new property "i2c-slave-reg"? This does not only
> > > prevent the confusion above, but also makes it very clear that this node
> > > is an I2C slave without the need to encode that somehow in the
> > > compatible property (although it probably should be described there as
> > > well, still).
> >
> > I admit I didn't follow the discussions referenced in the footnotes, but
> > I wonder if the slave part should be added to the device tree at all.
> > AFAICT it could (and so should) be completely userspace-defined which
> > slave driver is used on which address. I imagine that for most
> > controllers the bus addresses to use can be chosen more or less freely.
> > So what am I missing?
>
> if you had read the footnotes you would know :-) Our usecase is connect an
> embeedded controller via i2c to the host soc, similar to cros-ec, but here the
> ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other
> stuff, for which the drivers are best implemented in kernel code AFAIK.
Right, the driver might sensibly be implemented in kernel space. But I'd
vote that you still need to do the binding of these drivers to your
slave controller from userspace. Then there is no need to specify
anything in your dtb.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-06 8:09 ` Uwe Kleine-König
@ 2015-05-06 15:57 ` Stephen Warren
-1 siblings, 0 replies; 57+ messages in thread
From: Stephen Warren @ 2015-05-06 15:57 UTC (permalink / raw)
To: Uwe Kleine-König, Marc Dietrich
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang,
Greg Kroah-Hartman, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Andrey Danin,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 05/06/2015 02:09 AM, Uwe Kleine-König wrote:
> Hello Marc,
>
> On Wed, May 06, 2015 at 09:53:55AM +0200, Marc Dietrich wrote:
>> Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-König:
>>> On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
>>>> So what about adding a new property "i2c-slave-reg"? This does not only
>>>> prevent the confusion above, but also makes it very clear that this node
>>>> is an I2C slave without the need to encode that somehow in the
>>>> compatible property (although it probably should be described there as
>>>> well, still).
>>>
>>> I admit I didn't follow the discussions referenced in the footnotes, but
>>> I wonder if the slave part should be added to the device tree at all.
>>> AFAICT it could (and so should) be completely userspace-defined which
>>> slave driver is used on which address. I imagine that for most
>>> controllers the bus addresses to use can be chosen more or less freely.
>>> So what am I missing?
>>
>> if you had read the footnotes you would know :-) Our usecase is connect an
>> embeedded controller via i2c to the host soc, similar to cros-ec, but here the
>> ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other
>> stuff, for which the drivers are best implemented in kernel code AFAIK.
>
> Right, the driver might sensibly be implemented in kernel space. But I'd
> vote that you still need to do the binding of these drivers to your
> slave controller from userspace. Then there is no need to specify
> anything in your dtb.
I think the set of I2C slave devices that are implemented by the Linux
system can reasonably be considered part of the HW definition. Most DT
content to date has been a definition of the HW that's available to SW,
but at least in this case, this I2C slave device is something that must
be implemented (admittedly in conjunction with SW) using the I2C slave
HW on the main SoC, in order for the overall HW to work as intended.
BTW, I believe devicetree-spec-u79uwXL29TY76Z2rM5mHXA@public.gmane.org was created to address
subsystem-level DT schema questions like this. It's much lower volume,
so perhaps the thread would get noticed by the DT maintainers if posted
there (or perhaps just CC them)?
--
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 [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-05-06 15:57 ` Stephen Warren
0 siblings, 0 replies; 57+ messages in thread
From: Stephen Warren @ 2015-05-06 15:57 UTC (permalink / raw)
To: linux-arm-kernel
On 05/06/2015 02:09 AM, Uwe Kleine-K?nig wrote:
> Hello Marc,
>
> On Wed, May 06, 2015 at 09:53:55AM +0200, Marc Dietrich wrote:
>> Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-K?nig:
>>> On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
>>>> So what about adding a new property "i2c-slave-reg"? This does not only
>>>> prevent the confusion above, but also makes it very clear that this node
>>>> is an I2C slave without the need to encode that somehow in the
>>>> compatible property (although it probably should be described there as
>>>> well, still).
>>>
>>> I admit I didn't follow the discussions referenced in the footnotes, but
>>> I wonder if the slave part should be added to the device tree at all.
>>> AFAICT it could (and so should) be completely userspace-defined which
>>> slave driver is used on which address. I imagine that for most
>>> controllers the bus addresses to use can be chosen more or less freely.
>>> So what am I missing?
>>
>> if you had read the footnotes you would know :-) Our usecase is connect an
>> embeedded controller via i2c to the host soc, similar to cros-ec, but here the
>> ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other
>> stuff, for which the drivers are best implemented in kernel code AFAIK.
>
> Right, the driver might sensibly be implemented in kernel space. But I'd
> vote that you still need to do the binding of these drivers to your
> slave controller from userspace. Then there is no need to specify
> anything in your dtb.
I think the set of I2C slave devices that are implemented by the Linux
system can reasonably be considered part of the HW definition. Most DT
content to date has been a definition of the HW that's available to SW,
but at least in this case, this I2C slave device is something that must
be implemented (admittedly in conjunction with SW) using the I2C slave
HW on the main SoC, in order for the overall HW to work as intended.
BTW, I believe devicetree-spec at vger.kernel.org was created to address
subsystem-level DT schema questions like this. It's much lower volume,
so perhaps the thread would get noticed by the DT maintainers if posted
there (or perhaps just CC them)?
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-05 20:07 ` Stephen Warren
@ 2015-05-06 16:17 ` Wolfram Sang
-1 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-05-06 16:17 UTC (permalink / raw)
To: Stephen Warren
Cc: Marc Dietrich, Andrey Danin, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]
> The container node has a #address-cells property for this very reason. It's
> perfectly well-defined how to split up a property containing a large number
> of cells into separate values, by using the value of #address-cells. Plus,
> the canonical formatting (albeit not enforced by the DT compiler) for a
> property that contains an array of entries, each 2 cells in size, would be:
>
> reg = <0 0x1a>, <0 0x40>, <0 0x48>;
>
> rather than:
>
> reg = <0 0x1a 0 0x40 0 0x48>;
>
> ... so it's quite simple to make it very human-readable too.
I give in to the flag idea. I also noticed that we'd need another flag
anyhow to mark 10 bit addresses. I am still thinking between using two
address-cells in that case (clean seperation between address and flags)
or to encode the flags as MSB in the current address (all busses will
have same address-cells and child description, less code paths and no
overhead in dtbs).
That being said, for the loopback testcase, the I2C slave framework will
need updates as well. I think I can cook up something. Will be
interesting to see if my hardware can do this. Has the loopback already
been tested on Tegra?
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-05-06 16:17 ` Wolfram Sang
0 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-05-06 16:17 UTC (permalink / raw)
To: linux-arm-kernel
> The container node has a #address-cells property for this very reason. It's
> perfectly well-defined how to split up a property containing a large number
> of cells into separate values, by using the value of #address-cells. Plus,
> the canonical formatting (albeit not enforced by the DT compiler) for a
> property that contains an array of entries, each 2 cells in size, would be:
>
> reg = <0 0x1a>, <0 0x40>, <0 0x48>;
>
> rather than:
>
> reg = <0 0x1a 0 0x40 0 0x48>;
>
> ... so it's quite simple to make it very human-readable too.
I give in to the flag idea. I also noticed that we'd need another flag
anyhow to mark 10 bit addresses. I am still thinking between using two
address-cells in that case (clean seperation between address and flags)
or to encode the flags as MSB in the current address (all busses will
have same address-cells and child description, less code paths and no
overhead in dtbs).
That being said, for the loopback testcase, the I2C slave framework will
need updates as well. I think I can cook up something. Will be
interesting to see if my hardware can do this. Has the loopback already
been tested on Tegra?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150506/faec8eb4/attachment-0001.sig>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-06 16:17 ` Wolfram Sang
@ 2015-05-06 17:01 ` Stephen Warren
-1 siblings, 0 replies; 57+ messages in thread
From: Stephen Warren @ 2015-05-06 17:01 UTC (permalink / raw)
To: Wolfram Sang
Cc: Marc Dietrich, Andrey Danin, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
On 05/06/2015 10:17 AM, Wolfram Sang wrote:
>
>> The container node has a #address-cells property for this very reason. It's
>> perfectly well-defined how to split up a property containing a large number
>> of cells into separate values, by using the value of #address-cells. Plus,
>> the canonical formatting (albeit not enforced by the DT compiler) for a
>> property that contains an array of entries, each 2 cells in size, would be:
>>
>> reg = <0 0x1a>, <0 0x40>, <0 0x48>;
>>
>> rather than:
>>
>> reg = <0 0x1a 0 0x40 0 0x48>;
>>
>> ... so it's quite simple to make it very human-readable too.
>
> I give in to the flag idea. I also noticed that we'd need another flag
> anyhow to mark 10 bit addresses. I am still thinking between using two
> address-cells in that case (clean seperation between address and flags)
> or to encode the flags as MSB in the current address (all busses will
> have same address-cells and child description, less code paths and no
> overhead in dtbs).
>
> That being said, for the loopback testcase, the I2C slave framework will
> need updates as well. I think I can cook up something. Will be
> interesting to see if my hardware can do this. Has the loopback already
> been tested on Tegra?
I don't believe that loopback has been tested on Tegra, at least with
upstream SW. I have no idea whether our downstream SW or Silicon
validation team has tested it.
^ permalink raw reply [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-05-06 17:01 ` Stephen Warren
0 siblings, 0 replies; 57+ messages in thread
From: Stephen Warren @ 2015-05-06 17:01 UTC (permalink / raw)
To: linux-arm-kernel
On 05/06/2015 10:17 AM, Wolfram Sang wrote:
>
>> The container node has a #address-cells property for this very reason. It's
>> perfectly well-defined how to split up a property containing a large number
>> of cells into separate values, by using the value of #address-cells. Plus,
>> the canonical formatting (albeit not enforced by the DT compiler) for a
>> property that contains an array of entries, each 2 cells in size, would be:
>>
>> reg = <0 0x1a>, <0 0x40>, <0 0x48>;
>>
>> rather than:
>>
>> reg = <0 0x1a 0 0x40 0 0x48>;
>>
>> ... so it's quite simple to make it very human-readable too.
>
> I give in to the flag idea. I also noticed that we'd need another flag
> anyhow to mark 10 bit addresses. I am still thinking between using two
> address-cells in that case (clean seperation between address and flags)
> or to encode the flags as MSB in the current address (all busses will
> have same address-cells and child description, less code paths and no
> overhead in dtbs).
>
> That being said, for the loopback testcase, the I2C slave framework will
> need updates as well. I think I can cook up something. Will be
> interesting to see if my hardware can do this. Has the loopback already
> been tested on Tegra?
I don't believe that loopback has been tested on Tegra, at least with
upstream SW. I have no idea whether our downstream SW or Silicon
validation team has tested it.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-06 15:57 ` Stephen Warren
@ 2015-05-06 17:47 ` Uwe Kleine-König
-1 siblings, 0 replies; 57+ messages in thread
From: Uwe Kleine-König @ 2015-05-06 17:47 UTC (permalink / raw)
To: Stephen Warren
Cc: Marc Dietrich, devicetree-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang,
Greg Kroah-Hartman, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Andrey Danin,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-spec-u79uwXL29TY76Z2rM5mHXA
Hello Stephen,
On Wed, May 06, 2015 at 09:57:37AM -0600, Stephen Warren wrote:
> On 05/06/2015 02:09 AM, Uwe Kleine-König wrote:
> >On Wed, May 06, 2015 at 09:53:55AM +0200, Marc Dietrich wrote:
> >>Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-König:
> >>>On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
> >>>>So what about adding a new property "i2c-slave-reg"? This does not only
> >>>>prevent the confusion above, but also makes it very clear that this node
> >>>>is an I2C slave without the need to encode that somehow in the
> >>>>compatible property (although it probably should be described there as
> >>>>well, still).
> >>>
> >>>I admit I didn't follow the discussions referenced in the footnotes, but
> >>>I wonder if the slave part should be added to the device tree at all.
> >>>AFAICT it could (and so should) be completely userspace-defined which
> >>>slave driver is used on which address. I imagine that for most
> >>>controllers the bus addresses to use can be chosen more or less freely.
> >>>So what am I missing?
> >>
> >>if you had read the footnotes you would know :-) Our usecase is connect an
> >>embeedded controller via i2c to the host soc, similar to cros-ec, but here the
> >>ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other
> >>stuff, for which the drivers are best implemented in kernel code AFAIK.
> >
> >Right, the driver might sensibly be implemented in kernel space. But I'd
> >vote that you still need to do the binding of these drivers to your
> >slave controller from userspace. Then there is no need to specify
> >anything in your dtb.
>
> I think the set of I2C slave devices that are implemented by the
> Linux system can reasonably be considered part of the HW definition.
> Most DT content to date has been a definition of the HW that's
> available to SW, but at least in this case, this I2C slave device is
> something that must be implemented (admittedly in conjunction with
> SW) using the I2C slave HW on the main SoC, in order for the overall
> HW to work as intended.
I'm not convinced. Why must it be implemented? And is it too late if the
setup is done by userspace?
The devicetree description of a flash chip doesn't include the rootfs
content either, although a working rootfs is critical for most operating
systems. The devicetree description of an ethernet adapter doesn't
include its network setup although an ethernet adapter hardly makes any
sense without an IP.
> BTW, I believe devicetree-spec-u79uwXL29TY76Z2rM5mHXA@public.gmane.org was created to
> address subsystem-level DT schema questions like this. It's much
> lower volume, so perhaps the thread would get noticed by the DT
> maintainers if posted there (or perhaps just CC them)?
I added them to Cc.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-05-06 17:47 ` Uwe Kleine-König
0 siblings, 0 replies; 57+ messages in thread
From: Uwe Kleine-König @ 2015-05-06 17:47 UTC (permalink / raw)
To: linux-arm-kernel
Hello Stephen,
On Wed, May 06, 2015 at 09:57:37AM -0600, Stephen Warren wrote:
> On 05/06/2015 02:09 AM, Uwe Kleine-K?nig wrote:
> >On Wed, May 06, 2015 at 09:53:55AM +0200, Marc Dietrich wrote:
> >>Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-K?nig:
> >>>On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
> >>>>So what about adding a new property "i2c-slave-reg"? This does not only
> >>>>prevent the confusion above, but also makes it very clear that this node
> >>>>is an I2C slave without the need to encode that somehow in the
> >>>>compatible property (although it probably should be described there as
> >>>>well, still).
> >>>
> >>>I admit I didn't follow the discussions referenced in the footnotes, but
> >>>I wonder if the slave part should be added to the device tree at all.
> >>>AFAICT it could (and so should) be completely userspace-defined which
> >>>slave driver is used on which address. I imagine that for most
> >>>controllers the bus addresses to use can be chosen more or less freely.
> >>>So what am I missing?
> >>
> >>if you had read the footnotes you would know :-) Our usecase is connect an
> >>embeedded controller via i2c to the host soc, similar to cros-ec, but here the
> >>ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other
> >>stuff, for which the drivers are best implemented in kernel code AFAIK.
> >
> >Right, the driver might sensibly be implemented in kernel space. But I'd
> >vote that you still need to do the binding of these drivers to your
> >slave controller from userspace. Then there is no need to specify
> >anything in your dtb.
>
> I think the set of I2C slave devices that are implemented by the
> Linux system can reasonably be considered part of the HW definition.
> Most DT content to date has been a definition of the HW that's
> available to SW, but at least in this case, this I2C slave device is
> something that must be implemented (admittedly in conjunction with
> SW) using the I2C slave HW on the main SoC, in order for the overall
> HW to work as intended.
I'm not convinced. Why must it be implemented? And is it too late if the
setup is done by userspace?
The devicetree description of a flash chip doesn't include the rootfs
content either, although a working rootfs is critical for most operating
systems. The devicetree description of an ethernet adapter doesn't
include its network setup although an ethernet adapter hardly makes any
sense without an IP.
> BTW, I believe devicetree-spec at vger.kernel.org was created to
> address subsystem-level DT schema questions like this. It's much
> lower volume, so perhaps the thread would get noticed by the DT
> maintainers if posted there (or perhaps just CC them)?
I added them to Cc.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-06 17:47 ` Uwe Kleine-König
@ 2015-05-06 18:35 ` Stephen Warren
-1 siblings, 0 replies; 57+ messages in thread
From: Stephen Warren @ 2015-05-06 18:35 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Marc Dietrich, devicetree-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang,
Greg Kroah-Hartman, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Andrey Danin,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-spec-u79uwXL29TY76Z2rM5mHXA
On 05/06/2015 11:47 AM, Uwe Kleine-König wrote:
> Hello Stephen,
>
> On Wed, May 06, 2015 at 09:57:37AM -0600, Stephen Warren wrote:
>> On 05/06/2015 02:09 AM, Uwe Kleine-König wrote:
>>> On Wed, May 06, 2015 at 09:53:55AM +0200, Marc Dietrich wrote:
>>>> Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-König:
>>>>> On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
>>>>>> So what about adding a new property "i2c-slave-reg"? This does not only
>>>>>> prevent the confusion above, but also makes it very clear that this node
>>>>>> is an I2C slave without the need to encode that somehow in the
>>>>>> compatible property (although it probably should be described there as
>>>>>> well, still).
>>>>>
>>>>> I admit I didn't follow the discussions referenced in the footnotes, but
>>>>> I wonder if the slave part should be added to the device tree at all.
>>>>> AFAICT it could (and so should) be completely userspace-defined which
>>>>> slave driver is used on which address. I imagine that for most
>>>>> controllers the bus addresses to use can be chosen more or less freely.
>>>>> So what am I missing?
>>>>
>>>> if you had read the footnotes you would know :-) Our usecase is connect an
>>>> embeedded controller via i2c to the host soc, similar to cros-ec, but here the
>>>> ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other
>>>> stuff, for which the drivers are best implemented in kernel code AFAIK.
>>>
>>> Right, the driver might sensibly be implemented in kernel space. But I'd
>>> vote that you still need to do the binding of these drivers to your
>>> slave controller from userspace. Then there is no need to specify
>>> anything in your dtb.
>>
>> I think the set of I2C slave devices that are implemented by the
>> Linux system can reasonably be considered part of the HW definition.
>> Most DT content to date has been a definition of the HW that's
>> available to SW, but at least in this case, this I2C slave device is
>> something that must be implemented (admittedly in conjunction with
>> SW) using the I2C slave HW on the main SoC, in order for the overall
>> HW to work as intended.
>
> I'm not convinced. Why must it be implemented?
The other HW in the system expects the I2C slave device to exist. I
believe that makes it a HW level description, and hence suitable to
represent in DT.
Representing this in DT saves user-space from requiring board-specific
knowledge. The kernel should be abstracting HW as best it can, so that
user-space can be entirely generic, supporting classes of devices, not
enumeration/instantiation of devices. Assuming a system didn't store its
root filesystem on a particular SD card or USB controller, we could
instantiate those controllers from user-space too (or an initrd even if
the root fs was there). However, we choose not to do that...
In the specific case of nvec (the I2C slave device that triggered this
thread) and I dare say other devices too, there is more to the device
than just the I2C slave device. There's also a GPIO that serves to
provide synchronization between the SoC and external device.
> And is it too late if the setup is done by userspace?
The primary purpose of nvec is to provide keyboard and mouse (touchpad)
input, so enabling this once user space was booted would probably work.
However, the device should likely be enabled by the initrd not the root
filesystem (if such a split exists in a given distro) so that the user
can actually interact with any initrd shell if the boot fails. That
would bloat the initrd. System power management (power off at least)
goes through the same device, so deferring that might not be great, but
I suppose there likely wouldn't be a "turn power off" event requested
until user-space was booted.
> The devicetree description of a flash chip doesn't include the rootfs
> content either, although a working rootfs is critical for most operating
> systems. The devicetree description of an ethernet adapter doesn't
> include its network setup although an ethernet adapter hardly makes any
> sense without an IP.
>
>> BTW, I believe devicetree-spec-u79uwXL29TY76Z2rM5mHXA@public.gmane.org was created to
>> address subsystem-level DT schema questions like this. It's much
>> lower volume, so perhaps the thread would get noticed by the DT
>> maintainers if posted there (or perhaps just CC them)?
>
> I added them to Cc.
^ permalink raw reply [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-05-06 18:35 ` Stephen Warren
0 siblings, 0 replies; 57+ messages in thread
From: Stephen Warren @ 2015-05-06 18:35 UTC (permalink / raw)
To: linux-arm-kernel
On 05/06/2015 11:47 AM, Uwe Kleine-K?nig wrote:
> Hello Stephen,
>
> On Wed, May 06, 2015 at 09:57:37AM -0600, Stephen Warren wrote:
>> On 05/06/2015 02:09 AM, Uwe Kleine-K?nig wrote:
>>> On Wed, May 06, 2015 at 09:53:55AM +0200, Marc Dietrich wrote:
>>>> Am Mittwoch, 6. Mai 2015, 08:59:28 schrieb Uwe Kleine-K?nig:
>>>>> On Tue, May 05, 2015 at 12:55:13PM +0200, Wolfram Sang wrote:
>>>>>> So what about adding a new property "i2c-slave-reg"? This does not only
>>>>>> prevent the confusion above, but also makes it very clear that this node
>>>>>> is an I2C slave without the need to encode that somehow in the
>>>>>> compatible property (although it probably should be described there as
>>>>>> well, still).
>>>>>
>>>>> I admit I didn't follow the discussions referenced in the footnotes, but
>>>>> I wonder if the slave part should be added to the device tree at all.
>>>>> AFAICT it could (and so should) be completely userspace-defined which
>>>>> slave driver is used on which address. I imagine that for most
>>>>> controllers the bus addresses to use can be chosen more or less freely.
>>>>> So what am I missing?
>>>>
>>>> if you had read the footnotes you would know :-) Our usecase is connect an
>>>> embeedded controller via i2c to the host soc, similar to cros-ec, but here the
>>>> ec is the i2c master. The ec connects keyboard, mouse, pwrmngt, and other
>>>> stuff, for which the drivers are best implemented in kernel code AFAIK.
>>>
>>> Right, the driver might sensibly be implemented in kernel space. But I'd
>>> vote that you still need to do the binding of these drivers to your
>>> slave controller from userspace. Then there is no need to specify
>>> anything in your dtb.
>>
>> I think the set of I2C slave devices that are implemented by the
>> Linux system can reasonably be considered part of the HW definition.
>> Most DT content to date has been a definition of the HW that's
>> available to SW, but at least in this case, this I2C slave device is
>> something that must be implemented (admittedly in conjunction with
>> SW) using the I2C slave HW on the main SoC, in order for the overall
>> HW to work as intended.
>
> I'm not convinced. Why must it be implemented?
The other HW in the system expects the I2C slave device to exist. I
believe that makes it a HW level description, and hence suitable to
represent in DT.
Representing this in DT saves user-space from requiring board-specific
knowledge. The kernel should be abstracting HW as best it can, so that
user-space can be entirely generic, supporting classes of devices, not
enumeration/instantiation of devices. Assuming a system didn't store its
root filesystem on a particular SD card or USB controller, we could
instantiate those controllers from user-space too (or an initrd even if
the root fs was there). However, we choose not to do that...
In the specific case of nvec (the I2C slave device that triggered this
thread) and I dare say other devices too, there is more to the device
than just the I2C slave device. There's also a GPIO that serves to
provide synchronization between the SoC and external device.
> And is it too late if the setup is done by userspace?
The primary purpose of nvec is to provide keyboard and mouse (touchpad)
input, so enabling this once user space was booted would probably work.
However, the device should likely be enabled by the initrd not the root
filesystem (if such a split exists in a given distro) so that the user
can actually interact with any initrd shell if the boot fails. That
would bloat the initrd. System power management (power off at least)
goes through the same device, so deferring that might not be great, but
I suppose there likely wouldn't be a "turn power off" event requested
until user-space was booted.
> The devicetree description of a flash chip doesn't include the rootfs
> content either, although a working rootfs is critical for most operating
> systems. The devicetree description of an ethernet adapter doesn't
> include its network setup although an ethernet adapter hardly makes any
> sense without an IP.
>
>> BTW, I believe devicetree-spec at vger.kernel.org was created to
>> address subsystem-level DT schema questions like this. It's much
>> lower volume, so perhaps the thread would get noticed by the DT
>> maintainers if posted there (or perhaps just CC them)?
>
> I added them to Cc.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-06 16:17 ` Wolfram Sang
@ 2015-05-19 0:37 ` Rob Herring
-1 siblings, 0 replies; 57+ messages in thread
From: Rob Herring @ 2015-05-19 0:37 UTC (permalink / raw)
To: Wolfram Sang
Cc: Stephen Warren, Marc Dietrich, Andrey Danin,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
On Wed, May 6, 2015 at 11:17 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
>> The container node has a #address-cells property for this very reason. It's
>> perfectly well-defined how to split up a property containing a large number
>> of cells into separate values, by using the value of #address-cells. Plus,
>> the canonical formatting (albeit not enforced by the DT compiler) for a
>> property that contains an array of entries, each 2 cells in size, would be:
>>
>> reg = <0 0x1a>, <0 0x40>, <0 0x48>;
>>
>> rather than:
>>
>> reg = <0 0x1a 0 0x40 0 0x48>;
>>
>> ... so it's quite simple to make it very human-readable too.
>
> I give in to the flag idea. I also noticed that we'd need another flag
> anyhow to mark 10 bit addresses. I am still thinking between using two
> address-cells in that case (clean seperation between address and flags)
> or to encode the flags as MSB in the current address (all busses will
> have same address-cells and child description, less code paths and no
> overhead in dtbs).
Reading thru the thread, this seems good to me. I would go with adding
flags in the MSB of the reg cell rather than adding a cell.
Rob
--
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 [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-05-19 0:37 ` Rob Herring
0 siblings, 0 replies; 57+ messages in thread
From: Rob Herring @ 2015-05-19 0:37 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, May 6, 2015 at 11:17 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> The container node has a #address-cells property for this very reason. It's
>> perfectly well-defined how to split up a property containing a large number
>> of cells into separate values, by using the value of #address-cells. Plus,
>> the canonical formatting (albeit not enforced by the DT compiler) for a
>> property that contains an array of entries, each 2 cells in size, would be:
>>
>> reg = <0 0x1a>, <0 0x40>, <0 0x48>;
>>
>> rather than:
>>
>> reg = <0 0x1a 0 0x40 0 0x48>;
>>
>> ... so it's quite simple to make it very human-readable too.
>
> I give in to the flag idea. I also noticed that we'd need another flag
> anyhow to mark 10 bit addresses. I am still thinking between using two
> address-cells in that case (clean seperation between address and flags)
> or to encode the flags as MSB in the current address (all busses will
> have same address-cells and child description, less code paths and no
> overhead in dtbs).
Reading thru the thread, this seems good to me. I would go with adding
flags in the MSB of the reg cell rather than adding a cell.
Rob
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-19 0:37 ` Rob Herring
@ 2015-05-19 6:16 ` Wolfram Sang
-1 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-05-19 6:16 UTC (permalink / raw)
To: Rob Herring
Cc: Stephen Warren, Marc Dietrich, Andrey Danin,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
[-- Attachment #1: Type: text/plain, Size: 729 bytes --]
> > I give in to the flag idea. I also noticed that we'd need another flag
> > anyhow to mark 10 bit addresses. I am still thinking between using two
> > address-cells in that case (clean seperation between address and flags)
> > or to encode the flags as MSB in the current address (all busses will
> > have same address-cells and child description, less code paths and no
> > overhead in dtbs).
>
> Reading thru the thread, this seems good to me. I would go with adding
> flags in the MSB of the reg cell rather than adding a cell.
Thanks. I was leaning to the MSB idea, too, and am currently checking
how the code would look like. Feels much better to do knowing that it is
basically upstream compatible ;)
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-05-19 6:16 ` Wolfram Sang
0 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-05-19 6:16 UTC (permalink / raw)
To: linux-arm-kernel
> > I give in to the flag idea. I also noticed that we'd need another flag
> > anyhow to mark 10 bit addresses. I am still thinking between using two
> > address-cells in that case (clean seperation between address and flags)
> > or to encode the flags as MSB in the current address (all busses will
> > have same address-cells and child description, less code paths and no
> > overhead in dtbs).
>
> Reading thru the thread, this seems good to me. I would go with adding
> flags in the MSB of the reg cell rather than adding a cell.
Thanks. I was leaning to the MSB idea, too, and am currently checking
how the code would look like. Feels much better to do knowing that it is
basically upstream compatible ;)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150519/d60fd9fe/attachment.sig>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-05-19 6:16 ` Wolfram Sang
@ 2015-07-16 9:03 ` Andrey Danin
-1 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-07-16 9:03 UTC (permalink / raw)
To: Wolfram Sang
Cc: devicetree, Stephen Warren, Greg Kroah-Hartman, Marc Dietrich,
linux-i2c, linux-tegra, linux-arm-kernel
Hello Wolfram,
On 19.05.2015 9:16, Wolfram Sang wrote:
>
>>> I give in to the flag idea. I also noticed that we'd need another flag
>>> anyhow to mark 10 bit addresses. I am still thinking between using two
>>> address-cells in that case (clean seperation between address and flags)
>>> or to encode the flags as MSB in the current address (all busses will
>>> have same address-cells and child description, less code paths and no
>>> overhead in dtbs).
>>
>> Reading thru the thread, this seems good to me. I would go with adding
>> flags in the MSB of the reg cell rather than adding a cell.
>
> Thanks. I was leaning to the MSB idea, too, and am currently checking
> how the code would look like. Feels much better to do knowing that it is
> basically upstream compatible ;)
>
Did you have enough time to check this ?
(sorry for the noise, if I missed patch(es) in the ml)
Thanks.
^ permalink raw reply [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-07-16 9:03 ` Andrey Danin
0 siblings, 0 replies; 57+ messages in thread
From: Andrey Danin @ 2015-07-16 9:03 UTC (permalink / raw)
To: linux-arm-kernel
Hello Wolfram,
On 19.05.2015 9:16, Wolfram Sang wrote:
>
>>> I give in to the flag idea. I also noticed that we'd need another flag
>>> anyhow to mark 10 bit addresses. I am still thinking between using two
>>> address-cells in that case (clean seperation between address and flags)
>>> or to encode the flags as MSB in the current address (all busses will
>>> have same address-cells and child description, less code paths and no
>>> overhead in dtbs).
>>
>> Reading thru the thread, this seems good to me. I would go with adding
>> flags in the MSB of the reg cell rather than adding a cell.
>
> Thanks. I was leaning to the MSB idea, too, and am currently checking
> how the code would look like. Feels much better to do knowing that it is
> basically upstream compatible ;)
>
Did you have enough time to check this ?
(sorry for the noise, if I missed patch(es) in the ml)
Thanks.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: How to encode being an I2C slave in DT?
2015-07-16 9:03 ` Andrey Danin
@ 2015-07-16 18:14 ` Wolfram Sang
-1 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-07-16 18:14 UTC (permalink / raw)
To: Andrey Danin
Cc: Rob Herring, Stephen Warren, Marc Dietrich,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
[-- Attachment #1: Type: text/plain, Size: 147 bytes --]
> Did you have enough time to check this ?
> (sorry for the noise, if I missed patch(es) in the ml)
Nice coincidence, scheduled for tomorrow...
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* How to encode being an I2C slave in DT?
@ 2015-07-16 18:14 ` Wolfram Sang
0 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-07-16 18:14 UTC (permalink / raw)
To: linux-arm-kernel
> Did you have enough time to check this ?
> (sorry for the noise, if I missed patch(es) in the ml)
Nice coincidence, scheduled for tomorrow...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150716/ef4382ec/attachment.sig>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2 1/4] i2c: tegra: implement slave mode
2015-04-03 19:46 ` Wolfram Sang
(?)
@ 2015-07-20 9:50 ` Wolfram Sang
-1 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-07-20 9:50 UTC (permalink / raw)
To: Andrey Danin
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
ac100-oU9gvf+ajcQ97yFScArB1dHuzzzSOjJt, Laxman Dewangan,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Russell King, Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]
On Fri, Apr 03, 2015 at 09:46:26PM +0200, Wolfram Sang wrote:
> > +static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
> > +{
> > + i2c_writel(i2c_dev, val, I2C_SL_RCVD);
> > +
> > + /*
> > + * TODO: A correct fix needs to be found for this.
> > + *
> > + * We experience less incomplete messages with this delay than without
> > + * it, but we don't know why. Help is appreciated.
> > + */
>
> Uh oh. So I assume this is another reason for staging?
I think we can live with this upstream, no need for staging.
> > + if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
> > + return -EINVAL;
>
> The core checks for slave_cb being valid.
>
> > + i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
>
> You could use value here, too, or?
>
> > + if (!tegra_i2c_slave_isr(irq, i2c_dev))
> > + return IRQ_HANDLED;
>
> Minor nit: I'd prefer == 0 here, since it reads "if not slave_isr return
> handled".
>
> > + if (slave->addr > 0x7F)
> > + addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
>
> Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you
> need to check for the flag (slave->flags & I2C_CLIENT_TEN).
If you'd fix up these and resend, I'll happily include this patch for 4.3
already.
Thanks,
Wolfram
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2 1/4] i2c: tegra: implement slave mode
@ 2015-07-20 9:50 ` Wolfram Sang
0 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-07-20 9:50 UTC (permalink / raw)
To: Andrey Danin
Cc: devicetree, linux-i2c, linux-arm-kernel, linux-tegra,
linux-kernel, ac100, Laxman Dewangan, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
Stephen Warren, Thierry Reding, Alexandre Courbot,
Greg Kroah-Hartman, Julian Andres Klode, Marc Dietrich
[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]
On Fri, Apr 03, 2015 at 09:46:26PM +0200, Wolfram Sang wrote:
> > +static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
> > +{
> > + i2c_writel(i2c_dev, val, I2C_SL_RCVD);
> > +
> > + /*
> > + * TODO: A correct fix needs to be found for this.
> > + *
> > + * We experience less incomplete messages with this delay than without
> > + * it, but we don't know why. Help is appreciated.
> > + */
>
> Uh oh. So I assume this is another reason for staging?
I think we can live with this upstream, no need for staging.
> > + if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
> > + return -EINVAL;
>
> The core checks for slave_cb being valid.
>
> > + i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
>
> You could use value here, too, or?
>
> > + if (!tegra_i2c_slave_isr(irq, i2c_dev))
> > + return IRQ_HANDLED;
>
> Minor nit: I'd prefer == 0 here, since it reads "if not slave_isr return
> handled".
>
> > + if (slave->addr > 0x7F)
> > + addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
>
> Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you
> need to check for the flag (slave->flags & I2C_CLIENT_TEN).
If you'd fix up these and resend, I'll happily include this patch for 4.3
already.
Thanks,
Wolfram
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 1/4] i2c: tegra: implement slave mode
@ 2015-07-20 9:50 ` Wolfram Sang
0 siblings, 0 replies; 57+ messages in thread
From: Wolfram Sang @ 2015-07-20 9:50 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 03, 2015 at 09:46:26PM +0200, Wolfram Sang wrote:
> > +static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
> > +{
> > + i2c_writel(i2c_dev, val, I2C_SL_RCVD);
> > +
> > + /*
> > + * TODO: A correct fix needs to be found for this.
> > + *
> > + * We experience less incomplete messages with this delay than without
> > + * it, but we don't know why. Help is appreciated.
> > + */
>
> Uh oh. So I assume this is another reason for staging?
I think we can live with this upstream, no need for staging.
> > + if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
> > + return -EINVAL;
>
> The core checks for slave_cb being valid.
>
> > + i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
>
> You could use value here, too, or?
>
> > + if (!tegra_i2c_slave_isr(irq, i2c_dev))
> > + return IRQ_HANDLED;
>
> Minor nit: I'd prefer == 0 here, since it reads "if not slave_isr return
> handled".
>
> > + if (slave->addr > 0x7F)
> > + addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
>
> Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you
> need to check for the flag (slave->flags & I2C_CLIENT_TEN).
If you'd fix up these and resend, I'll happily include this patch for 4.3
already.
Thanks,
Wolfram
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/ca98dfad/attachment.sig>
^ permalink raw reply [flat|nested] 57+ messages in thread
end of thread, other threads:[~2015-07-20 9:51 UTC | newest]
Thread overview: 57+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-30 20:00 [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegra i2c Andrey Danin
2015-03-30 20:00 ` Andrey Danin
2015-03-30 20:00 ` Andrey Danin
2015-03-30 20:00 ` [PATCH v2 1/4] i2c: tegra: implement slave mode Andrey Danin
2015-03-30 20:00 ` Andrey Danin
2015-03-30 20:00 ` Andrey Danin
[not found] ` <1427745615-5428-2-git-send-email-danindrey-JGs/UdohzUI@public.gmane.org>
2015-04-03 19:46 ` Wolfram Sang
2015-04-03 19:46 ` Wolfram Sang
2015-04-03 19:46 ` Wolfram Sang
2015-07-20 9:50 ` Wolfram Sang
2015-07-20 9:50 ` Wolfram Sang
2015-07-20 9:50 ` Wolfram Sang
2015-03-30 20:00 ` [PATCH v2 2/4] staging/nvec: reimplement on top of tegra i2c driver Andrey Danin
2015-03-30 20:00 ` Andrey Danin
2015-03-30 20:00 ` Andrey Danin
[not found] ` <1427745615-5428-3-git-send-email-danindrey-JGs/UdohzUI@public.gmane.org>
2015-04-03 19:57 ` Wolfram Sang
2015-04-03 19:57 ` Wolfram Sang
2015-04-03 19:57 ` Wolfram Sang
2015-03-30 20:00 ` [PATCH v2 3/4] staging/nvec: remove old code Andrey Danin
2015-03-30 20:00 ` Andrey Danin
2015-03-30 20:00 ` Andrey Danin
2015-03-30 20:00 ` [PATCH v2 4/4] dt: paz00: define nvec as child of i2c bus Andrey Danin
2015-03-30 20:00 ` Andrey Danin
2015-03-30 20:00 ` Andrey Danin
2015-04-03 19:46 ` [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegra i2c Wolfram Sang
2015-04-03 19:46 ` Wolfram Sang
2015-04-07 11:37 ` [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegrai2c Marc Dietrich
2015-04-07 11:37 ` Marc Dietrich
2015-04-10 21:35 ` Wolfram Sang
2015-04-10 21:35 ` Wolfram Sang
2015-05-05 10:55 ` How to encode being an I2C slave in DT? Wolfram Sang
2015-05-05 20:07 ` Stephen Warren
2015-05-05 20:07 ` Stephen Warren
[not found] ` <554922EF.3050906-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
2015-05-06 16:17 ` Wolfram Sang
2015-05-06 16:17 ` Wolfram Sang
2015-05-06 17:01 ` Stephen Warren
2015-05-06 17:01 ` Stephen Warren
2015-05-19 0:37 ` Rob Herring
2015-05-19 0:37 ` Rob Herring
[not found] ` <CAL_Jsq+iyeK-bV3ggcJv8Cw3EU71fkBo1wqYZnhCoh2-b_tO-Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-05-19 6:16 ` Wolfram Sang
2015-05-19 6:16 ` Wolfram Sang
2015-07-16 9:03 ` Andrey Danin
2015-07-16 9:03 ` Andrey Danin
[not found] ` <55A7736F.5090802-JGs/UdohzUI@public.gmane.org>
2015-07-16 18:14 ` Wolfram Sang
2015-07-16 18:14 ` Wolfram Sang
2015-05-06 6:59 ` Uwe Kleine-König
2015-05-06 6:59 ` Uwe Kleine-König
[not found] ` <20150506065928.GP25193-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-05-06 7:53 ` Marc Dietrich
2015-05-06 7:53 ` Marc Dietrich
2015-05-06 8:09 ` Uwe Kleine-König
2015-05-06 8:09 ` Uwe Kleine-König
[not found] ` <20150506080951.GS25193-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-05-06 15:57 ` Stephen Warren
2015-05-06 15:57 ` Stephen Warren
[not found] ` <554A39F1.9060507-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
2015-05-06 17:47 ` Uwe Kleine-König
2015-05-06 17:47 ` Uwe Kleine-König
[not found] ` <20150506174757.GZ25193-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-05-06 18:35 ` Stephen Warren
2015-05-06 18:35 ` Stephen Warren
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.