Re: [PATCH] ASoC: tegra: convert to regmap

From: Mark Brown <broonie@opensource.wolfsonmicro.com>
To: Stephen Warren <swarren@wwwdotorg.org>
Cc: alsa-devel@alsa-project.org, Stephen Warren <swarren@nvidia.com>,
	Liam Girdwood <lrg@ti.com>
Subject: Re: [PATCH] ASoC: tegra: convert to regmap
Date: Tue, 10 Apr 2012 23:04:47 +0100	[thread overview]
Message-ID: <20120410220447.GW7499@opensource.wolfsonmicro.com> (raw)
In-Reply-To: <1334085365-11189-1-git-send-email-swarren@wwwdotorg.org>

[-- Attachment #1.1: Type: text/plain, Size: 1818 bytes --]

On Tue, Apr 10, 2012 at 01:16:05PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> For the Tegra I2S, SPDIF, and DAS drivers
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> Mark, this depends on the regmap->ASoC git pull request that I just sent.

...which would've been much better placed here so they don't get
misplaced.

>  sound/soc/tegra/Kconfig         |    1 +
>  sound/soc/tegra/tegra20_das.c   |   99 ++++++++--------------
>  sound/soc/tegra/tegra20_das.h   |    3 +-
>  sound/soc/tegra/tegra20_i2s.c   |  149 ++++++++++++++++-----------------
>  sound/soc/tegra/tegra20_i2s.h   |    3 +-
>  sound/soc/tegra/tegra20_spdif.c |  177 ++++++++++++++++++++-------------------
>  sound/soc/tegra/tegra20_spdif.h |    3 +-

It would have have been useful to split this into three smaller patches,
the I2S and S/PDIF changes look good but there's one funny in the DAS
driver.

> +#define LAST_REG(name) \
> +	(TEGRA20_DAS_##name + \
> +		(TEGRA20_DAS_##name##_STRIDE * TEGRA20_DAS_##name##_COUNT) - 4)

> +#define REG_IN_ARRAY(reg, name) \
> +	((reg >= TEGRA20_DAS_##name) && \
> +	 (reg <= LAST_REG(name) && \
> +	 (!((reg - TEGRA20_DAS_##name) % TEGRA20_DAS_##name##_STRIDE))))

> +static bool tegra20_das_wr_rd_reg(struct device *dev, unsigned int reg)
>  {
> +	if (REG_IN_ARRAY(reg, DAP_CTRL_SEL) ||
> +	    REG_IN_ARRAY(reg, DAC_INPUT_DATA_CLK_SEL))
> +		return true;

This seems a little too obscure for what's normally a very simple
function.  REG_IN_ARRAY() in particular seems odd, what's the array in
question and why do we need to do anything more than a simple mod to
check if the register is correctly aligned?  Some comments seem to be in
order at least, or just expanding REG_IN_ARRAY - there's only two uses.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]