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

[Liu Ying]

On Tue, Mar 03, 2026 at 09:05:43AM +0100, Marco Felsch wrote:
> On 26-03-03, Liu Ying wrote:
>> On Mon, Mar 02, 2026 at 05:10:41PM +0100, Marco Felsch wrote:
>>> From: Liu Ying <victor.liu@xxxxxxx>
>>>
>>> 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.
>>>
>>> Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
>>> [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]
>>> Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/bridge/imx/Kconfig      |  11 ++
>>>  drivers/gpu/drm/bridge/imx/Makefile     |   1 +
>>>  drivers/gpu/drm/bridge/imx/imx93-pdfc.c | 225 ++++++++++++++++++++++++++++++++
>>>  3 files changed, 237 insertions(+)
>>>
>>
>> [...]
>>
>>> +static bool imx93_pdfc_bus_output_fmt_supported(const  u32 fmt)
>>
>> As I mentioned in v9, can you drop const?
>> I don't think const is needed.
> 
> It makes sense to spot failures early. Albeit this function is very
> small, the fmt shouldn't be changed and therefore needs to be const.

If fmt is a pointer, then I think it makes sense to keep const.
Otherwise, it looks a bit over engineered to have const, since this
function is really small and no one would try to change fmt.

> 
> I forgot to add the double space fix though, thanks.
> 
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(imx93_pdfc_bus_output_fmts); i++) {
>>> +		if (imx93_pdfc_bus_output_fmts[i] == fmt)
>>> +			return true;
>>> +	}
>>> +
>>> +	return false;
>>> +}

[...]

>>> +static int imx93_pdfc_bridge_atomic_check(struct drm_bridge *bridge,
>>> +					  struct drm_bridge_state *bridge_state,
>>> +					  struct drm_crtc_state *crtc_state,
>>> +					  struct drm_connector_state *conn_state)
>>> +{
>>> +	struct imx93_pdfc *pdfc = bridge_to_imx93_pdfc(bridge);
>>> +	const u32 format = bridge_state->output_bus_cfg.format;
>>
>> Can you drop const?
> 
> No because this function is not supposed to change the format. The
> function documentation says: this function can alter the bus_cfg.flags
> bit _not_ the format.

The local variable format is not a pointer, so literally the value of
bridge_state->output_bus_cfg.format cannot be changed through format,
which follows the documentation.  So, I think const could be dropped.

> 
> Regards,
>   Marco
> 
> 
> 
>>
>> -- 
>> Regards,
>> Liu Ying
>>
> 

-- 
Regards,
Liu Ying