From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9F93DC43334 for ; Wed, 20 Jul 2022 13:15:01 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 3FDB31777; Wed, 20 Jul 2022 15:14:09 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 3FDB31777 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1658322899; bh=VV3TXfJF0EGk3ufQk/OkbrFq9j02fP3U8nsiny60AnA=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=NGE7Hrc3MJWGd2TcAn8KAwX6AtYec1ofjAL4eIaDsIv17jCkAtO9hDVbu5AeAaCbw jgA3mFOJMhWGtd/wm7Wsr3cqVjoHlfh/lk/KhYqexL/rrWuhRUAryULXf938I5oeTL Fprx6ksPZ8SOALfJYPSUJOHEhh9ZDxITSj8snOiA= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id BE323F80139; Wed, 20 Jul 2022 15:14:08 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id B42B9F80118; Wed, 20 Jul 2022 15:14:06 +0200 (CEST) Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id A6BF5F80118 for ; Wed, 20 Jul 2022 15:13:58 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz A6BF5F80118 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.b="tda1LyzF"; dkim=permerror (0-bit key) header.d=suse.de header.i=@suse.de header.b="5vgllVUj" Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 41D1833CAA; Wed, 20 Jul 2022 13:13:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1658322838; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=t4eDORIH0s8UCVW1NUUF4WcZVmgJBELyeyUkmABGBQo=; b=tda1LyzF4ypn0zvV8j3U+s+jXUp9O4nB0t///OkzG1VE4an4FDLxIFBN6uD6ZC9yhHqdpN CWVcvSk/bvTrIunGIrAUit0KzCqu2Y9ygwRuvP5Bl2a1C2DUZjZaS7O3VrDZHRIiftR93K qJuUk7xuiENyQmPz2Zrgz3KzvP4Nhcs= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1658322838; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=t4eDORIH0s8UCVW1NUUF4WcZVmgJBELyeyUkmABGBQo=; b=5vgllVUjj9/3HkZMt20LAQ+huv8Pf2fmHkHTRr7boAX4QaYZPT/WCuyo0b958pNh9xvW6/ QX1Ldi35y6sP4ZCA== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 071AC13AA1; Wed, 20 Jul 2022 13:13:58 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id 0ZPnAJb/12LUeQAAMHmgww (envelope-from ); Wed, 20 Jul 2022 13:13:58 +0000 Date: Wed, 20 Jul 2022 15:13:57 +0200 Message-ID: <875yjs53l6.wl-tiwai@suse.de> From: Takashi Iwai To: Cezary Rojewski Subject: Re: [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor In-Reply-To: <20220720130622.146973-3-cezary.rojewski@intel.com> References: <20220720130622.146973-1-cezary.rojewski@intel.com> <20220720130622.146973-3-cezary.rojewski@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0 MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Cc: alsa-devel@alsa-project.org, kai.vehmanen@linux.intel.com, lgirdwood@gmail.com, yung-chuan.liao@linux.intel.com, pierre-louis.bossart@linux.intel.com, tiwai@suse.com, hdegoede@redhat.com, broonie@kernel.org, ranjani.sridharan@linux.intel.com, amadeuszx.slawinski@linux.intel.com, peter.ujfalusi@linux.intel.com X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" On Wed, 20 Jul 2022 15:06:20 +0200, Cezary Rojewski wrote: > > Refactor snd_hdac_ext_bus_device_init() so that it makes use of > snd_hda_codec_device_init() to create and initialize new codec device. > Causes the latter to become the sole codec device constructor. > > Users of the refactored function are updated accordingly and now also > take responsibility for assigning driver's private data as that task is > no longer performed by hdac_hda_dev_probe(). Hrm, this doesn't look really right. It means you'll introduce a hard dependency chain in a reverse order: snd-hda-ext-core -> snd-hda-codec. Originally, the ext bus code was written completely independent from the legacy HD-audio implementations, and hdac-hda driver was a kind of wrapper / bridge for the legacy codec over the ext bus. If we want change this rule and make the legacy HD-audio codec always tied with the ext bus, a likely better way would be to call snd_hda_codec_device_init() in the caller's side (e.g. skl or sof), then pass the newly created codec object to snd_hdac_ext_bus_device_init() for further initialization. thanks, Takashi > > Signed-off-by: Cezary Rojewski > --- > include/sound/hdaudio_ext.h | 4 ++-- > sound/hda/ext/hdac_ext_bus.c | 34 +++++++++++++++------------------ > sound/soc/intel/skylake/skl.c | 24 ++++++++++------------- > sound/soc/sof/intel/hda-codec.c | 29 ++++++++++++---------------- > 4 files changed, 39 insertions(+), 52 deletions(-) > > diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h > index d26234f9ee46..25c7b13db278 100644 > --- a/include/sound/hdaudio_ext.h > +++ b/include/sound/hdaudio_ext.h > @@ -11,8 +11,8 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev, > const struct hdac_ext_bus_ops *ext_ops); > > void snd_hdac_ext_bus_exit(struct hdac_bus *bus); > -int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, > - struct hdac_device *hdev, int type); > +struct hda_codec * > +snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, int type); > void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev); > void snd_hdac_ext_bus_device_remove(struct hdac_bus *bus); > > diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c > index 765c40a6ccba..bd3c7124aca1 100644 > --- a/sound/hda/ext/hdac_ext_bus.c > +++ b/sound/hda/ext/hdac_ext_bus.c > @@ -12,6 +12,7 @@ > #include > #include > #include > +#include > #include > > MODULE_DESCRIPTION("HDA extended core"); > @@ -67,39 +68,34 @@ static void default_release(struct device *dev) > > /** > * snd_hdac_ext_bus_device_init - initialize the HDA extended codec base device > - * @bus: hdac bus to attach to > + * @bus: hda bus to attach to > * @addr: codec address > - * @hdev: hdac device to init > * @type: codec type (HDAC_DEV_*) to use for this device > * > - * Returns zero for success or a negative error code. > + * Returns pointer to newly created codec or ERR_PTR. > */ > -int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, > - struct hdac_device *hdev, int type) > +struct hda_codec * > +snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, int type) > { > - char name[15]; > + struct hda_codec *codec; > int ret; > > - hdev->bus = bus; > - > - snprintf(name, sizeof(name), "ehdaudio%dD%d", bus->idx, addr); > - > - ret = snd_hdac_device_init(hdev, bus, name, addr); > - if (ret < 0) { > + codec = snd_hda_codec_device_init(to_hda_bus(bus), addr, "ehdaudio%dD%d", bus->idx, addr); > + if (IS_ERR(codec)) { > dev_err(bus->dev, "device init failed for hdac device\n"); > - return ret; > + return codec; > } > - hdev->type = type; > - hdev->dev.release = default_release; > + codec->core.type = type; > + codec->core.dev.release = default_release; > > - ret = snd_hdac_device_register(hdev); > + ret = snd_hdac_device_register(&codec->core); > if (ret) { > dev_err(bus->dev, "failed to register hdac device\n"); > - snd_hdac_ext_bus_device_exit(hdev); > - return ret; > + snd_hdac_ext_bus_device_exit(&codec->core); > + return ERR_PTR(ret); > } > > - return 0; > + return codec; > } > EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_init); > > diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c > index 7e573a887118..5637292c2aa9 100644 > --- a/sound/soc/intel/skylake/skl.c > +++ b/sound/soc/intel/skylake/skl.c > @@ -700,9 +700,8 @@ static int probe_codec(struct hdac_bus *bus, int addr) > struct skl_dev *skl = bus_to_skl(bus); > #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) > struct hdac_hda_priv *hda_codec; > - int err; > #endif > - struct hdac_device *hdev; > + struct hda_codec *codec; > > mutex_lock(&bus->cmd_mutex); > snd_hdac_bus_send_cmd(bus, cmd); > @@ -718,25 +717,22 @@ static int probe_codec(struct hdac_bus *bus, int addr) > if (!hda_codec) > return -ENOMEM; > > - hda_codec->codec->bus = skl_to_hbus(skl); > - hdev = &hda_codec->codec->core; > + codec = snd_hdac_ext_bus_device_init(bus, addr, HDA_DEV_ASOC); > + if (IS_ERR(codec)) > + return PTR_ERR(codec); > > - err = snd_hdac_ext_bus_device_init(bus, addr, hdev, HDA_DEV_ASOC); > - if (err < 0) > - return err; > + hda_codec->codec = codec; > + dev_set_drvdata(&codec->core.dev, hda_codec); > > /* use legacy bus only for HDA codecs, idisp uses ext bus */ > if ((res & 0xFFFF0000) != IDISP_INTEL_VENDOR_ID) { > - hdev->type = HDA_DEV_LEGACY; > - load_codec_module(hda_codec->codec); > + codec->core.type = HDA_DEV_LEGACY; > + load_codec_module(codec); > } > return 0; > #else > - hdev = devm_kzalloc(&skl->pci->dev, sizeof(*hdev), GFP_KERNEL); > - if (!hdev) > - return -ENOMEM; > - > - return snd_hdac_ext_bus_device_init(bus, addr, hdev, HDA_DEV_ASOC); > + codec = snd_hdac_ext_bus_device_init(bus, addr, HDA_DEV_ASOC); > + return PTR_ERR_OR_ZERO(codec); > #endif /* CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC */ > } > > diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c > index de7c53224ac3..7c3ea4a12d63 100644 > --- a/sound/soc/sof/intel/hda-codec.c > +++ b/sound/soc/sof/intel/hda-codec.c > @@ -115,11 +115,10 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address, > { > #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC) > struct hdac_hda_priv *hda_priv; > - struct hda_codec *codec; > int type = HDA_DEV_LEGACY; > #endif > struct hda_bus *hbus = sof_to_hbus(sdev); > - struct hdac_device *hdev; > + struct hda_codec *codec; > u32 hda_cmd = (address << 28) | (AC_NODE_ROOT << 20) | > (AC_VERB_PARAMETERS << 8) | AC_PAR_VENDOR_ID; > u32 resp = -1; > @@ -142,20 +141,19 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address, > if (!hda_priv) > return -ENOMEM; > > - hda_priv->codec->bus = hbus; > - hdev = &hda_priv->codec->core; > - codec = hda_priv->codec; > - > /* only probe ASoC codec drivers for HDAC-HDMI */ > if (!hda_codec_use_common_hdmi && (resp & 0xFFFF0000) == IDISP_VID_INTEL) > type = HDA_DEV_ASOC; > > - ret = snd_hdac_ext_bus_device_init(&hbus->core, address, hdev, type); > - if (ret < 0) > - return ret; > + codec = snd_hdac_ext_bus_device_init(&hbus->core, address, type); > + if (IS_ERR(codec)) > + return PTR_ERR(codec); > + > + hda_priv->codec = codec; > + dev_set_drvdata(&codec->core.dev, hda_priv); > > if ((resp & 0xFFFF0000) == IDISP_VID_INTEL) { > - if (!hdev->bus->audio_component) { > + if (!hbus->core.audio_component) { > dev_dbg(sdev->dev, > "iDisp hw present but no driver\n"); > ret = -ENOENT; > @@ -181,15 +179,12 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address, > > out: > if (ret < 0) { > - snd_hdac_device_unregister(hdev); > - put_device(&hdev->dev); > + snd_hdac_device_unregister(&codec->core); > + put_device(&codec->core.dev); > } > #else > - hdev = devm_kzalloc(sdev->dev, sizeof(*hdev), GFP_KERNEL); > - if (!hdev) > - return -ENOMEM; > - > - ret = snd_hdac_ext_bus_device_init(&hbus->core, address, hdev, HDA_DEV_ASOC); > + codec = snd_hdac_ext_bus_device_init(&hbus->core, address, HDA_DEV_ASOC); > + ret = PTR_ERR_OR_ZERO(codec); > #endif > > return ret; > -- > 2.25.1 >