* [PATCH V2 2/5] include/video: Add Exynos5 specific FIMD register offsets
From: Leela Krishna Amudala @ 2012-07-31 12:35 UTC (permalink / raw)
To: linux-arm-kernel, linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, kgene.kim, joshi,
jg1.han, m.szyprowski
In-Reply-To: <1343737385-23337-1-git-send-email-l.krishna@samsung.com>
Exynos5 has VIDTCON and VIDCON registers at different offsets
from the previous SOCs. Hence, adding the macros.
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
---
include/video/samsung_fimd.h | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h
index e979f42..820f190 100644
--- a/include/video/samsung_fimd.h
+++ b/include/video/samsung_fimd.h
@@ -524,3 +524,10 @@
* 1110 -none- -none- -none- -none- -none-
* 1111 -none- -none- -none- -none- -none-
*/
+
+/*EXYNOS5 FIMD REG OFFSET */
+#define EXYNOS5_VIDTCON0 (0x20010)
+#define EXYNOS5_VIDTCON1 (0x20014)
+#define EXYNOS5_VIDTCON2 (0x20018)
+#define EXYNOS5_VIDTCON3 (0x2001C)
+#define EXYNOS5_VIDCON1 (0x20004)
--
1.7.0.4
^ permalink raw reply related
* [PATCH V2 1/5] include/video: Add samsung FIMD register header
From: Leela Krishna Amudala @ 2012-07-31 12:35 UTC (permalink / raw)
To: linux-arm-kernel, linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, kgene.kim, joshi,
jg1.han, m.szyprowski
In-Reply-To: <1343737385-23337-1-git-send-email-l.krishna@samsung.com>
This patch copies the contents from regs-fb-v4.h and regs-fb.h to
include/video/samsung_fimd.h
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
---
include/video/samsung_fimd.h | 526 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 526 insertions(+), 0 deletions(-)
create mode 100644 include/video/samsung_fimd.h
diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h
new file mode 100644
index 0000000..e979f42
--- /dev/null
+++ b/include/video/samsung_fimd.h
@@ -0,0 +1,526 @@
+/* include/video/samsung_fimd.h
+ *
+ * Copyright 2008 Openmoko, Inc.
+ * Copyright 2008 Simtec Electronics
+ * http://armlinux.simtec.co.uk/
+ * Ben Dooks <ben@simtec.co.uk>
+ *
+ * S3C Platform - new-style fimd and framebuffer register definitions
+ *
+ * This is the register set for the fimd and new style framebuffer interface
+ * found from the S3C2443 onwards into the S3C2416, S3C2450 and the
+ * S3C64XX series such as the S3C6400 and S3C6410.
+ *
+ * The file does not contain the cpu specific items which are based on
+ * whichever architecture is selected, it only contains the core of the
+ * register set. See <mach/regs-fb.h> to get the specifics.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+/* VIDCON0 */
+
+#define VIDCON0 (0x00)
+#define VIDCON0_INTERLACE (1 << 29)
+#define VIDCON0_VIDOUT_MASK (0x3 << 26)
+#define VIDCON0_VIDOUT_SHIFT (26)
+#define VIDCON0_VIDOUT_RGB (0x0 << 26)
+#define VIDCON0_VIDOUT_TV (0x1 << 26)
+#define VIDCON0_VIDOUT_I80_LDI0 (0x2 << 26)
+#define VIDCON0_VIDOUT_I80_LDI1 (0x3 << 26)
+
+#define VIDCON0_L1_DATA_MASK (0x7 << 23)
+#define VIDCON0_L1_DATA_SHIFT (23)
+#define VIDCON0_L1_DATA_16BPP (0x0 << 23)
+#define VIDCON0_L1_DATA_18BPP16 (0x1 << 23)
+#define VIDCON0_L1_DATA_18BPP9 (0x2 << 23)
+#define VIDCON0_L1_DATA_24BPP (0x3 << 23)
+#define VIDCON0_L1_DATA_18BPP (0x4 << 23)
+#define VIDCON0_L1_DATA_16BPP8 (0x5 << 23)
+
+#define VIDCON0_L0_DATA_MASK (0x7 << 20)
+#define VIDCON0_L0_DATA_SHIFT (20)
+#define VIDCON0_L0_DATA_16BPP (0x0 << 20)
+#define VIDCON0_L0_DATA_18BPP16 (0x1 << 20)
+#define VIDCON0_L0_DATA_18BPP9 (0x2 << 20)
+#define VIDCON0_L0_DATA_24BPP (0x3 << 20)
+#define VIDCON0_L0_DATA_18BPP (0x4 << 20)
+#define VIDCON0_L0_DATA_16BPP8 (0x5 << 20)
+
+#define VIDCON0_PNRMODE_MASK (0x3 << 17)
+#define VIDCON0_PNRMODE_SHIFT (17)
+#define VIDCON0_PNRMODE_RGB (0x0 << 17)
+#define VIDCON0_PNRMODE_BGR (0x1 << 17)
+#define VIDCON0_PNRMODE_SERIAL_RGB (0x2 << 17)
+#define VIDCON0_PNRMODE_SERIAL_BGR (0x3 << 17)
+
+#define VIDCON0_CLKVALUP (1 << 16)
+#define VIDCON0_CLKVAL_F_MASK (0xff << 6)
+#define VIDCON0_CLKVAL_F_SHIFT (6)
+#define VIDCON0_CLKVAL_F_LIMIT (0xff)
+#define VIDCON0_CLKVAL_F(_x) ((_x) << 6)
+#define VIDCON0_VLCKFREE (1 << 5)
+#define VIDCON0_CLKDIR (1 << 4)
+
+#define VIDCON0_CLKSEL_MASK (0x3 << 2)
+#define VIDCON0_CLKSEL_SHIFT (2)
+#define VIDCON0_CLKSEL_HCLK (0x0 << 2)
+#define VIDCON0_CLKSEL_LCD (0x1 << 2)
+#define VIDCON0_CLKSEL_27M (0x3 << 2)
+
+#define VIDCON0_ENVID (1 << 1)
+#define VIDCON0_ENVID_F (1 << 0)
+
+#define VIDCON1 (0x04)
+#define VIDCON1_LINECNT_MASK (0x7ff << 16)
+#define VIDCON1_LINECNT_SHIFT (16)
+#define VIDCON1_LINECNT_GET(_v) (((_v) >> 16) & 0x7ff)
+#define VIDCON1_VSTATUS_MASK (0x3 << 13)
+#define VIDCON1_VSTATUS_SHIFT (13)
+#define VIDCON1_VSTATUS_VSYNC (0x0 << 13)
+#define VIDCON1_VSTATUS_BACKPORCH (0x1 << 13)
+#define VIDCON1_VSTATUS_ACTIVE (0x2 << 13)
+#define VIDCON1_VSTATUS_FRONTPORCH (0x0 << 13)
+#define VIDCON1_VCLK_MASK (0x3 << 9)
+#define VIDCON1_VCLK_HOLD (0x0 << 9)
+#define VIDCON1_VCLK_RUN (0x1 << 9)
+
+#define VIDCON1_INV_VCLK (1 << 7)
+#define VIDCON1_INV_HSYNC (1 << 6)
+#define VIDCON1_INV_VSYNC (1 << 5)
+#define VIDCON1_INV_VDEN (1 << 4)
+
+/* VIDCON2 */
+
+#define VIDCON2 (0x08)
+#define VIDCON2_EN601 (1 << 23)
+#define VIDCON2_TVFMTSEL_SW (1 << 14)
+
+#define VIDCON2_TVFMTSEL1_MASK (0x3 << 12)
+#define VIDCON2_TVFMTSEL1_SHIFT (12)
+#define VIDCON2_TVFMTSEL1_RGB (0x0 << 12)
+#define VIDCON2_TVFMTSEL1_YUV422 (0x1 << 12)
+#define VIDCON2_TVFMTSEL1_YUV444 (0x2 << 12)
+
+#define VIDCON2_ORGYCbCr (1 << 8)
+#define VIDCON2_YUVORDCrCb (1 << 7)
+
+/* PRTCON (S3C6410, S5PC100)
+ * Might not be present in the S3C6410 documentation,
+ * but tests prove it's there almost for sure; shouldn't hurt in any case.
+ */
+#define PRTCON (0x0c)
+#define PRTCON_PROTECT (1 << 11)
+
+/* VIDTCON0 */
+
+#define VIDTCON0_VBPDE_MASK (0xff << 24)
+#define VIDTCON0_VBPDE_SHIFT (24)
+#define VIDTCON0_VBPDE_LIMIT (0xff)
+#define VIDTCON0_VBPDE(_x) ((_x) << 24)
+
+#define VIDTCON0_VBPD_MASK (0xff << 16)
+#define VIDTCON0_VBPD_SHIFT (16)
+#define VIDTCON0_VBPD_LIMIT (0xff)
+#define VIDTCON0_VBPD(_x) ((_x) << 16)
+
+#define VIDTCON0_VFPD_MASK (0xff << 8)
+#define VIDTCON0_VFPD_SHIFT (8)
+#define VIDTCON0_VFPD_LIMIT (0xff)
+#define VIDTCON0_VFPD(_x) ((_x) << 8)
+
+#define VIDTCON0_VSPW_MASK (0xff << 0)
+#define VIDTCON0_VSPW_SHIFT (0)
+#define VIDTCON0_VSPW_LIMIT (0xff)
+#define VIDTCON0_VSPW(_x) ((_x) << 0)
+
+/* VIDTCON1 */
+
+#define VIDTCON1_VFPDE_MASK (0xff << 24)
+#define VIDTCON1_VFPDE_SHIFT (24)
+#define VIDTCON1_VFPDE_LIMIT (0xff)
+#define VIDTCON1_VFPDE(_x) ((_x) << 24)
+
+#define VIDTCON1_HBPD_MASK (0xff << 16)
+#define VIDTCON1_HBPD_SHIFT (16)
+#define VIDTCON1_HBPD_LIMIT (0xff)
+#define VIDTCON1_HBPD(_x) ((_x) << 16)
+
+#define VIDTCON1_HFPD_MASK (0xff << 8)
+#define VIDTCON1_HFPD_SHIFT (8)
+#define VIDTCON1_HFPD_LIMIT (0xff)
+#define VIDTCON1_HFPD(_x) ((_x) << 8)
+
+#define VIDTCON1_HSPW_MASK (0xff << 0)
+#define VIDTCON1_HSPW_SHIFT (0)
+#define VIDTCON1_HSPW_LIMIT (0xff)
+#define VIDTCON1_HSPW(_x) ((_x) << 0)
+
+#define VIDTCON2 (0x18)
+#define VIDTCON2_LINEVAL_E(_x) ((((_x) & 0x800) >> 11) << 23)
+#define VIDTCON2_LINEVAL_MASK (0x7ff << 11)
+#define VIDTCON2_LINEVAL_SHIFT (11)
+#define VIDTCON2_LINEVAL_LIMIT (0x7ff)
+#define VIDTCON2_LINEVAL(_x) (((_x) & 0x7ff) << 11)
+
+#define VIDTCON2_HOZVAL_E(_x) ((((_x) & 0x800) >> 11) << 22)
+#define VIDTCON2_HOZVAL_MASK (0x7ff << 0)
+#define VIDTCON2_HOZVAL_SHIFT (0)
+#define VIDTCON2_HOZVAL_LIMIT (0x7ff)
+#define VIDTCON2_HOZVAL(_x) (((_x) & 0x7ff) << 0)
+
+/* WINCONx */
+
+
+#define WINCONx_BITSWP (1 << 18)
+#define WINCONx_BYTSWP (1 << 17)
+#define WINCONx_HAWSWP (1 << 16)
+#define WINCONx_WSWP (1 << 15)
+#define WINCONx_BURSTLEN_MASK (0x3 << 9)
+#define WINCONx_BURSTLEN_SHIFT (9)
+#define WINCONx_BURSTLEN_16WORD (0x0 << 9)
+#define WINCONx_BURSTLEN_8WORD (0x1 << 9)
+#define WINCONx_BURSTLEN_4WORD (0x2 << 9)
+
+#define WINCONx_ENWIN (1 << 0)
+#define WINCON0_BPPMODE_MASK (0xf << 2)
+#define WINCON0_BPPMODE_SHIFT (2)
+#define WINCON0_BPPMODE_1BPP (0x0 << 2)
+#define WINCON0_BPPMODE_2BPP (0x1 << 2)
+#define WINCON0_BPPMODE_4BPP (0x2 << 2)
+#define WINCON0_BPPMODE_8BPP_PALETTE (0x3 << 2)
+#define WINCON0_BPPMODE_16BPP_565 (0x5 << 2)
+#define WINCON0_BPPMODE_16BPP_1555 (0x7 << 2)
+#define WINCON0_BPPMODE_18BPP_666 (0x8 << 2)
+#define WINCON0_BPPMODE_24BPP_888 (0xb << 2)
+
+#define WINCON1_BLD_PIX (1 << 6)
+
+#define WINCON1_ALPHA_SEL (1 << 1)
+#define WINCON1_BPPMODE_MASK (0xf << 2)
+#define WINCON1_BPPMODE_SHIFT (2)
+#define WINCON1_BPPMODE_1BPP (0x0 << 2)
+#define WINCON1_BPPMODE_2BPP (0x1 << 2)
+#define WINCON1_BPPMODE_4BPP (0x2 << 2)
+#define WINCON1_BPPMODE_8BPP_PALETTE (0x3 << 2)
+#define WINCON1_BPPMODE_8BPP_1232 (0x4 << 2)
+#define WINCON1_BPPMODE_16BPP_565 (0x5 << 2)
+#define WINCON1_BPPMODE_16BPP_A1555 (0x6 << 2)
+#define WINCON1_BPPMODE_16BPP_I1555 (0x7 << 2)
+#define WINCON1_BPPMODE_18BPP_666 (0x8 << 2)
+#define WINCON1_BPPMODE_18BPP_A1665 (0x9 << 2)
+#define WINCON1_BPPMODE_19BPP_A1666 (0xa << 2)
+#define WINCON1_BPPMODE_24BPP_888 (0xb << 2)
+#define WINCON1_BPPMODE_24BPP_A1887 (0xc << 2)
+#define WINCON1_BPPMODE_25BPP_A1888 (0xd << 2)
+#define WINCON1_BPPMODE_28BPP_A4888 (0xd << 2)
+
+/* S5PV210 */
+#define SHADOWCON (0x34)
+#define SHADOWCON_WINx_PROTECT(_win) (1 << (10 + (_win)))
+/* DMA channels (all windows) */
+#define SHADOWCON_CHx_ENABLE(_win) (1 << (_win))
+/* Local input channels (windows 0-2) */
+#define SHADOWCON_CHx_LOCAL_ENABLE(_win) (1 << (5 + (_win)))
+
+#define VIDOSDxA_TOPLEFT_X_E(_x) ((((_x) & 0x800) >> 11) << 23)
+#define VIDOSDxA_TOPLEFT_X_MASK (0x7ff << 11)
+#define VIDOSDxA_TOPLEFT_X_SHIFT (11)
+#define VIDOSDxA_TOPLEFT_X_LIMIT (0x7ff)
+#define VIDOSDxA_TOPLEFT_X(_x) (((_x) & 0x7ff) << 11)
+
+#define VIDOSDxA_TOPLEFT_Y_E(_x) ((((_x) & 0x800) >> 11) << 22)
+#define VIDOSDxA_TOPLEFT_Y_MASK (0x7ff << 0)
+#define VIDOSDxA_TOPLEFT_Y_SHIFT (0)
+#define VIDOSDxA_TOPLEFT_Y_LIMIT (0x7ff)
+#define VIDOSDxA_TOPLEFT_Y(_x) (((_x) & 0x7ff) << 0)
+
+#define VIDOSDxB_BOTRIGHT_X_E(_x) ((((_x) & 0x800) >> 11) << 23)
+#define VIDOSDxB_BOTRIGHT_X_MASK (0x7ff << 11)
+#define VIDOSDxB_BOTRIGHT_X_SHIFT (11)
+#define VIDOSDxB_BOTRIGHT_X_LIMIT (0x7ff)
+#define VIDOSDxB_BOTRIGHT_X(_x) (((_x) & 0x7ff) << 11)
+
+#define VIDOSDxB_BOTRIGHT_Y_E(_x) ((((_x) & 0x800) >> 11) << 22)
+#define VIDOSDxB_BOTRIGHT_Y_MASK (0x7ff << 0)
+#define VIDOSDxB_BOTRIGHT_Y_SHIFT (0)
+#define VIDOSDxB_BOTRIGHT_Y_LIMIT (0x7ff)
+#define VIDOSDxB_BOTRIGHT_Y(_x) (((_x) & 0x7ff) << 0)
+
+/* For VIDOSD[1..4]C */
+#define VIDISD14C_ALPHA0_R(_x) ((_x) << 20)
+#define VIDISD14C_ALPHA0_G_MASK (0xf << 16)
+#define VIDISD14C_ALPHA0_G_SHIFT (16)
+#define VIDISD14C_ALPHA0_G_LIMIT (0xf)
+#define VIDISD14C_ALPHA0_G(_x) ((_x) << 16)
+#define VIDISD14C_ALPHA0_B_MASK (0xf << 12)
+#define VIDISD14C_ALPHA0_B_SHIFT (12)
+#define VIDISD14C_ALPHA0_B_LIMIT (0xf)
+#define VIDISD14C_ALPHA0_B(_x) ((_x) << 12)
+#define VIDISD14C_ALPHA1_R_MASK (0xf << 8)
+#define VIDISD14C_ALPHA1_R_SHIFT (8)
+#define VIDISD14C_ALPHA1_R_LIMIT (0xf)
+#define VIDISD14C_ALPHA1_R(_x) ((_x) << 8)
+#define VIDISD14C_ALPHA1_G_MASK (0xf << 4)
+#define VIDISD14C_ALPHA1_G_SHIFT (4)
+#define VIDISD14C_ALPHA1_G_LIMIT (0xf)
+#define VIDISD14C_ALPHA1_G(_x) ((_x) << 4)
+#define VIDISD14C_ALPHA1_B_MASK (0xf << 0)
+#define VIDISD14C_ALPHA1_B_SHIFT (0)
+#define VIDISD14C_ALPHA1_B_LIMIT (0xf)
+#define VIDISD14C_ALPHA1_B(_x) ((_x) << 0)
+
+/* Video buffer addresses */
+#define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8))
+#define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8))
+#define VIDW_BUF_END(_buff) (0xD0 + ((_buff) * 8))
+#define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8))
+#define VIDW_BUF_SIZE(_buff) (0x100 + ((_buff) * 4))
+
+#define VIDW_BUF_SIZE_OFFSET_E(_x) ((((_x) & 0x2000) >> 13) << 27)
+#define VIDW_BUF_SIZE_OFFSET_MASK (0x1fff << 13)
+#define VIDW_BUF_SIZE_OFFSET_SHIFT (13)
+#define VIDW_BUF_SIZE_OFFSET_LIMIT (0x1fff)
+#define VIDW_BUF_SIZE_OFFSET(_x) (((_x) & 0x1fff) << 13)
+
+#define VIDW_BUF_SIZE_PAGEWIDTH_E(_x) ((((_x) & 0x2000) >> 13) << 26)
+#define VIDW_BUF_SIZE_PAGEWIDTH_MASK (0x1fff << 0)
+#define VIDW_BUF_SIZE_PAGEWIDTH_SHIFT (0)
+#define VIDW_BUF_SIZE_PAGEWIDTH_LIMIT (0x1fff)
+#define VIDW_BUF_SIZE_PAGEWIDTH(_x) (((_x) & 0x1fff) << 0)
+
+/* Interrupt controls and status */
+
+#define VIDINTCON0_FIFOINTERVAL_MASK (0x3f << 20)
+#define VIDINTCON0_FIFOINTERVAL_SHIFT (20)
+#define VIDINTCON0_FIFOINTERVAL_LIMIT (0x3f)
+#define VIDINTCON0_FIFOINTERVAL(_x) ((_x) << 20)
+
+#define VIDINTCON0_INT_SYSMAINCON (1 << 19)
+#define VIDINTCON0_INT_SYSSUBCON (1 << 18)
+#define VIDINTCON0_INT_I80IFDONE (1 << 17)
+
+#define VIDINTCON0_FRAMESEL0_MASK (0x3 << 15)
+#define VIDINTCON0_FRAMESEL0_SHIFT (15)
+#define VIDINTCON0_FRAMESEL0_BACKPORCH (0x0 << 15)
+#define VIDINTCON0_FRAMESEL0_VSYNC (0x1 << 15)
+#define VIDINTCON0_FRAMESEL0_ACTIVE (0x2 << 15)
+#define VIDINTCON0_FRAMESEL0_FRONTPORCH (0x3 << 15)
+
+#define VIDINTCON0_FRAMESEL1 (1 << 13)
+#define VIDINTCON0_FRAMESEL1_MASK (0x3 << 13)
+#define VIDINTCON0_FRAMESEL1_NONE (0x0 << 13)
+#define VIDINTCON0_FRAMESEL1_BACKPORCH (0x1 << 13)
+#define VIDINTCON0_FRAMESEL1_VSYNC (0x2 << 13)
+#define VIDINTCON0_FRAMESEL1_FRONTPORCH (0x3 << 13)
+
+#define VIDINTCON0_INT_FRAME (1 << 12)
+#define VIDINTCON0_FIFIOSEL_MASK (0x7f << 5)
+#define VIDINTCON0_FIFIOSEL_SHIFT (5)
+#define VIDINTCON0_FIFIOSEL_WINDOW0 (0x1 << 5)
+#define VIDINTCON0_FIFIOSEL_WINDOW1 (0x2 << 5)
+
+#define VIDINTCON0_FIFOLEVEL_MASK (0x7 << 2)
+#define VIDINTCON0_FIFOLEVEL_SHIFT (2)
+#define VIDINTCON0_FIFOLEVEL_TO25PC (0x0 << 2)
+#define VIDINTCON0_FIFOLEVEL_TO50PC (0x1 << 2)
+#define VIDINTCON0_FIFOLEVEL_TO75PC (0x2 << 2)
+#define VIDINTCON0_FIFOLEVEL_EMPTY (0x3 << 2)
+#define VIDINTCON0_FIFOLEVEL_FULL (0x4 << 2)
+
+#define VIDINTCON0_INT_FIFO_MASK (0x3 << 0)
+#define VIDINTCON0_INT_FIFO_SHIFT (0)
+#define VIDINTCON0_INT_ENABLE (1 << 0)
+
+#define VIDINTCON1 (0x134)
+#define VIDINTCON1_INT_I180 (1 << 2)
+#define VIDINTCON1_INT_FRAME (1 << 1)
+#define VIDINTCON1_INT_FIFO (1 << 0)
+
+/* Window colour-key control registers */
+#define WKEYCON (0x140) /* 6410,V210 */
+
+#define WKEYCON0 (0x00)
+#define WKEYCON1 (0x04)
+
+#define WxKEYCON0_KEYBL_EN (1 << 26)
+#define WxKEYCON0_KEYEN_F (1 << 25)
+#define WxKEYCON0_DIRCON (1 << 24)
+#define WxKEYCON0_COMPKEY_MASK (0xffffff << 0)
+#define WxKEYCON0_COMPKEY_SHIFT (0)
+#define WxKEYCON0_COMPKEY_LIMIT (0xffffff)
+#define WxKEYCON0_COMPKEY(_x) ((_x) << 0)
+#define WxKEYCON1_COLVAL_MASK (0xffffff << 0)
+#define WxKEYCON1_COLVAL_SHIFT (0)
+#define WxKEYCON1_COLVAL_LIMIT (0xffffff)
+#define WxKEYCON1_COLVAL(_x) ((_x) << 0)
+
+
+/* Window blanking (MAP) */
+
+#define WINxMAP_MAP (1 << 24)
+#define WINxMAP_MAP_COLOUR_MASK (0xffffff << 0)
+#define WINxMAP_MAP_COLOUR_SHIFT (0)
+#define WINxMAP_MAP_COLOUR_LIMIT (0xffffff)
+#define WINxMAP_MAP_COLOUR(_x) ((_x) << 0)
+
+#define WPALCON_PAL_UPDATE (1 << 9)
+#define WPALCON_W1PAL_MASK (0x7 << 3)
+#define WPALCON_W1PAL_SHIFT (3)
+#define WPALCON_W1PAL_25BPP_A888 (0x0 << 3)
+#define WPALCON_W1PAL_24BPP (0x1 << 3)
+#define WPALCON_W1PAL_19BPP_A666 (0x2 << 3)
+#define WPALCON_W1PAL_18BPP_A665 (0x3 << 3)
+#define WPALCON_W1PAL_18BPP (0x4 << 3)
+#define WPALCON_W1PAL_16BPP_A555 (0x5 << 3)
+#define WPALCON_W1PAL_16BPP_565 (0x6 << 3)
+
+#define WPALCON_W0PAL_MASK (0x7 << 0)
+#define WPALCON_W0PAL_SHIFT (0)
+#define WPALCON_W0PAL_25BPP_A888 (0x0 << 0)
+#define WPALCON_W0PAL_24BPP (0x1 << 0)
+#define WPALCON_W0PAL_19BPP_A666 (0x2 << 0)
+#define WPALCON_W0PAL_18BPP_A665 (0x3 << 0)
+#define WPALCON_W0PAL_18BPP (0x4 << 0)
+#define WPALCON_W0PAL_16BPP_A555 (0x5 << 0)
+#define WPALCON_W0PAL_16BPP_565 (0x6 << 0)
+
+/* Blending equation control */
+#define BLENDCON (0x260)
+#define BLENDCON_NEW_MASK (1 << 0)
+#define BLENDCON_NEW_8BIT_ALPHA_VALUE (1 << 0)
+#define BLENDCON_NEW_4BIT_ALPHA_VALUE (0 << 0)
+
+#define S3C_FB_MAX_WIN (5) /* number of hardware windows available. */
+#define VIDCON1_FSTATUS_EVEN (1 << 15)
+
+/* Video timing controls */
+#define VIDTCON0 (0x10)
+#define VIDTCON1 (0x14)
+#define VIDTCON2 (0x18)
+
+/* Window position controls */
+
+#define WINCON(_win) (0x20 + ((_win) * 4))
+
+/* OSD1 and OSD4 do not have register D */
+
+#define VIDOSD_BASE (0x40)
+
+#define VIDINTCON0 (0x130)
+
+/* WINCONx */
+
+#define WINCONx_CSCWIDTH_MASK (0x3 << 26)
+#define WINCONx_CSCWIDTH_SHIFT (26)
+#define WINCONx_CSCWIDTH_WIDE (0x0 << 26)
+#define WINCONx_CSCWIDTH_NARROW (0x3 << 26)
+
+#define WINCONx_ENLOCAL (1 << 22)
+#define WINCONx_BUFSTATUS (1 << 21)
+#define WINCONx_BUFSEL (1 << 20)
+#define WINCONx_BUFAUTOEN (1 << 19)
+#define WINCONx_YCbCr (1 << 13)
+
+#define WINCON1_LOCALSEL_CAMIF (1 << 23)
+
+#define WINCON2_LOCALSEL_CAMIF (1 << 23)
+#define WINCON2_BLD_PIX (1 << 6)
+
+#define WINCON2_ALPHA_SEL (1 << 1)
+#define WINCON2_BPPMODE_MASK (0xf << 2)
+#define WINCON2_BPPMODE_SHIFT (2)
+#define WINCON2_BPPMODE_1BPP (0x0 << 2)
+#define WINCON2_BPPMODE_2BPP (0x1 << 2)
+#define WINCON2_BPPMODE_4BPP (0x2 << 2)
+#define WINCON2_BPPMODE_8BPP_1232 (0x4 << 2)
+#define WINCON2_BPPMODE_16BPP_565 (0x5 << 2)
+#define WINCON2_BPPMODE_16BPP_A1555 (0x6 << 2)
+#define WINCON2_BPPMODE_16BPP_I1555 (0x7 << 2)
+#define WINCON2_BPPMODE_18BPP_666 (0x8 << 2)
+#define WINCON2_BPPMODE_18BPP_A1665 (0x9 << 2)
+#define WINCON2_BPPMODE_19BPP_A1666 (0xa << 2)
+#define WINCON2_BPPMODE_24BPP_888 (0xb << 2)
+#define WINCON2_BPPMODE_24BPP_A1887 (0xc << 2)
+#define WINCON2_BPPMODE_25BPP_A1888 (0xd << 2)
+#define WINCON2_BPPMODE_28BPP_A4888 (0xd << 2)
+
+#define WINCON3_BLD_PIX (1 << 6)
+
+#define WINCON3_ALPHA_SEL (1 << 1)
+#define WINCON3_BPPMODE_MASK (0xf << 2)
+#define WINCON3_BPPMODE_SHIFT (2)
+#define WINCON3_BPPMODE_1BPP (0x0 << 2)
+#define WINCON3_BPPMODE_2BPP (0x1 << 2)
+#define WINCON3_BPPMODE_4BPP (0x2 << 2)
+#define WINCON3_BPPMODE_16BPP_565 (0x5 << 2)
+#define WINCON3_BPPMODE_16BPP_A1555 (0x6 << 2)
+#define WINCON3_BPPMODE_16BPP_I1555 (0x7 << 2)
+#define WINCON3_BPPMODE_18BPP_666 (0x8 << 2)
+#define WINCON3_BPPMODE_18BPP_A1665 (0x9 << 2)
+#define WINCON3_BPPMODE_19BPP_A1666 (0xa << 2)
+#define WINCON3_BPPMODE_24BPP_888 (0xb << 2)
+#define WINCON3_BPPMODE_24BPP_A1887 (0xc << 2)
+#define WINCON3_BPPMODE_25BPP_A1888 (0xd << 2)
+#define WINCON3_BPPMODE_28BPP_A4888 (0xd << 2)
+
+#define VIDINTCON0_FIFIOSEL_WINDOW2 (0x10 << 5)
+#define VIDINTCON0_FIFIOSEL_WINDOW3 (0x20 << 5)
+#define VIDINTCON0_FIFIOSEL_WINDOW4 (0x40 << 5)
+
+#define DITHMODE (0x170)
+#define WINxMAP(_win) (0x180 + ((_win) * 4))
+
+
+#define DITHMODE_R_POS_MASK (0x3 << 5)
+#define DITHMODE_R_POS_SHIFT (5)
+#define DITHMODE_R_POS_8BIT (0x0 << 5)
+#define DITHMODE_R_POS_6BIT (0x1 << 5)
+#define DITHMODE_R_POS_5BIT (0x2 << 5)
+
+#define DITHMODE_G_POS_MASK (0x3 << 3)
+#define DITHMODE_G_POS_SHIFT (3)
+#define DITHMODE_G_POS_8BIT (0x0 << 3)
+#define DITHMODE_G_POS_6BIT (0x1 << 3)
+#define DITHMODE_G_POS_5BIT (0x2 << 3)
+
+#define DITHMODE_B_POS_MASK (0x3 << 1)
+#define DITHMODE_B_POS_SHIFT (1)
+#define DITHMODE_B_POS_8BIT (0x0 << 1)
+#define DITHMODE_B_POS_6BIT (0x1 << 1)
+#define DITHMODE_B_POS_5BIT (0x2 << 1)
+
+#define DITHMODE_DITH_EN (1 << 0)
+
+#define WPALCON (0x1A0)
+
+/* Palette control */
+/* Note for S5PC100: you can still use those macros on WPALCON (aka WPALCON_L),
+ * but make sure that WPALCON_H W2PAL-W4PAL entries are zeroed out */
+#define WPALCON_W4PAL_16BPP_A555 (1 << 8)
+#define WPALCON_W3PAL_16BPP_A555 (1 << 7)
+#define WPALCON_W2PAL_16BPP_A555 (1 << 6)
+
+
+/* Notes on per-window bpp settings
+ *
+ * Value Win0 Win1 Win2 Win3 Win 4
+ * 0000 1(P) 1(P) 1(P) 1(P) 1(P)
+ * 0001 2(P) 2(P) 2(P) 2(P) 2(P)
+ * 0010 4(P) 4(P) 4(P) 4(P) -none-
+ * 0011 8(P) 8(P) -none- -none- -none-
+ * 0100 -none- 8(A232) 8(A232) -none- -none-
+ * 0101 16(565) 16(565) 16(565) 16(565) 16(565)
+ * 0110 -none- 16(A555) 16(A555) 16(A555) 16(A555)
+ * 0111 16(I555) 16(I565) 16(I555) 16(I555) 16(I555)
+ * 1000 18(666) 18(666) 18(666) 18(666) 18(666)
+ * 1001 -none- 18(A665) 18(A665) 18(A665) 16(A665)
+ * 1010 -none- 19(A666) 19(A666) 19(A666) 19(A666)
+ * 1011 24(888) 24(888) 24(888) 24(888) 24(888)
+ * 1100 -none- 24(A887) 24(A887) 24(A887) 24(A887)
+ * 1101 -none- 25(A888) 25(A888) 25(A888) 25(A888)
+ * 1110 -none- -none- -none- -none- -none-
+ * 1111 -none- -none- -none- -none- -none-
+*/
--
1.7.0.4
^ permalink raw reply related
* [PATCH V2 0/5] arm: samsung: Move FIMD headers to include/video/
From: Leela Krishna Amudala @ 2012-07-31 12:35 UTC (permalink / raw)
To: linux-arm-kernel, linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, kgene.kim, joshi,
jg1.han, m.szyprowski
This patchset moves the contents of regs-fb-v4.h and regs-fb.h from arch side
to include/video/samsung_fimd.h
This patchset is created and rebased against master branch of torvalds tree.
Tested on smdk5250 board, build tested for other boards.
Changes from version 1:
- Split the patches as per Sylwester comments
- Changed FIMD_V8_xxx macro to EXYNOS5_xxx
Leela Krishna Amudala (5):
include/video: Add samsung FIMD register header
include/video: Add Exynos5 specific FIMD register offsets
arm: samsung: Include the modified FIMD header file
driver: Include the modified FIMD header file
arm: samsung: delete frame buffer header files from platform
arch/arm/mach-exynos/mach-nuri.c | 2 +-
arch/arm/mach-exynos/mach-origen.c | 2 +-
arch/arm/mach-exynos/mach-smdk4x12.c | 2 +-
arch/arm/mach-exynos/mach-smdkv310.c | 2 +-
arch/arm/mach-exynos/mach-universal_c210.c | 2 +-
arch/arm/mach-exynos/setup-fimd0.c | 2 +-
arch/arm/mach-s3c24xx/mach-smdk2416.c | 2 +-
arch/arm/mach-s3c64xx/mach-anw6410.c | 2 +-
arch/arm/mach-s3c64xx/mach-crag6410.c | 2 +-
arch/arm/mach-s3c64xx/mach-hmt.c | 2 +-
arch/arm/mach-s3c64xx/mach-mini6410.c | 2 +-
arch/arm/mach-s3c64xx/mach-ncp.c | 2 +-
arch/arm/mach-s3c64xx/mach-real6410.c | 2 +-
arch/arm/mach-s3c64xx/mach-smartq5.c | 2 +-
arch/arm/mach-s3c64xx/mach-smartq7.c | 2 +-
arch/arm/mach-s3c64xx/mach-smdk6410.c | 2 +-
arch/arm/mach-s5p64x0/mach-smdk6440.c | 2 +-
arch/arm/mach-s5p64x0/mach-smdk6450.c | 2 +-
arch/arm/mach-s5pc100/mach-smdkc100.c | 2 +-
arch/arm/mach-s5pv210/mach-aquila.c | 2 +-
arch/arm/mach-s5pv210/mach-goni.c | 2 +-
arch/arm/mach-s5pv210/mach-smdkv210.c | 2 +-
arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 --------------------
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 2 +-
drivers/video/s3c-fb.c | 2 +-
.../plat/regs-fb.h => include/video/samsung_fimd.h | 152 +++++++++++++++++--
26 files changed, 165 insertions(+), 194 deletions(-)
delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
rename arch/arm/plat-samsung/include/plat/regs-fb.h => include/video/samsung_fimd.h (74%)
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Mitch Bradley @ 2012-07-31 12:22 UTC (permalink / raw)
To: Thierry Reding
Cc: Alex Courbot, linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Stephen Warren, Greg Kroah-Hartman, Mark Brown,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <20120731105640.GD16155-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>
On 7/31/2012 6:56 PM, Thierry Reding wrote:
> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>> I wonder if using the same structure/array as input and output would
>>> simplify the API; the platform data would fill in the fields mentioned
>>> above, and power_seq_build() would parse those, then set other fields in
>>> the same structs to the looked-up handle values?
>>
>> The thing is that I am not sure what happens to the platform data
>> once probe() is done. Isn't it customary to mark it with __devinit
>> and have it freed after probing is successful?
>
> No, platform data should stay around forever. Otherwise, consider what
> would happen if your driver is built as a module and you unload and load
> it again.
>
>> More generally, I think it is a good practice to have data
>> structures tailored right for what they need to do - code with
>> members that are meaningful only at given points of an instance's
>> life tends to be more confusing.
>
> I agree. Furthermore the driver unload/reload would be another reason
> not to reuse platform data as the output of the build() function.
>
> But maybe what Stephen meant was more like filling a structure with data
> taken from the platform data and pass that to a resolve() function which
> would fill in the missing pieces like pointers to actual resources. I
> imagine a managed interface would become a little trickier to do using
> such an approach.
>
>>> If the nodes have a unit address (i.e. end in "@n"), which they will
>>> have to if all named "step" and there's more than one of them, then they
>>> will need a matching reg property. Equally, the parent node will need
>>> #address-cells and #size-cells too. So, the last couple lines would be:
>>>
>>> power-on-sequence {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> step@0 {
>>> reg = <0>;
>>
>> That's precisely what I would like to avoid - I don't need the steps
>> to be numbered and I certainly have no use for a reg property. Isn't
>> there a way to make it simpler?
>
> It's not technically valid to not have the reg property. Or
> #address-cells and #size-cells properties for that matter.
I'm not keen on this representation where individual steps are nodes.
That seems like it could end up being too "heavyweight" for a long sequence.
>
> Thierry
>
>
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
^ permalink raw reply
* Re: [PATCH 2/4] ARM: dts: mxs: Add alternative I2C muxing options for imx28
From: Fabio Estevam @ 2012-07-31 12:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1343730576-20494-3-git-send-email-maxime.ripard@free-electrons.com>
On Tue, Jul 31, 2012 at 7:29 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> + i2c0_pins_b: i2c0@1 {
> + reg = <1>;
> + fsl,pinmux-ids = <0x3001 0x3011>;
Can you please also list the name of the pin as comment (just like you
did for pwm4)?
This would allow us to quickly identify the muxing option without the
need of looking at the mxs pinctrl doc.
Regards,
Fabio Estevam
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Thierry Reding @ 2012-07-31 10:56 UTC (permalink / raw)
To: Alex Courbot
Cc: Stephen Warren, Stephen Warren, Simon Glass, Grant Likely,
Rob Herring, Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <5017B434.2010706-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]
On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
> On 07/31/2012 07:45 AM, Stephen Warren wrote:
> >I wonder if using the same structure/array as input and output would
> >simplify the API; the platform data would fill in the fields mentioned
> >above, and power_seq_build() would parse those, then set other fields in
> >the same structs to the looked-up handle values?
>
> The thing is that I am not sure what happens to the platform data
> once probe() is done. Isn't it customary to mark it with __devinit
> and have it freed after probing is successful?
No, platform data should stay around forever. Otherwise, consider what
would happen if your driver is built as a module and you unload and load
it again.
> More generally, I think it is a good practice to have data
> structures tailored right for what they need to do - code with
> members that are meaningful only at given points of an instance's
> life tends to be more confusing.
I agree. Furthermore the driver unload/reload would be another reason
not to reuse platform data as the output of the build() function.
But maybe what Stephen meant was more like filling a structure with data
taken from the platform data and pass that to a resolve() function which
would fill in the missing pieces like pointers to actual resources. I
imagine a managed interface would become a little trickier to do using
such an approach.
> >If the nodes have a unit address (i.e. end in "@n"), which they will
> >have to if all named "step" and there's more than one of them, then they
> >will need a matching reg property. Equally, the parent node will need
> >#address-cells and #size-cells too. So, the last couple lines would be:
> >
> > power-on-sequence {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > step@0 {
> > reg = <0>;
>
> That's precisely what I would like to avoid - I don't need the steps
> to be numbered and I certainly have no use for a reg property. Isn't
> there a way to make it simpler?
It's not technically valid to not have the reg property. Or
#address-cells and #size-cells properties for that matter.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Thierry Reding @ 2012-07-31 10:46 UTC (permalink / raw)
To: Alex Courbot
Cc: Simon Glass, Stephen Warren, Grant Likely, Rob Herring,
Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <5017AF5D.2010204-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4848 bytes --]
On Tue, Jul 31, 2012 at 07:11:41PM +0900, Alex Courbot wrote:
> On 07/31/2012 06:13 PM, Thierry Reding wrote:
> >>I don't see any need for microseconds myself - anybody sees use for
> >>finer-grained delays?
> >>
> >>Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.
> >
> >You might want to take a look at Documentation/timers/timers-howto.txt.
> >msleep() isn't very accurate for periods shorter than 20 ms.
>
> Ok, looks like usleep_range is the way to go here. In that case it
> would probably not hurt to specify delays in microseconds in the DT
> and platform data as well.
>
> >>>>+Device tree
> >>>>+-----------
> >>>>+All the same, power sequences can be encoded as device tree nodes. The following
> >>>>+properties and nodes are equivalent to the platform data defined previously:
> >>>>+
> >>>>+ power-supply = <&mydevice_reg>;
> >>>>+ enable-gpio = <&gpio 6 0>;
> >>>>+
> >>>>+ power-on-sequence {
> >>>>+ regulator@0 {
> >>>>+ id = "power";
> >>>
> >>>Is there a reason not to put the phandle here, like:
> >>>
> >>> id = <&mydevice_reg>;
> >>>
> >>>(or maybe 'device' instead of id?)
> >>
> >>There is one reason, but it might be a bad one. On Tegra, PWM
> >>phandle uses an extra cell to encode the duty-cycle the PWM should
> >>have when we call get_pwm().
> >
> >This is not only the case on Tegra, but it is the default unless a
> >driver specifically overrides it. The second cell specifies the index of
> >the PWM device within the PWM chip. The third cell doesn't specify the
> >duty cycle but the period of the PWM.
>
> Then I think there is a mistake in
> Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt:
>
> "the second cell is the duty cycle in nanoseconds."
Yes, that's a mistake. =\
> >>This makes it possible to address the
> >>same PWM with different phandles (and different duty cycles),
> >
> >How so? A phandle will always refer to a PWM chip. Paired with the
> >second cell, of_pwm_request() will resolve that into the correct PWM
> >device.
>
> For tegra, we can only address PWMs this way IIRC:
>
> pwm = <&pwm 2 5000000>;
>
> If we had <&pwm 2>, I agree that there would be no problem. But here
> the period of the PWM is also given - and in practice, we can
> request the same PWM using different phandles. For instance, if the
> above property was part of the power-on sequence, and the following
>
> pwm = <&pwm 2 0>;
>
> was part of power-off, how can I know that these two different
> phandles refer to the same PWM without calling pwm_get a second time
> and getting -EBUSY?
You should specify the same period regardless of the sequence. But you
are right, you still cannot request the device twice.
> Of course if the same period is specified for both, I will not have
> this issue as the phandles will be identical, but the possibility
> remains open that we are given a faulty tree here.
I think the phandle is in fact only the reference to the PWM chip, that
is: &pwm. The second cell, the PWM index, is part of the PWM specifier.
However the issue doesn't go away if you drop the period cell because
you still won't be able to request the PWM device a second time. How is
this solved for regulators and GPIOs? At least for GPIOs I'm pretty sure
that you can't request them more than once either.
> More generally speaking, wouldn't it make more sense to have the
> period/duty cycle of a PWM encoded into separate properties when
> needed and have the phandle just reference the PWM instance? This
> also seems to stand from an API point of view, since the period is
> not specified when invoking pwm_request or pwm_get, but set by its
> own pwm_set_period function?
The problem with specifying the period in a separate property is how to
map them to the correct PWM device. From a hardware description point of
view, making the period part of the specifier makes a lot of sense and
hardware description is what DT is about.
> On an unrelated note, I also don't understand why the period is also
> a parameter of pwm_config and why pwm_set_period does not do
> anything beyond setting a struct member that is returned by
> pwm_get_period (but maybe I am missing something here).
pwm_config() is the legacy API that we need to support for compatibility
reasons. Eventually this interface should probably be changed.
pwm_get_period() and pwm_set_period() were merely introduced to support
DT, but I could imagine them becoming the canonical way for configuring
PWM devices in the future, perhaps with a complementary
pwm_set_duty_cycle().
But first we need to convert drivers and users.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-07-31 10:32 UTC (permalink / raw)
To: Stephen Warren
Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fbdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org
In-Reply-To: <50170EA0.1010408@wwwdotorg.org>
On 07/31/2012 07:45 AM, Stephen Warren wrote:
>> +- Delay to wait before performing the action,
>> +- Delay to wait after performing the action.
>
> I don't see a need to have a delay both before and after an action;
> except at the start of the sequence, step n's post-delay is at the same
> point in the sequence as step n+1's pre-delay. Perhaps make a "delay"
> step type?
My first version used this actually - and you're right, having a "delay"
step type would be more flexible and less redundant.
>> +Both new resources and parameters can be introduced, but the goal is of course
>> +to keep things as simple and compact as possible.
>
>> +The platform data is a simple array of platform_power_seq_step instances, each
>
> Rather than jumping straight into platform data here, I'd expect an
> enumeration of legal resource types, and what actions can be performed
> on each, followed by a description of a sequence (very simply, just a
> list of actions and their parameters). This could be followed by a
> section describing the mapping of the abstract concepts to concrete
> platform data representation (and concrete device tree representation).
Keeping that in mind for the next revision.
>> +instance describing a step. The type as well as one of id or gpio members
>> +(depending on the type) must be specified. The last step must be of type
>> +POWER_SEQ_STOP.
>
> I'd certainly suggest having a step count rather than a sentinel value
> in the list.
As Thierry did - I think I will go that way.
>> Regulator and PWM resources are identified by name. GPIO are
>> +identified by number.
>
> That's a little implementation-specific. I guess it's entirely true for
> a platform data representation, but not when mapping this into device tree.
If we can come with a way to properly use phandles within DT sequences
(and we should), then this will only apply to platform data.
>> +You will need an instance of power_seq_resources to keep track of the resources
>> +that are already allocated. On success, the function returns a devm allocated
>> +resolved sequence that is ready to be passed to power_seq_run(). In case of
>> +failure, and error code is returned.
>
> If the result is devm-allocated, the function probably should be named
> devm_power_seq_build().
Right - more generally this needs to have both devm and non-devm variants.
> I wonder if using the same structure/array as input and output would
> simplify the API; the platform data would fill in the fields mentioned
> above, and power_seq_build() would parse those, then set other fields in
> the same structs to the looked-up handle values?
The thing is that I am not sure what happens to the platform data once
probe() is done. Isn't it customary to mark it with __devinit and have
it freed after probing is successful?
More generally, I think it is a good practice to have data structures
tailored right for what they need to do - code with members that are
meaningful only at given points of an instance's life tends to be more
confusing.
> You can make a custom devm free routine for the power_seq_resources
> itself, so the overall free'ing of the content can be triggered by devm,
> but the free'ing function can then call whatever non-devm APIs it wants
> for the non-devm-allocated members.
That sounds good.
>> +Device tree
>> +-----------
>> +All the same, power sequences can be encoded as device tree nodes. The following
>> +properties and nodes are equivalent to the platform data defined previously:
>> +
>> + power-supply = <&mydevice_reg>;
>> + enable-gpio = <&gpio 6 0>;
>> +
>> + power-on-sequence {
>> + regulator@0 {
>
> As Thierry mentioned, the step nodes should be named for the type of
> object they are (a "step") not the type or name of resource they act
> upon ("regulator" or "gpio").
Will fix that.
> If the nodes have a unit address (i.e. end in "@n"), which they will
> have to if all named "step" and there's more than one of them, then they
> will need a matching reg property. Equally, the parent node will need
> #address-cells and #size-cells too. So, the last couple lines would be:
>
> power-on-sequence {
> #address-cells = <1>;
> #size-cells = <0>;
> step@0 {
> reg = <0>;
That's precisely what I would like to avoid - I don't need the steps to
be numbered and I certainly have no use for a reg property. Isn't there
a way to make it simpler?
>> + id = "power";
>
> "id" is usually a name or identifier. I think you mean "type" or perhaps
> "action" here:
>
> type = "regulator";
> action = "enable";
>
> or:
>
> action = "enable-regulator";
Right, that was a clear misuse.
> Oh I see. That's a little confusing. Why not just reference the relevant
> resources directly in each step; something more like:
>
> gpio@1 {
> action = "enable-gpio";
> gpio = <&gpio 1 0>;
> };
>
> I guess that might make parsing/building a little harder, since you'd
> have to detect when you'd already done gpio_request() on a given GPIO
> and not repeat it or something like that, but to me this makes the DT a
> lot easier to comprehend.
You can see my reply to Thierry for the reason - the only issue with
that is caused by PWM phandles. If we overcome this, then I agree we
should use phandles. The code should not even get more complex as I have
to check whether a resource is already allocated with strings as well.
Thanks,
Alex.
^ permalink raw reply
* [PATCH 4/4] ARM: dts: mxs: add oled support for the cfa-10036
From: Maxime Ripard @ 2012-07-31 10:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1343730576-20494-1-git-send-email-maxime.ripard@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Brian Lilly <brian@crystalfontz.com>
---
arch/arm/boot/dts/imx28-cfa10036.dts | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm/boot/dts/imx28-cfa10036.dts b/arch/arm/boot/dts/imx28-cfa10036.dts
index c03a577..92784a9 100644
--- a/arch/arm/boot/dts/imx28-cfa10036.dts
+++ b/arch/arm/boot/dts/imx28-cfa10036.dts
@@ -33,11 +33,31 @@
};
apbx@80040000 {
+ pwm: pwm@80064000 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pwm4_pins_a>;
+ status = "okay";
+ };
+
duart: serial@80074000 {
pinctrl-names = "default";
pinctrl-0 = <&duart_pins_b>;
status = "okay";
};
+
+ i2c0: i2c@80058000 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c0_pins_b>;
+ status = "okay";
+
+ ssd1307: oled@3c {
+ compatible = "solomon,ssd1307fb-i2c";
+ reg = <0x3c>;
+ pwms = <&pwm 4 3000>;
+ oled-reset-gpio = <&gpio2 7 1>;
+ oled-reset-active-low;
+ };
+ };
};
};
--
1.7.9.5
^ permalink raw reply related
* [PATCH 3/4] ARM: dts: mxs: Add pwm4 muxing options for imx28
From: Maxime Ripard @ 2012-07-31 10:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1343730576-20494-1-git-send-email-maxime.ripard@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Brian Lilly <brian@crystalfontz.com>
---
arch/arm/boot/dts/imx28.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 44ce1fd..98891d8 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -461,6 +461,16 @@
fsl,pull-up = <0>;
};
+ pwm4_pins_a: pwm4@0 {
+ reg = <0>;
+ fsl,pinmux-ids = <
+ 0x31d0 /* MX28_PAD_PWM4__PWM_4 */
+ >;
+ fsl,drive-strength = <0>;
+ fsl,voltage = <1>;
+ fsl,pull-up = <0>;
+ };
+
lcdif_24bit_pins_a: lcdif-24bit@0 {
reg = <0>;
fsl,pinmux-ids = <
--
1.7.9.5
^ permalink raw reply related
* [PATCH 2/4] ARM: dts: mxs: Add alternative I2C muxing options for imx28
From: Maxime Ripard @ 2012-07-31 10:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1343730576-20494-1-git-send-email-maxime.ripard@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Brian Lilly <brian@crystalfontz.com>
---
arch/arm/boot/dts/imx28.dtsi | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 915db89..44ce1fd 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -410,6 +410,14 @@
fsl,pull-up = <1>;
};
+ i2c0_pins_b: i2c0@1 {
+ reg = <1>;
+ fsl,pinmux-ids = <0x3001 0x3011>;
+ fsl,drive-strength = <1>;
+ fsl,voltage = <1>;
+ fsl,pull-up = <1>;
+ };
+
saif0_pins_a: saif0@0 {
reg = <0>;
fsl,pinmux-ids = <
--
1.7.9.5
^ permalink raw reply related
* [PATCH 1/4] video: Add support for the Solomon SSD1307 OLED Controller
From: Maxime Ripard @ 2012-07-31 10:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1343730576-20494-1-git-send-email-maxime.ripard@free-electrons.com>
This patch adds support for the Solomon SSD1307 OLED
controller found on the Crystalfontz CFA10036 board.
This controller can drive a display with a resolution up
to 128x39 and can operate over I2C or SPI.
The current driver has only been tested on the CFA-10036,
that is using this controller over I2C to driver a 96x16
OLED screen.
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Brian Lilly <brian@crystalfontz.com>
---
.../devicetree/bindings/video/ssd1307fb.txt | 24 ++
drivers/video/Kconfig | 14 +
drivers/video/Makefile | 1 +
drivers/video/ssd1307fb.c | 418 ++++++++++++++++++++
4 files changed, 457 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/ssd1307fb.txt
create mode 100644 drivers/video/ssd1307fb.c
diff --git a/Documentation/devicetree/bindings/video/ssd1307fb.txt b/Documentation/devicetree/bindings/video/ssd1307fb.txt
new file mode 100644
index 0000000..791e14f
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/ssd1307fb.txt
@@ -0,0 +1,24 @@
+* Solomon SSD1307 Framebuffer Driver
+
+Required properties:
+ - compatible: Should be "solomon,ssd1307fb-<bus>". The only supported bus for
+ now is i2c.
+ - reg: Should contain address of the controller on the I2C bus. Most likely
+ 0x3c or 0x3d
+ - pwm: Should contain the pwm to use according to the OF device tree PWM
+ specification [0]
+ - oled-reset-gpio: Should contain the GPIO used to reset the OLED display
+
+Optional properties:
+ - oled-reset-active-low: Is the reset gpio is active on physical low?
+
+[0]: Documentation/devicetree/bindings/pwm/pwm.txt
+
+Examples:
+ssd1307: oled@3c {
+ compatible = "solomon,ssd1307fb-i2c";
+ reg = <0x3c>;
+ pwms = <&pwm 4 3000>;
+ oled-reset-gpio = <&gpio2 7 1>;
+ oled-reset-active-low;
+};
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 0217f74..21ae6dd 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2469,4 +2469,18 @@ config FB_SH_MOBILE_MERAM
Up to 4 memory channels can be configured, allowing 4 RGB or
2 YCbCr framebuffers to be configured.
+
+config FB_SSD1307
+ tristate "Solomon SSD1307 framebuffer support"
+ depends on FB && I2C
+ select FB_SYS_FOPS
+ select FB_SYS_FILLRECT
+ select FB_SYS_COPYAREA
+ select FB_SYS_IMAGEBLIT
+ select FB_DEFERRED_IO
+ select PWM
+ help
+ This driver implements support for the Solomon SSD1307
+ OLED controller over I2C.
+
endmenu
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index ee8dafb..6bbb72c 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -164,6 +164,7 @@ obj-$(CONFIG_FB_BFIN_7393) += bfin_adv7393fb.o
obj-$(CONFIG_FB_MX3) += mx3fb.o
obj-$(CONFIG_FB_DA8XX) += da8xx-fb.o
obj-$(CONFIG_FB_MXS) += mxsfb.o
+obj-$(CONFIG_FB_SSD1307) += ssd1307fb.o
# the test framebuffer is last
obj-$(CONFIG_FB_VIRTUAL) += vfb.o
diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
new file mode 100644
index 0000000..c705ab4
--- /dev/null
+++ b/drivers/video/ssd1307fb.c
@@ -0,0 +1,418 @@
+/*
+ * Driver for the Solomon SSD1307 OLED controler
+ *
+ * Copyright 2012 Free Electrons
+ *
+ * Licensed under the GPLv2 or later.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/i2c.h>
+#include <linux/fb.h>
+#include <linux/uaccess.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/pwm.h>
+#include <linux/delay.h>
+
+#define SSD1307FB_WIDTH 96
+#define SSD1307FB_HEIGHT 16
+
+#define SSD1307FB_DATA 0x40
+#define SSD1307FB_COMMAND 0x80
+
+#define SSD1307FB_CONTRAST 0x81
+#define SSD1307FB_SEG_REMAP_ON 0xa1
+#define SSD1307FB_DISPLAY_OFF 0xae
+#define SSD1307FB_DISPLAY_ON 0xaf
+#define SSD1307FB_START_PAGE_ADDRESS 0xb0
+
+struct ssd1307fb_par {
+ struct i2c_client *client;
+ struct fb_info *info;
+ struct pwm_device *pwm;
+ u32 pwm_period;
+ int reset;
+};
+
+static struct fb_fix_screeninfo ssd1307fb_fix __devinitdata = {
+ .id = "Solomon SSD1307",
+ .type = FB_TYPE_PACKED_PIXELS,
+ .visual = FB_VISUAL_MONO10,
+ .xpanstep = 0,
+ .ypanstep = 0,
+ .ywrapstep = 0,
+ .line_length = SSD1307FB_WIDTH / 8,
+ .accel = FB_ACCEL_NONE,
+};
+
+static struct fb_var_screeninfo ssd1307fb_var __devinitdata = {
+ .xres = SSD1307FB_WIDTH,
+ .yres = SSD1307FB_HEIGHT,
+ .xres_virtual = SSD1307FB_WIDTH,
+ .yres_virtual = SSD1307FB_HEIGHT,
+ .bits_per_pixel = 1,
+};
+
+static int ssd1307fb_write_array(struct i2c_client *client, u8 type, u8* cmd, u32 len)
+{
+ u8 *buf;
+ int ret = 0;
+
+ buf = kzalloc(len + 1, GFP_KERNEL);
+ if (!buf) {
+ dev_err(&client->dev, "Couldn't allocate sending buffer.\n");
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ buf[0] = type;
+ memcpy(buf + 1, cmd, len);
+
+ ret = i2c_master_send(client, buf, len + 1);
+ if (ret != len + 1) {
+ dev_err(&client->dev, "Couldn't send I2C command.\n");
+ goto error;
+ }
+
+error:
+ kfree(buf);
+out:
+ return ret;
+}
+
+static inline int ssd1307fb_write_cmd_array(struct i2c_client *client, u8* cmd, u32 len)
+{
+ return ssd1307fb_write_array(client, SSD1307FB_COMMAND, cmd, len);
+}
+
+static inline int ssd1307fb_write_cmd(struct i2c_client *client, u8 cmd)
+{
+ return ssd1307fb_write_cmd_array(client, &cmd, 1);
+}
+
+static inline int ssd1307fb_write_data_array(struct i2c_client *client, u8* cmd, u32 len)
+{
+ return ssd1307fb_write_array(client, SSD1307FB_DATA, cmd, len);
+}
+
+static inline int ssd1307fb_write_data(struct i2c_client *client, u8 data)
+{
+ return ssd1307fb_write_data_array(client, &data, 1);
+}
+
+static int ssd1307fb_set(struct i2c_client *client, u8 value)
+{
+ int i, j, ret;
+
+ for (i = 1; i <= (SSD1307FB_HEIGHT / 8); i++) {
+ ret = ssd1307fb_write_cmd(client, SSD1307FB_START_PAGE_ADDRESS + i);
+ if (ret < 0)
+ goto i2c_error;
+
+ ret = ssd1307fb_write_cmd(client, 0x00);
+ if (ret < 0)
+ goto i2c_error;
+
+ ret = ssd1307fb_write_cmd(client, 0x10);
+ if (ret < 0)
+ goto i2c_error;
+
+ for (j = 0; j < SSD1307FB_WIDTH; j++)
+ ssd1307fb_write_data(client, value);
+ }
+
+ return 0;
+
+i2c_error:
+ dev_err(&client->dev, "Couldn't send i2c command: %d\n", ret);
+ return ret;
+}
+
+static void ssd1307fb_update_display(struct ssd1307fb_par *par)
+{
+ u8 *vmem = par->info->screen_base;
+ int i, j, k;
+
+ /*
+ * The screen is divided in pages, each having a height of 8
+ * pixels, and the width of the screen. When sending a byte of
+ * data to the controller, it gives the 8 bits for the current
+ * column. I.e, the first byte are the 8 bits of the first
+ * column, then the 8 bits for the second column, etc.
+ *
+ *
+ * Representation of the screen, assuming it is 5 bits
+ * wide. Each letter-number combination is a bit that controls
+ * one pixel.
+ *
+ * A0 A1 A2 A3 A4
+ * B0 B1 B2 B3 B4
+ * C0 C1 C2 C3 C4
+ * D0 D1 D2 D3 D4
+ * E0 E1 E2 E3 E4
+ * F0 F1 F2 F3 F4
+ * G0 G1 G2 G3 G4
+ * H0 H1 H2 H3 H4
+ *
+ * If you want to update this screen, you need to send 5 bytes:
+ * (1) A0 B0 C0 D0 E0 F0 G0 H0
+ * (2) A1 B1 C1 D1 E1 F1 G1 H1
+ * (3) A2 B2 C2 D2 E2 F2 G2 H2
+ * (4) A3 B3 C3 D3 E3 F3 G3 H3
+ * (5) A4 B4 C4 D4 E4 F4 G4 H4
+ */
+
+ for (i = 0; i < (SSD1307FB_HEIGHT / 8); i++) {
+ ssd1307fb_write_cmd(par->client, SSD1307FB_START_PAGE_ADDRESS + (i + 1));
+ ssd1307fb_write_cmd(par->client, 0x00);
+ ssd1307fb_write_cmd(par->client, 0x10);
+
+ for (j = 0; j < SSD1307FB_WIDTH; j++) {
+ u8 buf = 0;
+ for (k = 0; k < 8; k++) {
+ u32 page_length = SSD1307FB_WIDTH * i;
+ u32 index = page_length + (SSD1307FB_WIDTH * k + j) / 8;
+ u8 byte = *(vmem + index);
+ u8 bit = byte & (1 << (7 - (j % 8)));
+ bit = bit >> (7 - (j % 8));
+ buf |= bit << k;
+ }
+ ssd1307fb_write_data(par->client, buf);
+ }
+ }
+
+ return;
+}
+
+
+static ssize_t ssd1307fb_write(struct fb_info *info, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct ssd1307fb_par *par = info->par;
+ unsigned long p = *ppos;
+ void *dst;
+ int err = 0;
+
+ dst = (void __force *) (info->screen_base + p);
+
+ if (copy_from_user(dst, buf, count))
+ err = -EFAULT;
+
+ if (!err)
+ *ppos += count;
+
+ ssd1307fb_update_display(par);
+
+ return (err) ? err : count;
+}
+
+static void ssd1307fb_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
+{
+ struct ssd1307fb_par *par = info->par;
+ sys_fillrect(info, rect);
+ ssd1307fb_update_display(par);
+}
+
+static void ssd1307fb_copyarea(struct fb_info *info, const struct fb_copyarea *area)
+{
+ struct ssd1307fb_par *par = info->par;
+ sys_copyarea(info, area);
+ ssd1307fb_update_display(par);
+}
+
+static void ssd1307fb_imageblit(struct fb_info *info, const struct fb_image *image)
+{
+ struct ssd1307fb_par *par = info->par;
+ sys_imageblit(info, image);
+ ssd1307fb_update_display(par);
+}
+
+static struct fb_ops ssd1307fb_ops = {
+ .owner = THIS_MODULE,
+ .fb_read = fb_sys_read,
+ .fb_write = ssd1307fb_write,
+ .fb_fillrect = ssd1307fb_fillrect,
+ .fb_copyarea = ssd1307fb_copyarea,
+ .fb_imageblit = ssd1307fb_imageblit,
+};
+
+static void ssd1307fb_deferred_io(struct fb_info *info,
+ struct list_head *pagelist)
+{
+ ssd1307fb_update_display(info->par);
+}
+
+static struct fb_deferred_io ssd1307fb_defio = {
+ .delay = HZ,
+ .deferred_io = ssd1307fb_deferred_io,
+};
+
+static int __devinit ssd1307fb_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+ struct fb_info *info;
+ u32 vmem_size = SSD1307FB_WIDTH * SSD1307FB_HEIGHT / 8;
+ struct ssd1307fb_par *par;
+ u8 *vmem;
+ int ret;
+
+ if (!client->dev.of_node) {
+ dev_err(&client->dev, "No device tree data found!\n");
+ ret = -EINVAL;
+ goto generic_error;
+ }
+
+ info = framebuffer_alloc(sizeof(struct ssd1307fb_par), &client->dev);
+ if (!info) {
+ ret = -ENOMEM;
+ goto generic_error;
+ }
+
+ vmem = devm_kzalloc(&client->dev, vmem_size, GFP_KERNEL);
+ if (!vmem) {
+ dev_err(&client->dev, "Couldn't allocate graphical memory.\n");
+ ret = -ENOMEM;
+ goto generic_error;
+ }
+
+ info->fbops = &ssd1307fb_ops;
+ info->fix = ssd1307fb_fix;
+ info->fbdefio = &ssd1307fb_defio;
+
+ info->var = ssd1307fb_var;
+ info->var.red.length = 1;
+ info->var.red.offset = 0;
+ info->var.green.length = 1;
+ info->var.green.offset = 0;
+ info->var.blue.length = 1;
+ info->var.blue.offset = 0;
+
+ info->screen_base = (u8 __force __iomem *)vmem;
+ info->fix.smem_start = (unsigned long)vmem;
+ info->fix.smem_len = vmem_size;
+
+ fb_deferred_io_init(info);
+
+ par = info->par;
+ par->info = info;
+ par->client = client;
+
+ par->reset = of_get_named_gpio(client->dev.of_node,
+ "oled-reset-gpio", 0);
+ if (gpio_is_valid(par->reset)) {
+ int flags = GPIOF_OUT_INIT_HIGH;
+ if (of_get_property(client->dev.of_node,
+ "oled-reset-active-low", NULL))
+ flags = GPIOF_OUT_INIT_LOW;
+ ret = devm_gpio_request_one(&client->dev, par->reset,
+ flags, "oled-reset");
+ if (ret) {
+ dev_err(&client->dev,
+ "failed to request gpio %d: %d\n",
+ par->reset, ret);
+ goto reset_oled_error;
+ }
+ }
+
+ par->pwm = pwm_get(&client->dev, NULL);
+ if (IS_ERR(par->pwm)) {
+ dev_err(&client->dev, "Could not get PWM from device tree!\n");
+ ret = PTR_ERR(par->pwm);
+ goto pwm_error;
+ }
+
+ par->pwm_period = pwm_get_period(par->pwm);
+
+ dev_dbg(&client->dev, "Using PWM%d with a %dns period.\n", par->pwm->pwm, par->pwm_period);
+
+ ret = register_framebuffer(info);
+ if (ret) {
+ dev_err(&client->dev, "Couldn't register the framebuffer\n");
+ goto fbreg_error;
+ }
+
+ i2c_set_clientdata(client, info);
+
+ /* Reset the screen */
+ gpio_set_value(par->reset, 1);
+ udelay(4);
+ gpio_set_value(par->reset, 0);
+ udelay(4);
+
+ /* Enable the PWM */
+ pwm_config(par->pwm, par->pwm_period / 2, par->pwm_period);
+ pwm_enable(par->pwm);
+
+ /* Map column 127 of the OLED to segment 0 */
+ ret = ssd1307fb_write_cmd(client, SSD1307FB_SEG_REMAP_ON);
+ if (ret < 0) {
+ dev_err(&client->dev, "Couldn't remap the screen.\n");
+ goto remap_error;
+ }
+
+ /* Turn on the display */
+ ret = ssd1307fb_write_cmd(client, SSD1307FB_DISPLAY_ON);
+ if (ret < 0) {
+ dev_err(&client->dev, "Couldn't turn the display on.\n");
+ goto remap_error;
+ }
+
+ dev_info(&client->dev, "fb%d: %s framebuffer device registered, using %d bytes of video memory\n", info->node, info->fix.id, vmem_size);
+
+ return 0;
+
+remap_error:
+ unregister_framebuffer(info);
+ pwm_disable(par->pwm);
+fbreg_error:
+ pwm_put(par->pwm);
+pwm_error:
+reset_oled_error:
+ fb_deferred_io_cleanup(info);
+ framebuffer_release(info);
+generic_error:
+ return ret;
+}
+
+static int __devexit ssd1307fb_remove(struct i2c_client *client)
+{
+ struct fb_info *info = i2c_get_clientdata(client);
+ struct ssd1307fb_par *par = info->par;
+ unregister_framebuffer(info);
+ pwm_disable(par->pwm);
+ pwm_put(par->pwm);
+ fb_deferred_io_cleanup(info);
+ framebuffer_release(info);
+
+ return 0;
+}
+
+static const struct i2c_device_id ssd1307fb_i2c_id[] = {
+ { "ssd1307fb", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, ssd1307fb_i2c_id);
+
+static const struct of_device_id ssd1307fb_of_match[] = {
+ { .compatible = "solomon,ssd1307fb-i2c" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, ssd1307fb_of_match);
+
+static struct i2c_driver ssd1307fb_driver = {
+ .probe = ssd1307fb_probe,
+ .remove = __devexit_p(ssd1307fb_remove),
+ .id_table = ssd1307fb_i2c_id,
+ .driver = {
+ .name = "ssd1307fb",
+ .of_match_table = of_match_ptr(ssd1307fb_of_match),
+ .owner = THIS_MODULE,
+ },
+};
+
+module_i2c_driver(ssd1307fb_driver);
+
+MODULE_DESCRIPTION("FB driver for the Solomon SSD1307 OLED controler");
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
+MODULE_LICENSE("GPL");
--
1.7.9.5
^ permalink raw reply related
* [PATCHv2 0/4] Add support for the OLED in the CFA10036
From: Maxime Ripard @ 2012-07-31 10:29 UTC (permalink / raw)
To: linux-arm-kernel
Hi everyone,
This patchset adds support for the solomon SSD1307 OLED controller present
in the CFA-10036 board.
It first adds the framebuffer driver for this controller, and then the
needed bits to enable it in the cfa10036 dts.
Thanks,
Maxime
Changes from v1:
* Did some factorisation of the ssd1307fb_write* functions.
* Filled the smem_start field of the fb_fix_screeninfo structure
* Fixed the broken pwm declaration
* Change the names of oled-reset-gpios and reset-active-low properties in the
device tree to oled-reset-gpio and oled-reset-active-low for consistency
Maxime Ripard (4):
video: Add support for the Solomon SSD1307 OLED Controller
ARM: dts: mxs: Add alternative I2C muxing options for imx28
ARM: dts: mxs: Add pwm4 muxing options for imx28
ARM: dts: mxs: add oled support for the cfa-10036
.../devicetree/bindings/video/ssd1307fb.txt | 24 ++
arch/arm/boot/dts/imx28-cfa10036.dts | 20 +
arch/arm/boot/dts/imx28.dtsi | 18 +
drivers/video/Kconfig | 14 +
drivers/video/Makefile | 1 +
drivers/video/ssd1307fb.c | 418 ++++++++++++++++++++
6 files changed, 495 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/ssd1307fb.txt
create mode 100644 drivers/video/ssd1307fb.c
--
1.7.9.5
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Thierry Reding @ 2012-07-31 10:19 UTC (permalink / raw)
To: Alex Courbot
Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring,
Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <5017AA87.2040503-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 9833 bytes --]
On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote:
> On 07/30/2012 08:33 PM, Thierry Reding wrote:
> >>+You will need an instance of power_seq_resources to keep track of the resources
> >>+that are already allocated. On success, the function returns a devm allocated
> >>+resolved sequence that is ready to be passed to power_seq_run(). In case of
> >>+failure, and error code is returned.
> >
> >I don't quite understand why the struct power_seq_resources is needed.
> >Can this not be stored within power_seq?
>
> power_seq_resources serves two purposes:
> 1) When parsing sequences, it keeps track of the resources we have
> already allocated to avoid getting the same resource twice
> 2) On cleanup, it cleans the resources that needs to be freed (i.e.
> those that are not devm-handled).
>
> 2) can certainly be removed either by enforcing use of devm, or by
> doing reference counting. 1) seems more difficult to avoid - we need
> to keep track of the resources we already own between calls to
> power_seq_build(). I'd certainly be glad to remove that structure
> from public view and simplify the code if that is possible though.
I still don't see the problem. Managing the resources should be part of
the power_seq core and shouldn't be visible to users. Maybe what you are
worried about is that you may need the same resource both for a power-up
and a power-down sequence? I can see how that would require a global
list of resources.
However I still think it would be easier to encapsulate that completely.
Maybe another level of abstraction is required. You could for example
add another type to encapsulate several power sequences and that could
keep a list of used resources. I can't think of a good name, but maybe
the following DT snippet clarifies what I mean:
power-sequences {
#address-cells = <1>;
#size-cells = <0>;
sequence@0 {
name = "up";
#address-cells = <1>;
#size-cells = <0>;
step@0 {
...
};
...
};
sequence@1 {
name = "down";
#address-cells = <1>;
#size-cells = <0>;
step@0 {
...
};
...
};
};
If you add a name property like this, you could extend the API to
support running a named sequence:
power_seq_run(seq, "up");
...
power_seq_run(seq, "down);
> >Also, is there some way we can make the id property for GPIOs not
> >require the -gpio suffix? If the resource type is already GPIO, then it
> >seems redundant to add -gpio to the ID.
>
> There is unfortunately an inconsistency between the way regulators
> and GPIOs are gotten by name. regulator_get(id) will expect to find
> a property named "id-supply", while gpio_request_one(id) expects a
> property named exactly "id". To workaround this we could sprintf the
> correct property name from a non-suffixed property name within the
> driver, but I think this actually speaks more in favor of having
> phandles directly into the sequences.
Yes, if it can be made to work by specifying the phandle directly that
is certainly better.
> >>+static int power_seq_step_run(struct power_seq_step *step)
> >>+{
> >>+ int err = 0;
> >>+
> >>+ if (step->params.pre_delay)
> >>+ mdelay(step->params.pre_delay);
> >>+
> >>+ switch (step->resource->type) {
> >>+#ifdef CONFIG_REGULATOR
> >>+ case POWER_SEQ_REGULATOR:
> >>+ if (step->params.enable)
> >>+ err = regulator_enable(step->resource->regulator);
> >>+ else
> >>+ err = regulator_disable(step->resource->regulator);
> >>+ break;
> >>+#endif
> >>+#ifdef CONFIG_PWM
> >>+ case POWER_SEQ_PWM:
> >>+ if (step->params.enable)
> >>+ err = pwm_enable(step->resource->pwm);
> >>+ else
> >>+ pwm_disable(step->resource->pwm);
> >>+ break;
> >>+#endif
> >>+#ifdef CONFIG_GPIOLIB
> >>+ case POWER_SEQ_GPIO:
> >>+ gpio_set_value_cansleep(step->resource->gpio,
> >>+ step->params.enable);
> >>+ break;
> >>+#endif
> >
> >This kind of #ifdef'ery is quite ugly. I don't know if adding separate
> >*_run() functions for each type of resource would be any better, though.
> >Alternatively, maybe POWER_SEQ should depend on the REGULATOR, PWM and
> >GPIOLIB symbols to side-step the issue completely?
>
> If it is not realistic to consider a kernel built without regulator,
> pwm or gpiolib support, then we might as well do that. But isn't
> that a possibility?
I'd say that anything complex enough to make use of power-sequencing
probably has all of these enabled anyway. But maybe I'm not very
qualified to judge.
>
> >>+ if (!seq) return 0;
> >
> >I don't think this is acceptable according to the coding style. Also,
> >perhaps returning -EINVAL would be more meaningful?
>
> I neglected running checkpatch before submitting, apologies for
> that. The return value seems correct to me, a NULL sequence has no
> effect.
But seq == NULL should never happen anyway, right?
>
> >>+
> >>+ while (seq->resource) {
> >
> >Perhaps this should check for POWER_SEQ_STOP instead?
>
> There is no resource for POWER_SEQ_STOP - therefore, a NULL resource
> is used instead.
Still, you use POWER_SEQ_STOP as an explicit sentinel to mark the end of
a sequence, so intuitively I'd be looking for that as a stop condition.
> >>+typedef struct platform_power_seq_step platform_power_seq;
> >
> >Why are the parameters kept in a separate structure? What are the
> >disadvantages of keeping the in the sequence step structure directly?
>
> This ensures the same parameters are used for the platform data and
> resolved sequences, and also ensures they are all copied correctly
> using memcpy. But maybe I am just making something complex out of
> something that ought to be simpler.
>
> >>+struct power_seq_step {
> >>+ struct power_seq_resource *resource;
> >>+ struct power_step_params params;
> >>+};
> >>+typedef struct power_seq_step power_seq;
> >
> >Would it make sense to make the struct power_seq opaque? I don't see why
> >anyone but the power_seq code should access the internals.
>
> I would like to do that actually. The issue is that it did not work
> go well with the legacy pwm_backlight behavior: a power sequence
> needs to be constructed out of a PWM obtained through
> pwm_request(int pwm_id, char *label) and this behavior cannot be
> emulated using the new platform data interface (which only works
> with pwm_get()). But if I remove this old behavior, then I could
> make power_seq opaque. I don't think many drivers are using it. What
> do you think?
I don't see how that is relevant here, since this power-sequencing code
is supposed to be generic and not tied to any specific implementation.
Can you explain further?
In any case you shouldn't be using pwm_request() in new code.
> >For resource
> >managing it might also be easier to separate struct power_seq_step and
> >struct power_seq, making the power_seq basically something like:
> >
> > struct power_seq {
> > struct power_seq_step *steps;
> > unsigned int num_steps;
> > };
> >
> >Perhaps a name field can be included for diagnostic purposes.
>
> Yes, looks like we are going in that direction. If this can be made
> private then the number of public data structures will not be too
> confusing (platform data only, basically).
Yes, that sounds like a much cleaner approach.
>
> >>+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
> >>+ platform_power_seq *pseq);
> >
> >I already mentioned this above: I fail to see why the ress parameter is
> >needed here. It is an internal implementation detail of the power
> >sequence code. Maybe a better place would be to include it within the
> >struct power_seq.
>
> Problem is that I need to track which resources are already
> allocated between calls to power_seq_build(). Even if I attach the
> resources into struct power_seq, they won't be attainable by the
> next call. So I'm afraid we are bound to pass a tracking structure
> at least to power_seq_build.
I think this could be solved nicely by what I proposed earlier.
> >>+/**
> >>+ * Free all the resources previously allocated by power_seq_allocate_resources.
> >>+ */
> >>+void power_seq_free_resources(power_seq_resources *ress);
> >>+
> >>+/**
> >>+ * Run the given power sequence. Returns 0 on success, error code in case of
> >>+ * failure.
> >>+ */
> >>+int power_seq_run(struct device *dev, power_seq *seq);
> >
> >I think the API is too fine grained here. From a user's point of view,
> >I'd expect a sequence like this:
> >
> > seq = power_seq_build(dev, sequence);
> > ...
> > power_seq_run(seq);
> > ...
> > power_seq_free(seq);
> >
> >Perhaps with managed variants where the power_seq_free() is executed
> >automatically:
> >
> > seq = devm_power_seq_build(dev, sequence);
> > ...
> > power_seq_run(seq);
>
> I agree. On top of that, of_parse_power_seq() should directly return
> a resolved power sequence, not the platform data.
I'm not sure. The idiom seems to be to use DT as an alternative source
for platform data, which I guess is due to most drivers already using
platform data. But it has the advantage that after you have the platform
data, be it from DT or directly specified, the subsequent code remains
the same.
Of course you could provide a separate of_build_power_seq() that wraps
both steps for convenience.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* RE: [PATCH] video: da8xx-fb: do clock reset of revision 2 LCDC before enabling
From: Manjunathappa, Prakash @ 2012-07-31 10:18 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1342799471-10729-1-git-send-email-prakash.pm@ti.com>
Hi,
On Mon, Jul 30, 2012 at 02:44:33, Florian Tobias Schandinat wrote:
> On 07/20/2012 03:51 PM, Manjunathappa, Prakash wrote:
> > As in specification software reset should be applied for several
> > cycles before bringing it out of reset. Without this patch
> > particularly during suspend and resume clock reset is not guaranteed
> > to happen.
> >
> > Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
>
> Applied. But it would be better if you made the patch dependencies
> clear, as I didn't consider some of your patches mature enough for this
> merge window. Now I had to figure out why my final build failed and
> applied the patch below.
>
Thanks Florian Tobias Schandinat. I will take care to mention dependencies
in future.
Regards,
Prakash
>
> Best regards,
>
> Florian Tobias Schandinat
>
> ---
> commit a0239073fd75489d25575cf3aaf71ab55b416020
> Author: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> Date: Sun Jul 29 16:47:40 2012 +0000
>
> da8xx-fb: fix compile issue due to missing include
>
> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
>
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index ca9943a..7ae9d53 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -32,6 +32,7 @@
> #include <linux/console.h>
> #include <linux/spinlock.h>
> #include <linux/slab.h>
> +#include <linux/delay.h>
> #include <linux/lcm.h>
> #include <video/da8xx-fb.h>
> #include <asm/div64.h>
>
>
> > ---
> > drivers/video/da8xx-fb.c | 12 ++++++++----
> > 1 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> > index 3d2d0d1..4440292 100644
> > --- a/drivers/video/da8xx-fb.c
> > +++ b/drivers/video/da8xx-fb.c
> > @@ -262,10 +262,18 @@ static inline void lcd_enable_raster(void)
> > {
> > u32 reg;
> >
> > + /* Put LCDC in reset for several cycles */
> > + if (lcd_revision = LCD_VERSION_2)
> > + /* Write 1 to reset LCDC */
> > + lcdc_write(LCD_CLK_MAIN_RESET, LCD_CLK_RESET_REG);
> > + mdelay(1);
> > +
> > /* Bring LCDC out of reset */
> > if (lcd_revision = LCD_VERSION_2)
> > lcdc_write(0, LCD_CLK_RESET_REG);
> > + mdelay(1);
> >
> > + /* Above reset sequence doesnot reset register context */
> > reg = lcdc_read(LCD_RASTER_CTRL_REG);
> > if (!(reg & LCD_RASTER_ENABLE))
> > lcdc_write(reg | LCD_RASTER_ENABLE, LCD_RASTER_CTRL_REG);
> > @@ -279,10 +287,6 @@ static inline void lcd_disable_raster(void)
> > reg = lcdc_read(LCD_RASTER_CTRL_REG);
> > if (reg & LCD_RASTER_ENABLE)
> > lcdc_write(reg & ~LCD_RASTER_ENABLE, LCD_RASTER_CTRL_REG);
> > -
> > - if (lcd_revision = LCD_VERSION_2)
> > - /* Write 1 to reset LCDC */
> > - lcdc_write(LCD_CLK_MAIN_RESET, LCD_CLK_RESET_REG);
> > }
> >
> > static void lcd_blit(int load_mode, struct da8xx_fb_par *par)
>
>
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-07-31 10:15 UTC (permalink / raw)
To: Stephen Warren
Cc: Rob Herring, Stephen Warren, Thierry Reding, Simon Glass,
Grant Likely, Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <50170A14.6000201-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
On 07/31/2012 07:26 AM, Stephen Warren wrote:
> On 07/30/2012 09:44 AM, Rob Herring wrote:
>> On 07/27/2012 07:05 AM, Alexandre Courbot wrote:
>>> Some device drivers (panel backlights especially) need to follow precise
>>> sequences for powering on and off, involving gpios, regulators, PWMs
>>> with a precise powering order and delays to respect between each steps.
>>> These sequences are board-specific, and do not belong to a particular
>>> driver - therefore they have been performed by board-specific hook
>>> functions to far.
>>>
>>> With the advent of the device tree and of ARM kernels that are not
>>> board-tied, we cannot rely on these board-specific hooks anymore but
>>> need a way to implement these sequences in a portable manner. This patch
>>> introduces a simple interpreter that can execute such power sequences
>>> encoded either as platform data or within the device tree.
>>>
>>
>> Why not? We'll always have some amount of board code. The key is to
>> limit parts that are just data. I'm not sure this is something that
>> should be in devicetree.
>>
>> Perhaps what is needed is a better way to hook into the driver like
>> notifiers?
>
> I would answer that by asking the reverse question - why should we have
> to put some data in DT, and some data into board files still?
>
> I'd certainly argue that the sequence of which GPIOs/regulators/PWMs to
> manipulate is just data.
>
> To be honest, if we're going to have to put some parts of a board's
> configuration into board files anyway, then the entirety of DT seems
> useless; I'd far rather see all the configuration in one cohesive place
> than arbitrarily split into two/n different locations - that would make
> everything harder to maintain.
Also, having these sequences into the DT would allow an older kernel to
boot on and correctly initialize a newer board with - which is also part
of the DT's purpose if I am not mistaken.
Alex.
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-07-31 10:11 UTC (permalink / raw)
To: Thierry Reding
Cc: Simon Glass, Stephen Warren, Grant Likely, Rob Herring,
Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <20120731091324.GA15557-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>
On 07/31/2012 06:13 PM, Thierry Reding wrote:
>> I don't see any need for microseconds myself - anybody sees use for
>> finer-grained delays?
>>
>> Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.
>
> You might want to take a look at Documentation/timers/timers-howto.txt.
> msleep() isn't very accurate for periods shorter than 20 ms.
Ok, looks like usleep_range is the way to go here. In that case it would
probably not hurt to specify delays in microseconds in the DT and
platform data as well.
>>>> +Device tree
>>>> +-----------
>>>> +All the same, power sequences can be encoded as device tree nodes. The following
>>>> +properties and nodes are equivalent to the platform data defined previously:
>>>> +
>>>> + power-supply = <&mydevice_reg>;
>>>> + enable-gpio = <&gpio 6 0>;
>>>> +
>>>> + power-on-sequence {
>>>> + regulator@0 {
>>>> + id = "power";
>>>
>>> Is there a reason not to put the phandle here, like:
>>>
>>> id = <&mydevice_reg>;
>>>
>>> (or maybe 'device' instead of id?)
>>
>> There is one reason, but it might be a bad one. On Tegra, PWM
>> phandle uses an extra cell to encode the duty-cycle the PWM should
>> have when we call get_pwm().
>
> This is not only the case on Tegra, but it is the default unless a
> driver specifically overrides it. The second cell specifies the index of
> the PWM device within the PWM chip. The third cell doesn't specify the
> duty cycle but the period of the PWM.
Then I think there is a mistake in
Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt:
"the second cell is the duty cycle in nanoseconds."
>> This makes it possible to address the
>> same PWM with different phandles (and different duty cycles),
>
> How so? A phandle will always refer to a PWM chip. Paired with the
> second cell, of_pwm_request() will resolve that into the correct PWM
> device.
For tegra, we can only address PWMs this way IIRC:
pwm = <&pwm 2 5000000>;
If we had <&pwm 2>, I agree that there would be no problem. But here the
period of the PWM is also given - and in practice, we can request the
same PWM using different phandles. For instance, if the above property
was part of the power-on sequence, and the following
pwm = <&pwm 2 0>;
was part of power-off, how can I know that these two different phandles
refer to the same PWM without calling pwm_get a second time and getting
-EBUSY?
Of course if the same period is specified for both, I will not have this
issue as the phandles will be identical, but the possibility remains
open that we are given a faulty tree here.
More generally speaking, wouldn't it make more sense to have the
period/duty cycle of a PWM encoded into separate properties when needed
and have the phandle just reference the PWM instance? This also seems to
stand from an API point of view, since the period is not specified when
invoking pwm_request or pwm_get, but set by its own pwm_set_period function?
On an unrelated note, I also don't understand why the period is also a
parameter of pwm_config and why pwm_set_period does not do anything
beyond setting a struct member that is returned by pwm_get_period (but
maybe I am missing something here).
Alex.
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-07-31 9:51 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring,
Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fbdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org
In-Reply-To: <20120730113323.GA7303@avionic-0098.adnet.avionic-design.de>
On 07/30/2012 08:33 PM, Thierry Reding wrote:
>> +You will need an instance of power_seq_resources to keep track of the resources
>> +that are already allocated. On success, the function returns a devm allocated
>> +resolved sequence that is ready to be passed to power_seq_run(). In case of
>> +failure, and error code is returned.
>
> I don't quite understand why the struct power_seq_resources is needed.
> Can this not be stored within power_seq?
power_seq_resources serves two purposes:
1) When parsing sequences, it keeps track of the resources we have
already allocated to avoid getting the same resource twice
2) On cleanup, it cleans the resources that needs to be freed (i.e.
those that are not devm-handled).
2) can certainly be removed either by enforcing use of devm, or by doing
reference counting. 1) seems more difficult to avoid - we need to keep
track of the resources we already own between calls to
power_seq_build(). I'd certainly be glad to remove that structure from
public view and simplify the code if that is possible though.
>> +
>> +A resolved power sequence returned by power_seq_build can be run by
>> +power_run_run():
>> +
>> +int power_seq_run(struct device *dev, power_seq *seq);
>
> Why is the struct device required here? It already is passed during the
> call to pwm_seq_build(), so perhaps you should keep a reference to it
> within struct power_seq?
The device is only needed for printing error messages. But as you point
later, maybe messages should not be printed there at all. I will try to
remove that parameter.
>> +It returns 0 if the sequence has successfully been run, or an error code if a
>> +problem occured.
>> +
>> +Finally, some resources that cannot be allocated through devm need to be freed
>> +manually. Therefore, be sure to call power_seq_free_resources() in your device
>> +remove function:
>> +
>> +void power_seq_free_resources(power_seq_resources *ress);
>
> Could this not also be handled by a managed version? If a power_seq is
> always managed, then I would assume that it also takes care of freeing
> the resources, even if the resources have no managed equivalents.
Right.
> Perhaps it would also make sense to provide non-managed version of these
> functions. I think that would make the managed versions easier and more
> canonical to implement.
A power_seq is a single block of memory, so that should be reasonnably
doable indeed. Let me think a little bit more about that.
>> +Device tree
>> +-----------
>> +All the same, power sequences can be encoded as device tree nodes. The following
>> +properties and nodes are equivalent to the platform data defined previously:
>> +
>> + power-supply = <&mydevice_reg>;
>> + enable-gpio = <&gpio 6 0>;
>> +
>> + power-on-sequence {
>> + regulator@0 {
>> + id = "power";
>> + enable;
>> + post-delay = <10>;
>> + };
>> + gpio@1 {
>> + id = "enable-gpio";
>> + enable;
>> + };
>> + };
>> +
>> +Note that first, the phandles of the regulator and gpio used in the sequences
>> +are defined as properties. Then the sequence references them through the id
>> +property of every step. The name of sub-properties defines the type of the step.
>> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
>> +sequentially.
>
> I think there has been quite some discussion regarding the naming of
> subnodes and the conclusion seems to have been to name them uniformly
> after what they represent. As such the power-on-sequence subnodes should
> be called step@0, step@1, etc. However, that will require the addition
> of a property to define the type of resource.
That's fine I guess - just adds some footprint to the DT, but nothing crazy.
> Also, is there some way we can make the id property for GPIOs not
> require the -gpio suffix? If the resource type is already GPIO, then it
> seems redundant to add -gpio to the ID.
There is unfortunately an inconsistency between the way regulators and
GPIOs are gotten by name. regulator_get(id) will expect to find a
property named "id-supply", while gpio_request_one(id) expects a
property named exactly "id". To workaround this we could sprintf the
correct property name from a non-suffixed property name within the
driver, but I think this actually speaks more in favor of having
phandles directly into the sequences.
>> +config POWER_SEQ
>> + bool
>> + default n
>> +
>
> "default n" is already the default, so you can drop that line.
Did that, thanks.
>> +#ifdef CONFIG_OF
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#endif
>
> I think you don't need the CONFIG_OF guard around these. Both of.h and
> of_gpio.h can be included unconditionally and actually contain dummy
> definitions for the public functions in the !OF case.
Fixed, thanks.
>> +static int power_seq_step_run(struct power_seq_step *step)
>> +{
>> + int err = 0;
>> +
>> + if (step->params.pre_delay)
>> + mdelay(step->params.pre_delay);
>> +
>> + switch (step->resource->type) {
>> +#ifdef CONFIG_REGULATOR
>> + case POWER_SEQ_REGULATOR:
>> + if (step->params.enable)
>> + err = regulator_enable(step->resource->regulator);
>> + else
>> + err = regulator_disable(step->resource->regulator);
>> + break;
>> +#endif
>> +#ifdef CONFIG_PWM
>> + case POWER_SEQ_PWM:
>> + if (step->params.enable)
>> + err = pwm_enable(step->resource->pwm);
>> + else
>> + pwm_disable(step->resource->pwm);
>> + break;
>> +#endif
>> +#ifdef CONFIG_GPIOLIB
>> + case POWER_SEQ_GPIO:
>> + gpio_set_value_cansleep(step->resource->gpio,
>> + step->params.enable);
>> + break;
>> +#endif
>
> This kind of #ifdef'ery is quite ugly. I don't know if adding separate
> *_run() functions for each type of resource would be any better, though.
> Alternatively, maybe POWER_SEQ should depend on the REGULATOR, PWM and
> GPIOLIB symbols to side-step the issue completely?
If it is not realistic to consider a kernel built without regulator, pwm
or gpiolib support, then we might as well do that. But isn't that a
possibility?
>> + if (!seq) return 0;
>
> I don't think this is acceptable according to the coding style. Also,
> perhaps returning -EINVAL would be more meaningful?
I neglected running checkpatch before submitting, apologies for that.
The return value seems correct to me, a NULL sequence has no effect.
>> +
>> + while (seq->resource) {
>
> Perhaps this should check for POWER_SEQ_STOP instead?
There is no resource for POWER_SEQ_STOP - therefore, a NULL resource is
used instead.
>> + if ((err = power_seq_step_run(seq++))) {
>> + dev_err(dev, "error %d while running power sequence!\n",
>> + err);
>
> For this kind of diagnostics it could be useful to have a name
> associated with the power sequence. But I'm not sure that making the
> power sequence code output an error here is the best solution. I find it
> to be annoying when core code starts outputting too many error codes. In
> this case it's particularily easy to catch the errors in the caller.
Giving names to power sequences sounds like a good idea. Let me see how
this can be done. It might require some more data structuring.
>> +
>> + while ((child = of_get_next_child(node, child)))
>> + cpt++;
>
> for_each_child_of_node()?
>
>> +
>> + /* allocate one more step to signal end of sequence */
>> + ret = devm_kzalloc(dev, sizeof(*ret) * (cpt + 1), GFP_KERNEL);
>> + if (!ret)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + cpt = 0;
>> + while ((child = of_get_next_child(node, child))) {
>
> Here as well.
Ah, didn't know that. Thanks.
>> + /* first pass to count the number of steps to allocate */
>> + for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++);
>
> Wouldn't it be easier to pass around the number of steps in the sequence
> instead of having to count in various places? This would be more along
> the lines of how struct platform_device defines associated resources.
My goal was to limit the number of data structures, but if we add a name
to power sequences, we can add a steps count as well.
>> +
>> + if (!cpt)
>> + return seq;
>
> Perhaps this should return an error-code as well? I find it nice to not
> have to handle NULL specially when using ERR_PTR et al.
Agreed.
>> +typedef enum {
>> + POWER_SEQ_STOP = 0,
>> + POWER_SEQ_REGULATOR,
>> + POWER_SEQ_PWM,
>> + POWER_SEQ_GPIO,
>> + POWER_SEQ_MAX,
>> +} power_res_type;
>
> Maybe the prefix power_seq should be used here as well, so:
> power_seq_res_type.
Definitely.
>> +typedef struct list_head power_seq_resources;
>
> No type definitions like this, please. Also, why define this particular
> type globally?
I will move that into a proper structure with a name and number of steps.
>> +
>> +struct power_step_params {
>> + /* enable the resource if 1, disable if 0 */
>> + bool enable;
>> + /* delay (in ms) to wait before executing the step */
>> + int pre_delay;
>> + /* delay (in ms) to wait after executing the step */
>> + int post_delay;
>
> unsigned int for the delays?
Yup.
>> +typedef struct platform_power_seq_step platform_power_seq;
>
> Why are the parameters kept in a separate structure? What are the
> disadvantages of keeping the in the sequence step structure directly?
This ensures the same parameters are used for the platform data and
resolved sequences, and also ensures they are all copied correctly using
memcpy. But maybe I am just making something complex out of something
that ought to be simpler.
>> +struct power_seq_step {
>> + struct power_seq_resource *resource;
>> + struct power_step_params params;
>> +};
>> +typedef struct power_seq_step power_seq;
>
> Would it make sense to make the struct power_seq opaque? I don't see why
> anyone but the power_seq code should access the internals.
I would like to do that actually. The issue is that it did not work go
well with the legacy pwm_backlight behavior: a power sequence needs to
be constructed out of a PWM obtained through pwm_request(int pwm_id,
char *label) and this behavior cannot be emulated using the new platform
data interface (which only works with pwm_get()). But if I remove this
old behavior, then I could make power_seq opaque. I don't think many
drivers are using it. What do you think?
> For resource
> managing it might also be easier to separate struct power_seq_step and
> struct power_seq, making the power_seq basically something like:
>
> struct power_seq {
> struct power_seq_step *steps;
> unsigned int num_steps;
> };
>
> Perhaps a name field can be included for diagnostic purposes.
Yes, looks like we are going in that direction. If this can be made
private then the number of public data structures will not be too
confusing (platform data only, basically).
>> +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
>> + platform_power_seq *pseq);
>
> I already mentioned this above: I fail to see why the ress parameter is
> needed here. It is an internal implementation detail of the power
> sequence code. Maybe a better place would be to include it within the
> struct power_seq.
Problem is that I need to track which resources are already allocated
between calls to power_seq_build(). Even if I attach the resources into
struct power_seq, they won't be attainable by the next call. So I'm
afraid we are bound to pass a tracking structure at least to
power_seq_build.
>> +/**
>> + * Free all the resources previously allocated by power_seq_allocate_resources.
>> + */
>> +void power_seq_free_resources(power_seq_resources *ress);
>> +
>> +/**
>> + * Run the given power sequence. Returns 0 on success, error code in case of
>> + * failure.
>> + */
>> +int power_seq_run(struct device *dev, power_seq *seq);
>
> I think the API is too fine grained here. From a user's point of view,
> I'd expect a sequence like this:
>
> seq = power_seq_build(dev, sequence);
> ...
> power_seq_run(seq);
> ...
> power_seq_free(seq);
>
> Perhaps with managed variants where the power_seq_free() is executed
> automatically:
>
> seq = devm_power_seq_build(dev, sequence);
> ...
> power_seq_run(seq);
I agree. On top of that, of_parse_power_seq() should directly return a
resolved power sequence, not the platform data.
> Generally I really like where this is going.
Thanks - I really appreciate your review.
Alex.
^ permalink raw reply
* inquiry
From: roboth roli company @ 2012-07-31 9:25 UTC (permalink / raw)
i am robothroli, Purchase manager from roli Merchant Ltd. We are
Import/export Company based in taiwan. We are interested in purchasing
your product and I would like to make an inquiry. Please inform me on:
Sample availability and price
Minimum order quantity
FOB Prices
Sincerely
Purchase Manager
robothroli
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Thierry Reding @ 2012-07-31 9:16 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Alexandre Courbot, Stephen Warren, Simon Glass,
Grant Likely, Greg Kroah-Hartman, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ
In-Reply-To: <20120730154706.GL4468-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 760 bytes --]
On Mon, Jul 30, 2012 at 04:47:06PM +0100, Mark Brown wrote:
> On Mon, Jul 30, 2012 at 10:44:29AM -0500, Rob Herring wrote:
> > On 07/27/2012 07:05 AM, Alexandre Courbot wrote:
>
> > > + power-on-sequence {
> > > + regulator@0 {
> > > + id = "power";
> > > + enable;
>
> > What do this mean? Isn't this implied for a regulator?
>
> I assume you might have some sequences which need some things to be
> turned off for some reason; it at least seems to be something you'd want
> to design for.
Furthermore there is the power-off-sequence equivalent, which you use
for instance when you turn off the panel. Typically they would do the
inverse of the power-on-sequence, so turning off a regulator is
definitiely required.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Thierry Reding @ 2012-07-31 9:13 UTC (permalink / raw)
To: Alex Courbot
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Stephen Warren, Greg Kroah-Hartman, Mark Brown,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <50179933.9090501-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2282 bytes --]
On Tue, Jul 31, 2012 at 05:37:07PM +0900, Alex Courbot wrote:
> Hi Simon,
>
> On 07/30/2012 08:00 PM, Simon Glass wrote:
> >For the delay, I think milliseconds is reasonable. I suppose there is
> >no reasonable need for microseconds?
>
> I don't see any need for microseconds myself - anybody sees use for
> finer-grained delays?
>
> Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.
You might want to take a look at Documentation/timers/timers-howto.txt.
msleep() isn't very accurate for periods shorter than 20 ms.
> >>+Device tree
> >>+-----------
> >>+All the same, power sequences can be encoded as device tree nodes. The following
> >>+properties and nodes are equivalent to the platform data defined previously:
> >>+
> >>+ power-supply = <&mydevice_reg>;
> >>+ enable-gpio = <&gpio 6 0>;
> >>+
> >>+ power-on-sequence {
> >>+ regulator@0 {
> >>+ id = "power";
> >
> >Is there a reason not to put the phandle here, like:
> >
> > id = <&mydevice_reg>;
> >
> >(or maybe 'device' instead of id?)
>
> There is one reason, but it might be a bad one. On Tegra, PWM
> phandle uses an extra cell to encode the duty-cycle the PWM should
> have when we call get_pwm().
This is not only the case on Tegra, but it is the default unless a
driver specifically overrides it. The second cell specifies the index of
the PWM device within the PWM chip. The third cell doesn't specify the
duty cycle but the period of the PWM.
> This makes it possible to address the
> same PWM with different phandles (and different duty cycles),
How so? A phandle will always refer to a PWM chip. Paired with the
second cell, of_pwm_request() will resolve that into the correct PWM
device.
> which
> causes an issue to know whether a PWM is already used in a sequence
> (potentially resulting in multiple get_pwm calls on the same PWM,
> and also opens the door to ambiguities in behavior (what is the
> correct duty-cycle to use if several different values are passed?)
You can't request a PWM multiple times. The second call well return
-EBUSY, or rather ERR_PTR(-EBUSY).
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: DISPC: Use msleep instead of blocking mdelay
From: Tomi Valkeinen @ 2012-07-31 8:53 UTC (permalink / raw)
To: Jassi Brar; +Cc: a0393947, linux-omap, linux-fbdev
In-Reply-To: <1343138635-2802-1-git-send-email-jaswinder.singh@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 563 bytes --]
On Tue, 2012-07-24 at 19:33 +0530, Jassi Brar wrote:
> We have no reason to block in the error handler workqueue, so use msleep.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
> drivers/video/omap2/dss/dispc.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
Thanks, applied.
As a side note, I hate that error handler =). It was a quick emergency
hack to somehow handle certain situations, but generally it sucks.
However, I don't really have a good idea what to do in case we get
errors from DSS HW.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Do not require a VDDS_DSI regulator on am35xx
From: Tomi Valkeinen @ 2012-07-31 8:45 UTC (permalink / raw)
To: Raphael Assenat; +Cc: linux-omap, linux-fbdev, Chandrabhanu Mahapatra
In-Reply-To: <20120719200429.GD3850@renkinjitsu.usine.8d.com>
[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]
On Thu, 2012-07-19 at 16:04 -0400, Raphael Assenat wrote:
> On our AM3505 based board, dpi.c complains that there is no VDSS_DSI regulator
> and the framebuffer cannot be enabled. However, this check does not seem to
> apply to AM3505/17 chips.
>
> I am not the first facing this issue, see this thread from Nov. 2011:
> http://marc.info/?l=linux-omap&m=132147745930213&w=2
>
> The string 'vdds_dsi' does appear once in the technical reference manual[1]
> but there is no corresponding power pin on the package[2]. I failed to
> locate any signal that could be an equivalent. I am trying to obtain some
> clarifications on TI's forum[3]...
>
> In any case, I am currently running with the patch below. In order to keep
> cpu_is_xx uses to a minimum, I check for am35xx once at init time and allow
> dpi.vdds_dsi_reg to be NULL from then on, getting rid of all the other
> cpu_is_omap34xx uses in the process.
>
> Your comments would be appreciated. Please also consider for merging.
VDDS_DSI is used to power up some of the DSS pins on OMAP3. I don't know
why the HW was designed like that... If you have a correct image without
the power, then obviously it's not needed.
We don't currently deal with AM3xxx SoCs in any way in the driver. It's
difficult enough trying to handle just OMAP DSS versions, and now we
need to add AM3xxx to the mix. Sigh =).
However, I don't want to apply this patch, as we're trying to remove the
cpu_is checks (soc_is goes in the same category).
I guess we need to add entries for the AM3xxx SoCs in the
dss_features.c.
Any idea what other differences AM3xxx has compared to OMAP3?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-07-31 8:37 UTC (permalink / raw)
To: Simon Glass
Cc: Stephen Warren, Thierry Reding, Grant Likely, Rob Herring,
Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fbdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org
In-Reply-To: <CAPnjgZ0H2xrJcL-ytMaX11iYrrhCg7LEM00u_NgEaveM4gHMPw@mail.gmail.com>
Hi Simon,
On 07/30/2012 08:00 PM, Simon Glass wrote:
> For the delay, I think milliseconds is reasonable. I suppose there is
> no reasonable need for microseconds?
I don't see any need for microseconds myself - anybody sees use for
finer-grained delays?
Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.
>> +Device tree
>> +-----------
>> +All the same, power sequences can be encoded as device tree nodes. The following
>> +properties and nodes are equivalent to the platform data defined previously:
>> +
>> + power-supply = <&mydevice_reg>;
>> + enable-gpio = <&gpio 6 0>;
>> +
>> + power-on-sequence {
>> + regulator@0 {
>> + id = "power";
>
> Is there a reason not to put the phandle here, like:
>
> id = <&mydevice_reg>;
>
> (or maybe 'device' instead of id?)
There is one reason, but it might be a bad one. On Tegra, PWM phandle
uses an extra cell to encode the duty-cycle the PWM should have when we
call get_pwm(). This makes it possible to address the same PWM with
different phandles (and different duty cycles), which causes an issue to
know whether a PWM is already used in a sequence (potentially resulting
in multiple get_pwm calls on the same PWM, and also opens the door to
ambiguities in behavior (what is the correct duty-cycle to use if
several different values are passed?)
Maybe the problem lies in how PWM phandles are handled - if duty-cycle
was not part of the information, we would not have this problem.
>> +Note that first, the phandles of the regulator and gpio used in the sequences
>> +are defined as properties. Then the sequence references them through the id
>> +property of every step. The name of sub-properties defines the type of the step.
>> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
>> +sequentially.
>
> For the regulator and gpio types I think you only have an enable. For
> the pwm, what is the intended binding? Is that documented elsewhere?
Same thing, enable/disable which would call pwm_enable and pwm_disable.
One could also image an additional property to set the duty cycle if it
can be taken off the phandle.
> Also it might be worth mentioning how you get a struct power_seq from
> an fdt node, and perhaps given an example of a device which has an
> attached node, so we can see how it is referenced from the device
> (of_parse_power_seq I think). Do put the sequence inside the device
> node or reference it with a phandle?
Yes, this definitely needs more documentation - especially the DT part.
Thanks,
Alex.
^ permalink raw reply
* RE: [PATCH 1/3] Move FIMD register headers to include/video/
From: Marek Szyprowski @ 2012-07-31 8:32 UTC (permalink / raw)
To: 'Jingoo Han', 'Leela Krishna Amudala'
Cc: linux-arm-kernel, linux-samsung-soc, dri-devel, linux-fbdev,
ben-linux, inki.dae, kgene.kim, joshi
In-Reply-To: <000701cd6ef5$24315c70$6c941550$%han@samsung.com>
Hello,
On Tuesday, July 31, 2012 10:19 AM Jingoo Han wrote:
> On Tuesday, July 31, 2012 3:28 PM Marek Szyprowski wrote:
> >
> > Hello,
> >
> > On Tuesday, July 31, 2012 2:48 AM Jingoo Han wrote:
> >
> > > On Monday, July 30, 2012 8:16 PM, Leela Krishna Amudala wrote:
> > > >
> > > > Hello Jingoo Han,
> > > >
> > > > On Mon, Jul 30, 2012 at 2:23 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > > > > On Monday, July 30, 2012 5:45 PM, Leela Krishna Amudala wrote:
> > > > >>
> > > > >> Moved the contents of regs-fb-v4.h and regs-fb.h from arch side
> > > > >> to include/video/samsung_fimd.h
> > > > >>
> > > > >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> > > > >> ---
> > > > >> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 -------
> > > > >> arch/arm/plat-samsung/include/plat/regs-fb.h | 403 -----------------
> > > > >> include/video/samsung_fimd.h | 533 +++++++++++++++++++++++
> > > > >> 3 files changed, 533 insertions(+), 562 deletions(-)
> > > > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> > > > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
> > > > >> create mode 100644 include/video/samsung_fimd.h
> > > > >>
> > > > >> +*/
> > > > >> +
> > > > >> +/*FIMD V8 REG OFFSET */
> > > > >> +#define FIMD_V8_VIDTCON0 (0x20010)
> > > > >> +#define FIMD_V8_VIDTCON1 (0x20014)
> > > > >> +#define FIMD_V8_VIDTCON2 (0x20018)
> > > > >> +#define FIMD_V8_VIDTCON3 (0x2001C)
> > > > >> +#define FIMD_V8_VIDCON1 (0x20004)
> > >
> > >
> > > How about using soc_is_exynos5250()?
> > >
> > > +#define VIDTCON0 (soc_is_exynos5250() ? \
> > > + (0x20010) : (0x10))
> > >
> > > In this case, the FIMD driver does not need to change.
> > > Also, one binary is available.
> >
> > Please don't mix two methods of runtime detection. FIMD driver (s3c-fb) already
> > has runtime hw detection based on platform device id. Adding such detection for
> > exynos5 to DRM FIMD driver should not be a big issue too.
>
> Marek,
> Then, do you want use like this?
>
> #define VIDTCON0 (0x10)
> +#define FIMD_V8_VIDTCON0 (0x20010)
>
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -1924,7 +1924,7 @@ static struct s3c_fb_driverdata s3c_fb_data_exynos4 = {
> static struct s3c_fb_driverdata s3c_fb_data_exynos5 = {
> .variant = {
> .nr_windows = 5,
> - .vidtcon = VIDTCON0,
> + .vidtcon = FIMD_V8_VIDTCON0,
Yes, this method looks good imo. Maybe even having something like vidtcon_base in
variant structure will be enough to cover all VIDTCON0-3 registers.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox