Re: [patch 2.6.24-rc6-mm 8/9] gpiolib: pca9539 i2c gpio expander support

[David Brownell <david-b@pacbell.net>]

Le 02 Janvier 2008, Jean Delvare a écrit:
> 
> Hi David, hi Eric,
> 
> Le 29/12/2007, "David Brownell" <david-b@pacbell.net> a écrit:
> >From: eric miao <eric.miao@marvell.com>
> >
> >This adds a new-style I2C driver with basic support for the sixteen
> >bit PCA9539 GPIO expanders.
> >
> >			... 
> 
> Random comments:
> 
> >+static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off)
> >+{
> >+		...
> >+
> >+	ret = pca9539_read_reg(chip, PCA9539_INPUT, &reg_val);
> >+	if (ret < 0) {
> >+		/* NOTE:  diagnostic already omitted; that's the
> >+		 * best we can do here.
> >+		 */
> >+		return 0;
> >+	}
> 
> I guess that you really mean "emitted" here, not "omitted"?

Yeah, typo.


> More importantly, I don't agree that it's the best we can do here.
> Maybe it was already discussed before and there's a good reason to not
> report errors from "get" functions at the gpio-core level,

Yes there is.  It's by explicit request.  Expecting drivers to cope
with per-bit errors is at best unrealistic.  This was decided well
over a year ago ... nobody wants to see bit-banging code that spends
more time trying to figure out how to recover from "can't happen"
errors than getting real work done.  (None of the SOC-specific GPIO
interfaces being replaced by this generic one returned errors either.)

That said, with things like I2C there actually *could* be errors;
which are impossible with valid parameters to SOC-level GPIOs.
That might argue for gpio_{get,set}_value_cansleep() calls being
able to return fault codes that would be nonsense on the more
widely used gpio_{get,set}_value() alls.

But such a change would be for a different set of patches.  This
set does not change *any* driver programming interface.  At all.


> >+static int __devinit pca9539_probe(struct i2c_client *client)
> >+{
> >+ (...)
> >+	if (pdata->setup) {
> >+		ret = pdata->setup(client, chip->gpio_chip.base,
> >+				chip->gpio_chip.ngpio, pdata->context);
> >+		if (ret < 0)
> >+			dev_dbg(&client->dev, "setup failed, %d\n", ret);
> 
> Should be at least dev_warn() and maybe even dev_err().

It's not treated as an error (i.e. abort the probe); warning
is right.

Hmm, I thought both this issue and the previous one had been
fixed already ... oh, it was the pcf857x driver that fixed that.
Never mind.  ;)


> >+	}
> >+ (...)
> >+}
> >+
> >+static int pca9539_remove(struct i2c_client *client)
> >+{
> >+ (...)
> >+	if (pdata->teardown) {
> >+		ret = pdata->teardown(client, chip->gpio_chip.base,
> >+				chip->gpio_chip.ngpio, pdata->context);
> >+		if (ret < 0)
> >+			dev_dbg(&client->dev, "teardown failed, %d\n", ret);
> 
> Same thing here.

That was supposed to be dev_err() then "return ret" !


> >+	}
> >+
> >+	ret = gpiochip_remove(&chip->gpio_chip);
> >+	if (ret) {
> >+		dev_err(&client->dev, "failed remove gpio_chip\n");
> 
> This error message could certainly be reworded to sound better. Also, for
> consistency you should include the value of "ret" in the message.

Right.  So, pretty much like the appended.  (Which I'll merge into
refreshed version of this patch.)


--- a/drivers/gpio/pca9539.c
+++ b/drivers/gpio/pca9539.c
@@ -118,7 +118,7 @@ static int pca9539_gpio_get_value(struct
 
 	ret = pca9539_read_reg(chip, PCA9539_INPUT, &reg_val);
 	if (ret < 0) {
-		/* NOTE:  diagnostic already omitted; that's the
+		/* NOTE:  diagnostic already emitted; that's the
 		 * best we can do here.
 		 */
 		return 0;
@@ -205,7 +205,7 @@ static int __devinit pca9539_probe(struc
 		ret = pdata->setup(client, chip->gpio_chip.base,
 				chip->gpio_chip.ngpio, pdata->context);
 		if (ret < 0)
-			dev_dbg(&client->dev, "setup failed, %d\n", ret);
+			dev_warn(&client->dev, "setup failed, %d\n", ret);
 	}
 
 	i2c_set_clientdata(client, chip);
@@ -225,13 +225,17 @@ static int pca9539_remove(struct i2c_cli
 	if (pdata->teardown) {
 		ret = pdata->teardown(client, chip->gpio_chip.base,
 				chip->gpio_chip.ngpio, pdata->context);
-		if (ret < 0)
-			dev_dbg(&client->dev, "teardown failed, %d\n", ret);
+		if (ret < 0) {
+			dev_err(&client->dev, "%s failed, %d\n",
+					"teardown", ret);
+			return ret;
+		}
 	}
 
 	ret = gpiochip_remove(&chip->gpio_chip);
 	if (ret) {
-		dev_err(&client->dev, "failed remove gpio_chip\n");
+		dev_err(&client->dev, "%s failed, %d\n",
+				"gpiochip_remove()", ret);
 		return ret;
 	}