diff mbox

[3/9] drm/panel: simple: make it possible to override LCD bus format

Message ID 1507721021-28174-4-git-send-email-LW@KARO-electronics.de (mailing list archive)
State New, archived
Headers show

Commit Message

Lothar Waßmann Oct. 11, 2017, 11:23 a.m. UTC
The baseboards for the Ka-Ro electronics series of i.MX modules
use a 24bit LCD interface, no matter what LCD bus width the SoC on the
module provides and what the LCD panel expects. LCDs with 6bit per color
will ignore the 2 LSBs of each color lane, and modules using a SoC
that provides only 6bit per color, drive the display information on the
6 MSBs of each color lane and tie the 2 LSBs of each color lane to GND.

Thus, no matter what combination of LCD and SoC is used, the LCD port
can be used without shuffling bit lanes by always configuring the LCD
output to 24bit mode.

Add a function to handle certain quirks of the LCD interface to the
panel driver to be able to override the bus format specified in a
panel's display_mode.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 .../bindings/display/panel/simple-panel.txt        |  2 ++
 drivers/gpu/drm/panel/panel-simple.c               | 40 +++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

Comments

Rob Herring Oct. 13, 2017, 10:13 p.m. UTC | #1
On Wed, Oct 11, 2017 at 01:23:35PM +0200, Lothar Waßmann wrote:
> The baseboards for the Ka-Ro electronics series of i.MX modules
> use a 24bit LCD interface, no matter what LCD bus width the SoC on the
> module provides and what the LCD panel expects. LCDs with 6bit per color
> will ignore the 2 LSBs of each color lane, and modules using a SoC
> that provides only 6bit per color, drive the display information on the
> 6 MSBs of each color lane and tie the 2 LSBs of each color lane to GND.
> 
> Thus, no matter what combination of LCD and SoC is used, the LCD port
> can be used without shuffling bit lanes by always configuring the LCD
> output to 24bit mode.

Thanks for providing good reasoning as to why this is needed.

> 
> Add a function to handle certain quirks of the LCD interface to the
> panel driver to be able to override the bus format specified in a
> panel's display_mode.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  .../bindings/display/panel/simple-panel.txt        |  2 ++
>  drivers/gpu/drm/panel/panel-simple.c               | 40 +++++++++++++++++++++-
>  2 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> index 1341bbf..e2308c3 100644
> --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> @@ -7,6 +7,8 @@ Optional properties:
>  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
>  - enable-gpios: GPIO pin to enable or disable the panel
>  - backlight: phandle of the backlight device attached to the panel
> +- bus-format-override: override the bus_format setting of the panel's
> +  display_mode settings

You need to define valid values here.

However, we also have this proposal[1]. Please align to a common one. 
BTW, we don't need another common panel binding file that [1] added. We 
already have panel-dpi.txt for parallel interface panels. And 
personally, I'd like to see simple-panel.txt disappear.

Rob

[1] https://patchwork.ozlabs.org/patch/823104/
Thierry Reding Oct. 17, 2017, 12:12 p.m. UTC | #2
On Wed, Oct 11, 2017 at 01:23:35PM +0200, Lothar Waßmann wrote:
> The baseboards for the Ka-Ro electronics series of i.MX modules
> use a 24bit LCD interface, no matter what LCD bus width the SoC on the
> module provides and what the LCD panel expects. LCDs with 6bit per color
> will ignore the 2 LSBs of each color lane, and modules using a SoC
> that provides only 6bit per color, drive the display information on the
> 6 MSBs of each color lane and tie the 2 LSBs of each color lane to GND.
> 
> Thus, no matter what combination of LCD and SoC is used, the LCD port
> can be used without shuffling bit lanes by always configuring the LCD
> output to 24bit mode.
> 
> Add a function to handle certain quirks of the LCD interface to the
> panel driver to be able to override the bus format specified in a
> panel's display_mode.

I think the above paragraph clearly indicates that this is the wrong
place to workaround this. You say yourself that the LCD interface has
quirks that need to be handled, so why do you want to force this
handling into the panel driver?

The panel remains the same, no matter what interface you connect it to.

Thierry
Lothar Waßmann Oct. 17, 2017, 12:44 p.m. UTC | #3
Hi,

On Tue, 17 Oct 2017 14:12:40 +0200 Thierry Reding wrote:
> On Wed, Oct 11, 2017 at 01:23:35PM +0200, Lothar Waßmann wrote:
> > The baseboards for the Ka-Ro electronics series of i.MX modules
> > use a 24bit LCD interface, no matter what LCD bus width the SoC on the
> > module provides and what the LCD panel expects. LCDs with 6bit per color
> > will ignore the 2 LSBs of each color lane, and modules using a SoC
> > that provides only 6bit per color, drive the display information on the
> > 6 MSBs of each color lane and tie the 2 LSBs of each color lane to GND.
> > 
> > Thus, no matter what combination of LCD and SoC is used, the LCD port
> > can be used without shuffling bit lanes by always configuring the LCD
> > output to 24bit mode.
> > 
> > Add a function to handle certain quirks of the LCD interface to the
> > panel driver to be able to override the bus format specified in a
> > panel's display_mode.
> 
> I think the above paragraph clearly indicates that this is the wrong
> place to workaround this. You say yourself that the LCD interface has
> quirks that need to be handled, so why do you want to force this
> handling into the panel driver?
> 
The quirk is in the interfacing of the SoM's LCD output to the LCD
panel. Thus it can be handled in either place.

> The panel remains the same, no matter what interface you connect it to.
> 
Because that's just ONE place to change, no matter what LCD driver is
being used.


Lothar Waßmann
Thierry Reding Oct. 17, 2017, 12:56 p.m. UTC | #4
On Tue, Oct 17, 2017 at 02:44:23PM +0200, Lothar Waßmann wrote:
> Hi,
> 
> On Tue, 17 Oct 2017 14:12:40 +0200 Thierry Reding wrote:
> > On Wed, Oct 11, 2017 at 01:23:35PM +0200, Lothar Waßmann wrote:
> > > The baseboards for the Ka-Ro electronics series of i.MX modules
> > > use a 24bit LCD interface, no matter what LCD bus width the SoC on the
> > > module provides and what the LCD panel expects. LCDs with 6bit per color
> > > will ignore the 2 LSBs of each color lane, and modules using a SoC
> > > that provides only 6bit per color, drive the display information on the
> > > 6 MSBs of each color lane and tie the 2 LSBs of each color lane to GND.
> > > 
> > > Thus, no matter what combination of LCD and SoC is used, the LCD port
> > > can be used without shuffling bit lanes by always configuring the LCD
> > > output to 24bit mode.
> > > 
> > > Add a function to handle certain quirks of the LCD interface to the
> > > panel driver to be able to override the bus format specified in a
> > > panel's display_mode.
> > 
> > I think the above paragraph clearly indicates that this is the wrong
> > place to workaround this. You say yourself that the LCD interface has
> > quirks that need to be handled, so why do you want to force this
> > handling into the panel driver?
> > 
> The quirk is in the interfacing of the SoM's LCD output to the LCD
> panel. Thus it can be handled in either place.

Yes. What I'm saying is that the panel is, in my opinion, the wrong
place to handle an LCD interface (originating from the SoM) quirk.

> > The panel remains the same, no matter what interface you connect it to.
> > 
> Because that's just ONE place to change, no matter what LCD driver is
> being used.

That's a Linux specific implementation detail. If you ever want to use
a panel that is not driven by simple-panel you'd have to change it in
that driver, too.

Thierry
Rob Herring Oct. 17, 2017, 5:11 p.m. UTC | #5
On Tue, Oct 17, 2017 at 7:56 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, Oct 17, 2017 at 02:44:23PM +0200, Lothar Waßmann wrote:
>> Hi,
>>
>> On Tue, 17 Oct 2017 14:12:40 +0200 Thierry Reding wrote:
>> > On Wed, Oct 11, 2017 at 01:23:35PM +0200, Lothar Waßmann wrote:
>> > > The baseboards for the Ka-Ro electronics series of i.MX modules
>> > > use a 24bit LCD interface, no matter what LCD bus width the SoC on the
>> > > module provides and what the LCD panel expects. LCDs with 6bit per color
>> > > will ignore the 2 LSBs of each color lane, and modules using a SoC
>> > > that provides only 6bit per color, drive the display information on the
>> > > 6 MSBs of each color lane and tie the 2 LSBs of each color lane to GND.
>> > >
>> > > Thus, no matter what combination of LCD and SoC is used, the LCD port
>> > > can be used without shuffling bit lanes by always configuring the LCD
>> > > output to 24bit mode.
>> > >
>> > > Add a function to handle certain quirks of the LCD interface to the
>> > > panel driver to be able to override the bus format specified in a
>> > > panel's display_mode.
>> >
>> > I think the above paragraph clearly indicates that this is the wrong
>> > place to workaround this. You say yourself that the LCD interface has
>> > quirks that need to be handled, so why do you want to force this
>> > handling into the panel driver?
>> >
>> The quirk is in the interfacing of the SoM's LCD output to the LCD
>> panel. Thus it can be handled in either place.
>
> Yes. What I'm saying is that the panel is, in my opinion, the wrong
> place to handle an LCD interface (originating from the SoM) quirk.

There's 2 possible things we could need to describe here. First, is
any restrictions the SoM has. For example, the SoM has an 18-bit
interface while the SoC has a 24-bit interface. Second, is how a panel
with fewer bits than the interface (whether a SoM or direct to SoC) is
wired up. If we think about this in terms of a base DT plus overlays,
then the former case belongs in the base and the latter belongs in the
overlay. If the SoM interface needs to be SoC independent, then it
gets more complicated as we don't want to have the SoC's LCD
controller node in the overlay adding properties to it and therefore
need to have some sort of translation (i.e. a connector binding). But
we have no definition of how we would describe OF graph thru a
connector.

In the end, these are board level properties, so you can make the same
argument that this property doesn't belong in the SoC's LCD controller
node as you can it doesn't belong in the panel node. Generally, we
just default to the node bound to the driver we want to handle the
property.

In any case, I think this should be properties of the endpoints rather
than the endpoint parent nodes as it is a property of the interface,
not the controller nor the panel. This is also how camera interfaces
were done. An LCD controller node could have 2 interfaces for example.

>> > The panel remains the same, no matter what interface you connect it to.
>> >
>> Because that's just ONE place to change, no matter what LCD driver is
>> being used.
>
> That's a Linux specific implementation detail. If you ever want to use
> a panel that is not driven by simple-panel you'd have to change it in
> that driver, too.

I do agree the implementation belongs in the LCD controller side, but
just because the property is in the panel node that doesn't mean it's
the panel driver that has to handle the property. As long as the
property is parse-able in a standard way, the "parent" can do it.
There are cases where the driver tied to the parent node handles
properties in the child nodes. SPI timing modes is one example. In
this case, I think we'll need to handle this property being in the
local endpoint and the remote endpoint and resolving the format needed
based on that (as well as what the panel driver says). That's going to
be needed with overlays and connectors anyways and then you don't
really have to care where the restriction comes from.

Rob
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
index 1341bbf..e2308c3 100644
--- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
+++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
@@ -7,6 +7,8 @@  Optional properties:
 - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
 - enable-gpios: GPIO pin to enable or disable the panel
 - backlight: phandle of the backlight device attached to the panel
+- bus-format-override: override the bus_format setting of the panel's
+  display_mode settings
 
 Example:
 
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index fde9c41..f356a7b 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -87,6 +87,8 @@  struct panel_simple {
 	struct i2c_adapter *ddc;
 
 	struct gpio_desc *enable_gpio;
+
+	u32 bus_fmt_override;
 };
 
 #define SP_DISPLAY_MODE(freq, ha, hfp, hs, hbp, va, vfp, vs, vbp, vr, flgs) { \
@@ -165,7 +167,11 @@  static int panel_simple_get_fixed_modes(struct panel_simple *panel)
 	connector->display_info.bpc = panel->desc->bpc;
 	connector->display_info.width_mm = panel->desc->size.width;
 	connector->display_info.height_mm = panel->desc->size.height;
-	if (panel->desc->bus_format)
+
+	if (panel->bus_fmt_override)
+		drm_display_info_set_bus_formats(&connector->display_info,
+						 &panel->bus_fmt_override, 1);
+	else if (panel->desc->bus_format)
 		drm_display_info_set_bus_formats(&connector->display_info,
 						 &panel->desc->bus_format, 1);
 	connector->display_info.bus_flags = panel->desc->bus_flags;
@@ -298,6 +304,34 @@  static int panel_simple_get_timings(struct drm_panel *panel,
 	return p->desc->num_timings;
 }
 
+static inline int panel_simple_check_quirks(struct device *dev,
+					    struct panel_simple *p)
+{
+	const char *bus_fmt;
+
+	if (of_property_read_string(dev->of_node, "bus-format-override",
+				    &bus_fmt) == 0) {
+		if (strcmp(bus_fmt, "rgb24") == 0)
+			p->bus_fmt_override = MEDIA_BUS_FMT_RGB888_1X24;
+		else if (strcmp(bus_fmt, "rgb666") == 0)
+			p->bus_fmt_override = MEDIA_BUS_FMT_RGB666_1X18;
+		else if (strcmp(bus_fmt, "rgb565") == 0)
+			p->bus_fmt_override = MEDIA_BUS_FMT_RGB565_1X16;
+		else if (strcmp(bus_fmt, "spwg-18") == 0)
+			p->bus_fmt_override = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG;
+		else if (strcmp(bus_fmt, "spwg-24") == 0)
+			p->bus_fmt_override = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
+		else if (strcmp(bus_fmt, "jeida-24") == 0)
+			p->bus_fmt_override = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
+		else
+			dev_err(dev,
+				"Unsupported bus-format-override value: '%s'\n",
+				bus_fmt);
+		return p->bus_fmt_override ? 0 : -EINVAL;
+	}
+	return 0;
+}
+
 static const struct drm_panel_funcs panel_simple_funcs = {
 	.disable = panel_simple_disable,
 	.unprepare = panel_simple_unprepare,
@@ -353,6 +387,10 @@  static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 		}
 	}
 
+	err = panel_simple_check_quirks(dev, panel);
+	if (err)
+		goto free_ddc;
+
 	drm_panel_init(&panel->base);
 	panel->base.dev = dev;
 	panel->base.funcs = &panel_simple_funcs;