Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230526030559.326566-1-aford173@gmail.com> <20230526030559.326566-8-aford173@gmail.com>
 <20230526-cabana-humble-81e44944e378@spud>
In-Reply-To: <20230526-cabana-humble-81e44944e378@spud>
From:   Adam Ford <aford173@gmail.com>
Date:   Fri, 26 May 2023 14:24:21 -0500
Message-ID: <CAHCN7xJdfO4+q071sur7wpVg+gU_Fzw9zfXvM9NDBFCN=Axiog@mail.gmail.com>
Subject: Re: [PATCH V8 7/7] dt-bindings: bridge: samsung-dsim: Make some flags optional
To:     Conor Dooley <conor@kernel.org>
Cc:     dri-devel@lists.freedesktop.org, aford@beaconembedded.com,
        Inki Dae <inki.dae@samsung.com>,
        Jagan Teki <jagan@amarulasolutions.com>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Andrzej Hajda <andrzej.hajda@intel.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Robert Foss <rfoss@kernel.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Frieder Schrempf <frieder.schrempf@kontron.de>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, May 26, 2023 at 1:19=E2=80=AFPM Conor Dooley <conor@kernel.org> wro=
te:
>
> Adam, Neil,
>
> I meant to get to this earlier today, but broken CI got in the way...
>
> On Thu, May 25, 2023 at 10:05:59PM -0500, Adam Ford wrote:
> > In the event a device is connected to the samsung-dsim
> > controller that doesn't support the burst-clock, the
> > driver is able to get the requested pixel clock from the
> > attached device or bridge.  In these instances, the
> > samsung,burst-clock-frequency isn't needed, so remove
> > it from the required list.
> >
> > The pll-clock frequency can be set by the device tree entry
> > for samsung,pll-clock-frequency, but in some cases, the
> > pll-clock may have the same clock rate as sclk_mipi clock.
> > If they are equal, this flag is not needed since the driver
> > will use the sclk_mipi rate as a fallback.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> >  .../bindings/display/bridge/samsung,mipi-dsim.yaml       | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/samsung,m=
ipi-dsim.yaml b/Documentation/devicetree/bindings/display/bridge/samsung,mi=
pi-dsim.yaml
> > index 9f61ebdfefa8..360fea81f4b6 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsi=
m.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsi=
m.yaml
> > @@ -70,7 +70,9 @@ properties:
> >    samsung,burst-clock-frequency:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >      description:
> > -      DSIM high speed burst mode frequency.
> > +      DSIM high speed burst mode frequency when connected to devices
> > +      that support burst mode. If absent, the driver will use the pixe=
l
> > +      clock from the attached device or bridge.
>
> I'd rather this description did not say anything about drivers.
> How about:
>         If absent, the pixel clock from the attached device or bridge
>         will be used instead.

That makes sense.  I can do that.

"DSIM high speed burst mode frequency (optional). If absent, the pixel
clock from the attached device or bridge will be used instead."

> Or perhaps "must be used"? Ditto below.

"Must be" implies to me that the user needs to set something.  Are you
ok with the proposed suggestion above?
>
> Description aside, the removal seems to be backwards compatible - but
> can every device that this binding supports work using an "attached
> device or bridge", or are these properties going to be required for
> certain compatibles?

From what I can tell, the assumption is that the DSIM driver was
expecting it to attach to panels in the past.  With the additional
patch series, the DSIM can attach to bridge parts without a hard-coded
set of clocks.  I don't expect the existing Exynos devices to change,
but I also don't know what would preclude those SoC's from attaching
to a bridge should someone want to design a new product around them.

I'll wait a couple days for more feedback and send patch V2 with just
this patch since the rest of the series has been applied to the drm
branch.

adam

>
> Thanks,
> Conor.
>
> >
> >    samsung,esc-clock-frequency:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> > @@ -80,7 +82,8 @@ properties:
> >    samsung,pll-clock-frequency:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >      description:
> > -      DSIM oscillator clock frequency.
> > +      DSIM oscillator clock frequency. If absent, the driver will
> > +      use the clock frequency of sclk_mipi.
> >
> >    phys:
> >      maxItems: 1
> > @@ -134,9 +137,7 @@ required:
> >    - compatible
> >    - interrupts
> >    - reg
> > -  - samsung,burst-clock-frequency
> >    - samsung,esc-clock-frequency
> > -  - samsung,pll-clock-frequency
> >
> >  allOf:
> >    - $ref: ../dsi-controller.yaml#
> > --
> > 2.39.2
> >