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@xxxxxxx> > + * > + * 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@xxxxxxx> > + * > + * 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@xxxxxxx> > + * > + * 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@xxxxxxx> > + * > + * 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@xxxxxxx> > + * > + * 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@xxxxxxx>"); > 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@xxxxxxx> > + * > + * 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@xxxxxxx> > + * > + * 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 > > _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel