* Re: [PATCH v2] i2c: mux: fix up dependencies
From: Jonathan Cameron @ 2016-11-14 17:13 UTC (permalink / raw)
To: Peter Rosin, Linus Walleij, Wolfram Sang, linux-i2c; +Cc: stable
In-Reply-To: <6094de61-8745-fbff-e516-db95392aa142@axentia.se>
On 14 November 2016 14:39:54 GMT+00:00, Peter Rosin <peda@axentia.se> wrote:
>On 2016-11-14 15:34, Linus Walleij wrote:
>> We get the following build error from UM Linux after adding
>> an entry to drivers/iio/gyro/Kconfig that issues "select I2C_MUX":
>>
>> ERROR: "devm_ioremap_resource"
>> [drivers/i2c/muxes/i2c-mux-reg.ko] undefined!
>> ERROR: "of_address_to_resource"
>> [drivers/i2c/muxes/i2c-mux-reg.ko] undefined!
>>
>> It appears that the I2C mux core code depends on HAS_IOMEM
>> for historical reasons, while CONFIG_I2C_MUX_REG does *not*
>> have a direct dependency on HAS_IOMEM.
>>
>> This creates a situation where a allyesconfig or allmodconfig
>> for UM Linux will select I2C_MUX, and will implicitly enable
>> I2C_MUX_REG as well, and the compilation will fail for the
>> register driver.
>>
>> Fix this up by making I2C_MUX_REG depend on HAS_IOMEM and
>> removing the dependency from I2C_MUX.
>>
>> Cc: stable@vger.kernel.org
>> Reported-by: kbuild test robot <fengguang.wu@intel.com>
>> Reported-by: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
>> Cc: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
>> Cc: Peter Rosin <peda@axentia.se>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
>Acked-by: Peter Rosin <peda@axentia.se>
>
>Thanks!
Thanks for sorting this.
Acked-by: Jonathan Cameron <jic23@kernel.org>
>
>Cheers,
>Peter
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH v4 2/2] i2c: aspeed: added documentation for Aspeed I2C driver
From: Rob Herring @ 2016-11-14 15:57 UTC (permalink / raw)
To: Brendan Higgins; +Cc: wsa, mark.rutland, linux-i2c, devicetree, joel, openbmc
In-Reply-To: <1478311099-6771-3-git-send-email-brendanhiggins@google.com>
On Fri, Nov 04, 2016 at 06:58:19PM -0700, Brendan Higgins wrote:
> Added device tree binding documentation for Aspeed I2C controller and
> busses.
>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> Changes for v2:
> - None
> Changes for v3:
> - Removed reference to "bus" device tree param
> Changes for v4:
> - None
> ---
> .../devicetree/bindings/i2c/i2c-aspeed.txt | 61 ++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH v2] i2c: mux: fix up dependencies
From: Linus Walleij @ 2016-11-14 14:34 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: Linus Walleij, stable, Jonathan Cameron, Peter Rosin
We get the following build error from UM Linux after adding
an entry to drivers/iio/gyro/Kconfig that issues "select I2C_MUX":
ERROR: "devm_ioremap_resource"
[drivers/i2c/muxes/i2c-mux-reg.ko] undefined!
ERROR: "of_address_to_resource"
[drivers/i2c/muxes/i2c-mux-reg.ko] undefined!
It appears that the I2C mux core code depends on HAS_IOMEM
for historical reasons, while CONFIG_I2C_MUX_REG does *not*
have a direct dependency on HAS_IOMEM.
This creates a situation where a allyesconfig or allmodconfig
for UM Linux will select I2C_MUX, and will implicitly enable
I2C_MUX_REG as well, and the compilation will fail for the
register driver.
Fix this up by making I2C_MUX_REG depend on HAS_IOMEM and
removing the dependency from I2C_MUX.
Cc: stable@vger.kernel.org
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Reported-by: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
Cc: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
Cc: Peter Rosin <peda@axentia.se>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Drop the assumed dependency on OF from I2C_MUX
---
drivers/i2c/Kconfig | 1 -
drivers/i2c/muxes/Kconfig | 1 +
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index d223650a97e4..11edabf425ae 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -59,7 +59,6 @@ config I2C_CHARDEV
config I2C_MUX
tristate "I2C bus multiplexing support"
- depends on HAS_IOMEM
help
Say Y here if you want the I2C core to support the ability to
handle multiplexed I2C bus topologies, by presenting each
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index e280c8ecc0b5..96de9ce5669b 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -63,6 +63,7 @@ config I2C_MUX_PINCTRL
config I2C_MUX_REG
tristate "Register-based I2C multiplexer"
+ depends on HAS_IOMEM
help
If you say yes to this option, support will be included for a
register based I2C multiplexer. This driver provides access to
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] i2c: mux: fix up dependencies
From: Linus Walleij @ 2016-11-14 14:30 UTC (permalink / raw)
To: Peter Rosin
Cc: Wolfram Sang, linux-i2c@vger.kernel.org, stable, Jonathan Cameron
In-Reply-To: <3ba9773a-b32b-1be3-cf27-2c4e7842197f@axentia.se>
On Mon, Nov 14, 2016 at 2:59 PM, Peter Rosin <peda@axentia.se> wrote:
> It is not obvious to me that the i2c mux is dependent on
> CONFIG_OF. Sure, there are unconditional calls to things like
> of_property_read_u32 etc, but those are meant to be compiled
> away for the !CONFIG_OF case. I think. The code predates me,
> but that's how I read it...
(...)
> But don't add "depends on OF", or am I missing something?
Yeah that's right I guess. It can also use ACPI.
I'll update.
Yours,
Linus Walleij
^ permalink raw reply
* [Patch V4 2/2] i2c: imx: add devicetree binding for lpi2c
From: Gao Pan @ 2016-11-14 9:23 UTC (permalink / raw)
To: wsa, u.kleine-koenig, cmo, robh, vz
Cc: linux-i2c, pandy.gao, frank.li, fugang.duan
In-Reply-To: <1479115411-1537-1-git-send-email-pandy.gao@nxp.com>
Add a binding document for lpi2c driver
Signed-off-by: Gao Pan <pandy.gao@nxp.com>
---
.../devicetree/bindings/i2c/i2c-imx-lpi2c.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
new file mode 100644
index 0000000..edaf04f
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
@@ -0,0 +1,20 @@
+* Freescale Low Power Inter IC (LPI2C) for i.MX
+
+Required properties:
+- compatible :
+ - "fsl,imx8dv-lpi2c" for LPI2C compatible with the one integrated on i.MX8DV soc
+ - "fsl,imx7ulp-lpi2c" for LPI2C compatible with the one integrated on i.MX7ULP soc
+- reg : address and length of the lpi2c master registers
+- interrupt-parent : core interrupt controller
+- interrupts : lpi2c interrupt
+- clocks : lpi2c clock specifier
+
+Examples:
+
+lpi2c7: lpi2c7@40A50000 {
+ compatible = "fsl,imx8dv-lpi2c";
+ reg = <0x40A50000 0x10000>;
+ interrupt-parent = <&intc>;
+ interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks IMX7ULP_CLK_LPI2C7>;
+};
--
1.9.1
^ permalink raw reply related
* [PATCH] i2c: mux: fix up dependencies
From: Linus Walleij @ 2016-11-14 13:03 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: Linus Walleij, stable, Jonathan Cameron, Peter Rosin
We get the following build error from UM Linux after adding
an entry to drivers/iio/gyro/Kconfig that issues "select I2C_MUX":
ERROR: "devm_ioremap_resource"
[drivers/i2c/muxes/i2c-mux-reg.ko] undefined!
ERROR: "of_address_to_resource"
[drivers/i2c/muxes/i2c-mux-reg.ko] undefined!
It appears that the I2C mux core code actually only requires
CONFIG_OF, but depends on HAS_IOMEM for historical reasons,
while CONFIG_I2C_MUX_REG does *not* have a direct dependency
on HAS_IOMEM.
This creates a situation where a allyesconfig or allmodconfig
for UM Linux will select I2C_MUX, and will implicitly enable
I2C_MUX_REG as well, and the compilation will fail for the
register driver.
Fix this up by making I2C_MUX_REG depend on HAS_IOMEM and
the I2C_MUX depend on OF.
Cc: stable@vger.kernel.org
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Reported-by: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
Cc: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
Cc: Peter Rosin <peda@axentia.se>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/i2c/Kconfig | 2 +-
drivers/i2c/muxes/Kconfig | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index d223650a97e4..b9e378cfe1d1 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -59,7 +59,7 @@ config I2C_CHARDEV
config I2C_MUX
tristate "I2C bus multiplexing support"
- depends on HAS_IOMEM
+ depends on OF
help
Say Y here if you want the I2C core to support the ability to
handle multiplexed I2C bus topologies, by presenting each
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index e280c8ecc0b5..96de9ce5669b 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -63,6 +63,7 @@ config I2C_MUX_PINCTRL
config I2C_MUX_REG
tristate "Register-based I2C multiplexer"
+ depends on HAS_IOMEM
help
If you say yes to this option, support will be included for a
register based I2C multiplexer. This driver provides access to
--
2.7.4
^ permalink raw reply related
* RE: [Patch V3] i2c: imx: add low power i2c bus driver
From: Pandy Gao @ 2016-11-14 9:21 UTC (permalink / raw)
To: Vladimir Zapolskiy, wsa@the-dreams.de,
u.kleine-koenig@pengutronix.de, cmo@melexis.com
Cc: linux-i2c@vger.kernel.org, Frank Li, Andy Duan
In-Reply-To: <25d3aa71-cccd-320b-1bc4-cbfc829f019a@mleia.com>
From: Vladimir Zapolskiy <mailto:vz@mleia.com> Sent: Tuesday, October 25, 2016 7:15 AM
> To: Pandy Gao <pandy.gao@nxp.com>; wsa@the-dreams.de; u.kleine-
> koenig@pengutronix.de; cmo@melexis.com
> Cc: linux-i2c@vger.kernel.org; Frank Li <frank.li@nxp.com>; Andy Duan
> <fugang.duan@nxp.com>
> Subject: Re: [Patch V3] i2c: imx: add low power i2c bus driver
>
> Hello Pandy,
>
> On 17.08.2016 10:59, Gao Pan wrote:
> > This patch adds low power i2c bus driver to support new i.MX products
> > which use low power i2c instead of the old imx i2c.
> >
> > The low power i2c can continue operating in stop mode when an
> > appropriate clock is available. It is also designed for low CPU
> > overhead with DMA offloading of FIFO register accesses.
> >
> > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> > Reviewed-by: Fugang Duan <B38611@freescale.com>
> > ---
> > V2:
> > -stop i2c transfer under the wrong condition -add timeout check in
> > while() domain
> >
> > V3:
> > -fix typo inside commit message and the driver.
> >
> > .../devicetree/bindings/i2c/i2c-imx-lpi2c.txt | 25 +
> > drivers/i2c/busses/Kconfig | 10 +
> > drivers/i2c/busses/Makefile | 1 +
> > drivers/i2c/busses/i2c-imx-lpi2c.c | 667 +++++++++++++++++++++
> > 4 files changed, 703 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
> > b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
> > new file mode 100644
> > index 0000000..1f10cbf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
> > @@ -0,0 +1,25 @@
> > +* Freescale Low Power Inter IC (LPI2C) for i.MX
> > +
> > +Required properties:
> > +- compatible :
> > + - "fsl,imx8dv-lpi2c" for LPI2C compatible with the one integrated
> > +on i.MX8DV soc
> > + - "fsl,imx7ulp-lpi2c" for LPI2C compatible with the one integrated
> > +on i.MX7ULP soc
> > +- reg : Should contain LPI2C registers location and length
> > +- interrupts : Should contain LPI2C interrupt
> > +- clocks : Should contain LPI2C clock specifier
> > +- power-domains : should contain LPI2C power domain
> > +
> > +Optional properties:
> > +- clock-frequency : Constains desired LPI2C bus clock frequency in Hz.
>
> typo, what is "constains"? Contains, constrains?
It should be "contains", Thanks!
> > + The absence of the property indicates the default frequency 100 kHz.
> > +
> > +Examples:
> > +
> > +i2c1: i2c@5e110000 { /* LPI2C on i.MX8DV */
> > + compatible = "fsl,imx8dv-lpi2c";
> > + reg = <0x0 0x5e110000 0x0 0x4000>;
> > + interrupts = <0 88 4>;
> > + clocks = <&clk IMX8DV_I2C1_CLK>;
> > + clock-names = "per";
> > + power-domains = <&pd_lsio_i2c1>;
> > +};
>
> For this part please send the change to devicetree mailing list and get Rob
> Herring's ack. You may split it into a separate patch.
Thanks, it is better to split these part from i2c driver.
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index efa3d9b..1fc7a10 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -596,6 +596,16 @@ config I2C_IMX
> > This driver can also be built as a module. If so, the module
> > will be called i2c-imx.
> >
> > +config I2C_IMX_LPI2C
> > + tristate "IMX Low Power I2C interface"
> > + depends on ARCH_MXC || COMPILE_TEST
> > + help
> > + Say Y here if you want to use the Low Power IIC bus controller
> > + on the Freescale i.MX processors.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called i2c-imx-lpi2c.
> > +
> > config I2C_IOP3XX
> > tristate "Intel IOPx3xx and IXP4xx on-chip I2C interface"
> > depends on ARCH_IOP32X || ARCH_IOP33X || ARCH_IXP4XX ||
> ARCH_IOP13XX
> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index 37f2819..cc93457 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -56,6 +56,7 @@ obj-$(CONFIG_I2C_HIX5HD2) += i2c-hix5hd2.o
> > obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o
> > obj-$(CONFIG_I2C_IMG) += i2c-img-scb.o
> > obj-$(CONFIG_I2C_IMX) += i2c-imx.o
> > +obj-$(CONFIG_I2C_IMX_LPI2C) += i2c-imx-lpi2c.o
> > obj-$(CONFIG_I2C_IOP3XX) += i2c-iop3xx.o
> > obj-$(CONFIG_I2C_JZ4780) += i2c-jz4780.o
> > obj-$(CONFIG_I2C_KEMPLD) += i2c-kempld.o
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > new file mode 100644
> > index 0000000..308ecf5
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -0,0 +1,667 @@
> > +/*
> > + * This is i.MX low power i2c controller driver.
> > + *
> > + * Copyright 2016 Freescale Semiconductor, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/sched.h>
> > +#include <linux/slab.h>
> > +
> > +#define DRIVER_NAME "imx-lpi2c"
> > +
> > +#define LPI2C_PARAM 0x04 /* i2c RX/TX FIFO size */
> > +#define LPI2C_MCR 0x10 /* i2c contrl register */
> > +#define LPI2C_MSR 0x14 /* i2c status register */
> > +#define LPI2C_MIER 0x18 /* i2c interrupt enable */
> > +#define LPI2C_MCFGR0 0x20 /* i2c master configuration */
> > +#define LPI2C_MCFGR1 0x24 /* i2c master configuration */
> > +#define LPI2C_MCFGR2 0x28 /* i2c master configuration */
> > +#define LPI2C_MCFGR3 0x2C /* i2c master configuration */
> > +#define LPI2C_MCCR0 0x48 /* i2c master clk configuration */
> > +#define LPI2C_MCCR1 0x50 /* i2c master clk configuration */
> > +#define LPI2C_MFCR 0x58 /* i2c master FIFO control */
> > +#define LPI2C_MFSR 0x5C /* i2c master FIFO status */
> > +#define LPI2C_MTDR 0x60 /* i2c master TX data register */
> > +#define LPI2C_MRDR 0x70 /* i2c master RX data register */
> > +
> > +/* i2c command */
> > +#define TRAN_DATA 0X00
> > +#define RECV_DATA 0X01
> > +#define GEN_STOP 0X02
> > +#define RECV_DISCARD 0X03
> > +#define GEN_START 0X04
> > +#define START_NACK 0X05
> > +#define START_HIGH 0X06
> > +#define START_HIGH_NACK 0X07
> > +
> > +#define MCR_MEN (1 << 0)
> > +#define MCR_RST (1 << 1)
> > +#define MCR_DOZEN (1 << 2)
> > +#define MCR_DBGEN (1 << 3)
> > +#define MCR_RTF (1 << 8)
> > +#define MCR_RRF (1 << 9)
> > +#define MSR_TDF (1 << 0)
> > +#define MSR_RDF (1 << 1)
> > +#define MSR_SDF (1 << 9)
> > +#define MSR_NDF (1 << 10)
> > +#define MSR_ALF (1 << 11)
> > +#define MSR_MBF (1 << 24)
> > +#define MSR_BBF (1 << 25)
> > +#define MIER_TDIE (1 << 0)
> > +#define MIER_RDIE (1 << 1)
> > +#define MIER_SDIE (1 << 9)
> > +#define MIER_NDIE (1 << 10)
> > +#define MCFGR1_AUTOSTOP (1 << 8)
> > +#define MCFGR1_IGNACK (1 << 9)
> > +#define MRDR_RXEMPTY (1 << 14)
>
> Please use BIT() helper above.
Thanks, will change to BIT() in next version.
> Also please don't use tab symbol between #define and a token.
Got it, thanks!
> > +
> > +#define I2C_CLK_RATIO 2
> > +#define CHUNK_DATA 256
> > +
> > +#define LPI2C_RX_FIFOSIZE 4
> > +#define LPI2C_TX_FIFOSIZE 4
> > +
> > +#define LPI2C_DEFAULT_RATE 100000
> > +#define STARDARD_MAX_BITRATE 400000
> > +#define FAST_MAX_BITRATE 1000000
> > +#define FAST_PLUS_MAX_BITRATE 3400000
> > +#define HIGHSPEED_MAX_BITRATE 5000000
>
> Please don't use tab symbol right after #define
Thanks, will change it in next version.
> > +
> > +
>
> Double empty line, this kind of problem is reported by checkpatch --strict,
> please pay attention to all of them:
Thanks, I didn't use "--strict" option for checkpatch, so I missed this problem. Will change it in next version.
> total: 0 errors, 2 warnings, 33 checks, 715 lines checked
>
> > +enum lpi2c_imx_mode {
> > + STANDARD, /* 100+Kbps */
> > + FAST, /* 400+Kbps */
> > + FAST_PLUS, /* 1.0+Mbps */
> > + ULTRA_FAST, /* 5.0+Mbps */
> > + HS, /* 3.4+Mbps */
>
> Any reason why the list is not sorted by bus speed?
"HS" use different config with " ULTRA_FAST" and " FAST_PLUS", so I thought this order may be better. Will sort it by bus speed in next version. Thanks!
> > +};
> > +
> > +enum lpi2c_imx_pincfg {
> > + TWO_PIN_OD, /* 2-pin open drain mode */
> > + TWO_PIN_OO, /* 2-pin output only mode (utra-fast mode) */
> > + TWO_PIN_PP, /* 2-pin push-pull mode */
> > + FOUR_PIN_PP, /* 4-pin push-pull mode */
> > + TWO_PIN_OD_SS, /* 2-pin open drain mode with separate slave
> */
>
> Unused.
Will remove them in next version. Thanks!
> > + TWO_PIN_OO_SS, /* 2-pin output only mode with separate slave
> */
>
> Unused.
Thanks!
> > + TWO_PIN_PP_SS, /* 2-pin push-pull mode with separate slave */
>
> Unused.
Thanks!
> > + FOUR_PIN_PP_IO, /* 4-pin push-pull mode (inverted output) */
>
> Unused.
Thanks!
> > +};
> > +
> > +struct lpi2c_imx_clkcfg {
> > + u8 prescale;
> > + u8 filtscl;
> > + u8 filtsda;
> > + u8 sethold;
> > + u8 clklo;
> > + u8 clkhi;
> > + u8 datavd;
> > +};
> > +
> > +struct lpi2c_imx_struct {
> > + struct i2c_adapter adapter;
> > + struct clk *per_clk;
> > + void __iomem *base;
> > + __u8 *rx_buf;
> > + __u8 *tx_buf;
> > + struct completion complete;
> > + unsigned int msglen;
> > + unsigned int delivered;
> > + unsigned int block_data;
> > + unsigned int bitrate;
> > + enum lpi2c_imx_mode mode;
> > +};
> > +
> > +static void lpi2c_imx_intctrl(
> > + struct lpi2c_imx_struct *lpi2c_imx, unsigned int enable)
>
> Indentation issue.
Thanks!
> > +{
> > + writel(enable, lpi2c_imx->base + LPI2C_MIER); }
> > +
> > +static int lpi2c_imx_bus_busy(struct lpi2c_imx_struct *lpi2c_imx) {
> > + unsigned long orig_jiffies = jiffies;
> > + unsigned int temp;
> > +
> > + while (1) {
> > + temp = readl(lpi2c_imx->base + LPI2C_MSR);
> > +
> > + /* check for arbitration lost, clear if set */
> > + if (temp & MSR_ALF) {
> > + writel(temp, lpi2c_imx->base + LPI2C_MSR);
> > + return -EAGAIN;
> > + }
> > +
> > + if ((temp & MSR_BBF) && (temp & MSR_MBF))
>
> if (temp & (MSR_BBF | MSR_MBF))
Thanks, will change it in next version!
> > + break;
> > +
> > + if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
> > + dev_dbg(&lpi2c_imx->adapter.dev, "bus not work\n");
> > + return -ETIMEDOUT;
> > + }
> > + schedule();
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void lpi2c_imx_set_mode(struct lpi2c_imx_struct *lpi2c_imx) {
> > + enum lpi2c_imx_mode mode;
> > + unsigned int bitrate = lpi2c_imx->bitrate;
>
> unsigned int bitrate = lpi2c_imx->bitrate; enum lpi2c_imx_mode mode;
>
> If possible please use "reverse christmas tree" order while declaring local
> variables, this applies to some other functions below as well.
>
> I see that you mainly use "christmas tree" order, but this style isn't commonly
> used in the Linux kernel sources.
Thanks, will change it in next version.
> > +
> > + if (bitrate < STARDARD_MAX_BITRATE)
> > + mode = STANDARD;
> > + else if (bitrate < FAST_MAX_BITRATE)
> > + mode = FAST;
> > + else if (bitrate < FAST_PLUS_MAX_BITRATE)
> > + mode = FAST_PLUS;
> > + else if (bitrate < HIGHSPEED_MAX_BITRATE)
> > + mode = HS;
> > + else
> > + mode = ULTRA_FAST;
> > +
> > + lpi2c_imx->mode = mode;
> > +}
> > +
> > +static int lpi2c_imx_start(struct lpi2c_imx_struct *lpi2c_imx,
> > + struct i2c_msg *msgs)
> > +{
> > + u8 read;
> > + unsigned int temp;
> > +
> > + temp = readl(lpi2c_imx->base + LPI2C_MCR);
> > + temp |= MCR_RRF | MCR_RTF;
> > + writel(temp, lpi2c_imx->base + LPI2C_MCR);
> > + writel(0x7f00, lpi2c_imx->base + LPI2C_MSR);
> > +
> > + read = msgs->flags & I2C_M_RD;
> > + temp = (msgs->addr << 1 | read) | (GEN_START << 8);
> > + writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> > +
> > + return lpi2c_imx_bus_busy(lpi2c_imx); }
> > +
> > +static void lpi2c_imx_stop(struct lpi2c_imx_struct *lpi2c_imx) {
> > + unsigned int temp;
> > + unsigned long orig_jiffies = jiffies;
> > +
> > + writel(GEN_STOP << 8, lpi2c_imx->base + LPI2C_MTDR);
>
> Add an empty line here.
Thanks!
> > + do {
> > + temp = readl(lpi2c_imx->base + LPI2C_MSR);
> > + if (temp & MSR_SDF)
> > + break;
> > +
> > + if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
> > + dev_dbg(&lpi2c_imx->adapter.dev, "stop timeout\n");
> > + break;
> > + }
> > + schedule();
> > +
> > + } while (1);
> > +}
> > +
> > +
> > +/* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2
> > +*/ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx) {
> > + unsigned int temp;
> > + unsigned int per_clk_rate;
> > + unsigned int prescale, clk_high, clk_low, clk_cycle;
> > + enum lpi2c_imx_pincfg pincfg;
> > + struct lpi2c_imx_clkcfg clkcfg;
> > +
> > + lpi2c_imx_set_mode(lpi2c_imx);
> > + per_clk_rate = clk_get_rate(lpi2c_imx->per_clk);
> > +
> > + if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
>
> if (lpi2c_imx->mode > FAST_PLUS)
>
> > + clkcfg.filtscl = clkcfg.filtsda = 0;
> > + else
> > + clkcfg.filtscl = clkcfg.filtsda = 2;
> > +
>
> Multiple assignments on a single line are not welcome, in this case one variable
> "filt" assigned to 0 or 2 should be enough.
Thanks, will change it in next version.
> Why do you need struct lpi2c_imx_clkcfg in general?
>
> clkcfg.filtscl, clkcfg.filtsda etc. are all used locally inside this function only, it
> should be sufficient to replace "clkcfg"
> with a number of local variables.
Yes, you are right. Will change it in next version. Thanks!
> > + for (prescale = 0; prescale <= 7; prescale++) {
> > + clk_cycle = per_clk_rate / ((1 << prescale) * lpi2c_imx->bitrate)
> > + - 3 - (clkcfg.filtscl >> 1);
> > + clk_high = (clk_cycle + I2C_CLK_RATIO) / (I2C_CLK_RATIO + 1);
> > + clk_low = clk_cycle - clk_high;
> > + if (clk_low < 64)
> > + break;
> > + }
> > +
> > + if (prescale > 7)
> > + return -EINVAL;
> > +
> > + clkcfg.prescale = prescale;
> > + clkcfg.sethold = clk_high;
> > + clkcfg.clklo = clk_low;
> > + clkcfg.clkhi = clk_high;
> > + clkcfg.datavd = clk_high >> 1;
>
> Useless duplication of variables, see a note above.
Thanks!
> > +
> > + /* set MCFGR1: PINCFG, PRESCALE, IGNACK */
> > + if (lpi2c_imx->mode == ULTRA_FAST)
> > + pincfg = TWO_PIN_OO;
> > + else
> > + pincfg = TWO_PIN_OD;
> > + temp = clkcfg.prescale | pincfg << 24;
> > +
> > + if (lpi2c_imx->mode == ULTRA_FAST)
> > + temp |= MCFGR1_IGNACK;
> > +
> > + writel(temp, lpi2c_imx->base + LPI2C_MCFGR1);
> > +
> > + /* set MCFGR2: FILTSDA, FILTSCL */
> > + temp = (clkcfg.filtscl << 16) | (clkcfg.filtsda << 24);
> > + writel(temp, lpi2c_imx->base + LPI2C_MCFGR2);
> > +
> > +
> > + /* set MCCR: DATAVD, SETHOLD, CLKHI, CLKLO */
> > + temp = clkcfg.datavd << 24 | clkcfg.sethold << 16 |
> > + clkcfg.clkhi << 8 | clkcfg.clklo;
> > +
> > + if (lpi2c_imx->mode == HS)
> > + writel(temp, lpi2c_imx->base + LPI2C_MCCR1);
> > + else
> > + writel(temp, lpi2c_imx->base + LPI2C_MCCR0);
> > +
> > + return 0;
> > +}
> > +
> > +static int lpi2c_imx_master_enable(struct lpi2c_imx_struct
> > +*lpi2c_imx) {
> > + int ret;
> > + unsigned int temp;
> > +
> > + ret = clk_prepare_enable(lpi2c_imx->per_clk);
>
> You can do clk_prepare() in probe function and clk_unprepare() in remove
> function to avoid potential sleeping in runtime, then here you just do
> clk_enable()/clk_disable().
Thanks, will change it in next version!
> > + if (ret)
> > + return ret;
> > +
> > + temp = MCR_RST;
> > + writel(temp, lpi2c_imx->base + LPI2C_MCR);
> > + writel(0, lpi2c_imx->base + LPI2C_MCR);
> > +
> > + ret = lpi2c_imx_config(lpi2c_imx);
> > + if (ret)
> > + return ret;
> > +
> > + temp = readl(lpi2c_imx->base + LPI2C_MCR);
> > + temp |= MCR_MEN;
> > + writel(temp, lpi2c_imx->base + LPI2C_MCR);
> > +
> > + return 0;
> > +}
> > +
> > +static int lpi2c_imx_master_disable(struct lpi2c_imx_struct
> > +*lpi2c_imx) {
> > + unsigned int temp = 0;
> > +
> > + temp = readl(lpi2c_imx->base + LPI2C_MCR);
> > + temp &= ~MCR_MEN;
> > + writel(temp, lpi2c_imx->base + LPI2C_MCR);
> > +
> > + clk_disable_unprepare(lpi2c_imx->per_clk);
>
> See a note above.
Thanks!
> > +
> > + return 0;
> > +}
> > +
> > +static int lpi2c_imx_msg_complete(struct lpi2c_imx_struct *lpi2c_imx)
> > +{
> > + unsigned int timeout;
> > +
> > + timeout = wait_for_completion_timeout(&lpi2c_imx->complete, HZ);
> > +
> > + return timeout ? 0 : -ETIMEDOUT;
> > +}
> > +
> > +static int lpi2c_imx_txfifo_empty(struct lpi2c_imx_struct *lpi2c_imx)
> > +{
> > + u32 txcnt;
> > + unsigned long orig_jiffies = jiffies;
> > +
> > + do {
> > + txcnt = readl(lpi2c_imx->base + LPI2C_MFSR) & 0xff;
> > +
> > + if (readl(lpi2c_imx->base + LPI2C_MSR) & MSR_NDF) {
> > + dev_dbg(&lpi2c_imx->adapter.dev, "NDF detected\n");
> > + return -EIO;
> > + }
> > +
> > + if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
> > + dev_dbg(&lpi2c_imx->adapter.dev, "txfifo empty
> timeout\n");
> > + return -ETIMEDOUT;
> > + }
> > + schedule();
> > +
> > + } while (txcnt);
> > +
> > + return 0;
> > +}
> > +
> > +static void lpi2c_imx_set_tx_watermark(struct lpi2c_imx_struct
> > +*lpi2c_imx) {
> > + unsigned int temp;
> > +
> > + temp = LPI2C_TX_FIFOSIZE >> 1;
> > + writel(temp, lpi2c_imx->base + LPI2C_MFCR);
>
> writel(LPI2C_TX_FIFOSIZE >> 1, lpi2c_imx->base + LPI2C_MFCR);
Thanks, will change it in next version.
> > +}
> > +
> > +static void lpi2c_imx_set_rx_watermark(struct lpi2c_imx_struct
> > +*lpi2c_imx) {
> > + unsigned int temp, remaining;
> > +
> > + remaining = lpi2c_imx->msglen - lpi2c_imx->delivered;
> > +
> > + if (remaining > (LPI2C_RX_FIFOSIZE >> 1))
> > + temp = LPI2C_RX_FIFOSIZE >> 1;
> > + else
> > + temp = 0;
> > +
> > + writel(temp << 16, lpi2c_imx->base + LPI2C_MFCR); }
> > +
> > +static void lpi2c_imx_write_txfifo(struct lpi2c_imx_struct
> > +*lpi2c_imx) {
> > + unsigned int data, txcnt;
> > +
> > + txcnt = readl(lpi2c_imx->base + LPI2C_MFSR) & 0xff;
>
> Add an empty line here.
Thanks!
> > + while (txcnt < LPI2C_TX_FIFOSIZE) {
> > + if (lpi2c_imx->delivered == lpi2c_imx->msglen)
> > + break;
>
> Add an empty line here.
Thanks!
> > + data = lpi2c_imx->tx_buf[lpi2c_imx->delivered++];
> > + writel(data, lpi2c_imx->base + LPI2C_MTDR);
> > + txcnt++;
> > + }
> > +
> > + if (lpi2c_imx->delivered < lpi2c_imx->msglen)
> > + lpi2c_imx_intctrl(lpi2c_imx, MIER_TDIE | MIER_NDIE);
> > + else
> > + complete(&lpi2c_imx->complete);
> > +}
> > +
> > +static void lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx)
> > +{
> > + unsigned int temp, data;
> > + unsigned int blocklen, remaining;
> > +
> > + do {
> > + data = readl(lpi2c_imx->base + LPI2C_MRDR);
> > + if (data & MRDR_RXEMPTY)
> > + break;
> Add an empty line here.
Thanks!
>
> > + lpi2c_imx->rx_buf[lpi2c_imx->delivered++] = data & 0xff;
> > + } while (1);
> > +
> > + /*
> > + * First byte is the length of remaining packet in the SMBus block
> > + * data read. Add it to msgs->len.
> > + */
> > + if (lpi2c_imx->block_data) {
> > + blocklen = lpi2c_imx->rx_buf[0];
> > + lpi2c_imx->msglen += blocklen;
> > + }
> > +
> > + remaining = lpi2c_imx->msglen - lpi2c_imx->delivered;
> > + /* not finished, still waiting for rx data */
>
> Please move the comment under if (remaining) condition.
Thanks, will change it in next version!
> > + if (remaining) {
> > + lpi2c_imx_set_rx_watermark(lpi2c_imx);
> > + /* multiple receive commands */
> > + if (lpi2c_imx->block_data) {
> > + lpi2c_imx->block_data = 0;
> > + temp = remaining;
> > + temp |= (RECV_DATA << 8);
> > + writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> > + } else if (!(lpi2c_imx->delivered & 0xff)) {
> > + temp = remaining > CHUNK_DATA ?
> > + CHUNK_DATA - 1 : (remaining - 1);
> > + temp |= (RECV_DATA << 8);
> > + writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> > + }
> > +
> > + lpi2c_imx_intctrl(lpi2c_imx, MIER_RDIE);
> > + } else
> > + complete(&lpi2c_imx->complete);
>
> Start it from
>
> if (!remaining) {
> complete(&lpi2c_imx->complete);
> return;
> }
>
> /* not finished, still waiting for rx data */ ....
>
> Then you get less indentations. Generally please use more return points instead
> of if-if-if constructions.
>
Thanks, will change it in next version!
> > +}
> > +
> > +static void lpi2c_imx_write(struct lpi2c_imx_struct *lpi2c_imx,
> > + struct i2c_msg *msgs)
> > +{
> > + lpi2c_imx->tx_buf = msgs->buf;
> > + lpi2c_imx_set_tx_watermark(lpi2c_imx);
> > + lpi2c_imx_write_txfifo(lpi2c_imx);
> > +}
> > +
> > +static void lpi2c_imx_read(struct lpi2c_imx_struct *lpi2c_imx,
> > + struct i2c_msg *msgs)
> > +{
> > + unsigned int temp;
> > +
> > + lpi2c_imx->rx_buf = msgs->buf;
> > + lpi2c_imx->block_data = msgs->flags & I2C_M_RECV_LEN;
> > +
> > + lpi2c_imx_set_rx_watermark(lpi2c_imx);
> > + temp = msgs->len > CHUNK_DATA ? CHUNK_DATA - 1 : msgs->len - 1;
> > + temp |= (RECV_DATA << 8);
> > + writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> > +
> > + lpi2c_imx_intctrl(lpi2c_imx, MIER_RDIE | MIER_NDIE); }
> > +
> > +static int lpi2c_imx_xfer(struct i2c_adapter *adapter,
> > + struct i2c_msg *msgs, int num)
> > +{
> > + int i, result;
> > + unsigned int temp;
> > + struct lpi2c_imx_struct *lpi2c_imx = i2c_get_adapdata(adapter);
> > +
> > + result = lpi2c_imx_master_enable(lpi2c_imx);
> > + if (result)
> > + return result;
> > +
> > + for (i = 0; i < num; i++) {
> > + result = lpi2c_imx_start(lpi2c_imx, &msgs[i]);
> > + if (result)
> > + goto disable;
> > +
> > + /* quick smbus */
> > + if (num == 1 && msgs[0].len == 0)
> > + goto stop;
> > +
> > + lpi2c_imx->delivered = 0;
> > + lpi2c_imx->msglen = msgs[i].len;
> > + init_completion(&lpi2c_imx->complete);
> > +
> > + if (msgs[i].flags & I2C_M_RD)
> > + lpi2c_imx_read(lpi2c_imx, &msgs[i]);
> > + else
> > + lpi2c_imx_write(lpi2c_imx, &msgs[i]);
> > +
> > + result = lpi2c_imx_msg_complete(lpi2c_imx);
> > + if (result)
> > + goto stop;
> > +
> > + if (!(msgs[i].flags & I2C_M_RD)) {
> > + result = lpi2c_imx_txfifo_empty(lpi2c_imx);
> > + if (result)
> > + goto stop;
> > + }
> > + }
> > +
> > +stop:
> > + lpi2c_imx_stop(lpi2c_imx);
> > +
> > + temp = readl(lpi2c_imx->base + LPI2C_MSR);
> > + if ((temp & MSR_NDF) && !result)
> > + result = -EIO;
>
> Zero-length transactions are not supported, right?
The driver support zero-length transactions. For zero-length transactions, the transfer direction field represents data field.
It is transferred with i2c start CMD.
> > +
> > +disable:
> > + lpi2c_imx_master_disable(lpi2c_imx);
> > +
> > + dev_dbg(&lpi2c_imx->adapter.dev, "<%s> exit with: %s: %d\n",
> __func__,
> > + (result < 0) ? "error" : "success msg",
> > + (result < 0) ? result : num);
> > +
> > + return (result < 0) ? result : num;
> > +}
> > +
> > +static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id) {
> > + unsigned int temp;
> > + struct lpi2c_imx_struct *lpi2c_imx = dev_id;
> > +
> > + lpi2c_imx_intctrl(lpi2c_imx, 0);
> > + temp = readl(lpi2c_imx->base + LPI2C_MSR);
> > +
> > + if (temp & MSR_RDF) {
> > + lpi2c_imx_read_rxfifo(lpi2c_imx);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + if (temp & MSR_TDF) {
> > + lpi2c_imx_write_txfifo(lpi2c_imx);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + complete(&lpi2c_imx->complete);
>
> Add an empty line here.
Thanks, will change it in next version.
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static u32 lpi2c_imx_func(struct i2c_adapter *adapter) {
> > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
> > + | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
>
> checkpatch does not complain? I expect it should be
>
> return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> I2C_FUNC_SMBUS_READ_BLOCK_DATA;
>
Thanks, will change it in next version!
> > +}
> > +
> > +static struct i2c_algorithm lpi2c_imx_algo = {
> > + .master_xfer = lpi2c_imx_xfer,
> > + .functionality = lpi2c_imx_func,
> > +};
> > +
> > +static const struct of_device_id lpi2c_imx_of_match[] = {
> > + { .compatible = "fsl,imx8dv-lpi2c" },
> > + { .compatible = "fsl,imx7ulp-lpi2c" },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, lpi2c_imx_of_match)
> > +
> > +static int lpi2c_imx_probe(struct platform_device *pdev) {
> > + int irq, ret;
> > + void __iomem *base;
> > + struct resource *res;
> > + struct lpi2c_imx_struct *lpi2c_imx;
> > +
> > + lpi2c_imx = devm_kzalloc(&pdev->dev,
> > + sizeof(*lpi2c_imx), GFP_KERNEL);
> > + if (!lpi2c_imx)
> > + return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(base))
> > + return PTR_ERR(base);
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_err(&pdev->dev, "can't get irq number\n");
> > + return irq;
> > + }
> > +
> > + lpi2c_imx->adapter.owner = THIS_MODULE;
> > + lpi2c_imx->adapter.algo = &lpi2c_imx_algo;
> > + lpi2c_imx->adapter.dev.parent = &pdev->dev;
> > + lpi2c_imx->adapter.nr = pdev->id;
>
> Do you really need it? Please consider to use i2c_add_adapter().
You are right, i2c_add_adapter() is a better option. Will change it in next version, Thanks.
> > + lpi2c_imx->base = base;
>
> For sake of consistency please initialize lpi2c_imx->adapter fields in a row.
>
> You don't need this local 'base' variable, use lpi2c_imx->base instead.
Thanks, Will change it in next version.
> > + lpi2c_imx->adapter.dev.of_node = pdev->dev.of_node;
> > + strlcpy(lpi2c_imx->adapter.name, pdev->name,
> > + sizeof(lpi2c_imx->adapter.name));
> > +
> > + lpi2c_imx->per_clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(lpi2c_imx->per_clk)) {
> > + dev_err(&pdev->dev, "can't get I2C peripheral clock\n");
> > + return PTR_ERR(lpi2c_imx->per_clk);
> > + }
> > +
> > + ret = of_property_read_u32(pdev->dev.of_node,
> > + "clock-frequency", &lpi2c_imx->bitrate);
> > + if (ret)
> > + lpi2c_imx->bitrate = LPI2C_DEFAULT_RATE;
> > +
> > + ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
> > + pdev->name, lpi2c_imx);
> > + if (ret) {
> > + dev_err(&pdev->dev, "can't claim irq %d\n", irq);
> > + goto ret;
>
> Just return ret;
Thanks!
> > + }
> > +
> > + i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
> > + platform_set_drvdata(pdev, lpi2c_imx);
> > +
> > + ret = i2c_add_numbered_adapter(&lpi2c_imx->adapter);
> > + if (ret) {
> > + dev_err(&pdev->dev, "registration failed\n");
> > + goto ret;
>
> Just return ret;
Thanks!
> > + }
> > +
> > + dev_info(&lpi2c_imx->adapter.dev, "LPI2C adapter registered\n");
> > +
> > +ret:
> > + return ret;
>
> return 0;
Thanks!
> > +}
> > +
> > +static int lpi2c_imx_remove(struct platform_device *pdev) {
> > + struct lpi2c_imx_struct *lpi2c_imx = platform_get_drvdata(pdev);
> > +
> > + i2c_del_adapter(&lpi2c_imx->adapter);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver lpi2c_imx_driver = {
> > + .probe = lpi2c_imx_probe,
> > + .remove = lpi2c_imx_remove,
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .of_match_table = lpi2c_imx_of_match,
> > + },
> > +};
> > +
> > +static int __init i2c_adap_imx_init(void) {
> > + return platform_driver_register(&lpi2c_imx_driver);
> > +}
> > +module_init(i2c_adap_imx_init);
> > +
> > +static void __exit i2c_adap_imx_exit(void) {
> > + platform_driver_unregister(&lpi2c_imx_driver);
> > +}
> > +module_exit(i2c_adap_imx_exit);
> > +
>
> Please use module_platform_driver(lpi2c_imx_driver);
Thanks, will change it in next version!
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Gao Pan <pandy.gao@nxp.com>");
> MODULE_DESCRIPTION("I2C
> > +adapter driver for LPI2C bus"); MODULE_ALIAS("platform:"
> > +DRIVER_NAME);
> >
>
> Are you sure that the driver needs a platform alias here?
Thanks, will remove it in next version.
Thanks again for your precise review, it really helps to improve the code quality!
Best Regards
Gao Pan
^ permalink raw reply
* Re: [PATCH v4] i2c: designware: Implement support for SMBus block read and write
From: Andy Shevchenko @ 2016-11-14 11:03 UTC (permalink / raw)
To: Alexander Stein, linux-kernel
Cc: tnhuynh, Jarkko Nikula, Mika Westerberg, Wolfram Sang, linux-i2c,
Loc Ho, Thang Nguyen, Phong Vo, patches
In-Reply-To: <4917823.j8OxAtZYNT@ws-stein>
On Mon, 2016-11-14 at 11:59 +0100, Alexander Stein wrote:
> On Thursday 10 November 2016 09:56:33, tnhuynh@apm.com wrote:
> > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> > @@ -75,6 +75,7 @@ struct dw_pci_controller {
> > I2C_FUNC_SMBUS_BYTE |
> > \
> > I2C_FUNC_SMBUS_BYTE_DATA |
> > \
> > I2C_FUNC_SMBUS_WORD_DATA |
> > \
> > + I2C_FUNC_SMBUS_BLOCK_DATA |
> > \
> > I2C_FUNC_SMBUS_I2C_BLOCK)
> >
> > /* Merrifield HCNT/LCNT/SDA hold time */
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > b/drivers/i2c/busses/i2c-designware-platdrv.c index 0b42a12..886fb62
> > 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -220,6 +220,7 @@ static int dw_i2c_plat_probe(struct
> > platform_device
> > *pdev) I2C_FUNC_SMBUS_BYTE |
> > I2C_FUNC_SMBUS_BYTE_DATA |
> > I2C_FUNC_SMBUS_WORD_DATA |
> > + I2C_FUNC_SMBUS_BLOCK_DATA |
> > I2C_FUNC_SMBUS_I2C_BLOCK;
> >
> > dev->master_cfg = DW_IC_CON_MASTER |
> > DW_IC_CON_SLAVE_DISABLE |
>
> Shouldn't those functionality bits moved to a common place, like i2c-
> designware-core.h?
It would. But it's a separate story. So, if you are willing to do this,
go ahead and send a patch.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v4] i2c: designware: Implement support for SMBus block read and write
From: Alexander Stein @ 2016-11-14 10:59 UTC (permalink / raw)
To: linux-kernel
Cc: tnhuynh, Jarkko Nikula, Andy Shevchenko, Mika Westerberg,
Wolfram Sang, linux-i2c, Loc Ho, Thang Nguyen, Phong Vo, patches
In-Reply-To: <1478746593-10905-1-git-send-email-tnhuynh@apm.com>
On Thursday 10 November 2016 09:56:33, tnhuynh@apm.com wrote:
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
> b/drivers/i2c/busses/i2c-designware-pcidrv.c index 96f8230..8ffe2da 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -75,6 +75,7 @@ struct dw_pci_controller {
> I2C_FUNC_SMBUS_BYTE | \
> I2C_FUNC_SMBUS_BYTE_DATA | \
> I2C_FUNC_SMBUS_WORD_DATA | \
> + I2C_FUNC_SMBUS_BLOCK_DATA | \
> I2C_FUNC_SMBUS_I2C_BLOCK)
>
> /* Merrifield HCNT/LCNT/SDA hold time */
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c index 0b42a12..886fb62 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -220,6 +220,7 @@ static int dw_i2c_plat_probe(struct platform_device
> *pdev) I2C_FUNC_SMBUS_BYTE |
> I2C_FUNC_SMBUS_BYTE_DATA |
> I2C_FUNC_SMBUS_WORD_DATA |
> + I2C_FUNC_SMBUS_BLOCK_DATA |
> I2C_FUNC_SMBUS_I2C_BLOCK;
>
> dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
Shouldn't those functionality bits moved to a common place, like i2c-
designware-core.h?
Best regards,
Alexander
--
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein@systec-electronic.com
Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany
Office: +49 (0) 3765 38600-0
Fax: +49 (0) 3765 38600-4100
Managing Directors:
Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010
^ permalink raw reply
* Re: [PATCH v4] i2c: designware: Implement support for SMBus block read and write
From: Andy Shevchenko @ 2016-11-14 10:33 UTC (permalink / raw)
To: tnhuynh, Jarkko Nikula, Mika Westerberg, Wolfram Sang, linux-i2c,
linux-kernel
Cc: Loc Ho, Thang Nguyen, Phong Vo, patches
In-Reply-To: <1478746593-10905-1-git-send-email-tnhuynh@apm.com>
On Thu, 2016-11-10 at 09:56 +0700, tnhuynh@apm.com wrote:
> From: Tin Huynh <tnhuynh@apm.com>
>
> Free and Open IPMI use SMBUS BLOCK Read/Write to support SSIF
> protocol.
> However, I2C Designware Core Driver doesn't handle the case at the
> moment.
> The below patch supports this feature.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
> ---
> Change from V3:
> - Correct coding conventions
> - Make clean
> Change from V2:
> - Change subject of email
> - Add a helper function to handle
> length byte receiving
> Change from V1:
> - Remove empty lines
> - Add flags variable to make clean code
> - Change DW_DEFAULT_FUNCTIONALITY
> in i2c-designware-pcidrv.c
> ---
> drivers/i2c/busses/i2c-designware-core.c | 46
> +++++++++++++++++++++++++--
> drivers/i2c/busses/i2c-designware-pcidrv.c | 1 +
> drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
> 3 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.c
> b/drivers/i2c/busses/i2c-designware-core.c
> index 1fe93c4..c91d1b4 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -543,6 +543,8 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
> intr_mask = DW_IC_INTR_DEFAULT_MASK;
>
> for (; dev->msg_write_idx < dev->msgs_num; dev-
> >msg_write_idx++) {
> + u32 flags = msgs[dev->msg_write_idx].flags;
> +
> /*
> * if target address has changed, we need to
> * reprogram the target address in the i2c
> @@ -588,8 +590,15 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
> * detected from the registers so we set it
> always
> * when writing/reading the last byte.
> */
> +
> + /*
> + * i2c-core.c always sets the buffer length
> of
> + * I2C_FUNC_SMBUS_BLOCK_DATA to 1. The length
> will
> + * be adjusted when receiving the first byte.
> + * Thus we can't stop the transaction here.
> + */
> if (dev->msg_write_idx == dev->msgs_num - 1
> &&
> - buf_len == 1)
> + buf_len == 1 && !(flags &
> I2C_M_RECV_LEN))
> cmd |= BIT(9);
>
> if (need_restart) {
> @@ -614,7 +623,12 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
> dev->tx_buf = buf;
> dev->tx_buf_len = buf_len;
>
> - if (buf_len > 0) {
> + /*
> + * Because we don't know the buffer length in the
> + * I2C_FUNC_SMBUS_BLOCK_DATA case, we can't stop
> + * the transaction here.
> + */
> + if (buf_len > 0 || flags & I2C_M_RECV_LEN) {
> /* more bytes to be written */
> dev->status |= STATUS_WRITE_IN_PROGRESS;
> break;
> @@ -635,6 +649,24 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
> dw_writel(dev, intr_mask, DW_IC_INTR_MASK);
> }
>
> +static u8
> +i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
> +{
> + struct i2c_msg *msgs = dev->msgs;
> + u32 flags = msgs[dev->msg_read_idx].flags;
> +
> + /*
> + * Adjust the buffer length and mask the flag
> + * after receiving the first byte.
> + */
> + len += (flags & I2C_CLIENT_PEC) ? 2 : 1;
> + dev->tx_buf_len = len - min_t(u8, len, dev->rx_outstanding);
> + msgs[dev->msg_read_idx].len = len;
> + msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
> +
> + return len;
> +}
> +
> static void
> i2c_dw_read(struct dw_i2c_dev *dev)
> {
> @@ -659,7 +691,15 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
> rx_valid = dw_readl(dev, DW_IC_RXFLR);
>
> for (; len > 0 && rx_valid > 0; len--, rx_valid--) {
> - *buf++ = dw_readl(dev, DW_IC_DATA_CMD);
> + u32 flags = msgs[dev->msg_read_idx].flags;
> +
> + *buf = dw_readl(dev, DW_IC_DATA_CMD);
> + /* Ensure length byte is a valid value */
> + if (flags & I2C_M_RECV_LEN &&
> + *buf <= I2C_SMBUS_BLOCK_MAX && *buf >
> 0) {
> + len = i2c_dw_recv_len(dev, *buf);
> + }
> + buf++;
> dev->rx_outstanding--;
> }
>
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
> b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 96f8230..8ffe2da 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -75,6 +75,7 @@ struct dw_pci_controller {
> I2C_FUNC_SMBUS_BYTE |
> \
> I2C_FUNC_SMBUS_BYTE_DATA |
> \
> I2C_FUNC_SMBUS_WORD_DATA |
> \
> + I2C_FUNC_SMBUS_BLOCK_DATA |
> \
> I2C_FUNC_SMBUS_I2C_BLOCK)
>
> /* Merrifield HCNT/LCNT/SDA hold time */
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0b42a12..886fb62 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -220,6 +220,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> I2C_FUNC_SMBUS_BYTE |
> I2C_FUNC_SMBUS_BYTE_DATA |
> I2C_FUNC_SMBUS_WORD_DATA |
> + I2C_FUNC_SMBUS_BLOCK_DATA |
> I2C_FUNC_SMBUS_I2C_BLOCK;
>
> dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE
> |
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* [Patch V4 1/2] i2c: imx: add low power i2c bus driver
From: Gao Pan @ 2016-11-14 9:23 UTC (permalink / raw)
To: wsa, u.kleine-koenig, cmo, robh, vz
Cc: linux-i2c, pandy.gao, frank.li, fugang.duan
This patch adds lpi2c bus driver to support new i.MX products
which use lpi2c instead of the old imx i2c.
The lpi2c can continue operating in stop mode when an appropriate
clock is available. It is also designed for low CPU overhead with
DMA offloading of FIFO register accesses.
Signed-off-by: Gao Pan <pandy.gao@nxp.com>
Reviewed-by: Fugang Duan <B38611@freescale.com>
---
V2:
-stop i2c transfer under the wrong condition
-add timeout check in while() domain
V3:
-fix typo inside commit message and the driver.
V4:
As Vladimir Zapolskiy's review, the version do below changes:
-split devicetree binding with driver
-prepare clk in probe()
-replace "1<<n" BIT(n)
-remove unnecessary variables
-fix typo
-use module_platform_driver()
-use i2c_add_adapter()
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-imx-lpi2c.c | 640 +++++++++++++++++++++++++++++++++++++
3 files changed, 651 insertions(+)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index d252276..7cff9ec 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -597,6 +597,16 @@ config I2C_IMX
This driver can also be built as a module. If so, the module
will be called i2c-imx.
+config I2C_IMX_LPI2C
+ tristate "IMX Low Power I2C interface"
+ depends on ARCH_MXC || COMPILE_TEST
+ help
+ Say Y here if you want to use the Low Power IIC bus controller
+ on the Freescale i.MX processors.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-imx-lpi2c.
+
config I2C_IOP3XX
tristate "Intel IOPx3xx and IXP4xx on-chip I2C interface"
depends on ARCH_IOP32X || ARCH_IOP33X || ARCH_IXP4XX || ARCH_IOP13XX
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 29764cc..05195cd 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_I2C_HIX5HD2) += i2c-hix5hd2.o
obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o
obj-$(CONFIG_I2C_IMG) += i2c-img-scb.o
obj-$(CONFIG_I2C_IMX) += i2c-imx.o
+obj-$(CONFIG_I2C_IMX_LPI2C) += i2c-imx-lpi2c.o
obj-$(CONFIG_I2C_IOP3XX) += i2c-iop3xx.o
obj-$(CONFIG_I2C_JZ4780) += i2c-jz4780.o
obj-$(CONFIG_I2C_KEMPLD) += i2c-kempld.o
diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
new file mode 100644
index 0000000..c434fd0
--- /dev/null
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -0,0 +1,640 @@
+/*
+ * This is i.MX low power i2c controller driver.
+ *
+ * Copyright 2016 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+
+#define DRIVER_NAME "imx-lpi2c"
+
+#define LPI2C_PARAM 0x04 /* i2c RX/TX FIFO size */
+#define LPI2C_MCR 0x10 /* i2c contrl register */
+#define LPI2C_MSR 0x14 /* i2c status register */
+#define LPI2C_MIER 0x18 /* i2c interrupt enable */
+#define LPI2C_MCFGR0 0x20 /* i2c master configuration */
+#define LPI2C_MCFGR1 0x24 /* i2c master configuration */
+#define LPI2C_MCFGR2 0x28 /* i2c master configuration */
+#define LPI2C_MCFGR3 0x2C /* i2c master configuration */
+#define LPI2C_MCCR0 0x48 /* i2c master clk configuration */
+#define LPI2C_MCCR1 0x50 /* i2c master clk configuration */
+#define LPI2C_MFCR 0x58 /* i2c master FIFO control */
+#define LPI2C_MFSR 0x5C /* i2c master FIFO status */
+#define LPI2C_MTDR 0x60 /* i2c master TX data register */
+#define LPI2C_MRDR 0x70 /* i2c master RX data register */
+
+/* i2c command */
+#define TRAN_DATA 0X00
+#define RECV_DATA 0X01
+#define GEN_STOP 0X02
+#define RECV_DISCARD 0X03
+#define GEN_START 0X04
+#define START_NACK 0X05
+#define START_HIGH 0X06
+#define START_HIGH_NACK 0X07
+
+#define MCR_MEN BIT(0)
+#define MCR_RST BIT(1)
+#define MCR_DOZEN BIT(2)
+#define MCR_DBGEN BIT(3)
+#define MCR_RTF BIT(8)
+#define MCR_RRF BIT(9)
+#define MSR_TDF BIT(0)
+#define MSR_RDF BIT(1)
+#define MSR_SDF BIT(9)
+#define MSR_NDF BIT(10)
+#define MSR_ALF BIT(11)
+#define MSR_MBF BIT(24)
+#define MSR_BBF BIT(25)
+#define MIER_TDIE BIT(0)
+#define MIER_RDIE BIT(1)
+#define MIER_SDIE BIT(9)
+#define MIER_NDIE BIT(10)
+#define MCFGR1_AUTOSTOP BIT(8)
+#define MCFGR1_IGNACK BIT(9)
+#define MRDR_RXEMPTY BIT(14)
+
+#define I2C_CLK_RATIO 2
+#define CHUNK_DATA 256
+
+#define LPI2C_RX_FIFOSIZE 4
+#define LPI2C_TX_FIFOSIZE 4
+
+#define LPI2C_DEFAULT_RATE 100000
+#define STARDARD_MAX_BITRATE 400000
+#define FAST_MAX_BITRATE 1000000
+#define FAST_PLUS_MAX_BITRATE 3400000
+#define HIGHSPEED_MAX_BITRATE 5000000
+
+enum lpi2c_imx_mode {
+ STANDARD, /* 100+Kbps */
+ FAST, /* 400+Kbps */
+ FAST_PLUS, /* 1.0+Mbps */
+ HS, /* 3.4+Mbps */
+ ULTRA_FAST, /* 5.0+Mbps */
+};
+
+enum lpi2c_imx_pincfg {
+ TWO_PIN_OD,
+ TWO_PIN_OO,
+ TWO_PIN_PP,
+ FOUR_PIN_PP,
+};
+
+struct lpi2c_imx_struct {
+ struct i2c_adapter adapter;
+ struct clk *clk;
+ void __iomem *base;
+ __u8 *rx_buf;
+ __u8 *tx_buf;
+ struct completion complete;
+ unsigned int msglen;
+ unsigned int delivered;
+ unsigned int block_data;
+ unsigned int bitrate;
+ enum lpi2c_imx_mode mode;
+};
+
+static void lpi2c_imx_intctrl(struct lpi2c_imx_struct *lpi2c_imx,
+ unsigned int enable)
+{
+ writel(enable, lpi2c_imx->base + LPI2C_MIER);
+}
+
+static int lpi2c_imx_bus_busy(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ unsigned long orig_jiffies = jiffies;
+ unsigned int temp;
+
+ while (1) {
+ temp = readl(lpi2c_imx->base + LPI2C_MSR);
+
+ /* check for arbitration lost, clear if set */
+ if (temp & MSR_ALF) {
+ writel(temp, lpi2c_imx->base + LPI2C_MSR);
+ return -EAGAIN;
+ }
+
+ if (temp & (MSR_BBF | MSR_MBF))
+ break;
+
+ if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
+ dev_dbg(&lpi2c_imx->adapter.dev, "bus not work\n");
+ return -ETIMEDOUT;
+ }
+ schedule();
+ }
+
+ return 0;
+}
+
+static void lpi2c_imx_set_mode(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ unsigned int bitrate = lpi2c_imx->bitrate;
+ enum lpi2c_imx_mode mode;
+
+ if (bitrate < STARDARD_MAX_BITRATE)
+ mode = STANDARD;
+ else if (bitrate < FAST_MAX_BITRATE)
+ mode = FAST;
+ else if (bitrate < FAST_PLUS_MAX_BITRATE)
+ mode = FAST_PLUS;
+ else if (bitrate < HIGHSPEED_MAX_BITRATE)
+ mode = HS;
+ else
+ mode = ULTRA_FAST;
+
+ lpi2c_imx->mode = mode;
+}
+
+static int lpi2c_imx_start(struct lpi2c_imx_struct *lpi2c_imx,
+ struct i2c_msg *msgs)
+{
+ unsigned int temp;
+ u8 read;
+
+ temp = readl(lpi2c_imx->base + LPI2C_MCR);
+ temp |= MCR_RRF | MCR_RTF;
+ writel(temp, lpi2c_imx->base + LPI2C_MCR);
+ writel(0x7f00, lpi2c_imx->base + LPI2C_MSR);
+
+ read = msgs->flags & I2C_M_RD;
+ temp = (msgs->addr << 1 | read) | (GEN_START << 8);
+ writel(temp, lpi2c_imx->base + LPI2C_MTDR);
+
+ return lpi2c_imx_bus_busy(lpi2c_imx);
+}
+
+static void lpi2c_imx_stop(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ unsigned long orig_jiffies = jiffies;
+ unsigned int temp;
+
+ writel(GEN_STOP << 8, lpi2c_imx->base + LPI2C_MTDR);
+
+ do {
+ temp = readl(lpi2c_imx->base + LPI2C_MSR);
+ if (temp & MSR_SDF)
+ break;
+
+ if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
+ dev_dbg(&lpi2c_imx->adapter.dev, "stop timeout\n");
+ break;
+ }
+ schedule();
+
+ } while (1);
+}
+
+/* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2 */
+static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ u8 prescale, filt, sethold, clkhi, clklo, datavd;
+ unsigned int clk_rate, clk_cycle;
+ enum lpi2c_imx_pincfg pincfg;
+ unsigned int temp;
+
+ lpi2c_imx_set_mode(lpi2c_imx);
+
+ clk_rate = clk_get_rate(lpi2c_imx->clk);
+ if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
+ filt = 0;
+ else
+ filt = 2;
+
+ for (prescale = 0; prescale <= 7; prescale++) {
+ clk_cycle = clk_rate / ((1 << prescale) * lpi2c_imx->bitrate)
+ - 3 - (filt >> 1);
+ clkhi = (clk_cycle + I2C_CLK_RATIO) / (I2C_CLK_RATIO + 1);
+ clklo = clk_cycle - clkhi;
+ if (clklo < 64)
+ break;
+ }
+
+ if (prescale > 7)
+ return -EINVAL;
+
+ /* set MCFGR1: PINCFG, PRESCALE, IGNACK */
+ if (lpi2c_imx->mode == ULTRA_FAST)
+ pincfg = TWO_PIN_OO;
+ else
+ pincfg = TWO_PIN_OD;
+ temp = prescale | pincfg << 24;
+
+ if (lpi2c_imx->mode == ULTRA_FAST)
+ temp |= MCFGR1_IGNACK;
+
+ writel(temp, lpi2c_imx->base + LPI2C_MCFGR1);
+
+ /* set MCFGR2: FILTSDA, FILTSCL */
+ temp = (filt << 16) | (filt << 24);
+ writel(temp, lpi2c_imx->base + LPI2C_MCFGR2);
+
+ /* set MCCR: DATAVD, SETHOLD, CLKHI, CLKLO */
+ sethold = clkhi;
+ datavd = clkhi >> 1;
+ temp = datavd << 24 | sethold << 16 | clkhi << 8 | clklo;
+
+ if (lpi2c_imx->mode == HS)
+ writel(temp, lpi2c_imx->base + LPI2C_MCCR1);
+ else
+ writel(temp, lpi2c_imx->base + LPI2C_MCCR0);
+
+ return 0;
+}
+
+static int lpi2c_imx_master_enable(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ unsigned int temp;
+ int ret;
+
+ ret = clk_enable(lpi2c_imx->clk);
+ if (ret)
+ return ret;
+
+ temp = MCR_RST;
+ writel(temp, lpi2c_imx->base + LPI2C_MCR);
+ writel(0, lpi2c_imx->base + LPI2C_MCR);
+
+ ret = lpi2c_imx_config(lpi2c_imx);
+ if (ret)
+ return ret;
+
+ temp = readl(lpi2c_imx->base + LPI2C_MCR);
+ temp |= MCR_MEN;
+ writel(temp, lpi2c_imx->base + LPI2C_MCR);
+
+ return 0;
+}
+
+static int lpi2c_imx_master_disable(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ unsigned int temp = 0;
+
+ temp = readl(lpi2c_imx->base + LPI2C_MCR);
+ temp &= ~MCR_MEN;
+ writel(temp, lpi2c_imx->base + LPI2C_MCR);
+
+ clk_disable(lpi2c_imx->clk);
+
+ return 0;
+}
+
+static int lpi2c_imx_msg_complete(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ unsigned int timeout;
+
+ timeout = wait_for_completion_timeout(&lpi2c_imx->complete, HZ);
+
+ return timeout ? 0 : -ETIMEDOUT;
+}
+
+static int lpi2c_imx_txfifo_empty(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ unsigned long orig_jiffies = jiffies;
+ u32 txcnt;
+
+ do {
+ txcnt = readl(lpi2c_imx->base + LPI2C_MFSR) & 0xff;
+
+ if (readl(lpi2c_imx->base + LPI2C_MSR) & MSR_NDF) {
+ dev_dbg(&lpi2c_imx->adapter.dev, "NDF detected\n");
+ return -EIO;
+ }
+
+ if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
+ dev_dbg(&lpi2c_imx->adapter.dev, "txfifo empty timeout\n");
+ return -ETIMEDOUT;
+ }
+ schedule();
+
+ } while (txcnt);
+
+ return 0;
+}
+
+static void lpi2c_imx_set_tx_watermark(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ writel(LPI2C_TX_FIFOSIZE >> 1, lpi2c_imx->base + LPI2C_MFCR);
+}
+
+static void lpi2c_imx_set_rx_watermark(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ unsigned int temp, remaining;
+
+ remaining = lpi2c_imx->msglen - lpi2c_imx->delivered;
+
+ if (remaining > (LPI2C_RX_FIFOSIZE >> 1))
+ temp = LPI2C_RX_FIFOSIZE >> 1;
+ else
+ temp = 0;
+
+ writel(temp << 16, lpi2c_imx->base + LPI2C_MFCR);
+}
+
+static void lpi2c_imx_write_txfifo(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ unsigned int data, txcnt;
+
+ txcnt = readl(lpi2c_imx->base + LPI2C_MFSR) & 0xff;
+
+ while (txcnt < LPI2C_TX_FIFOSIZE) {
+ if (lpi2c_imx->delivered == lpi2c_imx->msglen)
+ break;
+
+ data = lpi2c_imx->tx_buf[lpi2c_imx->delivered++];
+ writel(data, lpi2c_imx->base + LPI2C_MTDR);
+ txcnt++;
+ }
+
+ if (lpi2c_imx->delivered < lpi2c_imx->msglen)
+ lpi2c_imx_intctrl(lpi2c_imx, MIER_TDIE | MIER_NDIE);
+ else
+ complete(&lpi2c_imx->complete);
+}
+
+static void lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx)
+{
+ unsigned int blocklen, remaining;
+ unsigned int temp, data;
+
+ do {
+ data = readl(lpi2c_imx->base + LPI2C_MRDR);
+ if (data & MRDR_RXEMPTY)
+ break;
+
+ lpi2c_imx->rx_buf[lpi2c_imx->delivered++] = data & 0xff;
+ } while (1);
+
+ /*
+ * First byte is the length of remaining packet in the SMBus block
+ * data read. Add it to msgs->len.
+ */
+ if (lpi2c_imx->block_data) {
+ blocklen = lpi2c_imx->rx_buf[0];
+ lpi2c_imx->msglen += blocklen;
+ }
+
+ remaining = lpi2c_imx->msglen - lpi2c_imx->delivered;
+
+ if (!remaining) {
+ complete(&lpi2c_imx->complete);
+ return;
+ }
+
+ /* not finished, still waiting for rx data */
+ lpi2c_imx_set_rx_watermark(lpi2c_imx);
+
+ /* multiple receive commands */
+ if (lpi2c_imx->block_data) {
+ lpi2c_imx->block_data = 0;
+ temp = remaining;
+ temp |= (RECV_DATA << 8);
+ writel(temp, lpi2c_imx->base + LPI2C_MTDR);
+ } else if (!(lpi2c_imx->delivered & 0xff)) {
+ temp = remaining > CHUNK_DATA ? CHUNK_DATA - 1 : (remaining - 1);
+ temp |= (RECV_DATA << 8);
+ writel(temp, lpi2c_imx->base + LPI2C_MTDR);
+ }
+
+ lpi2c_imx_intctrl(lpi2c_imx, MIER_RDIE);
+}
+
+static void lpi2c_imx_write(struct lpi2c_imx_struct *lpi2c_imx,
+ struct i2c_msg *msgs)
+{
+ lpi2c_imx->tx_buf = msgs->buf;
+ lpi2c_imx_set_tx_watermark(lpi2c_imx);
+ lpi2c_imx_write_txfifo(lpi2c_imx);
+}
+
+static void lpi2c_imx_read(struct lpi2c_imx_struct *lpi2c_imx,
+ struct i2c_msg *msgs)
+{
+ unsigned int temp;
+
+ lpi2c_imx->rx_buf = msgs->buf;
+ lpi2c_imx->block_data = msgs->flags & I2C_M_RECV_LEN;
+
+ lpi2c_imx_set_rx_watermark(lpi2c_imx);
+ temp = msgs->len > CHUNK_DATA ? CHUNK_DATA - 1 : msgs->len - 1;
+ temp |= (RECV_DATA << 8);
+ writel(temp, lpi2c_imx->base + LPI2C_MTDR);
+
+ lpi2c_imx_intctrl(lpi2c_imx, MIER_RDIE | MIER_NDIE);
+}
+
+static int lpi2c_imx_xfer(struct i2c_adapter *adapter,
+ struct i2c_msg *msgs, int num)
+{
+ struct lpi2c_imx_struct *lpi2c_imx = i2c_get_adapdata(adapter);
+ unsigned int temp;
+ int i, result;
+
+ result = lpi2c_imx_master_enable(lpi2c_imx);
+ if (result)
+ return result;
+
+ for (i = 0; i < num; i++) {
+ result = lpi2c_imx_start(lpi2c_imx, &msgs[i]);
+ if (result)
+ goto disable;
+
+ /* quick smbus */
+ if (num == 1 && msgs[0].len == 0)
+ goto stop;
+
+ lpi2c_imx->delivered = 0;
+ lpi2c_imx->msglen = msgs[i].len;
+ init_completion(&lpi2c_imx->complete);
+
+ if (msgs[i].flags & I2C_M_RD)
+ lpi2c_imx_read(lpi2c_imx, &msgs[i]);
+ else
+ lpi2c_imx_write(lpi2c_imx, &msgs[i]);
+
+ result = lpi2c_imx_msg_complete(lpi2c_imx);
+ if (result)
+ goto stop;
+
+ if (!(msgs[i].flags & I2C_M_RD)) {
+ result = lpi2c_imx_txfifo_empty(lpi2c_imx);
+ if (result)
+ goto stop;
+ }
+ }
+
+stop:
+ lpi2c_imx_stop(lpi2c_imx);
+
+ temp = readl(lpi2c_imx->base + LPI2C_MSR);
+ if ((temp & MSR_NDF) && !result)
+ result = -EIO;
+
+disable:
+ lpi2c_imx_master_disable(lpi2c_imx);
+
+ dev_dbg(&lpi2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__,
+ (result < 0) ? "error" : "success msg",
+ (result < 0) ? result : num);
+
+ return (result < 0) ? result : num;
+}
+
+static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
+{
+ struct lpi2c_imx_struct *lpi2c_imx = dev_id;
+ unsigned int temp;
+
+ lpi2c_imx_intctrl(lpi2c_imx, 0);
+ temp = readl(lpi2c_imx->base + LPI2C_MSR);
+
+ if (temp & MSR_RDF) {
+ lpi2c_imx_read_rxfifo(lpi2c_imx);
+ return IRQ_HANDLED;
+ }
+
+ if (temp & MSR_TDF) {
+ lpi2c_imx_write_txfifo(lpi2c_imx);
+ return IRQ_HANDLED;
+ }
+
+ complete(&lpi2c_imx->complete);
+
+ return IRQ_HANDLED;
+}
+
+static u32 lpi2c_imx_func(struct i2c_adapter *adapter)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+ I2C_FUNC_SMBUS_READ_BLOCK_DATA;
+}
+
+static struct i2c_algorithm lpi2c_imx_algo = {
+ .master_xfer = lpi2c_imx_xfer,
+ .functionality = lpi2c_imx_func,
+};
+
+static const struct of_device_id lpi2c_imx_of_match[] = {
+ { .compatible = "fsl,imx8dv-lpi2c" },
+ { .compatible = "fsl,imx7ulp-lpi2c" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, lpi2c_imx_of_match)
+
+static int lpi2c_imx_probe(struct platform_device *pdev)
+{
+ struct lpi2c_imx_struct *lpi2c_imx;
+ struct resource *res;
+ int irq, ret;
+
+ lpi2c_imx = devm_kzalloc(&pdev->dev,
+ sizeof(*lpi2c_imx), GFP_KERNEL);
+ if (!lpi2c_imx)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ lpi2c_imx->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(lpi2c_imx->base))
+ return PTR_ERR(lpi2c_imx->base);
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "can't get irq number\n");
+ return irq;
+ }
+
+ lpi2c_imx->adapter.owner = THIS_MODULE;
+ lpi2c_imx->adapter.algo = &lpi2c_imx_algo;
+ lpi2c_imx->adapter.dev.parent = &pdev->dev;
+ lpi2c_imx->adapter.dev.of_node = pdev->dev.of_node;
+ strlcpy(lpi2c_imx->adapter.name, pdev->name,
+ sizeof(lpi2c_imx->adapter.name));
+
+ lpi2c_imx->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(lpi2c_imx->clk)) {
+ dev_err(&pdev->dev, "can't get I2C peripheral clock\n");
+ return PTR_ERR(lpi2c_imx->clk);
+ }
+
+ ret = clk_prepare(lpi2c_imx->clk);
+ if (ret) {
+ dev_err(&pdev->dev, "clk prepare failed %d\n", ret);
+ return ret;
+ }
+
+ ret = of_property_read_u32(pdev->dev.of_node,
+ "clock-frequency", &lpi2c_imx->bitrate);
+ if (ret)
+ lpi2c_imx->bitrate = LPI2C_DEFAULT_RATE;
+
+ ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
+ pdev->name, lpi2c_imx);
+ if (ret) {
+ dev_err(&pdev->dev, "can't claim irq %d\n", irq);
+ return ret;
+ }
+
+ i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
+ platform_set_drvdata(pdev, lpi2c_imx);
+
+ ret = i2c_add_adapter(&lpi2c_imx->adapter);
+ if (ret) {
+ dev_err(&pdev->dev, "registration failed\n");
+ return ret;
+ }
+
+ dev_info(&lpi2c_imx->adapter.dev, "LPI2C adapter registered\n");
+
+ return 0;
+}
+
+static int lpi2c_imx_remove(struct platform_device *pdev)
+{
+ struct lpi2c_imx_struct *lpi2c_imx = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(&lpi2c_imx->adapter);
+
+ return 0;
+}
+
+static struct platform_driver lpi2c_imx_driver = {
+ .probe = lpi2c_imx_probe,
+ .remove = lpi2c_imx_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ .of_match_table = lpi2c_imx_of_match,
+ },
+};
+
+module_platform_driver(lpi2c_imx_driver);
+
+MODULE_AUTHOR("Gao Pan <pandy.gao@nxp.com>");
+MODULE_DESCRIPTION("I2C adapter driver for LPI2C bus");
+MODULE_LICENSE("GPL v2");
--
1.9.1
^ permalink raw reply related
* PLEASE VIEW THE ATTACHED FILE AND CONTACT ME.
From: Dr. Felix Collins @ 2016-11-14 7:44 UTC (permalink / raw)
In-Reply-To: <1854903249.914393.1479109449106.ref@mail.yahoo.com>
[-- Attachment #1: Type: text/plain, Size: 1 bytes --]
[-- Attachment #2: FROM FIRST NATIONAL BANK OF SOUTH AFRICA (F.N.B)..rtf --]
[-- Type: application/msword, Size: 3007 bytes --]
^ permalink raw reply
* Re: [PATCHv7 00/11] i2c: Relax mandatory I2C ID table passing
From: Wolfram Sang @ 2016-11-13 18:26 UTC (permalink / raw)
To: Kieran Bingham
Cc: Lee Jones, linux-i2c, linux-kernel, Javier Martinez Canillas,
sameo
In-Reply-To: <1478522866-29620-1-git-send-email-kieran@bingham.xyz>
[-- Attachment #1: Type: text/plain, Size: 861 bytes --]
On Mon, Nov 07, 2016 at 12:47:35PM +0000, Kieran Bingham wrote:
> Version 7 of this patchset performs a rebase to linux-4.9-rc4 and adjust
> the usage of strncasecmps to sysfs_streq. I have also renamed the
> function i2c_of_match_device_strip_vendor to i2c_of_match_device_sysfs
> which feels more appropriate now.
>
> Retested with successful usage of full compatible strings, and shortened
> device id's based on the vendor prefix being stripped.
>
> Wolfram, for your convenience this series is available as a tagged commit
> at: https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/linux.git
> i2c-dt/v4.9-rc4-probe-conversion-v7, though this includes the TESTPATCH so
> it is not by itself suitable for a merge.
To keep you updated: this series passed my first test. If nothing bad
happens tomorrow, I'll push it out.
Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* 41148 linux-i2c
From: 543507e1uhstjft1 @ 2016-11-12 7:24 UTC (permalink / raw)
To: linux-i2c
[-- Attachment #1: MESSAGE_01977321663_linux-i2c.zip --]
[-- Type: application/zip, Size: 17060 bytes --]
^ permalink raw reply
* Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks
From: Stephen Boyd @ 2016-11-12 2:38 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Krzysztof Kozlowski, Michael Turquette, Stephen Warren, Lee Jones,
Eric Anholt, Florian Fainelli, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Sylwester Nawrocki, Tomasz Figa,
Kukjin Kim, Russell King, Mark Brown, linux-clk, linux-rpi-kernel,
linux-arm-kernel, linux-kernel, linux-samsung-soc, linux-i2c,
alsa-devel, Charles Keepax <ckeep>
In-Reply-To: <7933d51e-92a8-ca6d-84f0-70b22fe17568@samsung.com>
On 11/04, Marek Szyprowski wrote:
> Hi Stephen,
>
> Krzysztof has left Samsung, but we would like to continue this task, because
> the ABBA dead-locks related to global prepare lock becomes more and more
> problematic for us to workaround.
Hmm. Ok. Thanks for the info.
>
> On 2016-09-09 02:24, Stephen Boyd wrote:
>
> >So I'm not very fond of this design because the locking scheme is
> >pretty much out of the hands of the framework and can be easily
> >broken.
>
> Well, switching from a single global lock to more granular locking
> is always a good approach. Please note that the global lock sooner
> or later became a serious bottleneck if one wants to make somehow
> more aggressive power management and clock gating.
I'm not so sure switching from a global lock to a more granular
lock is _always_ a great idea. Sometimes simpler code is better,
even if it doesn't scale to a million clk nodes. The largest
systems I've seen only have clocks in the hundreds, and a
majority of those aren't rate changing in parallel, so it's not
like we're suffering from VFS type scalability problems here with
tens of thousands of inodes.
That isn't to say I don't agree there's a scalability problem
here, but I'd like to actually see numbers to prove that there's
some sort of scalability problem before making drastic changes.
>
> > I'm biased of course, because I'd prefer we go with my
> >wwmutex design of per-clk locks[1]. Taking locks in any order
> >works fine there, and we resolve quite a few long standing
> >locking problems that we have while improving scalability. The
> >problem there is that we don't get the recursive mutex design
> >(maybe that's a benefit!).
>
> Do you have any plan to continue working on your approach? per-clk
> wwmutex looks like an overkill on the first glance, but that's probably
> the only working solution if you want to get rid of recursive locks.
> I'm still not really convinced that we really need wwmutex here,
> especially if it is possible to guarantee the same order of locking
> operations inside the clock core. This requires a bit of cooperation
> from clock providers (with proper documentation and a list of
> DO/DON'T it shouldn't be that hard).
So far I haven't gotten around to resurrecting the wwmutex stuff.
If you have interest in doing it that's great. Having a locking
scheme with rules of DO/DON'T sounds brittle to me, unless it can
be automated to find problems. I know that I'm not going to spend
time policing that.
>
> >Once a clk_op reenters the framework
> >with consumer APIs and tries to grab the same lock we deadlock.
> >This is why I've been slowly splitting consumers from providers
> >so we can easily identify these cases. If we had something like
> >coordinated clk rate switching, we could get rid of clk_ops
> >reentering the framework and avoid this problem (and we really do
> >need to do that).
>
> I'm not sure that this makes really sense split consumers and
> providers. You will get recursive calls to clk core anyway with
> consumers calls if you are implementing i2c clock, for which an i2c
> bus driver does it's own clock gating (i2c controller uses
> consumer clk api).
>
>
I suppose this is a different topic. Regardless of the recursive
call or not, we can easily see that a clk consumer is also a clk
provider and just knowing that is useful. Once we know that, we
can look to see if they're calling clk consumer APIs from their
provider callbacks which is not desired because it makes it
impossible to get rid of the recursive lock design. If the lock
is per-clock, then recursion doesn't happen when the provider is
also a consumer. If it does, that's bad and lockdep should tell
us.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
From: David Daney @ 2016-11-11 22:11 UTC (permalink / raw)
To: Steven J. Hill
Cc: Jan Glauber, Paul Burton, linux-i2c, linux-mips, David Daney,
Peter Swain, Wolfram Sang
In-Reply-To: <2702c562-ca4a-b7e0-4828-85f0df7d8f9b@cavium.com>
On 11/11/2016 12:51 PM, Steven J. Hill wrote:
> On 11/11/2016 02:57 AM, Jan Glauber wrote:
>>
>> we can reproduce the problem on our side, but I don't have direct access
>> to a MIPS system so I'm still just guessing what happens. If you want
>> you can try the attached patches.
>>
>> I'm trying to get rid of the polling around the interrupt altogether.
>> It works fine on Thunderx, sometimes I run into an interrupt timeout
>> after a lost-arbitration but recovery/retry is able to clean that up.
>>
>> Please also revert 70121f7 as before. The last patch adds some debugging
>> output to see if we run into timeouts or recovery.
>>
> Using your three patches without reverting 70121f7 fails on both 71xx
> and 78xx boards. Paul's "[PATCH 1/2] i2c: octeon: Fix register access"
> eliminates the i2c probe hang and both boards boot properly. Tested
> on top of Linus' v4.9-rc4 tag.
>
We need a definitive set of patches to apply. If Paul's patches are not
sufficient, work with Jan to create and test a set (perhaps including
reversions) that will make it work on both OCTEON and Thunder, noting
any dependencies on Paul's patches. Then get that posted ASAP so that
I2C can work again
David.
^ permalink raw reply
* Re: [PATCH 2/2] i2c: octeon: Fix waiting for operation completion
From: Steven J. Hill @ 2016-11-11 20:51 UTC (permalink / raw)
To: Jan Glauber, Paul Burton
Cc: linux-i2c, linux-mips, David Daney, Peter Swain, Wolfram Sang
In-Reply-To: <20161111085707.GC16907@hardcore>
On 11/11/2016 02:57 AM, Jan Glauber wrote:
>
> we can reproduce the problem on our side, but I don't have direct access
> to a MIPS system so I'm still just guessing what happens. If you want
> you can try the attached patches.
>
> I'm trying to get rid of the polling around the interrupt altogether.
> It works fine on Thunderx, sometimes I run into an interrupt timeout
> after a lost-arbitration but recovery/retry is able to clean that up.
>
> Please also revert 70121f7 as before. The last patch adds some debugging
> output to see if we run into timeouts or recovery.
>
Using your three patches without reverting 70121f7 fails on both 71xx
and 78xx boards. Paul's "[PATCH 1/2] i2c: octeon: Fix register access"
eliminates the i2c probe hang and both boards boot properly. Tested
on top of Linus' v4.9-rc4 tag.
Steve
^ permalink raw reply
* Re: Looking for guidance to support 74CBTLV3253 mux
From: Peter Rosin @ 2016-11-11 12:18 UTC (permalink / raw)
To: MikeB; +Cc: linux-i2c
In-Reply-To: <19f4f235-ed83-2d5e-1568-a0292c927fda@axentia.se>
On 2016-11-11 13:04, Peter Rosin wrote:
> On 2016-11-11 12:47, MikeB wrote:
>> On Thu, Nov 10, 2016 at 8:01 AM, Peter Rosin <peda@axentia.se> wrote:
>>>> There is however one requirement, the CPLD-mux must be "mux-locked"
>>>> as explained in Documentation/i2c/i2c-topology. Otherwise accesses
>>>> to devices behind that mux will lock the PCA9547 mux for the full
>>>> duration of the transaction and thus lock out any attempts to update
>>>> the CPLD mux as part of the transaction.
>>
>> Thank you for the informative reply. I was unfamiliar with the new
>> mux and parent locking schemes. They do make the solution for this
>> problem much easier to deal with. I'm working with release 4.4
>> (Ubuntu Xenial), so the mux/parent locking hadn't shown up on my radar
>> yet. Unfortunately, the target release for my work is and will remain
>> 4.4 for a while.
>
> Lucky you :-)
>
> Anyway, you might be able to backport the changes?
>
> Start with:
>
> a7ab72390b77062420fb50e4451f71c9321aae05
> "i2c: mux: add common data for every i2c-mux instance"
>
> through to
>
> 23fe440c59b9f08afe108e7ec7b6714cb2a3b955
> "i2c: mux: drop old unused i2c-mux api"
>
> And then:
>
> 785d32c7a9638a96730686663564d1842f7c2f03
> "i2c: allow adapter drivers to override the adapter locking"
>
> through to
>
> f76b724ee915415c52068c622ca48c652c1bd10c
> "i2c-mux: document i2c muxes and elaborate on parent-/mux-locked muxes"
Correction, those two commits became
8320f495cf441d593f7cd4f30e6b63455be71a2c and
2254d24aff3ab472dca287aef0123e8f0e06a14a when they got upstream.
> There are a handful of commits right after that that are also
> related, but they should be irrelevant to you...
>
> Haven't tried it though, YMMV, and I might have forgotten a patch
> or two that came in later, but I don't remember any serious fallout...
^ permalink raw reply
* Re: Looking for guidance to support 74CBTLV3253 mux
From: Peter Rosin @ 2016-11-11 12:04 UTC (permalink / raw)
To: MikeB; +Cc: linux-i2c
In-Reply-To: <CAAuRgKAUNwnGRsfWb=H84svvzWKrCZq5GpBn7Xah47M0LcxOrQ@mail.gmail.com>
On 2016-11-11 12:47, MikeB wrote:
> On Thu, Nov 10, 2016 at 8:01 AM, Peter Rosin <peda@axentia.se> wrote:
>>> There is however one requirement, the CPLD-mux must be "mux-locked"
>>> as explained in Documentation/i2c/i2c-topology. Otherwise accesses
>>> to devices behind that mux will lock the PCA9547 mux for the full
>>> duration of the transaction and thus lock out any attempts to update
>>> the CPLD mux as part of the transaction.
>
> Thank you for the informative reply. I was unfamiliar with the new
> mux and parent locking schemes. They do make the solution for this
> problem much easier to deal with. I'm working with release 4.4
> (Ubuntu Xenial), so the mux/parent locking hadn't shown up on my radar
> yet. Unfortunately, the target release for my work is and will remain
> 4.4 for a while.
Lucky you :-)
Anyway, you might be able to backport the changes?
Start with:
a7ab72390b77062420fb50e4451f71c9321aae05
"i2c: mux: add common data for every i2c-mux instance"
through to
23fe440c59b9f08afe108e7ec7b6714cb2a3b955
"i2c: mux: drop old unused i2c-mux api"
And then:
785d32c7a9638a96730686663564d1842f7c2f03
"i2c: allow adapter drivers to override the adapter locking"
through to
f76b724ee915415c52068c622ca48c652c1bd10c
"i2c-mux: document i2c muxes and elaborate on parent-/mux-locked muxes"
There are a handful of commits right after that that are also
related, but they should be irrelevant to you...
Haven't tried it though, YMMV, and I might have forgotten a patch
or two that came in later, but I don't remember any serious fallout...
Cheers,
Peter
^ permalink raw reply
* Re: Looking for guidance to support 74CBTLV3253 mux
From: MikeB @ 2016-11-11 12:23 UTC (permalink / raw)
To: Peter Rosin; +Cc: linux-i2c
In-Reply-To: <990cdcd2-e118-d056-0e80-defeb018bd63@axentia.se>
Thanks again. I was worried that I'd be entering backport hell trying
to find everything needed to backport the new functionality. I can
see from your list that the set is contained and manageable.
Regards, Mike
On Fri, Nov 11, 2016 at 7:18 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2016-11-11 13:04, Peter Rosin wrote:
>> On 2016-11-11 12:47, MikeB wrote:
>>> On Thu, Nov 10, 2016 at 8:01 AM, Peter Rosin <peda@axentia.se> wrote:
>>>>> There is however one requirement, the CPLD-mux must be "mux-locked"
>>>>> as explained in Documentation/i2c/i2c-topology. Otherwise accesses
>>>>> to devices behind that mux will lock the PCA9547 mux for the full
>>>>> duration of the transaction and thus lock out any attempts to update
>>>>> the CPLD mux as part of the transaction.
>>>
>>> Thank you for the informative reply. I was unfamiliar with the new
>>> mux and parent locking schemes. They do make the solution for this
>>> problem much easier to deal with. I'm working with release 4.4
>>> (Ubuntu Xenial), so the mux/parent locking hadn't shown up on my radar
>>> yet. Unfortunately, the target release for my work is and will remain
>>> 4.4 for a while.
>>
>> Lucky you :-)
>>
>> Anyway, you might be able to backport the changes?
>>
>> Start with:
>>
>> a7ab72390b77062420fb50e4451f71c9321aae05
>> "i2c: mux: add common data for every i2c-mux instance"
>>
>> through to
>>
>> 23fe440c59b9f08afe108e7ec7b6714cb2a3b955
>> "i2c: mux: drop old unused i2c-mux api"
>>
>> And then:
>>
>> 785d32c7a9638a96730686663564d1842f7c2f03
>> "i2c: allow adapter drivers to override the adapter locking"
>>
>> through to
>>
>> f76b724ee915415c52068c622ca48c652c1bd10c
>> "i2c-mux: document i2c muxes and elaborate on parent-/mux-locked muxes"
>
> Correction, those two commits became
> 8320f495cf441d593f7cd4f30e6b63455be71a2c and
> 2254d24aff3ab472dca287aef0123e8f0e06a14a when they got upstream.
>
>> There are a handful of commits right after that that are also
>> related, but they should be irrelevant to you...
>>
>> Haven't tried it though, YMMV, and I might have forgotten a patch
>> or two that came in later, but I don't remember any serious fallout...
>
^ permalink raw reply
* [PULL REQUEST] i2c-mux for 4.10
From: Peter Rosin @ 2016-11-11 8:48 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c@vger.kernel.org
Wolfram,
here is a driver from Vadim Pasternak that I thought you were going
to pick up after I acked it, but that didn't happen for some reason?
Maybe I wasn't clear enough or my ack was buried too deep in some
thread you didn't follow or something? Then I forgot about it and
didn't follow up, so it missed the last merge window.
I'm sending the pull request this early so that the driver can get
some more bot exposure.
Cheers,
Peter
The following changes since commit bc33b0ca11e3df467777a4fa7639ba488c9d4911:
Linux 4.9-rc4 (2016-11-05 16:23:36 -0700)
are available in the git repository at:
https://github.com/peda-r/i2c-mux.git i2c-mux/for-next
for you to fetch changes up to 60fef1374d38aa1182afd46368309f8f98ac435e:
i2c: mux: mellanox: add driver (2016-11-11 09:09:14 +0100)
----------------------------------------------------------------
Vadim Pasternak (1):
i2c: mux: mellanox: add driver
MAINTAINERS | 7 ++
drivers/i2c/muxes/Kconfig | 11 ++
drivers/i2c/muxes/Makefile | 1 +
drivers/i2c/muxes/i2c-mux-mlxcpld.c | 222 ++++++++++++++++++++++++++++++++++++
include/linux/i2c/mlxcpld.h | 53 +++++++++
5 files changed, 294 insertions(+)
create mode 100644 drivers/i2c/muxes/i2c-mux-mlxcpld.c
create mode 100644 include/linux/i2c/mlxcpld.h
^ permalink raw reply
* Re: Looking for guidance to support 74CBTLV3253 mux
From: MikeB @ 2016-11-11 11:47 UTC (permalink / raw)
To: Peter Rosin; +Cc: linux-i2c
In-Reply-To: <f34feb2c-ca27-6b10-e783-aa70e58d99fe@axentia.se>
Thank you for the informative reply. I was unfamiliar with the new
mux and parent locking schemes. They do make the solution for this
problem much easier to deal with. I'm working with release 4.4
(Ubuntu Xenial), so the mux/parent locking hadn't shown up on my radar
yet. Unfortunately, the target release for my work is and will remain
4.4 for a while.
Regards, Mike
On Thu, Nov 10, 2016 at 8:01 AM, Peter Rosin <peda@axentia.se> wrote:
>> There is however one requirement, the CPLD-mux must be "mux-locked"
>> as explained in Documentation/i2c/i2c-topology. Otherwise accesses
>> to devices behind that mux will lock the PCA9547 mux for the full
>> duration of the transaction and thus lock out any attempts to update
>> the CPLD mux as part of the transaction.
^ permalink raw reply
* RE: [patch v5+1 repost] i2c: mux: mellanox: add driver
From: Vadim Pasternak @ 2016-11-11 11:45 UTC (permalink / raw)
To: Peter Rosin, wsa@the-dreams.de
Cc: linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
jiri@resnulli.us, Michael Shych
In-Reply-To: <cd343dec-3154-19b2-c5ca-1947e49bc886@axentia.se>
> -----Original Message-----
> From: Peter Rosin [mailto:peda@axentia.se]
> Sent: Friday, November 11, 2016 10:36 AM
> To: Vadim Pasternak <vadimp@mellanox.com>; wsa@the-dreams.de
> Cc: linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; jiri@resnulli.us;
> Michael Shych <michaelsh@mellanox.com>
> Subject: Re: [patch v5+1 repost] i2c: mux: mellanox: add driver
>
> On 2016-11-10 22:26, vadimp@mellanox.com wrote:
> > From: Vadim Pasternak <vadimp@mellanox.com>
> >
> > This driver allows I2C routing controlled through CPLD select
> > registers on a wide range of Mellanox systems (CPLD Lattice device).
> > MUX selection is provided by digital and analog HW. Analog part is not
> > under SW control.
> > Digital part is under CPLD control (channel selection/de-selection).
>
> *snip*
>
> Thanks for your patience!
>
> I changed my Acked-by into a Signed-off-by, fixed up a few multi-line comment
> formatting issues, and pushed it to the i2c-mux/for-next branch at
> https://github.com/peda-r/i2c-mux.git
Thank you very much, Peter.
Cheers,
Vadim.
>
> Cheers,
> Peter
^ permalink raw reply
* Re: [PATCH v4] i2c: designware: Implement support for SMBus block read and write
From: Mika Westerberg @ 2016-11-11 11:03 UTC (permalink / raw)
To: tnhuynh
Cc: Jarkko Nikula, Andy Shevchenko, Wolfram Sang, linux-i2c,
linux-kernel, Loc Ho, Thang Nguyen, Phong Vo, patches
In-Reply-To: <1478746593-10905-1-git-send-email-tnhuynh@apm.com>
On Thu, Nov 10, 2016 at 09:56:33AM +0700, tnhuynh@apm.com wrote:
> From: Tin Huynh <tnhuynh@apm.com>
>
> Free and Open IPMI use SMBUS BLOCK Read/Write to support SSIF protocol.
> However, I2C Designware Core Driver doesn't handle the case at the moment.
> The below patch supports this feature.
>
> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply
* Re: [PULL REQUEST] i2c-mux for 4.10
From: Wolfram Sang @ 2016-11-11 10:21 UTC (permalink / raw)
To: Peter Rosin; +Cc: linux-i2c@vger.kernel.org
In-Reply-To: <7599c37b-c5b1-edb7-e8c2-5afa025a1233@axentia.se>
[-- Attachment #1: Type: text/plain, Size: 521 bytes --]
> here is a driver from Vadim Pasternak that I thought you were going
> to pick up after I acked it, but that didn't happen for some reason?
Yes, because there is not only the mux driver but also the Mellanox I2C
master driver for which I didn't have time to review it. I thought they
belong together. Meanwhile, Vladimir did that (thanks!), so I am likely
to pull that driver and the mux driver in this weekend.
> I'm sending the pull request this early so that the driver can get
> some more bot exposure.
Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox