* [PATCH 1/6] i2c: make kerneldoc about bus recovery more precise
2017-12-04 12:36 ` [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar Wolfram Sang
@ 2017-12-04 12:36 ` Wolfram Sang
2017-12-04 12:36 ` [PATCH 2/6] i2c: add identifier in declarations for i2c_bus_recovery Wolfram Sang
` (6 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Wolfram Sang @ 2017-12-04 12:36 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, Wolfram Sang, Phil Reid, Andy Shevchenko,
Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
"Used internally" is vague. What it actually means is that those fields
are populated by the core if valid GPIOs are provided. Change the
comments to reflect that.
Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
include/linux/i2c.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 5d7f3c1853ae4b..f8a9d81e911e52 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -545,12 +545,12 @@ struct i2c_timings {
* @recover_bus: Recover routine. Either pass driver's recover_bus() routine, or
* i2c_generic_scl_recovery().
* @get_scl: This gets current value of SCL line. Mandatory for generic SCL
- * recovery. Used internally for generic GPIO recovery.
- * @set_scl: This sets/clears SCL line. Mandatory for generic SCL recovery. Used
- * internally for generic GPIO recovery.
+ * recovery. Populated internally for generic GPIO recovery.
+ * @set_scl: This sets/clears the SCL line. Mandatory for generic SCL recovery.
+ * Populated internally for generic GPIO recovery.
* @get_sda: This gets current value of SDA line. Optional for generic SCL
- * recovery. Used internally, if sda_gpio is a valid GPIO, for generic GPIO
- * recovery.
+ * recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
+ * GPIO recovery.
* @prepare_recovery: This will be called before starting recovery. Platform may
* configure padmux here for SDA/SCL line or something else they want.
* @unprepare_recovery: This will be called after completing recovery. Platform
--
2.11.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 2/6] i2c: add identifier in declarations for i2c_bus_recovery
2017-12-04 12:36 ` [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar Wolfram Sang
2017-12-04 12:36 ` [PATCH 1/6] i2c: make kerneldoc about bus recovery more precise Wolfram Sang
@ 2017-12-04 12:36 ` Wolfram Sang
2017-12-13 15:23 ` Andy Shevchenko
2017-12-04 12:36 ` [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info Wolfram Sang
` (5 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Wolfram Sang @ 2017-12-04 12:36 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, Wolfram Sang, Phil Reid, Andy Shevchenko,
Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
No reason to have them undefined, so let's add them.
Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
include/linux/i2c.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f8a9d81e911e52..8a020617b4e780 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -561,9 +561,9 @@ struct i2c_timings {
struct i2c_bus_recovery_info {
int (*recover_bus)(struct i2c_adapter *);
- int (*get_scl)(struct i2c_adapter *);
- void (*set_scl)(struct i2c_adapter *, int val);
- int (*get_sda)(struct i2c_adapter *);
+ int (*get_scl)(struct i2c_adapter *adap);
+ void (*set_scl)(struct i2c_adapter *adap, int val);
+ int (*get_sda)(struct i2c_adapter *adap);
void (*prepare_recovery)(struct i2c_adapter *);
void (*unprepare_recovery)(struct i2c_adapter *);
--
2.11.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 2/6] i2c: add identifier in declarations for i2c_bus_recovery
2017-12-04 12:36 ` [PATCH 2/6] i2c: add identifier in declarations for i2c_bus_recovery Wolfram Sang
@ 2017-12-13 15:23 ` Andy Shevchenko
2018-01-09 11:17 ` Wolfram Sang
0 siblings, 1 reply; 25+ messages in thread
From: Andy Shevchenko @ 2017-12-13 15:23 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: linux-renesas-soc, Phil Reid, Jarkko Nikula, Claudio Foellmi,
Andrzej Hajda
On Mon, 2017-12-04 at 13:36 +0100, Wolfram Sang wrote:
> No reason to have them undefined, so let's add them.
>
> int (*recover_bus)(struct i2c_adapter *);
>
> - int (*get_scl)(struct i2c_adapter *);
> - void (*set_scl)(struct i2c_adapter *, int val);
> - int (*get_sda)(struct i2c_adapter *);
> + int (*get_scl)(struct i2c_adapter *adap);
> + void (*set_scl)(struct i2c_adapter *adap, int val);
> + int (*get_sda)(struct i2c_adapter *adap);
>
> void (*prepare_recovery)(struct i2c_adapter *);
> void (*unprepare_recovery)(struct i2c_adapter *);
It seems inconsistent with the rest of the members even from this
visible piece.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/6] i2c: add identifier in declarations for i2c_bus_recovery
2017-12-13 15:23 ` Andy Shevchenko
@ 2018-01-09 11:17 ` Wolfram Sang
0 siblings, 0 replies; 25+ messages in thread
From: Wolfram Sang @ 2018-01-09 11:17 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Phil Reid,
Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
[-- Attachment #1: Type: text/plain, Size: 834 bytes --]
On Wed, Dec 13, 2017 at 05:23:02PM +0200, Andy Shevchenko wrote:
> On Mon, 2017-12-04 at 13:36 +0100, Wolfram Sang wrote:
> > No reason to have them undefined, so let's add them.
> >
>
> > int (*recover_bus)(struct i2c_adapter *);
> >
> > - int (*get_scl)(struct i2c_adapter *);
> > - void (*set_scl)(struct i2c_adapter *, int val);
> > - int (*get_sda)(struct i2c_adapter *);
> > + int (*get_scl)(struct i2c_adapter *adap);
> > + void (*set_scl)(struct i2c_adapter *adap, int val);
> > + int (*get_sda)(struct i2c_adapter *adap);
> >
> > void (*prepare_recovery)(struct i2c_adapter *);
> > void (*unprepare_recovery)(struct i2c_adapter *);
>
> It seems inconsistent with the rest of the members even from this
> visible piece.
Agreed. Either overlooked, or lost during rebase. Any way, will fix!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-04 12:36 ` [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar Wolfram Sang
2017-12-04 12:36 ` [PATCH 1/6] i2c: make kerneldoc about bus recovery more precise Wolfram Sang
2017-12-04 12:36 ` [PATCH 2/6] i2c: add identifier in declarations for i2c_bus_recovery Wolfram Sang
@ 2017-12-04 12:36 ` Wolfram Sang
2017-12-05 1:12 ` Phil Reid
` (2 more replies)
2017-12-04 12:36 ` [PATCH 4/6] i2c: ensure SDA is released in recovery if SDA is controllable Wolfram Sang
` (4 subsequent siblings)
7 siblings, 3 replies; 25+ messages in thread
From: Wolfram Sang @ 2017-12-04 12:36 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, Wolfram Sang, Phil Reid, Andy Shevchenko,
Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
This will be needed when we want to create STOP conditions, too, later.
Create the needed fields and populate them for the GPIO case if the GPIO
is set to output.
Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
drivers/i2c/i2c-core-base.c | 11 ++++++++++-
include/linux/i2c.h | 4 ++++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index bb34a5d4113331..f4313801a0aaba 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -27,6 +27,7 @@
#include <linux/delay.h>
#include <linux/err.h>
#include <linux/errno.h>
+#include <linux/gpio.h>
#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/i2c-smbus.h>
@@ -147,6 +148,11 @@ static int get_sda_gpio_value(struct i2c_adapter *adap)
return gpiod_get_value_cansleep(adap->bus_recovery_info->sda_gpiod);
}
+static void set_sda_gpio_value(struct i2c_adapter *adap, int val)
+{
+ gpiod_set_value_cansleep(adap->bus_recovery_info->sda_gpiod, val);
+}
+
/*
* We are generating clock pulses. ndelay() determines durating of clk pulses.
* We will generate clock with rate 100 KHz and so duration of both clock levels
@@ -225,8 +231,11 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
if (bri->scl_gpiod && bri->recover_bus == i2c_generic_scl_recovery) {
bri->get_scl = get_scl_gpio_value;
bri->set_scl = set_scl_gpio_value;
- if (bri->sda_gpiod)
+ if (bri->sda_gpiod) {
bri->get_sda = get_sda_gpio_value;
+ if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
+ bri->set_sda = set_sda_gpio_value;
+ }
return;
}
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 8a020617b4e780..c3e402e647791c 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -551,6 +551,9 @@ struct i2c_timings {
* @get_sda: This gets current value of SDA line. Optional for generic SCL
* recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
* GPIO recovery.
+ * @set_sda: This sets/clears the SDA line. Optional for generic SCL recovery.
+ * Populated internally, if sda_gpio is a valid GPIO, for generic GPIO
+ * recovery.
* @prepare_recovery: This will be called before starting recovery. Platform may
* configure padmux here for SDA/SCL line or something else they want.
* @unprepare_recovery: This will be called after completing recovery. Platform
@@ -564,6 +567,7 @@ struct i2c_bus_recovery_info {
int (*get_scl)(struct i2c_adapter *adap);
void (*set_scl)(struct i2c_adapter *adap, int val);
int (*get_sda)(struct i2c_adapter *adap);
+ void (*set_sda)(struct i2c_adapter *adap, int val);
void (*prepare_recovery)(struct i2c_adapter *);
void (*unprepare_recovery)(struct i2c_adapter *);
--
2.11.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-04 12:36 ` [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info Wolfram Sang
@ 2017-12-05 1:12 ` Phil Reid
2017-12-05 8:39 ` Linus Walleij
2017-12-13 15:27 ` Andy Shevchenko
2 siblings, 0 replies; 25+ messages in thread
From: Phil Reid @ 2017-12-05 1:12 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: linux-renesas-soc, Andy Shevchenko, Jarkko Nikula,
Claudio Foellmi, Andrzej Hajda, Linus Walleij
G'day Wolfram,
On 4/12/2017 20:36, Wolfram Sang wrote:
> This will be needed when we want to create STOP conditions, too, later.
> Create the needed fields and populate them for the GPIO case if the GPIO
> is set to output.
>
> Cc: Phil Reid <preid@electromag.com.au>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> drivers/i2c/i2c-core-base.c | 11 ++++++++++-
> include/linux/i2c.h | 4 ++++
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index bb34a5d4113331..f4313801a0aaba 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -27,6 +27,7 @@
> #include <linux/delay.h>
> #include <linux/err.h>
> #include <linux/errno.h>
> +#include <linux/gpio.h>
I thought we weren't supposed to use this any more.
But it's the only place were gpiod_get_direction return flags are defined at present.
+cc Linus W.
> #include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/i2c-smbus.h>
> @@ -147,6 +148,11 @@ static int get_sda_gpio_value(struct i2c_adapter *adap)
> return gpiod_get_value_cansleep(adap->bus_recovery_info->sda_gpiod);
> }
>
> +static void set_sda_gpio_value(struct i2c_adapter *adap, int val)
> +{
> + gpiod_set_value_cansleep(adap->bus_recovery_info->sda_gpiod, val);
> +}
> +
> /*
> * We are generating clock pulses. ndelay() determines durating of clk pulses.
> * We will generate clock with rate 100 KHz and so duration of both clock levels
> @@ -225,8 +231,11 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
> if (bri->scl_gpiod && bri->recover_bus == i2c_generic_scl_recovery) {
> bri->get_scl = get_scl_gpio_value;
> bri->set_scl = set_scl_gpio_value;
> - if (bri->sda_gpiod)
> + if (bri->sda_gpiod) {
> bri->get_sda = get_sda_gpio_value;
> + if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
> + bri->set_sda = set_sda_gpio_value;
> + }
> return;
> }
>
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 8a020617b4e780..c3e402e647791c 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -551,6 +551,9 @@ struct i2c_timings {
> * @get_sda: This gets current value of SDA line. Optional for generic SCL
> * recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
> * GPIO recovery.
> + * @set_sda: This sets/clears the SDA line. Optional for generic SCL recovery.
> + * Populated internally, if sda_gpio is a valid GPIO, for generic GPIO
> + * recovery.
> * @prepare_recovery: This will be called before starting recovery. Platform may
> * configure padmux here for SDA/SCL line or something else they want.
> * @unprepare_recovery: This will be called after completing recovery. Platform
> @@ -564,6 +567,7 @@ struct i2c_bus_recovery_info {
> int (*get_scl)(struct i2c_adapter *adap);
> void (*set_scl)(struct i2c_adapter *adap, int val);
> int (*get_sda)(struct i2c_adapter *adap);
> + void (*set_sda)(struct i2c_adapter *adap, int val);
>
> void (*prepare_recovery)(struct i2c_adapter *);
> void (*unprepare_recovery)(struct i2c_adapter *);
>
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-04 12:36 ` [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info Wolfram Sang
2017-12-05 1:12 ` Phil Reid
@ 2017-12-05 8:39 ` Linus Walleij
2017-12-05 9:57 ` Phil Reid
` (2 more replies)
2017-12-13 15:27 ` Andy Shevchenko
2 siblings, 3 replies; 25+ messages in thread
From: Linus Walleij @ 2017-12-05 8:39 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, Linux-Renesas, Phil Reid, Andy Shevchenko,
Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
On Mon, Dec 4, 2017 at 1:36 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> This will be needed when we want to create STOP conditions, too, later.
> Create the needed fields and populate them for the GPIO case if the GPIO
> is set to output.
>
> Cc: Phil Reid <preid@electromag.com.au>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
(...)
> #include <linux/errno.h>
> +#include <linux/gpio.h>
Please no.
> + if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
> + bri->set_sda = set_sda_gpio_value;
Just compare it to zero. if (!gpiod_get_direction())
This flag is only for requesting GPIOs in the old API.
We didn't add a define in the new API, it seemed overengineered.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-05 8:39 ` Linus Walleij
@ 2017-12-05 9:57 ` Phil Reid
2017-12-05 10:32 ` Wolfram Sang
2017-12-05 10:34 ` Linus Walleij
2017-12-05 13:38 ` Wolfram Sang
2018-01-09 11:23 ` Wolfram Sang
2 siblings, 2 replies; 25+ messages in thread
From: Phil Reid @ 2017-12-05 9:57 UTC (permalink / raw)
To: Linus Walleij, Wolfram Sang
Cc: linux-i2c, Linux-Renesas, Andy Shevchenko, Jarkko Nikula,
Claudio Foellmi, Andrzej Hajda
On 5/12/2017 16:39, Linus Walleij wrote:
> On Mon, Dec 4, 2017 at 1:36 PM, Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
>
>> This will be needed when we want to create STOP conditions, too, later.
>> Create the needed fields and populate them for the GPIO case if the GPIO
>> is set to output.
>>
>> Cc: Phil Reid <preid@electromag.com.au>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> (...)
>> #include <linux/errno.h>
>> +#include <linux/gpio.h>
>
> Please no.
>
>> + if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
>> + bri->set_sda = set_sda_gpio_value;
>
> Just compare it to zero. if (!gpiod_get_direction())
>
> This flag is only for requesting GPIOs in the old API.
> We didn't add a define in the new API, it seemed overengineered.
>
Perhaps the docs need updating?
as of 4.15-rc2 it says
* Return GPIOF_DIR_IN or GPIOF_DIR_OUT, or an error code in case of error.
I can submit something if you wish.
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-05 9:57 ` Phil Reid
@ 2017-12-05 10:32 ` Wolfram Sang
2017-12-05 10:34 ` Linus Walleij
1 sibling, 0 replies; 25+ messages in thread
From: Wolfram Sang @ 2017-12-05 10:32 UTC (permalink / raw)
To: Phil Reid
Cc: Linus Walleij, Wolfram Sang, linux-i2c, Linux-Renesas,
Andy Shevchenko, Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
[-- Attachment #1: Type: text/plain, Size: 260 bytes --]
> Perhaps the docs need updating?
> as of 4.15-rc2 it says
> * Return GPIOF_DIR_IN or GPIOF_DIR_OUT, or an error code in case of error.
>
> I can submit something if you wish.
They surely need updating. I was relying on exactly this information.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-05 9:57 ` Phil Reid
2017-12-05 10:32 ` Wolfram Sang
@ 2017-12-05 10:34 ` Linus Walleij
1 sibling, 0 replies; 25+ messages in thread
From: Linus Walleij @ 2017-12-05 10:34 UTC (permalink / raw)
To: Phil Reid
Cc: Wolfram Sang, linux-i2c, Linux-Renesas, Andy Shevchenko,
Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
On Tue, Dec 5, 2017 at 10:57 AM, Phil Reid <preid@electromag.com.au> wrote:
> On 5/12/2017 16:39, Linus Walleij wrote:
>> Just compare it to zero. if (!gpiod_get_direction())
>>
>> This flag is only for requesting GPIOs in the old API.
>> We didn't add a define in the new API, it seemed overengineered.
>>
>
> Perhaps the docs need updating?
> as of 4.15-rc2 it says
> * Return GPIOF_DIR_IN or GPIOF_DIR_OUT, or an error code in case of error.
>
> I can submit something if you wish.
Hit me.
Sorry for the incoherent information.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-05 8:39 ` Linus Walleij
2017-12-05 9:57 ` Phil Reid
@ 2017-12-05 13:38 ` Wolfram Sang
2017-12-05 15:31 ` Linus Walleij
2018-01-09 11:23 ` Wolfram Sang
2 siblings, 1 reply; 25+ messages in thread
From: Wolfram Sang @ 2017-12-05 13:38 UTC (permalink / raw)
To: Linus Walleij
Cc: Wolfram Sang, linux-i2c, Linux-Renesas, Phil Reid,
Andy Shevchenko, Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
[-- Attachment #1: Type: text/plain, Size: 491 bytes --]
> > + if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
> > + bri->set_sda = set_sda_gpio_value;
>
> Just compare it to zero. if (!gpiod_get_direction())
So, this?
if (!gpiod_get_direction())
bri->set_sda = set_sda_gpio_value;
That looks like "if i couldn't the direction, then...", or?
Comparing to plain numbers feels like magic values?
Maybe this should be named 'gpiod_is_direction_input()'?
Or?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-05 13:38 ` Wolfram Sang
@ 2017-12-05 15:31 ` Linus Walleij
2017-12-05 16:43 ` Wolfram Sang
2017-12-13 15:30 ` Andy Shevchenko
0 siblings, 2 replies; 25+ messages in thread
From: Linus Walleij @ 2017-12-05 15:31 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wolfram Sang, linux-i2c, Linux-Renesas, Phil Reid,
Andy Shevchenko, Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
On Tue, Dec 5, 2017 at 2:38 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> Just compare it to zero. if (!gpiod_get_direction())
>
> So, this?
>
> if (!gpiod_get_direction())
> bri->set_sda = set_sda_gpio_value;
>
> That looks like "if i couldn't the direction, then...", or?
>
> Comparing to plain numbers feels like magic values?
Hehe :)
> Maybe this should be named 'gpiod_is_direction_input()'?
Two statice inlines in <linux/gpio/consumer.h>
named
int gpiod_is output()
int gpiod_is_input()
should conform to Rusty Russell's API hierarchy.
Interested in fixing it, or should I?
I can almost ACK it before you write the patch.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-05 15:31 ` Linus Walleij
@ 2017-12-05 16:43 ` Wolfram Sang
2017-12-07 11:25 ` Wolfram Sang
2017-12-13 15:30 ` Andy Shevchenko
1 sibling, 1 reply; 25+ messages in thread
From: Wolfram Sang @ 2017-12-05 16:43 UTC (permalink / raw)
To: Linus Walleij
Cc: Wolfram Sang, linux-i2c, Linux-Renesas, Phil Reid,
Andy Shevchenko, Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
[-- Attachment #1: Type: text/plain, Size: 464 bytes --]
> Two statice inlines in <linux/gpio/consumer.h>
> named
>
> int gpiod_is output()
> int gpiod_is_input()
>
> should conform to Rusty Russell's API hierarchy.
Yes, sounds good!
> Interested in fixing it, or should I?
Please do. I don't want to add magic numbers to the kernel ;)
Could you provide an immutable branch then that I could pick up, because
my series will depend on it.
> I can almost ACK it before you write the patch.
:)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-05 16:43 ` Wolfram Sang
@ 2017-12-07 11:25 ` Wolfram Sang
2017-12-10 0:24 ` Linus Walleij
0 siblings, 1 reply; 25+ messages in thread
From: Wolfram Sang @ 2017-12-07 11:25 UTC (permalink / raw)
To: Linus Walleij
Cc: Wolfram Sang, linux-i2c, Linux-Renesas, Phil Reid,
Andy Shevchenko, Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
[-- Attachment #1: Type: text/plain, Size: 334 bytes --]
> > Two statice inlines in <linux/gpio/consumer.h>
> > named
> >
> > int gpiod_is output()
> > int gpiod_is_input()
> >
> > should conform to Rusty Russell's API hierarchy.
>
> Yes, sounds good!
>
> > Interested in fixing it, or should I?
>
> Please do. I don't want to add magic numbers to the kernel ;)
OK?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-07 11:25 ` Wolfram Sang
@ 2017-12-10 0:24 ` Linus Walleij
0 siblings, 0 replies; 25+ messages in thread
From: Linus Walleij @ 2017-12-10 0:24 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wolfram Sang, linux-i2c, Linux-Renesas, Phil Reid,
Andy Shevchenko, Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
On Thu, Dec 7, 2017 at 12:25 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > Two statice inlines in <linux/gpio/consumer.h>
>> > named
>> >
>> > int gpiod_is output()
>> > int gpiod_is_input()
>> >
>> > should conform to Rusty Russell's API hierarchy.
>>
>> Yes, sounds good!
>>
>> > Interested in fixing it, or should I?
>>
>> Please do. I don't want to add magic numbers to the kernel ;)
>
> OK?
Just slow, sorry. Sent a patch now!
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-05 15:31 ` Linus Walleij
2017-12-05 16:43 ` Wolfram Sang
@ 2017-12-13 15:30 ` Andy Shevchenko
1 sibling, 0 replies; 25+ messages in thread
From: Andy Shevchenko @ 2017-12-13 15:30 UTC (permalink / raw)
To: Linus Walleij, Wolfram Sang
Cc: Wolfram Sang, linux-i2c, Linux-Renesas, Phil Reid, Jarkko Nikula,
Claudio Foellmi, Andrzej Hajda
On Tue, 2017-12-05 at 16:31 +0100, Linus Walleij wrote:
> On Tue, Dec 5, 2017 at 2:38 PM, Wolfram Sang <wsa@the-dreams.de>
> wrote:
>
> Two statice inlines in <linux/gpio/consumer.h>
> named
>
> int gpiod_is output()
> int gpiod_is_input()
Ha, just proposed similar.
>
> should conform to Rusty Russell's API hierarchy.
>
> Interested in fixing it, or should I?
> I can almost ACK it before you write the patch.
I vote for this type of API, and agree with Wolfram !_get_direction() is
confusing.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-05 8:39 ` Linus Walleij
2017-12-05 9:57 ` Phil Reid
2017-12-05 13:38 ` Wolfram Sang
@ 2018-01-09 11:23 ` Wolfram Sang
2 siblings, 0 replies; 25+ messages in thread
From: Wolfram Sang @ 2018-01-09 11:23 UTC (permalink / raw)
To: Linus Walleij
Cc: Wolfram Sang, linux-i2c, Linux-Renesas, Phil Reid,
Andy Shevchenko, Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]
On Tue, Dec 05, 2017 at 09:39:48AM +0100, Linus Walleij wrote:
> On Mon, Dec 4, 2017 at 1:36 PM, Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
>
> > This will be needed when we want to create STOP conditions, too, later.
> > Create the needed fields and populate them for the GPIO case if the GPIO
> > is set to output.
> >
> > Cc: Phil Reid <preid@electromag.com.au>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> (...)
> > #include <linux/errno.h>
> > +#include <linux/gpio.h>
>
> Please no.
>
> > + if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
> > + bri->set_sda = set_sda_gpio_value;
>
> Just compare it to zero. if (!gpiod_get_direction())
Okay, for now, I'll do:
/* FIXME: add proper flag once provided by GPIO core */
if (gpiod_get_direction(bri->sda_gpiod) == 0)
Thanks!
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info
2017-12-04 12:36 ` [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info Wolfram Sang
2017-12-05 1:12 ` Phil Reid
2017-12-05 8:39 ` Linus Walleij
@ 2017-12-13 15:27 ` Andy Shevchenko
2 siblings, 0 replies; 25+ messages in thread
From: Andy Shevchenko @ 2017-12-13 15:27 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c, Linus Walleij
Cc: linux-renesas-soc, Phil Reid, Jarkko Nikula, Claudio Foellmi,
Andrzej Hajda
On Mon, 2017-12-04 at 13:36 +0100, Wolfram Sang wrote:
> This will be needed when we want to create STOP conditions, too,
> later.
> Create the needed fields and populate them for the GPIO case if the
> GPIO
> is set to output.
> +#include <linux/gpio.h>
> #include <linux/gpio/consumer.h>
I think it should be not done like this. (Yes, I know why you did that)
Perhaps Linus will accept a patch to move direction flags to some header
feasible via consumer.h.
Linus?
Or if we wish to hide:
static inline bool gpiod_is_direction_out() {}
static inline bool gpiod_is_direction_in() {}
> if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
P.S. Yep, I saw some other upstreamed patch doing this kind of
comparison.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/6] i2c: ensure SDA is released in recovery if SDA is controllable
2017-12-04 12:36 ` [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar Wolfram Sang
` (2 preceding siblings ...)
2017-12-04 12:36 ` [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info Wolfram Sang
@ 2017-12-04 12:36 ` Wolfram Sang
2017-12-04 12:36 ` [PATCH 5/6] i2c: send STOP after successful bus recovery Wolfram Sang
` (3 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Wolfram Sang @ 2017-12-04 12:36 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, Wolfram Sang, Phil Reid, Andy Shevchenko,
Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
If we have a function to control SDA, we should ensure that SDA is not
held down by us. So, release the GPIO in this case.
Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
drivers/i2c/i2c-core-base.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index f4313801a0aaba..dae7f4a783dbe4 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -170,6 +170,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
bri->prepare_recovery(adap);
bri->set_scl(adap, val);
+ if (bri->set_sda)
+ bri->set_sda(adap, 1);
ndelay(RECOVERY_NDELAY);
/*
--
2.11.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 5/6] i2c: send STOP after successful bus recovery
2017-12-04 12:36 ` [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar Wolfram Sang
` (3 preceding siblings ...)
2017-12-04 12:36 ` [PATCH 4/6] i2c: ensure SDA is released in recovery if SDA is controllable Wolfram Sang
@ 2017-12-04 12:36 ` Wolfram Sang
2017-12-04 12:36 ` [PATCH 6/6] i2c: rcar: implement " Wolfram Sang
` (2 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Wolfram Sang @ 2017-12-04 12:36 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, Wolfram Sang, Phil Reid, Andy Shevchenko,
Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
If we managed to get a client release SDA again, send a STOP afterwards
to make sure we have a consistent state on the bus again.
Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
drivers/i2c/i2c-core-base.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index dae7f4a783dbe4..29d6606d653229 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -200,6 +200,18 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
if (bri->get_sda && !bri->get_sda(adap))
ret = -EBUSY;
+ /* If all went well, send STOP for a sane bus state. */
+ if (ret == 0 && bri->set_sda) {
+ bri->set_scl(adap, 0);
+ ndelay(RECOVERY_NDELAY / 2);
+ bri->set_sda(adap, 0);
+ ndelay(RECOVERY_NDELAY / 2);
+ bri->set_scl(adap, 1);
+ ndelay(RECOVERY_NDELAY / 2);
+ bri->set_sda(adap, 1);
+ ndelay(RECOVERY_NDELAY / 2);
+ }
+
if (bri->unprepare_recovery)
bri->unprepare_recovery(adap);
--
2.11.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 6/6] i2c: rcar: implement bus recovery
2017-12-04 12:36 ` [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar Wolfram Sang
` (4 preceding siblings ...)
2017-12-04 12:36 ` [PATCH 5/6] i2c: send STOP after successful bus recovery Wolfram Sang
@ 2017-12-04 12:36 ` Wolfram Sang
2017-12-05 1:13 ` [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar Phil Reid
2017-12-05 9:21 ` Andrzej Hajda
7 siblings, 0 replies; 25+ messages in thread
From: Wolfram Sang @ 2017-12-04 12:36 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, Wolfram Sang, Phil Reid, Andy Shevchenko,
Jarkko Nikula, Claudio Foellmi, Andrzej Hajda
We can force levels of SCL and SDA, so we can use that for bus recovery.
Note that we cannot read SDA back, because we will only get the internal
state of the bus free detection.
Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
drivers/i2c/busses/i2c-rcar.c | 54 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 52 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 8a2ae3e6c561c4..d4b7b5380c2983 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -132,6 +132,7 @@ struct rcar_i2c_priv {
int pos;
u32 icccr;
u32 flags;
+ u8 recovery_icmcr; /* protected by adapter lock */
enum rcar_i2c_type devtype;
struct i2c_client *slave;
@@ -158,6 +159,46 @@ static u32 rcar_i2c_read(struct rcar_i2c_priv *priv, int reg)
return readl(priv->io + reg);
}
+static int rcar_i2c_get_scl(struct i2c_adapter *adap)
+{
+ struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
+
+ return !!(rcar_i2c_read(priv, ICMCR) & FSCL);
+
+};
+
+static void rcar_i2c_set_scl(struct i2c_adapter *adap, int val)
+{
+ struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
+
+ if (val)
+ priv->recovery_icmcr |= FSCL;
+ else
+ priv->recovery_icmcr &= ~FSCL;
+
+ rcar_i2c_write(priv, ICMCR, priv->recovery_icmcr);
+};
+
+/* No get_sda, because the HW only reports its bus free logic, not SDA itself */
+
+static void rcar_i2c_set_sda(struct i2c_adapter *adap, int val)
+{
+ struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
+
+ if (val)
+ priv->recovery_icmcr |= FSDA;
+ else
+ priv->recovery_icmcr &= ~FSDA;
+
+ rcar_i2c_write(priv, ICMCR, priv->recovery_icmcr);
+};
+
+static struct i2c_bus_recovery_info rcar_i2c_bri = {
+ .get_scl = rcar_i2c_get_scl,
+ .set_scl = rcar_i2c_set_scl,
+ .set_sda = rcar_i2c_set_sda,
+ .recover_bus = i2c_generic_scl_recovery,
+};
static void rcar_i2c_init(struct rcar_i2c_priv *priv)
{
/* reset master mode */
@@ -170,7 +211,7 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
{
- int i;
+ int i, ret;
for (i = 0; i < LOOP_TIMEOUT; i++) {
/* make sure that bus is not busy */
@@ -179,7 +220,15 @@ static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
udelay(1);
}
- return -EBUSY;
+ /* Waiting did not help, try to recover */
+ priv->recovery_icmcr = MDBS | OBPC | FSDA | FSCL;
+ ret = i2c_recover_bus(&priv->adap);
+
+ /* No failure when recovering, so check bus busy bit again */
+ if (ret == 0)
+ ret = (rcar_i2c_read(priv, ICMCR) & FSDA) ? -EBUSY : 0;
+
+ return ret;
}
static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv, struct i2c_timings *t)
@@ -851,6 +900,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
adap->retries = 3;
adap->dev.parent = dev;
adap->dev.of_node = dev->of_node;
+ adap->bus_recovery_info = &rcar_i2c_bri;
i2c_set_adapdata(adap, priv);
strlcpy(adap->name, pdev->name, sizeof(adap->name));
--
2.11.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar
2017-12-04 12:36 ` [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar Wolfram Sang
` (5 preceding siblings ...)
2017-12-04 12:36 ` [PATCH 6/6] i2c: rcar: implement " Wolfram Sang
@ 2017-12-05 1:13 ` Phil Reid
2017-12-05 9:21 ` Andrzej Hajda
7 siblings, 0 replies; 25+ messages in thread
From: Phil Reid @ 2017-12-05 1:13 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: linux-renesas-soc, Andy Shevchenko, Jarkko Nikula,
Claudio Foellmi, Andrzej Hajda
On 4/12/2017 20:36, Wolfram Sang wrote:
> When implementing bus recovery for the i2c-rcar driver, two problems were
> encountered: 1) When reading the SDA bit, not the SDA status was returned but
> the internal state of the "bus_is_busy" logic. 2) This logic needs a STOP to
> consider the bus free again. SCL/SDA high is not enough, and there is no other
> way known to reset the internal logic otherwise.
>
> The obvious solution to just send STOP after recovery makes sense for the
> generic case, too, IMO. If we made a device release SDA again, and are about
> start a new transfer using START, then we should terminate the previous state
> properly with STOP. This may help with some devices and shouldn't create any
> drawback AFAICS.
>
> For this, we need to introduce a 'set_sda' callback to the recovery
> infrastructure. Because of this and some other minor recovery core changes, I
> CCed a few people working on I2C bus recovery recently. The first five patches
> may be interesting for you, so your input is greatly appreciated. Also, testing
> the new features with GPIO based recovery would be awesome to have.
>
> This was tested on a Renesas Lager board (r8a7790/R-Car H2). My test procedure
> is documented here:
Patches 1-5.
Tested-by: Phil Reid <preid@electromag.com.au>
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar
2017-12-04 12:36 ` [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar Wolfram Sang
` (6 preceding siblings ...)
2017-12-05 1:13 ` [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar Phil Reid
@ 2017-12-05 9:21 ` Andrzej Hajda
2018-01-09 11:15 ` Wolfram Sang
7 siblings, 1 reply; 25+ messages in thread
From: Andrzej Hajda @ 2017-12-05 9:21 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: linux-renesas-soc, Phil Reid, Andy Shevchenko, Jarkko Nikula,
Claudio Foellmi
On 04.12.2017 13:36, Wolfram Sang wrote:
> When implementing bus recovery for the i2c-rcar driver, two problems were
> encountered: 1) When reading the SDA bit, not the SDA status was returned but
> the internal state of the "bus_is_busy" logic. 2) This logic needs a STOP to
> consider the bus free again. SCL/SDA high is not enough, and there is no other
> way known to reset the internal logic otherwise.
>
> The obvious solution to just send STOP after recovery makes sense for the
> generic case, too, IMO. If we made a device release SDA again, and are about
> start a new transfer using START, then we should terminate the previous state
> properly with STOP. This may help with some devices and shouldn't create any
> drawback AFAICS.
>
> For this, we need to introduce a 'set_sda' callback to the recovery
> infrastructure. Because of this and some other minor recovery core changes, I
> CCed a few people working on I2C bus recovery recently. The first five patches
> may be interesting for you, so your input is greatly appreciated. Also, testing
> the new features with GPIO based recovery would be awesome to have.
>
> This was tested on a Renesas Lager board (r8a7790/R-Car H2). My test procedure
> is documented here:
>
> https://elinux.org/Tests:I2C-bus-recovery
>
> A branch is available here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/rcar-i2c-recovery
>
> Please let me know what you think.
I do not feel myself experienced I2C developer, so please be merciful if
I write stupid things :)
>From what I see the whole recovery infrastructure partially duplicates
i2c_bit_algo/i2c_algo_bit_data algorithm, and GPIO recovery duplicates
i2c-gpio driver.
Wouldn't be better to somehow reuse existing code? For example by adding
recovery callback in i2c_algorithm, and call i2c-gpio::recovery or
i2c_bit_algo::recovery from rcar-i2c-recovery (in this particular case).
I am not sure how much work it requires, and if it is worth it. Please
consider it as suggestion.
Regards
Andrzej
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar
2017-12-05 9:21 ` Andrzej Hajda
@ 2018-01-09 11:15 ` Wolfram Sang
0 siblings, 0 replies; 25+ messages in thread
From: Wolfram Sang @ 2018-01-09 11:15 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Phil Reid,
Andy Shevchenko, Jarkko Nikula, Claudio Foellmi
[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]
> I do not feel myself experienced I2C developer, so please be merciful if
> I write stupid things :)
Don't worry :) Thanks for sharing!
> From what I see the whole recovery infrastructure partially duplicates
> i2c_bit_algo/i2c_algo_bit_data algorithm, and GPIO recovery duplicates
> i2c-gpio driver.
> Wouldn't be better to somehow reuse existing code? For example by adding
> recovery callback in i2c_algorithm, and call i2c-gpio::recovery or
> i2c_bit_algo::recovery from rcar-i2c-recovery (in this particular case).
I understand what you mean but I also don't think it is a good idea in
practice. We can't save much by using i2c-gpio, because GPIO is only one
use-case, having custom set/get functions being another one, for
example. A bigger chance for reuse would, in deed, be i2c-algo-bit.c,
but there are some subtle differences and encoding them in the generic
functions would make them more unreadable IMHO. But the real argument
is: dependency hell. The core might be build-in, the rest is a module,
and things get more complicated. And I don't want to enforce this on
_every_ I2C user out there. So, given the really simple functions, I
think it makes sense to have them in the core.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread