[alsa-devel] [PATCH - CS4349 1/2] Add support for Cirrus Logic CS4349

[Mark Brown]

On Mon, Jul 13, 2015 at 03:08:25PM -0500, timothyc.howe at gmail.com wrote:
> From: Tim Howe <tim.howe at cirrus.com>
> 
> Signed-off-by: Tim Howe <tim.howe at cirrus.com>

This looks mostly good.  One nit on the submission format - please use
subject lines matching the style for the subsystem, "ASoC: cs4349: ..."
with just [PATCH 1/2] as the prefix in sending.  Also some fairly minor
issues below:

> +	if (pdata) {
> +		cs4349->pdata = *pdata;
> +	} else {
> +		pdata = devm_kzalloc(&client->dev,
> +				     sizeof(struct cs4349_platform_data),
> +				     GFP_KERNEL);
> +		if (!pdata) {
> +			dev_err(&client->dev,
> +				"could not allocate pdata\n");
> +			return -ENOMEM;
> +		}
> +		cs4349->pdata = *pdata;
> +	}

Why allocate the platform data if none is supplied only to copy the zero
initialized data into the driver data then never reference it again?
The entire else case can be dropped.

> +	dev_info(&client->dev,
> +		 "Cirrus Logic CS4349\n");

Remove this, it's not adding anything.

> +static int cs4349_runtime_suspend(struct device *dev)
> +{
> +	struct cs4349_private *cs4349 = dev_get_drvdata(dev);
> +	struct snd_soc_pcm_runtime *rtd = dev_get_drvdata(dev);
> +	int ret;
> +
> +	/* Hold down reset */
> +	if (cs4349->reset_gpio)
> +		gpiod_set_value_cansleep(cs4349->reset_gpio, 0);
> +
> +	ret = snd_soc_update_bits(rtd->codec, CS4349_MISC, PWR_DWN, 1);
> +	if (ret < 0)
> +		return ret;

Shouldn't the device be put into reset after powering it down rather
than before?  I'd expect there might be problems with the I2C I/O if the
device is held in reset...

> +static int cs4349_runtime_resume(struct device *dev)
> +{

> +	regcache_cache_only(cs4349->regmap, false);

I'd expect the device to be marked cache only on suspend?

> +static const struct of_device_id cs4349_of_match[] = {
> +	{ .compatible = "cirrus,cs4349", },
> +	{},
> +};

This is adding a compatible string but the binding is undocumented, all
new bindings must be documented.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150713/fbfa193d/attachment.sig>