Re: [PATCH v2] soc/lapis: add machine driver

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

On Fri, Dec 02, 2011 at 06:45:15PM +0900, Tomoya MORINAGA wrote:

> +#define CODEC_DEV_ADDR 0x1A
> +static struct i2c_board_info ioh_hwmon_info[] = {
> +	{I2C_BOARD_INFO("ioh_i2c-0", CODEC_DEV_ADDR + 1)},
> +	{I2C_BOARD_INFO("ioh_i2c-1", CODEC_DEV_ADDR + 2)},
> +	{I2C_BOARD_INFO("ioh_i2c-2", CODEC_DEV_ADDR + 3)},
> +	{I2C_BOARD_INFO("ioh_i2c-3", CODEC_DEV_ADDR + 4)},
> +	{I2C_BOARD_INFO("ioh_i2c-4", CODEC_DEV_ADDR + 5)},
> +	{I2C_BOARD_INFO("ioh_i2c-5", CODEC_DEV_ADDR + 0)},

This looks completely wrong.  I'd not expect to see any I2C_BOARD_INFO
usage at all in a machine driver (that should be done by whatever
enumerates the system as a whole) and I'm not clear what the intended
effect of the above is - the names look like the names of I2C
controllers...

> +static int ml7213_ioh_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct snd_soc_codec *codec = rtd->codec;
> +	struct snd_soc_dapm_context *dapm = &codec->dapm;
> +
> +	snd_soc_dapm_new_controls(dapm, ml7213_dapm_widgets,
> +				  ARRAY_SIZE(ml7213_dapm_widgets));
> +
> +	snd_soc_dapm_add_routes(dapm, ml7213_routes, ARRAY_SIZE(ml7213_routes));

Use table based init for these.

> +static int ml7213_startup(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_codec *codec = rtd->codec;
> +	struct i2c_client *i2c;
> +	struct i2c_adapter *adapter;
> +
> +	mutex_lock(&codec->mutex);
> +
> +	adapter = i2c_get_adapter(1);
> +	if (!adapter)
> +		return -ENODEV;
> +
> +	i2c = i2c_new_device(adapter, &ioh_hwmon_info[substream->number]);
> +	if (!i2c) {
> +		mutex_unlock(&codec->mutex);
> +		return -1;
> +	}
> +	i2c_put_adapter(adapter);
> +	mutex_unlock(&codec->mutex);

No, this is definitely wrong - we certainly shouldn't be registering new
devices whenever someone starts an audio stream.  I'm struggling to
understand what the intended effect is though.

> +	switch (params_rate(params)) {
> +	case 16000:
> +	case 32000:
> +	case 48000:
> +		clk = 12288800;
> +		break;

You may as well just set the CODEC clock rate once on init() and let the
CODEC driver worry about what it can do with the clock it's got, if the
values aren't going to change there's no need to set them every time.

> +	ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
> +				SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
> +	if (ret < 0)
> +		return ret;

Use the dai_fmt field in the dai_link to set this.

> +MODULE_AUTHOR("Tomoya MORINAGA <tomoya.rohm@gmail.com>");
> +MODULE_DESCRIPTION("LAPIS Semiconductor ML7213 IOH ALSA SoC machine driver");
> +MODULE_LICENSE("GPL");

Should have MODULE_ALIAS too.