2020-06-21 22:32:05

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v1 0/2] Improve descriptions of a few simple-panels

Hello,

This is a follow up to [1], which was already applied to drm-misc and then
Laurent Pinchart spotted some problems. This series addresses those problems.

[1] https://patchwork.ozlabs.org/project/linux-tegra/patch/[email protected]/

Dmitry Osipenko (2):
drm/panel-simple: Correct EDT ET057090DHU connector type
drm/panel-simple: Add missing BUS descriptions for some panels

drivers/gpu/drm/panel/panel-simple.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

--
2.26.0


2020-06-21 22:32:27

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v1 2/2] drm/panel-simple: Add missing BUS descriptions for some panels

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 <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
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,
.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,
};

--
2.26.0

2020-06-27 20:20:15

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH v1 0/2] Improve descriptions of a few simple-panels

On Mon, Jun 22, 2020 at 01:27:40AM +0300, Dmitry Osipenko wrote:
> Hello,
>
> This is a follow up to [1], which was already applied to drm-misc and then
> Laurent Pinchart spotted some problems. This series addresses those problems.
>
> [1] https://patchwork.ozlabs.org/project/linux-tegra/patch/[email protected]/
>
> Dmitry Osipenko (2):
> drm/panel-simple: Correct EDT ET057090DHU connector type
> drm/panel-simple: Add missing BUS descriptions for some panels

Thanks for the quick fixes, both are now applied to drm-misc-next.

Sam

2020-06-27 20:46:22

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] drm/panel-simple: Add missing BUS descriptions for some panels

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 <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> 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 <[email protected]>

> .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,
> };
>

--
Regards,

Laurent Pinchart

2020-06-27 23:45:01

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] drm/panel-simple: Add missing BUS descriptions for some panels

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 <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> 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 <[email protected]>

Thanks to you and Sam!

2020-06-28 07:08:34

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] drm/panel-simple: Add missing BUS descriptions for some panels

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 <[email protected]>
> >> Signed-off-by: Dmitry Osipenko <[email protected]>
> >> ---
> >> 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 <[email protected]>
>
> Thanks to you and Sam!

--
Regards,

Laurent Pinchart

2020-06-28 07:55:38

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] drm/panel-simple: Add missing BUS descriptions for some panels

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 <[email protected]>
> > >> Signed-off-by: Dmitry Osipenko <[email protected]>
> > >> ---
> > >> 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 <[email protected]>
> >
> > Thanks to you and Sam!
>
> --
> Regards,
>
> Laurent Pinchart

2020-06-28 08:04:21

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] drm/panel-simple: Add missing BUS descriptions for some panels

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 <[email protected]>
> >>>> Signed-off-by: Dmitry Osipenko <[email protected]>
> >>>> ---
> >>>> 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 <[email protected]>

--
Regards,

Laurent Pinchart

2020-06-28 08:54:36

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] drm/panel-simple: Add missing BUS descriptions for some panels

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 <[email protected]>
>
> --
> Regards,
>
> Laurent Pinchart