Re: [GIT PULL 2/5] dt-bindings: power: Add power-domain header for JH7110

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2736 bytes --]

On Tue, Sep 26, 2023 at 12:55:01PM +0200, Geert Uytterhoeven wrote:
> Hi Conor, Changhuang,
> 
> On Wed, Sep 13, 2023 at 3:58 PM Conor Dooley <conor@kernel.org> wrote:
> > From: Changhuang Liang <changhuang.liang@starfivetech.com>
> >
> > Add power-domain header for JH7110 SoC, it can use to operate dphy
> > power.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Thanks for your patch, which is now commit 41b66b54a72bd796
> ("dt-bindings: power: Add power-domain header for JH7110")
> in pmdomain/next.
> 
> Conor: looks like you forgot to update the patch description, as promised in
> https://lore.kernel.org/all/20230519-irk-dwelled-6a499c482e62@spud?

*sigh*, that's what happens when you apply the patches several months
after they were sent I guess. I'll try to make sure that that doesn't
happen again.

> > --- a/include/dt-bindings/power/starfive,jh7110-pmu.h
> > +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
> > @@ -1,6 +1,6 @@
> >  /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> >  /*
> > - * Copyright (C) 2022 StarFive Technology Co., Ltd.
> > + * Copyright (C) 2022-2023 StarFive Technology Co., Ltd.
> >   * Author: Walker Chen <walker.chen@starfivetech.com>
> >   */
> >  #ifndef __DT_BINDINGS_POWER_JH7110_POWER_H__
> > @@ -14,4 +14,7 @@
> >  #define JH7110_PD_ISP          5
> >  #define JH7110_PD_VENC         6
> >
> > +#define JH7110_PD_DPHY_TX      0
> > +#define JH7110_PD_DPHY_RX      1
> 
> These values are conflicting with similarly-named values above:
> 
>     #define JH7110_PD_SYSTOP        0
>     #define JH7110_PD_CPU           1
> 
> Upon closer look, and diving into the driver, this header file contains
> definitions for two separate number spaces: the first set is meant
> to be used in PM Domain specifiers referring to a node compatible
> with "starfive,jh7110-pmu", while the second set is meant to be
> used in PM Domain specifiers referring to a node compatible with
> "starfive,jh7110-aon-syscon".

Yeah, apologies for the confusion there. I maybe should have
requested an additional file, since the relevant compatibles
are different.

> I think it would be nice to have this reflected in the names of
> the defines (e.g. JH7110_PD_AON_DPHY_TX), and/or to add comments
> clearly delimiting the two separate number spaces, and explaining
> where they apply to.

Since this is 6.7 material, there's still plenty of time remaining to
make either of these changes. Changhuang Liang, do you think you would
be able to send a patch implementing either of the suggestions made by
Geert?

Thanks,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]