Re: [PATCH] ASoC: samsung: Minor simplification in i2s_set_fmt()

[Tobias Jakobi <tjakobi@math.uni-bielefeld.de>]

Hello Krzysztof,


Krzysztof Kozlowski wrote:
> On Mon, Oct 10, 2016 at 04:04:05AM +0200, Tobias Jakobi wrote:
>> From: Michael McConville <mmcco@mykolab.com>
>>
>> We can replace the if-statement with the simpler bit
>> toggle operation.
>>
>> Changes in v2:
>> - add proper commit header and description
>>
>> Signed-off-by: Michael McConville <mmcco@mykolab.com>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  sound/soc/samsung/i2s.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
>> index 50635ee..1c484ac 100644
>> --- a/sound/soc/samsung/i2s.c
>> +++ b/sound/soc/samsung/i2s.c
>> @@ -627,10 +627,7 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
>>  	case SND_SOC_DAIFMT_NB_NF:
>>  		break;
>>  	case SND_SOC_DAIFMT_NB_IF:
>> -		if (tmp & lrp_rlow)
>> -			tmp &= ~lrp_rlow;
>> -		else
>> -			tmp |= lrp_rlow;
>> +		tmp ^= lrp_rlow;
> 
> It is late, full day of work... so I might be thinking slowly... but
> this does not look like proper replacement. Unless for specific values
> used in the driver this works? From mathematical point of view, imagine
> tmp=6 abd lrp_rlow=3.
> 
> This is not equal...
> 
> But... maybe it is late... :)
No, you're correct, this obviously only works for lrp_rlow of the type
(1 << n). For arbitrary values this transformation isn't equivalent. But
it applies here.

Anyway, please drop this if you think it is misleading. I just found it
while cleaning my inbox.

- Tobias




> 
> Krzysztof
>