[PATCH RESEND 0/2] Add reset-gpios handling for max98927

[PATCH RESEND 0/2] Add reset-gpios handling for max98927

[Alejandro Tafalla]

The max98927 codec on some devices (i.e. Xiaomi Mi A2 Lite phone) require
hardware-resetting the codec by driving a reset-gpio. This series add
support for it through an optional reset-gpios property.

Alejandro Tafalla (2):
  ASoC: max98927: Handle reset gpio when probing i2c
  dt-bindings: sound: max98927: Add reset-gpios optional property

 .../devicetree/bindings/sound/max9892x.txt       |  3 +++
 sound/soc/codecs/max98927.c                      | 16 ++++++++++++++++
 sound/soc/codecs/max98927.h                      |  1 +
 3 files changed, 20 insertions(+)

-- 
2.32.0

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

[Alejandro Tafalla]

Drive the reset gpio if defined in the DTS node.

Signed-off-by: Alejandro Tafalla <atafalla@dnyon.com>
---
 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;
+	}
+
+	if (max98927->reset_gpio) {
+		gpiod_set_value_cansleep(max98927->reset_gpio, 0);
+		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;
 	unsigned int spk_gain;
 	unsigned int sysclk;
 	unsigned int v_l_slot;
-- 
2.32.0

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

[Andy Shevchenko]

On Sunday, August 29, 2021, Alejandro Tafalla <atafalla@dnyon.com> wrote:

> Drive the reset gpio if defined in the DTS node.
>
> Signed-off-by: Alejandro Tafalla <atafalla@dnyon.com>
> ---
>  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

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

[Alejandro Tafalla]

[-- 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

On Mon, Aug 30, 2021 at 12:13:41AM +0200, Alejandro Tafalla wrote:
> On Sun, Aug 29, 2021 at 11:22:35PM +0300, Andy Shevchenko wrote:

> > > +       struct gpio_desc *reset_gpio;

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

> No, I'll delete it and use a local variable.

It can be good to reassert reset when unloading the driver in order to
ensure that the device isn't active.  It doesn't really matter though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4437 bytes --]

Hi Alejandro,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on asoc/for-next]
[also build test ERROR on v5.14-rc7 next-20210827]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Alejandro-Tafalla/Add-reset-gpios-handling-for-max98927/20210830-010941
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: hexagon-randconfig-r013-20210829 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 510e106fa8635e7f9c51c896180b971de6309b2f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4cc91cc4ff05ef061e2247a49c8b7cf9084fe6fb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Alejandro-Tafalla/Add-reset-gpios-handling-for-max98927/20210830-010941
        git checkout 4cc91cc4ff05ef061e2247a49c8b7cf9084fe6fb
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=hexagon SHELL=/bin/bash sound/soc/codecs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> sound/soc/codecs/max98927.c:912:22: error: expected ';' after expression
                   usleep_range(5, 10)
                                      ^
                                      ;
   sound/soc/codecs/max98927.c:914:21: error: expected ';' after expression
                   usleep_range(1, 5)
                                     ^
                                     ;
   2 errors generated.


vim +912 sound/soc/codecs/max98927.c

   867	
   868		int ret = 0, value;
   869		int reg = 0;
   870		struct max98927_priv *max98927 = NULL;
   871	
   872		max98927 = devm_kzalloc(&i2c->dev,
   873			sizeof(*max98927), GFP_KERNEL);
   874	
   875		if (!max98927) {
   876			ret = -ENOMEM;
   877			return ret;
   878		}
   879		i2c_set_clientdata(i2c, max98927);
   880	
   881		/* update interleave mode info */
   882		if (!of_property_read_u32(i2c->dev.of_node,
   883			"interleave_mode", &value)) {
   884			if (value > 0)
   885				max98927->interleave_mode = true;
   886			else
   887				max98927->interleave_mode = false;
   888		} else
   889			max98927->interleave_mode = false;
   890	
   891		/* regmap initialization */
   892		max98927->regmap
   893			= devm_regmap_init_i2c(i2c, &max98927_regmap);
   894		if (IS_ERR(max98927->regmap)) {
   895			ret = PTR_ERR(max98927->regmap);
   896			dev_err(&i2c->dev,
   897				"Failed to allocate regmap: %d\n", ret);
   898			return ret;
   899		}
   900	
   901		max98927->reset_gpio
   902			= devm_gpiod_get_optional(&i2c->dev, "reset", GPIOD_OUT_HIGH);
   903		if (IS_ERR(max98927->reset_gpio)) {
   904			ret = PTR_ERR(max98927->reset_gpio);
   905			dev_err(&i2c->dev,
   906				"Failed to request GPIO reset pin, error %d\n", ret);
   907			return ret;
   908		}
   909	
   910		if (max98927->reset_gpio) {
   911			gpiod_set_value_cansleep(max98927->reset_gpio, 0);
 > 912			usleep_range(5, 10)
   913			gpiod_set_value_cansleep(max98927->reset_gpio, 1);
   914			usleep_range(1, 5)
   915		}
   916	
   917		/* Check Revision ID */
   918		ret = regmap_read(max98927->regmap,
   919			MAX98927_R01FF_REV_ID, &reg);
   920		if (ret < 0) {
   921			dev_err(&i2c->dev,
   922				"Failed to read: 0x%02X\n", MAX98927_R01FF_REV_ID);
   923			return ret;
   924		}
   925		dev_info(&i2c->dev, "MAX98927 revisionID: 0x%02X\n", reg);
   926	
   927		/* voltage/current slot configuration */
   928		max98927_slot_config(i2c, max98927);
   929	
   930		/* codec registeration */
   931		ret = devm_snd_soc_register_component(&i2c->dev,
   932			&soc_component_dev_max98927,
   933			max98927_dai, ARRAY_SIZE(max98927_dai));
   934		if (ret < 0)
   935			dev_err(&i2c->dev, "Failed to register component: %d\n", ret);
   936	
   937		return ret;
   938	}
   939	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30377 bytes --]

[PATCH RESEND 2/2] dt-bindings: sound: max98927: Add reset-gpios optional property

[Alejandro Tafalla]

Add the reset-gpios as an optional property because some devices might 
not need it to work properly.

Signed-off-by: Alejandro Tafalla <atafalla@dnyon.com>
---
 Documentation/devicetree/bindings/sound/max9892x.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/max9892x.txt b/Documentation/devicetree/bindings/sound/max9892x.txt
index f6171591ddc6..73733fb60136 100644
--- a/Documentation/devicetree/bindings/sound/max9892x.txt
+++ b/Documentation/devicetree/bindings/sound/max9892x.txt
@@ -30,6 +30,9 @@ Required properties:
 
   - reg : the I2C address of the device for I2C
 
+Optional Properties:
+  - reset-gpios : reference to the GPIO connected to the reset pin, if any.
+
 Example:
 
 codec: max98927@3a {
-- 
2.32.0