[alsa-devel] [PATCH v1 8/9] ASoC: hdac_hda: add asoc extension for legacy HDA codec drivers

[Takashi Iwai]

On Fri, 23 Feb 2018 09:12:29 +0100,
Rakesh Ughreja wrote:
> 
> +static int hdac_hda_codec_probe(struct snd_soc_codec *codec)
> +{
> +	struct hdac_hda_priv *hda_pvt = snd_soc_codec_get_drvdata(codec);
> +	struct snd_soc_dapm_context *dapm =
> +			snd_soc_component_get_dapm(&codec->component);
> +	struct hdac_device *hdev = &hda_pvt->codec.core;
> +	struct hda_codec *hcodec = &hda_pvt->codec;
> +	struct hdac_ext_link *hlink = NULL;
> +	hda_codec_patch_t patch;
> +	int ret, i = 0;
> +	u16 codec_mask;
> +
> +	hda_pvt->scodec = codec;
> +	hlink = snd_hdac_ext_bus_get_link(hdev->bus, dev_name(&hdev->dev));
> +	if (!hlink) {
> +		dev_err(&hdev->dev, "hdac link not found\n");
> +		return -EIO;
> +	}
> +
> +	ret = snd_hdac_ext_bus_link_get(hdev->bus, hlink);

So this is the essential part for the ext-hda stuff.  But...

> +
> +	udelay(1000);
> +	do {
> +		codec_mask = snd_hdac_chip_readw(hdev->bus, STATESTS);
> +		if (codec_mask)
> +			break;
> +		i++;
> +		udelay(100);
> +	} while (i < 100);
> +
> +	if (!codec_mask) {
> +		dev_err(&hdev->dev, "No codecs found after link reset\n");
> +		return -EIO;
> +	}
> +
> +	snd_hdac_chip_writew(hdev->bus, STATESTS, STATESTS_INT_MASK);

Why do you need this?  The callback gets called by the HD-audio
controller driver and it already should have checked the codec mask
bits.


> +	ret = snd_hda_codec_device_new(hcodec->bus,
> +			codec->component.card->snd_card, hdev->addr, hcodec);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "failed to create hda codec %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * snd_hda_codec_new1 decrements the usage count and so get the pm
> +	 * count else the device will be powered off
> +	 */
> +	pm_runtime_get_noresume(&hdev->dev);
> +
> +	hcodec->bus->card = dapm->card->snd_card;
> +
> +	patch = (hda_codec_patch_t)hcodec->preset->driver_data;
> +	if (patch) {
> +		ret = patch(hcodec);
> +		if (ret < 0) {
> +			dev_err(codec->dev, "patch failed %d\n", ret);
> +			return ret;
> +		}
> +	} else {
> +		dev_dbg(&hdev->dev, "no patch file found\n");
> +	}
> +
> +	ret = snd_hda_codec_set_name(hcodec, hcodec->preset->name);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "name failed %s\n", hcodec->preset->name);
> +		return ret;
> +	}
> +
> +	ret = snd_hdac_regmap_init(&hcodec->core);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "regmap init failed\n");
> +		return ret;
> +	}

The regmap and the codec name must be initialized before calling the
patch (i.e. the real probe stuff).

> +
> +	ret = snd_hda_codec_parse_pcms(hcodec);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "unable to map pcms to dai %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = snd_hda_codec_build_controls(hcodec);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "unable to create controls %d\n", ret);
> +		return ret;
> +	}
> +
> +	hcodec->core.lazy_cache = true;
> +
> +	/*
> +	 * hdac_device core already sets the state to active and calls
> +	 * get_noresume. So enable runtime and set the device to suspend.
> +	 * pm_runtime_enable is also called during codec registeration
> +	 */
> +	pm_runtime_put(&hdev->dev);
> +	pm_runtime_suspend(&hdev->dev);
> +
> +	return 0;
> +}
> +
> +static int hdac_hda_codec_remove(struct snd_soc_codec *codec)
> +{
> +	struct hdac_hda_priv *hda_pvt = snd_soc_codec_get_drvdata(codec);
> +	struct hdac_device *hdev = &hda_pvt->codec.core;
> +	struct hdac_ext_link *hlink = NULL;
> +
> +	hlink = snd_hdac_ext_bus_get_link(hdev->bus, dev_name(&hdev->dev));
> +	if (!hlink) {
> +		dev_err(&hdev->dev, "hdac link not found\n");
> +		return -EIO;
> +	}
> +
> +	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
> +	pm_runtime_disable(&hdev->dev);
> +
> +	return 0;
> +}

.... and I see no proper error paths there, and some cleanups seem
missing, too.

Now I think what you need is rather not to open-code again the mostly
same procedure from hda_codec_driver_probe() / _remove(), but to let
hda_codec_driver_probe() and remove() skip some unnecessary steps for
the ext codec (e.g. registration), in addition to the hlink setup.

That is, hda_codec_driver_probe() would be like:

static int hda_codec_driver_probe(struct device *dev)
{
	....

 	if (WARN_ON(!codec->preset))
 		return -EINVAL;

	if (bus->core.ext_ops) {
		if (!WARN_ON(bus->core.ext_ops->probe))
			return -EINVAL;
		/* register hlink */
		err = bus->core.ext_ops->probe(&codec->core);
		if (err < 0)
			return err;
	}

	err = snd_hda_codec_set_name(codec, codec->preset->name);
	if (err < 0)
		goto error;
	err = snd_hdac_regmap_init(&codec->core);
	if (err < 0)
		goto error;
	.....

	if (!bus->core.ext_ops &&
	    codec->card->registered) {
		err = snd_card_register(codec->card);
		if (err < 0)
		....
	}

	codec->core.lazy_cache = true;
	return 0;

 error_module:
	....
}

static int hda_codec_driver_remove(struct device *dev)
{
	struct hda_codec *codec = dev_to_hda_codec(dev);
	int err;

	if (codec->patch_ops.free)
		codec->patch_ops.free(codec);
	snd_hda_codec_cleanup_for_unbind(codec);
	if (codec->bus->core.ext_ops && codec->bus->core.ext_ops->remove)
		codec->bus->core.ext_ops->remove(&codec->core);
	module_put(dev->driver->owner);
	return 0;
}

... and looking at this, we may rather add the hlink add/remove to
hdac_bus_ops, instead of defining a new ops struct, too.

struct hdac_bus_ops {
	....
	/* reference/unreference ext-bus codec link */
	int (*link_ref)(struct hdac_bus *bus, struct hdac_device *dev);
	int (*link_unref)(struct hdac_bus *bus, struct hdac_device *dev);
};


thanks,

Takashi