Re: [PATCH v2] drm/bridge: microchip-lvds: fix bus format mismatch with VESA displays

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dharma,

Thank you for the patch.

On Mon, Jun 23, 2025 at 04:20:20PM +0530, Dharma Balasubiramani wrote:
> From: Sandeep Sheriker M <sandeep.sheriker@microchip.com>
> 
> The LVDS controller was hardcoded to JEIDA mapping, which leads to
> distorted output on panels expecting VESA mapping.
> 
> Update the driver to dynamically select the appropriate mapping and
> pixel size based on the panel's advertised media bus format. This
> ensures compatibility with both JEIDA and VESA displays.
> 
> Modernize the bridge ops to use atomic_enable/disable, and retrieve
> the bus format from the connector via the atomic bridge state.
> 
> Signed-off-by: Sandeep Sheriker M <sandeep.sheriker@microchip.com>
> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
> ---
> Note: Tested the changes on newvision 10.1 VESA display.
> 
> Changes in v2:
> - Switch to atomic bridge functions
> - Drop custom connector creation
> - Use drm_atomic_get_new_connector_for_encoder()
> - Link to v1: https://lore.kernel.org/r/20250618-microchip-lvds-v1-1-1eae5acd7a82@microchip.com
> ---
>  drivers/gpu/drm/bridge/microchip-lvds.c | 64 +++++++++++++++++++++++++++------
>  1 file changed, 54 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/microchip-lvds.c b/drivers/gpu/drm/bridge/microchip-lvds.c
> index 9f4ff82bc6b4..b71478aa36e9 100644
> --- a/drivers/gpu/drm/bridge/microchip-lvds.c
> +++ b/drivers/gpu/drm/bridge/microchip-lvds.c
> @@ -11,6 +11,7 @@
>  #include <linux/component.h>
>  #include <linux/delay.h>
>  #include <linux/jiffies.h>
> +#include <linux/media-bus-format.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/of_graph.h>
>  #include <linux/pinctrl/devinfo.h>
> @@ -41,9 +42,11 @@
>  
>  /* Bitfields in LVDSC_CFGR (Configuration Register) */
>  #define LVDSC_CFGR_PIXSIZE_24BITS	0
> +#define LVDSC_CFGR_PIXSIZE_18BITS	1

#define LVDSC_CFGR_PIXSIZE_18BITS	BIT(0)

>  #define LVDSC_CFGR_DEN_POL_HIGH		0
>  #define LVDSC_CFGR_DC_UNBALANCED	0
>  #define LVDSC_CFGR_MAPPING_JEIDA	BIT(6)
> +#define LVDSC_CFGR_MAPPING_VESA		0
>  
>  /*Bitfields in LVDSC_SR */
>  #define LVDSC_SR_CS	BIT(0)

I think you can drop the panel field of the mchp_lvds structure in the
same patch.

> @@ -76,9 +79,10 @@ static inline void lvds_writel(struct mchp_lvds *lvds, u32 offset, u32 val)
>  	writel_relaxed(val, lvds->regs + offset);
>  }
>  
> -static void lvds_serialiser_on(struct mchp_lvds *lvds)
> +static void lvds_serialiser_on(struct mchp_lvds *lvds, u32 bus_format)
>  {
>  	unsigned long timeout = jiffies + msecs_to_jiffies(LVDS_POLL_TIMEOUT_MS);
> +	u8 map, pix_size;
>  
>  	/* The LVDSC registers can only be written if WPEN is cleared */
>  	lvds_writel(lvds, LVDSC_WPMR, (LVDSC_WPMR_WPKEY_PSSWD &
> @@ -93,11 +97,24 @@ static void lvds_serialiser_on(struct mchp_lvds *lvds)
>  		usleep_range(1000, 2000);
>  	}
>  
> +	switch (bus_format) {
> +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> +		map = LVDSC_CFGR_MAPPING_JEIDA;
> +		pix_size = LVDSC_CFGR_PIXSIZE_18BITS;
> +		break;
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> +		map = LVDSC_CFGR_MAPPING_VESA;
> +		pix_size = LVDSC_CFGR_PIXSIZE_24BITS;
> +		break;
> +	default:
> +		map = LVDSC_CFGR_MAPPING_JEIDA;
> +		pix_size = LVDSC_CFGR_PIXSIZE_24BITS;
> +		break;
> +	}
> +
>  	/* Configure the LVDSC */
> -	lvds_writel(lvds, LVDSC_CFGR, (LVDSC_CFGR_MAPPING_JEIDA |
> -				LVDSC_CFGR_DC_UNBALANCED |
> -				LVDSC_CFGR_DEN_POL_HIGH |
> -				LVDSC_CFGR_PIXSIZE_24BITS));
> +	lvds_writel(lvds, LVDSC_CFGR, (map | LVDSC_CFGR_DC_UNBALANCED |
> +		    LVDSC_CFGR_DEN_POL_HIGH | pix_size));

You can drop the inner parentheses.

>  
>  	/* Enable the LVDS serializer */
>  	lvds_writel(lvds, LVDSC_CR, LVDSC_CR_SER_EN);
> @@ -113,7 +130,8 @@ static int mchp_lvds_attach(struct drm_bridge *bridge,
>  				 bridge, flags);
>  }
>  
> -static void mchp_lvds_enable(struct drm_bridge *bridge)
> +static void mchp_lvds_atomic_pre_enable(struct drm_bridge *bridge,
> +					struct drm_atomic_state *state)
>  {
>  	struct mchp_lvds *lvds = bridge_to_lvds(bridge);
>  	int ret;
> @@ -129,11 +147,35 @@ static void mchp_lvds_enable(struct drm_bridge *bridge)
>  		dev_err(lvds->dev, "failed to get pm runtime: %d\n", ret);
>  		return;
>  	}
> +}
> +
> +static void mchp_lvds_atomic_enable(struct drm_bridge *bridge,
> +				    struct drm_atomic_state *state)
> +{
> +	struct mchp_lvds *lvds = bridge_to_lvds(bridge);
> +	struct drm_connector *connector;
> +
> +	/* default to jeida-24 */
> +	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> +
> +	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> +	if (connector && connector->display_info.num_bus_formats)
> +		bus_format = connector->display_info.bus_formats[0];
> +
> +	lvds_serialiser_on(lvds, bus_format);
> +}
> +
> +static void mchp_lvds_atomic_disable(struct drm_bridge *bridge,
> +				     struct drm_atomic_state *state)
> +{
> +	struct mchp_lvds *lvds = bridge_to_lvds(bridge);
>  
> -	lvds_serialiser_on(lvds);
> +	/* Turn off the serialiser */
> +	lvds_writel(lvds, LVDSC_CR, 0);
>  }
>  
> -static void mchp_lvds_disable(struct drm_bridge *bridge)
> +static void mchp_lvds_atomic_post_disable(struct drm_bridge *bridge,
> +					  struct drm_atomic_state *state)
>  {
>  	struct mchp_lvds *lvds = bridge_to_lvds(bridge);
>  
> @@ -143,8 +185,10 @@ static void mchp_lvds_disable(struct drm_bridge *bridge)
>  
>  static const struct drm_bridge_funcs mchp_lvds_bridge_funcs = {
>  	.attach = mchp_lvds_attach,
> -	.enable = mchp_lvds_enable,
> -	.disable = mchp_lvds_disable,
> +	.atomic_pre_enable = mchp_lvds_atomic_pre_enable,
> +	.atomic_enable = mchp_lvds_atomic_enable,
> +	.atomic_disable = mchp_lvds_atomic_disable,
> +	.atomic_post_disable = mchp_lvds_atomic_post_disable,
>  };
>  
>  static int mchp_lvds_probe(struct platform_device *pdev)
> 
> ---
> base-commit: 4325743c7e209ae7845293679a4de94b969f2bef
> change-id: 20250618-microchip-lvds-b7151d96094a

-- 
Regards,

Laurent Pinchart