From: Alejandro Tafalla <atafalla@dnyon.com> To: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>, Rob Herring <robh+dt@kernel.org>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH RESEND 1/2] ASoC: max98927: Handle reset gpio when probing i2c Date: Mon, 30 Aug 2021 00:13:41 +0200 [thread overview] Message-ID: <YSwGlXEFRuUvNGZ7@dnyon.com> (raw) In-Reply-To: <CAHp75VdY9uoTWeJEroi3iChnToUSxe+Henmg7Yux+UWkK59jQg@mail.gmail.com> [-- Attachment #1: mensaje --] [-- Type: text/plain, Size: 1553 bytes --] Hello Andy, On Sun, Aug 29, 2021 at 11:22:35PM +0300, Andy Shevchenko wrote: > > + 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(...); Okay. > > + } > > + > > + 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. Right, i made the mistake of basing off an old driver that use legacy functions. > > 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()? No, I'll delete it and use a local variable. > With Best Regards, > Andy Shevchenko Thank you for the feedback, I'll address all the issues in a V2. Alejandro Tafalla
WARNING: multiple messages have this Message-ID (diff)
From: Alejandro Tafalla <atafalla@dnyon.com> To: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Takashi Iwai <tiwai@suse.com>, Rob Herring <robh+dt@kernel.org>, Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org> Subject: Re: [PATCH RESEND 1/2] ASoC: max98927: Handle reset gpio when probing i2c Date: Mon, 30 Aug 2021 00:13:41 +0200 [thread overview] Message-ID: <YSwGlXEFRuUvNGZ7@dnyon.com> (raw) In-Reply-To: <CAHp75VdY9uoTWeJEroi3iChnToUSxe+Henmg7Yux+UWkK59jQg@mail.gmail.com> [-- Attachment #1: mensaje --] [-- Type: text/plain, Size: 1553 bytes --] Hello Andy, On Sun, Aug 29, 2021 at 11:22:35PM +0300, Andy Shevchenko wrote: > > + 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(...); Okay. > > + } > > + > > + 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. Right, i made the mistake of basing off an old driver that use legacy functions. > > 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()? No, I'll delete it and use a local variable. > With Best Regards, > Andy Shevchenko Thank you for the feedback, I'll address all the issues in a V2. Alejandro Tafalla
next prev parent reply other threads:[~2021-08-29 22:13 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-29 17:00 [PATCH RESEND 0/2] Add reset-gpios handling for max98927 Alejandro Tafalla 2021-08-29 17:00 ` Alejandro Tafalla 2021-08-29 17:00 ` [PATCH RESEND 1/2] ASoC: max98927: Handle reset gpio when probing i2c Alejandro Tafalla 2021-08-29 17:00 ` Alejandro Tafalla 2021-08-29 20:22 ` Andy Shevchenko 2021-08-29 22:13 ` Alejandro Tafalla [this message] 2021-08-29 22:13 ` Alejandro Tafalla 2021-09-02 14:43 ` Mark Brown 2021-09-02 14:43 ` Mark Brown 2021-08-29 21:24 ` kernel test robot 2021-08-29 21:24 ` kernel test robot 2021-08-29 21:24 ` kernel test robot 2021-08-29 17:00 ` [PATCH RESEND 2/2] dt-bindings: sound: max98927: Add reset-gpios optional property Alejandro Tafalla 2021-08-29 17:00 ` Alejandro Tafalla
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YSwGlXEFRuUvNGZ7@dnyon.com \ --to=atafalla@dnyon.com \ --cc=alsa-devel@alsa-project.org \ --cc=andy.shevchenko@gmail.com \ --cc=broonie@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=perex@perex.cz \ --cc=robh+dt@kernel.org \ --cc=tiwai@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.