Re: [RFC PATCH 1/1] ASoC: dsd1791: Introduce driver for TI DSD1791 stereo codec

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

On Wed, Dec 14, 2011 at 06:40:43PM -0500, Michael Williamson wrote:

> +static int dsd1791_write(struct snd_soc_codec *codec, unsigned int reg,
> +			 unsigned int value)
> +{
> +	struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
> +	u8 buffer[2];
> +	int rc;
> +
> +	buffer[0] = (reg & 0x7F);
> +	buffer[1] = value & 0xFF;
> +	rc = spi_write(dsd1791->spi, buffer, 2);
> +	if (rc) {
> +		dev_err(&dsd1791->spi->dev, "DSD1791 reg write error (%d)\n",
> +			rc);
> +		return -EIO;
> +	}
> +	return 0;

You shouldn't be open coding register I/O - this looks like a basic 8x8
register map so should work just fine with regmap (or the ASoC stuff but
new drivers should really use regmap).

> +	case DSD1791_DAIFMT_I2S:
> +		if (dsd1791->pcm_fmt == DSD1971_FORMAT_S16_LE)
> +			fmt = DSD1791_FMT_16I2S;
> +		else if (dsd1791->pcm_fmt == DSD1971_FORMAT_S24_LE)
> +			fmt = DSD1791_FMT_24I2S;
> +		else
> +			return -EINVAL;

This should be a switch statement.  You've got quite a few instances of
this pattern.

> +	default:
> +		dev_dbg(&dsd1791->spi->dev, "bad format\n");

codec->dev is easier.

> +static int dsd1791_set_sysclk(struct snd_soc_dai *codec_dai,
> +				int clk_id, unsigned int freq, int dir)
> +{
> +	struct snd_soc_codec *codec = codec_dai->codec;
> +	struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
> +	dsd1791->mclk = freq;
> +	return 0;
> +}

Implement this as a CODEC wide operation, it's simpler.

> +static const struct snd_kcontrol_new dsd1791_snd_controls[] = {
> +};

Remove empty variables and functions.

> +	err = snd_soc_add_controls(codec, dsd1791_snd_controls,
> +		ARRAY_SIZE(dsd1791_snd_controls));

If this were non-empty it should be registered via the CODEC structure.

> +	dev_info(&spi->dev, "probing dsd1791 spi device\n");

This is just noise, remove it.

> +	dsd1791 = kzalloc(sizeof *dsd1791, GFP_KERNEL);
> +	if (!dsd1791)
> +		return -ENOMEM;

devm_kzalloc().

> +	} else
> +		dev_info(&spi->dev, "SPI device initialized\n");

Again, too chatty.

> +static struct spi_driver dsd1791_spi = {
> +	.driver = {
> +		.name = "dsd1791-codec",

No -codec.

> +static int __init dsd1791_init(void)
> +{
> +	spi_register_driver(&dsd1791_spi);
> +
> +	return 0;

Return the error code from spi_register_driver().