Re: [v1] dt-bindings: msm:disp: update dsi and dpu bindings

[harigovi]

On 2020-02-04 23:52, Doug Anderson wrote:
> Hi,
>
> On Tue, Feb 4, 2020 at 6:15 AM Harigovindan P <harigovi@xxxxxxxxxxxxxx> 
> wrote:
> > Updating bindings of dsi and dpu by adding and removing certain
> > properties.
> >
> > Signed-off-by: Harigovindan P <harigovi@xxxxxxxxxxxxxx>
> > ---
> >
> > Changes in v1:
> >         - Adding "ahb" clock as a required property.
> >         - Adding "bus", "rot", "lut" as optional properties for sc7180 
> > device.
> >         - Removing properties from dsi bindings that are unused.
> >         - Removing power-domain property since DSI is the child node 
> > of MDSS
> >           and it will inherit supply from its parent.
> >
> >  Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++
> >  Documentation/devicetree/bindings/display/msm/dsi.txt | 5 -----
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt 
> > b/Documentation/devicetree/bindings/display/msm/dpu.txt
> > index 551ae26..dd58472a 100644
> > --- a/Documentation/devicetree/bindings/display/msm/dpu.txt
> > +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
> > @@ -19,6 +19,7 @@ Required properties:
> >    The following clocks are required:
> >    * "iface"
> >    * "bus"
> > +  * "ahb"
>
> This is only required for sc7180?  ...or old SoCs should have had it
> all along too?
>
>
> >    * "core"
> >  - interrupts: interrupt signal from MDSS.
> >  - interrupt-controller: identifies the node as an interrupt 
> > controller.
> > @@ -50,6 +51,8 @@ Required properties:
> >  - clock-names: device clock names, must be in same order as clocks 
> > property.
> >    The following clocks are required.
> >    * "bus"
> > +  For the device "qcom,sc7180-dpu":
> > +  * "bus" - is an optional property due to architecture change.
>
> This is a really odd way to write it for two reasons:
> * You're breaking up the flow of the list.
> * This shouldn't be listed as "optional" in sc7180 but unless there is
> some reason to ever provide it on sc7180.  It should simply be
> disallowed.
>
> Maybe instead just:
>
>    The following clocks are required.
> -  * "bus"
> +  * "bus" (anything other than qcom,sc7180-dpu)
>
> We really need to get this into yaml ASAP but that'd probably be OK to
> tide us over.
>
> NOTE: when converting to yaml, ideally we'll have a separate file per
> SoC to avoid crazy spaghetti, see commit 2a8aa18c1131 ("dt-bindings:
> clk: qcom: Fix self-validation, split, and clean cruft") in clk-next
> for an example of starting the transition to one yaml per SoC (at
> least for anything majorly different).
>
>
> >    * "iface"
> >    * "core"
> >    * "vsync"
> > @@ -70,6 +73,10 @@ Optional properties:
> >  - assigned-clocks: list of clock specifiers for clocks needing rate 
> > assignment
> >  - assigned-clock-rates: list of clock frequencies sorted in the same 
> > order as
> >    the assigned-clocks property.
> > +- For the device "qcom,sc7180-dpu":
> > +  clock-names: optional device clocks, needed for accessing LUT 
> > blocks.
> > +  * "rot"
> > +  * "lut"
> >
> >  Example:
> >
> > diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt 
> > b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > index af95586..61d659a 100644
> > --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> > +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > @@ -8,13 +8,10 @@ Required properties:
> >  - reg-names: The names of register regions. The following regions are 
> > required:
> >    * "dsi_ctrl"
> >  - interrupts: The interrupt signal from the DSI block.
> > -- power-domains: Should be <&mmcc MDSS_GDSC>.
>
> Is this supposed to be removed from all SoCs using this bindings, or
> just yours?
>
> I'll also note that you left it in the "Example:" below.
>
>
> >  - clocks: Phandles to device clocks.
> >  - clock-names: the following clocks are required:
> > -  * "mdp_core"
> >    * "iface"
> >    * "bus"
> > -  * "core_mmss"
>
> As Jeffrey pointed out, you shouldn't be removing these from old SoCs.
> In "drivers/gpu/drm/msm/dsi/dsi_cfg.c" you can clearly see them used.
> Maybe it's time for you to do the yaml conversion and handle this
> correctly per-SoC.
>
> -Doug


Hi,

yaml files have been created and new patchset has been created for that.
https://patchwork.kernel.org/patch/11423653/