GPIO recovery has been added already for some I2C bus drivers, such as
imx, pxa and at91. These drivers use similar bindings and have more or
less the same code for recovery. For this reason, we aim to move the
GPIO bus recovery implementation to the I2C core so that other drivers
can benefit from it, with small modifications.
This implementation initializes the pinctrl states and the SDA/SCL
GPIOs based on common bindings. The I2C bus drivers can still use
different bindings or other particular recovery steps if needed.
The ugly part with this patch series is the handle of PROBE_DEFER
which could be returned by devm_gpiod_get(). This changes things a
little for i2c_register_adapter() and for this reason this step is
implemented in a sperate patch.
The at91 Microchip driver is the first to use this implementation,
with an AI to move the rest of the drivers in the following steps.
This patch series was previously sent as a RFC. Significant changes
since RFC:
- "recovery" pinctrl state marked as deprecared in bindings;
- move to "gpio" pinctrl state done after the call to prepare_recovery()
callback;
- glitch protection when SDA gpio is taken at initialization;
Codrin Ciubotariu (4):
dt-binding: i2c: add generic properties for GPIO bus recovery
i2c: core: add generic I2C GPIO recovery
i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs
i2c: at91: Move to generic GPIO bus recovery
Documentation/devicetree/bindings/i2c/i2c.txt | 10 ++
drivers/i2c/busses/i2c-at91-master.c | 69 +-------
drivers/i2c/busses/i2c-at91.h | 3 -
drivers/i2c/i2c-core-base.c | 150 ++++++++++++++++--
include/linux/i2c.h | 11 ++
5 files changed, 163 insertions(+), 80 deletions(-)
--
2.25.1
Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return
-EPROBE_DEFER, so we should at least treat that. This ends up with
i2c_register_adapter() to be able to return -EPROBE_DEFER.
Signed-off-by: Codrin Ciubotariu <[email protected]>
---
Changes from RFC:
- return -EINVAL if i2c_init_recovery() doesn't have the complete
information;
- 'else if' added when checking if i2c_generic_scl_recovery() is used;
- moved i2c_init_recovery() before class-link creation; class-link
cleanup removed;
- moved debug print when the adapter is probed after call to
i2c_init_recovery();
drivers/i2c/i2c-core-base.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index cf0c5eb152e1..99dbaead269e 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -375,15 +375,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapter *adap)
return i2c_gpio_init_generic_recovery(adap);
}
-static void i2c_init_recovery(struct i2c_adapter *adap)
+static int i2c_init_recovery(struct i2c_adapter *adap)
{
struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
char *err_str;
if (!bri)
- return;
+ return 0;
- i2c_gpio_init_recovery(adap);
+ if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
if (!bri->recover_bus) {
err_str = "no recover_bus() found";
@@ -399,10 +400,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
if (gpiod_get_direction(bri->sda_gpiod) == 0)
bri->set_sda = set_sda_gpio_value;
}
- return;
- }
-
- if (bri->recover_bus == i2c_generic_scl_recovery) {
+ } else if (bri->recover_bus == i2c_generic_scl_recovery) {
/* Generic SCL recovery */
if (!bri->set_scl || !bri->get_scl) {
err_str = "no {get|set}_scl() found";
@@ -414,10 +412,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
}
}
- return;
+ return 0;
err:
dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
adap->bus_recovery_info = NULL;
+
+ return -EINVAL;
}
static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
@@ -1444,12 +1444,16 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
if (res)
goto out_reg;
- dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
-
pm_runtime_no_callbacks(&adap->dev);
pm_suspend_ignore_children(&adap->dev, true);
pm_runtime_enable(&adap->dev);
+ res = i2c_init_recovery(adap);
+ if (res == -EPROBE_DEFER)
+ goto out_reg;
+
+ dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
+
#ifdef CONFIG_I2C_COMPAT
res = class_compat_create_link(i2c_adapter_compat_class, &adap->dev,
adap->dev.parent);
@@ -1458,8 +1462,6 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
"Failed to create compatibility class link\n");
#endif
- i2c_init_recovery(adap);
-
/* create pre-declared device nodes */
of_i2c_register_devices(adap);
i2c_acpi_register_devices(adap);
--
2.25.1
The I2C GPIO bus recovery properties consist of two GPIOS and one extra
pinctrl state ("gpio" or "recovery"). "recovery" pinctrl state is
considered deprecated and "gpio" should be used instead.
Not all are mandatory for recovery.
Signed-off-by: Codrin Ciubotariu <[email protected]>
---
Changes from RFC:
- "recovery" pinctrl state marked as deprecated; updated description to
reflect this;
Documentation/devicetree/bindings/i2c/i2c.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index 438ae123107e..150a67da633d 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -77,6 +77,16 @@ wants to support one of the below features, it should adapt these bindings.
this information to detect a stalled bus more reliably, for example.
Can not be combined with 'multi-master'.
+- scl-gpios
+ specify the gpio related to SCL pin. Used for GPIO bus recovery.
+
+- sda-gpios
+ specify the gpio related to SDA pin. Optional for GPIO bus recovery.
+
+- pinctrl
+ add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
+ recovery, call it "gpio" or "recovery"(deprecated) state
+
Required properties (per child device)
--------------------------------------
--
2.25.1
Make the Microchip at91 driver the first to use the generic GPIO bus
recovery support from the I2C core and discard the driver implementation.
Signed-off-by: Codrin Ciubotariu <[email protected]>
---
Changes from RFC:
- none;
drivers/i2c/busses/i2c-at91-master.c | 69 ++--------------------------
drivers/i2c/busses/i2c-at91.h | 3 --
2 files changed, 3 insertions(+), 69 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
index 363d540a8345..66864f9cf7ac 100644
--- a/drivers/i2c/busses/i2c-at91-master.c
+++ b/drivers/i2c/busses/i2c-at91-master.c
@@ -816,79 +816,16 @@ 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_gpio(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)) {
+ rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
+ if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) {
dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
- return PTR_ERR(dev->pinctrl);
+ return PTR_ERR(rinfo->pinctrl);
}
-
- dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
- PINCTRL_STATE_DEFAULT);
- dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
- "gpio");
- if (IS_ERR(dev->pinctrl_pins_default) ||
- IS_ERR(dev->pinctrl_pins_gpio)) {
- dev_info(&pdev->dev, "pinctrl states incomplete for recovery\n");
- return -EINVAL;
- }
-
- /*
- * pins will be taken as GPIO, so we might as well inform pinctrl about
- * this and move the state to GPIO
- */
- pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_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)) {
- dev_info(&pdev->dev, "recovery information incomplete\n");
- if (!IS_ERR(rinfo->sda_gpiod)) {
- gpiod_put(rinfo->sda_gpiod);
- rinfo->sda_gpiod = NULL;
- }
- if (!IS_ERR(rinfo->scl_gpiod)) {
- gpiod_put(rinfo->scl_gpiod);
- rinfo->scl_gpiod = NULL;
- }
- pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
- return -EINVAL;
- }
-
- /* change the state of the pins back to their default state */
- pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
-
- dev_info(&pdev->dev, "using scl, sda for recovery\n");
-
- 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;
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
index 7e7b4955ca7f..eae673ae786c 100644
--- a/drivers/i2c/busses/i2c-at91.h
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -157,9 +157,6 @@ struct at91_twi_dev {
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;
--
2.25.1
On Tue, Aug 04, 2020 at 12:59:23PM +0300, Codrin Ciubotariu wrote:
> The I2C GPIO bus recovery properties consist of two GPIOS and one extra
> pinctrl state ("gpio" or "recovery"). "recovery" pinctrl state is
> considered deprecated and "gpio" should be used instead.
> Not all are mandatory for recovery.
>
> Signed-off-by: Codrin Ciubotariu <[email protected]>
Kept the alphabetical sorting, added a space before '(', took the
liberty to add Rob's review from last version (no significant change
IMO) and applied to for-next, thanks!
On Tue, Aug 04, 2020 at 12:59:25PM +0300, Codrin Ciubotariu wrote:
> Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return
> -EPROBE_DEFER, so we should at least treat that. This ends up with
> i2c_register_adapter() to be able to return -EPROBE_DEFER.
>
> Signed-off-by: Codrin Ciubotariu <[email protected]>
Applied to for-next, thanks!
On Tue, Aug 04, 2020 at 12:59:26PM +0300, Codrin Ciubotariu wrote:
> Make the Microchip at91 driver the first to use the generic GPIO bus
> recovery support from the I2C core and discard the driver implementation.
>
> Signed-off-by: Codrin Ciubotariu <[email protected]>
Applied to for-next, thanks!
Codrin, everyone
> This patch series was previously sent as a RFC. Significant changes
> since RFC:
> - "recovery" pinctrl state marked as deprecared in bindings;
> - move to "gpio" pinctrl state done after the call to prepare_recovery()
> callback;
> - glitch protection when SDA gpio is taken at initialization;
Thanks for the fast update, now all merged for inclusion into 5.9. I
think it is really good, but to verify and double check, I think two
things would be even better..
One thing, I'll definately be doing is to add this feature to
i2c-sh_mobile.c and scope the results.
The other thing would be to convert the PXA driver and see if our
generic support can help their advanced use case or if we are missing
something. Codrin, do you have maybe time and interest to do that? That
would be awesome!
Happy hacking and kind regards,
Wolfram
On 05.08.2020 11:52, [email protected] wrote:
> Codrin, everyone
>
>> This patch series was previously sent as a RFC. Significant changes
>> since RFC:
>> - "recovery" pinctrl state marked as deprecared in bindings;
>> - move to "gpio" pinctrl state done after the call to prepare_recovery()
>> callback;
>> - glitch protection when SDA gpio is taken at initialization;
>
> Thanks for the fast update, now all merged for inclusion into 5.9. I
> think it is really good, but to verify and double check, I think two
> things would be even better..
Happy to help.
>
> One thing, I'll definately be doing is to add this feature to
> i2c-sh_mobile.c and scope the results.
>
> The other thing would be to convert the PXA driver and see if our
> generic support can help their advanced use case or if we are missing
> something. Codrin, do you have maybe time and interest to do that? That
> would be awesome!
Naturally, these would be the next steps. I can do this, sure, but I
don't have the HW. I'll look for some development kits. If you have any
recommendations, please let me know.
Best regards,
Codrin
> > The other thing would be to convert the PXA driver and see if our
> > generic support can help their advanced use case or if we are missing
> > something. Codrin, do you have maybe time and interest to do that? That
> > would be awesome!
>
> Naturally, these would be the next steps. I can do this, sure, but I
> don't have the HW. I'll look for some development kits. If you have any
> recommendations, please let me know.
No need for HW, I think. Just do a best effort conversion, double or
triple check it, and CC the patches also to Russell King. If he accepts
them, we are good.
Thanks for doing it!