| Message ID | 85DFEED57DC57344B2483EF7BF8CB60579ADCE9A@BGSMSX104.gar.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
>-----Original Message----- >From: Takashi Iwai [mailto:tiwai@suse.de] >Sent: Tuesday, December 19, 2017 4:58 PM >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> >Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre- >louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio ><patches.audio@intel.com>; broonie@kernel.org >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA >codec driver > >> >> 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. Are you referring to the calling overhead because of the wrapper involved ? The way I see is we have two options. 1. Single driver option. - There is going to be a common wrapper here, which needs to come into picture before it re-directs it to the appropriate driver. This is what is done in the following patch. 2. Separate driver for ASoC and Legacy. - This requires ID tables, Can we move id tables into separate header file ? then it can solve the problem involved in option 1. This also solves the problem related to wrappers as well as the problem related to accessing the ID tables via extern symbols, that you mentioned in the previous series. > >May we start from a "big picture" to describe the whole driver >bindings? > This patch registers the hdac_driver callback at the root level agnostic to asoc and legacy and then selects the appropriate legacy or asoc callbacks, based on the bus which enumerated the driver. I cannot think of any other approach if we want to go with a single driver approach. You will have to give some more hints :-) Thank you for taking time and giving feedback. Regards, Rakesh
On Tue, 19 Dec 2017 16:26:04 +0100, Ughreja, Rakesh A wrote: > > >-----Original Message----- > >From: Takashi Iwai [mailto:tiwai@suse.de] > >Sent: Tuesday, December 19, 2017 4:58 PM > >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> > >Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre- > >louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio > ><patches.audio@intel.com>; broonie@kernel.org > >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA > >codec driver > > > >> > >> 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. > > Are you referring to the calling overhead because of the wrapper involved ? > > The way I see is we have two options. > > 1. Single driver option. - There is going to be a common wrapper here, > which needs to come into picture before it re-directs it to the appropriate > driver. This is what is done in the following patch. > > 2. Separate driver for ASoC and Legacy. - This requires ID tables, Can we move > id tables into separate header file ? then it can solve the problem involved in > option 1. This also solves the problem related to wrappers as well as the > problem related to accessing the ID tables via extern symbols, that you > mentioned in the previous series. I believe (2) is no-go, it's a straight maintenance hell. > >May we start from a "big picture" to describe the whole driver > >bindings? > > > > This patch registers the hdac_driver callback at the root level agnostic to > asoc and legacy and then selects the appropriate legacy or asoc callbacks, > based on the bus which enumerated the driver. > > I cannot think of any other approach if we want to go with a single driver > approach. You will have to give some more hints :-) Well, a big unclear question to me is why do we need to bind the stuff so differently. Can't we simply provide the same binding to the legacy codec from asoc driver? In the legacy-support mode, asoc driver creates the legacy-compatible codec objects with the legacy-compatible hda_bus. Takashi
>-----Original Message----- >From: Takashi Iwai [mailto:tiwai@suse.de] >Sent: Tuesday, December 19, 2017 9:10 PM >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> >Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre- >louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio ><patches.audio@intel.com>; broonie@kernel.org >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA >codec driver > >> >> Are you referring to the calling overhead because of the wrapper involved ? >> >> The way I see is we have two options. >> >> 1. Single driver option. - There is going to be a common wrapper here, >> which needs to come into picture before it re-directs it to the appropriate >> driver. This is what is done in the following patch. >> >> 2. Separate driver for ASoC and Legacy. - This requires ID tables, Can we move >> id tables into separate header file ? then it can solve the problem involved in >> option 1. This also solves the problem related to wrappers as well as the >> problem related to accessing the ID tables via extern symbols, that you >> mentioned in the previous series. > >I believe (2) is no-go, it's a straight maintenance hell. Got it. > >> >May we start from a "big picture" to describe the whole driver >> >bindings? >> > >> >> This patch registers the hdac_driver callback at the root level agnostic to >> asoc and legacy and then selects the appropriate legacy or asoc callbacks, >> based on the bus which enumerated the driver. >> >> I cannot think of any other approach if we want to go with a single driver >> approach. You will have to give some more hints :-) > >Well, a big unclear question to me is why do we need to bind the stuff >so differently. Can't we simply provide the same binding to the >legacy codec from asoc driver? In the legacy-support mode, asoc >driver creates the legacy-compatible codec objects with the >legacy-compatible hda_bus. > Both the drivers i.e. ASoC and Legacy are registering the driver in exact same way by filling the hdac_driver fields. There is no difference in terms of HDA bus interactions. First series unifies even the data structures hdac_device, hdac_driver and hdac_bus. Once the device is enumerated and the hdac_driver->probe is called the difference starts, primarily because ASoC vs ALSA codec driver differences. Here also if you notice, after taking care of the ASoC related components, ASoC codec driver calls exact same APIs of the legacy HDA codec driver which are called by the legacy controller driver. The hda_bus and hda_codec data structures are also used by the ASoC driver as it is to call legacy codec driver APIs. So I am not sure if we are doing binding in two different ways, or I misunderstood you completely ? Regards, Rakesh
On Tue, 19 Dec 2017 17:14:38 +0100, Ughreja, Rakesh A wrote: > > > > >-----Original Message----- > >From: Takashi Iwai [mailto:tiwai@suse.de] > >Sent: Tuesday, December 19, 2017 9:10 PM > >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> > >Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre- > >louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio > ><patches.audio@intel.com>; broonie@kernel.org > >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA > >codec driver > > > > > >> > >> Are you referring to the calling overhead because of the wrapper involved ? > >> > >> The way I see is we have two options. > >> > >> 1. Single driver option. - There is going to be a common wrapper here, > >> which needs to come into picture before it re-directs it to the appropriate > >> driver. This is what is done in the following patch. > >> > >> 2. Separate driver for ASoC and Legacy. - This requires ID tables, Can we move > >> id tables into separate header file ? then it can solve the problem involved in > >> option 1. This also solves the problem related to wrappers as well as the > >> problem related to accessing the ID tables via extern symbols, that you > >> mentioned in the previous series. > > > >I believe (2) is no-go, it's a straight maintenance hell. > > Got it. > > > > >> >May we start from a "big picture" to describe the whole driver > >> >bindings? > >> > > >> > >> This patch registers the hdac_driver callback at the root level agnostic to > >> asoc and legacy and then selects the appropriate legacy or asoc callbacks, > >> based on the bus which enumerated the driver. > >> > >> I cannot think of any other approach if we want to go with a single driver > >> approach. You will have to give some more hints :-) > > > >Well, a big unclear question to me is why do we need to bind the stuff > >so differently. Can't we simply provide the same binding to the > >legacy codec from asoc driver? In the legacy-support mode, asoc > >driver creates the legacy-compatible codec objects with the > >legacy-compatible hda_bus. > > > > Both the drivers i.e. ASoC and Legacy are registering the driver in > exact same way by filling the hdac_driver fields. There is no difference > in terms of HDA bus interactions. First series unifies even the data > structures hdac_device, hdac_driver and hdac_bus. Yes, that's a good part. > Once the device is enumerated and the hdac_driver->probe > is called the difference starts, primarily because ASoC vs ALSA > codec driver differences. Why so different? Is it hard to integrate them somehow? What exactly do we need to do in addition to the existing legacy HD-audio codec probe/remove/whatever? > Here also if you notice, after taking care of > the ASoC related components, ASoC codec driver calls exact same APIs of the > legacy HDA codec driver which are called by the legacy controller driver. > > The hda_bus and hda_codec data structures are also used by the ASoC > driver as it is to call legacy codec driver APIs. > > So I am not sure if we are doing binding in two different ways, or I > misunderstood you completely ? Well, I still am not sure why do we need two ways, completely switching the whole binding. If it's a matter of some additional calls on top of the legacy probe, we can add a new optional bus ops, and call it in the probe function, too. At the time of probe callback, the bus is already present. thanks, Takashi
>-----Original Message----- >From: Takashi Iwai [mailto:tiwai@suse.de] >Sent: Tuesday, December 19, 2017 9:54 PM >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> >Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre- >louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio ><patches.audio@intel.com>; broonie@kernel.org >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA >codec driver > >> > >> >Well, a big unclear question to me is why do we need to bind the stuff >> >so differently. Can't we simply provide the same binding to the >> >legacy codec from asoc driver? In the legacy-support mode, asoc >> >driver creates the legacy-compatible codec objects with the >> >legacy-compatible hda_bus. >> > >> >> Both the drivers i.e. ASoC and Legacy are registering the driver in >> exact same way by filling the hdac_driver fields. There is no difference >> in terms of HDA bus interactions. First series unifies even the data >> structures hdac_device, hdac_driver and hdac_bus. > >Yes, that's a good part. > >> Once the device is enumerated and the hdac_driver->probe >> is called the difference starts, primarily because ASoC vs ALSA >> codec driver differences. > >Why so different? Is it hard to integrate them somehow? > >What exactly do we need to do in addition to the existing legacy >HD-audio codec probe/remove/whatever? probe/remove of the ASoC driver takes care of registering/unregistering the snd_soc_codec_driver with ASoC framework. Which would register dai, widgets and controls associated with the codec. But in this driver probe the card is not created and so nothing else is done. It waits until the ASoC framework calls snd_soc_codec probe. So the way I see is ASoC driver probe is just a bus level probe and its one level before the legacy codec driver probe. Legacy codec driver probe is kind of similar to snd_soc_codec probe. Again if you think from unification, we are calling the exact same APIs which are called in the legacy driver probe. Some legacy APIs are not used e.g. build_pcms. snd_soc_codec_driver callbacks are handled by ASoC framework and so we can not call it from anywhere else. But within this we call most of the APIs of the legacy codec driver. You can look at the ~/sound/soc/codecs/hdac_hda.c file, its just one file for the ASoC driver. > >> Here also if you notice, after taking care of >> the ASoC related components, ASoC codec driver calls exact same APIs of the >> legacy HDA codec driver which are called by the legacy controller driver. >> >> The hda_bus and hda_codec data structures are also used by the ASoC >> driver as it is to call legacy codec driver APIs. >> >> So I am not sure if we are doing binding in two different ways, or I >> misunderstood you completely ? > >Well, I still am not sure why do we need two ways, completely >switching the whole binding. If it's a matter of some additional >calls on top of the legacy probe, we can add a new optional bus ops, >and call it in the probe function, too. At the time of probe >callback, the bus is already present. From the bus point of view, there are no differences in the driver. The Series 1 has already done that. The difference is only due to ASoC vs ALSA framework. In the last patch, instead of checking the bus_type everywhere in the code, it uses it to call the individual driver function. That way the hdac_driver callback implementation remains separate and much cleaner for ASoC and legacy drivers. So the legacy codec driver APIs are still used but without too many if else conditions. Regards, Rakesh
On Tue, 19 Dec 2017 18:12:03 +0100, Ughreja, Rakesh A wrote: > > > > >-----Original Message----- > >From: Takashi Iwai [mailto:tiwai@suse.de] > >Sent: Tuesday, December 19, 2017 9:54 PM > >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> > >Cc: alsa-devel@alsa-project.org; Koul, Vinod <vinod.koul@intel.com>; pierre- > >louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio > ><patches.audio@intel.com>; broonie@kernel.org > >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA > >codec driver > > > >> > > >> >Well, a big unclear question to me is why do we need to bind the stuff > >> >so differently. Can't we simply provide the same binding to the > >> >legacy codec from asoc driver? In the legacy-support mode, asoc > >> >driver creates the legacy-compatible codec objects with the > >> >legacy-compatible hda_bus. > >> > > >> > >> Both the drivers i.e. ASoC and Legacy are registering the driver in > >> exact same way by filling the hdac_driver fields. There is no difference > >> in terms of HDA bus interactions. First series unifies even the data > >> structures hdac_device, hdac_driver and hdac_bus. > > > >Yes, that's a good part. > > > >> Once the device is enumerated and the hdac_driver->probe > >> is called the difference starts, primarily because ASoC vs ALSA > >> codec driver differences. > > > >Why so different? Is it hard to integrate them somehow? > > > >What exactly do we need to do in addition to the existing legacy > >HD-audio codec probe/remove/whatever? > > probe/remove of the ASoC driver takes care of registering/unregistering > the snd_soc_codec_driver with ASoC framework. Which would register > dai, widgets and controls associated with the codec. > > But in this driver probe the card is not created and so nothing else is done. > It waits until the ASoC framework calls snd_soc_codec probe. > > So the way I see is ASoC driver probe is just a bus level probe and its > one level before the legacy codec driver probe. > > Legacy codec driver probe is kind of similar to snd_soc_codec probe. > Again if you think from unification, we are calling the exact same APIs > which are called in the legacy driver probe. Some legacy APIs are not > used e.g. build_pcms. > > snd_soc_codec_driver callbacks are handled by ASoC framework > and so we can not call it from anywhere else. But within this we call > most of the APIs of the legacy codec driver. > > You can look at the ~/sound/soc/codecs/hdac_hda.c file, its just one file > for the ASoC driver. > > > > >> Here also if you notice, after taking care of > >> the ASoC related components, ASoC codec driver calls exact same APIs of the > >> legacy HDA codec driver which are called by the legacy controller driver. > >> > >> The hda_bus and hda_codec data structures are also used by the ASoC > >> driver as it is to call legacy codec driver APIs. > >> > >> So I am not sure if we are doing binding in two different ways, or I > >> misunderstood you completely ? > > > >Well, I still am not sure why do we need two ways, completely > >switching the whole binding. If it's a matter of some additional > >calls on top of the legacy probe, we can add a new optional bus ops, > >and call it in the probe function, too. At the time of probe > >callback, the bus is already present. > > From the bus point of view, there are no differences in the driver. > The Series 1 has already done that. > > The difference is only due to ASoC vs ALSA framework. > > In the last patch, instead of checking the bus_type everywhere in the code, > it uses it to call the individual driver function. That way the hdac_driver > callback implementation remains separate and much cleaner for ASoC > and legacy drivers. So the legacy codec driver APIs are still used but > without too many if else conditions. Well, honestly speaking, that complete morphing scares me. If it's a simple additional call, it's easier to track the code flow. But your patch essentially fakes the legacy codec and bus structs and twists the control by that switch... which looks too fragile to me. thanks, Takashi
On Tue, Dec 19, 2017 at 08:17:37PM +0100, Takashi Iwai wrote: > Well, honestly speaking, that complete morphing scares me. If it's a > simple additional call, it's easier to track the code flow. But your > patch essentially fakes the legacy codec and bus structs and twists > the control by that switch... which looks too fragile to me. Especially when it's layered on top of all the other x86 code which is also complicated and fragile - feels like there's lots of places things can go wrong and lots of potential unexpected interactions.
>-----Original Message----- >From: Mark Brown [mailto:broonie@kernel.org] >Sent: Wednesday, December 20, 2017 3:56 PM >To: Takashi Iwai <tiwai@suse.de> >Cc: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>; alsa-devel@alsa- >project.org; Koul, Vinod <vinod.koul@intel.com>; pierre- >louis.bossart@linux.intel.com; liam.r.girdwood@linux.intel.com; Patches Audio ><patches.audio@intel.com> >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based >HDA codec driver > >On Tue, Dec 19, 2017 at 08:17:37PM +0100, Takashi Iwai wrote: > >> Well, honestly speaking, that complete morphing scares me. If it's a >> simple additional call, it's easier to track the code flow. But your >> patch essentially fakes the legacy codec and bus structs and twists >> the control by that switch... which looks too fragile to me. > >Especially when it's layered on top of all the other x86 code which is >also complicated and fragile - feels like there's lots of places things >can go wrong and lots of potential unexpected interactions. Thanks, for the feedback Mark and Takashi. I will send the test patch based on the suggestions from Takashi within couple of days. Regards, Rakesh
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