Re: [PATCH v13] drm/bridge: imx: Add i.MX93 parallel display format configuration support

On Fri, Jul 24, 2026 at 11:08:06AM +0300, Laurentiu Palcu wrote:
> Hi Ying,

Hi Laurentiu,

> 
> On Thu, Jul 23, 2026 at 02:37:32PM +0800, Liu Ying wrote:
> > NXP i.MX93 mediamix blk-ctrl contains one DISPLAY_MUX register which
> > configures parallel display format by using the "PARALLEL_DISP_FORMAT"
> > field. Add a DRM bridge driver to support the display format configuration.
> > 
> > [[email protected]: port to v7.0-rc1]
> > [[email protected]: add review feedback (Alexander)]
> > [[email protected]: fix to short Kconfig description (checkpath)]
> > [[email protected]: use "GPL" instead of "GPL v2" (checkpatch)]
> > [[email protected]: add bus-width support]
> > [ Liu Ying: Port to drm-misc/drm-misc-next(v7.2-rc2 based) ]
> > [ Liu Ying: Add nxp,imx91-pdfc compatible string ]
> > [ Liu Ying: Call drm_bridge_get() for next_bridge ]
> > [ Liu Ying: Reject unsupported output bus fmt in .atomic_get_input_bus_fmts() ]
> > 
> > Reviewed-by: Luca Ceresoli <[email protected]>
> > Signed-off-by: Marco Felsch <[email protected]>
> > Signed-off-by: Liu Ying <[email protected]>
> > ---
> 
> [...]
> 
> > +static void imx93_pdfc_bridge_atomic_enable(struct drm_bridge *bridge,
> > +					    struct drm_atomic_commit *state)
> > +{
> > +	struct imx93_pdfc *pdfc = bridge_to_imx93_pdfc(bridge);
> > +	const struct drm_bridge_state *bridge_state;
> > +	unsigned int mask = PARALLEL_DISP_FORMAT;
> > +	unsigned int val;
> > +
> > +	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
> > +
> > +	switch (bridge_state->output_bus_cfg.format) {
> > +	case MEDIA_BUS_FMT_RGB888_1X24:
> > +	case MEDIA_BUS_FMT_FIXED:
> > +		val = FORMAT_RGB888_TO_RGB888;
> > +		if (pdfc->phy_bus_width == 18) {
> > +			/*
> > +			 * Can be valid if physical bus limitation exists,
> > +			 * therefore use dev_dbg().
> > +			 */
> > +			dev_dbg(pdfc->dev, "Truncate two LSBs from each color\n");
> > +			val = FORMAT_RGB888_TO_RGB666;
> > +		}
> > +		break;
> > +	case MEDIA_BUS_FMT_RGB666_1X18:
> > +		val = FORMAT_RGB888_TO_RGB666;
> > +		break;
> > +	case MEDIA_BUS_FMT_RGB565_1X16:
> > +		val = FORMAT_RGB565_TO_RGB565;
> > +		break;
> > +	}
> > +
> > +	regmap_update_bits(pdfc->regmap, IMX93_DISPLAY_MUX_REG, mask, val);
> 
> Apparently, the 'mask' variable is only used in this call, nowhere else. Why
> not use PARALLEL_DISP_FORMAT macro directly here instead of declaring a
> variable just for that?

Marco introduced the mask variable to the code.  I'd say it makes the
regmap_update_bits() call fit into one line, so it doesn't look too bad
and I'm fine with it.

> 
> Otherwise,
> Reviewed-by: Laurentiu Palcu <[email protected]>

If no objections, may I take this R-b tag as a valid one if keeping the
mask variable as-is?

> 
> -- 
> Thanks,
> Laurentiu

-- 
Regards,
Liu Ying