diff mbox series

[v1,2/2] drm/panel-simple: Add missing BUS descriptions for some panels

Message ID 20200621222742.25695-3-digetx@gmail.com
State New
Headers show
Series Improve descriptions of a few simple-panels | expand

Commit Message

Dmitry Osipenko June 21, 2020, 10:27 p.m. UTC
This patch adds missing BUS fields to the display panel descriptions of
the panels which are found on NVIDIA Tegra devices:

  1. AUO B101AW03
  2. Chunghwa CLAA070WP03XG
  3. Chunghwa CLAA101WA01A
  4. Chunghwa CLAA101WB01
  5. Innolux N156BGE L21
  6. Samsung LTN101NT05

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Laurent Pinchart June 27, 2020, 8:43 p.m. UTC | #1
Hi Dmitry,

Thank you for the patch.

On Mon, Jun 22, 2020 at 01:27:42AM +0300, Dmitry Osipenko wrote:
> This patch adds missing BUS fields to the display panel descriptions of
> the panels which are found on NVIDIA Tegra devices:
> 
>   1. AUO B101AW03
>   2. Chunghwa CLAA070WP03XG
>   3. Chunghwa CLAA101WA01A
>   4. Chunghwa CLAA101WB01
>   5. Innolux N156BGE L21
>   6. Samsung LTN101NT05
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 87edd2bdf09a..986df9937650 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -698,6 +698,8 @@ static const struct panel_desc auo_b101aw03 = {
>  		.width = 223,
>  		.height = 125,
>  	},
> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,

Does DRM_BUS_FLAG_PIXDATA_DRIVE_* make sense for LVDS ?

The rest looks good, except the Samsung panel for which I haven't been
able to locate a datasheet.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
> @@ -1352,6 +1354,8 @@ static const struct panel_desc chunghwa_claa070wp03xg = {
>  		.width = 94,
>  		.height = 150,
>  	},
> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
>  	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
> @@ -1375,6 +1379,8 @@ static const struct panel_desc chunghwa_claa101wa01a = {
>  		.width = 220,
>  		.height = 120,
>  	},
> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
>  	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
> @@ -1398,6 +1404,8 @@ static const struct panel_desc chunghwa_claa101wb01 = {
>  		.width = 223,
>  		.height = 125,
>  	},
> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
>  	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
> @@ -2071,6 +2079,8 @@ static const struct panel_desc innolux_n156bge_l21 = {
>  		.width = 344,
>  		.height = 193,
>  	},
> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
>  	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>  
> @@ -3018,6 +3028,8 @@ static const struct panel_desc samsung_ltn101nt05 = {
>  		.width = 223,
>  		.height = 125,
>  	},
> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
>  	.connector_type = DRM_MODE_CONNECTOR_LVDS,
>  };
>
Dmitry Osipenko June 27, 2020, 11:44 p.m. UTC | #2
27.06.2020 23:43, Laurent Pinchart пишет:
> Hi Dmitry,
> 
> Thank you for the patch.
> 
> On Mon, Jun 22, 2020 at 01:27:42AM +0300, Dmitry Osipenko wrote:
>> This patch adds missing BUS fields to the display panel descriptions of
>> the panels which are found on NVIDIA Tegra devices:
>>
>>   1. AUO B101AW03
>>   2. Chunghwa CLAA070WP03XG
>>   3. Chunghwa CLAA101WA01A
>>   4. Chunghwa CLAA101WB01
>>   5. Innolux N156BGE L21
>>   6. Samsung LTN101NT05
>>
>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/gpu/drm/panel/panel-simple.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>> index 87edd2bdf09a..986df9937650 100644
>> --- a/drivers/gpu/drm/panel/panel-simple.c
>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>> @@ -698,6 +698,8 @@ static const struct panel_desc auo_b101aw03 = {
>>  		.width = 223,
>>  		.height = 125,
>>  	},
>> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
>> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> 
> Does DRM_BUS_FLAG_PIXDATA_DRIVE_* make sense for LVDS ?

To be honest I don't know whether it make sense or not for LVDS. I saw
that other LVDS panels in panel-simple.c use the PIXDATA flag and then
looked at what timing diagrams in the datasheets show.

> The rest looks good, except the Samsung panel for which I haven't been
> able to locate a datasheet.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks to you and Sam!
Laurent Pinchart June 28, 2020, 7:07 a.m. UTC | #3
Hi Dmitry,

On Sun, Jun 28, 2020 at 02:44:15AM +0300, Dmitry Osipenko wrote:
> 27.06.2020 23:43, Laurent Pinchart пишет:
> > On Mon, Jun 22, 2020 at 01:27:42AM +0300, Dmitry Osipenko wrote:
> >> This patch adds missing BUS fields to the display panel descriptions of
> >> the panels which are found on NVIDIA Tegra devices:
> >>
> >>   1. AUO B101AW03
> >>   2. Chunghwa CLAA070WP03XG
> >>   3. Chunghwa CLAA101WA01A
> >>   4. Chunghwa CLAA101WB01
> >>   5. Innolux N156BGE L21
> >>   6. Samsung LTN101NT05
> >>
> >> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/gpu/drm/panel/panel-simple.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> >> index 87edd2bdf09a..986df9937650 100644
> >> --- a/drivers/gpu/drm/panel/panel-simple.c
> >> +++ b/drivers/gpu/drm/panel/panel-simple.c
> >> @@ -698,6 +698,8 @@ static const struct panel_desc auo_b101aw03 = {
> >>  		.width = 223,
> >>  		.height = 125,
> >>  	},
> >> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> >> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> > 
> > Does DRM_BUS_FLAG_PIXDATA_DRIVE_* make sense for LVDS ?
> 
> To be honest I don't know whether it make sense or not for LVDS. I saw
> that other LVDS panels in panel-simple.c use the PIXDATA flag and then
> looked at what timing diagrams in the datasheets show.

I think we should drop DRM_BUS_FLAG_PIXDATA_DRIVE_* for LVDS panels.
I'll submit a patch.

> > The rest looks good, except the Samsung panel for which I haven't been
> > able to locate a datasheet.
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks to you and Sam!
Sam Ravnborg June 28, 2020, 7:52 a.m. UTC | #4
Hi Laurent.

On Sun, Jun 28, 2020 at 10:07:45AM +0300, Laurent Pinchart wrote:
> Hi Dmitry,
> 
> On Sun, Jun 28, 2020 at 02:44:15AM +0300, Dmitry Osipenko wrote:
> > 27.06.2020 23:43, Laurent Pinchart пишет:
> > > On Mon, Jun 22, 2020 at 01:27:42AM +0300, Dmitry Osipenko wrote:
> > >> This patch adds missing BUS fields to the display panel descriptions of
> > >> the panels which are found on NVIDIA Tegra devices:
> > >>
> > >>   1. AUO B101AW03
> > >>   2. Chunghwa CLAA070WP03XG
> > >>   3. Chunghwa CLAA101WA01A
> > >>   4. Chunghwa CLAA101WB01
> > >>   5. Innolux N156BGE L21
> > >>   6. Samsung LTN101NT05
> > >>
> > >> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > >> ---
> > >>  drivers/gpu/drm/panel/panel-simple.c | 12 ++++++++++++
> > >>  1 file changed, 12 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > >> index 87edd2bdf09a..986df9937650 100644
> > >> --- a/drivers/gpu/drm/panel/panel-simple.c
> > >> +++ b/drivers/gpu/drm/panel/panel-simple.c
> > >> @@ -698,6 +698,8 @@ static const struct panel_desc auo_b101aw03 = {
> > >>  		.width = 223,
> > >>  		.height = 125,
> > >>  	},
> > >> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> > >> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> > > 
> > > Does DRM_BUS_FLAG_PIXDATA_DRIVE_* make sense for LVDS ?
> > 
> > To be honest I don't know whether it make sense or not for LVDS. I saw
> > that other LVDS panels in panel-simple.c use the PIXDATA flag and then
> > looked at what timing diagrams in the datasheets show.
> 
> I think we should drop DRM_BUS_FLAG_PIXDATA_DRIVE_* for LVDS panels.
> I'll submit a patch.

We should also clean up all the DRM_BUS_FLAG_* one day.
No need for the deprecated values, so a few files needs an update.
And we could document what flags makes sense for LVDS etc.

On the TODO list...

	Sam
> 
> > > The rest looks good, except the Samsung panel for which I haven't been
> > > able to locate a datasheet.
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Thanks to you and Sam!
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart June 28, 2020, 8:02 a.m. UTC | #5
Hi Sam,

On Sun, Jun 28, 2020 at 09:52:45AM +0200, Sam Ravnborg wrote:
> On Sun, Jun 28, 2020 at 10:07:45AM +0300, Laurent Pinchart wrote:
> > On Sun, Jun 28, 2020 at 02:44:15AM +0300, Dmitry Osipenko wrote:
> >> 27.06.2020 23:43, Laurent Pinchart пишет:
> >>> On Mon, Jun 22, 2020 at 01:27:42AM +0300, Dmitry Osipenko wrote:
> >>>> This patch adds missing BUS fields to the display panel descriptions of
> >>>> the panels which are found on NVIDIA Tegra devices:
> >>>>
> >>>>   1. AUO B101AW03
> >>>>   2. Chunghwa CLAA070WP03XG
> >>>>   3. Chunghwa CLAA101WA01A
> >>>>   4. Chunghwa CLAA101WB01
> >>>>   5. Innolux N156BGE L21
> >>>>   6. Samsung LTN101NT05
> >>>>
> >>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >>>> ---
> >>>>  drivers/gpu/drm/panel/panel-simple.c | 12 ++++++++++++
> >>>>  1 file changed, 12 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> >>>> index 87edd2bdf09a..986df9937650 100644
> >>>> --- a/drivers/gpu/drm/panel/panel-simple.c
> >>>> +++ b/drivers/gpu/drm/panel/panel-simple.c
> >>>> @@ -698,6 +698,8 @@ static const struct panel_desc auo_b101aw03 = {
> >>>>  		.width = 223,
> >>>>  		.height = 125,
> >>>>  	},
> >>>> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> >>>> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> >>> 
> >>> Does DRM_BUS_FLAG_PIXDATA_DRIVE_* make sense for LVDS ?
> >> 
> >> To be honest I don't know whether it make sense or not for LVDS. I saw
> >> that other LVDS panels in panel-simple.c use the PIXDATA flag and then
> >> looked at what timing diagrams in the datasheets show.
> > 
> > I think we should drop DRM_BUS_FLAG_PIXDATA_DRIVE_* for LVDS panels.
> > I'll submit a patch.
> 
> We should also clean up all the DRM_BUS_FLAG_* one day.
> No need for the deprecated values, so a few files needs an update.
> And we could document what flags makes sense for LVDS etc.

Where would you add that documentation ? The hardest part is to find a
place that will be noticed by developers :-)

I've just submitted a patch that adds a WARN_ON to catch similar issues
in the panel-simple driver. It's not ideal as we really shouldn't have
such code in the kernel, this is something that should be caught as part
of the integration process.

> On the TODO list...
>
> >>> The rest looks good, except the Samsung panel for which I haven't been
> >>> able to locate a datasheet.
> >>> 
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sam Ravnborg June 28, 2020, 8:53 a.m. UTC | #6
On Sun, Jun 28, 2020 at 11:02:53AM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> > We should also clean up all the DRM_BUS_FLAG_* one day.
> > No need for the deprecated values, so a few files needs an update.
> > And we could document what flags makes sense for LVDS etc.
> 
> Where would you add that documentation ? The hardest part is to find a
> place that will be noticed by developers :-)
I will try to extend drm_bus_flags documentation in drm_connector.h
And then add a few comments in panel-simple as well.

	Sam
> 
> I've just submitted a patch that adds a WARN_ON to catch similar issues
> in the panel-simple driver. It's not ideal as we really shouldn't have
> such code in the kernel, this is something that should be caught as part
> of the integration process.

> 
> > On the TODO list...
> >
> > >>> The rest looks good, except the Samsung panel for which I haven't been
> > >>> able to locate a datasheet.
> > >>> 
> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> -- 
> Regards,
> 
> Laurent Pinchart
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 87edd2bdf09a..986df9937650 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -698,6 +698,8 @@  static const struct panel_desc auo_b101aw03 = {
 		.width = 223,
 		.height = 125,
 	},
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
 	.connector_type = DRM_MODE_CONNECTOR_LVDS,
 };
 
@@ -1352,6 +1354,8 @@  static const struct panel_desc chunghwa_claa070wp03xg = {
 		.width = 94,
 		.height = 150,
 	},
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
 	.connector_type = DRM_MODE_CONNECTOR_LVDS,
 };
 
@@ -1375,6 +1379,8 @@  static const struct panel_desc chunghwa_claa101wa01a = {
 		.width = 220,
 		.height = 120,
 	},
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
 	.connector_type = DRM_MODE_CONNECTOR_LVDS,
 };
 
@@ -1398,6 +1404,8 @@  static const struct panel_desc chunghwa_claa101wb01 = {
 		.width = 223,
 		.height = 125,
 	},
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
 	.connector_type = DRM_MODE_CONNECTOR_LVDS,
 };
 
@@ -2071,6 +2079,8 @@  static const struct panel_desc innolux_n156bge_l21 = {
 		.width = 344,
 		.height = 193,
 	},
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
 	.connector_type = DRM_MODE_CONNECTOR_LVDS,
 };
 
@@ -3018,6 +3028,8 @@  static const struct panel_desc samsung_ltn101nt05 = {
 		.width = 223,
 		.height = 125,
 	},
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
 	.connector_type = DRM_MODE_CONNECTOR_LVDS,
 };