diff mbox series

[V8,7/7] dt-bindings: bridge: samsung-dsim: Make some flags optional

Message ID 20230526030559.326566-8-aford173@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm: bridge: samsung-dsim: Support variable clocking | expand

Commit Message

Adam Ford May 26, 2023, 3:05 a.m. UTC
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(-)

Comments

Conor Dooley May 26, 2023, 6:19 p.m. UTC | #1
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,mipi-dsim.yaml b/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
> index 9f61ebdfefa8..360fea81f4b6 100644
> --- a/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.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 pixel
> +      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.
Or perhaps "must be used"? Ditto below.

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?

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
>
Adam Ford May 26, 2023, 7:24 p.m. UTC | #2
On Fri, May 26, 2023 at 1:19 PM Conor Dooley <conor@kernel.org> wrote:
>
> 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,mipi-dsim.yaml b/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
> > index 9f61ebdfefa8..360fea81f4b6 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.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 pixel
> > +      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
> >
Conor Dooley May 26, 2023, 7:30 p.m. UTC | #3
On Fri, May 26, 2023 at 02:24:21PM -0500, Adam Ford wrote:
> On Fri, May 26, 2023 at 1:19 PM Conor Dooley <conor@kernel.org> wrote:
> > On Thu, May 25, 2023 at 10:05:59PM -0500, Adam Ford wrote:

> > >      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 pixel
> > > +      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.

Okay, that seems fair. With your revised wording,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

> 
> 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.

Sounds good. Krzysztof will hopefully be able to take a look then too to
make sure I am not making a hames of things.

Thanks,
Conor.
Krzysztof Kozlowski May 30, 2023, 7:48 a.m. UTC | #4
On 26/05/2023 21:30, Conor Dooley wrote:
> On Fri, May 26, 2023 at 02:24:21PM -0500, Adam Ford wrote:
>> On Fri, May 26, 2023 at 1:19 PM Conor Dooley <conor@kernel.org> wrote:
>>> On Thu, May 25, 2023 at 10:05:59PM -0500, Adam Ford wrote:
> 
>>>>      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 pixel
>>>> +      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.
> 
> Okay, that seems fair. With your revised wording,
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
>>
>> 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.
> 
> Sounds good. Krzysztof will hopefully be able to take a look then too to
> make sure I am not making a hames of things.

We should avoid references to driver, because bindings are used also in
other projects where driver can behave differently. Also "driver" is
then ambiguous - which driver do you mean? Please re-phrase.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml b/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
index 9f61ebdfefa8..360fea81f4b6 100644
--- a/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.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 pixel
+      clock from the attached device or bridge.
 
   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#