| 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 | expand |
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 *); >
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
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.
> 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.
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
> > + 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?
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
> 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. :)
> > 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?
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
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.
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.
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
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 *);
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(-)