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(-)
Use vendor name for directory, adding a suitable place for more atmel DRM drivers.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Boris Brezillon boris.brezillon@bootlin.com --- 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%)
diff --git a/MAINTAINERS b/MAINTAINERS index 96e98e206b0d..09ce76a9a1dc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4681,7 +4681,7 @@ DRM DRIVERS FOR ATMEL HLCDC M: Boris Brezillon boris.brezillon@bootlin.com L: dri-devel@lists.freedesktop.org S: Supported -F: drivers/gpu/drm/atmel-hlcdc/ +F: drivers/gpu/drm/atmel/atmel-hlcdc* F: Documentation/devicetree/bindings/display/atmel/ T: git git://anongit.freedesktop.org/drm/drm-misc
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 2a72d2feb76d..4130df0c0dba 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -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"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index ef9f3dab287f..ce9829967128 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -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/ diff --git a/drivers/gpu/drm/atmel-hlcdc/Kconfig b/drivers/gpu/drm/atmel/Kconfig similarity index 83% rename from drivers/gpu/drm/atmel-hlcdc/Kconfig rename to drivers/gpu/drm/atmel/Kconfig index 32bcc4bad06a..7cd3862f9d18 100644 --- a/drivers/gpu/drm/atmel-hlcdc/Kconfig +++ b/drivers/gpu/drm/atmel/Kconfig @@ -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 diff --git a/drivers/gpu/drm/atmel-hlcdc/Makefile b/drivers/gpu/drm/atmel/Makefile similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/Makefile rename to drivers/gpu/drm/atmel/Makefile diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel/atmel_hlcdc_crtc.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_crtc.c diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel/atmel_hlcdc_dc.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_dc.c diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel/atmel_hlcdc_dc.h similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h rename to drivers/gpu/drm/atmel/atmel_hlcdc_dc.h diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel/atmel_hlcdc_output.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_output.c diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel/atmel_hlcdc_plane.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_plane.c
On Sun, Aug 12, 2018 at 08:46:23PM +0200, Sam Ravnborg wrote:
Use vendor name for directory, adding a suitable place for more atmel DRM drivers.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Boris Brezillon boris.brezillon@bootlin.com
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%)
diff --git a/MAINTAINERS b/MAINTAINERS index 96e98e206b0d..09ce76a9a1dc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4681,7 +4681,7 @@ DRM DRIVERS FOR ATMEL HLCDC M: Boris Brezillon boris.brezillon@bootlin.com L: dri-devel@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. -Daniel
F: Documentation/devicetree/bindings/display/atmel/ T: git git://anongit.freedesktop.org/drm/drm-misc
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 2a72d2feb76d..4130df0c0dba 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -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"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index ef9f3dab287f..ce9829967128 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -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/ diff --git a/drivers/gpu/drm/atmel-hlcdc/Kconfig b/drivers/gpu/drm/atmel/Kconfig similarity index 83% rename from drivers/gpu/drm/atmel-hlcdc/Kconfig rename to drivers/gpu/drm/atmel/Kconfig index 32bcc4bad06a..7cd3862f9d18 100644 --- a/drivers/gpu/drm/atmel-hlcdc/Kconfig +++ b/drivers/gpu/drm/atmel/Kconfig @@ -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
diff --git a/drivers/gpu/drm/atmel-hlcdc/Makefile b/drivers/gpu/drm/atmel/Makefile similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/Makefile rename to drivers/gpu/drm/atmel/Makefile diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel/atmel_hlcdc_crtc.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_crtc.c diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel/atmel_hlcdc_dc.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_dc.c diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel/atmel_hlcdc_dc.h similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h rename to drivers/gpu/drm/atmel/atmel_hlcdc_dc.h diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel/atmel_hlcdc_output.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_output.c diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel/atmel_hlcdc_plane.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_plane.c -- 2.12.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel.
rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c (100%)
diff --git a/MAINTAINERS b/MAINTAINERS index 96e98e206b0d..09ce76a9a1dc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4681,7 +4681,7 @@ DRM DRIVERS FOR ATMEL HLCDC M: Boris Brezillon boris.brezillon@bootlin.com L: dri-devel@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.
Sam
On Tue, Aug 14, 2018 at 06:19:52PM +0200, Sam Ravnborg wrote:
Hi Daniel.
rename drivers/gpu/drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c (100%)
diff --git a/MAINTAINERS b/MAINTAINERS index 96e98e206b0d..09ce76a9a1dc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4681,7 +4681,7 @@ DRM DRIVERS FOR ATMEL HLCDC M: Boris Brezillon boris.brezillon@bootlin.com L: dri-devel@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.
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
Cheers, Daniel
Hi Daniel.
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:
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.
Sam
On Wed, Aug 22, 2018 at 10:09 PM, Sam Ravnborg sam@ravnborg.org wrote:
Hi Daniel.
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:
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.
Migrating the drm repos will start earliest next year (because fd.o admins need to rework some of the infrastructure). And this is the optimistic estimate. We'll also try to make the transition as seamless as possible for committers (the scripting we're using will auto-upgrade remotes and everything, plus accounts will get migrated).
Holding off imo doesn't make sense, just means your driver won't land for another 6 months or so, or at least be in some funny maintainer limbo state. -Daniel
On Sun, 12 Aug 2018 20:46:23 +0200 Sam Ravnborg sam@ravnborg.org wrote:
Use vendor name for directory, adding a suitable place for more atmel DRM drivers.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Boris Brezillon boris.brezillon@bootlin.com
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%)
diff --git a/MAINTAINERS b/MAINTAINERS index 96e98e206b0d..09ce76a9a1dc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4681,7 +4681,7 @@ DRM DRIVERS FOR ATMEL HLCDC M: Boris Brezillon boris.brezillon@bootlin.com L: dri-devel@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.
F: Documentation/devicetree/bindings/display/atmel/ T: git git://anongit.freedesktop.org/drm/drm-misc
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 2a72d2feb76d..4130df0c0dba 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -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"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index ef9f3dab287f..ce9829967128 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -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/ diff --git a/drivers/gpu/drm/atmel-hlcdc/Kconfig b/drivers/gpu/drm/atmel/Kconfig similarity index 83% rename from drivers/gpu/drm/atmel-hlcdc/Kconfig rename to drivers/gpu/drm/atmel/Kconfig index 32bcc4bad06a..7cd3862f9d18 100644 --- a/drivers/gpu/drm/atmel-hlcdc/Kconfig +++ b/drivers/gpu/drm/atmel/Kconfig @@ -1,6 +1,12 @@ +config DRM_ATMEL
- 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.
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
diff --git a/drivers/gpu/drm/atmel-hlcdc/Makefile b/drivers/gpu/drm/atmel/Makefile similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/Makefile rename to drivers/gpu/drm/atmel/Makefile diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel/atmel_hlcdc_crtc.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_crtc.c diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel/atmel_hlcdc_dc.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_dc.c diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel/atmel_hlcdc_dc.h similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h rename to drivers/gpu/drm/atmel/atmel_hlcdc_dc.h diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel/atmel_hlcdc_output.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_output.c diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel/atmel_hlcdc_plane.c similarity index 100% rename from drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c rename to drivers/gpu/drm/atmel/atmel_hlcdc_plane.c
Hi Boris.
On Fri, Aug 24, 2018 at 10:28:43AM +0200, Boris Brezillon wrote:
On Sun, 12 Aug 2018 20:46:23 +0200 Sam Ravnborg sam@ravnborg.org wrote:
Use vendor name for directory, adding a suitable place for more atmel DRM drivers.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Boris Brezillon boris.brezillon@bootlin.com
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%)
diff --git a/MAINTAINERS b/MAINTAINERS index 96e98e206b0d..09ce76a9a1dc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4681,7 +4681,7 @@ DRM DRIVERS FOR ATMEL HLCDC M: Boris Brezillon boris.brezillon@bootlin.com L: dri-devel@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.
Thanks, will do so in v2.
F: Documentation/devicetree/bindings/display/atmel/ T: git git://anongit.freedesktop.org/drm/drm-misc
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 2a72d2feb76d..4130df0c0dba 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -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"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index ef9f3dab287f..ce9829967128 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -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/ diff --git a/drivers/gpu/drm/atmel-hlcdc/Kconfig b/drivers/gpu/drm/atmel/Kconfig similarity index 83% rename from drivers/gpu/drm/atmel-hlcdc/Kconfig rename to drivers/gpu/drm/atmel/Kconfig index 32bcc4bad06a..7cd3862f9d18 100644 --- a/drivers/gpu/drm/atmel-hlcdc/Kconfig +++ b/drivers/gpu/drm/atmel/Kconfig @@ -1,6 +1,12 @@ +config DRM_ATMEL
- 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 extra config option is to avoid exposing every single option to the drm makefile, and it is indeed selected by the individual drivers. Similar pattern used in arm/Kconfig. But I see most other places have a prompt that enable the drivers so I will change this to a prompt in v2.
Sam
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 sam@ravnborg.org Cc: Lee Jones lee.jones@linaro.org Cc: Boris Brezillon boris.brezillon@free-electrons.com --- .../devicetree/bindings/mfd/atmel-lcdc.txt | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/atmel-lcdc.txt
diff --git a/Documentation/devicetree/bindings/mfd/atmel-lcdc.txt b/Documentation/devicetree/bindings/mfd/atmel-lcdc.txt new file mode 100644 index 000000000000..70e9b7bda6c7 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/atmel-lcdc.txt @@ -0,0 +1,75 @@ +Device-Tree bindings for Atmel's LCDC (LCD Controller) MFD driver + +Required properties: + - compatible: value should be one of the following: + "atmel,at91sam9261-lcdc-mfd" + "atmel,at91sam9263-lcdc-mfd" + "atmel,at91sam9g10-lcdc-mfd" + "atmel,at91sam9g45-lcdc-mfd" + "atmel,at91sam9g46-lcdc-mfd" + "atmel,at91sam9m10-lcdc-mfd" + "atmel,at91sam9m11-lcdc-mfd" + "atmel,at91sam9rl-lcdc-mfd" + - 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@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@0 { + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + lcdc_panel_output: endpoint@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@0 { + #address-cells = <1>; + #size-cells = <0>; + + panel_input: endpoint@0 { + reg = <0>; + remote-endpoint = <&lcdc_panel_output>; + }; + }; + };
On Sun, 12 Aug 2018 20:46:24 +0200 Sam Ravnborg sam@ravnborg.org wrote:
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 sam@ravnborg.org Cc: Lee Jones lee.jones@linaro.org Cc: Boris Brezillon boris.brezillon@free-electrons.com
.../devicetree/bindings/mfd/atmel-lcdc.txt | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/atmel-lcdc.txt
diff --git a/Documentation/devicetree/bindings/mfd/atmel-lcdc.txt b/Documentation/devicetree/bindings/mfd/atmel-lcdc.txt new file mode 100644 index 000000000000..70e9b7bda6c7 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/atmel-lcdc.txt @@ -0,0 +1,75 @@ +Device-Tree bindings for Atmel's LCDC (LCD Controller) MFD driver
+Required properties:
- compatible: value should be one of the following:
- "atmel,at91sam9261-lcdc-mfd"
- "atmel,at91sam9263-lcdc-mfd"
- "atmel,at91sam9g10-lcdc-mfd"
- "atmel,at91sam9g45-lcdc-mfd"
- "atmel,at91sam9g46-lcdc-mfd"
- "atmel,at91sam9m10-lcdc-mfd"
- "atmel,at91sam9m11-lcdc-mfd"
- "atmel,at91sam9rl-lcdc-mfd"
I'm pretty sure we don't want the -mfd suffix in the compatible. I know it's here to avoid clashes with the old binding, so maybe we can find name.
- 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@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@0 {
reg = <0>;
#address-cells = <1>;
#size-cells = <0>;
lcdc_panel_output: endpoint@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>;
};
- };
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@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@0 { reg = <0>; #address-cells = <1>; #size-cells = <0>; lcdc_panel_output: endpoint@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.
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.
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
#address-cells = <1>;
#size-cells = <0>;
lcdc_panel_output: endpoint@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@0 {
#address-cells = <1>;
#size-cells = <0>;
panel_input: endpoint@0 {
reg = <0>;
remote-endpoint = <&lcdc_panel_output>;
};
};
- };
Hi Boris.
+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@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@0 {
reg = <0>;
#address-cells = <1>;
#size-cells = <0>;
lcdc_panel_output: endpoint@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>;
};
- };
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@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@0 { reg = <0>; #address-cells = <1>; #size-cells = <0>; lcdc_panel_output: endpoint@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.
I have something similar in my tree now, just need to figure out the mfd parts.
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.
It was added only because I could do it and I was (and continue to be) a DT newbie. Will drop it in v2 as this is just an example which this part do not add any value to.
- panel: panel {
compatible = "logictechnologies,lttd800480070-l2rt", "simple-panel";
Reminds me that I need to dig out a panel-simple patch for a few displays I use (logic + seiko). But they are otherwise not related to this work.
Sam
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 sam@ravnborg.org Cc: Lee Jones lee.jones@linaro.org Cc: Boris Brezillon boris.brezillon@free-electrons.com --- 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
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index b860eb5aa194..f4851f0f033f 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -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 diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index e9fd20dba18d..dba8465e0d96 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -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 diff --git a/drivers/mfd/atmel-lcdc.c b/drivers/mfd/atmel-lcdc.c new file mode 100644 index 000000000000..8928976bafca --- /dev/null +++ b/drivers/mfd/atmel-lcdc.c @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2018 Sam Ravnborg + * + * Author: Sam Ravnborg sam@ravnborg.org + * + * Based on atmel-hlcdc.c wich is: + * Copyright (C) 2014 Free Electrons + * Copyright (C) 2014 Atmel + * Author: Boris BREZILLON boris.brezillon@free-electrons.com + */ + +#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 sam@ravnborg.org"); +MODULE_DESCRIPTION("Atmel LCDC mfd driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/mfd/atmel-lcdc.h b/include/linux/mfd/atmel-lcdc.h new file mode 100644 index 000000000000..fdab269baa8e --- /dev/null +++ b/include/linux/mfd/atmel-lcdc.h @@ -0,0 +1,184 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2018 Sam Ravnborg + * + * Author: Sam Ravnborg sam@ravnborg.org + */ + +#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 */
Hi Sam,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on at91/at91-next] [also build test WARNING on v4.18 next-20180813] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sam-Ravnborg/add-at91sam9-LCDC-DRM-... base: https://git.kernel.org/pub/scm/linux/kernel/git/nferre/linux-at91.git at91-next config: sparc64-allyesconfig (attached as .config) compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=sparc64
All warnings (new ones prefixed by >>):
drivers/mfd/atmel-lcdc.c: In function 'lcdc_probe':
drivers/mfd/atmel-lcdc.c:127:32: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long unsigned int' [-Wformat=]
dev_err(dev, "Failed to add %d mfd devices (%d)\n", ~^ %ld
vim +127 drivers/mfd/atmel-lcdc.c
66 67 static int lcdc_probe(struct platform_device *pdev) 68 { 69 struct atmel_mfd_lcdc *lcdc; 70 struct lcdc_regmap *regmap; 71 struct resource *res; 72 struct device *dev; 73 int ret; 74 75 dev = &pdev->dev; 76 77 regmap = devm_kzalloc(dev, sizeof(*regmap), GFP_KERNEL); 78 if (!regmap) 79 return -ENOMEM; 80 81 lcdc = devm_kzalloc(dev, sizeof(*lcdc), GFP_KERNEL); 82 if (!lcdc) 83 return -ENOMEM; 84 85 res = platform_get_resource(pdev, IORESOURCE_MEM, 0); 86 regmap->regs = devm_ioremap_resource(dev, res); 87 if (IS_ERR(regmap->regs)) { 88 dev_err(dev, "Failed to allocate IO mem (%ld)\n", 89 PTR_ERR(regmap->regs)); 90 return PTR_ERR(regmap->regs); 91 } 92 93 lcdc->irq = platform_get_irq(pdev, 0); 94 if (lcdc->irq < 0) { 95 dev_err(dev, "Failed to get irq (%d)\n", lcdc->irq); 96 return lcdc->irq; 97 } 98 99 lcdc->lcdc_clk = devm_clk_get(dev, "lcdc_clk"); 100 if (IS_ERR(lcdc->lcdc_clk)) { 101 dev_err(dev, "failed to get lcdc clock (%ld)\n", 102 PTR_ERR(lcdc->lcdc_clk)); 103 return PTR_ERR(lcdc->lcdc_clk); 104 } 105 106 lcdc->bus_clk = devm_clk_get(dev, "hclk"); 107 if (IS_ERR(lcdc->bus_clk)) { 108 dev_err(dev, "failed to get bus clock (%ld)\n", 109 PTR_ERR(lcdc->bus_clk)); 110 return PTR_ERR(lcdc->bus_clk); 111 } 112 113 lcdc->regmap = devm_regmap_init(dev, NULL, regmap, 114 &lcdc_regmap_config); 115 if (IS_ERR(lcdc->regmap)) { 116 dev_err(dev, "Failed to init regmap (%ld)\n", 117 PTR_ERR(lcdc->regmap)); 118 return PTR_ERR(lcdc->regmap); 119 } 120 121 dev_set_drvdata(dev, lcdc); 122 123 ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, 124 lcdc_cells, ARRAY_SIZE(lcdc_cells), 125 NULL, 0, NULL); 126 if (ret < 0)
127 dev_err(dev, "Failed to add %d mfd devices (%d)\n",
128 ARRAY_SIZE(lcdc_cells), ret); 129 130 return ret; 131 } 132
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sun, 12 Aug 2018, Sam Ravnborg wrote:
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 sam@ravnborg.org Cc: Lee Jones lee.jones@linaro.org Cc: Boris Brezillon boris.brezillon@free-electrons.com
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
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index b860eb5aa194..f4851f0f033f 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -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 diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index e9fd20dba18d..dba8465e0d96 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -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 diff --git a/drivers/mfd/atmel-lcdc.c b/drivers/mfd/atmel-lcdc.c new file mode 100644 index 000000000000..8928976bafca --- /dev/null +++ b/drivers/mfd/atmel-lcdc.c @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (C) 2018 Sam Ravnborg
- Author: Sam Ravnborg sam@ravnborg.org
- Based on atmel-hlcdc.c wich is:
- Copyright (C) 2014 Free Electrons
- Copyright (C) 2014 Atmel
- Author: Boris BREZILLON boris.brezillon@free-electrons.com
- */
+#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",
- },
+};
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.
+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.
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.
Sam
On 15/08/2018 at 22:40, Sam Ravnborg wrote:
Hi Lee.
+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.
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.
Best regards,
On Thu, 16 Aug 2018, Nicolas Ferre wrote:
On 15/08/2018 at 22:40, Sam Ravnborg wrote:
Hi Lee.
+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.
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....
Yes, that is true. They are very similar drivers. Would it make sense to use the same driver for both devices?
Anyway, I'd wait for Boris' feedback for making a decision.
On Thu, 16 Aug 2018 10:28:54 +0200 Nicolas Ferre nicolas.ferre@microchip.com wrote:
On 15/08/2018 at 22:40, Sam Ravnborg wrote:
Hi Lee.
+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.
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.
If possible I'd like to keep the MFD approach, but let's see if we can have a single node in the DT instead of one for the MFD and 2 child nodes (for the display controller and the PWM).
Hi Lee,
On Wed, 15 Aug 2018 06:24:35 +0100 Lee Jones lee.jones@linaro.org wrote:
+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.
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.
Regards,
Boris
On Fri, 24 Aug 2018, Boris Brezillon wrote:
Hi Lee,
On Wed, 15 Aug 2018 06:24:35 +0100 Lee Jones lee.jones@linaro.org wrote:
+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.
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 was erring towards that latter.
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.
Yes separation is good a good thing. Not placing them in MFD and having the individual parts reside in the appropriate subsystems are not mutually exclusive though. I assume the Display Controller is higher ranking than the PWM device right? Seeing as they are so closely bound i.e. the DC can't operated with the PWM, it would be justifiable to register the PWM from the DC.
Of course if there is complicated set-up to be done, lots of devices are involved or there are shared functions between them, then that is where MFD usually steps in.
However, since there is a very similar device already in MFD, I suggest you simply extend it to add support for this new device and have done.
Hi Sam,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on at91/at91-next] [also build test WARNING on v4.18 next-20180814] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sam-Ravnborg/add-at91sam9-LCDC-DRM-... base: https://git.kernel.org/pub/scm/linux/kernel/git/nferre/linux-at91.git at91-next config: x86_64-randconfig-g0-08151425 (attached as .config) compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4 reproduce: # save the attached .config to linux build tree make ARCH=x86_64
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:15:0, from include/linux/list.h:9, from include/linux/regmap.h:16, from include/linux/mfd/atmel-lcdc.h:11, from drivers/mfd/atmel-lcdc.c:13: drivers/mfd/atmel-lcdc.c: In function 'lcdc_probe':
include/linux/build_bug.h:29:45: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long unsigned int' [-Wformat=]
#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); })) ^ include/linux/compiler-gcc.h:65:28: note: in expansion of macro 'BUILD_BUG_ON_ZERO' #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) ^ include/linux/kernel.h:72:59: note: in expansion of macro '__must_be_array' #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) ^
drivers/mfd/atmel-lcdc.c:128:4: note: in expansion of macro 'ARRAY_SIZE'
ARRAY_SIZE(lcdc_cells), ret); ^ -- In file included from include/linux/kernel.h:15:0, from include/linux/list.h:9, from include/linux/regmap.h:16, from include/linux/mfd/atmel-lcdc.h:11, from drivers//mfd/atmel-lcdc.c:13: drivers//mfd/atmel-lcdc.c: In function 'lcdc_probe':
include/linux/build_bug.h:29:45: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long unsigned int' [-Wformat=]
#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); })) ^ include/linux/compiler-gcc.h:65:28: note: in expansion of macro 'BUILD_BUG_ON_ZERO' #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) ^ include/linux/kernel.h:72:59: note: in expansion of macro '__must_be_array' #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) ^ drivers//mfd/atmel-lcdc.c:128:4: note: in expansion of macro 'ARRAY_SIZE' ARRAY_SIZE(lcdc_cells), ret); ^
vim +/ARRAY_SIZE +128 drivers/mfd/atmel-lcdc.c
66 67 static int lcdc_probe(struct platform_device *pdev) 68 { 69 struct atmel_mfd_lcdc *lcdc; 70 struct lcdc_regmap *regmap; 71 struct resource *res; 72 struct device *dev; 73 int ret; 74 75 dev = &pdev->dev; 76 77 regmap = devm_kzalloc(dev, sizeof(*regmap), GFP_KERNEL); 78 if (!regmap) 79 return -ENOMEM; 80 81 lcdc = devm_kzalloc(dev, sizeof(*lcdc), GFP_KERNEL); 82 if (!lcdc) 83 return -ENOMEM; 84 85 res = platform_get_resource(pdev, IORESOURCE_MEM, 0); 86 regmap->regs = devm_ioremap_resource(dev, res); 87 if (IS_ERR(regmap->regs)) { 88 dev_err(dev, "Failed to allocate IO mem (%ld)\n", 89 PTR_ERR(regmap->regs)); 90 return PTR_ERR(regmap->regs); 91 } 92 93 lcdc->irq = platform_get_irq(pdev, 0); 94 if (lcdc->irq < 0) { 95 dev_err(dev, "Failed to get irq (%d)\n", lcdc->irq); 96 return lcdc->irq; 97 } 98 99 lcdc->lcdc_clk = devm_clk_get(dev, "lcdc_clk"); 100 if (IS_ERR(lcdc->lcdc_clk)) { 101 dev_err(dev, "failed to get lcdc clock (%ld)\n", 102 PTR_ERR(lcdc->lcdc_clk)); 103 return PTR_ERR(lcdc->lcdc_clk); 104 } 105 106 lcdc->bus_clk = devm_clk_get(dev, "hclk"); 107 if (IS_ERR(lcdc->bus_clk)) { 108 dev_err(dev, "failed to get bus clock (%ld)\n", 109 PTR_ERR(lcdc->bus_clk)); 110 return PTR_ERR(lcdc->bus_clk); 111 } 112 113 lcdc->regmap = devm_regmap_init(dev, NULL, regmap, 114 &lcdc_regmap_config); 115 if (IS_ERR(lcdc->regmap)) { 116 dev_err(dev, "Failed to init regmap (%ld)\n", 117 PTR_ERR(lcdc->regmap)); 118 return PTR_ERR(lcdc->regmap); 119 } 120 121 dev_set_drvdata(dev, lcdc); 122 123 ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, 124 lcdc_cells, ARRAY_SIZE(lcdc_cells), 125 NULL, 0, NULL); 126 if (ret < 0) 127 dev_err(dev, "Failed to add %d mfd devices (%d)\n",
128 ARRAY_SIZE(lcdc_cells), ret);
129 130 return ret; 131 } 132
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sun, 12 Aug 2018 20:46:25 +0200 Sam Ravnborg sam@ravnborg.org wrote:
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.
Looks like it's (almost?) the same logic. It's probably better to have only one driver and just add new compatibles.
The Atmel SOC's are at91sam9261, at91sam9263 etc.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Lee Jones lee.jones@linaro.org Cc: Boris Brezillon boris.brezillon@free-electrons.com
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
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index b860eb5aa194..f4851f0f033f 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -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 diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index e9fd20dba18d..dba8465e0d96 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -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 diff --git a/drivers/mfd/atmel-lcdc.c b/drivers/mfd/atmel-lcdc.c new file mode 100644 index 000000000000..8928976bafca --- /dev/null +++ b/drivers/mfd/atmel-lcdc.c @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (C) 2018 Sam Ravnborg
- Author: Sam Ravnborg sam@ravnborg.org
- Based on atmel-hlcdc.c wich is:
- Copyright (C) 2014 Free Electrons
- Copyright (C) 2014 Atmel
- Author: Boris BREZILLON boris.brezillon@free-electrons.com
- */
+#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 sam@ravnborg.org"); +MODULE_DESCRIPTION("Atmel LCDC mfd driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/mfd/atmel-lcdc.h b/include/linux/mfd/atmel-lcdc.h new file mode 100644 index 000000000000..fdab269baa8e --- /dev/null +++ b/include/linux/mfd/atmel-lcdc.h @@ -0,0 +1,184 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright (C) 2018 Sam Ravnborg
- Author: Sam Ravnborg sam@ravnborg.org
- */
+#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 */
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 pwm binding and refer back to the mfd device that this must be a child node of.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Boris Brezillon boris.brezillon@free-electrons.com --- .../devicetree/bindings/pwm/atmel-lcdc-pwm.txt | 30 ++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/atmel-lcdc-pwm.txt
diff --git a/Documentation/devicetree/bindings/pwm/atmel-lcdc-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-lcdc-pwm.txt new file mode 100644 index 000000000000..a7f11ac6972a --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/atmel-lcdc-pwm.txt @@ -0,0 +1,30 @@ +Device-Tree bindings for Atmel's LCDC (LCD Controller) PWM driver + +The Atmel LCDC PWM is subdevice of the LCDC MFD device. +See ../mfd/atmel-lcdc.txt for more details. + +Required properties: + - compatible: value should be one of the following: + "atmel,lcdc-pwm" + - pinctr-names: the pin control state names. Should contain "default". + - pinctrl-0: should contain the pinctrl states described by pinctrl + default. + - #pwm-cells: should be set to 3. This PWM chip use the default 3 cells + bindings defined in pwm.txt in this directory. + +Example: + lcdc0: lcdc@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_pwm: lcdc-pwm { + compatible = "atmel,lcdc-pwm"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_lcdc_pwm>; + #pwm-cells = <3>; + }; + + };
The LCDC IP used by some Atmel SOC's have a multifunction device that include two sub-devices: - pwm - display controller
This driver add support for the pwm sub-device exposing a single PWM device that can be used for backlight.
The driver is based on the pwm-atmel-hlcdc driver.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Boris Brezillon boris.brezillon@free-electrons.com --- drivers/pwm/Kconfig | 13 ++++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-atmel-lcdc.c | 178 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 192 insertions(+) create mode 100644 drivers/pwm/pwm-atmel-lcdc.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index a4d262db9945..3976c1840778 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -62,6 +62,19 @@ config PWM_ATMEL_HLCDC_PWM To compile this driver as a module, choose M here: the module will be called pwm-atmel-hlcdc.
+config PWM_ATMEL_LCDC_PWM + tristate "Atmel LCDC PWM support" + depends on MFD_ATMEL_LCDC + depends on HAVE_CLK + help + Generic PWM framework driver for the PWM output of the LCDC + (Atmel LCD Controller). This PWM output is mainly used + to control the LCD backlight. + The Atmel LCD Controller is found in the at91sam9x family. + + To compile this driver as a module, choose M here: the module + will be called pwm-atmel-lcdc. + config PWM_ATMEL_TCB tristate "Atmel TC Block PWM support" depends on ATMEL_TCLIB && OF diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 9c676a0dadf5..85d884cb0977 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -4,6 +4,7 @@ obj-$(CONFIG_PWM_SYSFS) += sysfs.o obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o +obj-$(CONFIG_PWM_ATMEL_LCDC_PWM) += pwm-atmel-lcdc.o obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o obj-$(CONFIG_PWM_BCM_IPROC) += pwm-bcm-iproc.o obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o diff --git a/drivers/pwm/pwm-atmel-lcdc.c b/drivers/pwm/pwm-atmel-lcdc.c new file mode 100644 index 000000000000..3d528e75cf9f --- /dev/null +++ b/drivers/pwm/pwm-atmel-lcdc.c @@ -0,0 +1,178 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2018 Sam Ravnborg + * + * Author: Sam Ravnborg sam@ravnborg.org + * + * PWM embedded in the LCD Controller. + * A sub-device of the Atmel LCDC driver. + * + * Based on pwm-atmel-hlcdc which is: + * Copyright (C) 2014 Free Electrons + * Copyright (C) 2014 Atmel + * Author: Boris BREZILLON boris.brezillon@free-electrons.com + */ + +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/clk.h> +#include <linux/pwm.h> + +#include <linux/platform_device.h> +#include <linux/mfd/atmel-lcdc.h> + +struct lcdc_pwm { + struct pwm_chip chip; + struct atmel_mfd_lcdc *mfd_lcdc; +}; + +static inline struct lcdc_pwm *to_lcdc_pwm(struct pwm_chip *chip) +{ + return container_of(chip, struct lcdc_pwm, chip); +} + +static int lcdc_pwm_apply(struct pwm_chip *pwm_chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct lcdc_pwm *chip; + int contrast_ctr; + int contrast_val; + int ret; + + chip = to_lcdc_pwm(pwm_chip); + + if (state->enabled) { + contrast_val = pwm_get_relative_duty_cycle(state, + ATMEL_LCDC_CVAL); + ret = regmap_write(chip->mfd_lcdc->regmap, + ATMEL_LCDC_CONTRAST_VAL, contrast_val); + if (ret) + return ret; + + contrast_ctr = ATMEL_LCDC_ENA_PWMENABLE | ATMEL_LCDC_PS_DIV8; + if (state->polarity == PWM_POLARITY_NORMAL) + contrast_ctr |= ATMEL_LCDC_POL_POSITIVE; + else + contrast_ctr |= ATMEL_LCDC_POL_NEGATIVE; + + ret = regmap_write(chip->mfd_lcdc->regmap, + ATMEL_LCDC_CONTRAST_CTR, contrast_ctr); + } else { + contrast_ctr = ATMEL_LCDC_ENA_PWMDISABLE; + ret = regmap_write(chip->mfd_lcdc->regmap, + ATMEL_LCDC_CONTRAST_CTR, contrast_ctr); + } + + return ret; +} + +static const struct pwm_ops lcdc_pwm_ops = { + .apply = lcdc_pwm_apply, + .owner = THIS_MODULE, +}; + +#ifdef CONFIG_PM_SLEEP +static int lcdc_pwm_suspend(struct device *dev) +{ + struct lcdc_pwm *chip = dev_get_drvdata(dev); + + /* Keep the lcdc clock enabled if the PWM is still running. */ + if (pwm_is_enabled(&chip->chip.pwms[0])) + clk_disable_unprepare(chip->mfd_lcdc->lcdc_clk); + + return 0; +} + +static int lcdc_pwm_resume(struct device *dev) +{ + struct lcdc_pwm *chip = dev_get_drvdata(dev); + struct pwm_state state; + int ret; + + pwm_get_state(&chip->chip.pwms[0], &state); + + /* Re-enable the lcdc clock if it was stopped during suspend. */ + if (!state.enabled) { + ret = clk_prepare_enable(chip->mfd_lcdc->lcdc_clk); + if (ret) + return ret; + } + + return lcdc_pwm_apply(&chip->chip, &chip->chip.pwms[0], &state); +} +#endif + +static SIMPLE_DEV_PM_OPS(lcdc_pwm_pm_ops, + lcdc_pwm_suspend, lcdc_pwm_resume); + +static int lcdc_pwm_probe(struct platform_device *pdev) +{ + struct atmel_mfd_lcdc *mfd_lcdc; + struct lcdc_pwm *chip; + struct device *dev; + int ret; + + dev = &pdev->dev; + mfd_lcdc = dev_get_drvdata(dev->parent); + + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + ret = clk_prepare_enable(mfd_lcdc->lcdc_clk); + if (ret) + return ret; + + chip->mfd_lcdc = mfd_lcdc; + chip->chip.ops = &lcdc_pwm_ops; + chip->chip.dev = dev; + chip->chip.base = -1; + chip->chip.npwm = 1; + chip->chip.of_xlate = of_pwm_xlate_with_flags; + chip->chip.of_pwm_n_cells = 3; + + ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED); + if (ret) { + clk_disable_unprepare(mfd_lcdc->lcdc_clk); + return ret; + } + + platform_set_drvdata(pdev, chip); + + return 0; +} + +static int lcdc_pwm_remove(struct platform_device *pdev) +{ + struct lcdc_pwm *chip = platform_get_drvdata(pdev); + int ret; + + ret = pwmchip_remove(&chip->chip); + if (ret) + return ret; + + clk_disable_unprepare(chip->mfd_lcdc->lcdc_clk); + + return 0; +} + +static const struct of_device_id lcdc_pwm_dt_ids[] = { + { .compatible = "atmel,lcdc-pwm" }, + { /* sentinel */ }, +}; + +static struct platform_driver lcdc_pwm_driver = { + .driver = { + .name = "atmel-lcdc-pwm", + .of_match_table = lcdc_pwm_dt_ids, + .pm = &lcdc_pwm_pm_ops, + }, + .probe = lcdc_pwm_probe, + .remove = lcdc_pwm_remove, +}; +module_platform_driver(lcdc_pwm_driver); + +MODULE_ALIAS("platform:pwm-atmel-lcdc"); +MODULE_AUTHOR("Sam Ravnborg sam@ravnborg.org"); +MODULE_DESCRIPTION("Atmel LCDC PWM driver"); +MODULE_LICENSE("GPL v2");
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 lcdc display controller and refer back to the mfd device that this must be a child node of.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Boris Brezillon boris.brezillon@free-electrons.com --- .../display/atmel/lcdc-display-controller.txt | 40 ++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/atmel/lcdc-display-controller.txt
diff --git a/Documentation/devicetree/bindings/display/atmel/lcdc-display-controller.txt b/Documentation/devicetree/bindings/display/atmel/lcdc-display-controller.txt new file mode 100644 index 000000000000..508a49b5d8b7 --- /dev/null +++ b/Documentation/devicetree/bindings/display/atmel/lcdc-display-controller.txt @@ -0,0 +1,40 @@ +Atmel LCD Controller (LCDC) Display Controller + +Required properties: + - compatible: value should be one of the following: + "atmel,lcdc-display-controller" + +Optional properties: +- lcd-supply: phandle to a regulator that supply the display + +Required children nodes: + One child node that references the panel connected. + (See ../graph.txt) + At least one port node is required. + + +Example: + lcdc0: lcdc@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@0 { + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + lcdc_panel_output: endpoint@0 { + reg = <0>; + remote-endpoint = <&panel_input>; + }; + }; + }; + };
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 sam@ravnborg.org Cc: Nicolas Ferre nicolas.ferre@atmel.com Cc: Boris Brezillon boris.brezillon@free-electrons.com Cc: Alexandre Belloni alexandre.belloni@bootlin.com --- 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
diff --git a/MAINTAINERS b/MAINTAINERS index 09ce76a9a1dc..0a594d02a7c0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4685,6 +4685,13 @@ F: drivers/gpu/drm/atmel/atmel-hlcdc* F: Documentation/devicetree/bindings/display/atmel/ T: git git://anongit.freedesktop.org/drm/drm-misc
+DRM DRIVERS FOR ATMEL LCDC +M: Sam Ravnborg sam@ravnborg.org +L: dri-devel@lists.freedesktop.org +S: Maintained +F: drivers/gpu/drm/atmel/atmel-lcdc* +F: Documentation/devicetree/bindings/display/atmel/ + DRM DRIVERS FOR BRIDGE CHIPS M: Archit Taneja architt@codeaurora.org M: Andrzej Hajda a.hajda@samsung.com diff --git a/drivers/gpu/drm/atmel/Kconfig b/drivers/gpu/drm/atmel/Kconfig index 7cd3862f9d18..c4c6150f271a 100644 --- a/drivers/gpu/drm/atmel/Kconfig +++ b/drivers/gpu/drm/atmel/Kconfig @@ -14,3 +14,15 @@ config DRM_ATMEL_HLCDC help Choose this option if you have an ATMEL SoC with an HLCDC display controller (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family). + +config DRM_ATMEL_LCDC + tristate "DRM Support for ATMEL LCDC Display Controller" + depends on DRM && OF && COMMON_CLK && MFD_ATMEL_LCDC && ARM + select DRM_ATMEL + select DRM_GEM_CMA_HELPER + select DRM_KMS_HELPER + select DRM_KMS_CMA_HELPER + select DRM_PANEL + help + Choose this option if you have an ATMEL SoC with an LCDC display + controller (only at91sam9263 or at91sam9rl). diff --git a/drivers/gpu/drm/atmel/Makefile b/drivers/gpu/drm/atmel/Makefile index 49dc89f36b73..9fdfada613d2 100644 --- a/drivers/gpu/drm/atmel/Makefile +++ b/drivers/gpu/drm/atmel/Makefile @@ -5,3 +5,5 @@ atmel-hlcdc-dc-y := atmel_hlcdc_crtc.o \ atmel_hlcdc_plane.o
obj-$(CONFIG_DRM_ATMEL_HLCDC) += atmel-hlcdc-dc.o + +obj-$(CONFIG_DRM_ATMEL_LCDC) += atmel_lcdc-dc.o diff --git a/drivers/gpu/drm/atmel/atmel_lcdc-dc.c b/drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file mode 100644 index 000000000000..275a09e2e100 --- /dev/null +++ b/drivers/gpu/drm/atmel/atmel_lcdc-dc.c @@ -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; + bool have_hozval; + bool have_alt_pixclock; +}; + +/* private data */ +struct lcdc_dc { + const struct atmel_lcdc_dc_desc *desc; + struct atmel_mfd_lcdc *mfd_lcdc; + struct regulator *lcd_supply; + struct drm_fbdev_cma *fbdev; + struct drm_bridge *bridge; + struct drm_panel *panel; + struct regmap *regmap; + struct device *dev; + + struct drm_simple_display_pipe pipe; + struct work_struct reset_lcdc_work; + struct drm_connector connector; +}; + +/* 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, + .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, +}; + +static const struct of_device_id atmel_lcdc_of_match[] = { + { + .compatible = "atmel,at91sam9261-lcdc-mfd", + .data = &atmel_lcdc_dc_at91sam9261, + }, { + .compatible = "atmel,at91sam9263-lcdc-mfd", + .data = &atmel_lcdc_dc_at91sam9263, + }, { + .compatible = "atmel,at91sam9g10-lcdc-mfd", + .data = &atmel_lcdc_dc_at91sam9g10, + }, { + .compatible = "atmel,at91sam9g45-lcdc-mfd", + .data = &atmel_lcdc_dc_at91sam9g45, + }, { + .compatible = "atmel,at91sam9g46-lcdc-mfd", + .data = &atmel_lcdc_dc_at91sam9g46, + }, { + .compatible = "atmel,at91sam9m10-lcdc-mfd", + .data = &atmel_lcdc_dc_at91sam9m10, + }, { + .compatible = "atmel,at91sam9m11-lcdc-mfd", + .data = &atmel_lcdc_dc_at91sam9m11, + }, { + .compatible = "atmel,at91sam9rl-lcdc-mfd", + .data = &atmel_lcdc_dc_at91sam9rl, + }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, atmel_lcdc_of_match); + +/* + * 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 + * Maybe actual wiring will impact mode support? + */ +static const u32 lcdc_dc_formats[] = { + DRM_FORMAT_BGR565, +}; + +/* Start LCD Controller (DMA + PWR) */ +static void lcdc_dc_start(struct lcdc_dc *lcdc_dc) +{ + // Enable DMA + 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); +} + +/* Stop LCD Controller (PWR + DMA) */ +static void lcdc_dc_stop(struct lcdc_dc *lcdc_dc) +{ + unsigned int pwrcon; + + might_sleep(); + + /* Turn off the LCD controller and the DMA controller */ + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_PWRCON, + lcdc_dc->desc->guard_time << ATMEL_LCDC_GUARDT_OFFSET); + + /* Wait for the LCDC core to become idle */ + regmap_read_poll_timeout(lcdc_dc->regmap, ATMEL_LCDC_PWRCON, pwrcon, + !(pwrcon & ATMEL_LCDC_BUSY), 100, 10000); + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_DMACON, !(ATMEL_LCDC_DMAEN)); +} + +static void lcdc_dc_start_clock(struct lcdc_dc *lcdc_dc) +{ + clk_prepare_enable(lcdc_dc->mfd_lcdc->bus_clk); + clk_prepare_enable(lcdc_dc->mfd_lcdc->lcdc_clk); +} + +static void lcdc_dc_stop_clock(struct lcdc_dc *lcdc_dc) +{ + clk_disable_unprepare(lcdc_dc->mfd_lcdc->bus_clk); + clk_disable_unprepare(lcdc_dc->mfd_lcdc->lcdc_clk); +} + +static int lcdc_dc_display_check(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *pstate, + struct drm_crtc_state *cstate) +{ + const struct drm_display_mode *dmode; + struct drm_framebuffer *old_fb; + struct drm_framebuffer *fb; + + dmode = &cstate->mode; + old_fb = pipe->plane.state->fb; + fb = pstate->fb; + + /* Check timing? */ + /* TODO */ + + return 0; +} + +/* Horizontal size of LCD module - configuration dependent */ +static unsigned int compute_hozval(struct lcdc_dc *lcdc_dc, unsigned int width) +{ + unsigned int valid_lcdd_data_line; + unsigned int hoz_display_size; + unsigned int disptype; + unsigned int scanmode; + unsigned int ifwidth; + unsigned int lcdcon2; + + if (!lcdc_dc->desc->have_hozval) + return width; + + regmap_read(lcdc_dc->regmap, ATMEL_LCDC_LCDCON2, &lcdcon2); + disptype = lcdcon2 & ATMEL_LCDC_DISTYPE; + + if (disptype == ATMEL_LCDC_DISTYPE_TFT) + return width; + + ifwidth = lcdcon2 & ATMEL_LCDC_IFWIDTH; + scanmode = lcdcon2 & ATMEL_LCDC_SCANMOD; + + /* + * STN display + * Based on algorithm from datasheet calculate hozval + */ + if (disptype == ATMEL_LCDC_DISTYPE_STNCOLOR) + hoz_display_size = width * 3; + else + hoz_display_size = width; + + switch (ifwidth) { + case ATMEL_LCDC_IFWIDTH_4: + valid_lcdd_data_line = 4; + break; + case ATMEL_LCDC_IFWIDTH_8: + if (scanmode == ATMEL_LCDC_SCANMOD_DUAL) + valid_lcdd_data_line = 4; + else + valid_lcdd_data_line = 8; + break; + default: + valid_lcdd_data_line = 8; + break; + } + + return DIV_ROUND_UP(hoz_display_size, valid_lcdd_data_line); +} + +static void set_vertical_timing(struct lcdc_dc *lcdc_dc, + struct drm_display_mode *dmode) +{ + unsigned int tim1; + unsigned int vfp; + unsigned int vbp; + unsigned int vpw; + unsigned int vhdly; + + /* VFP: Vertical Front Porch */ + vfp = dmode->vsync_start - dmode->vdisplay; + + /* VBP: Vertical Back Porch */ + vbp = dmode->vtotal - dmode->vsync_end; + + /* VPW: Vertical Synchronization pulse width */ + vpw = dmode->vsync_end - dmode->vsync_start - 1; + + /* VHDLY: Vertical to horizontal delay */ + vhdly = 0; + + tim1 = vfp << ATMEL_LCDC_VFP_OFFSET | + vbp << ATMEL_LCDC_VBP_OFFSET | + vpw << ATMEL_LCDC_VPW_OFFSET | + vhdly << ATMEL_LCDC_VHDLY_OFFSET; + + DRM_DEV_DEBUG(lcdc_dc->dev, " TIM1 = %08x\n", tim1); + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_TIM1, tim1); +} + +static void set_horizontal_timing(struct lcdc_dc *lcdc_dc, + struct drm_display_mode *dmode) +{ + unsigned int tim2; + unsigned int hbp; + unsigned int hpw; + unsigned int hfp; + + /* HBP: Horizontal Back Porch */ + hbp = dmode->htotal - dmode->hsync_end - 1; + + /* HPW: Horizontal synchronization pulse width */ + hpw = dmode->hsync_end - dmode->hsync_start - 1; + + /* HFP: Horizontal Front Porch */ + hfp = dmode->hsync_start - dmode->hdisplay - 2; + + tim2 = hbp << ATMEL_LCDC_HBP_OFFSET | + hpw << ATMEL_LCDC_HPW_OFFSET | + hfp << ATMEL_LCDC_HFP_OFFSET; + + DRM_DEV_DEBUG(lcdc_dc->dev, " TIM2 = %08x\n", tim2); + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_TIM2, tim2); +} + +static void lcdc_dc_crtc_mode_set_nofb(struct lcdc_dc *lcdc_dc) +{ + struct drm_display_mode *dmode; + unsigned int hozval_linesz; + unsigned int value; + + dmode = &lcdc_dc->pipe.crtc.state->adjusted_mode; + + /* Vertical & horizontal timing */ + set_vertical_timing(lcdc_dc, dmode); + set_horizontal_timing(lcdc_dc, dmode); + + /* Horizontal value (aka line size) */ + hozval_linesz = compute_hozval(lcdc_dc, dmode->crtc_hdisplay); + + /* Display size */ + value = (hozval_linesz - 1) << ATMEL_LCDC_HOZVAL_OFFSET; + value |= dmode->crtc_vdisplay - 1; + DRM_DEV_DEBUG(lcdc_dc->dev, " LCDFRMCFG = %08x\n", value); + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_LCDFRMCFG, value); + + /* FIFO Threshold: Use formula from data sheet */ + value = lcdc_dc->desc->fifo_size - (2 * ATMEL_LCDC_DMA_BURST_LEN + 3); + DRM_DEV_DEBUG(lcdc_dc->dev, " FIFO = %08x\n", value); + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_FIFO, value); + + /* + * Toggle LCD_MODE every frame + * Note: register not documented, this is from atmel_lcdfb + */ + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_MVAL, 0); +} + +static void lcdc_dc_enable(struct lcdc_dc *lcdc_dc, + struct drm_device *drm, + struct drm_crtc *crtc) +{ + const struct drm_format_info *format; + struct drm_display_mode *dmode; + unsigned long clk_value_khz; + unsigned int pix_factor; + unsigned int lcdcon1; + unsigned int lcdcon2; + u32 bus_flags; + + dmode = &lcdc_dc->pipe.crtc.state->adjusted_mode; + format = crtc->primary->state->fb->format; + + /* Control register 1 */ + + /* Set pixel clock */ + if (lcdc_dc->desc->have_alt_pixclock) + pix_factor = 1; + else + pix_factor = 2; + + clk_value_khz = clk_get_rate(lcdc_dc->mfd_lcdc->lcdc_clk) / 1000; + lcdcon1 = DIV_ROUND_UP(clk_value_khz, dmode->clock); + + if (lcdcon1 < pix_factor) { + DRM_DEV_INFO(lcdc_dc->dev, "Bypassing pixel clock divider\n"); + regmap_write(lcdc_dc->regmap, + ATMEL_LCDC_LCDCON1, ATMEL_LCDC_BYPASS); + } else { + + lcdcon1 = (lcdcon1 / pix_factor) - 1; + DRM_DEV_DEBUG(lcdc_dc->dev, "CLKVAL = 0x%08x\n", lcdcon1); + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_LCDCON1, + lcdcon1 << ATMEL_LCDC_CLKVAL_OFFSET); + dmode->clock = clk_value_khz / (pix_factor * (lcdcon1 + 1)); + DRM_DEV_DEBUG(lcdc_dc->dev, "updated pixclk: %u KHz\n", + dmode->clock); + } + + /* Control register 2 */ + /* Only TFT supported (Controller supports STN too) */ + lcdcon2 = ATMEL_LCDC_DISTYPE_TFT; + + /* scan mode (STN only) */ + if (dmode->flags & DRM_MODE_FLAG_DBLSCAN) + lcdcon2 |= ATMEL_LCDC_SCANMOD_DUAL; + else + lcdcon2 |= ATMEL_LCDC_SCANMOD_SINGLE; + + /* Interface width 4 bits (STN only) */ + lcdcon2 |= ATMEL_LCDC_IFWIDTH_4; + + /* bits per pixel */ + switch (format->depth) { + case 1: + lcdcon2 |= ATMEL_LCDC_PIXELSIZE_1; + break; + case 2: + lcdcon2 |= ATMEL_LCDC_PIXELSIZE_2; + break; + case 4: + lcdcon2 |= ATMEL_LCDC_PIXELSIZE_4; + break; + case 8: + lcdcon2 |= ATMEL_LCDC_PIXELSIZE_8; + break; + case 15: + /* fall through */ + case 16: + lcdcon2 |= ATMEL_LCDC_PIXELSIZE_16; + break; + case 24: + lcdcon2 |= ATMEL_LCDC_PIXELSIZE_24; + break; + case 32: + lcdcon2 |= ATMEL_LCDC_PIXELSIZE_32; + break; + default: + DRM_DEV_ERROR(lcdc_dc->dev, "Unexpected depth (%d)", + format->depth); + break; + } + + /* Polarity normal */ + lcdcon2 |= ATMEL_LCDC_INVVD_NORMAL; + + /* vsync polarity */ + if (dmode->flags & DRM_MODE_FLAG_PVSYNC) + lcdcon2 |= ATMEL_LCDC_INVFRAME_INVERTED; + else + lcdcon2 |= ATMEL_LCDC_INVFRAME_NORMAL; + + /* hsync polarity */ + if (dmode->flags & DRM_MODE_FLAG_PHSYNC) + lcdcon2 |= ATMEL_LCDC_INVLINE_INVERTED; + else + lcdcon2 |= ATMEL_LCDC_INVLINE_NORMAL; + + bus_flags = lcdc_dc->connector.display_info.bus_flags; + + /* dot clock (pix clock) polarity */ + if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) + lcdcon2 |= ATMEL_LCDC_INVCLK_INVERTED; + else + lcdcon2 |= ATMEL_LCDC_INVCLK_NORMAL; + + /* Date Enable polarity */ + if (bus_flags & DRM_BUS_FLAG_DE_LOW) + lcdcon2 |= ATMEL_LCDC_INVDVAL_INVERTED; + else + lcdcon2 |= ATMEL_LCDC_INVDVAL_NORMAL; + + /* Clock is always active */ + lcdcon2 |= ATMEL_LCDC_CLKMOD_ALWAYSACTIVE; + + /* Memory layout */ + if (format->format & DRM_FORMAT_BIG_ENDIAN) + lcdcon2 |= ATMEL_LCDC_MEMOR_BIG; + else + lcdcon2 |= ATMEL_LCDC_MEMOR_LITTLE; + + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_LCDCON2, lcdcon2); + + lcdc_dc_start(lcdc_dc); + + drm_crtc_vblank_on(crtc); +} + +static void lcdc_dc_display_enable(struct drm_simple_display_pipe *pipe, + struct drm_crtc_state *cstate, + struct drm_plane_state *plane_state) +{ + struct lcdc_dc *lcdc_dc; + struct drm_device *drm; + struct drm_crtc *crtc; + int ret; + + crtc = &pipe->crtc; + drm = crtc->dev; + lcdc_dc = drm->dev_private; + + ret = regulator_enable(lcdc_dc->lcd_supply); + if (ret) + DRM_DEV_ERROR(lcdc_dc->dev, "regulator_enable failed (%d)", + ret); + + drm_panel_prepare(lcdc_dc->panel); + lcdc_dc_crtc_mode_set_nofb(lcdc_dc); + + /* + * drm_simple_kms_helper have no support for gamma setup. + * Add: lcdc_dc_setcolor(lcdc_dc, crtc); when we have it + */ + lcdc_dc_enable(lcdc_dc, drm, crtc); + + drm_panel_enable(lcdc_dc->panel); +} + +static void lcdc_dc_display_disable(struct drm_simple_display_pipe *pipe) +{ + struct lcdc_dc *lcdc_dc; + struct drm_device *drm; + struct drm_crtc *crtc; + + crtc = &pipe->crtc; + drm = crtc->dev; + lcdc_dc = drm->dev_private; + + drm_crtc_vblank_off(crtc); + + drm_panel_disable(lcdc_dc->panel); + + lcdc_dc_stop(lcdc_dc); + + drm_panel_unprepare(lcdc_dc->panel); + regulator_disable(lcdc_dc->lcd_supply); +} + +/* Update DMA config */ +static void lcdc_dc_update_dma(struct lcdc_dc *lcdc_dc, + struct drm_simple_display_pipe *pipe) +{ + struct drm_plane_state *plane_state; + struct drm_framebuffer *fb; + unsigned int burst_length; + unsigned int frame_size; + dma_addr_t dma_addr; + + plane_state = pipe->plane.state; + fb = plane_state->fb; + + if (fb) { + unsigned int dmafrmcfg; + + dma_addr = drm_fb_cma_get_gem_addr(fb, pipe->plane.state, 0); + + /* Set frame buffer DMA base address */ + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_DMABADDR1, dma_addr); + + /* + * Set frame size and burst length + * Frame_size equals size of visible area * bits / 32 + * (size in 32 bit words) + */ + frame_size = plane_state->crtc_w * plane_state->crtc_h; + frame_size = (frame_size * fb->format->depth) / 32; + + burst_length = ATMEL_LCDC_DMA_BURST_LEN - 1; + + dmafrmcfg = frame_size; + dmafrmcfg |= burst_length << ATMEL_LCDC_BLENGTH_OFFSET; + + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_DMAFRMCFG, dmafrmcfg); + } +} + +static void lcdc_dc_update_event(struct drm_simple_display_pipe *pipe) +{ + struct drm_pending_vblank_event *event; + struct drm_crtc *crtc; + + crtc = &pipe->crtc; + if (!crtc) + return; + + spin_lock_irq(&crtc->dev->event_lock); + event = crtc->state->event; + if (event) { + crtc->state->event = NULL; + + if (drm_crtc_vblank_get(crtc) == 0) + drm_crtc_arm_vblank_event(crtc, event); + else + drm_crtc_send_vblank_event(crtc, event); + } + spin_unlock_irq(&crtc->dev->event_lock); +} + +static void lcdc_dc_display_update(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *old_pstate) +{ + struct drm_device *drm; + struct lcdc_dc *lcdc_dc; + + drm = pipe->crtc.dev; + lcdc_dc = drm->dev_private; + + /* Re-initialize the DMA engine... */ + lcdc_dc_update_dma(lcdc_dc, pipe); + + /* vblank event handling */ + lcdc_dc_update_event(pipe); +} + +static int lcdc_dc_display_enable_vblank(struct drm_simple_display_pipe *pipe) +{ + struct drm_crtc *crtc; + struct lcdc_dc *lcdc_dc; + + crtc = &pipe->crtc; + lcdc_dc = crtc->dev->dev_private; + + /* Last line interrupt enable */ + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_IER, ATMEL_LCDC_LSTLNI); + + return 0; +} + +static void lcdc_dc_display_disable_vblank(struct drm_simple_display_pipe *pipe) +{ + struct drm_crtc *crtc; + struct lcdc_dc *lcdc_dc; + + crtc = &pipe->crtc; + lcdc_dc = crtc->dev->dev_private; + + /* Last line interrupt disable */ + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_IDR, ATMEL_LCDC_LSTLNI); +} + +static const struct drm_simple_display_pipe_funcs lcdc_dc_display_funcs = { + .check = lcdc_dc_display_check, + .enable = lcdc_dc_display_enable, + .disable = lcdc_dc_display_disable, + .update = lcdc_dc_display_update, + .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, + .enable_vblank = lcdc_dc_display_enable_vblank, + .disable_vblank = lcdc_dc_display_disable_vblank, +}; + +static const struct drm_mode_config_funcs mode_config_funcs = { + .fb_create = drm_gem_fb_create, + .atomic_check = drm_atomic_helper_check, + .atomic_commit = drm_atomic_helper_commit, +}; + +/* scheduled worker to reset LCD */ +static void reset_lcdc_work(struct work_struct *work) +{ + struct lcdc_dc *lcdc_dc; + + lcdc_dc = container_of(work, struct lcdc_dc, reset_lcdc_work); + + 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) + return IRQ_NONE; + + if (status & ATMEL_LCDC_LSTLNI) + drm_crtc_handle_vblank(&lcdc_dc->pipe.crtc); + + if (status & ATMEL_LCDC_UFLWI) { + DRM_DEV_INFO(dev, "FIFO underflow %#x\n", status); + /* reset DMA and FIFO to avoid screen shifting */ + schedule_work(&lcdc_dc->reset_lcdc_work); + } + 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; +} + +static int lcdc_dc_irq_postinstall(struct drm_device *dev) +{ + struct lcdc_dc *lcdc_dc; + unsigned int ier; + + lcdc_dc = dev->dev_private; + + ier = 0; + /* FIFO underflow interrupt enable */ + ier |= ATMEL_LCDC_UFLWI; + /* FIFO overwrite interrupt enable */ + ier |= ATMEL_LCDC_OWRI; + /* DMA memory error interrupt enable */ + ier |= ATMEL_LCDC_MERI; + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_IER, ier); + + return 0; +} + +static void lcdc_dc_irq_uninstall(struct drm_device *dev) +{ + struct lcdc_dc *lcdc_dc; + unsigned int isr; + + lcdc_dc = dev->dev_private; + + /* disable all interrupts */ + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_IDR, ~0); + regmap_write(lcdc_dc->regmap, ATMEL_LCDC_ICR, ~0); + + /* Clear any pending interrupts */ + regmap_read(lcdc_dc->regmap, ATMEL_LCDC_ISR, &isr); +} + +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 | + DRIVER_PRIME | + DRIVER_ATOMIC, + .name = "atmel-lcdc", + .desc = "Atmel LCD Display Controller DRM", + .date = "20180808", + .major = 1, + .minor = 0, + .patchlevel = 0, + + .lastclose = drm_fb_helper_lastclose, + .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@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) { + 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; + +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); + + cancel_work_sync(&lcdc_dc->reset_lcdc_work); + + return ret; +} + +static void lcdc_dc_unload(struct drm_device *dev) +{ + struct lcdc_dc *lcdc_dc = dev->dev_private; + + drm_fb_cma_fbdev_fini(dev); + 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 sam@ravnborg.org"); +MODULE_DESCRIPTION("Atmel LCDC Display Controller DRM Driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:atmel-lcdc-dc");
+Noralf
Hi Sam,
On Sun, 12 Aug 2018 20:46:29 +0200 Sam Ravnborg sam@ravnborg.org wrote:
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 sam@ravnborg.org Cc: Nicolas Ferre nicolas.ferre@atmel.com Cc: Boris Brezillon boris.brezillon@free-electrons.com Cc: Alexandre Belloni alexandre.belloni@bootlin.com
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
diff --git a/MAINTAINERS b/MAINTAINERS index 09ce76a9a1dc..0a594d02a7c0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4685,6 +4685,13 @@ F: drivers/gpu/drm/atmel/atmel-hlcdc* F: Documentation/devicetree/bindings/display/atmel/ T: git git://anongit.freedesktop.org/drm/drm-misc
+DRM DRIVERS FOR ATMEL LCDC +M: Sam Ravnborg sam@ravnborg.org +L: dri-devel@lists.freedesktop.org +S: Maintained +F: drivers/gpu/drm/atmel/atmel-lcdc* +F: Documentation/devicetree/bindings/display/atmel/
As stated in one of my previous replies, I think we should keep a single entry for both drivers, just update the existing one and we should be good.
DRM DRIVERS FOR BRIDGE CHIPS M: Archit Taneja architt@codeaurora.org M: Andrzej Hajda a.hajda@samsung.com
[...]
+/*
- 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
- Maybe actual wiring will impact mode support?
- */
+static const u32 lcdc_dc_formats[] = {
- DRM_FORMAT_BGR565,
+};
+/* Start LCD Controller (DMA + PWR) */ +static void lcdc_dc_start(struct lcdc_dc *lcdc_dc) +{
- // Enable DMA
Avoid using C++ style comments.
- 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)
+{
- const struct drm_display_mode *dmode;
- struct drm_framebuffer *old_fb;
- struct drm_framebuffer *fb;
- dmode = &cstate->mode;
- old_fb = pipe->plane.state->fb;
- fb = pstate->fb;
- /* Check timing? */
Did you look at what other simple DRM drivers check in this hook?
- /* TODO */
- return 0;
+}
+/* scheduled worker to reset LCD */ +static void reset_lcdc_work(struct work_struct *work) +{
- struct lcdc_dc *lcdc_dc;
- lcdc_dc = container_of(work, struct lcdc_dc, reset_lcdc_work);
Hm, you need a lock to protect this section and you have to check the LCDC state, because it might have been disabled through the ->disable() hook just after you have scheduled this reset operation.
- 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)
return IRQ_NONE;
- if (status & ATMEL_LCDC_LSTLNI)
drm_crtc_handle_vblank(&lcdc_dc->pipe.crtc);
- if (status & ATMEL_LCDC_UFLWI) {
DRM_DEV_INFO(dev, "FIFO underflow %#x\n", status);
/* reset DMA and FIFO to avoid screen shifting */
schedule_work(&lcdc_dc->reset_lcdc_work);
- }
Add a blank line here.
- 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;
+}
[...]
+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@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;
return ret;
- }
- bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);
drm_of_find_panel_or_bridge() might directly return a bridge. And maybe we should declare a DPI connector. And finally, you can might want to use devm_drm_panel_bridge_add() instead of drm_panel_bridge_add()
if (panel) bridge = devm_drm_panel_bridge_add(dev, panel, DRM_MODE_CONNECTOR_DPI);
- if (IS_ERR(bridge)) {
ret = PTR_ERR(bridge);
DRM_DEV_ERROR(dev, "Failed to add bridge (%d)", ret);
goto err_panel_remove;
Hm, if drm_panel_bridge_add() failed, you shouldn't call drm_panel_bridge_remove(bridge). You can just do
return ret;
- }
- 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;
If you use devm_drm_panel_bridge_add(), you can return directly.
- }
- 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;
Ditto.
- }
- drm_mode_config_reset(drm);
- return 0;
+err_panel_remove:
- if (panel)
drm_panel_bridge_remove(bridge);
+err_out:
- return ret;
And now you should be able to get rid of the error path entirely.
+}
+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);
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.
- 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);
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.
- 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;
+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);
- cancel_work_sync(&lcdc_dc->reset_lcdc_work);
Isn't there a race here? What if there was a work queued but you disabled the clk? I guess reg read/write accesses might fail (don't know how bad this can go though).
- return ret;
+}
+static void lcdc_dc_unload(struct drm_device *dev) +{
- struct lcdc_dc *lcdc_dc = dev->dev_private;
- drm_fb_cma_fbdev_fini(dev);
- flush_work(&lcdc_dc->reset_lcdc_work);
- drm_kms_helper_poll_fini(dev);
- if (lcdc_dc->panel)
drm_panel_bridge_remove(lcdc_dc->bridge);
You can drop that one if you use the devm_ version.
- 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);
And again, it might be too late. Should be moved just after disabling the IRQ, since this is the only path where you queue this 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;
+}
That's all I see for now, but keep in mind that I don't know much about the DRM simple interface, so you'd better wait for a review from someone who knows better (Noralf?).
Regards,
Boris
Hi Boris.
Very usefull feedback! I am working on v2, and have addressed your review items. Just a few comments below to a few items. The rest is processed/done.
I hope to post v2 in the week to come.
+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);
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.
As suggested I have introduced suspend/resume hooks and have included start/stop clock calls.
But as the suspend/resume hooks depends on CONFIG_PM_SLEEP I still need the explicit call in this function.
- 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);
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.
Good catch! Added.
Sam
Den 12.08.2018 20.46, skrev Sam Ravnborg:
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 sam@ravnborg.org Cc: Nicolas Ferre nicolas.ferre@atmel.com Cc: Boris Brezillon boris.brezillon@free-electrons.com Cc: Alexandre Belloni alexandre.belloni@bootlin.com
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
<snip>
diff --git a/drivers/gpu/drm/atmel/atmel_lcdc-dc.c b/drivers/gpu/drm/atmel/atmel_lcdc-dc.c new file mode 100644 index 000000000000..275a09e2e100 --- /dev/null +++ b/drivers/gpu/drm/atmel/atmel_lcdc-dc.c @@ -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;
- bool have_hozval;
- bool have_alt_pixclock;
+};
+/* private data */ +struct lcdc_dc {
- const struct atmel_lcdc_dc_desc *desc;
- struct atmel_mfd_lcdc *mfd_lcdc;
- struct regulator *lcd_supply;
- struct drm_fbdev_cma *fbdev;
There's generic fbdev emulaton now, so you can drop this. More info further down.
- struct drm_bridge *bridge;
- struct drm_panel *panel;
- struct regmap *regmap;
- struct device *dev;
- struct drm_simple_display_pipe pipe;
- struct work_struct reset_lcdc_work;
- struct drm_connector connector;
+};
+/* 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?
- .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,
+};
<snip>
+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 |
DRIVER_PRIME |
DRIVER_ATOMIC,
- .name = "atmel-lcdc",
- .desc = "Atmel LCD Display Controller DRM",
- .date = "20180808",
- .major = 1,
- .minor = 0,
- .patchlevel = 0,
- .lastclose = drm_fb_helper_lastclose,
You can drop this, lastclose is handled by the generic fbdev emulation.
- .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@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) {
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);
You are using both drm_fb_cma_fbdev_init() and drm_fbdev_cma_init() which does the same. I'm to blame for the confusion, the generic fbdev emulation idea came up during refactoring, so there's an old way and a new way, which is now obsolete (both will be removed when drivers have been converted to drm_fbdev_generic_setup()).
You can use drm_fbdev_generic_setup(drm, 16) instead, and that's all you need to get fbdev emulation.
I see that many drivers register fbdev before they register the DRM device. Personally I find this strange to make the emulated device available before the real one. I suggest you call drm_fbdev_generic_setup() after drm_dev_register() and if it fails, just put a note in the log and let the driver succeed in probing.
- 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);
- cancel_work_sync(&lcdc_dc->reset_lcdc_work);
- return ret;
+}
+static void lcdc_dc_unload(struct drm_device *dev) +{
- struct lcdc_dc *lcdc_dc = dev->dev_private;
- drm_fb_cma_fbdev_fini(dev);
You can remove this. fbdev is automatically unregistered by drm_dev_unregister().
Noralf.
- 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 sam@ravnborg.org"); +MODULE_DESCRIPTION("Atmel LCDC Display Controller DRM Driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:atmel-lcdc-dc");
Hi Noralf.
+/* 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?
I kept is at in reality 0 is not an OK value. Will revisit the datasheets and check what they say.
As for all the other comments - thanks! I will address them in v2.
Currently working on a larger refactoring to address all the feedback. Things becomes simpler which is always good.
Sam
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... (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
On 12/08/2018 at 21:55, Sam Ravnborg wrote:
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... (No NDA required)
(old memories) this 2D engine prove itself of being quite limited as the fill polygon and clipping functions are not working.
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.
Or is this in reality a simple GPU that requires a dedicated GPU driver?
Sam
On Mon, Aug 13, 2018 at 04:47:26PM +0200, Nicolas Ferre wrote:
On 12/08/2018 at 21:55, Sam Ravnborg wrote:
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... (No NDA required)
(old memories) this 2D engine prove itself of being quite limited as the fill polygon and clipping functions are not working.
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.
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". -Daniel
Or is this in reality a simple GPU that requires a dedicated GPU driver?
Sam
-- Nicolas Ferre _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel.
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.
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)).
Sam
On Wed, Aug 22, 2018 at 10:12 PM, Sam Ravnborg sam@ravnborg.org wrote:
Hi Daniel.
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.
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.
Yeah I figured more detail than this here would be good, but also wanted to avoid having to retype that ever again.
(Or rather you will write the blog in 5 days, seems to be some time machine involved. (Date is the 27th below the title)).
My blog platform pulled one on me - pushed it out with a future date, with the idea to proof-read a bit more. Didn't work, so everyone read the slightly draft-ish version :-/ In the past this did work, and was very neat. -Daniel
On 12/08/2018 at 20:41, Sam Ravnborg wrote:
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!
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.
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").
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.
Good, this is why I would use the same type of compatibility string for the split mfd/pwm/lcd (just without the "h").
The hlcdc and lcdc has only a few things in common and trying to share the code for them was not a viable solution.
Yes, I agree.
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.
- 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.
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.
We'll be able to test it on other SoCs and boards.
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_mo...
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!
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...
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(-)
Hi Nicholas.
On Mon, Aug 13, 2018 at 05:54:48PM +0200, Nicolas Ferre wrote:
On 12/08/2018 at 20:41, Sam Ravnborg wrote:
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".
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.
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.
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 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.
Then I will delete all the STN bits that was ported over. If we need them later they can be found on the mailign list.
- 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.
Ok, I will delete these.
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.
Thanks!
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_mo...
I posted a picture to G+ here: https://plus.google.com/+SamRavnborg/posts/YBt4jUGLgWa
They look similar but are not equal. For now I assume this is OK. I will do some testing with a Qt app, and will test colors with this too.
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...
Will fix, as described above.
Thanks for the inputs!
Sam
On Mon, Aug 13, 2018 at 08:18:08PM +0200, Sam Ravnborg wrote:
Hi Nicholas.
On Mon, Aug 13, 2018 at 05:54:48PM +0200, Nicolas Ferre wrote:
On 12/08/2018 at 20:41, Sam Ravnborg wrote:
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".
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.
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.
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 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.
The fact that the OS has 2 drivers is irrelevant to the binding and DT is not a way to select drivers. Your choice with Linux is either kconfig or manual module loading.
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 would not do that. Then you have a dts with a mixture based on when you got around to writing each binding. i.MX has continued using 'fsl' even on new chips today. Probably a good choice had the QCom acq had gone thru.
Rob
Hi Rob.
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@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@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@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>; };
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?
Thanks in advance,
Sam
On Tue, Aug 14, 2018 at 06:43:43PM +0200, Sam Ravnborg wrote:
Hi Rob.
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@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@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@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>; };
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.
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?
- 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.
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?
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.
Rob
Hi Rob.
Thanks for the feedback, as I am rather new to this DT stuff a few iterations are no suprise.
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>; };
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.
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?
They have registers in the same memory area - and I think that it.
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?
So in other words - the existing binding in atmel,lcdc.txt needs to be extended in a backward compatible way. I will take a look at that and post a proposal in v2.
- 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.
Makes sense.
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?
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.
Sam
On Tue, Aug 14, 2018 at 10:48 PM Sam Ravnborg sam@ravnborg.org wrote:
[...]
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?
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
On Wed, Aug 15, 2018 at 4:45 PM, Rob Herring robh@kernel.org wrote:
On Tue, Aug 14, 2018 at 10:48 PM Sam Ravnborg sam@ravnborg.org wrote:
[...]
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?
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
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. -Daniel
On 2018-08-15 17:04, Daniel Vetter wrote:
On Wed, Aug 15, 2018 at 4:45 PM, Rob Herring robh@kernel.org wrote:
On Tue, Aug 14, 2018 at 10:48 PM Sam Ravnborg sam@ravnborg.org wrote:
[...]
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?
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
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.
Cheers, Peter
Hi Peter.
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
Some digging in diverse threads required. I may take the easy way in v2 and postpone this feature. But you all confirmed that doing something locally in the driver is the wrong approach so that is ruled out.
(or is it bgr565 that Sam needs? The above quoted text is ambiguous).
My bad. I need either rgb565 or bgr565 (at lest this is how I understood the application note from Atmel/Microchip - need to re-read this too).
Some of the IP cores have an intensify bit which I do not yet understand how to adapt to the supported formats. This is also on the todo list.
I think something like my bus-format binding in [3] is generic enough to also help Sam.
Thanks, if I find time I will look into this, but first priority is to get the bindinns correct, and then the overall structure of the driver.
Sam
On 13/08/2018 20:18:08+0200, Sam Ravnborg wrote:
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.
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.
On Tue, Aug 14, 2018 at 04:36:03PM +0200, Alexandre Belloni wrote:
On 13/08/2018 20:18:08+0200, Sam Ravnborg wrote:
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.
Much better, agreed.
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.
OK, will do so in v2. I had planned to keep both in the DT file but then I will just replace one with the other.
Sam
Hi Sam,
On Sun, 12 Aug 2018 20:41:52 +0200 Sam Ravnborg sam@ravnborg.org wrote:
New DRM based driver for at91sam9 SOC's that uses the Atmel LCDC IP core.
First of all, thanks for this contribution.
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.
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.
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.
I guess you can skip that for now.
- 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.
- support for extra modes as applicable (and lcd-wiring-mode)
Peter already suggested something I think.
- support for AVR32 (is it relevant?)
It is, AVR32 is no longer supported in mainline.
The first patch renames .../drm/atmel-hlcdc to .../drm/atmel to have a nice home for both drivers.
Sounds good.
Regards,
Boris
Hi Boris.
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.
I v2 I am working with a backward compatible approach. This is better that what I cam up with initially.
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.
I guess you can skip that for now.
Also based on feedback from Nicholas the STN parts will be dropped in v2. For the gamma stuff this looks feasible with a small extension to drm_simple_kms_helper - but I dunno if this is something that userspace will actually use. So that will wait until there is some good reason to implement it, and I know how to test it too.
- 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.
I will try to cook up something, have learned a little since posting v1.
- support for extra modes as applicable (and lcd-wiring-mode)
Peter already suggested something I think.
Yep, plenty of links to read.
Sam
dri-devel@lists.freedesktop.org