[alsa-devel] [PATCH v2 04/13] ALSA: dice: cache stream formats at current mode of sampling transmission frequency

[Takashi Sakamoto]

Hi,

On May 1 2018 15:54, Takashi Iwai wrote:
> On Tue, 01 May 2018 05:02:09 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On May 1 2018 00:10, Takashi Iwai wrote:
>>> On Sun, 29 Apr 2018 08:50:23 +0200,
>>> Takashi Sakamoto wrote:
>>>>
>>>> In former commits, proxy structure get members for cache of stream
>>>> formats. This commit fills the cache with stream formats at current mode
>>>> of sampling transmission frequency.
>>>>
>>>> Signed-off-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
>>>> ---
>>>>    sound/firewire/dice/dice-stream.c | 76 +++++++++++++++++++++++++++++++++++++++
>>>>    sound/firewire/dice/dice.c        |  4 +++
>>>>    sound/firewire/dice/dice.h        |  3 ++
>>>>    3 files changed, 83 insertions(+)
>>>>
>>>> diff --git a/sound/firewire/dice/dice-stream.c b/sound/firewire/dice/dice-stream.c
>>>> index 928a255bfc35..2a9f0cd994a5 100644
>>>> --- a/sound/firewire/dice/dice-stream.c
>>>> +++ b/sound/firewire/dice/dice-stream.c
>>>> @@ -30,6 +30,24 @@ const unsigned int snd_dice_rates[SND_DICE_RATES_COUNT] = {
>>>>    	[6] = 192000,
>>>>    };
>>>>    +int snd_dice_stream_get_rate_mode(struct snd_dice *dice,
>>>> unsigned int rate,
>>>> +				  unsigned int *mode)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(snd_dice_rates); i++) {
>>>> +		if (!(dice->clock_caps & BIT(i)))
>>>> +			continue;
>>>> +		if (snd_dice_rates[i] != rate)
>>>> +			continue;
>>>> +
>>>> +		*mode = (i - 1) / 2;
>>>
>>> What if i=0?  It'll be a negative value?
>>
>> Yes. But division by 2 to -1 results in 0 because in C language
>> specification 'truncate toward zero'[1] is applied to discard
>> fractional part. Then, the result is assigned to value of 'unsigned int'
>> type. As a result:
>>
>> 0: 32000/44100/48000
>> 1: 88200/96000
>> 2: 176400/192000
>>
>> This is what I expect.
>>
>> [1] footnote for '6.5.5 Multiplicative operators' clause in ISO/IEC 9899.
> 
> This is too tricky.  The result would change dramatically when i were
> declared as unsigned.
> 
> And I can think of someone trying to change it unsigned because of the
> comparison against ARRAY_SIZE() (we've got gcc warnings for that in
> the past).
> 
> Please make either it more robust or put a proper comment.

Hm. Any comment includes ambiguity, so I prefer the robust
representation with more consumption of .rodata section.

```
int snd_dice_stream_get_rate_mode(struct snd_dice *dice, unsigned int rate,
                                   enum snd_dice_rate_mode *mode)
{
	/* Corresponding to each entry in snd_dice_rates. */
         static const enum snd_dice_rate_mode modes[] = {
                 [0]     = SND_DICE_RATE_MODE_LOW,
                 [1]     = SND_DICE_RATE_MODE_LOW,
                 [2]     = SND_DICE_RATE_MODE_LOW,
                 [3]     = SND_DICE_RATE_MODE_MIDDLE,
                 [4]     = SND_DICE_RATE_MODE_MIDDLE,
                 [5]     = SND_DICE_RATE_MODE_HIGH,
                 [6]     = SND_DICE_RATE_MODE_HIGH,
         };
         int i;

         for (i = 0; i < ARRAY_SIZE(snd_dice_rates); i++) {
                 if (!(dice->clock_caps & BIT(i)))
                         continue;
                 if (snd_dice_rates[i] != rate)
                         continue;

                 *mode = modes[i];
                 return 0;
         }

         return -EINVAL;
}
```


Thanks

Takashi Sakamoto