Re: [PATCH v2 07/10] ALSA: oxfw: code refactoring for jumbo-payload quirk in OXFW970

[Takashi Iwai <tiwai@suse.de>]

On Sat, 15 May 2021 09:11:09 +0200,
Takashi Sakamoto wrote:
> --- a/sound/firewire/oxfw/oxfw.h
> +++ b/sound/firewire/oxfw/oxfw.h
> @@ -32,6 +32,12 @@
>  #include "../amdtp-am824.h"
>  #include "../cmp.h"
>  
> +enum snd_oxfw_quirk {
> +	// Postpone transferring packets during handling asynchronous transaction. As a result,
> +	// next isochronous packet includes more events than one packet can include.
> +	SND_OXFW_QUIRK_JUMBO_PAYLOAD = 0x01,
> +};
> +
>  /* This is an arbitrary number for convinience. */
>  #define	SND_OXFW_STREAM_FORMAT_ENTRIES	10
>  struct snd_oxfw {
> @@ -43,6 +49,7 @@ struct snd_oxfw {
>  	bool registered;
>  	struct delayed_work dwork;
>  
> +	enum snd_oxfw_quirk quirks;

Declaring the field as this enum type for bit flags isn't really
right, IMO.  Usually an enum *type* is used for storing only the
enumerated values as-is, but the actual code (in a later patch) stores
the combination of the defined values as bits.
That is, if a field is defined with an enum type, readers and
compilers may believe that only the defined values are stored there,
while the code doesn't follow that, which is a confusing situation.

I see that a similar pattern is used already in the firewire code, but
I don't think this justifies to introduce it to yet another place.


thanks,

Takashi