| Message ID | 1462050939-27940-3-git-send-email-robert.jarzmik@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Apr 30, 2016 at 11:15:34PM +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. I think this is basically what I was thinking of, yes. One thing we'll need to do is work out how to handle systems that need some explicit action to start clocks for the bus clock, though they were never very common and it's entirely possible nobody cares any more so perhaps we just punt and see if anyone notices for now. > +int ac97_digital_controller_register(const struct ac97_controller_ops *ops, > + struct device *dev); > +int ac97_digital_controller_unregister(const struct device *dev); Why "digital"?
Mark Brown <broonie@kernel.org> writes: > On Sat, Apr 30, 2016 at 11:15:34PM +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. > > I think this is basically what I was thinking of, yes. One thing we'll > need to do is work out how to handle systems that need some explicit > action to start clocks for the bus clock, though they were never very > common and it's entirely possible nobody cares any more so perhaps we > just punt and see if anyone notices for now. You probably mean the BITCLK clock. What is a bit pesky about this clock is that it can either be mastered by digital controller and the codec is a slave, or the other way around. So we had either the BITCLK provided by : - the controller => this could be passed in ac97_digital_controller_register() - the codec => this is trouble, I don't really know how to handle this case If the bus code has this clock, it can indeed prepare and enable it. >> +int ac97_digital_controller_register(const struct ac97_controller_ops *ops, >> + struct device *dev); >> +int ac97_digital_controller_unregister(const struct device *dev); > > Why "digital"? I copy-pasted this from Audio Codec '97 Revision 2.3, where in several places they call the controller a "digital controller". Quoting chapter 1.4: The digital link that connects the AC ‘97 Digital Controller to the AC ‘97 Codec, referred to as AC-link, is a bi- directional, 5-wire, serial time domain multiplexed (TDM) format interface. AC-link supports connections between a single Controller and up to 4 CODECs on a circuit board and/or riser card. Now if you prefer "ac97_controller" or something like that, that's as you wish, the name does not matter that much to me ;) Cheers.
On Tue, May 03, 2016 at 09:43:20PM +0200, Robert Jarzmik wrote: > You probably mean the BITCLK clock. > What is a bit pesky about this clock is that it can either be mastered by > digital controller and the codec is a slave, or the other way around. That's a bit surprising - I've never encountered a system that impelemnts this, it may be permitted by the spec but it's always the CODEC. The master clock from the CODEC is often provided by the SoC but I've not seen systems where anything other than the CODEC drives the actual AC'97 bus. > >> +int ac97_digital_controller_register(const struct ac97_controller_ops *ops, > >> + struct device *dev); > >> +int ac97_digital_controller_unregister(const struct device *dev); > > Why "digital"? > I copy-pasted this from Audio Codec '97 Revision 2.3, where in several places > they call the controller a "digital controller". It's not really adding anything though, it's just clumsy wording on their part - it's not like we need to distinguish this from analogue or any other type of AC'97 controllers. > Now if you prefer "ac97_controller" or something like that, that's as you wish, > the name does not matter that much to me ;) Yes.
Mark Brown <broonie@kernel.org> writes: > On Tue, May 03, 2016 at 09:43:20PM +0200, Robert Jarzmik wrote: > >> You probably mean the BITCLK clock. > >> What is a bit pesky about this clock is that it can either be mastered by >> digital controller and the codec is a slave, or the other way around. > > That's a bit surprising - I've never encountered a system that > impelemnts this, it may be permitted by the spec but it's always the > CODEC. The master clock from the CODEC is often provided by the SoC but > I've not seen systems where anything other than the CODEC drives the > actual AC'97 bus. Ok, so let's assume a one direction. I'll have a look how I could add this clock to the probe() exactly as amba_probe() does. > It's not really adding anything though, it's just clumsy wording on > their part - it's not like we need to distinguish this from analogue > or any other type of AC'97 controllers. > >> Now if you prefer "ac97_controller" or something like that, that's as you wish, >> the name does not matter that much to me ;) > > Yes. Okay, let me add this to my todo list for v2. I already have suspend/resume/pm_suspend/pm_resume. Cheers.
On Sat, 30 Apr 2016 23:15:34 +0200, Robert Jarzmik wrote: > > diff --git a/include/sound/ac97/codec.h b/include/sound/ac97/codec.h > new file mode 100644 > index 000000000000..4b8b3e570892 > --- /dev/null > +++ b/include/sound/ac97/codec.h > @@ -0,0 +1,98 @@ > +/* > + * 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_CODEC_H > +#define AC97_CODEC_H Let's be careful about the choice of the guard. > + > +#include <linux/device.h> > + > +#define AC97_ID(vendor_id1, vendor_id2) \ > + (((vendor_id1 & 0xffff) << 16) | (vendor_id2 & 0xffff)) > +#define AC97_DRIVER_ID(vendor_id1, vendor_id2, mask_id1, mask_id2, _data) \ > + { .id = ((vendor_id1 & 0xffff) << 16) | (vendor_id2 & 0xffff), \ > + .mask = ((mask_id1 & 0xffff) << 16) | (mask_id2 & 0xffff), \ > + .data = _data } Give parentheses around the macro arguments. > + > +#define to_ac97_device(d) container_of(d, struct ac97_codec_device, dev) > +#define to_ac97_driver(d) container_of(d, struct ac97_codec_driver, driver) > + > +struct ac97_controller; > + > +/** > + * struct ac97_id - matches a codec device and driver on an ac97 bus > + * @id: The significant bits if the codec vendor ID1 and ID2 > + * @mask: Bitmask specifying which bits of the id field are significant when > + * matching. A driver binds to a device when : > + * ((vendorID1 << 8 | vendorID2) & (mask_id1 << 8 | mask_id2)) == id. > + * @data: Private data used by the driver. > + */ > +struct ac97_id { > + unsigned int id; > + unsigned int mask; > + void *data; > +}; > + > +/** > + * ac97_codec_device - a ac97 codec > + * @dev: the code device > + * @vendor_id: the vendor_id of the codec, as sensed on the AC-link > + * @num: the codec number, 0 is primary, 1 is first slave, etc ... > + * @ac97_ctrl: ac97 digital controller on the same AC-link > + * > + * This is the device instanciated for each codec living on a AC-link. There are > + * normally 0 to 4 codec devices per AC-link, and all of them are controlled by > + * an AC97 digital controller. > + */ > +struct ac97_codec_device { > + struct device dev; /* Must stay first member */ This doesn't have to be the first element as long as you use container_of(). > + unsigned int vendor_id; > + unsigned int num; > + struct list_head list; > + struct ac97_controller *ac97_ctrl; > +}; > + > +/** > + * ac97_codec_driver - a ac97 codec driver > + * @driver: the device driver structure > + * @probe: the function called when a ac97_codec_device is matched > + * @remove: the function called when the device is unbound/removed > + * @suspend: suspend function (might be NULL) > + * @resume: resume function (might be NULL) > + * @shutdown: shutdown function (might be NULL) > + * @id_table: ac97 vendor_id match table, { } member terminated > + */ > +struct ac97_codec_driver { > + struct device_driver driver; > + int (*probe)(struct ac97_codec_device *); > + int (*remove)(struct ac97_codec_device *); > + int (*suspend)(struct ac97_codec_device *); > + int (*resume)(struct ac97_codec_device *); > + void (*shutdown)(struct ac97_codec_device *); > + struct ac97_id *id_table; Missing const? > +}; > + > +int ac97_codec_driver_register(struct ac97_codec_driver *drv); > +void ac97_codec_driver_unregister(struct ac97_codec_driver *drv); > + > +static inline struct device * > +ac97_codec_dev2dev(const struct ac97_codec_device *adev) > +{ > + return (struct device *)(adev); What's wrong with the simple &adev->dev ? Cast looks scary. > +} > + > +static inline void *ac97_get_drvdata(const struct ac97_codec_device *adev) > +{ > + return dev_get_drvdata(ac97_codec_dev2dev(adev)); > +} > + > +static inline void ac97_set_drvdata(const struct ac97_codec_device *adev, > + void *data) > +{ > + dev_set_drvdata(ac97_codec_dev2dev(adev), data); > +} > + > +#endif > diff --git a/include/sound/ac97/compat.h b/include/sound/ac97/compat.h > new file mode 100644 > index 000000000000..bf611f572f2d > --- /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> > + > +struct snd_ac97 *compat_alloc_snd_ac97_codec(struct snd_soc_codec *codec); > +void compat_release_snd_ac97_codec(struct snd_ac97 *ac97); > + > +#endif > diff --git a/include/sound/ac97/controller.h b/include/sound/ac97/controller.h > new file mode 100644 > index 000000000000..f1e5e645f5ef > --- /dev/null > +++ b/include/sound/ac97/controller.h > @@ -0,0 +1,39 @@ > +/* > + * 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> > + > +struct device; > +struct ac97_codec_device; > + > +struct ac97_controller_ops { > + void (*reset)(struct ac97_codec_device *ac97); > + void (*warm_reset)(struct ac97_codec_device *ac97); > + int (*write)(struct ac97_codec_device *ac97, unsigned short reg, > + unsigned short val); > + int (*read)(struct ac97_codec_device *ac97, unsigned short reg); > + void (*wait)(struct ac97_codec_device *ac97); > + void (*init)(struct ac97_codec_device *ac97); > +}; > + > +struct ac97_controller { > + const struct ac97_controller_ops *ops; > + struct list_head controllers; > + struct device *dev; > + int bus_idx; What is this bus_idx for? > + int bound_codecs; > + struct list_head codecs; > +}; > + > +int ac97_digital_controller_register(const struct ac97_controller_ops *ops, > + struct device *dev); > +int ac97_digital_controller_unregister(const struct device *dev); > + > +#endif > diff --git a/sound/ac97/Kconfig b/sound/ac97/Kconfig > new file mode 100644 > index 000000000000..fd2c2d031e62 > --- /dev/null > +++ b/sound/ac97/Kconfig > @@ -0,0 +1,9 @@ > +# > +# PCI configuration > +# Still only for PCI? :) > + > +config AC97 > + bool "AC97 bus" > + help > + Say Y here if you want to have AC97 devices, which are sound oriented > + devices around an AC-Link. > diff --git a/sound/ac97/Makefile b/sound/ac97/Makefile > new file mode 100644 > index 000000000000..5575909d46e2 > --- /dev/null > +++ b/sound/ac97/Makefile > @@ -0,0 +1,5 @@ > +# > +# make for AC97 bus drivers > +# > + > +obj-y += bus.o codec.o snd_ac97_compat.o No possibility for modules? > diff --git a/sound/ac97/ac97_core.h b/sound/ac97/ac97_core.h > new file mode 100644 > index 000000000000..db6e27288357 > --- /dev/null > +++ b/sound/ac97/ac97_core.h > @@ -0,0 +1,10 @@ > +/* > + * 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. > + */ > + > +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97, > + int codec_num); > diff --git a/sound/ac97/bus.c b/sound/ac97/bus.c > new file mode 100644 > index 000000000000..f9bf5632d4aa > --- /dev/null > +++ b/sound/ac97/bus.c > @@ -0,0 +1,330 @@ > +/* > + * 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. > + */ > + > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/list.h> > +#include <linux/mutex.h> > +#include <linux/pm.h> > +#include <linux/slab.h> > +#include <sound/ac97/codec.h> > +#include <sound/ac97/controller.h> > +#include <sound/ac97/regs.h> > + > +#define AC97_BUS_MAX_CODECS 4 > + > +/* > + * Protects ac97_controllers and each ac97_controller structure. > + */ > +static DEFINE_MUTEX(ac97_controllers_mutex); > +static LIST_HEAD(ac97_controllers); > +static int ac97_bus_idx; > + > +struct bus_type ac97_bus_type; > + > +static struct ac97_codec_device * > +ac97_codec_find(struct ac97_controller *ac97_ctrl, int codec_num) > +{ > + struct ac97_codec_device *codec; > + > + list_for_each_entry(codec, &ac97_ctrl->codecs, list) > + if (codec->num == codec_num) > + return codec; > + > + return NULL; > +} It's a question whether we need to manage the codecs in the linked list. There can be at most 4 codecs, so it fits in an array well, too. Then some codes like this would be simpler. (And it'll even reduce the footprint, too.) > + > +static void ac97_codec_release(struct device *dev) > +{ > + struct ac97_codec_device *codec; > + > + codec = container_of(dev, struct ac97_codec_device, dev); > + list_del(&codec->list); > + kfree(codec); > +} > + > +static int ac97_codec_add(struct ac97_controller *ac97_ctrl, int idx, > + unsigned int vendor_id) > +{ > + struct ac97_codec_device *codec; > + char *codec_name; > + int ret; > + > + codec = kzalloc(sizeof(*codec), GFP_KERNEL); > + if (!codec) > + return -ENOMEM; > + > + codec->vendor_id = vendor_id; > + codec->dev.release = ac97_codec_release; > + codec->dev.bus = &ac97_bus_type; > + codec->dev.parent = ac97_ctrl->dev; > + codec->num = idx; > + codec->ac97_ctrl = ac97_ctrl; > + INIT_LIST_HEAD(&codec->list); > + list_move_tail(&codec->list, &ac97_ctrl->codecs); > + > + codec_name = kasprintf(GFP_KERNEL, "%s:%d", dev_name(ac97_ctrl->dev), > + idx); > + codec->dev.init_name = codec_name; > + > + ret = device_register(&codec->dev); > + kfree(codec_name); > + > + return ret; > +} > + > +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97, > + int codec_num) > +{ > + struct ac97_codec_device codec; > + unsigned short vid1, vid2; > + int ret; > + > + codec.dev = *ac97->dev; > + codec.num = codec_num; > + ret = ac97->ops->read(&codec, AC97_VENDOR_ID1); > + vid1 = (ret & 0xffff); > + if (ret < 0) > + return 0; Hmm. This looks pretty hackish and dangerous. > + ret = ac97->ops->read(&codec, AC97_VENDOR_ID2); > + vid2 = (ret & 0xffff); > + if (ret < 0) > + return 0; > + > + dev_dbg(&codec.dev, "%s(codec_num=%d): 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; > + vendor_id = 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; This is one of concerns: we don't know whether the device really reacts well if you access to a non-existing slot. At least, it'd be safer to have the masks for the devices we already know the slots. > + } > + return 0; > +} > + > +void ac97_rescan_all_controllers(void) > +{ > + struct ac97_controller *ac97_ctrl; > + int ret; > + > + mutex_lock(&ac97_controllers_mutex); > + list_for_each_entry(ac97_ctrl, &ac97_controllers, controllers) { > + ret = ac97_bus_scan(ac97_ctrl); > + if (ret) > + dev_warn(ac97_ctrl->dev, "scan failed: %d\n", ret); > + } > + mutex_unlock(&ac97_controllers_mutex); > +} > +EXPORT_SYMBOL(ac97_rescan_all_controllers); > + > +static int ac97_bus_reset(struct ac97_controller *ac97_ctrl) > +{ > + struct ac97_codec_device codec; > + > + memset(&codec, 0, sizeof(codec)); > + codec.dev = *ac97_ctrl->dev; > + > + ac97_ctrl->ops->reset(&codec); So, this assumes that reset ops is mandatory? Then document it at least. thanks, Takashi > + return 0; > +} > + > +/** > + * 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 ac97_codec_driver_register(struct ac97_codec_driver *drv) > +{ > + int ret; > + > + drv->driver.bus = &ac97_bus_type; > + > + ret = driver_register(&drv->driver); > + if (!ret) > + ac97_rescan_all_controllers(); > + > + return ret; > +} > +EXPORT_SYMBOL(ac97_codec_driver_register); > + > +/** > + * ac97_codec_driver_unregister - unregister an AC97 codec driver > + * @dev: AC97 codec driver to unregister > + * > + * Unregister a previously registered ac97 codec driver. > + */ > +void ac97_codec_driver_unregister(struct ac97_codec_driver *drv) > +{ > + driver_unregister(&drv->driver); > +} > +EXPORT_SYMBOL(ac97_codec_driver_unregister); > + > +static int ac97_dc_codecs_unregister(struct ac97_controller *ac97_ctrl) > +{ > + struct ac97_codec_device *codec, *tmp; > + > + list_for_each_entry_safe(codec, tmp, &ac97_ctrl->codecs, list) > + put_device(&codec->dev); > + > + return 0; > +} > + > +/** > + * ac97_digital_controller_register - register an ac97 controller > + * @ops: the ac97 bus operations > + * @dev: the device providing the ac97 DC function > + * > + * Register a digital controller which can control up to 4 ac97 codecs. This is > + * the controller side of the AC97 AC-link, while the slave side are the codecs. > + * > + * Returns a positive bus index upon success, negative value upon error > + */ > +int ac97_digital_controller_register(const struct ac97_controller_ops *ops, > + struct device *dev) > +{ > + struct ac97_controller *ac97_ctrl; > + > + ac97_ctrl = kzalloc(sizeof(*ac97_ctrl), GFP_KERNEL); > + if (!ac97_ctrl) > + return -ENOMEM; > + > + mutex_lock(&ac97_controllers_mutex); > + ac97_ctrl->ops = ops; > + ac97_ctrl->bus_idx = ac97_bus_idx++; > + ac97_ctrl->dev = dev; > + INIT_LIST_HEAD(&ac97_ctrl->codecs); > + list_add(&ac97_ctrl->controllers, &ac97_controllers); > + mutex_unlock(&ac97_controllers_mutex); > + > + ac97_bus_reset(ac97_ctrl); > + ac97_bus_scan(ac97_ctrl); > + > + return ac97_ctrl->bus_idx; > +} > +EXPORT_SYMBOL(ac97_digital_controller_register); > + > +/** > + * ac97_digital_controller_unregister - unregister an ac97 controller > + * @dev: the device previously provided to ac97_digital_controller_register() > + * > + * Returns 0 on success, negative upon error > + */ > +int ac97_digital_controller_unregister(const struct device *dev) > +{ > + struct ac97_controller *ac97_ctrl, *tmp; > + int ret = -ENODEV; > + > + mutex_lock(&ac97_controllers_mutex); > + list_for_each_entry_safe(ac97_ctrl, tmp, &ac97_controllers, > + controllers) { > + if (ac97_ctrl->dev != dev) > + continue; > + if (ac97_ctrl->bound_codecs) > + ret = -EBUSY; > + else > + ret = ac97_dc_codecs_unregister(ac97_ctrl); > + if (!ret) > + list_del(&ac97_ctrl->controllers); > + } > + > + mutex_unlock(&ac97_controllers_mutex); > + return ret; > +} > +EXPORT_SYMBOL(ac97_digital_controller_unregister); > + > +static const struct dev_pm_ops ac97_pm = { > + .suspend = pm_generic_suspend, > + .resume = pm_generic_resume, > + .freeze = pm_generic_freeze, > + .thaw = pm_generic_thaw, > + .poweroff = pm_generic_poweroff, > + .restore = pm_generic_restore, > +}; > + > +static ssize_t vendor_id_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct ac97_codec_device *codec = to_ac97_device(dev); > + > + return sprintf(buf, "%08x", codec->vendor_id); > +} > + > +static struct device_attribute ac97_dev_attrs[] = { > + __ATTR_RO(vendor_id), > + __ATTR_NULL, > +}; > + > +int ac97_bus_match(struct device *dev, struct device_driver *drv) > +{ > + struct ac97_codec_device *adev = to_ac97_device(dev); > + struct ac97_codec_driver *adrv = to_ac97_driver(drv); > + struct ac97_id *id = adrv->id_table; > + > + if (adev->vendor_id == 0x0 || adev->vendor_id == 0xffffffff) > + return false; > + > + do { > + if ((id->id & id->mask) == (adev->vendor_id & id->mask)) > + return true; > + } while (++id->id); > + > + return false; > +} > + > +static int ac97_bus_probe(struct device *dev) > +{ > + struct ac97_codec_device *adev = to_ac97_device(dev); > + struct ac97_codec_driver *adrv = to_ac97_driver(dev->driver); > + > + return adrv->probe(adev); > +} > + > +static int ac97_bus_remove(struct device *dev) > +{ > + struct ac97_codec_device *adev = to_ac97_device(dev); > + struct ac97_codec_driver *adrv = to_ac97_driver(dev->driver); > + > + return adrv->remove(adev); > +} > + > +struct bus_type ac97_bus_type = { > + .name = "ac97", > + .dev_attrs = ac97_dev_attrs, > + .match = ac97_bus_match, > + .pm = &ac97_pm, > + .probe = ac97_bus_probe, > + .remove = ac97_bus_remove, > +}; > +EXPORT_SYMBOL(ac97_bus_type); > + > +static int __init ac97_bus_init(void) > +{ > + return bus_register(&ac97_bus_type); > +} > +subsys_initcall(ac97_bus_init); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Robert Jarzmik <robert.jarzmik@free.fr>"); > diff --git a/sound/ac97/codec.c b/sound/ac97/codec.c > new file mode 100644 > index 000000000000..a835f03744bf > --- /dev/null > +++ b/sound/ac97/codec.c > @@ -0,0 +1,15 @@ > +/* > + * 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. > + */ > + > +#include <sound/ac97_codec.h> > +#include <sound/ac97/codec.h> > +#include <sound/ac97/controller.h> > +#include <linux/device.h> > +#include <linux/slab.h> > +#include <sound/soc.h> /* For compat_ac97_* */ > + > diff --git a/sound/ac97/snd_ac97_compat.c b/sound/ac97/snd_ac97_compat.c > new file mode 100644 > index 000000000000..7e2f01c96fc9 > --- /dev/null > +++ b/sound/ac97/snd_ac97_compat.c > @@ -0,0 +1,104 @@ > +/* > + * 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. > + */ > + > +#include <linux/list.h> > +#include <linux/slab.h> > +#include <sound/ac97/codec.h> > +#include <sound/ac97/controller.h> > +#include <sound/soc.h> > + > +#include "ac97_core.h" > + > +static void compat_ac97_reset(struct snd_ac97 *ac97) > +{ > + struct ac97_codec_device *adev = to_ac97_device(ac97->private_data); > + struct ac97_controller *actrl = adev->ac97_ctrl; > + > + if (actrl->ops->reset) > + actrl->ops->reset(adev); > +} > + > +static void compat_ac97_warm_reset(struct snd_ac97 *ac97) > +{ > + struct ac97_codec_device *adev = to_ac97_device(ac97->private_data); > + struct ac97_controller *actrl = adev->ac97_ctrl; > + > + if (actrl->ops->warm_reset) > + actrl->ops->warm_reset(adev); > +} > + > +static void compat_ac97_write(struct snd_ac97 *ac97, unsigned short reg, > + unsigned short val) > +{ > + struct ac97_codec_device *adev = to_ac97_device(ac97->private_data); > + struct ac97_controller *actrl = adev->ac97_ctrl; > + > + actrl->ops->write(adev, reg, val); > +} > + > +static unsigned short compat_ac97_read(struct snd_ac97 *ac97, > + unsigned short reg) > +{ > + struct ac97_codec_device *adev = to_ac97_device(ac97->private_data); > + struct ac97_controller *actrl = adev->ac97_ctrl; > + > + return actrl->ops->read(adev, reg); > +} > + > +static struct snd_ac97_bus_ops compat_snd_ac97_bus_ops = { > + .reset = compat_ac97_reset, > + .warm_reset = compat_ac97_warm_reset, > + .write = compat_ac97_write, > + .read = compat_ac97_read, > +}; > + > +static struct snd_ac97_bus compat_soc_ac97_bus = { > + .ops = &compat_snd_ac97_bus_ops, > +}; > + > +struct snd_ac97 *compat_alloc_snd_ac97_codec(struct snd_soc_codec *codec) > +{ > + struct snd_ac97 *ac97; > + > + ac97 = kzalloc(sizeof(struct snd_ac97), GFP_KERNEL); > + if (ac97 == NULL) > + return ERR_PTR(-ENOMEM); > + > + ac97->dev = *codec->dev; > + ac97->private_data = codec->dev; > + ac97->bus = &compat_soc_ac97_bus; > + return ac97; > +} > +EXPORT_SYMBOL_GPL(compat_alloc_snd_ac97_codec); > + > +void compat_release_snd_ac97_codec(struct snd_ac97 *ac97) > +{ > + kfree(ac97); > +} > +EXPORT_SYMBOL_GPL(compat_release_snd_ac97_codec); > + > +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 (ac97_bus_scan_one(actrl, adev->num) == adev->vendor_id) > + return 1; > + } > + > + compat_ac97_reset(ac97); > + compat_ac97_warm_reset(ac97); > + if (ac97_bus_scan_one(actrl, adev->num) == adev->vendor_id) > + return 0; > + > + return -ENODEV; > +} > +EXPORT_SYMBOL_GPL(snd_ac97_reset); > -- > 2.1.4 > >
Takashi Iwai <tiwai@suse.de> writes: > On Sat, 30 Apr 2016 23:15:34 +0200, > Robert Jarzmik wrote: >> >> diff --git a/include/sound/ac97/codec.h b/include/sound/ac97/codec.h >> new file mode 100644 >> index 000000000000..4b8b3e570892 >> --- /dev/null >> +++ b/include/sound/ac97/codec.h >> @@ -0,0 +1,98 @@ >> +/* >> + * 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_CODEC_H >> +#define AC97_CODEC_H > > Let's be careful about the choice of the guard. Ok, would _SND_AC97_CODEC_H be better ? >> +#define AC97_ID(vendor_id1, vendor_id2) \ >> + (((vendor_id1 & 0xffff) << 16) | (vendor_id2 & 0xffff)) >> +#define AC97_DRIVER_ID(vendor_id1, vendor_id2, mask_id1, mask_id2, _data) \ >> + { .id = ((vendor_id1 & 0xffff) << 16) | (vendor_id2 & 0xffff), \ >> + .mask = ((mask_id1 & 0xffff) << 16) | (mask_id2 & 0xffff), \ >> + .data = _data } > > Give parentheses around the macro arguments. Right, for RFC v2. >> +struct ac97_codec_device { >> + struct device dev; /* Must stay first member */ > > This doesn't have to be the first element as long as you use container_of(). Ah yes, that's a leftover from a former idea, I'll remove that comment. In the initial code I'd done "struct ac97_codec_device" was hidden from this file (ie. there was only a "struct ac97_codec_device;" statement), the body of the struct was contained in sound/ac97/ac97_core.h. The only provided macro to access the "struct device" inside "struct ac97_codec_device" was relying on this "trick" (that's a bit like in the video4linux area). Anyway, good point, I'll remove that. >> +struct ac97_codec_driver { >> + struct device_driver driver; >> + int (*probe)(struct ac97_codec_device *); >> + int (*remove)(struct ac97_codec_device *); >> + int (*suspend)(struct ac97_codec_device *); >> + int (*resume)(struct ac97_codec_device *); >> + void (*shutdown)(struct ac97_codec_device *); >> + struct ac97_id *id_table; > > Missing const? Ah no, unfortunately not, or rather not yet. I tried that one, not very hard, but at least ac97_bus_match() with the pair "struct ac97_id *id = adrv->id_table" and "do { } while (++id->id);" is not possible AFAIK with a const. I will see if I can come up with something better for ac97_bus_match, such as array usage instead of pointer arithmetics. >> +}; >> + >> +int ac97_codec_driver_register(struct ac97_codec_driver *drv); >> +void ac97_codec_driver_unregister(struct ac97_codec_driver *drv); >> + >> +static inline struct device * >> +ac97_codec_dev2dev(const struct ac97_codec_device *adev) >> +{ >> + return (struct device *)(adev); > > What's wrong with the simple &adev->dev ? Cast looks scary. The same leftover than above, I'll change that for RFC v2. >> +struct ac97_controller { >> + const struct ac97_controller_ops *ops; >> + struct list_head controllers; >> + struct device *dev; >> + int bus_idx; > > What is this bus_idx for? Initially it was to distinguish 2 different AC97 controllers. In the current patchset state, it's not usefull anymore AFAICS. So let's remove it. >> + int bound_codecs; The same comment would apply here. I don't think that information is important anymore. I thought I would use that to prevent AC97 controler removal while codecs are still bound. In a second thought what would be better is to have get_device() called for each bound codec which will prevent ac97_digital_controller_unregister() to succeed (-EBUSY). >> + struct list_head codecs; >> +}; >> + >> +int ac97_digital_controller_register(const struct ac97_controller_ops *ops, >> + struct device *dev); >> +int ac97_digital_controller_unregister(const struct device *dev); >> + >> +#endif >> diff --git a/sound/ac97/Kconfig b/sound/ac97/Kconfig >> new file mode 100644 >> index 000000000000..fd2c2d031e62 >> --- /dev/null >> +++ b/sound/ac97/Kconfig >> @@ -0,0 +1,9 @@ >> +# >> +# PCI configuration >> +# > > Still only for PCI? :) Ouch ;) I'll amend that for RFC v2. > >> + >> +config AC97 >> + bool "AC97 bus" >> + help >> + Say Y here if you want to have AC97 devices, which are sound oriented >> + devices around an AC-Link. >> diff --git a/sound/ac97/Makefile b/sound/ac97/Makefile >> new file mode 100644 >> index 000000000000..5575909d46e2 >> --- /dev/null >> +++ b/sound/ac97/Makefile >> @@ -0,0 +1,5 @@ >> +# >> +# make for AC97 bus drivers >> +# >> + >> +obj-y += bus.o codec.o snd_ac97_compat.o > > No possibility for modules? There should be, so I'll put that on my TODO list for RFC v2. >> +static struct ac97_codec_device * >> +ac97_codec_find(struct ac97_controller *ac97_ctrl, int codec_num) >> +{ >> + struct ac97_codec_device *codec; >> + >> + list_for_each_entry(codec, &ac97_ctrl->codecs, list) >> + if (codec->num == codec_num) >> + return codec; >> + >> + return NULL; >> +} > > It's a question whether we need to manage the codecs in the linked > list. There can be at most 4 codecs, so it fits in an array well, > too. Then some codes like this would be simpler. (And it'll even > reduce the footprint, too.) Agreed. For RFC v2. >> +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97, >> + int codec_num) >> +{ >> + struct ac97_codec_device codec; >> + unsigned short vid1, vid2; >> + int ret; >> + >> + codec.dev = *ac97->dev; >> + codec.num = codec_num; >> + ret = ac97->ops->read(&codec, AC97_VENDOR_ID1); >> + vid1 = (ret & 0xffff); >> + if (ret < 0) >> + return 0; > > Hmm. This looks pretty hackish and dangerous. You mean returning 0 even if the read failed, right ? A better prototype would probably be (for RFC v2): int ac97_bus_scan_one(struct ac97_controller *ac97, int codec_num, unsigned int *vendor_id); >> +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; >> + vendor_id = 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; > > This is one of concerns: we don't know whether the device really > reacts well if you access to a non-existing slot. At least, it'd be > safer to have the masks for the devices we already know the slots. Ah you mean upon ac97 controller registration, the ac97_digital_controller_register() should provide the information for each of the 4 slots : - does the controller enable this slot (default yes) - does the controller support auto-scan for this slot (default yes) I'm not sure this "feature" is required, it looks a bit over-engineered. That could be a matter of 1 or 2 masks as input parameters to ac97_digital_controller_register(). >> +static int ac97_bus_reset(struct ac97_controller *ac97_ctrl) >> +{ >> + struct ac97_codec_device codec; >> + >> + memset(&codec, 0, sizeof(codec)); >> + codec.dev = *ac97_ctrl->dev; >> + >> + ac97_ctrl->ops->reset(&codec); > > So, this assumes that reset ops is mandatory? Then document it at > least. Ok, for RFC v2. Thanks for your review and feedbacks Takashi, I'll work on both Mark and your comments in the next weeks. Cheers.
On Sat, 14 May 2016 11:50:50 +0200, Robert Jarzmik wrote: > >> +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97, > >> + int codec_num) > >> +{ > >> + struct ac97_codec_device codec; > >> + unsigned short vid1, vid2; > >> + int ret; > >> + > >> + codec.dev = *ac97->dev; > >> + codec.num = codec_num; > >> + ret = ac97->ops->read(&codec, AC97_VENDOR_ID1); > >> + vid1 = (ret & 0xffff); > >> + if (ret < 0) > >> + return 0; > > > > Hmm. This looks pretty hackish and dangerous. > You mean returning 0 even if the read failed, right ? No, my concern is that it's creating a dummy codec object temporarily on the stack just by copying some fields and calling the ops with it. (And actually the current code may work wrongly because lack of zero-clear of the object.) IMO, a cleaner way would be to define the ops passed with both controller and codec objects as arguments, and pass NULL codec here. Takashi
Takashi Iwai <tiwai@suse.de> writes: > On Sat, 14 May 2016 11:50:50 +0200, > Robert Jarzmik wrote: >> >> +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97, >> >> + int codec_num) >> >> +{ >> >> + struct ac97_codec_device codec; >> >> + unsigned short vid1, vid2; >> >> + int ret; >> >> + >> >> + codec.dev = *ac97->dev; >> >> + codec.num = codec_num; >> >> + ret = ac97->ops->read(&codec, AC97_VENDOR_ID1); >> >> + vid1 = (ret & 0xffff); >> >> + if (ret < 0) >> >> + return 0; >> > >> > Hmm. This looks pretty hackish and dangerous. >> You mean returning 0 even if the read failed, right ? > > No, my concern is that it's creating a dummy codec object temporarily > on the stack just by copying some fields and calling the ops with it. > (And actually the current code may work wrongly because lack of > zero-clear of the object.) Ah yes, I remember now, the on-stack generated device, indeed ugly. > IMO, a cleaner way would be to define the ops passed with both > controller and codec objects as arguments, and pass NULL codec here. It's rather unusual to need both the device and its controller in bus operations. I must admit I have no better idea so far, so I'll try that just to see how it looks like, and let's see next ... Cheers.
On Sun, 15 May 2016 23:29:27 +0200, Robert Jarzmik wrote: > > Takashi Iwai <tiwai@suse.de> writes: > > > On Sat, 14 May 2016 11:50:50 +0200, > > Robert Jarzmik wrote: > >> >> +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97, > >> >> + int codec_num) > >> >> +{ > >> >> + struct ac97_codec_device codec; > >> >> + unsigned short vid1, vid2; > >> >> + int ret; > >> >> + > >> >> + codec.dev = *ac97->dev; > >> >> + codec.num = codec_num; > >> >> + ret = ac97->ops->read(&codec, AC97_VENDOR_ID1); > >> >> + vid1 = (ret & 0xffff); > >> >> + if (ret < 0) > >> >> + return 0; > >> > > >> > Hmm. This looks pretty hackish and dangerous. > >> You mean returning 0 even if the read failed, right ? > > > > No, my concern is that it's creating a dummy codec object temporarily > > on the stack just by copying some fields and calling the ops with it. > > (And actually the current code may work wrongly because lack of > > zero-clear of the object.) > Ah yes, I remember now, the on-stack generated device, indeed ugly. > > > IMO, a cleaner way would be to define the ops passed with both > > controller and codec objects as arguments, and pass NULL codec here. > It's rather unusual to need both the device and its controller in bus > operations. I must admit I have no better idea so far, so I'll try that just to > see how it looks like, and let's see next ... Thinking of this again, I wonder now why we need to pass the codec object at all. It's the read/write ops via ac97, so we just need the ac97_controller object and the address slot of the accessed codec? Takashi
Takashi Iwai <tiwai@suse.de> writes: > On Sun, 15 May 2016 23:29:27 +0200, > Robert Jarzmik wrote: >> >> Takashi Iwai <tiwai@suse.de> writes: >> >> > On Sat, 14 May 2016 11:50:50 +0200, >> > >> > No, my concern is that it's creating a dummy codec object temporarily >> > on the stack just by copying some fields and calling the ops with it. >> > (And actually the current code may work wrongly because lack of >> > zero-clear of the object.) >> Ah yes, I remember now, the on-stack generated device, indeed ugly. >> >> > IMO, a cleaner way would be to define the ops passed with both >> > controller and codec objects as arguments, and pass NULL codec here. >> It's rather unusual to need both the device and its controller in bus >> operations. I must admit I have no better idea so far, so I'll try that just to >> see how it looks like, and let's see next ... > > Thinking of this again, I wonder now why we need to pass the codec > object at all. It's the read/write ops via ac97, so we just need the > ac97_controller object and the address slot of the accessed codec? So far it would work. The only objection I would see is if in the future the bus operation needs a specialization which is ac97 codec dependent, such as a flag or a mask in ac97_codec_device structure. Even if I'd like to not have these in bus operations, the struct snd_ac97 had a need for a 'caps', 'ext_id', ... fields for example. Yet these could be contained in the ac97_codec_device structure and not exposed to bus operations. Another worry is the pattern (as an example) in atmel_ac97c_write() in sound/atmel/ac97.c, where the codec structure is used to get the controller through a container_of() type call. Yet passing the controller to bus operations takes care of this one. From a "purely API" point of view the couple (ac97_controller, ac97_slot_id) is what will route an ac97 bus operation, be that a read/write/reset/..., the remaining question is will it cover the cases we've not thought of ? Cheers.
On Mon, 16 May 2016 10:53:56 +0200, Robert Jarzmik wrote: > > Takashi Iwai <tiwai@suse.de> writes: > > > On Sun, 15 May 2016 23:29:27 +0200, > > Robert Jarzmik wrote: > >> > >> Takashi Iwai <tiwai@suse.de> writes: > >> > >> > On Sat, 14 May 2016 11:50:50 +0200, > >> > > >> > No, my concern is that it's creating a dummy codec object temporarily > >> > on the stack just by copying some fields and calling the ops with it. > >> > (And actually the current code may work wrongly because lack of > >> > zero-clear of the object.) > >> Ah yes, I remember now, the on-stack generated device, indeed ugly. > >> > >> > IMO, a cleaner way would be to define the ops passed with both > >> > controller and codec objects as arguments, and pass NULL codec here. > >> It's rather unusual to need both the device and its controller in bus > >> operations. I must admit I have no better idea so far, so I'll try that just to > >> see how it looks like, and let's see next ... > > > > Thinking of this again, I wonder now why we need to pass the codec > > object at all. It's the read/write ops via ac97, so we just need the > > ac97_controller object and the address slot of the accessed codec? > So far it would work. The only objection I would see is if in the future the bus > operation needs a specialization which is ac97 codec dependent, such as a flag > or a mask in ac97_codec_device structure. > > Even if I'd like to not have these in bus operations, the struct snd_ac97 had a > need for a 'caps', 'ext_id', ... fields for example. Yet these could be > contained in the ac97_codec_device structure and not exposed to bus operations. Do we have any example of such exceptions? For AC97, we don't need to think of future extensions at all. If any, we may provide two levels of ops abstractions: the lower ac97_controller_ops.read/write, and the upper ac97_codec_ops read/write that may override if defined, but as default it just wraps over controller ops. But I don't think we'd need such unless we really see the demand to be exposed outside the codec driver itself. > Another worry is the pattern (as an example) in atmel_ac97c_write() in > sound/atmel/ac97.c, where the codec structure is used to get the controller > through a container_of() type call. Yet passing the controller to bus operations > takes care of this one. Right. > From a "purely API" point of view the couple (ac97_controller, ac97_slot_id) is > what will route an ac97 bus operation, be that a read/write/reset/..., the > remaining question is will it cover the cases we've not thought of ? The remaining ops are rather codec-specific operations, and they don't have to be implemented in the bus (controller) level. We should keep the bus ops as small as possible. thanks, Takashi
On Mon, May 16, 2016 at 02:58:13PM +0200, Takashi Iwai wrote: > Robert Jarzmik wrote: > > Even if I'd like to not have these in bus operations, the struct snd_ac97 had a > > need for a 'caps', 'ext_id', ... fields for example. Yet these could be > > contained in the ac97_codec_device structure and not exposed to bus operations. > Do we have any example of such exceptions? For AC97, we don't need to > think of future extensions at all. I can't think of any, even for some of the more fancy CODECs used in embedded systems.
diff --git a/include/sound/ac97/codec.h b/include/sound/ac97/codec.h new file mode 100644 index 000000000000..4b8b3e570892 --- /dev/null +++ b/include/sound/ac97/codec.h @@ -0,0 +1,98 @@ +/* + * 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_CODEC_H +#define AC97_CODEC_H + +#include <linux/device.h> + +#define AC97_ID(vendor_id1, vendor_id2) \ + (((vendor_id1 & 0xffff) << 16) | (vendor_id2 & 0xffff)) +#define AC97_DRIVER_ID(vendor_id1, vendor_id2, mask_id1, mask_id2, _data) \ + { .id = ((vendor_id1 & 0xffff) << 16) | (vendor_id2 & 0xffff), \ + .mask = ((mask_id1 & 0xffff) << 16) | (mask_id2 & 0xffff), \ + .data = _data } + +#define to_ac97_device(d) container_of(d, struct ac97_codec_device, dev) +#define to_ac97_driver(d) container_of(d, struct ac97_codec_driver, driver) + +struct ac97_controller; + +/** + * struct ac97_id - matches a codec device and driver on an ac97 bus + * @id: The significant bits if the codec vendor ID1 and ID2 + * @mask: Bitmask specifying which bits of the id field are significant when + * matching. A driver binds to a device when : + * ((vendorID1 << 8 | vendorID2) & (mask_id1 << 8 | mask_id2)) == id. + * @data: Private data used by the driver. + */ +struct ac97_id { + unsigned int id; + unsigned int mask; + void *data; +}; + +/** + * ac97_codec_device - a ac97 codec + * @dev: the code device + * @vendor_id: the vendor_id of the codec, as sensed on the AC-link + * @num: the codec number, 0 is primary, 1 is first slave, etc ... + * @ac97_ctrl: ac97 digital controller on the same AC-link + * + * This is the device instanciated for each codec living on a AC-link. There are + * normally 0 to 4 codec devices per AC-link, and all of them are controlled by + * an AC97 digital controller. + */ +struct ac97_codec_device { + struct device dev; /* Must stay first member */ + unsigned int vendor_id; + unsigned int num; + struct list_head list; + struct ac97_controller *ac97_ctrl; +}; + +/** + * ac97_codec_driver - a ac97 codec driver + * @driver: the device driver structure + * @probe: the function called when a ac97_codec_device is matched + * @remove: the function called when the device is unbound/removed + * @suspend: suspend function (might be NULL) + * @resume: resume function (might be NULL) + * @shutdown: shutdown function (might be NULL) + * @id_table: ac97 vendor_id match table, { } member terminated + */ +struct ac97_codec_driver { + struct device_driver driver; + int (*probe)(struct ac97_codec_device *); + int (*remove)(struct ac97_codec_device *); + int (*suspend)(struct ac97_codec_device *); + int (*resume)(struct ac97_codec_device *); + void (*shutdown)(struct ac97_codec_device *); + struct ac97_id *id_table; +}; + +int ac97_codec_driver_register(struct ac97_codec_driver *drv); +void ac97_codec_driver_unregister(struct ac97_codec_driver *drv); + +static inline struct device * +ac97_codec_dev2dev(const struct ac97_codec_device *adev) +{ + return (struct device *)(adev); +} + +static inline void *ac97_get_drvdata(const struct ac97_codec_device *adev) +{ + return dev_get_drvdata(ac97_codec_dev2dev(adev)); +} + +static inline void ac97_set_drvdata(const struct ac97_codec_device *adev, + void *data) +{ + dev_set_drvdata(ac97_codec_dev2dev(adev), data); +} + +#endif diff --git a/include/sound/ac97/compat.h b/include/sound/ac97/compat.h new file mode 100644 index 000000000000..bf611f572f2d --- /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> + +struct snd_ac97 *compat_alloc_snd_ac97_codec(struct snd_soc_codec *codec); +void compat_release_snd_ac97_codec(struct snd_ac97 *ac97); + +#endif diff --git a/include/sound/ac97/controller.h b/include/sound/ac97/controller.h new file mode 100644 index 000000000000..f1e5e645f5ef --- /dev/null +++ b/include/sound/ac97/controller.h @@ -0,0 +1,39 @@ +/* + * 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> + +struct device; +struct ac97_codec_device; + +struct ac97_controller_ops { + void (*reset)(struct ac97_codec_device *ac97); + void (*warm_reset)(struct ac97_codec_device *ac97); + int (*write)(struct ac97_codec_device *ac97, unsigned short reg, + unsigned short val); + int (*read)(struct ac97_codec_device *ac97, unsigned short reg); + void (*wait)(struct ac97_codec_device *ac97); + void (*init)(struct ac97_codec_device *ac97); +}; + +struct ac97_controller { + const struct ac97_controller_ops *ops; + struct list_head controllers; + struct device *dev; + int bus_idx; + int bound_codecs; + struct list_head codecs; +}; + +int ac97_digital_controller_register(const struct ac97_controller_ops *ops, + struct device *dev); +int ac97_digital_controller_unregister(const struct device *dev); + +#endif diff --git a/sound/ac97/Kconfig b/sound/ac97/Kconfig new file mode 100644 index 000000000000..fd2c2d031e62 --- /dev/null +++ b/sound/ac97/Kconfig @@ -0,0 +1,9 @@ +# +# PCI configuration +# + +config AC97 + bool "AC97 bus" + help + Say Y here if you want to have AC97 devices, which are sound oriented + devices around an AC-Link. diff --git a/sound/ac97/Makefile b/sound/ac97/Makefile new file mode 100644 index 000000000000..5575909d46e2 --- /dev/null +++ b/sound/ac97/Makefile @@ -0,0 +1,5 @@ +# +# make for AC97 bus drivers +# + +obj-y += bus.o codec.o snd_ac97_compat.o diff --git a/sound/ac97/ac97_core.h b/sound/ac97/ac97_core.h new file mode 100644 index 000000000000..db6e27288357 --- /dev/null +++ b/sound/ac97/ac97_core.h @@ -0,0 +1,10 @@ +/* + * 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. + */ + +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97, + int codec_num); diff --git a/sound/ac97/bus.c b/sound/ac97/bus.c new file mode 100644 index 000000000000..f9bf5632d4aa --- /dev/null +++ b/sound/ac97/bus.c @@ -0,0 +1,330 @@ +/* + * 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. + */ + +#include <linux/module.h> +#include <linux/device.h> +#include <linux/list.h> +#include <linux/mutex.h> +#include <linux/pm.h> +#include <linux/slab.h> +#include <sound/ac97/codec.h> +#include <sound/ac97/controller.h> +#include <sound/ac97/regs.h> + +#define AC97_BUS_MAX_CODECS 4 + +/* + * Protects ac97_controllers and each ac97_controller structure. + */ +static DEFINE_MUTEX(ac97_controllers_mutex); +static LIST_HEAD(ac97_controllers); +static int ac97_bus_idx; + +struct bus_type ac97_bus_type; + +static struct ac97_codec_device * +ac97_codec_find(struct ac97_controller *ac97_ctrl, int codec_num) +{ + struct ac97_codec_device *codec; + + list_for_each_entry(codec, &ac97_ctrl->codecs, list) + if (codec->num == codec_num) + return codec; + + return NULL; +} + +static void ac97_codec_release(struct device *dev) +{ + struct ac97_codec_device *codec; + + codec = container_of(dev, struct ac97_codec_device, dev); + list_del(&codec->list); + kfree(codec); +} + +static int ac97_codec_add(struct ac97_controller *ac97_ctrl, int idx, + unsigned int vendor_id) +{ + struct ac97_codec_device *codec; + char *codec_name; + int ret; + + codec = kzalloc(sizeof(*codec), GFP_KERNEL); + if (!codec) + return -ENOMEM; + + codec->vendor_id = vendor_id; + codec->dev.release = ac97_codec_release; + codec->dev.bus = &ac97_bus_type; + codec->dev.parent = ac97_ctrl->dev; + codec->num = idx; + codec->ac97_ctrl = ac97_ctrl; + INIT_LIST_HEAD(&codec->list); + list_move_tail(&codec->list, &ac97_ctrl->codecs); + + codec_name = kasprintf(GFP_KERNEL, "%s:%d", dev_name(ac97_ctrl->dev), + idx); + codec->dev.init_name = codec_name; + + ret = device_register(&codec->dev); + kfree(codec_name); + + return ret; +} + +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97, + int codec_num) +{ + struct ac97_codec_device codec; + unsigned short vid1, vid2; + int ret; + + codec.dev = *ac97->dev; + codec.num = codec_num; + ret = ac97->ops->read(&codec, AC97_VENDOR_ID1); + vid1 = (ret & 0xffff); + if (ret < 0) + return 0; + + ret = ac97->ops->read(&codec, AC97_VENDOR_ID2); + vid2 = (ret & 0xffff); + if (ret < 0) + return 0; + + dev_dbg(&codec.dev, "%s(codec_num=%d): 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; + vendor_id = 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; +} + +void ac97_rescan_all_controllers(void) +{ + struct ac97_controller *ac97_ctrl; + int ret; + + mutex_lock(&ac97_controllers_mutex); + list_for_each_entry(ac97_ctrl, &ac97_controllers, controllers) { + ret = ac97_bus_scan(ac97_ctrl); + if (ret) + dev_warn(ac97_ctrl->dev, "scan failed: %d\n", ret); + } + mutex_unlock(&ac97_controllers_mutex); +} +EXPORT_SYMBOL(ac97_rescan_all_controllers); + +static int ac97_bus_reset(struct ac97_controller *ac97_ctrl) +{ + struct ac97_codec_device codec; + + memset(&codec, 0, sizeof(codec)); + codec.dev = *ac97_ctrl->dev; + + ac97_ctrl->ops->reset(&codec); + return 0; +} + +/** + * 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 ac97_codec_driver_register(struct ac97_codec_driver *drv) +{ + int ret; + + drv->driver.bus = &ac97_bus_type; + + ret = driver_register(&drv->driver); + if (!ret) + ac97_rescan_all_controllers(); + + return ret; +} +EXPORT_SYMBOL(ac97_codec_driver_register); + +/** + * ac97_codec_driver_unregister - unregister an AC97 codec driver + * @dev: AC97 codec driver to unregister + * + * Unregister a previously registered ac97 codec driver. + */ +void ac97_codec_driver_unregister(struct ac97_codec_driver *drv) +{ + driver_unregister(&drv->driver); +} +EXPORT_SYMBOL(ac97_codec_driver_unregister); + +static int ac97_dc_codecs_unregister(struct ac97_controller *ac97_ctrl) +{ + struct ac97_codec_device *codec, *tmp; + + list_for_each_entry_safe(codec, tmp, &ac97_ctrl->codecs, list) + put_device(&codec->dev); + + return 0; +} + +/** + * ac97_digital_controller_register - register an ac97 controller + * @ops: the ac97 bus operations + * @dev: the device providing the ac97 DC function + * + * Register a digital controller which can control up to 4 ac97 codecs. This is + * the controller side of the AC97 AC-link, while the slave side are the codecs. + * + * Returns a positive bus index upon success, negative value upon error + */ +int ac97_digital_controller_register(const struct ac97_controller_ops *ops, + struct device *dev) +{ + struct ac97_controller *ac97_ctrl; + + ac97_ctrl = kzalloc(sizeof(*ac97_ctrl), GFP_KERNEL); + if (!ac97_ctrl) + return -ENOMEM; + + mutex_lock(&ac97_controllers_mutex); + ac97_ctrl->ops = ops; + ac97_ctrl->bus_idx = ac97_bus_idx++; + ac97_ctrl->dev = dev; + INIT_LIST_HEAD(&ac97_ctrl->codecs); + list_add(&ac97_ctrl->controllers, &ac97_controllers); + mutex_unlock(&ac97_controllers_mutex); + + ac97_bus_reset(ac97_ctrl); + ac97_bus_scan(ac97_ctrl); + + return ac97_ctrl->bus_idx; +} +EXPORT_SYMBOL(ac97_digital_controller_register); + +/** + * ac97_digital_controller_unregister - unregister an ac97 controller + * @dev: the device previously provided to ac97_digital_controller_register() + * + * Returns 0 on success, negative upon error + */ +int ac97_digital_controller_unregister(const struct device *dev) +{ + struct ac97_controller *ac97_ctrl, *tmp; + int ret = -ENODEV; + + mutex_lock(&ac97_controllers_mutex); + list_for_each_entry_safe(ac97_ctrl, tmp, &ac97_controllers, + controllers) { + if (ac97_ctrl->dev != dev) + continue; + if (ac97_ctrl->bound_codecs) + ret = -EBUSY; + else + ret = ac97_dc_codecs_unregister(ac97_ctrl); + if (!ret) + list_del(&ac97_ctrl->controllers); + } + + mutex_unlock(&ac97_controllers_mutex); + return ret; +} +EXPORT_SYMBOL(ac97_digital_controller_unregister); + +static const struct dev_pm_ops ac97_pm = { + .suspend = pm_generic_suspend, + .resume = pm_generic_resume, + .freeze = pm_generic_freeze, + .thaw = pm_generic_thaw, + .poweroff = pm_generic_poweroff, + .restore = pm_generic_restore, +}; + +static ssize_t vendor_id_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct ac97_codec_device *codec = to_ac97_device(dev); + + return sprintf(buf, "%08x", codec->vendor_id); +} + +static struct device_attribute ac97_dev_attrs[] = { + __ATTR_RO(vendor_id), + __ATTR_NULL, +}; + +int ac97_bus_match(struct device *dev, struct device_driver *drv) +{ + struct ac97_codec_device *adev = to_ac97_device(dev); + struct ac97_codec_driver *adrv = to_ac97_driver(drv); + struct ac97_id *id = adrv->id_table; + + if (adev->vendor_id == 0x0 || adev->vendor_id == 0xffffffff) + return false; + + do { + if ((id->id & id->mask) == (adev->vendor_id & id->mask)) + return true; + } while (++id->id); + + return false; +} + +static int ac97_bus_probe(struct device *dev) +{ + struct ac97_codec_device *adev = to_ac97_device(dev); + struct ac97_codec_driver *adrv = to_ac97_driver(dev->driver); + + return adrv->probe(adev); +} + +static int ac97_bus_remove(struct device *dev) +{ + struct ac97_codec_device *adev = to_ac97_device(dev); + struct ac97_codec_driver *adrv = to_ac97_driver(dev->driver); + + return adrv->remove(adev); +} + +struct bus_type ac97_bus_type = { + .name = "ac97", + .dev_attrs = ac97_dev_attrs, + .match = ac97_bus_match, + .pm = &ac97_pm, + .probe = ac97_bus_probe, + .remove = ac97_bus_remove, +}; +EXPORT_SYMBOL(ac97_bus_type); + +static int __init ac97_bus_init(void) +{ + return bus_register(&ac97_bus_type); +} +subsys_initcall(ac97_bus_init); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Robert Jarzmik <robert.jarzmik@free.fr>"); diff --git a/sound/ac97/codec.c b/sound/ac97/codec.c new file mode 100644 index 000000000000..a835f03744bf --- /dev/null +++ b/sound/ac97/codec.c @@ -0,0 +1,15 @@ +/* + * 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. + */ + +#include <sound/ac97_codec.h> +#include <sound/ac97/codec.h> +#include <sound/ac97/controller.h> +#include <linux/device.h> +#include <linux/slab.h> +#include <sound/soc.h> /* For compat_ac97_* */ + diff --git a/sound/ac97/snd_ac97_compat.c b/sound/ac97/snd_ac97_compat.c new file mode 100644 index 000000000000..7e2f01c96fc9 --- /dev/null +++ b/sound/ac97/snd_ac97_compat.c @@ -0,0 +1,104 @@ +/* + * 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. + */ + +#include <linux/list.h> +#include <linux/slab.h> +#include <sound/ac97/codec.h> +#include <sound/ac97/controller.h> +#include <sound/soc.h> + +#include "ac97_core.h" + +static void compat_ac97_reset(struct snd_ac97 *ac97) +{ + struct ac97_codec_device *adev = to_ac97_device(ac97->private_data); + struct ac97_controller *actrl = adev->ac97_ctrl; + + if (actrl->ops->reset) + actrl->ops->reset(adev); +} + +static void compat_ac97_warm_reset(struct snd_ac97 *ac97) +{ + struct ac97_codec_device *adev = to_ac97_device(ac97->private_data); + struct ac97_controller *actrl = adev->ac97_ctrl; + + if (actrl->ops->warm_reset) + actrl->ops->warm_reset(adev); +} + +static void compat_ac97_write(struct snd_ac97 *ac97, unsigned short reg, + unsigned short val) +{ + struct ac97_codec_device *adev = to_ac97_device(ac97->private_data); + struct ac97_controller *actrl = adev->ac97_ctrl; + + actrl->ops->write(adev, reg, val); +} + +static unsigned short compat_ac97_read(struct snd_ac97 *ac97, + unsigned short reg) +{ + struct ac97_codec_device *adev = to_ac97_device(ac97->private_data); + struct ac97_controller *actrl = adev->ac97_ctrl; + + return actrl->ops->read(adev, reg); +} + +static struct snd_ac97_bus_ops compat_snd_ac97_bus_ops = { + .reset = compat_ac97_reset, + .warm_reset = compat_ac97_warm_reset, + .write = compat_ac97_write, + .read = compat_ac97_read, +}; + +static struct snd_ac97_bus compat_soc_ac97_bus = { + .ops = &compat_snd_ac97_bus_ops, +}; + +struct snd_ac97 *compat_alloc_snd_ac97_codec(struct snd_soc_codec *codec) +{ + struct snd_ac97 *ac97; + + ac97 = kzalloc(sizeof(struct snd_ac97), GFP_KERNEL); + if (ac97 == NULL) + return ERR_PTR(-ENOMEM); + + ac97->dev = *codec->dev; + ac97->private_data = codec->dev; + ac97->bus = &compat_soc_ac97_bus; + return ac97; +} +EXPORT_SYMBOL_GPL(compat_alloc_snd_ac97_codec); + +void compat_release_snd_ac97_codec(struct snd_ac97 *ac97) +{ + kfree(ac97); +} +EXPORT_SYMBOL_GPL(compat_release_snd_ac97_codec); + +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 (ac97_bus_scan_one(actrl, adev->num) == adev->vendor_id) + return 1; + } + + compat_ac97_reset(ac97); + compat_ac97_warm_reset(ac97); + if (ac97_bus_scan_one(actrl, adev->num) == adev->vendor_id) + return 0; + + return -ENODEV; +} +EXPORT_SYMBOL_GPL(snd_ac97_reset);
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> --- include/sound/ac97/codec.h | 98 ++++++++++++ include/sound/ac97/compat.h | 21 +++ include/sound/ac97/controller.h | 39 +++++ sound/ac97/Kconfig | 9 ++ sound/ac97/Makefile | 5 + sound/ac97/ac97_core.h | 10 ++ sound/ac97/bus.c | 330 ++++++++++++++++++++++++++++++++++++++++ sound/ac97/codec.c | 15 ++ sound/ac97/snd_ac97_compat.c | 104 +++++++++++++ 9 files changed, 631 insertions(+) create mode 100644 include/sound/ac97/codec.h create mode 100644 include/sound/ac97/compat.h create mode 100644 include/sound/ac97/controller.h create mode 100644 sound/ac97/Kconfig create mode 100644 sound/ac97/Makefile create mode 100644 sound/ac97/ac97_core.h create mode 100644 sound/ac97/bus.c create mode 100644 sound/ac97/codec.c create mode 100644 sound/ac97/snd_ac97_compat.c