| Message ID | 20210515071112.101535-8-o-takashi@sakamocchi.jp |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: oxfw: code refactoring for quirks specific to ASICs | expand |
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
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
diff --git a/sound/firewire/oxfw/oxfw-stream.c b/sound/firewire/oxfw/oxfw-stream.c index 80c9dc13f1b5..33a7d0f308f1 100644 --- a/sound/firewire/oxfw/oxfw-stream.c +++ b/sound/firewire/oxfw/oxfw-stream.c @@ -153,12 +153,18 @@ static int init_stream(struct snd_oxfw *oxfw, struct amdtp_stream *stream) struct cmp_connection *conn; enum cmp_direction c_dir; enum amdtp_stream_direction s_dir; + enum cip_flags flags = CIP_NONBLOCKING; int err; if (stream == &oxfw->tx_stream) { conn = &oxfw->out_conn; c_dir = CMP_OUTPUT; s_dir = AMDTP_IN_STREAM; + + if (oxfw->quirks & SND_OXFW_QUIRK_JUMBO_PAYLOAD) + flags |= CIP_JUMBO_PAYLOAD; + if (oxfw->wrong_dbs) + flags |= CIP_WRONG_DBS; } else { conn = &oxfw->in_conn; c_dir = CMP_INPUT; @@ -169,24 +175,12 @@ static int init_stream(struct snd_oxfw *oxfw, struct amdtp_stream *stream) if (err < 0) return err; - err = amdtp_am824_init(stream, oxfw->unit, s_dir, CIP_NONBLOCKING); + err = amdtp_am824_init(stream, oxfw->unit, s_dir, flags); if (err < 0) { cmp_connection_destroy(conn); return err; } - /* - * OXFW starts to transmit packets with non-zero dbc. - * OXFW postpone transferring packets till handling any asynchronous - * packets. As a result, next isochronous packet includes more data - * blocks than IEC 61883-6 defines. - */ - if (stream == &oxfw->tx_stream) { - oxfw->tx_stream.flags |= CIP_JUMBO_PAYLOAD; - if (oxfw->wrong_dbs) - oxfw->tx_stream.flags |= CIP_WRONG_DBS; - } - return 0; } diff --git a/sound/firewire/oxfw/oxfw.c b/sound/firewire/oxfw/oxfw.c index 9a9c84bc811a..90a66e1312fe 100644 --- a/sound/firewire/oxfw/oxfw.c +++ b/sound/firewire/oxfw/oxfw.c @@ -86,6 +86,9 @@ static int name_card(struct snd_oxfw *oxfw) goto end; be32_to_cpus(&firmware); + if (firmware >> 20 == 0x970) + oxfw->quirks |= SND_OXFW_QUIRK_JUMBO_PAYLOAD; + /* to apply card definitions */ if (oxfw->entry->vendor_id == VENDOR_GRIFFIN || oxfw->entry->vendor_id == VENDOR_LACIE) { diff --git a/sound/firewire/oxfw/oxfw.h b/sound/firewire/oxfw/oxfw.h index fa2d7f9e2dc3..9e1c12546ab5 100644 --- 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; bool wrong_dbs; bool has_output; bool has_input;
This commit adds enumeration to describe quirks of OXFW ASICs. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- sound/firewire/oxfw/oxfw-stream.c | 20 +++++++------------- sound/firewire/oxfw/oxfw.c | 3 +++ sound/firewire/oxfw/oxfw.h | 7 +++++++ 3 files changed, 17 insertions(+), 13 deletions(-)