* [bug report] ASoC: Intel: KMB: Enable TDM audio capture
@ 2020-08-25 13:21 Dan Carpenter
2020-08-25 13:49 ` Sit, Michael Wei Hong
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2020-08-25 13:21 UTC (permalink / raw)
To: michael.wei.hong.sit; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [bug report] ASoC: Intel: KMB: Enable TDM audio capture
2020-08-25 13:21 [bug report] ASoC: Intel: KMB: Enable TDM audio capture Dan Carpenter
@ 2020-08-25 13:49 ` Sit, Michael Wei Hong
2020-08-25 13:58 ` Dan Carpenter
0 siblings, 1 reply; 8+ messages in thread
From: Sit, Michael Wei Hong @ 2020-08-25 13:49 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel@alsa-project.org, Sia, Jee Heng
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [bug report] ASoC: Intel: KMB: Enable TDM audio capture
2020-08-25 13:49 ` Sit, Michael Wei Hong
@ 2020-08-25 13:58 ` Dan Carpenter
2020-08-25 14:47 ` Sit, Michael Wei Hong
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2020-08-25 13:58 UTC (permalink / raw)
To: Sit, Michael Wei Hong; +Cc: alsa-devel@alsa-project.org, Sia, Jee Heng
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [bug report] ASoC: Intel: KMB: Enable TDM audio capture
2020-08-25 13:58 ` Dan Carpenter
@ 2020-08-25 14:47 ` Sit, Michael Wei Hong
2020-08-25 15:58 ` Pierre-Louis Bossart
0 siblings, 1 reply; 8+ messages in thread
From: Sit, Michael Wei Hong @ 2020-08-25 14:47 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel@alsa-project.org, Sia, Jee Heng
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [bug report] ASoC: Intel: KMB: Enable TDM audio capture
2020-08-25 14:47 ` Sit, Michael Wei Hong
@ 2020-08-25 15:58 ` Pierre-Louis Bossart
2020-08-25 17:22 ` Sit, Michael Wei Hong
0 siblings, 1 reply; 8+ messages in thread
From: Pierre-Louis Bossart @ 2020-08-25 15:58 UTC (permalink / raw)
To: Sit, Michael Wei Hong, Dan Carpenter
Cc: alsa-devel@alsa-project.org, Sia, Jee Heng
>>>> 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?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [bug report] ASoC: Intel: KMB: Enable TDM audio capture
2020-08-25 15:58 ` Pierre-Louis Bossart
@ 2020-08-25 17:22 ` Sit, Michael Wei Hong
2020-08-25 18:23 ` Pierre-Louis Bossart
0 siblings, 1 reply; 8+ messages in thread
From: Sit, Michael Wei Hong @ 2020-08-25 17:22 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel@alsa-project.org, Sia, Jee Heng, Dan Carpenter
> 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?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [bug report] ASoC: Intel: KMB: Enable TDM audio capture
2020-08-25 17:22 ` Sit, Michael Wei Hong
@ 2020-08-25 18:23 ` Pierre-Louis Bossart
2020-08-26 3:09 ` Sit, Michael Wei Hong
0 siblings, 1 reply; 8+ messages in thread
From: Pierre-Louis Bossart @ 2020-08-25 18:23 UTC (permalink / raw)
To: Sit, Michael Wei Hong
Cc: alsa-devel@alsa-project.org, Sia, Jee Heng, 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?
>
> Yea you are right, would leaving this !MASTER_MODE out the best solution?
Sounds good to me. Thanks Dan for the report!
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [bug report] ASoC: Intel: KMB: Enable TDM audio capture
2020-08-25 18:23 ` Pierre-Louis Bossart
@ 2020-08-26 3:09 ` Sit, Michael Wei Hong
0 siblings, 0 replies; 8+ messages in thread
From: Sit, Michael Wei Hong @ 2020-08-26 3:09 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel@alsa-project.org, Sia, Jee Heng, Dan Carpenter
> -----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!
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-26 3:11 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-25 13:21 [bug report] ASoC: Intel: KMB: Enable TDM audio capture Dan Carpenter
2020-08-25 13:49 ` Sit, Michael Wei Hong
2020-08-25 13:58 ` Dan Carpenter
2020-08-25 14:47 ` Sit, Michael Wei Hong
2020-08-25 15:58 ` Pierre-Louis Bossart
2020-08-25 17:22 ` Sit, Michael Wei Hong
2020-08-25 18:23 ` Pierre-Louis Bossart
2020-08-26 3:09 ` Sit, Michael Wei Hong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.