The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to select input pixel data sampling edge. Add DT property "pixelclk-active", same as the one used by display timings, and configure bus flags based on this DT property.
Signed-off-by: Marek Vasut marex@denx.de Cc: Alexandre Torgue alexandre.torgue@st.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Antonio Borneo antonio.borneo@st.com Cc: Benjamin Gaignard benjamin.gaignard@st.com Cc: Biju Das biju.das.jz@bp.renesas.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maxime Coquelin mcoquelin.stm32@gmail.com Cc: Philippe Cornu philippe.cornu@st.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Vincent Abriou vincent.abriou@st.com Cc: Yannick Fertre yannick.fertre@st.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-stm32@st-md-mailman.stormreply.com To: dri-devel@lists.freedesktop.org --- V2: - Limit the pixelclk-active to encoders only - Update DT binding document --- .../bindings/display/bridge/lvds-codec.yaml | 7 +++ drivers/gpu/drm/bridge/lvds-codec.c | 52 +++++++++++++------ 2 files changed, 44 insertions(+), 15 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml index e5e3c72630cf..399a6528780a 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -74,6 +74,13 @@ properties:
additionalProperties: false
+ pixelclk-active: + description: | + Data sampling on rising or falling edge. + Use 0 to sample pixel data on rising edge and + Use 1 to sample pixel data on falling edge and + enum: [0, 1] + powerdown-gpios: description: The GPIO used to control the power down line of this device. diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index dcf579a4cf83..cab81ccd895d 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -15,13 +15,18 @@ #include <drm/drm_bridge.h> #include <drm/drm_panel.h>
+struct lvds_codec_data { + u32 connector_type; + bool is_encoder; +}; + 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; };
static inline struct lvds_codec *to_lvds_codec(struct drm_bridge *bridge) @@ -76,17 +81,20 @@ static const struct drm_bridge_funcs funcs = {
static int lvds_codec_probe(struct platform_device *pdev) { + const struct lvds_codec_data *data; struct device *dev = &pdev->dev; struct device_node *panel_node; struct drm_panel *panel; struct lvds_codec *lvds_codec; + u32 val;
lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); if (!lvds_codec) return -ENOMEM;
+ data = of_device_get_match_data(dev); + lvds_codec->dev = &pdev->dev; - lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
lvds_codec->vcc = devm_regulator_get(lvds_codec->dev, "power"); if (IS_ERR(lvds_codec->vcc)) @@ -115,10 +123,22 @@ static int lvds_codec_probe(struct platform_device *pdev)
lvds_codec->panel_bridge = devm_drm_panel_bridge_add_typed(dev, panel, - lvds_codec->connector_type); + data->connector_type); if (IS_ERR(lvds_codec->panel_bridge)) return PTR_ERR(lvds_codec->panel_bridge);
+ /* + * 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 (data->is_encoder && + !of_property_read_u32(dev->of_node, "pixelclk-active", &val)) { + lvds_codec->timings.input_bus_flags = val ? + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE : + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE; + } + /* * 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 @@ -126,6 +146,7 @@ static int lvds_codec_probe(struct platform_device *pdev) */ lvds_codec->bridge.of_node = dev->of_node; lvds_codec->bridge.funcs = &funcs; + lvds_codec->bridge.timings = &lvds_codec->timings; drm_bridge_add(&lvds_codec->bridge);
platform_set_drvdata(pdev, lvds_codec); @@ -142,19 +163,20 @@ static int lvds_codec_remove(struct platform_device *pdev) return 0; }
+static const struct lvds_codec_data decoder_data = { + .connector_type = DRM_MODE_CONNECTOR_DPI, + .is_encoder = false, +}; + +static const struct lvds_codec_data encoder_data = { + .connector_type = DRM_MODE_CONNECTOR_LVDS, + .is_encoder = true, +}; + static const struct of_device_id lvds_codec_match[] = { - { - .compatible = "lvds-decoder", - .data = (void *)DRM_MODE_CONNECTOR_DPI, - }, - { - .compatible = "lvds-encoder", - .data = (void *)DRM_MODE_CONNECTOR_LVDS, - }, - { - .compatible = "thine,thc63lvdm83d", - .data = (void *)DRM_MODE_CONNECTOR_LVDS, - }, + { .compatible = "lvds-decoder", .data = &decoder_data, }, + { .compatible = "lvds-encoder", .data = &encoder_data, }, + { .compatible = "thine,thc63lvdm83d", .data = &encoder_data, }, {}, }; MODULE_DEVICE_TABLE(of, lvds_codec_match);
On 12/24/20 7:18 AM, 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 "pixelclk-active", same as the one used by display timings, and configure bus flags based on this DT property.
Bump ?
On 1/24/21 6:04 PM, Marek Vasut wrote:
On 12/24/20 7:18 AM, 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 "pixelclk-active", same as the one used by display timings, and configure bus flags based on this DT property.
Bump ?
Any news on this patch ?
Hi Marek,
All my apologies for the awfully delayed review, and thank you for pinging me.
On Thu, Dec 24, 2020 at 07:18:32AM +0100, 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 "pixelclk-active", same as the one used by display timings, and configure bus flags based on this DT property.
Signed-off-by: Marek Vasut marex@denx.de Cc: Alexandre Torgue alexandre.torgue@st.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Antonio Borneo antonio.borneo@st.com Cc: Benjamin Gaignard benjamin.gaignard@st.com Cc: Biju Das biju.das.jz@bp.renesas.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maxime Coquelin mcoquelin.stm32@gmail.com Cc: Philippe Cornu philippe.cornu@st.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Vincent Abriou vincent.abriou@st.com Cc: Yannick Fertre yannick.fertre@st.com Cc: linux-arm-kernel@lists.infradead.org Cc: linux-stm32@st-md-mailman.stormreply.com To: dri-devel@lists.freedesktop.org
V2: - Limit the pixelclk-active to encoders only - Update DT binding document
.../bindings/display/bridge/lvds-codec.yaml | 7 +++ drivers/gpu/drm/bridge/lvds-codec.c | 52 +++++++++++++------ 2 files changed, 44 insertions(+), 15 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml index e5e3c72630cf..399a6528780a 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -74,6 +74,13 @@ properties:
additionalProperties: false
- pixelclk-active:
- description: |
Data sampling on rising or falling edge.Use 0 to sample pixel data on rising edge andUse 1 to sample pixel data on falling edge and- enum: [0, 1]
The idea is good, but instead of adding a custom property, how about reusing the pclk-sample property defined in ../../media/video-interfaces.yaml ?
The property is only valid for encoders, so I would at least mention that in the description, or, better, handle this based on the compatible string to allow validation.
- powerdown-gpios: description: The GPIO used to control the power down line of this device.
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index dcf579a4cf83..cab81ccd895d 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -15,13 +15,18 @@ #include <drm/drm_bridge.h> #include <drm/drm_panel.h>
+struct lvds_codec_data {
- u32 connector_type;
- bool is_encoder;
+};
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;
};
static inline struct lvds_codec *to_lvds_codec(struct drm_bridge *bridge) @@ -76,17 +81,20 @@ static const struct drm_bridge_funcs funcs = {
static int lvds_codec_probe(struct platform_device *pdev) {
const struct lvds_codec_data *data; struct device *dev = &pdev->dev; struct device_node *panel_node; struct drm_panel *panel; struct lvds_codec *lvds_codec;
u32 val;
lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); if (!lvds_codec) return -ENOMEM;
data = of_device_get_match_data(dev);
lvds_codec->dev = &pdev->dev;
lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
lvds_codec->vcc = devm_regulator_get(lvds_codec->dev, "power"); if (IS_ERR(lvds_codec->vcc))
@@ -115,10 +123,22 @@ static int lvds_codec_probe(struct platform_device *pdev)
lvds_codec->panel_bridge = devm_drm_panel_bridge_add_typed(dev, panel,
lvds_codec->connector_type);
data->connector_type);if (IS_ERR(lvds_codec->panel_bridge)) return PTR_ERR(lvds_codec->panel_bridge);
/*
* 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 (data->is_encoder &&
!of_property_read_u32(dev->of_node, "pixelclk-active", &val)) {lvds_codec->timings.input_bus_flags = val ?DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE :DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE;}
/*
- 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
@@ -126,6 +146,7 @@ static int lvds_codec_probe(struct platform_device *pdev) */ lvds_codec->bridge.of_node = dev->of_node; lvds_codec->bridge.funcs = &funcs;
lvds_codec->bridge.timings = &lvds_codec->timings; drm_bridge_add(&lvds_codec->bridge);
platform_set_drvdata(pdev, lvds_codec);
@@ -142,19 +163,20 @@ static int lvds_codec_remove(struct platform_device *pdev) return 0; }
+static const struct lvds_codec_data decoder_data = {
- .connector_type = DRM_MODE_CONNECTOR_DPI,
- .is_encoder = false,
The two fields are a bit redundant, as the decoder is always LVDS-to-DPI, and the encoder DPI-to-LVDS. I don't mind too much, but maybe we could drop the connector_type field, and derive the connector type from is_encoder ?
One may then say that we could drop the lvds_codec_data structure as it contains a single field, but I foresee a need to have device-specific timings at some point, so I think it's a good addition.
The patch otherwise looks good.
+};
+static const struct lvds_codec_data encoder_data = {
- .connector_type = DRM_MODE_CONNECTOR_LVDS,
- .is_encoder = true,
+};
static const struct of_device_id lvds_codec_match[] = {
- {
.compatible = "lvds-decoder",.data = (void *)DRM_MODE_CONNECTOR_DPI,- },
- {
.compatible = "lvds-encoder",.data = (void *)DRM_MODE_CONNECTOR_LVDS,- },
- {
.compatible = "thine,thc63lvdm83d",.data = (void *)DRM_MODE_CONNECTOR_LVDS,- },
- { .compatible = "lvds-decoder", .data = &decoder_data, },
- { .compatible = "lvds-encoder", .data = &encoder_data, },
- { .compatible = "thine,thc63lvdm83d", .data = &encoder_data, }, {},
}; MODULE_DEVICE_TABLE(of, lvds_codec_match);
On 3/22/21 2:14 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 e5e3c72630cf..399a6528780a 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -74,6 +74,13 @@ properties:
additionalProperties: false
- pixelclk-active:
- description: |
Data sampling on rising or falling edge.Use 0 to sample pixel data on rising edge andUse 1 to sample pixel data on falling edge and- enum: [0, 1]
The idea is good, but instead of adding a custom property, how about reusing the pclk-sample property defined in ../../media/video-interfaces.yaml ?
Repeating myself from V1 discussion ... Either is fine by me, but I think pixelclk-active, which comes from panel-timings.yaml is closer to the video than multimedia bindings.
The property is only valid for encoders, so I would at least mention that in the description, or, better, handle this based on the compatible string to allow validation.
How does that work in the Yaml file ?
- powerdown-gpios: description: The GPIO used to control the power down line of this device.
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index dcf579a4cf83..cab81ccd895d 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c
[...]
@@ -126,6 +146,7 @@ static int lvds_codec_probe(struct platform_device *pdev) */ lvds_codec->bridge.of_node = dev->of_node; lvds_codec->bridge.funcs = &funcs;
lvds_codec->bridge.timings = &lvds_codec->timings; drm_bridge_add(&lvds_codec->bridge);
platform_set_drvdata(pdev, lvds_codec);
@@ -142,19 +163,20 @@ static int lvds_codec_remove(struct platform_device *pdev) return 0; }
+static const struct lvds_codec_data decoder_data = {
- .connector_type = DRM_MODE_CONNECTOR_DPI,
- .is_encoder = false,
The two fields are a bit redundant, as the decoder is always LVDS-to-DPI, and the encoder DPI-to-LVDS. I don't mind too much, but maybe we could drop the connector_type field, and derive the connector type from is_encoder ?
Or the other way around instead ? That is, if the connector_type is LVDS, then it is encoder , otherwise its decoder ?
One may then say that we could drop the lvds_codec_data structure as it contains a single field, but I foresee a need to have device-specific timings at some point, so I think it's a good addition.
[...]
Hi Marek,
(CC'ing Ron and the DT mailing list for the DT discussion)
On Mon, Mar 22, 2021 at 11:29:04AM +0100, Marek Vasut wrote:
On 3/22/21 2:14 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 e5e3c72630cf..399a6528780a 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -74,6 +74,13 @@ properties:
additionalProperties: false
- pixelclk-active:
- description: |
Data sampling on rising or falling edge.Use 0 to sample pixel data on rising edge andUse 1 to sample pixel data on falling edge and- enum: [0, 1]
The idea is good, but instead of adding a custom property, how about reusing the pclk-sample property defined in ../../media/video-interfaces.yaml ?
Repeating myself from V1 discussion ... Either is fine by me, but I think pixelclk-active, which comes from panel-timings.yaml is closer to the video than multimedia bindings.
That's a good point. The part that bothers me a bit is that it would be nice to define the property in a single YAML schema, referenced by individual bindings. video-interfaces.yaml is there for that purpose. We could do something similar on the display side, or consider the pixelclk-active usage in panel-timings.yaml an exception that we can't switch to video-interfaces.yaml as backward compatibility must be preserved.
I don't have a too strong preference, whatever Rob prefers would be fine with me.
The property is only valid for encoders, so I would at least mention that in the description, or, better, handle this based on the compatible string to allow validation.
How does that work in the Yaml file ?
Something along the lines of
if: not: properties: compatible: contains: const: lvds-encoder then: properties: pixelclk-active: false
My YAML schema foo is a bit rusty though, I apologize if this doesn't work as-is. There are lots of similar examples in DT bindings that should hopefully be right :-)
- powerdown-gpios: description: The GPIO used to control the power down line of this device.
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index dcf579a4cf83..cab81ccd895d 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c
[...]
@@ -126,6 +146,7 @@ static int lvds_codec_probe(struct platform_device *pdev) */ lvds_codec->bridge.of_node = dev->of_node; lvds_codec->bridge.funcs = &funcs;
lvds_codec->bridge.timings = &lvds_codec->timings; drm_bridge_add(&lvds_codec->bridge);
platform_set_drvdata(pdev, lvds_codec);
@@ -142,19 +163,20 @@ static int lvds_codec_remove(struct platform_device *pdev) return 0; }
+static const struct lvds_codec_data decoder_data = {
- .connector_type = DRM_MODE_CONNECTOR_DPI,
- .is_encoder = false,
The two fields are a bit redundant, as the decoder is always LVDS-to-DPI, and the encoder DPI-to-LVDS. I don't mind too much, but maybe we could drop the connector_type field, and derive the connector type from is_encoder ?
Or the other way around instead ? That is, if the connector_type is LVDS, then it is encoder , otherwise its decoder ?
Either way works for me.
One may then say that we could drop the lvds_codec_data structure as it contains a single field, but I foresee a need to have device-specific timings at some point, so I think it's a good addition.
[...]
Hi Rob,
There's a question for you below.
On Mon, Mar 22, 2021 at 12:37:47PM +0200, Laurent Pinchart wrote:
Hi Marek,
(CC'ing Ron and the DT mailing list for the DT discussion)
On Mon, Mar 22, 2021 at 11:29:04AM +0100, Marek Vasut wrote:
On 3/22/21 2:14 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 e5e3c72630cf..399a6528780a 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -74,6 +74,13 @@ properties:
additionalProperties: false
- pixelclk-active:
- description: |
Data sampling on rising or falling edge.Use 0 to sample pixel data on rising edge andUse 1 to sample pixel data on falling edge and- enum: [0, 1]
The idea is good, but instead of adding a custom property, how about reusing the pclk-sample property defined in ../../media/video-interfaces.yaml ?
Repeating myself from V1 discussion ... Either is fine by me, but I think pixelclk-active, which comes from panel-timings.yaml is closer to the video than multimedia bindings.
That's a good point. The part that bothers me a bit is that it would be nice to define the property in a single YAML schema, referenced by individual bindings. video-interfaces.yaml is there for that purpose. We could do something similar on the display side, or consider the pixelclk-active usage in panel-timings.yaml an exception that we can't switch to video-interfaces.yaml as backward compatibility must be preserved.
I don't have a too strong preference, whatever Rob prefers would be fine with me.
The property is only valid for encoders, so I would at least mention that in the description, or, better, handle this based on the compatible string to allow validation.
How does that work in the Yaml file ?
Something along the lines of
if: not: properties: compatible: contains: const: lvds-encoder then: properties: pixelclk-active: false
My YAML schema foo is a bit rusty though, I apologize if this doesn't work as-is. There are lots of similar examples in DT bindings that should hopefully be right :-)
- powerdown-gpios: description: The GPIO used to control the power down line of this device.
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index dcf579a4cf83..cab81ccd895d 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c
[...]
@@ -126,6 +146,7 @@ static int lvds_codec_probe(struct platform_device *pdev) */ lvds_codec->bridge.of_node = dev->of_node; lvds_codec->bridge.funcs = &funcs;
lvds_codec->bridge.timings = &lvds_codec->timings; drm_bridge_add(&lvds_codec->bridge);
platform_set_drvdata(pdev, lvds_codec);
@@ -142,19 +163,20 @@ static int lvds_codec_remove(struct platform_device *pdev) return 0; }
+static const struct lvds_codec_data decoder_data = {
- .connector_type = DRM_MODE_CONNECTOR_DPI,
- .is_encoder = false,
The two fields are a bit redundant, as the decoder is always LVDS-to-DPI, and the encoder DPI-to-LVDS. I don't mind too much, but maybe we could drop the connector_type field, and derive the connector type from is_encoder ?
Or the other way around instead ? That is, if the connector_type is LVDS, then it is encoder , otherwise its decoder ?
Either way works for me.
One may then say that we could drop the lvds_codec_data structure as it contains a single field, but I foresee a need to have device-specific timings at some point, so I think it's a good addition.
[...]
On 3/22/21 11:29 AM, Marek Vasut wrote:
On 3/22/21 2:14 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 e5e3c72630cf..399a6528780a 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -74,6 +74,13 @@ properties: additionalProperties: false + pixelclk-active: + description: | + Data sampling on rising or falling edge. + Use 0 to sample pixel data on rising edge and + Use 1 to sample pixel data on falling edge and + enum: [0, 1]
The idea is good, but instead of adding a custom property, how about reusing the pclk-sample property defined in ../../media/video-interfaces.yaml ?
Repeating myself from V1 discussion ... Either is fine by me, but I think pixelclk-active, which comes from panel-timings.yaml is closer to the video than multimedia bindings.
Was there ever any reply from Rob on this ?
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Marek Vasut