mbox series

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

Message ID 20210510131357.17170-1-ckeepax@opensource.cirrus.com (mailing list archive)
Headers show
Series Tidy up device ID reading on legacy Cirrus parts | expand

Message

Charles Keepax May 10, 2021, 1:13 p.m. UTC
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

Comments

Pierre-Louis Bossart May 10, 2021, 2:59 p.m. UTC | #1
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
>
Mark Brown May 11, 2021, 8:26 a.m. UTC | #2
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
Charles Keepax May 11, 2021, 9:12 a.m. UTC | #3
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