Re: [PATCH RESEND 1/2] ASoC: max98927: Handle reset gpio when probing i2c

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sunday, August 29, 2021, Alejandro Tafalla <atafalla@xxxxxxxxx> wrote:

> Drive the reset gpio if defined in the DTS node.
>
> Signed-off-by: Alejandro Tafalla <atafalla@xxxxxxxxx>
> ---
>  sound/soc/codecs/max98927.c | 16 ++++++++++++++++
>  sound/soc/codecs/max98927.h |  1 +
>  2 files changed, 17 insertions(+)
>
> diff --git a/sound/soc/codecs/max98927.c b/sound/soc/codecs/max98927.c
> index 8b206ee77709..dacf64c4cdf7 100644
> --- a/sound/soc/codecs/max98927.c
> +++ b/sound/soc/codecs/max98927.c
> @@ -898,6 +898,22 @@ static int max98927_i2c_probe(struct i2c_client *i2c,
>                 return ret;
>         }
>
> +       max98927->reset_gpio
> +               = devm_gpiod_get_optional(&i2c->dev, "reset",
> GPIOD_OUT_HIGH);
> +       if (IS_ERR(max98927->reset_gpio)) {
> +               ret = PTR_ERR(max98927->reset_gpio);
> +               dev_err(&i2c->dev,
> +                       "Failed to request GPIO reset pin, error %d\n",
> ret);
> +               return ret;



Spamming logs is not good. Use

return dev_err_probe(...);




> +       }
> +
> +       if (max98927->reset_gpio) {
> +               gpiod_set_value_cansleep(max98927->reset_gpio, 0);



You may request the pin in a proper state, also with current code you seems
mishandle the conception of the logical pin level vs. physical one.


> +               usleep_range(5, 10)
> +               gpiod_set_value_cansleep(max98927->reset_gpio, 1);
> +               usleep_range(1, 5)
> +       }
> +
>         /* Check Revision ID */
>         ret = regmap_read(max98927->regmap,
>                 MAX98927_R01FF_REV_ID, &reg);
> diff --git a/sound/soc/codecs/max98927.h b/sound/soc/codecs/max98927.h
> index 05f495db914d..5c04bf38e24a 100644
> --- a/sound/soc/codecs/max98927.h
> +++ b/sound/soc/codecs/max98927.h
> @@ -255,6 +255,7 @@ struct max98927_priv {
>         struct regmap *regmap;
>         struct snd_soc_component *component;
>         struct max98927_pdata *pdata;



> +       struct gpio_desc *reset_gpio;


Why? Are you using it outside of ->probe()?


>         unsigned int spk_gain;
>         unsigned int sysclk;
>         unsigned int v_l_slot;
> --
> 2.32.0
>
>

-- 
With Best Regards,
Andy Shevchenko