Hi Marek,

Thank you for the patch.

On Sat, May 15, 2021 at 10:46:56PM +0200, Marek Vasut wrote:

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 marex@denx.de Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Sam Ravnborg sam@ravnborg.org To: dri-devel@lists.freedesktop.org

---

drivers/gpu/drm/bridge/lvds-codec.c | 50 +++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index 8a7cb267ab14..33f992d52902 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -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);
  

• }

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 ?

• 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;

I would have moved this variable to the if () { ... } below, but maybe that's just me.

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

Hi Marek,

On Tue, May 25, 2021 at 12:38:47PM +0200, Marek Vasut wrote:

On 5/18/21 1:03 AM, Laurent Pinchart wrote:

Hi,

[...]

...

...

@@ -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);
  

• }

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.

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.

And another thing:

On Tue, May 18, 2021 at 02:02:24AM +0300, Laurent Pinchart wrote:

On Sat, May 15, 2021 at 10:46:55PM +0200, Marek Vasut wrote:

...

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.

...

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

---

.../bindings/display/bridge/lvds-codec.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml index f4dd16bd69d2..f0abb94f8f2e 100644 --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml @@ -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

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 ?

...

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.