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