Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Tue, 26 Sep 2023 21:06:18 +0200
From:   Stephan Gerhold <stephan@gerhold.net>
To:     Konrad Dybcio <konrad.dybcio@linaro.org>
Cc:     Bjorn Andersson <andersson@kernel.org>,
        Andy Gross <agross@kernel.org>, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        phone-devel@vger.kernel.org, ~postmarketos/upstreaming@lists.sr.ht,
        Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Subject: Re: [PATCH 03/13] arm64: dts: qcom: msm8916: Add common
 msm8916-modem-qdsp6.dtsi
Message-ID: <ZRMrqsZ0QeDNFHFj@gerhold.net>
References: <20230926-msm8916-modem-v1-0-398eec74bac9@gerhold.net>
 <20230926-msm8916-modem-v1-3-398eec74bac9@gerhold.net>
 <45665b43-3be9-4f27-aa88-12cdef56346d@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <45665b43-3be9-4f27-aa88-12cdef56346d@linaro.org>
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, Sep 26, 2023 at 08:49:24PM +0200, Konrad Dybcio wrote:
> On 26.09.2023 18:51, Stephan Gerhold wrote:
> > Most MSM8916/MSM8939 devices use very similar setups for the modem,
> > because most of the device-specific details are abstracted by the modem
> > firmware. There are several definitions (status switches, DAI links
> > etc) that will be exactly the same for every board.
> > 
> > Introduce a common msm8916-modem-qdsp6.dtsi include that can be used to
> > simplify enabling the modem for such devices. By default the
> > digital/analog codec in the SoC/PMIC is used, but boards can define
> > additional codecs using the templates for Secondary and Quaternary
> > MI2S.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> I'd rather see at least one usage so that you aren't introducing
> effectively non-compiled code..
> 

There are 10 usages in the rest of the patch series.
Is that enough? :D

IMHO it doesn't make sense to squash this with one of the device
patches, especially considering several of them are primarily authored
by others.

> >  arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi | 163 ++++++++++++++++++++++
> >  1 file changed, 163 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi b/arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi
> > new file mode 100644
> > index 000000000000..ddd74d428406
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/msm8916-modem-qdsp6.dtsi
> > @@ -0,0 +1,163 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> > +/*
> > + * msm8916-modem-qdsp6.dtsi describes the typical modem setup on MSM8916 devices
> > + * (or similar SoCs) with audio routed via the QDSP6 services provided by the
> > + * modem firmware. The digital/analog codec in the SoC/PMIC is used by default,
> > + * but boards can define additional codecs using the templates for Secondary and
> > + * Quaternary MI2S.
> > + */
> > +
> > +#include <dt-bindings/sound/qcom,q6afe.h>
> > +#include <dt-bindings/sound/qcom,q6asm.h>
> > +
> > +&apr {
> > +	status = "okay";
> > +};
> > +
> > +&bam_dmux {
> > +	status = "okay";
> > +};
> > +
> > +&bam_dmux_dma {
> > +	status = "okay";
> > +};
> > +
> > +&lpass {
> > +	status = "reserved"; /* Controlled by QDSP6 */
> > +};
> > +
> > +&lpass_codec {
> > +	status = "okay";
> > +};
> Any reason for it to stay disabled?
> 

You mean in msm8916.dtsi? For the SoC dtsi we don't make assumptions
what devices use or not. There could be devices that ignore the internal
codec entirely. If there is nothing connected to the codec lpass_codec
should not be enabled (e.g. the msm8916-ufi.dtsi devices).

This include is a bit more "opinionated", to reduce duplication for
the most common setup. But it's separate and optional to use. The SoC
dtsi is included by everyone.

> > +
> > +&mba_mem {
> > +	status = "okay";
> > +};
> > +
> > +&mpss {
> > +	status = "okay";
> > +};
> > +
> > +&mpss_mem {
> > +	status = "okay";
> > +};
> > +
> > +&pm8916_codec {
> > +	status = "okay";
> > +};
> Ditto
> 

Same as above.

> > +	multimedia1-dai-link {
> > +		link-name = "MultiMedia1";
> Newline before last property and subnodes, please
> 

Thanks, will change this!
> 
> > +	sound_dai_secondary: mi2s-secondary-dai-link {
> > +		link-name = "Secondary MI2S";
> > +		status = "disabled"; /* Needs extra codec configuration */
> Hmm.. Potential good user of /omit-if-no-ref/?
> 

AFAICT /omit-if-no-ref/ is for phandle references only. Basically it
would only work if you would somewhere reference the phandle:

	list-of-sound-dais = <&sound_dai_primary &sound_dai_secondary>;

But this doesn't exist so /omit-if-no-ref/ cannot be used here.

Thanks,
Stephan