drm/panel: Add Samsung AMS495QA01 Panel
Subsystems (5)
v4 cover drm/panel: Add Samsung AMS495QA01 Panel
From: Chris Morgan <[email protected]>
Add the Samsung AMS495QA01 panel as found on the Anbernic RG503. This panel uses DSI to receive video signals, but 3-wire SPI to receive command signals.
Changes since V3:
- Updated documentation to add spi-peripheral-props.yaml per updates
made for similar devices. Note that I removed a "Reviewed-by" tag
from Rob Herring since this change probably needs to be confirmed.
- Added binding for RG503, since this device is now accepted with this
request: https://lore.kernel.org/linux-rockchip/[email protected]/
Changes since V2:
- Added 50hz mode at request of userspace devs.
- Renamed "dupa" to panel name. Good catch Maya.
- Added Maya's Signed-off-by.
- Removed check for max backlight, since it is already done by backlight_device_set_brightness.
- Fixed minor formatting issues on devicetree binding documentation and added port to provided example.
Changes since V1:
- Removed errant reference to backlight in documentation. This is an OLED panel.
- Made elvss regulator optional. In my case its hard wired and not controllable.
- Added "prepared" enum to track panel status to prevent unbalanced regulator enable/disable.
Chris Morgan (3):
dt-bindings: display: panel: Add Samsung AMS495QA01 drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel
arm64: dts: rockchip: add display to RG503
.../display/panel/samsung,ams495qa01.yaml | 57 ++ .../dts/rockchip/rk3566-anbernic-rg503.dts | 61 ++ drivers/gpu/drm/panel/Kconfig | 10 + drivers/gpu/drm/panel/Makefile | 1 + .../gpu/drm/panel/panel-samsung-ams495qa01.c | 547 ++++++++++++++++++ 5 files changed, 676 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,ams495qa01.yaml create mode 100644 drivers/gpu/drm/panel/panel-samsung-ams495qa01.c
Signature · 1 line
2.25.1
PATCH v4 1/3 dt-bindings: display: panel: Add Samsung AMS495QA01
From: Chris Morgan <[email protected]>
Add documentation for the Samsung AMS495QA01 panel.
--- .../display/panel/samsung,ams495qa01.yaml | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,ams495qa01.yaml
Documentation/devicetree/bindings/display/panel/samsung,ams495qa01.yaml new file +57 -0
@@ -0,0 +1,57 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/panel/samsung,ams495qa01.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Samsung AMS495QA01 4.95in 960x544 DSI/SPI panel + +maintainers: + - Chris Morgan <[email protected]> + +allOf: + - $ref: panel-common.yaml# + - $ref: /schemas/spi/spi-peripheral-props.yaml# + +properties: + compatible: + const: samsung,ams495qa01 + + reg: true + reset-gpios: true + elvdd-supply: + description: regulator that supplies voltage to the panel display + enable-gpios: true + port: true + vdd-supply: + description: regulator that supplies voltage to panel logic + +required: + - compatible + - reg + - vdd-supply + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + spi { + #address-cells = <1>; + #size-cells = <0>; + panel@0 { + compatible = "samsung,ams495qa01"; + reg = <0>; + enable-gpios = <&gpio4 15 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio4 0 GPIO_ACTIVE_LOW>; + vdd-supply = <&vcc3v3_lcd0_n>; + + port { + mipi_in_panel: endpoint { + remote-endpoint = <&mipi_out_panel>; + }; + }; + }; + }; + +...
Signature · 1 line
2.25.1
Re: [PATCH V4 1/3] dt-bindings: display: panel: Add Samsung AMS495QA01
Quoted 11 lines · Chris Morgan
From: Chris Morgan <[email protected]>
Add documentation for the Samsung AMS495QA01 panel.
Signed-off-by Chris Morgan <[email protected]>Signed-off-by Maya Matuszczyk <[email protected]>--- .../display/panel/samsung,ams495qa01.yaml | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,ams495qa01.yaml
Re: [PATCH V4 1/3] dt-bindings: display: panel: Add Samsung AMS495QA01
In reply to Chris MorganHi Chris,
thanks for your patch!
Quoted 8 lines · Chris Morgan
From: Chris Morgan <[email protected]>
Add documentation for the Samsung AMS495QA01 panel.
Signed-off-by Chris Morgan <[email protected]>Signed-off-by Maya Matuszczyk <[email protected]>+ reset-gpios: true
Can you add a description saying that this must always be specified GPIO_ACTIVE_LOW.
With that change:
Yours,
Linus Walleij
PATCH v4 2/3 drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Support Samsung AMS495QA01 panel as found on the Anbernic RG503. Note This panel receives video signals via DSI, however it receives commands via 3-wire SPI.
--- drivers/gpu/drm/panel/Kconfig | 10 + drivers/gpu/drm/panel/Makefile | 1 + .../gpu/drm/panel/panel-samsung-ams495qa01.c | 547 ++++++++++++++++++ 3 files changed, 558 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-samsung-ams495qa01.c
drivers/gpu/drm/panel/Kconfig +10 -0
@@ -444,6 +444,16 @@ config DRM_PANEL_RONBO_RB070D30 Say Y here if you want to enable support for Ronbo Electronics RB070D30 1024x600 DSI panel. +config DRM_PANEL_SAMSUNG_AMS495QA01 + tristate "Samsung AMS495QA01 DSI panel" + depends on OF && SPI + depends on DRM_MIPI_DSI + select DRM_MIPI_DBI + help + DRM panel driver for the Samsung AMS495QA01 panel. This panel + receives video data via DSI but commands via 3-Wire 9-bit + SPI. + config DRM_PANEL_SAMSUNG_ATNA33XC20 tristate "Samsung ATNA33XC20 eDP panel" depends on OF
drivers/gpu/drm/panel/Makefile +1 -0
@@ -42,6 +42,7 @@ obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM67191) += panel-raydium-rm67191.o obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o obj-$(CONFIG_DRM_PANEL_RONBO_RB070D30) += panel-ronbo-rb070d30.o +obj-$(CONFIG_DRM_PANEL_SAMSUNG_AMS495QA01) += panel-samsung-ams495qa01.o obj-$(CONFIG_DRM_PANEL_SAMSUNG_ATNA33XC20) += panel-samsung-atna33xc20.o obj-$(CONFIG_DRM_PANEL_SAMSUNG_DB7430) += panel-samsung-db7430.o obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +547 -0
547 lines not on this page · raw message
Signature · 1 line
2.25.1
Re: [PATCH V4 2/3] drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel
Hi Chris,
thanks for your patch!
overall this looks very good. Just some nitpicks.
drivers/gpu/drm/panel/Kconfig config DRM_PANEL_RONBO_RB070D30 · line 448
Quoted 11 lines · Chris Morgan
From: Chris Morgan <[email protected]>
Support Samsung AMS495QA01 panel as found on the Anbernic RG503. Note This panel receives video signals via DSI, however it receives commands via 3-wire SPI.
Signed-off-by Chris Morgan <[email protected]>Signed-off-by Maya Matuszczyk <[email protected]>drivers/gpu/drm/panel/Kconfig +2
-0+config DRM_PANEL_SAMSUNG_AMS495QA01 + tristate "Samsung AMS495QA01 DSI panel"
I am always a bit careful with these "Samsung" panels. Well the panel is surely Samsungs and usually it has that name, but very often it is mainly a display controller, made by someone else, then a physical panel slapped on and then the actual display controller is obscured.
Typical example:panel-novatek-nt35560.c handing Samsung ACX424AKP.
If you know the actual display controller, then the driver should be renamed after that, but keeping the compatible and DT bindings as is. The reason being that tomorrow there is an LG panel using the same display controller and then we don't get duplicate code.
It feels like this is a Novatek controller. Just a feeling I have. Mostly from the way it does brightness using ELVSS and gamma. But who knows.
drivers/gpu/drm/panel/Kconfig config DRM_PANEL_RONBO_RB070D30 · line 451
Quoted 3 lines
drivers/gpu/drm/panel/Kconfig +3
-0+ depends on OF && SPI + depends on DRM_MIPI_DSI + select DRM_MIPI_DBI
Nice that you use these helpers!
Quoted 1 line
+#include <
linux/media-bus-format.h>
(...)
Quoted 1 line
+#include <
linux/of_graph.h>
Hm. We get to this.
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 62
Quoted 2 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +2
-0+/** @prepared: value tracking panel prepare status */ +bool prepared;
Drop this and associated code. The framework keeps track of this.
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 143
Quoted 2 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +2
-0+/* Set gamma change */ +mipi_dbi_command (dbi,0xf9 ,0x00 );
For cases like this where you know what the command does, please add:
#define MCS_SET_GAMMA 0xf9
(MCS = Manufacturer Command Set, cfdrivers/gpu/drm/panel/panel-samsung-s6e63m0.h)
and collect them somewhere, then you can drop the comment because it is self-evident what is going on.
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 147
Quoted 4 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +4
-0+/* Undocumented command */ +mipi_dbi_command (dbi,0x26 ,0x00 ); +/* Set ELVSS value */ +mipi_dbi_command (dbi,0xb2 , ams495qa01_elvss[tmp]);
Same.
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 159
Quoted 8 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +8
-0+static int ams495qa01_prepare (struct drm_panel *panel) +{ +struct ams495qa01 *db =to_ams495qa01 (panel); +struct mipi_dbi *dbi = &db->dbi; +int ret; + +if (db->prepared) +return 0 ;
As mentioned skip this.
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 167
Quoted 7 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +7
-0+/* Power up */ + ret =regulator_enable (db->reg_vdd); +if (ret) { +dev_err (db->dev,"failed to enable vdd regulator: %d\n" , ret); +return ret; + } +if (db->reg_elvdd) {
Do you really need to if() this? I thought the regulator framework would just ignore the calls for an optional regulator.
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 193
Quoted 3 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +3
-0+/* Password to start command sequence */ +mipi_dbi_command (dbi,0xf0 ,0x5a ,0x5a ); +mipi_dbi_command (dbi,0xf1 ,0x5a ,0x5a );
Use the #defines from:drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
I'm pretty sure they mean the same thing:
#define MCS_LEVEL_2_KEY 0xf0
#define MCS_MTP_KEY 0xf1
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 201
Quoted 3 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +3
-0+/* Analog Power condition set */ +mipi_dbi_command (dbi,0xf4 ,0x33 ,0x42 ,0x00 ,0x08 ); +mipi_dbi_command (dbi,0xf5 ,0x00 ,0x06 ,0x26 ,0x35 ,0x03 );
Define MCS_ commands.
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 220
Quoted 13 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +13
-0+/* GTCON set */ +mipi_dbi_command (dbi,0xf7 ,0x20 ); + +/* TEMP_SWIRE set */ +mipi_dbi_command (dbi,0xb2 ,0x06 ,0x06 ,0x06 ,0x06 ); + +/* ELVSS_CON set */ +mipi_dbi_command (dbi,0xb1 ,0x07 ,0x00 ,0x10 ); + +/* Gateless signal set */ +mipi_dbi_command (dbi,0xf8 ,0x7f ,0x7a ,0x89 ,0x67 ,0x26 ,0x38 , +0x00 ,0x00 ,0x09 ,0x67 ,0x70 ,0x88 ,0x7a , +0x76 ,0x05 ,0x09 ,0x23 ,0x23 ,0x23 );
Dito
Quoted 1 line
+
db->prepared= true;
Drop.
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 248
Quoted 7 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +7
-0+static int ams495qa01_unprepare (struct drm_panel *panel) +{ +struct ams495qa01 *db =to_ams495qa01 (panel); +struct mipi_dbi *dbi = &db->dbi; + +if (!db->prepared) +return 0 ;
Drop.
Quoted 10 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +10
-0+static int ams495qa01_get_modes (struct drm_panel *panel, +struct drm_connector *connector) +{ +struct ams495qa01 *db =to_ams495qa01 (panel); +const struct ams495qa01_panel_info *panel_info = db->panel_info; +struct drm_display_mode *mode; +static const u32 bus_format =MEDIA_BUS_FMT_RGB888_1X24 ; +unsigned int i; + +for (i =0 ; i < panel_info->num_modes; i++) {
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 287
Quoted 10 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +10
-0+ mode =drm_mode_duplicate (connector->dev, + &panel_info->display_modes[i]); +if (!mode) +return -ENOMEM ; + +drm_mode_set_name (mode); + + mode->type =DRM_MODE_TYPE_DRIVER ; +if (panel_info->num_modes ==1 ) + mode->type |=DRM_MODE_TYPE_PREFERRED ;
I think you should probably set the preferred mode even if there are several of them? But maybe just on the first or something. (A bit unsure here, Sam?)
Quoted 4 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +4
-0+static int ams495qa01_probe (struct spi_device *spi) +{ +struct device *dev = &spi->dev; +struct device_node *endpoint, *dsi_host_node;
(...)
Quoted 1 line
+
db->reset=devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
I would request this GPIOD_OUT_HIGH so reset is asserted on probe.
Quoted 1 line
+
db->enable=devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
And this GPIOD_OUT_LOW so panel is disabled after probe.
Quoted 17 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +17
-0+/* +* Get the DSI controller that is supplying data for this display +* which is controlled via SPI 3-wire. +*/ + endpoint =of_graph_get_next_endpoint (dev->of_node,NULL ); +if (!endpoint) { +dev_err (dev,"failed to get endpoint\n" ); +return -ENODEV ; + } + dsi_host_node =of_graph_get_remote_port_parent (endpoint); +if (!dsi_host_node) { +dev_err (dev,"failed to get remote port parent\n" ); + goto put_endpoint; + } + dsi_host =of_find_mipi_dsi_host_by_node (dsi_host_node); +if (!dsi_host) { +dev_err (dev,"failed to find dsi host\n" );
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 421
Quoted 10 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +10
-0+ goto put_host; + } + info.node =of_graph_get_remote_port (endpoint); +if (!info.node) { +dev_err (dev,"failed to get remote port node\n" ); + ret = -ENODEV ; + goto put_host; + } + + db->dsi_dev =devm_mipi_dsi_device_register_full (dev, dsi_host, &info);
I don't get this part.
Why do you have to go through all this trouble when no other panel drivers, not even the other SPI DBI panel drivers, does this?
drm_of_find_panel_or_bridge() should typically result in the core doing all the lookup and connecting for you from the host side should it not?
At the very least this requires a big comment about what is going on and why and what makes this necessary.
I can only guess the following:
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 435
Quoted 7 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +7
-0+ db->dsi_dev->lanes =2 ; + db->dsi_dev->format =MIPI_DSI_FMT_RGB888 ; + db->dsi_dev->mode_flags =MIPI_DSI_MODE_VIDEO |MIPI_DSI_MODE_VIDEO_BURST | +MIPI_DSI_MODE_LPM |MIPI_DSI_MODE_NO_EOT_PACKET ; + +drm_panel_init (&db->panel, dev, &ams495qa01_drm_funcs, +DRM_MODE_CONNECTOR_DSI );
Pixel data passes to the display using DSI but all display control is done over SPI, and the core will not help with this.
So from the display controller POV this is a DSI display and from the display POV this is an SPI-controlled display. So it sits on two buses. Data path is on DSI, control path is on SPI.
This would be kind of odd actually, normally DSI displays do the display control over DSI as well. SPI control is usually used on DPI displays. But I'm not surprised.
If this is necessary, isn't this something we need to teach the core to handle instead of adding quirks like this to all drivers that have this characteristic?
Yours,
Linus Walleij
Heiko Stübner
maintainerRe: [PATCH V4 2/3] drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel
Hi Linus,
Am Samstag, 3. Dezember 2022, 10:03:42 CET schrieb Linus Walleij:
Quoted 28 lines · Chris Morgan
drivers/gpu/drm/panel/Kconfig config DRM_PANEL_RONBO_RB070D30 · line 448
Quoted 12 lines
From: Chris Morgan <[email protected]>
Support Samsung AMS495QA01 panel as found on the Anbernic RG503. Note This panel receives video signals via DSI, however it receives commands via 3-wire SPI.
Signed-off-by Chris Morgan <[email protected]>Signed-off-by Maya Matuszczyk <[email protected]>drivers/gpu/drm/panel/Kconfig +2
-0+config DRM_PANEL_SAMSUNG_AMS495QA01 + tristate "Samsung AMS495QA01 DSI panel"I am always a bit careful with these "Samsung" panels. Well the panel is surely Samsungs and usually it has that name, but very often it is mainly a display controller, made by someone else, then a physical panel slapped on and then the actual display controller is obscured.
Typical example:
panel-novatek-nt35560.chanding Samsung ACX424AKP.If you know the actual display controller, then the driver should be renamed after that, but keeping the compatible and DT bindings as is. The reason being that tomorrow there is an LG panel using the same display controller and then we don't get duplicate code.
Quoted 3 lines
It feels like this is a Novatek controller. Just a feeling I have. Mostly from the way it does brightness using ELVSS and gamma. But who knows.
Though in past projects I've seen the same display-controller used with different panels (and different dsi-init-sequences). In one project the display manufacturer even EOL'ed the first panel and provided a replacement with said same display controller (and a different init) - but the datasheets for the display-controller were for the same chip still.
So while in my experience the actual display name from the manufacturer identifies the display + controller combo, I don't really think you can go the other way with the controller name identifying the display+controller combination.
But that whole display business is arcane anyway, so this also only stems from past projects and not factual knowledge ;-) .
Heiko
Re: [PATCH V4 2/3] drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel
Quoted 10 lines · Heiko Stübner
Though in past projects I've seen the same display-controller used with different panels (and different dsi-init-sequences). In one project the display manufacturer even EOL'ed the first panel and provided a replacement with said same display controller (and a different init) - but the datasheets for the display-controller were for the same chip still.
So while in my experience the actual display name from the manufacturer identifies the display + controller combo, I don't really think you can go the other way with the controller name identifying the display+controller combination.
I don't mean we should do that.
What I mean is:
- Name the driver after the controller, if we know which one it is, such as panel-novatek-nt35510.c
- Provide identifications of the controller+panel combo in e.g. DT compatible strings, provide init data as arrays in the per-variant match data (or similar for ACPI).
Yours,
Linus Walleij
Re: [PATCH V4 2/3] drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel
In reply to Linus WalleijQuoted 34 lines · Linus Walleij
Hi Chris,
thanks for your patch!
overall this looks very good. Just some nitpicks.
drivers/gpu/drm/panel/Kconfig config DRM_PANEL_RONBO_RB070D30 · line 448
Quoted 11 lines · Chris Morgan
From: Chris Morgan <[email protected]>
Support Samsung AMS495QA01 panel as found on the Anbernic RG503. Note This panel receives video signals via DSI, however it receives commands via 3-wire SPI.
Signed-off-by Chris Morgan <[email protected]>Signed-off-by Maya Matuszczyk <[email protected]>drivers/gpu/drm/panel/Kconfig +2
-0+config DRM_PANEL_SAMSUNG_AMS495QA01 + tristate "Samsung AMS495QA01 DSI panel"I am always a bit careful with these "Samsung" panels. Well the panel is surely Samsungs and usually it has that name, but very often it is mainly a display controller, made by someone else, then a physical panel slapped on and then the actual display controller is obscured.
Typical example:
panel-novatek-nt35560.chanding Samsung ACX424AKP.If you know the actual display controller, then the driver should be renamed after that, but keeping the compatible and DT bindings as is. The reason being that tomorrow there is an LG panel using the same display controller and then we don't get duplicate code.
Quoted 4 lines
It feels like this is a Novatek controller. Just a feeling I have. Mostly from the way it does brightness using ELVSS and gamma. But who knows.
Will do. I'll make the changes and resubmit. For what it's worth the documentation says this one is a Samsung AMS495QA01 panel on a Magnachip D53E6EA8966 controller IC.
Quoted 11 lines
drivers/gpu/drm/panel/Kconfig config DRM_PANEL_RONBO_RB070D30 · line 451
Quoted 3 lines
drivers/gpu/drm/panel/Kconfig +3
-0+ depends on OF && SPI + depends on DRM_MIPI_DSI + select DRM_MIPI_DBINice that you use these helpers!
Quoted 1 line
+#include <
linux/media-bus-format.h>(...)
Quoted 1 line
+#include <
linux/of_graph.h>Hm. We get to this.
*gulp*
Quoted 6 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 62
Quoted 3 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +2
-0+/** @prepared: value tracking panel prepare status */ +bool prepared;Drop this and associated code. The framework keeps track of this.
Will do.
Quoted 6 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 143
Quoted 2 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +2
-0+/* Set gamma change */ +mipi_dbi_command (dbi,0xf9 ,0x00 );For cases like this where you know what the command does, please add:
Quoted 8 lines
#define
MCS_SET_GAMMA0xf9(MCS = Manufacturer Command Set, cf
drivers/gpu/drm/panel/panel-samsung-s6e63m0.h)and collect them somewhere, then you can drop the comment because it is self-evident what is going on.
Gotcha, will do.
Quoted 26 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 147
Quoted 4 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +4
-0+/* Undocumented command */ +mipi_dbi_command (dbi,0x26 ,0x00 ); +/* Set ELVSS value */ +mipi_dbi_command (dbi,0xb2 , ams495qa01_elvss[tmp]);Same.
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 159
Quoted 8 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +8
-0+static int ams495qa01_prepare (struct drm_panel *panel) +{ +struct ams495qa01 *db =to_ams495qa01 (panel); +struct mipi_dbi *dbi = &db->dbi; +int ret; + +if (db->prepared) +return 0 ;As mentioned skip this.
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 167
Quoted 7 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +7
-0+/* Power up */ + ret =regulator_enable (db->reg_vdd); +if (ret) { +dev_err (db->dev,"failed to enable vdd regulator: %d\n" , ret); +return ret; + } +if (db->reg_elvdd) {
Quoted 3 lines
Do you really need to
if()this? I thought the regulator framework would just ignore the calls for an optional regulator.
I don't know for sure, but I'll make the change if you request it. I think other drivers had an if in this scenario which is why I did it.
Quoted 64 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 193
Quoted 4 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +3
-0+/* Password to start command sequence */ +mipi_dbi_command (dbi,0xf0 ,0x5a ,0x5a ); +mipi_dbi_command (dbi,0xf1 ,0x5a ,0x5a );Use the #defines from:
drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
I'm pretty sure they mean the same thing:#define
MCS_LEVEL_2_KEY0xf0
#defineMCS_MTP_KEY0xf1drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 201
Quoted 3 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +3
-0+/* Analog Power condition set */ +mipi_dbi_command (dbi,0xf4 ,0x33 ,0x42 ,0x00 ,0x08 ); +mipi_dbi_command (dbi,0xf5 ,0x00 ,0x06 ,0x26 ,0x35 ,0x03 );Define
MCS_commands.drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 220
Quoted 13 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +13
-0+/* GTCON set */ +mipi_dbi_command (dbi,0xf7 ,0x20 ); + +/* TEMP_SWIRE set */ +mipi_dbi_command (dbi,0xb2 ,0x06 ,0x06 ,0x06 ,0x06 ); + +/* ELVSS_CON set */ +mipi_dbi_command (dbi,0xb1 ,0x07 ,0x00 ,0x10 ); + +/* Gateless signal set */ +mipi_dbi_command (dbi,0xf8 ,0x7f ,0x7a ,0x89 ,0x67 ,0x26 ,0x38 , +0x00 ,0x00 ,0x09 ,0x67 ,0x70 ,0x88 ,0x7a , +0x76 ,0x05 ,0x09 ,0x23 ,0x23 ,0x23 );Dito
Quoted 1 line
+
db->prepared= true;Drop.
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 248
Quoted 7 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +7
-0+static int ams495qa01_unprepare (struct drm_panel *panel) +{ +struct ams495qa01 *db =to_ams495qa01 (panel); +struct mipi_dbi *dbi = &db->dbi; + +if (!db->prepared) +return 0 ;Drop.
Quoted 16 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +16
-0+static int ams495qa01_get_modes (struct drm_panel *panel, +struct drm_connector *connector) +{ +struct ams495qa01 *db =to_ams495qa01 (panel); +const struct ams495qa01_panel_info *panel_info = db->panel_info; +struct drm_display_mode *mode; +static const u32 bus_format =MEDIA_BUS_FMT_RGB888_1X24 ; +unsigned int i; + +for (i =0 ; i < panel_info->num_modes; i++) { + mode =drm_mode_duplicate (connector->dev, + &panel_info->display_modes[i]); +if (!mode) +return -ENOMEM ; + +drm_mode_set_name (mode);
Quoted 9 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 287
Quoted 4 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +4
-0+ + mode->type =DRM_MODE_TYPE_DRIVER ; +if (panel_info->num_modes ==1 ) + mode->type |=DRM_MODE_TYPE_PREFERRED ;I think you should probably set the preferred mode even if there are several of them? But maybe just on the first or something. (A bit unsure here, Sam?)
I'll keep 60hz as the preferred. 50hz was added at the request of some userspace folks for running PAL based emulators and stuff.
Quoted 9 lines
Quoted 4 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +4
-0+static int ams495qa01_probe (struct spi_device *spi) +{ +struct device *dev = &spi->dev; +struct device_node *endpoint, *dsi_host_node;(...)
Quoted 1 line
+
db->reset=devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);I would request this
GPIOD_OUT_HIGHso reset is asserted on probe.
Will do.
Quoted 4 lines
Quoted 1 line
+
db->enable=devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);And this
GPIOD_OUT_LOWso panel is disabled after probe.
Will do.
Quoted 33 lines
Quoted 17 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +17
-0+/* +* Get the DSI controller that is supplying data for this display +* which is controlled via SPI 3-wire. +*/ + endpoint =of_graph_get_next_endpoint (dev->of_node,NULL ); +if (!endpoint) { +dev_err (dev,"failed to get endpoint\n" ); +return -ENODEV ; + } + dsi_host_node =of_graph_get_remote_port_parent (endpoint); +if (!dsi_host_node) { +dev_err (dev,"failed to get remote port parent\n" ); + goto put_endpoint; + } + dsi_host =of_find_mipi_dsi_host_by_node (dsi_host_node); +if (!dsi_host) { +dev_err (dev,"failed to find dsi host\n" );drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 421
Quoted 10 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +10
-0+ goto put_host; + } + info.node =of_graph_get_remote_port (endpoint); +if (!info.node) { +dev_err (dev,"failed to get remote port node\n" ); + ret = -ENODEV ; + goto put_host; + } + + db->dsi_dev =devm_mipi_dsi_device_register_full (dev, dsi_host, &info);I don't get this part.
Why do you have to go through all this trouble when no other panel drivers, not even the other SPI DBI panel drivers, does this?
Quoted 9 lines
drm_of_find_panel_or_bridge()should typically result in the core doing all the lookup and connecting for you from the host side should it not?At the very least this requires a big comment about what is going on and why and what makes this necessary.
I can only guess the following:
This is the path that "works", but I'll happily change to something else.
Quoted 16 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 435
Quoted 7 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +7
-0+ db->dsi_dev->lanes =2 ; + db->dsi_dev->format =MIPI_DSI_FMT_RGB888 ; + db->dsi_dev->mode_flags =MIPI_DSI_MODE_VIDEO |MIPI_DSI_MODE_VIDEO_BURST | +MIPI_DSI_MODE_LPM |MIPI_DSI_MODE_NO_EOT_PACKET ; + +drm_panel_init (&db->panel, dev, &ams495qa01_drm_funcs, +DRM_MODE_CONNECTOR_DSI );Pixel data passes to the display using DSI but all display control is done over SPI, and the core will not help with this.
So from the display controller POV this is a DSI display and from the display POV this is an SPI-controlled display. So it sits on two buses. Data path is on DSI, control path is on SPI.
Quoted 8 lines
This would be kind of odd actually, normally DSI displays do the display control over DSI as well. SPI control is usually used on DPI displays. But I'm not surprised.
If this is necessary, isn't this something we need to teach the core to handle instead of adding quirks like this to all drivers that have this characteristic?
You are correct, this panel is controlled via 3-wire SPI in my example. The panel can be controlled either by 3-wire SPI or DSI commands depending on whether or not pin 15 is driven high or low. Unfortunately in my case it's hardwired high, so I am forced to do it via 3-wire SPI. I have no way of testing it with pure DSI but that would simplify things quite a bit. Pixel data is transmitted soley through DSI.
The way I have it implemented currently is to put the panel on the SPI bus as a DBI panel; traverse through the DT bindings to find the associated DSI controller, then attach it as a DSI device so the DSI bus can transmit the pixel data.
I'm absolutely cool with making those functions part of the core and not just specific to this panel, only I might need a bit of help on that part to make sure I do it the right way. I just wasn't sure how often that would be needed since this is the only panel I've ever seen driven this way, especially since it seems like any sane person would just want to do the whole control data/pixel data over DSI to keep things simple.
Quoted 2 lines
Yours,
Linus Walleij
Thank you for your input.
Re: [PATCH V4 2/3] drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel
Quoted 3 lines · Chris Morgan
Will do. I'll make the changes and resubmit. For what it's worth the documentation says this one is a Samsung AMS495QA01 panel on a Magnachip D53E6EA8966 controller IC.
I would name the driver panel-magnachip-d53e6ea8966.c and KConfig PANEL_MAGNACHIP_D53E6EA8966 for now
but keep the Samsung compatible string & match.
Maybe this driver will match more magnachips in the future, maybe not. Sometimes people get hold of datasheets and submit proper code and #defines etc.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
+ if (
db->reg_elvdd) {Do you really need to
if()this? I thought the regulator framework would just ignore the calls for an optional regulator.I don't know for sure, but I'll make the change if you request it. I think other drivers had an if in this scenario which is why I did it.
Okay but I don't think that is necessary.
Quoted 10 lines
Quoted 7 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 287
Quoted 3 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +3
-0+ mode->type =DRM_MODE_TYPE_DRIVER ; +if (panel_info->num_modes ==1 ) + mode->type |=DRM_MODE_TYPE_PREFERRED ;I think you should probably set the preferred mode even if there are several of them? But maybe just on the first or something. (A bit unsure here, Sam?)
I'll keep 60hz as the preferred. 50hz was added at the request of some userspace folks for running PAL based emulators and stuff.
OK
Quoted 28 lines
This is the path that "works", but I'll happily change to something else.
Quoted 16 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c line 435
Quoted 7 lines
drivers/gpu/drm/panel/panel-samsung-ams495qa01.c new file +7
-0+ db->dsi_dev->lanes =2 ; + db->dsi_dev->format =MIPI_DSI_FMT_RGB888 ; + db->dsi_dev->mode_flags =MIPI_DSI_MODE_VIDEO |MIPI_DSI_MODE_VIDEO_BURST | +MIPI_DSI_MODE_LPM |MIPI_DSI_MODE_NO_EOT_PACKET ; + +drm_panel_init (&db->panel, dev, &ams495qa01_drm_funcs, +DRM_MODE_CONNECTOR_DSI );Pixel data passes to the display using DSI but all display control is done over SPI, and the core will not help with this.
So from the display controller POV this is a DSI display and from the display POV this is an SPI-controlled display. So it sits on two buses. Data path is on DSI, control path is on SPI.
Quoted 8 lines
This would be kind of odd actually, normally DSI displays do the display control over DSI as well. SPI control is usually used on DPI displays. But I'm not surprised.
If this is necessary, isn't this something we need to teach the core to handle instead of adding quirks like this to all drivers that have this characteristic?
Quoted 6 lines
You are correct, this panel is controlled via 3-wire SPI in my example. The panel can be controlled either by 3-wire SPI or DSI commands depending on whether or not pin 15 is driven high or low. Unfortunately in my case it's hardwired high, so I am forced to do it via 3-wire SPI. I have no way of testing it with pure DSI but that would simplify things quite a bit. Pixel data is transmitted soley through DSI.
OK
Quoted 5 lines
The way I have it implemented currently is to put the panel on the SPI bus as a DBI panel; traverse through the DT bindings to find the associated DSI controller, then attach it as a DSI device so the DSI bus can transmit the pixel data.
Quoted 7 lines
I'm absolutely cool with making those functions part of the core and not just specific to this panel, only I might need a bit of help on that part to make sure I do it the right way. I just wasn't sure how often that would be needed since this is the only panel I've ever seen driven this way, especially since it seems like any sane person would just want to do the whole control data/pixel data over DSI to keep things simple.
It doesn't seem all that unique.
Can you put some helper in drivers/gpu/drm/drm_of.c with the rest and it will (hopefully) not be linked in unless used anyway.
Yours,
Linus Walleij
PATCH v4 3/3 arm64: dts: rockchip: add display to RG503
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add Samsung AMS495QA01 panel to RG503.
--- .../dts/rockchip/rk3566-anbernic-rg503.dts | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+)
arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts +61 -0
@@ -47,6 +47,22 @@ gpio_spi: spi { mosi-gpios = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; cs-gpios = <&gpio4 RK_PA7 GPIO_ACTIVE_HIGH>; num-chipselects = <0>; + + panel@0 { + compatible = "samsung,ams495qa01"; + reg = <0>; + enable-gpios = <&gpio4 RK_PB7 GPIO_ACTIVE_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&lcd_enable>, <&lcd_reset>; + reset-gpios = <&gpio4 RK_PA0 GPIO_ACTIVE_LOW>; + vdd-supply = <&vcc3v3_lcd0_n>; + + port { + mipi_in_panel: endpoint { + remote-endpoint = <&mipi_out_panel>; + }; + }; + }; }; /* Channels reversed for both headphones and speakers. */
@@ -94,6 +110,32 @@ &cru { assigned-clock-rates = <1200000000>, <200000000>, <500000000>; }; +&dsi_dphy0 { + status = "okay"; +}; + +&dsi0 { + status = "okay"; + + ports { + dsi0_in: port@0 { + reg = <0>; + + dsi0_in_vp1: endpoint { + remote-endpoint = <&vp1_out_dsi0>; + }; + }; + + dsi0_out: port@1 { + reg = <1>; + + mipi_out_panel: endpoint { + remote-endpoint = <&mipi_in_panel>; + }; + }; + }; +}; + &gpio_keys_control { button-a { gpios = <&gpio3 RK_PC1 GPIO_ACTIVE_LOW>;
@@ -146,6 +188,18 @@ spk_amp_enable_h: spk-amp-enable-h { }; }; + gpio-lcd { + lcd_enable: lcd-enable { + rockchip,pins = + <4 RK_PB7 RK_FUNC_GPIO &pcfg_pull_none>; + }; + + lcd_reset: lcd-reset { + rockchip,pins = + <4 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>; + }; + }; + gpio-spi { spi_pins: spi-pins { rockchip,pins =
@@ -164,3 +218,10 @@ rk817_charger: charger { rockchip,sleep-filter-current-microamp = <100000>; }; }; + +&vp1 { + vp1_out_dsi0: endpoint@ROCKCHIP_VOP2_EP_MIPI0 { + reg = <ROCKCHIP_VOP2_EP_MIPI0>; + remote-endpoint = <&dsi0_in_vp1>; + }; +};
Signature · 1 line
2.25.1