Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Tue, 26 Sep 2023 21:35:23 +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>,
        Vincent Knecht <vincent.knecht@mailoo.org>
Subject: Re: [PATCH 07/13] arm64: dts: qcom: msm8916-alcatel-idol347: Add
 sound and modem
Message-ID: <ZRMye1HeiUno5N_p@gerhold.net>
References: <20230926-msm8916-modem-v1-0-398eec74bac9@gerhold.net>
 <20230926-msm8916-modem-v1-7-398eec74bac9@gerhold.net>
 <cb78cd52-c638-4977-a762-365a6fe5f723@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cb78cd52-c638-4977-a762-365a6fe5f723@linaro.org>
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, Sep 26, 2023 at 08:58:12PM +0200, Konrad Dybcio wrote:
> On 26.09.2023 18:51, Stephan Gerhold wrote:
> > From: Vincent Knecht <vincent.knecht@mailoo.org>
> > 
> > Enable sound and modem for the Alcatel Idol 3 (4.7"). The setup is
> > similar to most MSM8916 devices, i.e.:
> > 
> >  - QDSP6 audio
> >  - Microphones via digital/analog codec in MSM8916/PM8916
> >  - WWAN Internet via BAM-DMUX
> > 
> > except:
> > 
> >  - Stereo NXP TFA9890 codecs for speakers on Quaternary MI2S
> >    - These are also used as earpieces at the top/bottom.
> >  - Asahi Kasei AK4375 headphone codec on Secondary MI2S
> >  -> Primary MI2S is not used for playback
> > 
> > Signed-off-by: Vincent Knecht <vincent.knecht@mailoo.org>
> > [Stephan: minor cleanup, add consistent commit message]
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > There are some trivial conflicts unless
> > https://lore.kernel.org/linux-arm-msm/20230921-msm8916-rmem-fixups-v1-3-34d2b6e721cf@gerhold.net/
> > is applied first. But given that there are important fixups for the
> > dynamic reserved memory changes in that series it should preferably
> > get applied before this one anyway.
> > ---
> >  .../boot/dts/qcom/msm8916-alcatel-idol347.dts      | 164 +++++++++++++++++++++
> >  1 file changed, 164 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916-alcatel-idol347.dts b/arch/arm64/boot/dts/qcom/msm8916-alcatel-idol347.dts
> > index fade93c55299..ef5fc9289754 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8916-alcatel-idol347.dts
> > +++ b/arch/arm64/boot/dts/qcom/msm8916-alcatel-idol347.dts
> > @@ -3,6 +3,8 @@
> >  /dts-v1/;
> >  
> >  #include "msm8916-pm8916.dtsi"
> > +#include "msm8916-modem-qdsp6.dtsi"
> > +
> >  #include <dt-bindings/gpio/gpio.h>
> >  #include <dt-bindings/input/input.h>
> >  #include <dt-bindings/leds/common.h>
> > @@ -22,6 +24,19 @@ chosen {
> >  		stdout-path = "serial0";
> >  	};
> >  
> > +	reserved-memory {
> > +		/delete-node/ reserved@86680000;
> > +		/delete-node/ rmtfs@86700000;
> Deleting with a label reference is strongly preferred to avoid
> mistakes.
> 

I would say the opposite applies here. The deletions are based on the
assumption that the nodes are at the address that are listed here. If
you would move rmtfs somewhere else the adjustments made here must be
re-evaulated.

/delete-node/ throws an error if the referenced name does not exist,
so it's exactly the indication we need if someone makes changes to the
original node in the SoC dtsi.

Note that this is different from property assignments, i.e.

	/ {
		reserved-memory {
			rmtfs@86700000 {
				status = "disabled";
			};
		};
	};

instead of

	&rmtfs {
		status = "disabled";
	};

because here there would not be an error if the node is renamed.

> [...]
> 
> >  
> > +&q6afedai {
> > +	dai@18 {
> > +		reg = <SECONDARY_MI2S_RX>;
> > +		qcom,sd-lines = <0>;
> > +	};
> > +	dai@22 {
> Missing newline above
> 

Thanks, will fix this!

> 
> > +
> > +&sound_dai_primary {
> > +	status = "disabled";
> > +};
> > +
> Hm, gives me an idea to sprinkle a bit more /omit-if-no-ref/ in
> patch 3..
> 

(See reply in patch 3, /omit-if-no-ref/ sadly only works for phandle
 references...)

Thanks,
Stephan