Re: [PATCH 16/16] ASoC: Ux500: Add machine-driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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

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

> +config SND_SOC_UX500_AB8500
> +	bool "Codec/Machine - AB8500 (MSA)"

Wny non-modular?

> +	depends on SND_SOC_UX500_MSP_I2S && SND_SOC_UX500_PLATFORM && AB8500_CORE && AB8500_GPADC

Word wrapping and this should select pretty much all of this with the
possible exception of the MFD core.

> +ifdef CONFIG_SND_SOC_UX500_AB8500
> +snd-soc-ux500-machine-objs += ux500_ab8500.o
> +endif

Again, why are you doing this non-idiomatic stuff?

> +/* Create dummy devices for platform drivers */

> +static struct platform_device ux500_pcm = {
> +		.name = "ux500-pcm",
> +		.id = 0,
> +		.dev = {
> +			.platform_data = NULL,
> +		},
> +};

The SoC or CPU side drivers should be taking care of stuff like this.

> +/* Define the whole U8500 soundcard, linking platform to the codec-drivers  */
> +struct snd_soc_dai_link u8500_dai_links[] = {
> +	#ifdef CONFIG_SND_SOC_UX500_AB8500

What's this ifdef doing...?  It looks very, very suspicous.

> +	{
> +	.name = "ab8500_0",

Your indentation is also fairly broken here.

> +	pdev = platform_device_alloc("soc-audio", -1);
> +	if (!pdev)
> +		return -ENOMEM;

No, probe using the normal device model and register using
snd_soc_register_card().

> +++ b/sound/soc/ux500/ux500_ab8500.c

The fact that you've got multiple files for a machine driver is
worrying...

> +/* ANC States */
> +static const char * const enum_anc_state[] = {
> +	"Unconfigured",
> +	"Apply FIR and IIR",
> +	"FIR and IIR are configured",
> +	"Apply FIR",
> +	"FIR is configured",
> +	"Apply IIR",
> +	"IIR is configured"
> +};

Why is this not part of the CODEC driver?  What makes this machine
specific?

> +/* Regulators */
> +enum regulator_idx {
> +	REGULATOR_AUDIO,
> +	REGULATOR_DMIC,
> +	REGULATOR_AMIC1,
> +	REGULATOR_AMIC2
> +};
> +static struct regulator_bulk_data reg_info[4] = {
> +	{	.consumer = NULL, .supply = "V-AUD"	},
> +	{	.consumer = NULL, .supply = "V-DMIC"	},
> +	{	.consumer = NULL, .supply = "V-AMIC1"	},
> +	{	.consumer = NULL, .supply = "V-AMIC2"	}
> +};
> +static bool reg_enabled[4] =  {
> +	false,
> +	false,
> +	false,
> +	false
> +};
> +static int reg_claim[4];
> +enum amic_idx { AMIC_1A, AMIC_1B, AMIC_2 };
> +struct amic_conf {
> +	enum regulator_idx reg_id;
> +	bool enabled;
> +	char *name;

I have no idea what this code is intended to do but it looks *extremely*
confused, it's especially surprising given that your CODEC makes no use
of regulators even for basic power.

> +static int enable_regulator(struct device *dev, enum regulator_idx idx)
> +{

You appear to be defining some sort of subsystem on top of the regulator
API here but I'm not sure what it's intended to do or why you're doing
it...

> +static const struct snd_soc_dapm_widget ux500_ab8500_dapm_widgets[] = {
> +	SND_SOC_DAPM_SUPPLY("AUDIO Regulator",
> +			SND_SOC_NOPM, 0, 0, dapm_audioreg_event,
> +			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),

DAPM has native support for regulator supplies.

> +	/* Enable gpio.1-clock (needed by DSP in burst mode) */
> +	ret = clk_enable(clk_ptr_gpio1);
> +	if (ret) {
> +		dev_err(dev, "%s: ERROR: clk_enable(gpio.1) failed (ret = %d)!",
> +			__func__, ret);
> +		return ret;
> +	}

Why can't the DSP figure out that it needs to enable the clock?

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

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