add at91sam9 LCDC DRM driver

Use vendor name for directory, adding a suitable place for more atmel DRM drivers.

Signed-off-by Sam Ravnborg <[email protected]>

Cc Boris Brezillon <[email protected]>

---
 MAINTAINERS                                                 | 2 +-
 drivers/gpu/drm/Kconfig                                     | 2 +-
 drivers/gpu/drm/Makefile                                    | 2 +-
 drivers/gpu/drm/{atmel-hlcdc => atmel}/Kconfig              | 6 ++++++
 drivers/gpu/drm/{atmel-hlcdc => atmel}/Makefile             | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_crtc.c   | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.c     | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.h     | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_output.c | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c  | 0
 10 files changed, 9 insertions(+), 3 deletions(-)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/Kconfig (83%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/Makefile (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_crtc.c (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.c (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.h (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_output.c (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c (100%)

MAINTAINERS +0 -0

index 96e98e206b0d..09ce76a9a1dc 100644

MAINTAINERS +0 -0

 F:	Documentation/devicetree/bindings/display/atmel/
 T:	git git://anongit.freedesktop.org/drm/drm-misc
 

drivers/gpu/drm/Kconfig +1 -1

index 2a72d2feb76d..4130df0c0dba 100644
@@ -232,7 +232,7 @@ source "drivers/gpu/drm/cirrus/Kconfig"
 
 source "drivers/gpu/drm/armada/Kconfig"
 
-source "drivers/gpu/drm/atmel-hlcdc/Kconfig"
+source "drivers/gpu/drm/atmel/Kconfig"
 
 source "drivers/gpu/drm/rcar-du/Kconfig"
 

drivers/gpu/drm/Makefile +1 -1

index ef9f3dab287f..ce9829967128 100644
@@ -76,7 +76,7 @@ obj-$(CONFIG_DRM_GMA500) += gma500/
 obj-$(CONFIG_DRM_UDL) += udl/
 obj-$(CONFIG_DRM_AST) += ast/
 obj-$(CONFIG_DRM_ARMADA) += armada/
-obj-$(CONFIG_DRM_ATMEL_HLCDC)	+= atmel-hlcdc/
+obj-$(CONFIG_DRM_ATMEL)	+= atmel/
 obj-$(CONFIG_DRM_RCAR_DU) += rcar-du/
 obj-$(CONFIG_DRM_SHMOBILE) +=shmobile/
 obj-y			+= omapdrm/

drivers/gpu/drm/atmel/Kconfig renamed from drivers/gpu/drm/atmel-hlcdc/Kconfig +6 -0

similarity index 83%
rename from drivers/gpu/drm/atmel-hlcdc/Kconfig
rename to drivers/gpu/drm/atmel/Kconfig
index 32bcc4bad06a..7cd3862f9d18 100644
@@ -1,6 +1,12 @@
+config DRM_ATMEL
+	bool
+	help
+	  Enable Atmel DRM support
+
 config DRM_ATMEL_HLCDC
 	tristate "DRM Support for ATMEL HLCDC Display Controller"
 	depends on DRM && OF && COMMON_CLK && MFD_ATMEL_HLCDC && ARM
+	select DRM_ATMEL
 	select DRM_GEM_CMA_HELPER
 	select DRM_KMS_HELPER
 	select DRM_KMS_CMA_HELPER

drivers/gpu/drm/atmel/Makefile renamed from drivers/gpu/drm/atmel-hlcdc/Makefile +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/Makefile
rename to drivers/gpu/drm/atmel/Makefile

drivers/gpu/drm/atmel/atmel_hlcdc_crtc.c renamed from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
rename to drivers/gpu/drm/atmel/atmel_hlcdc_crtc.c

drivers/gpu/drm/atmel/atmel_hlcdc_dc.c renamed from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
rename to drivers/gpu/drm/atmel/atmel_hlcdc_dc.c

drivers/gpu/drm/atmel/atmel_hlcdc_dc.h renamed from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
rename to drivers/gpu/drm/atmel/atmel_hlcdc_dc.h

drivers/gpu/drm/atmel/atmel_hlcdc_output.c renamed from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
rename to drivers/gpu/drm/atmel/atmel_hlcdc_output.c

drivers/gpu/drm/atmel/atmel_hlcdc_plane.c renamed from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
rename to drivers/gpu/drm/atmel/atmel_hlcdc_plane.c

Signature · 6 lines

2.12.0

_______________________________________________
dri-devel mailing list
dri-devel at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

MAINTAINERS DRM DRIVERS FOR ATMEL HLCDC · line 4684

Quoted 12 lines

 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c (100%)

MAINTAINERS +1 -1

index 96e98e206b0d..09ce76a9a1dc 100644
@@ -4681,7 +4681,7 @@ DRM DRIVERS FOR ATMEL HLCDC
 M:	Boris Brezillon <[email protected]>
 L:	[email protected]
 S:	Supported
-F:	drivers/gpu/drm/atmel-hlcdc/
+F:	drivers/gpu/drm/atmel/atmel-hlcdc*

I'd strongly suggest you group-maintain the entire atmel stuff in drm-misc instead of only atmel-hlcdc.

Hi Daniel.

Quoted 15 lines

MAINTAINERS DRM DRIVERS FOR ATMEL HLCDC · line 4684

Quoted 12 lines

 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c (100%)

MAINTAINERS +1 -1

index 96e98e206b0d..09ce76a9a1dc 100644
@@ -4681,7 +4681,7 @@ DRM DRIVERS FOR ATMEL HLCDC
 M:	Boris Brezillon <[email protected]>
 L:	dri-devel at lists.freedesktop.org
 S:	Supported
-F:	drivers/gpu/drm/atmel-hlcdc/
+F:	drivers/gpu/drm/atmel/atmel-hlcdc*

I'd strongly suggest you group-maintain the entire atmel stuff in drm-misc instead of only atmel-hlcdc.

If OK with Boris then I am fine with this too.
Will suggest so in v2.

Quoted 6 lines

Quoted 3 lines

I'd strongly suggest you group-maintain the entire atmel stuff in drm-misc instead of only atmel-hlcdc.

If OK with Boris then I am fine with this too.
Will suggest so in v2.

Boris acked too, but he's on vacation. He said on irc he'll review the patches when he's back. Meanwhile I guess you can prep by getting the fd.o account requested:

https://www.freedesktop.org/wiki/AccountRequests/

And checking out the tooling we use to maintain drm-misc:

https://dri.freedesktop.org/docs/dim/getting-started.html

Hi Daniel.

Quoted 13 lines

Quoted 6 lines

Quoted 3 lines

I'd strongly suggest you group-maintain the entire atmel stuff in drm-misc instead of only atmel-hlcdc.

If OK with Boris then I am fine with this too.
Will suggest so in v2.

Boris acked too, but he's on vacation. He said on irc he'll review the patches when he's back. Meanwhile I guess you can prep by getting the fd.o account requested:

https://www.freedesktop.org/wiki/AccountRequests/

Quoted 3 lines

And checking out the tooling we use to maintain drm-misc:

https://dri.freedesktop.org/docs/dim/getting-started.html

With enough on the plate already I decided to postpone this part until the migration to gitlab is done.
Just to tell this was not ignored, only postponed.

Use vendor name for directory, adding a suitable place for more atmel DRM drivers.

Signed-off-by Sam Ravnborg <[email protected]>

Cc Boris Brezillon <[email protected]>

---
 MAINTAINERS                                                 | 2 +-
 drivers/gpu/drm/Kconfig                                     | 2 +-
 drivers/gpu/drm/Makefile                                    | 2 +-
 drivers/gpu/drm/{atmel-hlcdc => atmel}/Kconfig              | 6 ++++++
 drivers/gpu/drm/{atmel-hlcdc => atmel}/Makefile             | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_crtc.c   | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.c     | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.h     | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_output.c | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c  | 0
 10 files changed, 9 insertions(+), 3 deletions(-)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/Kconfig (83%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/Makefile (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_crtc.c (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.c (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.h (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_output.c (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c (100%)

MAINTAINERS +0 -0

index 96e98e206b0d..09ce76a9a1dc 100644

MAINTAINERS +0 -0

 F:	Documentation/devicetree/bindings/display/atmel/
 T:	git git://anongit.freedesktop.org/drm/drm-misc
 

drivers/gpu/drm/Kconfig +1 -1

index 2a72d2feb76d..4130df0c0dba 100644
@@ -232,7 +232,7 @@ source "drivers/gpu/drm/cirrus/Kconfig"
 
 source "drivers/gpu/drm/armada/Kconfig"
 
-source "drivers/gpu/drm/atmel-hlcdc/Kconfig"
+source "drivers/gpu/drm/atmel/Kconfig"
 
 source "drivers/gpu/drm/rcar-du/Kconfig"
 

drivers/gpu/drm/Makefile +1 -1

index ef9f3dab287f..ce9829967128 100644
@@ -76,7 +76,7 @@ obj-$(CONFIG_DRM_GMA500) += gma500/
 obj-$(CONFIG_DRM_UDL) += udl/
 obj-$(CONFIG_DRM_AST) += ast/
 obj-$(CONFIG_DRM_ARMADA) += armada/
-obj-$(CONFIG_DRM_ATMEL_HLCDC)	+= atmel-hlcdc/
+obj-$(CONFIG_DRM_ATMEL)	+= atmel/
 obj-$(CONFIG_DRM_RCAR_DU) += rcar-du/
 obj-$(CONFIG_DRM_SHMOBILE) +=shmobile/
 obj-y			+= omapdrm/

drivers/gpu/drm/atmel/Kconfig renamed from drivers/gpu/drm/atmel-hlcdc/Kconfig +0 -0

similarity index 83%
rename from drivers/gpu/drm/atmel-hlcdc/Kconfig
rename to drivers/gpu/drm/atmel/Kconfig
index 32bcc4bad06a..7cd3862f9d18 100644

drivers/gpu/drm/atmel/Kconfig renamed from drivers/gpu/drm/atmel-hlcdc/Kconfig +1 -0

 config DRM_ATMEL_HLCDC
 	tristate "DRM Support for ATMEL HLCDC Display Controller"
 	depends on DRM && OF && COMMON_CLK && MFD_ATMEL_HLCDC && ARM
+	select DRM_ATMEL
 	select DRM_GEM_CMA_HELPER
 	select DRM_KMS_HELPER
 	select DRM_KMS_CMA_HELPER

drivers/gpu/drm/atmel/Makefile renamed from drivers/gpu/drm/atmel-hlcdc/Makefile +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/Makefile
rename to drivers/gpu/drm/atmel/Makefile

drivers/gpu/drm/atmel/atmel_hlcdc_crtc.c renamed from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
rename to drivers/gpu/drm/atmel/atmel_hlcdc_crtc.c

drivers/gpu/drm/atmel/atmel_hlcdc_dc.c renamed from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
rename to drivers/gpu/drm/atmel/atmel_hlcdc_dc.c

drivers/gpu/drm/atmel/atmel_hlcdc_dc.h renamed from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
rename to drivers/gpu/drm/atmel/atmel_hlcdc_dc.h

drivers/gpu/drm/atmel/atmel_hlcdc_output.c renamed from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
rename to drivers/gpu/drm/atmel/atmel_hlcdc_output.c

drivers/gpu/drm/atmel/atmel_hlcdc_plane.c renamed from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +0 -0

similarity index 100%
rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
rename to drivers/gpu/drm/atmel/atmel_hlcdc_plane.c

Quoted 30 lines · On Sun, 12 Aug 2018 20:46:23 +0200 Sam Ravnborg <[email protected]> wrote:

Use vendor name for directory, adding a suitable place for more atmel DRM drivers.

Signed-off-by Sam Ravnborg <[email protected]>

Cc Boris Brezillon <[email protected]>

---
 MAINTAINERS                                                 | 2 +-
 drivers/gpu/drm/Kconfig                                     | 2 +-
 drivers/gpu/drm/Makefile                                    | 2 +-
 drivers/gpu/drm/{atmel-hlcdc => atmel}/Kconfig              | 6 ++++++
 drivers/gpu/drm/{atmel-hlcdc => atmel}/Makefile             | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_crtc.c   | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.c     | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.h     | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_output.c | 0
 drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c  | 0
 10 files changed, 9 insertions(+), 3 deletions(-)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/Kconfig (83%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/Makefile (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_crtc.c (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.c (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.h (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_output.c (100%)
 rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c (100%)

MAINTAINERS +0 -0

index 96e98e206b0d..09ce76a9a1dc 100644

MAINTAINERS DRM DRIVERS FOR ATMEL HLCDC · line 4684

Quoted 5 lines

MAINTAINERS +1 -1

@@ -4681,7 +4681,7 @@ DRM DRIVERS FOR ATMEL HLCDC
 M:	Boris Brezillon <[email protected]>
 L:	dri-devel at lists.freedesktop.org
 S:	Supported
-F:	drivers/gpu/drm/atmel-hlcdc/
+F:	drivers/gpu/drm/atmel/atmel-hlcdc*

atmel_hlcdc*

Also, I think you can keep the whole directory, and add your name plus update the entry description after adding code for the LCDC block.

Quoted 39 lines

MAINTAINERS +0 -0

 F:	Documentation/devicetree/bindings/display/atmel/
 T:	git git://anongit.freedesktop.org/drm/drm-misc
 

drivers/gpu/drm/Kconfig +1 -1

index 2a72d2feb76d..4130df0c0dba 100644
@@ -232,7 +232,7 @@ source "drivers/gpu/drm/cirrus/Kconfig"
 
 source "drivers/gpu/drm/armada/Kconfig"
 
-source "drivers/gpu/drm/atmel-hlcdc/Kconfig"
+source "drivers/gpu/drm/atmel/Kconfig"
 
 source "drivers/gpu/drm/rcar-du/Kconfig"
 

drivers/gpu/drm/Makefile +1 -1

index ef9f3dab287f..ce9829967128 100644
@@ -76,7 +76,7 @@ obj-$(CONFIG_DRM_GMA500) += gma500/
 obj-$(CONFIG_DRM_UDL) += udl/
 obj-$(CONFIG_DRM_AST) += ast/
 obj-$(CONFIG_DRM_ARMADA) += armada/
-obj-$(CONFIG_DRM_ATMEL_HLCDC)	+= atmel-hlcdc/
+obj-$(CONFIG_DRM_ATMEL)	+= atmel/
 obj-$(CONFIG_DRM_RCAR_DU) += rcar-du/
 obj-$(CONFIG_DRM_SHMOBILE) +=shmobile/
 obj-y			+= omapdrm/

drivers/gpu/drm/atmel/Kconfig renamed from drivers/gpu/drm/atmel-hlcdc/Kconfig +1 -0

similarity index 83%
rename from drivers/gpu/drm/atmel-hlcdc/Kconfig
rename to drivers/gpu/drm/atmel/Kconfig
index 32bcc4bad06a..7cd3862f9d18 100644
@@ -1,6 +1,12 @@
+config DRM_ATMEL

drivers/gpu/drm/atmel/Kconfig line 5

Quoted 4 lines

drivers/gpu/drm/atmel/Kconfig renamed from drivers/gpu/drm/atmel-hlcdc/Kconfig +4 -0

+	bool
+	help
+	  Enable Atmel DRM support
+

Not sure why you need an extra Kconfig option? And if you really do (because you want to share common code), it should be implicitly selected by DRM_ATMEL_HLCDC and DRM_ATMEL_LCDC.

The LCDC IP used by some Atmel SOC's have a multifunction device that include two sub-devices:
- pwm
- display controller

This binding describe the multi function device
that act as root for the sub-devices

The Atmel SOC's are at91sam9 etc.

The compatible name is intentionally
prefixed with -mfd to avoid clash with existing
compatible entries.

Signed-off-by Sam Ravnborg <[email protected]>

Cc Lee Jones <[email protected]>

Cc Boris Brezillon <[email protected]>

---
 .../devicetree/bindings/mfd/atmel-lcdc.txt         | 75 ++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/atmel-lcdc.txt

Documentation/devicetree/bindings/mfd/atmel-lcdc.txt new file +2 -0

new file mode 100644
index 000000000000..70e9b7bda6c7
@@ -0,0 +1,75 @@
+Device-Tree bindings for Atmel's LCDC (LCD Controller) MFD driver
+

Documentation/devicetree/bindings/mfd/atmel-lcdc.txt new file +35 -0

+ - reg: base address and size of the LCDC device registers.
+ - clock-names: the name of the 2 clocks requested by the LCDC device.
+   Should contain "lcdc_clk", and "hclk".
+ - clocks: should contain the 2 clocks requested by the LCDC device.
+	   May specify the same clock twice is there is no need to enable
+	   "hclk" to use the display.
+ - interrupts: should contain the description of the LCDC interrupt line
+
+The LCDC IP exposes two subdevices:
+ - a PWM chip: see ../pwm/atmel-lcdc-pwm.txt
+ - a Display Controller: see ../display/atmel/lcdc-display-controller.txt
+
+Example:
+	lcdc0: lcdc at 700000 {
+		compatible = "atmel,at91sam9263-lcdc-mfd";
+		reg = <0x700000 0x1000>;
+		interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>;
+		clocks = <&lcd_clk>, <&lcd_clk>;
+		clock-names = "lcdc_clk", "hclk";
+
+		lcdc-display-controller {
+			compatible = "atmel,lcdc-display-controller";
+			lcd-supply = <&lcdc_reg>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port at 0 {
+				reg = <0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				lcdc_panel_output: endpoint at 0 {
+					reg = <0>;
+					remote-endpoint = <&panel_input>;
+				};
+			};

Documentation/devicetree/bindings/mfd/atmel-lcdc.txt new file +40 -0

+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port at 0 {
+				reg = <0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				lcdc_panel_output: endpoint at 0 {
+					reg = <0>;
+					remote-endpoint = <&panel_input>;
+				};
+			};
+		};
+
+		lcdc_pwm: lcdc-pwm {
+			compatible = "atmel,lcdc-pwm";
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_lcdc_pwm>;
+			#pwm-cells = <3>;
+		};
+
+	};
 
+
+	panel: panel {
+		compatible = "logictechnologies,lttd800480070-l2rt", "simple-panel";
+		backlight = <&backlight>;
+		power-supply = <&panel_reg>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port at 0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			panel_input: endpoint at 0 {
+				reg = <0>;
+				remote-endpoint = <&lcdc_panel_output>;
+			};
+		};
+	};

Quoted 27 lines

Documentation/devicetree/bindings/mfd/atmel-lcdc.txt new file +27 -0

+The LCDC IP exposes two subdevices:
+ - a PWM chip: see ../pwm/atmel-lcdc-pwm.txt
+ - a Display Controller: see ../display/atmel/lcdc-display-controller.txt
+
+Example:
+	lcdc0: lcdc at 700000 {
+		compatible = "atmel,at91sam9263-lcdc-mfd";
+		reg = <0x700000 0x1000>;
+		interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>;
+		clocks = <&lcd_clk>, <&lcd_clk>;
+		clock-names = "lcdc_clk", "hclk";
+
+		lcdc-display-controller {
+			compatible = "atmel,lcdc-display-controller";
+			lcd-supply = <&lcdc_reg>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port at 0 {
+				reg = <0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				lcdc_panel_output: endpoint at 0 {
+					reg = <0>;
+					remote-endpoint = <&panel_input>;
+				};
+			};

Documentation/devicetree/bindings/mfd/atmel-lcdc.txt line 57

Quoted 10 lines

Documentation/devicetree/bindings/mfd/atmel-lcdc.txt new file +10 -0

+		};
+
+		lcdc_pwm: lcdc-pwm {
+			compatible = "atmel,lcdc-pwm";
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_lcdc_pwm>;
+			#pwm-cells = <3>;
+		};
+
+	};

Also, I don't remember why I decided to declare distinct nodes for the PWM and display controller, but you should probably try to only declare the lcdc node. Something like that:

	lcdc0: lcdc at 700000 {
		compatible = "atmel,at91sam9263-lcdc";
		reg = <0x700000 0x1000>;
		interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>;
		clocks = <&lcd_clk>, <&lcd_clk>;
		clock-names = "lcdc_clk", "hclk";
		pinctrl-names = "default";
		pinctrl-0 = <&pinctrl_lcdc_pwm ...>;
		#pwm-cells = <3>;
		#address-cells = <1>;
		#size-cells = <0>;

		port at 0 {
			reg = <0>;
			#address-cells = <1>;
			#size-cells = <0>;
			lcdc_panel_output: endpoint at 0 {
				reg = <0>;
				remote-endpoint = <&panel_input>;
			};
		};
	};

Note that this representation does not prevent us from having an MFD which declares the PWM and Display Engine, it's just that all devs will point to the same of_node.

Documentation/devicetree/bindings/mfd/atmel-lcdc.txt line 35

Quoted 5 lines

Documentation/devicetree/bindings/mfd/atmel-lcdc.txt new file +4 -0

+
+		lcdc-display-controller {
+			compatible = "atmel,lcdc-display-controller";
+			lcd-supply = <&lcdc_reg>;

Hm, is this a regulator for the LCD controller or the LCD? If it's for the LCD it should be placed under the panel node.

Documentation/devicetree/bindings/mfd/atmel-lcdc.txt line 60

Quoted 2 lines

Documentation/devicetree/bindings/mfd/atmel-lcdc.txt new file +2 -0

+	panel: panel {
+		compatible = "logictechnologies,lttd800480070-l2rt", "simple-panel";

The LCDC IP used by some Atmel SOC's have a
multifunction device that include two sub-devices:
- pwm
- display controller

This mfd device provide a regmap that can be used by the sub-devices to safely access the registers.
The mfd device also support the clock used by the LCDC IP + a bus clock that in some cases are required.

The driver is based on the atmel-hlcdc driver.

The Atmel SOC's are at91sam9261, at91sam9263 etc.

Signed-off-by Sam Ravnborg <[email protected]>

Cc Lee Jones <[email protected]>

Cc Boris Brezillon <[email protected]>

---
 drivers/mfd/Kconfig            |  10 +++
 drivers/mfd/Makefile           |   1 +
 drivers/mfd/atmel-lcdc.c       | 158 +++++++++++++++++++++++++++++++++++
 include/linux/mfd/atmel-lcdc.h | 184 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 353 insertions(+)
 create mode 100644 drivers/mfd/atmel-lcdc.c
 create mode 100644 include/linux/mfd/atmel-lcdc.h

drivers/mfd/Kconfig +10 -0

index b860eb5aa194..f4851f0f033f 100644
@@ -121,6 +121,16 @@ config MFD_ATMEL_HLCDC
 	  additional drivers must be enabled in order to use the
 	  functionality of the device.
 
+config MFD_ATMEL_LCDC
+	tristate "Atmel LCDC (LCD Controller)"
+	select MFD_CORE
+	depends on OF
+	help
+	  If you say yes here you get support for the LCDC block.
+	  This driver provides common support for accessing the device,
+	  additional drivers must be enabled in order to use the
+	  functionality of the device.
+
 config MFD_ATMEL_SMC
 	bool
 	select MFD_SYSCON

drivers/mfd/Makefile +1 -0

index e9fd20dba18d..dba8465e0d96 100644
@@ -186,6 +186,7 @@ obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
 obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
 obj-$(CONFIG_MFD_ATMEL_FLEXCOM)	+= atmel-flexcom.o
 obj-$(CONFIG_MFD_ATMEL_HLCDC)	+= atmel-hlcdc.o
+obj-$(CONFIG_MFD_ATMEL_LCDC)	+= atmel-lcdc.o
 obj-$(CONFIG_MFD_ATMEL_SMC)	+= atmel-smc.o
 obj-$(CONFIG_MFD_INTEL_LPSS)	+= intel-lpss.o
 obj-$(CONFIG_MFD_INTEL_LPSS_PCI)	+= intel-lpss-pci.o

drivers/mfd/atmel-lcdc.c new file +25 -0

new file mode 100644
index 000000000000..8928976bafca
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Sam Ravnborg
+ *
+ * Author: Sam Ravnborg <[email protected]>
+ *
+ * Based on atmel-hlcdc.c wich is:
+ * Copyright (C) 2014 Free Electrons
+ * Copyright (C) 2014 Atmel
+ * Author: Boris BREZILLON <[email protected]>
+ */
+
+#include <linux/mfd/atmel-lcdc.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+
+#define ATMEL_LCDC_REG_MAX		(0x1000 - 0x4)
+
+struct lcdc_regmap {
+	void __iomem *regs;
+};
+

Quoted 7 lines

drivers/mfd/atmel-lcdc.c new file +7 -0

+
+static const struct mfd_cell lcdc_cells[] = {
+	{
+		.name = "atmel-lcdc-pwm",
+		.of_compatible = "atmel,lcdc-pwm",
+	},
+	{

drivers/mfd/atmel-lcdc.c line 35

Quoted 4 lines

drivers/mfd/atmel-lcdc.c new file +4 -0

+		.name = "atmel-lcdc-dc",
+		.of_compatible = "atmel,lcdc-display-controller",
+	},
+};

Will you be adding any more devices, or is this the entirety of the device? If the latter, I suggest that this doesn't warrant being an MFD.

Hi Lee.

Quoted 7 lines

Quoted 7 lines

drivers/mfd/atmel-lcdc.c new file +7 -0

+
+static const struct mfd_cell lcdc_cells[] = {
+	{
+		.name = "atmel-lcdc-pwm",
+		.of_compatible = "atmel,lcdc-pwm",
+	},
+	{

Quoted 8 lines

drivers/mfd/atmel-lcdc.c line 35

Quoted 4 lines

drivers/mfd/atmel-lcdc.c new file +4 -0

+		.name = "atmel-lcdc-dc",
+		.of_compatible = "atmel,lcdc-display-controller",
+	},
+};

Will you be adding any more devices, or is this the entirety of the device? If the latter, I suggest that this doesn't warrant being an MFD.

Thats it. And others agree with you that this is not a good approach. So in v2 there will be no MFD.

Thanks for confirming that the non-mfd way is the better approach.

Quoted 17 lines · Sam Ravnborg

Hi Lee.

Quoted 15 lines

drivers/mfd/atmel-lcdc.c line 35

Quoted 11 lines

drivers/mfd/atmel-lcdc.c new file +11 -0

+
+static const struct mfd_cell lcdc_cells[] = {
+	{
+		.name = "atmel-lcdc-pwm",
+		.of_compatible = "atmel,lcdc-pwm",
+	},
+	{
+		.name = "atmel-lcdc-dc",
+		.of_compatible = "atmel,lcdc-display-controller",
+	},
+};

Will you be adding any more devices, or is this the entirety of the device? If the latter, I suggest that this doesn't warrant being an MFD.

Quoted 4 lines

Thats it. And others agree with you that this is not a good approach. So in v2 there will be no MFD.

Thanks for confirming that the non-mfd way is the better approach.

MFD approach would have had the benefit of keeping this driver series architecture close to the HLCD one. This would have been easier to understand and use one SoC or another one from the AT91 product line....

Anyway, I'd wait for Boris' feedback for making a decision.

Quoted 20 lines · Sam Ravnborg

Hi Lee.

Quoted 7 lines

Quoted 7 lines

drivers/mfd/atmel-lcdc.c new file +7 -0

+
+static const struct mfd_cell lcdc_cells[] = {
+	{
+		.name = "atmel-lcdc-pwm",
+		.of_compatible = "atmel,lcdc-pwm",
+	},
+	{

Quoted 8 lines

drivers/mfd/atmel-lcdc.c line 35

Quoted 4 lines

drivers/mfd/atmel-lcdc.c new file +4 -0

+		.name = "atmel-lcdc-dc",
+		.of_compatible = "atmel,lcdc-display-controller",
+	},
+};  

Will you be adding any more devices, or is this the entirety of the device? If the latter, I suggest that this doesn't warrant being an MFD.

Thats it. And others agree with you that this is not a good approach. So in v2 there will be no MFD.

Quoted 1 line

Thanks for confirming that the non-mfd way is the better approach.

MFD approach would have had the benefit of keeping this driver series architecture close to the HLCD one. This would have been easier to understand and use one SoC or another one from the AT91 product line....

Anyway, I'd wait for Boris' feedback for making a decision.

Quoted 6 lines

drivers/mfd/atmel-lcdc.c new file +6 -0

+static const struct mfd_cell lcdc_cells[] = {
+	{
+		.name = "atmel-lcdc-pwm",
+		.of_compatible = "atmel,lcdc-pwm",
+	},
+	{

drivers/mfd/atmel-lcdc.c line 35

Quoted 4 lines

drivers/mfd/atmel-lcdc.c new file +4 -0

+		.name = "atmel-lcdc-dc",
+		.of_compatible = "atmel,lcdc-display-controller",
+	},
+};  

Will you be adding any more devices, or is this the entirety of the device? If the latter, I suggest that this doesn't warrant being an MFD.

Hi Lee,

Quoted 6 lines · On Wed, 15 Aug 2018 06:24:35 +0100 Lee Jones <[email protected]> wrote:

Quoted 6 lines

drivers/mfd/atmel-lcdc.c new file +6 -0

+static const struct mfd_cell lcdc_cells[] = {
+	{
+		.name = "atmel-lcdc-pwm",
+		.of_compatible = "atmel,lcdc-pwm",
+	},
+	{

Quoted 9 lines

drivers/mfd/atmel-lcdc.c line 35

Quoted 4 lines

drivers/mfd/atmel-lcdc.c new file +4 -0

+		.name = "atmel-lcdc-dc",
+		.of_compatible = "atmel,lcdc-display-controller",
+	},
+};  

Will you be adding any more devices, or is this the entirety of the device? If the latter, I suggest that this doesn't warrant being an MFD.

Is there a lower limit to define when an MFD is recommended, or is it that you find a PWM (driving a backlight) and a display controller close enough to be implemented in a single driver?

I personally prefer the separation we have today, because I can then place the drivers where they belong (PWM subsystem and DRM subsystem) and the respective maintainers know about these drivers.

The LCDC IP used by some Atmel SOC's have a
multifunction device that include two sub-devices:
- pwm
- display controller

This mfd device provide a regmap that can be used by the sub-devices to safely access the registers.
The mfd device also support the clock used by the LCDC IP + a bus clock that in some cases are required.

The driver is based on the atmel-hlcdc driver.

The Atmel SOC's are at91sam9261, at91sam9263 etc.

Signed-off-by Sam Ravnborg <[email protected]>

Cc Lee Jones <[email protected]>

Cc Boris Brezillon <[email protected]>

---
 drivers/mfd/Kconfig            |  10 +++
 drivers/mfd/Makefile           |   1 +
 drivers/mfd/atmel-lcdc.c       | 158 +++++++++++++++++++++++++++++++++++
 include/linux/mfd/atmel-lcdc.h | 184 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 353 insertions(+)
 create mode 100644 drivers/mfd/atmel-lcdc.c
 create mode 100644 include/linux/mfd/atmel-lcdc.h

drivers/mfd/Kconfig +10 -0

index b860eb5aa194..f4851f0f033f 100644
@@ -121,6 +121,16 @@ config MFD_ATMEL_HLCDC
 	  additional drivers must be enabled in order to use the
 	  functionality of the device.
 
+config MFD_ATMEL_LCDC
+	tristate "Atmel LCDC (LCD Controller)"
+	select MFD_CORE
+	depends on OF
+	help
+	  If you say yes here you get support for the LCDC block.
+	  This driver provides common support for accessing the device,
+	  additional drivers must be enabled in order to use the
+	  functionality of the device.
+
 config MFD_ATMEL_SMC
 	bool
 	select MFD_SYSCON

drivers/mfd/Makefile +1 -0

index e9fd20dba18d..dba8465e0d96 100644
@@ -186,6 +186,7 @@ obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
 obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
 obj-$(CONFIG_MFD_ATMEL_FLEXCOM)	+= atmel-flexcom.o
 obj-$(CONFIG_MFD_ATMEL_HLCDC)	+= atmel-hlcdc.o
+obj-$(CONFIG_MFD_ATMEL_LCDC)	+= atmel-lcdc.o
 obj-$(CONFIG_MFD_ATMEL_SMC)	+= atmel-smc.o
 obj-$(CONFIG_MFD_INTEL_LPSS)	+= intel-lpss.o
 obj-$(CONFIG_MFD_INTEL_LPSS_PCI)	+= intel-lpss-pci.o

drivers/mfd/atmel-lcdc.c new file +158 -0

new file mode 100644
index 000000000000..8928976bafca
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Sam Ravnborg
+ *
+ * Author: Sam Ravnborg <[email protected]>
+ *
+ * Based on atmel-hlcdc.c wich is:
+ * Copyright (C) 2014 Free Electrons
+ * Copyright (C) 2014 Atmel
+ * Author: Boris BREZILLON <[email protected]>
+ */
+
+#include <linux/mfd/atmel-lcdc.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+
+#define ATMEL_LCDC_REG_MAX		(0x1000 - 0x4)
+
+struct lcdc_regmap {
+	void __iomem *regs;
+};
+
+static const struct mfd_cell lcdc_cells[] = {
+	{
+		.name = "atmel-lcdc-pwm",
+		.of_compatible = "atmel,lcdc-pwm",
+	},
+	{
+		.name = "atmel-lcdc-dc",
+		.of_compatible = "atmel,lcdc-display-controller",
+	},
+};
+
+static int regmap_lcdc_reg_write(void *context, unsigned int reg,
+				 unsigned int val)
+{
+	struct lcdc_regmap *regmap = context;
+
+	writel(val, regmap->regs + reg);
+
+	return 0;
+}
+
+static int regmap_lcdc_reg_read(void *context, unsigned int reg,
+				unsigned int *val)
+{
+	struct lcdc_regmap *regmap = context;
+
+	*val = readl(regmap->regs + reg);
+
+	return 0;
+}
+
+static const struct regmap_config lcdc_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = ATMEL_LCDC_REG_MAX,
+	.reg_write = regmap_lcdc_reg_write,
+	.reg_read = regmap_lcdc_reg_read,
+	.fast_io = true,
+};
+
+static int lcdc_probe(struct platform_device *pdev)
+{
+	struct atmel_mfd_lcdc *lcdc;
+	struct lcdc_regmap *regmap;
+	struct resource *res;
+	struct device *dev;
+	int ret;
+
+	dev = &pdev->dev;
+
+	regmap = devm_kzalloc(dev, sizeof(*regmap), GFP_KERNEL);
+	if (!regmap)
+		return -ENOMEM;
+
+	lcdc = devm_kzalloc(dev, sizeof(*lcdc), GFP_KERNEL);
+	if (!lcdc)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	regmap->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(regmap->regs)) {
+		dev_err(dev, "Failed to allocate IO mem (%ld)\n",
+			PTR_ERR(regmap->regs));
+		return PTR_ERR(regmap->regs);
+	}
+
+	lcdc->irq = platform_get_irq(pdev, 0);
+	if (lcdc->irq < 0) {
+		dev_err(dev, "Failed to get irq (%d)\n", lcdc->irq);
+		return lcdc->irq;
+	}
+
+	lcdc->lcdc_clk = devm_clk_get(dev, "lcdc_clk");
+	if (IS_ERR(lcdc->lcdc_clk)) {
+		dev_err(dev, "failed to get lcdc clock (%ld)\n",
+			PTR_ERR(lcdc->lcdc_clk));
+		return PTR_ERR(lcdc->lcdc_clk);
+	}
+
+	lcdc->bus_clk = devm_clk_get(dev, "hclk");
+	if (IS_ERR(lcdc->bus_clk)) {
+		dev_err(dev, "failed to get bus clock (%ld)\n",
+			PTR_ERR(lcdc->bus_clk));
+		return PTR_ERR(lcdc->bus_clk);
+	}
+
+	lcdc->regmap = devm_regmap_init(dev, NULL, regmap,
+					 &lcdc_regmap_config);
+	if (IS_ERR(lcdc->regmap)) {
+		dev_err(dev, "Failed to init regmap (%ld)\n",
+			PTR_ERR(lcdc->regmap));
+		return PTR_ERR(lcdc->regmap);
+	}
+
+	dev_set_drvdata(dev, lcdc);
+
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+				   lcdc_cells, ARRAY_SIZE(lcdc_cells),
+				   NULL, 0, NULL);
+	if (ret < 0)
+		dev_err(dev, "Failed to add %d mfd devices (%d)\n",
+			ARRAY_SIZE(lcdc_cells), ret);
+
+	return ret;
+}
+
+static const struct of_device_id lcdc_match[] = {
+	{ .compatible = "atmel,at91sam9261-lcdc-mfd" },
+	{ .compatible = "atmel,at91sam9263-lcdc-mfd" },
+	{ .compatible = "atmel,at91sam9g10-lcdc-mfd" },
+	{ .compatible = "atmel,at91sam9g45-lcdc-mfd" },
+	{ .compatible = "atmel,at91sam9g46-lcdc-mfd" },
+	{ .compatible = "atmel,at91sam9m10-lcdc-mfd" },
+	{ .compatible = "atmel,at91sam9m11-lcdc-mfd" },
+	{ .compatible = "atmel,at91sam9rl-lcdc-mfd" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, lcdc_match);
+
+static struct platform_driver lcdc_driver = {
+	.probe = lcdc_probe,
+	.driver = {
+		.name = "atmel-lcdc",
+		.of_match_table = lcdc_match,
+	},
+};
+module_platform_driver(lcdc_driver);
+
+MODULE_ALIAS("platform:atmel-lcdc");
+MODULE_AUTHOR("Sam Ravnborg <[email protected]>");
+MODULE_DESCRIPTION("Atmel LCDC mfd driver");
+MODULE_LICENSE("GPL v2");

include/linux/mfd/atmel-lcdc.h new file +184 -0

new file mode 100644
index 000000000000..fdab269baa8e
@@ -0,0 +1,184 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Sam Ravnborg
+ *
+ * Author: Sam Ravnborg <[email protected]>
+ */
+
+#ifndef __LINUX_MFD_LCDC_H
+#define __LINUX_MFD_LCDC_H
+
+#include <linux/regmap.h>
+#include <linux/clk.h>
+
+/**
+ * Structure shared by the Atmel LCD Controller device and its sub-devices.
+ *
+ * @regmap: register map used to access LCDC IP registers
+ * @lcdc_clk: the lcd controller peripheral clock
+ * @bus_clk: the bus clock clock (often the same as lcdc_clk)
+ * @irq: the lcdc irq
+ */
+struct atmel_mfd_lcdc {
+	struct regmap *regmap;
+	struct clk *lcdc_clk;
+	struct clk *bus_clk;
+	int irq;
+};
+
+#define ATMEL_LCDC_DMABADDR1	0x00
+#define ATMEL_LCDC_DMABADDR2	0x04
+#define ATMEL_LCDC_DMAFRMPT1	0x08
+#define ATMEL_LCDC_DMAFRMPT2	0x0c
+#define ATMEL_LCDC_DMAFRMADD1	0x10
+#define ATMEL_LCDC_DMAFRMADD2	0x14
+
+#define ATMEL_LCDC_DMAFRMCFG	0x18
+#define	ATMEL_LCDC_FRSIZE	(0x7fffff <<  0)
+#define	ATMEL_LCDC_BLENGTH_OFFSET	24
+#define	ATMEL_LCDC_BLENGTH	(0x7f     << ATMEL_LCDC_BLENGTH_OFFSET)
+
+#define ATMEL_LCDC_DMACON	0x1c
+#define	ATMEL_LCDC_DMAEN	(0x1 << 0)
+#define	ATMEL_LCDC_DMARST	(0x1 << 1)
+#define	ATMEL_LCDC_DMABUSY	(0x1 << 2)
+#define		ATMEL_LCDC_DMAUPDT	(0x1 << 3)
+#define		ATMEL_LCDC_DMA2DEN	(0x1 << 4)
+
+#define ATMEL_LCDC_DMA2DCFG	0x20
+#define		ATMEL_LCDC_ADDRINC_OFFSET	0
+#define		ATMEL_LCDC_ADDRINC		(0xffff)
+#define		ATMEL_LCDC_PIXELOFF_OFFSET	24
+#define		ATMEL_LCDC_PIXELOFF		(0x1f << 24)
+
+#define ATMEL_LCDC_LCDCON1	0x0800
+#define	ATMEL_LCDC_BYPASS	(1     <<  0)
+#define	ATMEL_LCDC_CLKVAL_OFFSET	12
+#define	ATMEL_LCDC_CLKVAL	(0x1ff << ATMEL_LCDC_CLKVAL_OFFSET)
+#define	ATMEL_LCDC_LINCNT	(0x7ff << 21)
+
+#define ATMEL_LCDC_LCDCON2	0x0804
+#define	ATMEL_LCDC_DISTYPE	(3 << 0)
+#define		ATMEL_LCDC_DISTYPE_STNMONO	(0 << 0)
+#define		ATMEL_LCDC_DISTYPE_STNCOLOR	(1 << 0)
+#define		ATMEL_LCDC_DISTYPE_TFT		(2 << 0)
+#define	ATMEL_LCDC_SCANMOD	(1 << 2)
+#define		ATMEL_LCDC_SCANMOD_SINGLE	(0 << 2)
+#define		ATMEL_LCDC_SCANMOD_DUAL		(1 << 2)
+#define	ATMEL_LCDC_IFWIDTH	(3 << 3)
+#define		ATMEL_LCDC_IFWIDTH_4		(0 << 3)
+#define		ATMEL_LCDC_IFWIDTH_8		(1 << 3)
+#define		ATMEL_LCDC_IFWIDTH_16		(2 << 3)
+#define	ATMEL_LCDC_PIXELSIZE	(7 << 5)
+#define		ATMEL_LCDC_PIXELSIZE_1		(0 << 5)
+#define		ATMEL_LCDC_PIXELSIZE_2		(1 << 5)
+#define		ATMEL_LCDC_PIXELSIZE_4		(2 << 5)
+#define		ATMEL_LCDC_PIXELSIZE_8		(3 << 5)
+#define		ATMEL_LCDC_PIXELSIZE_16		(4 << 5)
+#define		ATMEL_LCDC_PIXELSIZE_24		(5 << 5)
+#define		ATMEL_LCDC_PIXELSIZE_32		(6 << 5)
+#define	ATMEL_LCDC_INVVD	(1 << 8)
+#define		ATMEL_LCDC_INVVD_NORMAL		(0 << 8)
+#define		ATMEL_LCDC_INVVD_INVERTED	(1 << 8)
+#define	ATMEL_LCDC_INVFRAME	(1 << 9)
+#define		ATMEL_LCDC_INVFRAME_NORMAL	(0 << 9)
+#define		ATMEL_LCDC_INVFRAME_INVERTED	(1 << 9)
+#define	ATMEL_LCDC_INVLINE	(1 << 10)
+#define		ATMEL_LCDC_INVLINE_NORMAL	(0 << 10)
+#define		ATMEL_LCDC_INVLINE_INVERTED	(1 << 10)
+#define	ATMEL_LCDC_INVCLK	(1 << 11)
+#define		ATMEL_LCDC_INVCLK_NORMAL	(0 << 11)
+#define		ATMEL_LCDC_INVCLK_INVERTED	(1 << 11)
+#define	ATMEL_LCDC_INVDVAL	(1 << 12)
+#define		ATMEL_LCDC_INVDVAL_NORMAL	(0 << 12)
+#define		ATMEL_LCDC_INVDVAL_INVERTED	(1 << 12)
+#define	ATMEL_LCDC_CLKMOD	(1 << 15)
+#define		ATMEL_LCDC_CLKMOD_ACTIVEDISPLAY	(0 << 15)
+#define		ATMEL_LCDC_CLKMOD_ALWAYSACTIVE	(1 << 15)
+#define	ATMEL_LCDC_MEMOR	(1 << 31)
+#define		ATMEL_LCDC_MEMOR_BIG		(0 << 31)
+#define		ATMEL_LCDC_MEMOR_LITTLE		(1 << 31)
+
+#define ATMEL_LCDC_TIM1		0x0808
+#define	ATMEL_LCDC_VFP_OFFSET		0
+#define	ATMEL_LCDC_VFP		(0xffU <<  0)
+#define	ATMEL_LCDC_VBP_OFFSET		8
+#define	ATMEL_LCDC_VBP		(0xffU <<  ATMEL_LCDC_VBP_OFFSET)
+#define	ATMEL_LCDC_VPW_OFFSET		16
+#define	ATMEL_LCDC_VPW		(0x3fU << ATMEL_LCDC_VPW_OFFSET)
+#define	ATMEL_LCDC_VHDLY_OFFSET		24
+#define	ATMEL_LCDC_VHDLY	(0xfU  << ATMEL_LCDC_VHDLY_OFFSET)
+
+#define ATMEL_LCDC_TIM2		0x080c
+#define	ATMEL_LCDC_HBP_OFFSET		0
+#define	ATMEL_LCDC_HBP		(0xffU  <<  0)
+#define	ATMEL_LCDC_HPW_OFFSET		8
+#define	ATMEL_LCDC_HPW		(0x3fU  <<  ATMEL_LCDC_HPW_OFFSET)
+#define	ATMEL_LCDC_HFP_OFFSET		21
+#define	ATMEL_LCDC_HFP		(0x7ffU << ATMEL_LCDC_HFP_OFFSET)
+
+#define ATMEL_LCDC_LCDFRMCFG	0x0810
+#define	ATMEL_LCDC_LINEVAL	(0x7ff <<  0)
+#define	ATMEL_LCDC_HOZVAL_OFFSET	21
+#define	ATMEL_LCDC_HOZVAL	(0x7ff << ATMEL_LCDC_HOZVAL_OFFSET)
+
+#define ATMEL_LCDC_FIFO		0x0814
+#define	ATMEL_LCDC_FIFOTH	(0xffff)
+
+#define ATMEL_LCDC_MVAL		0x0818
+
+#define ATMEL_LCDC_DP1_2	0x081c
+#define ATMEL_LCDC_DP4_7	0x0820
+#define ATMEL_LCDC_DP3_5	0x0824
+#define ATMEL_LCDC_DP2_3	0x0828
+#define ATMEL_LCDC_DP5_7	0x082c
+#define ATMEL_LCDC_DP3_4	0x0830
+#define ATMEL_LCDC_DP4_5	0x0834
+#define ATMEL_LCDC_DP6_7	0x0838
+#define	ATMEL_LCDC_DP1_2_VAL	(0xff)
+#define	ATMEL_LCDC_DP4_7_VAL	(0xfffffff)
+#define	ATMEL_LCDC_DP3_5_VAL	(0xfffff)
+#define	ATMEL_LCDC_DP2_3_VAL	(0xfff)
+#define	ATMEL_LCDC_DP5_7_VAL	(0xfffffff)
+#define	ATMEL_LCDC_DP3_4_VAL	(0xffff)
+#define	ATMEL_LCDC_DP4_5_VAL	(0xfffff)
+#define	ATMEL_LCDC_DP6_7_VAL	(0xfffffff)
+
+#define ATMEL_LCDC_PWRCON	0x083c
+#define	ATMEL_LCDC_PWR		(1    <<  0)
+#define	ATMEL_LCDC_GUARDT_OFFSET	1
+#define	ATMEL_LCDC_GUARDT	(0x7f <<  ATMEL_LCDC_GUARDT_OFFSET)
+#define	ATMEL_LCDC_BUSY		(1    << 31)
+
+#define ATMEL_LCDC_CONTRAST_CTR	0x0840
+#define	ATMEL_LCDC_PS		(3 << 0)
+#define		ATMEL_LCDC_PS_DIV1		(0 << 0)
+#define		ATMEL_LCDC_PS_DIV2		(1 << 0)
+#define		ATMEL_LCDC_PS_DIV4		(2 << 0)
+#define		ATMEL_LCDC_PS_DIV8		(3 << 0)
+#define	ATMEL_LCDC_POL		(1 << 2)
+#define		ATMEL_LCDC_POL_NEGATIVE		(0 << 2)
+#define		ATMEL_LCDC_POL_POSITIVE		(1 << 2)
+#define	ATMEL_LCDC_ENA		(1 << 3)
+#define		ATMEL_LCDC_ENA_PWMDISABLE	(0 << 3)
+#define		ATMEL_LCDC_ENA_PWMENABLE	(1 << 3)
+
+#define ATMEL_LCDC_CONTRAST_VAL	0x0844
+#define	ATMEL_LCDC_CVAL	(0xff)
+
+#define ATMEL_LCDC_IER		0x0848
+#define ATMEL_LCDC_IDR		0x084c
+#define ATMEL_LCDC_IMR		0x0850
+#define ATMEL_LCDC_ISR		0x0854
+#define ATMEL_LCDC_ICR		0x0858
+#define	ATMEL_LCDC_LNI		(1 << 0)
+#define	ATMEL_LCDC_LSTLNI	(1 << 1)
+#define	ATMEL_LCDC_EOFI		(1 << 2)
+#define	ATMEL_LCDC_UFLWI	(1 << 4)
+#define	ATMEL_LCDC_OWRI		(1 << 5)
+#define	ATMEL_LCDC_MERI		(1 << 6)
+
+#define ATMEL_LCDC_LUT(n)	(0x0c00 + ((n)*4))
+#define ATMEL_LCDC_LUT_SIZE	256
+
+#endif /* __LINUX_MFD_LCDC_H */

This is a DRM based driver for the Atmel LCDC IP. There exist today a framebuffer based driver and this is a re-implmentation of the same on top of DRM.

The rewrite was based on the original fbdev driver but the driver has also seen inspiration from the atmel-hlcdc_dc driver and others.

The driver is not a full replacement:
- STN displays are not supported
Binding support is missing but most of the STN specific functionality is otherwise ported from the fbdev driver.
- gamma support is missing
The driver utilises drm_simple_kms_helper and this helper lacks support for settting up gamma
- modesetting is not checked (see TODO in file)
- support for extra modes as applicable (and lcd-wiring-mode)
- support for AVR32 (is it relevant?)

Signed-off-by Sam Ravnborg <[email protected]>

Cc Nicolas Ferre <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Alexandre Belloni <[email protected]>

---
 MAINTAINERS                           |    7 +
 drivers/gpu/drm/atmel/Kconfig         |   12 +
 drivers/gpu/drm/atmel/Makefile        |    2 +
 drivers/gpu/drm/atmel/atmel_lcdc-dc.c | 1094 +++++++++++++++++++++++++++++++++
 4 files changed, 1115 insertions(+)
 create mode 100644 drivers/gpu/drm/atmel/atmel_lcdc-dc.c

MAINTAINERS +0 -0

index 09ce76a9a1dc..0a594d02a7c0 100644

MAINTAINERS +0 -0

 DRM DRIVERS FOR BRIDGE CHIPS
 M:	Archit Taneja <[email protected]>
 M:	Andrzej Hajda <[email protected]>

MAINTAINERS +4 -0

+/*
+ * The Atmel LCD controller display-controller supports several formats but
+ * this driver supports only a small subset.
+ * TODO: atmel_lcdfb supports more - port it over

MAINTAINERS +10 -0

+	regmap_write(lcdc_dc->regmap, ATMEL_LCDC_DMACON, ATMEL_LCDC_DMAEN);
+	// Enable LCD
+	regmap_write(lcdc_dc->regmap, ATMEL_LCDC_PWRCON,
+		     (lcdc_dc->desc->guard_time << ATMEL_LCDC_GUARDT_OFFSET)
+		     | ATMEL_LCDC_PWR);
+}
 
+
+static int lcdc_dc_display_check(struct drm_simple_display_pipe *pipe,
+				 struct drm_plane_state *pstate,
+				 struct drm_crtc_state *cstate)

MAINTAINERS +4 -0

+	/* TODO */
+
+	return 0;
+}

MAINTAINERS +21 -0

+	lcdc_dc_stop(lcdc_dc);
+	lcdc_dc_start(lcdc_dc);
+}
+
+static irqreturn_t lcdc_dc_irq_handler(int irq, void *arg)
+{
+	struct lcdc_dc *lcdc_dc;
+	struct drm_device *drm;
+	unsigned int status;
+	struct device *dev;
+	unsigned int imr;
+	unsigned int isr;
+
+	drm = arg;
+	lcdc_dc = drm->dev_private;
+	dev = lcdc_dc->dev;
+
+	regmap_read(lcdc_dc->regmap, ATMEL_LCDC_IMR, &imr);
+	regmap_read(lcdc_dc->regmap, ATMEL_LCDC_ISR, &isr);
+	status = imr & isr;
+	if (!status)

MAINTAINERS +12 -0

+	if (status & ATMEL_LCDC_OWRI)
+		DRM_DEV_INFO(dev, "FIFO overwrite interrupt");
+
+	if (status & ATMEL_LCDC_MERI)
+		DRM_DEV_INFO(dev, "DMA memory error");
+
+	/* Clear all reported (from ISR) interrupts */
+	regmap_write(lcdc_dc->regmap, ATMEL_LCDC_ICR, isr);
+
+	return IRQ_HANDLED;
+}
+

MAINTAINERS +13 -0

+static int lcdc_dc_modeset_init(struct lcdc_dc *lcdc_dc, struct drm_device *drm)
+{
+	struct drm_bridge *bridge;
+	struct drm_panel *panel;
+	struct device_node *np;
+	struct device *dev;
+	int ret;
+
+	dev = drm->dev;
+
+	drm_mode_config_init(drm);
+	drm->mode_config.min_width  = lcdc_dc->desc->min_width;
+	drm->mode_config.min_height = lcdc_dc->desc->min_height;

MAINTAINERS +5 -0

+	}
+
+	lcdc_dc->panel = panel;
+	lcdc_dc->bridge = bridge;
+

MAINTAINERS +42 -0

+}
+
+static int lcdc_dc_load(struct drm_device *drm)
+{
+	const struct of_device_id *match;
+	struct platform_device *pdev;
+	struct lcdc_dc *lcdc_dc;
+	struct device *dev;
+	int ret;
+
+	dev = drm->dev;
+	pdev = to_platform_device(dev);
+
+	match = of_match_node(atmel_lcdc_of_match, dev->parent->of_node);
+	if (!match) {
+		DRM_DEV_ERROR(dev, "invalid compatible string (node=%s)",
+			      dev->parent->of_node->name);
+		return -ENODEV;
+	}
+
+	if (!match->data) {
+		DRM_DEV_ERROR(dev, "invalid lcdc_dc description\n");
+		return -EINVAL;
+	}
+
+	lcdc_dc = devm_kzalloc(dev, sizeof(*lcdc_dc), GFP_KERNEL);
+	if (!lcdc_dc) {
+		DRM_DEV_ERROR(dev, "Failed to allocate lcdc_dc\n");
+		return -ENOMEM;
+	}
+
+	/* reset of lcdc might sleep and require a preemptible task context */
+	INIT_WORK(&lcdc_dc->reset_lcdc_work, reset_lcdc_work);
+
+	platform_set_drvdata(pdev, drm);
+	dev_set_drvdata(dev, lcdc_dc);
+
+	lcdc_dc->mfd_lcdc = dev_get_drvdata(dev->parent);
+	drm->dev_private = lcdc_dc;
+
+	lcdc_dc->regmap = lcdc_dc->mfd_lcdc->regmap;
+	lcdc_dc->desc = match->data;

MAINTAINERS +7 -0

+
+	pm_runtime_enable(dev);
+
+	ret = drm_vblank_init(drm, 1);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "failed to initialize vblank (%d)\n",
+			      ret);

MAINTAINERS +27 -0

+	ret = drm_irq_install(drm, lcdc_dc->mfd_lcdc->irq);
+	pm_runtime_put_sync(dev);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev, "Failed to install IRQ (%d)\n", ret);
+
+		goto err_pm_runtime_disable;
+	}
+
+	/*
+	 * Passing in 16 here will make the RGB656 mode the default
+	 * Passing in 32 will use XRGB8888 mode
+	 */
+	drm_fb_cma_fbdev_init(drm, 16, 0);
+
+	drm_kms_helper_poll_init(drm);
+
+	lcdc_dc->fbdev = drm_fbdev_cma_init(drm, 8, 1);
+	if (IS_ERR(lcdc_dc->fbdev)) {
+		ret = PTR_ERR(lcdc_dc->fbdev);
+		DRM_DEV_ERROR(dev, "Failed to init FB CMA area (%d)", ret);
+		goto err_irq_uninstall;
+	}
+
+	drm_helper_hpd_irq_event(drm);
+
+	return 0;
+

MAINTAINERS +26 -0

+}
+
+
+static int lcdc_dc_probe(struct platform_device *pdev)
+{
+	struct drm_device *drm;
+	struct device *dev;
+	int ret;
+
+	dev = &pdev->dev;
+
+	drm = drm_dev_alloc(&lcdc_dc_drm_driver, dev);
+	if (IS_ERR(drm)) {
+		DRM_DEV_ERROR(dev, "Failed to allocate drm device\n");
+		return PTR_ERR(drm);
+	}
+
+	ret = lcdc_dc_load(drm);
+	if (ret)
+		goto err_put_ref;
+
+	ret = drm_dev_register(drm, 0);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "Failed to register drm (%d)\n", ret);
+		goto err_unload;
+	}

Quoted 46 lines

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +46 -0

+static int lcdc_dc_load(struct drm_device *drm)
+{
+	const struct of_device_id *match;
+	struct platform_device *pdev;
+	struct lcdc_dc *lcdc_dc;
+	struct device *dev;
+	int ret;
+
+	dev = drm->dev;
+	pdev = to_platform_device(dev);
+
+	match = of_match_node(atmel_lcdc_of_match, dev->parent->of_node);
+	if (!match) {
+		DRM_DEV_ERROR(dev, "invalid compatible string (node=%s)",
+			      dev->parent->of_node->name);
+		return -ENODEV;
+	}
+
+	if (!match->data) {
+		DRM_DEV_ERROR(dev, "invalid lcdc_dc description\n");
+		return -EINVAL;
+	}
+
+	lcdc_dc = devm_kzalloc(dev, sizeof(*lcdc_dc), GFP_KERNEL);
+	if (!lcdc_dc) {
+		DRM_DEV_ERROR(dev, "Failed to allocate lcdc_dc\n");
+		return -ENOMEM;
+	}
+
+	/* reset of lcdc might sleep and require a preemptible task context */
+	INIT_WORK(&lcdc_dc->reset_lcdc_work, reset_lcdc_work);
+
+	platform_set_drvdata(pdev, drm);
+	dev_set_drvdata(dev, lcdc_dc);
+
+	lcdc_dc->mfd_lcdc = dev_get_drvdata(dev->parent);
+	drm->dev_private = lcdc_dc;
+
+	lcdc_dc->regmap = lcdc_dc->mfd_lcdc->regmap;
+	lcdc_dc->desc = match->data;
+	lcdc_dc->dev = dev;
+
+	lcdc_dc->lcd_supply = devm_regulator_get(dev, "lcd");
+	if (IS_ERR(lcdc_dc->lcd_supply)) {
+		DRM_DEV_ERROR(dev, "Failed to get lcd-supply (%ld)\n",
+			      PTR_ERR(lcdc_dc->lcd_supply));

drivers/gpu/drm/atmel/atmel_lcdc-dc.c line 945

Quoted 4 lines

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +4 -0

+		lcdc_dc->lcd_supply = NULL;
+	}
+
+	lcdc_dc_start_clock(lcdc_dc);

Hm, do you really need to call that here? I'd make it part of the runtime PM resume hook, and put a lcdc_dc_stop_clock() in the suspend hook.

Quoted 7 lines

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +7 -0

+
+	pm_runtime_enable(dev);
+
+	ret = drm_vblank_init(drm, 1);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "failed to initialize vblank (%d)\n",
+			      ret);

drivers/gpu/drm/atmel/atmel_lcdc-dc.c line 962

Quoted 10 lines

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +10 -0

+		goto err_pm_runtime_disable;
+	}
+
+	ret = lcdc_dc_modeset_init(lcdc_dc, drm);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "modeset_init failed (%d)", ret);
+		goto err_pm_runtime_disable;
+	}
+
+	pm_runtime_get_sync(dev);

This call will automatically call the runtime PM resume hook, so if you need the clk to be enabled before that point you should put it earlier.

Also, you should call pm_runtime_get_sync() in the ->enable() path and pm_runtime_put() in the ->disable() path.

This is a DRM based driver for the Atmel LCDC IP. There exist today a framebuffer based driver and this is a re-implmentation of the same on top of DRM.

The rewrite was based on the original fbdev driver but the driver has also seen inspiration from the atmel-hlcdc_dc driver and others.

The driver is not a full replacement:
- STN displays are not supported
Binding support is missing but most of the STN specific functionality is otherwise ported from the fbdev driver.
- gamma support is missing
The driver utilises drm_simple_kms_helper and this helper lacks support for settting up gamma
- modesetting is not checked (see TODO in file)
- support for extra modes as applicable (and lcd-wiring-mode)
- support for AVR32 (is it relevant?)

Signed-off-by Sam Ravnborg <[email protected]>

Cc Nicolas Ferre <[email protected]>

Cc Boris Brezillon <[email protected]>

Cc Alexandre Belloni <[email protected]>

---
  MAINTAINERS                           |    7 +
  drivers/gpu/drm/atmel/Kconfig         |   12 +
  drivers/gpu/drm/atmel/Makefile        |    2 +
  drivers/gpu/drm/atmel/atmel_lcdc-dc.c | 1094 +++++++++++++++++++++++++++++++++
  4 files changed, 1115 insertions(+)

create mode 100644 drivers/gpu/drm/atmel/atmel_lcdc-dc.c

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +61 -0

new file mode 100644
index 000000000000..275a09e2e100
@@ -0,0 +1,1094 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Sam Ravnborg
+ *
+ * The driver is based on atmel_lcdfb which is:
+ * Copyright (C) 2007 Atmel Corporation
+ *
+ * Atmel LCD Controller Display Controller.
+ * A sub-device of the Atmel LCDC IP.
+ *
+ * The Atmel LCD Controller supports in the following configuration:
+ * - TFT only, with BGR565, 8 bits/pixel
+ * - Resolution up to 2048x2048
+ * - Single plane, crtc, one fixed output
+ *
+ * Features not (yet) ported from atmel_lcdfb:
+ * - Support for extra modes (and configurable intensify bit)
+ * - Check modesetting support - lcdc_dc_display_check()
+ * - set color / palette handling
+ * - support for STN displays (partly implemented)
+ * - AVR32 support (relevant?)
+ */
+
+#include <linux/regulator/consumer.h>
+#include <linux/mfd/atmel-lcdc.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+#include <linux/irqreturn.h>
+#include <linux/device.h>
+#include <linux/regmap.h>
+
+#include <video/videomode.h>
+
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_vblank.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_of.h>
+
+/* Parameters */
+#define ATMEL_LCDC_DMA_BURST_LEN	8	/* words */
+
+/**
+ * struct atmel_lcdc_dc_desc - CPU specific configuration properties
+ */
+struct atmel_lcdc_dc_desc {
+	int guard_time;
+	int fifo_size;
+	int min_width;
+	int min_height;
+	int max_width;
+	int max_height;

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +6 -0

+	struct drm_bridge *bridge;
+	struct drm_panel *panel;
+	struct regmap *regmap;
+	struct device *dev;
+
+	struct drm_simple_display_pipe pipe;

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +82 -0

+	.max_width = 2048,
+	.max_height = 2048,
+	.have_hozval = true,
+	.have_alt_pixclock = false,
+};
+
+static const struct atmel_lcdc_dc_desc atmel_lcdc_dc_at91sam9263 = {
+	.guard_time = 1,
+	.fifo_size = 2048,
+	.min_width = 0,
+	.min_height = 0,
+	.max_width = 2048,
+	.max_height = 2048,
+	.have_hozval = false,
+	.have_alt_pixclock = false,
+};
+
+static const struct atmel_lcdc_dc_desc atmel_lcdc_dc_at91sam9g10 = {
+	.guard_time = 1,
+	.fifo_size = 512,
+	.min_width = 0,
+	.min_height = 0,
+	.max_width = 2048,
+	.max_height = 2048,
+	.have_hozval = true,
+	.have_alt_pixclock = false,
+};
+
+static const struct atmel_lcdc_dc_desc atmel_lcdc_dc_at91sam9g45 = {
+	.guard_time = 1,
+	.fifo_size = 512,
+	.min_width = 0,
+	.min_height = 0,
+	.max_width = 2048,
+	.max_height = 2048,
+	.have_hozval = false,
+	.have_alt_pixclock = true,
+};
+
+static const struct atmel_lcdc_dc_desc atmel_lcdc_dc_at91sam9g46 = {
+	.guard_time = 1,
+	.fifo_size = 512,
+	.min_width = 0,
+	.min_height = 0,
+	.max_width = 2048,
+	.max_height = 2048,
+	.have_hozval = false,
+	.have_alt_pixclock = false,
+};
+
+static const struct atmel_lcdc_dc_desc atmel_lcdc_dc_at91sam9m10 = {
+	.guard_time = 1,
+	.fifo_size = 512,
+	.min_width = 0,
+	.min_height = 0,
+	.max_width = 2048,
+	.max_height = 2048,
+	.have_hozval = false,
+	.have_alt_pixclock = false,
+};
+
+static const struct atmel_lcdc_dc_desc atmel_lcdc_dc_at91sam9m11 = {
+	.guard_time = 1,
+	.fifo_size = 512,
+	.min_width = 0,
+	.min_height = 0,
+	.max_width = 2048,
+	.max_height = 2048,
+	.have_hozval = false,
+	.have_alt_pixclock = false,
+};
+
+static const struct atmel_lcdc_dc_desc atmel_lcdc_dc_at91sam9rl = {
+	.guard_time = 1,
+	.fifo_size = 512,
+	.min_width = 0,
+	.min_height = 0,
+	.max_width = 2048,
+	.max_height = 2048,
+	.have_hozval = false,
+	.have_alt_pixclock = false,
+};

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +6 -0

+DEFINE_DRM_GEM_CMA_FOPS(lcdc_dc_drm_fops);
+
+static struct drm_driver lcdc_dc_drm_driver = {
+	.driver_features = DRIVER_HAVE_IRQ |
+			   DRIVER_GEM |
+			   DRIVER_MODESET |

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +162 -0

+	.fops = &lcdc_dc_drm_fops,
+
+	.irq_handler = lcdc_dc_irq_handler,
+	.irq_preinstall = lcdc_dc_irq_uninstall,
+	.irq_postinstall = lcdc_dc_irq_postinstall,
+	.irq_uninstall = lcdc_dc_irq_uninstall,
+
+	.dumb_create = drm_gem_cma_dumb_create,
+
+	.gem_print_info = drm_gem_cma_print_info,
+	.gem_vm_ops = &drm_gem_cma_vm_ops,
+
+	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
+	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
+
+	.gem_prime_import = drm_gem_prime_import,
+	.gem_prime_export = drm_gem_prime_export,
+
+	.gem_prime_get_sg_table	= drm_gem_cma_prime_get_sg_table,
+	.gem_free_object_unlocked = drm_gem_cma_free_object,
+
+	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
+	.gem_prime_vmap = drm_gem_cma_prime_vmap,
+	.gem_prime_vunmap = drm_gem_cma_prime_vunmap,
+	.gem_prime_mmap = drm_gem_cma_prime_mmap,
+};
+
+static int lcdc_dc_modeset_init(struct lcdc_dc *lcdc_dc, struct drm_device *drm)
+{
+	struct drm_bridge *bridge;
+	struct drm_panel *panel;
+	struct device_node *np;
+	struct device *dev;
+	int ret;
+
+	dev = drm->dev;
+
+	drm_mode_config_init(drm);
+	drm->mode_config.min_width  = lcdc_dc->desc->min_width;
+	drm->mode_config.min_height = lcdc_dc->desc->min_height;
+	drm->mode_config.max_width  = lcdc_dc->desc->max_width;
+	drm->mode_config.max_height = lcdc_dc->desc->max_height;
+	drm->mode_config.funcs	    = &mode_config_funcs;
+
+	np = dev->of_node;
+	/* port at 0 is the output port */
+	ret = drm_of_find_panel_or_bridge(np, 0, 0, &panel, &bridge);
+	if (ret && ret != -ENODEV) {
+		DRM_DEV_ERROR(dev, "Failed to find panel (%d)\n", ret);
+		goto err_out;
+	}
+
+	bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);
+	if (IS_ERR(bridge)) {
+		ret = PTR_ERR(bridge);
+		DRM_DEV_ERROR(dev, "Failed to add bridge (%d)", ret);
+		goto err_panel_remove;
+	}
+
+	lcdc_dc->panel = panel;
+	lcdc_dc->bridge = bridge;
+
+	ret = drm_simple_display_pipe_init(drm,
+					   &lcdc_dc->pipe,
+					   &lcdc_dc_display_funcs,
+					   lcdc_dc_formats,
+					   ARRAY_SIZE(lcdc_dc_formats),
+					   NULL,
+					   &lcdc_dc->connector);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "Failed to init display pipe (%d)\n", ret);
+		goto err_panel_remove;
+	}
+
+	ret = drm_simple_display_pipe_attach_bridge(&lcdc_dc->pipe, bridge);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "failed to attach bridge (%d)", ret);
+		goto err_panel_remove;
+	}
+
+	drm_mode_config_reset(drm);
+
+	return 0;
+
+err_panel_remove:
+	if (panel)
+		drm_panel_bridge_remove(bridge);
+
+err_out:
+	return ret;
+}
+
+static int lcdc_dc_load(struct drm_device *drm)
+{
+	const struct of_device_id *match;
+	struct platform_device *pdev;
+	struct lcdc_dc *lcdc_dc;
+	struct device *dev;
+	int ret;
+
+	dev = drm->dev;
+	pdev = to_platform_device(dev);
+
+	match = of_match_node(atmel_lcdc_of_match, dev->parent->of_node);
+	if (!match) {
+		DRM_DEV_ERROR(dev, "invalid compatible string (node=%s)",
+			      dev->parent->of_node->name);
+		return -ENODEV;
+	}
+
+	if (!match->data) {
+		DRM_DEV_ERROR(dev, "invalid lcdc_dc description\n");
+		return -EINVAL;
+	}
+
+	lcdc_dc = devm_kzalloc(dev, sizeof(*lcdc_dc), GFP_KERNEL);
+	if (!lcdc_dc) {
+		DRM_DEV_ERROR(dev, "Failed to allocate lcdc_dc\n");
+		return -ENOMEM;
+	}
+
+	/* reset of lcdc might sleep and require a preemptible task context */
+	INIT_WORK(&lcdc_dc->reset_lcdc_work, reset_lcdc_work);
+
+	platform_set_drvdata(pdev, drm);
+	dev_set_drvdata(dev, lcdc_dc);
+
+	lcdc_dc->mfd_lcdc = dev_get_drvdata(dev->parent);
+	drm->dev_private = lcdc_dc;
+
+	lcdc_dc->regmap = lcdc_dc->mfd_lcdc->regmap;
+	lcdc_dc->desc = match->data;
+	lcdc_dc->dev = dev;
+
+	lcdc_dc->lcd_supply = devm_regulator_get(dev, "lcd");
+	if (IS_ERR(lcdc_dc->lcd_supply)) {
+		DRM_DEV_ERROR(dev, "Failed to get lcd-supply (%ld)\n",
+			      PTR_ERR(lcdc_dc->lcd_supply));
+		lcdc_dc->lcd_supply = NULL;
+	}
+
+	lcdc_dc_start_clock(lcdc_dc);
+
+	pm_runtime_enable(dev);
+
+	ret = drm_vblank_init(drm, 1);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "failed to initialize vblank (%d)\n",
+			      ret);
+		goto err_pm_runtime_disable;
+	}
+
+	ret = lcdc_dc_modeset_init(lcdc_dc, drm);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "modeset_init failed (%d)", ret);
+		goto err_pm_runtime_disable;
+	}
+
+	pm_runtime_get_sync(dev);
+	ret = drm_irq_install(drm, lcdc_dc->mfd_lcdc->irq);
+	pm_runtime_put_sync(dev);
+	if (ret < 0) {

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +23 -0

+
+	drm_kms_helper_poll_init(drm);
+
+	lcdc_dc->fbdev = drm_fbdev_cma_init(drm, 8, 1);
+	if (IS_ERR(lcdc_dc->fbdev)) {
+		ret = PTR_ERR(lcdc_dc->fbdev);
+		DRM_DEV_ERROR(dev, "Failed to init FB CMA area (%d)", ret);
+		goto err_irq_uninstall;
+	}
+
+	drm_helper_hpd_irq_event(drm);
+
+	return 0;
+
+err_irq_uninstall:
+	pm_runtime_get_sync(dev);
+	drm_irq_uninstall(drm);
+	pm_runtime_put_sync(dev);
+
+err_pm_runtime_disable:
+	pm_runtime_disable(dev);
+	lcdc_dc_stop_clock(lcdc_dc);
+

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +86 -0

+	flush_work(&lcdc_dc->reset_lcdc_work);
+	drm_kms_helper_poll_fini(dev);
+	if (lcdc_dc->panel)
+		drm_panel_bridge_remove(lcdc_dc->bridge);
+	drm_mode_config_cleanup(dev);
+
+	pm_runtime_get_sync(dev->dev);
+	drm_irq_uninstall(dev);
+	pm_runtime_put_sync(dev->dev);
+
+	dev->dev_private = NULL;
+
+	pm_runtime_disable(dev->dev);
+	lcdc_dc_stop_clock(lcdc_dc);
+	cancel_work_sync(&lcdc_dc->reset_lcdc_work);
+}
+
+
+static int lcdc_dc_probe(struct platform_device *pdev)
+{
+	struct drm_device *drm;
+	struct device *dev;
+	int ret;
+
+	dev = &pdev->dev;
+
+	drm = drm_dev_alloc(&lcdc_dc_drm_driver, dev);
+	if (IS_ERR(drm)) {
+		DRM_DEV_ERROR(dev, "Failed to allocate drm device\n");
+		return PTR_ERR(drm);
+	}
+
+	ret = lcdc_dc_load(drm);
+	if (ret)
+		goto err_put_ref;
+
+	ret = drm_dev_register(drm, 0);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "Failed to register drm (%d)\n", ret);
+		goto err_unload;
+	}
+
+	return 0;
+
+err_unload:
+	lcdc_dc_unload(drm);
+
+err_put_ref:
+	drm_dev_put(drm);
+	return ret;
+}
+
+static int lcdc_dc_remove(struct platform_device *pdev)
+{
+	struct drm_device *drm;
+
+	drm = platform_get_drvdata(pdev);
+
+	drm_dev_unregister(drm);
+	lcdc_dc_unload(drm);
+	drm_dev_unref(drm);
+
+	return 0;
+}
+
+static const struct of_device_id lcdc_dc_dt_ids[] = {
+	{ .compatible = "atmel,lcdc-display-controller", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, lcdc_dc_dt_ids);
+
+static struct platform_driver lcdc_dc_driver = {
+	.probe		= lcdc_dc_probe,
+	.remove		= lcdc_dc_remove,
+	.driver		= {
+		.of_match_table = lcdc_dc_dt_ids,
+		.name	= "atmel-lcdc-dc",
+	},
+};
+
+module_platform_driver(lcdc_dc_driver);
+
+MODULE_AUTHOR("Sam Ravnborg <[email protected]>");
+MODULE_DESCRIPTION("Atmel LCDC Display Controller DRM Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:atmel-lcdc-dc");

drivers/gpu/drm/atmel/atmel_lcdc-dc.c line 87

Quoted 6 lines

drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file +6 -0

+/* Configuration of individual CPU's */
+static const struct atmel_lcdc_dc_desc atmel_lcdc_dc_at91sam9261 = {
+	.guard_time = 1,
+	.fifo_size = 512,
+	.min_width = 0,
+	.min_height = 0,

Looks like the minimum is always zero, maybe drop it?

The at91sam9263 has a few interesting "GPU" features:

- 2D memory addressing
Data sheet says:
The LCDC can be configured to work on a frame buffer larger than the actual screen size. By changing the values in a few registers, it is easy to move the displayed area along the frame buffer width and height

- 2D Graphics controller
Data sheet says:
The Two D Graphics Controller (TDGC) features a hardware accelerator which highly simplifies drawing tasks and graphic management operations.
The hardware accelerator makes it easy to draw lines and complex polygons and to perform block transfers within the frame buffer.
The TDGC also features a draw command queue that automatically executes a more complex drawing function that is composed of several register accesses.

The datasheet text is from: chapter 43.9 + chapter 44:
http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6249-32-bit-ARM926EJ-S-Embedded-Microprocessor-SAM9263_Datasheet.pdf
(No NDA required)

Based on the above, would it be possible to utilise some of these features without any dedicated userspace (mesa) support? Any other driver that has something similar that can be used for inspiration?

Or is this in reality a simple GPU that requires a dedicated GPU driver?

Sam

Quoted 21 lines · Sam Ravnborg

The at91sam9263 has a few interesting "GPU" features:

- 2D memory addressing
Data sheet says:
The LCDC can be configured to work on a frame buffer larger than the actual screen size. By changing the values in a few registers, it is easy to move the displayed area along the frame buffer width and height

- 2D Graphics controller
Data sheet says:
The Two D Graphics Controller (TDGC) features a hardware accelerator which highly simplifies drawing tasks and graphic management operations.
The hardware accelerator makes it easy to draw lines and complex polygons and to perform block transfers within the frame buffer.
The TDGC also features a draw command queue that automatically executes a more complex drawing function that is composed of several register accesses.

Quoted 3 lines

The datasheet text is from: chapter 43.9 + chapter 44:
http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6249-32-bit-ARM926EJ-S-Embedded-Microprocessor-SAM9263_Datasheet.pdf
(No NDA required)

(old memories) this 2D engine prove itself of being quite limited as the fill polygon and clipping functions are not working.

Quoted 4 lines

Based on the above, would it be possible to utilise some of these features without any dedicated userspace (mesa) support? Any other driver that has something similar that can be used for inspiration?

This is really an interesting question indeed.

Quoted 6 lines

Or is this in reality a simple GPU that requires a dedicated GPU driver?

Sam

Signature · 5 lines

Nicolas Ferre
_______________________________________________
dri-devel mailing list
dri-devel at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 5 lines

Quoted 4 lines

Based on the above, would it be possible to utilise some of these features without any dedicated userspace (mesa) support? Any other driver that has something similar that can be used for inspiration?

Quoted 1 line

This is really an interesting question indeed.

Needs userspace like everything else. There's unfortunately no real standard for 2d apis in userspace, which means none of these efforts go very far. Mostly it's just some custom-made X drivers.

Adding a generic 2d api to drm is a FAQ, and the answer is "no".

Hi Daniel.

Quoted 7 lines

Quoted 6 lines

Quoted 4 lines

Based on the above, would it be possible to utilise some of these features without any dedicated userspace (mesa) support? Any other driver that has something similar that can be used for inspiration?

This is really an interesting question indeed.

Quoted 5 lines

Needs userspace like everything else. There's unfortunately no real standard for 2d apis in userspace, which means none of these efforts go very far. Mostly it's just some custom-made X drivers.

Adding a generic 2d api to drm is a FAQ, and the answer is "no".

And then you went off writing a blog post about it - thanks. The blog post provided a lot of useful answers - great.

(Or rather you will write the blog in 5 days, seems to be some time
machine involved. (Date is the 27th below the title)).

New DRM based driver for at91sam9 SOC's that uses the Atmel LCDC IP core.

This is first version of a patch set that adds
drivers for the Atmel LCDC IP core.
Posted for review as the basics works now.

The LCDC IP core contains two devices:
- a PWM often used for backlight
- a LCD display controller

Both devices are supported today by the atmel_lcdfb driver. For this new set of drivers the compatible strings was selected to avoid clash with the existing compatible strings used for the atmel_lcdfb driver to allow them to co-exist.

This patchset implements three drivers.
- A MFD driver that include the generic parts.
- A PWM driver.
- A DRM display controller driver.
This is the same split as used for the Atmel hlcdc IP.

The hlcdc and lcdc has only a few things in common and trying to share the code for them was not a viable solution.

The DRM implementation has a few shortcomings compared to the existing fbdev based driver:
- STN displays are not supported
Binding support is missing but most of the STN specific functionality is otherwise ported from the fbdev driver.
I assume the info should come from the panel but as I lack HW I have not looked too much into what is required.

- gamma support is missing
The driver utilises drm_simple_kms_helper and this helper lacks support for setting up gamma. If this is useful please let me know and I will extend drm_simple_kms_helper to support this and update the driver.
- modesetting is not checked (see TODO in file)
Is this required for such a simple setup?
- support for extra modes as applicable (and lcd-wiring-mode)
- support for AVR32 (is it relevant?)

The first patch renames .../drm/atmel-hlcdc to .../drm/atmel to have a nice home for both drivers.

The drivers _works_ on a proprietary at91sam9263 based board and I will eventually migrate the at91sam9263ek over to use it as well.

Works is maybe a stretch - the following was tested:
modetest shows reasonable output
modetest -s shows some nice colored squares

vbltest shows 45.70 Hz (after terminating modetest -s with SIGINT) drmdevice shows reasonable output
brightness can be controlled

Anything else that can be recommended for some basic tests? How to test suspend/resume?

REVIEW
Please give feedback on general structure/architecture Please check if the drm framework is used in the optimal way And then the usual review from spelling errors, names, style etc.

All feedback (from spelling errors to general structure) appreciated!

Sam

Sam Ravnborg (7):
atmel-hlcdc: renamed directory to drm/atmel/
dt-binding: add bindings for Atmel LCDC mfd
mfd: add atmel-lcdc driver
dt-bindings: add bindings for Atmel LCDC pwm
pwm: add pwm-atmel-lcdc driver
dt-bindings: add bindings for Atmel lcdc-display-controller
drm: add Atmel LCDC display controller support

  .../display/atmel/lcdc-display-controller.txt      |   40 +
  .../devicetree/bindings/mfd/atmel-lcdc.txt         |   75 ++
  .../devicetree/bindings/pwm/atmel-lcdc-pwm.txt     |   30 +
  MAINTAINERS                                        |    9 +-
  drivers/gpu/drm/Kconfig                            |    2 +-
  drivers/gpu/drm/Makefile                           |    2 +-
  drivers/gpu/drm/atmel-hlcdc/Kconfig                |   10 -
  drivers/gpu/drm/atmel/Kconfig                      |   28 +
  drivers/gpu/drm/{atmel-hlcdc => atmel}/Makefile    |    2 +
  .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_crtc.c  |    0
  .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.c    |    0
  .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.h    |    0
  .../{atmel-hlcdc => atmel}/atmel_hlcdc_output.c    |    0
  .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c |    0
  drivers/gpu/drm/atmel/atmel_lcdc-dc.c              | 1094 ++++++++++++++++++++
  drivers/mfd/Kconfig                                |   10 +
  drivers/mfd/Makefile                               |    1 +
  drivers/mfd/atmel-lcdc.c                           |  158 +++
  drivers/pwm/Kconfig                                |   13 +
  drivers/pwm/Makefile                               |    1 +
  drivers/pwm/pwm-atmel-lcdc.c                       |  178 ++++
  include/linux/mfd/atmel-lcdc.h                     |  184 ++++
  22 files changed, 1824 insertions(+), 13 deletions(-)

Quoted 2 lines · Sam Ravnborg

New DRM based driver for at91sam9 SOC's that uses the Atmel LCDC IP core.

I'm delighted to see this work: Thanks a lot Sam!

Quoted 9 lines

This is first version of a patch set that adds
drivers for the Atmel LCDC IP core.
Posted for review as the basics works now.

The LCDC IP core contains two devices:
- a PWM often used for backlight
- a LCD display controller

Quoted 5 lines

Both devices are supported today by the atmel_lcdfb driver. For this new set of drivers the compatible strings was selected to avoid clash with the existing compatible strings used for the atmel_lcdfb driver to allow them to co-exist.

Would be good to have a plan to phase-out the old atmel_lcdfb fbdev driver when this one addresses some TODO items that make sense.

The mfd suffix seems strange to me. What about "atmel,at91sam9263-lcdc-mfd" => "atmel,at91sam9263-lcd" (or "microchip,at91sam9263-lcdc").

Quoted 10 lines

The DRM implementation has a few shortcomings compared to the existing fbdev based driver:
- STN displays are not supported
Binding support is missing but most of the STN specific functionality is otherwise ported from the fbdev driver.
I assume the info should come from the panel but as I lack HW I have not looked too much into what is required.

Yes, I'm not even sure if STN displays are still available those days... Might not be worth it spending time on this.

Quoted 11 lines

- gamma support is missing
The driver utilises drm_simple_kms_helper and this helper lacks support for setting up gamma. If this is useful please let me know and I will extend drm_simple_kms_helper to support this and update the driver.
- modesetting is not checked (see TODO in file)
Is this required for such a simple setup?
- support for extra modes as applicable (and lcd-wiring-mode)
- support for AVR32 (is it relevant?)

No, AVR32 is not relevant anymore as the core and SoC were removed some years ago from Linux.
All mention of AT32 or AVR32 can be remove then.

Quoted 3 lines

The drivers _works_ on a proprietary at91sam9263 based board and I will eventually migrate the at91sam9263ek over to use it as well.

We'll be able to test it on other SoCs and boards.

Quoted 4 lines

Works is maybe a stretch - the following was tested:
modetest shows reasonable output
modetest -s shows some nice colored squares

You must see something like this (without the overlay additional black square):
http://www.at91.com/linux4sam/bin/view/Linux4SAM/UsingAtmelDRMDriver#Some_modetest_commands

Quoted 1 line

All feedback (from spelling errors to general structure) appreciated!

On my side, it's mostly Nitpicking ;-)
Now that we're Microchip for a little bit more than 2 years, I tend to make this name prevalent compared to Atmel for new work around our products... But I know this driver is for older SoCs and that it got inspired by two former drivers that have this "atmel" naming everywhere. MAINTAINERS and Kconfig changes could include Microchip name, so that it's obvious for the newcomers...

Hi Nicholas.

Quoted 5 lines · Nicolas Ferre

Quoted 2 lines · Sam Ravnborg

New DRM based driver for at91sam9 SOC's that uses the Atmel LCDC IP core.

I'm delighted to see this work: Thanks a lot Sam!

Glad to hear.
I was a bit worried that the response would be "this is waste of time as we have a working driver already".

Quoted 9 lines

Quoted 9 lines

This is first version of a patch set that adds
drivers for the Atmel LCDC IP core.
Posted for review as the basics works now.

The LCDC IP core contains two devices:
- a PWM often used for backlight
- a LCD display controller

Quoted 8 lines

Quoted 5 lines

Both devices are supported today by the atmel_lcdfb driver. For this new set of drivers the compatible strings was selected to avoid clash with the existing compatible strings used for the atmel_lcdfb driver to allow them to co-exist.

Would be good to have a plan to phase-out the old atmel_lcdfb fbdev driver when this one addresses some TODO items that make sense.

Agree on this.
One approach could be to say that when all in-kernel users of atmel_lcdfb are ported, then the old driver could be dropped after a kernel release.

Quoted 2 lines

The mfd suffix seems strange to me. What about "atmel,at91sam9263-lcdc-mfd" => "atmel,at91sam9263-lcd" (or "microchip,at91sam9263-lcdc").

The "-mfd" suffix was added to avoid clashing with the current compatible string used by the atmel_lcdfb driver.

I susggest we do the following:
- use the microchip prefix, as this is now owned by microchip
- and add the driver to a drm/microchip/ directory (Then we can only hope that microchip do not change name or are purchased by someone else).

I don't know that 2 registers for a backlight PWM constitute an MFD. A single node can be both an LCD controller and a PWM.

Hi Rob.

Quoted 2 lines

I don't know that 2 registers for a backlight PWM constitute an MFD. A single node can be both an LCD controller and a PWM.

Current suggestion from v1 patchset looks like this:

	lcdc0: lcdc at 700000 {
		compatible = "atmel,at91sam9263-lcdc-mfd";
		reg = <0x700000 0x1000>;
		interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>;
		clocks = <&lcd_clk>, <&lcd_clk>;
		clock-names = "lcdc_clk", "hclk";

		lcdc-display-controller {
			compatible = "atmel,lcdc-display-controller";
			lcd-supply = <&lcdc_reg>;

			port at 0 {
			};
		};

		lcdc_pwm: lcdc-pwm {
			compatible = "atmel,lcdc-pwm";
			#pwm-cells = <3>;
		};

	};

        backlight: backlight {
                compatible = "pwm-backlight";
                pwms = <&lcdc_pwm 0 50000 0>;
        };

We could have described the full binding in one file, rather than in three files like it is done in v1. But the structure was done so it matched what was done for hlcdc.

If I understand the proposal from you correct an example binding would look like this:

       lcdc0: lcdc at 700000 {
                compatible = "atmel,at91sam9263-lcdc";
                reg = <0x700000 0x1000>;
                interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>;
                clocks = <&lcd_clk>, <&lcd_clk>;
                clock-names = "lcdc_clk", "hclk";

                lcd-supply = <&lcdc_reg>;

                port at 0 {
                };

                #pwm-cells = <3>;
        };

        backlight: backlight {
                compatible = "pwm-backlight";
                pwms = <&lcd0 0 50000 0>;
        };

This is doable, but IMO it is less obvious that the LCDC IP core implements two different features - a PWM and a LCD controller.

Right now the preference is to stay with the v1 approach:
- It is a mirror of what we do today for hlcdc, so no suprises

- It shows in a nice way that the LCDC IP core implments both an LCD controller and a PWM
- The pwm functionality is not hiddin inside the lcdc stuff, and it is thus simpler to add good pin-ctrl handles with nice names that matches the usage. (I could always add more pin-ctrl, but it is common to refer to a single pin-ctrl.

One DT related Q:
The LCD Controller supports BGR565, but as this is less common some HW implmentations exchange R and B, expessed in the old binding as wiring-mode like this:

atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG"

How can we express this wiring-mode in a generic way, both in DT and in code? Is it something that in DRM belongs to the panel, the encoder, the connector, or? And can any of the exisitng flags be used?

Quoted 24 lines

If I understand the proposal from you correct an example binding would look like this:

       lcdc0: lcdc@700000 {
                compatible = "atmel,at91sam9263-lcdc";
                reg = <0x700000 0x1000>;
                interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>;
                clocks = <&lcd_clk>, <&lcd_clk>;
                clock-names = "lcdc_clk", "hclk";

                lcd-supply = <&lcdc_reg>;

                port@0 {
                };

                #pwm-cells = <3>;
        };

        backlight: backlight {
                compatible = "pwm-backlight";
                pwms = <&lcd0 0 50000 0>;
        };

Quoted 2 lines

This is doable, but IMO it is less obvious that the LCDC IP core implements two different features - a PWM and a LCD controller.

Features don't equate to nodes. If the sub-blocks can be separately instantiated or have their own resources then sub-nodes make sense. Otherwise, it's a single device (node) with multiple providers.

Quoted 2 lines

Right now the preference is to stay with the v1 approach:
- It is a mirror of what we do today for hlcdc, so no suprises

Which BTW doesn't appear to have actually been reviewed.

Do these IP blocks actually share anything?

Consistency is nice, but keeping compatibility with the existing binding by extending things in a backwards compatible way is more important. Implementing a new driver is not license to change the binding. Shall I let u-boot or *BSD developers change the binding too for their driver?

Quoted 4 lines

- It shows in a nice way that the LCDC IP core implments both an LCD controller and a PWM
- The pwm functionality is not hiddin inside the lcdc stuff, and it is thus simpler to add good pin-ctrl handles with nice names that matches the usage. (I could always add more pin-ctrl, but it is common to refer to a single pin-ctrl.

Does anyone actually use this for a non-backlight PWM? I'm not sure the abstraction is worth it. The driver could just register itself as a backlight provider rather than a PWM provider.

Quoted 7 lines

One DT related Q:
The LCD Controller supports BGR565, but as this is less common some HW implmentations exchange R and B, expessed in the old binding as wiring-mode like this:

atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG"

Quoted 3 lines

How can we express this wiring-mode in a generic way, both in DT and in code? Is it something that in DRM belongs to the panel, the encoder, the connector, or? And can any of the exisitng flags be used?

I thought we had come up with a common definition, but I guess it didn't make it upstream. It's definitely needed and I've been rejecting anything new that's vendor specific.

Quoted 10 lines

Quoted 9 lines

One DT related Q:
The LCD Controller supports BGR565, but as this is less common some HW implmentations exchange R and B, expessed in the old binding as wiring-mode like this:

atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG"

How can we express this wiring-mode in a generic way, both in DT and in code? Is it something that in DRM belongs to the panel, the encoder, the connector, or? And can any of the exisitng flags be used?

Quoted 3 lines

I thought we had come up with a common definition, but I guess it didn't make it upstream. It's definitely needed and I've been rejecting anything new that's vendor specific.

I found this: https://patchwork.ozlabs.org/patch/659570/
The suggestion with a boolean seems simple and I will try that.

Quoted 1 line · Sam Ravnborg

[...]

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

One DT related Q:
The LCD Controller supports BGR565, but as this is less common some HW implmentations exchange R and B, expessed in the old binding as wiring-mode like this:

atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG"

How can we express this wiring-mode in a generic way, both in DT and in code? Is it something that in DRM belongs to the panel, the encoder, the connector, or? And can any of the exisitng flags be used?

Quoted 5 lines

Quoted 3 lines

I thought we had come up with a common definition, but I guess it didn't make it upstream. It's definitely needed and I've been rejecting anything new that's vendor specific.

I found this: https://patchwork.ozlabs.org/patch/659570/
The suggestion with a boolean seems simple and I will try that.

That's really too simple to be common. There's been a few other attempts[1][2][3] (for Atmel HLCDC in fact) with [3] probably being the closest to merging. The binding looked fine to me, but looks like the DRM implementation had some issues.

Rob

[1] https://lists.freedesktop.org/archives/dri-devel/2018-April/173089.html
[2] https://patchwork.kernel.org/patch/9965079/
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/568731.html

Quoted 26 lines · Rob Herring

Quoted 1 line · Sam Ravnborg

[...]

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

One DT related Q:
The LCD Controller supports BGR565, but as this is less common some HW implmentations exchange R and B, expessed in the old binding as wiring-mode like this:

atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG"

How can we express this wiring-mode in a generic way, both in DT and in code? Is it something that in DRM belongs to the panel, the encoder, the connector, or? And can any of the exisitng flags be used?

Quoted 5 lines

Quoted 3 lines

I thought we had come up with a common definition, but I guess it didn't make it upstream. It's definitely needed and I've been rejecting anything new that's vendor specific.

I found this: https://patchwork.ozlabs.org/patch/659570/
The suggestion with a boolean seems simple and I will try that.

That's really too simple to be common. There's been a few other attempts[1][2][3] (for Atmel HLCDC in fact) with [3] probably being the closest to merging. The binding looked fine to me, but looks like the DRM implementation had some issues.

Quoted 5 lines

Rob

[1] https://lists.freedesktop.org/archives/dri-devel/2018-April/173089.html
[2] https://patchwork.kernel.org/patch/9965079/
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/568731.html

There's a standardized bus_format for drm_panel. IIrc there's been discussions to add something similar to drm_bridge, but didn't go all that far for reasons.

Lots of drivers just handle this internally in some way. So no idea why this all got stalled.

I stopped pushing that patch for reasons mentioned in
https://lkml.org/lkml/2018/4/9/108

However, since then, the component approach mentioned in that mail was shot down and instead the tda998x driver is now a bridge (or soon, I expect the series from Russell King that to land in 4.19-rc1) which means that the binding from that series is back on the table. At least I guess so? However, in that series the approach is that the bridge states its expected input and the output then adjusts to what is needed. However, the "problem" is really in the atmel-hlcdc output (it moves the MSB for the RGB colors around depending on the output mode) so I no longer subscribe to all ideas in that series and think it is cleaner to state the needed bus format closer to the atmel- hlcdc endpoint as is done e.g. in this latest series:

https://lkml.org/lkml/2018/8/10/309

This latest series uses the media/video-interface approach and specifies the bus-width in the endpoint. However, the bus-width is alone obviously not enough to differentiate rgb565 and brg565, so will not help Sam

(or is it bgr565 that Sam needs? The above quoted text is ambiguous).

I think something like my bus-format binding in [3] is generic enough to also help Sam.

Quoted 2 lines

Would be good to have a plan to phase-out the old atmel_lcdfb fbdev driver when this one addresses some TODO items that make sense.

Agree on this.
One approach could be to say that when all in-kernel users of atmel_lcdfb are ported, then the old driver could be dropped after a kernel release.

Quoted 2 lines

The mfd suffix seems strange to me. What about "atmel,at91sam9263-lcdc-mfd" => "atmel,at91sam9263-lcd" (or "microchip,at91sam9263-lcdc").

The "-mfd" suffix was added to avoid clashing with the current compatible string used by the atmel_lcdfb driver.

I susggest we do the following:
- use the microchip prefix, as this is now owned by microchip
- and add the driver to a drm/microchip/ directory (Then we can only hope that microchip do not change name or are purchased by someone else).

Quoted 6 lines · Sam Ravnborg

Quoted 2 lines

Would be good to have a plan to phase-out the old atmel_lcdfb fbdev driver when this one addresses some TODO items that make sense.

Agree on this.
One approach could be to say that when all in-kernel users of atmel_lcdfb are ported, then the old driver could be dropped after a kernel release.

I would drop it only after an LTS release.

Quoted 12 lines

Quoted 3 lines

The mfd suffix seems strange to me. What about "atmel,at91sam9263-lcdc-mfd" => "atmel,at91sam9263-lcd" (or "microchip,at91sam9263-lcdc").

The "-mfd" suffix was added to avoid clashing with the current compatible string used by the atmel_lcdfb driver.

I susggest we do the following:
- use the microchip prefix, as this is now owned by microchip
- and add the driver to a drm/microchip/ directory (Then we can only hope that microchip do not change name or are purchased by someone else).

The compatible string should remain the same but the drivers have to be mutually exclusive in Kconfig.

New DRM based driver for at91sam9 SOC's that uses the Atmel LCDC IP core.

This is first version of a patch set that adds
drivers for the Atmel LCDC IP core.
Posted for review as the basics works now.

The LCDC IP core contains two devices:
- a PWM often used for backlight
- a LCD display controller

Both devices are supported today by the atmel_lcdfb driver. For this new set of drivers the compatible strings was selected to avoid clash with the existing compatible strings used for the atmel_lcdfb driver to allow them to co-exist.

This patchset implements three drivers.
- A MFD driver that include the generic parts.
- A PWM driver.
- A DRM display controller driver.
This is the same split as used for the Atmel hlcdc IP.

The hlcdc and lcdc has only a few things in common and trying to share the code for them was not a viable solution.

The DRM implementation has a few shortcomings compared to the existing fbdev based driver:
- STN displays are not supported
Binding support is missing but most of the STN specific functionality is otherwise ported from the fbdev driver.
I assume the info should come from the panel but as I lack HW I have not looked too much into what is required.
- gamma support is missing
The driver utilises drm_simple_kms_helper and this helper lacks support for setting up gamma. If this is useful please let me know and I will extend drm_simple_kms_helper to support this and update the driver.

- modesetting is not checked (see TODO in file) Is this required for such a simple setup?

- support for extra modes as applicable (and lcd-wiring-mode)

- support for AVR32 (is it relevant?)

The first patch renames .../drm/atmel-hlcdc to .../drm/atmel to have a nice home for both drivers.

Quoted 6 lines

Both devices are supported today by the atmel_lcdfb driver. For this new set of drivers the compatible strings was selected to avoid clash with the existing compatible strings used for the atmel_lcdfb driver to allow them to co-exist.

Hm, I think Rob commented on that already, but we usually try to stay compatible with the exisiting/old bindings when introducing a new one. Don't know how feasible this is in this particular case though.

Quoted 7 lines

The DRM implementation has a few shortcomings compared to the existing fbdev based driver:
- STN displays are not supported
Binding support is missing but most of the STN specific functionality is otherwise ported from the fbdev driver.
I assume the info should come from the panel

Quoted 8 lines

but as I lack HW I have not looked too much into what is required.
- gamma support is missing
The driver utilises drm_simple_kms_helper and this helper lacks support for setting up gamma. If this is useful please let me know and I will extend drm_simple_kms_helper to support this and update the driver.

I guess you can skip that for now.

Quoted 3 lines

- modesetting is not checked (see TODO in file) Is this required for such a simple setup?

Well, that's always better if you can check that the requested display mode is supported before trying to apply it.

Quoted 2 lines

- support for extra modes as applicable (and lcd-wiring-mode)

Peter already suggested something I think.