Re: [PATCH 14/24] arm64: dts: qcom: Update the pmh0110.dtsi for Glymur

[Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>]

Hi Krzysztof,



On Wed, Oct 08, 2025 at 08:25:51PM +0900, Krzysztof Kozlowski wrote:
> On 08/10/2025 17:02, Krzysztof Kozlowski wrote:
> > On 08/10/2025 16:31, Kamal Wadhwa wrote:
> >> Hi Krzysztof, Dmitry, Konrad,
> >>
> >> On Thu, Sep 25, 2025 at 09:57:02PM +0300, Dmitry Baryshkov wrote:
> >>> On Thu, Sep 25, 2025 at 10:34:52PM +0900, Krzysztof Kozlowski wrote:
> >>>> On Thu, 25 Sept 2025 at 22:14, Dmitry Baryshkov
> >>>> <dmitry.baryshkov@oss.qualcomm.com> wrote:
> >>>>>
> >>>>> On Thu, Sep 25, 2025 at 05:08:54PM +0900, Krzysztof Kozlowski wrote:
> >>>>>> On Thu, 25 Sept 2025 at 15:34, Pankaj Patil
> >>>>>> <pankaj.patil@oss.qualcomm.com> wrote:
> >>>>>>>
> >>>>>>> From: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> >>>>>>>
> >>>>>>> Add multiple instance of PMH0110 DT node, one for each assigned
> >>>>>>> SID for this PMIC on the spmi_bus0 and spmi_bus1 on the Glymur
> >>>>>>> CRD.
> >>>>>>>
> >>>>>>> Take care to avoid compilation issue with the existing nodes by
> >>>>>>> gaurding each PMH0110 nodes with `#ifdef` for its corresponding
> >>>>>>> SID macro. So that only the nodes which have the their SID macro
> >>>>>>> defined are the only ones picked for compilation.
> >>>>>>>
> >>>>>>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> >>>>>>> Signed-off-by: Pankaj Patil <pankaj.patil@oss.qualcomm.com>
> >>>>>>> ---
> >>>>>>>  arch/arm64/boot/dts/qcom/pmh0110.dtsi | 66 ++++++++++++++++++++++++++++++++++-
> >>>>>>>  1 file changed, 65 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/pmh0110.dtsi b/arch/arm64/boot/dts/qcom/pmh0110.dtsi
> >>>>>>> index b99c33cba8860f1852231db33a127646c08c1e23..4a5c66e5c9fbc35cedb67601f4568844dc41fbea 100644
> >>>>>>> --- a/arch/arm64/boot/dts/qcom/pmh0110.dtsi
> >>>>>>> +++ b/arch/arm64/boot/dts/qcom/pmh0110.dtsi
> >>>>>>> @@ -7,6 +7,8 @@
> >>>>>>>  #include <dt-bindings/spmi/spmi.h>
> >>>>>>>
> >>>>>>>  &spmi_bus0 {
> >>>>>>> +
> >>>>>>> +#ifdef PMH0110_D_E0_SID
> >>>>>>
> >>>>>> NAK
> >>>>>>
> >>>>>> I already explained on IRC in great details why.
> >>>>>
> >>>>> A short summary or a link to a channel / date would be nice in order to
> >>>>> include other people into the discussion.
> >>>>>
> >>>>
> >>>> Of course but:
> >>>> 1. You were there so maybe you remember the arguments, and:
> >>>> 2. I'm offline, using phone, not having laptop, replying during my
> >>>> personal time off just before merge window so any emergency time
> >>>> should be spent on important matters instead these two huge patch
> >>>> bombs adding such usage I already said: NO, don't do this.
> >>>
> >>>
> >>> Well, If I'm asking, it means I don't rememebr the discussion. And I
> >>> defeinitely didn't know that you are spending your personal vacation
> >>> time in ML. And if the discussion was with some other people, then
> >>> somebody else can drop the response to the question.
> >>
> >> Just wanted to give some background on this patch.
> >> Even though PMH0104 and PMH0110 are common (b/w Kaanapali and Glymur),
> >> they don't share the SIDs. So we tried to use status="disabled" to handle
> >> this but we observed that because of the node name being common in the
> >> two included files, it ends up overwriting the previous node with the
> >> same name.
> >>
> >> eg-
> >> #include "pmh0104.dtsi"  // assume contains pmic@4 { ...};
> >> #include "pmh0110.dtsi"  // assume contains pmic@4 { status=disabled;};
> >>
> >> Here intention was to use the pmh0104 on sid-4, but it gets overwritten
> >> with the pmh0110 on sid-4 ( with status disabled). This is why we ended
> >> up using the `#ifdef`, ensuring that we can control the exact pmic that
> >> gets picked by using the PMXXX_SID macro.
> >>
> >> side note, i did `grep` in the `/arch/arm64/boot/dts/` and i see a lot
> >> of instances of `#if...` present in that.  Assuming the concern here is
> >> about the use of `#ifdef`.
> > 
> > 
> > #if are not desired in C code, so why would they be acceptable in DTS?
> > It is not making the code easier to read at all.
> > 
> > On IRC in these older discussions I was very strongly against any DTSI
> > which depends on some sort of outside values, except basic usage of
> > defines. Original pmh0110.dtsi from kaanapali is fine:
> > 	pmh0110_d_e0: pmic@PMH0110_D_E0_SID {
> > 
> > but doing ifdefs here that this define depends on something else makes
> > code ungreppable (lookup unit address from sysfs and then git grep
> > pmic@4) and difficult to follow.
> > 
> > My recommendation is either duplicate code or change DTSI files to not
> > contain entire node, but its contents. At least these are
> > recommendations I remember now.
> > 
> One more recommendation:
> Different DTSI files per SoC, so pmh0110-kaanapali.dtsi and
> pmh0110-glymur.dtsi.

Thanks for this suggestion.

- So for the common pmics between the Kaanapali and Glymur, we are going to
  postfix the soc name. 
- Files pmk8550.dtsi, pmh0101.dtsi which are exactly same between kaanapali
  and glymur we will continue to use the same name.

This is how the #includes will look (after dropping #ifdefs)

+#include "pmk8850.dtsi"        /* SPMI0: SID-0                         */
+#include "pmh0101.dtsi"        /* SPMI0: SID-1                         */
+#include "pmcx0102.dtsi"       /* SPMI0: SID-2/3/4/6  SPMI1: SID-2/3/4 */
+#include "pmh0110-glymur.dtsi" /* SPMI0: SID-5/7      SPMI1: SID-5     */
+#include "pmh0104-glymur.dtsi" /* SPMI0: SID-8/9      SPMI1: SID-11    */
+#include "smb2370.dtsi"        /* SPMI2: SID-9/10/11                   */

(hope this is alright now?)

Regards,
Kamal