[PATCH 00/10] Tidy up device ID reading on legacy Cirrus parts

[PATCH 00/10] Tidy up device ID reading on legacy Cirrus parts

[Charles Keepax]

Pierre requested I have a look at some cppcheck warnings in the cs42l42
driver, since it is reassigning the ret variable without ever checking
the result.  Looking a bit more broadly this happens in quite a few
legacy Cirrus parts, as they all use the same process to read the ID,
factor out a small helper so they can all share the same code. Whilst
in there fix up a couple of other trivial error path issues as well.

Thanks,
Charles

Charles Keepax (10):
  ASoC: cirrus: Add helper function for reading the device ID
  ASoC: cs35l32: Minor error paths fixups
  ASoC: cs35l33: Minor error paths fixups
  ASoC: cs35l34:  Minor error paths fixups
  ASoC: cs35l35:  Minor error paths fixups
  ASoC: cs35l35: Correct errata handling
  ASoC: cs42l42:  Minor error paths fixups
  ASoC: cs42l73:  Minor error paths fixups
  ASoC: cs43130:  Minor error paths fixups
  ASoC: cs53l30:  Minor error paths fixups

 sound/soc/codecs/cirrus_legacy.h | 21 +++++++++++++++++++++
 sound/soc/codecs/cs35l32.c       | 34 ++++++++++++++++++----------------
 sound/soc/codecs/cs35l33.c       | 15 +++++++++------
 sound/soc/codecs/cs35l34.c       | 39 ++++++++++++++++++++++-----------------
 sound/soc/codecs/cs35l35.c       | 21 ++++++++++-----------
 sound/soc/codecs/cs35l35.h       |  1 +
 sound/soc/codecs/cs42l42.c       | 18 ++++++++----------
 sound/soc/codecs/cs42l73.c       | 30 +++++++++++++++++-------------
 sound/soc/codecs/cs43130.c       | 31 +++++++++++++++++++------------
 sound/soc/codecs/cs53l30.c       | 22 +++++++++++-----------
 10 files changed, 136 insertions(+), 96 deletions(-)
 create mode 100644 sound/soc/codecs/cirrus_legacy.h

-- 
2.11.0

[PATCH 01/10] ASoC: cirrus: Add helper function for reading the device ID

[Charles Keepax]

Many of the older Cirrus devices share very similar code for reading the
device ID, and frequently this code is generating cppcheck warnings such
as:

sound/soc/codecs/cs42l42.c:1886:6: style: Variable 'ret' is reassigned
a value before the old one has been used. [redundantAssignment]
 ret = regmap_read(cs42l42->regmap, CS42L42_DEVID_CD, &reg);

Add a small helper function that older Cirrus devices can use to read
the device ID, which should help correct these issues.

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cirrus_legacy.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 sound/soc/codecs/cirrus_legacy.h

diff --git a/sound/soc/codecs/cirrus_legacy.h b/sound/soc/codecs/cirrus_legacy.h
new file mode 100644
index 0000000000000..87c6fd79290d4
--- /dev/null
+++ b/sound/soc/codecs/cirrus_legacy.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Some small helpers for older Cirrus Logic parts.
+ *
+ * Copyright (C) 2021 Cirrus Logic, Inc. and
+ *                    Cirrus Logic International Semiconductor Ltd.
+ */
+
+static inline int cirrus_read_device_id(struct regmap *regmap, unsigned int reg)
+{
+	u8 devid[3];
+	int ret;
+
+	ret = regmap_bulk_read(regmap, reg, devid, ARRAY_SIZE(devid));
+	if (ret < 0)
+		return ret;
+
+	return ((devid[0] & 0xFF) << 12) |
+	       ((devid[1] & 0xFF) <<  4) |
+	       ((devid[2] & 0xF0) >>  4);
+}
-- 
2.11.0

[PATCH 02/10] ASoC: cs35l32: Minor error paths fixups

[Charles Keepax]

Correct some unchecked re-allocations of ret whilst reading the device
ID and ensure the hardware state is returned to off on the error
paths.

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l32.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/sound/soc/codecs/cs35l32.c b/sound/soc/codecs/cs35l32.c
index f4067230ac425..7e1047362a901 100644
--- a/sound/soc/codecs/cs35l32.c
+++ b/sound/soc/codecs/cs35l32.c
@@ -30,6 +30,7 @@
 #include <dt-bindings/sound/cs35l32.h>
 
 #include "cs35l32.h"
+#include "cirrus_legacy.h"
 
 #define CS35L32_NUM_SUPPLIES 2
 static const char *const cs35l32_supply_names[CS35L32_NUM_SUPPLIES] = {
@@ -348,8 +349,7 @@ static int cs35l32_i2c_probe(struct i2c_client *i2c_client,
 	struct cs35l32_private *cs35l32;
 	struct cs35l32_platform_data *pdata =
 		dev_get_platdata(&i2c_client->dev);
-	int ret, i;
-	unsigned int devid = 0;
+	int ret, i, devid;
 	unsigned int reg;
 
 	cs35l32 = devm_kzalloc(&i2c_client->dev, sizeof(*cs35l32), GFP_KERNEL);
@@ -404,40 +404,40 @@ static int cs35l32_i2c_probe(struct i2c_client *i2c_client,
 	/* Reset the Device */
 	cs35l32->reset_gpio = devm_gpiod_get_optional(&i2c_client->dev,
 		"reset", GPIOD_OUT_LOW);
-	if (IS_ERR(cs35l32->reset_gpio))
-		return PTR_ERR(cs35l32->reset_gpio);
+	if (IS_ERR(cs35l32->reset_gpio)) {
+		ret = PTR_ERR(cs35l32->reset_gpio);
+		goto err_supplies;
+	}
 
 	gpiod_set_value_cansleep(cs35l32->reset_gpio, 1);
 
 	/* initialize codec */
-	ret = regmap_read(cs35l32->regmap, CS35L32_DEVID_AB, &reg);
-	devid = (reg & 0xFF) << 12;
-
-	ret = regmap_read(cs35l32->regmap, CS35L32_DEVID_CD, &reg);
-	devid |= (reg & 0xFF) << 4;
-
-	ret = regmap_read(cs35l32->regmap, CS35L32_DEVID_E, &reg);
-	devid |= (reg & 0xF0) >> 4;
+	devid = cirrus_read_device_id(cs35l32->regmap, CS35L32_DEVID_AB);
+	if (devid < 0) {
+		ret = devid;
+		dev_err(&i2c_client->dev, "Failed to read device ID: %d\n", ret);
+		goto err_disable;
+	}
 
 	if (devid != CS35L32_CHIP_ID) {
 		ret = -ENODEV;
 		dev_err(&i2c_client->dev,
 			"CS35L32 Device ID (%X). Expected %X\n",
 			devid, CS35L32_CHIP_ID);
-		return ret;
+		goto err_disable;
 	}
 
 	ret = regmap_read(cs35l32->regmap, CS35L32_REV_ID, &reg);
 	if (ret < 0) {
 		dev_err(&i2c_client->dev, "Get Revision ID failed\n");
-		return ret;
+		goto err_disable;
 	}
 
 	ret = regmap_register_patch(cs35l32->regmap, cs35l32_monitor_patch,
 				    ARRAY_SIZE(cs35l32_monitor_patch));
 	if (ret < 0) {
 		dev_err(&i2c_client->dev, "Failed to apply errata patch\n");
-		return ret;
+		goto err_disable;
 	}
 
 	dev_info(&i2c_client->dev,
@@ -478,7 +478,7 @@ static int cs35l32_i2c_probe(struct i2c_client *i2c_client,
 			    CS35L32_PDN_AMP);
 
 	/* Clear MCLK Error Bit since we don't have the clock yet */
-	ret = regmap_read(cs35l32->regmap, CS35L32_INT_STATUS_1, &reg);
+	regmap_read(cs35l32->regmap, CS35L32_INT_STATUS_1, &reg);
 
 	ret = devm_snd_soc_register_component(&i2c_client->dev,
 			&soc_component_dev_cs35l32, cs35l32_dai,
@@ -489,6 +489,8 @@ static int cs35l32_i2c_probe(struct i2c_client *i2c_client,
 	return 0;
 
 err_disable:
+	gpiod_set_value_cansleep(cs35l32->reset_gpio, 0);
+err_supplies:
 	regulator_bulk_disable(ARRAY_SIZE(cs35l32->supplies),
 			       cs35l32->supplies);
 	return ret;
-- 
2.11.0

[PATCH 03/10] ASoC: cs35l33: Minor error paths fixups

[Charles Keepax]

Correct some unchecked re-allocations of ret whilst reading the device
ID and ensure the hardware state is returned to off on the error
paths.

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l33.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/cs35l33.c b/sound/soc/codecs/cs35l33.c
index 7ad7b733af9b6..6f6b3c0c88b78 100644
--- a/sound/soc/codecs/cs35l33.c
+++ b/sound/soc/codecs/cs35l33.c
@@ -34,6 +34,7 @@
 #include <linux/of_irq.h>
 
 #include "cs35l33.h"
+#include "cirrus_legacy.h"
 
 #define CS35L33_BOOT_DELAY	50
 
@@ -1190,12 +1191,12 @@ static int cs35l33_i2c_probe(struct i2c_client *i2c_client,
 	regcache_cache_only(cs35l33->regmap, false);
 
 	/* initialize codec */
-	ret = regmap_read(cs35l33->regmap, CS35L33_DEVID_AB, &reg);
-	devid = (reg & 0xFF) << 12;
-	ret = regmap_read(cs35l33->regmap, CS35L33_DEVID_CD, &reg);
-	devid |= (reg & 0xFF) << 4;
-	ret = regmap_read(cs35l33->regmap, CS35L33_DEVID_E, &reg);
-	devid |= (reg & 0xF0) >> 4;
+	devid = cirrus_read_device_id(cs35l33->regmap, CS35L33_DEVID_AB);
+	if (devid < 0) {
+		ret = devid;
+		dev_err(&i2c_client->dev, "Failed to read device ID: %d\n", ret);
+		goto err_enable;
+	}
 
 	if (devid != CS35L33_CHIP_ID) {
 		dev_err(&i2c_client->dev,
@@ -1242,6 +1243,8 @@ static int cs35l33_i2c_probe(struct i2c_client *i2c_client,
 	return 0;
 
 err_enable:
+	gpiod_set_value_cansleep(cs35l33->reset_gpio, 0);
+
 	regulator_bulk_disable(cs35l33->num_core_supplies,
 			       cs35l33->core_supplies);
 
-- 
2.11.0

[PATCH 04/10] ASoC: cs35l34: Minor error paths fixups

[Charles Keepax]

Correct some unchecked re-allocations of ret whilst reading the device
ID and ensure the hardware state is returned to off on the error
paths.

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l34.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/sound/soc/codecs/cs35l34.c b/sound/soc/codecs/cs35l34.c
index 110ee2d063581..6657cc5db3e87 100644
--- a/sound/soc/codecs/cs35l34.c
+++ b/sound/soc/codecs/cs35l34.c
@@ -34,6 +34,7 @@
 #include <sound/cs35l34.h>
 
 #include "cs35l34.h"
+#include "cirrus_legacy.h"
 
 #define PDN_DONE_ATTEMPTS 10
 #define CS35L34_START_DELAY 50
@@ -996,9 +997,8 @@ static int cs35l34_i2c_probe(struct i2c_client *i2c_client,
 	struct cs35l34_private *cs35l34;
 	struct cs35l34_platform_data *pdata =
 		dev_get_platdata(&i2c_client->dev);
-	int i;
+	int i, devid;
 	int ret;
-	unsigned int devid = 0;
 	unsigned int reg;
 
 	cs35l34 = devm_kzalloc(&i2c_client->dev, sizeof(*cs35l34), GFP_KERNEL);
@@ -1039,13 +1039,15 @@ static int cs35l34_i2c_probe(struct i2c_client *i2c_client,
 	} else {
 		pdata = devm_kzalloc(&i2c_client->dev, sizeof(*pdata),
 				     GFP_KERNEL);
-		if (!pdata)
-			return -ENOMEM;
+		if (!pdata) {
+			ret = -ENOMEM;
+			goto err_regulator;
+		}
 
 		if (i2c_client->dev.of_node) {
 			ret = cs35l34_handle_of_data(i2c_client, pdata);
 			if (ret != 0)
-				return ret;
+				goto err_regulator;
 
 		}
 		cs35l34->pdata = *pdata;
@@ -1059,33 +1061,34 @@ static int cs35l34_i2c_probe(struct i2c_client *i2c_client,
 
 	cs35l34->reset_gpio = devm_gpiod_get_optional(&i2c_client->dev,
 				"reset-gpios", GPIOD_OUT_LOW);
-	if (IS_ERR(cs35l34->reset_gpio))
-		return PTR_ERR(cs35l34->reset_gpio);
+	if (IS_ERR(cs35l34->reset_gpio)) {
+		ret = PTR_ERR(cs35l34->reset_gpio);
+		goto err_regulator;
+	}
 
 	gpiod_set_value_cansleep(cs35l34->reset_gpio, 1);
 
 	msleep(CS35L34_START_DELAY);
 
-	ret = regmap_read(cs35l34->regmap, CS35L34_DEVID_AB, &reg);
-
-	devid = (reg & 0xFF) << 12;
-	ret = regmap_read(cs35l34->regmap, CS35L34_DEVID_CD, &reg);
-	devid |= (reg & 0xFF) << 4;
-	ret = regmap_read(cs35l34->regmap, CS35L34_DEVID_E, &reg);
-	devid |= (reg & 0xF0) >> 4;
+	devid = cirrus_read_device_id(cs35l34->regmap, CS35L34_DEVID_AB);
+	if (devid < 0) {
+		ret = devid;
+		dev_err(&i2c_client->dev, "Failed to read device ID: %d\n", ret);
+		goto err_reset;
+	}
 
 	if (devid != CS35L34_CHIP_ID) {
 		dev_err(&i2c_client->dev,
 			"CS35l34 Device ID (%X). Expected ID %X\n",
 			devid, CS35L34_CHIP_ID);
 		ret = -ENODEV;
-		goto err_regulator;
+		goto err_reset;
 	}
 
 	ret = regmap_read(cs35l34->regmap, CS35L34_REV_ID, &reg);
 	if (ret < 0) {
 		dev_err(&i2c_client->dev, "Get Revision ID failed\n");
-		goto err_regulator;
+		goto err_reset;
 	}
 
 	dev_info(&i2c_client->dev,
@@ -1110,11 +1113,13 @@ static int cs35l34_i2c_probe(struct i2c_client *i2c_client,
 	if (ret < 0) {
 		dev_err(&i2c_client->dev,
 			"%s: Register component failed\n", __func__);
-		goto err_regulator;
+		goto err_reset;
 	}
 
 	return 0;
 
+err_reset:
+	gpiod_set_value_cansleep(cs35l34->reset_gpio, 0);
 err_regulator:
 	regulator_bulk_disable(cs35l34->num_core_supplies,
 		cs35l34->core_supplies);
-- 
2.11.0

[PATCH 05/10] ASoC: cs35l35: Minor error paths fixups

[Charles Keepax]

Correct some unchecked re-allocations of ret whilst reading the device
ID and ensure the hardware state is returned to off on the error
paths.

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l35.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/cs35l35.c b/sound/soc/codecs/cs35l35.c
index f20ed838b9580..554b32f388d9a 100644
--- a/sound/soc/codecs/cs35l35.c
+++ b/sound/soc/codecs/cs35l35.c
@@ -33,6 +33,7 @@
 #include <linux/completion.h>
 
 #include "cs35l35.h"
+#include "cirrus_legacy.h"
 
 /*
  * Some fields take zero as a valid value so use a high bit flag that won't
@@ -1471,9 +1472,8 @@ static int cs35l35_i2c_probe(struct i2c_client *i2c_client,
 	struct cs35l35_private *cs35l35;
 	struct device *dev = &i2c_client->dev;
 	struct cs35l35_platform_data *pdata = dev_get_platdata(dev);
-	int i;
+	int i, devid;
 	int ret;
-	unsigned int devid = 0;
 	unsigned int reg;
 
 	cs35l35 = devm_kzalloc(dev, sizeof(struct cs35l35_private), GFP_KERNEL);
@@ -1552,13 +1552,12 @@ static int cs35l35_i2c_probe(struct i2c_client *i2c_client,
 		goto err;
 	}
 	/* initialize codec */
-	ret = regmap_read(cs35l35->regmap, CS35L35_DEVID_AB, &reg);
-
-	devid = (reg & 0xFF) << 12;
-	ret = regmap_read(cs35l35->regmap, CS35L35_DEVID_CD, &reg);
-	devid |= (reg & 0xFF) << 4;
-	ret = regmap_read(cs35l35->regmap, CS35L35_DEVID_E, &reg);
-	devid |= (reg & 0xF0) >> 4;
+	devid = cirrus_read_device_id(cs35l35->regmap, CS35L35_DEVID_AB);
+	if (devid < 0) {
+		ret = devid;
+		dev_err(dev, "Failed to read device ID: %d\n", ret);
+		goto err;
+	}
 
 	if (devid != CS35L35_CHIP_ID) {
 		dev_err(dev, "CS35L35 Device ID (%X). Expected ID %X\n",
-- 
2.11.0

[PATCH 06/10] ASoC: cs35l35: Correct errata handling

[Charles Keepax]

Currently the check of errata_chk will always evaluate to false since
the values tested don't come under the mask used. A shift of the field
is missing, add this. Also there is an error in the values tested, they
don't match the comment and the value 0x3 is not a valid value for the
field in question. Update the value to match the comment.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l35.c | 4 ++--
 sound/soc/codecs/cs35l35.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/cs35l35.c b/sound/soc/codecs/cs35l35.c
index 554b32f388d9a..a4309312e84f3 100644
--- a/sound/soc/codecs/cs35l35.c
+++ b/sound/soc/codecs/cs35l35.c
@@ -496,10 +496,10 @@ static int cs35l35_hw_params(struct snd_pcm_substream *substream,
 	 * the Class H algorithm does not enable weak-drive operation for
 	 * nonzero values of CH_WKFET_DELAY if SP_RATE = 01 or 10
 	 */
-	errata_chk = clk_ctl & CS35L35_SP_RATE_MASK;
+	errata_chk = (clk_ctl & CS35L35_SP_RATE_MASK) >> CS35L35_SP_RATE_SHIFT;
 
 	if (classh->classh_wk_fet_disable == 0x00 &&
-		(errata_chk == 0x01 || errata_chk == 0x03)) {
+		(errata_chk == 0x01 || errata_chk == 0x02)) {
 		ret = regmap_update_bits(cs35l35->regmap,
 					CS35L35_CLASS_H_FET_DRIVE_CTL,
 					CS35L35_CH_WKFET_DEL_MASK,
diff --git a/sound/soc/codecs/cs35l35.h b/sound/soc/codecs/cs35l35.h
index ffb154cd962c2..2117dcb08c467 100644
--- a/sound/soc/codecs/cs35l35.h
+++ b/sound/soc/codecs/cs35l35.h
@@ -168,6 +168,7 @@
 #define CS35L35_SP_SCLKS_48FS		0x0B
 #define CS35L35_SP_SCLKS_64FS		0x0F
 #define CS35L35_SP_RATE_MASK		0xC0
+#define CS35L35_SP_RATE_SHIFT		6
 
 #define CS35L35_PDN_BST_MASK		0x06
 #define CS35L35_PDN_BST_FETON_SHIFT	1
-- 
2.11.0

[PATCH 07/10] ASoC: cs42l42: Minor error paths fixups

[Charles Keepax]

Correct some unchecked re-allocations of ret whilst reading the device
ID and ensure the hardware state is returned to off on the error
paths.

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs42l42.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index bf982e145e945..594d771336719 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -36,6 +36,7 @@
 #include <dt-bindings/sound/cs42l42.h>
 
 #include "cs42l42.h"
+#include "cirrus_legacy.h"
 
 static const struct reg_default cs42l42_reg_defaults[] = {
 	{ CS42L42_FRZ_CTL,			0x00 },
@@ -1816,8 +1817,7 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 				       const struct i2c_device_id *id)
 {
 	struct cs42l42_private *cs42l42;
-	int ret, i;
-	unsigned int devid = 0;
+	int ret, i, devid;
 	unsigned int reg;
 
 	cs42l42 = devm_kzalloc(&i2c_client->dev, sizeof(struct cs42l42_private),
@@ -1880,14 +1880,12 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 			"Failed to request IRQ: %d\n", ret);
 
 	/* initialize codec */
-	ret = regmap_read(cs42l42->regmap, CS42L42_DEVID_AB, &reg);
-	devid = (reg & 0xFF) << 12;
-
-	ret = regmap_read(cs42l42->regmap, CS42L42_DEVID_CD, &reg);
-	devid |= (reg & 0xFF) << 4;
-
-	ret = regmap_read(cs42l42->regmap, CS42L42_DEVID_E, &reg);
-	devid |= (reg & 0xF0) >> 4;
+	devid = cirrus_read_device_id(cs42l42->regmap, CS42L42_DEVID_AB);
+	if (devid < 0) {
+		ret = devid;
+		dev_err(&i2c_client->dev, "Failed to read device ID: %d\n", ret);
+		goto err_disable;
+	}
 
 	if (devid != CS42L42_CHIP_ID) {
 		ret = -ENODEV;
-- 
2.11.0

[PATCH 08/10] ASoC: cs42l73: Minor error paths fixups

[Charles Keepax]

Correct some unchecked re-allocations of ret whilst reading the device
ID and ensure the hardware state is returned to off on the error
paths.

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs42l73.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/sound/soc/codecs/cs42l73.c b/sound/soc/codecs/cs42l73.c
index c3f974ec78e58..da5d77a5f55bb 100644
--- a/sound/soc/codecs/cs42l73.c
+++ b/sound/soc/codecs/cs42l73.c
@@ -27,6 +27,7 @@
 #include <sound/tlv.h>
 #include <sound/cs42l73.h>
 #include "cs42l73.h"
+#include "cirrus_legacy.h"
 
 struct sp_config {
 	u8 spc, mmcc, spfs;
@@ -1275,8 +1276,7 @@ static int cs42l73_i2c_probe(struct i2c_client *i2c_client,
 {
 	struct cs42l73_private *cs42l73;
 	struct cs42l73_platform_data *pdata = dev_get_platdata(&i2c_client->dev);
-	int ret;
-	unsigned int devid = 0;
+	int ret, devid;
 	unsigned int reg;
 	u32 val32;
 
@@ -1326,27 +1326,25 @@ static int cs42l73_i2c_probe(struct i2c_client *i2c_client,
 	}
 
 	/* initialize codec */
-	ret = regmap_read(cs42l73->regmap, CS42L73_DEVID_AB, &reg);
-	devid = (reg & 0xFF) << 12;
-
-	ret = regmap_read(cs42l73->regmap, CS42L73_DEVID_CD, &reg);
-	devid |= (reg & 0xFF) << 4;
-
-	ret = regmap_read(cs42l73->regmap, CS42L73_DEVID_E, &reg);
-	devid |= (reg & 0xF0) >> 4;
+	devid = cirrus_read_device_id(cs42l73->regmap, CS42L73_DEVID_AB);
+	if (devid < 0) {
+		ret = devid;
+		dev_err(&i2c_client->dev, "Failed to read device ID: %d\n", ret);
+		goto err_reset;
+	}
 
 	if (devid != CS42L73_DEVID) {
 		ret = -ENODEV;
 		dev_err(&i2c_client->dev,
 			"CS42L73 Device ID (%X). Expected %X\n",
 			devid, CS42L73_DEVID);
-		return ret;
+		goto err_reset;
 	}
 
 	ret = regmap_read(cs42l73->regmap, CS42L73_REVID, &reg);
 	if (ret < 0) {
 		dev_err(&i2c_client->dev, "Get Revision ID failed\n");
-		return ret;
+		goto err_reset;
 	}
 
 	dev_info(&i2c_client->dev,
@@ -1356,8 +1354,14 @@ static int cs42l73_i2c_probe(struct i2c_client *i2c_client,
 			&soc_component_dev_cs42l73, cs42l73_dai,
 			ARRAY_SIZE(cs42l73_dai));
 	if (ret < 0)
-		return ret;
+		goto err_reset;
+
 	return 0;
+
+err_reset:
+	gpio_set_value_cansleep(cs42l73->pdata.reset_gpio, 0);
+
+	return ret;
 }
 
 static const struct of_device_id cs42l73_of_match[] = {
-- 
2.11.0

[PATCH 09/10] ASoC: cs43130: Minor error paths fixups

[Charles Keepax]

Correct some unchecked re-allocations of ret whilst reading the device
ID and ensure the hardware state is returned to off on the error
paths.

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs43130.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c
index 80bc7c10ed757..642338cdc8b65 100644
--- a/sound/soc/codecs/cs43130.c
+++ b/sound/soc/codecs/cs43130.c
@@ -36,6 +36,7 @@
 #include <sound/jack.h>
 
 #include "cs43130.h"
+#include "cirrus_legacy.h"
 
 static const struct reg_default cs43130_reg_defaults[] = {
 	{CS43130_SYS_CLK_CTL_1, 0x06},
@@ -2424,9 +2425,8 @@ static int cs43130_i2c_probe(struct i2c_client *client,
 {
 	struct cs43130_private *cs43130;
 	int ret;
-	unsigned int devid = 0;
 	unsigned int reg;
-	int i;
+	int i, devid;
 
 	cs43130 = devm_kzalloc(&client->dev, sizeof(*cs43130), GFP_KERNEL);
 	if (!cs43130)
@@ -2464,20 +2464,21 @@ static int cs43130_i2c_probe(struct i2c_client *client,
 
 	cs43130->reset_gpio = devm_gpiod_get_optional(&client->dev,
 						      "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(cs43130->reset_gpio))
-		return PTR_ERR(cs43130->reset_gpio);
+	if (IS_ERR(cs43130->reset_gpio)) {
+		ret = PTR_ERR(cs43130->reset_gpio);
+		goto err_supplies;
+	}
 
 	gpiod_set_value_cansleep(cs43130->reset_gpio, 1);
 
 	usleep_range(2000, 2050);
 
-	ret = regmap_read(cs43130->regmap, CS43130_DEVID_AB, &reg);
-
-	devid = (reg & 0xFF) << 12;
-	ret = regmap_read(cs43130->regmap, CS43130_DEVID_CD, &reg);
-	devid |= (reg & 0xFF) << 4;
-	ret = regmap_read(cs43130->regmap, CS43130_DEVID_E, &reg);
-	devid |= (reg & 0xF0) >> 4;
+	devid = cirrus_read_device_id(cs43130->regmap, CS43130_DEVID_AB);
+	if (devid < 0) {
+		ret = devid;
+		dev_err(&client->dev, "Failed to read device ID: %d\n", ret);
+		goto err;
+	}
 
 	switch (devid) {
 	case CS43130_CHIP_ID:
@@ -2517,7 +2518,7 @@ static int cs43130_i2c_probe(struct i2c_client *client,
 					"cs43130", cs43130);
 	if (ret != 0) {
 		dev_err(&client->dev, "Failed to request IRQ: %d\n", ret);
-		return ret;
+		goto err;
 	}
 
 	cs43130->mclk_int_src = CS43130_MCLK_SRC_RCO;
@@ -2576,7 +2577,13 @@ static int cs43130_i2c_probe(struct i2c_client *client,
 			   CS43130_XSP_3ST_MASK, 0);
 
 	return 0;
+
 err:
+	gpiod_set_value_cansleep(cs43130->reset_gpio, 0);
+err_supplies:
+	regulator_bulk_disable(ARRAY_SIZE(cs43130->supplies),
+			       cs43130->supplies);
+
 	return ret;
 }
 
-- 
2.11.0

[PATCH 10/10] ASoC: cs53l30: Minor error paths fixups

[Charles Keepax]

Correct some unchecked re-allocations of ret whilst reading the device
ID and ensure the hardware state is returned to off on the error
paths.

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs53l30.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/sound/soc/codecs/cs53l30.c b/sound/soc/codecs/cs53l30.c
index 3d67cbf9eaaa2..bd33dd048c7ce 100644
--- a/sound/soc/codecs/cs53l30.c
+++ b/sound/soc/codecs/cs53l30.c
@@ -20,6 +20,7 @@
 #include <sound/tlv.h>
 
 #include "cs53l30.h"
+#include "cirrus_legacy.h"
 
 #define CS53L30_NUM_SUPPLIES 2
 static const char *const cs53l30_supply_names[CS53L30_NUM_SUPPLIES] = {
@@ -920,9 +921,8 @@ static int cs53l30_i2c_probe(struct i2c_client *client,
 	const struct device_node *np = client->dev.of_node;
 	struct device *dev = &client->dev;
 	struct cs53l30_private *cs53l30;
-	unsigned int devid = 0;
 	unsigned int reg;
-	int ret = 0, i;
+	int ret = 0, i, devid;
 	u8 val;
 
 	cs53l30 = devm_kzalloc(dev, sizeof(*cs53l30), GFP_KERNEL);
@@ -951,7 +951,7 @@ static int cs53l30_i2c_probe(struct i2c_client *client,
 						      GPIOD_OUT_LOW);
 	if (IS_ERR(cs53l30->reset_gpio)) {
 		ret = PTR_ERR(cs53l30->reset_gpio);
-		goto error;
+		goto error_supplies;
 	}
 
 	gpiod_set_value_cansleep(cs53l30->reset_gpio, 1);
@@ -968,14 +968,12 @@ static int cs53l30_i2c_probe(struct i2c_client *client,
 	}
 
 	/* Initialize codec */
-	ret = regmap_read(cs53l30->regmap, CS53L30_DEVID_AB, &reg);
-	devid = reg << 12;
-
-	ret = regmap_read(cs53l30->regmap, CS53L30_DEVID_CD, &reg);
-	devid |= reg << 4;
-
-	ret = regmap_read(cs53l30->regmap, CS53L30_DEVID_E, &reg);
-	devid |= (reg & 0xF0) >> 4;
+	devid = cirrus_read_device_id(cs53l30->regmap, CS53L30_DEVID_AB);
+	if (devid < 0) {
+		ret = devid;
+		dev_err(dev, "Failed to read device ID: %d\n", ret);
+		goto error;
+	}
 
 	if (devid != CS53L30_DEVID) {
 		ret = -ENODEV;
@@ -1037,6 +1035,8 @@ static int cs53l30_i2c_probe(struct i2c_client *client,
 	return 0;
 
 error:
+	gpiod_set_value_cansleep(cs53l30->reset_gpio, 0);
+error_supplies:
 	regulator_bulk_disable(ARRAY_SIZE(cs53l30->supplies),
 			       cs53l30->supplies);
 	return ret;
-- 
2.11.0

Re: [PATCH 00/10] Tidy up device ID reading on legacy Cirrus parts

[Pierre-Louis Bossart]

On 5/10/21 8:13 AM, Charles Keepax wrote:
> Pierre requested I have a look at some cppcheck warnings in the cs42l42
> driver, since it is reassigning the ret variable without ever checking
> the result.  Looking a bit more broadly this happens in quite a few
> legacy Cirrus parts, as they all use the same process to read the ID,
> factor out a small helper so they can all share the same code. Whilst
> in there fix up a couple of other trivial error path issues as well.
> 
> Thanks,
> Charles
> 
> Charles Keepax (10):
>    ASoC: cirrus: Add helper function for reading the device ID
>    ASoC: cs35l32: Minor error paths fixups
>    ASoC: cs35l33: Minor error paths fixups
>    ASoC: cs35l34:  Minor error paths fixups
>    ASoC: cs35l35:  Minor error paths fixups
>    ASoC: cs35l35: Correct errata handling
>    ASoC: cs42l42:  Minor error paths fixups
>    ASoC: cs42l73:  Minor error paths fixups
>    ASoC: cs43130:  Minor error paths fixups
>    ASoC: cs53l30:  Minor error paths fixups

Sounds all good to me, just wondering if while you're at it you can fix 
the remaining minor issues? Thanks!

cppcheck --platform=unix64 --force --max-configs=1024 --inconclusive 
--enable=all --suppress=variableScope --suppress=shiftTooManyBitsSigned 
--suppress=arithOperationsOnVoidPointer --suppress=bitwiseOnBoolean 
sound/soc/codecs/cs*.c

sound/soc/codecs/cs35l36.c:1159:10: style: Variable 'ret' is assigned a 
value that is never used. [unreadVariable]
  int ret = 0;
          ^
sound/soc/codecs/cs4265.c:619:6: style: Variable 'ret' is reassigned a 
value before the old one has been used. [redundantAssignment]
  ret = devm_snd_soc_register_component(&i2c_client->dev,
      ^
sound/soc/codecs/cs4265.c:604:6: note: ret is assigned
  ret = regmap_read(cs4265->regmap, CS4265_CHIP_ID, &reg);
      ^
sound/soc/codecs/cs4265.c:619:6: note: ret is overwritten
  ret = devm_snd_soc_register_component(&i2c_client->dev,
      ^
sound/soc/codecs/cs42l52.c:1202:6: style: Variable 'ret' is reassigned a 
value before the old one has been used. [redundantAssignment]
  ret =  devm_snd_soc_register_component(&i2c_client->dev,
      ^
sound/soc/codecs/cs42l52.c:1165:6: note: ret is assigned
  ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, &reg);
      ^
sound/soc/codecs/cs42l52.c:1202:6: note: ret is overwritten
  ret =  devm_snd_soc_register_component(&i2c_client->dev,
      ^


> 
>   sound/soc/codecs/cirrus_legacy.h | 21 +++++++++++++++++++++
>   sound/soc/codecs/cs35l32.c       | 34 ++++++++++++++++++----------------
>   sound/soc/codecs/cs35l33.c       | 15 +++++++++------
>   sound/soc/codecs/cs35l34.c       | 39 ++++++++++++++++++++++-----------------
>   sound/soc/codecs/cs35l35.c       | 21 ++++++++++-----------
>   sound/soc/codecs/cs35l35.h       |  1 +
>   sound/soc/codecs/cs42l42.c       | 18 ++++++++----------
>   sound/soc/codecs/cs42l73.c       | 30 +++++++++++++++++-------------
>   sound/soc/codecs/cs43130.c       | 31 +++++++++++++++++++------------
>   sound/soc/codecs/cs53l30.c       | 22 +++++++++++-----------
>   10 files changed, 136 insertions(+), 96 deletions(-)
>   create mode 100644 sound/soc/codecs/cirrus_legacy.h
> 

Re: [PATCH 00/10] Tidy up device ID reading on legacy Cirrus parts

[Mark Brown]

On Mon, 10 May 2021 14:13:47 +0100, Charles Keepax wrote:
> Pierre requested I have a look at some cppcheck warnings in the cs42l42
> driver, since it is reassigning the ret variable without ever checking
> the result.  Looking a bit more broadly this happens in quite a few
> legacy Cirrus parts, as they all use the same process to read the ID,
> factor out a small helper so they can all share the same code. Whilst
> in there fix up a couple of other trivial error path issues as well.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[01/10] ASoC: cirrus: Add helper function for reading the device ID
        commit: c8b198ed31000a48f507bcea3828374b75418a2f
[02/10] ASoC: cs35l32: Minor error paths fixups
        commit: 283160f1419ddebc8779c3488e800cd30b31289d
[03/10] ASoC: cs35l33: Minor error paths fixups
        commit: 77908dbecdb6be2e875ced738b5c036bb83e8d78
[04/10] ASoC: cs35l34: Minor error paths fixups
        commit: 8cb9b001635cfa0389ec54eb511cb459968ad1d7
[05/10] ASoC: cs35l35: Minor error paths fixups
        commit: 60ba916d87600684a1e127b484e1c407c355caad
[06/10] ASoC: cs35l35: Correct errata handling
        commit: 1a46b7b82df57b6b6a4e891cdbb2de1cf818a43b
[07/10] ASoC: cs42l42: Minor error paths fixups
        commit: 0a0eb567e1d43cb87e9740c8e417d6fcff061582
[08/10] ASoC: cs42l73: Minor error paths fixups
        commit: 26495252fe0d1ebf548c02cb63b51abae5d5e5a3
[09/10] ASoC: cs43130: Minor error paths fixups
        commit: e2bb1077cee4d13dc85d53d76deac73b24d7f845
[10/10] ASoC: cs53l30: Minor error paths fixups
        commit: 4fc81bc88ad9d6bac90d169382c6045c47d48648

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 00/10] Tidy up device ID reading on legacy Cirrus parts

[Charles Keepax]

On Mon, May 10, 2021 at 09:59:55AM -0500, Pierre-Louis Bossart wrote:
> On 5/10/21 8:13 AM, Charles Keepax wrote:
> Sounds all good to me, just wondering if while you're at it you can
> fix the remaining minor issues? Thanks!
> 
> cppcheck --platform=unix64 --force --max-configs=1024 --inconclusive
> --enable=all --suppress=variableScope
> --suppress=shiftTooManyBitsSigned
> --suppress=arithOperationsOnVoidPointer --suppress=bitwiseOnBoolean
> sound/soc/codecs/cs*.c
> 
> sound/soc/codecs/cs35l36.c:1159:10: style: Variable 'ret' is
> assigned a value that is never used. [unreadVariable]
>  int ret = 0;
>          ^
> sound/soc/codecs/cs4265.c:619:6: style: Variable 'ret' is reassigned
> a value before the old one has been used. [redundantAssignment]
>  ret = devm_snd_soc_register_component(&i2c_client->dev,
>      ^
> sound/soc/codecs/cs4265.c:604:6: note: ret is assigned
>  ret = regmap_read(cs4265->regmap, CS4265_CHIP_ID, &reg);
>      ^
> sound/soc/codecs/cs4265.c:619:6: note: ret is overwritten
>  ret = devm_snd_soc_register_component(&i2c_client->dev,
>      ^
> sound/soc/codecs/cs42l52.c:1202:6: style: Variable 'ret' is
> reassigned a value before the old one has been used.
> [redundantAssignment]
>  ret =  devm_snd_soc_register_component(&i2c_client->dev,
>      ^
> sound/soc/codecs/cs42l52.c:1165:6: note: ret is assigned
>  ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, &reg);
>      ^
> sound/soc/codecs/cs42l52.c:1202:6: note: ret is overwritten
>  ret =  devm_snd_soc_register_component(&i2c_client->dev,

Apologies my cppcheck doesn't seem to generate these, I guess that
is what I get for using the one that comes from using the one in
the package manager rather than building an up to date one.

I will have a look at these extra warnings as well.

Thanks,
Charles