[PATCH 00/10] drm/sun4i: Allwinner MIPI-DSI Burst mode support

[PATCH 00/10] drm/sun4i: Allwinner MIPI-DSI Burst mode support

[Jagan Teki]

This series support MIPI-DSI Burst mode on Allwinner platform, which
is tested in burst supported panel in Pine64-LTS board.

Series depends on previous A64 MIPI-DSI series[1] and all changes are
available in [2] repo with WIPI-A64-DSI branch.

Any inputs,
Jagan.

[2] https://github.com/amarula/linux-amarula
[1] https://patchwork.kernel.org/cover/10657467/

Jagan Teki (10):
  drm/sun4i: sun6i_mipi_dsi: Compute burst mode loop N1 instruction
    delay
  drm/sun4i: sun6i_mipi_dsi: Support instruction loop selection
  drm/sun4i: sun6i_mipi_dsi: Setup burst mode timings
  drm/sun4i: sun6i_mipi_dsi: Setup burst mode
  drm/sun4i: sun6i_mipi_dsi: Enable burst mode
  drm/sun4i: sun6i_mipi_dsi: Enable 2byte trail for 4-lane burst mode
  drm/sun4i: sun6i_mipi_dsi: Enable burst mode HBP, HSA_HSE
  dt-bindings: panel: Add Feiyang FY07024DI26A30-D MIPI-DSI LCD panel
  drm/panel: Add Feiyang FY07024DI26A30-D MIPI-DSI LCD panel
  [DO NOT MERGE] arm64: allwinner: a64: pine64-lts: Enable Feiyang FY07024DI26A30-D DSI
    panel

 .../display/panel/feiyang,fy07024di26a30d.txt |  20 ++
 .../dts/allwinner/sun50i-a64-pine64-lts.dts   |  37 +++
 drivers/gpu/drm/panel/Kconfig                 |   9 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../drm/panel/panel-feiyang-fy07024di26a30d.c | 305 ++++++++++++++++++
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c        | 186 +++++++----
 6 files changed, 500 insertions(+), 58 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/panel/feiyang,fy07024di26a30d.txt
 create mode 100644 drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c

-- 
2.18.0.321.gffc6fa0e3

[PATCH 01/10] drm/sun4i: sun6i_mipi_dsi: Compute burst mode loop N1 instruction delay

[Jagan Teki]

Loop N1 instruction delay for burst mode lcd panel are
computed as per BSP code.

Reference code is available in BSP
(in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
dsi_dev[sel]->dsi_inst_loop_num.bits.loop_n1=
(panel->lcd_ht-panel->lcd_x)*(150)/(panel->lcd_dclk_freq*8) - 50;
=> (((mode->htotal - mode->hdisplay) * 150) / ((mode->clock / 1000) * 8)) - 50;

So use the similar computation for loop N1 delay.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 86430efd9054..da152c21ec62 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -394,7 +394,14 @@ static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
 static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
 				      struct drm_display_mode *mode)
 {
-	u16 delay = 50 - 1;
+	struct mipi_dsi_device *device = dsi->device;
+	u16 delay;
+
+	if (device->mode_flags == MIPI_DSI_MODE_VIDEO_BURST)
+		delay = (((mode->htotal - mode->hdisplay) * 150) /
+			((mode->clock / 1000) * 8)) - 50;
+	else
+		delay = 50 - 1;
 
 	regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_NUM_REG(0),
 		     SUN6I_DSI_INST_LOOP_NUM_N0(50 - 1) |
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 01/10] drm/sun4i: sun6i_mipi_dsi: Compute burst mode loop N1 instruction delay

[Sergey Suloev]

Hi, Jagan,

On 11/3/18 1:08 PM, Jagan Teki wrote:
> Loop N1 instruction delay for burst mode lcd panel are
> computed as per BSP code.
>
> Reference code is available in BSP
> (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> dsi_dev[sel]->dsi_inst_loop_num.bits.loop_n1=
> (panel->lcd_ht-panel->lcd_x)*(150)/(panel->lcd_dclk_freq*8) - 50;
> => (((mode->htotal - mode->hdisplay) * 150) / ((mode->clock / 1000) * 8)) - 50;
>
> So use the similar computation for loop N1 delay.
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>   drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 86430efd9054..da152c21ec62 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -394,7 +394,14 @@ static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
>   static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
>   				      struct drm_display_mode *mode)
>   {
> -	u16 delay = 50 - 1;
> +	struct mipi_dsi_device *device = dsi->device;
> +	u16 delay;
> +
> +	if (device->mode_flags == MIPI_DSI_MODE_VIDEO_BURST)
> +		delay = (((mode->htotal - mode->hdisplay) * 150) /
> +			((mode->clock / 1000) * 8)) - 50;
> +	else
> +		delay = 50 - 1;
>   
>   	regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_NUM_REG(0),
>   		     SUN6I_DSI_INST_LOOP_NUM_N0(50 - 1) |


is this patch series comes in addition to the previous MIPI DSI series 
or in its stead?

Thanks

Re: [PATCH 01/10] drm/sun4i: sun6i_mipi_dsi: Compute burst mode loop N1 instruction delay

[Jagan Teki]

On Sat, Nov 3, 2018 at 8:53 PM Sergey Suloev <ssuloev@orpaltech.com> wrote:
>
> Hi, Jagan,
>
> On 11/3/18 1:08 PM, Jagan Teki wrote:
> > Loop N1 instruction delay for burst mode lcd panel are
> > computed as per BSP code.
> >
> > Reference code is available in BSP
> > (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > dsi_dev[sel]->dsi_inst_loop_num.bits.loop_n1=
> > (panel->lcd_ht-panel->lcd_x)*(150)/(panel->lcd_dclk_freq*8) - 50;
> > => (((mode->htotal - mode->hdisplay) * 150) / ((mode->clock / 1000) * 8)) - 50;
> >
> > So use the similar computation for loop N1 delay.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >   drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index 86430efd9054..da152c21ec62 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -394,7 +394,14 @@ static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
> >   static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
> >                                     struct drm_display_mode *mode)
> >   {
> > -     u16 delay = 50 - 1;
> > +     struct mipi_dsi_device *device = dsi->device;
> > +     u16 delay;
> > +
> > +     if (device->mode_flags == MIPI_DSI_MODE_VIDEO_BURST)
> > +             delay = (((mode->htotal - mode->hdisplay) * 150) /
> > +                     ((mode->clock / 1000) * 8)) - 50;
> > +     else
> > +             delay = 50 - 1;
> >
> >       regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_NUM_REG(0),
> >                    SUN6I_DSI_INST_LOOP_NUM_N0(50 - 1) |
>
>
> is this patch series comes in addition to the previous MIPI DSI series
> or in its stead?

Yes, all changes were added on top of previous MIPI DSI series(ie what
I mentioned in cover-letter) since it require clock patches from
previous version.

Re: [linux-sunxi] [PATCH 01/10] drm/sun4i: sun6i_mipi_dsi: Compute burst mode loop N1 instruction delay

[Priit Laes]

On Sat, Nov 03, 2018 at 03:38:51PM +0530, Jagan Teki wrote:
> Loop N1 instruction delay for burst mode lcd panel are
> computed as per BSP code.
> 
> Reference code is available in BSP
> (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> dsi_dev[sel]->dsi_inst_loop_num.bits.loop_n1=
> (panel->lcd_ht-panel->lcd_x)*(150)/(panel->lcd_dclk_freq*8) - 50;
> => (((mode->htotal - mode->hdisplay) * 150) / ((mode->clock / 1000) * 8)) - 50;
> 
> So use the similar computation for loop N1 delay.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 86430efd9054..da152c21ec62 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -394,7 +394,14 @@ static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
>  static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
>  				      struct drm_display_mode *mode)
>  {
> -	u16 delay = 50 - 1;
> +	struct mipi_dsi_device *device = dsi->device;
> +	u16 delay;
> +
> +	if (device->mode_flags == MIPI_DSI_MODE_VIDEO_BURST)

This looks fishy, as mode_flags is supposedly bitfield.

I guess you actually want mode_flags & MIPI_DSI_MODE_VIDEO_BURST. Other
patches are also affected by this.


> +		delay = (((mode->htotal - mode->hdisplay) * 150) /
> +			((mode->clock / 1000) * 8)) - 50;
> +	else
> +		delay = 50 - 1;
>  
>  	regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_NUM_REG(0),
>  		     SUN6I_DSI_INST_LOOP_NUM_N0(50 - 1) |
> -- 
> 2.18.0.321.gffc6fa0e3
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 01/10] drm/sun4i: sun6i_mipi_dsi: Compute burst mode loop N1 instruction delay

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

On Sat, Nov 03, 2018 at 03:38:51PM +0530, Jagan Teki wrote:
> Loop N1 instruction delay for burst mode lcd panel are
> computed as per BSP code.
> 
> Reference code is available in BSP
> (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> dsi_dev[sel]->dsi_inst_loop_num.bits.loop_n1=
> (panel->lcd_ht-panel->lcd_x)*(150)/(panel->lcd_dclk_freq*8) - 50;
> => (((mode->htotal - mode->hdisplay) * 150) / ((mode->clock / 1000) * 8)) - 50;
> 
> So use the similar computation for loop N1 delay.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 86430efd9054..da152c21ec62 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -394,7 +394,14 @@ static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
>  static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
>  				      struct drm_display_mode *mode)
>  {
> -	u16 delay = 50 - 1;
> +	struct mipi_dsi_device *device = dsi->device;
> +	u16 delay;
> +
> +	if (device->mode_flags == MIPI_DSI_MODE_VIDEO_BURST)
> +		delay = (((mode->htotal - mode->hdisplay) * 150) /
> +			((mode->clock / 1000) * 8)) - 50;
> +	else
> +		delay = 50 - 1;

Apart from the other comments, I'd really prefer to have a function
here that would take the mipi_dsi_device and compute the delay.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 02/10] drm/sun4i: sun6i_mipi_dsi: Support instruction loop selection

[Jagan Teki]

Instruction loop selection would require before writing
loop number registers, so enable idle, LP11 bits on
loop selection register.

Reference code available in BSP
(in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
(dsi_dev[sel]->dsi_inst_loop_sel.dwval = 2<<(4*DSI_INST_ID_LP11) |
	3<<(4*DSI_INST_ID_DLY);

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index da152c21ec62..077b57ec964c 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -397,6 +397,10 @@ static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
 	struct mipi_dsi_device *device = dsi->device;
 	u16 delay;
 
+	regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_SEL_REG,
+		     DSI_INST_ID_HSC  << (4 * DSI_INST_ID_LP11) |
+		     DSI_INST_ID_HSD  << (4 * DSI_INST_ID_DLY));
+
 	if (device->mode_flags == MIPI_DSI_MODE_VIDEO_BURST)
 		delay = (((mode->htotal - mode->hdisplay) * 150) /
 			((mode->clock / 1000) * 8)) - 50;
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 02/10] drm/sun4i: sun6i_mipi_dsi: Support instruction loop selection

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]

On Sat, Nov 03, 2018 at 03:38:52PM +0530, Jagan Teki wrote:
> Instruction loop selection would require before writing
> loop number registers, so enable idle, LP11 bits on
> loop selection register.
> 
> Reference code available in BSP
> (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> (dsi_dev[sel]->dsi_inst_loop_sel.dwval = 2<<(4*DSI_INST_ID_LP11) |
> 	3<<(4*DSI_INST_ID_DLY);
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index da152c21ec62..077b57ec964c 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -397,6 +397,10 @@ static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
>  	struct mipi_dsi_device *device = dsi->device;
>  	u16 delay;
>  
> +	regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_SEL_REG,
> +		     DSI_INST_ID_HSC  << (4 * DSI_INST_ID_LP11) |
> +		     DSI_INST_ID_HSD  << (4 * DSI_INST_ID_DLY));
> +

Please put this with the other instructions settings below.

hanks!
Maxime
 
-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 02/10] drm/sun4i: sun6i_mipi_dsi: Support instruction loop selection

[Jagan Teki]

On Mon, Nov 5, 2018 at 4:09 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Sat, Nov 03, 2018 at 03:38:52PM +0530, Jagan Teki wrote:
> > Instruction loop selection would require before writing
> > loop number registers, so enable idle, LP11 bits on
> > loop selection register.
> >
> > Reference code available in BSP
> > (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > (dsi_dev[sel]->dsi_inst_loop_sel.dwval = 2<<(4*DSI_INST_ID_LP11) |
> >       3<<(4*DSI_INST_ID_DLY);
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index da152c21ec62..077b57ec964c 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -397,6 +397,10 @@ static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
> >       struct mipi_dsi_device *device = dsi->device;
> >       u16 delay;
> >
> > +     regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_SEL_REG,
> > +                  DSI_INST_ID_HSC  << (4 * DSI_INST_ID_LP11) |
> > +                  DSI_INST_ID_HSD  << (4 * DSI_INST_ID_DLY));
> > +
>
> Please put this with the other instructions settings below.

Does that mean after computation delay code?

Re: [PATCH 02/10] drm/sun4i: sun6i_mipi_dsi: Support instruction loop selection

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1657 bytes --]

On Mon, Nov 05, 2018 at 04:56:35PM +0530, Jagan Teki wrote:
> On Mon, Nov 5, 2018 at 4:09 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Sat, Nov 03, 2018 at 03:38:52PM +0530, Jagan Teki wrote:
> > > Instruction loop selection would require before writing
> > > loop number registers, so enable idle, LP11 bits on
> > > loop selection register.
> > >
> > > Reference code available in BSP
> > > (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > > (dsi_dev[sel]->dsi_inst_loop_sel.dwval = 2<<(4*DSI_INST_ID_LP11) |
> > >       3<<(4*DSI_INST_ID_DLY);
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > index da152c21ec62..077b57ec964c 100644
> > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > @@ -397,6 +397,10 @@ static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
> > >       struct mipi_dsi_device *device = dsi->device;
> > >       u16 delay;
> > >
> > > +     regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_SEL_REG,
> > > +                  DSI_INST_ID_HSC  << (4 * DSI_INST_ID_LP11) |
> > > +                  DSI_INST_ID_HSD  << (4 * DSI_INST_ID_DLY));
> > > +
> >
> > Please put this with the other instructions settings below.
> 
> Does that mean after computation delay code?

Yes

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 03/10] drm/sun4i: sun6i_mipi_dsi: Setup burst mode timings

[Jagan Teki]

Burst mode display timings are different from convectional
video mode so update the horizontal and vertical timings.

Reference code taken from BSP
(in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
dsi_hsa  = 0;
dsi_hbp  = 0;
dsi_hact = x*dsi_pixel_bits[format]/8;
dsi_hblk = dsi_hact;
dsi_hfp  = 0;
dsi_vblk = 0;

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 108 ++++++++++++++-----------
 1 file changed, 60 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 077b57ec964c..4965b2c71e4c 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -479,59 +479,71 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 
 	/* Do all timing calculations up front to allocate buffer space */
 
-	/*
-	 * A sync period is composed of a blanking packet (4 bytes +
-	 * payload + 2 bytes) and a sync event packet (4 bytes). Its
-	 * minimal size is therefore 10 bytes
-	 */
+	if (device->mode_flags == MIPI_DSI_MODE_VIDEO_BURST) {
+		hsa = 0;
+		hbp = 0;
+		hblk = mode->hdisplay * Bpp;
+		hfp = 0;
+		vblk = 0;
+	} else {
+		/*
+		 * A sync period is composed of a blanking packet (4 bytes +
+		 * payload + 2 bytes) and a sync event packet (4 bytes). Its
+		 * minimal size is therefore 10 bytes
+		 */
 #define HSA_PACKET_OVERHEAD	10
-	hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
-		  (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD);
-
-	/*
-	 * The backporch is set using a blanking packet (4 bytes +
-	 * payload + 2 bytes). Its minimal size is therefore 6 bytes
-	 */
+		hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
+			  (mode->hsync_end - mode->hsync_start) * Bpp -
+			  HSA_PACKET_OVERHEAD);
+
+		/*
+		 * The backporch is set using a blanking packet (4 bytes +
+		 * payload + 2 bytes). Its minimal size is therefore 6 bytes
+		 */
 #define HBP_PACKET_OVERHEAD	6
-	hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
-		  (mode->htotal - mode->hsync_end) * Bpp - HBP_PACKET_OVERHEAD);
-
-	/*
-	 * hblk seems to be the line + porches length.
-	 * The blank is set using a blanking packet (4 bytes + 4 bytes +
-	 * payload + 2 bytes). So minimal size is 10 bytes
-	 */
+		hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
+			  (mode->htotal - mode->hsync_end) * Bpp -
+			  HBP_PACKET_OVERHEAD);
+
+		/*
+		 * hblk seems to be the line + porches length.
+		 * The blank is set using a blanking packet (4 bytes + 4 bytes
+		 * + payload + 2 bytes). So minimal size is 10 bytes
+		 */
 #define HBLK_PACKET_OVERHEAD	10
-	hblk_max = (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp;
-	hblk_max -= HBLK_PACKET_OVERHEAD;
-	hblk = max_t(unsigned int, HBLK_PACKET_OVERHEAD, hblk_max);
-
-	/*
-	 * The frontporch is set using a blanking packet (4 bytes +
-	 * payload + 2 bytes). Its minimal size is therefore 6 bytes
-	 *
-	 * According to BSP code, extra 10 bytes(which is hblk packet overhead)
-	 * is adding for hfp packet overhead since hfp depends on hblk.
-	 */
+		hblk_max = (mode->htotal -
+			   (mode->hsync_end - mode->hsync_start)) * Bpp;
+		hblk_max -= HBLK_PACKET_OVERHEAD;
+		hblk = max_t(unsigned int, HBLK_PACKET_OVERHEAD, hblk_max);
+
+		/*
+		 * The frontporch is set using a blanking packet (4 bytes +
+		 * payload + 2 bytes). Its minimal size is therefore 6 bytes
+		 *
+		 * According to BSP code, extra 10 bytes(which is hblk packet
+		 * overhead) is adding for hfp packet overhead since hfp
+		 * depends on hblk.
+		 */
 #define HFP_PACKET_OVERHEAD	6
-	hfp_pkt_overhead = (HFP_PACKET_OVERHEAD + HBLK_PACKET_OVERHEAD);
-	hfp = max((unsigned int)hfp_pkt_overhead,
-		  (mode->hsync_start - mode->hdisplay) * Bpp -
-		  hfp_pkt_overhead);
-
-	/*
-	 * The vertical blank is set using a blanking packet (4 bytes +
-	 * payload + 2 bytes). Its minimal size is therefore 6 bytes
-	 */
+		hfp_pkt_overhead = (HFP_PACKET_OVERHEAD + HBLK_PACKET_OVERHEAD);
+		hfp = max((unsigned int)hfp_pkt_overhead,
+			  (mode->hsync_start - mode->hdisplay) * Bpp -
+			  hfp_pkt_overhead);
+
+		/*
+		 * The vertical blank is set using a blanking packet (4 bytes +
+		 * payload + 2 bytes). Its minimal size is therefore 6 bytes
+		 */
 #define VBLK_PACKET_OVERHEAD	6
-	if (device->lanes == 4) {
-		int tmp;
-
-		tmp = (mode->htotal * Bpp) * mode->vtotal -
-		      (hblk + VBLK_PACKET_OVERHEAD);
-		vblk = (device->lanes - tmp % device->lanes);
-	} else {
-		vblk = 0;
+		if (device->lanes == 4) {
+			int tmp;
+
+			tmp = (mode->htotal * Bpp) * mode->vtotal -
+			      (hblk + VBLK_PACKET_OVERHEAD);
+			vblk = (device->lanes - tmp % device->lanes);
+		} else {
+			vblk = 0;
+		}
 	}
 
 	/* How many bytes do we need to send all payloads? */
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 03/10] drm/sun4i: sun6i_mipi_dsi: Setup burst mode timings

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5363 bytes --]

On Sat, Nov 03, 2018 at 03:38:53PM +0530, Jagan Teki wrote:
> Burst mode display timings are different from convectional
> video mode so update the horizontal and vertical timings.
> 
> Reference code taken from BSP
> (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> dsi_hsa  = 0;
> dsi_hbp  = 0;
> dsi_hact = x*dsi_pixel_bits[format]/8;
> dsi_hblk = dsi_hact;
> dsi_hfp  = 0;
> dsi_vblk = 0;
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 108 ++++++++++++++-----------
>  1 file changed, 60 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 077b57ec964c..4965b2c71e4c 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -479,59 +479,71 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
>  
>  	/* Do all timing calculations up front to allocate buffer space */
>  
> -	/*
> -	 * A sync period is composed of a blanking packet (4 bytes +
> -	 * payload + 2 bytes) and a sync event packet (4 bytes). Its
> -	 * minimal size is therefore 10 bytes
> -	 */
> +	if (device->mode_flags == MIPI_DSI_MODE_VIDEO_BURST) {
> +		hsa = 0;
> +		hbp = 0;
> +		hblk = mode->hdisplay * Bpp;
> +		hfp = 0;
> +		vblk = 0;
> +	} else {
> +		/*
> +		 * A sync period is composed of a blanking packet (4 bytes +
> +		 * payload + 2 bytes) and a sync event packet (4 bytes). Its
> +		 * minimal size is therefore 10 bytes
> +		 */
>  #define HSA_PACKET_OVERHEAD	10
> -	hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
> -		  (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD);
> -
> -	/*
> -	 * The backporch is set using a blanking packet (4 bytes +
> -	 * payload + 2 bytes). Its minimal size is therefore 6 bytes
> -	 */
> +		hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
> +			  (mode->hsync_end - mode->hsync_start) * Bpp -
> +			  HSA_PACKET_OVERHEAD);
> +
> +		/*
> +		 * The backporch is set using a blanking packet (4 bytes +
> +		 * payload + 2 bytes). Its minimal size is therefore 6 bytes
> +		 */
>  #define HBP_PACKET_OVERHEAD	6
> -	hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
> -		  (mode->htotal - mode->hsync_end) * Bpp - HBP_PACKET_OVERHEAD);
> -
> -	/*
> -	 * hblk seems to be the line + porches length.
> -	 * The blank is set using a blanking packet (4 bytes + 4 bytes +
> -	 * payload + 2 bytes). So minimal size is 10 bytes
> -	 */
> +		hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
> +			  (mode->htotal - mode->hsync_end) * Bpp -
> +			  HBP_PACKET_OVERHEAD);
> +
> +		/*
> +		 * hblk seems to be the line + porches length.
> +		 * The blank is set using a blanking packet (4 bytes + 4 bytes
> +		 * + payload + 2 bytes). So minimal size is 10 bytes
> +		 */
>  #define HBLK_PACKET_OVERHEAD	10
> -	hblk_max = (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp;
> -	hblk_max -= HBLK_PACKET_OVERHEAD;
> -	hblk = max_t(unsigned int, HBLK_PACKET_OVERHEAD, hblk_max);
> -
> -	/*
> -	 * The frontporch is set using a blanking packet (4 bytes +
> -	 * payload + 2 bytes). Its minimal size is therefore 6 bytes
> -	 *
> -	 * According to BSP code, extra 10 bytes(which is hblk packet overhead)
> -	 * is adding for hfp packet overhead since hfp depends on hblk.
> -	 */
> +		hblk_max = (mode->htotal -
> +			   (mode->hsync_end - mode->hsync_start)) * Bpp;
> +		hblk_max -= HBLK_PACKET_OVERHEAD;
> +		hblk = max_t(unsigned int, HBLK_PACKET_OVERHEAD, hblk_max);
> +
> +		/*
> +		 * The frontporch is set using a blanking packet (4 bytes +
> +		 * payload + 2 bytes). Its minimal size is therefore 6 bytes
> +		 *
> +		 * According to BSP code, extra 10 bytes(which is hblk packet
> +		 * overhead) is adding for hfp packet overhead since hfp
> +		 * depends on hblk.
> +		 */
>  #define HFP_PACKET_OVERHEAD	6
> -	hfp_pkt_overhead = (HFP_PACKET_OVERHEAD + HBLK_PACKET_OVERHEAD);
> -	hfp = max((unsigned int)hfp_pkt_overhead,
> -		  (mode->hsync_start - mode->hdisplay) * Bpp -
> -		  hfp_pkt_overhead);
> -
> -	/*
> -	 * The vertical blank is set using a blanking packet (4 bytes +
> -	 * payload + 2 bytes). Its minimal size is therefore 6 bytes
> -	 */
> +		hfp_pkt_overhead = (HFP_PACKET_OVERHEAD + HBLK_PACKET_OVERHEAD);
> +		hfp = max((unsigned int)hfp_pkt_overhead,
> +			  (mode->hsync_start - mode->hdisplay) * Bpp -
> +			  hfp_pkt_overhead);
> +
> +		/*
> +		 * The vertical blank is set using a blanking packet (4 bytes +
> +		 * payload + 2 bytes). Its minimal size is therefore 6 bytes
> +		 */
>  #define VBLK_PACKET_OVERHEAD	6
> -	if (device->lanes == 4) {
> -		int tmp;
> -
> -		tmp = (mode->htotal * Bpp) * mode->vtotal -
> -		      (hblk + VBLK_PACKET_OVERHEAD);
> -		vblk = (device->lanes - tmp % device->lanes);
> -	} else {
> -		vblk = 0;
> +		if (device->lanes == 4) {
> +			int tmp;
> +
> +			tmp = (mode->htotal * Bpp) * mode->vtotal -
> +			      (hblk + VBLK_PACKET_OVERHEAD);
> +			vblk = (device->lanes - tmp % device->lanes);
> +		} else {
> +			vblk = 0;
> +		}
>  	}

We should make this a couple of functions to make things more
readable.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 04/10] drm/sun4i: sun6i_mipi_dsi: Setup burst mode

[Jagan Teki]

Setting up burst mode display would require to compute
- Horizontal timing edge values to fill burst drq register
- Line, sync values to fill burst line register

Since there is no direct documentation for these computations
the edge and line formulas are taken from BSP code
(in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
line_num = panel->lcd_ht*dsi_pixel_bits[panel->lcd_dsi_format]/
	  (8*panel->lcd_dsi_lane);
edge1 = sync_point+(panel->lcd_x+panel->lcd_hbp+20)*
	dsi_pixel_bits[panel->lcd_dsi_format] /(8*panel->lcd_dsi_lane);
edge1 = (edge1>line_num)?line_num:edge1;
edge0 = edge1+(panel->lcd_x+40)*tcon_div/8;
edge0 = (edge0>line_num)?(edge0-line_num):1;

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 48 +++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 4965b2c71e4c..b6c01891df36 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -375,20 +375,52 @@ static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
 				  struct drm_display_mode *mode)
 {
 	struct mipi_dsi_device *device = dsi->device;
+	unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format);
+	u32 line_num, edge0, edge1, hact_sync_bp;
+	u32 sync_point, tcon_div;
 	u32 val = 0;
 
-	if ((mode->hsync_start - mode->hdisplay) > 20) {
-		/* Maaaaaagic */
-		u16 drq = (mode->hsync_start - mode->hdisplay) - 20;
+	if (device->mode_flags != MIPI_DSI_MODE_VIDEO_BURST) {
+		if ((mode->hsync_start - mode->hdisplay) > 20) {
+			/* Maaaaaagic */
+			u16 drq = (mode->hsync_start - mode->hdisplay) - 20;
 
-		drq *= mipi_dsi_pixel_format_to_bpp(device->format);
-		drq /= 32;
+			drq *= Bpp;
+			drq /= 32;
 
-		val = (SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
-		       SUN6I_DSI_TCON_DRQ_SET(drq));
+			val = (SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
+			       SUN6I_DSI_TCON_DRQ_SET(drq));
+		}
+
+		regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG, val);
+
+		return;
 	}
 
-	regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG, val);
+	sync_point = 40;
+	tcon_div = 8;	/* FIXME need to retrive the divider from TCON */
+
+	line_num = mode->htotal * Bpp / (8 * device->lanes);
+	/* Horizental timings duration excluding front porch */
+	hact_sync_bp = (mode->hdisplay + mode->htotal - mode->hsync_start);
+	edge1 = sync_point + ((hact_sync_bp + 20) * Bpp / (8 * device->lanes));
+	if (edge1 > line_num)
+		edge1 = line_num;
+
+	edge0 = edge1 + (mode->hdisplay + 40) * tcon_div / 8;
+	if (edge0 > line_num)
+		edge0 -= line_num;
+	else
+		edge0 = 1;
+
+	regmap_write(dsi->regs, SUN6I_DSI_BURST_DRQ_REG,
+		     SUN6I_DSI_BURST_DRQ_EDGE1(edge1) |
+		     SUN6I_DSI_BURST_DRQ_EDGE0(edge0));
+	regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG,
+		     SUN6I_DSI_TCON_DRQ_ENABLE_MODE);
+	regmap_write(dsi->regs, SUN6I_DSI_BURST_LINE_REG,
+		     SUN6I_DSI_BURST_LINE_NUM(line_num) |
+		     SUN6I_DSI_BURST_LINE_SYNC_POINT(sync_point));
 }
 
 static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 04/10] drm/sun4i: sun6i_mipi_dsi: Setup burst mode

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]

On Sat, Nov 03, 2018 at 03:38:54PM +0530, Jagan Teki wrote:
> Setting up burst mode display would require to compute
> - Horizontal timing edge values to fill burst drq register
> - Line, sync values to fill burst line register
> 
> Since there is no direct documentation for these computations
> the edge and line formulas are taken from BSP code
> (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> line_num = panel->lcd_ht*dsi_pixel_bits[panel->lcd_dsi_format]/
> 	  (8*panel->lcd_dsi_lane);
> edge1 = sync_point+(panel->lcd_x+panel->lcd_hbp+20)*
> 	dsi_pixel_bits[panel->lcd_dsi_format] /(8*panel->lcd_dsi_lane);
> edge1 = (edge1>line_num)?line_num:edge1;
> edge0 = edge1+(panel->lcd_x+40)*tcon_div/8;
> edge0 = (edge0>line_num)?(edge0-line_num):1;
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 48 +++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 4965b2c71e4c..b6c01891df36 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -375,20 +375,52 @@ static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
>  				  struct drm_display_mode *mode)
>  {
>  	struct mipi_dsi_device *device = dsi->device;
> +	unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> +	u32 line_num, edge0, edge1, hact_sync_bp;
> +	u32 sync_point, tcon_div;
>  	u32 val = 0;
>  
> -	if ((mode->hsync_start - mode->hdisplay) > 20) {
> -		/* Maaaaaagic */
> -		u16 drq = (mode->hsync_start - mode->hdisplay) - 20;
> +	if (device->mode_flags != MIPI_DSI_MODE_VIDEO_BURST) {
> +		if ((mode->hsync_start - mode->hdisplay) > 20) {
> +			/* Maaaaaagic */
> +			u16 drq = (mode->hsync_start - mode->hdisplay) - 20;
>  
> -		drq *= mipi_dsi_pixel_format_to_bpp(device->format);
> -		drq /= 32;
> +			drq *= Bpp;
> +			drq /= 32;
>  
> -		val = (SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
> -		       SUN6I_DSI_TCON_DRQ_SET(drq));
> +			val = (SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
> +			       SUN6I_DSI_TCON_DRQ_SET(drq));
> +		}
> +
> +		regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG, val);
> +
> +		return;
>  	}

Having functions to compute drq, the line_number and so on would help
the readibility a lot.

> -	regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG, val);
> +	sync_point = 40;
> +	tcon_div = 8;	/* FIXME need to retrive the divider from TCON */

Then do it. Especially since you have exactly 0 guarantee of the
divider being 8.

(also, s/retrive/retrieve/)

> +
> +	line_num = mode->htotal * Bpp / (8 * device->lanes);
> +	/* Horizental timings duration excluding front porch */

Horizontal

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 05/10] drm/sun4i: sun6i_mipi_dsi: Enable burst mode

[Jagan Teki]

Enable video_mode_burst bit from dsi base control register
for burst mode display panels.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index b6c01891df36..b18a01361f11 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -421,6 +421,10 @@ static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
 	regmap_write(dsi->regs, SUN6I_DSI_BURST_LINE_REG,
 		     SUN6I_DSI_BURST_LINE_NUM(line_num) |
 		     SUN6I_DSI_BURST_LINE_SYNC_POINT(sync_point));
+
+	regmap_read(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, &val);
+	val |= SUN6I_DSI_BASIC_CTL_VIDEO_BURST;
+	regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, val);
 }
 
 static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 05/10] drm/sun4i: sun6i_mipi_dsi: Enable burst mode

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 364 bytes --]

On Sat, Nov 03, 2018 at 03:38:55PM +0530, Jagan Teki wrote:
> Enable video_mode_burst bit from dsi base control register
> for burst mode display panels.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>

Shouldn't that be in the previous patch?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 06/10] drm/sun4i: sun6i_mipi_dsi: Enable 2byte trail for 4-lane burst mode

[Jagan Teki]

For 4-lane, burst mode panels would need to enable 2byte trail_fill
along with filling trail_env in dsi base control register.

Similar reference code avialable in BSP
(in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
if (panel->lcd_dsi_lane == 4)
{
   dsi_dev[sel]->dsi_basic_ctl.bits.trail_inv = 0xc;
   dsi_dev[sel]->dsi_basic_ctl.bits.trail_fill     = 1;
}

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index b18a01361f11..2d34e5f48d29 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -33,6 +33,8 @@
 #define SUN6I_DSI_CTL_EN			BIT(0)
 
 #define SUN6I_DSI_BASIC_CTL_REG		0x00c
+#define SUN6I_DSI_BASIC_CTL_TRAIL_INV(n)	(((n) & 0xf) << 4)
+#define SUN6I_DSI_BASIC_CTL_TRAIL_FILL		BIT(3)
 #define SUN6I_DSI_BASIC_CTL_HBP_DIS		BIT(2)
 #define SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS		BIT(1)
 #define SUN6I_DSI_BASIC_CTL_VIDEO_BURST		BIT(0)
@@ -424,6 +426,10 @@ static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
 
 	regmap_read(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, &val);
 	val |= SUN6I_DSI_BASIC_CTL_VIDEO_BURST;
+	if (device->lanes == 4) {
+		val |= SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc);
+		val |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL;
+	}
 	regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, val);
 }
 
-- 
2.18.0.321.gffc6fa0e3

[PATCH 07/10] drm/sun4i: sun6i_mipi_dsi: Enable burst mode HBP, HSA_HSE

[Jagan Teki]

Horizontal back porch, sync active and sync end bits are
needed to enable for burst mode panel operations.

So, enable them via dsi base control register.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 2d34e5f48d29..feb8c54c5146 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -518,6 +518,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 	u16 hbp, hfp_pkt_overhead, hfp, hsa, hblk, vblk;
 	size_t bytes;
 	u8 *buffer;
+	u32 val = 0;
 
 	/* Do all timing calculations up front to allocate buffer space */
 
@@ -527,6 +528,10 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 		hblk = mode->hdisplay * Bpp;
 		hfp = 0;
 		vblk = 0;
+
+		regmap_read(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, &val);
+		val |= SUN6I_DSI_BASIC_CTL_HBP_DIS;
+		val |= SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS;
 	} else {
 		/*
 		 * A sync period is composed of a blanking packet (4 bytes +
@@ -594,7 +599,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 	if (WARN_ON(!buffer))
 		return;
 
-	regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, 0);
+	regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, val);
 
 	regmap_write(dsi->regs, SUN6I_DSI_SYNC_HSS_REG,
 		     sun6i_dsi_build_sync_pkt(MIPI_DSI_H_SYNC_START,
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 07/10] drm/sun4i: sun6i_mipi_dsi: Enable burst mode HBP, HSA_HSE

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 470 bytes --]

On Sat, Nov 03, 2018 at 03:38:57PM +0530, Jagan Teki wrote:
> Horizontal back porch, sync active and sync end bits are
> needed to enable for burst mode panel operations.
> 
> So, enable them via dsi base control register.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>

What is needed seems to be the exact opposite. The bits to *disable*
them.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 08/10] dt-bindings: panel: Add Feiyang FY07024DI26A30-D MIPI-DSI LCD panel

[Jagan Teki]

Feiyang FY07024DI26A30-D is 1024x600, 4-lane MIPI-DSI LCD panel.

Add dt-bingings for it.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 .../display/panel/feiyang,fy07024di26a30d.txt | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/feiyang,fy07024di26a30d.txt

diff --git a/Documentation/devicetree/bindings/display/panel/feiyang,fy07024di26a30d.txt b/Documentation/devicetree/bindings/display/panel/feiyang,fy07024di26a30d.txt
new file mode 100644
index 000000000000..82caa7b65ae8
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/feiyang,fy07024di26a30d.txt
@@ -0,0 +1,20 @@
+Feiyang FY07024DI26A30-D 7" MIPI-DSI LCD Panel
+
+Required properties:
+- compatible: must be "feiyang,fy07024di26a30d"
+- reg: DSI virtual channel used by that screen
+- avdd-supply: analog regulator dc1 switch
+- dvdd-supply: 3v3 digital regulator
+- reset-gpios: a GPIO phandle for the reset pin
+
+Optional properties:
+- backlight: phandle for the backlight control.
+
+panel@0 {
+	compatible = "feiyang,fy07024di26a30d";
+	reg = <0>;
+	avdd-supply = <&reg_dc1sw>;
+	dvdd-supply = <&reg_dldo2>;
+	reset-gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* LCD-RST: PD24 */
+	backlight = <&backlight>;
+};
-- 
2.18.0.321.gffc6fa0e3

[PATCH 09/10] drm/panel: Add Feiyang FY07024DI26A30-D MIPI-DSI LCD panel

[Jagan Teki]

Feiyang FY07024DI26A30-D is 1024x600, 4-lane MIPI-DSI LCD panel.

Add panel driver for it.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Note: init sequence is referenced from 
https://github.com/longsleep/linux-pine64/blob/pine64-hacks-1.2/drivers/video/sunxi/disp2/disp/lcd/mb709_mipi.c

 drivers/gpu/drm/panel/Kconfig                 |   9 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../drm/panel/panel-feiyang-fy07024di26a30d.c | 305 ++++++++++++++++++
 3 files changed, 315 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index d0d4e60f5153..bc70896fe43c 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -47,6 +47,15 @@ config DRM_PANEL_SIMPLE
 	  that it can be automatically turned off when the panel goes into a
 	  low power state.
 
+config DRM_PANEL_FEIYANG_FY07024DI26A30D
+	tristate "Feiyang FY07024DI26A30-D MIPI-DSI LCD panel"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y if you want to enable support for panels based on the
+	  Feiyang FY07024DI26A30-D MIPI-DSI interface.
+
 config DRM_PANEL_ILITEK_IL9322
 	tristate "Ilitek ILI9322 320x240 QVGA panels"
 	depends on OF && SPI
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 88011f06edb8..e23c017639c7 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_DRM_PANEL_ARM_VERSATILE) += panel-arm-versatile.o
 obj-$(CONFIG_DRM_PANEL_BANANAPI_S070WV20_ICN6211) += panel-bananapi-s070wv20-icn6211.o
 obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o
 obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
+obj-$(CONFIG_DRM_PANEL_FEIYANG_FY07024DI26A30D) += panel-feiyang-fy07024di26a30d.o
 obj-$(CONFIG_DRM_PANEL_ILITEK_IL9322) += panel-ilitek-ili9322.o
 obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9881C) += panel-ilitek-ili9881c.o
 obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
diff --git a/drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c b/drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c
new file mode 100644
index 000000000000..718631a72d8b
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: (GPL-2.0+ or MIT)
+/*
+ * Copyright (C) 2018 Amarula Solutions
+ * Author: Jagan Teki <jagan@amarulasolutions.com>
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/fb.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+#include <video/mipi_display.h>
+
+struct fy07024di26a30d {
+	struct drm_panel	panel;
+	struct mipi_dsi_device	*dsi;
+
+	struct backlight_device	*backlight;
+	struct regulator	*dvdd;
+	struct regulator	*avdd;
+	struct gpio_desc	*reset;
+
+	bool			is_enabled;
+	bool			is_prepared;
+};
+
+static inline struct fy07024di26a30d *panel_to_fy07024di26a30d(struct drm_panel *panel)
+{
+	return container_of(panel, struct fy07024di26a30d, panel);
+}
+
+struct fy07024di26a30d_init_cmd {
+	size_t len;
+	const char *data;
+};
+
+#define FY07024DI26A30D(...) { \
+	.len = sizeof((char[]){__VA_ARGS__}), \
+	.data = (char[]){__VA_ARGS__} }
+
+static const struct fy07024di26a30d_init_cmd fy07024di26a30d_init_cmds[] = {
+	FY07024DI26A30D(0x80, 0x58),
+	FY07024DI26A30D(0x81, 0x47),
+	FY07024DI26A30D(0x82, 0xD4),
+	FY07024DI26A30D(0x83, 0x88),
+	FY07024DI26A30D(0x84, 0xA9),
+	FY07024DI26A30D(0x85, 0xC3),
+	FY07024DI26A30D(0x86, 0x82),
+};
+
+static int fy07024di26a30d_prepare(struct drm_panel *panel)
+{
+	struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	unsigned int i;
+	int ret;
+
+	if (ctx->is_prepared)
+		return 0;
+
+	gpiod_set_value(ctx->reset, 1);
+	msleep(50);
+
+	gpiod_set_value(ctx->reset, 0);
+	msleep(20);
+
+	gpiod_set_value(ctx->reset, 1);
+	msleep(200);
+
+	for (i = 0; i < ARRAY_SIZE(fy07024di26a30d_init_cmds); i++) {
+		const struct fy07024di26a30d_init_cmd *cmd =
+						&fy07024di26a30d_init_cmds[i];
+
+		ret = mipi_dsi_dcs_write_buffer(dsi, cmd->data, cmd->len);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = mipi_dsi_dcs_set_display_on(dsi);
+	if (ret < 0) {
+		dev_err(panel->dev, "failed to set display on: %d\n", ret);
+		return ret;
+	}
+
+	ctx->is_prepared = true;
+
+	return 0;
+}
+
+static int fy07024di26a30d_enable(struct drm_panel *panel)
+{
+	struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
+
+	if (ctx->is_enabled)
+		return 0;
+
+	msleep(120);
+
+	backlight_enable(ctx->backlight);
+	ctx->is_enabled = true;
+
+	return 0;
+}
+
+static int fy07024di26a30d_disable(struct drm_panel *panel)
+{
+	struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
+
+	if (!ctx->is_enabled)
+		return 0;
+
+	backlight_disable(ctx->backlight);
+	ctx->is_enabled = false;
+
+	return 0;
+}
+
+static int fy07024di26a30d_unprepare(struct drm_panel *panel)
+{
+	struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
+	int ret;
+
+	if (!ctx->is_prepared)
+		return 0;
+
+	ret = mipi_dsi_dcs_set_display_off(ctx->dsi);
+	if (ret < 0)
+		dev_err(panel->dev, "failed to set display off: %d\n", ret);
+
+	ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
+	if (ret < 0)
+		dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
+
+	msleep(100);
+
+	regulator_disable(ctx->avdd);
+
+	regulator_disable(ctx->dvdd);
+
+	gpiod_set_value(ctx->reset, 0);
+
+	gpiod_set_value(ctx->reset, 1);
+
+	gpiod_set_value(ctx->reset, 0);
+
+	ctx->is_prepared = false;
+
+	return 0;
+}
+
+static const struct drm_display_mode fy07024di26a30d_default_mode = {
+	.clock = 55000,
+	.vrefresh = 60,
+
+	.hdisplay = 1024,
+	.hsync_start = 1024 + 396,
+	.hsync_end = 1024 + 396 + 20,
+	.htotal = 1024 + 396 + 20 + 100,
+
+	.vdisplay = 600,
+	.vsync_start = 600 + 12,
+	.vsync_end = 600 + 12 + 2,
+	.vtotal = 600 + 12 + 2 + 21,
+};
+
+static int fy07024di26a30d_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(panel->drm, &fy07024di26a30d_default_mode);
+	if (!mode) {
+		dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
+			fy07024di26a30d_default_mode.hdisplay,
+			fy07024di26a30d_default_mode.vdisplay,
+			fy07024di26a30d_default_mode.vrefresh);
+		return -ENOMEM;
+	}
+
+	drm_mode_set_name(mode);
+
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs fy07024di26a30d_funcs = {
+	.disable = fy07024di26a30d_disable,
+	.unprepare = fy07024di26a30d_unprepare,
+	.prepare = fy07024di26a30d_prepare,
+	.enable = fy07024di26a30d_enable,
+	.get_modes = fy07024di26a30d_get_modes,
+};
+
+static int fy07024di26a30d_dsi_probe(struct mipi_dsi_device *dsi)
+{
+	struct device_node *np;
+	struct fy07024di26a30d *ctx;
+	int ret;
+
+	ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	mipi_dsi_set_drvdata(dsi, ctx);
+	ctx->dsi = dsi;
+
+	drm_panel_init(&ctx->panel);
+	ctx->panel.dev = &dsi->dev;
+	ctx->panel.funcs = &fy07024di26a30d_funcs;
+
+	ctx->dvdd = devm_regulator_get(&dsi->dev, "dvdd");
+	if (IS_ERR(ctx->dvdd)) {
+		dev_err(&dsi->dev, "Couldn't get dvdd regulator\n");
+		return PTR_ERR(ctx->dvdd);
+	}
+
+	ctx->avdd = devm_regulator_get(&dsi->dev, "avdd");
+	if (IS_ERR(ctx->avdd)) {
+		dev_err(&dsi->dev, "Couldn't get avdd regulator\n");
+		return PTR_ERR(ctx->avdd);
+	}
+
+	ret = regulator_enable(ctx->dvdd);
+	if (ret)
+		return ret;
+
+	msleep(100);
+
+	ret = regulator_enable(ctx->avdd);
+	if (ret)
+		return ret;
+
+	msleep(5);
+
+	ctx->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->reset)) {
+		dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
+		return PTR_ERR(ctx->reset);
+	}
+
+	np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
+	if (np) {
+		ctx->backlight = of_find_backlight_by_node(np);
+		of_node_put(np);
+
+		if (!ctx->backlight)
+			return -EPROBE_DEFER;
+	}
+
+	ret = drm_panel_add(&ctx->panel);
+	if (ret < 0)
+		return ret;
+
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO_BURST;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->lanes = 4;
+
+	return mipi_dsi_attach(dsi);
+}
+
+static int fy07024di26a30d_dsi_remove(struct mipi_dsi_device *dsi)
+{
+	struct fy07024di26a30d *ctx = mipi_dsi_get_drvdata(dsi);
+
+	mipi_dsi_detach(dsi);
+	drm_panel_remove(&ctx->panel);
+
+	if (ctx->backlight)
+		put_device(&ctx->backlight->dev);
+
+	return 0;
+}
+
+static const struct of_device_id fy07024di26a30d_of_match[] = {
+	{ .compatible = "feiyang,fy07024di26a30d", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, fy07024di26a30d_of_match);
+
+static struct mipi_dsi_driver fy07024di26a30d_driver = {
+	.probe = fy07024di26a30d_dsi_probe,
+	.remove = fy07024di26a30d_dsi_remove,
+	.driver = {
+		.name = "feiyang-fy07024di26a30d",
+		.of_match_table = fy07024di26a30d_of_match,
+	},
+};
+module_mipi_dsi_driver(fy07024di26a30d_driver);
+
+MODULE_AUTHOR("Jagan Teki <jagan@amarulasolutions.com>");
+MODULE_DESCRIPTION("Feiyang FY07024DI26A30-D MIPI-DSI LCD panel");
+MODULE_LICENSE("GPL v2");
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 09/10] drm/panel: Add Feiyang FY07024DI26A30-D MIPI-DSI LCD panel

[Sam Ravnborg]

Hi Jagan.

Reading through the driver triggered a few comments.
Read and decide what is usefull.

	Sam

> Add panel driver for it.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Note: init sequence is referenced from 
> https://github.com/longsleep/linux-pine64/blob/pine64-hacks-1.2/drivers/video/sunxi/disp2/disp/lcd/mb709_mipi.c

This note should perferably be part of the commit message or maybe included
in the code so it is easie to track back.


> +++ b/drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c
> +/*
> + * Copyright (C) 2018 Amarula Solutions
> + * Author: Jagan Teki <jagan@amarulasolutions.com>
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/fb.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +



> +#include <video/mipi_display.h>

I cannot see this include is used anywhere in this .c file.
The few MIPI_ constants originates from <drm/drm_mipi_dsi.h>
and I think the include can be dropped.

> +struct fy07024di26a30d {
> +	struct drm_panel	panel;
> +	struct mipi_dsi_device	*dsi;
> +
> +	struct backlight_device	*backlight;
> +	struct regulator	*dvdd;
> +	struct regulator	*avdd;
> +	struct gpio_desc	*reset;
> +
> +	bool			is_enabled;
> +	bool			is_prepared;
> +};
Maybe bikeshedding a little here.
But the use of names like fy07024di26a30d does not help readability.
Just name it feiyang like for example we see in panel-innolux-p079zca.c
where this drives is somehow inspired from. (Here the name is innolux).



> +
> +static inline struct fy07024di26a30d *panel_to_fy07024di26a30d(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct fy07024di26a30d, panel);
> +}
> +
> +struct fy07024di26a30d_init_cmd {
> +	size_t len;
> +	const char *data;
> +};
> +
> +#define FY07024DI26A30D(...) { \
> +	.len = sizeof((char[]){__VA_ARGS__}), \
> +	.data = (char[]){__VA_ARGS__} }
> +
> +static const struct fy07024di26a30d_init_cmd fy07024di26a30d_init_cmds[] = {
> +	FY07024DI26A30D(0x80, 0x58),
> +	FY07024DI26A30D(0x81, 0x47),
> +	FY07024DI26A30D(0x82, 0xD4),
> +	FY07024DI26A30D(0x83, 0x88),
> +	FY07024DI26A30D(0x84, 0xA9),
> +	FY07024DI26A30D(0x85, 0xC3),
> +	FY07024DI26A30D(0x86, 0x82),
> +};
> +
> +static int fy07024di26a30d_prepare(struct drm_panel *panel)
> +{
> +	struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
> +	struct mipi_dsi_device *dsi = ctx->dsi;
> +	unsigned int i;
> +	int ret;
> +
> +	if (ctx->is_prepared)
> +		return 0;
> +
> +	gpiod_set_value(ctx->reset, 1);
> +	msleep(50);
> +
> +	gpiod_set_value(ctx->reset, 0);
> +	msleep(20);
> +
> +	gpiod_set_value(ctx->reset, 1);
> +	msleep(200);
> +
> +	for (i = 0; i < ARRAY_SIZE(fy07024di26a30d_init_cmds); i++) {
> +		const struct fy07024di26a30d_init_cmd *cmd =
> +						&fy07024di26a30d_init_cmds[i];
> +
> +		ret = mipi_dsi_dcs_write_buffer(dsi, cmd->data, cmd->len);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = mipi_dsi_dcs_set_display_on(dsi);
> +	if (ret < 0) {
> +		dev_err(panel->dev, "failed to set display on: %d\n", ret);
> +		return ret;
> +	}
General comment.
Consider using DRM_DEV_ERROR(...) to be consistent with
what is used in many other drm drivers.


> +static int fy07024di26a30d_enable(struct drm_panel *panel)
> +{
> +	struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
> +
> +	if (ctx->is_enabled)
> +		return 0;
> +
> +	msleep(120);
This msleep() looks unjustified, as no other statement preceed it.
If prepare() requires a delay then maybe the delay should be there?

> +
> +static int fy07024di26a30d_unprepare(struct drm_panel *panel)
> +{
> +	struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
> +	int ret;
> +
> +	if (!ctx->is_prepared)
> +		return 0;
> +
> +	ret = mipi_dsi_dcs_set_display_off(ctx->dsi);
> +	if (ret < 0)
> +		dev_err(panel->dev, "failed to set display off: %d\n", ret);
> +
> +	ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> +	if (ret < 0)
> +		dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
> +
> +	msleep(100);
> +
> +	regulator_disable(ctx->avdd);
> +
> +	regulator_disable(ctx->dvdd);
> +
> +	gpiod_set_value(ctx->reset, 0);
> +
> +	gpiod_set_value(ctx->reset, 1);
> +
> +	gpiod_set_value(ctx->reset, 0);

In prepare() there are dealys around asserting reset and releasing again.
Consider using a small helper function and be consistent with the
delays.

> +
> +	ctx->is_prepared = false;
> +
> +	return 0;
> +}
> +
> +static const struct drm_display_mode fy07024di26a30d_default_mode = {
> +	.clock = 55000,
> +	.vrefresh = 60,
> +
> +	.hdisplay = 1024,
> +	.hsync_start = 1024 + 396,
> +	.hsync_end = 1024 + 396 + 20,
> +	.htotal = 1024 + 396 + 20 + 100,
> +
> +	.vdisplay = 600,
> +	.vsync_start = 600 + 12,
> +	.vsync_end = 600 + 12 + 2,
> +	.vtotal = 600 + 12 + 2 + 21,
> +};
Consider to assign .flags - if for nothign else then to document
the default values.
Many panels have started to do so in panel-simple.c for instance.

This is also a proper place to specify the width_mm and height_mm,
so the physical size is available.
Assuming this is known.

> +
> +static int fy07024di26a30d_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_connector *connector = panel->connector;
> +	struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(panel->drm, &fy07024di26a30d_default_mode);
> +	if (!mode) {
> +		dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
> +			fy07024di26a30d_default_mode.hdisplay,
> +			fy07024di26a30d_default_mode.vdisplay,
> +			fy07024di26a30d_default_mode.vrefresh);
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs fy07024di26a30d_funcs = {
> +	.disable = fy07024di26a30d_disable,
> +	.unprepare = fy07024di26a30d_unprepare,
> +	.prepare = fy07024di26a30d_prepare,
> +	.enable = fy07024di26a30d_enable,
> +	.get_modes = fy07024di26a30d_get_modes,
> +};
> +
> +static int fy07024di26a30d_dsi_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device_node *np;
> +	struct fy07024di26a30d *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +	ctx->dsi = dsi;
> +
> +	drm_panel_init(&ctx->panel);
> +	ctx->panel.dev = &dsi->dev;
> +	ctx->panel.funcs = &fy07024di26a30d_funcs;
> +
> +	ctx->dvdd = devm_regulator_get(&dsi->dev, "dvdd");
> +	if (IS_ERR(ctx->dvdd)) {
> +		dev_err(&dsi->dev, "Couldn't get dvdd regulator\n");
> +		return PTR_ERR(ctx->dvdd);
> +	}
> +
> +	ctx->avdd = devm_regulator_get(&dsi->dev, "avdd");
> +	if (IS_ERR(ctx->avdd)) {
> +		dev_err(&dsi->dev, "Couldn't get avdd regulator\n");
> +		return PTR_ERR(ctx->avdd);
> +	}
> +
> +	ret = regulator_enable(ctx->dvdd);
> +	if (ret)
> +		return ret;
> +
> +	msleep(100);
> +
> +	ret = regulator_enable(ctx->avdd);
> +	if (ret)
> +		return ret;
> +
> +	msleep(5);
The regulators are only enabled in the probe function.
But disabled in the unprepare() function.

Looking at other drivers is seems that the common way is
to enable these in prepare() and disable these in unprepare().
Any particular reason the regulators are enabled in probe()?
Maybe something to do with backlight?
If so, then please put a comment.
Also consider if there is somethign missing, as the panel cannot
be used anymore after an unpreare()


> +
> +	ctx->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->reset)) {
> +		dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
> +		return PTR_ERR(ctx->reset);
> +	}
> +
> +	np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
> +	if (np) {
> +		ctx->backlight = of_find_backlight_by_node(np);
> +		of_node_put(np);
> +
> +		if (!ctx->backlight)
> +			return -EPROBE_DEFER;
> +	}
> +
> +	ret = drm_panel_add(&ctx->panel);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO_BURST;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->lanes = 4;
> +
> +	return mipi_dsi_attach(dsi);
Several drivers undo the drm_panel_add() if mipi_dsi_attach() fails.
I dunno if this is correct and when it may fail.


> +MODULE_DESCRIPTION("Feiyang FY07024DI26A30-D MIPI-DSI LCD panel");

> +MODULE_LICENSE("GPL v2");
The SPDX tag mentioned MIT - so the above may not be correct.

Re: [PATCH 09/10] drm/panel: Add Feiyang FY07024DI26A30-D MIPI-DSI LCD panel

[Jagan Teki]

On Mon, Nov 5, 2018 at 2:14 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Jagan.
>
> Reading through the driver triggered a few comments.
> Read and decide what is usefull.
>
>         Sam
>
> > Add panel driver for it.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > Note: init sequence is referenced from
> > https://github.com/longsleep/linux-pine64/blob/pine64-hacks-1.2/drivers/video/sunxi/disp2/disp/lcd/mb709_mipi.c
>
> This note should perferably be part of the commit message or maybe included
> in the code so it is easie to track back.
>
>
> > +++ b/drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c
> > +/*
> > + * Copyright (C) 2018 Amarula Solutions
> > + * Author: Jagan Teki <jagan@amarulasolutions.com>
> > + */
> > +
> > +#include <linux/backlight.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/fb.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include <drm/drm_mipi_dsi.h>
> > +#include <drm/drm_modes.h>
> > +#include <drm/drm_panel.h>
> > +
>
>
>
> > +#include <video/mipi_display.h>
>
> I cannot see this include is used anywhere in this .c file.
> The few MIPI_ constants originates from <drm/drm_mipi_dsi.h>
> and I think the include can be dropped.
>
> > +struct fy07024di26a30d {
> > +     struct drm_panel        panel;
> > +     struct mipi_dsi_device  *dsi;
> > +
> > +     struct backlight_device *backlight;
> > +     struct regulator        *dvdd;
> > +     struct regulator        *avdd;
> > +     struct gpio_desc        *reset;
> > +
> > +     bool                    is_enabled;
> > +     bool                    is_prepared;
> > +};
> Maybe bikeshedding a little here.
> But the use of names like fy07024di26a30d does not help readability.
> Just name it feiyang like for example we see in panel-innolux-p079zca.c
> where this drives is somehow inspired from. (Here the name is innolux).
>
>
>
> > +
> > +static inline struct fy07024di26a30d *panel_to_fy07024di26a30d(struct drm_panel *panel)
> > +{
> > +     return container_of(panel, struct fy07024di26a30d, panel);
> > +}
> > +
> > +struct fy07024di26a30d_init_cmd {
> > +     size_t len;
> > +     const char *data;
> > +};
> > +
> > +#define FY07024DI26A30D(...) { \
> > +     .len = sizeof((char[]){__VA_ARGS__}), \
> > +     .data = (char[]){__VA_ARGS__} }
> > +
> > +static const struct fy07024di26a30d_init_cmd fy07024di26a30d_init_cmds[] = {
> > +     FY07024DI26A30D(0x80, 0x58),
> > +     FY07024DI26A30D(0x81, 0x47),
> > +     FY07024DI26A30D(0x82, 0xD4),
> > +     FY07024DI26A30D(0x83, 0x88),
> > +     FY07024DI26A30D(0x84, 0xA9),
> > +     FY07024DI26A30D(0x85, 0xC3),
> > +     FY07024DI26A30D(0x86, 0x82),
> > +};
> > +
> > +static int fy07024di26a30d_prepare(struct drm_panel *panel)
> > +{
> > +     struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
> > +     struct mipi_dsi_device *dsi = ctx->dsi;
> > +     unsigned int i;
> > +     int ret;
> > +
> > +     if (ctx->is_prepared)
> > +             return 0;
> > +
> > +     gpiod_set_value(ctx->reset, 1);
> > +     msleep(50);
> > +
> > +     gpiod_set_value(ctx->reset, 0);
> > +     msleep(20);
> > +
> > +     gpiod_set_value(ctx->reset, 1);
> > +     msleep(200);
> > +
> > +     for (i = 0; i < ARRAY_SIZE(fy07024di26a30d_init_cmds); i++) {
> > +             const struct fy07024di26a30d_init_cmd *cmd =
> > +                                             &fy07024di26a30d_init_cmds[i];
> > +
> > +             ret = mipi_dsi_dcs_write_buffer(dsi, cmd->data, cmd->len);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> > +     ret = mipi_dsi_dcs_set_display_on(dsi);
> > +     if (ret < 0) {
> > +             dev_err(panel->dev, "failed to set display on: %d\n", ret);
> > +             return ret;
> > +     }
> General comment.
> Consider using DRM_DEV_ERROR(...) to be consistent with
> what is used in many other drm drivers.
>
>
> > +static int fy07024di26a30d_enable(struct drm_panel *panel)
> > +{
> > +     struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
> > +
> > +     if (ctx->is_enabled)
> > +             return 0;
> > +
> > +     msleep(120);
> This msleep() looks unjustified, as no other statement preceed it.
> If prepare() requires a delay then maybe the delay should be there?

I will use mipi_dsi_dcs_set_display_on in enable and this can be
proper place other than prepare, since after prepare enable is the
caller.

>
> > +
> > +static int fy07024di26a30d_unprepare(struct drm_panel *panel)
> > +{
> > +     struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
> > +     int ret;
> > +
> > +     if (!ctx->is_prepared)
> > +             return 0;
> > +
> > +     ret = mipi_dsi_dcs_set_display_off(ctx->dsi);
> > +     if (ret < 0)
> > +             dev_err(panel->dev, "failed to set display off: %d\n", ret);
> > +
> > +     ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> > +     if (ret < 0)
> > +             dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
> > +
> > +     msleep(100);
> > +
> > +     regulator_disable(ctx->avdd);
> > +
> > +     regulator_disable(ctx->dvdd);
> > +
> > +     gpiod_set_value(ctx->reset, 0);
> > +
> > +     gpiod_set_value(ctx->reset, 1);
> > +
> > +     gpiod_set_value(ctx->reset, 0);
>
> In prepare() there are dealys around asserting reset and releasing again.
> Consider using a small helper function and be consistent with the
> delays.

Do you think exit calls also need delays?

>
> > +
> > +     ctx->is_prepared = false;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct drm_display_mode fy07024di26a30d_default_mode = {
> > +     .clock = 55000,
> > +     .vrefresh = 60,
> > +
> > +     .hdisplay = 1024,
> > +     .hsync_start = 1024 + 396,
> > +     .hsync_end = 1024 + 396 + 20,
> > +     .htotal = 1024 + 396 + 20 + 100,
> > +
> > +     .vdisplay = 600,
> > +     .vsync_start = 600 + 12,
> > +     .vsync_end = 600 + 12 + 2,
> > +     .vtotal = 600 + 12 + 2 + 21,
> > +};
> Consider to assign .flags - if for nothign else then to document
> the default values.
> Many panels have started to do so in panel-simple.c for instance.

I don't see .flags handle is done via bsp driver or with dts, ie
reason I didn't use it.

>
> This is also a proper place to specify the width_mm and height_mm,
> so the physical size is available.
> Assuming this is known.

Correct, I saw these active width, height is mentioned in datasheet.
but the BSP tested dts simply initialized to 0 so I'm not using it.
[2]

>
> > +
> > +static int fy07024di26a30d_get_modes(struct drm_panel *panel)
> > +{
> > +     struct drm_connector *connector = panel->connector;
> > +     struct fy07024di26a30d *ctx = panel_to_fy07024di26a30d(panel);
> > +     struct drm_display_mode *mode;
> > +
> > +     mode = drm_mode_duplicate(panel->drm, &fy07024di26a30d_default_mode);
> > +     if (!mode) {
> > +             dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
> > +                     fy07024di26a30d_default_mode.hdisplay,
> > +                     fy07024di26a30d_default_mode.vdisplay,
> > +                     fy07024di26a30d_default_mode.vrefresh);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     drm_mode_set_name(mode);
> > +
> > +     mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> > +     drm_mode_probed_add(connector, mode);
> > +
> > +     return 1;
> > +}
> > +
> > +static const struct drm_panel_funcs fy07024di26a30d_funcs = {
> > +     .disable = fy07024di26a30d_disable,
> > +     .unprepare = fy07024di26a30d_unprepare,
> > +     .prepare = fy07024di26a30d_prepare,
> > +     .enable = fy07024di26a30d_enable,
> > +     .get_modes = fy07024di26a30d_get_modes,
> > +};
> > +
> > +static int fy07024di26a30d_dsi_probe(struct mipi_dsi_device *dsi)
> > +{
> > +     struct device_node *np;
> > +     struct fy07024di26a30d *ctx;
> > +     int ret;
> > +
> > +     ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
> > +     if (!ctx)
> > +             return -ENOMEM;
> > +     mipi_dsi_set_drvdata(dsi, ctx);
> > +     ctx->dsi = dsi;
> > +
> > +     drm_panel_init(&ctx->panel);
> > +     ctx->panel.dev = &dsi->dev;
> > +     ctx->panel.funcs = &fy07024di26a30d_funcs;
> > +
> > +     ctx->dvdd = devm_regulator_get(&dsi->dev, "dvdd");
> > +     if (IS_ERR(ctx->dvdd)) {
> > +             dev_err(&dsi->dev, "Couldn't get dvdd regulator\n");
> > +             return PTR_ERR(ctx->dvdd);
> > +     }
> > +
> > +     ctx->avdd = devm_regulator_get(&dsi->dev, "avdd");
> > +     if (IS_ERR(ctx->avdd)) {
> > +             dev_err(&dsi->dev, "Couldn't get avdd regulator\n");
> > +             return PTR_ERR(ctx->avdd);
> > +     }
> > +
> > +     ret = regulator_enable(ctx->dvdd);
> > +     if (ret)
> > +             return ret;
> > +
> > +     msleep(100);
> > +
> > +     ret = regulator_enable(ctx->avdd);
> > +     if (ret)
> > +             return ret;
> > +
> > +     msleep(5);
> The regulators are only enabled in the probe function.
> But disabled in the unprepare() function.
>
> Looking at other drivers is seems that the common way is
> to enable these in prepare() and disable these in unprepare().
> Any particular reason the regulators are enabled in probe()?
> Maybe something to do with backlight?
> If so, then please put a comment.
> Also consider if there is somethign missing, as the panel cannot
> be used anymore after an unpreare()

Yes, ie true with many panels resides in mainline. but this panel is
enabling regulators separately wrt reset. not done in inline. But it
still work for me if I enable regulators in prepare, but not sure does
it may any issues.

>
>
> > +
> > +     ctx->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
> > +     if (IS_ERR(ctx->reset)) {
> > +             dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
> > +             return PTR_ERR(ctx->reset);
> > +     }
> > +
> > +     np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
> > +     if (np) {
> > +             ctx->backlight = of_find_backlight_by_node(np);
> > +             of_node_put(np);
> > +
> > +             if (!ctx->backlight)
> > +                     return -EPROBE_DEFER;
> > +     }
> > +
> > +     ret = drm_panel_add(&ctx->panel);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     dsi->mode_flags = MIPI_DSI_MODE_VIDEO_BURST;
> > +     dsi->format = MIPI_DSI_FMT_RGB888;
> > +     dsi->lanes = 4;
> > +
> > +     return mipi_dsi_attach(dsi);
> Several drivers undo the drm_panel_add() if mipi_dsi_attach() fails.
> I dunno if this is correct and when it may fail.

Yes it should be drm_panel_remove for fail case, will take care.

>
>
> > +MODULE_DESCRIPTION("Feiyang FY07024DI26A30-D MIPI-DSI LCD panel");
>
> > +MODULE_LICENSE("GPL v2");
> The SPDX tag mentioned MIT - so the above may not be correct.

Correct, "GPL V2+ MIT" is fine?

[2] https://github.com/armbian/build/blob/master/packages/blobs/sunxi/a64/pine64so.dts#L2182

[PATCH 10/10] [DO NOT MERGE] arm64: allwinner: a64: pine64-lts: Enable Feiyang FY07024DI26A30-D DSI panel

[Jagan Teki]

Feiyang FY07024DI26A30-D MIPI_DSI panel is desiged to attach with
DSI connector on pine64 boards, enable the same for pine64 LTS.

DSI panel connected via board DSI port with,
- DC1SW as AVDD supply
- DLDO2 as DVDD supply
- DLDO1 as VCC-DSI supply
- PD24 gpio for reset pin
- PH10 gpio for backlight enable pin

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 .../dts/allwinner/sun50i-a64-pine64-lts.dts   | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts
index 72d6961dc312..a5097cf7a8dc 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts
@@ -5,9 +5,46 @@
  */
 
 #include "sun50i-a64-sopine-baseboard.dts"
+#include <dt-bindings/pwm/pwm.h>
 
 / {
 	model = "Pine64 LTS";
 	compatible = "pine64,pine64-lts", "allwinner,sun50i-r18",
 		     "allwinner,sun50i-a64";
+
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		pwms = <&r_pwm 0 50000 PWM_POLARITY_INVERTED>;
+		brightness-levels = <1 2 4 8 16 32 64 128 512>;
+		default-brightness-level = <8>;
+		enable-gpios = <&pio 7 10 GPIO_ACTIVE_HIGH>; /* LCD-BL-EN: PH10 */
+	};
+};
+
+&de {
+	status = "okay";
+};
+
+&dphy {
+	status = "okay";
+};
+
+&dsi {
+	vcc-dsi-supply = <&reg_dldo1>;
+	status = "okay";
+
+	panel@0 {
+		compatible = "feiyang,fy07024di26a30d";
+		reg = <0>;
+		avdd-supply = <&reg_dc1sw>;
+		dvdd-supply = <&reg_dldo2>;
+		reset-gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* LCD-RST: PD24 */
+		backlight = <&backlight>;
+	};
+};
+
+&r_pwm {
+	pinctrl-names = "default";
+	pinctrl-0 = <&r_pwm_pin>;
+	status = "okay";
 };
-- 
2.18.0.321.gffc6fa0e3