linux-kernel - Re: [RFC][PATCH 21/26] alsa: ASoC: Add JZ4740 ASoC support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4C07E849.6080202@metafoo.de>
Date:	Thu, 03 Jun 2010 19:37:13 +0200
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Liam Girdwood <lrg@...mlogic.co.uk>
CC:	Ralf Baechle <ralf@...ux-mips.org>, linux-mips@...ux-mips.org,
	linux-kernel@...r.kernel.org,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	alsa-devel@...a-project.org
Subject: Re: [RFC][PATCH 21/26] alsa: ASoC: Add JZ4740 ASoC support

Liam Girdwood wrote:
> On Thu, 2010-06-03 at 19:16 +0200, Lars-Peter Clausen wrote:
>   
>> Liam Girdwood wrote:
>>     
>>> On Thu, 2010-06-03 at 18:50 +0200, Lars-Peter Clausen wrote:
>>>   
>>>       
>>>>>> +    config = snd_soc_dai_get_dma_data(rtd->dai->cpu_dai,
>>>>>>         
>>>>>>             
>>>> substream);
>>>>     
>>>>         
>>>>>> +    if (!prtd->dma) {
>>>>>> +            const char *dma_channel_name;
>>>>>> +            if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>>>>> +                    dma_channel_name = "PCM Playback";
>>>>>> +            else
>>>>>> +                    dma_channel_name = "PCM Capture";
>>>>>> +
>>>>>> +            prtd->dma = jz4740_dma_request(substream,
>>>>>>         
>>>>>>             
>>>> dma_channel_name);
>>>>     
>>>>         
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> dma_channel_name variable is not required here. Just use the const
>>>>>       
>>>>>           
>>>> char
>>>>     
>>>>         
>>>>> * directly.
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>> I actually had it like that before, but I think it is much more readable
>>>> in its current form.
>>>>     
>>>>         
>>> I disagree, having the char pointer here just adds an extra level of
>>> indirection and costs an extra two lines of code. 
>>>
>>> Liam 
>>>   
>>>       
>> Hi
>>
>> Could you give an concrete example of how you would code it?
>>
>>     
>
> Sure,
>
> if (!prtd->dma) {
>           if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>                    prtd->dma = jz4740_dma_request(substream, "PCM Playback");
>           else
>                    prtd->dma = jz4740_dma_request(substream, "PCM Capture");
> }
>
> Liam
>
>   
And now you have the same statement in two different lines. When it
needs to be changed you have to change both lines.
And furthermore in my opinion it distracts from the reason for the if
statement: We want a different channel name.

But ok, if you insist on it, I can live with changing it.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/