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

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

On Mon, May 17, 2021 at 11:11:07AM +0200, Takashi Iwai wrote:
> 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.

The concern is in the category of human practice, and heuristics, in my
opinion.

I check C language specification and it says that enumeration-constant
has type int, and enumerated type shall be compatible with char, a signed
integer type, or an unsigned integer type and the choice of type is
implementation-defined. The assignment of ORed enumeration-constant (int)
to enumerated type (int with 32 bit storage in most System V ABIs) is not
forbidden past and future though the specification mentions about its
warnings in the annex.

Nevertheless, the practical point should be respected as well. I'll
prepare take 3 patchset including fix for some issued points.


Thanks

Takashi Sakamoto