[RFC v2 2/4] dt-bindings: i2c: mux: demux-pinctrl: add bindings
From: Wolfram Sang
These bindings allow an I2C bus to switch between multiple masters. This
is not hot-swichting because connected I2C slaves will be
re-instantiated. It is meant to select the best I2C core at runtime once
the task is known. Example: Prefer i2c-gpio over another I2C core
because of HW errata affetcing your use case.
Signed-off-by: Wolfram Sang
---
.../devicetree/bindings/i2c/i2c-demux-pinctrl.txt | 155 +
1 file changed, 155 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
diff --git a/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
b/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
new file mode 100644
index 00..de571be68f4754
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
@@ -0,0 +1,155 @@
+Pinctrl-based I2C Bus DeMux
+
+This binding describes an I2C bus demultiplexer that uses pin multiplexing to
+route the I2C signals, and represents the pin multiplexing configuration using
+the pinctrl device tree bindings. This may be used to select one I2C IP core at
+runtime which may have a better feature set for a given task than another I2C
+IP core on the SoC. The most simple example is to fall back to GPIO bitbanging
+if your current runtime configuration hits an errata of the internal IP core.
+
++---+
+| SoC |
+| | +-+ +-+
+| ++ | | dev | | dev |
+| |I2C IP Core1|--\ | +-+ +-+
+| ++ \---+ | ||
+||Pinctrl|--|--++
+| ++ +---+ |
+| |I2C IP Core2|--/ |
+| ++ |
+| |
++---+
+
+Required properties:
+- compatible: "i2c-demux-pinctrl"
+- i2c-parent: List of phandles of I2C masters available for selection. The
first
+ one will be used as default.
+- i2c-bus-name: The name of this bus. Also needed as pinctrl-name for the I2C
+ parents.
+
+Furthermore, I2C mux properties and child nodes. See mux.txt in this directory.
+
+Example:
+
+Here is a snipplet for a bus to be demuxed. It contains various i2c clients for
+HDMI, so the bus is named "i2c-hdmi":
+
+ i2chdmi: i2c@8 {
+
+ compatible = "i2c-demux-pinctrl";
+ i2c-parent = <&gpioi2c>, <&iic2>, <&i2c2>;
+ i2c-bus-name = "i2c-hdmi";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ak4643: sound-codec@12 {
+ compatible = "asahi-kasei,ak4643";
+
+ #sound-dai-cells = <0>;
+ reg = <0x12>;
+ };
+
+ composite-in@20 {
+ compatible = "adi,adv7180";
+ reg = <0x20>;
+ remote = <&vin1>;
+
+ port {
+ adv7180: endpoint {
+ bus-width = <8>;
+ remote-endpoint = <&vin1ep0>;
+ };
+ };
+ };
+
+ hdmi@39 {
+ compatible = "adi,adv7511w";
+ reg = <0x39>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <15 IRQ_TYPE_LEVEL_LOW>;
+
+ adi,input-depth = <8>;
+ adi,input-colorspace = "rgb";
+ adi,input-clock = "1x";
+ adi,input-style = <1>;
+ adi,input-justification = "evenly";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ adv7511_in: endpoint {
+ remote-endpoint =
<&du_out_lvds0>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ adv7511_out: endpoint {
+ remote-endpoint = <&hdmi_con>;
+ };
+ };
+ };
+ };
+ };
+
+And for clarification, here are the sn
[RFC v2 0/4] i2c/of: switch I2C IP cores at runtime via OF_DYNAMIC
I know this is gonna be a controversial series, but we have a usecase for this :) This series allows an I2C bus to switch between multiple masters, i.e. a n-to-1-demuxer. This is not hot-switching because connected I2C slaves will be re-instantiated. It is meant to select the best I2C core at runtime once the task is known. Example: Prefer i2c-gpio over another I2C core because of HW errata affecting your current runtime configuration. It works by using OF_DYNAMIC and en-/disabling the i2c parent as needed. See the binding docs for more details. Because this is largely using OF_DYNAMIC, I decided to post the whole series to the dt list. Changes since RFC v1: * gracefully handle if i2c adapters are not present at runtime (driver not loaded yet) * added more documentation and examples * properly put the i2c adapters after use * respect PAGE_SIZE in sysfs_show This has been tested on a Renesas Lager board switching between i2c-gpio and two different IP cores (i2c-rcar and i2c-sh_mobile). The rebinding seems to be working as expected. However, in practice, I couldn't use the HDMI i2c slaves with another controller yet, because the rcar-du driver OOPSes when unbinding. This seems unrelated to this series because it can also be triggered via sysfs-unbind. soc-camera also had problems properly cleaning up on unbind, a patch for this already has been sent. So, this series is for sure a good test for the unbind path of drivers. Let me know what you think. Thanks, Wolfram Wolfram Sang (4): of: make of_mutex public dt-bindings: i2c: mux: demux-pinctrl: add bindings i2c: mux: demux-pinctrl: add driver ARM: shmobile: r8a7790: rework dts to use i2c demuxer .../devicetree/bindings/i2c/i2c-demux-pinctrl.txt | 155 arch/arm/boot/dts/r8a7790-lager.dts| 141 +++ drivers/i2c/muxes/Kconfig | 9 + drivers/i2c/muxes/Makefile | 2 + drivers/i2c/muxes/i2c-demux-pinctrl.c | 276 + drivers/of/of_private.h| 1 - include/linux/of.h | 2 + 7 files changed, 532 insertions(+), 54 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt create mode 100644 drivers/i2c/muxes/i2c-demux-pinctrl.c -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2 4/4] ARM: shmobile: r8a7790: rework dts to use i2c demuxer
From: Wolfram Sang
Create a seperate bus for HDMI related I2C slaves and assign it
to a i2c-gpio master. It can be switched to the i2c-rcar or
i2c-sh_mobile core at runtime.
Signed-off-by: Wolfram Sang
---
arch/arm/boot/dts/r8a7790-lager.dts | 141 ++--
1 file changed, 88 insertions(+), 53 deletions(-)
diff --git a/arch/arm/boot/dts/r8a7790-lager.dts
b/arch/arm/boot/dts/r8a7790-lager.dts
index c553abd711eeb3..d8f0ca8e094dad 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -49,6 +49,8 @@
aliases {
serial0 = &scifa0;
serial1 = &scifa1;
+ i2c8 = &i2chdmi;
+ i2c9 = &gpioi2c;
};
chosen {
@@ -252,6 +254,79 @@
#clock-cells = <0>;
clock-frequency = <14850>;
};
+
+
+ gpioi2c: i2c@9 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "i2c-gpio";
+ status = "disabled";
+ gpios = <&gpio5 6 GPIO_ACTIVE_HIGH /* sda */
+&gpio5 5 GPIO_ACTIVE_HIGH /* scl */
+ >;
+ i2c-gpio,delay-us = <5>;
+ };
+
+ i2chdmi: i2c@8 {
+
+ compatible = "i2c-demux-pinctrl";
+ i2c-parent = <&gpioi2c>, <&iic2>, <&i2c2>;
+ i2c-bus-name = "i2c-hdmi";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ak4643: sound-codec@12 {
+ compatible = "asahi-kasei,ak4643";
+
+ #sound-dai-cells = <0>;
+ reg = <0x12>;
+ };
+
+ composite-in@20 {
+ compatible = "adi,adv7180";
+ reg = <0x20>;
+ remote = <&vin1>;
+
+ port {
+ adv7180: endpoint {
+ bus-width = <8>;
+ remote-endpoint = <&vin1ep0>;
+ };
+ };
+ };
+
+ hdmi@39 {
+ compatible = "adi,adv7511w";
+ reg = <0x39>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <15 IRQ_TYPE_LEVEL_LOW>;
+
+ adi,input-depth = <8>;
+ adi,input-colorspace = "rgb";
+ adi,input-clock = "1x";
+ adi,input-style = <1>;
+ adi,input-justification = "evenly";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ adv7511_in: endpoint {
+ remote-endpoint =
<&du_out_lvds0>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ adv7511_out: endpoint {
+ remote-endpoint = <&hdmi_con>;
+ };
+ };
+ };
+ };
+ };
};
&du {
@@ -352,6 +427,11 @@
renesas,function = "iic1";
};
+ i2c2_pins: i2c2 {
+ renesas,groups = "i2c2";
+ renesas,function = "i2c2";
+ };
+
iic2_pins: iic2 {
renesas,groups = "iic2";
renesas,function = "iic2";
@@ -532,63 +612,18 @@
pinctrl-names = "default";
};
-&iic2 {
- status = "okay";
- pinctrl-0 = <&iic2_pins>;
- pinctrl-names = "default";
+&i2c2 {
+ pinctrl-0 = <&i2c2_pins>;
+ pinctrl-names = "i2c-hdmi";
clock-frequency = <10>;
+};
- ak4643: codec@12 {
- compatible = "asahi-kasei,ak4643";
- #sound-dai-cells = <0>;
- reg = <0x12>;
- };
-
- composite-in@20 {
- compatible = "adi,adv7180";
- reg = <0x20>;
- remote = <&vin1>;
-
- port {
- adv7180: endpoint {
-
[RFC v2 1/4] of: make of_mutex public
From: Wolfram Sang
If we want to use OF_DYNAMIC features outside the of framework, we need
to access this lock.
Signed-off-by: Wolfram Sang
---
drivers/of/of_private.h | 1 -
include/linux/of.h | 2 ++
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 8e882e706cd8c6..f92ec41efb5dfd 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -31,7 +31,6 @@ struct alias_prop {
char stem[0];
};
-extern struct mutex of_mutex;
extern struct list_head aliases_lookup;
extern struct kset *of_kset;
diff --git a/include/linux/of.h b/include/linux/of.h
index dd10626a615fb1..acac979063f067 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -32,6 +32,8 @@
typedef u32 phandle;
typedef u32 ihandle;
+extern struct mutex of_mutex;
+
struct property {
char*name;
int length;
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2 3/4] i2c: mux: demux-pinctrl: add driver
From: Wolfram Sang
This driver allows an I2C bus to switch between multiple masters. This
is not hot-swichting because connected I2C slaves will be
re-instantiated. It is meant to select the best I2C core at runtime once
the task is known. Example: Prefer i2c-gpio over another I2C core
because of HW errata affetcing your use case.
Signed-off-by: Wolfram Sang
---
drivers/i2c/muxes/Kconfig | 9 ++
drivers/i2c/muxes/Makefile| 2 +
drivers/i2c/muxes/i2c-demux-pinctrl.c | 276 ++
3 files changed, 287 insertions(+)
create mode 100644 drivers/i2c/muxes/i2c-demux-pinctrl.c
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index f06b0e24673b87..7b5da5ff7b16f9 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -72,4 +72,13 @@ config I2C_MUX_REG
This driver can also be built as a module. If so, the module
will be called i2c-mux-reg.
+config I2C_DEMUX_PINCTRL
+ tristate "pinctrl-based I2C demultiplexer"
+ depends on PINCTRL
+ select OF_DYNAMIC
+ help
+ If you say yes to this option, support will be included for an I2C
+ demultiplexer that uses the pinctrl subsystem. This is useful if you
+ want to change the I2C master at run-time depending on features.
+
endmenu
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index e89799b76a9280..7c267c29b19196 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -3,6 +3,8 @@
obj-$(CONFIG_I2C_ARB_GPIO_CHALLENGE) += i2c-arb-gpio-challenge.o
+obj-$(CONFIG_I2C_DEMUX_PINCTRL)+= i2c-demux-pinctrl.o
+
obj-$(CONFIG_I2C_MUX_GPIO) += i2c-mux-gpio.o
obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o
obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o
diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c
b/drivers/i2c/muxes/i2c-demux-pinctrl.c
new file mode 100644
index 00..5ec2779b176432
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
@@ -0,0 +1,276 @@
+/*
+ * Pinctrl based I2C DeMultiplexer
+ *
+ * Copyright (C) 2015-16 by Wolfram Sang, Sang Engineering
+ * Copyright (C) 2015-16 by Renesas Electronics Corporation
+ *
+ * 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; version 2 of the License.
+ *
+ * See the bindings doc for DTS setup.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct i2c_demux_pinctrl_chan {
+ struct device_node *parent_np;
+ struct i2c_adapter *parent_adap;
+ struct of_changeset chgset;
+};
+
+struct i2c_demux_pinctrl_priv {
+ int cur_chan;
+ int num_chan;
+ struct device *dev;
+ const char *bus_name;
+ struct i2c_adapter cur_adap;
+ struct i2c_algorithm algo;
+ struct i2c_demux_pinctrl_chan chan[];
+};
+
+static struct property status_okay = { .name = "status", .length = 3, .value =
"ok" };
+
+static int i2c_demux_master_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
+{
+ struct i2c_demux_pinctrl_priv *priv = adap->algo_data;
+ struct i2c_adapter *parent = priv->chan[priv->cur_chan].parent_adap;
+
+ return __i2c_transfer(parent, msgs, num);
+}
+
+static u32 i2c_demux_functionality(struct i2c_adapter *adap)
+{
+ struct i2c_demux_pinctrl_priv *priv = adap->algo_data;
+ struct i2c_adapter *parent = priv->chan[priv->cur_chan].parent_adap;
+
+ return parent->algo->functionality(parent);
+}
+
+static int i2c_demux_activate_master(struct i2c_demux_pinctrl_priv *priv, u32
new_chan)
+{
+ struct i2c_adapter *adap;
+ struct pinctrl *p;
+ int ret;
+
+ mutex_lock(&of_mutex);
+ ret = of_changeset_apply(&priv->chan[new_chan].chgset);
+ mutex_unlock(&of_mutex);
+ if (ret)
+ goto err;
+
+ adap = of_find_i2c_adapter_by_node(priv->chan[new_chan].parent_np);
+ if (!adap) {
+ ret = -ENODEV;
+ goto err;
+ }
+
+ p = devm_pinctrl_get_select(adap->dev.parent, priv->bus_name);
+ if (IS_ERR(p)) {
+ ret = PTR_ERR(p);
+ goto err_with_put;
+ }
+
+ priv->chan[new_chan].parent_adap = adap;
+ priv->cur_chan = new_chan;
+
+ /* Now fill out current adapter structure. cur_chan must be up to date
*/
+ priv->algo.master_xfer = i2c_demux_master_xfer;
+ priv->algo.functionality = i2c_demux_functionality;
+
+ snprintf(priv->cur_adap.name, sizeof(priv->cur_adap.name),
+"i2c-demux (master i2c-%d)", i2c_adapter_id(adap));
+ priv->cur_adap.owner = THIS_MODULE;
+ priv->cur_adap.algo = &priv->algo;
+ pri
Re: [PATCH] DT: i2c: Update vendor prefix for 24c00
On Mon, Jan 04, 2016 at 08:22:33PM +0200, Andy Shevchenko wrote: > On Sat, Jan 2, 2016 at 11:21 PM, Wolfram Sang wrote: > > On Sun, Dec 27, 2015 at 04:57:48PM +0200, Andy Shevchenko wrote: > >> On Wed, Dec 23, 2015 at 9:18 PM, Akshay Bhat > >> wrote: > >> > "at" is not a valid vendor prefix, correcting the same to "atmel" > >> > > >> > >> I'm afraid you can't just do this change alone as it's used in some > >> DTS. Though you may deprecated it along with update of current users. > > > > Well, in Linux, I2C core currently strips the vendor anyhow. This will > > probably be changed somewhen (tm), but for now, the impact for Linux > > should be extremly close to 0. > > Okay, no objections to the original patch then. Heh, I just go reminded that eeproms already have a seperate binding description (Documentation/devicetree/bindings/eeprom/eeprom.txt). I will update the eeprom bindings and remove the entries from trivial devices. signature.asc Description: Digital signature
Re: [PATCH v2 0/8] i2c mux cleanup and locking update
Peter, > PS. needs a bunch of testing, I do not have access to all the involved hw First of all, thanks for diving into this topic and the huge effort you apparently have put into it. It is obviously a quite intrusive series, so it needs careful review. TBH, I can't really tell when I have the bandwidth to do that, so I hope other people will step up. And yes, it needs serious testing. To all: Although I appreciate any review support, I'd think the first thing to be done should be a very high level review - is this series worth the huge update? Is the path chosen proper? Stuff like this. I'd appreciate Acks or Revs for that. Stuff like fixing checkpatch warnings and other minor stuff should come later. Thanks, Wolfram signature.asc Description: Digital signature
[PATCH v2] of/unittest: Show broken behaviour in the platform bus
From: Grant Likely
Add a single resource to the test bus device to exercise the platform
bus code a little more. This isn't strictly a devicetree test, but it is
a corner case that the devicetree runs into. Until we've got platform
device unittests, it can live here. It doesn't need to be an explicit
text because the kernel will oops when it is wrong.
Cc: Pantelis Antoniou
Cc: Rob Herring
Cc: Greg Kroah-Hartman
Cc: Ricardo Ribalda Delgado
Signed-off-by: Grant Likely
[wsa: added the comment provided by Grant, rebased, and tested]
Signed-off-by: Wolfram Sang
---
drivers/of/unittest.c | 14 ++
1 file changed, 14 insertions(+)
I found this patch in one of my branches and think it is still worth applying.
In v1, Rob wanted a comment added which Grant provided but never folded into
the patch. I have done so now :) Note that the fix to this problem was already
picked up by Rob.
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e16ea5717b7f76..bbff09dee1cf45 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -757,6 +757,11 @@ static void __init of_unittest_match_node(void)
}
}
+static struct resource test_bus_res = {
+ .start = 0xfff8,
+ .end = 0xfff9,
+ .flags = IORESOURCE_MEM,
+};
static const struct platform_device_info test_bus_info = {
.name = "unittest-bus",
};
@@ -800,6 +805,15 @@ static void __init of_unittest_platform_populate(void)
return;
test_bus->dev.of_node = np;
+ /*
+* Add a dummy resource to the test bus node after it is
+* registered to catch problems with un-inserted resources. The
+* DT code doesn't insert the resources, and it has caused the
+* kernel to oops in the past. This makes sure the same bug
+* doesn't crop up again.
+*/
+ platform_device_add_resources(test_bus, &test_bus_res, 1);
+
of_platform_populate(np, match, NULL, &test_bus->dev);
for_each_child_of_node(np, child) {
for_each_child_of_node(child, grandchild)
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DT: i2c: Update vendor prefix for 24c00
On Sun, Dec 27, 2015 at 04:57:48PM +0200, Andy Shevchenko wrote: > On Wed, Dec 23, 2015 at 9:18 PM, Akshay Bhat wrote: > > "at" is not a valid vendor prefix, correcting the same to "atmel" > > > > I'm afraid you can't just do this change alone as it's used in some > DTS. Though you may deprecated it along with update of current users. Well, in Linux, I2C core currently strips the vendor anyhow. This will probably be changed somewhen (tm), but for now, the impact for Linux should be extremly close to 0. > > > Signed-off-by: Akshay Bhat > > --- > > Documentation/devicetree/bindings/i2c/trivial-devices.txt | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt > > b/Documentation/devicetree/bindings/i2c/trivial-devices.txt > > index c50cf13..c4a01c0 100644 > > --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt > > +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt > > @@ -20,11 +20,11 @@ adi,adt7476 +/-1C TDM Extended Temp Range I.C > > adi,adt7490+/-1C TDM Extended Temp Range I.C > > adi,adxl345Three-Axis Digital Accelerometer > > adi,adxl346Three-Axis Digital Accelerometer > > (backward-compatibility value "adi,adxl345" must be listed too) > > -at,24c08 i2c serial eeprom (24cxx) > > atmel,24c00i2c serial eeprom (24cxx) > > atmel,24c01i2c serial eeprom (24cxx) > > atmel,24c02i2c serial eeprom (24cxx) > > atmel,24c04i2c serial eeprom (24cxx) > > +atmel,24c08i2c serial eeprom (24cxx) > > atmel,24c16i2c serial eeprom (24cxx) > > atmel,24c32i2c serial eeprom (24cxx) > > atmel,24c64i2c serial eeprom (24cxx) > > -- > > 2.6.3 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majord...@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > > > -- > With Best Regards, > Andy Shevchenko signature.asc Description: Digital signature
Re: [PATCH] DT: i2c: Add Epson RX8010 to list of trivial devices
On Sun, Dec 27, 2015 at 05:02:48PM +0200, Andy Shevchenko wrote: > On Wed, Dec 23, 2015 at 8:38 PM, Akshay Bhat wrote: > > This adds devicetree documentation for the bindings of rtc-rx8010 > > driver. > > > > Signed-off-by: Akshay Bhat > > --- > > Documentation/devicetree/bindings/i2c/trivial-devices.txt | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt > > b/Documentation/devicetree/bindings/i2c/trivial-devices.txt > > index c50cf13..0f9c1de 100644 > > --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt > > +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt > > @@ -49,6 +49,7 @@ dallas,ds4510 CPU Supervisor with Nonvolatile > > Memory and Programmable I/O > > dallas,ds75Digital Thermometer and Thermostat > > dlg,da9053 DA9053: flexible system level PMIC with multicore > > support > > dlg,da9063 DA9063: system PMIC for quad-core application > > processors > > +epson,rx8010 I2C-BUS INTERFACE REAL TIME CLOCK MODULE > > Is it indeed required to have all those capital letters together? I agree. Fixing that in all Epson RTC entries would be nice. signature.asc Description: Digital signature
Re: [PATCH] DT: i2c: Add Freescale MPL3115 to trivial devices list
On Tue, Dec 29, 2015 at 12:35:16PM -0600, Rob Herring wrote: > On Wed, Dec 23, 2015 at 01:59:07PM -0500, Akshay Bhat wrote: > > This adds devicetree documentation for the bindings of mpl3115 driver. > > > > Signed-off-by: Akshay Bhat > > --- > > Documentation/devicetree/bindings/i2c/trivial-devices.txt | 1 + > > 1 file changed, 1 insertion(+) > > Acked-by: Rob Herring > > Who did you want to apply this? Send patches TO that person which I > would expect to be Wolfram or me. I usually take these. Will do again :) signature.asc Description: PGP signature
Re: I2C eeprom compatibles? (was Re: [PATCH/RFC 03/19] ARM: shmobile: gose: add i2c2 bus to device tree)
On Fri, Dec 18, 2015 at 10:06:48AM +0100, Arnd Bergmann wrote: > On Friday 18 December 2015 08:35:32 Wolfram Sang wrote: > > > > > > It seems to me that we have some consensus around: > > > > > > compatible = "renesas,r1ex24002", "24c02"; > > > > Thinking again, "generic,24c02" or "generic-24c02" could also be an > > option. > > > > > Should this be added to > > > Documentation/devicetree/bindings/eeprom/eeprom.txt ? > > > Or documented elsewhere? > > > > Probably we need a DT maintainers advice here? I don't mind vendor > > specific compatibles being documented, but I'm reluctant to add all > > these compatibles for the myriads of I2C eeproms to the at24 driver. 99% > > are covered by the generic case. > > > > Adding DT to CC. > > I'd rather use some vendor string in addition to 24c02. Isn't this originally > an Atmel part? In that case, using "atmel,24c02" as the most generic string > would be appropriate, Yeah, the at24 driver is named after Atmel chips AFAIR. Having "atmel,*" as the generic fallback sounds like a good solution to me, too. > and IIRC the i2c framework will just match that with > the "24c02" entry in the i2c_device_id list. True, although this behaviour is often complained about. There have been attempts to make i2c/spi behave like the rest of the DT world and to deprecate the current way. It didn't happen because of lots gory details, however :/ signature.asc Description: Digital signature
I2C eeprom compatibles? (was Re: [PATCH/RFC 03/19] ARM: shmobile: gose: add i2c2 bus to device tree)
On Fri, Dec 18, 2015 at 12:02:39PM +0900, Simon Horman wrote:
> On Mon, Dec 14, 2015 at 10:39:47AM +0100, Geert Uytterhoeven wrote:
> > On Mon, Dec 14, 2015 at 8:11 AM, Wolfram Sang wrote:
> > >> > >+ eeprom@50 {
> > >> > >+ compatible = "renesas,24c02";
> > >> >
> > >> >This is not a valid value -- the Renesas chip model is different
> > >> > from 24c02.
> > >>
> > >> I copied this value from r8a7791.dtsi.
> > >>
> > >> Looking at the schematic for gose (v100), koelsch (rev024) and porter
> > >> (v300)
> > >> I see the following "R1EX24002ATAS0G#U0". Shall we update r8a7791 and
> > >> this patch to "renesas,24002" or leave things as is?
> > >
> > > I wouldn't like to update the at24 driver with all namings from all
> > > vendors for chips which in large cases are simple 24c02 devices.
> > >
> > > So, if Sergei insists on the change, I'd propose
> > >
> > > compatible = "renesas,24002", "24c02";
> >
> > "renesas,r1ex24002"?
> >
> > I don't think the "A" is relevant (<= 64 is A, >= 128 is B).
> >
> > http://www.renesas.com/products/memory/eeprom/product_selector.jsp
>
> Thanks.
>
> It seems to me that we have some consensus around:
>
> compatible = "renesas,r1ex24002", "24c02";
Thinking again, "generic,24c02" or "generic-24c02" could also be an
option.
> Should this be added to Documentation/devicetree/bindings/eeprom/eeprom.txt ?
> Or documented elsewhere?
Probably we need a DT maintainers advice here? I don't mind vendor
specific compatibles being documented, but I'm reluctant to add all
these compatibles for the myriads of I2C eeproms to the at24 driver. 99%
are covered by the generic case.
Adding DT to CC.
Thanks,
Wolfram
signature.asc
Description: Digital signature
[RFC 1/3] i2c: document binding for multi-master case
From: Wolfram Sang We need this binding because some I2C master drivers will need to adapt their PM settings for the arbitration circuitry. I skipped the "i2c-" prefix because I can imagine to be useful outside of i2c. Signed-off-by: Wolfram Sang Cc: devicetree@vger.kernel.org --- Documentation/devicetree/bindings/i2c/i2c.txt | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt index a00219f5ee0733..c8d977ed847f3c 100644 --- a/Documentation/devicetree/bindings/i2c/i2c.txt +++ b/Documentation/devicetree/bindings/i2c/i2c.txt @@ -54,6 +54,11 @@ wants to support one of the below features, it should adapt the bindings below. "irq" and "wakeup" names are recognized by I2C core, other names are left to individual drivers. +- multi-master + states that there is another master active on this bus. The OS can use + this information to adapt power management to keep the arbitration awake + all the time, for example. + - wakeup-source device can be used as a wakeup source. -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 3/4] ARM: at91/dt: sama5d4: update i2c compatible string
On Thu, Dec 03, 2015 at 11:33:24AM +0100, Nicolas Ferre wrote: > Le 03/12/2015 10:53, Ludovic Desroches a écrit : > > A new compatible string has been introduced: atmel,sama5d4-i2c. It > > allows to use the i2c-sda-hold-time-ns property if needed. > > > > Signed-off-by: Ludovic Desroches > > Wolfram, we'll take this one with us in the at91 branches that will go > into arm-soc. It'll be queued in at91-4.5-dt branch soon. > > Acked-by: Nicolas Ferre I squashed patch 1+2 now and applied them to for-next, thanks! signature.asc Description: Digital signature
[PATCH 1/9] i2c: document generic DT bindings for timing parameters
From: Wolfram Sang Also, sort the properties alphabetically and make indentation consistent. Wording largely taken from i2c-rk3x.txt, thanks guys! Only "i2c-scl-internal-delay-ns" is new, the rest is used by two drivers already and was documented in their driver binding documentation. Signed-off-by: Wolfram Sang Cc: devicetree@vger.kernel.org --- Documentation/devicetree/bindings/i2c/i2c.txt | 31 ++- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt index 8a99150ac3a7fd..a00219f5ee0733 100644 --- a/Documentation/devicetree/bindings/i2c/i2c.txt +++ b/Documentation/devicetree/bindings/i2c/i2c.txt @@ -29,12 +29,33 @@ Optional properties These properties may not be supported by all drivers. However, if a driver wants to support one of the below features, it should adapt the bindings below. -- clock-frequency - frequency of bus clock in Hz. -- wakeup-source- device can be used as a wakeup source. +- clock-frequency + frequency of bus clock in Hz. -- interrupts - interrupts used by the device. -- interrupt-names - "irq" and "wakeup" names are recognized by I2C core, - other names are left to individual drivers. +- i2c-scl-falling-time-ns + Number of nanoseconds the SCL signal takes to fall; t(f) in the I2C + specification. + +- i2c-scl-internal-delay-ns + Number of nanoseconds the IP core additionally needs to setup SCL. + +- i2c-scl-rising-time-ns + Number of nanoseconds the SCL signal takes to rise; t(r) in the I2C + specification. + +- i2c-sda-falling-time-ns + Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C + specification. + +- interrupts + interrupts used by the device. + +- interrupt-names + "irq" and "wakeup" names are recognized by I2C core, other names are + left to individual drivers. + +- wakeup-source + device can be used as a wakeup source. Binding may contain optional "interrupts" property, describing interrupts used by the device. I2C core will assign "irq" interrupt (or the very first -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] i2c: brcmstb: enable ACK condition
On Wed, Oct 21, 2015 at 11:36:56AM +0900, Jaedon Shin wrote:
> Removes the condition of a message with under 32 bytes in length. The
> messages that do not require an ACK are I2C_M_IGNORE_NAK flag.
Makes me wonder why it worked before? Kamal?
>
> Signed-off-by: Jaedon Shin
> ---
> drivers/i2c/busses/i2c-brcmstb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-brcmstb.c
> b/drivers/i2c/busses/i2c-brcmstb.c
> index 2d7d155029dc..53eb8b0c9bad 100644
> --- a/drivers/i2c/busses/i2c-brcmstb.c
> +++ b/drivers/i2c/busses/i2c-brcmstb.c
> @@ -330,7 +330,7 @@ static int brcmstb_i2c_xfer_bsc_data(struct
> brcmstb_i2c_dev *dev,
> int no_ack = pmsg->flags & I2C_M_IGNORE_NAK;
>
> /* see if the transaction needs to check NACK conditions */
> - if (no_ack || len <= N_DATA_BYTES) {
> + if (no_ack) {
> cmd = (pmsg->flags & I2C_M_RD) ? CMD_RD_NOACK
> : CMD_WR_NOACK;
> pi2creg->ctlhi_reg |= BSC_CTLHI_REG_IGNORE_ACK_MASK;
> --
> 2.6.1
>
signature.asc
Description: Digital signature
Re: [PATCH 5/9] i2c: brcmstb: fix start and stop conditions
On Wed, Oct 21, 2015 at 11:36:57AM +0900, Jaedon Shin wrote: > Fixes conditions for RESTART, NOSTART and NOSTOP. The masks of start and > stop is already in brcmstb_set_i2c_start_stop(). Therefore, the caller > does not need a mask value. Hmm, and what if that changes for some reason in the future (driver refactoring)? I'd rather leave it as it is; it is a micro-optimization after all. > > Signed-off-by: Jaedon Shin > --- > drivers/i2c/busses/i2c-brcmstb.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-brcmstb.c > b/drivers/i2c/busses/i2c-brcmstb.c > index 53eb8b0c9bad..dcd1209f843f 100644 > --- a/drivers/i2c/busses/i2c-brcmstb.c > +++ b/drivers/i2c/busses/i2c-brcmstb.c > @@ -464,7 +464,7 @@ static int brcmstb_i2c_xfer(struct i2c_adapter *adapter, > pmsg->buf ? pmsg->buf[0] : '0', pmsg->len); > > if (i < (num - 1) && (msgs[i + 1].flags & I2C_M_NOSTART)) > - brcmstb_set_i2c_start_stop(dev, ~(COND_START_STOP)); > + brcmstb_set_i2c_start_stop(dev, 0); > else > brcmstb_set_i2c_start_stop(dev, > COND_RESTART | COND_NOSTOP); > @@ -485,8 +485,7 @@ static int brcmstb_i2c_xfer(struct i2c_adapter *adapter, > bytes_to_xfer = min(len, N_DATA_BYTES); > > if (len <= N_DATA_BYTES && i == (num - 1)) > - brcmstb_set_i2c_start_stop(dev, > -~(COND_START_STOP)); > + brcmstb_set_i2c_start_stop(dev, 0); > > rc = brcmstb_i2c_xfer_bsc_data(dev, tmp_buf, > bytes_to_xfer, pmsg); > -- > 2.6.1 > signature.asc Description: Digital signature
Re: [PATCH 3/9] i2c: brcmstb: add missing parenthesis
On Wed, Oct 21, 2015 at 06:56:44PM -0700, Florian Fainelli wrote:
> Le 20/10/2015 19:36, Jaedon Shin a écrit :
> > Add the necessary parenthesis for NOACK condition.
> >
> > Signed-off-by: Jaedon Shin
>
> Acked-by: Florian Fainelli
I wouldn't call them necessary?
>
> > ---
> > drivers/i2c/busses/i2c-brcmstb.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-brcmstb.c
> > b/drivers/i2c/busses/i2c-brcmstb.c
> > index 6b8bbf99880d..2d7d155029dc 100644
> > --- a/drivers/i2c/busses/i2c-brcmstb.c
> > +++ b/drivers/i2c/busses/i2c-brcmstb.c
> > @@ -305,7 +305,7 @@ static int brcmstb_send_i2c_cmd(struct brcmstb_i2c_dev
> > *dev,
> > }
> >
> > if ((CMD_RD || CMD_WR) &&
> > - bsc_readl(dev, iic_enable) & BSC_IIC_EN_NOACK_MASK) {
> > + (bsc_readl(dev, iic_enable) & BSC_IIC_EN_NOACK_MASK)) {
> > rc = -EREMOTEIO;
> > dev_dbg(dev->device, "controller received NOACK intr for %s\n",
> > cmd_string[cmd]);
> >
>
>
> --
> Florian
signature.asc
Description: Digital signature
Re: [PATCH 1/3] i2c: at91: add setting HOLD field of TWIHS_CWGR via DT
On Tue, Nov 24, 2015 at 02:47:40PM +0100, Ludovic Desroches wrote: > From: Wenyou Yang > > Add the HOLD field management. Some i2c devices need a longer data hold > time than the one given in the i2c bus specification. Since this value > depends on the i2c device connected to the bus, add a DT property to > configure it: "atmel,twd-hold-cycles". We already have "i2c-sda-hold-time-ns". Can you use that one? Sorry that this is not obviously documented, I am working on it... signature.asc Description: Digital signature
Re: [PATCH 3/3] PCI: pcie-rcar: Add support for R-Car H3.
On Mon, Nov 02, 2015 at 04:36:15PM +, Phil Edworthy wrote: > From: Harunobu Kurokawa > > R-Car H3 device is r8a7795 > > Signed-off-by: Harunobu Kurokawa Acked-by: Wolfram Sang Tested-by: Wolfram Sang signature.asc Description: Digital signature
Re: [PATCH v2 0/2] i2c: uniphier: add two I2C controller drivers for UniPhier SoC platform
On Mon, Oct 26, 2015 at 09:31:15AM +0100, Geert Uytterhoeven wrote: > On Mon, Oct 26, 2015 at 9:27 AM, Masahiro Yamada > wrote: > > 2015-10-26 17:17 GMT+09:00 Lee Jones : > >>> This series adds two I2C controller drivers. > >>> (they are completely different IPs.) > >>> > >>> The first one is a very simple FIFO-less I2C controller, > >>> which is used on some older UniPhier SoCs. > >>> > >>> The other one is higher-performance I2C controller with TX/RX FIFO, > >>> used on newer UniPhier SoCs. > >> > >> And you have sent this to me because ... ? > > > > > > No special reason. > > > > > > I sent this series to linux-...@vger.kernel.org. > > > > I guess you were automatically CC'ed by scripts/get_maintainer.pl. > > > > Using get_maintainer.pl is a normal process when sending patches, I think. > > Please use common sense. It doesn't make much sense to CC everybody who > ever made a minor edit to an affected file. > > If checkpatch comes up with more than 5 names, this should ring a bell. I usually use get_maintainer.pl with --no-git-fallback to catch people listed in MAINTAINERS only. But mileages vary a lot in that area. signature.asc Description: Digital signature
Re: [PATCH-v5 RESEND 3/5] i2c: pxa: Add support for pxa910/988 & new configuration features
> And the i2c binding documentation, which says, > for platforms using REGS_PXA2XX, they need to provide additional node > "mrvl,pxa-i2c". Which is confusing :) Yes, the description of the compatible entry is pretty bogus. Could you fix that? signature.asc Description: Digital signature
Re: [PATCH-v5 RESEND 4/5] Documentation: binding: add sclk adjustment properties to i2c-pxa
On Mon, Aug 24, 2015 at 11:29:37AM +0530, Vaibhav Hiremath wrote: > With addition of PXA910 family of devices, the TWSI module supports > new feature which allows us to adjust SCLK. i2c-pxa driver takes input > configuration in nsec and converts it to respective bit-fields, > > - i2c-sclk-low-time-ns : SCLK low time (tlow) >This property is used along with mode selection. > - i2c-sclk-high-time-ns : SCLK high time (thigh) > - i2c-start-hold-time-ns : Used in case of high speed mode for start bit >hold/setup wait counter. > - i2c-stop-hold-time-ns : Used in case of high speed mode for stop bit >hold/setup wait counter. > - i2c-sda-hold-time-ns : Used to calculate hold/setup wait counter for >standard and fast mode. > > Signed-off-by: Vaibhav Hiremath Ooookay, after checking some datasheets, I finally stumbled over the fact that this driver is not using "clock-frequency" to determine the bus speed. However, this is the standard way of defining that in DT, so this driver should adhere to that, too, and not invent something new. Previously, I was under the impression that the above parameters were needed for fine-tuning and couldn't fully grasp why. But this is for defining the bus speed which is a no-go. It looks to me that older PXA only have a bit to select between 100 and 400kHz while this newer one can have arbitrary frequencies. Correct? So, what the driver should do: - Keep support for the old "mrvl,i2c-fast-mode" binding which should be marked as deprecated - get the value of the standard property "clock-frequency" - for old PXAs, complain if not 100 or 400kHz, otherwise set the bit accordingly - for new PXAs, calculate the ilcr and iwcr values from "clock-frequency" Makes sense? Regards, Wolfram signature.asc Description: Digital signature
Re: [PATCH-v5 RESEND 3/5] i2c: pxa: Add support for pxa910/988 & new configuration features
On Mon, Aug 24, 2015 at 11:29:36AM +0530, Vaibhav Hiremath wrote: > TWSI_ILCR & TWSI_IWCR registers are used to adjust clock rate > of standard & fast mode in pxa910/988; so this patch adds these two new > entries to "struct pxa_reg_layout" and "struct pxa_i2c". > > As discussed in the previous patch-series, the idea here is to add standard > DT properties for ilcr and iwcr configuration fields. > In case of Master ilcr is used for low/high time and in case of slave mode > of operation iwcr is used for setup/hold time. > > Signed-off-by: Jett.Zhou > Signed-off-by: Yi Zhang > Signed-off-by: Vaibhav Hiremath > Tested-by: Robert Jarzmik Fixed this checkpatch warning: CHECK: Please don't use multiple blank lines #82: FILE: drivers/i2c/busses/i2c-pxa.c:157: + + and applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH-v5 RESEND 2/5] i2c: pxa: enable/disable i2c module across msg xfer
> >>Enable i2c module/unit before transmission and disable when it > >>finishes. > > >Can't you use Runtime PM for that? > > > > I haven't tried it though, I can give a try and check. It should work. Other I2C drivers are doing exactly that. signature.asc Description: Digital signature
Re: [PATCH v2 2/2] i2c: uniphier_f: add UniPhier FIFO-builtin I2C driver
On Fri, Oct 23, 2015 at 07:52:00PM +0900, Masahiro Yamada wrote: > Add support for on-chip I2C controller used on newer UniPhier SoCs > such as PH1-Pro4, PH1-Pro5, etc. This adapter is equipped with > 8-depth TX/RX FIFOs. > > Signed-off-by: Masahiro Yamada Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH v2 1/2] i2c: uniphier: add UniPhier FIFO-less I2C driver
On Fri, Oct 23, 2015 at 07:51:59PM +0900, Masahiro Yamada wrote: > Add support for on-chip I2C controller used on old UniPhier SoCs > such as PH1-LD4, PH1-sLD8, etc. This adapter is so simple that > it has no FIFO in it. > > Signed-off-by: Masahiro Yamada Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH 3/3] i2c: uniphier: add bindings for UniPhier I2C controllers
On Thu, Jul 30, 2015 at 05:12:22PM +0900, Masahiro Yamada wrote: > Device Tree bindings for two I2C controllers embedded in > UniPhier SoCs. > > Signed-off-by: Masahiro Yamada Please split this into two files with filenames matching those of the drivers. I know they will be very similar but I'd like to keep consistent. signature.asc Description: Digital signature
Re: [PATCH v2] i2c: davinci: Optimize clock generation on Keystone SoC
On Mon, Sep 14, 2015 at 11:03:50AM +0200, Alexander Sverdlin wrote: > According to "KeyStone Architecture Inter-IC Control Bus User Guide", fixed > additive part of frequency divisors (referred as "d" in the code and > datasheet) > always equals to 6, independent of module clock prescaler. > > module clock frequency Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH v2] dts: keystone: Use new "ti,keystone-i2c" compatible
On Mon, Sep 14, 2015 at 11:07:02AM +0200, Alexander Sverdlin wrote: > Now as "i2c-davinci" driver has special handling for Keystone it's time to > switch > the device tree to use new "compatible" property. Old one is left for > backwards- > compatibility. > > Signed-off-by: Alexander Sverdlin Shall I take this one or shall it go via the davinci tree? signature.asc Description: Digital signature
Re: [PATCH v4 11/22] i2c: core: Probe i2c adapters and devices on demand
On Mon, Sep 07, 2015 at 02:23:36PM +0200, Tomeu Vizoso wrote: > When looking up an i2c adapter or device through its OF node, probe it > if it hasn't already. > > The goal is to reduce deferred probes to a minimum, as it makes it very > cumbersome to find out why a device failed to probe, and can introduce > very big delays in when a critical device is probed. > > Signed-off-by: Tomeu Vizoso If the I2C part stays that simple and the core part of the series is accepted, then: Acked-by: Wolfram Sang Thanks, Wolfram signature.asc Description: Digital signature
Re: [PATCH-v5 RESEND 3/5] i2c: pxa: Add support for pxa910/988 & new configuration features
On Mon, Aug 24, 2015 at 11:29:36AM +0530, Vaibhav Hiremath wrote:
> TWSI_ILCR & TWSI_IWCR registers are used to adjust clock rate
> of standard & fast mode in pxa910/988; so this patch adds these two new
> entries to "struct pxa_reg_layout" and "struct pxa_i2c".
>
> As discussed in the previous patch-series, the idea here is to add standard
> DT properties for ilcr and iwcr configuration fields.
> In case of Master ilcr is used for low/high time and in case of slave mode
> of operation iwcr is used for setup/hold time.
I need to rethink how to describe i2c bus timing parameters in DT in the
next days. But this is planned for 4.4., promised.
One thing I already wonder about this one...
> static const struct platform_device_id i2c_pxa_id_table[] = {
> { "pxa2xx-i2c", REGS_PXA2XX },
> { "pxa3xx-pwri2c", REGS_PXA3XX },
> { "ce4100-i2c", REGS_CE4100 },
> + { "pxa910-i2c", REGS_PXA910 },
> { },
You add a new platform_id...
> @@ -1135,7 +1170,7 @@ static const struct i2c_algorithm i2c_pxa_pio_algorithm
> = {
> static const struct of_device_id i2c_pxa_dt_ids[] = {
> { .compatible = "mrvl,pxa-i2c", .data = (void *)REGS_PXA2XX },
> { .compatible = "mrvl,pwri2c", .data = (void *)REGS_PXA3XX },
> - { .compatible = "mrvl,mmp-twsi", .data = (void *)REGS_PXA2XX },
> + { .compatible = "mrvl,mmp-twsi", .data = (void *)REGS_PXA910 },
> {}
...but change the compatible binding instead of adding a new one?
signature.asc
Description: Digital signature
Re: [PATCH-v5 RESEND 2/5] i2c: pxa: enable/disable i2c module across msg xfer
On Mon, Aug 24, 2015 at 11:29:35AM +0530, Vaibhav Hiremath wrote: > From: Yi Zhang > > Enable i2c module/unit before transmission and disable when it > finishes. > > why? > It's because the i2c bus may be disturbed if the slave device, > typically a touch, powers on. I am not convinced, "may disturb" is too weak for me. It would need a way more detailed description of the problem which makes clear that this mechanism is the only way to go forward. Can't you use Runtime PM for that? signature.asc Description: Digital signature
Re: [PATCH v3 1/4] i2c: tegra: implement slave mode
> Sorry for the long delay. I tried to analyze the issue. Attached patch works > on AC100 (Misha Komarovsky helped me with testing). > > Wolfram could you please try the patch with your environment? No change, sadly. I don't get slave interrupts. > Init function is called multuple times. If I2C controller works > in slave mode, then driver must keep slave registers otherwise > slave configuration will be reseted. This patch does not tackle the main issue, though. There should not be a "slave mode" for the controller. It can be a master and slave simultaneously and should do the right thing depending on what's happening on the bus. The Tegra2 manual I have says "The Master can address the internal slave (for basic testing) or an external 7-bit or 10-bit addressed Slave device." So even a loopback should be possible (if we trust the manual ;)). signature.asc Description: Digital signature
Re: [PATCH] of/irq: export of_get_irq_byname()
On Tue, Aug 25, 2015 at 05:04:02PM -0700, Dmitry Torokhov wrote: > Similarly to of_get_irq(), let's export of_irq_get_byname(), so if a bus core > can be compiled as a module (such as I2C) it can have access to the symbol. > > Reported-by: Stephen Rothwell > Reported-by: kbuild test robot > Signed-off-by: Dmitry Torokhov Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH] of/irq: export of_get_irq_byname()
On Wed, Aug 26, 2015 at 10:24:25AM +1000, Stephen Rothwell wrote: > Hi Dmitry, > > On Tue, 25 Aug 2015 17:04:02 -0700 Dmitry Torokhov > wrote: > > > > Similarly to of_get_irq(), let's export of_irq_get_byname(), so if a bus > > core > > can be compiled as a module (such as I2C) it can have access to the symbol. > > > > Reported-by: Stephen Rothwell > > Reported-by: kbuild test robot > > Signed-off-by: Dmitry Torokhov > > I applied that to linux-next today. Pelase get it applied to the i2c > tree (with appropriate acks). Waiting for Rob's Ack, then I'll pick it up... signature.asc Description: Digital signature
Re: [PATCH v2] i2c: allow specifying separate wakeup interrupt in device tree
On Mon, Aug 17, 2015 at 11:52:51PM -0700, Dmitry Torokhov wrote: > Instead of having each i2c driver individually parse device tree data in > case it or platform supports separate wakeup interrupt, and handle > enabling and disabling wakeup interrupts in their power management > routines, let's have i2c core do that for us. > > Platforms wishing to specify separate wakeup interrupt for the device > should use named interrupt syntax in their DTSes: > > interrupt-parent = <&intc1>; > interrupts = <5 0>, <6 0>; > interrupt-names = "irq", "wakeup"; > > This patch is inspired by work done by Vignesh R for > pixcir_i2c_ts driver. > > Signed-off-by: Dmitry Torokhov Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH v2 0/2] I2C support for NXP LPC18xx family
On Sun, Aug 16, 2015 at 08:10:15PM +0200, Joachim Eastwood wrote: > This patch set adds a I2C driver and documentation for the I2C > peripheral found on many NXP LPC MCUs. > > The driver is a rework of an old driver by Kevin Wells. It has been > modified to support modern resource allocation, device tree and > generally cleaned up. Driver was originally written to support the > LPC2xxx platforms. > > Patches based on i2c/for-next and tested on Embedded Artists' LPC4357 > Dev Kit and Hitex LPC4350 Eval board with several I2C devices. > > This version address comments from Wolfram Sang. > > Changes since v1: > - remove dev_dbg() leftovers from development/debugging > - use i2c_add_adapter > - change return value on NACK and arb lost >(I think all the others should be okay as they are) > - fix 80 char warnings (involved s/reg_base/base) > - rebase on i2c/for-next > > Joachim Eastwood (2): > i2c: add i2c-lpc2k driver > doc: dt: add documentation for nxp,lpc1788-i2c Applied to for-next, thanks! Changes regarding readl_poll_timeout() can be sent incrementally. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] i2c: add i2c-lpc2k driver
On Mon, Jul 13, 2015 at 12:47:21AM +0200, Joachim Eastwood wrote:
> Add support for the I2C controller found on several NXP devices
> including LPC2xxx, LPC178x/7x and LPC18xx/43xx. The controller
> is implemented as a state machine and the driver act upon the
> state changes when the bus is accessed.
>
> The I2C controller supports master/slave operation, bus
> arbitration, programmable clock rate, and speeds up to 1 Mbit/s.
>
> Signed-off-by: Joachim Eastwood
Thanks for the submission! Looking mostly good already.
> +static void i2c_lpc2k_pump_msg(struct lpc2k_i2c *i2c)
> +{
> + unsigned char data;
> + u32 status;
> +
> + /*
> + * I2C in the LPC2xxx series is basically a state machine.
> + * Just run through the steps based on the current status.
> + */
> + status = readl(i2c->reg_base + LPC24XX_I2STAT);
> +
> + switch (status) {
> + case M_START:
> + case M_REPSTART:
> + /* Start bit was just sent out, send out addr and dir */
> + data = (i2c->msg->addr << 1);
> + if (i2c->msg->flags & I2C_M_RD)
> + data |= 1;
> +
> + writel(data, i2c->reg_base + LPC24XX_I2DAT);
> + writel(LPC24XX_STA, i2c->reg_base + LPC24XX_I2CONCLR);
> +
> + dev_dbg(&i2c->adap.dev, "Start sent with addr 0x%02x\n", data);
I assume the driver is basically working. So, in my book, most of this
debug output is useful when the driver was developed, not so much
afterwards. Also, it is printed in interrupt context possibly causing
latency. However, the information is useful for understanding the
driver, so I'd suggest to convert them into comments?
> + i2c->adap.nr = pdev->id;
> +
> + ret = i2c_add_numbered_adapter(&i2c->adap);
Intended? Most DT enabled drivers simply user i2c_add_adapter(). The
core then takes care of aliases defined in DT.
And please have a look at Documentation/i2c/fault-codes. Arbitration
Lost should be -EAGAIN, NACK should be -ENXIO, and so forth...
Thanks,
Wolfram
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 13/22] i2c: core: Probe i2c master devices on demand
On Tue, Jul 28, 2015 at 03:19:44PM +0200, Tomeu Vizoso wrote:
> When looking up an i2c master through its firmware node, probe it if it
> hasn't already.
>
> The goal is to reduce deferred probes to a minimum, as it makes it very
> cumbersome to find out why a device failed to probe, and can introduce
> very big delays in when a critical device is probed.
>
> Signed-off-by: Tomeu Vizoso
What is the status of this series? The boot time reduction sounds great.
> ---
>
> Changes in v2: None
>
> drivers/i2c/i2c-core.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index e6d4935161e4..5520b413e3db 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1353,6 +1353,8 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct
> device_node *node)
> {
> struct device *dev;
>
> + fwnode_ensure_device(&node->fwnode);
TBH, the function name doesn't tell me a lot. It ensures what?
> +
> dev = bus_find_device(&i2c_bus_type, NULL, node,
>of_dev_node_match);
> if (!dev)
> --
> 2.4.3
>
signature.asc
Description: Digital signature
Re: [PATCH-v5 1/5] Documentation: binding: add new property 'disable_after_xfer' to i2c-pxa
> Yeah, we could start i2c.txt, probably better to have separate new > patch all together. I will start such a file today as part of the i2c slave framework update which introduces flags to the reg property. Will post to the i2c list this week. signature.asc Description: Digital signature
Re: [PATCH v2] i2c: removed work arounds in i2c driver for Zynq Ultrascale+ MPSoC
On Fri, Jul 10, 2015 at 08:10:14PM +0530, Anurag Kumar Vulisha wrote: > Cadence 1.0 version has bugs which have been fixed in the cadence 1.4 version. > This patch removes the quirks present in the driver for cadence 1.4 version. > > Signed-off-by: Anurag Kumar Vulisha Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH v3 1/4] i2c: tegra: implement slave mode
> At the begin of my work on this patchset I even denied clock disable call if > slave is registered (to minimize code that can affect transfer). I hacked something like this, but it seems it was not enough. > If only slave mode is used, then this logic is not needed. This is not sufficent. We shouldn't break being a master only because we also listen to a slave address (as long as the HW supports that of course). > tegra_i2c_init is called on probe and resume. Also it is called in case of > xfer fail. If xfer is ok, then I think slave addr must be kept unchanged. This is fragile. Try scanning the bus with i2cdetect and slave setup will be gone. > As far as I understand it is a loopback mode. Probably it will not work > (Stephen Warren already mentioned this). Just to make clear: I am not saying we should support talking to our own slave address. But it should still be possible to communicate with other remote devices on the bus. Thanks, Wolfram signature.asc Description: Digital signature
Re: [PATCH v3 1/4] i2c: tegra: implement slave mode
Hi Andrey,
On Mon, Jul 20, 2015 at 11:35:43PM +0300, Andrey Danin wrote:
> Initialization code is based on NVEC driver.
>
> There is a HW bug in AP20 that was also mentioned in kernel sources
> for Toshiba AC100.
>
> Signed-off-by: Andrey Danin
Still doesn't work for me and I think I understand why. Do you run your
I2C controller in slave mode only? That might work, but using it in
master/slave mode simultanously won't work yet as I see it:
* After every transfer (as master), clocks get disabled. I assume the IP
core won't be able to detect its own address then.
* There is this code in tegra_i2c_init():
if (!i2c_dev->is_dvc) {
u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
sl_cfg |= I2C_SL_CNFG_NACK | I2C_SL_CNFG_NEWSL;
i2c_writel(i2c_dev, sl_cfg, I2C_SL_CNFG);
i2c_writel(i2c_dev, 0xfc, I2C_SL_ADDR1);
i2c_writel(i2c_dev, 0x00, I2C_SL_ADDR2);
}
It probably messes up the slave initialization in tegra_reg_slave().
At least I see that the slave address gets overwritten when I peek
the register after boot.
Does that make sense to you?
Thanks,
Wolfram
signature.asc
Description: Digital signature
Re: [PATCH v3 4/4] dt: paz00: define nvec as child of i2c bus
> It's up to you. I think if the NV guys are ok with the tegra i2c change, > Wolfram can merge it right away for 4.3. I asked him to resend, because I *want* to merge it for 4.3 :) Only the slave-mode enablement for i2c, of course. The other patches need to go via some other tree (when they are done). So, V4 with only this one patch is a good idea in my book. signature.asc Description: Digital signature
Re: [PATCH v2 1/4] i2c: tegra: implement slave mode
On Fri, Apr 03, 2015 at 09:46:26PM +0200, Wolfram Sang wrote:
> > +static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
> > +{
> > + i2c_writel(i2c_dev, val, I2C_SL_RCVD);
> > +
> > + /*
> > +* TODO: A correct fix needs to be found for this.
> > +*
> > +* We experience less incomplete messages with this delay than without
> > +* it, but we don't know why. Help is appreciated.
> > +*/
>
> Uh oh. So I assume this is another reason for staging?
I think we can live with this upstream, no need for staging.
> > + if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
> > + return -EINVAL;
>
> The core checks for slave_cb being valid.
>
> > + i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
>
> You could use value here, too, or?
>
> > + if (!tegra_i2c_slave_isr(irq, i2c_dev))
> > + return IRQ_HANDLED;
>
> Minor nit: I'd prefer == 0 here, since it reads "if not slave_isr return
> handled".
>
> > + if (slave->addr > 0x7F)
> > + addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
>
> Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you
> need to check for the flag (slave->flags & I2C_CLIENT_TEN).
If you'd fix up these and resend, I'll happily include this patch for 4.3
already.
Thanks,
Wolfram
signature.asc
Description: Digital signature
Re: How to encode being an I2C slave in DT?
> Did you have enough time to check this ? > (sorry for the noise, if I missed patch(es) in the ml) Nice coincidence, scheduled for tomorrow... signature.asc Description: Digital signature
Re: [PATCH-v4 06/11] i2c:pxa: Use devm_ variants in probe function
> So this additional error message would print one of, -EINVAL, -EBUSY > or -ENOMEM. The driver core can report this; dunno from head if it is verbose output or not. signature.asc Description: Digital signature
Re: [PATCH-v4 06/11] i2c:pxa: Use devm_ variants in probe function
> + i2c->reg_base = devm_ioremap_resource(&dev->dev, res);
> + if (IS_ERR(i2c->reg_base)) {
> + dev_err(&dev->dev, "failed to map resource: %ld\n",
> + PTR_ERR(i2c->reg_base));
> + return PTR_ERR(i2c->reg_base);
> + }
One change I did when applying: removed this error message.
devm_ioremap_resource prints out the errors it finds.
signature.asc
Description: Digital signature
Re: [PATCH-v4 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family
On Tue, Jul 14, 2015 at 01:06:39PM +0530, Vaibhav Hiremath wrote: > This patch series fixes bugs/warnings, cleans up the code and adds > support for PXA910 family of devices to PXA I2C bus driver. > > There has been one attempt made sometime back in 2012 to upstream > some of the patches from below list, but did not get follow up later. > I have consolidated all the patches, cleaned them up, splited into > logical changes, added new patches and submitting it now. > > I tried to maintain authorship & Signoff except where I did some > significant changes to the code/logic. So, I applied patches 1-6 to for-next to make some progress. The others need more thought because of the bindings which shall be discussed replying to the patches in question. Thanks for the updated work with lots of proper references. signature.asc Description: Digital signature
Re: [PATCH-v3 02/11] i2c: pxa: No need to set slave addr for i2c master mode reset
On Fri, Jul 10, 2015 at 07:55:31PM +0530, Vaibhav Hiremath wrote: > > > On Friday 10 July 2015 07:44 PM, Wolfram Sang wrote: > >On Fri, Jul 10, 2015 at 06:08:43PM +0530, Vaibhav Hiremath wrote: > >> > >> > >>On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote: > >>>On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote: > >>>>Normally i2c controller works as master, so slave addr is not needed, or > >>>>it > >>>>will impact some slave device (eg. ST NFC chip) i2c accesses, because it > >>>>has > >>>>the same i2c address with controller. > >>> > >>>Just to make sure: Does it? As I read the code, slave interrupts are > >>>enabled later only when slave mode is selected? Is that a HW bug? And if > >>>so, can't the code just be moved into this #ifdef block later? > >>> > >> > >>Yes we could, infact I thought about it; > >>but I would break recommended sequence here. > > > >And did you set the "own slave address" to a value which one of your > >existing i2c slaves also has (without enabling slave mode)? Did it > >disturb communication? > > > > Since slave and master mode are mutual exclusive, > I did not try this. Ehrm, what I meant was. Did you see the issue mentioned in the above commit message? Can you reproduce it? You don't need to enable slave mode for that, no? signature.asc Description: Digital signature
Re: [PATCH-v3 02/11] i2c: pxa: No need to set slave addr for i2c master mode reset
On Fri, Jul 10, 2015 at 06:08:43PM +0530, Vaibhav Hiremath wrote: > > > On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote: > >On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote: > >>Normally i2c controller works as master, so slave addr is not needed, or it > >>will impact some slave device (eg. ST NFC chip) i2c accesses, because it has > >>the same i2c address with controller. > > > >Just to make sure: Does it? As I read the code, slave interrupts are > >enabled later only when slave mode is selected? Is that a HW bug? And if > >so, can't the code just be moved into this #ifdef block later? > > > > Yes we could, infact I thought about it; > but I would break recommended sequence here. And did you set the "own slave address" to a value which one of your existing i2c slaves also has (without enabling slave mode)? Did it disturb communication? signature.asc Description: Digital signature
Re: [PATCH-v3 07/11] i2c: pxa: enable/disable i2c module across msg xfer
On Tue, Jul 07, 2015 at 12:54:51AM +0530, Vaibhav Hiremath wrote:
> From: Yi Zhang
>
> Enable i2c module/unit before transmission and disable when it finishes.
>
> why?
> It's because the i2c bus may be distrubed if the slave device,
> typically a touch, powers on.
>
> As we do not want to break slave mode support, this patch introduces
> DT property to control disable of the I2C module after xfer in master mode
> of operation.
>
> i2c-disable-after-xfer : If set, driver will disable I2C module after msg xfer
Hmm, I am not sure this property fits into the "describing hardware"
category. And we can't make the below behaviour default, because the
udelay(100) will cause quite some latency.
>
> Signed-off-by: Yi Zhang
> Signed-off-by: Vaibhav Hiremath
> ---
> drivers/i2c/busses/i2c-pxa.c | 43 +--
> 1 file changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 2de9300..dfd1dd0 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -161,6 +161,7 @@ struct pxa_i2c {
> unsigned char master_code;
> unsigned long rate;
> boolhighmode_enter;
> + booldisable_after_xfer;
> };
>
> #define _IBMR(i2c) ((i2c)->reg_ibmr)
> @@ -284,6 +285,24 @@ static void i2c_pxa_scream_blue_murder(struct pxa_i2c
> *i2c, const char *why)
> static void i2c_pxa_master_complete(struct pxa_i2c *i2c, int ret);
> static irqreturn_t i2c_pxa_handler(int this_irq, void *dev_id);
>
> +/* enable/disable i2c unit */
> +static inline int i2c_pxa_is_enabled(struct pxa_i2c *i2c)
> +{
> + return (readl(_ICR(i2c)) & ICR_IUE);
> +}
> +
> +static inline void i2c_pxa_enable(struct pxa_i2c *i2c, bool enable)
> +{
> + if (enable) {
> + if (!i2c_pxa_is_enabled(i2c)) {
> + writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c));
> + udelay(100);
> + }
> + } else {
> + writel(readl(_ICR(i2c)) & ~ICR_IUE, _ICR(i2c));
> + }
> +}
> +
> static inline int i2c_pxa_is_slavemode(struct pxa_i2c *i2c)
> {
> return !(readl(_ICR(i2c)) & ICR_SCLE);
> @@ -480,8 +499,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
> i2c_pxa_set_slave(i2c, 0);
>
> /* enable unit */
> - writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c));
> - udelay(100);
> + i2c_pxa_enable(i2c, true);
> }
>
>
> @@ -832,6 +850,9 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap,
> struct pxa_i2c *i2c = adap->algo_data;
> int ret, i;
>
> + /* Enable i2c unit */
> + i2c_pxa_enable(i2c, true);
> +
> /* If the I2C controller is disabled we need to reset it
> (probably due to a suspend/resume destroying state). We do
> this here as we can then avoid worrying about resuming the
> @@ -852,6 +873,11 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap,
> ret = -EREMOTEIO;
> out:
> i2c_pxa_set_slave(i2c, ret);
> +
> + /* disable i2c unit */
> + if (i2c->disable_after_xfer)
> + i2c_pxa_enable(i2c, false);
> +
> return ret;
> }
>
> @@ -1067,6 +1093,9 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap,
> struct i2c_msg msgs[], int num
> struct pxa_i2c *i2c = adap->algo_data;
> int ret, i;
>
> + /* Enable i2c unit */
> + i2c_pxa_enable(i2c, true);
> +
> for (i = adap->retries; i >= 0; i--) {
> ret = i2c_pxa_do_xfer(i2c, msgs, num);
> if (ret != I2C_RETRY)
> @@ -1080,6 +1109,10 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap,
> struct i2c_msg msgs[], int num
> ret = -EREMOTEIO;
> out:
> i2c_pxa_set_slave(i2c, ret);
> + /* disable i2c unit */
> + if (i2c->disable_after_xfer)
> + i2c_pxa_enable(i2c, false);
> +
> return ret;
> }
>
> @@ -1120,6 +1153,9 @@ static int i2c_pxa_probe_dt(struct platform_device
> *pdev, struct pxa_i2c *i2c,
> /* For device tree we always use the dynamic or alias-assigned ID */
> i2c->adap.nr = -1;
>
> + i2c->disable_after_xfer = of_property_read_bool(np,
> + "i2c-disable-after-xfer");
> +
> if (of_get_property(np, "mrvl,i2c-polling", NULL))
> i2c->use_pio = 1;
> if (of_get_property(np, "mrvl,i2c-fast-mode", NULL))
> @@ -1271,6 +1307,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
>
> platform_set_drvdata(dev, i2c);
>
> + if (i2c->disable_after_xfer)
> + i2c_pxa_enable(i2c, false);
> +
> #ifdef CONFIG_I2C_PXA_SLAVE
> dev_info(&i2c->adap.dev, " PXA I2C adapter, slave address %d\n",
> i2c->slave_addr);
> --
> 1.9.1
>
signature.asc
Description: Digital signature
Re: [PATCH-v3 06/11] i2c:pxa: Use devm_ variants in probe function
> - i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
> + i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
> if (!i2c) {
> - ret = -ENOMEM;
> - goto emalloc;
> + dev_err(&dev->dev, "memory allocation failed\n");
No message here, we get a dump anyhow.
> + res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&dev->dev, "no mem resource\n");
> + return -ENODEV;
> + }
You can skip this check, devm_ioremap_resource will do it.
> + dev_err(&dev->dev, "failed to get the clk: %ld\n",
> + PTR_ERR(i2c->clk));
Sidenote for all patches: I am not so strict with the 80 char limit. I'd
think the above code would be more readable if it was one line. But you
decide.
signature.asc
Description: Digital signature
Re: [PATCH-v3 04/11] i2c: pxa: Remove compile warnning in 64bit mode
On Tue, Jul 07, 2015 at 12:54:48AM +0530, Vaibhav Hiremath wrote: > From: Yipeng Yao > > Fix below warning message, coming from 64 bit toolchain. > > drivers/i2c/busses/i2c-pxa.c:1237:15: warning: cast from pointer to integer > of different size [-Wpointer-to-int-cast] > > Signed-off-by: Yipeng Yao > [vaibhav.hirem...@linaro.org: Updated Changelog] > Signed-off-by: Vaibhav Hiremath > Cc: Wolfram Sang > Acked-by: Robert Jarzmik Huh? Why long? Shouldn't that be casted to enum pxa_i2c_types? > --- > drivers/i2c/busses/i2c-pxa.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c > index 632008f..4c92694 100644 > --- a/drivers/i2c/busses/i2c-pxa.c > +++ b/drivers/i2c/busses/i2c-pxa.c > @@ -1116,7 +1116,9 @@ static int i2c_pxa_probe_dt(struct platform_device > *pdev, struct pxa_i2c *i2c, > i2c->use_pio = 1; > if (of_get_property(np, "mrvl,i2c-fast-mode", NULL)) > i2c->fast_mode = 1; > - *i2c_types = (u32)(of_id->data); > + > + *i2c_types = (long)(of_id->data); > + > return 0; > } > > -- > 1.9.1 > signature.asc Description: Digital signature
Re: [PATCH-v3 02/11] i2c: pxa: No need to set slave addr for i2c master mode reset
On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote: > Normally i2c controller works as master, so slave addr is not needed, or it > will impact some slave device (eg. ST NFC chip) i2c accesses, because it has > the same i2c address with controller. Just to make sure: Does it? As I read the code, slave interrupts are enabled later only when slave mode is selected? Is that a HW bug? And if so, can't the code just be moved into this #ifdef block later? > > Signed-off-by: Jett.Zhou > Signed-off-by: Vaibhav Hiremath > Acked-by: Robert Jarzmik > --- > drivers/i2c/busses/i2c-pxa.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c > index f4ac8c5..023e59f 100644 > --- a/drivers/i2c/busses/i2c-pxa.c > +++ b/drivers/i2c/busses/i2c-pxa.c > @@ -459,7 +459,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c) > writel(I2C_ISR_INIT, _ISR(i2c)); > writel(readl(_ICR(i2c)) & ~ICR_UR, _ICR(i2c)); > > - if (i2c->reg_isar) > + if (i2c->reg_isar && IS_ENABLED(CONFIG_I2C_PXA_SLAVE)) > writel(i2c->slave_addr, _ISAR(i2c)); > > /* set control register values */ > -- > 1.9.1 > signature.asc Description: Digital signature
Re: [PATCH] i2c: Mark instantiated device nodes with OF_POPULATE
On Wed, May 06, 2015 at 08:37:33PM +0200, Wolfram Sang wrote: > On Wed, May 06, 2015 at 10:51:07AM -0700, Kevin Hilman wrote: > > On Wed, Apr 22, 2015 at 12:40 AM, Wolfram Sang wrote: > > > On Sat, Jan 24, 2015 at 09:16:29AM +0200, Pantelis Antoniou wrote: > > >> Mark (and unmark) device nodes with the POPULATE flag as appropriate. > > >> This is required to avoid multi probing when using I2C and device > > >> overlays containing a mux. > > >> This patch is also more careful with the release of the adapter device > > >> which caused a deadlock with muxes, and does not break the build > > >> on !OF since the node flag accessors are not defined then. > > >> > > >> Signed-off-by: Pantelis Antoniou > > > > > > Now, that the dependency is upstream: applied to for-current, thanks! > > > > I'm not seeing this in linux-next, or in your for-current branch. > > > > Was this dropped or superseded by something else? > > I dropped it because it caused a build bug. So, we need another > dependency in... I am not sure if this is already in. Applied to for-current AGAIN, let's hope it will work this time... signature.asc Description: Digital signature
Re: [PATCH] i2c: removed work arounds in i2c driver for Zynq Ultrascale+ MPSoC
Hi,
thanks for the submission!
> +#define CDNS_I2C_BROKEN_HOLD_BIT 0x0001
BIT(0) maybe?
> +static const struct cdns_platform_data r1p10_i2c_def = {
> + .quirks = CDNS_I2C_BROKEN_HOLD_BIT, };
Closing '}' should be on seperate line.
And what Mark said.
Other than that, looks okay I'd say.
signature.asc
Description: Digital signature
Re: [PATCH v2 5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery
On Tue, Jul 07, 2015 at 03:48:52PM +0200, Uwe Kleine-König wrote: > Hello, > > On Tue, Jul 07, 2015 at 03:37:49PM +0200, Jan Lübbe wrote: > > On Mi, 2014-11-26 at 19:05 +0200, Grygorii Strashko wrote: > > > On 11/26/2014 06:04 PM, Uwe Kleine-König wrote: > > > > On Wed, Nov 26, 2014 at 03:59:53PM +0200, Grygorii Strashko wrote: > > > >> Having a board where the I2C bus locks up occasionally made it clear > > > >> that the bus recovery in the i2c-davinci driver will only work on > > > >> some boards, because on regular boards, this will only toggle GPIO > > > >> lines that aren't muxed to the actual pins. > > > >> > > > >> The I2C controller on SoCs like da850 (and da830), Keystone 2 has the > > > >> built-in capability to bit-bang its lines by using the ICPFUNC > > > >> registers > > > >> of the i2c controller. > > > >> Implement the suggested procedure by toggling SCL and checking SDA > > > >> using > > > >> the ICPFUNC registers of the I2C controller when present. Allow > > > >> platforms > > > >> to indicate the presence of the ICPFUNC registers with a has_pfunc > > > >> platform > > > >> data flag and add optional DT property "ti,has-pfunc" to indicate > > > >> the same in DT. > > > > On what does it depend if this pfunc stuff works or not? Only the SoC, > > > > or also on some board specific properties? > > > > > > SoC / set of SoCs. Also, similar feature is supported by OMAP and > > > AM335x/AM437x SoCs > > > using I2C_SYSTEST register. > > > > > > > Given the former using the > > > > compatible string to detect its availability would be better. (In this > > > > case also sorry, didn't consider this case when requesting the property > > > > in the last round.) > > > > I only stumbled across this after it was merged, with the additional > I also wonder how it came to the Reviewed-by tag for me. The last thing > that I said about the patch was "On what does it depend if this pfunc > stuff works or not? Only the SoC, or also on some board specific > properties?" (see above) and "the patch looks ok". IMHO this hardly > justifies to add the Reviewed-by tag for the next round. :-( That needs to be discussed with Grygorii. I can't verify the correctness of tags for every patch, although I do try to keep an eye on it... signature.asc Description: Digital signature
Re: [PATCH v2 5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery
> I only stumbled across this after it was merged, with the additional > ti,has-pfunc property instead of using the compatible string (which > would be better for a soc-dependent feature). Can we still fix this? Makes sense to me. signature.asc Description: Digital signature
Re: [RFC 1/3] of: make of_mutex public
> What is it that we’re trying to do here? What is the use case? http://www.spinics.net/lists/devicetree/msg85462.html I'll bounce the mails to you as well. Thanks, Wolfram signature.asc Description: Digital signature
Re: [RFC 3/3] ARM: shmobile: r8a7790: rework dts to use i2c demuxer
On Wed, Jul 01, 2015 at 09:10:23AM +0200, Geert Uytterhoeven wrote:
> On Tue, Jun 30, 2015 at 11:44 PM, Wolfram Sang wrote:
> > --- a/arch/arm/boot/dts/r8a7790-lager.dts
> > +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> > @@ -49,6 +49,8 @@
> > aliases {
> > serial0 = &scifa0;
> > serial1 = &scifa1;
> > + i2c8 = &i2chdmi;
> > + i2c9 = &gpioi2c;
>
> "i2c" or "i2c"?
i2c describes the bus here. i2c another master. Could be
made more obvious, probably.
signature.asc
Description: Digital signature
Re: [RFC 2/3] i2c: mux: demux-pinctrl: add driver
> > +Changing I2C controllers:
> > +
> > +The created mux-device will have a file "cur_master" in its sysfs-entry.
> > Write
> > +0 there for the first master listed in the "i2c-parent" property, 1 for the
> > +second etc. Reading the file will give you a list with the active master
> > +marked.
>
> This paragraph doesn't belong in the DT binding doc, but somewhere
> under Documentation/i2c/
Yes.
> > +struct i2c_demux_pinctrl_priv {
> > + int cur_chan;
> > + int num_chan;
>
> unsigned int
When comparing variables, I prefer to have them the same signedness.
> > +static struct property status_okay = { .name = "status", .length = 3,
> > .value = "ok" };
>
> "okay" or "ok"?
Both are valid, I took the shorter one.
> > + struct i2c_demux_pinctrl_priv *priv = dev_get_drvdata(dev);
> > + int count = 0, i;
>
> unsigned int i
Same as above.
>
> > +
> > + for (i = 0; i < priv->num_chan; i++)
> > + count += sprintf(buf + count, "%c %d - %s\n",
> > +i == priv->cur_chan ? '*' : ' ', i,
> > +priv->chan[i].parent_np->full_name);
> > + //FIXME: Check count > PAGE_SIZE
>
> Can you use seq_printf() for device attributes?
I can't find a reference to that.
> > +static ssize_t cur_master_store(struct device *dev, struct
> > device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct i2c_demux_pinctrl_priv *priv = dev_get_drvdata(dev);
> > + unsigned long val;
>
> unsigned int
>
> > + int ret;
> > +
> > + ret = kstrtoul(buf, 0, &val);
>
> kstrtouint()
Yes, I agree to the last two.
Thanks for the review,
Wolfram
signature.asc
Description: Digital signature
[RFC 1/3] of: make of_mutex public
From: Wolfram Sang
If we want to use OF_DYNAMIC features outside the of framework, we need
to access this lock. If OF maintainers don't like exporting, we can
maybe create accessor functions that we can use?
Signed-off-by: Wolfram Sang
---
drivers/of/of_private.h | 1 -
include/linux/of.h | 2 ++
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 8e882e706cd8c6..f92ec41efb5dfd 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -31,7 +31,6 @@ struct alias_prop {
char stem[0];
};
-extern struct mutex of_mutex;
extern struct list_head aliases_lookup;
extern struct kset *of_kset;
diff --git a/include/linux/of.h b/include/linux/of.h
index b871ff9d81d720..f0464efcbdc5aa 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -32,6 +32,8 @@
typedef u32 phandle;
typedef u32 ihandle;
+extern struct mutex of_mutex;
+
struct property {
char*name;
int length;
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 3/3] ARM: shmobile: r8a7790: rework dts to use i2c demuxer
From: Wolfram Sang
Create a seperate bus for HDMI related I2C slaves and assign it
to a i2c-gpio master. It can be switched to the i2c-rcar or
i2c-sh_mobile core at runtime.
Signed-off-by: Wolfram Sang
---
arch/arm/boot/dts/r8a7790-lager.dts | 141 ++--
1 file changed, 88 insertions(+), 53 deletions(-)
diff --git a/arch/arm/boot/dts/r8a7790-lager.dts
b/arch/arm/boot/dts/r8a7790-lager.dts
index aaa4f258e279cc..c695728b451506 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -49,6 +49,8 @@
aliases {
serial0 = &scifa0;
serial1 = &scifa1;
+ i2c8 = &i2chdmi;
+ i2c9 = &gpioi2c;
};
chosen {
@@ -245,6 +247,79 @@
#clock-cells = <0>;
clock-frequency = <14850>;
};
+
+
+ gpioi2c: i2c@9 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "i2c-gpio";
+ status = "disabled";
+ gpios = <&gpio5 6 GPIO_ACTIVE_HIGH /* sda */
+&gpio5 5 GPIO_ACTIVE_HIGH /* scl */
+ >;
+ i2c-gpio,delay-us = <5>;
+ };
+
+ i2chdmi: i2c@8 {
+
+ compatible = "i2c-demux-pinctrl";
+ i2c-parent = <&gpioi2c>, <&iic2>, <&i2c2>;
+ i2c-bus-name = "i2c-hdmi";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ak4643: sound-codec@12 {
+ compatible = "asahi-kasei,ak4643";
+
+ #sound-dai-cells = <0>;
+ reg = <0x12>;
+ };
+
+ composite-in@20 {
+ compatible = "adi,adv7180";
+ reg = <0x20>;
+ remote = <&vin1>;
+
+ port {
+ adv7180: endpoint {
+ bus-width = <8>;
+ remote-endpoint = <&vin1ep0>;
+ };
+ };
+ };
+
+ hdmi@39 {
+ compatible = "adi,adv7511w";
+ reg = <0x39>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
+
+ adi,input-depth = <8>;
+ adi,input-colorspace = "rgb";
+ adi,input-clock = "1x";
+ adi,input-style = <1>;
+ adi,input-justification = "evenly";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ adv7511_in: endpoint {
+ remote-endpoint =
<&du_out_lvds0>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ adv7511_out: endpoint {
+ remote-endpoint = <&hdmi_con>;
+ };
+ };
+ };
+ };
+ };
};
&du {
@@ -340,6 +415,11 @@
renesas,function = "iic1";
};
+ i2c2_pins: i2c2 {
+ renesas,groups = "i2c2";
+ renesas,function = "i2c2";
+ };
+
iic2_pins: iic2 {
renesas,groups = "iic2";
renesas,function = "iic2";
@@ -518,63 +598,18 @@
pinctrl-names = "default";
};
-&iic2 {
- status = "okay";
- pinctrl-0 = <&iic2_pins>;
- pinctrl-names = "default";
+&i2c2 {
+ pinctrl-0 = <&i2c2_pins>;
+ pinctrl-names = "i2c-hdmi";
clock-frequency = <10>;
+};
- ak4643: sound-codec@12 {
- compatible = "asahi-kasei,ak4643";
- #sound-dai-cells = <0>;
- reg = <0x12>;
- };
-
- composite-in@20 {
- compatible = "adi,adv7180";
- reg = <0x20>;
- remote = <&vin1>;
-
- port {
- adv7180:
[RFC 0/3] switch I2C IP cores at runtime
This series allows an I2C bus to switch between multiple masters. This is not hot-switching because connected I2C slaves will be re-instantiated. It is meant to select the best I2C core at runtime once the task is known. Example: Prefer i2c-gpio over another I2C core because of HW errata affecting your use case. It works by using OF_DYNAMIC and en-/disabling the devices as needed. See patch 2 for the implementation and more documentation. Changes since alpha v2 (only sent to sh-devel): * don't use the i2c-mux infrastructure but create own adapter This makes the DT binding proper. Otherwise, they were nested another level deeper only for a useless reg = <0>; It makes the approach also less I2C specific. * the bus can now be named I like this and consider moving this out of this series and implement it in the i2c-core. * pinctrl states to be used are now named as the bus "active" was just a too random name IMO. * bugfixes (removed two OOPSes) * more error checking * beautified the sysfs-readout to make distinction of adapters easier So, this not only a brush up since the last version, rather a new revision. I couldn't send this to the public with these obvious points left open :) PS: Since these patches depend on two others, I pushed out a branch here: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/ip-core-switch-v2-on-4.1-experimental Wolfram Sang (3): of: make of_mutex public i2c: mux: demux-pinctrl: add driver ARM: shmobile: r8a7790: rework dts to use i2c demuxer .../devicetree/bindings/i2c/i2c-demux-pinctrl.txt | 44 arch/arm/boot/dts/r8a7790-lager.dts| 141 +++ drivers/i2c/muxes/Kconfig | 9 + drivers/i2c/muxes/Makefile | 2 + drivers/i2c/muxes/i2c-demux-pinctrl.c | 266 + drivers/of/of_private.h| 1 - include/linux/of.h | 2 + 7 files changed, 411 insertions(+), 54 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt create mode 100644 drivers/i2c/muxes/i2c-demux-pinctrl.c -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 2/3] i2c: mux: demux-pinctrl: add driver
From: Wolfram Sang
This driver allows an I2C bus to switch between multiple masters. This
is not hot-switching because connected I2C slaves will be
re-instantiated. It is meant to select the best I2C core at runtime once
the task is known. Example: Prefer i2c-gpio over another I2C core
because of HW errata affecting your use case.
Signed-off-by: Wolfram Sang
---
.../devicetree/bindings/i2c/i2c-demux-pinctrl.txt | 44
drivers/i2c/muxes/Kconfig | 9 +
drivers/i2c/muxes/Makefile | 2 +
drivers/i2c/muxes/i2c-demux-pinctrl.c | 266 +
4 files changed, 321 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
create mode 100644 drivers/i2c/muxes/i2c-demux-pinctrl.c
diff --git a/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
b/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
new file mode 100644
index 00..2211f6acbf8c0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-demux-pinctrl.txt
@@ -0,0 +1,44 @@
+Pinctrl-based I2C Bus DeMux
+
+This binding describes an I2C bus demultiplexer that uses pin multiplexing to
+route the I2C signals, and represents the pin multiplexing configuration using
+the pinctrl device tree bindings. This may be used to select one I2C IP core at
+runtime which may have a better feature set for a given task than another I2C
+IP core on the SoC.
+
++--+
+| SoC |
+| | +-+ +-+
+| ++ | | dev | | dev |
+| |I2C IP Core1|--\ | +-+ +-+
+| ++ \--+ | ||
+||Pinmux|--|--++
+| ++ +--+ |
+| |I2C IP Core2|--/ |
+| ++ |
+| |
++--+
+
+Required properties:
+- compatible: "i2c-demux-pinctrl"
+- i2c-parent: List of phandles of I2C cores to be selected
+- i2c-bus-name: the name of this bus
+
+Also I2C mux properties and child nodes. See mux.txt in this directory.
+
+Also required:
+
+- pinctrl properties for the parent I2C controllers need a pinctrl state
+ with the same name as i2c-bus-name, not "default"!
+
+- the i2c masters must have their status "disabled". This driver will
+ enable them at runtime when needed.
+
+//FIXME: add example
+
+Changing I2C controllers:
+
+The created mux-device will have a file "cur_master" in its sysfs-entry. Write
+0 there for the first master listed in the "i2c-parent" property, 1 for the
+second etc. Reading the file will give you a list with the active master
+marked.
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index f6d313e528de34..b99cc15d683a50 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -60,4 +60,13 @@ config I2C_MUX_PINCTRL
This driver can also be built as a module. If so, the module will be
called pinctrl-i2cmux.
+config I2C_DEMUX_PINCTRL
+ tristate "pinctrl-based I2C demultiplexer"
+ depends on PINCTRL
+ select OF_DYNAMIC
+ help
+ If you say yes to this option, support will be included for an I2C
+ demultiplexer that uses the pinctrl subsystem. This is useful if you
+ want to change the I2C master at run-time depending on features.
+
endmenu
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 465778b5d5dc86..4d14cb88d83438 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -3,6 +3,8 @@
obj-$(CONFIG_I2C_ARB_GPIO_CHALLENGE) += i2c-arb-gpio-challenge.o
+obj-$(CONFIG_I2C_DEMUX_PINCTRL)+= i2c-demux-pinctrl.o
+
obj-$(CONFIG_I2C_MUX_GPIO) += i2c-mux-gpio.o
obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o
obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o
diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c
b/drivers/i2c/muxes/i2c-demux-pinctrl.c
new file mode 100644
index 00..5444c65de76c55
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
@@ -0,0 +1,266 @@
+/*
+ * Pinctrl based I2C DeMultiplexer V2 PROTOTYPE!
+ *
+ * Copyright (C) 2015 by Wolfram Sang, Sang Engineering
+ * Copyright (C) 2015 by Renesas Electronics Corporation
+ *
+ * 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; version 2 of the License.
+ *
+ * See the bindings doc for DTS setup.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct i2c_demux_pinctrl_chan {
+ struct device_node *parent_np;
+ struct i2c_adapter *parent_adap;
+ struct of_changeset chgset;
Re: [PATCH 2/2] drivercore: Fix unregistration path of platform devices
On Sun, Jun 07, 2015 at 03:20:11PM +0100, Grant Likely wrote:
> The unregister path of platform_device is broken. On registration, it
> will register all resources with either a parent already set, or
> type==IORESOURCE_{IO,MEM}. However, on unregister it will release
> everything with type==IORESOURCE_{IO,MEM}, but ignore the others. There
> are also cases where resources don't get registered in the first place,
> like with devices created by of_platform_populate()*.
>
> Fix the unregister path to be symmetrical with the register path by
> checking the parent pointer instead of the type field to decide which
> resources to unregister. This is safe because the upshot of the
> registration path algorithm is that registered resources have a parent
> pointer, and non-registered resources do not.
>
> * It can be argued that of_platform_populate() should be registering
> it's resources, and they argument has some merit. However, there are
> quite a few platforms that end up broken if we try to do that due to
> overlapping resources in the device tree. Until that is fixed, we need
> to solve the immediate problem.
>
> Cc: Pantelis Antoniou
> Cc: Wolfram Sang
> Cc: Rob Herring
> Cc: Greg Kroah-Hartman
> Cc: Ricardo Ribalda Delgado
> Signed-off-by: Grant Likely
Tested-by: Wolfram Sang
signature.asc
Description: Digital signature
Re: [PATCH 1/2] of/unittest: Show broken behaviour in the platform bus
On Sun, Jun 07, 2015 at 03:20:10PM +0100, Grant Likely wrote: > Add a single resource to the test bus device to exercise the platform > bus code a little more. This isn't strictly a devicetree test, but it is > a corner case that the devicetree runs into. Until we've got platform > device unittests, it can live here. It doesn't need to be an explicit > text because the kernel will oops when it is wrong. > > Cc: Pantelis Antoniou > Cc: Wolfram Sang > Cc: Rob Herring > Cc: Greg Kroah-Hartman > Cc: Ricardo Ribalda Delgado > Signed-off-by: Grant Likely Thanks for doing this! Tested-by: Wolfram Sang signature.asc Description: Digital signature
Re: [PATCH 2/2] drivercore: Fix unregistration path of platform devices
> >> This patch from Grant needs to be applied: > >> > >> [PATCH 2/2] drivercore: Fix unregistration path of platform devices > > > > I need some acks before I apply anything else as this is a total mess. > > Yes please. Rob, Pantelis, Wolfram. Can you test my patch and provides acks? Ah, this is the mail I forgot to write yesterday, sorry. I am on travel and can not test before Monday, I am afraid. Will that do? Thanks a lot for working on this, though! Much appreciated. signature.asc Description: Digital signature
Re: [PATCH] i2c: Mark instantiated device nodes with OF_POPULATE
> > IIRC it is the one which adds OF_POPULATED. If so, why is this not > > scheduled for 4.1 as this bugfix depends on it? > > No idea. This is an obvious bugfix. Dunno if you did but mentioning after the commit message where you think it should be applied is very helpful for maintainers. > I would like to, yes. It’s just the nature of the process when dealing with > multiple kernel trees. I would be happy for someone to pick up and queue it > for 4.1. That might be too late now... You could also have sent the patch to me so that I could apply it via i2c with rob's ack. Next time... signature.asc Description: Digital signature
Re: [PATCH v6 0/6] i2c: at91: add support to FIFOs and alternative command
> After fixing the version string in patch 5 as mentioned there, and added > the acks from Ludovic for the previous version. > > Applied to for-next, thanks! And unrelated to your series, just in case you feel like it, my code checkers say: drivers/i2c/busses/i2c-at91.c:213: style: Checking if unsigned variable 'buf_len' is less than zero. drivers/i2c/busses/i2c-at91.c:254: style: Checking if unsigned variable 'buf_len' is less than zero. drivers/i2c/busses/i2c-at91.c:293: style: Checking if unsigned variable 'buf_len' is less than zero. signature.asc Description: Digital signature
Re: [PATCH v6 0/6] i2c: at91: add support to FIFOs and alternative command
On Tue, Jun 09, 2015 at 06:22:13PM +0200, Cyrille Pitchen wrote:
> ChangeLog
>
> v6:
> - replace "at91sama5d2" by "sama5d2".
>
> v5:
> - print I2C controller version in an already existing dev_info() instead of
> adding a new one.
>
> v4:
> - replace 0x%x by %#x when printing I2C controller version
> - change the order of patches: the race condition bug fix becomes the first
> patch so it be can more easily applied to older kernels.
>
> v3:
> - fix braces {} coding style issue
> - split the alternative command patch into 2 patches: the first one fixes
> a race condition whereas the second one is the actual alternative command
> patch
>
> v2:
> - fix typo in comment for AT91_TWI_SVEN.
> - document new device tree bindings like "atmel,fifo-size".
> - explicitly set the has_alt_cmd boolean to false to already existing chip
> configs.
> - use the BIT() macro to define the register bits and do a little cleanup in a
> dedicated patch.
> - reword some comments to better explain why the TXCOMP interrupt is no longer
> enabled in at91_do_twi_transfer() but later in
> at91_twi_write_data_dma_callback() to avoid a race condition when DMA is
> used.
> - remove useless TXCOMP interrupt enable line in at91_twi_write_next_byte()
> since this interrupt is also enabled by at91_do_twi_transfer() for PIO
> transfers.
>
> v1:
> This series of patches adds support of two new features which will be
> introduced with Atmel sama5d2x SoC.
>
> First, the alternative command mode eases the sending of STOP conditions.
> Before starting an I2C transaction, the size data to be transfered is
> written into the new Alternative Command Register. For each byte transferred,
> the I2C controller decreases this counter and automatically sends a STOP
> condition when the counter value reaches 0, that is to say when the last byte
> of the transaction has been sent/received. So there is no longer need to set
> the STOP bit into the Control Register.
>
> Then the use of FIFOs allows to reduce number I/O accesses: for instance,
> the TX FIFO allows to write up to 4 data in a single access to the Transmit
> Holding Register. Also the RX FIFO allows to read up to 4 data in a single
> access to the Receive Holding Register. Currently only DMA transfers take
> advantage of FIFOs.
After fixing the version string in patch 5 as mentioned there, and added
the acks from Ludovic for the previous version.
Applied to for-next, thanks!
signature.asc
Description: Digital signature
Re: [PATCH] i2c: Mark instantiated device nodes with OF_POPULATE
On Wed, Jun 10, 2015 at 07:42:59AM -0500, Rob Herring wrote: > On Wed, Jun 10, 2015 at 12:40 AM, Wolfram Sang wrote: > > > >> >>>> Mark (and unmark) device nodes with the POPULATE flag as appropriate. > >> >>>> This is required to avoid multi probing when using I2C and device > >> >>>> overlays containing a mux. > >> >>>> This patch is also more careful with the release of the adapter device > >> >>>> which caused a deadlock with muxes, and does not break the build > >> >>>> on !OF since the node flag accessors are not defined then. > >> >>>> > >> >>>> Signed-off-by: Pantelis Antoniou > >> >>> > >> >>> Now, that the dependency is upstream: applied to for-current, thanks! > >> >> > >> >> I'm not seeing this in linux-next, or in your for-current branch. > >> >> > >> >> Was this dropped or superseded by something else? > >> > > >> > I dropped it because it caused a build bug. So, we need another > >> > dependency in... I am not sure if this is already in. > >> > > >> > >> FWIW, I’ve posted the dependency patch already; it’s trivial as you recall. > >> > >> Lets hope it gets picked so that this one can go in. > > > > Is that in now? > > You mean "of: Move OF flags to be visible even when !CONFIG_OF"? If > so, it is queued up for 4.2 in my tree. Sorry, I lost track of all the fixes needed for this one. I also can't recall being CCed to them. Pity. IIRC it is the one which adds OF_POPULATED. If so, why is this not scheduled for 4.1 as this bugfix depends on it? Pantelis, thanks for all the work yet I must say I am seriously confused with the handling of this patch. I thought you wanted the original I2C fix applied asap? signature.asc Description: Digital signature
Re: [PATCH v6 5/6] i2c: at91: print hardware version
> >>>- dev_info(dev->dev, "AT91 i2c bus driver.\n"); > >>>+ dev_info(dev->dev, "AT91 i2c bus driver (version: %#x).\n", > > >>It looks as if you rather print the driver's version. :-) > > > From my point of view, having a version number for a Linux driver would > >be strange > >Not everybody shares your opinion. I tend to agree with Sergei but let's keep things simple: I'll make it "hw version" before applying. signature.asc Description: Digital signature
Re: [PATCH] i2c: Mark instantiated device nodes with OF_POPULATE
> Mark (and unmark) device nodes with the POPULATE flag as appropriate. > This is required to avoid multi probing when using I2C and device > overlays containing a mux. > This patch is also more careful with the release of the adapter device > which caused a deadlock with muxes, and does not break the build > on !OF since the node flag accessors are not defined then. > > Signed-off-by: Pantelis Antoniou > >>> > >>> Now, that the dependency is upstream: applied to for-current, thanks! > >> > >> I'm not seeing this in linux-next, or in your for-current branch. > >> > >> Was this dropped or superseded by something else? > > > > I dropped it because it caused a build bug. So, we need another > > dependency in... I am not sure if this is already in. > > > > FWIW, I’ve posted the dependency patch already; it’s trivial as you recall. > > Lets hope it gets picked so that this one can go in. Is that in now? signature.asc Description: Digital signature
Re: [PATCH v5 4/6] i2c: at91: add support for new alternative command mode
Nicolas,
> > +static struct at91_twi_pdata at91sama5d2_config = {
>
> No, please name it:
>
> "sama5d2_config"
Thanks for your input, but please don't quote the whole mail for those
tiny comments. Only the relevant parts, please. Otherwise it is too easy
to miss things.
Thanks,
Wolfram
signature.asc
Description: Digital signature
Re: [PATCH v4 5/6] i2c: at91: print hardware version
On Wed, Jun 03, 2015 at 11:04:26AM +0200, Cyrille Pitchen wrote:
> The probe() function now prints the hardware version of the I2C
> controller.
>
> Signed-off-by: Cyrille Pitchen
Kernel has already very much printout, so I don't see the gain in adding
another one. At least, combine it with the other printout we have
already, but I wonder what new information will be in here? We should
know from the compatible binding which version we serve? Is it really
needed? Helpful for the user?
> ---
> drivers/i2c/busses/i2c-at91.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 67b4f15..cbe6684 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -85,6 +85,8 @@
> #define AT91_TWI_ACR_DATAL(len) ((len) & 0xff)
> #define AT91_TWI_ACR_DIRBIT(8)
>
> +#define AT91_TWI_VER0x00fc /* Version Register */
> +
> struct at91_twi_pdata {
> unsigned clk_max_div;
> unsigned clk_offset;
> @@ -872,6 +874,8 @@ static int at91_twi_probe(struct platform_device *pdev)
> return rc;
> }
>
> + dev_info(dev->dev, "version: %#x\n", at91_twi_read(dev, AT91_TWI_VER));
> +
> rc = of_property_read_u32(dev->dev->of_node, "clock-frequency",
> &bus_clk_rate);
> if (rc)
> --
> 1.8.2.2
>
signature.asc
Description: Digital signature
Re: [PATCH v3 0/6] i2c: at91: add support to FIFOs and alternative command
> I was thinking the same so I have tried to cherry-pick the patch on a 3.10.79 > and it didn't cause any conflicts so I thought it could be sent to stable as > is. OK, so reducing context seems to help. Thanks for checking! signature.asc Description: Digital signature
Re: [PATCH v3 0/6] i2c: at91: add support to FIFOs and alternative command
> If you send a new version, could you add stable in Cc to patch 3/6: No need to send a new version just because of an additional ack. I can apply that. However... > Cc: sta...@vger.kernel.org #3.10 and later > If not, Wolfram could you do it please? The problem with 3/6 is that it seems to depend on 1/6 which is a cleanup patch. If cleaning up is not essential for the bugfix, the latter should be done first. To avoid exactly this problem. signature.asc Description: Digital signature
Re: [PATCH v9 0/3] ARM: mediatek: Add driver for Mediatek I2C
On Thu, May 21, 2015 at 04:53:27PM +0800, Eddie Huang wrote: > This series is for Mediatek SoCs I2C controller common bus driver. > > Earlier MTK SoC (for example, MT6589, MT8135) I2C HW has some limitations. > New generation SoC like MT8173 fix following limitations: > > 1. Only support one i2c_msg number. One exception is WRRD (write then read) > mode. WRRD can have two i2c_msg numbers. > > 2. Mediatek I2C controller support WRRD(write then read) mode, in WRRD > mode the Repeat Start will be issued between 2 messages. > In this driver if 2 messages is first write then read, the driver will > combine 2 messages using Write-Read mode so the RS will be issued between > the 2 messages. > > 3. The max transfer data length is 255 in one message. In WRRD mode, the > max data length of second msg is 31. > > MT8135 and MT6589 can control I2C pins on PMIC(MT6397) by setting the i2c > registers in MT8135 side. In this case, driver should set OFFSET_PATH_DIR > bit first, the operation on other registers are still the same. > For now MT6589/MT8135 support this, MT6577/MT6595/MT8127 do not support. > For example, If want to use I2C4/5/6 pins on MT8135 just need to enable > the pinmux, else if want to use I2C pins on PMIC(MT6397) need to add > "mediatek,have-pmic" property in the .dts file of each platform. > > This driver is based on 4.1-rc1. Applied to for-next, thanks! Especially for being an early adaptor of the quirk framework and for keeping at this during this thorough review from all sides. Thanks to the reviewers as well! signature.asc Description: Digital signature
Re: [PATCH v9 0/3] ARM: mediatek: Add driver for Mediatek I2C
> This series already reviewed in public more than six months. In the > early version, 8173 CCF and Pinctrl not be accepted yet, so we don't put > device node with this series. Now both CCF and pinctrl have been > accepted, maybe it's time to add device node.I am glad that if this I2C > version can be accepted, If not, I will send new series with device node > in next round. I think this is good to go. Further changes should probably done as incremental patches. I am just waiting a little so people can add tags to the patches. signature.asc Description: Digital signature
Re: [PATCH RESEND V4 0/4] i2c: busses: xgene: I2C proxy driver for X-Gene
> - Fix previous comments. Next time, please specify what exactly you changed. This speeds up reviewing *a lot*, because otherwise I have to do this myself. signature.asc Description: Digital signature
Re: [PATCH] of: Move OF flags to be visible even when !CONFIG_OF
On Fri, Apr 24, 2015 at 12:41:56PM +0300, Pantelis Antoniou wrote: > We need those to be visible even when compiling with CONFIG_OF > disabled, since even the empty of_node_*_flag() method use the > flag. > > Signed-off-by: Pantelis Antoniou Acked-by: Wolfram Sang You should have mentioned that this is needed to get a bugfix for OF_DYNAMIC in i2c upstream! signature.asc Description: Digital signature
Re: [PATCH] TESTCASE: of: OOPS when disabling node via OF_DYNAMIC
> This patch has been posted before but up to now, no-one had a test case that > triggered > the bug, At your service... ;) signature.asc Description: Digital signature
Re: [PATCH] TESTCASE: of: OOPS when disabling node via OF_DYNAMIC
> Sorry for the non-prompt reply; but just for kicks, can you try the attached > patch? > > I have a hunch this might be the problem. Yeah, this even makes my more complex driver work \o/ I will post it once -rc1 is out. Thanks a lot for your help, much appreciated! signature.asc Description: Digital signature
Re: [PATCH] i2c: Mark instantiated device nodes with OF_POPULATE
On Sat, Jan 24, 2015 at 09:16:29AM +0200, Pantelis Antoniou wrote: > Mark (and unmark) device nodes with the POPULATE flag as appropriate. > This is required to avoid multi probing when using I2C and device > overlays containing a mux. > This patch is also more careful with the release of the adapter device > which caused a deadlock with muxes, and does not break the build > on !OF since the node flag accessors are not defined then. > > Signed-off-by: Pantelis Antoniou Now, that the dependency is upstream: applied to for-current, thanks! signature.asc Description: Digital signature
Re: [PATCH] TESTCASE: of: OOPS when disabling node via OF_DYNAMIC
Hi Pantelis,
thanks for your prompt reply. Unfortunately, I had to wait until I could
access the test system again.
> > + struct property status_chg = { .name = "status", .length = 9, .value =
> > "disabled" };
> > + int ret;
> > +
>
> ^ The status_chg property is on the stack. You can’t do that, because after
> you go out
> of scope the property is garbage. You should dynamically allocate.
My actual driver does that dynamically. For the test case, I considered
the stack to be enough to demonstrate my case.
> I bet that even after you fix that you’ll crash anyway.
Yes, it does.
> Note is that on many platforms the path of removing platform devices is
> borken.
> I see that you’re using PM, that’s even more problematic.
>
> The good news is that we can probably fix it if you give us a detailed log and
> stack trace.
I'll attach the full boot log. I wonder what it contains what was not in
the previous log :)
Thanks,
Wolfram
===
[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 4.0.0-rc4-9-gcb55587ebc4047 (ninja@katana)
(gcc version 4.9.2 (OSELAS.Toolchain-2014.12.0) ) #90 Tue Mar 31 17:04:52 CEST
2015
[0.00] CPU: ARMv7 Processor [413fc0f2] revision 2 (ARMv7), cr=10c5347d
[0.00] CPU: PIPT / VIPT nonaliasing data cache, PIPT instruction cache
[0.00] Machine model: Lager
[0.00] Ignoring memory block 0x14000 - 0x2
[0.00] debug: ignoring loglevel setting.
[0.00] Memory policy: Data cache writeback
[0.00] On node 0 totalpages: 262144
[0.00] free_area_init_node: node 0, pgdat c03dd6c0, node_mem_map
eeff8000
[0.00] Normal zone: 1520 pages used for memmap
[0.00] Normal zone: 0 pages reserved
[0.00] Normal zone: 194560 pages, LIFO batch:31
[0.00] HighMem zone: 67584 pages, LIFO batch:15
[0.00] [ cut here ]
[0.00] WARNING: CPU: 0 PID: 0 at arch/arm/kernel/devtree.c:144
arm_dt_init_cpu_maps+0xc0/0x130()
[0.00] DT /cpu 2 nodes greater than max cores 1, capping them
[0.00] CPU: 0 PID: 0 Comm: swapper Not tainted
4.0.0-rc4-9-gcb55587ebc4047 #90
[0.00] Hardware name: Generic R8A7790 (Flattened Device Tree)
[0.00] Backtrace:
[0.00] [] (dump_backtrace) from []
(show_stack+0x18/0x1c)
[0.00] r6:c02ee547 r5:0009 r4: r3:0020
[0.00] [] (show_stack) from []
(dump_stack+0x20/0x28)
[0.00] [] (dump_stack) from []
(warn_slowpath_common+0x8c/0xb4)
[0.00] [] (warn_slowpath_common) from []
(warn_slowpath_fmt+0x38/0x40)
[0.00] r8:eefcc690 r7:0001 r6: r5:0001 r4:eefcca98
[0.00] [] (warn_slowpath_fmt) from []
(arm_dt_init_cpu_maps+0xc0/0x130)
[0.00] r3:0002 r2:c02ee5d3
[0.00] [] (arm_dt_init_cpu_maps) from []
(setup_arch+0x5f0/0x6c8)
[0.00] r8:c02ee071 r7:c03cc630 r6:c03c7190 r5:c0340938 r4:ef7fcec0
[0.00] [] (setup_arch) from []
(start_kernel+0x88/0x394)
[0.00] r10: r9:413fc0f2 r8:40004059 r7:c03c4040 r6:
r5:c03de2c0
[0.00] r4:
[0.00] [] (start_kernel) from [<40008070>] (0x40008070)
[0.00] r9:413fc0f2 r8:40004059 r7:c03c72bc r6:c0341744 r5:c03c40b4
r4:c03de494
[0.00] ---[ end trace cb88537fdc8fa200 ]---
[0.00] CPU: All CPU(s) started in SVC mode.
[0.00] pcpu-alloc: s0 r0 d32768 u32768 alloc=1*32768
[0.00] pcpu-alloc: [0] 0
[0.00] Built 1 zonelists in Zone order, mobility grouping on. Total
pages: 260624
[0.00] Kernel command line: ignore_loglevel rw root=/dev/nfs ip=dhcp
[0.00] PID hash table entries: 4096 (order: 2, 16384 bytes)
[0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
[0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
[0.00] Memory: 1027336K/1048576K available (2403K kernel code, 112K
rwdata, 764K rodata, 644K init, 8035K bss, 21240K reserved, 0K cma-reserved,
270336K highmem)
[0.00] Virtual kernel memory layout:
[0.00] vector : 0x - 0x1000 ( 4 kB)
[0.00] fixmap : 0xffc0 - 0xfff0 (3072 kB)
[0.00] vmalloc : 0xf000 - 0xff00 ( 240 MB)
[0.00] lowmem : 0xc000 - 0xef80 ( 760 MB)
[0.00] pkmap : 0xbfe0 - 0xc000 ( 2 MB)
[0.00] .text : 0xc0008000 - 0xc0320f7c (3172 kB)
[0.00] .init : 0xc0321000 - 0xc03c2000 ( 644 kB)
[0.00] .data : 0xc03c2000 - 0xc03de2c0 ( 113 kB)
[0.00].bss : 0xc03de2c0 - 0xc0bb7038 (8036 kB)
[0.00] NR_IRQS:16 nr_irqs:16 16
[0.00] Architected cp15 timer(s) running at 10.00MHz (virt).
[0.08] sched_clock: 56 bits at 10MHz, resolution 100ns, wraps every
3435973836800ns
[0.28] Switching to timer-based delay loop, resolution 100ns
[
Re: [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegrai2c.
> One thing where we need your help as a I2C maintainer is how to represent an > i2c slave device using device-tree. You may remember our discussion in the > past from here [1] where you suggested to just make a slave client by its > compatible name. Stephen Warren from NVIDIA raised some concerns about this > solution because it may not be appropriate in all possible future cases > (which > is what a proper device-tree representation should take care off). He instead > suggested to mark a slave client by adding some flag to the reg property, to > be able to handle a situation where both master client and slave client have > the same i2c bus address forming a loopback (e.g. for testing purpose) on the > same bus. More details here [2]. > > I hope with this post I can join the different discussions somehow so we are > able to find a common sense which is acceptable for all. I'll have a look again for 4.2. signature.asc Description: Digital signature
Re: [PATCH v2 2/4] staging/nvec: reimplement on top of tegra i2c driver
> +/**
> + * nvec_slave_cb - I2C slave callback
> + *
> + * This callback fills our RX buffers and empties our TX
> + * buffers. This uses a finite state machine.
> + */
> +static int nvec_slave_cb(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> +{
> + struct nvec_chip *nvec = i2c_get_clientdata(client);
> +
> + switch (event) {
> + case I2C_SLAVE_WRITE_REQUESTED:
> + /* Alloc new msg only if prev transaction finished */
> + if (nvec->state == ST_NONE)
> + nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
> +
> + /* Should not happen in a normal world */
> + if (unlikely(nvec->rx == NULL)) {
> + nvec->state = ST_NONE;
> + return -1;
> + }
> + nvec->rx->pos = 0;
> +
> + if (client->addr != ((*val) >> 1)) {
Uh, I2C_SLAVE_WRITE_REQUESTED should not use val.
> + dev_err(&client->dev,
> + "received address 0x%02x, expected 0x%02x\n",
> + ((*val) >> 1), client->addr);
> + return -1;
> + }
> + nvec->state = ST_TRANS_START;
> + break;
> +
...
> + case I2C_SLAVE_READ_PROCESSED:
> + if (nvec->state != ST_RX &&
> + nvec->state != ST_TX) {
> + dev_err(&client->dev,
> + "unexpected read: state %d\n",
> + nvec->state);
> + return -1;
> + }
> +
> + if (!nvec->tx || nvec->tx->pos >= nvec->tx->size) {
> + dev_err(nvec->dev,
> + "tx buffer underflow on %p (%u > %u)\n",
> + nvec->tx,
> + (uint) (nvec->tx ? nvec->tx->pos : 0),
> + (uint) (nvec->tx ? nvec->tx->size : 0));
> + nvec->state = ST_NONE;
> + break;
> + }
> +
> + nvec->state = ST_TX;
> + *val = nvec->tx->data[nvec->tx->pos++];
Are you sure you want to increase the pointer here? Remember that this
byte is requested but might not be sent out if the remote master stops
the transfer after the previous byte using NACK instead of ACK.
> + break;
> +
signature.asc
Description: Digital signature
Re: [PATCH v2 0/4] arm: tegra: implement NVEC driver using tegra i2c.
On Mon, Mar 30, 2015 at 11:00:11PM +0300, Andrey Danin wrote: > NVEC driver contains code to manage tegra i2c controller in slave mode. > I2C slave support was implemented in linux kernel. The goal of this > patch serie is to implement I2C slave mode in tegra drived and rework > NVEC driver to use it. Thanks. Looks pretty good already. If being I2C slave was the reason for this driver being in staging, we soon need to think of a proper place outside staging. signature.asc Description: Digital signature
Re: [PATCH v2 1/4] i2c: tegra: implement slave mode
> +static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val)
> +{
> + i2c_writel(i2c_dev, val, I2C_SL_RCVD);
> +
> + /*
> + * TODO: A correct fix needs to be found for this.
> + *
> + * We experience less incomplete messages with this delay than without
> + * it, but we don't know why. Help is appreciated.
> + */
Uh oh. So I assume this is another reason for staging?
> + if (!i2c_dev->slave || !i2c_dev->slave->slave_cb)
> + return -EINVAL;
The core checks for slave_cb being valid.
> + i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy);
You could use value here, too, or?
> + if (!tegra_i2c_slave_isr(irq, i2c_dev))
> + return IRQ_HANDLED;
Minor nit: I'd prefer == 0 here, since it reads "if not slave_isr return
handled".
> + if (slave->addr > 0x7F)
> + addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE;
Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you
need to check for the flag (slave->flags & I2C_CLIENT_TEN).
signature.asc
Description: Digital signature
Re: [PATCH_V3 0/2] i2c: jz4780: Add Ingenic JZ4780 i2c driver
On Tue, Mar 31, 2015 at 02:03:54PM +0100, Zubair Lutfullah Kakakhel wrote: > Hi, > > Here we have two patches that add support for the i2c > controller present in the Ingenic JZ4780. > > Driver tested on the MIPS Creator CI20. > > V2 - > V3 > Fixes based on feedback from Wolfram Sang. Thank-you. > Details in individual patch commit message > Rebased to v4.0-rc6 > > V1 - > V2 > Rebased to v4.0-rc3 > Minor tweaks/fixes. Details in individual patch commits. > > Feedback welcome. Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH V2 1/3] i2c: busses: add SLIMpro I2C device driver on APM X-Gene platform
> >> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen, > >> +DMA_FROM_DEVICE); > > > > The device should be the device of the dma channel. > > The device is not visible on linux, DMA is done by the helper processor. > Perhaps you cah give me some idea how to do this. Thanks Well, that is a question for the DMA maintainers. But we can fix this later, I guess. signature.asc Description: Digital signature
Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes
> While thinking about it (and I still think of it as a 'big issue' > compared to the intention of the initial patch) I came to the > conclusion that I should maybe just go for a board-specific > i2c-mux-pinctrl node instead of adding it to the SoC dtsi. That will > also avoid doubled i2c busses on boards with just the default i2c > option. Ehrm, then please let me know what you decided on. If you chose the above road, then I don't need to think about the other questions :) signature.asc Description: Digital signature
[PATCH] TESTCASE: of: OOPS when disabling node via OF_DYNAMIC
From: Wolfram Sang I wanted to disable a node via OF_DYNAMIC by setting its status to disabled. This code is the minimal testcase, the same happens in a more complex scenario. There is something wrong with freeing resources. Is my module wrong? Or is it a bug? Crashlog without CONFIG_I2C_CHARDEV, slightly shortened: [5.127314] of/notify UPDATE_PROPERTY /i2c@e60b:status [5.132814] i2c-sh_mobile e60b.i2c: pm_clk_notify() 2 [5.138504] bus: 'platform': remove device e60b.i2c [5.143762] i2c-sh_mobile e60b.i2c: pm_clk_notify() 6 [5.149231] device: '7-0068': device_unregister [5.153982] bus: 'i2c': remove device 7-0068 [5.158679] device: 'regulator.2': device_unregister [5.165827] i2c 7-0068: uevent [5.168997] i2c i2c-7: adapter [e60b.i2c] unregistered [5.174462] device: 'i2c-7': device_unregister [5.179057] bus: 'i2c': remove device i2c-7 [5.183570] platform e60b.i2c: pm_clk_notify() 7 [5.188595] platform e60b.i2c: Runtime PM disabled, clock forced off. [5.195368] platform e60b.i2c: pm_clk_notify() 3 [5.200426] Unable to handle kernel NULL pointer dereference at virtual address 0018 [5.208474] pgd = c0004000 [5.211164] [0018] *pgd= [5.214745] Internal error: Oops: 5 [#1] ARM [5.218995] CPU: 0 PID: 1 Comm: swapper Tainted: GW 4.0.0-rc4-9-gcb55587ebc4047 #90 [5.228148] Hardware name: Generic R8A7790 (Flattened Device Tree) [5.234288] task: ee84d340 ti: ee84e000 task.ti: ee84e000 [5.239656] PC is at release_resource+0x20/0x68 [5.244171] LR is at _raw_write_lock+0x3c/0x44 [5.248591] pc : []lr : []psr: a113 [5.248591] sp : ee84fcd8 ip : ee84fcb0 fp : ee84fcec [5.259989] r10: r9 : c03de2c0 r8 : c03d5694 [5.265180] r7 : ee84fdf4 r6 : 001c r5 : r4 : ee8efd00 [5.271664] r3 : r2 : 0018 r1 : 0009 r0 : c03c7cd8 [5.278149] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel [5.285410] Control: 10c5347d Table: 40004059 DAC: 0015 [5.291117] Process swapper (pid: 1, stack limit = 0xee84e208) [5.296911] Stack: (0xee84fcd8 to 0xee85) [5.301243] fcc0: ee8f3e00 [5.309375] fce0: ee84fd0c ee84fcf0 c01604fc c0024168 c03d39a0 ee8f3e00 0005 [5.317507] fd00: ee84fd24 ee84fd10 c016054c c0160494 c03d39a0 ee8f3e10 ee84fd3c ee84fd28 [5.325639] fd20: c01aad54 c0160544 ee8f3e34 ee8f3e00 ee84fd64 ee84fd40 c01ab3f8 c01aad08 ... [5.512643] Backtrace: [5.515097] [] (release_resource) from [] (platform_device_del+0x74/0x84) [5.523561] r4:ee8f3e00 r3: [5.527145] [] (platform_device_del) from [] (platform_device_unregister+0x14/0x20) [5.536469] r6:0005 r5: r4:ee8f3e00 r3:c03d39a0 [5.542152] [] (platform_device_unregister) from [] (of_platform_device_destroy+0x58/0xa8) [5.552080] r4:ee8f3e10 r3:c03d39a0 [5.555667] [] (of_platform_device_destroy) from [] (of_platform_notify+0xcc/0xe8) [5.564905] r4:ee8f3e00 r3:ee8f3e34 [5.568493] [] (of_platform_notify) from [] (notifier_call_chain+0x48/0x70) [5.577128] r6:0005 r5: r4: [5.581759] [] (notifier_call_chain) from [] (__blocking_notifier_call_chain+0x4c/0x64) [5.591429] r8:0005 r7:ee84fdf4 r6: r5:c03d7ba4 r4:c03d7b98 r3: [5.599192] [] (__blocking_notifier_call_chain) from [] (blocking_notifier_call_chain+0x20/0x28) [5.609637] r8:c0340d2c r7:c0340d2c r6:ee84fe88 r5:0005 r4:ee84fdf4 [5.616365] [] (blocking_notifier_call_chain) from [] (of_reconfig_notify+0x88/0xa8) [5.625788] [] (of_reconfig_notify) from [] (of_property_notify+0x44/0x4c) [5.634338] r5:ee84fe88 r4:ee940440 [5.637925] [] (of_property_notify) from [] (__of_changeset_entry_notify+0x84/0xc4) [5.647263] [] (__of_changeset_entry_notify) from [] (of_changeset_apply+0xa4/0xe4) [5.656588] r4:ee940440 [5.659126] [] (of_changeset_apply) from [] (test_init+0x84/0xa8) [5.666900] r6: r5:c03356c0 r4:eefd18b8 [5.671533] [] (test_init) from [] (do_one_initcall+0x108/0x1bc) [5.679220] r4:ee940440 Signed-off-by: Wolfram Sang --- drivers/of/Makefile | 2 +- drivers/of/wsa_testcase.c | 48 +++ 2 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 drivers/of/wsa_testcase.c diff --git a/drivers/of/Makefile b/drivers/of/Makefile index 7563f36c71db34..4ec30a5bab39d5 100644 --- a/drivers/of/Makefile +++ b/drivers/of/Makefile @@ -1,4 +1,4 @@ -obj-y = base.o device.o platform.o +obj-y = base.o device.o platform.o wsa_testcase.o obj-$(CONFIG_OF_DYNAMIC) += dynamic.o obj-$(CONFIG_OF_FLATTREE) += fdt.o obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o diff --git a/drivers
Re: [PATCH_V2 2/2] i2c: jz4780: Add i2c bus controller driver for Ingenic JZ4780
Hi,
mostly looking good. checkpatch.pl --strict has some whitespaces and
continuation issues, please fix them.
And sparse rightfully says:
drivers/i2c/busses/i2c-jz4780.c:510:51: warning: cast truncates bits from
constant value (100 becomes 0)
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 22da9c2..50b0c91 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -909,6 +909,15 @@ config I2C_RCAR
> This driver can also be built as a module. If so, the module
> will be called i2c-rcar.
>
> +config I2C_JZ4780
> + tristate "JZ4780 I2C controller interface support"
> + depends on MACH_JZ4780
|| COMPILE_TEST ?
> + help
> + If you say yes to this option, support will be included for the
> + Ingenic JZ4780 I2C controller.
> +
> + If you don't know what to do here, say N.
> +
Sorting looks broken.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Please sort the includes, that prevents duplicates.
> +static inline unsigned short jz4780_i2c_readw(struct jz4780_i2c *i2c,
> + unsigned long offset)
> +{
> + return readw(i2c->iomem + offset);
> +}
> +
> +static inline void jz4780_i2c_writew(struct jz4780_i2c *i2c,
> + unsigned long offset, unsigned short val)
> +{
> + writew(val, i2c->iomem + offset);
> +}
I don't think these functions add much compared to readw/writew, but you
can keep them if you really want.
> + do {
> + i2c_sta = jz4780_i2c_readw(i2c, JZ4780_I2C_STA);
> + if ((i2c_sta & JZ4780_I2C_STA_TFNF)
> + && (i2c->wt_len > 0)) {
> + data = *i2c->wbuf;
> + data &= (~JZ4780_I2C_DC_READ);
> + jz4780_i2c_writew(i2c, JZ4780_I2C_DC,
> + data);
> + i2c->wbuf++;
> + i2c->wt_len--;
> + } else {
> + break;
> + }
> + } while (1);
do/while(1) with else break;
That can be simplified with a while loop?
> +static void jz4780_i2c_txabrt(struct jz4780_i2c *i2c, int src)
> +{
> + int i;
> +
> + dev_err(&i2c->adap.dev, "txabrt: 0x%08x\n", src);
> + dev_err(&i2c->adap.dev, "device addr=%x\n",
> + jz4780_i2c_readw(i2c, JZ4780_I2C_TAR));
> + dev_err(&i2c->adap.dev, "send cmd count:%d %d\n",
> + i2c->cmd, i2c->cmd_buf[i2c->cmd]);
> + dev_err(&i2c->adap.dev, "receive data count:%d %d\n",
> + i2c->cmd, i2c->data_buf[i2c->cmd]);
> +
> + for (i = 0; i < 16; i++) {
> + if (src & BIT(i))
> + dev_info(&i2c->adap.dev, "I2C TXABRT[%d]=%s\n",
> + i, jz4780_i2c_abrt_src[i]);
> + }
Isn't that more dev_dbg?
> + if (!timeout) {
> + dev_err(&i2c->adap.dev, "irq read timeout\n");
> + dev_err(&i2c->adap.dev, "send cmd count:%d %d\n",
> + i2c->cmd, i2c->cmd_buf[i2c->cmd]);
> + dev_err(&i2c->adap.dev, "receive data count:%d %d\n",
> + i2c->cmd, i2c->data_buf[i2c->cmd]);
Same here with the last two messages?
> + if (msg->addr != jz4780_i2c_readw(i2c, JZ4780_I2C_TAR)) {
> + ret = jz4780_i2c_set_target(i2c, msg->addr);
> + if (ret) {
> + dev_err(&i2c->adap.dev, "I2C set target failed\n");
> + goto out;
> + }
set_target already has an error message. And set_target should return
some errno, not -1.
> + }
> + for (i = 0; i < count; i++, msg++) {
> + if (msg->flags & I2C_M_RD)
> + ret = jz4780_i2c_xfer_read(i2c, msg->buf, msg->len,
> +count, i);
> + else
> + ret = jz4780_i2c_xfer_write(i2c, msg->buf, msg->len,
> + count, i);
> +
> + if (ret) {
> + dev_err(&i2c->adap.dev, "I2C xfer failed\n");
> + ret = -EAGAIN;
No use error codes like described in Documentation/i2c/fault-codes.
-EAGAIN has only one well defined meaning.
> + i2c->adap.owner = THIS_MODULE;
> + i2c->adap.algo = &jz4780_i2c_algorithm;
> + i2c->adap.algo_data = i2c;
> + i2c->adap.timeout = 5;
5 jiffies? That's probably not what you meant. Why not leave the
default?
Re: [PATCH V2 1/3] i2c: busses: add SLIMpro I2C device driver on APM X-Gene platform
On Wed, Feb 18, 2015 at 11:34:01AM -0800, Feng Kan wrote:
> Add SLIMpro I2C device driver on APM X-Gene platform. This I2C
> device driver use the SLIMpro Mailbox driver to tunnel message to
> the SLIMpro coprocessor to do the work of accessing I2C components.
>
> Signed-off-by: Feng Kan
> Signed-off-by: Hieu Le
Sigh, I lost my first review, so here we go again...
It looks mostly good. Using checkpatch with '--strict' will show some
whitespace issues. Please fix these.
> +config I2C_XGENE_SLIMPRO
> + tristate "APM X-Gene SoC I2C SLIMpro devices support"
> + depends on ARCH_XGENE && MAILBOX
COMPILE_TEST?
> +static int start_i2c_msg_xfer(struct slimpro_i2c_dev *ctx)
> +{
> + if (ctx->mbox_client.tx_block) {
> + if (!wait_for_completion_timeout(&ctx->rd_complete,
> + msecs_to_jiffies
> + (MAILBOX_OP_TIMEOUT)))
Don't be too strict with the 80 char limit IMHO. I think it is more
readable to combine the last two lines into one.
> + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip,
> + SLIMPRO_IIC_READ, protocol, addrlen,
> + readlen);
ditto
> + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip,
> + SLIMPRO_IIC_WRITE, protocol, addrlen,
> + writelen);
ditto
> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen,
> +DMA_FROM_DEVICE);
The device should be the device of the dma channel.
> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, writelen,
> +DMA_TO_DEVICE);
ditto
> + rc = dma_mapping_error(ctx->dev, paddr);
> + if (dma_mapping_error(ctx->dev, paddr)) {
if (rc)
> +static struct i2c_algorithm xgene_slimpro_i2c_algorithm = {
> + .smbus_xfer = xgene_slimpro_i2c_xfer,
Might be a tad less confusing to name this function
xgene_slimpro_smbus_xfer. You decide.
> + rc = i2c_add_adapter(adapter);
> + if (rc) {
> + dev_err(&pdev->dev, "Adapter registeration failed\n");
> + return rc;
> + }
> +
> + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> + if (rc)
> + dev_warn(&pdev->dev, "Unable to set dma mask\n");
Shouldn't that be before i2c_add_adapter?
> +static struct platform_driver xgene_slimpro_i2c_driver = {
> + .probe = xgene_slimpro_i2c_probe,
> + .remove = xgene_slimpro_i2c_remove,
> + .driver = {
> + .name = "xgene-slimpro-i2c",
> + .of_match_table = of_match_ptr(xgene_slimpro_i2c_id)
You are DT only, do we need of_match_ptr?
> +MODULE_DESCRIPTION("APM X-Gene SLIMpro I2C driver");
> +MODULE_LICENSE("GPL");
MODULE_AUTHOR left out intentionally?
Thanks,
Wolfram
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes
> If modifying i2c-mux-pinctrl to respect the sub-bus status property is > such a big issue, I'd rather leave the driver as is and expose all > sub-busses to userspace. Well, dunno what 'big issue' is in your book :) What definately needs to be done is: * handle "status" at mux-core level, not mux-driver * that probably needs us to convert i2c_add_mux_adapter() to return ERR_PTRs instead of NULL, so we can distinguish the "disabled" case * that would mean to fix all existing users That's all not groundbreaking stuff, but needs caution and thoroughness. There might be some gory details left, though... Regards, Wolfram -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
