Re: [PATCH 03/13] arm64: dts: qcom: msm8916: Add common msm8916-modem-qdsp6.dtsi

[Konrad Dybcio]

On 9/26/23 22:17, Stephan Gerhold wrote:
> On Tue, Sep 26, 2023 at 10:01:21PM +0200, Konrad Dybcio wrote:
> > On 26.09.2023 21:06, Stephan Gerhold wrote:
> > > 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@xxxxxxxxxxx>
> > > > > ---
> > > > 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.
> > I see..
> >
> > Well, I guess I don't have better counter-arguments, but please
> > consider this the next time around.
>
> Will do!
>
> > [...]
> >
> > > > > +&lpass_codec {
> > > > > +	status = "okay";
> > > > > +};
> > > > Any reason for it to stay disabled?
> > >
> > > You mean in msm8916.dtsi?
> > Yes
> >
> > > 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).
> > See my reply to patch 5
> >
> > [...]
>
> Let's continue discussing that there I guess. :D
>
> > > > > +	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.
> > Ahh right, this is the one we don't reference.. Too bad,
> > would be a nice fit :/
> >
> > I only see one usage of it though (patch 7), perhaps it could
> > be kept local to that one?
>
> This patch series just contains the initial set of
> msm8916-modem-qdsp6.dtsi users (for devices which are already upstream).
> We probably have like 20 more that still need to be upstreamed. :D
>
> sound_dai_secondary is fairly rare, but there is at least one more user
> that will probably end up upstream soon.
2 users don't sound particularly great in a devicetree included by 20 
other non-users

> I think the overhead of these template notes is absolutely negligible
> compared to all the (potentially) unused SoC nodes we have. :D
Yes, however the unused SoC nodes are mostly standardized and could be 
used as-they-are on a vast majority of devices

Konrad