Hi Sam,

On Sun, Oct 10, 2021 at 10:16:22AM +0200, Sam Ravnborg wrote:

On Sun, Oct 10, 2021 at 12:41:52AM +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.

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com 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

I am late to the game here - sorry. A few details in the following. With these considered: Reviewed-by: Sam Ravnborg sam@ravnborg.org

...

---

V2: - Move the data-mapping to endpoint V3: - Rebase on V2 submitted a while ago, reinstate changelog - Use .atomic_get_input_bus_fmts for the decoder, separate funcs for encoder V4: - No change V5: - Move the data-mapping property to port@0 , decoder LVDS input - Add RB from Laurent

---

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

diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index dcf579a4cf83..08091bab4857 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -12,6 +12,7 @@ #include <linux/platform_device.h> #include <linux/regulator/consumer.h>

+#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_panel.h>

@@ -22,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) @@ -74,12 +76,50 @@ static const struct drm_bridge_funcs funcs = { .disable = lvds_codec_disable, };

+#define MAX_INPUT_SEL_FORMATS 1 +static u32 * +lvds_codec_atomic_get_input_bus_fmts(struct drm_bridge *bridge,

• 		     struct drm_bridge_state *bridge_state,
  

• 		     struct drm_crtc_state *crtc_state,
  

• 		     struct drm_connector_state *conn_state,
  

• 		     u32 output_fmt,
  

• 		     unsigned int *num_input_fmts)
  

+{

• struct lvds_codec *lvds_codec = to_lvds_codec(bridge);
• u32 *input_fmts;
• *num_input_fmts = 0;
• input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),

• 	     GFP_KERNEL);
  

• if (!input_fmts)

• return NULL;
  

• input_fmts[0] = lvds_codec->bus_format;
• *num_input_fmts = MAX_INPUT_SEL_FORMATS;
• return input_fmts;

+}

+static const struct drm_bridge_funcs funcs_decoder = {

• .attach = lvds_codec_attach,
• .enable = lvds_codec_enable,
• .disable = lvds_codec_disable,
• .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
• .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
• .atomic_reset = drm_atomic_helper_bridge_reset,
• .atomic_get_input_bus_fmts = lvds_codec_atomic_get_input_bus_fmts,

+};

static int lvds_codec_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct device_node *panel_node;

• struct device_node *bus_node; struct drm_panel *panel; struct lvds_codec *lvds_codec;

• const char *mapping;

• int ret;

  lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); if (!lvds_codec)

@@ -119,13 +159,47 @@ static int lvds_codec_probe(struct platform_device *pdev) if (IS_ERR(lvds_codec->panel_bridge)) return PTR_ERR(lvds_codec->panel_bridge);

• lvds_codec->bridge.funcs = &funcs;
• /*

• * 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.
  

The missing data-mapping property is reported as an error, but this comments says it is OK. Info?

It's not a fatal error, probe still continues in backward-compatibility mode. The message is there to tell that the DT should be updated. Would you downgrade that to a warning ?

...

• */
  

• if (lvds_codec->connector_type != DRM_MODE_CONNECTOR_LVDS) {

• bus_node = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
  

Are there any reg specified in the binding? If not then the second parameter should be -1 here.

Yes, the DT node has two ports, with port 0 being the input. For LVDS decoders, that's where the LVDS bus is.

...

• if (!bus_node) {
  

• 	dev_dbg(dev, "bus DT node not found\n");
  

• 	return -ENXIO;
  

• }
  

• ret = of_property_read_string(bus_node, "data-mapping",
  

• 			      &mapping);
  

• of_node_put(bus_node);
  

• if (ret < 0) {
  

• 	dev_err(dev, "missing 'data-mapping' DT property\n");
  

• } else {
  

It would be nice with a helper for the below code if we need this a third time.

Where would you store it ?

...

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

• 	}
  

Add empty line here.

...

• 	lvds_codec->bridge.funcs = &funcs_decoder;
  

• }
  

• }
• /*
  • 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.funcs = &funcs; drm_bridge_add(&lvds_codec->bridge);

  platform_set_drvdata(pdev, lvds_codec);