Re: [PATCH] ALSA: ASoC: add codec driver for TI TAS5086

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

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

On Fri, Mar 08, 2013 at 12:07:13PM +0100, Daniel Mack wrote:
> This patch adds a driver for TI's TA5086 6-channel PWM processor.
> 
> This chip has a very unusual register layout, specifically because the
> registers are of unequal size, and multi-byte registers require bulk
> writes to take effect. Regmap does not support these kind of mappings.

Well, now we have the no-bus mechanism so...  doesn't matter anyway
though given that in this version the funky registers aren't used.

> Currently, the driver does not touch any of the registers >= 0x20, so
> it doesn't matter, because the register map is mapped to an 8-bit array.
> In case more features will be added in the future that require access
> to higher registers, the entire regmap H/W I/O routines have to be
> open-coded.

We should be able to interoperate with regmap for this somehow, though
it'll need us to add lock/unlock access to ensure that the core won't
interfere.

> +static bool tas5086_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	return reg == TAS5086_DEV_ID ||
> +	       reg == TAS5086_ERROR_STATUS;
> +}

switch please.

> +static int tas5086_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> +				  int clk_id, unsigned int freq, int dir)
> +{
> +	struct snd_soc_codec *codec = codec_dai->codec;
> +	struct tas5086_private *priv = snd_soc_codec_get_drvdata(codec);
> +
> +	switch (clk_id) {
> +	case 0:		/* MCLK */
> +		priv->mclk = freq;
> +		break;
> +	case 1:		/* SCLK */
> +		priv->sclk = freq;
> +		break;

Defines in a header please.

> +static int tas5086_digital_mute(struct snd_soc_dai *dai, int mute)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	struct tas5086_private *priv = snd_soc_codec_get_drvdata(codec);
> +
> +	return regmap_write(priv->regmap, TAS5086_SOFT_MUTE,
> +			    mute ? 0x3f : 0x00);

Please avoid the ternery operator.  It'd be nice to switch over to
mute_stream() too.

> +#ifdef CONFIG_PM
> +static int tas5086_soc_suspend(struct snd_soc_codec *codec)
> +{
> +	return 0;
> +}

Empty functions can just be omitted, though it might make sense to hold
the device in reset over suspend.

> +		/* Codec needs ~15ms to wake up */
> +		msleep(15);
> +	}
> +msleep(300);
> +	priv->gpio_nreset = gpio_nreset;

Indentation.

> +static int tas5086_i2c_probe(struct i2c_client *i2c,
> +			     const struct i2c_device_id *id)
> +{
> +	struct tas5086_private *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->regmap = devm_regmap_init_i2c(i2c, &tas5086_regmap);
> +	if (IS_ERR(priv->regmap)) {
> +		ret = PTR_ERR(priv->regmap);
> +		dev_err(&i2c->dev, "Failed to create regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	i2c_set_clientdata(i2c, priv);
> +
> +	return snd_soc_register_codec(&i2c->dev, &soc_codec_dev_tas5086,
> +		&tas5086_dai, 1);

I'd expect things like checking the device ID (and probably most if not
all of the basic setup and GPIO free stuff) to be pulled out to the I2C
level.

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

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