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@xxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
