| Message ID | 1477510907-23495-3-git-send-email-robert.jarzmik@free.fr (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 10/26/2016 09:41 PM, 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. > Good work, a couple of comments inline. [...] > diff --git a/include/sound/ac97/codec.h b/include/sound/ac97/codec.h > new file mode 100644 > index 000000000000..8901c1200522 > --- /dev/null > +++ b/include/sound/ac97/codec.h > @@ -0,0 +1,115 @@ > +/* > + * 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 __SOUND_AC97_CODEC2_H > +#define __SOUND_AC97_CODEC2_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) In my opinion these should be inline functions rather than macros as that generates much more legible compiler errors e.g. in case there is a type mismatch. [...] > +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 *); The suspend, resume and shutdown callbacks are never used. Which is good, since all new frameworks should use dev_pm_ops. Just drop the from the struct. > + const struct ac97_id *id_table; > +}; [...] > diff --git a/include/sound/ac97/controller.h b/include/sound/ac97/controller.h > new file mode 100644 > index 000000000000..5ff59bd7e324 > --- /dev/null > +++ b/include/sound/ac97/controller.h > @@ -0,0 +1,85 @@ > +/* > + * 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; > + > +/** > + * struct ac97_controller - The AC97 controller of the AC-Link > + * @ops: the AC97 operations. > + * @controllers: linked list of all existing controllers. > + * @dev: 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; > + struct list_head controllers; > + struct device *dev; I'd make the controller itself a struct dev, rather than just having the pointer to the parent. This is more idiomatic and matches what other subsystems do. It has several advantages, you get proper refcounting of your controller structure, the controller gets its own sysfs directory where the CODECs appear as children, you don't need the manual sysfs attribute creation and removal in ac97_controler_{un,}register(). > + unsigned short slots_available; > + struct ac97_codec_device *codecs[AC97_BUS_MAX_CODECS]; If you make the controller a struct dev you can also remove this, since the device driver core tracks the children of a device. > + void *codecs_pdata[AC97_BUS_MAX_CODECS]; > +}; > + > +/** > + * struct ac97_controller_ops - The AC97 operations > + * @reset: Cold reset of the AC97 AC-Link. > + * @warm_reset: Warm reset of the AC97 AC-Link. > + * @read: Read of a single AC97 register. > + * Returns the register value or a negative error code. > + * @write: Write of a single AC97 register. > + * @wait: Wait for the current AC97 operation to finish (might be NULL). > + * @init: Initialization of the AC97 AC-Link (might be NULL). > + * > + * These are the basic operation an AC97 controller must provide for an AC97 > + * access functions. Amongst these, all but the last 2 are mandatory. > + * The slot number is also known as the AC97 codec number, between 0 and 3. > + */ > +struct ac97_controller_ops { > + void (*reset)(struct ac97_controller *adrv); > + void (*warm_reset)(struct ac97_controller *adrv); > + int (*write)(struct ac97_controller *adrv, int slot, > + unsigned short reg, unsigned short val); > + int (*read)(struct ac97_controller *adrv, int slot, unsigned short reg); > + void (*wait)(struct ac97_controller *adrv, int slot); > + void (*init)(struct ac97_controller *adrv, int slot); Neither wait nor init are ever used. > +}; > + [...] > +/* > + * Protects ac97_controllers and each ac97_controller structure. > + */ > +static DEFINE_MUTEX(ac97_controllers_mutex); > +static LIST_HEAD(ac97_controllers); > + > +static struct bus_type ac97_bus_type; > + > +static struct ac97_codec_device * > +ac97_codec_find(struct ac97_controller *ac97_ctrl, int codec_num) unsigned int codec_num > +{ > + if ((codec_num < 0) || (codec_num >= AC97_BUS_MAX_CODECS)) > + return ERR_PTR(-ERANGE); I'd make this EINVAL. > + > + return ac97_ctrl->codecs[codec_num]; > +} > + > +static void ac97_codec_release(struct device *dev) > +{ > + struct ac97_codec_device *adev; > + struct ac97_controller *ac97_ctrl; > + > + adev = container_of(dev, struct ac97_codec_device, dev); to_ac97_device() > + ac97_ctrl = adev->ac97_ctrl; > + ac97_ctrl->codecs[adev->num] = NULL; > + sysfs_remove_link(&dev->kobj, "ac97_controller"); > + kfree(adev); > +} > + > +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; > + 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->dev; > + codec->num = idx; > + codec->ac97_ctrl = ac97_ctrl; > + > + codec_name = kasprintf(GFP_KERNEL, "%s:%d", dev_name(ac97_ctrl->dev), > + idx); > + codec->dev.init_name = codec_name; init_name is only for statically allocated devices. Use dev_set_name(dev, ...). No need for kasprintf() either as dev_set_name() takes a format string. For this you need to split device_register into device_initialize() and device_add(). But usually that is what you want anyway. > + > + ret = device_register(&codec->dev); > + kfree(codec_name); > + if (ret) > + goto err_free_codec; > + > + ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj, > + "ac97_controller"); Since the CODEC is a child of the controller this should not be necessary as this just points one directory up. It's like `ln -s .. parent` > + if (ret) > + goto err_unregister_device; > + > + return 0; > +err_unregister_device: > + put_device(&codec->dev); > +err_free_codec: > + kfree(codec); Since the struct is reference counted, the freeing is done in the release callback and this leads to a double free. > + ac97_ctrl->codecs[idx] = NULL; > + > + return ret; > +} [...] > +/** > + * 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); > + if (!ret) > + ac97_rescan_all_controllers(); Rescanning the bus when a new codec driver is registered should not be neccessary. The bus is scanned once when the controller is registered, this creates the device. The device driver core will take care of binding the device to the driver, if the driver is registered after thed evice. > + > + return ret; > +} > +EXPORT_SYMBOL(snd_ac97_codec_driver_register); > + [...] > +static int ac97_ctrl_codecs_unregister(struct ac97_controller *ac97_ctrl) > +{ > + int i; > + > + for (i = 0; i < AC97_BUS_MAX_CODECS; i++) > + if (ac97_ctrl->codecs[i]) > + put_device(&ac97_ctrl->codecs[i]->dev); This should be device_unregister() to match the device_register() in ac97_codec_add(). > + > + return 0; > +} > + > +static ssize_t cold_reset_store(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t len) > +{ > + struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev); > + > + if (!dev) > + return -ENODEV; dev is never NULL here. And for the ac97_ctrl there is a race condition. It could be unregistered and freed after ac97_ctrl_find() returned sucessfully, but before ac97_ctrl->ops is used. > + > + ac97_ctrl->ops->reset(ac97_ctrl); > + return len; > +} > +static DEVICE_ATTR_WO(cold_reset); > + > +static ssize_t warm_reset_store(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t len) > +{ > + struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev); > + > + if (!dev) > + return -ENODEV; Same here. > + > + ac97_ctrl->ops->warm_reset(ac97_ctrl); > + return len; > +} > +static DEVICE_ATTR_WO(warm_reset); > + > +static struct attribute *ac97_controller_device_attrs[] = { > + &dev_attr_cold_reset.attr, > + &dev_attr_warm_reset.attr, > + NULL > +}; This adds new userspace ABI that is not documented at the moment. > + > +static const struct attribute_group ac97_controller_attr_group = { > + .name = "ac97_operations", > + .attrs = ac97_controller_device_attrs, > +}; > + > +/** > + * snd_ac97_controller_register - register an ac97 controller > + * @ops: the ac97 bus operations > + * @dev: the device providing the ac97 DC function > + * @slots_available: mask of the ac97 codecs that can be scanned and probed > + * bit0 => codec 0, bit1 => codec 1 ... bit 3 => codec 3 > + * > + * 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 0 upon success, negative value upon error > + */ > +int snd_ac97_controller_register(const struct ac97_controller_ops *ops, > + struct device *dev, > + unsigned short slots_available, > + void **codecs_pdata) In my opinion this should return a handle to a ac97 controller which can then be passed to snd_ac97_controller_unregister(). This is in my opinion the better approach rather than looking up the controller by parent device. > +{ > + struct ac97_controller *ac97_ctrl; > + int ret, i; > + > + ac97_ctrl = kzalloc(sizeof(*ac97_ctrl), GFP_KERNEL); > + if (!ac97_ctrl) > + return -ENOMEM; > + > + for (i = 0; i < AC97_BUS_MAX_CODECS && codecs_pdata; i++) > + ac97_ctrl->codecs_pdata[i] = codecs_pdata[i]; > + > + ret = sysfs_create_group(&dev->kobj, &ac97_controller_attr_group); > + if (ret) > + return ret; > + > + mutex_lock(&ac97_controllers_mutex); > + ac97_ctrl->ops = ops; > + ac97_ctrl->slots_available = slots_available; > + ac97_ctrl->dev = dev; > + list_add(&ac97_ctrl->controllers, &ac97_controllers); Stricly speeaking only the list_add needs to be protected. > + mutex_unlock(&ac97_controllers_mutex); > + > + ac97_bus_reset(ac97_ctrl); > + ac97_bus_scan(ac97_ctrl); > + > + return 0; > +} > +EXPORT_SYMBOL(snd_ac97_controller_register); > + > +/** > + * snd_ac97_controller_unregister - unregister an ac97 controller > + * @dev: the device previously provided to ac97_controller_register() > + * > + * Returns 0 on success, negative upon error Unregister must not be able to fail. Hotunplug is one of the core concepts of the device driver model and there is really nothing that can be done to prevent a device from disappearing, so there is no sensible way of handling the error (and your pxa driver modifications simply ignore it as well). This also means the framework needs to cope with the case where the controller is removed and the CODEC devices are still present. All future operations should return -ENODEV in that case. > + */ > +int snd_ac97_controller_unregister(struct device *dev) > +{ > + struct ac97_controller *ac97_ctrl; > + int ret = -ENODEV, i; > + > + mutex_lock(&ac97_controllers_mutex); > + ac97_ctrl = ac97_ctrl_find(dev); > + if (ac97_ctrl) { > + ret = 0; > + for (i = 0; i < AC97_BUS_MAX_CODECS; i++) > + if (ac97_ctrl->codecs[i] && > + device_is_registered(&ac97_ctrl->codecs[i]->dev)) > + ret = -EBUSY; > + if (!ret) > + ret = ac97_ctrl_codecs_unregister(ac97_ctrl); > + if (!ret) { > + list_del(&ac97_ctrl->controllers); > + sysfs_remove_group(&dev->kobj, > + &ac97_controller_attr_group); > + } > + } > + mutex_unlock(&ac97_controllers_mutex); > + > + return ret; > +} > +EXPORT_SYMBOL(snd_ac97_controller_unregister); [...] > +static struct bus_type ac97_bus_type = { > + .name = "ac97", > + .dev_attrs = ac97_dev_attrs, dev_attrs is deprecated in favor of dev_groups (See commit 880ffb5c6). > + .match = ac97_bus_match, > + .pm = &ac97_pm, > + .probe = ac97_bus_probe, > + .remove = ac97_bus_remove, > +}; > + > +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_* */ > + I'm not sure I understand what this file does. [...] -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen <lars@metafoo.de> writes: > On 10/26/2016 09:41 PM, Robert Jarzmik wrote: >> +#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) > > In my opinion these should be inline functions rather than macros as that > generates much more legible compiler errors e.g. in case there is a type > mismatch. Sure, why not. >> +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 *); > > The suspend, resume and shutdown callbacks are never used. Which is good, > since all new frameworks should use dev_pm_ops. Just drop the from the struct. Ok. >> +/** >> + * struct ac97_controller - The AC97 controller of the AC-Link >> + * @ops: the AC97 operations. >> + * @controllers: linked list of all existing controllers. >> + * @dev: 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; >> + struct list_head controllers; >> + struct device *dev; > > I'd make the controller itself a struct dev, rather than just having the > pointer to the parent. This is more idiomatic and matches what other > subsystems do. It has several advantages, you get proper refcounting of your > controller structure, the controller gets its own sysfs directory where the > CODECs appear as children, you don't need the manual sysfs attribute > creation and removal in ac97_controler_{un,}register(). If you mean having "struct device dev" instead of "struct device *dev", it has also a drawback : all the legacy platforms have already a probing method, be that platform data, device-tree or something else. I'm a bit converned about the conversion toll. Maybe I've not understood your point fully, so please feel free to explain, with an actual example even better. >> + void (*wait)(struct ac97_controller *adrv, int slot); >> + void (*init)(struct ac97_controller *adrv, int slot); > > Neither wait nor init are ever used. This is because I've not begun to porting sound/pci/ac97_codec.c into sound/ac97. >> + if ((codec_num < 0) || (codec_num >= AC97_BUS_MAX_CODECS)) >> + return ERR_PTR(-ERANGE); > > I'd make this EINVAL. Ok. >> + adev = container_of(dev, struct ac97_codec_device, dev); > > to_ac97_device() Sure. >> + codec_name = kasprintf(GFP_KERNEL, "%s:%d", dev_name(ac97_ctrl->dev), >> + idx); >> + codec->dev.init_name = codec_name; > > init_name is only for statically allocated devices. Use dev_set_name(dev, > ...). No need for kasprintf() either as dev_set_name() takes a format string. I'll try again, I seem to remember having tried that and it failed, but I can't remember why. > For this you need to split device_register into device_initialize() and > device_add(). But usually that is what you want anyway. Let me try again. >> + ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj, >> + "ac97_controller"); > > Since the CODEC is a child of the controller this should not be necessary as > this just points one directory up. It's like `ln -s .. parent` This creates : /sys/bus/ac97/devices/pxa2xx-ac97\:0/ac97_controller Of course as you pointed out, it's a 'ln -s .. parent' like link, but it seems to me very unfriendly to have : - /sys/bus/ac97/devices/pxa2xx-ac97\:0/../ac97_operations - while /sys/bus/ac97/devices/ac97_operations doesn't exist (obviously) Mmm, I don't know for this one, my mind is not set, it's a bit hard to tell if it's only an unecessary link bringing "comfort", or something usefull. > >> + if (ret) >> + goto err_unregister_device; >> + >> + return 0; >> +err_unregister_device: >> + put_device(&codec->dev); >> +err_free_codec: >> + kfree(codec); > > Since the struct is reference counted, the freeing is done in the release > callback and this leads to a double free. A yes in the "goto err_unregister_device" case right, while it's necessary in the "goto err_free_codec" case ... I need to rework that a bit. >> +int snd_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(); > > Rescanning the bus when a new codec driver is registered should not be > neccessary. The bus is scanned once when the controller is registered, this > creates the device. The device driver core will take care of binding the > device to the driver, if the driver is registered after thed evice. That's because you suppose the initial scanning found all the ac97 codecs. But that's an incomplete vision as a codec might be powered off at that time and not seen by the first scanning, while the new scanning will discover it. >> +static int ac97_ctrl_codecs_unregister(struct ac97_controller *ac97_ctrl) >> +{ >> + int i; >> + >> + for (i = 0; i < AC97_BUS_MAX_CODECS; i++) >> + if (ac97_ctrl->codecs[i]) >> + put_device(&ac97_ctrl->codecs[i]->dev); > > This should be device_unregister() to match the device_register() in > ac97_codec_add(). Right. >> + >> + return 0; >> +} >> + >> +static ssize_t cold_reset_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, >> + size_t len) >> +{ >> + struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev); >> + >> + if (!dev) >> + return -ENODEV; > > dev is never NULL here. Ok. > And for the ac97_ctrl there is a race condition. It could be unregistered and > freed after ac97_ctrl_find() returned sucessfully, but before ac97_ctrl->ops > is used. A good catch, the ac97_controllers_mutex is missing. > Same here. Indeed. >> + >> + ac97_ctrl->ops->warm_reset(ac97_ctrl); >> + return len; >> +} >> +static DEVICE_ATTR_WO(warm_reset); >> + >> +static struct attribute *ac97_controller_device_attrs[] = { >> + &dev_attr_cold_reset.attr, >> + &dev_attr_warm_reset.attr, >> + NULL >> +}; > > This adds new userspace ABI that is not documented at the moment. Very true. And as all userspace interface, it needs to be discussed. If nobody complains, I'll add the documentation for next patch round. > >> +int snd_ac97_controller_register(const struct ac97_controller_ops *ops, >> + struct device *dev, >> + unsigned short slots_available, >> + void **codecs_pdata) > > In my opinion this should return a handle to a ac97 controller which can > then be passed to snd_ac97_controller_unregister(). This is in my opinion > the better approach rather than looking up the controller by parent device. This is another "legacy drivers" issue. The legacy driver (the one probed by platform_data or devicetree) doesn't necessarily have a private structure to store this ac97_controller pointer. This enables an "easier" porting of existing drivers. >> +/** >> + * snd_ac97_controller_unregister - unregister an ac97 controller >> + * @dev: the device previously provided to ac97_controller_register() >> + * >> + * Returns 0 on success, negative upon error > > Unregister must not be able to fail. Hotunplug is one of the core concepts > of the device driver model and there is really nothing that can be done to > prevent a device from disappearing, so there is no sensible way of handling > the error (and your pxa driver modifications simply ignore it as well). > > This also means the framework needs to cope with the case where the > controller is removed and the CODEC devices are still present. All future > operations should return -ENODEV in that case. Ah ... that will require a serious modification, and I see your point, so I'll prepare this for next patchset. >> +static struct bus_type ac97_bus_type = { >> + .name = "ac97", >> + .dev_attrs = ac97_dev_attrs, > > dev_attrs is deprecated in favor of dev_groups (See commit 880ffb5c6). Ok. >> 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_* */ >> + > > I'm not sure I understand what this file does. Ah yes, I'll remove it. It's the future conversion of sound/pci/ac97_codec.c, but it's ... empty so far. Thanks very much for the very detailed review.
On 11/08/2016 10:18 PM, Robert Jarzmik wrote: [...] >>> +/** >>> + * struct ac97_controller - The AC97 controller of the AC-Link >>> + * @ops: the AC97 operations. >>> + * @controllers: linked list of all existing controllers. >>> + * @dev: 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; >>> + struct list_head controllers; >>> + struct device *dev; >> >> I'd make the controller itself a struct dev, rather than just having the >> pointer to the parent. This is more idiomatic and matches what other >> subsystems do. It has several advantages, you get proper refcounting of your >> controller structure, the controller gets its own sysfs directory where the >> CODECs appear as children, you don't need the manual sysfs attribute >> creation and removal in ac97_controler_{un,}register(). > > If you mean having "struct device dev" instead of "struct device *dev", it has > also a drawback : all the legacy platforms have already a probing method, be > that platform data, device-tree or something else. > > I'm a bit converned about the conversion toll. Maybe I've not understood your > point fully, so please feel free to explain, with an actual example even better. This would be a struct device that is not bound to a driver, but just acts as a shell for the controller and places it inside the device hierarchy. You get reference counting and other management functions as well as a consistent naming scheme. E.g. you can call the devices ac97c%d (or something similar) and then call the CODEC ac97c%d.%d. This is how most frameworks implementing some kind of control bus are structured in the Linux kernel. E.g. take a look at I2C or SPI. Your controller driver itself is unaffected by this, you still call snd_ac97_controller_register() from the probe function and so on. > >>> + void (*wait)(struct ac97_controller *adrv, int slot); >>> + void (*init)(struct ac97_controller *adrv, int slot); >> >> Neither wait nor init are ever used. > This is because I've not begun to porting sound/pci/ac97_codec.c into > sound/ac97. Ok, makes sense. But maybe just leave them out for now and add them when they are used. [...] >>> + ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj, >>> + "ac97_controller"); >> >> Since the CODEC is a child of the controller this should not be necessary as >> this just points one directory up. It's like `ln -s .. parent` > This creates : > /sys/bus/ac97/devices/pxa2xx-ac97\:0/ac97_controller > > Of course as you pointed out, it's a 'ln -s .. parent' like link, but it seems > to me very unfriendly to have : > - /sys/bus/ac97/devices/pxa2xx-ac97\:0/../ac97_operations > - while /sys/bus/ac97/devices/ac97_operations doesn't exist (obviously) > > Mmm, I don't know for this one, my mind is not set, it's a bit hard to tell if > it's only an unecessary link bringing "comfort", or something usefull. It is additional ABI and we do not have this for any other bus either (e.g. you don't see a backlink for a I2C or SPI device to the parent). In my opinion for the sake of keeping things consistent and simple we should not add this. >> >>> + if (ret) >>> + goto err_unregister_device; >>> + >>> + return 0; >>> +err_unregister_device: >>> + put_device(&codec->dev); >>> +err_free_codec: >>> + kfree(codec); >> >> Since the struct is reference counted, the freeing is done in the release >> callback and this leads to a double free. > A yes in the "goto err_unregister_device" case right, while it's necessary in > the "goto err_free_codec" case ... I need to rework that a bit. It should use put_device() in both cases, check the the device_register() documentation. It says that put_device() must be used, even if device_register() fails. > >>> +int snd_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(); >> >> Rescanning the bus when a new codec driver is registered should not be >> neccessary. The bus is scanned once when the controller is registered, this >> creates the device. The device driver core will take care of binding the >> device to the driver, if the driver is registered after thed evice. > That's because you suppose the initial scanning found all the ac97 codecs. > But that's an incomplete vision as a codec might be powered off at that time and > not seen by the first scanning, while the new scanning will discover it. But why would the device become suddenly visible when the driver is registered. This seems to be an as arbitrary point in time as any other. Also consider that when you build a driver as a module, the module will typically only be auto-loaded after the device has been detected, based on the device ID. On the other hand, if the driver is built-in driver registration will happen either before or shortly after the controller registration. If there is the expectation that the AC97 CODEC might randomly appear on the bus, the core should periodically scan the bus. [...] >>> + ac97_ctrl->ops->warm_reset(ac97_ctrl); >>> + return len; >>> +} >>> +static DEVICE_ATTR_WO(warm_reset); >>> + >>> +static struct attribute *ac97_controller_device_attrs[] = { >>> + &dev_attr_cold_reset.attr, >>> + &dev_attr_warm_reset.attr, >>> + NULL >>> +}; >> >> This adds new userspace ABI that is not documented at the moment. > Very true. And as all userspace interface, it needs to be discussed. If nobody > complains, I'll add the documentation for next patch round. >> >>> +int snd_ac97_controller_register(const struct ac97_controller_ops *ops, >>> + struct device *dev, >>> + unsigned short slots_available, >>> + void **codecs_pdata) >> >> In my opinion this should return a handle to a ac97 controller which can >> then be passed to snd_ac97_controller_unregister(). This is in my opinion >> the better approach rather than looking up the controller by parent device. > This is another "legacy drivers" issue. > > The legacy driver (the one probed by platform_data or devicetree) doesn't > necessarily have a private structure to store this ac97_controller pointer. I might be missing something, but I'm not convinced by this. Can you point me to such a legacy driver where you think this would not work? -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen <lars@metafoo.de> writes: > On 11/08/2016 10:18 PM, Robert Jarzmik wrote: >>> I'd make the controller itself a struct dev, rather than just having the >>> pointer to the parent. This is more idiomatic and matches what other >>> subsystems do. It has several advantages, you get proper refcounting of your >>> controller structure, the controller gets its own sysfs directory where the >>> CODECs appear as children, you don't need the manual sysfs attribute >>> creation and removal in ac97_controler_{un,}register(). >> >> If you mean having "struct device dev" instead of "struct device *dev", it has >> also a drawback : all the legacy platforms have already a probing method, be >> that platform data, device-tree or something else. >> >> I'm a bit converned about the conversion toll. Maybe I've not understood your >> point fully, so please feel free to explain, with an actual example even better. > > This would be a struct device that is not bound to a driver, but just acts > as a shell for the controller and places it inside the device hierarchy. You > get reference counting and other management functions as well as a > consistent naming scheme. E.g. you can call the devices ac97c%d (or > something similar) and then call the CODEC ac97c%d.%d. Let me think about it. If I get you right, the device model would be, from a parenthood point of view: pxa2xx_ac97 (platform_device) ^ | ac97_controller.dev (device, son of pxa2xx_ac97) / \ / \ / \ / \ / \ wm97xx-core codec2 (sons ac97_controller.dev) I have already the device hierarchy AFAIK, created in ac97_codec_add(). I'm still failing to see the difference, as the refcounting is already used. The only additional thing I see is the introduction of an intermediate ac97_controller.dev which is refcounted. In current model, pxa2xx_ac97 refcount is incremented for each codec device. In the one you propose, pxa2xx_ac97 will be 1 refcounted (maybe 2 actually), and ac97_controller.dev will be refcounted for each codec. This ac97_controller.dev will be a spiritual twin of spi_master/i2c_adapter. As I said, I must think about it and find which value is brought by this additionnal layer. As for the name change, I must check if this breaks the sound machine files, and their dai_link structures (ex: mioa701_wm9713.c, structure mioa701_dai). In a former state of this patchset I had changed the device names, ie. wm9713-codec became pxa2xx-ac97.0, and the my machine file became broken. > This is how most frameworks implementing some kind of control bus are > structured in the Linux kernel. E.g. take a look at I2C or SPI. I will. >>>> + ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj, >>>> + "ac97_controller"); >>> >>> Since the CODEC is a child of the controller this should not be necessary as >>> this just points one directory up. It's like `ln -s .. parent` >> This creates : >> /sys/bus/ac97/devices/pxa2xx-ac97\:0/ac97_controller >> >> Of course as you pointed out, it's a 'ln -s .. parent' like link, but it seems >> to me very unfriendly to have : >> - /sys/bus/ac97/devices/pxa2xx-ac97\:0/../ac97_operations >> - while /sys/bus/ac97/devices/ac97_operations doesn't exist (obviously) >> >> Mmm, I don't know for this one, my mind is not set, it's a bit hard to tell if >> it's only an unecessary link bringing "comfort", or something usefull. > > It is additional ABI and we do not have this for any other bus either (e.g. > you don't see a backlink for a I2C or SPI device to the parent). In my > opinion for the sake of keeping things consistent and simple we should not > add this. Fair enough. >>>> +int snd_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(); >>> >>> Rescanning the bus when a new codec driver is registered should not be >>> neccessary. The bus is scanned once when the controller is registered, this >>> creates the device. The device driver core will take care of binding the >>> device to the driver, if the driver is registered after thed evice. >> That's because you suppose the initial scanning found all the ac97 codecs. >> But that's an incomplete vision as a codec might be powered off at that time and >> not seen by the first scanning, while the new scanning will discover it. > > But why would the device become suddenly visible when the driver is > registered. This seems to be an as arbitrary point in time as any other. Because in the meantime, a regulator or something else provided power to the AC97 IP, or a clock, and it can answer to requests after that. And another point if that the clock of controller might be provided by the AC97 codec, and the scan will only succeed once the codec is actually providing this clock, which it might do only once the module_init() is done. > Also consider that when you build a driver as a module, the module will > typically only be auto-loaded after the device has been detected, based on > the device ID. On the other hand, if the driver is built-in driver > registration will happen either before or shortly after the controller > registration. > If there is the expectation that the AC97 CODEC might randomly appear on the > bus, the core should periodically scan the bus. Power wise a periodical scan doesn't look good, at all. More globally, I don't see if there is an actual issue we're trying to address here, ie. that the rescan is a bug, or if it's more an "Occam's razor" discussion ? >>> In my opinion this should return a handle to a ac97 controller which can >>> then be passed to snd_ac97_controller_unregister(). This is in my opinion >>> the better approach rather than looking up the controller by parent device. >> This is another "legacy drivers" issue. >> >> The legacy driver (the one probed by platform_data or devicetree) doesn't >> necessarily have a private structure to store this ac97_controller pointer. > > I might be missing something, but I'm not convinced by this. Can you point > me to such a legacy driver where you think this would not work? The first one that popped out: - hac_soc_platform_probe() Cheers.
On 11/09/2016 10:27 PM, Robert Jarzmik wrote: [...] >>>>> +int snd_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(); >>>> >>>> Rescanning the bus when a new codec driver is registered should not be >>>> neccessary. The bus is scanned once when the controller is registered, this >>>> creates the device. The device driver core will take care of binding the >>>> device to the driver, if the driver is registered after thed evice. >>> That's because you suppose the initial scanning found all the ac97 codecs. >>> But that's an incomplete vision as a codec might be powered off at that time and >>> not seen by the first scanning, while the new scanning will discover it. >> >> But why would the device become suddenly visible when the driver is >> registered. This seems to be an as arbitrary point in time as any other. > Because in the meantime, a regulator or something else provided power to the > AC97 IP, or a clock, and it can answer to requests after that. > > And another point if that the clock of controller might be provided by the AC97 > codec, and the scan will only succeed once the codec is actually providing this > clock, which it might do only once the module_init() is done. > >> Also consider that when you build a driver as a module, the module will >> typically only be auto-loaded after the device has been detected, based on >> the device ID. On the other hand, if the driver is built-in driver >> registration will happen either before or shortly after the controller >> registration. >> If there is the expectation that the AC97 CODEC might randomly appear on the >> bus, the core should periodically scan the bus. > Power wise a periodical scan doesn't look good, at all. > > More globally, I don't see if there is an actual issue we're trying to address > here, ie. that the rescan is a bug, or if it's more an "Occam's razor" > discussion ? It's a framework design discussion. In my opinion the driver being registered and the device becoming visible on the physical bus are two completely unrelated operations. Especially in the Linux device driver model. You have a generic driver that calls ac97_codec_driver_register() in its module_init() section. This generic driver does (at module_init() time) not know anything about a device instance specific clocks, regulators or other resources. Resources are handled on a per device instance basis, and you won't have a device instance until the device has been detected, which happens in ac97_bus_scan(). So you have a cyclic dependency loop here. Maybe we can just leave the rescanning out for now and think about how to best handle it when the need arises. > >>>> In my opinion this should return a handle to a ac97 controller which can >>>> then be passed to snd_ac97_controller_unregister(). This is in my opinion >>>> the better approach rather than looking up the controller by parent device. >>> This is another "legacy drivers" issue. >>> >>> The legacy driver (the one probed by platform_data or devicetree) doesn't >>> necessarily have a private structure to store this ac97_controller pointer. >> >> I might be missing something, but I'm not convinced by this. Can you point >> me to such a legacy driver where you think this would not work? > The first one that popped out: > - hac_soc_platform_probe() I think that driver should be able to use platform_set_drvdata() to store the pointer. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 10, 2016 at 12:38:40PM +0100, Lars-Peter Clausen wrote: > On 11/09/2016 10:27 PM, Robert Jarzmik wrote: > > here, ie. that the rescan is a bug, or if it's more an "Occam's razor" > > discussion ? > Maybe we can just leave the rescanning out for now and think about how to > best handle it when the need arises. Yes, I think that's best - I suspect it'd be something that'd need to be triggered by a driver for an AC'97 CODEC doing power management but I imagine that if we were going to have such a system we'd have run into it already anyway.
Lars-Peter Clausen <lars@metafoo.de> writes: ... Ok Lars, let's do this : I integrate as many of your remarks as I can, and I respin this serie, limited to patches 1, 2, 3, 4 and 5. Then we'll iterate, that will enable me to settle the serie, as I was on holidays and lost a bit my track. I'll send it hopefully next week once I've gathered my thoughts. Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/sound/ac97/codec.h b/include/sound/ac97/codec.h new file mode 100644 index 000000000000..8901c1200522 --- /dev/null +++ b/include/sound/ac97/codec.h @@ -0,0 +1,115 @@ +/* + * 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 __SOUND_AC97_CODEC2_H +#define __SOUND_AC97_CODEC2_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 clk; + +/** + * 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 core 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 ... + * @clk: the clock BIT_CLK provided by the codec + * @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; + unsigned int vendor_id; + unsigned int num; + struct clk *clk; + 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 *); + const struct ac97_id *id_table; +}; + +#if defined(CONFIG_AC97_BUS_NEW) +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv); +void snd_ac97_codec_driver_unregister(struct ac97_codec_driver *drv); +#else +static inline int +snd_ac97_codec_driver_register(struct ac97_codec_driver *drv) +{ + return 0; +} +static inline void +snd_ac97_codec_driver_unregister(struct ac97_codec_driver *drv) +{ +} +#endif + + +static inline struct device * +ac97_codec_dev2dev(struct ac97_codec_device *adev) +{ + return &adev->dev; +} + +static inline void *ac97_get_drvdata(struct ac97_codec_device *adev) +{ + return dev_get_drvdata(ac97_codec_dev2dev(adev)); +} + +static inline void ac97_set_drvdata(struct ac97_codec_device *adev, + void *data) +{ + dev_set_drvdata(ac97_codec_dev2dev(adev), data); +} + +void *snd_ac97_codec_get_platdata(const struct ac97_codec_device *adev); + +#endif 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> + +struct snd_ac97 *snd_ac97_compat_alloc(struct ac97_codec_device *adev); +void snd_ac97_compat_release(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..5ff59bd7e324 --- /dev/null +++ b/include/sound/ac97/controller.h @@ -0,0 +1,85 @@ +/* + * 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; + +/** + * struct ac97_controller - The AC97 controller of the AC-Link + * @ops: the AC97 operations. + * @controllers: linked list of all existing controllers. + * @dev: 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; + struct list_head controllers; + struct device *dev; + unsigned short slots_available; + struct ac97_codec_device *codecs[AC97_BUS_MAX_CODECS]; + void *codecs_pdata[AC97_BUS_MAX_CODECS]; +}; + +/** + * struct ac97_controller_ops - The AC97 operations + * @reset: Cold reset of the AC97 AC-Link. + * @warm_reset: Warm reset of the AC97 AC-Link. + * @read: Read of a single AC97 register. + * Returns the register value or a negative error code. + * @write: Write of a single AC97 register. + * @wait: Wait for the current AC97 operation to finish (might be NULL). + * @init: Initialization of the AC97 AC-Link (might be NULL). + * + * These are the basic operation an AC97 controller must provide for an AC97 + * access functions. Amongst these, all but the last 2 are mandatory. + * The slot number is also known as the AC97 codec number, between 0 and 3. + */ +struct ac97_controller_ops { + void (*reset)(struct ac97_controller *adrv); + void (*warm_reset)(struct ac97_controller *adrv); + int (*write)(struct ac97_controller *adrv, int slot, + unsigned short reg, unsigned short val); + int (*read)(struct ac97_controller *adrv, int slot, unsigned short reg); + void (*wait)(struct ac97_controller *adrv, int slot); + void (*init)(struct ac97_controller *adrv, int slot); +}; + +#if defined(CONFIG_AC97_BUS_NEW) +int snd_ac97_controller_register(const struct ac97_controller_ops *ops, + struct device *dev, + unsigned short slots_available, + void **codecs_pdata); +int snd_ac97_controller_unregister(struct device *dev); +#else +static inline int +snd_ac97_controller_register(const struct ac97_controller_ops *ops, + struct device *dev, + unsigned short slots_available, + void **codecs_pdata) +{ + return 0; +} + +static inline int snd_ac97_controller_unregister(struct device *dev) +{ + return 0; +} +#endif + +#endif diff --git a/sound/ac97/Kconfig b/sound/ac97/Kconfig new file mode 100644 index 000000000000..f8a64e15e5bf --- /dev/null +++ b/sound/ac97/Kconfig @@ -0,0 +1,19 @@ +# +# AC97 configuration +# + + +config AC97_BUS_NEW + tristate + select AC97 + help + This is the new AC97 bus type, successor of AC97_BUS. The ported + drivers which benefit from the AC97 automatic probing should "select" + this instead of the AC97_BUS. + Say Y here if you want to have AC97 devices, which are sound oriented + devices around an AC-Link. + +config AC97_BUS_COMPAT + bool + depends on AC97_BUS_NEW + depends on !AC97_BUS diff --git a/sound/ac97/Makefile b/sound/ac97/Makefile new file mode 100644 index 000000000000..f9c2640bfb59 --- /dev/null +++ b/sound/ac97/Makefile @@ -0,0 +1,8 @@ +# +# make for AC97 bus drivers +# + +obj-$(CONFIG_AC97_BUS_NEW) += ac97.o + +ac97-y += bus.o codec.o +ac97-$(CONFIG_AC97_BUS_COMPAT) += snd_ac97_compat.o diff --git a/sound/ac97/ac97_core.h b/sound/ac97/ac97_core.h new file mode 100644 index 000000000000..10cdfad8ad52 --- /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 snd_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..6f26053d8be9 --- /dev/null +++ b/sound/ac97/bus.c @@ -0,0 +1,510 @@ +/* + * 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/bitops.h> +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/list.h> +#include <linux/mutex.h> +#include <linux/pm.h> +#include <linux/pm_runtime.h> +#include <linux/slab.h> +#include <linux/sysfs.h> +#include <sound/ac97/codec.h> +#include <sound/ac97/controller.h> +#include <sound/ac97/regs.h> + +#include "ac97_core.h" + +/* + * Protects ac97_controllers and each ac97_controller structure. + */ +static DEFINE_MUTEX(ac97_controllers_mutex); +static LIST_HEAD(ac97_controllers); + +static struct bus_type ac97_bus_type; + +static struct ac97_codec_device * +ac97_codec_find(struct ac97_controller *ac97_ctrl, int codec_num) +{ + if ((codec_num < 0) || (codec_num >= AC97_BUS_MAX_CODECS)) + return ERR_PTR(-ERANGE); + + return ac97_ctrl->codecs[codec_num]; +} + +static void ac97_codec_release(struct device *dev) +{ + struct ac97_codec_device *adev; + struct ac97_controller *ac97_ctrl; + + adev = container_of(dev, struct ac97_codec_device, dev); + ac97_ctrl = adev->ac97_ctrl; + ac97_ctrl->codecs[adev->num] = NULL; + sysfs_remove_link(&dev->kobj, "ac97_controller"); + kfree(adev); +} + +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; + 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->dev; + codec->num = idx; + codec->ac97_ctrl = ac97_ctrl; + + 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); + if (ret) + goto err_free_codec; + + ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj, + "ac97_controller"); + if (ret) + goto err_unregister_device; + + return 0; +err_unregister_device: + put_device(&codec->dev); +err_free_codec: + kfree(codec); + ac97_ctrl->codecs[idx] = NULL; + + return ret; +} + +unsigned int snd_ac97_bus_scan_one(struct ac97_controller *adrv, + 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->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; + 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 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); +} + +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); + if (!ret) + ac97_rescan_all_controllers(); + + return ret; +} +EXPORT_SYMBOL(snd_ac97_codec_driver_register); + +/** + * snd_ac97_codec_driver_unregister - unregister an AC97 codec driver + * @dev: AC97 codec driver to unregister + * + * Unregister a previously registered ac97 codec driver. + */ +void snd_ac97_codec_driver_unregister(struct ac97_codec_driver *drv) +{ + driver_unregister(&drv->driver); +} +EXPORT_SYMBOL(snd_ac97_codec_driver_unregister); + +/** + * snd_ac97_codec_get_platdata - get platform_data + * @adev: the ac97 codec device + * + * For legacy platforms, in order to have platform_data in codec drivers + * available, while ac97 device are auto-created upon probe, this retrieves the + * platdata which was setup on ac97 controller registration. + * + * Returns the platform data pointer + */ +void *snd_ac97_codec_get_platdata(const struct ac97_codec_device *adev) +{ + struct ac97_controller *ac97_ctrl = adev->ac97_ctrl; + + return ac97_ctrl->codecs_pdata[adev->num]; +} +EXPORT_SYMBOL(snd_ac97_codec_get_platdata); + +static struct ac97_controller *ac97_ctrl_find(struct device *dev) +{ + struct ac97_controller *ac97_ctrl, *tmp; + + list_for_each_entry_safe(ac97_ctrl, tmp, &ac97_controllers, + controllers) + if (ac97_ctrl->dev == dev) + return ac97_ctrl; + + return NULL; +} +static int ac97_ctrl_codecs_unregister(struct ac97_controller *ac97_ctrl) +{ + int i; + + for (i = 0; i < AC97_BUS_MAX_CODECS; i++) + if (ac97_ctrl->codecs[i]) + put_device(&ac97_ctrl->codecs[i]->dev); + + return 0; +} + +static ssize_t cold_reset_store(struct device *dev, + struct device_attribute *attr, const char *buf, + size_t len) +{ + struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev); + + if (!dev) + return -ENODEV; + + ac97_ctrl->ops->reset(ac97_ctrl); + return len; +} +static DEVICE_ATTR_WO(cold_reset); + +static ssize_t warm_reset_store(struct device *dev, + struct device_attribute *attr, const char *buf, + size_t len) +{ + struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev); + + if (!dev) + return -ENODEV; + + ac97_ctrl->ops->warm_reset(ac97_ctrl); + return len; +} +static DEVICE_ATTR_WO(warm_reset); + +static struct attribute *ac97_controller_device_attrs[] = { + &dev_attr_cold_reset.attr, + &dev_attr_warm_reset.attr, + NULL +}; + +static const struct attribute_group ac97_controller_attr_group = { + .name = "ac97_operations", + .attrs = ac97_controller_device_attrs, +}; + +/** + * snd_ac97_controller_register - register an ac97 controller + * @ops: the ac97 bus operations + * @dev: the device providing the ac97 DC function + * @slots_available: mask of the ac97 codecs that can be scanned and probed + * bit0 => codec 0, bit1 => codec 1 ... bit 3 => codec 3 + * + * 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 0 upon success, negative value upon error + */ +int snd_ac97_controller_register(const struct ac97_controller_ops *ops, + struct device *dev, + unsigned short slots_available, + void **codecs_pdata) +{ + struct ac97_controller *ac97_ctrl; + int ret, i; + + ac97_ctrl = kzalloc(sizeof(*ac97_ctrl), GFP_KERNEL); + if (!ac97_ctrl) + return -ENOMEM; + + for (i = 0; i < AC97_BUS_MAX_CODECS && codecs_pdata; i++) + ac97_ctrl->codecs_pdata[i] = codecs_pdata[i]; + + ret = sysfs_create_group(&dev->kobj, &ac97_controller_attr_group); + if (ret) + return ret; + + mutex_lock(&ac97_controllers_mutex); + ac97_ctrl->ops = ops; + ac97_ctrl->slots_available = slots_available; + ac97_ctrl->dev = dev; + list_add(&ac97_ctrl->controllers, &ac97_controllers); + mutex_unlock(&ac97_controllers_mutex); + + ac97_bus_reset(ac97_ctrl); + ac97_bus_scan(ac97_ctrl); + + return 0; +} +EXPORT_SYMBOL(snd_ac97_controller_register); + +/** + * snd_ac97_controller_unregister - unregister an ac97 controller + * @dev: the device previously provided to ac97_controller_register() + * + * Returns 0 on success, negative upon error + */ +int snd_ac97_controller_unregister(struct device *dev) +{ + struct ac97_controller *ac97_ctrl; + int ret = -ENODEV, i; + + mutex_lock(&ac97_controllers_mutex); + ac97_ctrl = ac97_ctrl_find(dev); + if (ac97_ctrl) { + ret = 0; + for (i = 0; i < AC97_BUS_MAX_CODECS; i++) + if (ac97_ctrl->codecs[i] && + device_is_registered(&ac97_ctrl->codecs[i]->dev)) + ret = -EBUSY; + if (!ret) + ret = ac97_ctrl_codecs_unregister(ac97_ctrl); + if (!ret) { + list_del(&ac97_ctrl->controllers); + sysfs_remove_group(&dev->kobj, + &ac97_controller_attr_group); + } + } + mutex_unlock(&ac97_controllers_mutex); + + return ret; +} +EXPORT_SYMBOL(snd_ac97_controller_unregister); + +#ifdef CONFIG_PM +static int ac97_pm_runtime_suspend(struct device *dev) +{ + struct ac97_codec_device *codec = to_ac97_device(dev); + int ret = pm_generic_runtime_suspend(dev); + + if (ret == 0 && dev->driver) { + if (pm_runtime_is_irq_safe(dev)) + clk_disable(codec->clk); + else + clk_disable_unprepare(codec->clk); + } + + return ret; +} + +static int ac97_pm_runtime_resume(struct device *dev) +{ + struct ac97_codec_device *codec = to_ac97_device(dev); + int ret; + + if (dev->driver) { + if (pm_runtime_is_irq_safe(dev)) + ret = clk_enable(codec->clk); + else + ret = clk_prepare_enable(codec->clk); + if (ret) + return ret; + } + + return pm_generic_runtime_resume(dev); +} +#endif /* CONFIG_PM */ + +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, + SET_RUNTIME_PM_OPS( + ac97_pm_runtime_suspend, + ac97_pm_runtime_resume, + NULL) +}; + +static int ac97_get_enable_clk(struct ac97_codec_device *adev) +{ + int ret; + + adev->clk = clk_get(&adev->dev, "ac97_clk"); + if (IS_ERR(adev->clk)) + return PTR_ERR(adev->clk); + + ret = clk_prepare_enable(adev->clk); + if (ret) + clk_put(adev->clk); + + return ret; +} + +static void ac97_put_disable_clk(struct ac97_codec_device *adev) +{ + clk_disable_unprepare(adev->clk); + clk_put(adev->clk); +} + +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, +}; + +static 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); + const struct ac97_id *id = adrv->id_table; + int i = 0; + + if (adev->vendor_id == 0x0 || adev->vendor_id == 0xffffffff) + return false; + + do { + if ((id[i].id & id->mask) == (adev->vendor_id & id[i].mask)) + return true; + } while (id[i++].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); + int ret; + + ret = ac97_get_enable_clk(adev); + if (ret) + return ret; + + pm_runtime_get_noresume(dev); + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + + ret = adrv->probe(adev); + if (ret == 0) + return 0; + + pm_runtime_disable(dev); + pm_runtime_set_suspended(dev); + pm_runtime_put_noidle(dev); + ac97_put_disable_clk(adev); + + return ret; +} + +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); + int ret; + + ret = pm_runtime_get_sync(dev); + if (ret) + return ret; + + ret = adrv->remove(adev); + pm_runtime_put_noidle(dev); + if (ret == 0) + ac97_put_disable_clk(adev); + + return ret; +} + +static 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, +}; + +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..ac8d835c1513 --- /dev/null +++ b/sound/ac97/snd_ac97_compat.c @@ -0,0 +1,105 @@ +/* + * 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/compat.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(actrl); +} + +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(actrl); +} + +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(actrl, ac97->num, 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(actrl, ac97->num, 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 *snd_ac97_compat_alloc(struct ac97_codec_device *adev) +{ + struct snd_ac97 *ac97; + + ac97 = kzalloc(sizeof(struct snd_ac97), GFP_KERNEL); + if (ac97 == NULL) + return ERR_PTR(-ENOMEM); + + ac97->dev = adev->dev; + ac97->private_data = adev; + ac97->bus = &compat_soc_ac97_bus; + return ac97; +} +EXPORT_SYMBOL_GPL(snd_ac97_compat_alloc); + +void snd_ac97_compat_release(struct snd_ac97 *ac97) +{ + kfree(ac97); +} +EXPORT_SYMBOL_GPL(snd_ac97_compat_release); + +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) + return 1; + } + + compat_ac97_reset(ac97); + compat_ac97_warm_reset(ac97); + if (snd_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> --- Since v1: - 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 v2: - more snd_ac97 namespace review - change the compat allocation prototype to force the user to provide and ac97_codec_device structure pointer --- include/sound/ac97/codec.h | 115 +++++++++ include/sound/ac97/compat.h | 21 ++ include/sound/ac97/controller.h | 85 +++++++ sound/ac97/Kconfig | 19 ++ sound/ac97/Makefile | 8 + sound/ac97/ac97_core.h | 10 + sound/ac97/bus.c | 510 ++++++++++++++++++++++++++++++++++++++++ sound/ac97/codec.c | 15 ++ sound/ac97/snd_ac97_compat.c | 105 +++++++++ 9 files changed, 888 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