[alsa-devel] [PATCH 3/3] ASoC: Ux500: Add driver for Ux500/AB8500 platform/codec

[Mark Brown]

On Mon, Jan 30, 2012 at 03:18:30PM +0100, Ola Lilja wrote:
> Add the ST-Ericsson Ux500 ASoC platform-driver, AB8500 codec-driver and
> machine-driver for U8500 hardware.

This *needs* to be split up for review, there's a *lot* of only
tangentally related things in here and...

>  26 files changed, 8519 insertions(+), 9 deletions(-)

...an over 8000 line patch is not easy to review at the best of times.

I'm going to comment on a few things but this is really not a detailed
review.  From the brief look I've had (most of my comments are on the
CODEC driver but I did at least glance at most of the code) my overall
comment is that the code doesn't look like ASoC code at a structural
level.  The fact that it's all submitted in one patch seems indicative
of this, and for example what looks like it's supposed to be a machine
driver is peering into the CODEC power management directly rather than
letting the CODEC driver get on with things.

Please make much more of an effort to work with the framework, you
should have a series of isolated drivers for the various components of
the system and they should all be using framework features where
possible.

In terms of ths split:

>  arch/arm/mach-ux500/board-mop500-msp.c        |  195 ++
>  arch/arm/mach-ux500/board-mop500-regulators.c |   28 +
>  arch/arm/mach-ux500/board-mop500.c            |    7 +-
>  arch/arm/mach-ux500/board-mop500.h            |    1 +

This adds machine support for a particular board on the ARM side.

>  arch/arm/mach-ux500/clock.c                   |    8 +-
>  arch/arm/mach-ux500/devices-common.h          |    2 +-
>  arch/arm/mach-ux500/include/mach/msp.h        | 1030 +++++++++

This looks like some random CPU core stuff.

>  include/sound/ux500_ab8500.h                  |   36 +
>  sound/soc/Kconfig                             |    1 +
>  sound/soc/Makefile                            |    1 +

This should be a separate patch on the end of the series which enables
the build.

>  sound/soc/codecs/Kconfig                      |    4 +
>  sound/soc/codecs/Makefile                     |    6 +
>  sound/soc/codecs/Makefile.rej                 |   10 +
>  sound/soc/codecs/ab8500_audio.c               | 2928 +++++++++++++++++++++++++
>  sound/soc/codecs/ab8500_audio.h               |  673 ++++++

This adds the CODEC driver and should be done separately and you
shouldn't be sending .rej files as part of your patch.

>  sound/soc/ux500/Kconfig                       |   33 +
>  sound/soc/ux500/Makefile                      |   25 +
>  sound/soc/ux500/u8500.c                       |  150 ++
>  sound/soc/ux500/ux500_ab8500.c                |  790 +++++++

One of these is a machine driver, again should be separate.

>  sound/soc/ux500/ux500_msp_dai.c               |  998 +++++++++
>  sound/soc/ux500/ux500_msp_dai.h               |   83 +
>  sound/soc/ux500/ux500_msp_i2s.c               | 1004 +++++++++
>  sound/soc/ux500/ux500_msp_i2s.h               |   40 +

Not sure what the split is here but this looks like the I2S controller
and can go as a separate patch.

>  sound/soc/ux500/ux500_pcm.c                   |  428 ++++
>  sound/soc/ux500/ux500_pcm.h                   |   44 +

The DMA controller should also be split.

> +/*** Sample Frequencies ***/
> +/* These are no longer required, frequencies in Hz can be used directly */

If they are not required then remove them.

> +#ifndef UX500_AB8500_H
> +#define UX500_AB8500_H
> +
> +extern struct snd_soc_ops ux500_ab8500_ops[];
> +
> +struct snd_soc_pcm_runtime;
> +
> +int ux500_ab8500_startup(struct snd_pcm_substream *substream);
> +
> +void ux500_ab8500_shutdown(struct snd_pcm_substream *substream);
> +
> +int ux500_ab8500_hw_params(struct snd_pcm_substream *substream,
> +			struct snd_pcm_hw_params *params);
> +
> +int ux500_ab8500_soc_machine_drv_init(void);
> +
> +void ux500_ab8500_soc_machine_drv_cleanup(void);
> +
> +int ux500_ab8500_machine_codec_init(struct snd_soc_pcm_runtime *runtime);
> +
> +extern void ux500_ab8500_jack_report(int);
> +

Nothing in this header looks like it should be exported, I've not
actually looked at any of the code to see what this stuff is doing but
it all looks like it should be private to the relevant drivers.

> --- a/sound/soc/Kconfig
> +++ b/sound/soc/Kconfig
> @@ -45,6 +45,7 @@ source "sound/soc/s6000/Kconfig"
>  source "sound/soc/sh/Kconfig"
>  source "sound/soc/tegra/Kconfig"
>  source "sound/soc/txx9/Kconfig"
> +source "sound/soc/ux500/Kconfig"
>  
>  # Supported codecs
>  source "sound/soc/codecs/Kconfig"

Keep Kconfig and Makefiles sorted.

> index 7c205e7..a02b2c3 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -12,6 +12,7 @@ config SND_SOC_ALL_CODECS
>  	tristate "Build all ASoC CODEC drivers"
>  	select SND_SOC_88PM860X if MFD_88PM860X
>  	select SND_SOC_L3
> +	select SND_SOC_AB8500
>  	select SND_SOC_AC97_CODEC if SND_SOC_AC97_BUS
>  	select SND_SOC_AD1836 if SPI_MASTER
>  	select SND_SOC_AD193X if SND_SOC_I2C_AND_SPI

Only if the MFD is there or it won't build.

> @@ -201,3 +203,7 @@ obj-$(CONFIG_SND_SOC_WM_HUBS)	+= snd-soc-wm-hubs.o
>  # Amp
>  obj-$(CONFIG_SND_SOC_MAX9877)	+= snd-soc-max9877.o
>  obj-$(CONFIG_SND_SOC_TPA6130A2)	+= snd-soc-tpa6130a2.o
> +
> +ifdef CONFIG_SND_SOC_UX500_DEBUG
> +CFLAGS_ab8500_audio.o := -DDEBUG
> +endif

No other driver has this.  Why does your driver have it?

> +/* Macros to simplify implementation of register write sequences and error handling */
> +#define AB8500_SET_BIT_LOCKED(xreg, xbit, xerr, xerr_hdl) { \
> +	xerr = snd_soc_update_bits_locked(ab8500_codec, xreg, REG_MASK_NONE, BMASK(xbit)); \
> +	if (xerr < 0) \
> +		goto xerr_hdl; }
> +#define AB8500_CLEAR_BIT_LOCKED(xreg, xbit, xerr, xerr_hdl) { \
> +	xerr = snd_soc_update_bits_locked(ab8500_codec, xreg, BMASK(xbit), REG_MASK_NONE); \
> +	if (xerr < 0) \
> +		goto xerr_hdl; }
> +#define AB8500_WRITE(xreg, xvalue, xerr, xerr_hdl) { \
> +	xerr = snd_soc_write(ab8500_codec, xreg, xvalue); \
> +	if (xerr < 0) \
> +		goto xerr_hdl; }

This just obscures the code.

> +/*
> + * AB8500 register cache & default register settings
> + */
> +static const u8 ab8500_reg_cache[AB8500_CACHEREGNUM] = {

Hrm, I would say the core ought to be converted over to regmap but it
looks like the "I2C" I/O actually goes via some magic "prcmu" interface
and not I2C.  I'd really like to try to avoid adding new cache users but
this does look like a valid use.

> +static struct snd_soc_codec *ab8500_codec;

If you need this something is seriously wrong.

> +/* ANC FIR- & IIR-coeff caches */
> +static int ab8500_anc_fir_coeff_cache[REG_ANC_FIR_COEFFS];
> +static int ab8500_anc_iir_coeff_cache[REG_ANC_IIR_COEFFS];

No global static data, store it per device.

> +static int ab8500_codec_read_reg(struct snd_soc_codec *codec, unsigned int bank,
> +		unsigned int reg)
> +{
> +	u8 value;
> +	int status = abx500_get_register_interruptible(
> +		codec->dev, bank, reg, &value);

Non-interruptible I/O please.  The user killing their application
shouldn't cause us to fail to shut down the audio path.

> +	if (status < 0) {
> +		pr_err("%s: Register (%02x:%02x) read failed (%d).\n",
> +			__func__, (u8)bank, (u8)reg, status);
> +	} else {
> +		pr_debug("Read 0x%02x from register %02x:%02x\n",
> +			(u8)value, (u8)bank, (u8)reg);
> +		status = value;

We have plenty of diagnsotic support in the core for basic stuff like
register I/O.  When you are logging use dev_ variants.

> +static unsigned int ab8500_codec_read_reg_audio(struct snd_soc_codec *codec,
> +		unsigned int reg)
> +{
> +	u8 *cache = codec->reg_cache;
> +	return cache[reg];
> +}

Absolutely no open coded cache implementations, use the soc-cache
facilities.

> +static inline void ab8500_codec_dump_all_reg(struct snd_soc_codec *codec)
> +{
> +	int i;
> +
> +	pr_debug("%s Enter.\n", __func__);
> +
> +	for (i = AB8500_FIRST_REG; i <= AB8500_LAST_REG; i++)
> +		ab8500_codec_read_reg_audio_nocache(codec, i);
> +}

We have support for this in the core.

> +/* Updates an audio register.
> + */
> +static inline int ab8500_codec_update_reg_audio(struct snd_soc_codec *codec,
> +		unsigned int reg, unsigned int clr, unsigned int ins)
> +{
> +	unsigned int new, old;
> +
> +	old = ab8500_codec_read_reg_audio(codec, reg);
> +	new = (old & ~clr) | ins;
> +	if (old == new)
> +		return 0;
> +
> +	return ab8500_codec_write_reg_audio(codec, reg, new);
> +}

Ditto for this.

> +/* Generic soc info for signed register controls. */
> +int snd_soc_info_s(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_info *uinfo)

If this is generic (and it looks like it is) then what is it doing in a
particular driver?

> +static int st_fir_value_control_get(struct snd_kcontrol *kcontrol,
> +			   struct snd_ctl_elem_value *ucontrol)
> +{
> +	return 0;
> +}

This looks obviously buggy.

> +/* Controls - DAPM */

> +/* Inverted order - Ascending/Descending */
> +enum control_inversion {
> +	NORMAL = 0,
> +	INVERT = 1
> +};

Seriously?

> +	case SND_SOC_DAIFMT_CBS_CFM: /* codec clk slave & FRM master */
> +	case SND_SOC_DAIFMT_CBM_CFS: /* codec clk master & frame slave */
> +		pr_err("%s: ERROR: The device is either a master or a slave.\n",
> +			__func__);
> +	default:
> +		pr_err("%s: ERROR: Unsupporter master mask 0x%x\n",
> +			__func__,
> +			fmt & SND_SOC_DAIFMT_MASTER_MASK);
> +		return -EINVAL;
> +	}

All those three cases do the same thinng.

> +static int ab8500_codec_set_bit_delay_if1(struct snd_soc_codec *codec, unsigned int delay)
> +{
> +	unsigned int clear_mask, set_mask;
> +
> +	clear_mask = BMASK(REG_DIGIFCONF4_IF1DEL);
> +	set_mask = 0;
> +
> +	switch (delay) {
> +	case 0:
> +		break;
> +	case 1:
> +		set_mask |= BMASK(REG_DIGIFCONF4_IF1DEL);
> +		break;
> +	default:
> +		pr_err("%s: ERROR: Unsupported bit-delay (0x%x)!\n", __func__, delay);
> +		return -EINVAL;
> +	}

Eh?  Why is this done separately to the configuration of the AIF mode?

> +	data = ab8500_codec_read_reg(codec, AB8500_MISC, AB8500_GPIO_DIR4_REG);
> +	data |= GPIO27_DIR_OUTPUT | GPIO29_DIR_OUTPUT | GPIO31_DIR_OUTPUT;
> +	ab8500_codec_write_reg(codec, AB8500_MISC, AB8500_GPIO_DIR4_REG, data);

This loooks like platform data...

> +void ab8500_audio_power_control(bool power_on)
> +{
> +	if (ab8500_codec == NULL) {
> +		pr_err("%s: ERROR: AB8500 ASoC-driver not yet probed!\n", __func__);
> +		return;
> +	}

Use the framework features for power management, we've got enough ways
for your driver to get told when to power up and down none of which
require a global pointer to a device.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20120130/1a1a0a32/attachment.sig