ASoC: codecs: add uspport for the TI SRC4392 codec

Hi Matt,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on tiwai-sound/for-next linus/master v5.19 next-20220808] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matt-Flax/ASoC-codecs-add-uspport-for-the-TI-SRC4392-codec/20220809-054524
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: hexagon-randconfig-r002-20220808 (https://download.01.org/0day-ci/archive/20220809/202208090909.Pg0BZGie-lkp(a)intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/7a9219a8431d7740c0958e53078820cbfef4f3f7
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Matt-Flax/ASoC-codecs-add-uspport-for-the-TI-SRC4392-codec/20220809-054524 git checkout 7a9219a8431d7740c0958e53078820cbfef4f3f7
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash sound/soc/codecs/

If you fix the issue, kindly add following tag where applicable

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 1 line

Quoted 1 line

sound/soc/codecs/src4xxx.c:277:3: warning: variable 'd' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized]

                   default:
                   ^~~~~~~
   sound/soc/codecs/src4xxx.c:294:59: note: uninitialized use occurs here
                   ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_11, d);
                                                                           ^
   sound/soc/codecs/src4xxx.c:221:20: note: initialize the variable 'd' to silence this warning
           int val, pj, jd, d;
                             ^
                              = 0

Quoted 1 line

Quoted 1 line

sound/soc/codecs/src4xxx.c:277:3: warning: variable 'jd' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized]

                   default:
                   ^~~~~~~
   sound/soc/codecs/src4xxx.c:289:59: note: uninitialized use occurs here
                   ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_10, jd);
                                                                           ^~
   sound/soc/codecs/src4xxx.c:221:17: note: initialize the variable 'jd' to silence this warning
           int val, pj, jd, d;
                          ^
                           = 0

Quoted 1 line

Quoted 1 line

sound/soc/codecs/src4xxx.c:277:3: warning: variable 'pj' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized]

                   default:
                   ^~~~~~~
   sound/soc/codecs/src4xxx.c:284:59: note: uninitialized use occurs here
                   ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_0F, pj);
                                                                           ^~
   sound/soc/codecs/src4xxx.c:221:13: note: initialize the variable 'pj' to silence this warning
           int val, pj, jd, d;
                      ^
                       = 0

3 warnings generated.
--

Quoted 1 line

Quoted 1 line

sound/soc/codecs/src4xxx-i2c.c:35:34: warning: unused variable 'src4xxx_of_match' [-Wunused-const-variable]

   static const struct of_device_id src4xxx_of_match[] = {
                                    ^

1 warning generated.

vim +/d +277 sound/soc/codecs/src4xxx.c

   213	
   214	static int src4xxx_hw_params(struct snd_pcm_substream *substream,
   215				struct snd_pcm_hw_params *params,
   216				struct snd_soc_dai *dai)
   217	{
   218		struct snd_soc_component *component = dai->component;
   219		struct src4xxx *src4xxx = snd_soc_component_get_drvdata(component);
   220		unsigned int mclk_div;
   221		int val, pj, jd, d;
   222		int reg;
   223		int ret;
   224	
   225		switch (dai->id) {
   226		case SRC4XXX_PORTB:
   227			reg = SRC4XXX_PORTB_CTL_06;
   228			break;
   229		default:
   230			reg = SRC4XXX_PORTA_CTL_04;
   231		}
   232	
   233		if (src4xxx->master[dai->id]) {
   234			mclk_div = src4xxx->mclk_hz/params_rate(params);
   235			if (src4xxx->mclk_hz != mclk_div*params_rate(params)) {
   236				dev_err(component->dev,
   237					"mclk %d / rate %d has a remainder.\n",
   238					src4xxx->mclk_hz, params_rate(params));
   239				return -EINVAL;
   240			}
   241	
   242			val = ((int)mclk_div - 128) / 128;
   243			if ((val < 0) | (val > 3)) {
   244				dev_err(component->dev,
   245					"div register setting %d is out of range\n",
   246					val);
   247				dev_err(component->dev,
   248					"unsupported sample rate %d Hz for the master clock of %d Hz\n",
   249					params_rate(params), src4xxx->mclk_hz);
   250				return -EINVAL;
   251			}
   252	
   253			/* set the TX DIV */
   254			ret = regmap_update_bits(src4xxx->regmap,
   255				SRC4XXX_TX_CTL_07, SRC4XXX_TX_MCLK_DIV_MASK,
   256				val<<SRC4XXX_TX_MCLK_DIV_SHIFT);
   257			if (ret) {
   258				dev_err(component->dev,
   259					"Couldn't set the TX's div register to %d << %d = 0x%x\n",
   260					val, SRC4XXX_TX_MCLK_DIV_SHIFT,
   261					val<<SRC4XXX_TX_MCLK_DIV_SHIFT);
   262				return ret;
   263			}
   264	
   265			/* set the PLL for the digital receiver */
   266			switch (src4xxx->mclk_hz) {
   267			case 24576000:
   268				pj = 0x22;
   269				jd = 0x00;
   270				d = 0x00;
   271				break;
   272			case 22579200:
   273				pj = 0x22;
   274				jd = 0x1b;
   275				d = 0xa3;
   276				break;

Quoted 1 line

277 default:

   278				/* don't error out here,
   279				 * other parts of the chip are still functional
   280				 */
   281				dev_info(component->dev,
   282					"Couldn't set the RCV PLL as this master clock rate is unknown\n");

   283			}
   284			ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_0F, pj);
   285			if (ret < 0)
   286				dev_err(component->dev,
   287					"Failed to update PLL register 0x%x\n",
   288					SRC4XXX_RCV_PLL_0F);

Quoted 1 line

289 ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_10, jd);

290 if (ret < 0)
291 dev_err(component->dev,
292 "Failed to update PLL register 0x%x\n",
293 SRC4XXX_RCV_PLL_10);

Quoted 1 line

294 ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_11, d);

295 if (ret < 0)
296 dev_err(component->dev,
297 "Failed to update PLL register 0x%x\n",

   298					SRC4XXX_RCV_PLL_11);
   299	
   300			ret = regmap_update_bits(src4xxx->regmap,
   301				SRC4XXX_TX_CTL_07, SRC4XXX_TX_MCLK_DIV_MASK,
   302				val<<SRC4XXX_TX_MCLK_DIV_SHIFT);
   303			if (ret < 0) {
   304				dev_err(component->dev,
   305					"Couldn't set the TX's div register to %d << %d = 0x%x\n",
   306					val, SRC4XXX_TX_MCLK_DIV_SHIFT,
   307					val<<SRC4XXX_TX_MCLK_DIV_SHIFT);
   308				return ret;
   309			}
   310	

   311			return regmap_update_bits(src4xxx->regmap, reg,
   312						SRC4XXX_MCLK_DIV_MASK, val);
   313		} else
   314			dev_info(dai->dev, "not setting up MCLK as not master\n");
   315	
   316		return 0;
   317	};
   318	

Signature · 2 lines

0-DAY CI Kernel Test Service
https://01.org/lkp

Quoted 12 lines

    265			/* set the PLL for the digital receiver */
    266			switch (src4xxx->mclk_hz) {
    267			case 24576000:
    268				pj = 0x22;
    269				jd = 0x00;
    270				d = 0x00;
    271				break;
    272			case 22579200:
    273				pj = 0x22;
    274				jd = 0x1b;
    275				d = 0xa3;
    276				break;

Quoted 6 lines

Quoted 1 line

277 default:

    278				/* don't error out here,
    279				 * other parts of the chip are still functional
    280				 */
    281				dev_info(component->dev,
    282					"Couldn't set the RCV PLL as this master clock rate is unknown\n");

In the final commit, there is a 'break' here. Should it be a 'return 0' instead? Or should there be a different fix for these warnings?

Quoted 11 lines

    283			}
    284			ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_0F, pj);
    285			if (ret < 0)
    286				dev_err(component->dev,
    287					"Failed to update PLL register 0x%x\n",
    288					SRC4XXX_RCV_PLL_0F);

Quoted 1 line

289 ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_10, jd);

    290			if (ret < 0)
    291				dev_err(component->dev,
    292					"Failed to update PLL register 0x%x\n",
    293					SRC4XXX_RCV_PLL_10);

Quoted 6 lines

Quoted 1 line

294 ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_11, d);

    295			if (ret < 0)
    296				dev_err(component->dev,
    297					"Failed to update PLL register 0x%x\n",
    298					SRC4XXX_RCV_PLL_11);

Cheers,
Nathan

Quoted 18 lines · Nathan Chancellor

Quoted 12 lines

    265			/* set the PLL for the digital receiver */
    266			switch (src4xxx->mclk_hz) {
    267			case 24576000:
    268				pj = 0x22;
    269				jd = 0x00;
    270				d = 0x00;
    271				break;
    272			case 22579200:
    273				pj = 0x22;
    274				jd = 0x1b;
    275				d = 0xa3;
    276				break;

Quoted 6 lines

Quoted 1 line

277 default:

    278				/* don't error out here,
    279				 * other parts of the chip are still functional
    280				 */
    281				dev_info(component->dev,
    282					"Couldn't set the RCV PLL as this master clock rate is unknown\n");

Quoted 2 lines

In the final commit, there is a 'break' here. Should it be a 'return 0' instead? Or should there be a different fix for these warnings?

The data sheet for the src4392 doesn't list defaults for these registers (loaded with pj, jd and d). The actual state of these registers is not known until we load them with something, arbitrary or not.

	{  SRC4XXX_RCV_PLL_0F,		0x00  },  /* not spec. in the datasheet */
	{  SRC4XXX_RCV_PLL_10,		0xff  },  /* not spec. in the datasheet */
	{  SRC4XXX_RCV_PLL_11,		0xff  },  /* not spec. in the datasheet */

The state of DIR PLL registers aren't important if the user doesn't specify a known mclk frequency.  The implication is that the DIR will not function, however that is already implied by the user lacking to specify a known mclk frequency.

The other functions on the chip (port A/B I2S, SRC, DIT, etc) will behave as per usual, only the DIR will be dysfunctional.

Quoted 18 lines

Quoted 11 lines

    283			}
    284			ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_0F, pj);
    285			if (ret < 0)
    286				dev_err(component->dev,
    287					"Failed to update PLL register 0x%x\n",
    288					SRC4XXX_RCV_PLL_0F);

Quoted 1 line

289 ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_10, jd);

    290			if (ret < 0)
    291				dev_err(component->dev,
    292					"Failed to update PLL register 0x%x\n",
    293					SRC4XXX_RCV_PLL_10);

Quoted 5 lines

Quoted 1 line

294 ret = regmap_write(src4xxx->regmap, SRC4XXX_RCV_PLL_11, d);

    295			if (ret < 0)
    296				dev_err(component->dev,
    297					"Failed to update PLL register 0x%x\n",
    298					SRC4XXX_RCV_PLL_11);

Cheers,
Nathan

Quoted 9 lines · Matt Flax

Quoted 2 lines · Nathan Chancellor

In the final commit, there is a 'break' here. Should it be a 'return 0' instead? Or should there be a different fix for these warnings?

The state of DIR PLL registers aren't important if the user doesn't specify a known mclk frequency.  The implication is that the DIR will not function, however that is already implied by the user lacking to specify a known mclk frequency.

Quoted 2 lines

The other functions on the chip (port A/B I2S, SRC, DIT, etc) will behave as per usual, only the DIR will be dysfunctional.

So I suppose there is little point to all of the calls to regmap_write() and regmap_update_bits() in the default case then, meaning a 'return 0' would be appropriate? Sorry, I am having a hard time parsing what should be done about the warnings, which are fatal for allmodconfig due to CONFIG_WERROR.

Quoted 13 lines · Nathan Chancellor

Quoted 12 lines · Matt Flax

Quoted 2 lines · Nathan Chancellor

In the final commit, there is a 'break' here. Should it be a 'return 0' instead? Or should there be a different fix for these warnings?

The state of DIR PLL registers aren't important if the user doesn't specify a known mclk frequency.  The implication is that the DIR will not function, however that is already implied by the user lacking to specify a known mclk frequency.

The other functions on the chip (port A/B I2S, SRC, DIT, etc) will behave as per usual, only the DIR will be dysfunctional.

Quoted 5 lines

So I suppose there is little point to all of the calls to regmap_write() and regmap_update_bits() in the default case then, meaning a 'return 0' would be appropriate? Sorry, I am having a hard time parsing what should be done about the warnings, which are fatal for allmodconfig due to CONFIG_WERROR.

Are either of you still looking at fixing this?

Quoted 2 lines · kernel test robot

config: hexagon-randconfig-r002-20220808 (https://download.01.org/0day-ci/archive/20220809/202208090909.Pg0BZGie-lkp(a)intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)

It doesn't look like these warnings were addressed before the change was applied to -next as commit 4e6bedd3c396 ("ASoC: codecs: add support for the TI SRC4392 codec"). I now see them in next-20220816.

Quoted 4 lines · Nathan Chancellor

Quoted 2 lines · kernel test robot

config: hexagon-randconfig-r002-20220808 (https://download.01.org/0day-ci/archive/20220809/202208090909.Pg0BZGie-lkp(a)intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)

Quoted 3 lines

It doesn't look like these warnings were addressed before the change was applied to -next as commit 4e6bedd3c396 ("ASoC: codecs: add support for the TI SRC4392 codec"). I now see them in next-20220816.

It's probably worth talking to the 0day people about prioritising what they're reporting against, especially given that the reports have evolved into a bit of an eye chart - this was reported against a Hexagon randconfig with an unreleased compiler which is underselling it rather.

Quoted 4 lines · Mark Brown

It's probably worth talking to the 0day people about prioritising what they're reporting against, especially given that the reports have evolved into a bit of an eye chart - this was reported against a Hexagon randconfig with an unreleased compiler which is underselling it rather.

At the same time, I would expect developers and maintainers to focus on the warning text first and foremost, not what architecture, configuration, or compiler is being used. This issue is very clearly not architecture or configuration specific, there is no #ifdef in this

function that changes the nature of the warning. While it is compiler specific (because possible uninitialized variable warnings are disabled with GCC), it is not dependent on the version (although I guess that isn't apparent). I suppose I can just comment on future randconfig reports to point out how they will affect allmodconfig and such.

* Documented the devicetree binding