Le 02 Janvier 2008, Jean Delvare a écrit:
>
> Hi David, hi Eric,
>
> Le 29/12/2007, "David Brownell" <[EMAIL PROTECTED]> a écrit:
> >From: eric miao <[EMAIL PROTECTED]>
> >
> >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, ®_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, ®_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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
