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

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

On Wed Sep 16, 2026 at 1:43 PM CEST, Francesco Dolcini wrote:
> On Wed, Sep 16, 2026 at 01:19:08PM +0200, Luca Ceresoli wrote:
>> On Wed Sep 16, 2026 at 11:15 AM CEST, Francesco Dolcini wrote:
>> > Hello Luca,
>> >
>> > On Wed, Sep 16, 2026 at 10:42:13AM +0200, Luca Ceresoli wrote:
>> >> > 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, or it has an invalid value, continue
>> >> > using the format reported by the downstream display device, preserving
>> >> > the existing behavior. The code is not validating the DT for
>> >> > correctness and just fall back to the previous behavior in case of
>> >> > errors.
>> >>
>> >> Why? Generally speaking errors are better spotted immediately, not ignored
>> >> silently.
>> >
>> > My understanding is that the C driver code should not validate the DT
>> > and that this is the general recommendation when parsing properties from
>> > the DT.
>> >
>> > The DT checker is going to spot errors in the DT even earlier and in a
>> > static way.
>>
>> Drivers typically check for invalid values.
>>
>> And people writing their private dts can do mistakes and not run the static
>> checks. Bad on their side, sure, but having a runtime error would be useful
>> there.
>
> Understood. I would personally not do it. I can add the required error
> checking code. No other DRM maintainer/reviewer commented so far, so I
> am taking this as a request.
>
> What should I do in case there is an invalid value? Fail or have a
> warning message?

Just fail. You are in the probe path, so it's OK.

>> Also consider potential regressions. Say someone has an invalid value in
>> their product, say bus-width = <15>, which works because the default 18
>> which gets used as a fallback happens to be the correct value for their
>> hardware. Some years later we implement the new bus-width = <15>, and then
>> that user upgrades to a newer kernel and their display won't work because
>> it will start using that incorrect bus-witch = <15>. Better stopping this
>> before it starts, IMO.
>
> I do not see this situation honestly, if you have a wrong value, e.g. 15
> bits, before this patch is ignored, and after this patch is also
> ignored ...

There is no problem with the code in this patch alone.

The problem is when in two years in the future we do implement 15
bits. (That's an example to explain the principle, I don't expect it to
happen with this driver.) But imagine in the future we add:

	switch (bus_width) {
    +	case 15:
    +		mxsfb->bus_format = MEDIA_BUS_FMT_RGB555_1X15;
    +		break;
	case 16:
		mxsfb->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
		break;

At that point the device in the wild with the incorrect 'bus-width = <15>'
will start using RGB555 and fail, because they really have a 18 bit panel
which worked thanks to the automagic "if DT is wrong, let's go with the
default". That's a regression for that device.

Reporting an error from day 0 means 'bus-width = <15>' will be blocked, the
device won't probe and the mistake in the device tree gets fixed before
shipping.

I hope it's clearer now.

Cheers,
Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com