M-Audio Firewire 1814 and ProjectMix I/O change their stream formation according to current digital format for input/output, although drivers cannot recognize current formation by any commands. Thus the drivers need to remember the formations. The special_stream_formation_set() is for this purpose.

This function is expected to be called just after current digital formats are changed. The way to change current digital format is to call avc_maudio_set_special_clk() and there is no other ways.

For simplicity, this commit make the avc_maudio_set_special_clk() as a sole caller for the special_stream_formation_set().

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/bebob/bebob_maudio.c | 62 ++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c index 0a33045..83f8d5b 100644 --- a/sound/firewire/bebob/bebob_maudio.c +++ b/sound/firewire/bebob/bebob_maudio.c @@ -160,6 +160,35 @@ end: return err; }

+static void +special_stream_formation_set(struct snd_bebob *bebob) +{ + static const unsigned int ch_table[2][2][3] = { + /* AMDTP_OUT_STREAM */ + { { 6, 6, 4 }, /* SPDIF */ + { 12, 8, 4 } }, /* ADAT */ + /* AMDTP_IN_STREAM */ + { { 10, 10, 2 }, /* SPDIF */ + { 16, 12, 2 } } /* ADAT */ + }; + struct special_params *params = bebob->maudio_special_quirk; + unsigned int i, max; + + max = SND_BEBOB_STRM_FMT_ENTRIES - 1; + if (!params->is1814) + max -= 2; + + for (i = 0; i < max; i++) { + bebob->tx_stream_formations[i + 1].pcm = + ch_table[AMDTP_IN_STREAM][params->dig_in_fmt][i / 2]; + bebob->tx_stream_formations[i + 1].midi = 1; + + bebob->rx_stream_formations[i + 1].pcm = + ch_table[AMDTP_OUT_STREAM][params->dig_out_fmt][i / 2]; + bebob->rx_stream_formations[i + 1].midi = 1; + } +} + /* * dig_fmt: 0x00:S/PDIF, 0x01:ADAT * clk_lock: 0x00:unlock, 0x01:lock @@ -212,6 +241,8 @@ avc_maudio_set_special_clk(struct snd_bebob *bebob, unsigned int clk_src, params->dig_out_fmt = buf[8]; params->clk_lock = buf[9];

+ special_stream_formation_set(bebob); + if (params->ctl_id_sync) snd_ctl_notify(bebob->card, SNDRV_CTL_EVENT_MASK_VALUE, params->ctl_id_sync); @@ -221,34 +252,6 @@ end: kfree(buf); return err; } -static void -special_stream_formation_set(struct snd_bebob *bebob) -{ - static const unsigned int ch_table[2][2][3] = { - /* AMDTP_OUT_STREAM */ - { { 6, 6, 4 }, /* SPDIF */ - { 12, 8, 4 } }, /* ADAT */ - /* AMDTP_IN_STREAM */ - { { 10, 10, 2 }, /* SPDIF */ - { 16, 12, 2 } } /* ADAT */ - }; - struct special_params *params = bebob->maudio_special_quirk; - unsigned int i, max; - - max = SND_BEBOB_STRM_FMT_ENTRIES - 1; - if (!params->is1814) - max -= 2; - - for (i = 0; i < max; i++) { - bebob->tx_stream_formations[i + 1].pcm = - ch_table[AMDTP_IN_STREAM][params->dig_in_fmt][i / 2]; - bebob->tx_stream_formations[i + 1].midi = 1; - - bebob->rx_stream_formations[i + 1].pcm = - ch_table[AMDTP_OUT_STREAM][params->dig_out_fmt][i / 2]; - bebob->rx_stream_formations[i + 1].midi = 1; - } -}

static int add_special_controls(struct snd_bebob *bebob); int @@ -280,8 +283,6 @@ snd_bebob_maudio_special_discover(struct snd_bebob *bebob, bool is1814) if (err < 0) goto end;

- special_stream_formation_set(bebob); - if (params->is1814) { bebob->midi_input_ports = 1; bebob->midi_output_ports = 1; @@ -513,7 +514,6 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl, dev_err(&bebob->unit->device, "fail to set digital input interface: %d\n", err); end: - special_stream_formation_set(bebob); mutex_unlock(&bebob->mutex); return err; }

At Mon, 21 Jul 2014 11:10:02 +0900, Takashi Sakamoto wrote:

This commit adds some dev_err() to help debug when avc_maudio_set_special_clk() failed.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp

sound/firewire/bebob/bebob_maudio.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c index 83f8d5b..008ff2c 100644 --- a/sound/firewire/bebob/bebob_maudio.c +++ b/sound/firewire/bebob/bebob_maudio.c @@ -393,6 +393,10 @@ static int special_clk_ctl_set(struct snd_kcontrol *kctl,

mutex_unlock(&bebob->mutex);

• if (err < 0)

• dev_err(&bebob->unit->device,
  

• 	"fail to change clock source: %d\n", err);
  

The error message at the end of put callback is often useless. If it's an error, it's notified to user-space and you'll see the error there. The unconditional error message via dev_err() may flood the kernel messages, too.

For the debugging purpose, rather put dev_dbg() to each point receiving an error and give the individual message.

thanks,

Takashi

return err >= 0; } static struct snd_kcontrol_new special_clk_ctl = { @@ -506,7 +510,14 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl, dig_in_fmt, params->dig_out_fmt, params->clk_lock);

• if ((err < 0) || (params->dig_in_fmt > 0)) /* ADAT */

• if (err < 0) {

• dev_err(&bebob->unit->device,
  

• 	"fail to change clock source: %d\n", err);
  

• goto end;
  

• }

• /* For ADAT, optical interface is only available. */

• if (params->dig_in_fmt > 0) goto end;

  err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface);

@@ -567,10 +578,13 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl, params->clk_src, params->dig_in_fmt, id, params->clk_lock);

• if (err >= 0)

• special_stream_formation_set(bebob);
  
  

  mutex_unlock(&bebob->mutex);

• if (err < 0)

• dev_err(&bebob->unit->device,
  

• 	"fail to change digital output interface: %d\n", err);
  

• return err;

} static struct snd_kcontrol_new special_dig_out_iface_ctl = { -- 1.9.1

At Mon, 21 Jul 2014 11:10:03 +0900, Takashi Sakamoto wrote:

This commit use different labels for control elements of digital input/output interfaces to correct my misunderstanding about M-Audio Firewire 1814 and ProjectMix I/O.

According to user manuals for these two models, they have two modes for digital input; one is S/PDIF in both of optical and coaxial interfaces, another is ADAT in optical interface only.

But in current implementation, a control element for digital input uses reduced labels which a control element for digital output uses because of my misunderstanding that optical interface is not available for digital input with S/PDIF mode.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp

sound/firewire/bebob/bebob_maudio.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c index 008ff2c..f2277ce 100644 --- a/sound/firewire/bebob/bebob_maudio.c +++ b/sound/firewire/bebob/bebob_maudio.c @@ -440,8 +440,8 @@ static struct snd_kcontrol_new special_sync_ctl = { .get = special_sync_ctl_get, };

-/* Digital interface control for special firmware */ -static char *const special_dig_iface_labels[] = { +/* Digital input interface control for special firmware */ +static char *const special_dig_in_iface_labels[] = { "S/PDIF Optical", "S/PDIF Coaxial", "ADAT Optical" }; static int special_dig_in_iface_ctl_info(struct snd_kcontrol *kctl, @@ -449,13 +449,13 @@ static int special_dig_in_iface_ctl_info(struct snd_kcontrol *kctl, { einf->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; einf->count = 1;

• einf->value.enumerated.items = ARRAY_SIZE(special_dig_iface_labels);

• einf->value.enumerated.items = ARRAY_SIZE(special_dig_in_iface_labels);

  if (einf->value.enumerated.item >= einf->value.enumerated.items) einf->value.enumerated.item = einf->value.enumerated.items - 1;

  strcpy(einf->value.enumerated.name,

•       special_dig_iface_labels[einf->value.enumerated.item]);
  

•       special_dig_in_iface_labels[einf->value.enumerated.item]);
  
  

  return 0;

} @@ -498,6 +498,8 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl, int err;

id = uval->value.enumerated.item[0];

• if (id >= ARRAY_SIZE(special_dig_in_iface_labels))

• return 0;
  

This should return an error.

/* decode user value */ dig_in_fmt = (id >> 1) & 0x01; @@ -537,18 +539,22 @@ static struct snd_kcontrol_new special_dig_in_iface_ctl = { .put = special_dig_in_iface_ctl_set };

+/* Digital output interface control for special firmware */ +static char *const special_dig_out_iface_labels[] = {

• "S/PDIF Optical and Coaxial", "ADAT Optical"

+}; static int special_dig_out_iface_ctl_info(struct snd_kcontrol *kctl, struct snd_ctl_elem_info *einf) { einf->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; einf->count = 1;

• einf->value.enumerated.items = ARRAY_SIZE(special_dig_iface_labels) - 1;

• einf->value.enumerated.items = ARRAY_SIZE(special_dig_out_iface_labels);

  if (einf->value.enumerated.item >= einf->value.enumerated.items) einf->value.enumerated.item = einf->value.enumerated.items - 1;

  strcpy(einf->value.enumerated.name,

•       special_dig_iface_labels[einf->value.enumerated.item + 1]);
  

•       special_dig_out_iface_labels[einf->value.enumerated.item]);
  
  

  return 0;

} @@ -571,6 +577,8 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl, int err;

id = uval->value.enumerated.item[0];

• if (id >= ARRAY_SIZE(special_dig_out_iface_labels))

• return 0;
  

Ditto.

thanks,

Takashi

mutex_lock(&bebob->mutex);

-- 1.9.1

This commit is for correction of my misunderstanding for return value of .put callback of ALSA Control interface.

According to 'Writing ALSA Driver' (*1), return value of the callback has three patterns; 1: changed, 0: not changed, an negative value: fatal error.

But I misunderstood that it's boolean; zero or nonzero.

*1: Writing an ALSA Driver (2005, Takashi Iwai) http://www.alsa-project.org/main/index.php/ALSA_Driver_Documentation

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/bebob/bebob_maudio.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c index f2277ce..acfe94a 100644 --- a/sound/firewire/bebob/bebob_maudio.c +++ b/sound/firewire/bebob/bebob_maudio.c @@ -396,8 +396,10 @@ static int special_clk_ctl_set(struct snd_kcontrol *kctl, if (err < 0) dev_err(&bebob->unit->device, "fail to change clock source: %d\n", err); + else + err = 1;

- return err >= 0; + return err; } static struct snd_kcontrol_new special_clk_ctl = { .name = "Clock Source", @@ -519,13 +521,17 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl, }

/* For ADAT, optical interface is only available. */ - if (params->dig_in_fmt > 0) + if (params->dig_in_fmt > 0) { + err = 1; goto end; + }

err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface); if (err < 0) dev_err(&bebob->unit->device, "fail to set digital input interface: %d\n", err); + else + err = 1; end: mutex_unlock(&bebob->mutex); return err; @@ -592,6 +598,8 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl, if (err < 0) dev_err(&bebob->unit->device, "fail to change digital output interface: %d\n", err); + else + err = 1;

return err; }

This commit uses different labels for control elements of digital input/output interfaces to correct my misunderstanding about M-Audio Firewire 1814 and ProjectMix I/O.

According to user manuals for these two models, they have two modes for digital input; one is S/PDIF in both of optical and coaxial interfaces, another is ADAT in optical interface only.

But in current implementation, a control element for it reduced labels which a control element for digital output uses because of my misunderstanding that optical interface is not available for digital input with S/PDIF mode.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/bebob/bebob_maudio.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c index fc470c6..42e6f22 100644 --- a/sound/firewire/bebob/bebob_maudio.c +++ b/sound/firewire/bebob/bebob_maudio.c @@ -434,8 +434,8 @@ static struct snd_kcontrol_new special_sync_ctl = { .get = special_sync_ctl_get, };

-/* Digital interface control for special firmware */ -static char *const special_dig_iface_labels[] = { +/* Digital input interface control for special firmware */ +static char *const special_dig_in_iface_labels[] = { "S/PDIF Optical", "S/PDIF Coaxial", "ADAT Optical" }; static int special_dig_in_iface_ctl_info(struct snd_kcontrol *kctl, @@ -443,13 +443,13 @@ static int special_dig_in_iface_ctl_info(struct snd_kcontrol *kctl, { einf->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; einf->count = 1; - einf->value.enumerated.items = ARRAY_SIZE(special_dig_iface_labels); + einf->value.enumerated.items = ARRAY_SIZE(special_dig_in_iface_labels);

if (einf->value.enumerated.item >= einf->value.enumerated.items) einf->value.enumerated.item = einf->value.enumerated.items - 1;

strcpy(einf->value.enumerated.name, - special_dig_iface_labels[einf->value.enumerated.item]); + special_dig_in_iface_labels[einf->value.enumerated.item]);

return 0; } @@ -504,9 +504,14 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl, dig_in_fmt, params->dig_out_fmt, params->clk_lock); - if ((err < 0) || (params->dig_in_fmt > 0)) /* ADAT */ + if (err < 0) + goto end; + + /* For ADAT, optical interface is only available. */ + if (params->dig_in_fmt > 0) goto end;

+ /* For S/PDIF, optical/coaxial interfaces are selectable. */ err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface); if (err < 0) dev_err(&bebob->unit->device, @@ -525,18 +530,22 @@ static struct snd_kcontrol_new special_dig_in_iface_ctl = { .put = special_dig_in_iface_ctl_set };

+/* Digital output interface control for special firmware */ +static char *const special_dig_out_iface_labels[] = { + "S/PDIF Optical and Coaxial", "ADAT Optical" +}; static int special_dig_out_iface_ctl_info(struct snd_kcontrol *kctl, struct snd_ctl_elem_info *einf) { einf->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; einf->count = 1; - einf->value.enumerated.items = ARRAY_SIZE(special_dig_iface_labels) - 1; + einf->value.enumerated.items = ARRAY_SIZE(special_dig_out_iface_labels);

if (einf->value.enumerated.item >= einf->value.enumerated.items) einf->value.enumerated.item = einf->value.enumerated.items - 1;

strcpy(einf->value.enumerated.name, - special_dig_iface_labels[einf->value.enumerated.item + 1]); + special_dig_out_iface_labels[einf->value.enumerated.item]);

return 0; }

Oops. I miss a line for this patch. Please drop this patch and apply another one which I will send now.

Regards

Takashi Sakamoto o-takashi@sakamocchi.jp

(Jul 22 2014 23:13), Takashi Sakamoto wrote:

This commit is for correction of my misunderstanding about return value of .put callback in ALSA Control interface.

According to 'Writing ALSA Driver' (*1), return value of the callback has three patterns; 1: changed, 0: not changed, an negative value: fatal error.

But I misunderstood that it's boolean; zero or nonzero.

*1: Writing an ALSA Driver (2005, Takashi Iwai) http://www.alsa-project.org/main/index.php/ALSA_Driver_Documentation

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp

sound/firewire/bebob/bebob_maudio.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c index 42e6f22..d6d6ff8 100644 --- a/sound/firewire/bebob/bebob_maudio.c +++ b/sound/firewire/bebob/bebob_maudio.c @@ -391,7 +391,10 @@ static int special_clk_ctl_put(struct snd_kcontrol *kctl, params->clk_lock); mutex_unlock(&bebob->mutex);

• return err >= 0;

• if (err >= 0)

• err = 1;
  

• return err;

} static struct snd_kcontrol_new special_clk_ctl = { .name = "Clock Source", @@ -491,14 +494,16 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl, unsigned int id, dig_in_fmt, dig_in_iface; int err;

• mutex_lock(&bebob->mutex);
• id = uval->value.enumerated.item[0];

• if (id >= ARRAY_SIZE(special_dig_in_iface_labels))

• return -EINVAL;
  
  

  /* decode user value */ dig_in_fmt = (id >> 1) & 0x01; dig_in_iface = id & 0x01;

• mutex_lock(&bebob->mutex);

• err = avc_maudio_set_special_clk(bebob, params->clk_src, dig_in_fmt,

@@ -508,14 +513,17 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl, goto end;

/* For ADAT, optical interface is only available. */

• if (params->dig_in_fmt > 0)

• if (params->dig_in_fmt > 0) {

• err = 1;
  

  goto end;

• }

  /* For S/PDIF, optical/coaxial interfaces are selectable. */ err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface); if (err < 0) dev_err(&bebob->unit->device, "fail to set digital input interface: %d\n", err);

• err = 1;

end: special_stream_formation_set(bebob); mutex_unlock(&bebob->mutex); @@ -567,16 +575,20 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl, unsigned int id; int err;

• mutex_lock(&bebob->mutex);
• id = uval->value.enumerated.item[0];

• if (id >= ARRAY_SIZE(special_dig_out_iface_labels))

• return -EINVAL;
  

• mutex_lock(&bebob->mutex);

  err = avc_maudio_set_special_clk(bebob, params->clk_src, params->dig_in_fmt, id, params->clk_lock);

• if (err >= 0)

• if (err >= 0) { special_stream_formation_set(bebob);

• err = 1;
  

• }

  mutex_unlock(&bebob->mutex); return err;

At Tue, 22 Jul 2014 23:11:03 +0900, Takashi Sakamoto wrote:

In error handling case, special_clk_ctl_put() returns without unlock_mutex(), therefore the mutex is still locked. This commit moves mutex_lock() after the error handling case.

This commit is my solution for this post.

[PATCH -next] ALSA: bebob: Fix missing unlock on error in special_clk_ctl_put() https://lkml.org/lkml/2014/7/20/12

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp

Thanks, applied all three patches now.

BTW, at the next time, put "v2" or such in the subject line so that people can distinguish the new patch series from the previous ones. You can use --subject-prefix option for git-format-patch or else.

Preferably, write the changes since the previous revision, too (usually below the --- line, so that it's not merged into the git changelog; some people like to have them in the final commit, though.)

Last but not least, don't forget to put a proper subject prefix. For ALSA codes, we put "ALSA:" prefix in the subject. Study git changelogs.

Takashi

---

sound/firewire/bebob/bebob_maudio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c index 6af50eb..fc470c6 100644 --- a/sound/firewire/bebob/bebob_maudio.c +++ b/sound/firewire/bebob/bebob_maudio.c @@ -379,12 +379,12 @@ static int special_clk_ctl_put(struct snd_kcontrol *kctl, struct special_params *params = bebob->maudio_special_quirk; int err, id;

• mutex_lock(&bebob->mutex);
• id = uval->value.enumerated.item[0]; if (id >= ARRAY_SIZE(special_clk_labels)) return 0;

• mutex_lock(&bebob->mutex);
• err = avc_maudio_set_special_clk(bebob, id, params->dig_in_fmt, params->dig_out_fmt,

-- 1.9.1

This commit is a supplement to my previous patch. http://mailman.alsa-project.org/pipermail/alsa-devel/2014-July/079190.html

The special_clk_ctl_put() still returns 0 in error handling case. It should return -EINVAL.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/bebob/bebob_maudio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c index d6d6ff8..70faa3a 100644 --- a/sound/firewire/bebob/bebob_maudio.c +++ b/sound/firewire/bebob/bebob_maudio.c @@ -381,7 +381,7 @@ static int special_clk_ctl_put(struct snd_kcontrol *kctl,

id = uval->value.enumerated.item[0]; if (id >= ARRAY_SIZE(special_clk_labels)) - return 0; + return -EINVAL;

mutex_lock(&bebob->mutex);