Re: [PATCH v2 2/4] drm: mxsfb: Add optional DPI output bus-width configuration

[Francesco Dolcini]

Hello Marco,
thanks for the review

On Thu, Jul 23, 2026 at 11:48:03AM +0200, Marco Felsch wrote:
> On 26-07-23, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx>
> > 
> > LCDIF programs LCD_DATABUS_WIDTH from the selected media bus format. The
> > format reported by the downstream panel or bridge describes the display
> > input, but it does not describe how the LCDIF data pins are physically
> > wired on the board.
> > 
> > These can differ. For example, a 16-bit LCDIF bus can be connected to a
> > 24-bit display by wiring the available color bits to the corresponding
> > display inputs. In that case, using the display's 24-bit format to
> > configure LCDIF selects the wrong data-bus mode and changes the assignment
> > of color bits on the LCD_DATA pins.
> > 
> > Read the optional bus-width endpoint property from the LCDIF output port
> > and use it to select the media bus format used to configure LCDIF. This
> > allows the LCDIF bus mode to describe the physical interface
> > independently of the downstream display format.
> > 
> > When the optional property is absent, continue using the format reported
> > by the downstream display device, preserving the existing behavior.
> > 
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx>
> > ---
> > v2: use the common bus-width property instead of the legacy interface-pix-fmt
> > ---
> >  drivers/gpu/drm/mxsfb/mxsfb_drv.c | 24 ++++++++++++++++++++++++
> >  drivers/gpu/drm/mxsfb/mxsfb_drv.h |  2 ++
> >  drivers/gpu/drm/mxsfb/mxsfb_kms.c | 12 ++++++++++++
> >  3 files changed, 38 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> > index 9b8fbda85d28..0545718a4b65 100644
> > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> > @@ -12,7 +12,10 @@
> >  #include <linux/clk.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/io.h>
> > +#include <linux/media-bus-format.h>
> >  #include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_graph.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/property.h>
> >  #include <linux/pm_runtime.h>
> > @@ -209,7 +212,10 @@ static int mxsfb_load(struct drm_device *drm,
> >  		      const struct mxsfb_devdata *devdata)
> >  {
> >  	struct platform_device *pdev = to_platform_device(drm->dev);
> > +	struct device_node *np = pdev->dev.of_node;
> >  	struct mxsfb_drm_private *mxsfb;
> > +	struct device_node *ep;
> > +	u32 bus_width = 0;
> 
> With this beeing set to '0' and ...
> 
> >  	int ret;
> >  
> >  	mxsfb = devm_kzalloc(&pdev->dev, sizeof(*mxsfb), GFP_KERNEL);
> > @@ -236,6 +242,24 @@ static int mxsfb_load(struct drm_device *drm,
> >  	if (IS_ERR(mxsfb->clk_disp_axi))
> >  		mxsfb->clk_disp_axi = NULL;
> >  
> > +	ep = of_graph_get_next_endpoint(np, NULL);
> > +	if (ep) {
> > +		of_property_read_u32(ep, "bus-width", &bus_width);
> 
> no bus-width property present, this ...
> 
> > +		of_node_put(ep);
> 
> Furthermore please see
> 20260723-v6-18-topic-imx93-parallel-display-v13-1-ccf3f9bbc0fc@xxxxxxx
> for a proper error handling.

Not sure if I see the value on doing this. You are just doing the
validation of the DT again in the code. My understanding is that this is
not the way to go.

BTW, I assume you are suggesting something like this, if this is not the
case please let me know

	if (ep) {
		err = of_property_read_u32();
		of_node_put(ep);

		if (err && err != -EINVAL)
			return dev_err_probe(dev, err,
					     "failed to query bus-width\n");
	}

> 
> > +	}
> > +
> > +	switch (bus_width) {
> 
> switch should fail. Therefore I would recommend to set the default to
> 24 (bit) aka no limitation.

I would not do that.

We have an optional property, the DT is validated against the binding
for correctness, we should not validate it again in the code. If the
property is not present or it contains an invalid value it is
just ignored, preserving the existing behavior.

Francesco