Re: [PATCH V2 RESEND 1/6] dt-bindings: clock: qcom: Add SM8650 video clock controller

[Jagadeesh Kona]

On 4/19/2024 2:31 AM, Vladimir Zapolskiy wrote:
> Hello Jagadeesh,
>
> On 3/25/24 08:07, Jagadeesh Kona wrote:
> > On 3/21/2024 6:42 PM, Dmitry Baryshkov wrote:
> > > On Thu, 21 Mar 2024 at 11:26, Jagadeesh Kona <quic_jkona@xxxxxxxxxxx> 
> > > wrote:
> > > > Extend device tree bindings of SM8450 videocc to add support
> > > > for SM8650 videocc. While it at, fix the incorrect header
> > > > include in sm8450 videocc yaml documentation.
> > > >
> > > > Signed-off-by: Jagadeesh Kona <quic_jkona@xxxxxxxxxxx>
> > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> > > > ---
> > > >    .../devicetree/bindings/clock/qcom,sm8450-videocc.yaml    | 4 +++-
> > > >    include/dt-bindings/clock/qcom,sm8450-videocc.h           | 8 
> > > > +++++++-
> > > >    2 files changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git 
> > > > a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml 
> > > > b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
> > > > index bad8f019a8d3..79f55620eb70 100644
> > > > --- a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
> > > > +++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
> > > > @@ -8,18 +8,20 @@ title: Qualcomm Video Clock & Reset Controller on 
> > > > SM8450
> > > >
> > > >    maintainers:
> > > >      - Taniya Das <quic_tdas@xxxxxxxxxxx>
> > > > +  - Jagadeesh Kona <quic_jkona@xxxxxxxxxxx>
> > > >
> > > >    description: |
> > > >      Qualcomm video clock control module provides the clocks, resets 
> > > > and power
> > > >      domains on SM8450.
> > > >
> > > > -  See also:: include/dt-bindings/clock/qcom,videocc-sm8450.h
> > > > +  See also:: include/dt-bindings/clock/qcom,sm8450-videocc.h
> > >
> > > This almost pleads to go to a separate patch. Fixes generally should
> > > be separated from the rest of the changes.
> >
> > Thanks Dmitry for your review.
> >
> > Sure, will separate this into a separate patch in next series.
> >
> > > >    properties:
> > > >      compatible:
> > > >        enum:
> > > >          - qcom,sm8450-videocc
> > > >          - qcom,sm8550-videocc
> > > > +      - qcom,sm8650-videocc
> > > >
> > > >      reg:
> > > >        maxItems: 1
> > > > diff --git a/include/dt-bindings/clock/qcom,sm8450-videocc.h 
> > > > b/include/dt-bindings/clock/qcom,sm8450-videocc.h
> > > > index 9d795adfe4eb..ecfebe52e4bb 100644
> > > > --- a/include/dt-bindings/clock/qcom,sm8450-videocc.h
> > > > +++ b/include/dt-bindings/clock/qcom,sm8450-videocc.h
> > > > @@ -1,6 +1,6 @@
> > > >    /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > > >    /*
> > > > - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights 
> > > > reserved.
> > > > + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All 
> > > > rights reserved.
> > > >     */
> > > >
> > > >    #ifndef _DT_BINDINGS_CLK_QCOM_VIDEO_CC_SM8450_H
> > > > @@ -19,6 +19,11 @@
> > > >    #define 
> > > > VIDEO_CC_MVS1C_DIV2_DIV_CLK_SRC                                9
> > > >    #define VIDEO_CC_PLL0                                          10
> > > >    #define VIDEO_CC_PLL1                                          11
> > > > +#define 
> > > > VIDEO_CC_MVS0_SHIFT_CLK                                        12
> > > > +#define VIDEO_CC_MVS0C_SHIFT_CLK                               13
> > > > +#define 
> > > > VIDEO_CC_MVS1_SHIFT_CLK                                        14
> > > > +#define VIDEO_CC_MVS1C_SHIFT_CLK                               15
> > > > +#define VIDEO_CC_XO_CLK_SRC                                    16
> > >
> > > Are these values applicable to sm8450?
> >
> > No, the shift clocks above are part of SM8650 only. To reuse the
> > existing SM8550 videocc driver for SM8650 and to register these shift
> > clocks for SM8650, I added them here.
>
> In such case I'd strongly suggest to add a new qcom,sm8650-videocc.h file,
> and do #include qcom,sm8450-videocc.h in it, thus the new header will be
> really a short one.
>
> This will add pristine clarity.

Thanks Vladimir for your suggestion. I believe adding a comment for 
these set of clocks should be sufficient to indicate these clocks are 
applicable only for SM8650, I can add the required comment and post the 
next series. Please let me know if this works?

Thanks,
Jagadeesh