[PATCH RFC 17/19] drm/bridge: lvds-encoder: Implement bus format negotiation for sn75lvds83

[Laurent Pinchart]

Hi Boris,

Thank you for the patch.

On Thu, Aug 08, 2019 at 05:11:48PM +0200, Boris Brezillon wrote:
> The SN75LVDS83 bridge support several input formats except the input
> format is directly related to the expected output format. Let's express
> that constraint by setting the bridge_state->output_bus_cfg.fmt field
> to the appropriate value.
> 
> The previous element in the chain might be able to use this information
> to pick the appropriate output bus format.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>

This driver is supposed to only support LVDS encoders that require no
special configuration, and thus not contain device-specific code. I'm
not sure I like departing from this :-S

Would there be a way to make this more generic ? Do you think the code
below could be generalized, or is it really device-specific ?

> ---
>  drivers/gpu/drm/bridge/lvds-encoder.c | 48 +++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> index da7013c5faf1..8d399d1828b0 100644
> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
> @@ -159,9 +159,57 @@ static int lvds_encoder_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int sn75lvds83_atomic_check(struct drm_bridge *bridge,
> +				   struct drm_bridge_state *bridge_state,
> +				   struct drm_crtc_state *crtc_state,
> +				   struct drm_connector_state *conn_state)
> +{
> +	int ret;
> +
> +	ret = drm_atomic_bridge_choose_output_bus_cfg(bridge_state, crtc_state,
> +						      conn_state);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The output bus format has a direct impact on the expected input bus
> +	 * format.
> +	 */
> +	switch (bridge_state->output_bus_cfg.fmt) {
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> +		/*
> +		 * JEIDA and SPWG variants theoretically require different pin
> +		 * mapping, but MEDIA_BUS_FMT_ definitions do not allow
> +		 * fined-grained pin placement definition, and this is
> +		 * something we expect to be taken care of at board design
> +		 * time, so let's ignore this for now.
> +		 * If it becomes a problem, we can always add a way to override
> +		 * the bus format with a FW property.
> +		 */
> +		bridge_state->input_bus_cfg.fmt = MEDIA_BUS_FMT_RGB888_1X24;
> +		break;
> +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> +		bridge_state->input_bus_cfg.fmt = MEDIA_BUS_FMT_RGB666_1X18;
> +		break;
> +	default:
> +		bridge_state->input_bus_cfg.fmt = 0;
> +		break;
> +	}

These seems a bit fragile. The SN75LVDS83 has a 28-bit input data bus,
and maps those bits to 3x8 RGB data + 1x4 control signals. It serialises
the data and outputs them on 4 LVDS pairs. When a panel uses
MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA or MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, the
4 LVDS pairs are used, and MEDIA_BUS_FMT_RGB888_1X24 is required. When
outputting MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, the fourth LVDS pair is
ignored, but the 3x8 RGB input bus can still receive
MEDIA_BUS_FMT_RGB888_1X24, the 2 LSBs of each component will just be
discarded. The source can thus ignore those 3x2 LSBs, but it still has
to map the 3x6 MSBs to the high order bits of the 3x8 signals.
*However*, if the hardware designs wires the 3x6 MSBs of the SN75LVDS83
to the 3x6 LSBs of the source, then the source will have to produce
MEDIA_BUS_FMT_RGB666_1X18. Hardcoding the mapping as done here thus
seems to be system-specific. I think you need to check how signals are
connected (through DT properties, bus-width and data-shift come to
mind).

> +
> +	/* Propagate the bus_flags. */
> +	bridge_state->input_bus_cfg.flags = bridge_state->output_bus_cfg.flags;
> +	return 0;
> +}
> +
> +static const struct lvds_encoder_ops sn75lvds83_ops = {
> +	.atomic_check = sn75lvds83_atomic_check,
> +};
> +
>  static const struct of_device_id lvds_encoder_match[] = {
>  	{ .compatible = "lvds-encoder" },
>  	{ .compatible = "thine,thc63lvdm83d" },
> +	{ .compatible = "ti,sn75lvds83", .data = &sn75lvds83_ops },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, lvds_encoder_match);

-- 
Regards,

Laurent Pinchart