Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Tue, 26 Sep 2023 21:44:49 +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>,
        "Lin, Meng-Bo" <linmengbo0689@protonmail.com>,
        Markuss Broks <markuss.broks@gmail.com>
Subject: Re: [PATCH 12/13] arm64: dts: qcom: msm8916-samsung-j5: Add sound
 and modem
Message-ID: <ZRM0sRgw1j6IJi3Y@gerhold.net>
References: <20230926-msm8916-modem-v1-0-398eec74bac9@gerhold.net>
 <20230926-msm8916-modem-v1-12-398eec74bac9@gerhold.net>
 <9d0ad1cf-77d5-4704-9861-4eaae4cbec42@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9d0ad1cf-77d5-4704-9861-4eaae4cbec42@linaro.org>
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, Sep 26, 2023 at 09:04:22PM +0200, Konrad Dybcio wrote:
> On 26.09.2023 18:51, Stephan Gerhold wrote:
> > From: "Lin, Meng-Bo" <linmengbo0689@protonmail.com>
> > 
> > Enable sound and modem for the Samsung J5 smartphones. The setup is
> > similar to most MSM8916 devices, i.e.:
> > 
> >  - QDSP6 audio
> >  - Speaker/earpiece/headphones/microphones via digital/analog codec
> >    in MSM8916/PM8916
> >  - WWAN Internet via BAM-DMUX
> > 
> > except:
> > 
> >  - There is no secondary microphone, so a different "model" is used to
> >    differentiate that in the UCM configuration.
> >  - Samsung-specific audio jack detection (not supported yet)
> > 
> > Co-developed-by: Markuss Broks <markuss.broks@gmail.com>
> > Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> > Signed-off-by: "Lin, Meng-Bo" <linmengbo0689@protonmail.com>
> > [Stephan: Add consistent commit message]
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >  arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi | 15 +++++++++++++++
> >  arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts         |  4 ++++
> >  2 files changed, 19 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi b/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi
> > index fe59be3505fe..2caa820b0c26 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-j5-common.dtsi
> > @@ -1,6 +1,8 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> >  
> >  #include "msm8916-pm8916.dtsi"
> > +#include "msm8916-modem-qdsp6.dtsi"
> > +
> >  #include <dt-bindings/gpio/gpio.h>
> >  #include <dt-bindings/input/input.h>
> >  #include <dt-bindings/interrupt-controller/irq.h>
> > @@ -135,6 +137,10 @@ &blsp_uart2 {
> >  	status = "okay";
> >  };
> >  
> > +&mpss_mem {
> > +	reg = <0x0 0x86800000 0x0 0x5800000>;
> > +};
> > +
> >  &pm8916_resin {
> >  	status = "okay";
> >  	linux,code = <KEY_VOLUMEDOWN>;
> > @@ -154,6 +160,15 @@ &sdhc_2 {
> >  	cd-gpios = <&tlmm 38 GPIO_ACTIVE_LOW>;
> >  };
> >  
> > +&sound {
> > +	model = "msm8916-1mic";
> That's.. vague.. Is that intended?
> 

msm8916-modem-qdsp6.dtsi defines model = "msm8916" by default since we
have a standard UCM configuration that configures the typical audio
mixer setup when all outputs/inputs are connected to the digital/analog
codec of the SoC/PMIC. "msm8916-1mic" is exactly the same just with the
SecondaryMic disabled.

Unfortunately these names are effectively limited to 15 chars
(everything after will be cut off), so there is extremely limited
potential for more expressive names. :(

> I also noticed only now that random patches have status
> at random places in the property tree.. Removing the disablement
> would aid that as well (wink wink)!
> 

Hm, I think I could do the status = "okay" in msm8916-modem-qdsp6.dtsi.
I don't think it should be enabled by default in the SoC dtsi but there
it would probably be fine. :)

Thanks,
Stephan