Re: [PATCH v2 3/3] arm64: dts: qcom: sc7180: Add board id for lazor/limozeen

[Doug Anderson]

Hi,

On Sun, Aug 6, 2023 at 11:34 PM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 04/08/2023 11:58, Sheng-Liang Pan wrote:
> > add BRD_ID(0, Z, 0) = 10 for new board with ALC5682i-VS
> >
> > Signed-off-by: Sheng-Liang Pan <sheng-liang.pan@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >
> > Changes in v2:
> > - correct newly create dts files
> >
>
>
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r10.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r10.dts
> > new file mode 100644
> > index 000000000000..5a58e94c228e
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r10.dts
> > @@ -0,0 +1,30 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Google Lazor board device tree source
> > + *
> > + * Copyright 2023 Google LLC.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "sc7180-trogdor.dtsi"
> > +#include "sc7180-trogdor-parade-ps8640.dtsi"
> > +#include "sc7180-trogdor-lazor.dtsi"
> > +#include "sc7180-lite.dtsi"
> > +
> > +/ {
> > +     model = "Google Lazor (rev10+)";
> > +     compatible = "google,lazor", "qcom,sc7180";
> > +};
> > +
> > +&alc5682 {
> > +     compatible = "realtek,rt5682s";
> > +     /delete-property/ VBAT-supply;
>
> No, don't delete properties. First of all, why you do not have this
> supply here? I doubt it... Especially that this DTS has vbat-supply
> regulator!
>
> Second, define the properties where applicable instead.

It looks like v3 is out, but responding here since it looks like
Sheng-Liang didn't make any changes in v3 but also didn't respond and
explain why he didn't make any changes. Sheng-Liang: for future
reference you should make sure to address comments folks have on the
list. If your new version takes their feedback into account then
there's no reason to just respond with "Done", but if (like in this
case) you ignored feedback you need to say why.

In this case the extra "/delete-property/" is needed to pass bindings
checks. Specifically this revision of the board replaces the "rt5682i"
with the newer "rt5682s". This new codec is _almost_ a drop-in
replacement for the old codec with just a few tiny changes. One such
change is that the new codec doesn't need a "VBAT-supply".

Since most trogdor devices have the older "rt5682i" codec, the default
in "sc7180-trogdor.dtsi" specifies the properties for that codec. Only
the handful of boards that have been spun to use the new codec have an
override like this. You can see that the override done here matches
the one done in a few other trogdor boards. A good grep is:

git grep -A4 realtek,rt5682s -- arch/arm64/boot/dts/qcom/sc7180-*

Ironically, that grep finds that "sc7180-trogdor-pazquel360.dtsi" is
missing the "/delete-property/" which I'm fairly certain means that
it's giving a validation warning today.

I'm happy to have a bikeshed discussion about doing this better. In a
previous reply [1] I suggested that it's probably time to move the
"realtek,rt5682s" snippet to something like
"sc7180-trogdor-rt5682s-sku.dtsi". Then we could include it in the
devices and avoid duplicating this bit of dts. I didn't insist on it,
but if you feel strongly then maybe Sheng-Liang could add that to his
series? Once done, we could have further bikeshed discussions about
whether we should continue to use the "/delete-property/" solution or
if we have to also create a "sc7180-trogdor-rt5682i-sku.dtsi" and
force all older SKUs to include that. Personally I don't hate this
"/delete-property/" but I don't care a whole lot either way.

[1] https://lore.kernel.org/r/CAD=FV=XRq8ymnPrMPCa=c7PkSH+Kj9aG29_hCjCNSL3fY-qaGg@xxxxxxxxxxxxxx


-Doug