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

[Wolfram Sang]

On Tue, Dec 05, 2017 at 09:39:48AM +0100, Linus Walleij wrote:
> On Mon, Dec 4, 2017 at 1:36 PM, 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 
> > Cc: Andy Shevchenko 
> > Cc: Jarkko Nikula 
> > Cc: Claudio Foellmi 
> > Cc: Andrzej Hajda 
> > Signed-off-by: Wolfram Sang 
> 
> (...)
> >  #include 
> > +#include 
> 
> 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



signature.asc
Description: PGP signature

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

[Andy Shevchenko]

On Tue, 2017-12-05 at 16:31 +0100, Linus Walleij wrote:
> On Tue, Dec 5, 2017 at 2:38 PM, Wolfram Sang 
> wrote:
> 


> Two statice inlines in 
> 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 
Intel Finland Oy

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

[Andy Shevchenko]

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 
>  #include 

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 
Intel Finland Oy

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

[Linus Walleij]

On Thu, Dec 7, 2017 at 12:25 PM, Wolfram Sang  wrote:
>
>> > Two statice inlines in 
>> > 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

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

[Wolfram Sang]

> > Two statice inlines in 
> > 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?



signature.asc
Description: PGP signature

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

[Wolfram Sang]

> Two statice inlines in 
> 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.

:)



signature.asc
Description: PGP signature

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

[Linus Walleij]

On Tue, Dec 5, 2017 at 2:38 PM, Wolfram Sang  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 
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

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

[Wolfram Sang]

> > +   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?



signature.asc
Description: PGP signature

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

[Linus Walleij]

On Tue, Dec 5, 2017 at 10:57 AM, Phil Reid  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

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

[Wolfram Sang]

> 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.



signature.asc
Description: PGP signature

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

[Phil Reid]

On 5/12/2017 16:39, Linus Walleij wrote:

On Mon, Dec 4, 2017 at 1:36 PM, 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 
Cc: Andy Shevchenko 
Cc: Jarkko Nikula 
Cc: Claudio Foellmi 
Cc: Andrzej Hajda 
Signed-off-by: Wolfram Sang 


(...)

  #include 
+#include 


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

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

[Linus Walleij]

On Mon, Dec 4, 2017 at 1:36 PM, 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 
> Cc: Andy Shevchenko 
> Cc: Jarkko Nikula 
> Cc: Claudio Foellmi 
> Cc: Andrzej Hajda 
> Signed-off-by: Wolfram Sang 

(...)
>  #include 
> +#include 

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

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

[Phil Reid]

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 
Cc: Andy Shevchenko 
Cc: Jarkko Nikula 
Cc: Claudio Foellmi 
Cc: Andrzej Hajda 
Signed-off-by: Wolfram Sang 
---
  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 
  #include 
  #include 
+#include 


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 
  #include 
  #include 
@@ -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