[PATCH 0/4] i2c bus recovery for Microchip SoCs.

[PATCH 0/4] i2c bus recovery for Microchip SoCs.

[Kamel Bouhara]

This patch series introduce the kernel i2c-gpio bus recovery mechanism
for the Microchip SoCs. Updated the corresponding dts to add i2c
gpio pinctrl. The bus recovery is configured for the sama5d3/4 xplained
boards in dts.

Kamel Bouhara (4):
  dt-bindings: i2c: at91: document optional bus recovery properties
  i2c: at91: implement i2c bus recovery
  ARM: at91/dt: sama5d3: add i2c gpio pinctrl
  ARM: at91/dt: sama5d4: add i2c gpio pinctrl

 .../devicetree/bindings/i2c/i2c-at91.txt      | 10 +++
 arch/arm/boot/dts/sama5d3.dtsi                | 33 +++++++++-
 arch/arm/boot/dts/sama5d4.dtsi                | 33 +++++++++-
 drivers/i2c/busses/i2c-at91-master.c          | 63 +++++++++++++++++++
 drivers/i2c/busses/i2c-at91.h                 |  8 +++
 5 files changed, 141 insertions(+), 6 deletions(-)

--
2.23.0

[PATCH 0/4] i2c bus recovery for Microchip SoCs.

[Kamel Bouhara]

This patch series introduce the kernel i2c-gpio bus recovery mechanism
for the Microchip SoCs. Updated the corresponding dts to add i2c
gpio pinctrl. The bus recovery is configured for the sama5d3/4 xplained
boards in dts.

Kamel Bouhara (4):
  dt-bindings: i2c: at91: document optional bus recovery properties
  i2c: at91: implement i2c bus recovery
  ARM: at91/dt: sama5d3: add i2c gpio pinctrl
  ARM: at91/dt: sama5d4: add i2c gpio pinctrl

 .../devicetree/bindings/i2c/i2c-at91.txt      | 10 +++
 arch/arm/boot/dts/sama5d3.dtsi                | 33 +++++++++-
 arch/arm/boot/dts/sama5d4.dtsi                | 33 +++++++++-
 drivers/i2c/busses/i2c-at91-master.c          | 63 +++++++++++++++++++
 drivers/i2c/busses/i2c-at91.h                 |  8 +++
 5 files changed, 141 insertions(+), 6 deletions(-)

--
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] dt-bindings: i2c: at91: document optional bus recovery properties

[Kamel Bouhara]

The at91 I2C controller can support bus recovery by re-assigning SCL
and SDA to gpios. Add the optional pinctrl and gpio properties to do
so.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 Documentation/devicetree/bindings/i2c/i2c-at91.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
index b7cec17c3daf..8ea2ce5d8610 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
@@ -19,8 +19,13 @@ Optional properties:
   capable I2C controllers.
 - i2c-sda-hold-time-ns: TWD hold time, only available for "atmel,sama5d4-i2c"
   and "atmel,sama5d2-i2c".
+- scl-gpios: specify the gpio related to SCL pin
+- sda-gpios: specify the gpio related to SDA pin
+- pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
+  bus recovery, call it "gpio" state
 - Child nodes conforming to i2c bus binding
 
+
 Examples :
 
 i2c0: i2c@fff84000 {
@@ -55,6 +60,11 @@ i2c0: i2c@f8034600 {
 	clocks = <&flx0>;
 	atmel,fifo-size = <16>;
 	i2c-sda-hold-time-ns = <336>;
+	pinctrl-names = "default", "gpio";
+	pinctrl-0 = <&pinctrl_i2c0>;
+	pinctrl-1 = <&pinctrl_i2c0_gpio>;
+	sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>;
+	scl-gpios = <&pioA 31 GPIO_ACTIVE_HIGH>;
 
 	wm8731: wm8731@1a {
 		compatible = "wm8731";
-- 
2.23.0

[PATCH 1/4] dt-bindings: i2c: at91: document optional bus recovery properties

[Kamel Bouhara]

The at91 I2C controller can support bus recovery by re-assigning SCL
and SDA to gpios. Add the optional pinctrl and gpio properties to do
so.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 Documentation/devicetree/bindings/i2c/i2c-at91.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
index b7cec17c3daf..8ea2ce5d8610 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
@@ -19,8 +19,13 @@ Optional properties:
   capable I2C controllers.
 - i2c-sda-hold-time-ns: TWD hold time, only available for "atmel,sama5d4-i2c"
   and "atmel,sama5d2-i2c".
+- scl-gpios: specify the gpio related to SCL pin
+- sda-gpios: specify the gpio related to SDA pin
+- pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
+  bus recovery, call it "gpio" state
 - Child nodes conforming to i2c bus binding
 
+
 Examples :
 
 i2c0: i2c@fff84000 {
@@ -55,6 +60,11 @@ i2c0: i2c@f8034600 {
 	clocks = <&flx0>;
 	atmel,fifo-size = <16>;
 	i2c-sda-hold-time-ns = <336>;
+	pinctrl-names = "default", "gpio";
+	pinctrl-0 = <&pinctrl_i2c0>;
+	pinctrl-1 = <&pinctrl_i2c0_gpio>;
+	sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>;
+	scl-gpios = <&pioA 31 GPIO_ACTIVE_HIGH>;
 
 	wm8731: wm8731@1a {
 		compatible = "wm8731";
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] i2c: at91: implement i2c bus recovery

[Kamel Bouhara]

Implement i2c bus recovery when slaves devices might hold SDA low.
In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
until the slave release SDA.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 drivers/i2c/busses/i2c-at91-master.c | 63 ++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-at91.h        |  8 ++++
 2 files changed, 71 insertions(+)

diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
index a3fcc35ffd3b..df5bb93f952d 100644
--- a/drivers/i2c/busses/i2c-at91-master.c
+++ b/drivers/i2c/busses/i2c-at91-master.c
@@ -18,11 +18,13 @@
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/dma-atmel.h>
 #include <linux/pm_runtime.h>
@@ -768,6 +770,63 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
 	return ret;
 }
 
+static void at91_prepare_twi_recovery(struct i2c_adapter *adap)
+{
+	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+
+	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
+}
+
+static void at91_unprepare_twi_recovery(struct i2c_adapter *adap)
+{
+	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+
+	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
+}
+
+static int at91_init_twi_recovery_info(struct platform_device *pdev,
+				       struct at91_twi_dev *dev)
+{
+	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
+
+	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
+		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
+		return PTR_ERR(dev->pinctrl);
+	}
+
+	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
+							 PINCTRL_STATE_DEFAULT);
+	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
+						      "gpio");
+	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
+	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl",
+					  GPIOD_OUT_HIGH_OPEN_DRAIN);
+	if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (IS_ERR(rinfo->sda_gpiod) ||
+		   IS_ERR(rinfo->scl_gpiod) ||
+		   IS_ERR(dev->pinctrl_pins_default) ||
+		   IS_ERR(dev->pinctrl_pins_gpio)) {
+		dev_info(&pdev->dev, "recovery information incomplete\n");
+		return -EINVAL;
+	}
+
+	dev_info(&pdev->dev, "using scl%s for recovery\n",
+		 rinfo->sda_gpiod ? ",sda" : "");
+
+	rinfo->prepare_recovery = at91_prepare_twi_recovery;
+	rinfo->unprepare_recovery = at91_unprepare_twi_recovery;
+	rinfo->recover_bus = i2c_generic_scl_recovery;
+	dev->adapter.bus_recovery_info = rinfo;
+
+	return 0;
+}
+
 int at91_twi_probe_master(struct platform_device *pdev,
 			  u32 phy_addr, struct at91_twi_dev *dev)
 {
@@ -795,6 +854,10 @@ int at91_twi_probe_master(struct platform_device *pdev,
 
 	at91_calc_twi_clock(dev);
 
+	rc = at91_init_twi_recovery_info(pdev, dev);
+	if (rc == -EPROBE_DEFER)
+		return rc;
+
 	dev->adapter.algo = &at91_twi_algorithm;
 	dev->adapter.quirks = &at91_twi_quirks;
 
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
index 499b506f6128..b89dab55e776 100644
--- a/drivers/i2c/busses/i2c-at91.h
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -141,6 +141,10 @@ struct at91_twi_dev {
 	u32 fifo_size;
 	struct at91_twi_dma dma;
 	bool slave_detected;
+	struct i2c_bus_recovery_info rinfo;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pinctrl_pins_default;
+	struct pinctrl_state *pinctrl_pins_gpio;
 #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
 	unsigned smr;
 	struct i2c_client *slave;
@@ -158,6 +162,10 @@ void at91_init_twi_bus_master(struct at91_twi_dev *dev);
 int at91_twi_probe_master(struct platform_device *pdev, u32 phy_addr,
 			  struct at91_twi_dev *dev);
 
+void at91_twi_prepare_recovery(struct i2c_adapter *adap);
+void at91_twi_unprepare_recovery(struct i2c_adapter *adap);
+void at91_twi_init_recovery_info(struct at91_twi_dev *dev);
+
 #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
 void at91_init_twi_bus_slave(struct at91_twi_dev *dev);
 int at91_twi_probe_slave(struct platform_device *pdev, u32 phy_addr,
-- 
2.23.0

[PATCH 2/4] i2c: at91: implement i2c bus recovery

[Kamel Bouhara]

Implement i2c bus recovery when slaves devices might hold SDA low.
In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
until the slave release SDA.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 drivers/i2c/busses/i2c-at91-master.c | 63 ++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-at91.h        |  8 ++++
 2 files changed, 71 insertions(+)

diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
index a3fcc35ffd3b..df5bb93f952d 100644
--- a/drivers/i2c/busses/i2c-at91-master.c
+++ b/drivers/i2c/busses/i2c-at91-master.c
@@ -18,11 +18,13 @@
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/dma-atmel.h>
 #include <linux/pm_runtime.h>
@@ -768,6 +770,63 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
 	return ret;
 }
 
+static void at91_prepare_twi_recovery(struct i2c_adapter *adap)
+{
+	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+
+	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
+}
+
+static void at91_unprepare_twi_recovery(struct i2c_adapter *adap)
+{
+	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+
+	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
+}
+
+static int at91_init_twi_recovery_info(struct platform_device *pdev,
+				       struct at91_twi_dev *dev)
+{
+	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
+
+	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
+		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
+		return PTR_ERR(dev->pinctrl);
+	}
+
+	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
+							 PINCTRL_STATE_DEFAULT);
+	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
+						      "gpio");
+	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
+	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl",
+					  GPIOD_OUT_HIGH_OPEN_DRAIN);
+	if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (IS_ERR(rinfo->sda_gpiod) ||
+		   IS_ERR(rinfo->scl_gpiod) ||
+		   IS_ERR(dev->pinctrl_pins_default) ||
+		   IS_ERR(dev->pinctrl_pins_gpio)) {
+		dev_info(&pdev->dev, "recovery information incomplete\n");
+		return -EINVAL;
+	}
+
+	dev_info(&pdev->dev, "using scl%s for recovery\n",
+		 rinfo->sda_gpiod ? ",sda" : "");
+
+	rinfo->prepare_recovery = at91_prepare_twi_recovery;
+	rinfo->unprepare_recovery = at91_unprepare_twi_recovery;
+	rinfo->recover_bus = i2c_generic_scl_recovery;
+	dev->adapter.bus_recovery_info = rinfo;
+
+	return 0;
+}
+
 int at91_twi_probe_master(struct platform_device *pdev,
 			  u32 phy_addr, struct at91_twi_dev *dev)
 {
@@ -795,6 +854,10 @@ int at91_twi_probe_master(struct platform_device *pdev,
 
 	at91_calc_twi_clock(dev);
 
+	rc = at91_init_twi_recovery_info(pdev, dev);
+	if (rc == -EPROBE_DEFER)
+		return rc;
+
 	dev->adapter.algo = &at91_twi_algorithm;
 	dev->adapter.quirks = &at91_twi_quirks;
 
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
index 499b506f6128..b89dab55e776 100644
--- a/drivers/i2c/busses/i2c-at91.h
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -141,6 +141,10 @@ struct at91_twi_dev {
 	u32 fifo_size;
 	struct at91_twi_dma dma;
 	bool slave_detected;
+	struct i2c_bus_recovery_info rinfo;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pinctrl_pins_default;
+	struct pinctrl_state *pinctrl_pins_gpio;
 #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
 	unsigned smr;
 	struct i2c_client *slave;
@@ -158,6 +162,10 @@ void at91_init_twi_bus_master(struct at91_twi_dev *dev);
 int at91_twi_probe_master(struct platform_device *pdev, u32 phy_addr,
 			  struct at91_twi_dev *dev);
 
+void at91_twi_prepare_recovery(struct i2c_adapter *adap);
+void at91_twi_unprepare_recovery(struct i2c_adapter *adap);
+void at91_twi_init_recovery_info(struct at91_twi_dev *dev);
+
 #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
 void at91_init_twi_bus_slave(struct at91_twi_dev *dev);
 int at91_twi_probe_slave(struct platform_device *pdev, u32 phy_addr,
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Claudiu.Beznea]

Hi Kamel,

On 02.10.2019 17:46, Kamel Bouhara wrote:
> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> +				       struct at91_twi_dev *dev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {

You may use IS_ERR_OR_NULL() here.

> +		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
> +		return PTR_ERR(dev->pinctrl);
> +	}
> +

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Claudiu.Beznea]

Hi Kamel,

On 02.10.2019 17:46, Kamel Bouhara wrote:
> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> +				       struct at91_twi_dev *dev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {

You may use IS_ERR_OR_NULL() here.

> +		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
> +		return PTR_ERR(dev->pinctrl);
> +	}
> +
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Claudiu.Beznea]

Hi Kamel,

On 02.10.2019 17:46, Kamel Bouhara wrote:
> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> +				       struct at91_twi_dev *dev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {

You may use IS_ERR_OR_NULL() here.

> +		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
> +		return PTR_ERR(dev->pinctrl);
> +	}
> +

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Uwe Kleine-König]

On Fri, Oct 04, 2019 at 09:35:23AM +0000, Claudiu.Beznea@microchip.com wrote:
> Hi Kamel,
> 
> On 02.10.2019 17:46, Kamel Bouhara wrote:
> > +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> > +				       struct at91_twi_dev *dev)
> > +{
> > +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> > +
> > +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
> 
> You may use IS_ERR_OR_NULL() here.

Can devm_pinctrl_get return NULL? From a quick look, it cannot.

rule of thumb: IS_ERR_OR_NULL is wrong as it is a sign of poor return
value semantics.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Uwe Kleine-König]

On Fri, Oct 04, 2019 at 09:35:23AM +0000, Claudiu.Beznea@microchip.com wrote:
> Hi Kamel,
> 
> On 02.10.2019 17:46, Kamel Bouhara wrote:
> > +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> > +				       struct at91_twi_dev *dev)
> > +{
> > +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> > +
> > +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
> 
> You may use IS_ERR_OR_NULL() here.

Can devm_pinctrl_get return NULL? From a quick look, it cannot.

rule of thumb: IS_ERR_OR_NULL is wrong as it is a sign of poor return
value semantics.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Claudiu.Beznea]

On 04.10.2019 23:39, Uwe Kleine-König wrote:
> External E-Mail
> 
> 
> On Fri, Oct 04, 2019 at 09:35:23AM +0000, Claudiu.Beznea@microchip.com wrote:
>> Hi Kamel,
>>
>> On 02.10.2019 17:46, Kamel Bouhara wrote:
>>> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
>>> +				       struct at91_twi_dev *dev)
>>> +{
>>> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
>>> +
>>> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
>>> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
>>
>> You may use IS_ERR_OR_NULL() here.
> 
> Can devm_pinctrl_get return NULL? From a quick look, it cannot.

Looking quickly though it, yes, it seems it can't.

> 
> rule of thumb: IS_ERR_OR_NULL is wrong as it is a sign of poor return
> value semantics.
> 
> Best regards
> Uwe
> 

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Claudiu.Beznea]

On 04.10.2019 23:39, Uwe Kleine-König wrote:
> External E-Mail
> 
> 
> On Fri, Oct 04, 2019 at 09:35:23AM +0000, Claudiu.Beznea@microchip.com wrote:
>> Hi Kamel,
>>
>> On 02.10.2019 17:46, Kamel Bouhara wrote:
>>> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
>>> +				       struct at91_twi_dev *dev)
>>> +{
>>> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
>>> +
>>> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
>>> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
>>
>> You may use IS_ERR_OR_NULL() here.
> 
> Can devm_pinctrl_get return NULL? From a quick look, it cannot.

Looking quickly though it, yes, it seems it can't.

> 
> rule of thumb: IS_ERR_OR_NULL is wrong as it is a sign of poor return
> value semantics.
> 
> Best regards
> Uwe
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Claudiu.Beznea]

On 04.10.2019 23:39, Uwe Kleine-König wrote:
> External E-Mail
> 
> 
> On Fri, Oct 04, 2019 at 09:35:23AM +0000, Claudiu.Beznea@microchip.com wrote:
>> Hi Kamel,
>>
>> On 02.10.2019 17:46, Kamel Bouhara wrote:
>>> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
>>> +				       struct at91_twi_dev *dev)
>>> +{
>>> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
>>> +
>>> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
>>> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
>>
>> You may use IS_ERR_OR_NULL() here.
> 
> Can devm_pinctrl_get return NULL? From a quick look, it cannot.

Looking quickly though it, yes, it seems it can't.

> 
> rule of thumb: IS_ERR_OR_NULL is wrong as it is a sign of poor return
> value semantics.
> 
> Best regards
> Uwe
> 

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Ludovic Desroches]

On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> External E-Mail
> 
> 
> Implement i2c bus recovery when slaves devices might hold SDA low.
> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> until the slave release SDA.
> 

Hi Kamel,

Thanks for adding this new feature. As I see patches only for sama5d3 and
sama5d4, I assume it has not been tested with a sama5d2, isn't it?

I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
work if we add .strict = true to pinmux_ops which is something plan for the
future...

Are you able to test these points? It would be nice to be aware of
possible side effects.

Regards

Ludovic

> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
>  drivers/i2c/busses/i2c-at91-master.c | 63 ++++++++++++++++++++++++++++
>  drivers/i2c/busses/i2c-at91.h        |  8 ++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index a3fcc35ffd3b..df5bb93f952d 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -18,11 +18,13 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/dmaengine.h>
>  #include <linux/err.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/dma-atmel.h>
>  #include <linux/pm_runtime.h>
> @@ -768,6 +770,63 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
>  	return ret;
>  }
>  
> +static void at91_prepare_twi_recovery(struct i2c_adapter *adap)
> +{
> +	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
> +}
> +
> +static void at91_unprepare_twi_recovery(struct i2c_adapter *adap)
> +{
> +	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
> +}
> +
> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> +				       struct at91_twi_dev *dev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
> +		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
> +		return PTR_ERR(dev->pinctrl);
> +	}
> +
> +	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
> +							 PINCTRL_STATE_DEFAULT);
> +	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
> +						      "gpio");
> +	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
> +	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl",
> +					  GPIOD_OUT_HIGH_OPEN_DRAIN);
> +	if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (IS_ERR(rinfo->sda_gpiod) ||
> +		   IS_ERR(rinfo->scl_gpiod) ||
> +		   IS_ERR(dev->pinctrl_pins_default) ||
> +		   IS_ERR(dev->pinctrl_pins_gpio)) {
> +		dev_info(&pdev->dev, "recovery information incomplete\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_info(&pdev->dev, "using scl%s for recovery\n",
> +		 rinfo->sda_gpiod ? ",sda" : "");
> +
> +	rinfo->prepare_recovery = at91_prepare_twi_recovery;
> +	rinfo->unprepare_recovery = at91_unprepare_twi_recovery;
> +	rinfo->recover_bus = i2c_generic_scl_recovery;
> +	dev->adapter.bus_recovery_info = rinfo;
> +
> +	return 0;
> +}
> +
>  int at91_twi_probe_master(struct platform_device *pdev,
>  			  u32 phy_addr, struct at91_twi_dev *dev)
>  {
> @@ -795,6 +854,10 @@ int at91_twi_probe_master(struct platform_device *pdev,
>  
>  	at91_calc_twi_clock(dev);
>  
> +	rc = at91_init_twi_recovery_info(pdev, dev);
> +	if (rc == -EPROBE_DEFER)
> +		return rc;
> +
>  	dev->adapter.algo = &at91_twi_algorithm;
>  	dev->adapter.quirks = &at91_twi_quirks;
>  
> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> index 499b506f6128..b89dab55e776 100644
> --- a/drivers/i2c/busses/i2c-at91.h
> +++ b/drivers/i2c/busses/i2c-at91.h
> @@ -141,6 +141,10 @@ struct at91_twi_dev {
>  	u32 fifo_size;
>  	struct at91_twi_dma dma;
>  	bool slave_detected;
> +	struct i2c_bus_recovery_info rinfo;
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *pinctrl_pins_default;
> +	struct pinctrl_state *pinctrl_pins_gpio;
>  #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
>  	unsigned smr;
>  	struct i2c_client *slave;
> @@ -158,6 +162,10 @@ void at91_init_twi_bus_master(struct at91_twi_dev *dev);
>  int at91_twi_probe_master(struct platform_device *pdev, u32 phy_addr,
>  			  struct at91_twi_dev *dev);
>  
> +void at91_twi_prepare_recovery(struct i2c_adapter *adap);
> +void at91_twi_unprepare_recovery(struct i2c_adapter *adap);
> +void at91_twi_init_recovery_info(struct at91_twi_dev *dev);
> +
>  #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
>  void at91_init_twi_bus_slave(struct at91_twi_dev *dev);
>  int at91_twi_probe_slave(struct platform_device *pdev, u32 phy_addr,
> -- 
> 2.23.0
> 
> 

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Ludovic Desroches]

On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> External E-Mail
> 
> 
> Implement i2c bus recovery when slaves devices might hold SDA low.
> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> until the slave release SDA.
> 

Hi Kamel,

Thanks for adding this new feature. As I see patches only for sama5d3 and
sama5d4, I assume it has not been tested with a sama5d2, isn't it?

I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
work if we add .strict = true to pinmux_ops which is something plan for the
future...

Are you able to test these points? It would be nice to be aware of
possible side effects.

Regards

Ludovic

> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
>  drivers/i2c/busses/i2c-at91-master.c | 63 ++++++++++++++++++++++++++++
>  drivers/i2c/busses/i2c-at91.h        |  8 ++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index a3fcc35ffd3b..df5bb93f952d 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -18,11 +18,13 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/dmaengine.h>
>  #include <linux/err.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/dma-atmel.h>
>  #include <linux/pm_runtime.h>
> @@ -768,6 +770,63 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
>  	return ret;
>  }
>  
> +static void at91_prepare_twi_recovery(struct i2c_adapter *adap)
> +{
> +	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
> +}
> +
> +static void at91_unprepare_twi_recovery(struct i2c_adapter *adap)
> +{
> +	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
> +}
> +
> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> +				       struct at91_twi_dev *dev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
> +		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
> +		return PTR_ERR(dev->pinctrl);
> +	}
> +
> +	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
> +							 PINCTRL_STATE_DEFAULT);
> +	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
> +						      "gpio");
> +	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
> +	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl",
> +					  GPIOD_OUT_HIGH_OPEN_DRAIN);
> +	if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (IS_ERR(rinfo->sda_gpiod) ||
> +		   IS_ERR(rinfo->scl_gpiod) ||
> +		   IS_ERR(dev->pinctrl_pins_default) ||
> +		   IS_ERR(dev->pinctrl_pins_gpio)) {
> +		dev_info(&pdev->dev, "recovery information incomplete\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_info(&pdev->dev, "using scl%s for recovery\n",
> +		 rinfo->sda_gpiod ? ",sda" : "");
> +
> +	rinfo->prepare_recovery = at91_prepare_twi_recovery;
> +	rinfo->unprepare_recovery = at91_unprepare_twi_recovery;
> +	rinfo->recover_bus = i2c_generic_scl_recovery;
> +	dev->adapter.bus_recovery_info = rinfo;
> +
> +	return 0;
> +}
> +
>  int at91_twi_probe_master(struct platform_device *pdev,
>  			  u32 phy_addr, struct at91_twi_dev *dev)
>  {
> @@ -795,6 +854,10 @@ int at91_twi_probe_master(struct platform_device *pdev,
>  
>  	at91_calc_twi_clock(dev);
>  
> +	rc = at91_init_twi_recovery_info(pdev, dev);
> +	if (rc == -EPROBE_DEFER)
> +		return rc;
> +
>  	dev->adapter.algo = &at91_twi_algorithm;
>  	dev->adapter.quirks = &at91_twi_quirks;
>  
> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> index 499b506f6128..b89dab55e776 100644
> --- a/drivers/i2c/busses/i2c-at91.h
> +++ b/drivers/i2c/busses/i2c-at91.h
> @@ -141,6 +141,10 @@ struct at91_twi_dev {
>  	u32 fifo_size;
>  	struct at91_twi_dma dma;
>  	bool slave_detected;
> +	struct i2c_bus_recovery_info rinfo;
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *pinctrl_pins_default;
> +	struct pinctrl_state *pinctrl_pins_gpio;
>  #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
>  	unsigned smr;
>  	struct i2c_client *slave;
> @@ -158,6 +162,10 @@ void at91_init_twi_bus_master(struct at91_twi_dev *dev);
>  int at91_twi_probe_master(struct platform_device *pdev, u32 phy_addr,
>  			  struct at91_twi_dev *dev);
>  
> +void at91_twi_prepare_recovery(struct i2c_adapter *adap);
> +void at91_twi_unprepare_recovery(struct i2c_adapter *adap);
> +void at91_twi_init_recovery_info(struct at91_twi_dev *dev);
> +
>  #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
>  void at91_init_twi_bus_slave(struct at91_twi_dev *dev);
>  int at91_twi_probe_slave(struct platform_device *pdev, u32 phy_addr,
> -- 
> 2.23.0
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Ludovic Desroches]

On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> External E-Mail
> 
> 
> Implement i2c bus recovery when slaves devices might hold SDA low.
> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> until the slave release SDA.
> 

Hi Kamel,

Thanks for adding this new feature. As I see patches only for sama5d3 and
sama5d4, I assume it has not been tested with a sama5d2, isn't it?

I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
work if we add .strict = true to pinmux_ops which is something plan for the
future...

Are you able to test these points? It would be nice to be aware of
possible side effects.

Regards

Ludovic

> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
>  drivers/i2c/busses/i2c-at91-master.c | 63 ++++++++++++++++++++++++++++
>  drivers/i2c/busses/i2c-at91.h        |  8 ++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index a3fcc35ffd3b..df5bb93f952d 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -18,11 +18,13 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/dmaengine.h>
>  #include <linux/err.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/dma-atmel.h>
>  #include <linux/pm_runtime.h>
> @@ -768,6 +770,63 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
>  	return ret;
>  }
>  
> +static void at91_prepare_twi_recovery(struct i2c_adapter *adap)
> +{
> +	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
> +}
> +
> +static void at91_unprepare_twi_recovery(struct i2c_adapter *adap)
> +{
> +	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
> +}
> +
> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> +				       struct at91_twi_dev *dev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
> +		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
> +		return PTR_ERR(dev->pinctrl);
> +	}
> +
> +	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
> +							 PINCTRL_STATE_DEFAULT);
> +	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
> +						      "gpio");
> +	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
> +	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl",
> +					  GPIOD_OUT_HIGH_OPEN_DRAIN);
> +	if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (IS_ERR(rinfo->sda_gpiod) ||
> +		   IS_ERR(rinfo->scl_gpiod) ||
> +		   IS_ERR(dev->pinctrl_pins_default) ||
> +		   IS_ERR(dev->pinctrl_pins_gpio)) {
> +		dev_info(&pdev->dev, "recovery information incomplete\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_info(&pdev->dev, "using scl%s for recovery\n",
> +		 rinfo->sda_gpiod ? ",sda" : "");
> +
> +	rinfo->prepare_recovery = at91_prepare_twi_recovery;
> +	rinfo->unprepare_recovery = at91_unprepare_twi_recovery;
> +	rinfo->recover_bus = i2c_generic_scl_recovery;
> +	dev->adapter.bus_recovery_info = rinfo;
> +
> +	return 0;
> +}
> +
>  int at91_twi_probe_master(struct platform_device *pdev,
>  			  u32 phy_addr, struct at91_twi_dev *dev)
>  {
> @@ -795,6 +854,10 @@ int at91_twi_probe_master(struct platform_device *pdev,
>  
>  	at91_calc_twi_clock(dev);
>  
> +	rc = at91_init_twi_recovery_info(pdev, dev);
> +	if (rc == -EPROBE_DEFER)
> +		return rc;
> +
>  	dev->adapter.algo = &at91_twi_algorithm;
>  	dev->adapter.quirks = &at91_twi_quirks;
>  
> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> index 499b506f6128..b89dab55e776 100644
> --- a/drivers/i2c/busses/i2c-at91.h
> +++ b/drivers/i2c/busses/i2c-at91.h
> @@ -141,6 +141,10 @@ struct at91_twi_dev {
>  	u32 fifo_size;
>  	struct at91_twi_dma dma;
>  	bool slave_detected;
> +	struct i2c_bus_recovery_info rinfo;
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *pinctrl_pins_default;
> +	struct pinctrl_state *pinctrl_pins_gpio;
>  #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
>  	unsigned smr;
>  	struct i2c_client *slave;
> @@ -158,6 +162,10 @@ void at91_init_twi_bus_master(struct at91_twi_dev *dev);
>  int at91_twi_probe_master(struct platform_device *pdev, u32 phy_addr,
>  			  struct at91_twi_dev *dev);
>  
> +void at91_twi_prepare_recovery(struct i2c_adapter *adap);
> +void at91_twi_unprepare_recovery(struct i2c_adapter *adap);
> +void at91_twi_init_recovery_info(struct at91_twi_dev *dev);
> +
>  #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
>  void at91_init_twi_bus_slave(struct at91_twi_dev *dev);
>  int at91_twi_probe_slave(struct platform_device *pdev, u32 phy_addr,
> -- 
> 2.23.0
> 
> 

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Alexandre Belloni]

On 09/10/2019 15:55:00+0200, Ludovic Desroches wrote:
> On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> > External E-Mail
> > 
> > 
> > Implement i2c bus recovery when slaves devices might hold SDA low.
> > In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> > until the slave release SDA.
> > 
> 
> Hi Kamel,
> 
> Thanks for adding this new feature. As I see patches only for sama5d3 and
> sama5d4, I assume it has not been tested with a sama5d2, isn't it?
> 

I there a point having it on sama5d2 as the controller already supports
this feature?

> I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
> work if we add .strict = true to pinmux_ops which is something plan for the
> future...
> 

I don't see why it wouldn't work with strict as this is switching muxing
properly instead of using the pins for two functions at the same time.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Alexandre Belloni]

On 09/10/2019 15:55:00+0200, Ludovic Desroches wrote:
> On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> > External E-Mail
> > 
> > 
> > Implement i2c bus recovery when slaves devices might hold SDA low.
> > In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> > until the slave release SDA.
> > 
> 
> Hi Kamel,
> 
> Thanks for adding this new feature. As I see patches only for sama5d3 and
> sama5d4, I assume it has not been tested with a sama5d2, isn't it?
> 

I there a point having it on sama5d2 as the controller already supports
this feature?

> I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
> work if we add .strict = true to pinmux_ops which is something plan for the
> future...
> 

I don't see why it wouldn't work with strict as this is switching muxing
properly instead of using the pins for two functions at the same time.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Ludovic Desroches]

On Wed, Oct 09, 2019 at 04:01:47PM +0200, Alexandre Belloni wrote:
> 
> On 09/10/2019 15:55:00+0200, Ludovic Desroches wrote:
> > On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> > > External E-Mail
> > > 
> > > 
> > > Implement i2c bus recovery when slaves devices might hold SDA low.
> > > In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> > > until the slave release SDA.
> > > 
> > 
> > Hi Kamel,
> > 
> > Thanks for adding this new feature. As I see patches only for sama5d3 and
> > sama5d4, I assume it has not been tested with a sama5d2, isn't it?
> > 
> 
> I there a point having it on sama5d2 as the controller already supports
> this feature?
> 

Right, I was focused on pinctrl and forget we have this feature
supported by the IP.

> > I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
> > work if we add .strict = true to pinmux_ops which is something plan for the
> > future...
> > 
> 
> I don't see why it wouldn't work with strict as this is switching muxing
> properly instead of using the pins for two functions at the same time.
> 

Not sure devm_gpiod_get won't fail with strict.

Ludovic

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Ludovic Desroches]

On Wed, Oct 09, 2019 at 04:01:47PM +0200, Alexandre Belloni wrote:
> 
> On 09/10/2019 15:55:00+0200, Ludovic Desroches wrote:
> > On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> > > External E-Mail
> > > 
> > > 
> > > Implement i2c bus recovery when slaves devices might hold SDA low.
> > > In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> > > until the slave release SDA.
> > > 
> > 
> > Hi Kamel,
> > 
> > Thanks for adding this new feature. As I see patches only for sama5d3 and
> > sama5d4, I assume it has not been tested with a sama5d2, isn't it?
> > 
> 
> I there a point having it on sama5d2 as the controller already supports
> this feature?
> 

Right, I was focused on pinctrl and forget we have this feature
supported by the IP.

> > I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
> > work if we add .strict = true to pinmux_ops which is something plan for the
> > future...
> > 
> 
> I don't see why it wouldn't work with strict as this is switching muxing
> properly instead of using the pins for two functions at the same time.
> 

Not sure devm_gpiod_get won't fail with strict.

Ludovic

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Ludovic Desroches]

On Wed, Oct 09, 2019 at 04:01:47PM +0200, Alexandre Belloni wrote:
> 
> On 09/10/2019 15:55:00+0200, Ludovic Desroches wrote:
> > On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> > > External E-Mail
> > > 
> > > 
> > > Implement i2c bus recovery when slaves devices might hold SDA low.
> > > In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> > > until the slave release SDA.
> > > 
> > 
> > Hi Kamel,
> > 
> > Thanks for adding this new feature. As I see patches only for sama5d3 and
> > sama5d4, I assume it has not been tested with a sama5d2, isn't it?
> > 
> 
> I there a point having it on sama5d2 as the controller already supports
> this feature?
> 

Right, I was focused on pinctrl and forget we have this feature
supported by the IP.

> > I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
> > work if we add .strict = true to pinmux_ops which is something plan for the
> > future...
> > 
> 
> I don't see why it wouldn't work with strict as this is switching muxing
> properly instead of using the pins for two functions at the same time.
> 

Not sure devm_gpiod_get won't fail with strict.

Ludovic

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Kamel Bouhara]

On 10/10/2019 08:54, Ludovic Desroches wrote:
> On Wed, Oct 09, 2019 at 04:01:47PM +0200, Alexandre Belloni wrote:
>>
>> On 09/10/2019 15:55:00+0200, Ludovic Desroches wrote:
>>> On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
>>>> External E-Mail
>>>>
>>>>
>>>> Implement i2c bus recovery when slaves devices might hold SDA low.
>>>> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
>>>> until the slave release SDA.
>>>>
>>>
>>> Hi Kamel,
>>>
Hi Ludovic,

>>> Thanks for adding this new feature. As I see patches only for sama5d3 and
>>> sama5d4, I assume it has not been tested with a sama5d2, isn't it?
>>>
>>
>> I there a point having it on sama5d2 as the controller already supports
>> this feature?
>>
> 
> Right, I was focused on pinctrl and forget we have this feature
> supported by the IP.
> 
>>> I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
>>> work if we add .strict = true to pinmux_ops which is something plan for the
>>> future...
>>>
>>
>> I don't see why it wouldn't work with strict as this is switching muxing
>> properly instead of using the pins for two functions at the same time.
>>
> 
> Not sure devm_gpiod_get won't fail with strict.
> 
Actually the strict flag is checked in the pinmux core to allow the pin 
request.

What is the purpose to enable it ? It seems to break a lot things, eg. 
on sama5d3 :

# dmesg |grep pin
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioA18 already requested by 
f801c000.i2c; cannot claim for fffff200.gpio:18
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-18 (fffff200.gpio:18) status -22
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioA19 already requested by 
f801c000.i2c; cannot claim for fffff200.gpio:19
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-19 (fffff200.gpio:19) status -22
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioE9 already requested by 
500000.gadget; cannot claim for fffffa00.gpio:137
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-137 (fffffa00.gpio:137) 
status -22
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioE0 already requested by 
f0000000.mmc; cannot claim for fffffa00.gpio:128
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-128 (fffffa00.gpio:128) 
status -22
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioE1 already requested by 
f8000000.mmc; cannot claim for fffffa00.gpio:129
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-129 (fffffa00.gpio:129) 
status -22
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioE29 already requested by 
gpio_keys; cannot claim for fffffa00.gpio:157
pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-157 (fffffa00.gpio:157) 
status -22

> Ludovic
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Ludovic.Desroches]

On Thu, Oct 24, 2019 at 02:29:13PM +0200, Kamel Bouhara wrote:
> 
> On 10/10/2019 08:54, Ludovic Desroches wrote:
> > On Wed, Oct 09, 2019 at 04:01:47PM +0200, Alexandre Belloni wrote:
> > > 
> > > On 09/10/2019 15:55:00+0200, Ludovic Desroches wrote:
> > > > On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> > > > > External E-Mail
> > > > > 
> > > > > 
> > > > > Implement i2c bus recovery when slaves devices might hold SDA low.
> > > > > In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> > > > > until the slave release SDA.
> > > > > 
> > > > 
> > > > Hi Kamel,
> > > > 
> Hi Ludovic,
> 
> > > > Thanks for adding this new feature. As I see patches only for sama5d3 and
> > > > sama5d4, I assume it has not been tested with a sama5d2, isn't it?
> > > > 
> > > 
> > > I there a point having it on sama5d2 as the controller already supports
> > > this feature?
> > > 
> > 
> > Right, I was focused on pinctrl and forget we have this feature
> > supported by the IP.
> > 
> > > > I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
> > > > work if we add .strict = true to pinmux_ops which is something plan for the
> > > > future...
> > > > 
> > > 
> > > I don't see why it wouldn't work with strict as this is switching muxing
> > > properly instead of using the pins for two functions at the same time.
> > > 
> > 
> > Not sure devm_gpiod_get won't fail with strict.
> > 
> Actually the strict flag is checked in the pinmux core to allow the pin
> request.
> 
> What is the purpose to enable it ? It seems to break a lot things, eg. on
> sama5d3 :

Hi Kamel,

First, to be clear, I am not against this patch, I'll ack the new
version. My goal is only to be aware if the use of strict can have side
effects.

I am more used to the pio4 but I assume it's the same with the older
one. As you notice enabling it should break many things. It involves DT
changes for pins used as gpio. They have to be removed from the
pinmuxing or to find a way to allow a gpio request on a pin muxed as a
gpio.

I would like to enable it, because, at the moment, if you request a gpio,
for example using the sysfs, you can change the muxing of the pin and
breaks a device using it. If strict is enabled, this change will be
rejected and it's probably safer.

> 
> # dmesg |grep pin
> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioA18 already requested by
> f801c000.i2c; cannot claim for fffff200.gpio:18
> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-18 (fffff200.gpio:18) status -22
> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioA19 already requested by
> f801c000.i2c; cannot claim for fffff200.gpio:19
> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-19 (fffff200.gpio:19) status -22

Thanks for testing it, it confirms what I had in mind.

Regards

Ludovic

> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioE9 already requested by
> 500000.gadget; cannot claim for fffffa00.gpio:137
> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-137 (fffffa00.gpio:137) status
> -22
> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioE0 already requested by
> f0000000.mmc; cannot claim for fffffa00.gpio:128
> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-128 (fffffa00.gpio:128) status
> -22
> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioE1 already requested by
> f8000000.mmc; cannot claim for fffffa00.gpio:129
> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-129 (fffffa00.gpio:129) status
> -22
> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin pioE29 already requested by
> gpio_keys; cannot claim for fffffa00.gpio:157
> pinctrl-at91 ahb:apb:pinctrl@fffff200: pin-157 (fffffa00.gpio:157) status
> -22
> 
> > Ludovic
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> -- 
> Kamel Bouhara, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> Implement i2c bus recovery when slaves devices might hold SDA low.
> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> until the slave release SDA.
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>

Setting up the bus_recovery looks OK. However, I don't see any call to
i2c_recover_bus(), so the bus_recovery is never used. Did you test this
and see an effect?

Also, I think we should merge this patch "[PATCH v3] i2c: at91: Send bus
clear command if SCL or SDA is down" into this series. The crucial thing
for both is when to apply the recovery (at the beginning of a
transfer!). The rest is "just" that some HW needs a bus_recovery_info
for pinctrl/GPIO handling (from this patch), while other HW needs a
bus_recovery_info with a custom recover_bus callback.

Opinions?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 894 bytes --]

On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> Implement i2c bus recovery when slaves devices might hold SDA low.
> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> until the slave release SDA.
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>

Setting up the bus_recovery looks OK. However, I don't see any call to
i2c_recover_bus(), so the bus_recovery is never used. Did you test this
and see an effect?

Also, I think we should merge this patch "[PATCH v3] i2c: at91: Send bus
clear command if SCL or SDA is down" into this series. The crucial thing
for both is when to apply the recovery (at the beginning of a
transfer!). The rest is "just" that some HW needs a bus_recovery_info
for pinctrl/GPIO handling (from this patch), while other HW needs a
bus_recovery_info with a custom recover_bus callback.

Opinions?


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Kamel Bouhara]

On 21/10/2019 22:20, Wolfram Sang wrote:
> On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
>> Implement i2c bus recovery when slaves devices might hold SDA low.
>> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
>> until the slave release SDA.
>>
>> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> 
> Setting up the bus_recovery looks OK. However, I don't see any call to
> i2c_recover_bus(), so the bus_recovery is never used. Did you test this
> and see an effect?
> 
Indeed, I guess I mess it up while doing some git stuff, it should be 
called from at91_do_twi_transfer() when the transfer times out...
I actually tested it and verified the recovery is triggered by pulling 
the SCL to the ground ...

> Also, I think we should merge this patch "[PATCH v3] i2c: at91: Send bus
> clear command if SCL or SDA is down" into this series. The crucial thing
> for both is when to apply the recovery (at the beginning of a
> transfer!). The rest is "just" that some HW needs a bus_recovery_info
> for pinctrl/GPIO handling (from this patch), while other HW needs a
> bus_recovery_info with a custom recover_bus callback.
> 
> Opinions?
> 
I'm OK to merge the two series.

> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Codrin.Ciubotariu]

On 22.10.2019 10:59, Kamel Bouhara wrote:
> On 21/10/2019 22:20, Wolfram Sang wrote:
>> On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
>>> Implement i2c bus recovery when slaves devices might hold SDA low.
>>> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
>>> until the slave release SDA.
>>>
>>> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
>>
>> Setting up the bus_recovery looks OK. However, I don't see any call to
>> i2c_recover_bus(), so the bus_recovery is never used. Did you test this
>> and see an effect?
>>
> Indeed, I guess I mess it up while doing some git stuff, it should be 
> called from at91_do_twi_transfer() when the transfer times out...
> I actually tested it and verified the recovery is triggered by pulling 
> the SCL to the ground ...
> 
>> Also, I think we should merge this patch "[PATCH v3] i2c: at91: Send bus
>> clear command if SCL or SDA is down" into this series. The crucial thing
>> for both is when to apply the recovery (at the beginning of a
>> transfer!). The rest is "just" that some HW needs a bus_recovery_info
>> for pinctrl/GPIO handling (from this patch), while other HW needs a
>> bus_recovery_info with a custom recover_bus callback.
>>
>> Opinions?
>>
> I'm OK to merge the two series.

So at the beginning of a new transfer, we should check if SDA (or SCL?) 
is low and, if it's true, only then we should try recover the bus.

Kamel, let me know if I can help with anything.

Best regards,
Codrin

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Codrin.Ciubotariu]

On 22.10.2019 10:59, Kamel Bouhara wrote:
> On 21/10/2019 22:20, Wolfram Sang wrote:
>> On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
>>> Implement i2c bus recovery when slaves devices might hold SDA low.
>>> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
>>> until the slave release SDA.
>>>
>>> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
>>
>> Setting up the bus_recovery looks OK. However, I don't see any call to
>> i2c_recover_bus(), so the bus_recovery is never used. Did you test this
>> and see an effect?
>>
> Indeed, I guess I mess it up while doing some git stuff, it should be 
> called from at91_do_twi_transfer() when the transfer times out...
> I actually tested it and verified the recovery is triggered by pulling 
> the SCL to the ground ...
> 
>> Also, I think we should merge this patch "[PATCH v3] i2c: at91: Send bus
>> clear command if SCL or SDA is down" into this series. The crucial thing
>> for both is when to apply the recovery (at the beginning of a
>> transfer!). The rest is "just" that some HW needs a bus_recovery_info
>> for pinctrl/GPIO handling (from this patch), while other HW needs a
>> bus_recovery_info with a custom recover_bus callback.
>>
>> Opinions?
>>
> I'm OK to merge the two series.

So at the beginning of a new transfer, we should check if SDA (or SCL?) 
is low and, if it's true, only then we should try recover the bus.

Kamel, let me know if I can help with anything.

Best regards,
Codrin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Codrin.Ciubotariu]

On 22.10.2019 10:59, Kamel Bouhara wrote:
> On 21/10/2019 22:20, Wolfram Sang wrote:
>> On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
>>> Implement i2c bus recovery when slaves devices might hold SDA low.
>>> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
>>> until the slave release SDA.
>>>
>>> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
>>
>> Setting up the bus_recovery looks OK. However, I don't see any call to
>> i2c_recover_bus(), so the bus_recovery is never used. Did you test this
>> and see an effect?
>>
> Indeed, I guess I mess it up while doing some git stuff, it should be 
> called from at91_do_twi_transfer() when the transfer times out...
> I actually tested it and verified the recovery is triggered by pulling 
> the SCL to the ground ...
> 
>> Also, I think we should merge this patch "[PATCH v3] i2c: at91: Send bus
>> clear command if SCL or SDA is down" into this series. The crucial thing
>> for both is when to apply the recovery (at the beginning of a
>> transfer!). The rest is "just" that some HW needs a bus_recovery_info
>> for pinctrl/GPIO handling (from this patch), while other HW needs a
>> bus_recovery_info with a custom recover_bus callback.
>>
>> Opinions?
>>
> I'm OK to merge the two series.

So at the beginning of a new transfer, we should check if SDA (or SCL?) 
is low and, if it's true, only then we should try recover the bus.

Kamel, let me know if I can help with anything.

Best regards,
Codrin

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 234 bytes --]


> So at the beginning of a new transfer, we should check if SDA (or SCL?) 
> is low and, if it's true, only then we should try recover the bus.

Yes, this is the proper time to do it. Remember, I2C does not define a
timeout.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 234 bytes --]


> So at the beginning of a new transfer, we should check if SDA (or SCL?) 
> is low and, if it's true, only then we should try recover the bus.

Yes, this is the proper time to do it. Remember, I2C does not define a
timeout.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Phil Reid]

On 24/10/2019 23:07, Wolfram Sang wrote:
> 
>> So at the beginning of a new transfer, we should check if SDA (or SCL?)
>> is low and, if it's true, only then we should try recover the bus.
> 
> Yes, this is the proper time to do it. Remember, I2C does not define a
> timeout.
> 

FYI: Just a single poll at the start of the transfer, for it being low, will cause problems with multi-master buses.
Bus recovery should be attempted after a timeout when trying to communicate, even thou i2c doesn't define a timeout.

I'm trying to fix the designware drivers handling of this at the moment.

-- 
Regards
Phil Reid

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Phil Reid]

On 24/10/2019 23:07, Wolfram Sang wrote:
> 
>> So at the beginning of a new transfer, we should check if SDA (or SCL?)
>> is low and, if it's true, only then we should try recover the bus.
> 
> Yes, this is the proper time to do it. Remember, I2C does not define a
> timeout.
> 

FYI: Just a single poll at the start of the transfer, for it being low, will cause problems with multi-master buses.
Bus recovery should be attempted after a timeout when trying to communicate, even thou i2c doesn't define a timeout.

I'm trying to fix the designware drivers handling of this at the moment.

-- 
Regards
Phil Reid


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Phil Reid]

On 24/10/2019 23:07, Wolfram Sang wrote:
> 
>> So at the beginning of a new transfer, we should check if SDA (or SCL?)
>> is low and, if it's true, only then we should try recover the bus.
> 
> Yes, this is the proper time to do it. Remember, I2C does not define a
> timeout.
> 

FYI: Just a single poll at the start of the transfer, for it being low, will cause problems with multi-master buses.
Bus recovery should be attempted after a timeout when trying to communicate, even thou i2c doesn't define a timeout.

I'm trying to fix the designware drivers handling of this at the moment.

-- 
Regards
Phil Reid

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

Hi Phil,

yes, this thread is old but a similar issue came up again...

On Fri, Oct 25, 2019 at 09:14:00AM +0800, Phil Reid wrote:

> > 
> > > So at the beginning of a new transfer, we should check if SDA (or SCL?)
> > > is low and, if it's true, only then we should try recover the bus.
> > 
> > Yes, this is the proper time to do it. Remember, I2C does not define a
> > timeout.
> > 
> 
> FYI: Just a single poll at the start of the transfer, for it being low, will cause problems with multi-master buses.
> Bus recovery should be attempted after a timeout when trying to communicate, even thou i2c doesn't define a timeout.
> 
> I'm trying to fix the designware drivers handling of this at the moment.

I wonder what you ended up with? You are right, a single poll is not
enough. It only might be if one applies the new "single-master" binding
for a given bus. If that is not present, my best idea so far is to poll
SDA for the time defined in adapter->timeout and if it is all low, then
initiate a recovery.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1069 bytes --]

Hi Phil,

yes, this thread is old but a similar issue came up again...

On Fri, Oct 25, 2019 at 09:14:00AM +0800, Phil Reid wrote:

> > 
> > > So at the beginning of a new transfer, we should check if SDA (or SCL?)
> > > is low and, if it's true, only then we should try recover the bus.
> > 
> > Yes, this is the proper time to do it. Remember, I2C does not define a
> > timeout.
> > 
> 
> FYI: Just a single poll at the start of the transfer, for it being low, will cause problems with multi-master buses.
> Bus recovery should be attempted after a timeout when trying to communicate, even thou i2c doesn't define a timeout.
> 
> I'm trying to fix the designware drivers handling of this at the moment.

I wonder what you ended up with? You are right, a single poll is not
enough. It only might be if one applies the new "single-master" binding
for a given bus. If that is not present, my best idea so far is to poll
SDA for the time defined in adapter->timeout and if it is all low, then
initiate a recovery.

All the best,

   Wolfram


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Phil Reid]

On 25/08/2020 21:28, Wolfram Sang wrote:
> Hi Phil,
> 
> yes, this thread is old but a similar issue came up again...
> 
> On Fri, Oct 25, 2019 at 09:14:00AM +0800, Phil Reid wrote:
> 
>>>
>>>> So at the beginning of a new transfer, we should check if SDA (or SCL?)
>>>> is low and, if it's true, only then we should try recover the bus.
>>>
>>> Yes, this is the proper time to do it. Remember, I2C does not define a
>>> timeout.
>>>
>>
>> FYI: Just a single poll at the start of the transfer, for it being low, will cause problems with multi-master buses.
>> Bus recovery should be attempted after a timeout when trying to communicate, even thou i2c doesn't define a timeout.
>>
>> I'm trying to fix the designware drivers handling of this at the moment.
> 
> I wonder what you ended up with? You are right, a single poll is not
> enough. It only might be if one applies the new "single-master" binding
> for a given bus. If that is not present, my best idea so far is to poll
> SDA for the time defined in adapter->timeout and if it is all low, then
> initiate a recovery.
> 

On my todo list still.

Our system eventually recovers at the moment and the multi-master bus
doesn't contain anything that's time critical to our systems operation.


-- 
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: preid@electromag.com.au

Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Phil Reid]

On 25/08/2020 21:28, Wolfram Sang wrote:
> Hi Phil,
> 
> yes, this thread is old but a similar issue came up again...
> 
> On Fri, Oct 25, 2019 at 09:14:00AM +0800, Phil Reid wrote:
> 
>>>
>>>> So at the beginning of a new transfer, we should check if SDA (or SCL?)
>>>> is low and, if it's true, only then we should try recover the bus.
>>>
>>> Yes, this is the proper time to do it. Remember, I2C does not define a
>>> timeout.
>>>
>>
>> FYI: Just a single poll at the start of the transfer, for it being low, will cause problems with multi-master buses.
>> Bus recovery should be attempted after a timeout when trying to communicate, even thou i2c doesn't define a timeout.
>>
>> I'm trying to fix the designware drivers handling of this at the moment.
> 
> I wonder what you ended up with? You are right, a single poll is not
> enough. It only might be if one applies the new "single-master" binding
> for a given bus. If that is not present, my best idea so far is to poll
> SDA for the time defined in adapter->timeout and if it is all low, then
> initiate a recovery.
> 

On my todo list still.

Our system eventually recovers at the moment and the multi-master bus
doesn't contain anything that's time critical to our systems operation.


-- 
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: preid@electromag.com.au

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] ARM: at91/dt: sama5d3: add i2c gpio pinctrl

[Kamel Bouhara]

Add the i2c gpio pinctrls to support the i2c bus recovery

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 arch/arm/boot/dts/sama5d3.dtsi | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index f770aace0efd..faf8907d8d7d 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -159,8 +159,11 @@
 				dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(7)>,
 				       <&dma0 2 AT91_DMA_CFG_PER_ID(8)>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c0>;
+				pinctrl-1 = <&pinctrl_i2c0_gpio>;
+				sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioA 31 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&twi0_clk>;
@@ -174,8 +177,11 @@
 				dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(9)>,
 				       <&dma0 2 AT91_DMA_CFG_PER_ID(10)>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c1>;
+				pinctrl-1 = <&pinctrl_i2c1_gpio>;
+				sda-gpios = <&pioC 26 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioC 27 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&twi1_clk>;
@@ -357,8 +363,11 @@
 				dmas = <&dma1 2 AT91_DMA_CFG_PER_ID(11)>,
 				       <&dma1 2 AT91_DMA_CFG_PER_ID(12)>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c2>;
+				pinctrl-1 = <&pinctrl_i2c2_gpio>;
+				sda-gpios = <&pioA 18 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioA 19 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&twi2_clk>;
@@ -639,6 +648,12 @@
 							<AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_NONE	/* PA30 periph A TWD0 pin, conflicts with URXD1, ISI_VSYNC */
 							 AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_NONE>;	/* PA31 periph A TWCK0 pin, conflicts with UTXD1, ISI_HSYNC */
 					};
+
+					pinctrl_i2c0_gpio: i2c0-gpio {
+						atmel,pins =
+							<AT91_PIOA 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				i2c1 {
@@ -647,6 +662,12 @@
 							<AT91_PIOC 26 AT91_PERIPH_B AT91_PINCTRL_NONE	/* PC26 periph B TWD1 pin, conflicts with SPI1_NPCS1, ISI_D11 */
 							 AT91_PIOC 27 AT91_PERIPH_B AT91_PINCTRL_NONE>;	/* PC27 periph B TWCK1 pin, conflicts with SPI1_NPCS2, ISI_D10 */
 					};
+
+					pinctrl_i2c1_gpio: i2c1-gpio {
+						atmel,pins =
+							<AT91_PIOC 26 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOC 27 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				i2c2 {
@@ -655,6 +676,12 @@
 							<AT91_PIOA 18 AT91_PERIPH_B AT91_PINCTRL_NONE	/* TWD2 pin, conflicts with LCDDAT18, ISI_D2 */
 							 AT91_PIOA 19 AT91_PERIPH_B AT91_PINCTRL_NONE>; /* TWCK2 pin, conflicts with LCDDAT19, ISI_D3 */
 					};
+
+					pinctrl_i2c2_gpio: i2c2-gpio {
+						atmel,pins =
+							<AT91_PIOA 18 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOA 19 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				isi {
-- 
2.23.0

[PATCH 3/4] ARM: at91/dt: sama5d3: add i2c gpio pinctrl

[Kamel Bouhara]

Add the i2c gpio pinctrls to support the i2c bus recovery

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 arch/arm/boot/dts/sama5d3.dtsi | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index f770aace0efd..faf8907d8d7d 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -159,8 +159,11 @@
 				dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(7)>,
 				       <&dma0 2 AT91_DMA_CFG_PER_ID(8)>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c0>;
+				pinctrl-1 = <&pinctrl_i2c0_gpio>;
+				sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioA 31 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&twi0_clk>;
@@ -174,8 +177,11 @@
 				dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(9)>,
 				       <&dma0 2 AT91_DMA_CFG_PER_ID(10)>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c1>;
+				pinctrl-1 = <&pinctrl_i2c1_gpio>;
+				sda-gpios = <&pioC 26 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioC 27 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&twi1_clk>;
@@ -357,8 +363,11 @@
 				dmas = <&dma1 2 AT91_DMA_CFG_PER_ID(11)>,
 				       <&dma1 2 AT91_DMA_CFG_PER_ID(12)>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c2>;
+				pinctrl-1 = <&pinctrl_i2c2_gpio>;
+				sda-gpios = <&pioA 18 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioA 19 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&twi2_clk>;
@@ -639,6 +648,12 @@
 							<AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_NONE	/* PA30 periph A TWD0 pin, conflicts with URXD1, ISI_VSYNC */
 							 AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_NONE>;	/* PA31 periph A TWCK0 pin, conflicts with UTXD1, ISI_HSYNC */
 					};
+
+					pinctrl_i2c0_gpio: i2c0-gpio {
+						atmel,pins =
+							<AT91_PIOA 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				i2c1 {
@@ -647,6 +662,12 @@
 							<AT91_PIOC 26 AT91_PERIPH_B AT91_PINCTRL_NONE	/* PC26 periph B TWD1 pin, conflicts with SPI1_NPCS1, ISI_D11 */
 							 AT91_PIOC 27 AT91_PERIPH_B AT91_PINCTRL_NONE>;	/* PC27 periph B TWCK1 pin, conflicts with SPI1_NPCS2, ISI_D10 */
 					};
+
+					pinctrl_i2c1_gpio: i2c1-gpio {
+						atmel,pins =
+							<AT91_PIOC 26 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOC 27 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				i2c2 {
@@ -655,6 +676,12 @@
 							<AT91_PIOA 18 AT91_PERIPH_B AT91_PINCTRL_NONE	/* TWD2 pin, conflicts with LCDDAT18, ISI_D2 */
 							 AT91_PIOA 19 AT91_PERIPH_B AT91_PINCTRL_NONE>; /* TWCK2 pin, conflicts with LCDDAT19, ISI_D3 */
 					};
+
+					pinctrl_i2c2_gpio: i2c2-gpio {
+						atmel,pins =
+							<AT91_PIOA 18 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOA 19 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				isi {
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] ARM: at91/dt: sama5d4: add i2c gpio pinctrl

[Kamel Bouhara]

Add the i2c gpio pinctrls so the i2c bus recovery option can be enabled

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 arch/arm/boot/dts/sama5d4.dtsi | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index 6ab27a7b388d..34351baab985 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -458,8 +458,11 @@
 					(AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1)
 					| AT91_XDMAC_DT_PERID(3))>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c0>;
+				pinctrl-1 = <&pinctrl_i2c0_gpio>;
+				sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioA 31 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&pmc PMC_TYPE_PERIPHERAL 32>;
@@ -477,8 +480,11 @@
 					(AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1)
 					| AT91_XDMAC_DT_PERID(5))>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c1>;
+				pinctrl-1 = <&pinctrl_i2c1_gpio>;
+				sda-gpios = <&pioE 29 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioE 30 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&pmc PMC_TYPE_PERIPHERAL 33>;
@@ -519,8 +525,11 @@
 					(AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1)
 					| AT91_XDMAC_DT_PERID(7))>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c2>;
+				pinctrl-1 = <&pinctrl_i2c2_gpio>;
+				sda-gpios = <&pioB 29 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioB 30 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&pmc PMC_TYPE_PERIPHERAL 34>;
@@ -1122,6 +1131,12 @@
 							<AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_NONE
 							 AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_NONE>;
 					};
+
+					pinctrl_i2c0_gpio: i2c0-gpio {
+						atmel,pins =
+							<AT91_PIOA 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				i2c1 {
@@ -1130,6 +1145,12 @@
 							<AT91_PIOE 29 AT91_PERIPH_C AT91_PINCTRL_NONE	/* TWD1, conflicts with UART0 RX and DIBP */
 							 AT91_PIOE 30 AT91_PERIPH_C AT91_PINCTRL_NONE>;	/* TWCK1, conflicts with UART0 TX and DIBN */
 					};
+
+					pinctrl_i2c1_gpio: i2c1-gpio {
+						atmel,pins =
+							<AT91_PIOE 29 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOE 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				i2c2 {
@@ -1138,6 +1159,12 @@
 							<AT91_PIOB 29 AT91_PERIPH_A AT91_PINCTRL_NONE	/* TWD2, conflicts with RD0 and PWML1 */
 							 AT91_PIOB 30 AT91_PERIPH_A AT91_PINCTRL_NONE>; /* TWCK2, conflicts with RF0 */
 					};
+
+					pinctrl_i2c2_gpio: i2c2-gpio {
+						atmel,pins =
+							<AT91_PIOB 29 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOB 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				isi {
-- 
2.23.0

[PATCH 4/4] ARM: at91/dt: sama5d4: add i2c gpio pinctrl

[Kamel Bouhara]

Add the i2c gpio pinctrls so the i2c bus recovery option can be enabled

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 arch/arm/boot/dts/sama5d4.dtsi | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index 6ab27a7b388d..34351baab985 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -458,8 +458,11 @@
 					(AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1)
 					| AT91_XDMAC_DT_PERID(3))>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c0>;
+				pinctrl-1 = <&pinctrl_i2c0_gpio>;
+				sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioA 31 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&pmc PMC_TYPE_PERIPHERAL 32>;
@@ -477,8 +480,11 @@
 					(AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1)
 					| AT91_XDMAC_DT_PERID(5))>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c1>;
+				pinctrl-1 = <&pinctrl_i2c1_gpio>;
+				sda-gpios = <&pioE 29 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioE 30 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&pmc PMC_TYPE_PERIPHERAL 33>;
@@ -519,8 +525,11 @@
 					(AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1)
 					| AT91_XDMAC_DT_PERID(7))>;
 				dma-names = "tx", "rx";
-				pinctrl-names = "default";
+				pinctrl-names = "default", "gpio";
 				pinctrl-0 = <&pinctrl_i2c2>;
+				pinctrl-1 = <&pinctrl_i2c2_gpio>;
+				sda-gpios = <&pioB 29 GPIO_ACTIVE_HIGH>;
+				scl-gpios = <&pioB 30 GPIO_ACTIVE_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				clocks = <&pmc PMC_TYPE_PERIPHERAL 34>;
@@ -1122,6 +1131,12 @@
 							<AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_NONE
 							 AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_NONE>;
 					};
+
+					pinctrl_i2c0_gpio: i2c0-gpio {
+						atmel,pins =
+							<AT91_PIOA 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				i2c1 {
@@ -1130,6 +1145,12 @@
 							<AT91_PIOE 29 AT91_PERIPH_C AT91_PINCTRL_NONE	/* TWD1, conflicts with UART0 RX and DIBP */
 							 AT91_PIOE 30 AT91_PERIPH_C AT91_PINCTRL_NONE>;	/* TWCK1, conflicts with UART0 TX and DIBN */
 					};
+
+					pinctrl_i2c1_gpio: i2c1-gpio {
+						atmel,pins =
+							<AT91_PIOE 29 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOE 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				i2c2 {
@@ -1138,6 +1159,12 @@
 							<AT91_PIOB 29 AT91_PERIPH_A AT91_PINCTRL_NONE	/* TWD2, conflicts with RD0 and PWML1 */
 							 AT91_PIOB 30 AT91_PERIPH_A AT91_PINCTRL_NONE>; /* TWCK2, conflicts with RF0 */
 					};
+
+					pinctrl_i2c2_gpio: i2c2-gpio {
+						atmel,pins =
+							<AT91_PIOB 29 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP
+							 AT91_PIOB 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				isi {
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] i2c bus recovery for Microchip SoCs.

[Rob Herring]

On Wed, Oct 02, 2019 at 04:46:54PM +0200, Kamel Bouhara wrote:
> This patch series introduce the kernel i2c-gpio bus recovery mechanism
> for the Microchip SoCs. Updated the corresponding dts to add i2c
> gpio pinctrl. The bus recovery is configured for the sama5d3/4 xplained
> boards in dts.

Now we have 2 drivers with the same binding and code for using GPIO for 
bus recovery. Perhaps all this should be common.

Rob

Re: [PATCH 0/4] i2c bus recovery for Microchip SoCs.

[Rob Herring]

On Wed, Oct 02, 2019 at 04:46:54PM +0200, Kamel Bouhara wrote:
> This patch series introduce the kernel i2c-gpio bus recovery mechanism
> for the Microchip SoCs. Updated the corresponding dts to add i2c
> gpio pinctrl. The bus recovery is configured for the sama5d3/4 xplained
> boards in dts.

Now we have 2 drivers with the same binding and code for using GPIO for 
bus recovery. Perhaps all this should be common.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel