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