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

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

[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.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
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(-)

diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
index a17fa9f..bd40e32 100644
--- a/drivers/staging/iio/magnetometer/ak8975.c
+++ b/drivers/staging/iio/magnetometer/ak8975.c
@@ -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 */
-- 
1.7.0.4

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

[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.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/staging/iio/magnetometer/ak8975.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
index bd40e32..79ed183 100644
--- a/drivers/staging/iio/magnetometer/ak8975.c
+++ b/drivers/staging/iio/magnetometer/ak8975.c
@@ -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;
-- 
1.7.0.4

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

[Jonathan Cameron]

On 08/31/11 20:40, Stephen Warren wrote:
> 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.

> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> ---
>  drivers/staging/iio/magnetometer/ak8975.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
> index bd40e32..79ed183 100644
> --- a/drivers/staging/iio/magnetometer/ak8975.c
> +++ b/drivers/staging/iio/magnetometer/ak8975.c
> @@ -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;

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

[Andrew Chew]

> 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 <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---

The three patches in this set LGTM.

Acked-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 

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

[Jonathan Cameron]

On 08/31/11 20:45, Andrew Chew wrote:
>> 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 <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
> 
> The three patches in this set LGTM.
> 
> Acked-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 
> 

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.

[PATCH] staging:iio:magnetometer:ak8975 use platform_data to pass the gpio number.

[Jonathan Cameron]

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

Signed-off-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
---
 drivers/staging/iio/magnetometer/ak8975.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
index c1ff430..ab0e26c 100644
--- a/drivers/staging/iio/magnetometer/ak8975.c
+++ b/drivers/staging/iio/magnetometer/ak8975.c
@@ -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 */
-- 
1.7.3.4

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

[Arnd Bergmann]

On Thursday 01 September 2011, Jonathan Cameron wrote:
> On 08/31/11 20:45, Andrew Chew wrote:
> >> 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 <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> ---
> > 
> > The three patches in this set LGTM.
> > 
> > Acked-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 
> > 
> 
> 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

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

[Stephen Warren]

Arnd Bergmann wrote at Thursday, September 01, 2011 5:07 AM:
> On Thursday 01 September 2011, Jonathan Cameron wrote:
> > On 08/31/11 20:45, Andrew Chew wrote:
> > >> 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 <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > >> ---
> > >
> > > The three patches in this set LGTM.
> > >
> > > Acked-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > >
> >
> > 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.

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 instantiate
the driver to initialize the field to some invalid value like -1?

-- 
nvpublic

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

[Arnd Bergmann]

On Thursday 01 September 2011, Stephen Warren wrote:
> 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

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

[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 <swarren@nvidia.com>
---
 drivers/staging/iio/magnetometer/ak8975.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
index 79ed183..d92b3d0 100644
--- a/drivers/staging/iio/magnetometer/ak8975.c
+++ b/drivers/staging/iio/magnetometer/ak8975.c
@@ -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);
-- 
1.7.0.4

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

[Jonathan Cameron]

On 08/31/11 20:40, Stephen Warren wrote:
> 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 <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>

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.)

> ---
>  drivers/staging/iio/magnetometer/ak8975.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
> index 79ed183..d92b3d0 100644
> --- a/drivers/staging/iio/magnetometer/ak8975.c
> +++ b/drivers/staging/iio/magnetometer/ak8975.c
> @@ -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);

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

[Jonathan Cameron]

On 08/31/11 20:40, Stephen Warren wrote:
> 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.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
> 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(-)
> 
> diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
> index a17fa9f..bd40e32 100644
> --- a/drivers/staging/iio/magnetometer/ak8975.c
> +++ b/drivers/staging/iio/magnetometer/ak8975.c
> @@ -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 */

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

[Jonathan Cameron]

On 09/01/11 10:07, Jonathan Cameron wrote:
> On 08/31/11 20:40, Stephen Warren wrote:
>> 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
>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
>> index a17fa9f..bd40e32 100644
>> --- a/drivers/staging/iio/magnetometer/ak8975.c
>> +++ b/drivers/staging/iio/magnetometer/ak8975.c
>> @@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>