On 26-03-03, Liu Ying wrote:
> 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 <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> [[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]
> >>> Signed-off-by: Marco Felsch <[email protected]>
> >>> ---
> >>> 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'm aware that this is a copy of the output_cfg.format value. Would it
be a non const value you could mess with it and return the wrong value
afterwards. Keeping it const avoids this. Not sure why a 'const' is
worth the discussion :/
> > 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.
Same here, I'm aware that this is not the pointer value but it holds a
copy of the value. Any adaptions made to the copy will fail, any
assignment of a const value to a non const value will prints at least a
warning to show that something is wrong within this function. Only a
direct bridge_state->output_bus_cfg.format assigned would cause no
compiler error/warning, fingers crossed that this will not happen.
I'm not sure why the 'const' is such a big problem in your oppinion.
Everything works just fine and we've done everything in our power to
avoid a wrong function behavior.
Regards,
Marco
>
> >
> > Regards,
> > Marco
> >
> >
> >
> >>
> >> --
> >> Regards,
> >> Liu Ying
> >>
> >
>
> --
> Regards,
> Liu Ying
>
--
#gernperDu
#CallMeByMyFirstName
Pengutronix e.K. | |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
