Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio
To:     "Viorel Suman (OSS)" <viorel.suman@oss.nxp.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
        Shengjiu Wang <shengjiu.wang@nxp.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Viorel Suman <viorel.suman@nxp.com>,
        Lee Jones <lee.jones@linaro.org>,
        <alsa-devel@alsa-project.org>, <linux-kernel@vger.kernel.org>
References: <20201116222036.343635-1-viorel.suman@oss.nxp.com>
From:   Peter Ujfalusi <peter.ujfalusi@ti.com>
Message-ID: <cee287f3-29c1-771f-ca20-812de1ae8044@ti.com>
Date:   Thu, 19 Nov 2020 14:48:44 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.2
MIME-Version: 1.0
In-Reply-To: <20201116222036.343635-1-viorel.suman@oss.nxp.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 17/11/2020 0.20, Viorel Suman (OSS) wrote:
> From: Viorel Suman <viorel.suman@nxp.com>
> 
> Using GPIO API seems not a way to go for the case
> when the same reset GPIO is used to control several codecs.
> For a such case we can use the "gpio-reset" driver
> and the "shared" reset API to manage the reset GPIO -
> to deassert the reset when the first codec is powered up
> and assert it when there is no codec in use.
> DTS is supposed to look as follows:
> 
> / {
>     ak4458_reset: gpio-reset {
>        compatible = "gpio-reset";
>        reset-gpios = <&pca6416 4 GPIO_ACTIVE_LOW>;
>        #reset-cells = <0>;
>        initially-in-reset;

I can not find anything resembling to this in next-20201119.

Where is the implementation and documentation for this gpio-reset?

>     };
> };
> 
> &i2c3 {
>     pca6416: gpio@20 {
>        compatible = "ti,tca6416";
>        reg = <0x20>;
>        gpio-controller;
>        #gpio-cells = <2>;
>     };
> 
>     ak4458_1: ak4458@10 {
>        compatible = "asahi-kasei,ak4458";
>        reg = <0x10>;
>        resets = <&ak4458_reset>;
>     };
> 
>     ak4458_2: ak4458@11 {
>        compatible = "asahi-kasei,ak4458";
>        reg = <0x11>;
>        resets = <&ak4458_reset>;
>     };
> 
>     ak4458_3: ak4458@12 {
>        compatible = "asahi-kasei,ak4458";
>        reg = <0x12>;
>        resets = <&ak4458_reset>;
>     };
> };
> 
> Signed-off-by: Viorel Suman <viorel.suman@nxp.com>
> ---
>  sound/soc/codecs/ak4458.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/codecs/ak4458.c b/sound/soc/codecs/ak4458.c
> index 1010c9ee2e83..f27727cb1382 100644
> --- a/sound/soc/codecs/ak4458.c
> +++ b/sound/soc/codecs/ak4458.c
> @@ -13,6 +13,7 @@
>  #include <linux/of_gpio.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <sound/initval.h>
>  #include <sound/pcm_params.h>
> @@ -45,7 +46,7 @@ struct ak4458_priv {
>  	const struct ak4458_drvdata *drvdata;
>  	struct device *dev;
>  	struct regmap *regmap;
> -	struct gpio_desc *reset_gpiod;
> +	struct reset_control *reset;
>  	struct gpio_desc *mute_gpiod;
>  	int digfil;	/* SSLOW, SD, SLOW bits */
>  	int fs;		/* sampling rate */
> @@ -597,17 +598,17 @@ static struct snd_soc_dai_driver ak4497_dai = {
>  
>  static void ak4458_power_off(struct ak4458_priv *ak4458)
>  {
> -	if (ak4458->reset_gpiod) {
> -		gpiod_set_value_cansleep(ak4458->reset_gpiod, 0);
> -		usleep_range(1000, 2000);
> +	if (ak4458->reset) {
> +		reset_control_assert(ak4458->reset);
> +		msleep(20);
>  	}
>  }
>  
>  static void ak4458_power_on(struct ak4458_priv *ak4458)
>  {
> -	if (ak4458->reset_gpiod) {
> -		gpiod_set_value_cansleep(ak4458->reset_gpiod, 1);
> -		usleep_range(1000, 2000);
> +	if (ak4458->reset) {
> +		reset_control_deassert(ak4458->reset);
> +		msleep(20);
>  	}
>  }
>  
> @@ -685,7 +686,6 @@ static int __maybe_unused ak4458_runtime_resume(struct device *dev)
>  	if (ak4458->mute_gpiod)
>  		gpiod_set_value_cansleep(ak4458->mute_gpiod, 1);
>  
> -	ak4458_power_off(ak4458);
>  	ak4458_power_on(ak4458);
>  
>  	regcache_cache_only(ak4458->regmap, false);
> @@ -771,10 +771,9 @@ static int ak4458_i2c_probe(struct i2c_client *i2c)
>  
>  	ak4458->drvdata = of_device_get_match_data(&i2c->dev);
>  
> -	ak4458->reset_gpiod = devm_gpiod_get_optional(ak4458->dev, "reset",
> -						      GPIOD_OUT_LOW);
> -	if (IS_ERR(ak4458->reset_gpiod))
> -		return PTR_ERR(ak4458->reset_gpiod);
> +	ak4458->reset = devm_reset_control_get_optional_shared(ak4458->dev, NULL);
> +	if (IS_ERR(ak4458->reset))
> +		return PTR_ERR(ak4458->reset);

The binding documentation must be updated and you must support the gpio
way as well.

When I had this discussion around using the reset framework for shared
enable and/or reset pins it was suggested that _if_ such a driver makes
sense then it should internally handle (by using magic strings) the
fallback and work with pre-reset binding.

>  
>  	ak4458->mute_gpiod = devm_gpiod_get_optional(ak4458->dev, "mute",
>  						     GPIOD_OUT_LOW);
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki