diff mbox series

[1/2] drm/panel: Add Starry KR070PE2T

Message ID 20200310102725.14591-2-dev@pascalroeleven.nl
State Changes Requested, archived
Headers show
Series Add support for Topwise A721 tablet | expand

Checks

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

Commit Message

Pascal Roeleven March 10, 2020, 10:27 a.m. UTC 
The KR070PE2T is a 7" panel with a resolution of 800x480.

KR070PE2T is the marking present on the ribbon cable. As this panel is
probably available under different brands, this marking will catch
most devices.

Signed-off-by: Pascal Roeleven <dev@pascalroeleven.nl>
---
 .../display/panel/starry,kr070pe2t.txt        |  7 +++++
 drivers/gpu/drm/panel/panel-simple.c          | 26 +++++++++++++++++++
 2 files changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt

Comments

Sam Ravnborg March 10, 2020, 6:54 p.m. UTC | #1 
Hi Pascal.

Thanks for submitting.

On Tue, Mar 10, 2020 at 11:27:23AM +0100, Pascal Roeleven wrote:
> The KR070PE2T is a 7" panel with a resolution of 800x480.
> 
> KR070PE2T is the marking present on the ribbon cable. As this panel is
> probably available under different brands, this marking will catch
> most devices.
> 
> Signed-off-by: Pascal Roeleven <dev@pascalroeleven.nl>

A few things to improve.

The binding should be a separate patch.
subject - shall start with dt-bindings:
Shall be sent to deveicetree mailing list.

For panel we no longer accept .txt bindings.
But the good news is that since the panel is simple,
you only need to list your compatible in the file
bindings/display/panel/panel-simple.yaml
- must be en alphabetical order
- vendor prefix must be present in vendor-prefixes



> ---
>  .../display/panel/starry,kr070pe2t.txt        |  7 +++++
>  drivers/gpu/drm/panel/panel-simple.c          | 26 +++++++++++++++++++
>  2 files changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt b/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt
> new file mode 100644
> index 000000000..699ad5eb2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt
> @@ -0,0 +1,7 @@
> +Starry 7" (800x480 pixels) LCD panel
> +
> +Required properties:
> +- compatible: should be "starry,kr070pe2t"
> +
> +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 e14c14ac6..027a2612b 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -2842,6 +2842,29 @@ static const struct panel_desc shelly_sca07010_bfn_lnn = {
>  	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
>  };
>  
> +static const struct drm_display_mode starry_kr070pe2t_mode = {
> +	.clock = 33000,
> +	.hdisplay = 800,
> +	.hsync_start = 800 + 209,
> +	.hsync_end = 800 + 209 + 1,
> +	.htotal = 800 + 209 + 1 + 45,
> +	.vdisplay = 480,
> +	.vsync_start = 480 + 22,
> +	.vsync_end = 480 + 22 + 1,
> +	.vtotal = 480 + 22 + 1 + 22,
> +	.vrefresh = 60,
> +};

Please adjust so:
vrefresh * htotal * vtotal == clock.
I cannot say what needs to be adjusted.
But we are moving away from specifying vrefresh and want the
data to be OK.

> +
> +static const struct panel_desc starry_kr070pe2t = {
> +	.modes = &starry_kr070pe2t_mode,
> +	.num_modes = 1,
> +	.bpc = 8,
> +	.size = {
> +		.width = 152,
> +		.height = 86,
> +	},
> +};

For this part specifying the connector type is today mandatory.
And please investigate if you can be more precise concerning
bus_format, flags, etc.
See also what other panels do in the same file.

	Sam


> +
>  static const struct drm_display_mode starry_kr122ea0sra_mode = {
>  	.clock = 147000,
>  	.hdisplay = 1920,
> @@ -3474,6 +3497,9 @@ static const struct of_device_id platform_of_match[] = {
>  	}, {
>  		.compatible = "shelly,sca07010-bfn-lnn",
>  		.data = &shelly_sca07010_bfn_lnn,
> +	}, {
> +		.compatible = "starry,kr070pe2t",
> +		.data = &starry_kr070pe2t,
>  	}, {
>  		.compatible = "starry,kr122ea0sra",
>  		.data = &starry_kr122ea0sra,
> -- 
> 2.20.1
Ville Syrjälä March 10, 2020, 7:10 p.m. UTC | #2 
On Tue, Mar 10, 2020 at 07:54:23PM +0100, Sam Ravnborg wrote:
> Hi Pascal.
> 
> Thanks for submitting.
> 
> On Tue, Mar 10, 2020 at 11:27:23AM +0100, Pascal Roeleven wrote:
> > The KR070PE2T is a 7" panel with a resolution of 800x480.
> > 
> > KR070PE2T is the marking present on the ribbon cable. As this panel is
> > probably available under different brands, this marking will catch
> > most devices.
> > 
> > Signed-off-by: Pascal Roeleven <dev@pascalroeleven.nl>
> 
> A few things to improve.
> 
> The binding should be a separate patch.
> subject - shall start with dt-bindings:
> Shall be sent to deveicetree mailing list.
> 
> For panel we no longer accept .txt bindings.
> But the good news is that since the panel is simple,
> you only need to list your compatible in the file
> bindings/display/panel/panel-simple.yaml
> - must be en alphabetical order
> - vendor prefix must be present in vendor-prefixes
> 
> 
> 
> > ---
> >  .../display/panel/starry,kr070pe2t.txt        |  7 +++++
> >  drivers/gpu/drm/panel/panel-simple.c          | 26 +++++++++++++++++++
> >  2 files changed, 33 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt b/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt
> > new file mode 100644
> > index 000000000..699ad5eb2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt
> > @@ -0,0 +1,7 @@
> > +Starry 7" (800x480 pixels) LCD panel
> > +
> > +Required properties:
> > +- compatible: should be "starry,kr070pe2t"
> > +
> > +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 e14c14ac6..027a2612b 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -2842,6 +2842,29 @@ static const struct panel_desc shelly_sca07010_bfn_lnn = {
> >  	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> >  };
> >  
> > +static const struct drm_display_mode starry_kr070pe2t_mode = {
> > +	.clock = 33000,
> > +	.hdisplay = 800,
> > +	.hsync_start = 800 + 209,
> > +	.hsync_end = 800 + 209 + 1,
> > +	.htotal = 800 + 209 + 1 + 45,
> > +	.vdisplay = 480,
> > +	.vsync_start = 480 + 22,
> > +	.vsync_end = 480 + 22 + 1,
> > +	.vtotal = 480 + 22 + 1 + 22,
> > +	.vrefresh = 60,
> > +};
> 
> Please adjust so:
> vrefresh * htotal * vtotal == clock.
> I cannot say what needs to be adjusted.
> But we are moving away from specifying vrefresh and want the
> data to be OK.

This one actually looks OK to me. Unless I typoed the numbers
the timings give us a vrefresh of 59.58 which gets rounded to 60.
So no change once .vrefresh disappears AFAICS.
Pascal Roeleven March 11, 2020, 10:23 a.m. UTC | #3 
On 2020-03-10 19:54, Sam Ravnborg wrote:
> A few things to improve.
> 
> The binding should be a separate patch.
> subject - shall start with dt-bindings:
> Shall be sent to deveicetree mailing list.

Hi Sam,

Thank you very much for your review.
I did consider this. The reason I combined the patches, is that the 
binding depends on the display so I thought they were related in some 
way. Didn't know the correct procedure to handle this. I will split them 
apart in v2.

>> ---
>>  .../display/panel/starry,kr070pe2t.txt        |  7 +++++
>>  drivers/gpu/drm/panel/panel-simple.c          | 26 
>> +++++++++++++++++++
>>  2 files changed, 33 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt 
>> b/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt
>> new file mode 100644
>> index 000000000..699ad5eb2
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt
>> @@ -0,0 +1,7 @@
>> +Starry 7" (800x480 pixels) LCD panel
>> +
>> +Required properties:
>> +- compatible: should be "starry,kr070pe2t"
>> +
>> +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 e14c14ac6..027a2612b 100644
>> --- a/drivers/gpu/drm/panel/panel-simple.c
>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>> @@ -2842,6 +2842,29 @@ static const struct panel_desc 
>> shelly_sca07010_bfn_lnn = {
>>  	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
>>  };
>> 
>> +static const struct drm_display_mode starry_kr070pe2t_mode = {
>> +	.clock = 33000,
>> +	.hdisplay = 800,
>> +	.hsync_start = 800 + 209,
>> +	.hsync_end = 800 + 209 + 1,
>> +	.htotal = 800 + 209 + 1 + 45,
>> +	.vdisplay = 480,
>> +	.vsync_start = 480 + 22,
>> +	.vsync_end = 480 + 22 + 1,
>> +	.vtotal = 480 + 22 + 1 + 22,
>> +	.vrefresh = 60,
>> +};
> 
> Please adjust so:
> vrefresh * htotal * vtotal == clock.
> I cannot say what needs to be adjusted.
> But we are moving away from specifying vrefresh and want the
> data to be OK.

Just like Ville Syrjälä, I ran the numbers and vrefresh indeed 
calculates to 59.58.
Rob Herring (Arm) March 23, 2020, 9:27 p.m. UTC | #4 
On Wed, Mar 11, 2020 at 11:23:27AM +0100, Pascal Roeleven wrote:
> On 2020-03-10 19:54, Sam Ravnborg wrote:
> > A few things to improve.
> > 
> > The binding should be a separate patch.
> > subject - shall start with dt-bindings:
> > Shall be sent to deveicetree mailing list.
> 
> Hi Sam,
> 
> Thank you very much for your review.
> I did consider this. The reason I combined the patches, is that the binding
> depends on the display so I thought they were related in some way. Didn't
> know the correct procedure to handle this. I will split them apart in v2.

FYI, checkpatch.pl will tell you both bindings should be a separate 
patch and that they should be in DT schema format.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt b/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt
new file mode 100644
index 000000000..699ad5eb2
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/starry,kr070pe2t.txt
@@ -0,0 +1,7 @@ 
+Starry 7" (800x480 pixels) LCD panel
+
+Required properties:
+- compatible: should be "starry,kr070pe2t"
+
+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 e14c14ac6..027a2612b 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -2842,6 +2842,29 @@  static const struct panel_desc shelly_sca07010_bfn_lnn = {
 	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
 };
 
+static const struct drm_display_mode starry_kr070pe2t_mode = {
+	.clock = 33000,
+	.hdisplay = 800,
+	.hsync_start = 800 + 209,
+	.hsync_end = 800 + 209 + 1,
+	.htotal = 800 + 209 + 1 + 45,
+	.vdisplay = 480,
+	.vsync_start = 480 + 22,
+	.vsync_end = 480 + 22 + 1,
+	.vtotal = 480 + 22 + 1 + 22,
+	.vrefresh = 60,
+};
+
+static const struct panel_desc starry_kr070pe2t = {
+	.modes = &starry_kr070pe2t_mode,
+	.num_modes = 1,
+	.bpc = 8,
+	.size = {
+		.width = 152,
+		.height = 86,
+	},
+};
+
 static const struct drm_display_mode starry_kr122ea0sra_mode = {
 	.clock = 147000,
 	.hdisplay = 1920,
@@ -3474,6 +3497,9 @@  static const struct of_device_id platform_of_match[] = {
 	}, {
 		.compatible = "shelly,sca07010-bfn-lnn",
 		.data = &shelly_sca07010_bfn_lnn,
+	}, {
+		.compatible = "starry,kr070pe2t",
+		.data = &starry_kr070pe2t,
 	}, {
 		.compatible = "starry,kr122ea0sra",
 		.data = &starry_kr122ea0sra,