diff mbox series

[v3,10/11] drm/panel: simple: Add NewEast Optoelectronics CO., LTD WJFH116008A panel support

Message ID 20190215050957.20755-11-anarsoul@gmail.com
State Changes Requested, archived
Headers show
Series Analogix ANX6345 RGB-(e)DP bridge support | expand

Checks

Context Check Description
robh/checkpatch warning "total: 0 errors, 3 warnings, 58 lines checked"

Commit Message

Vasily Khoruzhick Feb. 15, 2019, 5:09 a.m. UTC
This commit adds support for the NewEast Optoelectronics CO., LTD
WJFH116008A 11.6" 1920x1080 TFT LCD panel.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 .../display/panel/neweast,wjfh116008a.txt     |  7 ++++
 drivers/gpu/drm/panel/panel-simple.c          | 39 +++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt

Comments

Rob Herring Feb. 18, 2019, 6:33 p.m. UTC | #1
On Thu, Feb 14, 2019 at 09:09:56PM -0800, Vasily Khoruzhick wrote:
> This commit adds support for the NewEast Optoelectronics CO., LTD
> WJFH116008A 11.6" 1920x1080 TFT LCD panel.
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  .../display/panel/neweast,wjfh116008a.txt     |  7 ++++
>  drivers/gpu/drm/panel/panel-simple.c          | 39 +++++++++++++++++++
>  2 files changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt b/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
> new file mode 100644
> index 000000000000..d76579f9f55e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
> @@ -0,0 +1,7 @@
> +NewEast Optoelectronics CO., LTD WJFH116008A 11.6" 1920x1080 TFT LCD panel
> +
> +Required properties:
> +- compatible: should be "neweast,wjfh116008a"
> +
> +This binding is compatible with the simple-panel binding, which is specified
> +in simple-panel.txt in this directory.

We already established that this goes thru a standard eDP connector. We 
should describe that and everything associated with it.

Rob
Vasily Khoruzhick Feb. 18, 2019, 7:06 p.m. UTC | #2
On Mon, Feb 18, 2019 at 10:33 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Feb 14, 2019 at 09:09:56PM -0800, Vasily Khoruzhick wrote:
> > This commit adds support for the NewEast Optoelectronics CO., LTD
> > WJFH116008A 11.6" 1920x1080 TFT LCD panel.
> >
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> >  .../display/panel/neweast,wjfh116008a.txt     |  7 ++++
> >  drivers/gpu/drm/panel/panel-simple.c          | 39 +++++++++++++++++++
> >  2 files changed, 46 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt b/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
> > new file mode 100644
> > index 000000000000..d76579f9f55e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
> > @@ -0,0 +1,7 @@
> > +NewEast Optoelectronics CO., LTD WJFH116008A 11.6" 1920x1080 TFT LCD panel
> > +
> > +Required properties:
> > +- compatible: should be "neweast,wjfh116008a"
> > +
> > +This binding is compatible with the simple-panel binding, which is specified
> > +in simple-panel.txt in this directory.
>
> We already established that this goes thru a standard eDP connector. We
> should describe that and everything associated with it.

I believe using eDP connector binding wouldn't help much in my case
and it won't improve accuracy of hardware description while adding
unnecessary code duplication (edp-connector will be pretty much
simple-panel).

Since currently there're no standalone connector drivers, implementing
one requires significant refactoring of the code that I'm not
familiar.

> Rob
Rob Herring Feb. 19, 2019, 2:54 p.m. UTC | #3
On Mon, Feb 18, 2019 at 1:07 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> On Mon, Feb 18, 2019 at 10:33 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Feb 14, 2019 at 09:09:56PM -0800, Vasily Khoruzhick wrote:
> > > This commit adds support for the NewEast Optoelectronics CO., LTD
> > > WJFH116008A 11.6" 1920x1080 TFT LCD panel.
> > >
> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > ---
> > >  .../display/panel/neweast,wjfh116008a.txt     |  7 ++++
> > >  drivers/gpu/drm/panel/panel-simple.c          | 39 +++++++++++++++++++
> > >  2 files changed, 46 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt b/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
> > > new file mode 100644
> > > index 000000000000..d76579f9f55e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
> > > @@ -0,0 +1,7 @@
> > > +NewEast Optoelectronics CO., LTD WJFH116008A 11.6" 1920x1080 TFT LCD panel
> > > +
> > > +Required properties:
> > > +- compatible: should be "neweast,wjfh116008a"
> > > +
> > > +This binding is compatible with the simple-panel binding, which is specified
> > > +in simple-panel.txt in this directory.
> >
> > We already established that this goes thru a standard eDP connector. We
> > should describe that and everything associated with it.
>
> I believe using eDP connector binding wouldn't help much in my case
> and it won't improve accuracy of hardware description while adding
> unnecessary code duplication (edp-connector will be pretty much
> simple-panel).
>
> Since currently there're no standalone connector drivers, implementing
> one requires significant refactoring of the code that I'm not
> familiar.

I'm not talking about drivers. I'm talking about bindings. Those are
not necessarily 1-1. There's no reason the simple panel driver can't
have an 'edp-connector' entry.

Also, since you have EDID, you should be using that for timing data
IMO, and the binding needs to have enough information to support that.
It may if DP-aux comes from the bridge chip or it may not if you need
to describe that connection. Again, this is independent from what
Linux chooses to do. If Linux chooses to have its own timing
information that's its choice. Another OS may choose to use EDID.

Rob
Vasily Khoruzhick Feb. 19, 2019, 9:35 p.m. UTC | #4
On Tue, Feb 19, 2019 at 6:54 AM Rob Herring <robh@kernel.org> wrote:

> > I believe using eDP connector binding wouldn't help much in my case
> > and it won't improve accuracy of hardware description while adding
> > unnecessary code duplication (edp-connector will be pretty much
> > simple-panel).
> >
> > Since currently there're no standalone connector drivers, implementing
> > one requires significant refactoring of the code that I'm not
> > familiar.
>
> I'm not talking about drivers. I'm talking about bindings. Those are
> not necessarily 1-1. There's no reason the simple panel driver can't
> have an 'edp-connector' entry.

These aren't independent things. Bindings are useless without drivers.

Also how are you going to address mainlined platforms that have eDP
and use panel bindings? E.g. rk3399 supports eDP and uses panel
binding - see rk3399-gru-kevin.dts as example.

> Also, since you have EDID, you should be using that for timing data
> IMO, and the binding needs to have enough information to support that.
> It may if DP-aux comes from the bridge chip or it may not if you need
> to describe that connection. Again, this is independent from what
> Linux chooses to do. If Linux chooses to have its own timing
> information that's its choice. Another OS may choose to use EDID.

I see nothing wrong here - anx6345 driver reads EDID (over AUX) and
uses timing from it, if reading EDID failed it fallback to timings
defined in panel driver.

> Rob
Rob Herring Feb. 22, 2019, 6:37 p.m. UTC | #5
On Tue, Feb 19, 2019 at 01:35:26PM -0800, Vasily Khoruzhick wrote:
> On Tue, Feb 19, 2019 at 6:54 AM Rob Herring <robh@kernel.org> wrote:
> 
> > > I believe using eDP connector binding wouldn't help much in my case
> > > and it won't improve accuracy of hardware description while adding
> > > unnecessary code duplication (edp-connector will be pretty much
> > > simple-panel).
> > >
> > > Since currently there're no standalone connector drivers, implementing
> > > one requires significant refactoring of the code that I'm not
> > > familiar.
> >
> > I'm not talking about drivers. I'm talking about bindings. Those are
> > not necessarily 1-1. There's no reason the simple panel driver can't
> > have an 'edp-connector' entry.
> 
> These aren't independent things. Bindings are useless without drivers.

What I mean is bindings for panels and connectors are basically the same 
thing. The kernel handles them quite differently, but that is a kernel 
design decision independent of bindings. Another client (of DT) may do 
things differently or the kernel may do things differently in the 
future.

There is not any simple panel binding really. This originated I think 
from a 'simple-panel' compatible that was originally attempted. What we 
have is a collection of common properties for panels which panel 
bindings can use. And we have some common panel docs too. I've been 
meaning to clean-up and unify all this. What I'd like is a pool of 
common properties for any connector or panel to use. Then we can define 
what subset any panel or connector use. When there's something standard 
across devices like LVDS modes or eDP connectors, then we can further 
define a subset. It is easiest if these subsets are also encapsulated by 
a compatible string, but that's not required and some times undesirable. 
For the latter, the 'simple-panel' compatible is what I'm thinking of. 
It needs to have a well defined meaning such as following some spec.

> Also how are you going to address mainlined platforms that have eDP
> and use panel bindings? E.g. rk3399 supports eDP and uses panel
> binding - see rk3399-gru-kevin.dts as example.

I'm not.

Supporting eDP and having a standard eDP connector are 2 different 
things. I'd guess chromebooks would do something standard, but I've got 
no idea. Assuming they do, there's nothing to do for existing bindings. 
They are already defined. But I imagine having a 'edp-connector' 
fallback would be helpful to the chromebook folks.

We can easily map a specific binding to a specific or generic driver. So 
if we ever have a generic eDP connector driver, then we could perhaps 
move rk3399-gru-kevin to use that. Kind of depends if it matches 
whatever set of properties is defined for eDP connectors.

> > Also, since you have EDID, you should be using that for timing data
> > IMO, and the binding needs to have enough information to support that.
> > It may if DP-aux comes from the bridge chip or it may not if you need
> > to describe that connection. Again, this is independent from what
> > Linux chooses to do. If Linux chooses to have its own timing
> > information that's its choice. Another OS may choose to use EDID.
> 
> I see nothing wrong here - anx6345 driver reads EDID (over AUX) and
> uses timing from it, if reading EDID failed it fallback to timings
> defined in panel driver.

Okay, I didn't look at the driver part closely. So really, it's just 
having a fallback compatible and defining what that includes.

Rob
Vasily Khoruzhick March 7, 2019, 6:12 a.m. UTC | #6
On Fri, Feb 22, 2019 at 10:37 AM Rob Herring <robh@kernel.org> wrote:

> There is not any simple panel binding really. This originated I think
> from a 'simple-panel' compatible that was originally attempted. What we
> have is a collection of common properties for panels which panel
> bindings can use. And we have some common panel docs too. I've been
> meaning to clean-up and unify all this.

I'm afraid I'm not right person to clean it up. My understanding of
what different kind of panels and connectors need is pretty limited.
Moreover I'm doing pinebook hacking in my spare time which is few
hours a week and major panel/connector bindings clean up is not
something I want to invest my time in.

Since it seems to be a blocker I'll wait for someone to clean up
panel/connector bindings and then respin the series. Meanwhile it'll
live in my github.

Thanks for review and sharing your ideas.

Regards,
Vasily
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt b/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
new file mode 100644
index 000000000000..d76579f9f55e
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
@@ -0,0 +1,7 @@ 
+NewEast Optoelectronics CO., LTD WJFH116008A 11.6" 1920x1080 TFT LCD panel
+
+Required properties:
+- compatible: should be "neweast,wjfh116008a"
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 9a4c9dd02c6c..3edc77e49d8f 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1843,6 +1843,42 @@  static const struct panel_desc netron_dy_e231732 = {
 	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
 };
 
+static const struct drm_display_mode neweast_wjfh116008a_modes[] = {
+{
+	.clock = 138500,
+	.hdisplay = 1920,
+	.hsync_start = 1920 + 48,
+	.hsync_end = 1920 + 48 + 32,
+	.htotal = 1920 + 48 + 32 + 80,
+	.vdisplay = 1080,
+	.vsync_start = 1080 + 3,
+	.vsync_end = 1080 + 3 + 5,
+	.vtotal = 1080 + 3 + 5 + 23,
+	.vrefresh = 60,
+	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+}, {
+	.clock = 110920,
+	.hdisplay = 1920,
+	.hsync_start = 1920 + 48,
+	.hsync_end = 1920 + 48 + 32,
+	.htotal = 1920 + 48 + 32 + 80,
+	.vdisplay = 1080,
+	.vsync_start = 1080 + 3,
+	.vsync_end = 1080 + 3 + 5,
+	.vtotal = 1080 + 3 + 5 + 23,
+	.vrefresh = 48,
+	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+} };
+
+static const struct panel_desc neweast_wjfh116008a = {
+	.modes = neweast_wjfh116008a_modes,
+	.num_modes = 2,
+	.size = {
+		.width = 260,
+		.height = 150,
+	},
+};
+
 static const struct drm_display_mode newhaven_nhd_43_480272ef_atxl_mode = {
 	.clock = 9000,
 	.hdisplay = 480,
@@ -2690,6 +2726,9 @@  static const struct of_device_id platform_of_match[] = {
 	}, {
 		.compatible = "netron-dy,e231732",
 		.data = &netron_dy_e231732,
+	}, {
+		.compatible = "neweast,wjfh116008a",
+		.data = &neweast_wjfh116008a,
 	}, {
 		.compatible = "newhaven,nhd-4.3-480272ef-atxl",
 		.data = &newhaven_nhd_43_480272ef_atxl,