[bug report] ASoC: Intel: KMB: Enable TDM audio capture
Hello Michael Sit Wei Hong,
The patch 9c3bab3c4f15: "ASoC: Intel: KMB: Enable TDM audio capture" from Aug 11, 2020, leads to the following static checker warning:
sound/soc/intel/keembay/kmb_platform.c:518 kmb_dai_hw_params() warn: potential ! vs ~ typo
sound/soc/intel/keembay/kmb_platform.c 502 } 503 504 config->chan_nr = params_channels(hw_params); 505 506 switch (config->chan_nr) { 507 case 8: 508 case 4: 509 /* 510 * Platform is not capable of providing clocks for 511 * multi channel audio 512 */ 513 if (kmb_i2s->master) 514 return -EINVAL; 515 516 write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) | 517 (config->data_width << DATA_WIDTH_CONFIG_BIT) | 518 !MASTER_MODE | TDM_OPERATION; ^^^^^^^^^^^^ MASTER_MODE is BIT(13). It's unclear what this is supposed to be. My best guess is that the ! should just be deleted.
519 520 writel(write_val, kmb_i2s->pss_base + I2S_GEN_CFG_0); 521 break; 522 case 2: 523 /* 524 * Platform is only capable of providing clocks need for 525 * 2 channel master mode 526 */ 527 if (!(kmb_i2s->master)) 528 return -EINVAL; 529 530 write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) | 531 (config->data_width << DATA_WIDTH_CONFIG_BIT) | 532 MASTER_MODE | I2S_OPERATION; 533 534 writel(write_val, kmb_i2s->pss_base + I2S_GEN_CFG_0); 535 break; 536 default: 537 dev_dbg(kmb_i2s->dev, "channel not supported\n"); 538 return -EINVAL; 539 }
regards, dan carpenter
On 25 Aug 2020, at 9:21 PM, Dan Carpenter dan.carpenter@oracle.com wrote:
Hello Michael Sit Wei Hong,
The patch 9c3bab3c4f15: "ASoC: Intel: KMB: Enable TDM audio capture" from Aug 11, 2020, leads to the following static checker warning:
sound/soc/intel/keembay/kmb_platform.c:518 kmb_dai_hw_params() warn: potential ! vs ~ typo
sound/soc/intel/keembay/kmb_platform.c 502 } 503 504 config->chan_nr = params_channels(hw_params); 505 506 switch (config->chan_nr) { 507 case 8: 508 case 4: 509 /* 510 * Platform is not capable of providing clocks for 511 * multi channel audio 512 */ 513 if (kmb_i2s->master) 514 return -EINVAL; 515 516 write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) | 517 (config->data_width << DATA_WIDTH_CONFIG_BIT) | 518 !MASTER_MODE | TDM_OPERATION; ^^^^^^^^^^^^ MASTER_MODE is BIT(13). It's unclear what this is supposed to be. My best guess is that the ! should just be deleted.
This ! is intentional because it is meant to be Slave mode. Would a better approach be to create another #define for slave mode?
519 520 writel(write_val, kmb_i2s->pss_base + I2S_GEN_CFG_0); 521 break; 522 case 2: 523 /* 524 * Platform is only capable of providing clocks need for 525 * 2 channel master mode 526 */ 527 if (!(kmb_i2s->master)) 528 return -EINVAL; 529 530 write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) | 531 (config->data_width << DATA_WIDTH_CONFIG_BIT) | 532 MASTER_MODE | I2S_OPERATION; 533 534 writel(write_val, kmb_i2s->pss_base + I2S_GEN_CFG_0); 535 break; 536 default: 537 dev_dbg(kmb_i2s->dev, "channel not supported\n"); 538 return -EINVAL; 539 }
regards, dan carpenter
On Tue, Aug 25, 2020 at 01:49:25PM +0000, Sit, Michael Wei Hong wrote:
On 25 Aug 2020, at 9:21 PM, Dan Carpenter dan.carpenter@oracle.com wrote:
Hello Michael Sit Wei Hong,
The patch 9c3bab3c4f15: "ASoC: Intel: KMB: Enable TDM audio capture" from Aug 11, 2020, leads to the following static checker warning:
sound/soc/intel/keembay/kmb_platform.c:518 kmb_dai_hw_params() warn: potential ! vs ~ typo
sound/soc/intel/keembay/kmb_platform.c 502 } 503 504 config->chan_nr = params_channels(hw_params); 505 506 switch (config->chan_nr) { 507 case 8: 508 case 4: 509 /* 510 * Platform is not capable of providing clocks for 511 * multi channel audio 512 */ 513 if (kmb_i2s->master) 514 return -EINVAL; 515 516 write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) | 517 (config->data_width << DATA_WIDTH_CONFIG_BIT) | 518 !MASTER_MODE | TDM_OPERATION; ^^^^^^^^^^^^ MASTER_MODE is BIT(13). It's unclear what this is supposed to be. My best guess is that the ! should just be deleted.
This ! is intentional because it is meant to be Slave mode. Would a better approach be to create another #define for slave mode?
In my opinion, it's better to just leave it out. ORing with zero causes a different static checker warning on my unreleased checks... Is it 0 << 13? I feel like ORing with zero just makes things more confusing.
regards, dan carpenter
On 25 Aug 2020, at 10:00 PM, Dan Carpenter dan.carpenter@oracle.com wrote:
On Tue, Aug 25, 2020 at 01:49:25PM +0000, Sit, Michael Wei Hong wrote:
On 25 Aug 2020, at 9:21 PM, Dan Carpenter dan.carpenter@oracle.com wrote:
Hello Michael Sit Wei Hong,
The patch 9c3bab3c4f15: "ASoC: Intel: KMB: Enable TDM audio capture" from Aug 11, 2020, leads to the following static checker warning:
sound/soc/intel/keembay/kmb_platform.c:518 kmb_dai_hw_params() warn: potential ! vs ~ typo
sound/soc/intel/keembay/kmb_platform.c 502 } 503 504 config->chan_nr = params_channels(hw_params); 505 506 switch (config->chan_nr) { 507 case 8: 508 case 4: 509 /* 510 * Platform is not capable of providing clocks for 511 * multi channel audio 512 */ 513 if (kmb_i2s->master) 514 return -EINVAL; 515 516 write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) | 517 (config->data_width << DATA_WIDTH_CONFIG_BIT) | 518 !MASTER_MODE | TDM_OPERATION; ^^^^^^^^^^^^ MASTER_MODE is BIT(13). It's unclear what this is supposed to be. My best guess is that the ! should just be deleted.
This ! is intentional because it is meant to be Slave mode. Would a better approach be to create another #define for slave mode?
In my opinion, it's better to just leave it out. ORing with zero causes a different static checker warning on my unreleased checks... Is it 0 << 13? I feel like ORing with zero just makes things more confusing.
It is 0<<13, in the event it was previously configured to Master I would need to unset the bit
regards, dan carpenter
506 switch (config->chan_nr) { 507 case 8: 508 case 4: 509 /* 510 * Platform is not capable of providing clocks for 511 * multi channel audio 512 */ 513 if (kmb_i2s->master) 514 return -EINVAL; 515 516 write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) | 517 (config->data_width << DATA_WIDTH_CONFIG_BIT) | 518 !MASTER_MODE | TDM_OPERATION; ^^^^^^^^^^^^ MASTER_MODE is BIT(13). It's unclear what this is supposed to be. My best guess is that the ! should just be deleted.
This ! is intentional because it is meant to be Slave mode. Would a better approach be to create another #define for slave mode?
In my opinion, it's better to just leave it out. ORing with zero causes a different static checker warning on my unreleased checks... Is it 0 << 13? I feel like ORing with zero just makes things more confusing.
It is 0<<13, in the event it was previously configured to Master I would need to unset the bit
You are assigning the result to write_val, so there's no memory of what was configured before?
On 25 Aug 2020, at 11:58 PM, Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
506 switch (config->chan_nr) { 507 case 8: 508 case 4: 509 /* 510 * Platform is not capable of providing clocks for 511 * multi channel audio 512 */ 513 if (kmb_i2s->master) 514 return -EINVAL; 515 516 write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) | 517 (config->data_width << DATA_WIDTH_CONFIG_BIT) | 518 !MASTER_MODE | TDM_OPERATION; ^^^^^^^^^^^^ MASTER_MODE is BIT(13). It's unclear what this is supposed to be. My best guess is that the ! should just be deleted.
This ! is intentional because it is meant to be Slave mode. Would a better approach be to create another #define for slave mode?
In my opinion, it's better to just leave it out. ORing with zero causes a different static checker warning on my unreleased checks... Is it 0 << 13? I feel like ORing with zero just makes things more confusing.
It is 0<<13, in the event it was previously configured to Master I would need to unset the bit
You are assigning the result to write_val, so there's no memory of what was configured before?
Yea you are right, would leaving this !MASTER_MODE out the best solution?
506 switch (config->chan_nr) { 507 case 8: 508 case 4: 509 /* 510 * Platform is not capable of providing clocks for 511 * multi channel audio 512 */ 513 if (kmb_i2s->master) 514 return -EINVAL; 515 516 write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) | 517 (config->data_width << DATA_WIDTH_CONFIG_BIT) | 518 !MASTER_MODE | TDM_OPERATION; ^^^^^^^^^^^^ MASTER_MODE is BIT(13). It's unclear what this is supposed to be. My best guess is that the ! should just be deleted.
This ! is intentional because it is meant to be Slave mode. Would a better approach be to create another #define for slave mode?
In my opinion, it's better to just leave it out. ORing with zero causes a different static checker warning on my unreleased checks... Is it 0 << 13? I feel like ORing with zero just makes things more confusing.
It is 0<<13, in the event it was previously configured to Master I would need to unset the bit
You are assigning the result to write_val, so there's no memory of what was configured before?
Yea you are right, would leaving this !MASTER_MODE out the best solution?
Sounds good to me. Thanks Dan for the report!
-----Original Message----- From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Sent: Wednesday, 26 August, 2020 2:23 AM To: Sit, Michael Wei Hong michael.wei.hong.sit@intel.com Cc: Dan Carpenter dan.carpenter@oracle.com; alsa-devel@alsa- project.org; Sia, Jee Heng jee.heng.sia@intel.com Subject: Re: [bug report] ASoC: Intel: KMB: Enable TDM audio capture
> 506 switch (config->chan_nr) { > 507 case 8: > 508 case 4: > 509 /* > 510 * Platform is not capable of providing clocks
for
> 511 * multi channel audio > 512 */ > 513 if (kmb_i2s->master) > 514 return -EINVAL; > 515 > 516 write_val = ((config->chan_nr / 2) <<
TDM_CHANNEL_CONFIG_BIT) |
> 517 (config->data_width <<
DATA_WIDTH_CONFIG_BIT) |
> 518 !MASTER_MODE | TDM_OPERATION; > ^^^^^^^^^^^^ MASTER_MODE > is BIT(13). It's unclear what this is supposed to be. My best > guess is that the ! should just be deleted.
This ! is intentional because it is meant to be Slave mode.
Would a better approach be to create another #define for slave mode?
In my opinion, it's better to just leave it out. ORing with zero causes a different static checker warning on my unreleased checks... Is it 0 << 13? I feel like ORing with zero just makes things more
confusing.
It is 0<<13, in the event it was previously configured to Master I would need to unset the bit
You are assigning the result to write_val, so there's no memory
of what was configured before?
Yea you are right, would leaving this !MASTER_MODE out the best
solution?
Sounds good to me. Thanks Dan for the report!
Tested removing the !MASTER_MODE from the write_val, it still works as expected Removing !MASTER_MODE sounds good to me. Thanks Dan for the report!
participants (3)
-
Dan Carpenter -
Pierre-Louis Bossart -
Sit, Michael Wei Hong