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@xxxxxxx> > >Cc: alsa-devel@xxxxxxxxxxxxxxxx; broonie@xxxxxxxxxx; > >liam.r.girdwood@xxxxxxxxxxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx; Koul, > >Vinod <vinod.koul@xxxxxxxxx>; Patches Audio <patches.audio@xxxxxxxxx> > >Subject: RE: [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec > >driver > > > > > > > >>-----Original Message----- > >>From: Takashi Iwai [mailto:tiwai@xxxxxxx] > >>Sent: Saturday, December 16, 2017 2:44 PM > >>To: Ughreja, Rakesh A <rakesh.a.ughreja@xxxxxxxxx> > >>Cc: alsa-devel@xxxxxxxxxxxxxxxx; broonie@xxxxxxxxxx; > >>liam.r.girdwood@xxxxxxxxxxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx; Koul, > >Vinod > >><vinod.koul@xxxxxxxxx>; Patches Audio <patches.audio@xxxxxxxxx> > >>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@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxx > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
