Re: [PATCH] i2c: omap: ensure writes to dev->buf_len are ordered
Hi, On Thu, Oct 25, 2012 at 09:38:06AM -0700, Kevin Hilman wrote: > +Paul > > Felipe Balbi writes: > > > if we allow compiler reorder our writes, we could > > fall into a situation where dev->buf_len is reset > > for no apparent reason. > > Any chance this would help with the bug Paul found with I2C timeouts on > beagle userspace startup? I replied to the original thread asking the same ;-) -- balbi signature.asc Description: Digital signature
Re: [PATCH] i2c: omap: ensure writes to dev->buf_len are ordered
+Paul
Felipe Balbi writes:
> if we allow compiler reorder our writes, we could
> fall into a situation where dev->buf_len is reset
> for no apparent reason.
Any chance this would help with the bug Paul found with I2C timeouts on
beagle userspace startup?
Kevin
> This bug was found with a simple script which would
> transfer data to an i2c client from 1 to 1024 bytes
> (a simple for loop), when we got to transfer sizes
> bigger than the fifo size, dev->buf_len was reset
> to zero before we had an oportunity to handle XDR
> Interrupt. Because dev->buf_len was zero, we entered
> omap_i2c_transmit_data() to transfer zero bytes,
> which would mean we would just silently exit
> omap_i2c_transmit_data() without actually writing
> anything to DATA register. That would cause XDR
> IRQ to trigger forever and we would never transfer
> the remaining bytes.
>
> After adding the memory barrier, we also drop resetting
> dev->buf_len to zero in omap_i2c_xfer_msg() because
> both omap_i2c_transmit_data() and omap_i2c_receive_data()
> will act until dev->buf_len reaches zero, rendering the
> other write in omap_i2c_xfer_msg() redundant.
>
> This patch has been tested with pandaboard for a few
> iterations of the script mentioned above.
>
> Signed-off-by: Felipe Balbi
> ---
>
> This bug has been there forever, but it's quite annoying.
> I think it deserves being pushed upstream during this -rc
> cycle, but if Wolfram decides to wait until v3.8, I don't
> mind.
>
> drivers/i2c/busses/i2c-omap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index db31eae..1ec4e6e 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -521,6 +521,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> /* REVISIT: Could the STB bit of I2C_CON be used with probing? */
> dev->buf = msg->buf;
> dev->buf_len = msg->len;
> + wmb();
>
> omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev->buf_len);
>
> @@ -579,7 +580,6 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>*/
> timeout = wait_for_completion_timeout(&dev->cmd_complete,
> OMAP_I2C_TIMEOUT);
> - dev->buf_len = 0;
> if (timeout == 0) {
> dev_err(dev->dev, "controller timed out\n");
> omap_i2c_init(dev);
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] ARM: dts: cfa10049: Add the i2c muxer buses to the CFA-10049
This will allow to add the 3 Nuvoton NAU7802 ADCs and the NXP PCA9555
GPIO expander eventually.
Signed-off-by: Maxime Ripard
---
arch/arm/boot/dts/imx28-cfa10049.dts | 24
1 file changed, 24 insertions(+)
diff --git a/arch/arm/boot/dts/imx28-cfa10049.dts
b/arch/arm/boot/dts/imx28-cfa10049.dts
index 97ee098..2cda823 100644
--- a/arch/arm/boot/dts/imx28-cfa10049.dts
+++ b/arch/arm/boot/dts/imx28-cfa10049.dts
@@ -76,6 +76,30 @@
status = "okay";
};
+ i2cmux {
+ compatible = "i2c-mux-gpio";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
+ i2c-parent = <&i2c1>;
+
+ i2c@0 {
+ reg = <0>;
+ };
+
+ i2c@1 {
+ reg = <1>;
+ };
+
+ i2c@2 {
+ reg = <2>;
+ };
+
+ i2c@3 {
+ reg = <3>;
+ };
+ };
+
usbphy1: usbphy@8007e000 {
status = "okay";
};
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] i2c: mux: Add dt support to i2c-mux-gpio driver
Allow the i2c-mux-gpio to be used by a device tree enabled device. The
bindings are inspired by the one found in the i2c-mux-pinctrl driver.
Signed-off-by: Maxime Ripard
Reviewed-by: Stephen Warren
Acked-by: Peter Korsgaard
---
.../devicetree/bindings/i2c/i2c-mux-gpio.txt | 81 +++
drivers/i2c/muxes/i2c-mux-gpio.c | 146 +++-
2 files changed, 196 insertions(+), 31 deletions(-)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
new file mode 100644
index 000..66709a8
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt
@@ -0,0 +1,81 @@
+GPIO-based I2C Bus Mux
+
+This binding describes an I2C bus multiplexer that uses GPIOs to
+route the I2C signals.
+
+ +-+ +-+
+ | dev | | dev |
++++-+ +-+
+| SoC| ||
+|| /++
+| +--+ | +--+child bus A, on GPIO value set to 0
+| | I2C |-|--| Mux |
+| +--+ | +--+---+child bus B, on GPIO value set to 1
+|| |\--+++
+| +--+ | | |||
+| | GPIO |-|-++-+ +-+ +-+
+| +--+ | | dev | | dev | | dev |
+++ +-+ +-+ +-+
+
+Required properties:
+- compatible: i2c-mux-gpio
+- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
+ port is connected to.
+- mux-gpios: list of gpios used to control the muxer
+* Standard I2C mux properties. See mux.txt in this directory.
+* I2C child bus nodes. See mux.txt in this directory.
+
+Optional properties:
+- idle-state: value to set the muxer to when idle. When no value is
+ given, it defaults to the last value used.
+
+For each i2c child node, an I2C child bus will be created. They will
+be numbered based on their order in the device tree.
+
+Whenever an access is made to a device on a child bus, the value set
+in the revelant node's reg property will be output using the list of
+GPIOs, the first in the list holding the least-significant value.
+
+If an idle state is defined, using the idle-state (optional) property,
+whenever an access is not being made to a device on a child bus, the
+GPIOs will be set according to the idle value.
+
+If an idle state is not defined, the most recently used value will be
+left programmed into hardware whenever no access is being made to a
+device on a child bus.
+
+Example:
+ i2cmux {
+ compatible = "i2c-mux-gpio";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
+ i2c-parent = <&i2c1>;
+
+ i2c@1 {
+ reg = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ssd1307: oled@3c {
+ compatible = "solomon,ssd1307fb-i2c";
+ reg = <0x3c>;
+ pwms = <&pwm 4 3000>;
+ reset-gpios = <&gpio2 7 1>;
+ reset-active-low;
+ };
+ };
+
+ i2c@3 {
+ reg = <3>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pca9555: pca9555@20 {
+ compatible = "nxp,pca9555";
+ gpio-controller;
+ #gpio-cells = <2>;
+ reg = <0x20>;
+ };
+ };
+ };
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 566a675..e446f05 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -16,6 +16,8 @@
#include
#include
#include
+#include
+#include
struct gpiomux {
struct i2c_adapter *parent;
@@ -57,29 +59,111 @@ static int __devinit match_gpio_chip_by_label(struct
gpio_chip *chip,
return !strcmp(chip->label, data);
}
+#ifdef CONFIG_OF
+static int __devinit i2c_mux_gpio_probe_dt(struct gpiomux *mux,
+ struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct device_node *adapter_np, *child;
+ struct i2c_adapter *adapter;
+ unsigned *values, *gpios;
+ int i = 0;
+
+ if (!np)
+ return -ENODEV;
+
+ adapter_np = of_parse_phandle(np, "i2c-parent", 0);
+ if (!adapter_np) {
+ dev_err(&pdev->dev, "Cannot parse i
[PATCHv7 0/2] ARM: I2C: Add device tree bindings to i2c-mux-gpio
Hi everyone, This patchset adds the device tree entry to the CFA-10049 board of its i2c muxer. This muxer controls sub-buses that contains three Nuvoton NAU7802 ADCs and a NXP PCA955 GPIO expander. Support for these will be added eventually. Thanks, Maxime Changes from v6: - Changed the return value when neither platform data nor dt was available - Fix a sentence in the documentation Maxime Ripard (2): i2c: mux: Add dt support to i2c-mux-gpio driver ARM: dts: cfa10049: Add the i2c muxer buses to the CFA-10049 .../devicetree/bindings/i2c/i2c-mux-gpio.txt | 81 +++ arch/arm/boot/dts/imx28-cfa10049.dts | 24 drivers/i2c/muxes/i2c-mux-gpio.c | 146 +++- 3 files changed, 220 insertions(+), 31 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-gpio.txt -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv4 1/2] i2c: omap: re-factor omap_i2c_init function
re-factor omap_i2c_init() so that we can re-use it for resume.
While at it also remove the bufstate variable as we write it
in omap_i2c_resize_fifo for every transfer.
Signed-off-by: Shubhrajyoti D
---
v4: add spaces for readability
drivers/i2c/busses/i2c-omap.c | 74 +++-
1 files changed, 35 insertions(+), 39 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index be329e9..38acf1a 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -209,7 +209,6 @@ struct omap_i2c_dev {
u16 pscstate;
u16 scllstate;
u16 sclhstate;
- u16 bufstate;
u16 syscstate;
u16 westate;
u16 errata;
@@ -285,9 +284,34 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev
*i2c_dev, int reg)
}
}
+static void __omap_i2c_init(struct omap_i2c_dev *dev)
+{
+
+ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
+
+ /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
+ omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
+
+ /* SCL low and high time values */
+ omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate);
+ omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate);
+ if (dev->rev >= OMAP_I2C_REV_ON_3430_3530)
+ omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
+
+ /* Take the I2C module out of reset: */
+ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
+
+ /*
+* Don't write to this register if the IE state is 0 as it can
+* cause deadlock.
+*/
+ if (dev->iestate)
+ omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
+}
+
static int omap_i2c_init(struct omap_i2c_dev *dev)
{
- u16 psc = 0, scll = 0, sclh = 0, buf = 0;
+ u16 psc = 0, scll = 0, sclh = 0;
u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
unsigned long fclk_rate = 1200;
unsigned long timeout;
@@ -337,11 +361,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
* REVISIT: Some wkup sources might not be needed.
*/
dev->westate = OMAP_I2C_WE_ALL;
- omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
- dev->westate);
}
}
- omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
if (dev->flags & OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
/*
@@ -426,28 +447,18 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
sclh = fclk_rate / (dev->speed * 2) - 7 + psc;
}
- /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
- omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, psc);
-
- /* SCL low and high time values */
- omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll);
- omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh);
-
- /* Take the I2C module out of reset: */
- omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
-
/* Enable interrupts */
dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
OMAP_I2C_IE_NACK | OMAP_I2C_IE_AL) |
((dev->fifo_size) ? (OMAP_I2C_IE_RDR |
OMAP_I2C_IE_XDR) : 0);
- omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
- if (dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
- dev->pscstate = psc;
- dev->scllstate = scll;
- dev->sclhstate = sclh;
- dev->bufstate = buf;
- }
+
+ dev->pscstate = psc;
+ dev->scllstate = scll;
+ dev->sclhstate = sclh;
+
+ __omap_i2c_init(dev);
+
return 0;
}
@@ -1267,23 +1278,8 @@ static int omap_i2c_runtime_resume(struct device *dev)
{
struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
- if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
- omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
- omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_SCLL_REG, _dev->scllstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_SCLH_REG, _dev->sclhstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_BUF_REG, _dev->bufstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_SYSC_REG, _dev->syscstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_WE_REG, _dev->westate);
- omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
- }
-
- /*
-* Don't write to this register if the IE state is 0 as it can
-* cause deadlock.
-*/
- if (_dev->iestate)
- omap_i2c_write_reg(_dev, OMAP_I2C_
[PATCHv4 0/2] i2c: omap: cleanups
Applies on Felipe's series http://www.spinics.net/lists/linux-omap/msg79995.html Tested with Terro sys fix + Felipe's stop sched_clock() during suspend on omap3630 beagle both idle and suspend. Functional testing on omap4sdp. Todo: all the error cases may not need a reset. Shubhrajyoti D (2): i2c: omap: re-factor omap_i2c_init function i2c: omap: make reset a seperate function drivers/i2c/busses/i2c-omap.c | 97 + 1 files changed, 50 insertions(+), 47 deletions(-) -- 1.7.5.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv4 2/2] i2c: omap: make reset a seperate function
Implement reset as a separate function.
This will enable us to make sure that we don't do the
calculation again on every transfer.
Also at probe the reset is not added as the hwmod is doing that
for us.
Signed-off-by: Shubhrajyoti D
---
some of the errors may not need a reset.
will check and post separate patch.
drivers/i2c/busses/i2c-omap.c | 25 -
1 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 38acf1a..a25b7b0 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -309,15 +309,9 @@ static void __omap_i2c_init(struct omap_i2c_dev *dev)
omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
}
-static int omap_i2c_init(struct omap_i2c_dev *dev)
+static int omap_i2c_reset(struct omap_i2c_dev *dev)
{
- u16 psc = 0, scll = 0, sclh = 0;
- u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
- unsigned long fclk_rate = 1200;
unsigned long timeout;
- unsigned long internal_clk = 0;
- struct clk *fclk;
-
if (dev->rev >= OMAP_I2C_OMAP1_REV_2) {
/* Disable I2C controller before soft reset */
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
@@ -363,6 +357,17 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
dev->westate = OMAP_I2C_WE_ALL;
}
}
+ return 0;
+}
+
+static int omap_i2c_init(struct omap_i2c_dev *dev)
+{
+ u16 psc = 0, scll = 0, sclh = 0;
+ u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
+ unsigned long fclk_rate = 1200;
+ unsigned long internal_clk = 0;
+ struct clk *fclk;
+
if (dev->flags & OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
/*
@@ -595,7 +600,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (timeout == 0) {
dev_err(dev->dev, "controller timed out\n");
ret = -ETIMEDOUT;
- omap_i2c_init(dev);
+ omap_i2c_reset(dev);
+ __omap_i2c_init(dev);
goto out;
}
@@ -606,7 +612,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
OMAP_I2C_STAT_XUDF)) {
ret = -EIO;
- omap_i2c_init(dev);
+ omap_i2c_reset(dev);
+ __omap_i2c_init(dev);
goto out;
}
--
1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv4 1/2] i2c: omap: re-factor omap_i2c_init function
re-factor omap_i2c_init() so that we can re-use it for resume.
While at it also remove the bufstate variable as we write it
in omap_i2c_resize_fifo for every transfer.
Signed-off-by: Shubhrajyoti D
---
v4: add spaces for readability
drivers/i2c/busses/i2c-omap.c | 74 +++-
1 files changed, 35 insertions(+), 39 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index be329e9..38acf1a 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -209,7 +209,6 @@ struct omap_i2c_dev {
u16 pscstate;
u16 scllstate;
u16 sclhstate;
- u16 bufstate;
u16 syscstate;
u16 westate;
u16 errata;
@@ -285,9 +284,34 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev
*i2c_dev, int reg)
}
}
+static void __omap_i2c_init(struct omap_i2c_dev *dev)
+{
+
+ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
+
+ /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
+ omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
+
+ /* SCL low and high time values */
+ omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate);
+ omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate);
+ if (dev->rev >= OMAP_I2C_REV_ON_3430_3530)
+ omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
+
+ /* Take the I2C module out of reset: */
+ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
+
+ /*
+* Don't write to this register if the IE state is 0 as it can
+* cause deadlock.
+*/
+ if (dev->iestate)
+ omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
+}
+
static int omap_i2c_init(struct omap_i2c_dev *dev)
{
- u16 psc = 0, scll = 0, sclh = 0, buf = 0;
+ u16 psc = 0, scll = 0, sclh = 0;
u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
unsigned long fclk_rate = 1200;
unsigned long timeout;
@@ -337,11 +361,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
* REVISIT: Some wkup sources might not be needed.
*/
dev->westate = OMAP_I2C_WE_ALL;
- omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
- dev->westate);
}
}
- omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
if (dev->flags & OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
/*
@@ -426,28 +447,18 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
sclh = fclk_rate / (dev->speed * 2) - 7 + psc;
}
- /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
- omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, psc);
-
- /* SCL low and high time values */
- omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll);
- omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh);
-
- /* Take the I2C module out of reset: */
- omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
-
/* Enable interrupts */
dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
OMAP_I2C_IE_NACK | OMAP_I2C_IE_AL) |
((dev->fifo_size) ? (OMAP_I2C_IE_RDR |
OMAP_I2C_IE_XDR) : 0);
- omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
- if (dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
- dev->pscstate = psc;
- dev->scllstate = scll;
- dev->sclhstate = sclh;
- dev->bufstate = buf;
- }
+
+ dev->pscstate = psc;
+ dev->scllstate = scll;
+ dev->sclhstate = sclh;
+
+ __omap_i2c_init(dev);
+
return 0;
}
@@ -1267,23 +1278,8 @@ static int omap_i2c_runtime_resume(struct device *dev)
{
struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
- if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
- omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
- omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_SCLL_REG, _dev->scllstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_SCLH_REG, _dev->sclhstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_BUF_REG, _dev->bufstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_SYSC_REG, _dev->syscstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_WE_REG, _dev->westate);
- omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
- }
-
- /*
-* Don't write to this register if the IE state is 0 as it can
-* cause deadlock.
-*/
- if (_dev->iestate)
- omap_i2c_write_reg(_dev, OMAP_I2C_
[PATCHv4 0/2] i2c: omap: cleanups
Applies on Felipe's series http://www.spinics.net/lists/linux-omap/msg79995.html Tested with Terro sys fix + Felipe's stop sched_clock() during suspend on omap3630 beagle both idle and suspend. Functional testing on omap4sdp. Todo: all the error cases may not need a reset. Shubhrajyoti D (2): i2c: omap: re-factor omap_i2c_init function i2c: omap: make reset a seperate function drivers/i2c/busses/i2c-omap.c | 97 + 1 files changed, 50 insertions(+), 47 deletions(-) -- 1.7.5.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv4 2/2] i2c: omap: make reset a seperate function
Implement reset as a separate function.
This will enable us to make sure that we don't do the
calculation again on every transfer.
Also at probe the reset is not added as the hwmod is doing that
for us.
Signed-off-by: Shubhrajyoti D
---
some of the errors may not need a reset.
will check and post separate patch.
drivers/i2c/busses/i2c-omap.c | 25 -
1 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 38acf1a..a25b7b0 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -309,15 +309,9 @@ static void __omap_i2c_init(struct omap_i2c_dev *dev)
omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
}
-static int omap_i2c_init(struct omap_i2c_dev *dev)
+static int omap_i2c_reset(struct omap_i2c_dev *dev)
{
- u16 psc = 0, scll = 0, sclh = 0;
- u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
- unsigned long fclk_rate = 1200;
unsigned long timeout;
- unsigned long internal_clk = 0;
- struct clk *fclk;
-
if (dev->rev >= OMAP_I2C_OMAP1_REV_2) {
/* Disable I2C controller before soft reset */
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
@@ -363,6 +357,17 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
dev->westate = OMAP_I2C_WE_ALL;
}
}
+ return 0;
+}
+
+static int omap_i2c_init(struct omap_i2c_dev *dev)
+{
+ u16 psc = 0, scll = 0, sclh = 0;
+ u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
+ unsigned long fclk_rate = 1200;
+ unsigned long internal_clk = 0;
+ struct clk *fclk;
+
if (dev->flags & OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
/*
@@ -595,7 +600,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (timeout == 0) {
dev_err(dev->dev, "controller timed out\n");
ret = -ETIMEDOUT;
- omap_i2c_init(dev);
+ omap_i2c_reset(dev);
+ __omap_i2c_init(dev);
goto out;
}
@@ -606,7 +612,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
OMAP_I2C_STAT_XUDF)) {
ret = -EIO;
- omap_i2c_init(dev);
+ omap_i2c_reset(dev);
+ __omap_i2c_init(dev);
goto out;
}
--
1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/7] i2c: omap: wait for transfer completion before sending STP bit
HI, On Thu, Oct 25, 2012 at 06:31:58PM +0530, Santosh Shilimkar wrote: > On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote: > >Later patches will come adding support for > >reporting amount of bytes transferred so that > >client drivers can count how many bytes are > >left to transfer. > > > >This is useful mostly in case of NACKs when > >client driver wants to know exactly which > >byte got NACKed so it doesn't have to resend > >all bytes again. > > > >In order to make that work with OMAP's I2C > >controller, we need to prevent sending STP > >bit until message is transferred. The reason > >behind that is because OMAP_I2C_CNT_REG gets > >reset to original value after STP bit is > >shifted through I2C_SDA line and that would > >prevent us from reading the correct number of > >bytes left to transfer. > > > >The full programming model suggested by IP > >owner was the following: > > > >- start I2C transfer (without STP bit) > >- upon completion or NACK, read I2C_CNT register > >- write STP bit to I2C_CON register > >- wait for ARDY bit > > > >With this patch we're implementing all steps > >except step #2 which will come in a later > >patch adding such support. > > > Will this not break the bisect since CNT and > NACK, completion is added in later patch not really. It keeps current behavior. -- balbi signature.asc Description: Digital signature
Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg
On Thu, Oct 25, 2012 at 03:56:33PM +0200, Jean Delvare wrote: > On Thu, 25 Oct 2012 14:46:09 +0100, Russell King - ARM Linux wrote: > > On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote: > > > On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote: > > > > You also miss one very very very big point. This will break every I2C > > > > using userspace program out there unless it is rebuilt - this change > > > > will > > > > require the exact right version of those userspace programs for the > > > > kernel that they're being used on. > > > > > > How that? The extra field is added in a hole, so we don't change the > > > struct size nor the relative positions of existing fields. Why would > > > user-space care? > > > > You know the layout of that struct for certain across all Linux supported > > architectures, including some of the more obscure ones which may not > > require pointers to be aligned? > > No I don't, I naively supposed it would be fine. I would expect gcc to > always align pointers even when the architecture doesn't need them to > be, for performance reasons, even when it doesn't have to, as long as > attribute packed isn't used. After all, integers don't _have_ to be > aligned on x86, but gcc aligns them. > > The original idea of using the hole in the i2c_msg structure is from > David Brownell, who was apparently familiar with such practice, so I > assumed it was OK. Actually I still assume it is, until someone comes > with an supported architecture where it is not. According to Al Viro, that would be m68k. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg
On Thu, 25 Oct 2012 14:46:09 +0100, Russell King - ARM Linux wrote: > On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote: > > On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote: > > > You also miss one very very very big point. This will break every I2C > > > using userspace program out there unless it is rebuilt - this change will > > > require the exact right version of those userspace programs for the > > > kernel that they're being used on. > > > > How that? The extra field is added in a hole, so we don't change the > > struct size nor the relative positions of existing fields. Why would > > user-space care? > > You know the layout of that struct for certain across all Linux supported > architectures, including some of the more obscure ones which may not > require pointers to be aligned? No I don't, I naively supposed it would be fine. I would expect gcc to always align pointers even when the architecture doesn't need them to be, for performance reasons, even when it doesn't have to, as long as attribute packed isn't used. After all, integers don't _have_ to be aligned on x86, but gcc aligns them. The original idea of using the hole in the i2c_msg structure is from David Brownell, who was apparently familiar with such practice, so I assumed it was OK. Actually I still assume it is, until someone comes with an supported architecture where it is not. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c: omap: make reset a seperate function
On Thursday 25 October 2012 04:25 PM, Felipe Balbi wrote: > overflow, underflow these are data errors personally feel may be removed. > and arbitration lost) will investigate. > and try to make sure if > actually need to reset the controller all the time. I find it really odd > that we're always resetting the IP in every error condition without > actually trying to figure out what's wrong with the driver (if there is > something wrong with the driver, of course). -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg
On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote:
> On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote:
> > On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote:
> > > Hi Felipe, Shubhrajyoti,
> > >
> > > On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> > > > From: Shubhrajyoti D
> > > >
> > > > In case of a NACK, it's wise to tell our clients
> > > > drivers about how many bytes were actually transferred.
> > > >
> > > > Support this by adding an extra field to the struct
> > > > i2c_msg which gets incremented the amount of bytes
> > > > actually transferred.
> > > >
> > > > Signed-off-by: Shubhrajyoti D
> > > > Signed-off-by: Felipe Balbi
> > > > ---
> > > > include/uapi/linux/i2c.h | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> > > > index 0e949cb..4b35c9b 100644
> > > > --- a/include/uapi/linux/i2c.h
> > > > +++ b/include/uapi/linux/i2c.h
> > > > @@ -77,6 +77,7 @@ struct i2c_msg {
> > > > #define I2C_M_NO_RD_ACK0x0800 /* if
> > > > I2C_FUNC_PROTOCOL_MANGLING */
> > > > #define I2C_M_RECV_LEN 0x0400 /* length will be first
> > > > received byte */
> > > > __u16 len; /* msg length
> > > > */
> > > > + __u16 transferred; /* actual bytes transferred
> > > > */
> > > > __u8 *buf; /* pointer to msg data
> > > > */
> > > > };
> > >
> > > On the principle I am fine with this, however you also need to define
> > > who should initialize this field, and to what value.
> >
> > You also miss one very very very big point. This will break every I2C
> > using userspace program out there unless it is rebuilt - this change will
> > require the exact right version of those userspace programs for the
> > kernel that they're being used on.
>
> How that? The extra field is added in a hole, so we don't change the
> struct size nor the relative positions of existing fields. Why would
> user-space care?
You know the layout of that struct for certain across all Linux supported
architectures, including some of the more obscure ones which may not
require pointers to be aligned?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg
On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote:
> On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote:
> > Hi Felipe, Shubhrajyoti,
> >
> > On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> > > From: Shubhrajyoti D
> > >
> > > In case of a NACK, it's wise to tell our clients
> > > drivers about how many bytes were actually transferred.
> > >
> > > Support this by adding an extra field to the struct
> > > i2c_msg which gets incremented the amount of bytes
> > > actually transferred.
> > >
> > > Signed-off-by: Shubhrajyoti D
> > > Signed-off-by: Felipe Balbi
> > > ---
> > > include/uapi/linux/i2c.h | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> > > index 0e949cb..4b35c9b 100644
> > > --- a/include/uapi/linux/i2c.h
> > > +++ b/include/uapi/linux/i2c.h
> > > @@ -77,6 +77,7 @@ struct i2c_msg {
> > > #define I2C_M_NO_RD_ACK 0x0800 /* if
> > > I2C_FUNC_PROTOCOL_MANGLING */
> > > #define I2C_M_RECV_LEN 0x0400 /* length will be first
> > > received byte */
> > > __u16 len; /* msg length */
> > > + __u16 transferred; /* actual bytes transferred */
> > > __u8 *buf; /* pointer to msg data */
> > > };
> >
> > On the principle I am fine with this, however you also need to define
> > who should initialize this field, and to what value.
>
> You also miss one very very very big point. This will break every I2C
> using userspace program out there unless it is rebuilt - this change will
> require the exact right version of those userspace programs for the
> kernel that they're being used on.
How that? The extra field is added in a hole, so we don't change the
struct size nor the relative positions of existing fields. Why would
user-space care?
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg
On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote:
> Hi Felipe, Shubhrajyoti,
>
> On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> > From: Shubhrajyoti D
> >
> > In case of a NACK, it's wise to tell our clients
> > drivers about how many bytes were actually transferred.
> >
> > Support this by adding an extra field to the struct
> > i2c_msg which gets incremented the amount of bytes
> > actually transferred.
> >
> > Signed-off-by: Shubhrajyoti D
> > Signed-off-by: Felipe Balbi
> > ---
> > include/uapi/linux/i2c.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> > index 0e949cb..4b35c9b 100644
> > --- a/include/uapi/linux/i2c.h
> > +++ b/include/uapi/linux/i2c.h
> > @@ -77,6 +77,7 @@ struct i2c_msg {
> > #define I2C_M_NO_RD_ACK0x0800 /* if
> > I2C_FUNC_PROTOCOL_MANGLING */
> > #define I2C_M_RECV_LEN 0x0400 /* length will be first
> > received byte */
> > __u16 len; /* msg length */
> > + __u16 transferred; /* actual bytes transferred */
> > __u8 *buf; /* pointer to msg data */
> > };
>
> On the principle I am fine with this, however you also need to define
> who should initialize this field, and to what value.
You also miss one very very very big point. This will break every I2C
using userspace program out there unless it is rebuilt - this change will
require the exact right version of those userspace programs for the
kernel that they're being used on.
Now that we have the userspace API headers separated, this is now much
easier to detect: a patch which touches a uapi header needs much more
careful consideration than one which doesn't.
So no, strong NAK. This is not how we treat userspace.
If we want to change userspace API then we do it in a sane manner, where
we provide the new functionality in a way that it doesn't break existing
users. There's two ways to do this:
1. Leave the existing struct alone, introduce a new one with new ioctls.
Schedule the removal of the old interfaces for maybe 10 years time.
2. Rename the existing struct (eg struct old_i2c_msg), and create a new
struct called i2c_msg. Rename the existing ioctls to have OLD_ in
their names. Provide the existing ioctls under those names, and
make them print a warning once that userspace programs need updating.
Schedule the removal of the old interfaces for a shorter number of
years than (1);
Remember all those "old" syscalls we have... this is no different from
those.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/7] i2c: omap: reorder exit path of omap_i2c_xfer_msg()
Hi,
Santosh Shilimkar writes:
> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> > just a cleanup patch trying to make exit path
> > more straightforward. No changes otherwise.
> >
> > Signed-off-by: Felipe Balbi
> > ---
> > drivers/i2c/busses/i2c-omap.c | 26 +-
> > 1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index c07d9c4..bea0277 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > {
> > struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
> > unsigned long timeout;
> > + int ret;
[...]
> > + ret = -EREMOTEIO;
> > + goto err;
> > }
> > - return -EIO;
> > +
> > + return 0;
> With initialized value you can use
> return ret;
>
Doing it this way has the advantage, that if an additional error exit
is added it will generate an 'uninitialized variable' warning, if it
fails to set the return value.
Lothar Waßmann
--
___
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | i...@karo-electronics.de
___
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/7] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS
On Thursday 25 October 2012 06:22 PM, Felipe Balbi wrote:
Hi,
On Thu, Oct 25, 2012 at 06:23:57PM +0530, Santosh Shilimkar wrote:
On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
on OMAP4+ we want to read IRQSTATUS_RAW register,
instead of IRQSTATUS. The reason being that IRQSTATUS
will only contain the bits which were enabled on
IRQENABLE_SET and that will break when we need to
poll for a certain bit which wasn't enabled as an
IRQ source.
One such case is after we finish converting to
deferred stop bit, we will have to poll for ARDY
bit before returning control for the client driver
in order to prevent us from trying to start a
transfer on a bus which is already busy.
Note, however, that omap-i2c.c needs a big rework
on register definition and register access. Such
work will be done in a separate series of patches.
Cc: Benoit Cousson
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index b004126..20f9ad6 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev
*i2c_dev,
static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
{
- return __raw_readw(i2c_dev->base +
+ /* if we are OMAP_I2C_IP_VERSION_2, we need to read from
+* IRQSTATUS_RAW, but write to IRQSTATUS
+*/
+ if ((i2c_dev->dtrev == OMAP_I2C_IP_VERSION_2) &&
+ (reg == OMAP_I2C_STAT_REG)) {
Doing this check on every I2C register read seems to
expensive to me. Can you not sort this in init with some offset
which can be 0 or non zero ? Sorry in case this is already dicussed.
could be. I didn't go that route because I'm planning a complete rewrite
of all register accesses. The way it's done today is completely broken
and already expensive (with reg_shift and different map tables and so
on).
If it's really a big of a deal, I can try to find another way, maybe
just adding omap_i2c_read_stat() and limit the version check just to
I2C_STAT reads would do it for now...
Its a hot path since you read many I2C register reads, so getting
rid of that additional check will be good.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/7] i2c: omap: wait for transfer completion before sending STP bit
On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote: Later patches will come adding support for reporting amount of bytes transferred so that client drivers can count how many bytes are left to transfer. This is useful mostly in case of NACKs when client driver wants to know exactly which byte got NACKed so it doesn't have to resend all bytes again. In order to make that work with OMAP's I2C controller, we need to prevent sending STP bit until message is transferred. The reason behind that is because OMAP_I2C_CNT_REG gets reset to original value after STP bit is shifted through I2C_SDA line and that would prevent us from reading the correct number of bytes left to transfer. The full programming model suggested by IP owner was the following: - start I2C transfer (without STP bit) - upon completion or NACK, read I2C_CNT register - write STP bit to I2C_CON register - wait for ARDY bit With this patch we're implementing all steps except step #2 which will come in a later patch adding such support. Will this not break the bisect since CNT and NACK, completion is added in later patch Signed-off-by: Felipe Balbi --- Apart from above, rest of the change follow the change log and looks fine tome. The change is quite drastic so hopefully it has gone through wider testing. Regards santosh -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/7] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS
Hi,
On Thu, Oct 25, 2012 at 06:23:57PM +0530, Santosh Shilimkar wrote:
> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> >on OMAP4+ we want to read IRQSTATUS_RAW register,
> >instead of IRQSTATUS. The reason being that IRQSTATUS
> >will only contain the bits which were enabled on
> >IRQENABLE_SET and that will break when we need to
> >poll for a certain bit which wasn't enabled as an
> >IRQ source.
> >
> >One such case is after we finish converting to
> >deferred stop bit, we will have to poll for ARDY
> >bit before returning control for the client driver
> >in order to prevent us from trying to start a
> >transfer on a bus which is already busy.
> >
> >Note, however, that omap-i2c.c needs a big rework
> >on register definition and register access. Such
> >work will be done in a separate series of patches.
> >
> >Cc: Benoit Cousson
> >Signed-off-by: Felipe Balbi
> >---
> > drivers/i2c/busses/i2c-omap.c | 12 +++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> >index b004126..20f9ad6 100644
> >--- a/drivers/i2c/busses/i2c-omap.c
> >+++ b/drivers/i2c/busses/i2c-omap.c
> >@@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct
> >omap_i2c_dev *i2c_dev,
> >
> > static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
> > {
> >-return __raw_readw(i2c_dev->base +
> >+/* if we are OMAP_I2C_IP_VERSION_2, we need to read from
> >+ * IRQSTATUS_RAW, but write to IRQSTATUS
> >+ */
> >+if ((i2c_dev->dtrev == OMAP_I2C_IP_VERSION_2) &&
> >+(reg == OMAP_I2C_STAT_REG)) {
> Doing this check on every I2C register read seems to
> expensive to me. Can you not sort this in init with some offset
> which can be 0 or non zero ? Sorry in case this is already dicussed.
could be. I didn't go that route because I'm planning a complete rewrite
of all register accesses. The way it's done today is completely broken
and already expensive (with reg_shift and different map tables and so
on).
If it's really a big of a deal, I can try to find another way, maybe
just adding omap_i2c_read_stat() and limit the version check just to
I2C_STAT reads would do it for now...
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg
Hi Felipe, Shubhrajyoti,
On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> From: Shubhrajyoti D
>
> In case of a NACK, it's wise to tell our clients
> drivers about how many bytes were actually transferred.
>
> Support this by adding an extra field to the struct
> i2c_msg which gets incremented the amount of bytes
> actually transferred.
>
> Signed-off-by: Shubhrajyoti D
> Signed-off-by: Felipe Balbi
> ---
> include/uapi/linux/i2c.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> index 0e949cb..4b35c9b 100644
> --- a/include/uapi/linux/i2c.h
> +++ b/include/uapi/linux/i2c.h
> @@ -77,6 +77,7 @@ struct i2c_msg {
> #define I2C_M_NO_RD_ACK 0x0800 /* if
> I2C_FUNC_PROTOCOL_MANGLING */
> #define I2C_M_RECV_LEN 0x0400 /* length will be first
> received byte */
> __u16 len; /* msg length */
> + __u16 transferred; /* actual bytes transferred */
> __u8 *buf; /* pointer to msg data */
> };
On the principle I am fine with this, however you also need to define
who should initialize this field, and to what value.
Not all i2c bus drivers will implement this functionality at first.
Some may even be unable to ever implement it. As soon as i2c chip
drivers start checking this value, you will hit combinations of chip
driver checking the value and bus driver not setting it. And it has to
work.
We have to decide whether it is enough to initialize "transferred" to
0, or if we need a more reliable way to distinguish between "0 bytes
transferred" and "the bus driver can't tell". What's your take on this?
If we need to distinguish, this could be done using a new I2C_FUNC_*
flag, or by initializing "transferred" to (__u16)(-1) instead of 0 for
example.
Then we have to decide whether initialization is done by the individual
drivers, or by i2c-core. By the drivers might be faster, but this may
also mean more code (and bugs more likely) than letting i2c-core handle
it. The exact balance probably depends on the answer to the previous
question (initializing a field to 0 is free in many cases.)
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/7] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS
On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
on OMAP4+ we want to read IRQSTATUS_RAW register,
instead of IRQSTATUS. The reason being that IRQSTATUS
will only contain the bits which were enabled on
IRQENABLE_SET and that will break when we need to
poll for a certain bit which wasn't enabled as an
IRQ source.
One such case is after we finish converting to
deferred stop bit, we will have to poll for ARDY
bit before returning control for the client driver
in order to prevent us from trying to start a
transfer on a bus which is already busy.
Note, however, that omap-i2c.c needs a big rework
on register definition and register access. Such
work will be done in a separate series of patches.
Cc: Benoit Cousson
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index b004126..20f9ad6 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev
*i2c_dev,
static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
{
- return __raw_readw(i2c_dev->base +
+ /* if we are OMAP_I2C_IP_VERSION_2, we need to read from
+* IRQSTATUS_RAW, but write to IRQSTATUS
+*/
+ if ((i2c_dev->dtrev == OMAP_I2C_IP_VERSION_2) &&
+ (reg == OMAP_I2C_STAT_REG)) {
Doing this check on every I2C register read seems to
expensive to me. Can you not sort this in init with some offset
which can be 0 or non zero ? Sorry in case this is already dicussed.
regards
santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/7] i2c: omap: reorder exit path of omap_i2c_xfer_msg()
Hi,
On Thu, Oct 25, 2012 at 06:12:00PM +0530, Santosh Shilimkar wrote:
> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> >just a cleanup patch trying to make exit path
> >more straightforward. No changes otherwise.
> >
> >Signed-off-by: Felipe Balbi
> >---
> > drivers/i2c/busses/i2c-omap.c | 26 +-
> > 1 file changed, 17 insertions(+), 9 deletions(-)
> >
> >diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> >index c07d9c4..bea0277 100644
> >--- a/drivers/i2c/busses/i2c-omap.c
> >+++ b/drivers/i2c/busses/i2c-omap.c
> >@@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > {
> > struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
> > unsigned long timeout;
> >+int ret;
> You might want to initialize the error value to avoid return 0. Compiler
> might be already cribbing for it
>
> > u16 w;
> >
> > dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
> >@@ -582,31 +583,38 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > dev->buf_len = 0;
> > if (timeout == 0) {
> > dev_err(dev->dev, "controller timed out\n");
> >-omap_i2c_init(dev);
> >-return -ETIMEDOUT;
> >+ret = -ETIMEDOUT;
> >+goto err_i2c_init;
> > }
> >
> >-if (likely(!dev->cmd_err))
> >-return 0;
> >-
> Have you have drooped this check completely ?
yes, the idea is to have a single exit point, so all checks below would
be executed.
> > /* We have an error */
> > if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> > OMAP_I2C_STAT_XUDF)) {
> >-omap_i2c_init(dev);
> >-return -EIO;
> >+ret = -EIO;
> >+goto err_i2c_init;
> > }
> >
> > if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
> > if (msg->flags & I2C_M_IGNORE_NAK)
> > return 0;
> >+
> > if (stop) {
> > w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
> > w |= OMAP_I2C_CON_STP;
> > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
> > }
> >-return -EREMOTEIO;
> >+
> >+ret = -EREMOTEIO;
> >+goto err;
> > }
> >-return -EIO;
> >+
> >+return 0;
> With initialized value you can use
> return ret;
hmm, I guess I did that on a follow up patch where I grouped the stop
handling.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v2 3/7] i2c: omap: also complete() when stat becomes zero
Hi, On Thu, Oct 25, 2012 at 06:13:16PM +0530, Santosh Shilimkar wrote: > On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote: > >In case we loop on IRQ handler until stat is > >finally zero, we would end up in a situation > >where all I2C transfers would misteriously > >timeout because we were not calling complete() > >in that situation. > > > >Fix the issue by moving omap_i2c_complete_cmd() > >call inside the 'out' label. > > > >Signed-off-by: Felipe Balbi > >--- > Looks fine. Have you hit this issue in any corner case ? in fact, yes. With a difficult to reproduce situation with drv2665 (one of TI's piezo drivers) I saw that I was missing the ack and all transfers were timing out ;-) -- balbi signature.asc Description: Digital signature
Re: [PATCH v2 3/7] i2c: omap: also complete() when stat becomes zero
On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote: In case we loop on IRQ handler until stat is finally zero, we would end up in a situation where all I2C transfers would misteriously timeout because we were not calling complete() in that situation. Fix the issue by moving omap_i2c_complete_cmd() call inside the 'out' label. Signed-off-by: Felipe Balbi --- Looks fine. Have you hit this issue in any corner case ? Regards santosh -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/7] i2c: omap: reorder exit path of omap_i2c_xfer_msg()
On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
just a cleanup patch trying to make exit path
more straightforward. No changes otherwise.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 26 +-
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c07d9c4..bea0277 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
{
struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
unsigned long timeout;
+ int ret;
You might want to initialize the error value to avoid return 0. Compiler
might be already cribbing for it
u16 w;
dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
@@ -582,31 +583,38 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
dev->buf_len = 0;
if (timeout == 0) {
dev_err(dev->dev, "controller timed out\n");
- omap_i2c_init(dev);
- return -ETIMEDOUT;
+ ret = -ETIMEDOUT;
+ goto err_i2c_init;
}
- if (likely(!dev->cmd_err))
- return 0;
-
Have you have drooped this check completely ?
/* We have an error */
if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
OMAP_I2C_STAT_XUDF)) {
- omap_i2c_init(dev);
- return -EIO;
+ ret = -EIO;
+ goto err_i2c_init;
}
if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
if (msg->flags & I2C_M_IGNORE_NAK)
return 0;
+
if (stop) {
w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
w |= OMAP_I2C_CON_STP;
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
}
- return -EREMOTEIO;
+
+ ret = -EREMOTEIO;
+ goto err;
}
- return -EIO;
+
+ return 0;
With initialized value you can use
return ret;
Regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/7] i2c: omap: no need to access platform_device
On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote: PM callbacks pass our device pointer as argument and we don't need to access the platform_device just to dereference that down to dev->drvdata. instead, just use dev_get_drvdata() directly. Signed-off-by: Felipe Balbi --- Acked-by: Santosh Shilimkar -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/7] i2c: omap: wait for transfer completion before sending STP bit
Later patches will come adding support for
reporting amount of bytes transferred so that
client drivers can count how many bytes are
left to transfer.
This is useful mostly in case of NACKs when
client driver wants to know exactly which
byte got NACKed so it doesn't have to resend
all bytes again.
In order to make that work with OMAP's I2C
controller, we need to prevent sending STP
bit until message is transferred. The reason
behind that is because OMAP_I2C_CNT_REG gets
reset to original value after STP bit is
shifted through I2C_SDA line and that would
prevent us from reading the correct number of
bytes left to transfer.
The full programming model suggested by IP
owner was the following:
- start I2C transfer (without STP bit)
- upon completion or NACK, read I2C_CNT register
- write STP bit to I2C_CON register
- wait for ARDY bit
With this patch we're implementing all steps
except step #2 which will come in a later
patch adding such support.
Signed-off-by: Felipe Balbi
---
Changes since v2:
- remove unnecessary omap_i2c_init() which was added during a rebase.
drivers/i2c/busses/i2c-omap.c | 88 ---
1 file changed, 32 insertions(+), 56 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 20f9ad6..726b916 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -438,9 +438,9 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
/* Enable interrupts */
dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
- OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
- OMAP_I2C_IE_AL) | ((dev->fifo_size) ?
- (OMAP_I2C_IE_RDR | OMAP_I2C_IE_XDR) : 0);
+ OMAP_I2C_IE_NACK | OMAP_I2C_IE_AL) |
+ ((dev->fifo_size) ? (OMAP_I2C_IE_RDR |
+ OMAP_I2C_IE_XDR) : 0);
omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
if (dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
dev->pscstate = psc;
@@ -470,6 +470,22 @@ static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
return 0;
}
+static int omap_i2c_wait_for_ardy(struct omap_i2c_dev *dev)
+{
+ unsigned long timeout;
+
+ timeout = jiffies + OMAP_I2C_TIMEOUT;
+ while (!(omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG) &
OMAP_I2C_STAT_ARDY)) {
+ if (time_after(jiffies, timeout)) {
+ dev_warn(dev->dev, "timeout waiting for access
ready\n");
+ return -ETIMEDOUT;
+ }
+ usleep_range(800, 1200);
+ }
+
+ return 0;
+}
+
static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
{
u16 buf;
@@ -515,7 +531,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
{
struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
unsigned long timeout;
- int ret;
+ int ret = 0;
u16 w;
dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
@@ -556,35 +572,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (!(msg->flags & I2C_M_RD))
w |= OMAP_I2C_CON_TRX;
- if (!dev->b_hw && stop)
- w |= OMAP_I2C_CON_STP;
-
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
/*
-* Don't write stt and stp together on some hardware.
-*/
- if (dev->b_hw && stop) {
- unsigned long delay = jiffies + OMAP_I2C_TIMEOUT;
- u16 con = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
- while (con & OMAP_I2C_CON_STT) {
- con = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
-
- /* Let the user know if i2c is in a bad state */
- if (time_after(jiffies, delay)) {
- dev_err(dev->dev, "controller timed out "
- "waiting for start condition to finish\n");
- return -ETIMEDOUT;
- }
- cpu_relax();
- }
-
- w |= OMAP_I2C_CON_STP;
- w &= ~OMAP_I2C_CON_STT;
- omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
- }
-
- /*
* REVISIT: We should abort the transfer on signals, but the bus goes
* into arbitration and we're currently unable to recover from it.
*/
@@ -594,36 +584,35 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (timeout == 0) {
dev_err(dev->dev, "controller timed out\n");
ret = -ETIMEDOUT;
- goto err_i2c_init;
+ omap_i2c_init(dev);
+ goto out;
}
/* We have an error */
if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
OMAP_I2C_STAT_XUDF)) {
Re: [PATCH v2 5/7] i2c: omap: wait for transfer completion before sending STP bit
Hi,
On Thu, Oct 25, 2012 at 03:25:13PM +0300, Felipe Balbi wrote:
> Later patches will come adding support for
> reporting amount of bytes transferred so that
> client drivers can count how many bytes are
> left to transfer.
>
> This is useful mostly in case of NACKs when
> client driver wants to know exactly which
> byte got NACKed so it doesn't have to resend
> all bytes again.
>
> In order to make that work with OMAP's I2C
> controller, we need to prevent sending STP
> bit until message is transferred. The reason
> behind that is because OMAP_I2C_CNT_REG gets
> reset to original value after STP bit is
> shifted through I2C_SDA line and that would
> prevent us from reading the correct number of
> bytes left to transfer.
>
> The full programming model suggested by IP
> owner was the following:
>
> - start I2C transfer (without STP bit)
> - upon completion or NACK, read I2C_CNT register
> - write STP bit to I2C_CON register
> - wait for ARDY bit
>
> With this patch we're implementing all steps
> except step #2 which will come in a later
> patch adding such support.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/i2c/busses/i2c-omap.c | 89
> ---
> 1 file changed, 33 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 20f9ad6..699fa12 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -438,9 +438,9 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>
> /* Enable interrupts */
> dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
> - OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
> - OMAP_I2C_IE_AL) | ((dev->fifo_size) ?
> - (OMAP_I2C_IE_RDR | OMAP_I2C_IE_XDR) : 0);
> + OMAP_I2C_IE_NACK | OMAP_I2C_IE_AL) |
> + ((dev->fifo_size) ? (OMAP_I2C_IE_RDR |
> + OMAP_I2C_IE_XDR) : 0);
> omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
> if (dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
> dev->pscstate = psc;
> @@ -470,6 +470,22 @@ static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
> return 0;
> }
>
> +static int omap_i2c_wait_for_ardy(struct omap_i2c_dev *dev)
> +{
> + unsigned long timeout;
> +
> + timeout = jiffies + OMAP_I2C_TIMEOUT;
> + while (!(omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG) &
> OMAP_I2C_STAT_ARDY)) {
> + if (time_after(jiffies, timeout)) {
> + dev_warn(dev->dev, "timeout waiting for access
> ready\n");
> + return -ETIMEDOUT;
> + }
> + usleep_range(800, 1200);
> + }
> +
> + return 0;
> +}
> +
> static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool
> is_rx)
> {
> u16 buf;
> @@ -515,7 +531,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> {
> struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
> unsigned long timeout;
> - int ret;
> + int ret = 0;
> u16 w;
>
> dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
> @@ -556,35 +572,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> if (!(msg->flags & I2C_M_RD))
> w |= OMAP_I2C_CON_TRX;
>
> - if (!dev->b_hw && stop)
> - w |= OMAP_I2C_CON_STP;
> -
> omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
>
> /*
> - * Don't write stt and stp together on some hardware.
> - */
> - if (dev->b_hw && stop) {
> - unsigned long delay = jiffies + OMAP_I2C_TIMEOUT;
> - u16 con = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
> - while (con & OMAP_I2C_CON_STT) {
> - con = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
> -
> - /* Let the user know if i2c is in a bad state */
> - if (time_after(jiffies, delay)) {
> - dev_err(dev->dev, "controller timed out "
> - "waiting for start condition to finish\n");
> - return -ETIMEDOUT;
> - }
> - cpu_relax();
> - }
> -
> - w |= OMAP_I2C_CON_STP;
> - w &= ~OMAP_I2C_CON_STT;
> - omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
> - }
> -
> - /*
>* REVISIT: We should abort the transfer on signals, but the bus goes
>* into arbitration and we're currently unable to recover from it.
>*/
> @@ -594,36 +584,36 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> if (timeout == 0) {
> dev_err(dev->dev, "controller timed out\n");
> ret = -ETIMEDOUT;
> - goto err_i2c_init;
> + omap_i2c_init(dev);
> + goto out;
> }
>
> /* We have an error */
> if (dev->cmd_e
[PATCH v2 4/7] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS
on OMAP4+ we want to read IRQSTATUS_RAW register,
instead of IRQSTATUS. The reason being that IRQSTATUS
will only contain the bits which were enabled on
IRQENABLE_SET and that will break when we need to
poll for a certain bit which wasn't enabled as an
IRQ source.
One such case is after we finish converting to
deferred stop bit, we will have to poll for ARDY
bit before returning control for the client driver
in order to prevent us from trying to start a
transfer on a bus which is already busy.
Note, however, that omap-i2c.c needs a big rework
on register definition and register access. Such
work will be done in a separate series of patches.
Cc: Benoit Cousson
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index b004126..20f9ad6 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev
*i2c_dev,
static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
{
- return __raw_readw(i2c_dev->base +
+ /* if we are OMAP_I2C_IP_VERSION_2, we need to read from
+* IRQSTATUS_RAW, but write to IRQSTATUS
+*/
+ if ((i2c_dev->dtrev == OMAP_I2C_IP_VERSION_2) &&
+ (reg == OMAP_I2C_STAT_REG)) {
+ return __raw_readw(i2c_dev->base +
+ ((i2c_dev->regs[reg] - 0x04)
+<< i2c_dev->reg_shift));
+ } else {
+ return __raw_readw(i2c_dev->base +
(i2c_dev->regs[reg] << i2c_dev->reg_shift));
+ }
}
static int omap_i2c_init(struct omap_i2c_dev *dev)
--
1.8.0
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/7] i2c: omap: wait for transfer completion before sending STP bit
Later patches will come adding support for
reporting amount of bytes transferred so that
client drivers can count how many bytes are
left to transfer.
This is useful mostly in case of NACKs when
client driver wants to know exactly which
byte got NACKed so it doesn't have to resend
all bytes again.
In order to make that work with OMAP's I2C
controller, we need to prevent sending STP
bit until message is transferred. The reason
behind that is because OMAP_I2C_CNT_REG gets
reset to original value after STP bit is
shifted through I2C_SDA line and that would
prevent us from reading the correct number of
bytes left to transfer.
The full programming model suggested by IP
owner was the following:
- start I2C transfer (without STP bit)
- upon completion or NACK, read I2C_CNT register
- write STP bit to I2C_CON register
- wait for ARDY bit
With this patch we're implementing all steps
except step #2 which will come in a later
patch adding such support.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 89 ---
1 file changed, 33 insertions(+), 56 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 20f9ad6..699fa12 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -438,9 +438,9 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
/* Enable interrupts */
dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
- OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
- OMAP_I2C_IE_AL) | ((dev->fifo_size) ?
- (OMAP_I2C_IE_RDR | OMAP_I2C_IE_XDR) : 0);
+ OMAP_I2C_IE_NACK | OMAP_I2C_IE_AL) |
+ ((dev->fifo_size) ? (OMAP_I2C_IE_RDR |
+ OMAP_I2C_IE_XDR) : 0);
omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
if (dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
dev->pscstate = psc;
@@ -470,6 +470,22 @@ static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
return 0;
}
+static int omap_i2c_wait_for_ardy(struct omap_i2c_dev *dev)
+{
+ unsigned long timeout;
+
+ timeout = jiffies + OMAP_I2C_TIMEOUT;
+ while (!(omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG) &
OMAP_I2C_STAT_ARDY)) {
+ if (time_after(jiffies, timeout)) {
+ dev_warn(dev->dev, "timeout waiting for access
ready\n");
+ return -ETIMEDOUT;
+ }
+ usleep_range(800, 1200);
+ }
+
+ return 0;
+}
+
static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
{
u16 buf;
@@ -515,7 +531,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
{
struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
unsigned long timeout;
- int ret;
+ int ret = 0;
u16 w;
dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
@@ -556,35 +572,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (!(msg->flags & I2C_M_RD))
w |= OMAP_I2C_CON_TRX;
- if (!dev->b_hw && stop)
- w |= OMAP_I2C_CON_STP;
-
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
/*
-* Don't write stt and stp together on some hardware.
-*/
- if (dev->b_hw && stop) {
- unsigned long delay = jiffies + OMAP_I2C_TIMEOUT;
- u16 con = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
- while (con & OMAP_I2C_CON_STT) {
- con = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
-
- /* Let the user know if i2c is in a bad state */
- if (time_after(jiffies, delay)) {
- dev_err(dev->dev, "controller timed out "
- "waiting for start condition to finish\n");
- return -ETIMEDOUT;
- }
- cpu_relax();
- }
-
- w |= OMAP_I2C_CON_STP;
- w &= ~OMAP_I2C_CON_STT;
- omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
- }
-
- /*
* REVISIT: We should abort the transfer on signals, but the bus goes
* into arbitration and we're currently unable to recover from it.
*/
@@ -594,36 +584,36 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (timeout == 0) {
dev_err(dev->dev, "controller timed out\n");
ret = -ETIMEDOUT;
- goto err_i2c_init;
+ omap_i2c_init(dev);
+ goto out;
}
/* We have an error */
if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
OMAP_I2C_STAT_XUDF)) {
ret = -EIO;
- goto err_i2c_init;
+ omap_i2c_init(dev);
+
[PATCH v2 2/7] i2c: omap: reorder exit path of omap_i2c_xfer_msg()
just a cleanup patch trying to make exit path
more straightforward. No changes otherwise.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 26 +-
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c07d9c4..bea0277 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
{
struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
unsigned long timeout;
+ int ret;
u16 w;
dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
@@ -582,31 +583,38 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
dev->buf_len = 0;
if (timeout == 0) {
dev_err(dev->dev, "controller timed out\n");
- omap_i2c_init(dev);
- return -ETIMEDOUT;
+ ret = -ETIMEDOUT;
+ goto err_i2c_init;
}
- if (likely(!dev->cmd_err))
- return 0;
-
/* We have an error */
if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
OMAP_I2C_STAT_XUDF)) {
- omap_i2c_init(dev);
- return -EIO;
+ ret = -EIO;
+ goto err_i2c_init;
}
if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
if (msg->flags & I2C_M_IGNORE_NAK)
return 0;
+
if (stop) {
w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
w |= OMAP_I2C_CON_STP;
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
}
- return -EREMOTEIO;
+
+ ret = -EREMOTEIO;
+ goto err;
}
- return -EIO;
+
+ return 0;
+
+err_i2c_init:
+ omap_i2c_init(dev);
+
+err:
+ return ret;
}
--
1.8.0
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 6/7] i2c: add 'transferred' field to struct i2c_msg
From: Shubhrajyoti D
In case of a NACK, it's wise to tell our clients
drivers about how many bytes were actually transferred.
Support this by adding an extra field to the struct
i2c_msg which gets incremented the amount of bytes
actually transferred.
Signed-off-by: Shubhrajyoti D
Signed-off-by: Felipe Balbi
---
include/uapi/linux/i2c.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index 0e949cb..4b35c9b 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -77,6 +77,7 @@ struct i2c_msg {
#define I2C_M_NO_RD_ACK0x0800 /* if
I2C_FUNC_PROTOCOL_MANGLING */
#define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */
__u16 len; /* msg length */
+ __u16 transferred; /* actual bytes transferred */
__u8 *buf; /* pointer to msg data */
};
--
1.8.0
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 7/7] i2c: omap: implement handling for 'transferred' bytes
this is important in cases where client driver
wants to know how many bytes were actually
transferred.
There is one trick here: if transfer is completed,
meaning I2C_CNT reaches zero, then ARDY will be
asserted to let SW know that it can program a
new transfer.
When ARDY is asserted, I2C_CNT is reset to the
original value (msg->len), which means that
for a successful message, msg->transferred = msg->len
and we don't need to spend time with a register
read.
In case of NACK condition, however, I2C_CNT will
remain with the end value which is the amount of
data transferred until NACK condition found on the
bus inclusive. In this situation, msg->transferred
needs to be initialized with:
msg->len - read(I2C_CNT) - 1;
This patch implements exactly that handling.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 699fa12..d268e92 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -588,6 +588,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
goto out;
}
+ msg->transferred = msg->len;
+ wmb();
+
/* We have an error */
if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
OMAP_I2C_STAT_XUDF)) {
@@ -597,6 +600,15 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
}
if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
+ /* In case of a NACK, we need to check how many bytes we
+* actually transferred, so we can tell our client driver about
+* it.
+*
+* Let's check it here and overwrite msg->transferred.
+*/
+ w = omap_i2c_read_reg(dev, OMAP_I2C_CNT_REG);
+ msg->transferred = msg->len - w - 1;
+
if (msg->flags & I2C_M_IGNORE_NAK)
return 0;
--
1.8.0
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/7] i2c: omap: no need to access platform_device
PM callbacks pass our device pointer as argument
and we don't need to access the platform_device
just to dereference that down to dev->drvdata.
instead, just use dev_get_drvdata() directly.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..c07d9c4 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1239,8 +1239,7 @@ static int __devexit omap_i2c_remove(struct
platform_device *pdev)
#ifdef CONFIG_PM_RUNTIME
static int omap_i2c_runtime_suspend(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+ struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
u16 iv;
_dev->iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG);
@@ -1261,8 +1260,7 @@ static int omap_i2c_runtime_suspend(struct device *dev)
static int omap_i2c_runtime_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+ struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
--
1.8.0
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/7] I2C patches for v3.8 merge window
Hi, here's another series for OMAP I2C driver. There are a few cleanups and one very nice new feature: we can now report how many bytes we transferred until NACK. Note that the implemementation for OMAP-I2C turned out to be a little more complex then I expected, mainly because of the way I2C_CNT register behaves and because of the very buggy register usage on that driver. I have boot tested all patches on beagle xM (3630) and pandaboard rev A3 (4430), will send boot-logs if anyone wants to see. All patches are available at [1] if anyone wants an easy way to test the patches. Changes since v1: - remove unnecessary omap_i2c_init() in case of NACK - drop "i2c: omap: fix error checking" Felipe Balbi (6): i2c: omap: no need to access platform_device i2c: omap: reorder exit path of omap_i2c_xfer_msg() i2c: omap: also complete() when stat becomes zero i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS i2c: omap: wait for transfer completion before sending STP bit i2c: omap: implement handling for 'transferred' bytes Shubhrajyoti D (1): i2c: add 'transferred' field to struct i2c_msg drivers/i2c/busses/i2c-omap.c | 124 ++ include/uapi/linux/i2c.h | 1 + 2 files changed, 65 insertions(+), 60 deletions(-) -- 1.8.0 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/7] i2c: omap: also complete() when stat becomes zero
In case we loop on IRQ handler until stat is finally zero, we would end up in a situation where all I2C transfers would misteriously timeout because we were not calling complete() in that situation. Fix the issue by moving omap_i2c_complete_cmd() call inside the 'out' label. Signed-off-by: Felipe Balbi --- drivers/i2c/busses/i2c-omap.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index bea0277..b004126 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1021,9 +1021,8 @@ omap_i2c_isr_thread(int this_irq, void *dev_id) } } while (stat); - omap_i2c_complete_cmd(dev, err); - out: + omap_i2c_complete_cmd(dev, err); spin_unlock_irqrestore(&dev->lock, flags); return IRQ_HANDLED; -- 1.8.0 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/8] i2c: omap: reorder exit path of omap_i2c_xfer_msg()
Hi,
On Thu, Oct 25, 2012 at 05:10:10PM +0530, Shubhrajyoti Datta wrote:
> On Mon, Oct 22, 2012 at 3:16 PM, Felipe Balbi wrote:
> > just a cleanup patch trying to make exit path
> > more straightforward. No changes otherwise.
> >
> > Signed-off-by: Felipe Balbi
> > ---
> > drivers/i2c/busses/i2c-omap.c | 26 +-
> > 1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index c07d9c4..bea0277 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > {
> > struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
> > unsigned long timeout;
> > + int ret;
> > u16 w;
> >
> > dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
> > @@ -582,31 +583,38 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > dev->buf_len = 0;
> > if (timeout == 0) {
> > dev_err(dev->dev, "controller timed out\n");
> > - omap_i2c_init(dev);
> > - return -ETIMEDOUT;
> > + ret = -ETIMEDOUT;
> > + goto err_i2c_init;
> > }
> >
> > - if (likely(!dev->cmd_err))
> > - return 0;
> > -
> > /* We have an error */
> > if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> > OMAP_I2C_STAT_XUDF)) {
> > - omap_i2c_init(dev);
> > - return -EIO;
> > + ret = -EIO;
> > + goto err_i2c_init;
> > }
> >
> > if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
> > if (msg->flags & I2C_M_IGNORE_NAK)
> > return 0;
> > +
> > if (stop) {
> > w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
> > w |= OMAP_I2C_CON_STP;
> > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
> > }
> > - return -EREMOTEIO;
> > +
> > + ret = -EREMOTEIO;
> > + goto err;
>
> This adds reset to nack may be that can be removed.
great catch. I will respin this series.
thanks
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 2/8] i2c: omap: reorder exit path of omap_i2c_xfer_msg()
On Mon, Oct 22, 2012 at 3:16 PM, Felipe Balbi wrote:
> just a cleanup patch trying to make exit path
> more straightforward. No changes otherwise.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/i2c/busses/i2c-omap.c | 26 +-
> 1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index c07d9c4..bea0277 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> {
> struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
> unsigned long timeout;
> + int ret;
> u16 w;
>
> dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
> @@ -582,31 +583,38 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> dev->buf_len = 0;
> if (timeout == 0) {
> dev_err(dev->dev, "controller timed out\n");
> - omap_i2c_init(dev);
> - return -ETIMEDOUT;
> + ret = -ETIMEDOUT;
> + goto err_i2c_init;
> }
>
> - if (likely(!dev->cmd_err))
> - return 0;
> -
> /* We have an error */
> if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> OMAP_I2C_STAT_XUDF)) {
> - omap_i2c_init(dev);
> - return -EIO;
> + ret = -EIO;
> + goto err_i2c_init;
> }
>
> if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
> if (msg->flags & I2C_M_IGNORE_NAK)
> return 0;
> +
> if (stop) {
> w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
> w |= OMAP_I2C_CON_STP;
> omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
> }
> - return -EREMOTEIO;
> +
> + ret = -EREMOTEIO;
> + goto err;
This adds reset to nack may be that can be removed.
> }
> - return -EIO;
> +
> + return 0;
> +
> +err_i2c_init:
> + omap_i2c_init(dev);
> +
> +err:
> + return ret;
> }
>
>
> --
> 1.8.0.rc0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c: omap: make reset a seperate function
Hi,
On Thu, Oct 25, 2012 at 03:53:06PM +0530, Shubhrajyoti D wrote:
> Implement reset as a seperate function.
> This will enable us to make sure that we don't do the
> calculation again on every transfer.
> Also at probe the reset is not added as the hwmod is doing that
> for us.
since you're touching registers which supposedly only hwmod should
touch, you ought to Cc Benoit to make sure he knows what's you're doing
here. I'm adding him to Cc
> @@ -592,7 +597,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> if (timeout == 0) {
> dev_err(dev->dev, "controller timed out\n");
> ret = -ETIMEDOUT;
> - omap_i2c_init(dev);
> + omap_i2c_reset(dev);
> + __omap_i2c_init(dev);
> goto out;
> }
>
> @@ -603,7 +609,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> || (dev->cmd_err & OMAP_I2C_STAT_ROVR)
> || (dev->cmd_err & OMAP_I2C_STAT_XUDF)) {
> ret = -EIO;
> - omap_i2c_init(dev);
> + omap_i2c_reset(dev);
> + __omap_i2c_init(dev);
> goto out;
> }
>
> @@ -621,7 +628,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> return 0;
>
> ret = -EREMOTEIO;
> - omap_i2c_init(dev);
> + omap_i2c_reset(dev);
> + __omap_i2c_init(dev);
eventually we need to try to forcefully trigger these errors above
(nack, overflow, underflow and arbitration lost) and try to make sure if
actually need to reset the controller all the time. I find it really odd
that we're always resetting the IP in every error condition without
actually trying to figure out what's wrong with the driver (if there is
something wrong with the driver, of course).
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 1/2] i2c: omap: re-factor omap_i2c_init function
Hi,
(a small top-post here, don't forget to keep the patch version in the
subject, I think this is v3 already, so next patch should be v4)
On Thu, Oct 25, 2012 at 03:53:05PM +0530, Shubhrajyoti D wrote:
> re-factor omap_i2c_init() so that we can re-use it for resume.
> While at it also remove the bufstate variable as we write it
> in omap_i2c_resize_fifo for every transfer.
>
> Signed-off-by: Shubhrajyoti D
> ---
> drivers/i2c/busses/i2c-omap.c | 71 ++--
> 1 files changed, 32 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 5e5cefb..3d400b1 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -209,7 +209,6 @@ struct omap_i2c_dev {
> u16 pscstate;
> u16 scllstate;
> u16 sclhstate;
> - u16 bufstate;
> u16 syscstate;
> u16 westate;
> u16 errata;
> @@ -285,9 +284,31 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev
> *i2c_dev, int reg)
> }
> }
>
> +static void __omap_i2c_init(struct omap_i2c_dev *dev)
> +{
> +
> + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
> + /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
> + omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
> +
> + /* SCL low and high time values */
> + omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate);
> + omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate);
> + if (dev->rev >= OMAP_I2C_REV_ON_3430_3530)
> + omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
> + /* Take the I2C module out of reset: */
> + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
> + /*
> + * Don't write to this register if the IE state is 0 as it can
> + * cause deadlock.
> + */
> + if (dev->iestate)
> + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
you still miss the extra blank lines here. Try something like below:
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
/* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
/* SCL low and high time values */
omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate);
omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate);
if (dev->rev >= OMAP_I2C_REV_ON_3430_3530)
omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
/* Take the I2C module out of reset: */
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
/*
* Don't write to this register if the IE state is 0 as it can
* cause deadlock.
*/
if (dev->iestate)
omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 3/8] i2c: omap: fix error checking
Hi,
On Thu, Oct 25, 2012 at 12:33:18PM +0200, Michael Trimarchi wrote:
> >>> @@ -587,9 +587,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> >>> goto err_i2c_init;
> >>> }
> >>>
> >>> - /* We have an error */
> >>> - if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> >>> - OMAP_I2C_STAT_XUDF)) {
> >>> + if ((dev->cmd_err & OMAP_I2C_STAT_AL)
> >>> + || (dev->cmd_err & OMAP_I2C_STAT_ROVR)
> >>> + || (dev->cmd_err & OMAP_I2C_STAT_XUDF)) {
> >>
> >> Sorry, what is the difference? I didn't understand the optimisation
> >> and why now is more clear. Can you just add a comment?
> >
> > semantically they're not the same, right ? We want to check if each of
> > those bits are set, not if all of them are set together.
> >
> > my 2 cents.
>
> You are doing the same thing, but of course is better with just one
I never claimed the contrary. I said *semantically* they're not the
same.
> *if* as before . A general rule is: when you have logic expression you
We still have a single *if* and I'm sure compiler will optimize that
expression as much as it likes.
> can use undefined states to simplify the logic.
don't-care is not the same as undefined states.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 3/8] i2c: omap: fix error checking
Hi
On 10/25/2012 12:10 PM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Oct 24, 2012 at 04:41:11PM +0200, Michael Trimarchi wrote:
>> Hi
>>
>> On 10/22/2012 11:46 AM, Felipe Balbi wrote:
>>> It's impossible to have Arbitration Lost,
>>> Read Overflow, and Tranmist Underflow all
>>> asserted at the same time.
>>>
>>> Those error conditions are mutually exclusive
>>> so what the code should be doing, instead, is
>>> check each error flag separataly.
>>>
>>> Signed-off-by: Felipe Balbi
>>> ---
>>> drivers/i2c/busses/i2c-omap.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>>> index bea0277..e0eab38 100644
>>> --- a/drivers/i2c/busses/i2c-omap.c
>>> +++ b/drivers/i2c/busses/i2c-omap.c
>>> @@ -587,9 +587,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>>> goto err_i2c_init;
>>> }
>>>
>>> - /* We have an error */
>>> - if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
>>> - OMAP_I2C_STAT_XUDF)) {
>>> + if ((dev->cmd_err & OMAP_I2C_STAT_AL)
>>> + || (dev->cmd_err & OMAP_I2C_STAT_ROVR)
>>> + || (dev->cmd_err & OMAP_I2C_STAT_XUDF)) {
>>
>> Sorry, what is the difference? I didn't understand the optimisation
>> and why now is more clear. Can you just add a comment?
>
> semantically they're not the same, right ? We want to check if each of
> those bits are set, not if all of them are set together.
>
> my 2 cents.
You are doing the same thing, but of course is better with just one *if* as
before . A general rule is: when you have logic expression you can use
undefined states to simplify the logic.
Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] i2c: omap: re-factor omap_i2c_init function
re-factor omap_i2c_init() so that we can re-use it for resume.
While at it also remove the bufstate variable as we write it
in omap_i2c_resize_fifo for every transfer.
Signed-off-by: Shubhrajyoti D
---
drivers/i2c/busses/i2c-omap.c | 71 ++--
1 files changed, 32 insertions(+), 39 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 5e5cefb..3d400b1 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -209,7 +209,6 @@ struct omap_i2c_dev {
u16 pscstate;
u16 scllstate;
u16 sclhstate;
- u16 bufstate;
u16 syscstate;
u16 westate;
u16 errata;
@@ -285,9 +284,31 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev
*i2c_dev, int reg)
}
}
+static void __omap_i2c_init(struct omap_i2c_dev *dev)
+{
+
+ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
+ /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
+ omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
+
+ /* SCL low and high time values */
+ omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate);
+ omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate);
+ if (dev->rev >= OMAP_I2C_REV_ON_3430_3530)
+ omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
+ /* Take the I2C module out of reset: */
+ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
+ /*
+* Don't write to this register if the IE state is 0 as it can
+* cause deadlock.
+*/
+ if (dev->iestate)
+ omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
+}
+
static int omap_i2c_init(struct omap_i2c_dev *dev)
{
- u16 psc = 0, scll = 0, sclh = 0, buf = 0;
+ u16 psc = 0, scll = 0, sclh = 0;
u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
unsigned long fclk_rate = 1200;
unsigned long timeout;
@@ -337,11 +358,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
* REVISIT: Some wkup sources might not be needed.
*/
dev->westate = OMAP_I2C_WE_ALL;
- omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
- dev->westate);
}
}
- omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
if (dev->flags & OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
/*
@@ -426,28 +444,18 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
sclh = fclk_rate / (dev->speed * 2) - 7 + psc;
}
- /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */
- omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, psc);
-
- /* SCL low and high time values */
- omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll);
- omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh);
-
- /* Take the I2C module out of reset: */
- omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
-
/* Enable interrupts */
dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
OMAP_I2C_IE_NACK | OMAP_I2C_IE_AL) |
((dev->fifo_size) ? (OMAP_I2C_IE_RDR |
OMAP_I2C_IE_XDR) : 0);
- omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
- if (dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
- dev->pscstate = psc;
- dev->scllstate = scll;
- dev->sclhstate = sclh;
- dev->bufstate = buf;
- }
+
+ dev->pscstate = psc;
+ dev->scllstate = scll;
+ dev->sclhstate = sclh;
+
+ __omap_i2c_init(dev);
+
return 0;
}
@@ -1268,23 +1276,8 @@ static int omap_i2c_runtime_resume(struct device *dev)
{
struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
- if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
- omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
- omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_SCLL_REG, _dev->scllstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_SCLH_REG, _dev->sclhstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_BUF_REG, _dev->bufstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_SYSC_REG, _dev->syscstate);
- omap_i2c_write_reg(_dev, OMAP_I2C_WE_REG, _dev->westate);
- omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
- }
-
- /*
-* Don't write to this register if the IE state is 0 as it can
-* cause deadlock.
-*/
- if (_dev->iestate)
- omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, _dev->iestate);
+ if (_d
[PATCH 0/2] i2c: omap: cleanups
Applies on Felipe's series http://www.spinics.net/lists/linux-omap/msg79995.html Tested with Terro sys fix + Felipe's stop sched_clock() during suspend on omap3630 beagle both idle and suspend. Functional testing on omap4sdp. Shubhrajyoti D (2): i2c: omap: re-factor omap_i2c_init function i2c: omap: make reset a seperate function drivers/i2c/busses/i2c-omap.c | 97 + 1 files changed, 49 insertions(+), 48 deletions(-) -- 1.7.5.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] i2c: omap: make reset a seperate function
Implement reset as a seperate function.
This will enable us to make sure that we don't do the
calculation again on every transfer.
Also at probe the reset is not added as the hwmod is doing that
for us.
Signed-off-by: Shubhrajyoti D
---
drivers/i2c/busses/i2c-omap.c | 28 ++--
1 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 3d400b1..5a87ff9 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -306,15 +306,9 @@ static void __omap_i2c_init(struct omap_i2c_dev *dev)
omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
}
-static int omap_i2c_init(struct omap_i2c_dev *dev)
+static int omap_i2c_reset(struct omap_i2c_dev *dev)
{
- u16 psc = 0, scll = 0, sclh = 0;
- u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
- unsigned long fclk_rate = 1200;
unsigned long timeout;
- unsigned long internal_clk = 0;
- struct clk *fclk;
-
if (dev->rev >= OMAP_I2C_OMAP1_REV_2) {
/* Disable I2C controller before soft reset */
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
@@ -360,6 +354,17 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
dev->westate = OMAP_I2C_WE_ALL;
}
}
+ return 0;
+}
+
+static int omap_i2c_init(struct omap_i2c_dev *dev)
+{
+ u16 psc = 0, scll = 0, sclh = 0;
+ u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
+ unsigned long fclk_rate = 1200;
+ unsigned long internal_clk = 0;
+ struct clk *fclk;
+
if (dev->flags & OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
/*
@@ -592,7 +597,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (timeout == 0) {
dev_err(dev->dev, "controller timed out\n");
ret = -ETIMEDOUT;
- omap_i2c_init(dev);
+ omap_i2c_reset(dev);
+ __omap_i2c_init(dev);
goto out;
}
@@ -603,7 +609,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
|| (dev->cmd_err & OMAP_I2C_STAT_ROVR)
|| (dev->cmd_err & OMAP_I2C_STAT_XUDF)) {
ret = -EIO;
- omap_i2c_init(dev);
+ omap_i2c_reset(dev);
+ __omap_i2c_init(dev);
goto out;
}
@@ -621,7 +628,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
return 0;
ret = -EREMOTEIO;
- omap_i2c_init(dev);
+ omap_i2c_reset(dev);
+ __omap_i2c_init(dev);
}
out:
--
1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] i2c: omap: fix error checking
Hi,
On Wed, Oct 24, 2012 at 04:41:11PM +0200, Michael Trimarchi wrote:
> Hi
>
> On 10/22/2012 11:46 AM, Felipe Balbi wrote:
> > It's impossible to have Arbitration Lost,
> > Read Overflow, and Tranmist Underflow all
> > asserted at the same time.
> >
> > Those error conditions are mutually exclusive
> > so what the code should be doing, instead, is
> > check each error flag separataly.
> >
> > Signed-off-by: Felipe Balbi
> > ---
> > drivers/i2c/busses/i2c-omap.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index bea0277..e0eab38 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -587,9 +587,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > goto err_i2c_init;
> > }
> >
> > - /* We have an error */
> > - if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
> > - OMAP_I2C_STAT_XUDF)) {
> > + if ((dev->cmd_err & OMAP_I2C_STAT_AL)
> > + || (dev->cmd_err & OMAP_I2C_STAT_ROVR)
> > + || (dev->cmd_err & OMAP_I2C_STAT_XUDF)) {
>
> Sorry, what is the difference? I didn't understand the optimisation
> and why now is more clear. Can you just add a comment?
semantically they're not the same, right ? We want to check if each of
those bits are set, not if all of them are set together.
my 2 cents.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v2] i2c: omap: re-factor omap_i2c_init function
Hi,
On Thu, Oct 25, 2012 at 03:04:29PM +0530, Shubhrajyoti Datta wrote:
> On Thu, Oct 25, 2012 at 12:06 PM, Felipe Balbi wrote:
>
> [...]
> >> + * Don't write to this register if the IE state is 0 as it can
> >> + * cause deadlock.
> >> + */
> >> + if (dev->iestate)
> >> + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
> >> +}
> >> +
> >> static int omap_i2c_init(struct omap_i2c_dev *dev)
> >> {
> >> - u16 psc = 0, scll = 0, sclh = 0, buf = 0;
> >> + u16 psc = 0, scll = 0, sclh = 0;
> >> u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
> >> unsigned long fclk_rate = 1200;
> >> unsigned long timeout;
> >> @@ -337,11 +358,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
> >>* REVISIT: Some wkup sources might not be needed.
> >>*/
> >> dev->westate = OMAP_I2C_WE_ALL;
> >> - omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
> >> - dev->westate);
> >
> > remove the comment too since now that's done by some other function ?
>
> The comment is applicable to the OMAP_I2C_WE_ALL value.
> So I thought it could be kept.
>
> dont feel strongly though.
I see. that's ok then.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v2] i2c: omap: re-factor omap_i2c_init function
On Thu, Oct 25, 2012 at 12:06 PM, Felipe Balbi wrote:
[...]
>> + * Don't write to this register if the IE state is 0 as it can
>> + * cause deadlock.
>> + */
>> + if (dev->iestate)
>> + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
>> +}
>> +
>> static int omap_i2c_init(struct omap_i2c_dev *dev)
>> {
>> - u16 psc = 0, scll = 0, sclh = 0, buf = 0;
>> + u16 psc = 0, scll = 0, sclh = 0;
>> u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
>> unsigned long fclk_rate = 1200;
>> unsigned long timeout;
>> @@ -337,11 +358,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>>* REVISIT: Some wkup sources might not be needed.
>>*/
>> dev->westate = OMAP_I2C_WE_ALL;
>> - omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
>> - dev->westate);
>
> remove the comment too since now that's done by some other function ?
The comment is applicable to the OMAP_I2C_WE_ALL value.
So I thought it could be kept.
dont feel strongly though.
>
>> }
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: omap: ensure writes to dev->buf_len are ordered
On Thu, Oct 25, 2012 at 2:30 PM, Felipe Balbi wrote:
> if we allow compiler reorder our writes, we could
> fall into a situation where dev->buf_len is reset
> for no apparent reason.
>
> This bug was found with a simple script which would
> transfer data to an i2c client from 1 to 1024 bytes
> (a simple for loop), when we got to transfer sizes
> bigger than the fifo size, dev->buf_len was reset
> to zero before we had an oportunity to handle XDR
> Interrupt. Because dev->buf_len was zero, we entered
> omap_i2c_transmit_data() to transfer zero bytes,
> which would mean we would just silently exit
> omap_i2c_transmit_data() without actually writing
> anything to DATA register. That would cause XDR
> IRQ to trigger forever and we would never transfer
> the remaining bytes.
>
> After adding the memory barrier, we also drop resetting
> dev->buf_len to zero in omap_i2c_xfer_msg() because
> both omap_i2c_transmit_data() and omap_i2c_receive_data()
> will act until dev->buf_len reaches zero, rendering the
> other write in omap_i2c_xfer_msg() redundant.
>
> This patch has been tested with pandaboard for a few
> iterations of the script mentioned above.
looks good to me
Acked-by: Shubhrajyoti D
>
> Signed-off-by: Felipe Balbi
> ---
>
> This bug has been there forever, but it's quite annoying.
> I think it deserves being pushed upstream during this -rc
> cycle, but if Wolfram decides to wait until v3.8, I don't
> mind.
>
> drivers/i2c/busses/i2c-omap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index db31eae..1ec4e6e 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -521,6 +521,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> /* REVISIT: Could the STB bit of I2C_CON be used with probing? */
> dev->buf = msg->buf;
> dev->buf_len = msg->len;
> + wmb();
>
> omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev->buf_len);
>
> @@ -579,7 +580,6 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> */
> timeout = wait_for_completion_timeout(&dev->cmd_complete,
> OMAP_I2C_TIMEOUT);
> - dev->buf_len = 0;
> if (timeout == 0) {
> dev_err(dev->dev, "controller timed out\n");
> omap_i2c_init(dev);
> --
> 1.8.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: omap: ensure writes to dev->buf_len are ordered
if we allow compiler reorder our writes, we could
fall into a situation where dev->buf_len is reset
for no apparent reason.
This bug was found with a simple script which would
transfer data to an i2c client from 1 to 1024 bytes
(a simple for loop), when we got to transfer sizes
bigger than the fifo size, dev->buf_len was reset
to zero before we had an oportunity to handle XDR
Interrupt. Because dev->buf_len was zero, we entered
omap_i2c_transmit_data() to transfer zero bytes,
which would mean we would just silently exit
omap_i2c_transmit_data() without actually writing
anything to DATA register. That would cause XDR
IRQ to trigger forever and we would never transfer
the remaining bytes.
After adding the memory barrier, we also drop resetting
dev->buf_len to zero in omap_i2c_xfer_msg() because
both omap_i2c_transmit_data() and omap_i2c_receive_data()
will act until dev->buf_len reaches zero, rendering the
other write in omap_i2c_xfer_msg() redundant.
This patch has been tested with pandaboard for a few
iterations of the script mentioned above.
Signed-off-by: Felipe Balbi
---
This bug has been there forever, but it's quite annoying.
I think it deserves being pushed upstream during this -rc
cycle, but if Wolfram decides to wait until v3.8, I don't
mind.
drivers/i2c/busses/i2c-omap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..1ec4e6e 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -521,6 +521,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
/* REVISIT: Could the STB bit of I2C_CON be used with probing? */
dev->buf = msg->buf;
dev->buf_len = msg->len;
+ wmb();
omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev->buf_len);
@@ -579,7 +580,6 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
*/
timeout = wait_for_completion_timeout(&dev->cmd_complete,
OMAP_I2C_TIMEOUT);
- dev->buf_len = 0;
if (timeout == 0) {
dev_err(dev->dev, "controller timed out\n");
omap_i2c_init(dev);
--
1.8.0
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Support Elan Touchscreen eKTF product.
On Thu, Oct 25, 2012 at 12:32:39PM +0800, 劉嘉駿 wrote: > Hi Dmitry, > Thanks for review. > > > -Original Message- > > From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com] > > Sent: Thursday, October 25, 2012 2:13 AM > > To: Scott Liu > > Cc: linux-in...@vger.kernel.org; linux-i2c@vger.kernel.org; > linux-ker...@vger.kernel.org; > > Benjamin Tissoires; Jesse; Vincent Wang; Paul > > Subject: Re: [PATCH v2] Support Elan Touchscreen eKTF product. > > > > Hi Scott, > > > > On Wed, Oct 24, 2012 at 09:41:43AM +0800, Scott Liu wrote: > > > This patch is for Elan eKTF Touchscreen product, I2C adpater module. > > > > > > Signed-off-by: Scott Liu > > > --- > > > > > > Hi, > > > v2 revision I have fixed some bug as your advise. > > > 1. To target the mainline > > > 2. No Android dependency > > > 3. reuse those duplication code from Henrik's patchset. > > > (input_mt_sync_frame() / input_mt_get_slot_by_key()) > > > > Just a quick run through the code, so: > > > > - please remove polling support, it is not useful in production; > > OK. > > > - why do you need a separate probe work instead of doing what you > > need in elants_probe() > > will fix. > > > - it is not a good idea to register input device first and then > > allocating memory for MT handling. > > Ooop...will fix. > > > - I do not understand why kfifo is needed > > The firmware and the host would conflict by read command and finger report > simultaneously. So I'm simply using kfifo in IRQ thread function. > > * read command: writing 4 bytes commands and the device asserts GPIO > interrupt and then response 4 bytes data. > > There was an error if we do not use kfifo: > With heavy loading by finger report / read command, the driver may > get finger report as response data. > > So, do you understand my meaning? No I don't. Most of my confusion stems from the fact that you only put data into kfifo but not actually use it anywhere. You do fetch the data in your "drop old" function, but that data is just dropped. So I really do not see the point. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c: omap: re-factor omap_i2c_init function
Hi,
On Thu, Oct 25, 2012 at 12:23:56PM +0530, Shubhrajyoti Datta wrote:
> >> @@ -1268,23 +1276,8 @@ static int omap_i2c_runtime_resume(struct device
> >> *dev)
> >> {
> >> struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
> >>
> >> - if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
> >> - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
> >> - omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate);
> >> - omap_i2c_write_reg(_dev, OMAP_I2C_SCLL_REG, _dev->scllstate);
> >> - omap_i2c_write_reg(_dev, OMAP_I2C_SCLH_REG, _dev->sclhstate);
> >> - omap_i2c_write_reg(_dev, OMAP_I2C_BUF_REG, _dev->bufstate);
> >> - omap_i2c_write_reg(_dev, OMAP_I2C_SYSC_REG, _dev->syscstate);
> >> - omap_i2c_write_reg(_dev, OMAP_I2C_WE_REG, _dev->westate);
> >> - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
> >> - }
> >> -
> >> - /*
> >> - * Don't write to this register if the IE state is 0 as it can
> >> - * cause deadlock.
> >> - */
> >> - if (_dev->iestate)
> >> - omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, _dev->iestate);
> >> + if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE)
> >> + __omap_i2c_init(_dev);
> >>
> >> return 0;
> >> }
> >
> > you continue to miss the changes in omap_i2c_xfer_msg() and your
> > explanation of why not doing it wasn't good enough IMHO.
>
> Will do that . I am preparing a seperate patch for moving the
> calculation to a seperate function.
why do you need yet another function ? omap_i2c_init() does all
calculation and __omap_i2c_init() doesn't do any calculations. What's
the point for yet another function ?
--
balbi
signature.asc
Description: Digital signature
