[PATCH v3 0/2] Add WL-355608-A8 panel

[PATCH v3 0/2] Add WL-355608-A8 panel

[Ryan Walklin]

Hello,

V3 of this patch with further cleanup to DT binding example code and whitespace fixes in driver code. No functional change from V2.

Original cover below.
--

The WL_355608_A8 panel is a VGA LCD display with an NV3052C-compatible driver IC, used in a number of Anbernic handheld gaming devices. This patch adds a device tree binding, and support for the display timings and init sequence to the NV3052C SPI/RGB driver.

Regards,

Ryan

Ryan Walklin (2):
  dt-bindings: display: panel: Add WL-355608-A8 panel
  drm: panel: nv3052c: Add WL-355608-A8 panel

 .../bindings/display/panel/wl-355608-a8.yaml  |  60 +++++
 .../gpu/drm/panel/panel-newvision-nv3052c.c   | 225 ++++++++++++++++++
 2 files changed, 285 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/wl-355608-a8.yaml

-- 
2.45.1

[PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Ryan Walklin]

The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display used in a
number of handheld gaming devices made by Anbernic. By consensus a
vendor prefix is not provided as the panel OEM is unknown.

Add a device tree binding for the panel.

Signed-off-by: Ryan Walklin <ryan@testtoast.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

---

Changelog v1..v2:
- Correct compatible string and filename
- Note #dri-devel discussion regarding vendor prefix
- Remove unnecessary node names, spi-gpio compatible string and GPIOs from example

Changelog v2..v3:
- Remove errant tab and un-needed spi node label from example
- Add Reviewed-by tag
---
 .../bindings/display/panel/wl-355608-a8.yaml  | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/wl-355608-a8.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/wl-355608-a8.yaml b/Documentation/devicetree/bindings/display/panel/wl-355608-a8.yaml
new file mode 100644
index 0000000000..e552d01b52
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/wl-355608-a8.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/wl-355608-a8.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: WL-355608-A8 3.5" (640x480 pixels) 24-bit IPS LCD panel
+
+maintainers:
+  - Ryan Walklin <ryan@testtoast.com>
+
+allOf:
+  - $ref: panel-common.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: wl-355608-a8
+
+  reg:
+    maxItems: 1
+
+  spi-3wire: true
+
+required:
+  - compatible
+  - reg
+  - port
+  - power-supply
+  - reset-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        panel@0 {
+            compatible = "wl-355608-a8";
+            reg = <0>;
+
+            spi-3wire;
+            spi-max-frequency = <3125000>;
+
+            reset-gpios = <&pio 8 14 GPIO_ACTIVE_LOW>; // PI14
+
+            backlight = <&backlight>;
+            power-supply = <&reg_lcd>;
+
+            port {
+                endpoint {
+                    remote-endpoint = <&tcon_lcd0_out_lcd>;
+                };
+            };
+        };
+    };
-- 
2.45.1

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Maxime Ripard]

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

On Fri, May 31, 2024 at 09:12:14AM GMT, Ryan Walklin wrote:
> The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display used in a
> number of handheld gaming devices made by Anbernic. By consensus a
> vendor prefix is not provided as the panel OEM is unknown.

Where has this consensus been found?

I had a look at the previous discussions, and I can't find any consensus
being reached there. And for that kind of thing, having the ack or
review of any of the DT maintainers would have been great.

For this kind of cases, we usually use the device it's attached to as
the vendor, so anbernic in this case. Can you send a followup patch?

Maxime

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

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Neil Armstrong]

On 06/06/2024 11:32, Maxime Ripard wrote:
> On Fri, May 31, 2024 at 09:12:14AM GMT, Ryan Walklin wrote:
>> The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display used in a
>> number of handheld gaming devices made by Anbernic. By consensus a
>> vendor prefix is not provided as the panel OEM is unknown.
> 
> Where has this consensus been found?
> 
> I had a look at the previous discussions, and I can't find any consensus
> being reached there. And for that kind of thing, having the ack or
> review of any of the DT maintainers would have been great.

There was a consensus with Conor, this is why he acked v2, see
https://lore.kernel.org/all/20240525-velvet-citable-a45dd06847a7@spud/

```
I think if we genuinely do not know what the vendor is then we just
don't have a prefix.
```

I agree with Conor so I applied the patchset after Connor reviewed it and the comment was fixed in v3:
https://lore.kernel.org/all/20240530-satchel-playgroup-e8aa6937b8b9@spud/

> 
> For this kind of cases, we usually use the device it's attached to as
> the vendor, so anbernic in this case. Can you send a followup patch?
> 
> Maxime

Neil

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Ryan Walklin]

On Thu, 6 Jun 2024, at 9:32 PM, Maxime Ripard wrote:
Hi Maxime, thanks for reviewing.

> Where has this consensus been found?

As Neil notes Conor suggested it [1], and we did consider Hironori's suggestion [2] of using anbernic as the vendor prefix, although my (not strong) feeling at the time was because Anbernic is not the panel vendor, just integrating an unknown OEM's panel into their devices, so at the time I fit was not quite accurate to say Anbernic was the vendor.

Some discussion was also had on IRC at #linux-sunxi [3]. Admittedly not a *broad* consensus, but all offered opinions were taken and the patch was accepted subsequently.

> For this kind of cases, we usually use the device it's attached to as
> the vendor, so anbernic in this case. Can you send a followup patch?

Thanks for the clarification, I was not aware of this. Happy to prepare one but will perhaps wait to see if there are any other comments.

> Maxime

Regards,

Ryan

[1] https://lore.kernel.org/dri-devel/20240525-velvet-citable-a45dd06847a7@spud/
[2] https://lore.kernel.org/dri-devel/CAG40kxGEw4AyHk3P_dixKRdesGT0pLtMfpYcCCTbVGUs2NMD-w@mail.gmail.com/
[3] https://oftc.irclog.whitequark.org/linux-sunxi/2024-05-27#332264

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Maxime Ripard]

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

On Thu, Jun 06, 2024 at 09:48:54PM GMT, Ryan Walklin wrote:
> On Thu, 6 Jun 2024, at 9:32 PM, Maxime Ripard wrote:
> Hi Maxime, thanks for reviewing.
> 
> > Where has this consensus been found?
> 
> As Neil notes Conor suggested it [1], and we did consider Hironori's
> suggestion [2] of using anbernic as the vendor prefix, although my
> (not strong) feeling at the time was because Anbernic is not the panel
> vendor, just integrating an unknown OEM's panel into their devices, so
> at the time I fit was not quite accurate to say Anbernic was the
> vendor.
> 
> Some discussion was also had on IRC at #linux-sunxi [3]. Admittedly
> not a *broad* consensus, but all offered opinions were taken and the
> patch was accepted subsequently.

Respectfully, #linux-sunxi isn't the persons you should be discussing
this with.

Maxime

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

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Maxime Ripard]

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

On Thu, Jun 06, 2024 at 11:37:31AM GMT, Neil Armstrong wrote:
> On 06/06/2024 11:32, Maxime Ripard wrote:
> > On Fri, May 31, 2024 at 09:12:14AM GMT, Ryan Walklin wrote:
> > > The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display used in a
> > > number of handheld gaming devices made by Anbernic. By consensus a
> > > vendor prefix is not provided as the panel OEM is unknown.
> > 
> > Where has this consensus been found?
> > 
> > I had a look at the previous discussions, and I can't find any consensus
> > being reached there. And for that kind of thing, having the ack or
> > review of any of the DT maintainers would have been great.
> 
> There was a consensus with Conor, this is why he acked v2, see
> https://lore.kernel.org/all/20240525-velvet-citable-a45dd06847a7@spud/

It's probably a matter of semantics here, but if it's with only one
person, it's not a consensus but an agreement.

> ```
> I think if we genuinely do not know what the vendor is then we just
> don't have a prefix.
> ```

And even then, I don't interpret Conor's statement as a formal agreement
but rather an acknowledgment of the issue.

> I agree with Conor so I applied the patchset after Connor reviewed it and the comment was fixed in v3:
> https://lore.kernel.org/all/20240530-satchel-playgroup-e8aa6937b8b9@spud/

Yeah, I know. Still, it's a major deviation to what we've always been
doing, getting the DT maintainers voice on that would have been a good
idea.

Maxime

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

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Conor Dooley]

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

On Thu, Jun 06, 2024 at 01:23:03PM +0200, Maxime Ripard wrote:
> On Thu, Jun 06, 2024 at 11:37:31AM GMT, Neil Armstrong wrote:
> > On 06/06/2024 11:32, Maxime Ripard wrote:
> > > On Fri, May 31, 2024 at 09:12:14AM GMT, Ryan Walklin wrote:
> > > > The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display used in a
> > > > number of handheld gaming devices made by Anbernic. By consensus a
> > > > vendor prefix is not provided as the panel OEM is unknown.
> > > 
> > > Where has this consensus been found?
> > > 
> > > I had a look at the previous discussions, and I can't find any consensus
> > > being reached there. And for that kind of thing, having the ack or
> > > review of any of the DT maintainers would have been great.
> > 
> > There was a consensus with Conor, this is why he acked v2, see
> > https://lore.kernel.org/all/20240525-velvet-citable-a45dd06847a7@spud/
> 
> It's probably a matter of semantics here, but if it's with only one
> person, it's not a consensus but an agreement.
> 
> > ```
> > I think if we genuinely do not know what the vendor is then we just
> > don't have a prefix.
> > ```
> 
> And even then, I don't interpret Conor's statement as a formal agreement
> but rather an acknowledgment of the issue.

I mean, I specifically left an r-b below that line in v2:
https://lore.kernel.org/all/20240530-satchel-playgroup-e8aa6937b8b9@spud/

I'm not a displays guy, so my sources were limited to what I could find
from search engines, but I spent some time looking for an actual vendor
of the panel and could not. All I found was various listings on places
like AliExpress that did not mention an manufacturer. I'd rather not
invent a vendor because we could not find the actual vendor of the
panel & it seemed rather unreasonable to block support for the device
on the basis of not being able to figure out the vendor. If you, as
someone knowledgeable on displays, can figure the vendor out, then
yeah we should definitely add it.

> > I agree with Conor so I applied the patchset after Connor reviewed it and the comment was fixed in v3:
> > https://lore.kernel.org/all/20240530-satchel-playgroup-e8aa6937b8b9@spud/
> 
> Yeah, I know. Still, it's a major deviation to what we've always been
> doing, getting the DT maintainers voice on that would have been a good
> idea.

Is it a consensus of DT maintainers you're looking for?

Cheers,
Conor.

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

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Maxime Ripard]

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

Hi Conor,

Sorry, I missed the news of you becoming a DT maintainer, so most of my
previous points are obviously bogus. And congrats :)

On Thu, Jun 06, 2024 at 12:51:33PM GMT, Conor Dooley wrote:
> On Thu, Jun 06, 2024 at 01:23:03PM +0200, Maxime Ripard wrote:
> > On Thu, Jun 06, 2024 at 11:37:31AM GMT, Neil Armstrong wrote:
> > > On 06/06/2024 11:32, Maxime Ripard wrote:
> > > > On Fri, May 31, 2024 at 09:12:14AM GMT, Ryan Walklin wrote:
> > > > > The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display used in a
> > > > > number of handheld gaming devices made by Anbernic. By consensus a
> > > > > vendor prefix is not provided as the panel OEM is unknown.
> > > > 
> > > > Where has this consensus been found?
> > > > 
> > > > I had a look at the previous discussions, and I can't find any consensus
> > > > being reached there. And for that kind of thing, having the ack or
> > > > review of any of the DT maintainers would have been great.
> > > 
> > > There was a consensus with Conor, this is why he acked v2, see
> > > https://lore.kernel.org/all/20240525-velvet-citable-a45dd06847a7@spud/
> > 
> > It's probably a matter of semantics here, but if it's with only one
> > person, it's not a consensus but an agreement.
> > 
> > > ```
> > > I think if we genuinely do not know what the vendor is then we just
> > > don't have a prefix.
> > > ```
> > 
> > And even then, I don't interpret Conor's statement as a formal agreement
> > but rather an acknowledgment of the issue.
> 
> I mean, I specifically left an r-b below that line in v2:
> https://lore.kernel.org/all/20240530-satchel-playgroup-e8aa6937b8b9@spud/
> 
> I'm not a displays guy, so my sources were limited to what I could find
> from search engines, but I spent some time looking for an actual vendor
> of the panel and could not. All I found was various listings on places
> like AliExpress that did not mention an manufacturer. I'd rather not
> invent a vendor because we could not find the actual vendor of the
> panel & it seemed rather unreasonable to block support for the device
> on the basis of not being able to figure out the vendor. If you, as
> someone knowledgeable on displays, can figure the vendor out, then
> yeah we should definitely add it.

It's still a bit surprising to me. We've merged[1][2][3][4], and are still
merging[5], panels from this particular vendor that have no clearly
identified OEMs. Just like any other panel, really. We almost *never*
have the actual OEM, we just go with whatever is the easiest to identify
it.

Plus, if there ever is another WL-355608-A8 part from a completely
unrelated vendor, then you'll have a naming clash with no clear
indication about which is which.

Maxime

1: https://lore.kernel.org/all/20230426143213.4178586-1-macroalpha82@gmail.com/
2: https://lore.kernel.org/all/20231003163355.143704-1-macroalpha82@gmail.com/
3: https://lore.kernel.org/all/20231117202536.1387815-1-macroalpha82@gmail.com/
4: https://lore.kernel.org/all/20231208154847.130615-1-macroalpha82@gmail.com/
5: https://lore.kernel.org/dri-devel/20240618081515.1215552-1-kikuchan98@gmail.com/

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

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Conor Dooley]

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

On Tue, Jun 18, 2024 at 11:04:09AM +0200, Maxime Ripard wrote:
> Hi Conor,
> 
> Sorry, I missed the news of you becoming a DT maintainer, so most of my
> previous points are obviously bogus. And congrats :)

I've been doing it for over a year, so news travels to some corners slowly
I guess. I'm not just being a pest in dozens of subsystems for fun!

> On Thu, Jun 06, 2024 at 12:51:33PM GMT, Conor Dooley wrote:
> > On Thu, Jun 06, 2024 at 01:23:03PM +0200, Maxime Ripard wrote:
> > > On Thu, Jun 06, 2024 at 11:37:31AM GMT, Neil Armstrong wrote:
> > > > On 06/06/2024 11:32, Maxime Ripard wrote:
> > > > > On Fri, May 31, 2024 at 09:12:14AM GMT, Ryan Walklin wrote:
> > > > > > The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display used in a
> > > > > > number of handheld gaming devices made by Anbernic. By consensus a
> > > > > > vendor prefix is not provided as the panel OEM is unknown.
> > > > > 
> > > > > Where has this consensus been found?
> > > > > 
> > > > > I had a look at the previous discussions, and I can't find any consensus
> > > > > being reached there. And for that kind of thing, having the ack or
> > > > > review of any of the DT maintainers would have been great.
> > > > 
> > > > There was a consensus with Conor, this is why he acked v2, see
> > > > https://lore.kernel.org/all/20240525-velvet-citable-a45dd06847a7@spud/
> > > 
> > > It's probably a matter of semantics here, but if it's with only one
> > > person, it's not a consensus but an agreement.
> > > 
> > > > ```
> > > > I think if we genuinely do not know what the vendor is then we just
> > > > don't have a prefix.
> > > > ```
> > > 
> > > And even then, I don't interpret Conor's statement as a formal agreement
> > > but rather an acknowledgment of the issue.
> > 
> > I mean, I specifically left an r-b below that line in v2:
> > https://lore.kernel.org/all/20240530-satchel-playgroup-e8aa6937b8b9@spud/
> > 
> > I'm not a displays guy, so my sources were limited to what I could find
> > from search engines, but I spent some time looking for an actual vendor
> > of the panel and could not. All I found was various listings on places
> > like AliExpress that did not mention an manufacturer. I'd rather not
> > invent a vendor because we could not find the actual vendor of the
> > panel & it seemed rather unreasonable to block support for the device
> > on the basis of not being able to figure out the vendor. If you, as
> > someone knowledgeable on displays, can figure the vendor out, then
> > yeah we should definitely add it.
> 
> It's still a bit surprising to me. We've merged[1][2][3][4], and are still
> merging[5], panels from this particular vendor that have no clearly
> identified OEMs. Just like any other panel, really. We almost *never*
> have the actual OEM, we just go with whatever is the easiest to identify
> it.

It wasn't (isn't?) clear to me that Abernic is even the vendor of the
panel, just that it works for their devices. If there's an established
policy here of making up vendors for these panels, then sure, override
me and use them as the prefix.

> Plus, if there ever is another WL-355608-A8 part from a completely
> unrelated vendor, then you'll have a naming clash with no clear
> indication about which is which.
>
> 1: https://lore.kernel.org/all/20230426143213.4178586-1-macroalpha82@gmail.com/
> 2: https://lore.kernel.org/all/20231003163355.143704-1-macroalpha82@gmail.com/
> 3: https://lore.kernel.org/all/20231117202536.1387815-1-macroalpha82@gmail.com/
> 4: https://lore.kernel.org/all/20231208154847.130615-1-macroalpha82@gmail.com/
> 5: https://lore.kernel.org/dri-devel/20240618081515.1215552-1-kikuchan98@gmail.com/



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

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Neil Armstrong]

On 18/06/2024 13:13, Conor Dooley wrote:
> On Tue, Jun 18, 2024 at 11:04:09AM +0200, Maxime Ripard wrote:
>> Hi Conor,
>>
>> Sorry, I missed the news of you becoming a DT maintainer, so most of my
>> previous points are obviously bogus. And congrats :)
> 
> I've been doing it for over a year, so news travels to some corners slowly
> I guess. I'm not just being a pest in dozens of subsystems for fun!
> 
>> On Thu, Jun 06, 2024 at 12:51:33PM GMT, Conor Dooley wrote:
>>> On Thu, Jun 06, 2024 at 01:23:03PM +0200, Maxime Ripard wrote:
>>>> On Thu, Jun 06, 2024 at 11:37:31AM GMT, Neil Armstrong wrote:
>>>>> On 06/06/2024 11:32, Maxime Ripard wrote:
>>>>>> On Fri, May 31, 2024 at 09:12:14AM GMT, Ryan Walklin wrote:
>>>>>>> The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display used in a
>>>>>>> number of handheld gaming devices made by Anbernic. By consensus a
>>>>>>> vendor prefix is not provided as the panel OEM is unknown.
>>>>>>
>>>>>> Where has this consensus been found?
>>>>>>
>>>>>> I had a look at the previous discussions, and I can't find any consensus
>>>>>> being reached there. And for that kind of thing, having the ack or
>>>>>> review of any of the DT maintainers would have been great.
>>>>>
>>>>> There was a consensus with Conor, this is why he acked v2, see
>>>>> https://lore.kernel.org/all/20240525-velvet-citable-a45dd06847a7@spud/
>>>>
>>>> It's probably a matter of semantics here, but if it's with only one
>>>> person, it's not a consensus but an agreement.
>>>>
>>>>> ```
>>>>> I think if we genuinely do not know what the vendor is then we just
>>>>> don't have a prefix.
>>>>> ```
>>>>
>>>> And even then, I don't interpret Conor's statement as a formal agreement
>>>> but rather an acknowledgment of the issue.
>>>
>>> I mean, I specifically left an r-b below that line in v2:
>>> https://lore.kernel.org/all/20240530-satchel-playgroup-e8aa6937b8b9@spud/
>>>
>>> I'm not a displays guy, so my sources were limited to what I could find
>>> from search engines, but I spent some time looking for an actual vendor
>>> of the panel and could not. All I found was various listings on places
>>> like AliExpress that did not mention an manufacturer. I'd rather not
>>> invent a vendor because we could not find the actual vendor of the
>>> panel & it seemed rather unreasonable to block support for the device
>>> on the basis of not being able to figure out the vendor. If you, as
>>> someone knowledgeable on displays, can figure the vendor out, then
>>> yeah we should definitely add it.
>>
>> It's still a bit surprising to me. We've merged[1][2][3][4], and are still
>> merging[5], panels from this particular vendor that have no clearly
>> identified OEMs. Just like any other panel, really. We almost *never*
>> have the actual OEM, we just go with whatever is the easiest to identify
>> it.
> 
> It wasn't (isn't?) clear to me that Abernic is even the vendor of the
> panel, just that it works for their devices. If there's an established
> policy here of making up vendors for these panels, then sure, override
> me and use them as the prefix.
> 
>> Plus, if there ever is another WL-355608-A8 part from a completely
>> unrelated vendor, then you'll have a naming clash with no clear
>> indication about which is which.

Not sure we can say there's an established policy ongoing here, we try to
use the marking we find on the panel when possible and when not possible
we use the vendor + name of the device in last ressort.

Neil

>>
>> 1: https://lore.kernel.org/all/20230426143213.4178586-1-macroalpha82@gmail.com/
>> 2: https://lore.kernel.org/all/20231003163355.143704-1-macroalpha82@gmail.com/
>> 3: https://lore.kernel.org/all/20231117202536.1387815-1-macroalpha82@gmail.com/
>> 4: https://lore.kernel.org/all/20231208154847.130615-1-macroalpha82@gmail.com/
>> 5: https://lore.kernel.org/dri-devel/20240618081515.1215552-1-kikuchan98@gmail.com/
> 
> 

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Maxime Ripard]

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

On Tue, Jun 18, 2024 at 02:05:50PM GMT, Neil Armstrong wrote:
> On 18/06/2024 13:13, Conor Dooley wrote:
> > On Tue, Jun 18, 2024 at 11:04:09AM +0200, Maxime Ripard wrote:
> > > Hi Conor,
> > > 
> > > Sorry, I missed the news of you becoming a DT maintainer, so most of my
> > > previous points are obviously bogus. And congrats :)
> > 
> > I've been doing it for over a year, so news travels to some corners slowly
> > I guess. I'm not just being a pest in dozens of subsystems for fun!
> > 
> > > On Thu, Jun 06, 2024 at 12:51:33PM GMT, Conor Dooley wrote:
> > > > On Thu, Jun 06, 2024 at 01:23:03PM +0200, Maxime Ripard wrote:
> > > > > On Thu, Jun 06, 2024 at 11:37:31AM GMT, Neil Armstrong wrote:
> > > > > > On 06/06/2024 11:32, Maxime Ripard wrote:
> > > > > > > On Fri, May 31, 2024 at 09:12:14AM GMT, Ryan Walklin wrote:
> > > > > > > > The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display used in a
> > > > > > > > number of handheld gaming devices made by Anbernic. By consensus a
> > > > > > > > vendor prefix is not provided as the panel OEM is unknown.
> > > > > > > 
> > > > > > > Where has this consensus been found?
> > > > > > > 
> > > > > > > I had a look at the previous discussions, and I can't find any consensus
> > > > > > > being reached there. And for that kind of thing, having the ack or
> > > > > > > review of any of the DT maintainers would have been great.
> > > > > > 
> > > > > > There was a consensus with Conor, this is why he acked v2, see
> > > > > > https://lore.kernel.org/all/20240525-velvet-citable-a45dd06847a7@spud/
> > > > > 
> > > > > It's probably a matter of semantics here, but if it's with only one
> > > > > person, it's not a consensus but an agreement.
> > > > > 
> > > > > > ```
> > > > > > I think if we genuinely do not know what the vendor is then we just
> > > > > > don't have a prefix.
> > > > > > ```
> > > > > 
> > > > > And even then, I don't interpret Conor's statement as a formal agreement
> > > > > but rather an acknowledgment of the issue.
> > > > 
> > > > I mean, I specifically left an r-b below that line in v2:
> > > > https://lore.kernel.org/all/20240530-satchel-playgroup-e8aa6937b8b9@spud/
> > > > 
> > > > I'm not a displays guy, so my sources were limited to what I could find
> > > > from search engines, but I spent some time looking for an actual vendor
> > > > of the panel and could not. All I found was various listings on places
> > > > like AliExpress that did not mention an manufacturer. I'd rather not
> > > > invent a vendor because we could not find the actual vendor of the
> > > > panel & it seemed rather unreasonable to block support for the device
> > > > on the basis of not being able to figure out the vendor. If you, as
> > > > someone knowledgeable on displays, can figure the vendor out, then
> > > > yeah we should definitely add it.
> > > 
> > > It's still a bit surprising to me. We've merged[1][2][3][4], and are still
> > > merging[5], panels from this particular vendor that have no clearly
> > > identified OEMs. Just like any other panel, really. We almost *never*
> > > have the actual OEM, we just go with whatever is the easiest to identify
> > > it.
> > 
> > It wasn't (isn't?) clear to me that Abernic is even the vendor of the
> > panel, just that it works for their devices. If there's an established
> > policy here of making up vendors for these panels, then sure, override
> > me and use them as the prefix.
> > 
> > > Plus, if there ever is another WL-355608-A8 part from a completely
> > > unrelated vendor, then you'll have a naming clash with no clear
> > > indication about which is which.
> 
> Not sure we can say there's an established policy ongoing here, we try to
> use the marking we find on the panel when possible and when not possible
> we use the vendor + name of the device in last ressort.

So pretty much what I was asking for?

We're getting fairly late into the release cycle and I'd like to get it
fixed before the release. Can you send a patch to address it please?

Maxime

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

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Ryan Walklin]

Hi Maxime,

On Wed, 26 Jun 2024, at 8:56 PM, Maxime Ripard wrote:

> We're getting fairly late into the release cycle and I'd like to get it
> fixed before the release. Can you send a patch to address it please?

Sure, happy to. So to confirm add 'anbernic' to the vendor binding list and 'anbernic,wl-355608-a8' as the panel compatible?

Regards,

Ryan

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Neil Armstrong]

On 26/06/2024 11:10, Ryan Walklin wrote:
> Hi Maxime,
> 
> On Wed, 26 Jun 2024, at 8:56 PM, Maxime Ripard wrote:
> 
>> We're getting fairly late into the release cycle and I'd like to get it
>> fixed before the release. Can you send a patch to address it please?
> 
> Sure, happy to. So to confirm add 'anbernic' to the vendor binding list and 'anbernic,wl-355608-a8' as the panel compatible?

Well anbernic is not the wl-355608-a8 panel manufaturer, so as Maxime is suggesting to use the
name of the device where the panel is found like anbernic,rg353v-panel-v2 as submitted
in https://lore.kernel.org/all/20230426143213.4178586-2-macroalpha82@gmail.com/

Personally I don't care, if DT maintainers agrees having "wl-355608-a8" as compatible,
it's perfectly fine to use it from the driver since it's solely a bindings decision
and not a driver design issue.

Neil

> 
> Regards,
> 
> Ryan

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Ryan Walklin]

On Wed, 26 Jun 2024, at 9:16 PM, Neil Armstrong wrote:
> Well anbernic is not the wl-355608-a8 panel manufaturer, so as Maxime 
> is suggesting to use the
> name of the device where the panel is found like 
> anbernic,rg353v-panel-v2 as submitted
> in 
> https://lore.kernel.org/all/20230426143213.4178586-2-macroalpha82@gmail.com/
Show quoted text

Understood thanks. I have no strong feelings either, using the device name is sensible. Will prepare a patch.

Regards,

Ryan 

(apologies, replying-all this time)

Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

[Neil Armstrong]

On 06/06/2024 13:23, Maxime Ripard wrote:
> On Thu, Jun 06, 2024 at 11:37:31AM GMT, Neil Armstrong wrote:
>> On 06/06/2024 11:32, Maxime Ripard wrote:
>>> On Fri, May 31, 2024 at 09:12:14AM GMT, Ryan Walklin wrote:
>>>> The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display used in a
>>>> number of handheld gaming devices made by Anbernic. By consensus a
>>>> vendor prefix is not provided as the panel OEM is unknown.
>>>
>>> Where has this consensus been found?
>>>
>>> I had a look at the previous discussions, and I can't find any consensus
>>> being reached there. And for that kind of thing, having the ack or
>>> review of any of the DT maintainers would have been great.
>>
>> There was a consensus with Conor, this is why he acked v2, see
>> https://lore.kernel.org/all/20240525-velvet-citable-a45dd06847a7@spud/
> 
> It's probably a matter of semantics here, but if it's with only one
> person, it's not a consensus but an agreement.
> 
>> ```
>> I think if we genuinely do not know what the vendor is then we just
>> don't have a prefix.
>> ```
> 
> And even then, I don't interpret Conor's statement as a formal agreement
> but rather an acknowledgment of the issue.

Well since both maintainers (DT and Panel) agreed, isn't it all good ?

> 
>> I agree with Conor so I applied the patchset after Connor reviewed it and the comment was fixed in v3:
>> https://lore.kernel.org/all/20240530-satchel-playgroup-e8aa6937b8b9@spud/
> 
> Yeah, I know. Still, it's a major deviation to what we've always been
> doing, getting the DT maintainers voice on that would have been a good
> idea.

I consider Conor's voice enough as one of the DT maintainers.

Neil

> 
> Maxime

[PATCH v3 2/2] drm: panel: nv3052c: Add WL-355608-A8 panel

[Ryan Walklin]

The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display from an unknown
OEM used in a number of handheld gaming devices made by Anbernic.
Limited information is available online however the panel timing values
(below) have been obtained from the vendor BSP. The panel appears to
integrate a NV3052C LCD driver (or clone). Available devices address it
in SPI/RGB mode, with the timing signals generated from the device
SoC (Allwinner H700) and passed through.

Add a panel definition and display mode to the existing NV3502C driver.

It was assumed during bringup that the initialisation sequence was the
same as the existing Fascontek FS035VG158 panel, proved working during
experimentation, however subsequent dumping of the init sequence with a
logic analyser confirms one small change to VCOM_ADJ3 from 0x4a to 0x44,
therefore a separate set of registers is also added.

Timings:
           | Active |  FP  | Sync |  BP  | Total
-----------|--------|------|------|------|-------
Horizontal |   640  |  64  |  20  |  46  |  770
  Vertical |   480  |  21  |   4  |  15  |  520

Signed-off-by: Ryan Walklin <ryan@testtoast.com>
Co-developed-by: Hironori KIKUCHI <kikuchan98@gmail.com>
Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
Reviewed-by: John Watts <contact@jookia.org>

---

Changelog v1..v2:
- Update .compatible string to match dt-binding
- Add co-developer sign-off and review tags

Changelog v2..v3:
- Trailing whitespace as per checkpatch.pl (no functional changes)
---
 .../gpu/drm/panel/panel-newvision-nv3052c.c   | 225 ++++++++++++++++++
 1 file changed, 225 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
index 1aab0c9ae5..e986d98235 100644
--- a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
+++ b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
@@ -433,6 +433,202 @@ static const struct nv3052c_reg fs035vg158_panel_regs[] = {
 	{ 0x36, 0x0a }, // bgr = 1, ss = 1, gs = 0
 };
 
+
+static const struct nv3052c_reg wl_355608_a8_panel_regs[] = {
+	// EXTC Command set enable, select page 1
+	{ 0xff, 0x30 }, { 0xff, 0x52 }, { 0xff, 0x01 },
+	// Mostly unknown registers
+	{ 0xe3, 0x00 },
+	{ 0x40, 0x00 },
+	{ 0x03, 0x40 },
+	{ 0x04, 0x00 },
+	{ 0x05, 0x03 },
+	{ 0x08, 0x00 },
+	{ 0x09, 0x07 },
+	{ 0x0a, 0x01 },
+	{ 0x0b, 0x32 },
+	{ 0x0c, 0x32 },
+	{ 0x0d, 0x0b },
+	{ 0x0e, 0x00 },
+	{ 0x23, 0xa0 },
+	{ 0x24, 0x0c },
+	{ 0x25, 0x06 },
+	{ 0x26, 0x14 },
+	{ 0x27, 0x14 },
+	{ 0x38, 0xcc }, // VCOM_ADJ1
+	{ 0x39, 0xd7 }, // VCOM_ADJ2
+	{ 0x3a, 0x44 }, // VCOM_ADJ3
+	{ 0x28, 0x40 },
+	{ 0x29, 0x01 },
+	{ 0x2a, 0xdf },
+	{ 0x49, 0x3c },
+	{ 0x91, 0x77 }, // EXTPW_CTRL2
+	{ 0x92, 0x77 }, // EXTPW_CTRL3
+	{ 0xa0, 0x55 },
+	{ 0xa1, 0x50 },
+	{ 0xa4, 0x9c },
+	{ 0xa7, 0x02 },
+	{ 0xa8, 0x01 },
+	{ 0xa9, 0x01 },
+	{ 0xaa, 0xfc },
+	{ 0xab, 0x28 },
+	{ 0xac, 0x06 },
+	{ 0xad, 0x06 },
+	{ 0xae, 0x06 },
+	{ 0xaf, 0x03 },
+	{ 0xb0, 0x08 },
+	{ 0xb1, 0x26 },
+	{ 0xb2, 0x28 },
+	{ 0xb3, 0x28 },
+	{ 0xb4, 0x33 },
+	{ 0xb5, 0x08 },
+	{ 0xb6, 0x26 },
+	{ 0xb7, 0x08 },
+	{ 0xb8, 0x26 },
+	{ 0xf0, 0x00 },
+	{ 0xf6, 0xc0 },
+	// EXTC Command set enable, select page 2
+	{ 0xff, 0x30 }, { 0xff, 0x52 }, { 0xff, 0x02 },
+	// Set gray scale voltage to adjust gamma
+	{ 0xb0, 0x0b }, // PGAMVR0
+	{ 0xb1, 0x16 }, // PGAMVR1
+	{ 0xb2, 0x17 }, // PGAMVR2
+	{ 0xb3, 0x2c }, // PGAMVR3
+	{ 0xb4, 0x32 }, // PGAMVR4
+	{ 0xb5, 0x3b }, // PGAMVR5
+	{ 0xb6, 0x29 }, // PGAMPR0
+	{ 0xb7, 0x40 }, // PGAMPR1
+	{ 0xb8, 0x0d }, // PGAMPK0
+	{ 0xb9, 0x05 }, // PGAMPK1
+	{ 0xba, 0x12 }, // PGAMPK2
+	{ 0xbb, 0x10 }, // PGAMPK3
+	{ 0xbc, 0x12 }, // PGAMPK4
+	{ 0xbd, 0x15 }, // PGAMPK5
+	{ 0xbe, 0x19 }, // PGAMPK6
+	{ 0xbf, 0x0e }, // PGAMPK7
+	{ 0xc0, 0x16 }, // PGAMPK8
+	{ 0xc1, 0x0a }, // PGAMPK9
+	// Set gray scale voltage to adjust gamma
+	{ 0xd0, 0x0c }, // NGAMVR0
+	{ 0xd1, 0x17 }, // NGAMVR0
+	{ 0xd2, 0x14 }, // NGAMVR1
+	{ 0xd3, 0x2e }, // NGAMVR2
+	{ 0xd4, 0x32 }, // NGAMVR3
+	{ 0xd5, 0x3c }, // NGAMVR4
+	{ 0xd6, 0x22 }, // NGAMPR0
+	{ 0xd7, 0x3d }, // NGAMPR1
+	{ 0xd8, 0x0d }, // NGAMPK0
+	{ 0xd9, 0x07 }, // NGAMPK1
+	{ 0xda, 0x13 }, // NGAMPK2
+	{ 0xdb, 0x13 }, // NGAMPK3
+	{ 0xdc, 0x11 }, // NGAMPK4
+	{ 0xdd, 0x15 }, // NGAMPK5
+	{ 0xde, 0x19 }, // NGAMPK6
+	{ 0xdf, 0x10 }, // NGAMPK7
+	{ 0xe0, 0x17 }, // NGAMPK8
+	{ 0xe1, 0x0a }, // NGAMPK9
+	// EXTC Command set enable, select page 3
+	{ 0xff, 0x30 }, { 0xff, 0x52 }, { 0xff, 0x03 },
+	// Set various timing settings
+	{ 0x00, 0x2a }, // GIP_VST_1
+	{ 0x01, 0x2a }, // GIP_VST_2
+	{ 0x02, 0x2a }, // GIP_VST_3
+	{ 0x03, 0x2a }, // GIP_VST_4
+	{ 0x04, 0x61 }, // GIP_VST_5
+	{ 0x05, 0x80 }, // GIP_VST_6
+	{ 0x06, 0xc7 }, // GIP_VST_7
+	{ 0x07, 0x01 }, // GIP_VST_8
+	{ 0x08, 0x03 }, // GIP_VST_9
+	{ 0x09, 0x04 }, // GIP_VST_10
+	{ 0x70, 0x22 }, // GIP_ECLK1
+	{ 0x71, 0x80 }, // GIP_ECLK2
+	{ 0x30, 0x2a }, // GIP_CLK_1
+	{ 0x31, 0x2a }, // GIP_CLK_2
+	{ 0x32, 0x2a }, // GIP_CLK_3
+	{ 0x33, 0x2a }, // GIP_CLK_4
+	{ 0x34, 0x61 }, // GIP_CLK_5
+	{ 0x35, 0xc5 }, // GIP_CLK_6
+	{ 0x36, 0x80 }, // GIP_CLK_7
+	{ 0x37, 0x23 }, // GIP_CLK_8
+	{ 0x40, 0x03 }, // GIP_CLKA_1
+	{ 0x41, 0x04 }, // GIP_CLKA_2
+	{ 0x42, 0x05 }, // GIP_CLKA_3
+	{ 0x43, 0x06 }, // GIP_CLKA_4
+	{ 0x44, 0x11 }, // GIP_CLKA_5
+	{ 0x45, 0xe8 }, // GIP_CLKA_6
+	{ 0x46, 0xe9 }, // GIP_CLKA_7
+	{ 0x47, 0x11 }, // GIP_CLKA_8
+	{ 0x48, 0xea }, // GIP_CLKA_9
+	{ 0x49, 0xeb }, // GIP_CLKA_10
+	{ 0x50, 0x07 }, // GIP_CLKB_1
+	{ 0x51, 0x08 }, // GIP_CLKB_2
+	{ 0x52, 0x09 }, // GIP_CLKB_3
+	{ 0x53, 0x0a }, // GIP_CLKB_4
+	{ 0x54, 0x11 }, // GIP_CLKB_5
+	{ 0x55, 0xec }, // GIP_CLKB_6
+	{ 0x56, 0xed }, // GIP_CLKB_7
+	{ 0x57, 0x11 }, // GIP_CLKB_8
+	{ 0x58, 0xef }, // GIP_CLKB_9
+	{ 0x59, 0xf0 }, // GIP_CLKB_10
+	// Map internal GOA signals to GOA output pad
+	{ 0xb1, 0x01 }, // PANELD2U2
+	{ 0xb4, 0x15 }, // PANELD2U5
+	{ 0xb5, 0x16 }, // PANELD2U6
+	{ 0xb6, 0x09 }, // PANELD2U7
+	{ 0xb7, 0x0f }, // PANELD2U8
+	{ 0xb8, 0x0d }, // PANELD2U9
+	{ 0xb9, 0x0b }, // PANELD2U10
+	{ 0xba, 0x00 }, // PANELD2U11
+	{ 0xc7, 0x02 }, // PANELD2U24
+	{ 0xca, 0x17 }, // PANELD2U27
+	{ 0xcb, 0x18 }, // PANELD2U28
+	{ 0xcc, 0x0a }, // PANELD2U29
+	{ 0xcd, 0x10 }, // PANELD2U30
+	{ 0xce, 0x0e }, // PANELD2U31
+	{ 0xcf, 0x0c }, // PANELD2U32
+	{ 0xd0, 0x00 }, // PANELD2U33
+	// Map internal GOA signals to GOA output pad
+	{ 0x81, 0x00 }, // PANELU2D2
+	{ 0x84, 0x15 }, // PANELU2D5
+	{ 0x85, 0x16 }, // PANELU2D6
+	{ 0x86, 0x10 }, // PANELU2D7
+	{ 0x87, 0x0a }, // PANELU2D8
+	{ 0x88, 0x0c }, // PANELU2D9
+	{ 0x89, 0x0e }, // PANELU2D10
+	{ 0x8a, 0x02 }, // PANELU2D11
+	{ 0x97, 0x00 }, // PANELU2D24
+	{ 0x9a, 0x17 }, // PANELU2D27
+	{ 0x9b, 0x18 }, // PANELU2D28
+	{ 0x9c, 0x0f }, // PANELU2D29
+	{ 0x9d, 0x09 }, // PANELU2D30
+	{ 0x9e, 0x0b }, // PANELU2D31
+	{ 0x9f, 0x0d }, // PANELU2D32
+	{ 0xa0, 0x01 }, // PANELU2D33
+	// EXTC Command set enable, select page 2
+	{ 0xff, 0x30 }, { 0xff, 0x52 }, { 0xff, 0x02 },
+	// Unknown registers
+	{ 0x01, 0x01 },
+	{ 0x02, 0xda },
+	{ 0x03, 0xba },
+	{ 0x04, 0xa8 },
+	{ 0x05, 0x9a },
+	{ 0x06, 0x70 },
+	{ 0x07, 0xff },
+	{ 0x08, 0x91 },
+	{ 0x09, 0x90 },
+	{ 0x0a, 0xff },
+	{ 0x0b, 0x8f },
+	{ 0x0c, 0x60 },
+	{ 0x0d, 0x58 },
+	{ 0x0e, 0x48 },
+	{ 0x0f, 0x38 },
+	{ 0x10, 0x2b },
+	// EXTC Command set enable, select page 0
+	{ 0xff, 0x30 }, { 0xff, 0x52 }, { 0xff, 0x00 },
+	// Display Access Control
+	{ 0x36, 0x0a }, // bgr = 1, ss = 1, gs = 0
+};
+
 static inline struct nv3052c *to_nv3052c(struct drm_panel *panel)
 {
 	return container_of(panel, struct nv3052c, panel);
@@ -670,6 +866,21 @@ static const struct drm_display_mode fs035vg158_modes[] = {
 	},
 };
 
+static const struct drm_display_mode wl_355608_a8_mode[] = {
+	{
+		.clock = 24000,
+		.hdisplay = 640,
+		.hsync_start = 640 + 64,
+		.hsync_end = 640 + 64 + 20,
+		.htotal = 640 + 64 + 20 + 46,
+		.vdisplay = 480,
+		.vsync_start = 480 + 21,
+		.vsync_end = 480 + 21 + 4,
+		.vtotal = 480 + 21 + 4 + 15,
+		.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+	},
+};
+
 static const struct nv3052c_panel_info ltk035c5444t_panel_info = {
 	.display_modes = ltk035c5444t_modes,
 	.num_modes = ARRAY_SIZE(ltk035c5444t_modes),
@@ -692,9 +903,21 @@ static const struct nv3052c_panel_info fs035vg158_panel_info = {
 	.panel_regs_len = ARRAY_SIZE(fs035vg158_panel_regs),
 };
 
+static const struct nv3052c_panel_info wl_355608_a8_panel_info = {
+	.display_modes = wl_355608_a8_mode,
+	.num_modes = ARRAY_SIZE(wl_355608_a8_mode),
+	.width_mm = 150,
+	.height_mm = 94,
+	.bus_format = MEDIA_BUS_FMT_RGB888_1X24,
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
+	.panel_regs = wl_355608_a8_panel_regs,
+	.panel_regs_len = ARRAY_SIZE(wl_355608_a8_panel_regs),
+};
+
 static const struct spi_device_id nv3052c_ids[] = {
 	{ "ltk035c5444t", },
 	{ "fs035vg158", },
+	{ "wl-355608-a8", },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(spi, nv3052c_ids);
@@ -702,6 +925,7 @@ MODULE_DEVICE_TABLE(spi, nv3052c_ids);
 static const struct of_device_id nv3052c_of_match[] = {
 	{ .compatible = "leadtek,ltk035c5444t", .data = &ltk035c5444t_panel_info },
 	{ .compatible = "fascontek,fs035vg158", .data = &fs035vg158_panel_info },
+	{ .compatible = "wl-355608-a8", .data = &wl_355608_a8_panel_info },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, nv3052c_of_match);
@@ -719,4 +943,5 @@ module_spi_driver(nv3052c_driver);
 
 MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
 MODULE_AUTHOR("Christophe Branchereau <cbranchereau@gmail.com>");
+MODULE_AUTHOR("Ryan Walklin <ryan@testtoast.com");
 MODULE_LICENSE("GPL v2");
-- 
2.45.1

Re: [PATCH v3 2/2] drm: panel: nv3052c: Add WL-355608-A8 panel

[Neil Armstrong]

On 30/05/2024 23:12, Ryan Walklin wrote:
> The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display from an unknown
> OEM used in a number of handheld gaming devices made by Anbernic.
> Limited information is available online however the panel timing values
> (below) have been obtained from the vendor BSP. The panel appears to
> integrate a NV3052C LCD driver (or clone). Available devices address it
> in SPI/RGB mode, with the timing signals generated from the device
> SoC (Allwinner H700) and passed through.
> 
> Add a panel definition and display mode to the existing NV3502C driver.
> 
> It was assumed during bringup that the initialisation sequence was the
> same as the existing Fascontek FS035VG158 panel, proved working during
> experimentation, however subsequent dumping of the init sequence with a
> logic analyser confirms one small change to VCOM_ADJ3 from 0x4a to 0x44,
> therefore a separate set of registers is also added.
> 
> Timings:
>             | Active |  FP  | Sync |  BP  | Total
> -----------|--------|------|------|------|-------
> Horizontal |   640  |  64  |  20  |  46  |  770
>    Vertical |   480  |  21  |   4  |  15  |  520
> 
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> Co-developed-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> Reviewed-by: John Watts <contact@jookia.org>
> 
> ---
> 
> Changelog v1..v2:
> - Update .compatible string to match dt-binding
> - Add co-developer sign-off and review tags
> 
> Changelog v2..v3:
> - Trailing whitespace as per checkpatch.pl (no functional changes)
> ---
>   .../gpu/drm/panel/panel-newvision-nv3052c.c   | 225 ++++++++++++++++++
>   1 file changed, 225 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> index 1aab0c9ae5..e986d98235 100644
> --- a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> +++ b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> @@ -433,6 +433,202 @@ static const struct nv3052c_reg fs035vg158_panel_regs[] = {
>   	{ 0x36, 0x0a }, // bgr = 1, ss = 1, gs = 0
>   };
>   
> +
> +static const struct nv3052c_reg wl_355608_a8_panel_regs[] = {
> +	// EXTC Command set enable, select page 1
> +	{ 0xff, 0x30 }, { 0xff, 0x52 }, { 0xff, 0x01 },
> +	// Mostly unknown registers
> +	{ 0xe3, 0x00 },
> +	{ 0x40, 0x00 },
> +	{ 0x03, 0x40 },
> +	{ 0x04, 0x00 },
> +	{ 0x05, 0x03 },
> +	{ 0x08, 0x00 },
> +	{ 0x09, 0x07 },
> +	{ 0x0a, 0x01 },
> +	{ 0x0b, 0x32 },
> +	{ 0x0c, 0x32 },
> +	{ 0x0d, 0x0b },
> +	{ 0x0e, 0x00 },
> +	{ 0x23, 0xa0 },
> +	{ 0x24, 0x0c },
> +	{ 0x25, 0x06 },
> +	{ 0x26, 0x14 },
> +	{ 0x27, 0x14 },
> +	{ 0x38, 0xcc }, // VCOM_ADJ1
> +	{ 0x39, 0xd7 }, // VCOM_ADJ2
> +	{ 0x3a, 0x44 }, // VCOM_ADJ3
> +	{ 0x28, 0x40 },
> +	{ 0x29, 0x01 },
> +	{ 0x2a, 0xdf },
> +	{ 0x49, 0x3c },
> +	{ 0x91, 0x77 }, // EXTPW_CTRL2
> +	{ 0x92, 0x77 }, // EXTPW_CTRL3
> +	{ 0xa0, 0x55 },
> +	{ 0xa1, 0x50 },
> +	{ 0xa4, 0x9c },
> +	{ 0xa7, 0x02 },
> +	{ 0xa8, 0x01 },
> +	{ 0xa9, 0x01 },
> +	{ 0xaa, 0xfc },
> +	{ 0xab, 0x28 },
> +	{ 0xac, 0x06 },
> +	{ 0xad, 0x06 },
> +	{ 0xae, 0x06 },
> +	{ 0xaf, 0x03 },
> +	{ 0xb0, 0x08 },
> +	{ 0xb1, 0x26 },
> +	{ 0xb2, 0x28 },
> +	{ 0xb3, 0x28 },
> +	{ 0xb4, 0x33 },
> +	{ 0xb5, 0x08 },
> +	{ 0xb6, 0x26 },
> +	{ 0xb7, 0x08 },
> +	{ 0xb8, 0x26 },
> +	{ 0xf0, 0x00 },
> +	{ 0xf6, 0xc0 },
> +	// EXTC Command set enable, select page 2
> +	{ 0xff, 0x30 }, { 0xff, 0x52 }, { 0xff, 0x02 },
> +	// Set gray scale voltage to adjust gamma
> +	{ 0xb0, 0x0b }, // PGAMVR0
> +	{ 0xb1, 0x16 }, // PGAMVR1
> +	{ 0xb2, 0x17 }, // PGAMVR2
> +	{ 0xb3, 0x2c }, // PGAMVR3
> +	{ 0xb4, 0x32 }, // PGAMVR4
> +	{ 0xb5, 0x3b }, // PGAMVR5
> +	{ 0xb6, 0x29 }, // PGAMPR0
> +	{ 0xb7, 0x40 }, // PGAMPR1
> +	{ 0xb8, 0x0d }, // PGAMPK0
> +	{ 0xb9, 0x05 }, // PGAMPK1
> +	{ 0xba, 0x12 }, // PGAMPK2
> +	{ 0xbb, 0x10 }, // PGAMPK3
> +	{ 0xbc, 0x12 }, // PGAMPK4
> +	{ 0xbd, 0x15 }, // PGAMPK5
> +	{ 0xbe, 0x19 }, // PGAMPK6
> +	{ 0xbf, 0x0e }, // PGAMPK7
> +	{ 0xc0, 0x16 }, // PGAMPK8
> +	{ 0xc1, 0x0a }, // PGAMPK9
> +	// Set gray scale voltage to adjust gamma
> +	{ 0xd0, 0x0c }, // NGAMVR0
> +	{ 0xd1, 0x17 }, // NGAMVR0
> +	{ 0xd2, 0x14 }, // NGAMVR1
> +	{ 0xd3, 0x2e }, // NGAMVR2
> +	{ 0xd4, 0x32 }, // NGAMVR3
> +	{ 0xd5, 0x3c }, // NGAMVR4
> +	{ 0xd6, 0x22 }, // NGAMPR0
> +	{ 0xd7, 0x3d }, // NGAMPR1
> +	{ 0xd8, 0x0d }, // NGAMPK0
> +	{ 0xd9, 0x07 }, // NGAMPK1
> +	{ 0xda, 0x13 }, // NGAMPK2
> +	{ 0xdb, 0x13 }, // NGAMPK3
> +	{ 0xdc, 0x11 }, // NGAMPK4
> +	{ 0xdd, 0x15 }, // NGAMPK5
> +	{ 0xde, 0x19 }, // NGAMPK6
> +	{ 0xdf, 0x10 }, // NGAMPK7
> +	{ 0xe0, 0x17 }, // NGAMPK8
> +	{ 0xe1, 0x0a }, // NGAMPK9
> +	// EXTC Command set enable, select page 3
> +	{ 0xff, 0x30 }, { 0xff, 0x52 }, { 0xff, 0x03 },
> +	// Set various timing settings
> +	{ 0x00, 0x2a }, // GIP_VST_1
> +	{ 0x01, 0x2a }, // GIP_VST_2
> +	{ 0x02, 0x2a }, // GIP_VST_3
> +	{ 0x03, 0x2a }, // GIP_VST_4
> +	{ 0x04, 0x61 }, // GIP_VST_5
> +	{ 0x05, 0x80 }, // GIP_VST_6
> +	{ 0x06, 0xc7 }, // GIP_VST_7
> +	{ 0x07, 0x01 }, // GIP_VST_8
> +	{ 0x08, 0x03 }, // GIP_VST_9
> +	{ 0x09, 0x04 }, // GIP_VST_10
> +	{ 0x70, 0x22 }, // GIP_ECLK1
> +	{ 0x71, 0x80 }, // GIP_ECLK2
> +	{ 0x30, 0x2a }, // GIP_CLK_1
> +	{ 0x31, 0x2a }, // GIP_CLK_2
> +	{ 0x32, 0x2a }, // GIP_CLK_3
> +	{ 0x33, 0x2a }, // GIP_CLK_4
> +	{ 0x34, 0x61 }, // GIP_CLK_5
> +	{ 0x35, 0xc5 }, // GIP_CLK_6
> +	{ 0x36, 0x80 }, // GIP_CLK_7
> +	{ 0x37, 0x23 }, // GIP_CLK_8
> +	{ 0x40, 0x03 }, // GIP_CLKA_1
> +	{ 0x41, 0x04 }, // GIP_CLKA_2
> +	{ 0x42, 0x05 }, // GIP_CLKA_3
> +	{ 0x43, 0x06 }, // GIP_CLKA_4
> +	{ 0x44, 0x11 }, // GIP_CLKA_5
> +	{ 0x45, 0xe8 }, // GIP_CLKA_6
> +	{ 0x46, 0xe9 }, // GIP_CLKA_7
> +	{ 0x47, 0x11 }, // GIP_CLKA_8
> +	{ 0x48, 0xea }, // GIP_CLKA_9
> +	{ 0x49, 0xeb }, // GIP_CLKA_10
> +	{ 0x50, 0x07 }, // GIP_CLKB_1
> +	{ 0x51, 0x08 }, // GIP_CLKB_2
> +	{ 0x52, 0x09 }, // GIP_CLKB_3
> +	{ 0x53, 0x0a }, // GIP_CLKB_4
> +	{ 0x54, 0x11 }, // GIP_CLKB_5
> +	{ 0x55, 0xec }, // GIP_CLKB_6
> +	{ 0x56, 0xed }, // GIP_CLKB_7
> +	{ 0x57, 0x11 }, // GIP_CLKB_8
> +	{ 0x58, 0xef }, // GIP_CLKB_9
> +	{ 0x59, 0xf0 }, // GIP_CLKB_10
> +	// Map internal GOA signals to GOA output pad
> +	{ 0xb1, 0x01 }, // PANELD2U2
> +	{ 0xb4, 0x15 }, // PANELD2U5
> +	{ 0xb5, 0x16 }, // PANELD2U6
> +	{ 0xb6, 0x09 }, // PANELD2U7
> +	{ 0xb7, 0x0f }, // PANELD2U8
> +	{ 0xb8, 0x0d }, // PANELD2U9
> +	{ 0xb9, 0x0b }, // PANELD2U10
> +	{ 0xba, 0x00 }, // PANELD2U11
> +	{ 0xc7, 0x02 }, // PANELD2U24
> +	{ 0xca, 0x17 }, // PANELD2U27
> +	{ 0xcb, 0x18 }, // PANELD2U28
> +	{ 0xcc, 0x0a }, // PANELD2U29
> +	{ 0xcd, 0x10 }, // PANELD2U30
> +	{ 0xce, 0x0e }, // PANELD2U31
> +	{ 0xcf, 0x0c }, // PANELD2U32
> +	{ 0xd0, 0x00 }, // PANELD2U33
> +	// Map internal GOA signals to GOA output pad
> +	{ 0x81, 0x00 }, // PANELU2D2
> +	{ 0x84, 0x15 }, // PANELU2D5
> +	{ 0x85, 0x16 }, // PANELU2D6
> +	{ 0x86, 0x10 }, // PANELU2D7
> +	{ 0x87, 0x0a }, // PANELU2D8
> +	{ 0x88, 0x0c }, // PANELU2D9
> +	{ 0x89, 0x0e }, // PANELU2D10
> +	{ 0x8a, 0x02 }, // PANELU2D11
> +	{ 0x97, 0x00 }, // PANELU2D24
> +	{ 0x9a, 0x17 }, // PANELU2D27
> +	{ 0x9b, 0x18 }, // PANELU2D28
> +	{ 0x9c, 0x0f }, // PANELU2D29
> +	{ 0x9d, 0x09 }, // PANELU2D30
> +	{ 0x9e, 0x0b }, // PANELU2D31
> +	{ 0x9f, 0x0d }, // PANELU2D32
> +	{ 0xa0, 0x01 }, // PANELU2D33
> +	// EXTC Command set enable, select page 2
> +	{ 0xff, 0x30 }, { 0xff, 0x52 }, { 0xff, 0x02 },
> +	// Unknown registers
> +	{ 0x01, 0x01 },
> +	{ 0x02, 0xda },
> +	{ 0x03, 0xba },
> +	{ 0x04, 0xa8 },
> +	{ 0x05, 0x9a },
> +	{ 0x06, 0x70 },
> +	{ 0x07, 0xff },
> +	{ 0x08, 0x91 },
> +	{ 0x09, 0x90 },
> +	{ 0x0a, 0xff },
> +	{ 0x0b, 0x8f },
> +	{ 0x0c, 0x60 },
> +	{ 0x0d, 0x58 },
> +	{ 0x0e, 0x48 },
> +	{ 0x0f, 0x38 },
> +	{ 0x10, 0x2b },
> +	// EXTC Command set enable, select page 0
> +	{ 0xff, 0x30 }, { 0xff, 0x52 }, { 0xff, 0x00 },
> +	// Display Access Control
> +	{ 0x36, 0x0a }, // bgr = 1, ss = 1, gs = 0
> +};
> +
>   static inline struct nv3052c *to_nv3052c(struct drm_panel *panel)
>   {
>   	return container_of(panel, struct nv3052c, panel);
> @@ -670,6 +866,21 @@ static const struct drm_display_mode fs035vg158_modes[] = {
>   	},
>   };
>   
> +static const struct drm_display_mode wl_355608_a8_mode[] = {
> +	{
> +		.clock = 24000,
> +		.hdisplay = 640,
> +		.hsync_start = 640 + 64,
> +		.hsync_end = 640 + 64 + 20,
> +		.htotal = 640 + 64 + 20 + 46,
> +		.vdisplay = 480,
> +		.vsync_start = 480 + 21,
> +		.vsync_end = 480 + 21 + 4,
> +		.vtotal = 480 + 21 + 4 + 15,
> +		.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> +	},
> +};
> +
>   static const struct nv3052c_panel_info ltk035c5444t_panel_info = {
>   	.display_modes = ltk035c5444t_modes,
>   	.num_modes = ARRAY_SIZE(ltk035c5444t_modes),
> @@ -692,9 +903,21 @@ static const struct nv3052c_panel_info fs035vg158_panel_info = {
>   	.panel_regs_len = ARRAY_SIZE(fs035vg158_panel_regs),
>   };
>   
> +static const struct nv3052c_panel_info wl_355608_a8_panel_info = {
> +	.display_modes = wl_355608_a8_mode,
> +	.num_modes = ARRAY_SIZE(wl_355608_a8_mode),
> +	.width_mm = 150,
> +	.height_mm = 94,
> +	.bus_format = MEDIA_BUS_FMT_RGB888_1X24,
> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
> +	.panel_regs = wl_355608_a8_panel_regs,
> +	.panel_regs_len = ARRAY_SIZE(wl_355608_a8_panel_regs),
> +};
> +
>   static const struct spi_device_id nv3052c_ids[] = {
>   	{ "ltk035c5444t", },
>   	{ "fs035vg158", },
> +	{ "wl-355608-a8", },
>   	{ /* sentinel */ }
>   };
>   MODULE_DEVICE_TABLE(spi, nv3052c_ids);
> @@ -702,6 +925,7 @@ MODULE_DEVICE_TABLE(spi, nv3052c_ids);
>   static const struct of_device_id nv3052c_of_match[] = {
>   	{ .compatible = "leadtek,ltk035c5444t", .data = &ltk035c5444t_panel_info },
>   	{ .compatible = "fascontek,fs035vg158", .data = &fs035vg158_panel_info },
> +	{ .compatible = "wl-355608-a8", .data = &wl_355608_a8_panel_info },
>   	{ /* sentinel */ }
>   };
>   MODULE_DEVICE_TABLE(of, nv3052c_of_match);
> @@ -719,4 +943,5 @@ module_spi_driver(nv3052c_driver);
>   
>   MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
>   MODULE_AUTHOR("Christophe Branchereau <cbranchereau@gmail.com>");
> +MODULE_AUTHOR("Ryan Walklin <ryan@testtoast.com");
>   MODULE_LICENSE("GPL v2");

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH v3 0/2] Add WL-355608-A8 panel

[Neil Armstrong]

Hi,

On Fri, 31 May 2024 09:12:12 +1200, Ryan Walklin wrote:
> V3 of this patch with further cleanup to DT binding example code and whitespace fixes in driver code. No functional change from V2.
> 
> Original cover below.
> --
> 
> The WL_355608_A8 panel is a VGA LCD display with an NV3052C-compatible driver IC, used in a number of Anbernic handheld gaming devices. This patch adds a device tree binding, and support for the display timings and init sequence to the NV3052C SPI/RGB driver.
> 
> [...]

Thanks, Applied to https://gitlab.freedesktop.org/drm/misc/kernel.git (drm-misc-next)

[1/2] dt-bindings: display: panel: Add WL-355608-A8 panel
      https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/45b888a8980ae9a09fbf2f50b0ffb7505a834533
[2/2] drm: panel: nv3052c: Add WL-355608-A8 panel
      https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/62ea2eeba7bf11f4b04e080475de93c2f8ee0f92

-- 
Neil