Some panels need an active-low data enable (DE) signal for the RGB
interface. This requires flipping a bit in the TCON0 polarity register
when setting up the mode for the RGB interface.
Add a new helper function to match specific bus flags and use it to set
the polarity inversion bit for the DE signal when required.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++--
drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 3fb084f802e2..d249a462ec09 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -78,6 +78,24 @@ static int sun4i_tcon_get_pixel_depth(const struct drm_encoder *encoder)
return -EINVAL;
}
+static bool sun4i_tcon_match_bus_flags(const struct drm_encoder *encoder,
+ u32 bus_flags)
+{
+ struct drm_connector *connector;
+ struct drm_display_info *info;
+
+ connector = sun4i_tcon_get_connector(encoder);
+ if (!connector)
+ return false;
+
+ info = &connector->display_info;
+
+ if ((info->bus_flags & bus_flags) == bus_flags)
+ return true;
+
+ return false;
+}
+
static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
bool enabled)
{
@@ -415,6 +433,7 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
}
static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
+ const struct drm_encoder *encoder,
const struct drm_display_mode *mode)
{
unsigned int bp, hsync, vsync;
@@ -474,8 +493,13 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
if (mode->flags & DRM_MODE_FLAG_PVSYNC)
val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
+ if (sun4i_tcon_match_bus_flags(encoder, DRM_BUS_FLAG_DE_LOW))
+ val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
+
regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
- SUN4I_TCON0_IO_POL_HSYNC_POSITIVE | SUN4I_TCON0_IO_POL_VSYNC_POSITIVE,
+ SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
+ SUN4I_TCON0_IO_POL_VSYNC_POSITIVE |
+ SUN4I_TCON0_IO_POL_DE_NEGATIVE,
val);
/* Map output pins to channel 0 */
@@ -596,7 +620,7 @@ void sun4i_tcon_mode_set(struct sun4i_tcon *tcon,
sun4i_tcon0_mode_set_lvds(tcon, encoder, mode);
break;
case DRM_MODE_ENCODER_NONE:
- sun4i_tcon0_mode_set_rgb(tcon, mode);
+ sun4i_tcon0_mode_set_rgb(tcon, encoder, mode);
sun4i_tcon_set_mux(tcon, 0, encoder);
break;
case DRM_MODE_ENCODER_TVDAC:
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index f6a071cd5a6f..708399c80561 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -113,6 +113,7 @@
#define SUN4I_TCON0_IO_POL_REG 0x88
#define SUN4I_TCON0_IO_POL_DCLK_PHASE(phase) ((phase & 3) << 28)
+#define SUN4I_TCON0_IO_POL_DE_NEGATIVE BIT(27)
#define SUN4I_TCON0_IO_POL_HSYNC_POSITIVE BIT(25)
#define SUN4I_TCON0_IO_POL_VSYNC_POSITIVE BIT(24)
--
2.19.0
This adds support for the 3.5" LCD panel from Lemaker, sold for use with
BananaPi boards. It comes with a 24-bit RGB888 parallel interface and
requires an active-low DE signal
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/panel/panel-simple.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 97964f7f2ace..229080fcf65e 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1461,6 +1461,30 @@ static const struct panel_desc kyo_tcg121xglp = {
.bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
};
+static const struct drm_display_mode lemaker_bl035_mode = {
+ .clock = 7000,
+ .hdisplay = 320,
+ .hsync_start = 320 + 20,
+ .hsync_end = 320 + 20 + 30,
+ .htotal = 320 + 20 + 30 + 38,
+ .vdisplay = 240,
+ .vsync_start = 240 + 4,
+ .vsync_end = 240 + 4 + 3,
+ .vtotal = 240 + 4 + 3 + 15,
+ .vrefresh = 60,
+};
+
+static const struct panel_desc lemaker_bl035 = {
+ .modes = &lemaker_bl035_mode,
+ .num_modes = 1,
+ .size = {
+ .width = 70,
+ .height = 52,
+ },
+ .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
+ .bus_flags = DRM_BUS_FLAG_DE_LOW,
+};
+
static const struct drm_display_mode lg_lb070wv8_mode = {
.clock = 33246,
.hdisplay = 800,
@@ -2456,6 +2480,9 @@ static const struct of_device_id platform_of_match[] = {
}, {
.compatible = "kyo,tcg121xglp",
.data = &kyo_tcg121xglp,
+ }, {
+ .compatible = "lemaker,bl035",
+ .data = &lemaker_bl035,
}, {
.compatible = "lg,lb070wv8",
.data = &lg_lb070wv8,
--
2.19.0
This adds the backlight panel, power, pwm and tcon0 device-tree bindings
required for supporting the 3.5" LCD from Lemaker on the BananaPi M1.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
arch/arm/boot/dts/sun7i-a20-bananapi.dts | 89 ++++++++++++++++++++++++
1 file changed, 89 insertions(+)
diff --git a/arch/arm/boot/dts/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
index 70dfc4ac0bb5..dd2f8bc41fae 100644
--- a/arch/arm/boot/dts/sun7i-a20-bananapi.dts
+++ b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
@@ -48,6 +48,7 @@
#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/pwm/pwm.h>
/ {
model = "LeMaker Banana Pi";
@@ -63,6 +64,75 @@
stdout-path = "serial0:115200n8";
};
+
+ backlight: backlight {
+ compatible = "pwm-backlight";
+ pwms = <&pwm 0 50000 0>;
+ brightness-levels = < 0 1 1 1 1 2 2 2
+ 2 3 3 3 3 4 4 4
+ 5 5 5 6 6 6 7 7
+ 8 8 8 9 9 9 10 10
+ 10 11 11 12 12 12 13 13
+ 14 14 14 15 15 16 16 17
+ 17 17 18 18 19 19 20 20
+ 21 21 21 22 22 23 23 24
+ 24 25 25 26 26 27 27 28
+ 28 29 30 30 31 31 32 32
+ 33 33 34 35 35 36 36 37
+ 38 38 39 39 40 41 41 42
+ 43 43 44 44 45 46 47 47
+ 48 49 49 50 51 51 52 53
+ 54 54 55 56 57 57 58 59
+ 60 61 61 62 63 64 65 65
+ 66 67 68 69 70 71 71 72
+ 73 74 75 76 77 78 79 80
+ 81 82 83 84 85 86 87 88
+ 89 90 91 92 93 94 95 96
+ 97 98 99 101 102 103 104 105
+ 106 108 109 110 111 112 114 115
+ 116 117 119 120 121 123 124 125
+ 127 128 129 131 132 133 135 136
+ 138 139 141 142 144 145 147 148
+ 150 151 153 154 156 157 159 161
+ 162 164 166 167 169 171 173 174
+ 176 178 180 181 183 185 187 189
+ 191 192 194 196 198 200 202 204
+ 206 208 210 212 214 216 219 221
+ 223 225 227 229 232 234 236 238
+ 241 242 244 246 248 250 253 255>;
+ default-brightness-level = <128>;
+ enable-gpios = <&pio 7 8 GPIO_ACTIVE_HIGH>; /* PH8 */
+ };
+
+ panel: panel {
+ compatible = "lemaker,bl035";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ power-supply = <&panel_power>;
+ backlight = <&backlight>;
+
+ port@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ panel_input: endpoint@0 {
+ reg = <0>;
+ remote-endpoint = <&tcon0_out_panel>;
+ };
+ };
+ };
+
+ panel_power: panel_power {
+ compatible = "regulator-fixed";
+ regulator-name = "panel-power";
+ regulator-min-microvolt = <10400000>;
+ regulator-max-microvolt = <10400000>;
+ gpio = <&pio 7 12 GPIO_ACTIVE_HIGH>; /* PH12 */
+ enable-active-high;
+ regulator-boot-on;
+ };
+
hdmi-connector {
compatible = "hdmi-connector";
type = "a";
@@ -275,6 +345,12 @@
};
};
+&pwm {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pwm0_pins_a>;
+ status = "okay";
+};
+
#include "axp209.dtsi"
®_dcdc2 {
@@ -322,6 +398,19 @@
status = "okay";
};
+&tcon0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&lcd0_rgb888_pins>;
+ status = "okay";
+};
+
+&tcon0_out {
+ tcon0_out_panel: endpoint@0 {
+ reg = <0>;
+ remote-endpoint = <&panel_input>;
+ };
+};
+
&uart0 {
pinctrl-names = "default";
pinctrl-0 = <&uart0_pins_a>;
--
2.19.0
This adds the pin muxing definition for configuring the PD pins in LCD0
mode for a RGB888 format to the sun7i device-tree.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
arch/arm/boot/dts/sun7i-a20.dtsi | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 02e40da9f028..684dd008039d 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -837,6 +837,17 @@
function = "ir1";
};
+ lcd0_rgb888_pins: lcd0-rgb888 {
+ pins = "PD0", "PD1", "PD2", "PD3",
+ "PD4", "PD5", "PD6", "PD7",
+ "PD8", "PD9", "PD10", "PD11",
+ "PD12", "PD13", "PD14", "PD15",
+ "PD16", "PD17", "PD18", "PD19",
+ "PD20", "PD21", "PD22", "PD23",
+ "PD24", "PD25", "PD26", "PD27";
+ function = "lcd0";
+ };
+
mmc0_pins_a: mmc0@0 {
pins = "PF0", "PF1", "PF2",
"PF3", "PF4", "PF5";
--
2.19.0
On Wed, Oct 10, 2018 at 01:41:31PM +0200, Paul Kocialkowski wrote:
> Some panels need an active-low data enable (DE) signal for the RGB
> interface. This requires flipping a bit in the TCON0 polarity register
> when setting up the mode for the RGB interface.
>
> Add a new helper function to match specific bus flags and use it to set
> the polarity inversion bit for the DE signal when required.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++--
> drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
> 2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 3fb084f802e2..d249a462ec09 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -78,6 +78,24 @@ static int sun4i_tcon_get_pixel_depth(const struct drm_encoder *encoder)
> return -EINVAL;
> }
>
> +static bool sun4i_tcon_match_bus_flags(const struct drm_encoder *encoder,
> + u32 bus_flags)
> +{
> + struct drm_connector *connector;
> + struct drm_display_info *info;
> +
> + connector = sun4i_tcon_get_connector(encoder);
> + if (!connector)
> + return false;
> +
> + info = &connector->display_info;
> +
> + if ((info->bus_flags & bus_flags) == bus_flags)
> + return true;
> +
> + return false;
> +}
> +
> static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
> bool enabled)
> {
> @@ -415,6 +433,7 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
> }
>
> static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> + const struct drm_encoder *encoder,
> const struct drm_display_mode *mode)
> {
> unsigned int bp, hsync, vsync;
> @@ -474,8 +493,13 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>
> + if (sun4i_tcon_match_bus_flags(encoder, DRM_BUS_FLAG_DE_LOW))
> + val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
> +
There's other flags in use in that function, you should also migrate
them (ideally by splitting that new function into a separate patch).
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Oct 10, 2018 at 01:41:32PM +0200, Paul Kocialkowski wrote:
> This adds support for the 3.5" LCD panel from Lemaker, sold for use with
> BananaPi boards. It comes with a 24-bit RGB888 parallel interface and
> requires an active-low DE signal
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/gpu/drm/panel/panel-simple.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 97964f7f2ace..229080fcf65e 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1461,6 +1461,30 @@ static const struct panel_desc kyo_tcg121xglp = {
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> };
>
> +static const struct drm_display_mode lemaker_bl035_mode = {
> + .clock = 7000,
> + .hdisplay = 320,
> + .hsync_start = 320 + 20,
> + .hsync_end = 320 + 20 + 30,
> + .htotal = 320 + 20 + 30 + 38,
> + .vdisplay = 240,
> + .vsync_start = 240 + 4,
> + .vsync_end = 240 + 4 + 3,
> + .vtotal = 240 + 4 + 3 + 15,
> + .vrefresh = 60,
> +};
> +
> +static const struct panel_desc lemaker_bl035 = {
> + .modes = &lemaker_bl035_mode,
> + .num_modes = 1,
> + .size = {
> + .width = 70,
> + .height = 52,
> + },
> + .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
> + .bus_flags = DRM_BUS_FLAG_DE_LOW,
> +};
> +
> static const struct drm_display_mode lg_lb070wv8_mode = {
> .clock = 33246,
> .hdisplay = 800,
> @@ -2456,6 +2480,9 @@ static const struct of_device_id platform_of_match[] = {
> }, {
> .compatible = "kyo,tcg121xglp",
> .data = &kyo_tcg121xglp,
> + }, {
> + .compatible = "lemaker,bl035",
> + .data = &lemaker_bl035,
You should document that new compatible. Also, where is this name
coming from? Is it the name it's sold under? something you came up
with?
Thanks,
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Oct 10, 2018 at 01:41:33PM +0200, Paul Kocialkowski wrote:
> This adds the pin muxing definition for configuring the PD pins in LCD0
> mode for a RGB888 format to the sun7i device-tree.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
OUr policy so far has been that if there's no user for that pin group,
we won't merge it in order not to cripple the DTSI with useless groups.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi,
Le mercredi 10 octobre 2018 à 16:58 +0200, Maxime Ripard a écrit :
> On Wed, Oct 10, 2018 at 01:41:32PM +0200, Paul Kocialkowski wrote:
> > This adds support for the 3.5" LCD panel from Lemaker, sold for use with
> > BananaPi boards. It comes with a 24-bit RGB888 parallel interface and
> > requires an active-low DE signal
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/gpu/drm/panel/panel-simple.c | 27 +++++++++++++++++++++++++++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index 97964f7f2ace..229080fcf65e 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -1461,6 +1461,30 @@ static const struct panel_desc kyo_tcg121xglp = {
> > .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> > };
> >
> > +static const struct drm_display_mode lemaker_bl035_mode = {
> > + .clock = 7000,
> > + .hdisplay = 320,
> > + .hsync_start = 320 + 20,
> > + .hsync_end = 320 + 20 + 30,
> > + .htotal = 320 + 20 + 30 + 38,
> > + .vdisplay = 240,
> > + .vsync_start = 240 + 4,
> > + .vsync_end = 240 + 4 + 3,
> > + .vtotal = 240 + 4 + 3 + 15,
> > + .vrefresh = 60,
> > +};
> > +
> > +static const struct panel_desc lemaker_bl035 = {
> > + .modes = &lemaker_bl035_mode,
> > + .num_modes = 1,
> > + .size = {
> > + .width = 70,
> > + .height = 52,
> > + },
> > + .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
> > + .bus_flags = DRM_BUS_FLAG_DE_LOW,
> > +};
> > +
> > static const struct drm_display_mode lg_lb070wv8_mode = {
> > .clock = 33246,
> > .hdisplay = 800,
> > @@ -2456,6 +2480,9 @@ static const struct of_device_id platform_of_match[] = {
> > }, {
> > .compatible = "kyo,tcg121xglp",
> > .data = &kyo_tcg121xglp,
> > + }, {
> > + .compatible = "lemaker,bl035",
> > + .data = &lemaker_bl035,
>
> You should document that new compatible. Also, where is this name
> coming from? Is it the name it's sold under? something you came up
> with?
Oh right, I forgot to document the compatible (and add the new vendor).
The name "BL035" comes from the PCB itself, where it reads "BL035-RGB-
002" under the Lemaker logo. I went for "BL035" to keep it short.
The latter part seems to be the PCB revision. According to the fex
files[0], the timings are the same for all the screens of the same size
(regardless of revision), so that revision probably shouldn't be in the
panel name.
As for the "RGB" part, Lemaker only makes 3.5 and 5 inch LCDs with a
parallel RGB interface and 7 and 10.1 inch LCDs with LVDS, so there is
no ambiguity for now.
Do you think it makes sense to keep that part in the panel name
nevertheless?
[0]: https://github.com/LeMaker/fex_configuration
Cheers,
Paul
--
Developer of free digital technology and hardware support.
Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
Hi,
Le mercredi 10 octobre 2018 à 16:57 +0200, Maxime Ripard a écrit :
> On Wed, Oct 10, 2018 at 01:41:31PM +0200, Paul Kocialkowski wrote:
> > Some panels need an active-low data enable (DE) signal for the RGB
> > interface. This requires flipping a bit in the TCON0 polarity register
> > when setting up the mode for the RGB interface.
> >
> > Add a new helper function to match specific bus flags and use it to set
> > the polarity inversion bit for the DE signal when required.
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++--
> > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
> > 2 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index 3fb084f802e2..d249a462ec09 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -78,6 +78,24 @@ static int sun4i_tcon_get_pixel_depth(const struct drm_encoder *encoder)
> > return -EINVAL;
> > }
> >
> > +static bool sun4i_tcon_match_bus_flags(const struct drm_encoder *encoder,
> > + u32 bus_flags)
> > +{
> > + struct drm_connector *connector;
> > + struct drm_display_info *info;
> > +
> > + connector = sun4i_tcon_get_connector(encoder);
> > + if (!connector)
> > + return false;
> > +
> > + info = &connector->display_info;
> > +
> > + if ((info->bus_flags & bus_flags) == bus_flags)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
> > bool enabled)
> > {
> > @@ -415,6 +433,7 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
> > }
> >
> > static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> > + const struct drm_encoder *encoder,
> > const struct drm_display_mode *mode)
> > {
> > unsigned int bp, hsync, vsync;
> > @@ -474,8 +493,13 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> > if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> >
> > + if (sun4i_tcon_match_bus_flags(encoder, DRM_BUS_FLAG_DE_LOW))
> > + val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
> > +
>
> There's other flags in use in that function, you should also migrate
> them (ideally by splitting that new function into a separate patch).
Actually, these other flags are not DRM bus flags but DRM mode flags,
which can be accessed directly from the mode pointer passed as
argument. Thus, they don't require a helper.
If you'd like, I could still split this patch into one introducing the
helper and one using it for checking the DE_LOW flag.
What do you think?
Cheers,
Paul
--
Developer of free digital technology and hardware support.
Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
On Tue, Oct 23, 2018 at 11:33:10AM +0200, Paul Kocialkowski wrote:
> Hi,
>
> Le mercredi 10 octobre 2018 ? 16:57 +0200, Maxime Ripard a ?crit :
> > On Wed, Oct 10, 2018 at 01:41:31PM +0200, Paul Kocialkowski wrote:
> > > Some panels need an active-low data enable (DE) signal for the RGB
> > > interface. This requires flipping a bit in the TCON0 polarity register
> > > when setting up the mode for the RGB interface.
> > >
> > > Add a new helper function to match specific bus flags and use it to set
> > > the polarity inversion bit for the DE signal when required.
> > >
> > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > ---
> > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++--
> > > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
> > > 2 files changed, 27 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > index 3fb084f802e2..d249a462ec09 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > @@ -78,6 +78,24 @@ static int sun4i_tcon_get_pixel_depth(const struct drm_encoder *encoder)
> > > return -EINVAL;
> > > }
> > >
> > > +static bool sun4i_tcon_match_bus_flags(const struct drm_encoder *encoder,
> > > + u32 bus_flags)
> > > +{
> > > + struct drm_connector *connector;
> > > + struct drm_display_info *info;
> > > +
> > > + connector = sun4i_tcon_get_connector(encoder);
> > > + if (!connector)
> > > + return false;
> > > +
> > > + info = &connector->display_info;
> > > +
> > > + if ((info->bus_flags & bus_flags) == bus_flags)
> > > + return true;
> > > +
> > > + return false;
> > > +}
> > > +
> > > static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
> > > bool enabled)
> > > {
> > > @@ -415,6 +433,7 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
> > > }
> > >
> > > static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> > > + const struct drm_encoder *encoder,
> > > const struct drm_display_mode *mode)
> > > {
> > > unsigned int bp, hsync, vsync;
> > > @@ -474,8 +493,13 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> > > if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > > val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> > >
> > > + if (sun4i_tcon_match_bus_flags(encoder, DRM_BUS_FLAG_DE_LOW))
> > > + val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
> > > +
> >
> > There's other flags in use in that function, you should also migrate
> > them (ideally by splitting that new function into a separate patch).
>
> Actually, these other flags are not DRM bus flags but DRM mode flags,
> which can be accessed directly from the mode pointer passed as
> argument. Thus, they don't require a helper.
I was talking about DRM_BUS_FLAG_PIXDATA_POSEDGE and
DRM_BUS_FLAG_PIXDATA_NEGEDGE, which are in the current tree.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Oct 23, 2018 at 12:08:17PM +0200, Paul Kocialkowski wrote:
> Hi,
>
> Le mercredi 10 octobre 2018 ? 16:58 +0200, Maxime Ripard a ?crit :
> > On Wed, Oct 10, 2018 at 01:41:32PM +0200, Paul Kocialkowski wrote:
> > > This adds support for the 3.5" LCD panel from Lemaker, sold for use with
> > > BananaPi boards. It comes with a 24-bit RGB888 parallel interface and
> > > requires an active-low DE signal
> > >
> > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > ---
> > > drivers/gpu/drm/panel/panel-simple.c | 27 +++++++++++++++++++++++++++
> > > 1 file changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index 97964f7f2ace..229080fcf65e 100644
> > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > @@ -1461,6 +1461,30 @@ static const struct panel_desc kyo_tcg121xglp = {
> > > .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> > > };
> > >
> > > +static const struct drm_display_mode lemaker_bl035_mode = {
> > > + .clock = 7000,
> > > + .hdisplay = 320,
> > > + .hsync_start = 320 + 20,
> > > + .hsync_end = 320 + 20 + 30,
> > > + .htotal = 320 + 20 + 30 + 38,
> > > + .vdisplay = 240,
> > > + .vsync_start = 240 + 4,
> > > + .vsync_end = 240 + 4 + 3,
> > > + .vtotal = 240 + 4 + 3 + 15,
> > > + .vrefresh = 60,
> > > +};
> > > +
> > > +static const struct panel_desc lemaker_bl035 = {
> > > + .modes = &lemaker_bl035_mode,
> > > + .num_modes = 1,
> > > + .size = {
> > > + .width = 70,
> > > + .height = 52,
> > > + },
> > > + .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
> > > + .bus_flags = DRM_BUS_FLAG_DE_LOW,
> > > +};
> > > +
> > > static const struct drm_display_mode lg_lb070wv8_mode = {
> > > .clock = 33246,
> > > .hdisplay = 800,
> > > @@ -2456,6 +2480,9 @@ static const struct of_device_id platform_of_match[] = {
> > > }, {
> > > .compatible = "kyo,tcg121xglp",
> > > .data = &kyo_tcg121xglp,
> > > + }, {
> > > + .compatible = "lemaker,bl035",
> > > + .data = &lemaker_bl035,
> >
> > You should document that new compatible. Also, where is this name
> > coming from? Is it the name it's sold under? something you came up
> > with?
>
> Oh right, I forgot to document the compatible (and add the new vendor).
>
> The name "BL035" comes from the PCB itself, where it reads "BL035-RGB-
> 002" under the Lemaker logo. I went for "BL035" to keep it short.
>
> The latter part seems to be the PCB revision. According to the fex
> files[0], the timings are the same for all the screens of the same size
> (regardless of revision), so that revision probably shouldn't be in the
> panel name.
>
> As for the "RGB" part, Lemaker only makes 3.5 and 5 inch LCDs with a
> parallel RGB interface and 7 and 10.1 inch LCDs with LVDS, so there is
> no ambiguity for now.
>
> Do you think it makes sense to keep that part in the panel name
> nevertheless?
I'd just put the full reference, to be on the safe side.
Thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi,
Le mercredi 24 octobre 2018 à 17:47 +0100, Maxime Ripard a écrit :
> On Tue, Oct 23, 2018 at 11:33:10AM +0200, Paul Kocialkowski wrote:
> > Hi,
> >
> > Le mercredi 10 octobre 2018 à 16:57 +0200, Maxime Ripard a écrit :
> > > On Wed, Oct 10, 2018 at 01:41:31PM +0200, Paul Kocialkowski wrote:
> > > > Some panels need an active-low data enable (DE) signal for the RGB
> > > > interface. This requires flipping a bit in the TCON0 polarity register
> > > > when setting up the mode for the RGB interface.
> > > >
> > > > Add a new helper function to match specific bus flags and use it to set
> > > > the polarity inversion bit for the DE signal when required.
> > > >
> > > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++--
> > > > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
> > > > 2 files changed, 27 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > index 3fb084f802e2..d249a462ec09 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > @@ -78,6 +78,24 @@ static int sun4i_tcon_get_pixel_depth(const struct drm_encoder *encoder)
> > > > return -EINVAL;
> > > > }
> > > >
> > > > +static bool sun4i_tcon_match_bus_flags(const struct drm_encoder *encoder,
> > > > + u32 bus_flags)
> > > > +{
> > > > + struct drm_connector *connector;
> > > > + struct drm_display_info *info;
> > > > +
> > > > + connector = sun4i_tcon_get_connector(encoder);
> > > > + if (!connector)
> > > > + return false;
> > > > +
> > > > + info = &connector->display_info;
> > > > +
> > > > + if ((info->bus_flags & bus_flags) == bus_flags)
> > > > + return true;
> > > > +
> > > > + return false;
> > > > +}
> > > > +
> > > > static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
> > > > bool enabled)
> > > > {
> > > > @@ -415,6 +433,7 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
> > > > }
> > > >
> > > > static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> > > > + const struct drm_encoder *encoder,
> > > > const struct drm_display_mode *mode)
> > > > {
> > > > unsigned int bp, hsync, vsync;
> > > > @@ -474,8 +493,13 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> > > > if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > > > val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> > > >
> > > > + if (sun4i_tcon_match_bus_flags(encoder, DRM_BUS_FLAG_DE_LOW))
> > > > + val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
> > > > +
> > >
> > > There's other flags in use in that function, you should also migrate
> > > them (ideally by splitting that new function into a separate patch).
> >
> > Actually, these other flags are not DRM bus flags but DRM mode flags,
> > which can be accessed directly from the mode pointer passed as
> > argument. Thus, they don't require a helper.
>
> I was talking about DRM_BUS_FLAG_PIXDATA_POSEDGE and
> DRM_BUS_FLAG_PIXDATA_NEGEDGE, which are in the current tree.
Thanks, I guess I need to rebase these patches atop the current tree!
Looking at how these flags are used in the current tree, I see that the
connector's display_info structure (that contains the bus flags) is
retreived from tcon->panel.
Would it make sense to change this and retreive it from the encoder,
passed as argument to that function (as I've done in this patch)
instead? This way, this wouldn't only apply to panels, but also to
external bridges. It would also somewhat streamline the logic.
What do you think?
Cheers,
Paul
--
Developer of free digital technology and hardware support.
Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
On Wed, Oct 24, 2018 at 07:24:32PM +0200, Paul Kocialkowski wrote:
> Hi,
>
> Le mercredi 24 octobre 2018 ? 17:47 +0100, Maxime Ripard a ?crit :
> > On Tue, Oct 23, 2018 at 11:33:10AM +0200, Paul Kocialkowski wrote:
> > > Hi,
> > >
> > > Le mercredi 10 octobre 2018 ? 16:57 +0200, Maxime Ripard a ?crit :
> > > > On Wed, Oct 10, 2018 at 01:41:31PM +0200, Paul Kocialkowski wrote:
> > > > > Some panels need an active-low data enable (DE) signal for the RGB
> > > > > interface. This requires flipping a bit in the TCON0 polarity register
> > > > > when setting up the mode for the RGB interface.
> > > > >
> > > > > Add a new helper function to match specific bus flags and use it to set
> > > > > the polarity inversion bit for the DE signal when required.
> > > > >
> > > > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > > > ---
> > > > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++--
> > > > > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
> > > > > 2 files changed, 27 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > index 3fb084f802e2..d249a462ec09 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > @@ -78,6 +78,24 @@ static int sun4i_tcon_get_pixel_depth(const struct drm_encoder *encoder)
> > > > > return -EINVAL;
> > > > > }
> > > > >
> > > > > +static bool sun4i_tcon_match_bus_flags(const struct drm_encoder *encoder,
> > > > > + u32 bus_flags)
> > > > > +{
> > > > > + struct drm_connector *connector;
> > > > > + struct drm_display_info *info;
> > > > > +
> > > > > + connector = sun4i_tcon_get_connector(encoder);
> > > > > + if (!connector)
> > > > > + return false;
> > > > > +
> > > > > + info = &connector->display_info;
> > > > > +
> > > > > + if ((info->bus_flags & bus_flags) == bus_flags)
> > > > > + return true;
> > > > > +
> > > > > + return false;
> > > > > +}
> > > > > +
> > > > > static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
> > > > > bool enabled)
> > > > > {
> > > > > @@ -415,6 +433,7 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
> > > > > }
> > > > >
> > > > > static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> > > > > + const struct drm_encoder *encoder,
> > > > > const struct drm_display_mode *mode)
> > > > > {
> > > > > unsigned int bp, hsync, vsync;
> > > > > @@ -474,8 +493,13 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> > > > > if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > > > > val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> > > > >
> > > > > + if (sun4i_tcon_match_bus_flags(encoder, DRM_BUS_FLAG_DE_LOW))
> > > > > + val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
> > > > > +
> > > >
> > > > There's other flags in use in that function, you should also migrate
> > > > them (ideally by splitting that new function into a separate patch).
> > >
> > > Actually, these other flags are not DRM bus flags but DRM mode flags,
> > > which can be accessed directly from the mode pointer passed as
> > > argument. Thus, they don't require a helper.
> >
> > I was talking about DRM_BUS_FLAG_PIXDATA_POSEDGE and
> > DRM_BUS_FLAG_PIXDATA_NEGEDGE, which are in the current tree.
>
> Thanks, I guess I need to rebase these patches atop the current tree!
>
> Looking at how these flags are used in the current tree, I see that the
> connector's display_info structure (that contains the bus flags) is
> retreived from tcon->panel.
>
> Would it make sense to change this and retreive it from the encoder,
> passed as argument to that function (as I've done in this patch)
> instead? This way, this wouldn't only apply to panels, but also to
> external bridges. It would also somewhat streamline the logic.
>
> What do you think?
That looks like a good idea :)
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com