[PATCH 00/10] Tidy up device ID reading on legacy Cirrus parts
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
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, ®);
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); +}
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, ®); - devid = (reg & 0xFF) << 12; - - ret = regmap_read(cs35l32->regmap, CS35L32_DEVID_CD, ®); - devid |= (reg & 0xFF) << 4; - - ret = regmap_read(cs35l32->regmap, CS35L32_DEVID_E, ®); - 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, ®); 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, ®); + regmap_read(cs35l32->regmap, CS35L32_INT_STATUS_1, ®);
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;
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, ®); - devid = (reg & 0xFF) << 12; - ret = regmap_read(cs35l33->regmap, CS35L33_DEVID_CD, ®); - devid |= (reg & 0xFF) << 4; - ret = regmap_read(cs35l33->regmap, CS35L33_DEVID_E, ®); - 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);
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, ®); - - devid = (reg & 0xFF) << 12; - ret = regmap_read(cs35l34->regmap, CS35L34_DEVID_CD, ®); - devid |= (reg & 0xFF) << 4; - ret = regmap_read(cs35l34->regmap, CS35L34_DEVID_E, ®); - 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, ®); 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);
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, ®); - - devid = (reg & 0xFF) << 12; - ret = regmap_read(cs35l35->regmap, CS35L35_DEVID_CD, ®); - devid |= (reg & 0xFF) << 4; - ret = regmap_read(cs35l35->regmap, CS35L35_DEVID_E, ®); - 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",
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
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, ®); - devid = (reg & 0xFF) << 12; - - ret = regmap_read(cs42l42->regmap, CS42L42_DEVID_CD, ®); - devid |= (reg & 0xFF) << 4; - - ret = regmap_read(cs42l42->regmap, CS42L42_DEVID_E, ®); - 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;
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, ®); - devid = (reg & 0xFF) << 12; - - ret = regmap_read(cs42l73->regmap, CS42L73_DEVID_CD, ®); - devid |= (reg & 0xFF) << 4; - - ret = regmap_read(cs42l73->regmap, CS42L73_DEVID_E, ®); - 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, ®); 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[] = {
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, ®); - - devid = (reg & 0xFF) << 12; - ret = regmap_read(cs43130->regmap, CS43130_DEVID_CD, ®); - devid |= (reg & 0xFF) << 4; - ret = regmap_read(cs43130->regmap, CS43130_DEVID_E, ®); - 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; }
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, ®); - devid = reg << 12; - - ret = regmap_read(cs53l30->regmap, CS53L30_DEVID_CD, ®); - devid |= reg << 4; - - ret = regmap_read(cs53l30->regmap, CS53L30_DEVID_E, ®); - 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;
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, ®); ^ 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, ®); ^ 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
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, ®); ^ 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, ®); ^ 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
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
participants (3)
-
Charles Keepax -
Mark Brown -
Pierre-Louis Bossart