On Tue, 19 Dec 2017 10:19:17 +0100, Ughreja, Rakesh A wrote:
-----Original Message----- From: Ughreja, Rakesh A Sent: Monday, December 18, 2017 9:36 AM To: Takashi Iwai tiwai@suse.de Cc: alsa-devel@alsa-project.org; broonie@kernel.org; liam.r.girdwood@linux.intel.com; pierre-louis.bossart@linux.intel.com; Koul, Vinod vinod.koul@intel.com; Patches Audio patches.audio@intel.com Subject: RE: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Saturday, December 16, 2017 2:44 PM To: Ughreja, Rakesh A rakesh.a.ughreja@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org; liam.r.girdwood@linux.intel.com; pierre-louis.bossart@linux.intel.com; Koul,
Vinod
vinod.koul@intel.com; Patches Audio patches.audio@intel.com Subject: Re: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec
driver
I am not sure if I understand you fully, so asking some follow up Questions.
I am assuming you are asking me to implement something like following. Where I have to implement snd_hda_get_mode() function which would return "true" if we need to register the driver as "asoc" driver.
Is that right understanding ?
int __hda_codec_driver_register(struct hda_codec_driver *drv, const char
*name,
struct module *owner){ /* * check if we need to register ASoC HDA driver */ #if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA) int asoc_mode = snd_hda_get_mode(); if (asoc_mode) { drv->core.id_table = drv->id; return __hdac_hda_codec_driver_register(&drv->core, name,
owner);
}#endif return __hda_legacy_codec_driver_register(drv, name, owner); }
If above is true then the follow up question is, what are the criteria to
determine
the mode. Since I cannot assume that the bus instance is already created at
the
time of driver registration, I am not sure how to determine what kind of
platform
driver would be loaded in future.
My assumption is that there is only one hda_codec_driver_register(). The legacy code needs to be rewritten to implement the standard probe/remove as preliminary. Any the rest differentiation is done via additional callbacks at probe/remove time.
Oh I see. Let me work on this and may ask you some additional clarifications.
Regards, Rakesh
Hi Takashi,
I worked on the concept that you proposed and here is the patch with main code change. Basically I wrote common driver handlers for HDA Driver snd_hdac_drv_probe, snd_hdac_drv_remove, snd_hdac_drv_shutdown, snd_hdac_drv_match, snd_hdac_drv_unsol_event etc.
Once the common driver is probed it checks what kind of bus it is enumerated on by calling the bus API. If it is ASOC bus type it calls the ASOC driver registered callbacks and if it is LEGACY bus type it calls the Legacy driver registered callbacks.
ASoC based platform drivers would create ASOC bus type while the legacy controller drivers would create LEGACY bus type. I added the bus_type as a part of hdac_bus, which gets set during snd_hdac_bus_init or snd_hdac_ext_bus_init. hdac_device->type and hdac_driver->type are set the HDA_DEV_CORE during registrations and enumeration.
Also I have kept these routines as part of codec library, so that all the other drivers can use it without duplicating the code.
Let me know if you are okay, I can include these changes as part of my next series.
I need to think more deeply, but after a quick look, I find it too overhead.
May we start from a "big picture" to describe the whole driver bindings?
Takashi
Regards, Rakesh
--- patch-- This patch adds common enumeration functions for HDA codec drivers. Once the codec is enumerated on the hdac_bus, the bus_type is checked before the corresponding callback functions are called.
Signed-off-by: Rakesh Ughreja rakesh.a.ughreja@intel.com
sound/pci/hda/hda_bind.c | 252 ++++++++++++++++++++++++++++++++++++++++------ sound/pci/hda/hda_codec.h | 22 ++-- 2 files changed, 231 insertions(+), 43 deletions(-)
diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c index d8715a1..7d8d54c 100644 --- a/sound/pci/hda/hda_bind.c +++ b/sound/pci/hda/hda_bind.c @@ -11,6 +11,7 @@ #include <linux/pm.h> #include <linux/pm_runtime.h> #include <sound/core.h> +#include <sound/hdaudio_ext.h> #include "hda_codec.h" #include "hda_local.h"
@@ -74,10 +75,10 @@ int snd_hda_codec_set_name(struct hda_codec *codec, const char *name) } EXPORT_SYMBOL_GPL(snd_hda_codec_set_name);
-static int hda_codec_driver_probe(struct device *dev) +static int hda_codec_driver_probe(struct hdac_device *hdev) {
- struct hda_codec *codec = dev_to_hda_codec(dev);
- struct module *owner = dev->driver->owner;
- struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
- struct module *owner = hdev->dev.driver->owner; hda_codec_patch_t patch; int err;
@@ -130,48 +131,25 @@ static int hda_codec_driver_probe(struct device *dev) return err; }
-static int hda_codec_driver_remove(struct device *dev) +static int hda_codec_driver_remove(struct hdac_device *hdev) {
- struct hda_codec *codec = dev_to_hda_codec(dev);
struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
if (codec->patch_ops.free) codec->patch_ops.free(codec); snd_hda_codec_cleanup_for_unbind(codec);
- module_put(dev->driver->owner);
- module_put(hdev->dev.driver->owner); return 0;
}
-static void hda_codec_driver_shutdown(struct device *dev) +static void hda_codec_driver_shutdown(struct hdac_device *hdev) {
- struct hda_codec *codec = dev_to_hda_codec(dev);
- struct hda_codec *codec = dev_to_hda_codec(&hdev->dev);
- if (!pm_runtime_suspended(dev) && codec->patch_ops.reboot_notify)
- if (!pm_runtime_suspended(&hdev->dev) && codec->patch_ops.reboot_notify) codec->patch_ops.reboot_notify(codec);
}
-int __hda_legacy_codec_driver_register(struct hda_codec_driver *drv,
const char *name, struct module *owner)-{
- drv->core.driver.name = name;
- drv->core.driver.owner = owner;
- drv->core.driver.bus = &snd_hda_bus_type;
- drv->core.driver.probe = hda_codec_driver_probe;
- drv->core.driver.remove = hda_codec_driver_remove;
- drv->core.driver.shutdown = hda_codec_driver_shutdown;
- drv->core.driver.pm = &hda_codec_driver_pm;
- drv->core.type = HDA_DEV_LEGACY;
- drv->core.match = hda_codec_match;
- drv->core.unsol_event = hda_codec_unsol_event;
- return driver_register(&drv->core.driver);
-} -EXPORT_SYMBOL_GPL(__hda_legacy_codec_driver_register);
-void hda_legacy_codec_driver_unregister(struct hda_codec_driver *drv) -{
- driver_unregister(&drv->core.driver);
-} -EXPORT_SYMBOL_GPL(hda_legacy_codec_driver_unregister);
static inline bool codec_probed(struct hda_codec *codec) { return device_attach(hda_codec_dev(codec)) > 0 && codec->preset; @@ -305,3 +283,213 @@ int snd_hda_codec_configure(struct hda_codec *codec) return err; } EXPORT_SYMBOL_GPL(snd_hda_codec_configure);
+/*
- Newly implemented functions for common routines
- */
+static struct hdac_driver *snd_hdac_drvs[2];
+int snd_hdac_drv_probe(struct hdac_device *hdev) +{
- int bus_type;
- struct hdac_driver *drv;
- bus_type = snd_hdac_get_bus_type(hdev->bus);
- bus_type --;
- dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
- drv = snd_hdac_drvs[bus_type];
- if (drv->probe)
return drv->probe(hdev);- return 0;
+} +EXPORT_SYMBOL_GPL(snd_hdac_drv_probe);
+int snd_hdac_drv_remove(struct hdac_device *hdev) +{
- int bus_type;
- struct hdac_driver *drv;
- bus_type = snd_hdac_get_bus_type(hdev->bus);
- bus_type --;
- dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
- drv = snd_hdac_drvs[bus_type];
- if (drv->remove)
return drv->remove(hdev);- return 0;
+} +EXPORT_SYMBOL_GPL(snd_hdac_drv_remove);
+void snd_hdac_drv_shutdown(struct hdac_device *hdev) +{
- int bus_type;
- struct hdac_driver *drv;
- bus_type = snd_hdac_get_bus_type(hdev->bus);
- bus_type --;
- dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
- drv = snd_hdac_drvs[bus_type];
- if (drv->shutdown)
return drv->shutdown(hdev);+} +EXPORT_SYMBOL_GPL(snd_hdac_drv_shutdown);
+int snd_hdac_drv_match(struct hdac_device *hdev, struct hdac_driver *drv) +{
- int bus_type;
- struct hdac_driver *cdrv;
- bus_type = snd_hdac_get_bus_type(hdev->bus);
- bus_type --;
- dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
- cdrv = snd_hdac_drvs[bus_type];
- if (cdrv->match)
return cdrv->match(hdev, drv);- return 0;
+} +EXPORT_SYMBOL_GPL(snd_hdac_drv_match);
+void snd_hdac_drv_unsol_event(struct hdac_device *hdev, unsigned int event) +{
- int bus_type;
- struct hdac_driver *drv;
- bus_type = snd_hdac_get_bus_type(hdev->bus);
- bus_type --;
- dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
- drv = snd_hdac_drvs[bus_type];
- if (drv->unsol_event)
return drv->unsol_event(hdev, event);+} +EXPORT_SYMBOL_GPL(snd_hdac_drv_unsol_event);
+int snd_hdac_runtime_suspend(struct device *dev) +{
- int bus_type;
- struct hdac_driver *drv;
- struct hdac_device *hdev = dev_to_hdac_dev(dev);
- bus_type = snd_hdac_get_bus_type(hdev->bus);
- bus_type --;
- dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
- drv = snd_hdac_drvs[bus_type];
- if (drv->driver.pm->runtime_suspend)
return drv->driver.pm->runtime_suspend(dev);- return 0;
+}
+int snd_hdac_runtime_resume(struct device *dev) +{
- int bus_type;
- struct hdac_driver *drv;
- struct hdac_device *hdev = dev_to_hdac_dev(dev);
- bus_type = snd_hdac_get_bus_type(hdev->bus);
- bus_type --;
- dev_dbg(&hdev->dev, "%s: entry ..%d\n", __func__, bus_type);
- drv = snd_hdac_drvs[bus_type];
- if (drv->driver.pm->runtime_resume)
return drv->driver.pm->runtime_resume(dev);- return 0;
+}
+const struct dev_pm_ops snd_hdac_drv_pm = {
- SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
pm_runtime_force_resume)- SET_RUNTIME_PM_OPS(snd_hdac_runtime_suspend, snd_hdac_runtime_resume,
NULL)+};
+int snd_hdac_callback_register(struct hdac_driver *drv) +{
- int idx;
- if(drv->type != HDA_DEV_LEGACY && drv->type != HDA_DEV_ASOC)
return -EINVAL;- printk("%s: registered callback %d\n", drv->driver.name, drv->type);
- idx = drv->type - 1;
- if (!snd_hdac_drvs[idx])
snd_hdac_drvs[idx] = drv;- return 0;
+} +EXPORT_SYMBOL_GPL(snd_hdac_callback_register);
+int snd_hdac_callback_unregister(struct hdac_driver *drv) +{
- int idx;
- if(drv->type != HDA_DEV_LEGACY && drv->type != HDA_DEV_ASOC)
return -EINVAL;- idx = drv->type - 1;
- snd_hdac_drvs[idx] = NULL;
- return 0;
+} +EXPORT_SYMBOL_GPL(snd_hdac_callback_unregister);
+static struct hdac_driver hdac_legacy_driver = {
- .type = HDA_DEV_LEGACY,
- .match = hda_codec_match,
- .unsol_event = hda_codec_unsol_event,
- .probe = hda_codec_driver_probe,
- .remove = hda_codec_driver_remove,
- .shutdown = hda_codec_driver_shutdown,
- .driver.pm = &hda_codec_driver_pm,
+};
+int __snd_hdac_driver_register(struct hda_codec_driver *drv, const char *name,
struct module *owner)+{
- int ret;
- struct hdac_driver *core = &drv->core;
- printk("%s: entry ..%s\n", __func__, name);
- core->id_table = drv->id;
- core->driver.name = name;
- core->driver.owner = owner;
- core->driver.bus = &snd_hda_bus_type;
- core->driver.pm = &snd_hdac_drv_pm;
- core->type = HDA_DEV_CORE;
- core->probe = snd_hdac_drv_probe;
- core->remove = snd_hdac_drv_remove;
- core->shutdown = snd_hdac_drv_shutdown;
- core->match = snd_hdac_drv_match;
- core->unsol_event = snd_hdac_drv_unsol_event;
- ret = snd_hdac_callback_register(&hdac_legacy_driver);
- if (ret < 0)
return ret;- return snd_hda_ext_driver_register(core);
+} +EXPORT_SYMBOL_GPL(__snd_hdac_driver_register);
+void snd_hdac_driver_unregister(struct hda_codec_driver *drv) +{
- snd_hda_ext_driver_unregister(&drv->core);
+} +EXPORT_SYMBOL_GPL(snd_hdac_driver_unregister); diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index dda8401..0a3b56e 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -99,18 +99,18 @@ struct hda_codec_driver { const struct hda_device_id *id; };
-int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
struct module *owner);-#define hda_codec_driver_register(drv) \
- __hda_codec_driver_register(drv, KBUILD_MODNAME, THIS_MODULE)
-void hda_codec_driver_unregister(struct hda_codec_driver *drv); +int __snd_hdac_driver_register(struct hda_codec_driver *drv, const char *name,
struct module *owner);+#define snd_hdac_driver_register(drv) \
- __snd_hdac_driver_register(drv, KBUILD_MODNAME, THIS_MODULE)
+void snd_hdac_driver_unregister(struct hda_codec_driver *drv);
#define module_hda_codec_driver(drv) \
- module_driver(drv, hda_codec_driver_register, \
hda_codec_driver_unregister)
- module_driver(drv, snd_hdac_driver_register, \
snd_hdac_driver_unregister)+int snd_hdac_callback_register(struct hdac_driver *drv); +int snd_hdac_callback_unregister(struct hdac_driver *drv);
/* ops set by the preset patch */ struct hda_codec_ops { -- 2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel