Skip to contentSkip to conversation

GPIO: Fix probe() error return in gpio driver probes

PATCH Ben Dooks 3 participants 9 messages Dec 12, 2008 – Dec 18, 2008

Subsystems (2)
GPIO I2C subsystem host drivers
lore
Share & tools
lore

Ben Dooks

author
#0 lore raw

A number of drivers in drivers/gpio return -ENODEV when confronted with missing setup parameters such as the platform data. However, returning -ENODEV causes the driver layer to silently ignore the driver as it assumes the probe did not find anything and was only speculative.

To make life easier to discern why a driver is not being attached, change to returning -EINVAL, which is a better description of the fact that the driver data was not valid.

Signed-off-by Ben Dooks <[email protected]>
drivers/gpio/max7301.c +1 -1
Index: linux.git7/drivers/gpio/max7301.c
===================================================================
@@ -218,7 +218,7 @@ static int __devinit max7301_probe(struc
 
 	pdata = spi->dev.platform_data;
 	if (!pdata || !pdata->base)
-		return -ENODEV;
+		return -EINVAL;
 
 	/*
 	 * bits_per_word cannot be configured in platform data
drivers/gpio/max732x.c +1 -1
Index: linux.git7/drivers/gpio/max732x.c
===================================================================
@@ -268,7 +268,7 @@ static int __devinit max732x_probe(struc
 
 	pdata = client->dev.platform_data;
 	if (pdata == NULL)
-		return -ENODEV;
+		return -EINVAL;
 
 	chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL);
 	if (chip == NULL)
drivers/gpio/mcp23s08.c +1 -1
Index: linux.git7/drivers/gpio/mcp23s08.c
===================================================================
@@ -311,7 +311,7 @@ static int mcp23s08_probe(struct spi_dev
 
 	pdata = spi->dev.platform_data;
 	if (!pdata || !gpio_is_valid(pdata->base))
-		return -ENODEV;
+		return -EINVAL;
 
 	for (addr = 0; addr < 4; addr++) {
 		if (!pdata->chip[addr].is_present)
drivers/gpio/pca953x.c +1 -1
Index: linux.git7/drivers/gpio/pca953x.c
===================================================================
@@ -201,7 +201,7 @@ static int __devinit pca953x_probe(struc
 
 	pdata = client->dev.platform_data;
 	if (pdata == NULL)
-		return -ENODEV;
+		return -EINVAL;
 
 	chip = kzalloc(sizeof(struct pca953x_chip), GFP_KERNEL);
 	if (chip == NULL)
drivers/gpio/pcf857x.c +2 -2
Index: linux.git7/drivers/gpio/pcf857x.c
===================================================================
@@ -189,7 +189,7 @@ static int pcf857x_probe(struct i2c_clie
 
 	pdata = client->dev.platform_data;
 	if (!pdata)
-		return -ENODEV;
+		return -EINVAL;
 
 	/* Allocate, initialize, and register this gpio_chip. */
 	gpio = kzalloc(sizeof *gpio, GFP_KERNEL);

@@ -249,7 +249,7 @@ static int pcf857x_probe(struct i2c_clie
 			status = i2c_read_le16(client);
 
 	} else
-		status = -ENODEV;
+		status = -EINVAL;
 
 	if (status < 0)
 		goto fail;
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
author
#1 lore raw
Quoted 11 lines · Ben Dooks

A number of drivers in drivers/gpio return -ENODEV when confronted with missing setup parameters such as the platform data. However, returning -ENODEV causes the driver layer to silently ignore the driver as it assumes the probe did not find anything and was only speculative.

To make life easier to discern why a driver is not being attached, change to returning -EINVAL, which is a better description of the fact that the driver data was not valid.

Signed-off-by Ben Dooks <[email protected]>

Has anyone reveiwed this patch? Are there any comments, or can this be commited at somepoint (even if it is during the next merge window)?

Quoted 84 lines

Index: linux.git7/drivers/gpio/max7301.c
===================================================================

drivers/gpio/max7301.c +1 -1
@@ -218,7 +218,7 @@ static int __devinit max7301_probe(struc
 
 	pdata = spi->dev.platform_data;
 	if (!pdata || !pdata->base)
-		return -ENODEV;
+		return -EINVAL;
 
 	/*
 	 * bits_per_word cannot be configured in platform data

Index: linux.git7/drivers/gpio/max732x.c
===================================================================

drivers/gpio/max732x.c +1 -1
@@ -268,7 +268,7 @@ static int __devinit max732x_probe(struc
 
 	pdata = client->dev.platform_data;
 	if (pdata == NULL)
-		return -ENODEV;
+		return -EINVAL;
 
 	chip = kzalloc(sizeof(struct max732x_chip), GFP_KERNEL);
 	if (chip == NULL)

Index: linux.git7/drivers/gpio/mcp23s08.c
===================================================================

drivers/gpio/mcp23s08.c +1 -1
@@ -311,7 +311,7 @@ static int mcp23s08_probe(struct spi_dev
 
 	pdata = spi->dev.platform_data;
 	if (!pdata || !gpio_is_valid(pdata->base))
-		return -ENODEV;
+		return -EINVAL;
 
 	for (addr = 0; addr < 4; addr++) {
 		if (!pdata->chip[addr].is_present)

Index: linux.git7/drivers/gpio/pca953x.c
===================================================================

drivers/gpio/pca953x.c +1 -1
@@ -201,7 +201,7 @@ static int __devinit pca953x_probe(struc
 
 	pdata = client->dev.platform_data;
 	if (pdata == NULL)
-		return -ENODEV;
+		return -EINVAL;
 
 	chip = kzalloc(sizeof(struct pca953x_chip), GFP_KERNEL);
 	if (chip == NULL)

Index: linux.git7/drivers/gpio/pcf857x.c
===================================================================

drivers/gpio/pcf857x.c +2 -2
@@ -189,7 +189,7 @@ static int pcf857x_probe(struct i2c_clie
 
 	pdata = client->dev.platform_data;
 	if (!pdata)
-		return -ENODEV;
+		return -EINVAL;
 
 	/* Allocate, initialize, and register this gpio_chip. */
 	gpio = kzalloc(sizeof *gpio, GFP_KERNEL);

@@ -249,7 +249,7 @@ static int pcf857x_probe(struct i2c_clie
 			status = i2c_read_le16(client);
 
 	} else
-		status = -ENODEV;
+		status = -EINVAL;
 
 	if (status < 0)
 		goto fail;
Signature · 9 lines
-- 
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
--
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/
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
#2 lore raw
Quoted 2 lines · Ben Dooks

Has anyone reveiwed this patch? Are there any comments, or can this be commited at somepoint (even if it is during the next merge window)?

I was thinking that -EINVAL is almost the least informative diagnostic code possible, since so many places return it that it's usually hard to find out *which* invalid parameter triggered ...

Is there a less-overloaded code you could return?

I have no issue with the patch other than that.

- Dave

#3 lore raw
Quoted 10 lines · David Brownell
Quoted 2 lines · Ben Dooks

Has anyone reveiwed this patch? Are there any comments, or can this be commited at somepoint (even if it is during the next merge window)?

I was thinking that -EINVAL is almost the least informative diagnostic code possible, since so many places return it that it's usually hard to find out *which* invalid parameter triggered ...

Is there a less-overloaded code you could return?

-EINVAL sounds right to me, all that's really missing is dev_dbg() messages in the drivers to log what the exact problem was.

Quoted 1 line

I have no issue with the patch other than that.

Signature · 1 line
Jean Delvare
author
#4 lore raw
Quoted 10 lines · Jean Delvare
Quoted 9 lines · David Brownell
Quoted 2 lines · Ben Dooks

Has anyone reveiwed this patch? Are there any comments, or can this be commited at somepoint (even if it is during the next merge window)?

I was thinking that -EINVAL is almost the least informative diagnostic code possible, since so many places return it that it's usually hard to find out *which* invalid parameter triggered ...

Quoted 4 lines
Quoted 1 line

Is there a less-overloaded code you could return?

-EINVAL sounds right to me, all that's really missing is dev_dbg() messages in the drivers to log what the exact problem was.

It might be more acceptable to be dev_err(), that way it will get printed no matter what debug options have been selected. If so, a seperate patch is probably in order to make the change.

Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
#5 lore raw
Quoted 11 lines · Ben Dooks
Quoted 10 lines · Jean Delvare
Quoted 9 lines · David Brownell
Quoted 2 lines · Ben Dooks

Has anyone reveiwed this patch? Are there any comments, or can this be commited at somepoint (even if it is during the next merge window)?

I was thinking that -EINVAL is almost the least informative diagnostic code possible, since so many places return it that it's usually hard to find out *which* invalid parameter triggered ...

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

Is there a less-overloaded code you could return?

-EINVAL sounds right to me, all that's really missing is dev_dbg() messages in the drivers to log what the exact problem was.

It might be more acceptable to be dev_err(), that way it will get printed no matter what debug options have been selected. If so, a seperate patch is probably in order to make the change.

As far as I can see, such errors would be caused by development-time mistakes, so dev_dbg() seems appropriate. dev_err() would make the binaries larger for all end-users.

Signature · 1 line
Jean Delvare
#6 lore raw
Quoted 10 lines · Jean Delvare
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines

I was thinking that -EINVAL is almost the least informative diagnostic code possible, since so many places return it that it's usually hard to find out *which* invalid parameter triggered ...

Is there a less-overloaded code you could return?

-EINVAL sounds right to me, all that's really missing is dev_dbg() messages in the drivers to log what the exact problem was.

Fair enough, though it just papers over how ambiguous -EINVAL is.

Quoted 7 lines
Quoted 3 lines

It might be more acceptable to be dev_err(), that way it will get printed no matter what debug options have been selected. If so, a seperate patch is probably in order to make the change.

As far as I can see, such errors would be caused by development-time mistakes, so dev_dbg() seems appropriate. dev_err() would make the binaries larger for all end-users.

Right, dev_dbg() is the way to go. I'd ack a version of this patch which pairs these -EINVAL changes with dev_dbg() messages to make these problems less painful to track down. dev_err() is much abused.

- Dave

author
#7 lore raw
Quoted 7 lines · David Brownell
Quoted 6 lines · Jean Delvare
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

I was thinking that -EINVAL is almost the least informative diagnostic code possible, since so many places return it that it's usually hard to find out *which* invalid parameter triggered ...

Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 1 line

Is there a less-overloaded code you could return?

-EINVAL sounds right to me, all that's really missing is dev_dbg() messages in the drivers to log what the exact problem was.

Fair enough, though it just papers over how ambiguous -EINVAL is.

Unforunately there's not a lot of choice in errno.h for other options.

Quoted 11 lines
Quoted 7 lines
Quoted 3 lines

It might be more acceptable to be dev_err(), that way it will get printed no matter what debug options have been selected. If so, a seperate patch is probably in order to make the change.

As far as I can see, such errors would be caused by development-time mistakes, so dev_dbg() seems appropriate. dev_err() would make the binaries larger for all end-users.

Right, dev_dbg() is the way to go. I'd ack a version of this patch which pairs these -EINVAL changes with dev_dbg() messages to make these problems less painful to track down. dev_err() is much abused.

Ok, I'll try and sort that out for you as soon as possible.

Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
author
#8 lore raw
In reply to David Brownell
Quoted 10 lines · David Brownell
Quoted 2 lines · Ben Dooks

Has anyone reveiwed this patch? Are there any comments, or can this be commited at somepoint (even if it is during the next merge window)?

I was thinking that -EINVAL is almost the least informative diagnostic code possible, since so many places return it that it's usually hard to find out *which* invalid parameter triggered ...

Is there a less-overloaded code you could return?

The only other ones that I think would be close are:

ENOTSUP Operation not supported (POSIX.1)
EPROTO Protocol error (POSIX.1)
ENOENT No such file or directory
EFAULT Bad address

Feedback welcone.

Quoted 8 lines

I have no issue with the patch other than that.

- Dave
--
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/

Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'