| Message ID | 85DFEED57DC57344B2483EF7BF8CB60579ADF4DE@BGSMSX104.gar.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 21 Dec 2017 16:36:47 +0100, Ughreja, Rakesh A wrote: > > > > >-----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. Yes, this is a sort of idea I had. My original thought was to have an extra ops in hdac_bus, and refer directly there instead of setting in each hda_codec object. Not coded yet, so I can't judge which one is better. Maybe you can actually quick-try and test differences. But still an open question is where to hook. You've put the branch at the very beginning of each probe/remove, that is, completely replacing the whole probe/remove callbacks. Meanwhile, the legacy codec driver still expects the legacy hda_codec object handling, so keeping more common stuff would make sense. That is, if we do switching at the beginning, the rest should just compose the same helper functions in slightly different ways. Or we share most parts of probe/remove in both legacy and asoc but branch off after some later point in the probe/remove functions. In anyway, a whole patchset would be helpful so that I can give it a try, too. But maybe I'll have little time tomorrow and a few days thereafter due to vacation. thanks, Takashi > > --- 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) > -- > 2.7.4 > > Regards, > Rakesh >
>-----Original Message----- >From: Takashi Iwai [mailto:tiwai@suse.de] >Sent: Thursday, December 21, 2017 9:18 PM >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> >Cc: Mark Brown <broonie@kernel.org>; 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 >> >> 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. > >Yes, this is a sort of idea I had. >My original thought was to have an extra ops in hdac_bus, and refer >directly there instead of setting in each hda_codec object. Not coded >yet, so I can't judge which one is better. Maybe you can actually >quick-try and test differences. Let me try that out and will move to hdac_bus if I don't run into any practical issues. > >But still an open question is where to hook. You've put the branch at >the very beginning of each probe/remove, that is, completely replacing >the whole probe/remove callbacks. Meanwhile, the legacy codec driver >still expects the legacy hda_codec object handling, so keeping more >common stuff would make sense. That is, if we do switching at the >beginning, the rest should just compose the same helper functions in >slightly different ways. Or we share most parts of probe/remove in >both legacy and asoc but branch off after some later point in the >probe/remove functions. I think I am wrong in doing the above. I was thinking that I cannot do all those steps done in the probe before the snd_soc_codec probe gets called. After looking at the code again after your comments, It looks like I can call all these before the card is registered by the machine driver. Is that right understanding ? same thing is happening in legacy driver also. The card is registered at the end. So I will just skip that step. > >In anyway, a whole patchset would be helpful so that I can give it a >try, too. But maybe I'll have little time tomorrow and a few days >thereafter due to vacation. Sure, will send you the series tomorrow by correcting few more things. Regards, Rakesh
On Thu, 21 Dec 2017 17:39:10 +0100, Ughreja, Rakesh A wrote: > > > > >-----Original Message----- > >From: Takashi Iwai [mailto:tiwai@suse.de] > >Sent: Thursday, December 21, 2017 9:18 PM > >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> > >Cc: Mark Brown <broonie@kernel.org>; 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 > > >> > >> 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. > > > >Yes, this is a sort of idea I had. > >My original thought was to have an extra ops in hdac_bus, and refer > >directly there instead of setting in each hda_codec object. Not coded > >yet, so I can't judge which one is better. Maybe you can actually > >quick-try and test differences. > > Let me try that out and will move to hdac_bus if I don't run into any > practical issues. > > > > >But still an open question is where to hook. You've put the branch at > >the very beginning of each probe/remove, that is, completely replacing > >the whole probe/remove callbacks. Meanwhile, the legacy codec driver > >still expects the legacy hda_codec object handling, so keeping more > >common stuff would make sense. That is, if we do switching at the > >beginning, the rest should just compose the same helper functions in > >slightly different ways. Or we share most parts of probe/remove in > >both legacy and asoc but branch off after some later point in the > >probe/remove functions. > > I think I am wrong in doing the above. I was thinking that I cannot do > all those steps done in the probe before the snd_soc_codec probe gets > called. > > After looking at the code again after your comments, It looks like > I can call all these before the card is registered by the machine driver. > > Is that right understanding ? same thing is happening in legacy > driver also. The card is registered at the end. So I will just skip that step. Well, I'm not 100% sure about the ordering, so we just need trying. > >In anyway, a whole patchset would be helpful so that I can give it a > >try, too. But maybe I'll have little time tomorrow and a few days > >thereafter due to vacation. > > Sure, will send you the series tomorrow by correcting few more things. OK, thanks. Takashi
>-----Original Message----- >From: Takashi Iwai [mailto:tiwai@suse.de] >Sent: Thursday, December 21, 2017 10:15 PM >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> >Cc: Mark Brown <broonie@kernel.org>; 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 > >> >> I think I am wrong in doing the above. I was thinking that I cannot do >> all those steps done in the probe before the snd_soc_codec probe gets >> called. >> >> After looking at the code again after your comments, It looks like >> I can call all these before the card is registered by the machine driver. >> >> Is that right understanding ? same thing is happening in legacy >> driver also. The card is registered at the end. So I will just skip that step. > >Well, I'm not 100% sure about the ordering, so we just need trying. > Hi Takashi, I tried various things, but I could not unify the probe of the legacy hda codec driver and the asoc library probe, mainly because all the APIs in the legacy hda codec drivers require "snd_card" pointer, which is getting allocated by the machine driver when the machine driver is loaded. So except the regmap init I could not do anything in the legacy probe. So for now I have kept them completely separate but I can do regmap init when the bus type is EXT. Regards, Rakesh
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)