diff mbox

media: adv7604: improve usage of gpiod API

Message ID 1425279644-25873-1-git-send-email-u.kleine-koenig@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Uwe Kleine-König March 2, 2015, 7 a.m. UTC 
Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
which appeared in v3.17-rc1, the gpiod_get* functions take an additional
parameter that allows to specify direction and initial value for output.
Simplify accordingly.

Moreover use devm_gpiod_get_index_optional instead of
devm_gpiod_get_index with ignoring all errors.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
BTW, sparse fails to check this file with many errors like:

	drivers/media/i2c/adv7604.c:311:11: error: unknown field name in initializer

Didn't look into that.
---
 drivers/media/i2c/adv7604.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

Comments

Hans Verkuil March 3, 2015, 9:55 a.m. UTC | #1 
Hi Uwe,

On 03/02/2015 08:00 AM, Uwe Kleine-König wrote:
> Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
> which appeared in v3.17-rc1, the gpiod_get* functions take an additional
> parameter that allows to specify direction and initial value for output.
> Simplify accordingly.
> 
> Moreover use devm_gpiod_get_index_optional instead of
> devm_gpiod_get_index with ignoring all errors.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> BTW, sparse fails to check this file with many errors like:
> 
> 	drivers/media/i2c/adv7604.c:311:11: error: unknown field name in initializer
> 
> Didn't look into that.

That's a sparse bug that's been fixed in the sparse repo, but not in the 0.5.0
release (they really should make a new sparse release IMHO).

Some comments below:

> ---
>  drivers/media/i2c/adv7604.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 5a7c9389a605..ddeeb6695a4b 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -537,12 +537,8 @@ static void adv7604_set_hpd(struct adv7604_state *state, unsigned int hpd)
>  {
>  	unsigned int i;
>  
> -	for (i = 0; i < state->info->num_dv_ports; ++i) {
> -		if (IS_ERR(state->hpd_gpio[i]))
> -			continue;

Why this change? See also below:

> -
> +	for (i = 0; i < state->info->num_dv_ports; ++i)
>  		gpiod_set_value_cansleep(state->hpd_gpio[i], hpd & BIT(i));
> -	}
>  
>  	v4l2_subdev_notify(&state->sd, ADV7604_HOTPLUG, &hpd);
>  }
> @@ -2720,13 +2716,13 @@ static int adv7604_probe(struct i2c_client *client,
>  	/* Request GPIOs. */
>  	for (i = 0; i < state->info->num_dv_ports; ++i) {
>  		state->hpd_gpio[i] =
> -			devm_gpiod_get_index(&client->dev, "hpd", i);
> +			devm_gpiod_get_index_optional(&client->dev, "hpd", i,
> +						      GPIOD_OUT_LOW);
>  		if (IS_ERR(state->hpd_gpio[i]))
> -			continue;
> -
> -		gpiod_direction_output(state->hpd_gpio[i], 0);
> +			return PTR_ERR(state->hpd_gpio[i]);

This isn't correct. The use of gpio is optional, on some boards a different
mechanism is used to control the hpd, and there devm_gpiod_get_index will just
return an error. That's OK, we just continue in that case.

Regards,

	Hans

>  
> -		v4l_info(client, "Handling HPD %u GPIO\n", i);
> +		if (state->hpd_gpio[i])
> +			v4l_info(client, "Handling HPD %u GPIO\n", i);
>  	}
>  
>  	state->timings = cea640x480;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil March 3, 2015, 9:59 a.m. UTC | #2 
On 03/03/2015 10:55 AM, Hans Verkuil wrote:
> Hi Uwe,
> 
> On 03/02/2015 08:00 AM, Uwe Kleine-König wrote:
>> Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
>> which appeared in v3.17-rc1, the gpiod_get* functions take an additional
>> parameter that allows to specify direction and initial value for output.
>> Simplify accordingly.
>>
>> Moreover use devm_gpiod_get_index_optional instead of
>> devm_gpiod_get_index with ignoring all errors.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>> BTW, sparse fails to check this file with many errors like:
>>
>> 	drivers/media/i2c/adv7604.c:311:11: error: unknown field name in initializer
>>
>> Didn't look into that.
> 
> That's a sparse bug that's been fixed in the sparse repo, but not in the 0.5.0
> release (they really should make a new sparse release IMHO).
> 
> Some comments below:

Never mind those comments, after checking what devm_gpiod_get_index_optional
does it's clear that this patch is correct.

Sorry about the noise.

	Hans

> 
>> ---
>>  drivers/media/i2c/adv7604.c | 16 ++++++----------
>>  1 file changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
>> index 5a7c9389a605..ddeeb6695a4b 100644
>> --- a/drivers/media/i2c/adv7604.c
>> +++ b/drivers/media/i2c/adv7604.c
>> @@ -537,12 +537,8 @@ static void adv7604_set_hpd(struct adv7604_state *state, unsigned int hpd)
>>  {
>>  	unsigned int i;
>>  
>> -	for (i = 0; i < state->info->num_dv_ports; ++i) {
>> -		if (IS_ERR(state->hpd_gpio[i]))
>> -			continue;
> 
> Why this change? See also below:
> 
>> -
>> +	for (i = 0; i < state->info->num_dv_ports; ++i)
>>  		gpiod_set_value_cansleep(state->hpd_gpio[i], hpd & BIT(i));
>> -	}
>>  
>>  	v4l2_subdev_notify(&state->sd, ADV7604_HOTPLUG, &hpd);
>>  }
>> @@ -2720,13 +2716,13 @@ static int adv7604_probe(struct i2c_client *client,
>>  	/* Request GPIOs. */
>>  	for (i = 0; i < state->info->num_dv_ports; ++i) {
>>  		state->hpd_gpio[i] =
>> -			devm_gpiod_get_index(&client->dev, "hpd", i);
>> +			devm_gpiod_get_index_optional(&client->dev, "hpd", i,
>> +						      GPIOD_OUT_LOW);
>>  		if (IS_ERR(state->hpd_gpio[i]))
>> -			continue;
>> -
>> -		gpiod_direction_output(state->hpd_gpio[i], 0);
>> +			return PTR_ERR(state->hpd_gpio[i]);
> 
> This isn't correct. The use of gpio is optional, on some boards a different
> mechanism is used to control the hpd, and there devm_gpiod_get_index will just
> return an error. That's OK, we just continue in that case.
> 
> Regards,
> 
> 	Hans
> 
>>  
>> -		v4l_info(client, "Handling HPD %u GPIO\n", i);
>> +		if (state->hpd_gpio[i])
>> +			v4l_info(client, "Handling HPD %u GPIO\n", i);
>>  	}
>>  
>>  	state->timings = cea640x480;
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Uwe Kleine-König March 3, 2015, 10:09 a.m. UTC | #3 
Hello Hans,

On Tue, Mar 03, 2015 at 10:59:22AM +0100, Hans Verkuil wrote:
> Never mind those comments, after checking what devm_gpiod_get_index_optional
> does it's clear that this patch is correct.
> 
> Sorry about the noise.
No problem. Is this an Ack then? Who picks up this patch?

Best regards
Uwe
Hans Verkuil March 3, 2015, 10:11 a.m. UTC | #4 
On 03/03/2015 11:09 AM, Uwe Kleine-König wrote:
> Hello Hans,
> 
> On Tue, Mar 03, 2015 at 10:59:22AM +0100, Hans Verkuil wrote:
>> Never mind those comments, after checking what devm_gpiod_get_index_optional
>> does it's clear that this patch is correct.
>>
>> Sorry about the noise.
> No problem. Is this an Ack then? Who picks up this patch?

I do, I've just accepted it and will post a pull request for v4.1 in a minute.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 5a7c9389a605..ddeeb6695a4b 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -537,12 +537,8 @@  static void adv7604_set_hpd(struct adv7604_state *state, unsigned int hpd)
 {
 	unsigned int i;
 
-	for (i = 0; i < state->info->num_dv_ports; ++i) {
-		if (IS_ERR(state->hpd_gpio[i]))
-			continue;
-
+	for (i = 0; i < state->info->num_dv_ports; ++i)
 		gpiod_set_value_cansleep(state->hpd_gpio[i], hpd & BIT(i));
-	}
 
 	v4l2_subdev_notify(&state->sd, ADV7604_HOTPLUG, &hpd);
 }
@@ -2720,13 +2716,13 @@  static int adv7604_probe(struct i2c_client *client,
 	/* Request GPIOs. */
 	for (i = 0; i < state->info->num_dv_ports; ++i) {
 		state->hpd_gpio[i] =
-			devm_gpiod_get_index(&client->dev, "hpd", i);
+			devm_gpiod_get_index_optional(&client->dev, "hpd", i,
+						      GPIOD_OUT_LOW);
 		if (IS_ERR(state->hpd_gpio[i]))
-			continue;
-
-		gpiod_direction_output(state->hpd_gpio[i], 0);
+			return PTR_ERR(state->hpd_gpio[i]);
 
-		v4l_info(client, "Handling HPD %u GPIO\n", i);
+		if (state->hpd_gpio[i])
+			v4l_info(client, "Handling HPD %u GPIO\n", i);
 	}
 
 	state->timings = cea640x480;