On Mon, 24 Jul 2017 22:49:18 +0200, Robert Jarzmik wrote:
AC97 is a bus for sound usage. It enables for a AC97 AC-Link to link one controller to 0 to 4 AC97 codecs.
The goal of this new implementation is to implement a device/driver model for AC97, with an automatic scan of the bus and automatic discovery of AC97 codec devices.
Signed-off-by: Robert Jarzmik robert.jarzmik@free.fr
Since RFCv1:
Takashi's review
- changed the codec.h guard ... a better name could be found ...
- added the AC97_* macros missing parenthesis
- constantified the id_table in the codec driver structure
- changed the 4 codecs linked list into an array
- enabled the ac97 bus to be a module
- added a slots_available to snd_ac97_controller_register() to have a way to prevent scanning and probing of unconnected codecs
- removed useless ac97 bus index
- all exported functions begin with snd_ac97_*()
- change bus operations to controller+slot parameters instead of codec device
Mark's review
- changed ac97_digital_controller into ac97_controller
- rename ac97_digital_controller_*() into ac97_controller_*()
- add the ac97 ac-link clock to the codec device (ie. the AC'97 BIT_CLK)
Since RFCv2:
- more snd_ac97 namespace review
- change the compat allocation prototype to force the user to provide and ac97_codec_device structure pointer
Since v1:
- took into account all Lars comments
Since v3:
- took into account Takashi's comments (ac97bus naming and module_exit)
It looks mostly OK, but some nitpicking:
diff --git a/include/sound/ac97/compat.h b/include/sound/ac97/compat.h new file mode 100644 index 000000000000..d876464bf7e4 --- /dev/null +++ b/include/sound/ac97/compat.h @@ -0,0 +1,21 @@ +/*
- Copyright (C) 2016 Robert Jarzmik robert.jarzmik@free.fr
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- This file is for backward compatibility with snd_ac97 structure and its
- multiple usages, such as the snd_ac97_bus and snd_ac97_build_ops.
- */
+#ifndef AC97_COMPAT_H +#define AC97_COMPAT_H
+#include <sound/ac97_codec.h> +#include <sound/soc.h>
Is this inclusion needed? The code here doesn't look ASoC-specific at all.
--- /dev/null +++ b/include/sound/ac97/controller.h @@ -0,0 +1,84 @@ +/*
- Copyright (C) 2016 Robert Jarzmik robert.jarzmik@free.fr
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- */
+#ifndef AC97_CONTROLLER_H +#define AC97_CONTROLLER_H
+#include <linux/list.h>
+#define AC97_BUS_MAX_CODECS 4 +#define AC97_SLOTS_AVAILABLE_ALL 0xf
+struct device;
You need the definition struct device in below (it's no pointer), thus you have to include <linux/device.h> instead.
+/**
- struct ac97_controller - The AC97 controller of the AC-Link
- @ops: the AC97 operations.
- @controllers: linked list of all existing controllers.
- @adap: the shell device ac97-%d, ie. ac97 adapter
- @nr: the number of the shell device
- @parent: the device providing the AC97 controller.
- @slots_available: the mask of accessible/scanable codecs.
- @codecs: the 4 possible AC97 codecs (NULL if none found).
- @codecs_pdata: platform_data for each codec (NULL if no pdata).
- This structure is internal to AC97 bus, and should not be used by the
- controllers themselves, excepting for using @dev.
- */
+struct ac97_controller {
- const struct ac97_controller_ops *ops;
The struct isn't declared beforehand? GCC will warn.
- struct list_head controllers;
- struct device adap;
- int nr;
- struct device *parent;
- unsigned short slots_available;
I'd move parent field below, so that 64bit pointer can be aligned better.
+static int ac97_codec_add(struct ac97_controller *ac97_ctrl, int idx,
unsigned int vendor_id)+{
- struct ac97_codec_device *codec;
- int ret;
- codec = kzalloc(sizeof(*codec), GFP_KERNEL);
- if (!codec)
return -ENOMEM;- ac97_ctrl->codecs[idx] = codec;
- codec->vendor_id = vendor_id;
- codec->dev.release = ac97_codec_release;
- codec->dev.bus = &ac97_bus_type;
- codec->dev.parent = &ac97_ctrl->adap;
- codec->num = idx;
- codec->ac97_ctrl = ac97_ctrl;
- device_initialize(&codec->dev);
- dev_set_name(&codec->dev, "%s:%u", dev_name(ac97_ctrl->parent), idx);
- ret = device_add(&codec->dev);
- if (ret)
goto err_free_codec;- return 0;
+err_free_codec:
- kfree(codec);
This may leave the device name string. You need to call put_device() even if device_add() returns an error.
- ac97_ctrl->codecs[idx] = NULL;
- return ret;
+}
+unsigned int snd_ac97_bus_scan_one(struct ac97_controller *adrv,
unsigned int codec_num)+{
- unsigned short vid1, vid2;
- int ret;
- ret = adrv->ops->read(adrv, codec_num, AC97_VENDOR_ID1);
- vid1 = (ret & 0xffff);
- if (ret < 0)
return 0;- ret = adrv->ops->read(adrv, codec_num, AC97_VENDOR_ID2);
- vid2 = (ret & 0xffff);
- if (ret < 0)
return 0;- dev_dbg(&adrv->adap, "%s(codec_num=%u): vendor_id=0x%08x\n",
__func__, codec_num, AC97_ID(vid1, vid2));- return AC97_ID(vid1, vid2);
+}
+static int ac97_bus_scan(struct ac97_controller *ac97_ctrl) +{
- int ret, i;
- unsigned int vendor_id;
- for (i = 0; i < AC97_BUS_MAX_CODECS; i++) {
if (ac97_codec_find(ac97_ctrl, i))continue;if (!(ac97_ctrl->slots_available & BIT(i)))continue;vendor_id = snd_ac97_bus_scan_one(ac97_ctrl, i);if (!vendor_id)continue;ret = ac97_codec_add(ac97_ctrl, i, vendor_id);if (ret < 0)return ret;- }
- return 0;
+}
+static int ac97_bus_reset(struct ac97_controller *ac97_ctrl) +{
- ac97_ctrl->ops->reset(ac97_ctrl);
- return 0;
+}
+/**
- snd_ac97_codec_driver_register - register an AC97 codec driver
- @dev: AC97 driver codec to register
- Register an AC97 codec driver to the ac97 bus driver, aka. the AC97 digital
- controller.
- Returns 0 on success or error code
- */
+int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv) +{
- int ret;
- drv->driver.bus = &ac97_bus_type;
- ret = driver_register(&drv->driver);
- return ret;
This can be simplified.
+} +EXPORT_SYMBOL(snd_ac97_codec_driver_register);
No GPL? (Ditto for other entries, too)
+int snd_ac97_reset(struct snd_ac97 *ac97, bool try_warm, unsigned int id,
- unsigned int id_mask)
+{
- struct ac97_codec_device *adev = to_ac97_device(ac97->private_data);
- struct ac97_controller *actrl = adev->ac97_ctrl;
- if (try_warm) {
compat_ac97_warm_reset(ac97);if (snd_ac97_bus_scan_one(actrl, adev->num) == adev->vendor_id)
Can we ignore id_mask here? I'm not quite sure whether it's fixed...
thanks,
Takashi