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

[Laurentiu Palcu]

On Fri, Jul 24, 2026 at 04:23:17PM +0800, Liu Ying wrote:
> 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.
> > > 
> > > [m.felsch@xxxxxxxxxxxxxx: port to v7.0-rc1]
> > > [m.felsch@xxxxxxxxxxxxxx: add review feedback (Alexander)]
> > > [m.felsch@xxxxxxxxxxxxxx: fix to short Kconfig description (checkpath)]
> > > [m.felsch@xxxxxxxxxxxxxx: use "GPL" instead of "GPL v2" (checkpatch)]
> > > [m.felsch@xxxxxxxxxxxxxx: 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 <luca.ceresoli@xxxxxxxxxxx>
> > > Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
> > > Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> > > ---
> > 
> > [...]
> > 
> > > +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 <laurentiu.palcu@xxxxxxxxxxx>
> 
> If no objections, may I take this R-b tag as a valid one if keeping the
> mask variable as-is?

ok

-- 
Thanks,
Laurentiu