[PATCH 1/2] ASoC: max9892x: Add documentation for reset-gpio support

[PATCH 1/2] ASoC: max9892x: Add documentation for reset-gpio support

[Cheng-Yi Chiang]

max98927 codec driver will support reset-gpio binding so it can toggle
reset line in its probe function.

Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
---
 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 f6171591ddc61..8bf30483eedcd 100644
--- a/Documentation/devicetree/bindings/sound/max9892x.txt
+++ b/Documentation/devicetree/bindings/sound/max9892x.txt
@@ -29,6 +29,8 @@ Required properties:
                    Range : 0 (off), 1 (on),  Default : 0
 
   - reg : the I2C address of the device for I2C
+  - reset-gpio: reset gpio the chip is connected to. This is supported
+                on max98927.
 
 Example:
 
@@ -38,4 +40,5 @@ codec: max98927@3a {
    imon-slot-no = <1>;
    interleave-mode = <0>;
    reg = <0x3a>;
+   reset-gpio = <&tlmm 75 GPIO_ACTIVE_LOW>;
 };
-- 
2.16.4

[PATCH 2/2] ASoC: max98927: Add reset-gpio support

[Cheng-Yi Chiang]

Toggle reset line in max98927_i2c_probe.
Use a list to store max98927 instances so we do not toggle reset line
again if more than one instances share the same reset line.

Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
---
 sound/soc/codecs/max98927.c | 78 +++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/max98927.h |  2 ++
 2 files changed, 80 insertions(+)

diff --git a/sound/soc/codecs/max98927.c b/sound/soc/codecs/max98927.c
index 941712058a8f5..789b27cdfd9e0 100644
--- a/sound/soc/codecs/max98927.c
+++ b/sound/soc/codecs/max98927.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
@@ -24,6 +25,11 @@
 #include <sound/tlv.h>
 #include "max98927.h"
 
+#define MAX98927_MIN_RESET_US 1
+#define MAX98927_RELEASE_RESET_DELAY_US 500
+
+static LIST_HEAD(reset_list);
+
 static struct reg_default max98927_reg[] = {
 	{MAX98927_R0001_INT_RAW1,  0x00},
 	{MAX98927_R0002_INT_RAW2,  0x00},
@@ -877,6 +883,54 @@ static void max98927_slot_config(struct i2c_client *i2c,
 		max98927->i_l_slot = 1;
 }
 
+static int max98927_i2c_toggle_reset(struct device *dev,
+				     struct max98927_priv *max98927)
+{
+	/*
+	 * If we do not have reset gpio, assume platform firmware
+	 * controls the regulator and toggles it for us.
+	 */
+	if (!max98927->reset_gpio)
+		return 0;
+
+	gpiod_set_value_cansleep(max98927->reset_gpio, 1);
+
+	/*
+	 * We need to wait a bit before we are allowed to release reset GPIO.
+	 */
+	usleep_range(MAX98927_MIN_RESET_US, MAX98927_MIN_RESET_US + 5);
+
+	gpiod_set_value_cansleep(max98927->reset_gpio, 0);
+
+	/*
+	 * We need to wait a bit before I2C communication is available.
+	 */
+	usleep_range(MAX98927_RELEASE_RESET_DELAY_US,
+		     MAX98927_RELEASE_RESET_DELAY_US + 5);
+
+	/*
+	 * Release reset GPIO because we are not going to use it.
+	 */
+	devm_gpiod_put(dev, max98927->reset_gpio);
+
+	return 0;
+}
+
+static bool max98927_is_first_to_reset(struct max98927_priv *max98927)
+{
+	struct max98927_priv *p;
+
+	if (!max98927->reset_gpio)
+		return false;
+
+	list_for_each_entry(p, &reset_list, list) {
+		if (max98927->reset_gpio == p->reset_gpio)
+			return false;
+	}
+
+	return true;
+}
+
 static int max98927_i2c_probe(struct i2c_client *i2c,
 	const struct i2c_device_id *id)
 {
@@ -904,6 +958,28 @@ static int max98927_i2c_probe(struct i2c_client *i2c,
 	} else
 		max98927->interleave_mode = 0;
 
+	/* Gets optional GPIO for reset line. */
+	max98927->reset_gpio = devm_gpiod_get_optional(
+			&i2c->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(max98927->reset_gpio)) {
+		ret = PTR_ERR(max98927->reset_gpio);
+		dev_err(&i2c->dev, "error getting reset gpio: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Only toggle reset line for the first instance when the
+	 * reset line is shared among instances. For example,
+	 * left and right amplifier share the same reset line, and
+	 * we should only toggle the reset line once.
+	 */
+	if (max98927_is_first_to_reset(max98927)) {
+		dev_info(&i2c->dev, "%s: toggle reset line\n", __func__);
+		ret = max98927_i2c_toggle_reset(&i2c->dev, max98927);
+		if (ret)
+			return ret;
+	}
+
 	/* regmap initialization */
 	max98927->regmap
 		= devm_regmap_init_i2c(i2c, &max98927_regmap);
@@ -934,6 +1010,8 @@ static int max98927_i2c_probe(struct i2c_client *i2c,
 	if (ret < 0)
 		dev_err(&i2c->dev, "Failed to register component: %d\n", ret);
 
+	list_add(&max98927->list, &reset_list);
+
 	return ret;
 }
 
diff --git a/sound/soc/codecs/max98927.h b/sound/soc/codecs/max98927.h
index 538992a238b28..d48f61f6c3ba5 100644
--- a/sound/soc/codecs/max98927.h
+++ b/sound/soc/codecs/max98927.h
@@ -275,5 +275,7 @@ struct max98927_priv {
 	unsigned int master;
 	unsigned int digital_gain;
 	bool tdm_mode;
+	struct gpio_desc *reset_gpio;
+	struct list_head list;
 };
 #endif
-- 
2.16.4

Re: [alsa-devel] [PATCH 2/2] ASoC: max98927: Add reset-gpio support

[Rohit Kumar]

On 9/12/2018 5:49 PM, Cheng-Yi Chiang wrote:
> Toggle reset line in max98927_i2c_probe.
> Use a list to store max98927 instances so we do not toggle reset line
> again if more than one instances share the same reset line.
>
> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
Reviewed-and-tested-by: Rohit kumar <rohitkr@codeaurora.org>

> ---
>   sound/soc/codecs/max98927.c | 78 +++++++++++++++++++++++++++++++++++++++++++++
>   sound/soc/codecs/max98927.h |  2 ++
>   2 files changed, 80 insertions(+)
>
> diff --git a/sound/soc/codecs/max98927.c b/sound/soc/codecs/max98927.c
> index 941712058a8f5..789b27cdfd9e0 100644
> --- a/sound/soc/codecs/max98927.c
> +++ b/sound/soc/codecs/max98927.c
> @@ -11,6 +11,7 @@
>    */
>   
>   #include <linux/acpi.h>
> +#include <linux/delay.h>
>   #include <linux/i2c.h>
>   #include <linux/module.h>
>   #include <linux/regmap.h>
> @@ -24,6 +25,11 @@
>   #include <sound/tlv.h>
>   #include "max98927.h"
>   
> +#define MAX98927_MIN_RESET_US 1
> +#define MAX98927_RELEASE_RESET_DELAY_US 500
> +
> +static LIST_HEAD(reset_list);
> +
>   static struct reg_default max98927_reg[] = {
>   	{MAX98927_R0001_INT_RAW1,  0x00},
>   	{MAX98927_R0002_INT_RAW2,  0x00},
> @@ -877,6 +883,54 @@ static void max98927_slot_config(struct i2c_client *i2c,
>   		max98927->i_l_slot = 1;
>   }
>   
> +static int max98927_i2c_toggle_reset(struct device *dev,
> +				     struct max98927_priv *max98927)
> +{
> +	/*
> +	 * If we do not have reset gpio, assume platform firmware
> +	 * controls the regulator and toggles it for us.
> +	 */
> +	if (!max98927->reset_gpio)
> +		return 0;
> +
> +	gpiod_set_value_cansleep(max98927->reset_gpio, 1);
> +
> +	/*
> +	 * We need to wait a bit before we are allowed to release reset GPIO.
> +	 */
> +	usleep_range(MAX98927_MIN_RESET_US, MAX98927_MIN_RESET_US + 5);
> +
> +	gpiod_set_value_cansleep(max98927->reset_gpio, 0);
> +
> +	/*
> +	 * We need to wait a bit before I2C communication is available.
> +	 */
> +	usleep_range(MAX98927_RELEASE_RESET_DELAY_US,
> +		     MAX98927_RELEASE_RESET_DELAY_US + 5);
> +
> +	/*
> +	 * Release reset GPIO because we are not going to use it.
> +	 */
> +	devm_gpiod_put(dev, max98927->reset_gpio);
> +
> +	return 0;
> +}
> +
> +static bool max98927_is_first_to_reset(struct max98927_priv *max98927)
> +{
> +	struct max98927_priv *p;
> +
> +	if (!max98927->reset_gpio)
> +		return false;
> +
> +	list_for_each_entry(p, &reset_list, list) {
> +		if (max98927->reset_gpio == p->reset_gpio)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>   static int max98927_i2c_probe(struct i2c_client *i2c,
>   	const struct i2c_device_id *id)
>   {
> @@ -904,6 +958,28 @@ static int max98927_i2c_probe(struct i2c_client *i2c,
>   	} else
>   		max98927->interleave_mode = 0;
>   
> +	/* Gets optional GPIO for reset line. */
> +	max98927->reset_gpio = devm_gpiod_get_optional(
> +			&i2c->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(max98927->reset_gpio)) {
> +		ret = PTR_ERR(max98927->reset_gpio);
> +		dev_err(&i2c->dev, "error getting reset gpio: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Only toggle reset line for the first instance when the
> +	 * reset line is shared among instances. For example,
> +	 * left and right amplifier share the same reset line, and
> +	 * we should only toggle the reset line once.
> +	 */
> +	if (max98927_is_first_to_reset(max98927)) {
> +		dev_info(&i2c->dev, "%s: toggle reset line\n", __func__);
> +		ret = max98927_i2c_toggle_reset(&i2c->dev, max98927);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	/* regmap initialization */
>   	max98927->regmap
>   		= devm_regmap_init_i2c(i2c, &max98927_regmap);
> @@ -934,6 +1010,8 @@ static int max98927_i2c_probe(struct i2c_client *i2c,
>   	if (ret < 0)
>   		dev_err(&i2c->dev, "Failed to register component: %d\n", ret);
>   
> +	list_add(&max98927->list, &reset_list);
> +
>   	return ret;
>   }
>   
> diff --git a/sound/soc/codecs/max98927.h b/sound/soc/codecs/max98927.h
> index 538992a238b28..d48f61f6c3ba5 100644
> --- a/sound/soc/codecs/max98927.h
> +++ b/sound/soc/codecs/max98927.h
> @@ -275,5 +275,7 @@ struct max98927_priv {
>   	unsigned int master;
>   	unsigned int digital_gain;
>   	bool tdm_mode;
> +	struct gpio_desc *reset_gpio;
> +	struct list_head list;
>   };
>   #endif

Re: [alsa-devel] [PATCH 2/2] ASoC: max98927: Add reset-gpio support

[Cheng-yi Chiang]

Hi Mark,
Do you have any concern about this patch ?

Thanks!
On Thu, Sep 13, 2018 at 12:47 AM Rohit Kumar <rohitkr@codeaurora.org> wrote:
>
>
>
> On 9/12/2018 5:49 PM, Cheng-Yi Chiang wrote:
> > Toggle reset line in max98927_i2c_probe.
> > Use a list to store max98927 instances so we do not toggle reset line
> > again if more than one instances share the same reset line.
> >
> > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> Reviewed-and-tested-by: Rohit kumar <rohitkr@codeaurora.org>
>
> > ---
> >   sound/soc/codecs/max98927.c | 78 +++++++++++++++++++++++++++++++++++++++++++++
> >   sound/soc/codecs/max98927.h |  2 ++
> >   2 files changed, 80 insertions(+)
> >
> > diff --git a/sound/soc/codecs/max98927.c b/sound/soc/codecs/max98927.c
> > index 941712058a8f5..789b27cdfd9e0 100644
> > --- a/sound/soc/codecs/max98927.c
> > +++ b/sound/soc/codecs/max98927.c
> > @@ -11,6 +11,7 @@
> >    */
> >
> >   #include <linux/acpi.h>
> > +#include <linux/delay.h>
> >   #include <linux/i2c.h>
> >   #include <linux/module.h>
> >   #include <linux/regmap.h>
> > @@ -24,6 +25,11 @@
> >   #include <sound/tlv.h>
> >   #include "max98927.h"
> >
> > +#define MAX98927_MIN_RESET_US 1
> > +#define MAX98927_RELEASE_RESET_DELAY_US 500
> > +
> > +static LIST_HEAD(reset_list);
> > +
> >   static struct reg_default max98927_reg[] = {
> >       {MAX98927_R0001_INT_RAW1,  0x00},
> >       {MAX98927_R0002_INT_RAW2,  0x00},
> > @@ -877,6 +883,54 @@ static void max98927_slot_config(struct i2c_client *i2c,
> >               max98927->i_l_slot = 1;
> >   }
> >
> > +static int max98927_i2c_toggle_reset(struct device *dev,
> > +                                  struct max98927_priv *max98927)
> > +{
> > +     /*
> > +      * If we do not have reset gpio, assume platform firmware
> > +      * controls the regulator and toggles it for us.
> > +      */
> > +     if (!max98927->reset_gpio)
> > +             return 0;
> > +
> > +     gpiod_set_value_cansleep(max98927->reset_gpio, 1);
> > +
> > +     /*
> > +      * We need to wait a bit before we are allowed to release reset GPIO.
> > +      */
> > +     usleep_range(MAX98927_MIN_RESET_US, MAX98927_MIN_RESET_US + 5);
> > +
> > +     gpiod_set_value_cansleep(max98927->reset_gpio, 0);
> > +
> > +     /*
> > +      * We need to wait a bit before I2C communication is available.
> > +      */
> > +     usleep_range(MAX98927_RELEASE_RESET_DELAY_US,
> > +                  MAX98927_RELEASE_RESET_DELAY_US + 5);
> > +
> > +     /*
> > +      * Release reset GPIO because we are not going to use it.
> > +      */
> > +     devm_gpiod_put(dev, max98927->reset_gpio);
> > +
> > +     return 0;
> > +}
> > +
> > +static bool max98927_is_first_to_reset(struct max98927_priv *max98927)
> > +{
> > +     struct max98927_priv *p;
> > +
> > +     if (!max98927->reset_gpio)
> > +             return false;
> > +
> > +     list_for_each_entry(p, &reset_list, list) {
> > +             if (max98927->reset_gpio == p->reset_gpio)
> > +                     return false;
> > +     }
> > +
> > +     return true;
> > +}
> > +
> >   static int max98927_i2c_probe(struct i2c_client *i2c,
> >       const struct i2c_device_id *id)
> >   {
> > @@ -904,6 +958,28 @@ static int max98927_i2c_probe(struct i2c_client *i2c,
> >       } else
> >               max98927->interleave_mode = 0;
> >
> > +     /* Gets optional GPIO for reset line. */
> > +     max98927->reset_gpio = devm_gpiod_get_optional(
> > +                     &i2c->dev, "reset", GPIOD_OUT_LOW);
> > +     if (IS_ERR(max98927->reset_gpio)) {
> > +             ret = PTR_ERR(max98927->reset_gpio);
> > +             dev_err(&i2c->dev, "error getting reset gpio: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     /*
> > +      * Only toggle reset line for the first instance when the
> > +      * reset line is shared among instances. For example,
> > +      * left and right amplifier share the same reset line, and
> > +      * we should only toggle the reset line once.
> > +      */
> > +     if (max98927_is_first_to_reset(max98927)) {
> > +             dev_info(&i2c->dev, "%s: toggle reset line\n", __func__);
> > +             ret = max98927_i2c_toggle_reset(&i2c->dev, max98927);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       /* regmap initialization */
> >       max98927->regmap
> >               = devm_regmap_init_i2c(i2c, &max98927_regmap);
> > @@ -934,6 +1010,8 @@ static int max98927_i2c_probe(struct i2c_client *i2c,
> >       if (ret < 0)
> >               dev_err(&i2c->dev, "Failed to register component: %d\n", ret);
> >
> > +     list_add(&max98927->list, &reset_list);
> > +
> >       return ret;
> >   }
> >
> > diff --git a/sound/soc/codecs/max98927.h b/sound/soc/codecs/max98927.h
> > index 538992a238b28..d48f61f6c3ba5 100644
> > --- a/sound/soc/codecs/max98927.h
> > +++ b/sound/soc/codecs/max98927.h
> > @@ -275,5 +275,7 @@ struct max98927_priv {
> >       unsigned int master;
> >       unsigned int digital_gain;
> >       bool tdm_mode;
> > +     struct gpio_desc *reset_gpio;
> > +     struct list_head list;
> >   };
> >   #endif
>

Re: [alsa-devel] [PATCH 2/2] ASoC: max98927: Add reset-gpio support

[Mark Brown]

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

On Mon, Sep 17, 2018 at 05:00:28PM +0800, Cheng-yi Chiang wrote:
> Hi Mark,
> Do you have any concern about this patch ?

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.
Sending content free pings just adds to the mail volume (if they are
seen at all) and if something has gone wrong you'll have to resend the
patches anyway.

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

Re: [alsa-devel] [PATCH 2/2] ASoC: max98927: Add reset-gpio support

[kbuild test robot]

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

Hi Cheng-Yi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on asoc/for-next]
[also build test ERROR on v4.19-rc4 next-20180919]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Cheng-Yi-Chiang/ASoC-max9892x-Add-documentation-for-reset-gpio-support/20180914-010006
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: x86_64-randconfig-s3-09201857 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   sound/soc/codecs/max98927.c: In function 'max98927_i2c_toggle_reset':
>> sound/soc/codecs/max98927.c:884:2: error: implicit declaration of function 'gpiod_set_value_cansleep'; did you mean 'gpio_set_value_cansleep'? [-Werror=implicit-function-declaration]
     gpiod_set_value_cansleep(max98927->reset_gpio, 1);
     ^~~~~~~~~~~~~~~~~~~~~~~~
     gpio_set_value_cansleep
>> sound/soc/codecs/max98927.c:902:2: error: implicit declaration of function 'devm_gpiod_put'; did you mean 'devm_gpio_free'? [-Werror=implicit-function-declaration]
     devm_gpiod_put(dev, max98927->reset_gpio);
     ^~~~~~~~~~~~~~
     devm_gpio_free
   sound/soc/codecs/max98927.c: In function 'max98927_i2c_probe':
>> sound/soc/codecs/max98927.c:950:25: error: implicit declaration of function 'devm_gpiod_get_optional'; did you mean 'devm_gpio_request_one'? [-Werror=implicit-function-declaration]
     max98927->reset_gpio = devm_gpiod_get_optional(
                            ^~~~~~~~~~~~~~~~~~~~~~~
                            devm_gpio_request_one
>> sound/soc/codecs/max98927.c:951:24: error: 'GPIOD_OUT_LOW' undeclared (first use in this function); did you mean 'GPIOF_INIT_LOW'?
       &i2c->dev, "reset", GPIOD_OUT_LOW);
                           ^~~~~~~~~~~~~
                           GPIOF_INIT_LOW
   sound/soc/codecs/max98927.c:951:24: note: each undeclared identifier is reported only once for each function it appears in
   cc1: some warnings being treated as errors

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

   873	
   874	static int max98927_i2c_toggle_reset(struct device *dev,
   875					     struct max98927_priv *max98927)
   876	{
   877		/*
   878		 * If we do not have reset gpio, assume platform firmware
   879		 * controls the regulator and toggles it for us.
   880		 */
   881		if (!max98927->reset_gpio)
   882			return 0;
   883	
 > 884		gpiod_set_value_cansleep(max98927->reset_gpio, 1);
   885	
   886		/*
   887		 * We need to wait a bit before we are allowed to release reset GPIO.
   888		 */
   889		usleep_range(MAX98927_MIN_RESET_US, MAX98927_MIN_RESET_US + 5);
   890	
   891		gpiod_set_value_cansleep(max98927->reset_gpio, 0);
   892	
   893		/*
   894		 * We need to wait a bit before I2C communication is available.
   895		 */
   896		usleep_range(MAX98927_RELEASE_RESET_DELAY_US,
   897			     MAX98927_RELEASE_RESET_DELAY_US + 5);
   898	
   899		/*
   900		 * Release reset GPIO because we are not going to use it.
   901		 */
 > 902		devm_gpiod_put(dev, max98927->reset_gpio);
   903	
   904		return 0;
   905	}
   906	
   907	static bool max98927_is_first_to_reset(struct max98927_priv *max98927)
   908	{
   909		struct max98927_priv *p;
   910	
   911		if (!max98927->reset_gpio)
   912			return false;
   913	
   914		list_for_each_entry(p, &reset_list, list) {
   915			if (max98927->reset_gpio == p->reset_gpio)
   916				return false;
   917		}
   918	
   919		return true;
   920	}
   921	
   922	static int max98927_i2c_probe(struct i2c_client *i2c,
   923		const struct i2c_device_id *id)
   924	{
   925	
   926		int ret = 0, value;
   927		int reg = 0;
   928		struct max98927_priv *max98927 = NULL;
   929	
   930		max98927 = devm_kzalloc(&i2c->dev,
   931			sizeof(*max98927), GFP_KERNEL);
   932	
   933		if (!max98927) {
   934			ret = -ENOMEM;
   935			return ret;
   936		}
   937		i2c_set_clientdata(i2c, max98927);
   938	
   939		/* update interleave mode info */
   940		if (!of_property_read_u32(i2c->dev.of_node,
   941			"interleave_mode", &value)) {
   942			if (value > 0)
   943				max98927->interleave_mode = 1;
   944			else
   945				max98927->interleave_mode = 0;
   946		} else
   947			max98927->interleave_mode = 0;
   948	
   949		/* Gets optional GPIO for reset line. */
 > 950		max98927->reset_gpio = devm_gpiod_get_optional(
 > 951				&i2c->dev, "reset", GPIOD_OUT_LOW);
   952		if (IS_ERR(max98927->reset_gpio)) {
   953			ret = PTR_ERR(max98927->reset_gpio);
   954			dev_err(&i2c->dev, "error getting reset gpio: %d\n", ret);
   955			return ret;
   956		}
   957	
   958		/*
   959		 * Only toggle reset line for the first instance when the
   960		 * reset line is shared among instances. For example,
   961		 * left and right amplifier share the same reset line, and
   962		 * we should only toggle the reset line once.
   963		 */
   964		if (max98927_is_first_to_reset(max98927)) {
   965			dev_info(&i2c->dev, "%s: toggle reset line\n", __func__);
   966			ret = max98927_i2c_toggle_reset(&i2c->dev, max98927);
   967			if (ret)
   968				return ret;
   969		}
   970	
   971		/* regmap initialization */
   972		max98927->regmap
   973			= devm_regmap_init_i2c(i2c, &max98927_regmap);
   974		if (IS_ERR(max98927->regmap)) {
   975			ret = PTR_ERR(max98927->regmap);
   976			dev_err(&i2c->dev,
   977				"Failed to allocate regmap: %d\n", ret);
   978			return ret;
   979		}
   980	
   981		/* Check Revision ID */
   982		ret = regmap_read(max98927->regmap,
   983			MAX98927_R01FF_REV_ID, &reg);
   984		if (ret < 0) {
   985			dev_err(&i2c->dev,
   986				"Failed to read: 0x%02X\n", MAX98927_R01FF_REV_ID);
   987			return ret;
   988		}
   989		dev_info(&i2c->dev, "MAX98927 revisionID: 0x%02X\n", reg);
   990	
   991		/* voltage/current slot configuration */
   992		max98927_slot_config(i2c, max98927);
   993	
   994		/* codec registeration */
   995		ret = devm_snd_soc_register_component(&i2c->dev,
   996			&soc_component_dev_max98927,
   997			max98927_dai, ARRAY_SIZE(max98927_dai));
   998		if (ret < 0)
   999			dev_err(&i2c->dev, "Failed to register component: %d\n", ret);
  1000	
  1001		list_add(&max98927->list, &reset_list);
  1002	
  1003		return ret;
  1004	}
  1005	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

Re: [PATCH 2/2] ASoC: max98927: Add reset-gpio support

[Mark Brown]

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

On Wed, Sep 12, 2018 at 08:19:55PM +0800, Cheng-Yi Chiang wrote:

> +	/*
> +	 * Release reset GPIO because we are not going to use it.
> +	 */
> +	devm_gpiod_put(dev, max98927->reset_gpio);

There is no need to do this, it's still potentially useful information
for userspace and it's also going to surprise anyone who tries to modify
the code to use this function at some other time (eg, when coming out of
suspend sometimes it's useful to reset the device).

> @@ -934,6 +1010,8 @@ static int max98927_i2c_probe(struct i2c_client *i2c,
>  	if (ret < 0)
>  		dev_err(&i2c->dev, "Failed to register component: %d\n", ret);
>  
> +	list_add(&max98927->list, &reset_list);
> +

I'm not seeing any locking of this list.  This also feels like something
that shouldn't be in this driver but should be pushed up a level - it's
only going to do the right thing if the reset line is only shared with
other devices using this driver, if someone does something like put the
CODEC and an external speaker amp on the same reset pin then it won't
work.  I'm not sure where the best place to do that is; drivers/reset
isn't really for this use case but feels right perhaps?

Can you perhaps split the basic reset handling out from this list
handling as a separate patch?

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

Re: [PATCH 2/2] ASoC: max98927: Add reset-gpio support

[Cheng-yi Chiang]

+reset controller maintainer Philipp

Hi Mark,
  Sorry for the late reply. It took me a while to investigate reset
controller and its possible usage. I would like to figure out the
proper way of reset handling because it is common to have a shared
reset line for two max98927 codecs for left and right channels.
Without supporting this usage, a simple reset-gpios change for single
codec would not be useful, and perhaps will be duplicated if reset
controller is a better way.

Hi Philipp,
  I would like to seek your advice about whether we can use reset
controller to support the use case where multiple devices share the
same reset line.

Let me summarize the use case:
There are two max98927 codecs sharing the same reset line.
The reset line is controlled by a GPIO.
The goal is to toggle reset line once and only once.

There was a similar scenario in tlv320aic3x codec driver [1].
A static list is used in the codec driver so probe function can know
whether it is needed to toggle reset line.
Mark pointed out that it is not suitable to handle the shared reset
line inside codec driver.
A point is that this only works for multiple devices using the same
device driver.
He suggested to look into reset controller so I searched through the
usage for common reset line.

Here I found a shared reset line use case [2].
With the patch [2], reset_control_reset can support this “reset once
and for all” use case.

However, I found that there are some missing pieces:

Let’s assume there are two codecs c1 and c2 sharing a reset line
controlled by GPIO.

c1’s spec:
Hold time: The minimum time to assert the reset line is t_a1.
Release time: The minimum time to access the chip after deassert of
the reset line is t_d1.

c2’s spec:
Hold time: The minimum time to assert the reset line is t_a2.
Release time: The minimum time to access the chip after deassert of
the reset line is t_d2.

For both c1 and c2 to work properly, we need a reset controller that
can assert the reset line for
T = max(t_a1, t_a2).

1. We need reset controller to support GPIO.
2. We need to specify hold time T to reset controller in device tree
so it knows that it needs hold reset line for that long in its
implementation of reset_control_reset.
3. Assuming we have 1 and 2 in place. In codec driver of c1, it can
call reset_control_reset and wait for t_a1 + t_d1. In codec driver of
c2, it can call reset_control_reset and wait for t_a2 + t_d2.
    We need to wait for hold time + release time because
triggered_count is increased before reset ops is called. When the
second driver finds that triggered_count is 1 and skip the real reset
operation, reset ops might just begin doing its work a short time ago.
    I am not sure whether we would need a flag in reset controller to
mark that "reset is done". When driver sees this flag is done, it can
just wait for release time instead of hold time + release time.

And I found that you already solved 1 and mentioned the
possible usage of 2 in [3].
There were discussion about letting device driver to deal with
reset-gpios by itself in [4], but it seems that reset controller can
better deal with the shared reset line situation.
Maybe we could revive the patch of GPIO support for reset controller ?

Please let me know what direction I should look into.
Thanks a lot!

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2010-November/033246.html
https://patchwork.kernel.org/patch/9424123/

[2] https://patchwork.kernel.org/patch/9424123/

[3] https://lore.kernel.org/patchwork/patch/455839/

[4] https://patchwork.kernel.org/patch/3978121/



On Fri, Sep 21, 2018 at 12:19 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Sep 12, 2018 at 08:19:55PM +0800, Cheng-Yi Chiang wrote:
>
> > +     /*
> > +      * Release reset GPIO because we are not going to use it.
> > +      */
> > +     devm_gpiod_put(dev, max98927->reset_gpio);
>
> There is no need to do this, it's still potentially useful information
> for userspace and it's also going to surprise anyone who tries to modify
> the code to use this function at some other time (eg, when coming out of
> suspend sometimes it's useful to reset the device).
>
> > @@ -934,6 +1010,8 @@ static int max98927_i2c_probe(struct i2c_client *i2c,
> >       if (ret < 0)
> >               dev_err(&i2c->dev, "Failed to register component: %d\n", ret);
> >
> > +     list_add(&max98927->list, &reset_list);
> > +
>
> I'm not seeing any locking of this list.  This also feels like something
> that shouldn't be in this driver but should be pushed up a level - it's
> only going to do the right thing if the reset line is only shared with
> other devices using this driver, if someone does something like put the
> CODEC and an external speaker amp on the same reset pin then it won't
> work.  I'm not sure where the best place to do that is; drivers/reset
> isn't really for this use case but feels right perhaps?
>
> Can you perhaps split the basic reset handling out from this list
> handling as a separate patch?

Re: [PATCH 2/2] ASoC: max98927: Add reset-gpio support

[Philipp Zabel]

Hi Cheng-yi,

[adding Maxime, devicetree to Cc:, the old discussion about GPIO resets
in [4] has never been resolved]

On Tue, 2018-10-09 at 21:46 +0800, Cheng-yi Chiang wrote:
> +reset controller maintainer Philipp
> 
> Hi Mark,
>   Sorry for the late reply. It took me a while to investigate reset
> controller and its possible usage. I would like to figure out the
> proper way of reset handling because it is common to have a shared
> reset line for two max98927 codecs for left and right channels.
> Without supporting this usage, a simple reset-gpios change for single
> codec would not be useful, and perhaps will be duplicated if reset
> controller is a better way.
> 
> Hi Philipp,
>   I would like to seek your advice about whether we can use reset
> controller to support the use case where multiple devices share the
> same reset line.
>
> Let me summarize the use case:
> There are two max98927 codecs sharing the same reset line.
> The reset line is controlled by a GPIO.
> The goal is to toggle reset line once and only once.
> 
> There was a similar scenario in tlv320aic3x codec driver [1].
> A static list is used in the codec driver so probe function can know
> whether it is needed to toggle reset line.
> Mark pointed out that it is not suitable to handle the shared reset
> line inside codec driver.
> A point is that this only works for multiple devices using the same
> device driver.
> He suggested to look into reset controller so I searched through the
> usage for common reset line.
>
> Here I found a shared reset line use case [2].
> With the patch [2], reset_control_reset can support this “reset once
> and for all” use case.

This patch was applied as 7da33a37b48f. So the reset framework has
support for shared reset controls where only the first reset request
actually causes a reset pulse, and all others are no-ops.

> However, I found that there are some missing pieces:
> 
> Let’s assume there are two codecs c1 and c2 sharing a reset line
> controlled by GPIO.
> 
> c1’s spec:
> Hold time: The minimum time to assert the reset line is t_a1.
> Release time: The minimum time to access the chip after deassert of
> the reset line is t_d1.
> 
> c2’s spec:
> Hold time: The minimum time to assert the reset line is t_a2.
> Release time: The minimum time to access the chip after deassert of
> the reset line is t_d2.
> 
> For both c1 and c2 to work properly, we need a reset controller that
> can assert the reset line for
> T = max(t_a1, t_a2).
> 
> 1. We need reset controller to support GPIO.

I still don't like the idea of describing a separate gpio reset
controller "device" in DT very much, as this is really just a software
abstraction, not actual hardware. For example:

	gpio_reset: reset-controller {
		compatible = "gpio-reset";
		reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>,
			      <&gpio1
16 GPIO_ACTIVE_HIGH>;
		reset-delays-us = <10000>, <500>;
	};

	c1 {
		resets = <&gpio_reset 0>; /* maps to <&gpio0 15 ...> */
	};

	c2 {
		resets = <&gpio_reset 0>;
	};

What I would like better would be to let the consumers keep their reset-
gpios bindings, but add an optional hold time override in the DT:

	c1 {
		reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
		reset-delays-us = <10000>;
	};

	c2 {
		reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
		re
set-delays-us = <10000>;
	};

The reset framework could create a reset_control backed by a gpio
instead of a rcdev. I have an unfinished patch for this, but without the
sharing requirement adding the reset framework abstraction between gpiod
and drivers never seemed really worth it.

> 2. We need to specify hold time T to reset controller in device tree
> so it knows that it needs hold reset line for that long in its
> implementation of reset_control_reset.

Agreed. Ideally, I'd like to make this optional, and have the drivers
continue to provide the hold time if they think they know it.

> 3. Assuming we have 1 and 2 in place. In codec driver of c1, it can
> call reset_control_reset and wait for t_a1 + t_d1. In codec driver of
> c2, it can call reset_control_reset and wait for t_a2 + t_d2.

The reset framework should wait for the configured assertion time,
max(t_a1, t_a2). The drivers only should only have to wait for
t_d1 / t_d2 after reset_control_reset returns.

>     We need to wait for hold time + release time because
> triggered_count is increased before reset ops is called. When the
> second driver finds that triggered_count is 1 and skip the real reset
> operation, reset ops might just begin doing its work a short time ago.

That is a good point. Maybe the reset framework should just wait for the
hold time even for the second reset. Another possibility would be to
serialize them with a mutex.

>     I am not sure whether we would need a flag in reset controller to
> mark that "reset is done". When driver sees this flag is done, it can
> just wait for release time instead of hold time + release time.

Let's not complicate the drivers with this too much. I think
reset_control_reset should guarantee that the reset line is not asserted
anymore upon return.

> And I found that you already solved 1 and mentioned the
> possible usage of 2 in [3].
> There were discussion about letting device driver to deal with
> reset-gpios by itself in [4], but it seems that reset controller can
> better deal with the shared reset line situation.
> Maybe we could revive the patch of GPIO support for reset controller ?

The discussion with Maxime in [4] hasn't really been resolved. I still
think the reset-gpios property should be kept, and the reset framework
could adapt to it. This is something the device tree maintainers' input
would be welcome on.

> Please let me know what direction I should look into.
> Thanks a lot!
> 
> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2010-November/033246.html
> https://patchwork.kernel.org/patch/9424123/
> 
> [2] https://patchwork.kernel.org/patch/9424123/
> 
> [3] https://lore.kernel.org/patchwork/patch/455839/
> 
> [4] https://patchwork.kernel.org/patch/3978121/

regards
Philipp

Re: [PATCH 2/2] ASoC: max98927: Add reset-gpio support

[Maxime Ripard]

On Fri, Oct 12, 2018 at 12:05:16PM +0200, Philipp Zabel wrote:
> Hi Cheng-yi,
> 
> [adding Maxime, devicetree to Cc:, the old discussion about GPIO resets
> in [4] has never been resolved]
> 
> On Tue, 2018-10-09 at 21:46 +0800, Cheng-yi Chiang wrote:
> > +reset controller maintainer Philipp
> > 
> > Hi Mark,
> >   Sorry for the late reply. It took me a while to investigate reset
> > controller and its possible usage. I would like to figure out the
> > proper way of reset handling because it is common to have a shared
> > reset line for two max98927 codecs for left and right channels.
> > Without supporting this usage, a simple reset-gpios change for single
> > codec would not be useful, and perhaps will be duplicated if reset
> > controller is a better way.
> > 
> > Hi Philipp,
> >   I would like to seek your advice about whether we can use reset
> > controller to support the use case where multiple devices share the
> > same reset line.
> >
> > Let me summarize the use case:
> > There are two max98927 codecs sharing the same reset line.
> > The reset line is controlled by a GPIO.
> > The goal is to toggle reset line once and only once.
> > 
> > There was a similar scenario in tlv320aic3x codec driver [1].
> > A static list is used in the codec driver so probe function can know
> > whether it is needed to toggle reset line.
> > Mark pointed out that it is not suitable to handle the shared reset
> > line inside codec driver.
> > A point is that this only works for multiple devices using the same
> > device driver.
> > He suggested to look into reset controller so I searched through the
> > usage for common reset line.
> >
> > Here I found a shared reset line use case [2].
> > With the patch [2], reset_control_reset can support this “reset once
> > and for all” use case.
> 
> This patch was applied as 7da33a37b48f. So the reset framework has
> support for shared reset controls where only the first reset request
> actually causes a reset pulse, and all others are no-ops.
> 
> > However, I found that there are some missing pieces:
> > 
> > Let’s assume there are two codecs c1 and c2 sharing a reset line
> > controlled by GPIO.
> > 
> > c1’s spec:
> > Hold time: The minimum time to assert the reset line is t_a1.
> > Release time: The minimum time to access the chip after deassert of
> > the reset line is t_d1.
> > 
> > c2’s spec:
> > Hold time: The minimum time to assert the reset line is t_a2.
> > Release time: The minimum time to access the chip after deassert of
> > the reset line is t_d2.
> > 
> > For both c1 and c2 to work properly, we need a reset controller that
> > can assert the reset line for
> > T = max(t_a1, t_a2).
> > 
> > 1. We need reset controller to support GPIO.
> 
> I still don't like the idea of describing a separate gpio reset
> controller "device" in DT very much, as this is really just a software
> abstraction, not actual hardware. For example:
> 
> 	gpio_reset: reset-controller {
> 		compatible = "gpio-reset";
> 		reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>,
> 			      <&gpio1
> 16 GPIO_ACTIVE_HIGH>;
> 		reset-delays-us = <10000>, <500>;
> 	};
> 
> 	c1 {
> 		resets = <&gpio_reset 0>; /* maps to <&gpio0 15 ...> */
> 	};
> 
> 	c2 {
> 		resets = <&gpio_reset 0>;
> 	};
> 
> What I would like better would be to let the consumers keep their reset-
> gpios bindings, but add an optional hold time override in the DT:
> 
> 	c1 {
> 		reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
> 		reset-delays-us = <10000>;
> 	};
> 
> 	c2 {
> 		reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
> 		reset-delays-us = <10000>;
> 	};
> 
> The reset framework could create a reset_control backed by a gpio
> instead of a rcdev. I have an unfinished patch for this, but without the
> sharing requirement adding the reset framework abstraction between gpiod
> and drivers never seemed really worth it.

I don't remember the exact details of our past discussion, but I
(still) don't really think that this would work well. I see two main
shortcomings with that approach:

  - I guess that the main reason you want to do that would be to have
    easy DT backward compatibility. Yet, the addition of the
    reset-delay-us would break that compatibility, since drivers that
    would have been converted now need to have it somehow, but older
    DTs wouldn't have it

  - There's so many variations of the reset-gpios property name that
    you wouldn't be able to cover all the cases anyhow, at least
    without breaking the compatibility (again).

But I also see your point, and you're right that converting everyone
to a gpio-reset node will not happen (even though I'd still really
like to have that binding).

What about having a function that would be called by the consumer to
instantiate that reset controller from a GPIO for us, instead of
trying to do it automatically? That function could take the property
name that holds the GPIO, which would cover the second drawback, and
the delay, which would cover the first.

And in order to cover shared GPIO reset lines, we could just look at
the one already created by other drivers, and if one has been created,
we just give the reference to that one instead of creating it.

Does that make sense?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/2] ASoC: max98927: Add reset-gpio support

[Philipp Zabel]

Hi Maxime,

On Fri, 2018-10-12 at 15:46 +0200, Maxime Ripard wrote:
> On Fri, Oct 12, 2018 at 12:05:16PM +0200, Philipp Zabel wrote:
[...]
> > What I would like better would be to let the consumers keep their reset-
> > gpios bindings, but add an optional hold time override in the DT:
> > 
> > 	c1 {
> > 		reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
> > 		reset-delays-us = <10000>;
> > 	};
> > 
> > 	c2 {
> > 		reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
> > 		reset-delays-us = <10000>;
> > 	};
> > 
> > The reset framework could create a reset_control backed by a gpio
> > instead of a rcdev. I have an unfinished patch for this, but without the
> > sharing requirement adding the reset framework abstraction between gpiod
> > and drivers never seemed really worth it.
> 
> I don't remember the exact details of our past discussion, but I
> (still) don't really think that this would work well.

It has been a while :) Thanks for jumping back in.

> I see two main shortcomings with that approach:
> 
>   - I guess that the main reason you want to do that would be to have
>     easy DT backward compatibility.

Yes, that is true. The other reason is that I'd like devices to have a
single binding, regardless of whether somebody decided to put them onto
a board with shared reset lines.

I'd find it hard to advocate for changing the thankfully common case
of device-exclusive reset gpios from:

	some-device {
		reset-gpios = <&gpiox y>;
	};

to:

	rstc: reset-controller {
		compatible = "gpio-reset";
		reset-gpios <&gpiox y>;
	};

	some-device {
		resets = <&rstc 0>;
	};

even for new bindings.

If the reset framework only supports the latter, and not the former,
drivers for devices which already have reset-gpios would have to handle
both reset_control and gpiod functionality, which I think should not be
necessary.

Note that this is not really an argument against a "gpio-reset"
controller driver, but an argument for reset-gpios support integrated
into the reset framework.
My argument against a "gpio-reset" device node in the DT is only that it
is basically a virtual device that would only be used to work around
missing reset-gpios support in the reset framework. Physically, this
"device" consists of no more than a few PCB traces.

>     Yet, the addition of the
>     reset-delay-us would break that compatibility, since drivers that
>     would have been converted now need to have it somehow, but older
>     DTs wouldn't have it

That is why such a property would have to be optional, and the drivers
would have to keep providing the reset delay themselves, as they
currently do when handling GPIOs directly.
It would only serve to override the driver default in case of additional
delay requirements due to board specifics (such as delay elements, or
shared resets).

>   - There's so many variations of the reset-gpios property name that
>     you wouldn't be able to cover all the cases anyhow, at least
>     without breaking the compatibility (again).

This is true, and I do not have a solution for this. Especially for
those cases that don't use gpiod yet.

	of_get_named_gpio(node, "reset-gpio")

is still quite common, but I'd really like on gpiolib for polarity
support.

> But I also see your point, and you're right that converting everyone
> to a gpio-reset node will not happen (even though I'd still really
> like to have that binding).

See above. I'd be a lot less reluctant to support this binding if
somebody could demonstrate a real gpio controlled reset controller
device of some kind. And even then I would not want to have to use
this device just to connect a single GPIO with a single reset input.

> What about having a function that would be called by the consumer to
> instantiate that reset controller from a GPIO for us, instead of
> trying to do it automatically? That function could take the property
> name that holds the GPIO, which would cover the second drawback, and
> the delay, which would cover the first.

I like this idea.

I'd like to avoid having to fall back from gpiod to of_get_named_gpio if
possible, so maybe we'd have to extend gpiolib-of.c with an
of_find_reset_gpio function, as is done for SPI and regulators already.

We probably would have to support delay ranges. I see a lot of
usleep_range calls between reset GPIO toggles in the drivers.

> And in order to cover shared GPIO reset lines, we could just look at
> the one already created by other drivers, and if one has been created,
> we just give the reference to that one instead of creating it.
> 
> Does that make sense?

I'm not quite sure how to match an already requested reset control from
the list given of_node and gpio property name at the moment - this might
involve exporting gpiod_find from gpiolib, but API wise I think this is
a sane proposal.

I would not want to add reset controller devices for each of these GPIO
reset controls, but rather store the gpio_desc reference in the
reset_control instead of the rcdev reference.

regards
Philipp

Re: [PATCH 2/2] ASoC: max98927: Add reset-gpio support

[Cheng-yi Chiang]

Hi Philipp,

On Thu, Oct 18, 2018 at 1:02 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Maxime,
>
> On Fri, 2018-10-12 at 15:46 +0200, Maxime Ripard wrote:
> > On Fri, Oct 12, 2018 at 12:05:16PM +0200, Philipp Zabel wrote:
> [...]
> > > What I would like better would be to let the consumers keep their reset-
> > > gpios bindings, but add an optional hold time override in the DT:
> > >
> > >     c1 {
> > >             reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
> > >             reset-delays-us = <10000>;
> > >     };
> > >
> > >     c2 {
> > >             reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
> > >             reset-delays-us = <10000>;
> > >     };
> > >
> > > The reset framework could create a reset_control backed by a gpio
> > > instead of a rcdev. I have an unfinished patch for this, but without the
> > > sharing requirement adding the reset framework abstraction between gpiod
> > > and drivers never seemed really worth it.
> >
> > I don't remember the exact details of our past discussion, but I
> > (still) don't really think that this would work well.
>
> It has been a while :) Thanks for jumping back in.
>
> > I see two main shortcomings with that approach:
> >
> >   - I guess that the main reason you want to do that would be to have
> >     easy DT backward compatibility.
>
> Yes, that is true. The other reason is that I'd like devices to have a
> single binding, regardless of whether somebody decided to put them onto
> a board with shared reset lines.
>
> I'd find it hard to advocate for changing the thankfully common case
> of device-exclusive reset gpios from:
>
>         some-device {
>                 reset-gpios = <&gpiox y>;
>         };
>
> to:
>
>         rstc: reset-controller {
>                 compatible = "gpio-reset";
>                 reset-gpios <&gpiox y>;
>         };
>
>         some-device {
>                 resets = <&rstc 0>;
>         };
>
> even for new bindings.
>
> If the reset framework only supports the latter, and not the former,
> drivers for devices which already have reset-gpios would have to handle
> both reset_control and gpiod functionality, which I think should not be
> necessary.
>
> Note that this is not really an argument against a "gpio-reset"
> controller driver, but an argument for reset-gpios support integrated
> into the reset framework.
> My argument against a "gpio-reset" device node in the DT is only that it
> is basically a virtual device that would only be used to work around
> missing reset-gpios support in the reset framework. Physically, this
> "device" consists of no more than a few PCB traces.
>
> >     Yet, the addition of the
> >     reset-delay-us would break that compatibility, since drivers that
> >     would have been converted now need to have it somehow, but older
> >     DTs wouldn't have it
>
> That is why such a property would have to be optional, and the drivers
> would have to keep providing the reset delay themselves, as they
> currently do when handling GPIOs directly.
> It would only serve to override the driver default in case of additional
> delay requirements due to board specifics (such as delay elements, or
> shared resets).
>
> >   - There's so many variations of the reset-gpios property name that
> >     you wouldn't be able to cover all the cases anyhow, at least
> >     without breaking the compatibility (again).
>
> This is true, and I do not have a solution for this. Especially for
> those cases that don't use gpiod yet.
>
>         of_get_named_gpio(node, "reset-gpio")
>
> is still quite common, but I'd really like on gpiolib for polarity
> support.
>
> > But I also see your point, and you're right that converting everyone
> > to a gpio-reset node will not happen (even though I'd still really
> > like to have that binding).
>
> See above. I'd be a lot less reluctant to support this binding if
> somebody could demonstrate a real gpio controlled reset controller
> device of some kind. And even then I would not want to have to use
> this device just to connect a single GPIO with a single reset input.
>
> > What about having a function that would be called by the consumer to
> > instantiate that reset controller from a GPIO for us, instead of
> > trying to do it automatically? That function could take the property
> > name that holds the GPIO, which would cover the second drawback, and
> > the delay, which would cover the first.
>
> I like this idea.
>
> I'd like to avoid having to fall back from gpiod to of_get_named_gpio if
> possible, so maybe we'd have to extend gpiolib-of.c with an
> of_find_reset_gpio function, as is done for SPI and regulators already.
>
> We probably would have to support delay ranges. I see a lot of
> usleep_range calls between reset GPIO toggles in the drivers.
>

This looks great.

I am not sure what is the plan proceeding from here.
Are you going to implement this feature with the unfinished patch you
mentioned to create reset_control backed by a GPIO?
I am willing to help.
If you have patch I can help testing it.
It will be easier to test the shared reset line on my side since the
board I am working now has this use case.

If you don't have time for this, I can work on it based on your WIP patch.
In that case could you please post the patch ?
I just want to make sure I am on the right direction solving this
without making duplicate efforts..

> > And in order to cover shared GPIO reset lines, we could just look at
> > the one already created by other drivers, and if one has been created,
> > we just give the reference to that one instead of creating it.
> >
> > Does that make sense?
>
> I'm not quite sure how to match an already requested reset control from
> the list given of_node and gpio property name at the moment - this might
> involve exporting gpiod_find from gpiolib, but API wise I think this is
> a sane proposal.
>
> I would not want to add reset controller devices for each of these GPIO
> reset controls, but rather store the gpio_desc reference in the
> reset_control instead of the rcdev reference.

This plan to support shared reset line makes sense.

Thanks a lot!

>
> regards
> Philipp

Re: [PATCH 1/2] ASoC: max9892x: Add documentation for reset-gpio support

[Mark Brown]

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

On Wed, Sep 12, 2018 at 08:19:54PM +0800, Cheng-Yi Chiang wrote:
> max98927 codec driver will support reset-gpio binding so it can toggle
> reset line in its probe function.

All GPIO properties are supposed to end -gpios even if there can only
ever be a single GPIO.

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

Re: [PATCH 1/2] ASoC: max9892x: Add documentation for reset-gpio support

[Cheng-yi Chiang]

On Wed, Sep 12, 2018 at 8:28 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Sep 12, 2018 at 08:19:54PM +0800, Cheng-Yi Chiang wrote:
> > max98927 codec driver will support reset-gpio binding so it can toggle
> > reset line in its probe function.
>
> All GPIO properties are supposed to end -gpios even if there can only
> ever be a single GPIO.

Hi Mark. I see.
I will update this along with the comments of the change in max98927.c
if there is any.
Thanks!