[1/2] ASoC: rt5645: fix error handling for gpio detection

Commit Message

Takashi Sakamoto Feb. 2, 2017, 2:40 a.m. UTC

Hi,

On Feb 2 2017 03:27, Pierre-Louis Bossart wrote:
> Optional gpio handling should not cause an error status and prevent
> probing if it's missing.  Remove error return for -ENOENT case and
> move error message to dev_info
>
> Signed-off-by: Pierre-Louis Bossart <[e-mail]>
> ---
>  sound/soc/codecs/rt5645.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
> index 65ac841..e149f3c 100644
> --- a/sound/soc/codecs/rt5645.c
> +++ b/sound/soc/codecs/rt5645.c
> @@ -3660,8 +3660,14 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
>  						       GPIOD_IN);
>
>  	if (IS_ERR(rt5645->gpiod_hp_det)) {
> -		dev_err(&i2c->dev, "failed to initialize gpiod\n");
> -		return PTR_ERR(rt5645->gpiod_hp_det);
> +		dev_info(&i2c->dev, "failed to initialize gpiod\n");
> +		ret = PTR_ERR(rt5645->gpiod_hp_det);
> +		/*
> +		 * Continue if optional gpiod is missing, bail for all other
> +		 * errors, including -EPROBE_DEFER
> +		 */
> +		if (ret != -ENOENT)
> +			return ret;
>  	}
>
>  	for (i = 0; i < ARRAY_SIZE(rt5645->supplies); i++)

(sound/soc/codecs/rt5645.c)
rt5645_i2c_probe()
  (drivers/gpio/devres.c)
  ->devm_gpiod_get_optional()
    ->devm_gpiod_get_index_optional()

As long as seeing current implementation of 
'devm_gpiod_get_index_optional()', this function never returns ENOENT. 
In this case, it returns NULL.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/devres.c#n185

IS_ERR(NULL) is false, thus the additional condition is useless. When 
entering the branch, error code should be always returned.

For your purpose, checking NULL at first, then handles the error 
properly, like:

$ git diff
         for (i = 0; i < ARRAY_SIZE(rt5645->supplies); i++)


Regards

Takashi Sakamoto

Comments

Pierre-Louis Bossart Feb. 2, 2017, 5:41 a.m. UTC | #1

>>      if (IS_ERR(rt5645->gpiod_hp_det)) {
>> -        dev_err(&i2c->dev, "failed to initialize gpiod\n");
>> -        return PTR_ERR(rt5645->gpiod_hp_det);
>> +        dev_info(&i2c->dev, "failed to initialize gpiod\n");
>> +        ret = PTR_ERR(rt5645->gpiod_hp_det);
>> +        /*
>> +         * Continue if optional gpiod is missing, bail for all other
>> +         * errors, including -EPROBE_DEFER
>> +         */
>> +        if (ret != -ENOENT)
>> +            return ret;
>>      }
>>
>>      for (i = 0; i < ARRAY_SIZE(rt5645->supplies); i++)
>
> (sound/soc/codecs/rt5645.c)
> rt5645_i2c_probe()
>  (drivers/gpio/devres.c)
>  ->devm_gpiod_get_optional()
>    ->devm_gpiod_get_index_optional()
>
> As long as seeing current implementation of 
> 'devm_gpiod_get_index_optional()', this function never returns ENOENT. 
> In this case, it returns NULL.
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/devres.c#n185 
>
Thanks for pointing this out, I didn't see it and naively thought that 
everyone followed the same conventions. Oh well. I'll respin a v2.

Patch

diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index 1ac96ef..a588454 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -3656,10 +3656,15 @@  static int rt5645_i2c_probe(struct i2c_client *i2c,

         rt5645->gpiod_hp_det = devm_gpiod_get_optional(&i2c->dev, 
"hp-detect",
                                                        GPIOD_IN);
-
-       if (IS_ERR(rt5645->gpiod_hp_det)) {
-               dev_err(&i2c->dev, "failed to initialize gpiod\n");
-               return PTR_ERR(rt5645->gpiod_hp_det);
+       if (rt5645->gpiod_hp_det && IS_ERR(rt5645->gpiod_hp_det)) {
+               dev_info(&i2c->dev, "failed to initialize gpiod\n");
+               ret = PTR_ERR(rt5645->gpiod_hp_det);
+               /*
+                * Continue if optional gpiod is missing, bail for all other
+                * errors, including -EPROBE_DEFER. -ENOENT is never returns
+                * according to implementation of devm_gpiod_get_optional().
+                */
+               return ret;
         }