[alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Takashi Iwai]

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 at kernel.org>; Takashi Iwai <tiwai at suse.de>
> >Cc: alsa-devel at alsa-project.org; Koul, Vinod <vinod.koul at intel.com>; pierre-
> >louis.bossart at linux.intel.com; liam.r.girdwood at linux.intel.com; Patches Audio
> ><patches.audio at 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 at 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
>