Re: [PATCH V4 2/2] drm/bridge: lvds-codec: Add support for LVDS data mapping select

Hi Marek,

Thank you for the patch, and all my apologies for the delay.

On Tue, Jul 27, 2021 at 06:13:57PM +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

---

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

---

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 dcf579a4cf833..afa7ce7ea01e8 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;

Are there cases where the input format would depend on the output format ? For instance, a 24-bit RGB output that could also work in RGB666 mode, where we would select between MEDIA_BUS_FMT_RGB666_1X7X3_SPWG and MEDIA_BUS_FMT_RGB888_1X7X4_SPWG depending on whether the output is RGB666 or RGB888 ? I don't think we need to handle this now, and I believe it will be possible to do this in a backward-compatible way if the need ever arises. A confirmation that I'm not missing something obvious would be nice.

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

• */
  

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

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

I think this should be

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

(see the comment on patch 1/2).

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

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

• 	}
  

• 	lvds_codec->bridge.funcs = &funcs_decoder;
  

• }
  

I may have split this to a separate function to reduce line length and simplify error handling, up to you.

With at least the port number fix above (unless I'm mistaken of course),

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

• }
• /*
  • 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);