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

Message ID 20210515071112.101535-8-o-takashi@sakamocchi.jp
State Superseded
Headers show
Series
  • ALSA: oxfw: code refactoring for quirks specific to ASICs
Related show

Commit Message

Takashi Sakamoto May 15, 2021, 7:11 a.m.
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(-)

Comments

Takashi Iwai May 17, 2021, 9:11 a.m. | #1
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
Takashi Sakamoto May 18, 2021, 1:45 a.m. | #2
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

Patch

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;