Re: [PATCH 1/7] ALSA: ac97: Add helper function to reset the AC97 device

From: Takashi Iwai <tiwai@suse.de>
To: Lars-Peter Clausen <lars@metafoo.de>
Cc: alsa-devel@alsa-project.org, patches@opensource.wolfsonmicro.com,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Manuel Lauss <manuel.lauss@gmail.com>,
	Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Subject: Re: [PATCH 1/7] ALSA: ac97: Add helper function to reset the AC97 device
Date: Wed, 22 Jul 2015 10:44:30 +0200	[thread overview]
Message-ID: <s5h4mkwintd.wl-tiwai@suse.de> (raw)
In-Reply-To: <1437508386-13828-2-git-send-email-lars@metafoo.de>

On Tue, 21 Jul 2015 21:53:00 +0200,
Lars-Peter Clausen wrote:
> 
> There is currently a lot of code duplication in ASoC drivers regarding the
> reset handling of devices. This patch introduces a new generic reset
> function in the generic AC'97 framework that can be used to replace most
> the custom reset functions.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Reviewed-by: Takashi Iwai <tiwai@suse.de>

One possible improvement would be to allow to pass id_mask=0 as the
full bit, so that you don't have to define the mask 0xffffffff at each
time.

thanks,

Takashi

> ---
>  include/sound/ac97_codec.h |  2 ++
>  sound/ac97_bus.c           | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/include/sound/ac97_codec.h b/include/sound/ac97_codec.h
> index 0e9d75b..74bc8547 100644
> --- a/include/sound/ac97_codec.h
> +++ b/include/sound/ac97_codec.h
> @@ -584,6 +584,8 @@ static inline int snd_ac97_update_power(struct snd_ac97 *ac97, int reg,
>  void snd_ac97_suspend(struct snd_ac97 *ac97);
>  void snd_ac97_resume(struct snd_ac97 *ac97);
>  #endif
> +int snd_ac97_reset(struct snd_ac97 *ac97, bool try_warm, unsigned int id,
> +	unsigned int id_mask);
>  
>  /* quirk types */
>  enum {
> diff --git a/sound/ac97_bus.c b/sound/ac97_bus.c
> index 2b50cbe..55791a0 100644
> --- a/sound/ac97_bus.c
> +++ b/sound/ac97_bus.c
> @@ -18,6 +18,68 @@
>  #include <sound/ac97_codec.h>
>  
>  /*
> + * snd_ac97_check_id() - Reads and checks the vendor ID of the device
> + * @ac97: The AC97 device to check
> + * @id: The ID to compare to
> + * @id_mask: Mask that is applied to the device ID before comparing to @id
> + *
> + * If @id is 0 this function returns true if the read device vendor ID is
> + * a valid ID. If @id is non 0 this functions returns true if @id
> + * matches the read vendor ID. Otherwise the function returns false.
> + */
> +static bool snd_ac97_check_id(struct snd_ac97 *ac97, unsigned int id,
> +	unsigned int id_mask)
> +{
> +	ac97->id = ac97->bus->ops->read(ac97, AC97_VENDOR_ID1) << 16;
> +	ac97->id |= ac97->bus->ops->read(ac97, AC97_VENDOR_ID2);
> +
> +	if (ac97->id == 0x0 || ac97->id == 0xffffffff)
> +		return false;
> +
> +	if (id != 0 && id != (ac97->id & id_mask))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * snd_ac97_reset() - Reset AC'97 device
> + * @ac97: The AC'97 device to reset
> + * @try_warm: Try a warm reset first
> + * @id: Expected device vendor ID
> + * @id_mask: Mask that is applied to the device ID before comparing to @id
> + *
> + * This function resets the AC'97 device. If @try_warm is true the function
> + * first performs a warm reset. If the warm reset is successful the function
> + * returns 1. Otherwise or if @try_warm is false the function issues cold reset
> + * followed by a warm reset. If this is successful the function returns 0,
> + * otherwise a negative error code. If @id is 0 any valid device ID will be
> + * accepted, otherwise only the ID that matches @id and @id_mask is accepted.
> + */
> +int snd_ac97_reset(struct snd_ac97 *ac97, bool try_warm, unsigned int id,
> +	unsigned int id_mask)
> +{
> +	struct snd_ac97_bus_ops *ops = ac97->bus->ops;
> +
> +	if (try_warm && ops->warm_reset) {
> +		ops->warm_reset(ac97);
> +		if (snd_ac97_check_id(ac97, id, id_mask))
> +			return 1;
> +	}
> +
> +	if (ops->reset)
> +		ops->reset(ac97);
> +	if (ops->warm_reset)
> +		ops->warm_reset(ac97);
> +
> +	if (snd_ac97_check_id(ac97, id, id_mask))
> +		return 0;
> +
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(snd_ac97_reset);
> +
> +/*
>   * Let drivers decide whether they want to support given codec from their
>   * probe method. Drivers have direct access to the struct snd_ac97
>   * structure and may  decide based on the id field amongst other things.
> -- 
> 2.1.4
> 
> 