[RFC 0/3] ALSA: Add soc hda bus support

[RFC 0/3] ALSA: Add soc hda bus support

[Vinod Koul]

This series of 3 patches adds support for ASoC HDA
bus. ASoC HDA bus is essentially a wrapper for
HDA core bus and it manages ASoC hda drivers

Ramesh Babu (3):
  ALSA: hda - remove assigning dev type inside core
  ALSA: hda - add ASoC device type for hda core
  ALSA: hda - add soc hda bus wrapper

 include/sound/hdaudio.h      |    1 +
 include/sound/soc-hdac-bus.h |  103 +++++++++++++++++++
 sound/hda/hdac_device.c      |    1 -
 sound/soc/Kconfig            |    1 +
 sound/soc/Makefile           |    1 +
 sound/soc/hda/Kconfig        |    4 +
 sound/soc/hda/Makefile       |    3 +
 sound/soc/hda/soc-hdac-bus.c |  233 ++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 346 insertions(+), 1 deletion(-)
 create mode 100644 include/sound/soc-hdac-bus.h
 create mode 100644 sound/soc/hda/Kconfig
 create mode 100644 sound/soc/hda/Makefile
 create mode 100644 sound/soc/hda/soc-hdac-bus.c

-- 
1.7.9.5

[RFC 1/3] ALSA: hda - remove assigning dev type inside core

[Vinod Koul]

From: Ramesh Babu <ramesh.babu@intel.com>

We don't need to init the hda dev type inside
core device init code. It is expected to be
initialized by the caller.

Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/hda/hdac_device.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index 6e8ee1d6974a..aefc80ffecee 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -51,7 +51,6 @@ int snd_hdac_device_init(struct hdac_device *codec, struct hdac_bus *bus,
 
 	codec->bus = bus;
 	codec->addr = addr;
-	codec->type = HDA_DEV_CORE;
 	pm_runtime_set_active(&codec->dev);
 	pm_runtime_get_noresume(&codec->dev);
 	atomic_set(&codec->in_pm, 0);
-- 
1.7.9.5

[RFC 2/3] ALSA: hda - add ASoC device type for hda core

[Vinod Koul]

From: Ramesh Babu <ramesh.babu@intel.com>

Add HDA_DEV_ASOC device/driver type to support
ASoC HDA drivers.

Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/sound/hdaudio.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 9aeede3c8ea1..1652764ed770 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -88,6 +88,7 @@ struct hdac_device {
 enum {
 	HDA_DEV_CORE,
 	HDA_DEV_LEGACY,
+	HDA_DEV_ASOC,
 };
 
 /* direction */
-- 
1.7.9.5

[RFC 3/3] ALSA: hda - add soc hda bus wrapper

[Vinod Koul]

From: Ramesh Babu <ramesh.babu@intel.com>

The SKL controller will be ASoC device but needs access to HDA code, so
create asoc wrppers on top of hdac.

Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/sound/soc-hdac-bus.h |  103 +++++++++++++++++++
 sound/soc/Kconfig            |    1 +
 sound/soc/Makefile           |    1 +
 sound/soc/hda/Kconfig        |    4 +
 sound/soc/hda/Makefile       |    3 +
 sound/soc/hda/soc-hdac-bus.c |  233 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 345 insertions(+)
 create mode 100644 include/sound/soc-hdac-bus.h
 create mode 100644 sound/soc/hda/Kconfig
 create mode 100644 sound/soc/hda/Makefile
 create mode 100644 sound/soc/hda/soc-hdac-bus.c

diff --git a/include/sound/soc-hdac-bus.h b/include/sound/soc-hdac-bus.h
new file mode 100644
index 000000000000..aaa91ef934c5
--- /dev/null
+++ b/include/sound/soc-hdac-bus.h
@@ -0,0 +1,103 @@
+/*
+ *  soc-hdac-bus.h - ASoC HDA bus wrapper
+ *
+ *  Copyright (c) 2015 Intel Corporation
+ *  Author: Jeeja KP <jeeja.kp@intel.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under the terms of the GNU General Public License as published by the Free
+ *  Software Foundation; either version 2 of the License, or (at your option)
+ *  any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but WITHOUT
+ *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ *  more details.
+ *
+ */
+
+#ifndef _SOC_HDAC_BUS_H_
+#define _SOC_HDAC_BUS_H_
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <sound/hdaudio.h>
+
+struct snd_soc_hdac_device {
+	struct hdac_device hdac;
+	struct snd_soc_hdac_bus *hbus;
+	const char	*name;
+	unsigned int	id;
+	const struct snd_soc_hda_device_id *id_entry;
+};
+
+#define soc_hda_get_device_id(pdev)    ((pdev)->id_entry)
+
+#define to_soc_hdac_device(x) container_of((x), struct snd_soc_hdac_device, hdac.dev)
+
+int snd_soc_hdac_device_register(struct snd_soc_hdac_device *);
+void snd_soc_hdac_device_unregister(struct snd_soc_hdac_device *);
+
+int snd_soc_hdac_add_devices(struct snd_soc_hdac_device **, int);
+struct snd_soc_hdac_device *snd_soc_hdac_device_alloc(const char *name,
+					int addr, unsigned int id);
+
+int snd_soc_hdac_device_add_data(struct snd_soc_hdac_device *pdev,
+				const void *data, size_t size);
+int snd_soc_hdac_device_add(struct hdac_stream *hstream, struct snd_soc_hdac_device *pdev);
+void snd_soc_hdac_device_del(struct snd_soc_hdac_device *pdev);
+void snd_soc_hdac_device_put(struct snd_soc_hdac_device *pdev);
+
+struct snd_soc_hdac_driver {
+	struct hdac_driver hdrv;
+	const struct snd_soc_hda_device_id *id_table;
+	int	(*probe)(struct snd_soc_hdac_device *hda);
+	int	(*remove)(struct snd_soc_hdac_device *hda);
+	void	(*shutdown)(struct snd_soc_hdac_device *hda);
+	int	(*suspend)(struct snd_soc_hdac_device *hda, pm_message_t mesg);
+	int	(*resume)(struct snd_soc_hdac_device *hda);
+	void	(*unsol_event)(struct snd_soc_hdac_device *hda,
+					unsigned int res);
+};
+
+#define to_soc_hdac_driver(drv) (container_of((drv), struct snd_soc_hdac_driver, \
+				hdrv.driver))
+
+int snd_soc_hdac_driver_register(struct snd_soc_hdac_driver *);
+void snd_soc_hdac_driver_unregister(struct snd_soc_hdac_driver *);
+static inline void *snd_soc_hdac_get_drvdata(
+			const struct snd_soc_hdac_device *pdev)
+{
+	return dev_get_drvdata(&pdev->hdac.dev);
+}
+
+static inline void snd_soc_hdac_set_drvdata(struct snd_soc_hdac_device *pdev,
+						void *data)
+{
+	dev_set_drvdata(&pdev->hdac.dev, data);
+}
+
+struct snd_soc_hdac_bus {
+	struct hdac_bus bus;
+	/* unsolicited event queue */
+	struct snd_soc_hdac_bus_unsolicited *unsol;
+	struct pci_dev *pci;
+	struct mutex prepare_mutex;
+};
+
+int snd_soc_hdac_bus_init(struct pci_dev *pci, void *data,
+			struct hdac_bus_ops ops,
+			struct snd_soc_hdac_bus **busp);
+void snd_soc_hdac_bus_release(struct snd_soc_hdac_bus *hbus);
+
+/*Soc HDA Device*/
+#define HDA_NAME_SIZE      20
+#define HDA_MODULE_PREFIX  "hda:"
+struct snd_soc_hda_device_id {
+	__u32 id;
+	__u8 addr;
+	char name[HDA_NAME_SIZE];
+	kernel_ulong_t driver_data;
+};
+
+#endif /* _SOC_HDAC_BUS_H_ */
diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index dcc79aa0236b..c19b882a0854 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -40,6 +40,7 @@ source "sound/soc/cirrus/Kconfig"
 source "sound/soc/davinci/Kconfig"
 source "sound/soc/dwc/Kconfig"
 source "sound/soc/fsl/Kconfig"
+source "sound/soc/hda/Kconfig"
 source "sound/soc/jz4740/Kconfig"
 source "sound/soc/nuc900/Kconfig"
 source "sound/soc/omap/Kconfig"
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 5b3c8f67c8db..3269d19b20b9 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_SND_SOC)	+= cirrus/
 obj-$(CONFIG_SND_SOC)	+= davinci/
 obj-$(CONFIG_SND_SOC)	+= dwc/
 obj-$(CONFIG_SND_SOC)	+= fsl/
+obj-$(CONFIG_SND_SOC)	+= hda/
 obj-$(CONFIG_SND_SOC)	+= jz4740/
 obj-$(CONFIG_SND_SOC)	+= intel/
 obj-$(CONFIG_SND_SOC)	+= mxs/
diff --git a/sound/soc/hda/Kconfig b/sound/soc/hda/Kconfig
new file mode 100644
index 000000000000..09ef831b8513
--- /dev/null
+++ b/sound/soc/hda/Kconfig
@@ -0,0 +1,4 @@
+config SND_SOC_HDA_BUS
+	tristate "SoC HDA bus wrapper"
+	help
+	 This adds support the SOC HDA Bus wrapper
diff --git a/sound/soc/hda/Makefile b/sound/soc/hda/Makefile
new file mode 100644
index 000000000000..0cb26a68f2dc
--- /dev/null
+++ b/sound/soc/hda/Makefile
@@ -0,0 +1,3 @@
+#ccflags-y += -Werror
+snd-soc-hda-bus-objs := soc-hdac-bus.o
+obj-$(CONFIG_SND_SOC_HDA_BUS) += snd-soc-hda-bus.o
diff --git a/sound/soc/hda/soc-hdac-bus.c b/sound/soc/hda/soc-hdac-bus.c
new file mode 100644
index 000000000000..d505a6ffb30f
--- /dev/null
+++ b/sound/soc/hda/soc-hdac-bus.c
@@ -0,0 +1,233 @@
+/*
+ *  soc-hdac-bus.c - SoC HDA bus Interface for SoC HDA devices
+ *
+ *  Copyright (C) 2015 Intel Corp
+ *  Author: Jeeja KP<jeeja.kp@intel.com>
+ *	Ramesh Babu <Ramesh.Babu@intel.com>
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <linux/string.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/pm_runtime.h>
+#include <linux/pci.h>
+#include <sound/core.h>
+#include <sound/hdaudio.h>
+#include <sound/soc-hdac-bus.h>
+
+
+/**
+ * hda_device_put - destroy a hda device
+ * @pdev: hda device to free
+ *
+ * Free all memory associated with a hda device.  This function must
+ * _only_ be externally called in error cases.  All other usage is a bug.
+ */
+void snd_soc_hdac_device_put(struct snd_soc_hdac_device *pdev)
+{
+	if (pdev)
+		put_device(&pdev->hdac.dev);
+}
+EXPORT_SYMBOL_GPL(snd_soc_hdac_device_put);
+
+/**
+ * snd_soc_hdac_device_add_data - add platform-specific data to a hda device
+ * @pdev: hda device allocated by platform_device_alloc to add resources to
+ * @data: hda specific data for this hda device
+ * @size: size of platform specific data
+ *
+ */
+int snd_soc_hdac_device_add_data(struct snd_soc_hdac_device *pdev,
+	const void *data, size_t size)
+{
+	void *d = NULL;
+
+	if (data) {
+		d = kmemdup(data, size, GFP_KERNEL);
+		if (!d)
+			return -ENOMEM;
+	}
+
+	kfree(pdev->hdac.dev.platform_data);
+	pdev->hdac.dev.platform_data = d;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_hdac_device_add_data);
+
+/**
+ * hda_device_unregister - unregister a hda device
+ * @pdev: hda device we're unregistering
+ *
+ * Unregistration is done in 2 steps. First we release all resources
+ * and remove it from the subsystem, then we drop reference count by
+ * calling hda_device_put().
+ */
+void snd_soc_hdac_device_unregister(struct snd_soc_hdac_device *pdev)
+{
+	snd_hdac_device_exit(&pdev->hdac);
+	snd_hdac_device_unregister(&pdev->hdac);
+	snd_soc_hdac_device_put(pdev);
+}
+EXPORT_SYMBOL_GPL(snd_soc_hdac_device_unregister);
+
+static int soc_hdac_drv_probe(struct device *dev)
+{
+	struct snd_soc_hdac_driver *drv = to_soc_hdac_driver(dev->driver);
+
+	return drv->probe(to_soc_hdac_device(dev));
+}
+
+
+static int soc_hdac_drv_remove(struct device *dev)
+{
+	struct snd_soc_hdac_driver *drv = to_soc_hdac_driver(dev->driver);
+
+	return drv->remove(to_soc_hdac_device(dev));
+}
+
+static void soc_hdac_drv_shutdown(struct device *dev)
+{
+	struct snd_soc_hdac_driver *drv = to_soc_hdac_driver(dev->driver);
+
+	drv->shutdown(to_soc_hdac_device(dev));
+}
+
+/**
+ * hda_driver_register - register a driver for hda devices
+ * @drv: hda driver structure
+ */
+int snd_soc_hdac_driver_register(struct snd_soc_hdac_driver *drv)
+{
+	if (drv->probe)
+		drv->hdrv.driver.probe = soc_hdac_drv_probe;
+	if (drv->remove)
+		drv->hdrv.driver.remove = soc_hdac_drv_remove;
+	if (drv->shutdown)
+		drv->hdrv.driver.shutdown = soc_hdac_drv_shutdown;
+
+	return driver_register(&drv->hdrv.driver);
+}
+EXPORT_SYMBOL_GPL(snd_soc_hdac_driver_register);
+
+/**
+ * hda_driver_unregister - unregister a driver for hda devices
+ * @drv: hda driver structure
+ */
+void snd_soc_hdac_driver_unregister(struct snd_soc_hdac_driver *drv)
+{
+	driver_unregister(&drv->hdrv.driver);
+}
+EXPORT_SYMBOL_GPL(snd_soc_hdac_driver_unregister);
+
+static const struct snd_soc_hda_device_id *soc_hda_match_id(
+			const struct snd_soc_hda_device_id *id,
+			struct snd_soc_hdac_device *pdev)
+{
+	while (id->name[0]) {
+		if (pdev->id == id->id) {
+			pdev->id_entry = id;
+			return id;
+		} else if (strcmp(pdev->name, id->name) == 0) {
+				pdev->id_entry = id;
+				return id;
+		}
+		id++;
+	}
+	return NULL;
+}
+
+/**
+ * hda_match - bind hda device to hda driver.
+ * @dev: device.
+ * @drv: driver.
+ *
+ */
+static int soc_hdac_match(struct device *dev, struct device_driver *drv)
+{
+	struct snd_soc_hdac_device *pdev = to_soc_hdac_device(dev);
+	struct snd_soc_hdac_driver *pdrv = to_soc_hdac_driver(drv);
+
+	/* Then try to match against the id table */
+	if (pdrv->id_table)
+		return soc_hda_match_id(pdrv->id_table, pdev) != NULL;
+
+	/* fall-back to driver name match */
+	return (strcmp(pdev->name, drv->name) == 0);
+}
+EXPORT_SYMBOL_GPL(soc_hdac_match);
+
+
+int snd_soc_hda_bus_init(struct pci_dev *pci, void *data,
+	struct snd_soc_hdac_bus **busp)
+{
+	struct snd_soc_hdac_bus *sbus;
+	int err;
+
+	sbus = kzalloc(sizeof(*sbus), GFP_KERNEL);
+	if (sbus == NULL)
+		return -ENOMEM;
+
+	err = snd_hdac_bus_init(&sbus->bus, &pci->dev, NULL);
+	if (err < 0) {
+		kfree(sbus);
+		return err;
+	}
+	if (busp)
+		*busp = sbus;
+	sbus->pci = pci;
+	mutex_init(&sbus->prepare_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_hda_bus_init);
+
+static int soc_hda_bus_free(struct snd_soc_hdac_bus *sbus)
+{
+	struct hdac_bus *bus;
+
+	if (!sbus)
+		return 0;
+
+	bus = &sbus->bus;
+	snd_hdac_bus_exit(bus);
+	kfree(sbus);
+	return 0;
+}
+
+static int soc_hda_release_device(struct device *dev, void *data)
+{
+	struct snd_soc_hdac_device *pdev = to_soc_hdac_device(dev);
+
+	snd_soc_hdac_device_unregister(pdev);
+	return 0;
+}
+
+void snd_soc_hdac_bus_release(struct snd_soc_hdac_bus *hbus)
+{
+	/*unregister all devices on bus */
+	bus_for_each_dev(&snd_hda_bus_type, NULL, NULL,
+			soc_hda_release_device);
+
+	soc_hda_bus_free(hbus);
+}
+EXPORT_SYMBOL_GPL(snd_soc_hdac_bus_release);
+
+MODULE_AUTHOR("KP Jeeja, jeeja.kp@intel.com");
+MODULE_AUTHOR("Ramesh Babu, Ramesh.Babu@intel.com");
+MODULE_DESCRIPTION("ASOC HDA bus core");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [RFC 1/3] ALSA: hda - remove assigning dev type inside core

[Takashi Iwai]

At Thu,  2 Apr 2015 15:27:29 +0530,
Vinod Koul wrote:
> 
> From: Ramesh Babu <ramesh.babu@intel.com>
> 
> We don't need to init the hda dev type inside
> core device init code. It is expected to be
> initialized by the caller.

As there is no way in C to guarantee the initialization of some
virtual fields in the inherited class, it's safer to do the complete
init in the base class initializer and let the child class override
manually.


Takashi

> 
> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  sound/hda/hdac_device.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
> index 6e8ee1d6974a..aefc80ffecee 100644
> --- a/sound/hda/hdac_device.c
> +++ b/sound/hda/hdac_device.c
> @@ -51,7 +51,6 @@ int snd_hdac_device_init(struct hdac_device *codec, struct hdac_bus *bus,
>  
>  	codec->bus = bus;
>  	codec->addr = addr;
> -	codec->type = HDA_DEV_CORE;
>  	pm_runtime_set_active(&codec->dev);
>  	pm_runtime_get_noresume(&codec->dev);
>  	atomic_set(&codec->in_pm, 0);
> -- 
> 1.7.9.5
> 

Re: [RFC 3/3] ALSA: hda - add soc hda bus wrapper

[Takashi Iwai]

At Thu,  2 Apr 2015 15:27:31 +0530,
Vinod Koul wrote:
> 
> From: Ramesh Babu <ramesh.babu@intel.com>
> 
> The SKL controller will be ASoC device but needs access to HDA code, so
> create asoc wrppers on top of hdac.
> 
> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>

Just a few comment after a quicky glance.
Will do more reviews once after processing my huge backlogs after
vacation.

> +/**
> + * hda_device_unregister - unregister a hda device
> + * @pdev: hda device we're unregistering
> + *
> + * Unregistration is done in 2 steps. First we release all resources
> + * and remove it from the subsystem, then we drop reference count by
> + * calling hda_device_put().
> + */
> +void snd_soc_hdac_device_unregister(struct snd_soc_hdac_device *pdev)
> +{
> +	snd_hdac_device_exit(&pdev->hdac);
> +	snd_hdac_device_unregister(&pdev->hdac);
> +	snd_soc_hdac_device_put(pdev);
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_hdac_device_unregister);
> +

Doing this in a shot is basically wrong.  Since ASoC doesn't care much
about the hotplug/unplug, maybe it doesn't matter so much for now,
though...

In general, you should unregister from the subsystem at first.  And
calling snd_hdac_device_exit() should be done in the device object
destructor.  Unregister doesn't guarantee the immediate release of
resources that was being used, thus it's pretty much racy.

BTW, the function name in the comment must be same as the real name.

> +/**
> + * hda_match - bind hda device to hda driver.
> + * @dev: device.
> + * @drv: driver.
> + *
> + */
> +static int soc_hdac_match(struct device *dev, struct device_driver *drv)
> +{
> +	struct snd_soc_hdac_device *pdev = to_soc_hdac_device(dev);
> +	struct snd_soc_hdac_driver *pdrv = to_soc_hdac_driver(drv);
> +
> +	/* Then try to match against the id table */
> +	if (pdrv->id_table)
> +		return soc_hda_match_id(pdrv->id_table, pdev) != NULL;
> +
> +	/* fall-back to driver name match */
> +	return (strcmp(pdev->name, drv->name) == 0);
> +}
> +EXPORT_SYMBOL_GPL(soc_hdac_match);

Any reason to drop snd_ prefix only for this function?

> +MODULE_DESCRIPTION("ASOC HDA bus core");

ASoC.


Takashi

Re: [RFC 3/3] ALSA: hda - add soc hda bus wrapper

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2328 bytes --]

On Thu, Apr 02, 2015 at 03:27:31PM +0530, Vinod Koul wrote:

> index 000000000000..09ef831b8513
> --- /dev/null
> +++ b/sound/soc/hda/Kconfig
> @@ -0,0 +1,4 @@
> +config SND_SOC_HDA_BUS
> +	tristate "SoC HDA bus wrapper"
> +	help
> +	 This adds support the SOC HDA Bus wrapper

ASoC (in both places please).

> +void snd_soc_hdac_device_put(struct snd_soc_hdac_device *pdev)
> +{
> +	if (pdev)
> +		put_device(&pdev->hdac.dev);
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_hdac_device_put);
> +
> +/**
> + * snd_soc_hdac_device_add_data - add platform-specific data to a hda device
> + * @pdev: hda device allocated by platform_device_alloc to add resources to
> + * @data: hda specific data for this hda device
> + * @size: size of platform specific data
> + *
> + */
> +int snd_soc_hdac_device_add_data(struct snd_soc_hdac_device *pdev,
> +	const void *data, size_t size)
> +{
> +	void *d = NULL;
> +
> +	if (data) {
> +		d = kmemdup(data, size, GFP_KERNEL);
> +		if (!d)
> +			return -ENOMEM;
> +	}
> +
> +	kfree(pdev->hdac.dev.platform_data);
> +	pdev->hdac.dev.platform_data = d;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_hdac_device_add_data);

I didn't notice the put function cleaning this up?

> +/**
> + * hda_device_unregister - unregister a hda device
> + * @pdev: hda device we're unregistering
> + *
> + * Unregistration is done in 2 steps. First we release all resources
> + * and remove it from the subsystem, then we drop reference count by
> + * calling hda_device_put().
> + */
> +void snd_soc_hdac_device_unregister(struct snd_soc_hdac_device *pdev)
> +{
> +	snd_hdac_device_exit(&pdev->hdac);
> +	snd_hdac_device_unregister(&pdev->hdac);
> +	snd_soc_hdac_device_put(pdev);
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_hdac_device_unregister);

Takashi queried bits of this too...  I have to say I don't entirely
understand how this device management is supposed to work and the lack
of any users in the current series isn't giving me anything to refer to
here, perhaps it's all obvious given them but I don't have them just
now.  A lot of this looks like it's boilerplate which should be
duplicating something device modelish and there certainly seem to be
some things with object lifetimes that need paying attention to.

All the commit message says is "create asoc wrppers on top of hdac", can
we have more words please?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 3/3] ALSA: hda - add soc hda bus wrapper

[Ramesh Babu]

On Mon, Apr 06, 2015 at 06:08:04PM +0100, Mark Brown wrote:

Hi Takashi, Mark

> On Thu, Apr 02, 2015 at 03:27:31PM +0530, Vinod Koul wrote:
> 
> 
> > +/**
> > + * hda_device_unregister - unregister a hda device
> > + * @pdev: hda device we're unregistering
> > + *
> > + * Unregistration is done in 2 steps. First we release all resources
> > + * and remove it from the subsystem, then we drop reference count by
> > + * calling hda_device_put().
> > + */
> > +void snd_soc_hdac_device_unregister(struct snd_soc_hdac_device *pdev)
> > +{
> > +	snd_hdac_device_exit(&pdev->hdac);
> > +	snd_hdac_device_unregister(&pdev->hdac);
> > +	snd_soc_hdac_device_put(pdev);
> > +}
> > +EXPORT_SYMBOL_GPL(snd_soc_hdac_device_unregister);
> 
> Takashi queried bits of this too...  I have to say I don't entirely
> understand how this device management is supposed to work and the lack
> of any users in the current series isn't giving me anything to refer to
> here, perhaps it's all obvious given them but I don't have them just
> now.  A lot of this looks like it's boilerplate which should be
> duplicating something device modelish and there certainly seem to be
> some things with object lifetimes that need paying attention to.
> 
> All the commit message says is "create asoc wrppers on top of hdac", can
> we have more words please?

Will fix your comments in next version of the RFC patches.
-- 

Re: [alsa-devel] [RFC 3/3] ALSA: hda - add soc hda bus wrapper

[Ramesh Babu]

On Mon, Apr 06, 2015 at 06:08:04PM +0100, Mark Brown wrote:

Hi Takashi, Mark

> On Thu, Apr 02, 2015 at 03:27:31PM +0530, Vinod Koul wrote:
> 
> 
> > +/**
> > + * hda_device_unregister - unregister a hda device
> > + * @pdev: hda device we're unregistering
> > + *
> > + * Unregistration is done in 2 steps. First we release all resources
> > + * and remove it from the subsystem, then we drop reference count by
> > + * calling hda_device_put().
> > + */
> > +void snd_soc_hdac_device_unregister(struct snd_soc_hdac_device *pdev)
> > +{
> > +	snd_hdac_device_exit(&pdev->hdac);
> > +	snd_hdac_device_unregister(&pdev->hdac);
> > +	snd_soc_hdac_device_put(pdev);
> > +}
> > +EXPORT_SYMBOL_GPL(snd_soc_hdac_device_unregister);
> 
> Takashi queried bits of this too...  I have to say I don't entirely
> understand how this device management is supposed to work and the lack
> of any users in the current series isn't giving me anything to refer to
> here, perhaps it's all obvious given them but I don't have them just
> now.  A lot of this looks like it's boilerplate which should be
> duplicating something device modelish and there certainly seem to be
> some things with object lifetimes that need paying attention to.
> 
> All the commit message says is "create asoc wrppers on top of hdac", can
> we have more words please?

Will fix your comments in next version of the RFC patches.
-- 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC 0/3] ALSA: Add soc hda bus support

[David Henningsson]

On 2015-04-02 11:57, Vinod Koul wrote:
> This series of 3 patches adds support for ASoC HDA
> bus. ASoC HDA bus is essentially a wrapper for
> HDA core bus and it manages ASoC hda drivers

Hi,

I'm seeing several patch sets - as well as the added hdac indirection 
layer earlier - related to HDA and ASoC. But I'm curious about the 
bigger picture here.

In addition, I notice the rt286/rt288 ASoC driver looks very much 
hda-like, i e, we're in practice sending HDA commands over an i2s (or 
i2c) bus.

Adding two and two together it could mean you're trying to move 
rt286/rt288 over to use the hda codec driver (which would be awesome!). 
And perhaps other, similar ASoC codecs as well, if there are any.

But I still feel I'm missing the bigger roadmap here. Anyone who wants 
to enlighten me? :-)

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [RFC 0/3] ALSA: Add soc hda bus support

[Vinod Koul]

On Wed, Apr 08, 2015 at 08:28:57AM +0200, David Henningsson wrote:
> 
> 
> On 2015-04-02 11:57, Vinod Koul wrote:
> >This series of 3 patches adds support for ASoC HDA
> >bus. ASoC HDA bus is essentially a wrapper for
> >HDA core bus and it manages ASoC hda drivers
> 
> Hi,
> 
> I'm seeing several patch sets - as well as the added hdac
> indirection layer earlier - related to HDA and ASoC. But I'm curious
> about the bigger picture here.
Sure, as we discussed in last meeting, the SKL audio controller is HDA audio
controller which also supports a DSP and I2S ports. Although if you get a
SKL board with a HDA codec you can bypass DSP and it will work as previuos
gen Intel HDA controller so current hda code works well for "legacy" HDA
mode

Now we are moving bits and pieces out of current hda code into hdac layer
(hda-core) which will be used by SKL ASoC driver as well as current
implementations.
So first remaining parts of hdac will be merged follow by asoc wrappers,
then SKL controller driver and fllowed up by DSP and I2S support.

> In addition, I notice the rt286/rt288 ASoC driver looks very much
> hda-like, i e, we're in practice sending HDA commands over an i2s
> (or i2c) bus.
> 
> Adding two and two together it could mean you're trying to move
> rt286/rt288 over to use the hda codec driver (which would be
> awesome!). And perhaps other, similar ASoC codecs as well, if there
> are any.
some of the RT codecs support both HDA and I2S, but right now merging them
is not in my radar but yes it is doable and I will look into this once base
driver and merged
> 
> But I still feel I'm missing the bigger roadmap here. Anyone who
> wants to enlighten me? :-)
Hope this would have helped, do let me know if you have more questions :)

-- 
~Vinod