Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 7 Jan 2020 14:27:42 +0000
From:   Lee Jones <lee.jones@linaro.org>
To:     Charles Keepax <ckeepax@opensource.cirrus.com>
Cc:     linux-kernel@vger.kernel.org, patches@opensource.cirrus.com
Subject: Re: [PATCH 1/2] mfd: madera: Allow more time for hardware reset
Message-ID: <20200107142742.GN14821@dell>
References: <20200106102834.31301-1-ckeepax@opensource.cirrus.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200106102834.31301-1-ckeepax@opensource.cirrus.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 06 Jan 2020, Charles Keepax wrote:

> Both manual and power on resets have a brief period where the chip will
> not be accessible immediately afterwards. Extend the time allowed for
> this from a minimum of 1mS to 2mS based on newer evaluation of the
> hardware and ensure this reset happens in all reset conditions. Whilst
> making the change also remove the redundant NULL checks in the reset
> functions as the GPIO functions already check for this.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  drivers/mfd/madera-core.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
> index a8cfadc1fc01e..f41ce408259fb 100644
> --- a/drivers/mfd/madera-core.c
> +++ b/drivers/mfd/madera-core.c
> @@ -238,6 +238,11 @@ static int madera_wait_for_boot(struct madera *madera)
>  	return ret;
>  }
>  
> +static inline void madera_reset_delay(void)
> +{
> +	usleep_range(2000, 3000);
> +}

Hmm ... We usually shy away from abstraction for the sake of
abstraction.  What's preventing you from using the preferred method of
simply calling the abstracted function from each of the call-sites?

I could understand (a little) if you needed to frequently change these
values, since changing them in once place is obviously simpler than
changing them in 3, but even then it's marginal.

>  static int madera_soft_reset(struct madera *madera)
>  {
>  	int ret;
> @@ -249,16 +254,13 @@ static int madera_soft_reset(struct madera *madera)
>  	}
>  
>  	/* Allow time for internal clocks to startup after reset */
> -	usleep_range(1000, 2000);
> +	madera_reset_delay();
>  
>  	return 0;
>  }
>  
>  static void madera_enable_hard_reset(struct madera *madera)
>  {
> -	if (!madera->pdata.reset)
> -		return;
> -
>  	/*
>  	 * There are many existing out-of-tree users of these codecs that we
>  	 * can't break so preserve the expected behaviour of setting the line
> @@ -269,11 +271,9 @@ static void madera_enable_hard_reset(struct madera *madera)
>  
>  static void madera_disable_hard_reset(struct madera *madera)
>  {
> -	if (!madera->pdata.reset)
> -		return;
> -
>  	gpiod_set_raw_value_cansleep(madera->pdata.reset, 1);
> -	usleep_range(1000, 2000);
> +
> +	madera_reset_delay();
>  }
>  
>  static int __maybe_unused madera_runtime_resume(struct device *dev)
> @@ -292,6 +292,8 @@ static int __maybe_unused madera_runtime_resume(struct device *dev)
>  	regcache_cache_only(madera->regmap, false);
>  	regcache_cache_only(madera->regmap_32bit, false);
>  
> +	madera_reset_delay();
> +
>  	ret = madera_wait_for_boot(madera);
>  	if (ret)
>  		goto err;

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog