| Message ID | 20230526030559.326566-8-aford173@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | drm: bridge: samsung-dsim: Support variable clocking | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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 >
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 > >
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.
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 --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#
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(-)