> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Wednesday, 26 August, 2020 2:23 AM
> To: Sit, Michael Wei Hong <michael.wei.hong.sit@xxxxxxxxx>
> Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>; alsa-devel@alsa-
> project.org; Sia, Jee Heng <jee.heng.sia@xxxxxxxxx>
> 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!
