[alsa-devel] [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

On Tue, Mar 13, 2012 at 04:11:40PM +0100, Ola Lilja wrote:

> +++ 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

This driver really has no dependencies?  That seems *extremely*
surprising - have you tried building on x86?

> +
> +ifdef CONFIG_SND_SOC_UX500_DEBUG
> +CFLAGS_ab8500_audio.o := -DDEBUG
> +endif

Once again if you're adding random custom stuff like this for your
driver you shouldn't be.

> diff --git a/sound/soc/codecs/ab8500_audio.c b/sound/soc/codecs/ab8500_audio.c

Everything else uses -codec.

> +/* AB8500 register cache */
> +static const u8 ab8500_reg_cache[AB8500_CACHEREGNUM];

> +static enum sid_state sid_status = SID_UNCONFIGURED;

No static data or custom cache implementations.

> +/* Read a register from the audio-bank of AB8500 */
> +static unsigned int ab8500_codec_read_reg(struct snd_soc_codec *codec, unsigned int reg)
> +{
> +	int status;
> +	unsigned int value = 0;
> +
> +	if (reg < VIRT_REG_BASE) {

Why are you doing this virtual register stuff?  It's making the code a
lot more complex and doesn't look at all driver specific.

> +		} else {
> +			dev_dbg(codec->dev, "%s: Read 0x%02x from register 0x%02x:0x%02x\n",
> +				__func__, value8, (u8)AB8500_AUDIO, (u8)reg);

There's already *plenty* of trace for register I/O in the framework.

> +static const char * const enum_ena_dis[] = {"Enabled", "Disabled"};
> +static const char * const enum_dis_ena[] = {"Disabled", "Enabled"};

Why are the controls using these enums and not Switch controls?  UIs
know how to display switches.

> +/* Earpiece - Mute */
> +static const struct snd_kcontrol_new dapm_ear_mute[] = {
> +	SOC_DAPM_SINGLE("Playback Switch", REG_MUTECONF, REG_MUTECONF_MUTEAR, 1, 1),
> +};

This looks like it's just a mute rather than a mixer input enable
(there's only one control...) so should be a regular sound control;
unless there's a very good reason mutes other than mixer inputs normally
don't affect the audio path as you might get pops and you probably don't
want to do things like stop clocking just because the output is
silenced.  Similar thing applies in several other places.

> +/* Earpiece source selector */
> +static const char * const enum_ear_lineout_source[] = {"Headset Left", "IHF Left"};
> +static SOC_ENUM_SINGLE_DECL(dapm_enum_ear_lineout_source, REG_DMICFILTCONF,
> +			REG_DMICFILTCONF_DA3TOEAR, enum_ear_lineout_source);
> +static const struct snd_kcontrol_new dapm_ear_lineout_source =
> +	SOC_DAPM_ENUM("Earpiece or LineOut Mono Source", dapm_enum_ear_lineout_source);

This control can probably have a much better name... 

> +/* IHF - Enable/Disable */
> +static const struct soc_enum enum_ihf_left = SOC_ENUM_SINGLE(0, 0, 2, enum_dis_ena);

80 columns please.

> +/* from -31 to 31 dB in 1 dB steps (mute instead of -32 dB) */
> +static DECLARE_TLV_DB_SCALE(adx_dig_gain_tlv, -3200, 100, 1);

I'm really not a big fan of this sort of comment, adds greatly to the
verbosity but just duplicates the code.

> +static const char * const enum_earselcm[] = {"0.95V", "1.10V", "1.27V", "1.58V"};
> +static SOC_ENUM_SINGLE_DECL(soc_enum_earselcm,
> +	REG_ANACONF1, REG_ANACONF1_EARSELCM, enum_earselcm);

Should this really be configured by the application layer and not
platform data?

> +/* Digital interface - DA from slot mapping */
> +static const char * const enum_da_from_slot_map[] = {"SLOT0",
> +					"SLOT1",

Indentation.  Also are you sure that this isn't DAPM stuff?

> +	SOC_ENUM("Earpiece DAC Low Power Playback Switch",
> +		soc_enum_eardaclowpow),

These aren't switches, they're enums.  Switches are strictly boolean
things with values 0 and 1.

> +	snd_soc_update_bits(codec,
> +			REG_DIGIFCONF3,
> +			BMASK(REG_DIGIFCONF3_IF1MASTER),
> +			BMASK(REG_DIGIFCONF3_IF1MASTER));

You're unconditionally setting these bits?

> +	snd_soc_update_bits(codec, REG_DIGIFCONF4, clr_mask, set_mask);

Either clr_mask and set_mask are misnamed or this doesn't do what you
think it does.  The bits specified by the third argument are set to the
values given in the fourth argument.  It looks like the code thinks that
the bits in the third argument will be cleared and the bits in the
fourth argument will be set.

> +static int ab8500_codec_set_word_length_if1(struct snd_soc_codec *codec, unsigned int wl)
> +{

Just inline stuff like this.

> +static int ab8500_codec_set_bit_delay_if1(struct snd_soc_codec *codec, unsigned int delay)

This too.

> +/* Configures audio macrocell into the AB8500 Chip */
> +static int ab8500_codec_configure_audio_macrocell(struct snd_soc_codec *codec)
> +{
> +	u8 value8;
> +	unsigned int value;
> +	int status;
> +
> +	status = ab8500_sysctrl_write(AB8500_STW4500CTRL3,
> +		AB8500_STW4500CTRL3_CLK32KOUT2DIS | AB8500_STW4500CTRL3_RESETAUDN,
> +		AB8500_STW4500CTRL3_RESETAUDN);
> +	if (status < 0)
> +		return status;

You're just unconditionally setting these values - shouldn't they be
platform data?

> +static int ab8500_codec_add_widgets(struct snd_soc_codec *codec)
> +{
> +	int status;
> +
> +	status = snd_soc_dapm_new_controls(&codec->dapm, ab8500_dapm_widgets,
> +			ARRAY_SIZE(ab8500_dapm_widgets));

Just point at the tables from the snd_soc_codec and remove all this
code.

> +static int ab8500_codec_pcm_hw_params(struct snd_pcm_substream *substream,
> +		struct snd_pcm_hw_params *hw_params, struct snd_soc_dai *dai)
> +{
> +	dev_dbg(dai->codec->dev, "%s Enter.\n", __func__);
> +
> +	return 0;
> +}

Remove all empty functions.
> +
> +struct snd_soc_dai_driver ab8500_codec_dai[] = {
> +	{
> +		.name = "ab8500-codec-dai.0",
> +		.id = 0,
> +		.playback = {
> +			.stream_name = "ab8500_0p",

You should hook up the links to the widgets using DAPM routes, currently
you're using the implicitly created routes from the stream name which is
still supported but isn't best practice.

> +static int ab8500_codec_remove(struct snd_soc_codec *codec)
> +{
> +	dev_dbg(codec->dev, "%s Enter.\n", __func__);
> +
> +	snd_soc_dapm_free(&codec->dapm);

Why are you doing this?

> +	/* Create register cache */
> +	ab8500_codec_driver.reg_cache_size = AB8500_LAST_REG - AB8500_FIRST_REG;
> +	ab8500_codec_driver.reg_cache_default =
> +		kzalloc(ab8500_codec_driver.reg_cache_size * sizeof(u8), GFP_KERNEL);

The driver struct should be constant, the stuff you're initialising it
with here is compile time constant - why are you doing this dynamically
at runtime/

> +
> +	/* Create driver private-data struct */
> +	drvdata = kzalloc(sizeof(struct ab8500_codec_drvdata), GFP_KERNEL);

Anything you do alloc should be devm_kzalloc().

> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
> +
> +	dev_info(&pdev->dev, "%s Enter.\n", __func__);
> +
> +	kfree(ab8500_codec_driver.reg_cache_default);
> +
> +	kfree(drvdata->virt_reg_cache);
> +	kfree(drvdata);
> +
> +	snd_soc_unregister_codec(&pdev->dev);

This would avoid issues like this where you free the data the device
uses before you free the device.

> +static int __devinit ab8500_codec_platform_driver_init(void)

module_platform_driver.

> +/* Extended interface for codec-driver */
> +
> +int ab8500_audio_power_control(struct snd_soc_codec *codec, bool power_on);
> +int ab8500_audio_set_word_length(struct snd_soc_dai *dai, unsigned int wl);
> +int ab8500_audio_set_bit_delay(struct snd_soc_dai *dai, unsigned int delay);
> +int ab8500_audio_setup_if1(struct snd_soc_codec *codec,
> +			unsigned int fmt,
> +			unsigned int wl,
> +			unsigned int delay);
> +void ab8500_audio_anc_configure(struct snd_soc_codec *codec,
> +			bool apply_fir, bool apply_iir);

No, you shouldn't be exporting this stuff - it all looks like basic
framework stuff.

> +/* Virtual register base
> + * This address should be located above any physical register used by ASoC.
> + * It is used to map virtual registers needed by the codec-driver.
> + */
> +#define VIRT_REG_BASE				0x3000

Why are you doing this virtual register stuff?

> +#define REG_DAPATHCONF_ENDACHSL			5

All these constants need to be driver local or namespaced (preferrably
namespaced whatever).
-------------- 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/20120313/86afb212/attachment.sig