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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
