Re: [PATCH 1/2 v2] ASoC: Add HA (HEAD acoustics) DSP codec driver template

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1708 bytes --]

On Tue, May 06, 2014 at 12:36:35PM +0200, Stefan Roese wrote:

> This codec driver template represents an I2C controlled multichannel audio

It's not really a template, it's a driver for a specific device.

Again, naming is the biggest issue here - it's going to get confusing
with a second product.  While the code is pretty clean the template
comment and the really generic name make this seem like it's more of a
placeholder for something that people will hack on to deploy than an
actual driver that someone can pick up and use.

I'm also missing any real management of the actual DSP, this appears to
only control some analogue around the DSP and a single mode switch which
is a bit odd.

> codec that has many typical ASoC codec driver features like volume controls,
> mixer stages, mux selection, output power control, in-codec audio routings,
> codec bias management and DAI link configuration.

> +static int ha_dsp_probe(struct snd_soc_codec *codec)
> +{
> +	int ret;
> +
> +	codec->control_data = dev_get_regmap(codec->dev, NULL);
> +	ret = snd_soc_codec_set_cache_io(codec, codec->control_data);
> +	if (ret != 0) {
> +		dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
> +		return ret;
> +	}

This will be done automatically by the core.

> +	snd_soc_write(codec, HA_DSP_CTRL, HA_DSP_SW_RESET);

I'd expect this to be done in the I2C level probe.

> +/*
> + * This name/ID is neded to match the DT node for the codec
> + */
> +static const struct i2c_device_id ha_dsp_i2c_id[] = {
> +	{ "ha-dsp-audio", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ha_dsp_i2c_id);

For use with DT you should have an explicit compatible string.  There's
also no binding document here that I can see.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]