dt-bindings: display: bridge: lvds-codec: Document LVDS data mapping select
Subsystems (4)
PATCH 1/2 dt-bindings: display: bridge: lvds-codec: Document LVDS data mapping select
Decoder input LVDS format is a property of the decoder chip or even its strapping. Add DT property data-mapping the same way lvds-panel does, to define the LVDS data mapping.
--- .../bindings/display/bridge/lvds-codec.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +19 -0
@@ -64,6 +64,15 @@ properties: - port@0 - port@1 + data-mapping: + enum: + - jeida-18 + - jeida-24 + - vesa-24 + description: | + The color signals mapping order. See details in + Documentation/devicetree/bindings/display/panel/lvds.yaml + pclk-sample: description: Data sampling on rising or falling edge.
@@ -79,6 +88,16 @@ properties: power-supply: true +if: + not: + properties: + compatible: + contains: + const: lvds-decoder +then: + properties: + data-mapping: false + if: not: properties:
Signature · 1 line
2.30.2
PATCH 2/2 drm/bridge: lvds-codec: Add support for LVDS data mapping select
Decoder input LVDS format is a property of the decoder chip or even its strapping. Handle data-mapping the same way lvds-panel does. In case data-mapping is not present, do nothing, since there are still legacy bindings which do not specify this property.
--- drivers/gpu/drm/bridge/lvds-codec.c | 50 +++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)
drivers/gpu/drm/bridge/lvds-codec.c +50 -0
@@ -23,6 +23,7 @@ struct lvds_codec {struct regulator *vcc;struct gpio_desc *powerdown_gpio;u32 connector_type; +unsigned int bus_format; };static inline struct lvds_codec *to_lvds_codec (struct drm_bridge *bridge)
@@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge) "Failed to disable regulator \"vcc\": %d\n", ret); } +static bool lvds_codec_mode_fixup (struct drm_bridge *bridge, +const struct drm_display_mode *mode, +struct drm_display_mode *adj) +{ +struct lvds_codec *lvds_codec =to_lvds_codec (bridge); +struct drm_encoder *encoder = bridge->encoder; +struct drm_device *ddev = encoder->dev; +struct drm_connector *connector; + +/* If 'data-mapping' was not specified, do nothing. */ +if (!lvds_codec->bus_format) +return true; + +/* Patch in the LVDS format */ +list_for_each_entry (connector, &ddev->mode_config.connector_list, head) { +drm_display_info_set_bus_formats (&connector->display_info, + &lvds_codec->bus_format,1 ); + } + +return true; +} +static const struct drm_bridge_funcs funcs = { .attach = lvds_codec_attach, .enable = lvds_codec_enable, .disable = lvds_codec_disable, + .mode_fixup = lvds_codec_mode_fixup, };static int lvds_codec_probe (struct platform_device *pdev)
@@ -81,6 +105,7 @@ static int lvds_codec_probe(struct platform_device *pdev)struct device_node *panel_node;struct drm_panel *panel;struct lvds_codec *lvds_codec; +const char *mapping;u32 val; lvds_codec =devm_kzalloc (dev,sizeof (*lvds_codec),GFP_KERNEL );
@@ -133,6 +158,31 @@ static int lvds_codec_probe(struct platform_device *pdev)DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE ; } +/* +* Decoder input LVDS format is a property of the decoder chip or even +* its strapping. Handle data-mapping the same way lvds-panel does. In +* case data-mapping is not present, do nothing, since there are still +* legacy bindings which do not specify this property. +*/ +if (lvds_codec->connector_type !=DRM_MODE_CONNECTOR_LVDS ) { + ret =of_property_read_string (dev->of_node,"data-mapping" , + &mapping); +if (ret <0 ) { +dev_err (dev,"missing 'data-mapping' DT property\n" ); + }else { +if (!strcmp (mapping,"jeida-18" )) { + lvds_codec->bus_format =MEDIA_BUS_FMT_RGB666_1X7X3_SPWG ; + }else if (!strcmp (mapping,"jeida-24" )) { + lvds_codec->bus_format =MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA ; + }else if (!strcmp (mapping,"vesa-24" )) { + lvds_codec->bus_format =MEDIA_BUS_FMT_RGB888_1X7X4_SPWG ; + }else { +dev_err (dev,"invalid 'data-mapping' DT property\n" ); +return -EINVAL ; + } + } + } +/* * 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
Signature · 1 line
2.30.2
Laurent Pinchart
reviewerRe: [PATCH 2/2] drm/bridge: lvds-codec: Add support for LVDS data mapping select
Hi Marek,
Thank you for the patch.
Quoted 37 lines · Marek Vasut
Decoder input LVDS format is a property of the decoder chip or even its strapping. Handle data-mapping the same way lvds-panel does. In case data-mapping is not present, do nothing, since there are still legacy bindings which do not specify this property.
Signed-off-by Marek Vasut <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Sam Ravnborg <[email protected]>--- drivers/gpu/drm/bridge/lvds-codec.c | 50 +++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)drivers/gpu/drm/bridge/lvds-codec.c +9
-0@@ -23,6 +23,7 @@ struct lvds_codec {struct regulator *vcc;struct gpio_desc *powerdown_gpio;u32 connector_type; +unsigned int bus_format; };static inline struct lvds_codec *to_lvds_codec (struct drm_bridge *bridge)@@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge) "Failed to disable regulator \"vcc\": %d\n", ret); } +static bool lvds_codec_mode_fixup (struct drm_bridge *bridge, +const struct drm_display_mode *mode, +struct drm_display_mode *adj) +{ +struct lvds_codec *lvds_codec =to_lvds_codec (bridge); +struct drm_encoder *encoder = bridge->encoder; +struct drm_device *ddev = encoder->dev; +struct drm_connector *connector;
drivers/gpu/drm/bridge/lvds-codec.c static void lvds_codec_disable(struct drm_bridge *bridge) · line 90
Quoted 10 lines
drivers/gpu/drm/bridge/lvds-codec.c +10
-0+ +/* If 'data-mapping' was not specified, do nothing. */ +if (!lvds_codec->bus_format) +return true; + +/* Patch in the LVDS format */ +list_for_each_entry (connector, &ddev->mode_config.connector_list, head) { +drm_display_info_set_bus_formats (&connector->display_info, + &lvds_codec->bus_format,1 ); + }
This part bothers me, as the format at the input of the LVDS decoder doesn't match the format on the connector. Shouldn't you implement .atomic_get_output_bus_fmts() instead ?
Quoted 13 lines
drivers/gpu/drm/bridge/lvds-codec.c +5
-0+ +return true; +} +static const struct drm_bridge_funcs funcs = { .attach = lvds_codec_attach, .enable = lvds_codec_enable, .disable = lvds_codec_disable, + .mode_fixup = lvds_codec_mode_fixup, };static int lvds_codec_probe (struct platform_device *pdev)
drivers/gpu/drm/bridge/lvds-codec.c static int lvds_codec_probe(struct platform_device *pdev) · line 108
Quoted 4 lines
drivers/gpu/drm/bridge/lvds-codec.c +1
-0@@ -81,6 +105,7 @@ static int lvds_codec_probe(struct platform_device *pdev)struct device_node *panel_node;struct drm_panel *panel;struct lvds_codec *lvds_codec; +const char *mapping;
I would have moved this variable to the if () { ... } below, but maybe that's just me.
Quoted 35 lines
drivers/gpu/drm/bridge/lvds-codec.c +25
-0u32 val; lvds_codec =devm_kzalloc (dev,sizeof (*lvds_codec),GFP_KERNEL );@@ -133,6 +158,31 @@ static int lvds_codec_probe(struct platform_device *pdev)DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE ; } +/* +* Decoder input LVDS format is a property of the decoder chip or even +* its strapping. Handle data-mapping the same way lvds-panel does. In +* case data-mapping is not present, do nothing, since there are still +* legacy bindings which do not specify this property. +*/ +if (lvds_codec->connector_type !=DRM_MODE_CONNECTOR_LVDS ) { + ret =of_property_read_string (dev->of_node,"data-mapping" , + &mapping); +if (ret <0 ) { +dev_err (dev,"missing 'data-mapping' DT property\n" ); + }else { +if (!strcmp (mapping,"jeida-18" )) { + lvds_codec->bus_format =MEDIA_BUS_FMT_RGB666_1X7X3_SPWG ; + }else if (!strcmp (mapping,"jeida-24" )) { + lvds_codec->bus_format =MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA ; + }else if (!strcmp (mapping,"vesa-24" )) { + lvds_codec->bus_format =MEDIA_BUS_FMT_RGB888_1X7X4_SPWG ; + }else { +dev_err (dev,"invalid 'data-mapping' DT property\n" ); +return -EINVAL ; + } + } + } +/* * 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
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH 2/2] drm/bridge: lvds-codec: Add support for LVDS data mapping select
On 5/18/21 1:03 AM, Laurent Pinchart wrote:
Hi,
[...]
Quoted 18 lines
Quoted 18 lines
drivers/gpu/drm/bridge/lvds-codec.c +14
-0@@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge) "Failed to disable regulator \"vcc\": %d\n", ret); } +static bool lvds_codec_mode_fixup (struct drm_bridge *bridge, +const struct drm_display_mode *mode, +struct drm_display_mode *adj) +{ +struct lvds_codec *lvds_codec =to_lvds_codec (bridge); +struct drm_encoder *encoder = bridge->encoder; +struct drm_device *ddev = encoder->dev; +struct drm_connector *connector; + +/* If 'data-mapping' was not specified, do nothing. */ +if (!lvds_codec->bus_format) +return true; + +/* Patch in the LVDS format */
Quoted 8 lines
drivers/gpu/drm/bridge/lvds-codec.c static void lvds_codec_disable(struct drm_bridge *bridge) · line 90
Quoted 4 lines
drivers/gpu/drm/bridge/lvds-codec.c +4
-0+list_for_each_entry (connector, &ddev->mode_config.connector_list, head) { +drm_display_info_set_bus_formats (&connector->display_info, + &lvds_codec->bus_format,1 ); + }This part bothers me, as the format at the input of the LVDS decoder doesn't match the format on the connector. Shouldn't you implement .atomic_get_output_bus_fmts() instead ?
No, I tried that option before this solution and that didn't work. The atomic stuff seems to be separate from what I need to pass here, i.e. without this, e.g. the mxsfb scanout engine still receives the wrong format.
[...]
Laurent Pinchart
reviewerRe: [PATCH 2/2] drm/bridge: lvds-codec: Add support for LVDS data mapping select
Hi Marek,
Quoted 33 lines · Marek Vasut
On 5/18/21 1:03 AM, Laurent Pinchart wrote:
Hi,
[...]
Quoted 18 lines
Quoted 18 lines
drivers/gpu/drm/bridge/lvds-codec.c +14
-0@@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge) "Failed to disable regulator \"vcc\": %d\n", ret); } +static bool lvds_codec_mode_fixup (struct drm_bridge *bridge, +const struct drm_display_mode *mode, +struct drm_display_mode *adj) +{ +struct lvds_codec *lvds_codec =to_lvds_codec (bridge); +struct drm_encoder *encoder = bridge->encoder; +struct drm_device *ddev = encoder->dev; +struct drm_connector *connector; + +/* If 'data-mapping' was not specified, do nothing. */ +if (!lvds_codec->bus_format) +return true; + +/* Patch in the LVDS format */ Quoted 8 lines
drivers/gpu/drm/bridge/lvds-codec.c static void lvds_codec_disable(struct drm_bridge *bridge) · line 90
Quoted 4 lines
drivers/gpu/drm/bridge/lvds-codec.c +4
-0+list_for_each_entry (connector, &ddev->mode_config.connector_list, head) { +drm_display_info_set_bus_formats (&connector->display_info, + &lvds_codec->bus_format,1 ); + }This part bothers me, as the format at the input of the LVDS decoder doesn't match the format on the connector. Shouldn't you implement .atomic_get_output_bus_fmts() instead ?
Quoted 3 lines
No, I tried that option before this solution and that didn't work. The atomic stuff seems to be separate from what I need to pass here, i.e. without this, e.g. the mxsfb scanout engine still receives the wrong format.
I fear this needs to be investigated then, as the connected format isn't the same as the LVDS decoder input format, so the above isn't right.
Signature · 3 lines
Regards, Laurent Pinchart
Laurent Pinchart
reviewerHi Marek,
Thank you for the patch.
Quoted 3 lines · Marek Vasut
Decoder input LVDS format is a property of the decoder chip or even its strapping. Add DT property data-mapping the same way lvds-panel does, to define the LVDS data mapping.
The information could also be derived by the driver from the compatible string in the case when this is an intrinsic property of the device (or when it's configurable by software), but the fact that it can also be controlled by strapping makes a DT property needed. We may want to limit the usage of the DT property to the strapping case though, but I don't have a real preference here, so I'm fine with this approach.
Quoted 37 lines
Signed-off-by Marek Vasut <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Rob Herring <[email protected]>Cc Sam Ravnborg <[email protected]>--- .../bindings/display/bridge/lvds-codec.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +12
-0@@ -64,6 +64,15 @@ properties: - port@0 - port@1 + data-mapping: + enum: + - jeida-18 + - jeida-24 + - vesa-24 + description: | + The color signals mapping order. See details in + Documentation/devicetree/bindings/display/panel/lvds.yaml + pclk-sample: description: Data sampling on rising or falling edge.@@ -79,6 +88,16 @@ properties: power-supply: true +if: + not: + properties:
Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml properties: · line 103
Quoted 10 lines
Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +7
-0+ compatible: + contains: + const: lvds-decoder +then: + properties: + data-mapping: false + if: not: properties:
Unless I'm mistaken, you can't have identically named properties at the same level (multiple 'if' at the root level). You can group them in a 'allOf' property.
Signature · 3 lines
Regards, Laurent Pinchart
Laurent Pinchart
reviewerAnd another thing:
Quoted 46 lines · Laurent Pinchart
Quoted 3 lines · Marek Vasut
Decoder input LVDS format is a property of the decoder chip or even its strapping. Add DT property data-mapping the same way lvds-panel does, to define the LVDS data mapping.
The information could also be derived by the driver from the compatible string in the case when this is an intrinsic property of the device (or when it's configurable by software), but the fact that it can also be controlled by strapping makes a DT property needed. We may want to limit the usage of the DT property to the strapping case though, but I don't have a real preference here, so I'm fine with this approach.
Quoted 34 lines
Signed-off-by Marek Vasut <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Rob Herring <[email protected]>Cc Sam Ravnborg <[email protected]>--- .../bindings/display/bridge/lvds-codec.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +9
-0@@ -64,6 +64,15 @@ properties: - port@0 - port@1 + data-mapping: + enum: + - jeida-18 + - jeida-24 + - vesa-24 + description: | + The color signals mapping order. See details in + Documentation/devicetree/bindings/display/panel/lvds.yaml + pclk-sample: description: Data sampling on rising or falling edge.@@ -79,6 +88,16 @@ properties: power-supply: true
Quoted 9 lines
Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml properties: · line 99
Quoted 9 lines
Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +9
-0+if: + not: + properties: + compatible: + contains: + const: lvds-decoder +then: + properties: + data-mapping: false
Should we make the property required for lvds-decoder ? We need to support backward compatibility in the driver, but from a DT bindings point of view, should all new DTs require the property ?
Quoted 8 lines
Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml properties: · line 103
Quoted 4 lines
Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +1
-0+ if: not: properties:Unless I'm mistaken, you can't have identically named properties at the same level (multiple 'if' at the root level). You can group them in a 'allOf' property.
Signature · 3 lines
Regards, Laurent Pinchart
On 5/18/21 1:03 AM, Laurent Pinchart wrote:
[...]
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +5
-0+if: + not: + properties: + compatible: + contains:
Quoted 8 lines
Quoted 4 lines
Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml properties: · line 99
Quoted 4 lines
Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +4
-0+ const: lvds-decoder +then: + properties: + data-mapping: falseShould we make the property required for lvds-decoder ? We need to support backward compatibility in the driver, but from a DT bindings point of view, should all new DTs require the property ?
I think so.
Quoted 1 line · Laurent Pinchart
Hi Marek,
Hi,
Quoted 7 lines
Thank you for the patch.
Quoted 3 lines · Marek Vasut
Decoder input LVDS format is a property of the decoder chip or even its strapping. Add DT property data-mapping the same way lvds-panel does, to define the LVDS data mapping.
Quoted 6 lines
The information could also be derived by the driver from the compatible string in the case when this is an intrinsic property of the device (or when it's configurable by software), but the fact that it can also be controlled by strapping makes a DT property needed. We may want to limit the usage of the DT property to the strapping case though, but I don't have a real preference here, so I'm fine with this approach.
You'd soon end up piling up compatible strings like the panel-simple does, I don't think that scales. Besides, there are bridges where you can pick the mapping with a strap, and then the compatible string is no longer enough.
[...]