[3/6] i2c: add 'set_sda' to bus_recovery_info

Message ID 20171204123640.3382-4-wsa+renesas@sang-engineering.com
State Superseded
Headers show
Series
  • i2c: send STOP after recovery; use it for i2c-rcar
Related show

Commit Message

Wolfram Sang Dec. 4, 2017, 12:36 p.m.
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(-)

Comments

Phil Reid Dec. 5, 2017, 1:12 a.m. | #1
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 *);
>
Linus Walleij Dec. 5, 2017, 8:39 a.m. | #2
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
Phil Reid Dec. 5, 2017, 9:57 a.m. | #3
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.
Wolfram Sang Dec. 5, 2017, 10:32 a.m. | #4
> 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.
Linus Walleij Dec. 5, 2017, 10:34 a.m. | #5
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
Wolfram Sang Dec. 5, 2017, 1:38 p.m. | #6
> > +                       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?
Linus Walleij Dec. 5, 2017, 3:31 p.m. | #7
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
Wolfram Sang Dec. 5, 2017, 4:43 p.m. | #8
> 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.

:)
Wolfram Sang Dec. 7, 2017, 11:25 a.m. | #9
> > 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?
Linus Walleij Dec. 10, 2017, 12:24 a.m. | #10
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
Andy Shevchenko Dec. 13, 2017, 3:27 p.m. | #11
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 Dec. 13, 2017, 3:30 p.m. | #12
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.
Wolfram Sang Jan. 9, 2018, 11:23 a.m. | #13
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

Patch

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 *);