Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher <max.oss.09@gmail.com>]

On Wed, Mar 2, 2022 at 5:22 PM Marek Vasut <marex@denx.de> wrote:
>
> On 3/2/22 15:21, Maxime Ripard wrote:
> > Hi,
>
> Hi,
>
> > Please try to avoid top posting
Sorry.

> >
> > On Wed, Feb 23, 2022 at 04:25:19PM +0100, Max Krummenacher wrote:
> >> The goal here is to set the element bus_format in the struct
> >> panel_desc. This is an enum with the possible values defined in
> >> include/uapi/linux/media-bus-format.h.
> >>
> >> The enum values are not constructed in a way that you could calculate
> >> the value from color channel width/shift/mapping/whatever. You rather
> >> would have to check if the combination of color channel
> >> width/shift/mapping/whatever maps to an existing value and otherwise
> >> EINVAL out.
> >>
> >> I don't see the value in having yet another way of how this
> >> information can be specified and then having to write a more
> >> complicated parser which maps the dt data to bus_format.
> >
> > Generally speaking, sending an RFC without explicitly stating what you
> > want a comment on isn't very efficient.
>
> Isn't that what RFC stands for -- Request For Comment ?

I hoped that the link to the original discussion was enough.

panel-simple used to have a finite number of hardcoded panels selected
by their compatible.
The following patchsets added a compatible 'panel-dpi' which should
allow to specify the panel in the device tree with timing etc.
  https://patchwork.kernel.org/project/dri-devel/patch/20200216181513.28109-6-sam@ravnborg.org/
In the same release cycle part of it got reverted:
  https://patchwork.kernel.org/project/dri-devel/patch/20200314153047.2486-3-sam@ravnborg.org/
With this it is no longer possible to set bus_format.

The explanation what makes the use of a property "data-mapping" not a
suitable way in that revert
is a bit vague.

The RFC revert of the revert
  https://patchwork.kernel.org/project/dri-devel/patch/20220201110717.3585-1-cniedermaier@dh-electronics.com/
tried to get feedback what would be a way forward. This RFC tries the
same by giving a possible solution should
the property name and/or the a bit short strings of the original be
the reason why it is not suitable.

So the requested comments would be about what was not good enough with
'data-mapping' and what would be a way forward.

Especially since in my limited view it is not clear why in panel-lvds
'data-mapping' is used to state how the bits representing colour are
mapped to the 21 or 28 possible bit position in the LVDS lanes vs.
here where we want to say how the bits representing colour are mapped
to the 16/18/24 lines of the parallel interface would need a different
binding pattern.

>
> > That being said, what I (and I can only assume Marek) don't like is the
> > string encoding. Especially when the similar bus-type property uses a
> > integer with the various available bus options we have.
>
> Right, the string encoding isn't good.
>
> > Having an integer, with a set of defines that you would map to the
> > proper MEDIA_BUS_* would be more efficient and more elegant.

I have a look at that.

> >
> > That being said, the first question that needs to be answered is why
> > does this have to be in the DT in the first place?

The way I understand the compatible panel-dp, iti should allow to fill
a 'struct panel_desc'
with data provided by the device tree rather than having the info
hardcoded in the C source.
The missing element is bus_format which currently is kept at 0.

>
> Because panel-simple panel-dpi , you may need to specify the bus format
> between the last bridge and the panel .

Exactly.

Max