[PATCH v5 1/2] dt-bindings: display: bridge: lvds-codec: Document pixel data sampling edge select
The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to select input pixel data sampling edge. Add DT property "pclk-sample", not the same as the one used by display timings but rather the same as used by media, to define the pixel data sampling edge.
Signed-off-by: Marek Vasut marex@denx.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Rob Herring robh+dt@kernel.org Cc: Sam Ravnborg sam@ravnborg.org Cc: devicetree@vger.kernel.org To: dri-devel@lists.freedesktop.org --- V4: New patch split from combined V3 V5: Rebase on recent linux-next --- .../bindings/display/bridge/lvds-codec.yaml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml index 1faae3e323a4..708de84ac138 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -79,6 +79,14 @@ properties: - port@0 - port@1
+ pclk-sample: + description: + Data sampling on rising or falling edge. + enum: + - 0 # Falling edge + - 1 # Rising edge + default: 0 + powerdown-gpios: description: The GPIO used to control the power down line of this device. @@ -102,6 +110,16 @@ then: properties: data-mapping: false
+if: + not: + properties: + compatible: + contains: + const: lvds-encoder +then: + properties: + pclk-sample: false + required: - compatible - ports
The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to select input pixel data sampling edge. Add DT property "pclk-sample", not the same as the one used by display timings but rather the same as used by media, and configure bus flags based on this DT property.
Signed-off-by: Marek Vasut marex@denx.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Rob Herring robh+dt@kernel.org Cc: Sam Ravnborg sam@ravnborg.org Cc: devicetree@vger.kernel.org To: dri-devel@lists.freedesktop.org --- V2: - Limit the pixelclk-active to encoders only - Update DT binding document V3: - Determine whether this is encoder from connector, i.e. lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS V4: - Switch to pclk-sample. Note that the value of this is inverted, so all the existing users of pixelclk-active using previous previous version of this patch must be reworked V5: Rebase on recent linux-next --- drivers/gpu/drm/bridge/lvds-codec.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index f991842a161f..702ea803a743 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -21,6 +21,7 @@ struct lvds_codec { struct device *dev; struct drm_bridge bridge; struct drm_bridge *panel_bridge; + struct drm_bridge_timings timings; struct regulator *vcc; struct gpio_desc *powerdown_gpio; u32 connector_type; @@ -119,6 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev) struct device_node *bus_node; struct drm_panel *panel; struct lvds_codec *lvds_codec; + u32 val; int ret;
lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); @@ -187,12 +189,25 @@ static int lvds_codec_probe(struct platform_device *pdev) } }
+ /* + * Encoder might sample data on different clock edge than the display, + * for example OnSemi FIN3385 has a dedicated strapping pin to select + * the sampling edge. + */ + if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS && + !of_property_read_u32(dev->of_node, "pclk-sample", &val)) { + lvds_codec->timings.input_bus_flags = val ? + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE : + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; + } + /* * The panel_bridge bridge is attached to the panel's of_node, * but we need a bridge attached to our of_node for our user * to look up. */ lvds_codec->bridge.of_node = dev->of_node; + lvds_codec->bridge.timings = &lvds_codec->timings; drm_bridge_add(&lvds_codec->bridge);
platform_set_drvdata(pdev, lvds_codec);
Hi Marek,
On Sun, Oct 17, 2021 at 02:12:04AM +0200, Marek Vasut wrote:
The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to select input pixel data sampling edge. Add DT property "pclk-sample", not the same as the one used by display timings but rather the same as used by media, and configure bus flags based on this DT property.
Late to the party here - sorry.
Signed-off-by: Marek Vasut marex@denx.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Rob Herring robh+dt@kernel.org Cc: Sam Ravnborg sam@ravnborg.org Cc: devicetree@vger.kernel.org To: dri-devel@lists.freedesktop.org
V2: - Limit the pixelclk-active to encoders only - Update DT binding document V3: - Determine whether this is encoder from connector, i.e. lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS V4: - Switch to pclk-sample. Note that the value of this is inverted, so all the existing users of pixelclk-active using previous previous version of this patch must be reworked V5: Rebase on recent linux-next
drivers/gpu/drm/bridge/lvds-codec.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index f991842a161f..702ea803a743 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -21,6 +21,7 @@ struct lvds_codec { struct device *dev; struct drm_bridge bridge; struct drm_bridge *panel_bridge;
- struct drm_bridge_timings timings; struct regulator *vcc; struct gpio_desc *powerdown_gpio; u32 connector_type;
@@ -119,6 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev) struct device_node *bus_node; struct drm_panel *panel; struct lvds_codec *lvds_codec;
u32 val; int ret;
lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
@@ -187,12 +189,25 @@ static int lvds_codec_probe(struct platform_device *pdev) } }
- /*
* Encoder might sample data on different clock edge than the display,* for example OnSemi FIN3385 has a dedicated strapping pin to select* the sampling edge.*/- if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS &&
!of_property_read_u32(dev->of_node, "pclk-sample", &val)) {lvds_codec->timings.input_bus_flags = val ?DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE :DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;- }
- /*
*/ lvds_codec->bridge.of_node = dev->of_node;
- The panel_bridge bridge is attached to the panel's of_node,
- but we need a bridge attached to our of_node for our user
- to look up.
- lvds_codec->bridge.timings = &lvds_codec->timings;
I do not understand how this will work. The only field that is set is timings.input_bus_flags but any user will see bridge.timings is set and will think this is all timing info.
Maybe I just misses something obvious?
Sam
drm_bridge_add(&lvds_codec->bridge);
platform_set_drvdata(pdev, lvds_codec);
2.33.0
On 10/17/21 6:49 PM, Sam Ravnborg wrote:
[...]
- /*
* Encoder might sample data on different clock edge than the display,* for example OnSemi FIN3385 has a dedicated strapping pin to select* the sampling edge.*/- if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS &&
!of_property_read_u32(dev->of_node, "pclk-sample", &val)) {lvds_codec->timings.input_bus_flags = val ?DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE :DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;- }
- /*
*/ lvds_codec->bridge.of_node = dev->of_node;
- The panel_bridge bridge is attached to the panel's of_node,
- but we need a bridge attached to our of_node for our user
- to look up.
- lvds_codec->bridge.timings = &lvds_codec->timings;
I do not understand how this will work. The only field that is set is timings.input_bus_flags but any user will see bridge.timings is set and will think this is all timing info.
Maybe I just misses something obvious?
Is there anything else in those timings that should be set ? See include/drm/drm_bridge.h around line 640
setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, so it is 0 or false anyway, i.e. no change.
Hi Marek,
On Sun, Oct 17, 2021 at 07:29:51PM +0200, Marek Vasut wrote:
On 10/17/21 6:49 PM, Sam Ravnborg wrote:
[...]
- /*
* Encoder might sample data on different clock edge than the display,* for example OnSemi FIN3385 has a dedicated strapping pin to select* the sampling edge.*/- if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS &&
!of_property_read_u32(dev->of_node, "pclk-sample", &val)) {lvds_codec->timings.input_bus_flags = val ?DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE :DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;- }
- /*
*/ lvds_codec->bridge.of_node = dev->of_node;
- The panel_bridge bridge is attached to the panel's of_node,
- but we need a bridge attached to our of_node for our user
- to look up.
- lvds_codec->bridge.timings = &lvds_codec->timings;
I do not understand how this will work. The only field that is set is timings.input_bus_flags but any user will see bridge.timings is set and will think this is all timing info.
Maybe I just misses something obvious?
Is there anything else in those timings that should be set ? See include/drm/drm_bridge.h around line 640
setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, so it is 0 or false anyway, i.e. no change.
Just me being confused with display_timings. Patch looks good. Reviewed-by: Sam Ravnborg sam@ravnborg.org
Ping me in a few days to apply it if there is no more feedback.
Sam
On 10/17/21 7:40 PM, Sam Ravnborg wrote:
Hi Marek,
On Sun, Oct 17, 2021 at 07:29:51PM +0200, Marek Vasut wrote:
On 10/17/21 6:49 PM, Sam Ravnborg wrote:
[...]
- /*
* Encoder might sample data on different clock edge than the display,* for example OnSemi FIN3385 has a dedicated strapping pin to select* the sampling edge.*/- if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS &&
!of_property_read_u32(dev->of_node, "pclk-sample", &val)) {lvds_codec->timings.input_bus_flags = val ?DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE :DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;- }
- /*
*/ lvds_codec->bridge.of_node = dev->of_node;
- The panel_bridge bridge is attached to the panel's of_node,
- but we need a bridge attached to our of_node for our user
- to look up.
- lvds_codec->bridge.timings = &lvds_codec->timings;
I do not understand how this will work. The only field that is set is timings.input_bus_flags but any user will see bridge.timings is set and will think this is all timing info.
Maybe I just misses something obvious?
Is there anything else in those timings that should be set ? See include/drm/drm_bridge.h around line 640
setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, so it is 0 or false anyway, i.e. no change.
Just me being confused with display_timings. Patch looks good. Reviewed-by: Sam Ravnborg sam@ravnborg.org
Ping me in a few days to apply it if there is no more feedback.
ACK
On 10/17/21 7:40 PM, Sam Ravnborg wrote:
Hi Marek,
Hi,
On Sun, Oct 17, 2021 at 07:29:51PM +0200, Marek Vasut wrote:
On 10/17/21 6:49 PM, Sam Ravnborg wrote:
[...]
- /*
* Encoder might sample data on different clock edge than the display,* for example OnSemi FIN3385 has a dedicated strapping pin to select* the sampling edge.*/- if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS &&
!of_property_read_u32(dev->of_node, "pclk-sample", &val)) {lvds_codec->timings.input_bus_flags = val ?DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE :DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;- }
- /*
*/ lvds_codec->bridge.of_node = dev->of_node;
- The panel_bridge bridge is attached to the panel's of_node,
- but we need a bridge attached to our of_node for our user
- to look up.
- lvds_codec->bridge.timings = &lvds_codec->timings;
I do not understand how this will work. The only field that is set is timings.input_bus_flags but any user will see bridge.timings is set and will think this is all timing info.
Maybe I just misses something obvious?
Is there anything else in those timings that should be set ? See include/drm/drm_bridge.h around line 640
setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, so it is 0 or false anyway, i.e. no change.
Just me being confused with display_timings. Patch looks good. Reviewed-by: Sam Ravnborg sam@ravnborg.org
Ping me in a few days to apply it if there is no more feedback.
Ping I guess ... Laurent ?
On 10/24/21 1:04 AM, Marek Vasut wrote:
On 10/17/21 7:40 PM, Sam Ravnborg wrote:
Hi Marek,
Hi,
On Sun, Oct 17, 2021 at 07:29:51PM +0200, Marek Vasut wrote:
On 10/17/21 6:49 PM, Sam Ravnborg wrote:
[...]
+ /* + * Encoder might sample data on different clock edge than the display, + * for example OnSemi FIN3385 has a dedicated strapping pin to select + * the sampling edge. + */ + if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS && + !of_property_read_u32(dev->of_node, "pclk-sample", &val)) { + lvds_codec->timings.input_bus_flags = val ? + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE : + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; + }
/* * The panel_bridge bridge is attached to the panel's of_node, * but we need a bridge attached to our of_node for our user * to look up. */ lvds_codec->bridge.of_node = dev->of_node; + lvds_codec->bridge.timings = &lvds_codec->timings;
I do not understand how this will work. The only field that is set is timings.input_bus_flags but any user will see bridge.timings is set and will think this is all timing info.
Maybe I just misses something obvious?
Is there anything else in those timings that should be set ? See include/drm/drm_bridge.h around line 640
setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, so it is 0 or false anyway, i.e. no change.
Just me being confused with display_timings. Patch looks good. Reviewed-by: Sam Ravnborg sam@ravnborg.org
Ping me in a few days to apply it if there is no more feedback.
Ping I guess ... Laurent ?
Ping one more time ?
On 11/24/21 04:02, Marek Vasut wrote:
On 10/24/21 1:04 AM, Marek Vasut wrote:
On 10/17/21 7:40 PM, Sam Ravnborg wrote:
Hi Marek,
Hi,
On Sun, Oct 17, 2021 at 07:29:51PM +0200, Marek Vasut wrote:
On 10/17/21 6:49 PM, Sam Ravnborg wrote:
[...]
+ /* + * Encoder might sample data on different clock edge than the display, + * for example OnSemi FIN3385 has a dedicated strapping pin to select + * the sampling edge. + */ + if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS && + !of_property_read_u32(dev->of_node, "pclk-sample", &val)) { + lvds_codec->timings.input_bus_flags = val ? + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE : + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; + }
/* * The panel_bridge bridge is attached to the panel's of_node, * but we need a bridge attached to our of_node for our user * to look up. */ lvds_codec->bridge.of_node = dev->of_node; + lvds_codec->bridge.timings = &lvds_codec->timings;
I do not understand how this will work. The only field that is set is timings.input_bus_flags but any user will see bridge.timings is set and will think this is all timing info.
Maybe I just misses something obvious?
Is there anything else in those timings that should be set ? See include/drm/drm_bridge.h around line 640
setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, so it is 0 or false anyway, i.e. no change.
Just me being confused with display_timings. Patch looks good. Reviewed-by: Sam Ravnborg sam@ravnborg.org
Ping me in a few days to apply it if there is no more feedback.
Ping I guess ... Laurent ?
Ping one more time ?
Ping yet again ?
On Sun, 17 Oct 2021 02:12:03 +0200, Marek Vasut wrote:
The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to select input pixel data sampling edge. Add DT property "pclk-sample", not the same as the one used by display timings but rather the same as used by media, to define the pixel data sampling edge.
Signed-off-by: Marek Vasut marex@denx.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Rob Herring robh+dt@kernel.org Cc: Sam Ravnborg sam@ravnborg.org Cc: devicetree@vger.kernel.org To: dri-devel@lists.freedesktop.org
V4: New patch split from combined V3 V5: Rebase on recent linux-next
.../bindings/display/bridge/lvds-codec.yaml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
Reviewed-by: Rob Herring robh@kernel.org
Hi Marek,
Thank you for the patch.
On Sun, Oct 17, 2021 at 02:12:03AM +0200, Marek Vasut wrote:
The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to select input pixel data sampling edge. Add DT property "pclk-sample", not the same as the one used by display timings but rather the same as used by media, to define the pixel data sampling edge.
Signed-off-by: Marek Vasut marex@denx.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Rob Herring robh+dt@kernel.org Cc: Sam Ravnborg sam@ravnborg.org Cc: devicetree@vger.kernel.org To: dri-devel@lists.freedesktop.org
V4: New patch split from combined V3 V5: Rebase on recent linux-next
.../bindings/display/bridge/lvds-codec.yaml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml index 1faae3e323a4..708de84ac138 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -79,6 +79,14 @@ properties: - port@0 - port@1
- pclk-sample:
- description:
Data sampling on rising or falling edge.- enum:
- 0 # Falling edge- 1 # Rising edge- default: 0
Shouldn't this be moved to the endpoint, the same way data-mapping is defined as an endpoint property ?
powerdown-gpios: description: The GPIO used to control the power down line of this device. @@ -102,6 +110,16 @@ then: properties: data-mapping: false
+if:
- not:
- properties:
compatible:contains:const: lvds-encoder+then:
- properties:
- pclk-sample: false
required:
- compatible
- ports
On 10/18/21 8:08 PM, Laurent Pinchart wrote:
[...]
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml index 1faae3e323a4..708de84ac138 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -79,6 +79,14 @@ properties: - port@0 - port@1
- pclk-sample:
- description:
Data sampling on rising or falling edge.- enum:
- 0 # Falling edge- 1 # Rising edge- default: 0
Shouldn't this be moved to the endpoint, the same way data-mapping is defined as an endpoint property ?
The strapping is a chip property, not port property, so no.
Hi Marek,
On Mon, Oct 18, 2021 at 09:47:13PM +0200, Marek Vasut wrote:
On 10/18/21 8:08 PM, Laurent Pinchart wrote:
[...]
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml index 1faae3e323a4..708de84ac138 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -79,6 +79,14 @@ properties: - port@0 - port@1
- pclk-sample:
- description:
Data sampling on rising or falling edge.- enum:
- 0 # Falling edge- 1 # Rising edge- default: 0
Shouldn't this be moved to the endpoint, the same way data-mapping is defined as an endpoint property ?
The strapping is a chip property, not port property, so no.
For this particular chip that's true. I'm still not convinced overall. For some cases it could be a per-port property, and moving it there for lvds-codec too could allow implementing helpers to parse DT properties, without much drawback for this particular use case as far as I can see. It's hard to predict the future with certainty of course, so I won't insist.
On 10/18/21 9:57 PM, Laurent Pinchart wrote:
Hi,
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml index 1faae3e323a4..708de84ac138 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -79,6 +79,14 @@ properties: - port@0 - port@1
- pclk-sample:
- description:
Data sampling on rising or falling edge.- enum:
- 0 # Falling edge- 1 # Rising edge- default: 0
Shouldn't this be moved to the endpoint, the same way data-mapping is defined as an endpoint property ?
The strapping is a chip property, not port property, so no.
For this particular chip that's true. I'm still not convinced overall. For some cases it could be a per-port property
Can you be more specific about "some cases" ?
, and moving it there for lvds-codec too could allow implementing helpers to parse DT properties, without much drawback for this particular use case as far as I can see. It's hard to predict the future with certainty of course, so I won't insist.
The DT bindings and the OS drivers are separate thing, we really shouldn't start bending DT bindings so that they would fit nicely with a specific OS driver model.
Hi Marek,
On Tue, Oct 19, 2021 at 12:18:11AM +0200, Marek Vasut wrote:
On 10/18/21 9:57 PM, Laurent Pinchart wrote:
Hi,
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml index 1faae3e323a4..708de84ac138 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -79,6 +79,14 @@ properties: - port@0 - port@1
- pclk-sample:
- description:
Data sampling on rising or falling edge.- enum:
- 0 # Falling edge- 1 # Rising edge- default: 0
Shouldn't this be moved to the endpoint, the same way data-mapping is defined as an endpoint property ?
The strapping is a chip property, not port property, so no.
For this particular chip that's true. I'm still not convinced overall. For some cases it could be a per-port property
Can you be more specific about "some cases" ?
I'm thinking about bridges that could have multiple parallel inputs.
, and moving it there for lvds-codec too could allow implementing helpers to parse DT properties, without much drawback for this particular use case as far as I can see. It's hard to predict the future with certainty of course, so I won't insist.
The DT bindings and the OS drivers are separate thing, we really shouldn't start bending DT bindings so that they would fit nicely with a specific OS driver model.
DT bindings are not holy beings that live in a mythical heaven way above the mere mortal drivers, they would be useless without implementations. It's not about bending them, which I regularly push against during review, but about structuring them in a way that facilitates implementations when all other things are equal.
As I said, despite wondering whether or not it would be better to move the property to the endpoint (and that was a genuine open question), I won't insist in this case.
On 10/19/21 8:49 AM, Laurent Pinchart wrote:
Hi Marek,
Hi,
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml index 1faae3e323a4..708de84ac138 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -79,6 +79,14 @@ properties: - port@0 - port@1
- pclk-sample:
- description:
Data sampling on rising or falling edge.- enum:
- 0 # Falling edge- 1 # Rising edge- default: 0
Shouldn't this be moved to the endpoint, the same way data-mapping is defined as an endpoint property ?
The strapping is a chip property, not port property, so no.
For this particular chip that's true. I'm still not convinced overall. For some cases it could be a per-port property
Can you be more specific about "some cases" ?
I'm thinking about bridges that could have multiple parallel inputs.
Can you draft an example how such a binding would look like within the confines of this lvds-codec.yaml ?
I also have to wonder how such a hypothetical device would work, would it serialize two parallel bussed into single LVDS one ?
, and moving it there for lvds-codec too could allow implementing helpers to parse DT properties, without much drawback for this particular use case as far as I can see. It's hard to predict the future with certainty of course, so I won't insist.
The DT bindings and the OS drivers are separate thing, we really shouldn't start bending DT bindings so that they would fit nicely with a specific OS driver model.
DT bindings are not holy beings that live in a mythical heaven way above the mere mortal drivers, they would be useless without implementations. It's not about bending them, which I regularly push against during review, but about structuring them in a way that facilitates implementations when all other things are equal.
Note that the pclk-sample isn't a property of the input, but of the chip, I don't think it is a good idea to say they are equal and conflate them like so.
As I said, despite wondering whether or not it would be better to move the property to the endpoint (and that was a genuine open question), I won't insist in this case.
[...]
On Tue, Oct 19, 2021 at 04:39:05PM +0200, Marek Vasut wrote:
On 10/19/21 8:49 AM, Laurent Pinchart wrote:
Hi Marek,
Hi,
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > index 1faae3e323a4..708de84ac138 100644 > --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > @@ -79,6 +79,14 @@ properties: > - port@0 > - port@1 > > + pclk-sample: > + description: > + Data sampling on rising or falling edge. > + enum: > + - 0 # Falling edge > + - 1 # Rising edge > + default: 0 > +
Shouldn't this be moved to the endpoint, the same way data-mapping is defined as an endpoint property ?
The strapping is a chip property, not port property, so no.
For this particular chip that's true. I'm still not convinced overall. For some cases it could be a per-port property
Can you be more specific about "some cases" ?
I'm thinking about bridges that could have multiple parallel inputs.
Can you draft an example how such a binding would look like within the confines of this lvds-codec.yaml ?
I also have to wonder how such a hypothetical device would work, would it serialize two parallel bussed into single LVDS one ?
Such a device would require custom bindings I think, as lvds-codec is limited to a single input and a single output. thine,thc63lvd1024.yaml is an example of such a device.
, and moving it there for lvds-codec too could allow implementing helpers to parse DT properties, without much drawback for this particular use case as far as I can see. It's hard to predict the future with certainty of course, so I won't insist.
The DT bindings and the OS drivers are separate thing, we really shouldn't start bending DT bindings so that they would fit nicely with a specific OS driver model.
DT bindings are not holy beings that live in a mythical heaven way above the mere mortal drivers, they would be useless without implementations. It's not about bending them, which I regularly push against during review, but about structuring them in a way that facilitates implementations when all other things are equal.
Note that the pclk-sample isn't a property of the input, but of the chip, I don't think it is a good idea to say they are equal and conflate them like so.
With a chip that has a single input, that's always the case :-)
Anyway, I don't mind a chip-level property for this binding as we're limited to a single port. If other devices need to specify this at the port level, I'm sure we'll be able to cope with the lack of uniformity.
As I said, despite wondering whether or not it would be better to move the property to the endpoint (and that was a genuine open question), I won't insist in this case.
[...]
On 10/27/21 1:43 AM, Laurent Pinchart wrote:
[...]
>> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml >> index 1faae3e323a4..708de84ac138 100644 >> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml >> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml >> @@ -79,6 +79,14 @@ properties: >> - port@0 >> - port@1 >> >> + pclk-sample: >> + description: >> + Data sampling on rising or falling edge. >> + enum: >> + - 0 # Falling edge >> + - 1 # Rising edge >> + default: 0 >> + > > Shouldn't this be moved to the endpoint, the same way data-mapping is > defined as an endpoint property ?
The strapping is a chip property, not port property, so no.
For this particular chip that's true. I'm still not convinced overall. For some cases it could be a per-port property
Can you be more specific about "some cases" ?
I'm thinking about bridges that could have multiple parallel inputs.
Can you draft an example how such a binding would look like within the confines of this lvds-codec.yaml ?
I also have to wonder how such a hypothetical device would work, would it serialize two parallel bussed into single LVDS one ?
Such a device would require custom bindings I think, as lvds-codec is limited to a single input and a single output. thine,thc63lvd1024.yaml is an example of such a device.
It seems THC63LVD1024 is LVDS->to->Parallel DPI, so pclk-sample does not seem applicable there either.
[...]
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Marek Vasut -
Rob Herring -
Sam Ravnborg