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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 3 Jan 2008 17:41:16 -0800
From:	David Brownell <david-b@...bell.net>
To:	"Jean Delvare" <khali@...ux-fr.org>
Cc:	"eric miao" <eric.y.miao@...il.com>,
	"Andrew Morton" <akpm@...ux-foundation.org>,
	"Linux Kernel list" <linux-kernel@...r.kernel.org>
Subject: Re: [patch 2.6.24-rc6-mm 8/9] gpiolib: pca9539 i2c gpio expander support

Le 02 Janvier 2008, Jean Delvare a écrit:
> 
> Hi David, hi Eric,
> 
> Le 29/12/2007, "David Brownell" <david-b@...bell.net> a écrit:
> >From: eric miao <eric.miao@...vell.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;
 	}
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/