* [PATCH 2/4] em28xx: simplify usb audio class handling
2014-09-13 8:52 [PATCH 1/4] em28xx: remove some unnecessary fields from struct em28xx_audio_mode Frank Schäfer
@ 2014-09-13 8:52 ` Frank Schäfer
2014-09-13 8:52 ` [PATCH 3/4] em28xx: get rid of field has_audio in struct em28xx_audio_mode Frank Schäfer
2014-09-13 8:52 ` [PATCH 4/4] em28xx: get rid of structs em28xx_ac97_mode and em28xx_audio_mode Frank Schäfer
2 siblings, 0 replies; 6+ messages in thread
From: Frank Schäfer @ 2014-09-13 8:52 UTC (permalink / raw)
To: m.chehab; +Cc: linux-media, Frank Schäfer
As far as we know devices can either have audio class or vendor class usb
interfaces but not both at the same time. Even if both interface types could be
provided by devices at the same time, the current code is totally broken for
that case.
So clean up and simplify the usb audio class handling by replacing fields
"has_audio_class" (device has usb audio class compliant interface) and
"has_alsa_audio" (device has vendor audio interface) in struct em28xx with a
single enum em28xx_usb_audio_type.
Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
drivers/media/usb/em28xx/em28xx-audio.c | 8 ++++----
drivers/media/usb/em28xx/em28xx-cards.c | 30 ++++++++++++++++--------------
drivers/media/usb/em28xx/em28xx-core.c | 8 ++++----
drivers/media/usb/em28xx/em28xx.h | 9 +++++++--
4 files changed, 31 insertions(+), 24 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index e881ef7..90c7a83 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -893,7 +893,7 @@ static int em28xx_audio_init(struct em28xx *dev)
static int devnr;
int err;
- if (!dev->has_alsa_audio) {
+ if (dev->usb_audio_type != EM28XX_USB_AUDIO_VENDOR) {
/* This device does not support the extension (in this case
the device is expecting the snd-usb-audio module or
doesn't have analog audio support at all) */
@@ -975,7 +975,7 @@ static int em28xx_audio_fini(struct em28xx *dev)
if (dev == NULL)
return 0;
- if (!dev->has_alsa_audio) {
+ if (dev->usb_audio_type != EM28XX_USB_AUDIO_VENDOR) {
/* This device does not support the extension (in this case
the device is expecting the snd-usb-audio module or
doesn't have analog audio support at all) */
@@ -1003,7 +1003,7 @@ static int em28xx_audio_suspend(struct em28xx *dev)
if (dev == NULL)
return 0;
- if (!dev->has_alsa_audio)
+ if (dev->usb_audio_type != EM28XX_USB_AUDIO_VENDOR)
return 0;
em28xx_info("Suspending audio extension");
@@ -1017,7 +1017,7 @@ static int em28xx_audio_resume(struct em28xx *dev)
if (dev == NULL)
return 0;
- if (!dev->has_alsa_audio)
+ if (dev->usb_audio_type != EM28XX_USB_AUDIO_VENDOR)
return 0;
em28xx_info("Resuming audio extension");
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 582c1e1..f142588 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -2931,9 +2931,9 @@ static void request_module_async(struct work_struct *work)
#if defined(CONFIG_MODULES) && defined(MODULE)
if (dev->has_video)
request_module("em28xx-v4l");
- if (dev->has_audio_class)
+ if (dev->usb_audio_type == EM28XX_USB_AUDIO_CLASS)
request_module("snd-usb-audio");
- else if (dev->has_alsa_audio)
+ else if (dev->usb_audio_type == EM28XX_USB_AUDIO_VENDOR)
request_module("em28xx-alsa");
if (dev->board.has_dvb)
request_module("em28xx-dvb");
@@ -3180,7 +3180,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
struct usb_device *udev;
struct em28xx *dev = NULL;
int retval;
- bool has_audio = false, has_video = false, has_dvb = false;
+ bool has_vendor_audio = false, has_video = false, has_dvb = false;
int i, nr, try_bulk;
const int ifnum = interface->altsetting[0].desc.bInterfaceNumber;
char *speed;
@@ -3262,7 +3262,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
break;
case 0x83:
if (usb_endpoint_xfer_isoc(e)) {
- has_audio = true;
+ has_vendor_audio = true;
} else {
printk(KERN_INFO DRIVER_NAME
": error: skipping audio endpoint 0x83, because it uses bulk transfers !\n");
@@ -3318,7 +3318,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
}
}
- if (!(has_audio || has_video || has_dvb)) {
+ if (!(has_vendor_audio || has_video || has_dvb)) {
retval = -ENODEV;
goto err_free;
}
@@ -3365,25 +3365,27 @@ static int em28xx_usb_probe(struct usb_interface *interface,
dev->devno = nr;
dev->model = id->driver_info;
dev->alt = -1;
- dev->is_audio_only = has_audio && !(has_video || has_dvb);
- dev->has_alsa_audio = has_audio;
+ dev->is_audio_only = has_vendor_audio && !(has_video || has_dvb);
dev->has_video = has_video;
dev->ifnum = ifnum;
- /* Checks if audio is provided by some interface */
+ if (has_vendor_audio) {
+ printk(KERN_INFO DRIVER_NAME ": Audio interface %i found %s\n",
+ ifnum, "(Vendor Class)");
+ dev->usb_audio_type = EM28XX_USB_AUDIO_VENDOR;
+ }
+ /* Checks if audio is provided by a USB Audio Class interface */
for (i = 0; i < udev->config->desc.bNumInterfaces; i++) {
struct usb_interface *uif = udev->config->interface[i];
if (uif->altsetting[0].desc.bInterfaceClass == USB_CLASS_AUDIO) {
- dev->has_audio_class = 1;
+ if (has_vendor_audio)
+ em28xx_err("em28xx: device seems to have vendor AND usb audio class interfaces !\n"
+ "\t\tThe vendor interface will be ignored. Please contact the developers <linux-media@vger.kernel.org>\n");
+ dev->usb_audio_type = EM28XX_USB_AUDIO_CLASS;
break;
}
}
- if (has_audio)
- printk(KERN_INFO DRIVER_NAME
- ": Audio interface %i found %s\n",
- ifnum,
- dev->has_audio_class ? "(USB Audio Class)" : "(Vendor Class)");
if (has_video)
printk(KERN_INFO DRIVER_NAME
": Video interface %i found:%s%s\n",
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 0fe05ff..16747ca 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -513,8 +513,7 @@ int em28xx_audio_setup(struct em28xx *dev)
dev->chip_id == CHIP_ID_EM28178) {
/* Digital only device - don't load any alsa module */
dev->audio_mode.has_audio = false;
- dev->has_audio_class = false;
- dev->has_alsa_audio = false;
+ dev->usb_audio_type = EM28XX_USB_AUDIO_NONE;
return 0;
}
@@ -528,8 +527,8 @@ int em28xx_audio_setup(struct em28xx *dev)
cfg = EM28XX_CHIPCFG_AC97; /* Be conservative */
} else if ((cfg & EM28XX_CHIPCFG_AUDIOMASK) == 0x00) {
/* The device doesn't have vendor audio at all */
- dev->has_alsa_audio = false;
dev->audio_mode.has_audio = false;
+ dev->usb_audio_type = EM28XX_USB_AUDIO_NONE;
return 0;
} else if ((cfg & EM28XX_CHIPCFG_AUDIOMASK) != EM28XX_CHIPCFG_AC97) {
if (dev->chip_id < CHIP_ID_EM2860 &&
@@ -560,7 +559,8 @@ int em28xx_audio_setup(struct em28xx *dev)
*/
em28xx_warn("AC97 chip type couldn't be determined\n");
dev->audio_mode.ac97 = EM28XX_NO_AC97;
- dev->has_alsa_audio = false;
+ if (dev->usb_audio_type == EM28XX_USB_AUDIO_VENDOR)
+ dev->usb_audio_type = EM28XX_USB_AUDIO_NONE;
dev->audio_mode.has_audio = false;
goto init_audio;
}
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 1f38163..3fd176f 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -312,6 +312,12 @@ struct em28xx_audio_mode {
unsigned int has_audio:1;
};
+enum em28xx_usb_audio_type {
+ EM28XX_USB_AUDIO_NONE = 0,
+ EM28XX_USB_AUDIO_CLASS,
+ EM28XX_USB_AUDIO_VENDOR,
+};
+
/* em28xx has two audio inputs: tuner and line in.
However, on most devices, an auxiliary AC97 codec device is used.
The AC97 device may have several different inputs and outputs,
@@ -601,9 +607,8 @@ struct em28xx {
unsigned int is_em25xx:1; /* em25xx/em276x/7x/8x family bridge */
unsigned char disconnected:1; /* device has been diconnected */
unsigned int has_video:1;
- unsigned int has_audio_class:1;
- unsigned int has_alsa_audio:1;
unsigned int is_audio_only:1;
+ enum em28xx_usb_audio_type usb_audio_type;
struct em28xx_board board;
--
1.8.4.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/4] em28xx: get rid of field has_audio in struct em28xx_audio_mode
2014-09-13 8:52 [PATCH 1/4] em28xx: remove some unnecessary fields from struct em28xx_audio_mode Frank Schäfer
2014-09-13 8:52 ` [PATCH 2/4] em28xx: simplify usb audio class handling Frank Schäfer
@ 2014-09-13 8:52 ` Frank Schäfer
2014-09-13 8:52 ` [PATCH 4/4] em28xx: get rid of structs em28xx_ac97_mode and em28xx_audio_mode Frank Schäfer
2 siblings, 0 replies; 6+ messages in thread
From: Frank Schäfer @ 2014-09-13 8:52 UTC (permalink / raw)
To: m.chehab; +Cc: linux-media, Frank Schäfer
Field has_audio in struct em28xx_audio_mode is used together with value
EM28XX_NO_AC97 of field ac97 to determine the internal type of audio (none/i2s/ac97).
This makes the code difficult to understand:
!dev->audio_mode.has_audio && audio_mode.ac97 == EM28XX_NO_AC97 => no audio
!dev->audio_mode.has_audio && audio_mode.ac97 != EM28XX_NO_AC97 => BUG
dev->audio_mode.has_audio && dev->audio_mode.ac97 == EM28XX_NO_AC97 => AC97 audio
dev->audio_mode.has_audio && dev->audio_mode.ac97 != EM28XX_NO_AC97 => I2S audio
Simplify the whole thing by introducing an enum em28xx_int_audio_type which
describes the internal audio type (none, ac97, i2s) and is hooked directly to
the device struct.
Then get rid of field has_audio in struct em28xx_audio_mode.
A follow-up patch will then remove struct em28xx_ac97_mode and finally the
whole struct em28xx_audio_mode.
Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
drivers/media/usb/em28xx/em28xx-core.c | 14 ++++++++------
drivers/media/usb/em28xx/em28xx-video.c | 6 +++---
drivers/media/usb/em28xx/em28xx.h | 8 +++++++-
3 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 16747ca..ed83e4e 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -433,7 +433,7 @@ int em28xx_audio_analog_set(struct em28xx *dev)
int ret, i;
u8 xclk;
- if (!dev->audio_mode.has_audio)
+ if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE)
return 0;
/* It is assumed that all devices use master volume for output.
@@ -512,25 +512,25 @@ int em28xx_audio_setup(struct em28xx *dev)
dev->chip_id == CHIP_ID_EM28174 ||
dev->chip_id == CHIP_ID_EM28178) {
/* Digital only device - don't load any alsa module */
- dev->audio_mode.has_audio = false;
+ dev->int_audio_type = EM28XX_INT_AUDIO_NONE;
dev->usb_audio_type = EM28XX_USB_AUDIO_NONE;
return 0;
}
- dev->audio_mode.has_audio = true;
-
/* See how this device is configured */
cfg = em28xx_read_reg(dev, EM28XX_R00_CHIPCFG);
em28xx_info("Config register raw data: 0x%02x\n", cfg);
if (cfg < 0) {
/* Register read error? */
cfg = EM28XX_CHIPCFG_AC97; /* Be conservative */
+ dev->int_audio_type = EM28XX_INT_AUDIO_AC97;
} else if ((cfg & EM28XX_CHIPCFG_AUDIOMASK) == 0x00) {
/* The device doesn't have vendor audio at all */
- dev->audio_mode.has_audio = false;
+ dev->int_audio_type = EM28XX_INT_AUDIO_NONE;
dev->usb_audio_type = EM28XX_USB_AUDIO_NONE;
return 0;
} else if ((cfg & EM28XX_CHIPCFG_AUDIOMASK) != EM28XX_CHIPCFG_AC97) {
+ dev->int_audio_type = EM28XX_INT_AUDIO_I2S;
if (dev->chip_id < CHIP_ID_EM2860 &&
(cfg & EM28XX_CHIPCFG_AUDIOMASK) ==
EM2820_CHIPCFG_I2S_1_SAMPRATE)
@@ -546,6 +546,8 @@ int em28xx_audio_setup(struct em28xx *dev)
/* Skip the code that does AC97 vendor detection */
dev->audio_mode.ac97 = EM28XX_NO_AC97;
goto init_audio;
+ } else {
+ dev->int_audio_type = EM28XX_INT_AUDIO_AC97;
}
dev->audio_mode.ac97 = EM28XX_AC97_OTHER;
@@ -561,7 +563,7 @@ int em28xx_audio_setup(struct em28xx *dev)
dev->audio_mode.ac97 = EM28XX_NO_AC97;
if (dev->usb_audio_type == EM28XX_USB_AUDIO_VENDOR)
dev->usb_audio_type = EM28XX_USB_AUDIO_NONE;
- dev->audio_mode.has_audio = false;
+ dev->int_audio_type = EM28XX_INT_AUDIO_NONE;
goto init_audio;
}
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 3642438..3284de9 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -1720,7 +1720,7 @@ static int vidioc_querycap(struct file *file, void *priv,
else
cap->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VBI_CAPTURE;
- if (dev->audio_mode.has_audio)
+ if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
cap->device_caps |= V4L2_CAP_AUDIO;
if (dev->tuner_type != TUNER_ABSENT)
@@ -2514,7 +2514,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
}
- if (!dev->audio_mode.has_audio) {
+ if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
}
@@ -2544,7 +2544,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
v4l2_disable_ioctl(v4l2->vbi_dev, VIDIOC_G_FREQUENCY);
v4l2_disable_ioctl(v4l2->vbi_dev, VIDIOC_S_FREQUENCY);
}
- if (!dev->audio_mode.has_audio) {
+ if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
v4l2_disable_ioctl(v4l2->vbi_dev, VIDIOC_G_AUDIO);
v4l2_disable_ioctl(v4l2->vbi_dev, VIDIOC_S_AUDIO);
}
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 3fd176f..857ad0c 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -309,7 +309,12 @@ enum em28xx_ac97_mode {
struct em28xx_audio_mode {
enum em28xx_ac97_mode ac97;
- unsigned int has_audio:1;
+};
+
+enum em28xx_int_audio_type {
+ EM28XX_INT_AUDIO_NONE = 0,
+ EM28XX_INT_AUDIO_AC97,
+ EM28XX_INT_AUDIO_I2S,
};
enum em28xx_usb_audio_type {
@@ -608,6 +613,7 @@ struct em28xx {
unsigned char disconnected:1; /* device has been diconnected */
unsigned int has_video:1;
unsigned int is_audio_only:1;
+ enum em28xx_int_audio_type int_audio_type;
enum em28xx_usb_audio_type usb_audio_type;
struct em28xx_board board;
--
1.8.4.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 4/4] em28xx: get rid of structs em28xx_ac97_mode and em28xx_audio_mode
2014-09-13 8:52 [PATCH 1/4] em28xx: remove some unnecessary fields from struct em28xx_audio_mode Frank Schäfer
2014-09-13 8:52 ` [PATCH 2/4] em28xx: simplify usb audio class handling Frank Schäfer
2014-09-13 8:52 ` [PATCH 3/4] em28xx: get rid of field has_audio in struct em28xx_audio_mode Frank Schäfer
@ 2014-09-13 8:52 ` Frank Schäfer
2014-09-23 0:02 ` Mauro Carvalho Chehab
2 siblings, 1 reply; 6+ messages in thread
From: Frank Schäfer @ 2014-09-13 8:52 UTC (permalink / raw)
To: m.chehab; +Cc: linux-media, Frank Schäfer
Now that we have enum em28xx_int_audio (none/i2s/ac97), it is no longer
necessary to check dev->audio_mode.ac97 to determine the type of internal audio connection.
There is also no need to save the type of the detected AC97 chip.
So replce the remaining checks of dev->audio_mode.ac97 with equivalent checks
of dev->int_audio_type and get rid of struct em28xx_ac97_mode and finally the
whole struct em28xx_audio_mode.
Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
drivers/media/usb/em28xx/em28xx-audio.c | 2 +-
drivers/media/usb/em28xx/em28xx-core.c | 36 ++++++---------------------------
drivers/media/usb/em28xx/em28xx-video.c | 2 +-
drivers/media/usb/em28xx/em28xx.h | 13 ------------
4 files changed, 8 insertions(+), 45 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index 90c7a83..c3a4224 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -933,7 +933,7 @@ static int em28xx_audio_init(struct em28xx *dev)
INIT_WORK(&adev->wq_trigger, audio_trigger);
- if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
+ if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
em28xx_cvol_new(card, dev, "Video", AC97_VIDEO);
em28xx_cvol_new(card, dev, "Line In", AC97_LINE);
em28xx_cvol_new(card, dev, "Phone", AC97_PHONE);
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index ed83e4e..7464e70 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -405,12 +405,8 @@ static int em28xx_set_audio_source(struct em28xx *dev)
return ret;
msleep(5);
- switch (dev->audio_mode.ac97) {
- case EM28XX_NO_AC97:
- break;
- default:
+ if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97)
ret = set_ac97_input(dev);
- }
return ret;
}
@@ -439,7 +435,7 @@ int em28xx_audio_analog_set(struct em28xx *dev)
/* It is assumed that all devices use master volume for output.
It would be possible to use also line output.
*/
- if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
+ if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
/* Mute all outputs */
for (i = 0; i < ARRAY_SIZE(outputs); i++) {
ret = em28xx_write_ac97(dev, outputs[i].reg, 0x8000);
@@ -462,7 +458,7 @@ int em28xx_audio_analog_set(struct em28xx *dev)
ret = em28xx_set_audio_source(dev);
/* Sets volume */
- if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
+ if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
int vol;
em28xx_write_ac97(dev, AC97_POWERDOWN, 0x4200);
@@ -544,14 +540,11 @@ int em28xx_audio_setup(struct em28xx *dev)
em28xx_info("I2S Audio (%d sample rate(s))\n",
i2s_samplerates);
/* Skip the code that does AC97 vendor detection */
- dev->audio_mode.ac97 = EM28XX_NO_AC97;
goto init_audio;
} else {
dev->int_audio_type = EM28XX_INT_AUDIO_AC97;
}
- dev->audio_mode.ac97 = EM28XX_AC97_OTHER;
-
vid1 = em28xx_read_ac97(dev, AC97_VENDOR_ID1);
if (vid1 < 0) {
/*
@@ -560,7 +553,6 @@ int em28xx_audio_setup(struct em28xx *dev)
* CHIPCFG register, even not having an AC97 chip
*/
em28xx_warn("AC97 chip type couldn't be determined\n");
- dev->audio_mode.ac97 = EM28XX_NO_AC97;
if (dev->usb_audio_type == EM28XX_USB_AUDIO_VENDOR)
dev->usb_audio_type = EM28XX_USB_AUDIO_NONE;
dev->int_audio_type = EM28XX_INT_AUDIO_NONE;
@@ -582,30 +574,14 @@ int em28xx_audio_setup(struct em28xx *dev)
/* Try to identify what audio processor we have */
if (((vid == 0xffffffff) || (vid == 0x83847650)) && (feat == 0x6a90))
- dev->audio_mode.ac97 = EM28XX_AC97_EM202;
- else if ((vid >> 8) == 0x838476)
- dev->audio_mode.ac97 = EM28XX_AC97_SIGMATEL;
-
-init_audio:
- /* Reports detected AC97 processor */
- switch (dev->audio_mode.ac97) {
- case EM28XX_NO_AC97:
- em28xx_info("No AC97 audio processor\n");
- break;
- case EM28XX_AC97_EM202:
em28xx_info("Empia 202 AC97 audio processor detected\n");
- break;
- case EM28XX_AC97_SIGMATEL:
+ else if ((vid >> 8) == 0x838476)
em28xx_info("Sigmatel audio processor detected (stac 97%02x)\n",
vid & 0xff);
- break;
- case EM28XX_AC97_OTHER:
+ else
em28xx_warn("Unknown AC97 audio processor detected!\n");
- break;
- default:
- break;
- }
+init_audio:
return em28xx_audio_analog_set(dev);
}
EXPORT_SYMBOL_GPL(em28xx_audio_setup);
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 3284de9..c4e1364 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -2384,7 +2384,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
__func__, ret);
goto unregister_dev;
}
- if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
+ if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
v4l2_ctrl_new_std(hdl, &em28xx_ctrl_ops,
V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
v4l2_ctrl_new_std(hdl, &em28xx_ctrl_ops,
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 857ad0c..3108eee 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -300,17 +300,6 @@ enum enum28xx_itype {
EM28XX_RADIO,
};
-enum em28xx_ac97_mode {
- EM28XX_NO_AC97 = 0,
- EM28XX_AC97_EM202,
- EM28XX_AC97_SIGMATEL,
- EM28XX_AC97_OTHER,
-};
-
-struct em28xx_audio_mode {
- enum em28xx_ac97_mode ac97;
-};
-
enum em28xx_int_audio_type {
EM28XX_INT_AUDIO_NONE = 0,
EM28XX_INT_AUDIO_AC97,
@@ -627,8 +616,6 @@ struct em28xx {
u32 i2s_speed; /* I2S speed for audio digital stream */
- struct em28xx_audio_mode audio_mode;
-
int tuner_type; /* type of the tuner */
/* i2c i/o */
--
1.8.4.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 4/4] em28xx: get rid of structs em28xx_ac97_mode and em28xx_audio_mode
2014-09-13 8:52 ` [PATCH 4/4] em28xx: get rid of structs em28xx_ac97_mode and em28xx_audio_mode Frank Schäfer
@ 2014-09-23 0:02 ` Mauro Carvalho Chehab
2014-09-23 18:33 ` Frank Schäfer
0 siblings, 1 reply; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2014-09-23 0:02 UTC (permalink / raw)
To: Frank Schäfer; +Cc: linux-media
Em Sat, 13 Sep 2014 10:52:22 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> Now that we have enum em28xx_int_audio (none/i2s/ac97), it is no longer
> necessary to check dev->audio_mode.ac97 to determine the type of internal audio connection.
> There is also no need to save the type of the detected AC97 chip.
Removing the AC97 chip is a bad idea, as the mux of each AC97 device
is different.
I don't remember anymore what device comes with the sigmatel chips,
but it does have a different mixer than em202. The logic to set it
different is not there basically for two reasons:
1) I don't have the device with sigmatel (I think someone borrowed it to me
at that time, and for a very limited period of time);
2) there's a hole ac97 support at /sound. The idea is to re-use it instead
of reinventing the wheel, but that requires time and a few different AC97
setups.
Btw, there are other em28xx devices with other non-em202 AC97 chips out
there, with different settings (and even different sampling rates).
Those AC97 devices are generally found at the "grabber" devices.
Regards,
Mauro
>
> So replce the remaining checks of dev->audio_mode.ac97 with equivalent checks
> of dev->int_audio_type and get rid of struct em28xx_ac97_mode and finally the
> whole struct em28xx_audio_mode.
>
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
> drivers/media/usb/em28xx/em28xx-audio.c | 2 +-
> drivers/media/usb/em28xx/em28xx-core.c | 36 ++++++---------------------------
> drivers/media/usb/em28xx/em28xx-video.c | 2 +-
> drivers/media/usb/em28xx/em28xx.h | 13 ------------
> 4 files changed, 8 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
> index 90c7a83..c3a4224 100644
> --- a/drivers/media/usb/em28xx/em28xx-audio.c
> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
> @@ -933,7 +933,7 @@ static int em28xx_audio_init(struct em28xx *dev)
>
> INIT_WORK(&adev->wq_trigger, audio_trigger);
>
> - if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
> + if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
> em28xx_cvol_new(card, dev, "Video", AC97_VIDEO);
> em28xx_cvol_new(card, dev, "Line In", AC97_LINE);
> em28xx_cvol_new(card, dev, "Phone", AC97_PHONE);
> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
> index ed83e4e..7464e70 100644
> --- a/drivers/media/usb/em28xx/em28xx-core.c
> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> @@ -405,12 +405,8 @@ static int em28xx_set_audio_source(struct em28xx *dev)
> return ret;
> msleep(5);
>
> - switch (dev->audio_mode.ac97) {
> - case EM28XX_NO_AC97:
> - break;
> - default:
> + if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97)
> ret = set_ac97_input(dev);
> - }
>
> return ret;
> }
> @@ -439,7 +435,7 @@ int em28xx_audio_analog_set(struct em28xx *dev)
> /* It is assumed that all devices use master volume for output.
> It would be possible to use also line output.
> */
> - if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
> + if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
> /* Mute all outputs */
> for (i = 0; i < ARRAY_SIZE(outputs); i++) {
> ret = em28xx_write_ac97(dev, outputs[i].reg, 0x8000);
> @@ -462,7 +458,7 @@ int em28xx_audio_analog_set(struct em28xx *dev)
> ret = em28xx_set_audio_source(dev);
>
> /* Sets volume */
> - if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
> + if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
> int vol;
>
> em28xx_write_ac97(dev, AC97_POWERDOWN, 0x4200);
> @@ -544,14 +540,11 @@ int em28xx_audio_setup(struct em28xx *dev)
> em28xx_info("I2S Audio (%d sample rate(s))\n",
> i2s_samplerates);
> /* Skip the code that does AC97 vendor detection */
> - dev->audio_mode.ac97 = EM28XX_NO_AC97;
> goto init_audio;
> } else {
> dev->int_audio_type = EM28XX_INT_AUDIO_AC97;
> }
>
> - dev->audio_mode.ac97 = EM28XX_AC97_OTHER;
> -
> vid1 = em28xx_read_ac97(dev, AC97_VENDOR_ID1);
> if (vid1 < 0) {
> /*
> @@ -560,7 +553,6 @@ int em28xx_audio_setup(struct em28xx *dev)
> * CHIPCFG register, even not having an AC97 chip
> */
> em28xx_warn("AC97 chip type couldn't be determined\n");
> - dev->audio_mode.ac97 = EM28XX_NO_AC97;
> if (dev->usb_audio_type == EM28XX_USB_AUDIO_VENDOR)
> dev->usb_audio_type = EM28XX_USB_AUDIO_NONE;
> dev->int_audio_type = EM28XX_INT_AUDIO_NONE;
> @@ -582,30 +574,14 @@ int em28xx_audio_setup(struct em28xx *dev)
>
> /* Try to identify what audio processor we have */
> if (((vid == 0xffffffff) || (vid == 0x83847650)) && (feat == 0x6a90))
> - dev->audio_mode.ac97 = EM28XX_AC97_EM202;
> - else if ((vid >> 8) == 0x838476)
> - dev->audio_mode.ac97 = EM28XX_AC97_SIGMATEL;
> -
> -init_audio:
> - /* Reports detected AC97 processor */
> - switch (dev->audio_mode.ac97) {
> - case EM28XX_NO_AC97:
> - em28xx_info("No AC97 audio processor\n");
> - break;
> - case EM28XX_AC97_EM202:
> em28xx_info("Empia 202 AC97 audio processor detected\n");
> - break;
> - case EM28XX_AC97_SIGMATEL:
> + else if ((vid >> 8) == 0x838476)
> em28xx_info("Sigmatel audio processor detected (stac 97%02x)\n",
> vid & 0xff);
> - break;
> - case EM28XX_AC97_OTHER:
> + else
> em28xx_warn("Unknown AC97 audio processor detected!\n");
> - break;
> - default:
> - break;
> - }
>
> +init_audio:
> return em28xx_audio_analog_set(dev);
> }
> EXPORT_SYMBOL_GPL(em28xx_audio_setup);
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 3284de9..c4e1364 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -2384,7 +2384,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> __func__, ret);
> goto unregister_dev;
> }
> - if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
> + if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
> v4l2_ctrl_new_std(hdl, &em28xx_ctrl_ops,
> V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
> v4l2_ctrl_new_std(hdl, &em28xx_ctrl_ops,
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index 857ad0c..3108eee 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -300,17 +300,6 @@ enum enum28xx_itype {
> EM28XX_RADIO,
> };
>
> -enum em28xx_ac97_mode {
> - EM28XX_NO_AC97 = 0,
> - EM28XX_AC97_EM202,
> - EM28XX_AC97_SIGMATEL,
> - EM28XX_AC97_OTHER,
> -};
> -
> -struct em28xx_audio_mode {
> - enum em28xx_ac97_mode ac97;
> -};
> -
> enum em28xx_int_audio_type {
> EM28XX_INT_AUDIO_NONE = 0,
> EM28XX_INT_AUDIO_AC97,
> @@ -627,8 +616,6 @@ struct em28xx {
>
> u32 i2s_speed; /* I2S speed for audio digital stream */
>
> - struct em28xx_audio_mode audio_mode;
> -
> int tuner_type; /* type of the tuner */
>
> /* i2c i/o */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 4/4] em28xx: get rid of structs em28xx_ac97_mode and em28xx_audio_mode
2014-09-23 0:02 ` Mauro Carvalho Chehab
@ 2014-09-23 18:33 ` Frank Schäfer
0 siblings, 0 replies; 6+ messages in thread
From: Frank Schäfer @ 2014-09-23 18:33 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media
Am 23.09.2014 um 02:02 schrieb Mauro Carvalho Chehab:
> Em Sat, 13 Sep 2014 10:52:22 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Now that we have enum em28xx_int_audio (none/i2s/ac97), it is no longer
>> necessary to check dev->audio_mode.ac97 to determine the type of internal audio connection.
>> There is also no need to save the type of the detected AC97 chip.
> Removing the AC97 chip is a bad idea, as the mux of each AC97 device
> is different.
>
> I don't remember anymore what device comes with the sigmatel chips,
> but it does have a different mixer than em202. The logic to set it
> different is not there basically for two reasons:
>
> 1) I don't have the device with sigmatel (I think someone borrowed it to me
> at that time, and for a very limited period of time);
>
> 2) there's a hole ac97 support at /sound. The idea is to re-use it instead
> of reinventing the wheel, but that requires time and a few different AC97
> setups.
1.) WHICH devices to you think need AC97 chip type specific code ?
2.) WHEN are you going to implement it ?
It's just a big dead ugly chunk of code, which causes confusion.
So please, let's simplify the code.
If we really need to distinguish between AC97 chips one day, we can
reintroduce it again.
Please note that I'm not removing the AC97 chip type detection, so we
are not loosing any information that might be useful in the future.
> Btw, there are other em28xx devices with other non-em202 AC97 chips out
> there, with different settings (and even different sampling rates).
> Those AC97 devices are generally found at the "grabber" devices.
I know. The VAD Laplace webcam is one of them. And the current audio
code is broken for it.
But the reason for that isn't that we do not distinguish enough between
AC97 chips.
The reason for that is, that we touch AC97 _at_all_ for USB audio class
devices, which is evil, unnecessary and therefore also not done by the
Windows driver.
The situation is of course different for devices with usb vendor class
audio (em28xx-alsa).
But that's a separate issue which has nothing to do with this patch... ;-)
Regards,
Frank
>
> Regards,
> Mauro
>
>> So replce the remaining checks of dev->audio_mode.ac97 with equivalent checks
>> of dev->int_audio_type and get rid of struct em28xx_ac97_mode and finally the
>> whole struct em28xx_audio_mode.
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>> drivers/media/usb/em28xx/em28xx-audio.c | 2 +-
>> drivers/media/usb/em28xx/em28xx-core.c | 36 ++++++---------------------------
>> drivers/media/usb/em28xx/em28xx-video.c | 2 +-
>> drivers/media/usb/em28xx/em28xx.h | 13 ------------
>> 4 files changed, 8 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
>> index 90c7a83..c3a4224 100644
>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
>> @@ -933,7 +933,7 @@ static int em28xx_audio_init(struct em28xx *dev)
>>
>> INIT_WORK(&adev->wq_trigger, audio_trigger);
>>
>> - if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
>> + if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
>> em28xx_cvol_new(card, dev, "Video", AC97_VIDEO);
>> em28xx_cvol_new(card, dev, "Line In", AC97_LINE);
>> em28xx_cvol_new(card, dev, "Phone", AC97_PHONE);
>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
>> index ed83e4e..7464e70 100644
>> --- a/drivers/media/usb/em28xx/em28xx-core.c
>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
>> @@ -405,12 +405,8 @@ static int em28xx_set_audio_source(struct em28xx *dev)
>> return ret;
>> msleep(5);
>>
>> - switch (dev->audio_mode.ac97) {
>> - case EM28XX_NO_AC97:
>> - break;
>> - default:
>> + if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97)
>> ret = set_ac97_input(dev);
>> - }
>>
>> return ret;
>> }
>> @@ -439,7 +435,7 @@ int em28xx_audio_analog_set(struct em28xx *dev)
>> /* It is assumed that all devices use master volume for output.
>> It would be possible to use also line output.
>> */
>> - if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
>> + if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
>> /* Mute all outputs */
>> for (i = 0; i < ARRAY_SIZE(outputs); i++) {
>> ret = em28xx_write_ac97(dev, outputs[i].reg, 0x8000);
>> @@ -462,7 +458,7 @@ int em28xx_audio_analog_set(struct em28xx *dev)
>> ret = em28xx_set_audio_source(dev);
>>
>> /* Sets volume */
>> - if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
>> + if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
>> int vol;
>>
>> em28xx_write_ac97(dev, AC97_POWERDOWN, 0x4200);
>> @@ -544,14 +540,11 @@ int em28xx_audio_setup(struct em28xx *dev)
>> em28xx_info("I2S Audio (%d sample rate(s))\n",
>> i2s_samplerates);
>> /* Skip the code that does AC97 vendor detection */
>> - dev->audio_mode.ac97 = EM28XX_NO_AC97;
>> goto init_audio;
>> } else {
>> dev->int_audio_type = EM28XX_INT_AUDIO_AC97;
>> }
>>
>> - dev->audio_mode.ac97 = EM28XX_AC97_OTHER;
>> -
>> vid1 = em28xx_read_ac97(dev, AC97_VENDOR_ID1);
>> if (vid1 < 0) {
>> /*
>> @@ -560,7 +553,6 @@ int em28xx_audio_setup(struct em28xx *dev)
>> * CHIPCFG register, even not having an AC97 chip
>> */
>> em28xx_warn("AC97 chip type couldn't be determined\n");
>> - dev->audio_mode.ac97 = EM28XX_NO_AC97;
>> if (dev->usb_audio_type == EM28XX_USB_AUDIO_VENDOR)
>> dev->usb_audio_type = EM28XX_USB_AUDIO_NONE;
>> dev->int_audio_type = EM28XX_INT_AUDIO_NONE;
>> @@ -582,30 +574,14 @@ int em28xx_audio_setup(struct em28xx *dev)
>>
>> /* Try to identify what audio processor we have */
>> if (((vid == 0xffffffff) || (vid == 0x83847650)) && (feat == 0x6a90))
>> - dev->audio_mode.ac97 = EM28XX_AC97_EM202;
>> - else if ((vid >> 8) == 0x838476)
>> - dev->audio_mode.ac97 = EM28XX_AC97_SIGMATEL;
>> -
>> -init_audio:
>> - /* Reports detected AC97 processor */
>> - switch (dev->audio_mode.ac97) {
>> - case EM28XX_NO_AC97:
>> - em28xx_info("No AC97 audio processor\n");
>> - break;
>> - case EM28XX_AC97_EM202:
>> em28xx_info("Empia 202 AC97 audio processor detected\n");
>> - break;
>> - case EM28XX_AC97_SIGMATEL:
>> + else if ((vid >> 8) == 0x838476)
>> em28xx_info("Sigmatel audio processor detected (stac 97%02x)\n",
>> vid & 0xff);
>> - break;
>> - case EM28XX_AC97_OTHER:
>> + else
>> em28xx_warn("Unknown AC97 audio processor detected!\n");
>> - break;
>> - default:
>> - break;
>> - }
>>
>> +init_audio:
>> return em28xx_audio_analog_set(dev);
>> }
>> EXPORT_SYMBOL_GPL(em28xx_audio_setup);
>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
>> index 3284de9..c4e1364 100644
>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>> @@ -2384,7 +2384,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> __func__, ret);
>> goto unregister_dev;
>> }
>> - if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
>> + if (dev->int_audio_type == EM28XX_INT_AUDIO_AC97) {
>> v4l2_ctrl_new_std(hdl, &em28xx_ctrl_ops,
>> V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
>> v4l2_ctrl_new_std(hdl, &em28xx_ctrl_ops,
>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>> index 857ad0c..3108eee 100644
>> --- a/drivers/media/usb/em28xx/em28xx.h
>> +++ b/drivers/media/usb/em28xx/em28xx.h
>> @@ -300,17 +300,6 @@ enum enum28xx_itype {
>> EM28XX_RADIO,
>> };
>>
>> -enum em28xx_ac97_mode {
>> - EM28XX_NO_AC97 = 0,
>> - EM28XX_AC97_EM202,
>> - EM28XX_AC97_SIGMATEL,
>> - EM28XX_AC97_OTHER,
>> -};
>> -
>> -struct em28xx_audio_mode {
>> - enum em28xx_ac97_mode ac97;
>> -};
>> -
>> enum em28xx_int_audio_type {
>> EM28XX_INT_AUDIO_NONE = 0,
>> EM28XX_INT_AUDIO_AC97,
>> @@ -627,8 +616,6 @@ struct em28xx {
>>
>> u32 i2s_speed; /* I2S speed for audio digital stream */
>>
>> - struct em28xx_audio_mode audio_mode;
>> -
>> int tuner_type; /* type of the tuner */
>>
>> /* i2c i/o */
^ permalink raw reply [flat|nested] 6+ messages in thread