Skip to contentSkip to conversation

i2c-mux-gpio: test if the gpio can sleep

PATCH In review Ionut Nicu 3 participants 6 messages Oct 10, 2013

Subsystems (4)
lore
Share & tools
lore

Ionut Nicu

author
#0 lore raw

PATCH 1/2 i2c-mux-gpio: test if the gpio can sleep

Some gpio chips may have get/set operations that
can sleep. For this type of chips we must use the
_cansleep() version of gpio_set_value.

---
 drivers/i2c/muxes/i2c-mux-gpio.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)
drivers/i2c/muxes/i2c-mux-gpio.c +8 -3
index a764da7..b5f17ef 100644
@@ -27,11 +27,16 @@ struct gpiomux {
 
 static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
 {
+	unsigned gpio;
 	int i;
 
-	for (i = 0; i < mux->data.n_gpios; i++)
-		gpio_set_value(mux->gpio_base + mux->data.gpios[i],
-			       val & (1 << i));
+	for (i = 0; i < mux->data.n_gpios; i++) {
+		gpio = mux->gpio_base + mux->data.gpios[i];
+		if (gpio_cansleep(gpio))
+			gpio_set_value_cansleep(gpio, val & (1 << i));
+		else
+			gpio_set_value(gpio, val & (1 << i));
+	}
 }
 
 static int i2c_mux_gpio_select(struct i2c_adapter *adap, void *data, u32 chan)
Signature · 1 line
1.7.1
#1 lore raw
Quoted 19 lines · Ionut Nicu

Some gpio chips may have get/set operations that
can sleep. For this type of chips we must use the
_cansleep() version of gpio_set_value.

---
 drivers/i2c/muxes/i2c-mux-gpio.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)
drivers/i2c/muxes/i2c-mux-gpio.c +1 -0
index a764da7..b5f17ef 100644
@@ -27,11 +27,16 @@ struct gpiomux {
 
 static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
 {
+	unsigned gpio;
 	int i;

drivers/i2c/muxes/i2c-mux-gpio.c struct gpiomux { · line 38

Quoted 10 lines
drivers/i2c/muxes/i2c-mux-gpio.c +6 -3
 
-	for (i = 0; i < mux->data.n_gpios; i++)
-		gpio_set_value(mux->gpio_base + mux->data.gpios[i],
-			       val & (1 << i));
+	for (i = 0; i < mux->data.n_gpios; i++) {
+		gpio = mux->gpio_base + mux->data.gpios[i];
+		if (gpio_cansleep(gpio))
+			gpio_set_value_cansleep(gpio, val & (1 << i));
+		else
+			gpio_set_value(gpio, val & (1 << i));

The proper way to do this is just always use the _cansleep() version. gpio_set_value() only works for chips which do not sleep, gpio_set_value_cansleep() works for both those who do sleep and those who do not.

Quoted 5 lines
drivers/i2c/muxes/i2c-mux-gpio.c +1 -0
+	}
 }
 
 static int i2c_mux_gpio_select(struct i2c_adapter *adap, void *data, u32 chan)
author
#2 lore raw
Quoted 21 lines · ext Lars-Peter Clausen
Quoted 20 lines · Ionut Nicu

Some gpio chips may have get/set operations that
can sleep. For this type of chips we must use the
_cansleep() version of gpio_set_value.

---
 drivers/i2c/muxes/i2c-mux-gpio.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)
drivers/i2c/muxes/i2c-mux-gpio.c +1 -0
index a764da7..b5f17ef 100644
@@ -27,11 +27,16 @@ struct gpiomux {
 
 static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
 {
+	unsigned gpio;
 	int i;
 
Quoted 15 lines

drivers/i2c/muxes/i2c-mux-gpio.c struct gpiomux { · line 38

Quoted 9 lines
drivers/i2c/muxes/i2c-mux-gpio.c +6 -3
-	for (i = 0; i < mux->data.n_gpios; i++)
-		gpio_set_value(mux->gpio_base + mux->data.gpios[i],
-			       val & (1 << i));
+	for (i = 0; i < mux->data.n_gpios; i++) {
+		gpio = mux->gpio_base + mux->data.gpios[i];
+		if (gpio_cansleep(gpio))
+			gpio_set_value_cansleep(gpio, val & (1 << i));
+		else
+			gpio_set_value(gpio, val & (1 << i));

The proper way to do this is just always use the _cansleep() version. gpio_set_value() only works for chips which do not sleep, gpio_set_value_cansleep() works for both those who do sleep and those who do not.

Ok, then I will re-work the patch to do just that. Thanks!

#3 lore raw
In reply to Lars-Peter Clausen
Quoted 9 lines · Lars-Peter Clausen

drivers/i2c/muxes/i2c-mux-gpio.c struct gpiomux { · line 38

Quoted 4 lines
drivers/i2c/muxes/i2c-mux-gpio.c +4 -0
+		if (gpio_cansleep(gpio))
+			gpio_set_value_cansleep(gpio, val & (1 << i));
+		else
+			gpio_set_value(gpio, val & (1 << i));

The proper way to do this is just always use the _cansleep() version. gpio_set_value() only works for chips which do not sleep, gpio_set_value_cansleep() works for both those who do sleep and those who do not.

To the gpio-list: Has it been considered to have sth. like gpio_set_value and gpio_set_value_nosleep? I'd think it makes more sense to have the specific function have the specific name.

Regards,

Wolfram

#4 lore raw
Quoted 12 lines · Wolfram Sang
Quoted 9 lines · Lars-Peter Clausen

drivers/i2c/muxes/i2c-mux-gpio.c struct gpiomux { · line 38

Quoted 4 lines
drivers/i2c/muxes/i2c-mux-gpio.c +4 -0
+		if (gpio_cansleep(gpio))
+			gpio_set_value_cansleep(gpio, val & (1 << i));
+		else
+			gpio_set_value(gpio, val & (1 << i));

The proper way to do this is just always use the _cansleep() version. gpio_set_value() only works for chips which do not sleep, gpio_set_value_cansleep() works for both those who do sleep and those who do not.

Quoted 3 lines

To the gpio-list: Has it been considered to have sth. like gpio_set_value and gpio_set_value_nosleep? I'd think it makes more sense to have the specific function have the specific name.

It has been a few times, but I think the conclusion has always been that it is now too late to invert the semantics of gpio_set_value(). If you want to look up the discussions the keyword is gpio_set_value_atomic().

- Lars

#5 lore raw
In reply to Ionut Nicu
Quoted 20 lines · Ionut Nicu

Some gpio chips may have get/set operations that
can sleep. For this type of chips we must use the
_cansleep() version of gpio_set_value.

---
 drivers/i2c/muxes/i2c-mux-gpio.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)
drivers/i2c/muxes/i2c-mux-gpio.c +1 -0
index a764da7..b5f17ef 100644
@@ -27,11 +27,16 @@ struct gpiomux {
 
 static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
 {
+	unsigned gpio;
 	int i;
 

drivers/i2c/muxes/i2c-mux-gpio.c struct gpiomux { · line 39

Quoted 10 lines
drivers/i2c/muxes/i2c-mux-gpio.c +7 -3
-	for (i = 0; i < mux->data.n_gpios; i++)
-		gpio_set_value(mux->gpio_base + mux->data.gpios[i],
-			       val & (1 << i));
+	for (i = 0; i < mux->data.n_gpios; i++) {
+		gpio = mux->gpio_base + mux->data.gpios[i];
+		if (gpio_cansleep(gpio))
+			gpio_set_value_cansleep(gpio, val & (1 << i));
+		else
+			gpio_set_value(gpio, val & (1 << i));
+	}

There should be a wrapper for that in the gpio-subsystem IMO. Adding linux-gpio to cc.