staging:iio:magnetometer:ak8975 Don't use irq_to_gpio()

PATCH In review Stephen Warren 4 participants 13 messages Aug 31, 2011 – Sep 1, 2011

Subsystems (4)

Staging IIO Tegra architecture support Staging - industrial io

lore

Share & tools

lore

Stephen Warren

author

2011-08-31 19:40 UTC #0 lore raw

PATCH 1/3 staging:iio:magnetometer:ak8975 Don't use irq_to_gpio()

Tegra doesn't have irq_to_gpio() any more, and ak8975 is included in tegra_defconfig. This causes a build failure. Solve this with a heavy-handed method for now.

I suspect the long-term solution is to pass both the IRQ and GPIO IDs to the driver; the GPIO ID coming from either platform data, or perhaps enhancing struct i2c_client to add a gpio field alongside irq.

Signed-off-by Stephen Warren <[email protected]>

---
Russell, now that irq_to_gpio() is going away, can you comment on how you'd like to fix drivers that do this kind of thing? Thanks.

 drivers/staging/iio/magnetometer/ak8975.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/staging/iio/magnetometer/ak8975.c +1 -1

index a17fa9f..bd40e32 100644
@@ -477,7 +477,7 @@ static int ak8975_probe(struct i2c_client *client,
 	int err;
 
 	/* Grab and set up the supplied GPIO. */
-	eoc_gpio = irq_to_gpio(client->irq);
+	eoc_gpio = -1; /* FIXME: irq_to_gpio(client->irq) */
 
 	/* We may not have a GPIO based IRQ to scan, that is fine, we will
 	   poll if so */

Signature · 1 line

1.7.0.4

Stephen Warren

author

2011-08-31 19:40 UTC #1 lore raw

PATCH 2/3 staging:iio:magnetometer:ak8975: Don't assume 0 is an invalid GPIO

gpio_is_valid() is the defined mechanism to determine whether a GPIO is valid. Use this instead of assuming that 0 is an invalid GPIO.

Signed-off-by Stephen Warren <[email protected]>

---
 drivers/staging/iio/magnetometer/ak8975.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

drivers/staging/iio/magnetometer/ak8975.c +5 -6

index bd40e32..79ed183 100644
@@ -373,7 +373,7 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 	}
 
 	/* Wait for the conversion to complete. */
-	if (data->eoc_gpio)
+	if (gpio_is_valid(data->eoc_gpio))
 		ret = wait_conversion_complete_gpio(data);
 	else
 		ret = wait_conversion_complete_polled(data);

@@ -481,7 +481,7 @@ static int ak8975_probe(struct i2c_client *client,
 
 	/* We may not have a GPIO based IRQ to scan, that is fine, we will
 	   poll if so */
-	if (eoc_gpio > 0) {
+	if (gpio_is_valid(eoc_gpio)) {
 		err = gpio_request(eoc_gpio, "ak_8975");
 		if (err < 0) {
 			dev_err(&client->dev,

@@ -497,8 +497,7 @@ static int ak8975_probe(struct i2c_client *client,
 						eoc_gpio, err);
 			goto exit_gpio;
 		}
-	} else
-		eoc_gpio = 0;	/* No GPIO available */
+	}
 
 	/* Register with IIO */
 	indio_dev = iio_allocate_device(sizeof(*data));

@@ -534,7 +533,7 @@ static int ak8975_probe(struct i2c_client *client,
 exit_free_iio:
 	iio_free_device(indio_dev);
 exit_gpio:
-	if (eoc_gpio)
+	if (gpio_is_valid(eoc_gpio))
 		gpio_free(eoc_gpio);
 exit:
 	return err;

@@ -549,7 +548,7 @@ static int ak8975_remove(struct i2c_client *client)
 	iio_device_unregister(indio_dev);
 	iio_free_device(indio_dev);
 
-	if (eoc_gpio)
+	if (gpio_is_valid(eoc_gpio))
 		gpio_free(eoc_gpio);
 
 	return 0;

Signature · 1 line

1.7.0.4

Jonathan Cameron

Acked-by on v1 2/3

2011-09-01 08:57 UTC #2 lore raw

Re: [PATCH 2/3] staging:iio:magnetometer:ak8975: Don't assume 0 is an invalid GPIO

Quoted 3 lines · Stephen Warren

gpio_is_valid() is the defined mechanism to determine whether a GPIO is valid. Use this instead of assuming that 0 is an invalid GPIO.

Greg, depending on merge order you will get some fuzz between this and something in my tree.

Quoted 1 line

Signed-off-by Stephen Warren <[email protected]>

Acked-by Jonathan Cameron <[email protected]>

Quoted 54 lines

---
 drivers/staging/iio/magnetometer/ak8975.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

drivers/staging/iio/magnetometer/ak8975.c +5 -6

index bd40e32..79ed183 100644
@@ -373,7 +373,7 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 	}
 
 	/* Wait for the conversion to complete. */
-	if (data->eoc_gpio)
+	if (gpio_is_valid(data->eoc_gpio))
 		ret = wait_conversion_complete_gpio(data);
 	else
 		ret = wait_conversion_complete_polled(data);

@@ -481,7 +481,7 @@ static int ak8975_probe(struct i2c_client *client,
 
 	/* We may not have a GPIO based IRQ to scan, that is fine, we will
 	   poll if so */
-	if (eoc_gpio > 0) {
+	if (gpio_is_valid(eoc_gpio)) {
 		err = gpio_request(eoc_gpio, "ak_8975");
 		if (err < 0) {
 			dev_err(&client->dev,

@@ -497,8 +497,7 @@ static int ak8975_probe(struct i2c_client *client,
 						eoc_gpio, err);
 			goto exit_gpio;
 		}
-	} else
-		eoc_gpio = 0;	/* No GPIO available */
+	}
 
 	/* Register with IIO */
 	indio_dev = iio_allocate_device(sizeof(*data));

@@ -534,7 +533,7 @@ static int ak8975_probe(struct i2c_client *client,
 exit_free_iio:
 	iio_free_device(indio_dev);
 exit_gpio:
-	if (eoc_gpio)
+	if (gpio_is_valid(eoc_gpio))
 		gpio_free(eoc_gpio);
 exit:
 	return err;

@@ -549,7 +548,7 @@ static int ak8975_remove(struct i2c_client *client)
 	iio_device_unregister(indio_dev);
 	iio_free_device(indio_dev);
 
-	if (eoc_gpio)
+	if (gpio_is_valid(eoc_gpio))
 		gpio_free(eoc_gpio);
 
 	return 0;

Stephen Warren

author

2011-08-31 19:40 UTC #3 lore raw

PATCH 3/3 staging:iio:magnetometer:ak8975: Fix probe() error-handling

In reply to Stephen Warren

Fix ak8975_probe() to jump to the appropriate exit labels when an error occurs. With the previous code, some cleanup actions were being skipped for some error conditions.

Signed-off-by Stephen Warren <[email protected]>

---
 drivers/staging/iio/magnetometer/ak8975.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/staging/iio/magnetometer/ak8975.c +1 -1

index 79ed183..d92b3d0 100644
@@ -510,7 +510,7 @@ static int ak8975_probe(struct i2c_client *client,
 	err = ak8975_setup(client);
 	if (err < 0) {
 		dev_err(&client->dev, "AK8975 initialization fails\n");
-		goto exit_gpio;
+		goto exit_free_iio;
 	}
 
 	i2c_set_clientdata(client, indio_dev);

Signature · 1 line

1.7.0.4

Jonathan Cameron

Acked-by on v1 3/3

2011-09-01 09:00 UTC #4 lore raw

Re: [PATCH 3/3] staging:iio:magnetometer:ak8975: Fix probe() error-handling

Quoted 5 lines · Stephen Warren

Fix ak8975_probe() to jump to the appropriate exit labels when an error occurs. With the previous code, some cleanup actions were being skipped for some error conditions.
Signed-off-by Stephen Warren <[email protected]>

Acked-by Jonathan Cameron <[email protected]>

Thanks for these Stephen. There's one other driver in tree that uses irq_to_gpio (I hadn't registered it was going away till you sent these.)

Quoted 17 lines

---
 drivers/staging/iio/magnetometer/ak8975.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/staging/iio/magnetometer/ak8975.c +1 -1

index 79ed183..d92b3d0 100644
@@ -510,7 +510,7 @@ static int ak8975_probe(struct i2c_client *client,
 	err = ak8975_setup(client);
 	if (err < 0) {
 		dev_err(&client->dev, "AK8975 initialization fails\n");
-		goto exit_gpio;
+		goto exit_free_iio;
 	}
 
 	i2c_set_clientdata(client, indio_dev);

Andrew Chew

Acked-by on v1 1/3

2011-08-31 19:45 UTC #5 lore raw

In reply to Stephen Warren

Quoted 13 lines

Subject: [PATCH 1/3] staging:iio:magnetometer:ak8975 Don't=20 use irq_to_gpio()
=20
Tegra doesn't have irq_to_gpio() any more, and ak8975 is included in tegra_defconfig. This causes a build failure. Solve this with=20 a heavy-handed
method for now.
=20
I suspect the long-term solution is to pass both the IRQ and GPIO IDs to the driver; the GPIO ID coming from either platform data,=20 or perhaps
enhancing struct i2c_client to add a gpio field alongside irq. =20

Quoted 2 lines

Signed-off-by Stephen Warren <[email protected]>
---

The three patches in this set LGTM.

Acked-by Andrew Chew <[email protected]>=20

Jonathan Cameron

2011-09-01 08:53 UTC #6 lore raw

Quoted 13 lines · Andrew Chew

Quoted 13 lines
Subject: [PATCH 1/3] staging:iio:magnetometer:ak8975 Don't use irq_to_gpio()
Tegra doesn't have irq_to_gpio() any more, and ak8975 is included in tegra_defconfig. This causes a build failure. Solve this with a heavy-handed
method for now.
I suspect the long-term solution is to pass both the IRQ and GPIO IDs to the driver; the GPIO ID coming from either platform data, or perhaps
enhancing struct i2c_client to add a gpio field alongside irq.

Quoted 7 lines

Quoted 2 lines
Signed-off-by Stephen Warren <[email protected]>
---
The three patches in this set LGTM.
Acked-by Andrew Chew <[email protected]>

Hmm.. I'd like to see some means of passing that in. Perhaps as simple as passing a pointer to an int in as platform_data. Patch to follow.

Jonathan Cameron

2011-09-01 08:56 UTC #7 lore raw

PATCH 1/1 staging:iio:magnetometer:ak8975 use platform_data to pass the gpio number.

Basically provide another means to set this value given irq_to_gpio has gone away.

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/staging/iio/magnetometer/ak8975.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

drivers/staging/iio/magnetometer/ak8975.c +4 -1

index c1ff430..ab0e26c 100644
@@ -477,7 +477,10 @@ static int ak8975_probe(struct i2c_client *client,
 	int err;
 
 	/* Grab and set up the supplied GPIO. */
-	eoc_gpio = -1; /* FIXME: irq_to_gpio(client->irq) */
+	if (client->dev.platform_data == NULL)
+		eoc_gpio = -1;
+	else
+		eoc_gpio = *(int *)(client->dev.platform_data);
 
 	/* We may not have a GPIO based IRQ to scan, that is fine, we will
 	   poll if so */

Signature · 1 line

1.7.3.4

Arnd Bergmann

2011-09-01 11:06 UTC #8 lore raw

In reply to Jonathan Cameron

Quoted 17 lines · Jonathan Cameron

Quoted 16 lines · Andrew Chew
Quoted 15 lines
Subject: [PATCH 1/3] staging:iio:magnetometer:ak8975 Don't use irq_to_gpio()
Tegra doesn't have irq_to_gpio() any more, and ak8975 is included in tegra_defconfig. This causes a build failure. Solve this with a heavy-handed
method for now.
I suspect the long-term solution is to pass both the IRQ and GPIO IDs to the driver; the GPIO ID coming from either platform data, or perhaps
enhancing struct i2c_client to add a gpio field alongside irq.
Signed-off-by Stephen Warren <[email protected]>
---

Quoted 7 lines

Quoted 4 lines
The three patches in this set LGTM.
Acked-by Andrew Chew <[email protected]>
Hmm.. I'd like to see some means of passing that in. Perhaps as simple as passing a pointer to an int in as platform_data. Patch to follow.

My feeling is that we should just add another field to struct i2c_client. There are probably more drivers that need the same thing, making it appropriate to increase the size of that struct for everything device instead of adding platform_data to a subset of the devices.

Arnd

Stephen Warren

author

2011-09-01 15:36 UTC #9 lore raw

Arnd Bergmann wrote at Thursday, September 01, 2011 5:07 AM:

Quoted 11 lines · Jonathan Cameron

Quoted 10 lines · Andrew Chew
Quoted 9 lines
Quoted 9 lines
Subject: [PATCH 1/3] staging:iio:magnetometer:ak8975 Don't use irq_to_gpio()
Tegra doesn't have irq_to_gpio() any more, and ak8975 is included in tegra_defconfig. This causes a build failure. Solve this with a heavy-handed
method for now.
I suspect the long-term solution is to pass both the IRQ and GPIO ID=

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
to the driver; the GPIO ID coming from either platform data, or perhaps
enhancing struct i2c_client to add a gpio field alongside irq.
Signed-off-by Stephen Warren <[email protected]>
---

Quoted 6 lines

Quoted 6 lines
Quoted 4 lines
The three patches in this set LGTM.
Acked-by Andrew Chew <[email protected]>
Hmm.. I'd like to see some means of passing that in. Perhaps as simple =

as passing

Quoted 7 lines

Quoted 1 line
a pointer to an int in as platform_data. Patch to follow.
=20
My feeling is that we should just add another field to struct i2c_client. There are probably more drivers that need the same thing, making it appropriate to increase the size of that struct for everything device instead of adding platform_data to a subset of the devices.

That sounds reasonable to me.

One question: When we add this field, how do drivers tell whether a value of 0 is an uninitialized field, or a legitimate GPIO value of 0? Should we add a flag to indicate validity, or just work hard to not enable driver- side code to use this value until we've fixed up all places that instantiat= e
the driver to initialize the field to some invalid value like -1?

--=20
nvpublic

Arnd Bergmann

2011-09-01 16:20 UTC #10 lore raw

Quoted 5 lines · Stephen Warren

One question: When we add this field, how do drivers tell whether a value of 0 is an uninitialized field, or a legitimate GPIO value of 0? Should we add a flag to indicate validity, or just work hard to not enable driver- side code to use this value until we've fixed up all places that instantiate the driver to initialize the field to some invalid value like -1?

I think it's enough to coordinate the driver with the initialization of the i2c data. If a driver requires a GPIO number, it can assume that it's valid. Drivers that don't need one don't care. If it's an optional feature, you might want to either use platform_data after all, or use different identifiers for devices that have a gpio vs. those that have none -- in effect those are different types of devices handled by the same driver.

Arnd

Jonathan Cameron

Acked-by on v1 1/3

2011-09-01 08:59 UTC #11 lore raw

In reply to Stephen Warren

Quoted 7 lines · Stephen Warren

Tegra doesn't have irq_to_gpio() any more, and ak8975 is included in tegra_defconfig. This causes a build failure. Solve this with a heavy-handed method for now.
I suspect the long-term solution is to pass both the IRQ and GPIO IDs to the driver; the GPIO ID coming from either platform data, or perhaps enhancing struct i2c_client to add a gpio field alongside irq.

Definitely on the platform data front. We need some means of doing this now hence my patch putting most trivial form of that in.

Quoted 2 lines

Signed-off-by Stephen Warren <[email protected]>

Acked-by Jonathan Cameron <[email protected]>

Quoted 20 lines

---
Russell, now that irq_to_gpio() is going away, can you comment on how you'd like to fix drivers that do this kind of thing? Thanks.

 drivers/staging/iio/magnetometer/ak8975.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/staging/iio/magnetometer/ak8975.c +1 -1

index a17fa9f..bd40e32 100644
@@ -477,7 +477,7 @@ static int ak8975_probe(struct i2c_client *client,
 	int err;
 
 	/* Grab and set up the supplied GPIO. */
-	eoc_gpio = irq_to_gpio(client->irq);
+	eoc_gpio = -1; /* FIXME: irq_to_gpio(client->irq) */
 
 	/* We may not have a GPIO based IRQ to scan, that is fine, we will
 	   poll if so */

Jonathan Cameron

2011-09-01 09:16 UTC #12 lore raw

Quoted 10 lines · Jonathan Cameron

Quoted 7 lines · Stephen Warren
Tegra doesn't have irq_to_gpio() any more, and ak8975 is included in tegra_defconfig. This causes a build failure. Solve this with a heavy-handed method for now.
I suspect the long-term solution is to pass both the IRQ and GPIO IDs to the driver; the GPIO ID coming from either platform data, or perhaps enhancing struct i2c_client to add a gpio field alongside irq.
Definitely on the platform data front. We need some means of doing this now hence my patch putting most trivial form of that in.

Actually taking the time to look at the code...

Can someone explain to me (I have a feeling I may just have forgotten this) why we aren't using an interrupt here? It looks like a bus transaction triggered read. So why not do a enable_irq followed by triggering the read and use a waitqueue to wait for an interrupt handler to signal it is done?

Having found the datahsheeting lying around on the net I can't see why this won't work and be somewhat cleaner than current polling approach.

Jonathan

Quoted 28 lines

Quoted 1 line
Signed-off-by Stephen Warren <[email protected]>
Acked-by Jonathan Cameron <[email protected]>
Quoted 12 lines
---
Russell, now that irq_to_gpio() is going away, can you comment on how you'd like to fix drivers that do this kind of thing? Thanks.
 drivers/staging/iio/magnetometer/ak8975.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/staging/iio/magnetometer/ak8975.c +0 -0
index a17fa9f..bd40e32 100644
drivers/staging/iio/magnetometer/ak8975.c static int ak8975_probe(struct i2c_client *client, · line 483
Quoted 8 lines
drivers/staging/iio/magnetometer/ak8975.c +1 -1
@@ -477,7 +477,7 @@ static int ak8975_probe(struct i2c_client *client,
 	int err;
 
 	/* Grab and set up the supplied GPIO. */
-	eoc_gpio = irq_to_gpio(client->irq);
+	eoc_gpio = -1; /* FIXME: irq_to_gpio(client->irq) */
 
 	/* We may not have a GPIO based IRQ to scan, that is fine, we will
 	   poll if so */
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html