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

[Takashi Iwai]

On Tue, 01 May 2018 11:14:56 +0200,
Takashi Sakamoto wrote:
> 
> 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;
> }
> ```

That's one way, yes.
(BTW, you can write down in a style
	[0 ... 2] = SND_DICE_RATE_MODE_LOW,
too.)

Or, deal the exception explicity, e.g.

	*mode = i > 0 ? ((i - 1) / 2) : 0;

Or, mention just that i=0 shall give mode=0 with signed int.

I don't mind either way.


thanks,

Takashi