Hi Maxime,
On 18/06/25 2:11 pm, Maxime Ripard wrote:
> Hi,
>
> On Wed, Jun 18, 2025 at 10:02:42AM +0530, Dharma Balasubiramani wrote:
>> From: Sandeep Sheriker M <sandeep.sheri...@microchip.com>
>>
>> The current LVDS controller driver is hardcoded to map LVDS lanes to the
>> JEIDA format. Consequently, connecting an LVDS display that supports the
>> VESA format results in a distorted display due to the format mismatch.
>>
>> Query the panel driver and set the appropriate format to resolve the issue.
>>
>> Signed-off-by: Sandeep Sheriker M <sandeep.sheri...@microchip.com>
>> Signed-off-by: Dharma Balasubiramani <dharm...@microchip.com>
>
> It looks like there's a bit of context missing to explain why you needed
> to do it that way. See below.
>
>> ---
>> drivers/gpu/drm/bridge/microchip-lvds.c | 108
>> ++++++++++++++++++++++++++++++--
>> 1 file changed, 102 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/microchip-lvds.c
>> b/drivers/gpu/drm/bridge/microchip-lvds.c
>> index 9f4ff82bc6b4..5e99c01033bb 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_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)
>> @@ -58,6 +61,7 @@ struct mchp_lvds {
>> struct clk *pclk;
>> struct drm_panel *panel;
>> struct drm_bridge bridge;
>> + struct drm_connector connector;
>> struct drm_bridge *panel_bridge;
>> };
>>
>> @@ -66,6 +70,11 @@ static inline struct mchp_lvds *bridge_to_lvds(struct
>> drm_bridge *bridge)
>> return container_of(bridge, struct mchp_lvds, bridge);
>> }
>>
>> +static inline struct mchp_lvds *drm_connector_to_mchp_lvds(struct
>> drm_connector *connector)
>> +{
>> + return container_of(connector, struct mchp_lvds, connector);
>> +}
>> +
>> static inline u32 lvds_readl(struct mchp_lvds *lvds, u32 offset)
>> {
>> return readl_relaxed(lvds->regs + offset);
>> @@ -79,6 +88,11 @@ static inline void lvds_writel(struct mchp_lvds *lvds,
>> u32 offset, u32 val)
>> static void lvds_serialiser_on(struct mchp_lvds *lvds)
>> {
>> unsigned long timeout = jiffies +
>> msecs_to_jiffies(LVDS_POLL_TIMEOUT_MS);
>> + struct drm_connector *connector = &lvds->connector;
>
> How does that work if the bridge was attached with NO_CONNECTOR? Would
> the structure be uninitialized?
Thanks for pointing it out, I will perform the changes as you suggested
below.
>
>> +
>> + /* default to jeida-24 */
>> + u32 bus_formats = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
>> + 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,24 +107,106 @@ static void lvds_serialiser_on(struct mchp_lvds *lvds)
>> usleep_range(1000, 2000);
>> }
>>
>> + if (connector && connector->display_info.num_bus_formats)
>> + bus_formats = connector->display_info.bus_formats[0];
>> +
>> /* 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));
>> + switch (bus_formats) {
>> + 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;
>> + }
>> +
>> + lvds_writel(lvds, LVDSC_CFGR, (map | LVDSC_CFGR_DC_UNBALANCED |
>> + LVDSC_CFGR_DEN_POL_HIGH | pix_size));
>>
>> /* Enable the LVDS serializer */
>> lvds_writel(lvds, LVDSC_CR, LVDSC_CR_SER_EN);
>> }
>
> Aside from the bit above, that part looks fine to me.
>
>>
>> +static int mchp_lvds_connector_get_modes(struct drm_connector *connector)
>> +{
>> + struct mchp_lvds *lvds = drm_connector_to_mchp_lvds(connector);
>> +
>> + return drm_panel_get_modes(lvds->panel, connector);
>> +}
>> +
>> +static const struct drm_connector_helper_funcs
>> mchp_lvds_connector_helper_funcs = {
>> + .get_modes = mchp_lvds_connector_get_modes,
>> +};
>> +
>> +static const struct drm_connector_funcs panel_bridge_connector_funcs = {
>> + .reset = drm_atomic_helper_connector_reset,
>> + .fill_modes = drm_helper_probe_single_connector_modes,
>> + .destroy = drm_connector_cleanup,
>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +};
>> +
>> static int mchp_lvds_attach(struct drm_bridge *bridge,
>> struct drm_encoder *encoder,
>> enum drm_bridge_attach_flags flags)
>> {
>> struct mchp_lvds *lvds = bridge_to_lvds(bridge);
>> + struct drm_connector *connector = &lvds->connector;
>> + int ret;
>> +
>> + ret = drm_bridge_attach(encoder, lvds->panel_bridge,
>> + bridge, flags);
>> +
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>> + return 0;
>> +
>> + if (!encoder) {
>> + dev_err(lvds->dev, "Missing encoder\n");
>> + return -ENODEV;
>> + }
>> +
>> + drm_connector_helper_add(connector,
>> + &mchp_lvds_connector_helper_funcs);
>> +
>> + ret = drm_connector_init(bridge->dev, connector,
>> + &panel_bridge_connector_funcs,
>> + DRM_MODE_CONNECTOR_LVDS);
>> + if (ret) {
>> + dev_err(lvds->dev, "Failed to initialize connector %d\n", ret);
>> + return ret;
>> + }
>>
>> - return drm_bridge_attach(encoder, lvds->panel_bridge,
>> - bridge, flags);
>> + drm_panel_bridge_set_orientation(connector, bridge);
>> +
>> + ret = drm_connector_attach_encoder(&lvds->connector, encoder);
>> + if (ret) {
>> + dev_err(lvds->dev, "Failed to attach connector to encoder
>> %d\n", ret);
>> + drm_connector_cleanup(connector);
>> + return ret;
>> + }
>> +
>> + if (bridge->dev->registered) {
>> + if (connector->funcs->reset)
>> + connector->funcs->reset(connector);
>> +
>> + ret = drm_connector_register(connector);
>> + if (ret) {
>> + dev_err(lvds->dev, "Failed to attach connector to
>> register %d\n", ret);
>> + drm_connector_cleanup(connector);
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>
> However, this is the part I don't really get. AFAIU, you create a
> connector, for the sole purpose of calling the panel get_modes. But the
> panel bridge you already using is calling that function already. So
> there must be something more.
>
> Did you create the connector only to be able to access it in
> lvds_serialiser_on and thus retrieve the bus_formats? If so, you should
> convert enable / disable to atomic_enable / atomic_disable, pass
> drm_atomic_state to lvds_serialiser_on, and then call
> drm_atomic_get_new_connector_for_encoder(bridge->encoder).
Sure, I will drop enable / disable and add
.atomic_pre_enable,.atomic_enable,.atomic_disable,.atomic_post_disable
I will get the bus_format as you suggested
"
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);
"
and will drop the rest in v2.
Thanks.
>
> Maximee
--
With Best Regards,
Dharma B.
