> On 25 Aug 2020, at 11:58 PM, Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx> 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?