-----Original Message----- From: Ughreja, Rakesh A Sent: Wednesday, December 20, 2017 4:23 PM To: Mark Brown broonie@kernel.org; Takashi Iwai tiwai@suse.de 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 Subject: RE: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver
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.
Hi Takashi/Mark,
I tried working on the concept and it turned out to be much simpler than what I thought. I added a type field for the bus which can be queried at runtime by the legacy codec driver and based on the type it can call the callbacks of the hdac_hda library. With this change the hdac_hda.c file is just a passive library and not a driver, which provides APIs which are called by the legacy codec driver to work with ASoC platform drivers.
There are only two callbacks required, i.e. probe and remove. This is to register and unregister the asoc codec.
Let me know if this is the right way to go. I can send you the full series once you are okay with the direction.
--- test patch below -- This patch adds bus type field in hdac_bus structure to distinguish the bus instance. Bus allocated using snd_hdac_bus_init API sets the bus type to HDA_BUS_LEGACY and snd_hdac_ext_bus_init API sets it to HDA_BUS_EXT. codec drivers can identify the bus type by calling snd_hdac_get_bus_type API.
Signed-off-by: Rakesh Ughreja rakesh.a.ughreja@intel.com --- include/sound/hdaudio.h | 15 +++++++++++++++ sound/hda/ext/hdac_ext_bus.c | 1 + sound/hda/hdac_bus.c | 1 + sound/pci/hda/hda_bind.c | 14 ++++++++++++++ sound/pci/hda/hda_codec.h | 12 ++++++++++++ 5 files changed, 43 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 30ec1b0..9db6a11 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -97,6 +97,12 @@ enum { HDA_DEV_ASOC, };
+enum { + HDA_BUS_INVALID, + HDA_BUS_LEGACY, + HDA_BUS_EXT, +}; + /* direction */ enum { HDA_INPUT, HDA_OUTPUT @@ -265,6 +271,7 @@ struct hdac_rb { */ struct hdac_bus { struct device *dev; + int type; const struct hdac_bus_ops *ops; const struct hdac_io_ops *io_ops;
@@ -341,6 +348,14 @@ struct hdac_bus {
};
+static inline int snd_hdac_get_bus_type(struct hdac_bus *bus) +{ + if (bus) + return bus->type; + else + return HDA_BUS_INVALID; +} + int snd_hdac_bus_init(struct hdac_bus *bus, struct device *dev, const struct hdac_bus_ops *ops, const struct hdac_io_ops *io_ops); diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c index e4bcb76..9d002da 100644 --- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c @@ -102,6 +102,7 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev, if (ret < 0) return ret;
+ bus->type = HDA_BUS_EXT; INIT_LIST_HEAD(&bus->hlink_list); bus->idx = idx++;
diff --git a/sound/hda/hdac_bus.c b/sound/hda/hdac_bus.c index 714a517..bb7567d 100644 --- a/sound/hda/hdac_bus.c +++ b/sound/hda/hdac_bus.c @@ -35,6 +35,7 @@ int snd_hdac_bus_init(struct hdac_bus *bus, struct device *dev, else bus->ops = &default_ops; bus->io_ops = io_ops; + bus->type = HDA_BUS_LEGACY; INIT_LIST_HEAD(&bus->stream_list); INIT_LIST_HEAD(&bus->codec_list); INIT_WORK(&bus->unsol_work, process_unsol_events); diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c index d361bb7..1998376 100644 --- a/sound/pci/hda/hda_bind.c +++ b/sound/pci/hda/hda_bind.c @@ -81,6 +81,15 @@ static int hda_codec_driver_probe(struct device *dev) hda_codec_patch_t patch; int err;
+#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA) + dev_err(dev, "BUS Type = %d\n", snd_hdac_get_bus_type(&codec->bus->core)); + if (HDA_BUS_EXT == snd_hdac_get_bus_type(&codec->bus->core)) { + if (!codec->ext_ops) + return -EINVAL; + return codec->ext_ops->probe(&codec->core); + } +#endif + if (WARN_ON(!codec->preset)) return -EINVAL;
@@ -134,6 +143,11 @@ static int hda_codec_driver_remove(struct device *dev) { struct hda_codec *codec = dev_to_hda_codec(dev);
+#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA) + if (HDA_BUS_EXT == snd_hdac_get_bus_type(&codec->bus->core)) { + return codec->ext_ops->remove(&codec->core); + } +#endif if (codec->patch_ops.free) codec->patch_ops.free(codec); snd_hda_codec_cleanup_for_unbind(codec); diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 8bbedf7..2b7b7db 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -108,6 +108,13 @@ void hda_codec_driver_unregister(struct hda_codec_driver *drv); module_driver(drv, hda_codec_driver_register, \ hda_codec_driver_unregister)
+#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA) +struct snd_hda_ext_ops { + int (*probe)(struct hdac_device *hdev); + int (*remove)(struct hdac_device *hdev); +}; +#endif + /* ops set by the preset patch */ struct hda_codec_ops { int (*build_controls)(struct hda_codec *codec); @@ -289,6 +296,11 @@ struct hda_codec {
/* additional init verbs */ struct snd_array verbs; + +#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA) + struct snd_hda_ext_ops *ext_ops; +#endif + };
#define dev_to_hda_codec(_dev) container_of(_dev, struct hda_codec, core.dev)