* [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and machine.
@ 2010-11-23 7:16 Inki Dae
2010-11-27 17:07 ` Sylwester Nawrocki
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Inki Dae @ 2010-11-23 7:16 UTC (permalink / raw)
To: linux-arm-kernel
clock.c
- added dsim clock gate.
dev-dsim.c
- platform and machine specific definitions.
Now just supports only MACHINE GONI so for other machines,
mipi_1_1v_name and mipi_1_8v_name values should be changed to
proper regulator name at each machine file.
setup-dsim.c
- platform specific definitions.
Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/mach-s5pv210/Kconfig | 11 ++
arch/arm/mach-s5pv210/Makefile | 2 +
arch/arm/mach-s5pv210/clock.c | 6 +
arch/arm/mach-s5pv210/dev-dsim.c | 149 +++++++++++++++++++++++
arch/arm/mach-s5pv210/include/mach/map.h | 4 +
arch/arm/mach-s5pv210/include/mach/regs-clock.h | 3 +-
arch/arm/mach-s5pv210/mach-goni.c | 12 ++
arch/arm/mach-s5pv210/setup-dsim.c | 144 ++++++++++++++++++++++
arch/arm/plat-s5p/Kconfig | 5 +
9 files changed, 335 insertions(+), 1 deletions(-)
create mode 100644 arch/arm/mach-s5pv210/dev-dsim.c
create mode 100644 arch/arm/mach-s5pv210/setup-dsim.c
diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
index 862f239..447701f 100644
--- a/arch/arm/mach-s5pv210/Kconfig
+++ b/arch/arm/mach-s5pv210/Kconfig
@@ -53,6 +53,12 @@ config S5PV210_SETUP_SDHCI_GPIO
help
Common setup code for SDHCI gpio.
+config S5PV210_SETUP_DSIM
+ bool
+ depends on REGULATOR
+ help
+ Common setup code for MIPI-DSIM.
+
menu "S5PC110 Machines"
config MACH_AQUILA
@@ -68,6 +74,7 @@ config MACH_AQUILA
select S5P_DEV_ONENAND
select S5PV210_SETUP_FB_24BPP
select S5PV210_SETUP_SDHCI
+ select S5PV210_SETUP_DSIM
help
Machine support for the Samsung Aquila target based on S5PC110 SoC
@@ -86,12 +93,14 @@ config MACH_GONI
select S3C_DEV_I2C2
select S3C_DEV_USB_HSOTG
select S5P_DEV_ONENAND
+ select S5P_DEV_DSIM
select SAMSUNG_DEV_KEYPAD
select S5PV210_SETUP_FB_24BPP
select S5PV210_SETUP_I2C1
select S5PV210_SETUP_I2C2
select S5PV210_SETUP_KEYPAD
select S5PV210_SETUP_SDHCI
+ select S5PV210_SETUP_DSIM
help
Machine support for Samsung GONI board
S5PC110(MCP) is one of package option of S5PV210
@@ -107,6 +116,7 @@ config MACH_SMDKC110
select S5PV210_SETUP_I2C1
select S5PV210_SETUP_I2C2
select S5PV210_SETUP_IDE
+ select S5PV210_SETUP_DSIM
help
Machine support for Samsung SMDKC110
S5PC110(MCP) is one of package option of S5PV210
@@ -135,6 +145,7 @@ config MACH_SMDKV210
select S5PV210_SETUP_IDE
select S5PV210_SETUP_KEYPAD
select S5PV210_SETUP_SDHCI
+ select S5PV210_SETUP_DSIM
help
Machine support for Samsung SMDKV210
diff --git a/arch/arm/mach-s5pv210/Makefile b/arch/arm/mach-s5pv210/Makefile
index ff1a0db..f6a6443 100644
--- a/arch/arm/mach-s5pv210/Makefile
+++ b/arch/arm/mach-s5pv210/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_MACH_TORBRECK) += mach-torbreck.o
obj-y += dev-audio.o
obj-$(CONFIG_S3C64XX_DEV_SPI) += dev-spi.o
+obj-$(CONFIG_S5P_DEV_DSIM) += dev-dsim.o
obj-$(CONFIG_S5PV210_SETUP_FB_24BPP) += setup-fb-24bpp.o
obj-$(CONFIG_S5PV210_SETUP_I2C1) += setup-i2c1.o
@@ -37,3 +38,4 @@ obj-$(CONFIG_S5PV210_SETUP_IDE) += setup-ide.o
obj-$(CONFIG_S5PV210_SETUP_KEYPAD) += setup-keypad.o
obj-$(CONFIG_S5PV210_SETUP_SDHCI) += setup-sdhci.o
obj-$(CONFIG_S5PV210_SETUP_SDHCI_GPIO) += setup-sdhci-gpio.o
+obj-$(CONFIG_S5PV210_SETUP_DSIM) += setup-dsim.o
diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
index 019c3a6..9cbf244 100644
--- a/arch/arm/mach-s5pv210/clock.c
+++ b/arch/arm/mach-s5pv210/clock.c
@@ -347,6 +347,12 @@ static struct clk init_clocks_disable[] = {
.enable = s5pv210_clk_ip0_ctrl,
.ctrlbit = (1 << 26),
}, {
+ .name = "dsim",
+ .id = -1,
+ .parent = &clk_hclk_dsys.clk,
+ .enable = s5pv210_clk_ip1_ctrl,
+ .ctrlbit = (1<<2),
+ }, {
.name = "otg",
.id = -1,
.parent = &clk_hclk_psys.clk,
diff --git a/arch/arm/mach-s5pv210/dev-dsim.c b/arch/arm/mach-s5pv210/dev-dsim.c
new file mode 100644
index 0000000..3f43d72
--- /dev/null
+++ b/arch/arm/mach-s5pv210/dev-dsim.c
@@ -0,0 +1,149 @@
+/* linux/arch/arm/plat-s5pc11x/dev-dsim.c
+ *
+ * Copyright 2009 Samsung Electronics
+ * InKi Dae <inki.dae@samsung.com>
+ *
+ * S5PC1XX series device definition for MIPI-DSIM
+ *
+ * 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.
+*/
+
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/fb.h>
+
+#include <mach/map.h>
+#include <mach/irqs.h>
+
+#include <plat/devs.h>
+#include <plat/cpu.h>
+#include <plat/fb.h>
+
+#include <plat/dsim.h>
+#include <plat/mipi_ddi.h>
+
+static struct dsim_config dsim_info = {
+ /* main frame fifo auto flush at VSYNC pulse */
+ .auto_flush = false,
+ .eot_disable = false,
+
+ .auto_vertical_cnt = false,
+ .hse = false,
+ .hfp = false,
+ .hbp = false,
+ .hsa = false,
+
+ .e_no_data_lane = DSIM_DATA_LANE_2,
+ .e_byte_clk = DSIM_PLL_OUT_DIV8,
+
+ /*
+ * =====================+ * | P | M | S | MHz |
+ * -------------------------------------------
+ * | 3 | 100 | 3 | 100 |
+ * | 3 | 100 | 2 | 200 |
+ * | 3 | 63 | 1 | 252 |
+ * | 4 | 100 | 1 | 300 |
+ * | 4 | 110 | 1 | 330 |
+ * | 12 | 350 | 1 | 350 |
+ * | 3 | 100 | 1 | 400 |
+ * | 4 | 150 | 1 | 450 |
+ * | 3 | 118 | 1 | 472 |
+ * | 12 | 250 | 0 | 500 |
+ * | 4 | 100 | 0 | 600 |
+ * | 3 | 81 | 0 | 648 |
+ * | 3 | 88 | 0 | 704 |
+ * | 3 | 90 | 0 | 720 |
+ * | 3 | 100 | 0 | 800 |
+ * | 12 | 425 | 0 | 850 |
+ * | 4 | 150 | 0 | 900 |
+ * | 12 | 475 | 0 | 950 |
+ * | 6 | 250 | 0 | 1000 |
+ * -------------------------------------------
+ */
+
+ /* 472MHz: LSI Recommended */
+ .p = 3,
+ .m = 118,
+ .s = 1,
+
+ /* D-PHY PLL stable time spec :min = 200usec ~ max 400usec */
+ .pll_stable_time = 400,
+
+ .esc_clk = 10 * 1000000, /* escape clk : 10MHz */
+
+ /* stop state holding counter after bta change count 0 ~ 0xfff */
+ .stop_holding_cnt = 0x0f,
+ .bta_timeout = 0xff, /* bta timeout 0 ~ 0xff */
+ .rx_timeout = 0xffff, /* lp rx timeout 0 ~ 0xffff */
+
+ .e_lane_swap = DSIM_NO_CHANGE,
+};
+
+/* define ddi platform data based on MIPI-DSI. */
+static struct mipi_ddi_platform_data mipi_ddi_pd = {
+ .cmd_write = s5p_dsim_wr_data,
+ .cmd_read = NULL,
+ .get_dsim_frame_done = s5p_dsim_get_frame_done_status,
+ .clear_dsim_frame_done = s5p_dsim_clear_frame_done,
+ .change_dsim_transfer_mode = s5p_dsim_change_transfer_mode,
+ .get_fb_frame_done = NULL,
+ .trigger = NULL,
+};
+
+static struct dsim_lcd_config dsim_lcd_info = {
+ .e_interface = DSIM_COMMAND,
+ .parameter[DSI_VIRTUAL_CH_ID] = (unsigned int)DSIM_VIRTUAL_CH_0,
+ .parameter[DSI_FORMAT] = (unsigned int)DSIM_24BPP_888,
+ .parameter[DSI_VIDEO_MODE_SEL] = (unsigned int)DSIM_BURST,
+ .mipi_ddi_pd = (void *)&mipi_ddi_pd,
+};
+
+static struct resource s5p_dsim_resource[] = {
+ [0] = {
+ .start = S5PV210_PA_DSI,
+ .end = S5PV210_PA_DSI + S5PV210_SZ_DSI - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ [1] = {
+ .start = IRQ_MIPIDSI,
+ .end = IRQ_MIPIDSI,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct s5p_platform_dsim dsim_platform_data = {
+ .clk_name = "dsim",
+ .mipi_1_1v_name = "vmipi_1.1v",
+ .mipi_1_8v_name = "vmipi_1.8v",
+ .dsim_info = &dsim_info,
+ .dsim_lcd_info = &dsim_lcd_info,
+
+ .mipi_power = s5p_dsim_mipi_power,
+ .part_reset = s5p_dsim_part_reset,
+ .init_d_phy = s5p_dsim_init_d_phy,
+ .get_fb_frame_done = NULL,
+ .trigger = NULL,
+
+ .platform_rev = 1,
+
+ /*
+ * the stable time of needing to write data on SFR
+ * when the mipi mode becomes LP mode.
+ */
+ .delay_for_stabilization = 600,
+};
+
+struct platform_device s5p_device_dsim = {
+ .name = "s5p-dsim",
+ .id = 0,
+ .num_resources = ARRAY_SIZE(s5p_dsim_resource),
+ .resource = s5p_dsim_resource,
+ .dev = {
+ .platform_data = (void *)&dsim_platform_data,
+ },
+};
diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-s5pv210/include/mach/map.h
index 861d7fe..1ad2dad 100644
--- a/arch/arm/mach-s5pv210/include/mach/map.h
+++ b/arch/arm/mach-s5pv210/include/mach/map.h
@@ -69,6 +69,10 @@
#define S5PV210_PA_FB (0xF8000000)
+/* MIPI-DSI */
+#define S5PV210_PA_DSI (0xFA500000)
+#define S5PV210_SZ_DSI SZ_1M
+
#define S5PV210_PA_FIMC0 (0xFB200000)
#define S5PV210_PA_FIMC1 (0xFB300000)
#define S5PV210_PA_FIMC2 (0xFB400000)
diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
index ebaabe0..c8b9366 100644
--- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
+++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
@@ -196,7 +196,8 @@
#define S5P_OTHERS_USB_SIG_MASK (1 << 16)
/* MIPI */
-#define S5P_MIPI_DPHY_EN (3)
+#define S5P_MIPI_DPHY_EN (3 << 0)
+#define S5P_MIPI_M_RESETN (1 << 1)
/* S5P_DAC_CONTROL */
#define S5P_DAC_ENABLE (1)
diff --git a/arch/arm/mach-s5pv210/mach-goni.c b/arch/arm/mach-s5pv210/mach-goni.c
index b1dcf96..500cc1b 100644
--- a/arch/arm/mach-s5pv210/mach-goni.c
+++ b/arch/arm/mach-s5pv210/mach-goni.c
@@ -269,10 +269,18 @@ static void __init goni_tsp_init(void)
/* MAX8998 regulators */
#if defined(CONFIG_REGULATOR_MAX8998) || defined(CONFIG_REGULATOR_MAX8998_MODULE)
+static struct regulator_consumer_supply goni_ldo3_consumers[] = {
+ REGULATOR_SUPPLY("vmipi_1.1v", ""),
+};
+
static struct regulator_consumer_supply goni_ldo5_consumers[] = {
REGULATOR_SUPPLY("vmmc", "s3c-sdhci.0"),
};
+static struct regulator_consumer_supply goni_ldo7_consumers[] = {
+ REGULATOR_SUPPLY("vmipi_1.8v", ""),
+};
+
static struct regulator_init_data goni_ldo2_data = {
.constraints = {
.name = "VALIVE_1.1V",
@@ -294,6 +302,8 @@ static struct regulator_init_data goni_ldo3_data = {
.apply_uV = 1,
.always_on = 1,
},
+ .num_consumer_supplies = ARRAY_SIZE(goni_ldo3_consumers),
+ .consumer_supplies = goni_ldo3_consumers,
};
static struct regulator_init_data goni_ldo4_data = {
@@ -333,6 +343,8 @@ static struct regulator_init_data goni_ldo7_data = {
.apply_uV = 1,
.always_on = 1,
},
+ .num_consumer_supplies = ARRAY_SIZE(goni_ldo7_consumers),
+ .consumer_supplies = goni_ldo7_consumers,
};
static struct regulator_init_data goni_ldo8_data = {
diff --git a/arch/arm/mach-s5pv210/setup-dsim.c b/arch/arm/mach-s5pv210/setup-dsim.c
new file mode 100644
index 0000000..874efa0
--- /dev/null
+++ b/arch/arm/mach-s5pv210/setup-dsim.c
@@ -0,0 +1,144 @@
+/*
+ * S5PC110 MIPI-DSIM driver.
+ *
+ * Author: InKi Dae <inki.dae@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/regulator/consumer.h>
+
+#include <mach/map.h>
+#include <mach/regs-clock.h>
+
+#include <plat/dsim.h>
+#include <plat/clock.h>
+#include <plat/regs-dsim.h>
+
+static int s5p_dsim_enable_d_phy(struct dsim_global *dsim, unsigned int enable)
+{
+ unsigned int reg;
+
+ WARN_ON(dsim = NULL);
+
+ reg = readl(S5P_MIPI_CONTROL) & ~(1 << 0);
+ reg |= (enable << 0);
+ writel(reg, S5P_MIPI_CONTROL);
+
+ dev_dbg(dsim->dev, "%s : %x\n", __func__, reg);
+
+ return 0;
+}
+
+static int s5p_dsim_enable_dsi_master(struct dsim_global *dsim,
+ unsigned int enable)
+{
+ unsigned int reg;
+
+ WARN_ON(dsim = NULL);
+
+ reg = readl(S5P_MIPI_CONTROL) & ~(1 << 2);
+ reg |= (enable << 2);
+ writel(reg, S5P_MIPI_CONTROL);
+
+ dev_dbg(dsim->dev, "%s : %x\n", __func__, reg);
+
+ return 0;
+}
+
+int s5p_dsim_part_reset(struct dsim_global *dsim)
+{
+ WARN_ON(dsim = NULL);
+
+ writel(S5P_MIPI_M_RESETN, S5P_MIPI_PHY_CON0);
+
+ dev_dbg(dsim->dev, "%s\n", __func__);
+
+ return 0;
+}
+
+int s5p_dsim_init_d_phy(struct dsim_global *dsim)
+{
+ WARN_ON(dsim = NULL);
+
+ /**
+ * DPHY and Master block must be enabled at the system initialization
+ * step before data access from/to DPHY begins.
+ */
+ s5p_dsim_enable_d_phy(dsim, 1);
+
+ s5p_dsim_enable_dsi_master(dsim, 1);
+
+ dev_dbg(dsim->dev, "%s\n", __func__);
+
+ return 0;
+}
+
+int s5p_dsim_mipi_power(struct dsim_global *dsim, struct regulator *p_mipi_1_1v,
+ struct regulator *p_mipi_1_8v, unsigned int enable)
+{
+ int ret = -1;
+
+ WARN_ON(dsim = NULL);
+
+ if (IS_ERR(p_mipi_1_1v) || IS_ERR(p_mipi_1_8v)) {
+ dev_err(dsim->dev, "p_mipi_1_1v or p_mipi_1_8v is NULL.\n");
+ return -EINVAL;
+ }
+
+ if (enable) {
+ if (p_mipi_1_1v)
+ ret = regulator_enable(p_mipi_1_1v);
+
+ if (ret < 0) {
+ dev_err(dsim->dev,
+ "failed to enable regulator mipi_1_1v.\n");
+ return ret;
+ }
+
+ if (p_mipi_1_8v)
+ ret = regulator_enable(p_mipi_1_8v);
+
+ if (ret < 0) {
+ dev_err(dsim->dev,
+ "failed to enable regulator mipi_1_8v.\n");
+ return ret;
+ }
+ } else {
+ if (p_mipi_1_1v)
+ ret = regulator_force_disable(p_mipi_1_1v);
+ if (ret < 0) {
+ dev_err(dsim->dev,
+ "failed to disable regulator mipi_1_1v.\n");
+ return ret;
+ }
+
+ if (p_mipi_1_8v)
+ ret = regulator_force_disable(p_mipi_1_8v);
+ if (ret < 0) {
+ dev_err(dsim->dev,
+ "failed to disable regulator mipi_1_8v.\n");
+ return ret;
+ }
+ }
+
+ return ret;
+}
diff --git a/arch/arm/plat-s5p/Kconfig b/arch/arm/plat-s5p/Kconfig
index 65dbfa8..73a34e0 100644
--- a/arch/arm/plat-s5p/Kconfig
+++ b/arch/arm/plat-s5p/Kconfig
@@ -56,3 +56,8 @@ config S5P_DEV_ONENAND
bool
help
Compile in platform device definition for OneNAND controller
+
+config S5P_DEV_DSIM
+ bool
+ help
+ Compile in platform device definitions for MIPI-DSI
--
1.5.4.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and machine.
2010-11-23 7:16 [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and machine Inki Dae
@ 2010-11-27 17:07 ` Sylwester Nawrocki
2010-11-30 1:30 ` Inki Dae
2010-12-01 2:29 ` Inki Dae
2 siblings, 0 replies; 5+ messages in thread
From: Sylwester Nawrocki @ 2010-11-27 17:07 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On 11/23/2010 08:16 AM, Inki Dae wrote:
> clock.c
> - added dsim clock gate.
>
> dev-dsim.c
> - platform and machine specific definitions.
> Now just supports only MACHINE GONI so for other machines,
> mipi_1_1v_name and mipi_1_8v_name values should be changed to
> proper regulator name at each machine file.
>
> setup-dsim.c
> - platform specific definitions.
>
> Signed-off-by: Inki Dae<inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> ---
> arch/arm/mach-s5pv210/Kconfig | 11 ++
> arch/arm/mach-s5pv210/Makefile | 2 +
> arch/arm/mach-s5pv210/clock.c | 6 +
> arch/arm/mach-s5pv210/dev-dsim.c | 149 +++++++++++++++++++++++
> arch/arm/mach-s5pv210/include/mach/map.h | 4 +
> arch/arm/mach-s5pv210/include/mach/regs-clock.h | 3 +-
> arch/arm/mach-s5pv210/mach-goni.c | 12 ++
> arch/arm/mach-s5pv210/setup-dsim.c | 144 ++++++++++++++++++++++
> arch/arm/plat-s5p/Kconfig | 5 +
> 9 files changed, 335 insertions(+), 1 deletions(-)
> create mode 100644 arch/arm/mach-s5pv210/dev-dsim.c
> create mode 100644 arch/arm/mach-s5pv210/setup-dsim.c
>
> diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
> index 862f239..447701f 100644
> --- a/arch/arm/mach-s5pv210/Kconfig
> +++ b/arch/arm/mach-s5pv210/Kconfig
> @@ -53,6 +53,12 @@ config S5PV210_SETUP_SDHCI_GPIO
> help
> Common setup code for SDHCI gpio.
>
> +config S5PV210_SETUP_DSIM
> + bool
> + depends on REGULATOR
> + help
> + Common setup code for MIPI-DSIM.
> +
> menu "S5PC110 Machines"
>
> config MACH_AQUILA
> @@ -68,6 +74,7 @@ config MACH_AQUILA
> select S5P_DEV_ONENAND
> select S5PV210_SETUP_FB_24BPP
> select S5PV210_SETUP_SDHCI
> + select S5PV210_SETUP_DSIM
> help
> Machine support for the Samsung Aquila target based on S5PC110 SoC
>
> @@ -86,12 +93,14 @@ config MACH_GONI
> select S3C_DEV_I2C2
> select S3C_DEV_USB_HSOTG
> select S5P_DEV_ONENAND
> + select S5P_DEV_DSIM
> select SAMSUNG_DEV_KEYPAD
> select S5PV210_SETUP_FB_24BPP
> select S5PV210_SETUP_I2C1
> select S5PV210_SETUP_I2C2
> select S5PV210_SETUP_KEYPAD
> select S5PV210_SETUP_SDHCI
> + select S5PV210_SETUP_DSIM
> help
> Machine support for Samsung GONI board
> S5PC110(MCP) is one of package option of S5PV210
> @@ -107,6 +116,7 @@ config MACH_SMDKC110
> select S5PV210_SETUP_I2C1
> select S5PV210_SETUP_I2C2
> select S5PV210_SETUP_IDE
> + select S5PV210_SETUP_DSIM
> help
> Machine support for Samsung SMDKC110
> S5PC110(MCP) is one of package option of S5PV210
> @@ -135,6 +145,7 @@ config MACH_SMDKV210
> select S5PV210_SETUP_IDE
> select S5PV210_SETUP_KEYPAD
> select S5PV210_SETUP_SDHCI
> + select S5PV210_SETUP_DSIM
> help
> Machine support for Samsung SMDKV210
>
> diff --git a/arch/arm/mach-s5pv210/Makefile b/arch/arm/mach-s5pv210/Makefile
> index ff1a0db..f6a6443 100644
> --- a/arch/arm/mach-s5pv210/Makefile
> +++ b/arch/arm/mach-s5pv210/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_MACH_TORBRECK) += mach-torbreck.o
>
> obj-y += dev-audio.o
> obj-$(CONFIG_S3C64XX_DEV_SPI) += dev-spi.o
> +obj-$(CONFIG_S5P_DEV_DSIM) += dev-dsim.o
>
> obj-$(CONFIG_S5PV210_SETUP_FB_24BPP) += setup-fb-24bpp.o
> obj-$(CONFIG_S5PV210_SETUP_I2C1) += setup-i2c1.o
> @@ -37,3 +38,4 @@ obj-$(CONFIG_S5PV210_SETUP_IDE) += setup-ide.o
> obj-$(CONFIG_S5PV210_SETUP_KEYPAD) += setup-keypad.o
> obj-$(CONFIG_S5PV210_SETUP_SDHCI) += setup-sdhci.o
> obj-$(CONFIG_S5PV210_SETUP_SDHCI_GPIO) += setup-sdhci-gpio.o
> +obj-$(CONFIG_S5PV210_SETUP_DSIM) += setup-dsim.o
> diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c
> index 019c3a6..9cbf244 100644
> --- a/arch/arm/mach-s5pv210/clock.c
> +++ b/arch/arm/mach-s5pv210/clock.c
> @@ -347,6 +347,12 @@ static struct clk init_clocks_disable[] = {
> .enable = s5pv210_clk_ip0_ctrl,
> .ctrlbit = (1<< 26),
> }, {
> + .name = "dsim",
> + .id = -1,
> + .parent =&clk_hclk_dsys.clk,
> + .enable = s5pv210_clk_ip1_ctrl,
> + .ctrlbit = (1<<2),
> + }, {
> .name = "otg",
> .id = -1,
> .parent =&clk_hclk_psys.clk,
> diff --git a/arch/arm/mach-s5pv210/dev-dsim.c b/arch/arm/mach-s5pv210/dev-dsim.c
> new file mode 100644
> index 0000000..3f43d72
> --- /dev/null
> +++ b/arch/arm/mach-s5pv210/dev-dsim.c
> @@ -0,0 +1,149 @@
> +/* linux/arch/arm/plat-s5pc11x/dev-dsim.c
> + *
> + * Copyright 2009 Samsung Electronics
> + * InKi Dae<inki.dae@samsung.com>
> + *
> + * S5PC1XX series device definition for MIPI-DSIM
> + *
> + * 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.
> +*/
> +
> +#include<linux/kernel.h>
> +#include<linux/string.h>
> +#include<linux/platform_device.h>
> +#include<linux/regulator/consumer.h>
> +#include<linux/fb.h>
> +
> +#include<mach/map.h>
> +#include<mach/irqs.h>
> +
> +#include<plat/devs.h>
> +#include<plat/cpu.h>
> +#include<plat/fb.h>
> +
> +#include<plat/dsim.h>
> +#include<plat/mipi_ddi.h>
> +
> +static struct dsim_config dsim_info = {
> + /* main frame fifo auto flush at VSYNC pulse */
> + .auto_flush = false,
> + .eot_disable = false,
> +
> + .auto_vertical_cnt = false,
> + .hse = false,
> + .hfp = false,
> + .hbp = false,
> + .hsa = false,
Since static struct members are implicitly initialized with constant
value 0 do you really need all that initializations to 'false'?
> +
> + .e_no_data_lane = DSIM_DATA_LANE_2,
> + .e_byte_clk = DSIM_PLL_OUT_DIV8,
> +
> + /*
> + * =====================> + * | P | M | S | MHz |
> + * -------------------------------------------
> + * | 3 | 100 | 3 | 100 |
> + * | 3 | 100 | 2 | 200 |
> + * | 3 | 63 | 1 | 252 |
> + * | 4 | 100 | 1 | 300 |
> + * | 4 | 110 | 1 | 330 |
> + * | 12 | 350 | 1 | 350 |
> + * | 3 | 100 | 1 | 400 |
> + * | 4 | 150 | 1 | 450 |
> + * | 3 | 118 | 1 | 472 |
> + * | 12 | 250 | 0 | 500 |
> + * | 4 | 100 | 0 | 600 |
> + * | 3 | 81 | 0 | 648 |
> + * | 3 | 88 | 0 | 704 |
> + * | 3 | 90 | 0 | 720 |
> + * | 3 | 100 | 0 | 800 |
> + * | 12 | 425 | 0 | 850 |
> + * | 4 | 150 | 0 | 900 |
> + * | 12 | 475 | 0 | 950 |
> + * | 6 | 250 | 0 | 1000 |
> + * -------------------------------------------
> + */
> +
> + /* 472MHz: LSI Recommended */
> + .p = 3,
> + .m = 118,
> + .s = 1,
> +
> + /* D-PHY PLL stable time spec :min = 200usec ~ max 400usec */
> + .pll_stable_time = 400,
> +
> + .esc_clk = 10 * 1000000, /* escape clk : 10MHz */
> +
> + /* stop state holding counter after bta change count 0 ~ 0xfff */
> + .stop_holding_cnt = 0x0f,
> + .bta_timeout = 0xff, /* bta timeout 0 ~ 0xff */
> + .rx_timeout = 0xffff, /* lp rx timeout 0 ~ 0xffff */
> +
> + .e_lane_swap = DSIM_NO_CHANGE,
> +};
> +
> +/* define ddi platform data based on MIPI-DSI. */
> +static struct mipi_ddi_platform_data mipi_ddi_pd = {
> + .cmd_write = s5p_dsim_wr_data,
> + .cmd_read = NULL,
> + .get_dsim_frame_done = s5p_dsim_get_frame_done_status,
> + .clear_dsim_frame_done = s5p_dsim_clear_frame_done,
> + .change_dsim_transfer_mode = s5p_dsim_change_transfer_mode,
> + .get_fb_frame_done = NULL,
> + .trigger = NULL,
No need to initialize to NULL.
> +};
> +
> +static struct dsim_lcd_config dsim_lcd_info = {
> + .e_interface = DSIM_COMMAND,
> + .parameter[DSI_VIRTUAL_CH_ID] = (unsigned int)DSIM_VIRTUAL_CH_0,
> + .parameter[DSI_FORMAT] = (unsigned int)DSIM_24BPP_888,
> + .parameter[DSI_VIDEO_MODE_SEL] = (unsigned int)DSIM_BURST,
Wouldn't it be cleaner to define struct members of relevant type and
name in place of the array and avoid casting?
> + .mipi_ddi_pd = (void *)&mipi_ddi_pd,
> +};
> +
> +static struct resource s5p_dsim_resource[] = {
> + [0] = {
> + .start = S5PV210_PA_DSI,
> + .end = S5PV210_PA_DSI + S5PV210_SZ_DSI - 1,
> + .flags = IORESOURCE_MEM,
> + },
> + [1] = {
> + .start = IRQ_MIPIDSI,
> + .end = IRQ_MIPIDSI,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct s5p_platform_dsim dsim_platform_data = {
> + .clk_name = "dsim",
> + .mipi_1_1v_name = "vmipi_1.1v",
> + .mipi_1_8v_name = "vmipi_1.8v",
You could avoid passing regulator's names in here. All you need to do is
defining regulator supplies properly (see further comments below).
Creating well known (e.g. as defined in the datasheet) regulator supply
names in the actual driver file (s5p-dsim.c) should be enough.
The regulator API can be used to match a regulator and its consumer.
> + .dsim_info =&dsim_info,
> + .dsim_lcd_info =&dsim_lcd_info,
> +
> + .mipi_power = s5p_dsim_mipi_power,
> + .part_reset = s5p_dsim_part_reset,
> + .init_d_phy = s5p_dsim_init_d_phy,
> + .get_fb_frame_done = NULL,
> + .trigger = NULL,
> +
> + .platform_rev = 1,
> +
> + /*
> + * the stable time of needing to write data on SFR
> + * when the mipi mode becomes LP mode.
> + */
> + .delay_for_stabilization = 600,
> +};
> +
> +struct platform_device s5p_device_dsim = {
> + .name = "s5p-dsim",
> + .id = 0,
> + .num_resources = ARRAY_SIZE(s5p_dsim_resource),
> + .resource = s5p_dsim_resource,
> + .dev = {
> + .platform_data = (void *)&dsim_platform_data,
> + },
> +};
> diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-s5pv210/include/mach/map.h
> index 861d7fe..1ad2dad 100644
> --- a/arch/arm/mach-s5pv210/include/mach/map.h
> +++ b/arch/arm/mach-s5pv210/include/mach/map.h
> @@ -69,6 +69,10 @@
>
> #define S5PV210_PA_FB (0xF8000000)
>
> +/* MIPI-DSI */
> +#define S5PV210_PA_DSI (0xFA500000)
> +#define S5PV210_SZ_DSI SZ_1M
How about removing S5PV210_SZ_DSI and directly using SZ_1M in dev-dsim.c
? Also, 1MB is probably excessive.
> +
> #define S5PV210_PA_FIMC0 (0xFB200000)
> #define S5PV210_PA_FIMC1 (0xFB300000)
> #define S5PV210_PA_FIMC2 (0xFB400000)
> diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> index ebaabe0..c8b9366 100644
> --- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> +++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> @@ -196,7 +196,8 @@
> #define S5P_OTHERS_USB_SIG_MASK (1<< 16)
>
> /* MIPI */
> -#define S5P_MIPI_DPHY_EN (3)
> +#define S5P_MIPI_DPHY_EN (3<< 0)
> +#define S5P_MIPI_M_RESETN (1<< 1)
>
> /* S5P_DAC_CONTROL */
> #define S5P_DAC_ENABLE (1)
> diff --git a/arch/arm/mach-s5pv210/mach-goni.c b/arch/arm/mach-s5pv210/mach-goni.c
> index b1dcf96..500cc1b 100644
> --- a/arch/arm/mach-s5pv210/mach-goni.c
> +++ b/arch/arm/mach-s5pv210/mach-goni.c
> @@ -269,10 +269,18 @@ static void __init goni_tsp_init(void)
> /* MAX8998 regulators */
> #if defined(CONFIG_REGULATOR_MAX8998) || defined(CONFIG_REGULATOR_MAX8998_MODULE)
>
> +static struct regulator_consumer_supply goni_ldo3_consumers[] = {
> + REGULATOR_SUPPLY("vmipi_1.1v", ""),
> +};
You could just do:
REGULATOR_SUPPLY("vmipi_1.1v", "s5p-dsim.0"),
The second argument is used to match your consumer device with the
relevant regulator supply.
Similar could be done in other machine's board setup files and there is
no need to pass anything in the platform data.
Then in your driver probe you might just do:
... = regulator_get(&pdev->dev, "vmipi_1.1v");
> +
> static struct regulator_consumer_supply goni_ldo5_consumers[] = {
> REGULATOR_SUPPLY("vmmc", "s3c-sdhci.0"),
> };
>
> +static struct regulator_consumer_supply goni_ldo7_consumers[] = {
> + REGULATOR_SUPPLY("vmipi_1.8v", ""),
> +};
Ditto.
> +
> static struct regulator_init_data goni_ldo2_data = {
> .constraints = {
> .name = "VALIVE_1.1V",
> @@ -294,6 +302,8 @@ static struct regulator_init_data goni_ldo3_data = {
> .apply_uV = 1,
> .always_on = 1,
> },
> + .num_consumer_supplies = ARRAY_SIZE(goni_ldo3_consumers),
> + .consumer_supplies = goni_ldo3_consumers,
> };
>
> static struct regulator_init_data goni_ldo4_data = {
> @@ -333,6 +343,8 @@ static struct regulator_init_data goni_ldo7_data = {
> .apply_uV = 1,
> .always_on = 1,
> },
> + .num_consumer_supplies = ARRAY_SIZE(goni_ldo7_consumers),
> + .consumer_supplies = goni_ldo7_consumers,
> };
>
> static struct regulator_init_data goni_ldo8_data = {
> diff --git a/arch/arm/mach-s5pv210/setup-dsim.c b/arch/arm/mach-s5pv210/setup-dsim.c
> new file mode 100644
> index 0000000..874efa0
> --- /dev/null
> +++ b/arch/arm/mach-s5pv210/setup-dsim.c
> @@ -0,0 +1,144 @@
> +/*
> + * S5PC110 MIPI-DSIM driver.
> + *
> + * Author: InKi Dae<inki.dae@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +#include<linux/kernel.h>
> +#include<linux/string.h>
> +#include<linux/io.h>
> +#include<linux/err.h>
> +#include<linux/platform_device.h>
> +#include<linux/clk.h>
> +#include<linux/regulator/consumer.h>
> +
> +#include<mach/map.h>
> +#include<mach/regs-clock.h>
> +
> +#include<plat/dsim.h>
> +#include<plat/clock.h>
> +#include<plat/regs-dsim.h>
> +
> +static int s5p_dsim_enable_d_phy(struct dsim_global *dsim, unsigned int enable)
> +{
> + unsigned int reg;
> +
> + WARN_ON(dsim = NULL);
> +
> + reg = readl(S5P_MIPI_CONTROL)& ~(1<< 0);
> + reg |= (enable<< 0);
> + writel(reg, S5P_MIPI_CONTROL);
You may want to use __raw_readl(), __raw_writel here instead
since the addresses used are statically remapped.
> +
> + dev_dbg(dsim->dev, "%s : %x\n", __func__, reg);
> +
> + return 0;
> +}
> +
> +static int s5p_dsim_enable_dsi_master(struct dsim_global *dsim,
> + unsigned int enable)
> +{
> + unsigned int reg;
> +
> + WARN_ON(dsim = NULL);
Issuing a warning when "dsim" is NULL and then crashing on a dereference
of it seems rather pointless to me. But it might be just me.
> +
> + reg = readl(S5P_MIPI_CONTROL)& ~(1<< 2);
> + reg |= (enable<< 2);
> + writel(reg, S5P_MIPI_CONTROL);
> +
> + dev_dbg(dsim->dev, "%s : %x\n", __func__, reg);
> +
> + return 0;
> +}
> +
> +int s5p_dsim_part_reset(struct dsim_global *dsim)
> +{
> + WARN_ON(dsim = NULL);
> +
> + writel(S5P_MIPI_M_RESETN, S5P_MIPI_PHY_CON0);
> +
> + dev_dbg(dsim->dev, "%s\n", __func__);
> +
> + return 0;
> +}
> +
> +int s5p_dsim_init_d_phy(struct dsim_global *dsim)
> +{
> + WARN_ON(dsim = NULL);
> +
> + /**
> + * DPHY and Master block must be enabled at the system initialization
> + * step before data access from/to DPHY begins.
> + */
> + s5p_dsim_enable_d_phy(dsim, 1);
> +
> + s5p_dsim_enable_dsi_master(dsim, 1);
It looks like two separate functions which are just changing different
bits in the same IO register are created and then these functions are
used only here. I think it would be worth to make it more compact. We
should be trying try to keep the platform device helpers as lean as
possible.
> +
> + dev_dbg(dsim->dev, "%s\n", __func__);
> +
> + return 0;
> +}
> +
> +int s5p_dsim_mipi_power(struct dsim_global *dsim, struct regulator *p_mipi_1_1v,
> + struct regulator *p_mipi_1_8v, unsigned int enable)
> +{
> + int ret = -1;
> +
> + WARN_ON(dsim = NULL);
> +
> + if (IS_ERR(p_mipi_1_1v) || IS_ERR(p_mipi_1_8v)) {
> + dev_err(dsim->dev, "p_mipi_1_1v or p_mipi_1_8v is NULL.\n");
> + return -EINVAL;
> + }
> +
> + if (enable) {
> + if (p_mipi_1_1v)
> + ret = regulator_enable(p_mipi_1_1v);
> +
> + if (ret< 0) {
> + dev_err(dsim->dev,
> + "failed to enable regulator mipi_1_1v.\n");
> + return ret;
> + }
> +
> + if (p_mipi_1_8v)
> + ret = regulator_enable(p_mipi_1_8v);
> +
> + if (ret< 0) {
> + dev_err(dsim->dev,
> + "failed to enable regulator mipi_1_8v.\n");
> + return ret;
> + }
> + } else {
> + if (p_mipi_1_1v)
> + ret = regulator_force_disable(p_mipi_1_1v);
> + if (ret< 0) {
> + dev_err(dsim->dev,
> + "failed to disable regulator mipi_1_1v.\n");
> + return ret;
> + }
> +
> + if (p_mipi_1_8v)
> + ret = regulator_force_disable(p_mipi_1_8v);
> + if (ret< 0) {
> + dev_err(dsim->dev,
> + "failed to disable regulator mipi_1_8v.\n");
> + return ret;
> + }
> + }
> +
> + return ret;
> +}
This function seem to be called only in the driver and it uses driver's
(private?) data, so can you explain what is the purpose of having it in
arch?
I suspect all the above regulator handling code could well be moved to
the driver. Am I missing something?
> diff --git a/arch/arm/plat-s5p/Kconfig b/arch/arm/plat-s5p/Kconfig
> index 65dbfa8..73a34e0 100644
> --- a/arch/arm/plat-s5p/Kconfig
> +++ b/arch/arm/plat-s5p/Kconfig
> @@ -56,3 +56,8 @@ config S5P_DEV_ONENAND
> bool
> help
> Compile in platform device definition for OneNAND controller
> +
> +config S5P_DEV_DSIM
> + bool
> + help
> + Compile in platform device definitions for MIPI-DSI
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and machine.
2010-11-23 7:16 [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and machine Inki Dae
2010-11-27 17:07 ` Sylwester Nawrocki
@ 2010-11-30 1:30 ` Inki Dae
2010-11-30 22:57 ` Sylwester Nawrocki
2010-12-01 2:29 ` Inki Dae
2 siblings, 1 reply; 5+ messages in thread
From: Inki Dae @ 2010-11-30 1:30 UTC (permalink / raw)
To: linux-fbdev
Hello, Sylwester.
Long time no see. :)
Thank you for your comments.
> -----Original Message-----
> From: Sylwester Nawrocki [mailto:spnlinux@gmail.com]
> Sent: Sunday, November 28, 2010 2:07 AM
> To: Inki Dae
> Cc: linux-fbdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> kyungmin.park@samsung.com; kgene.kim@samsung.com; akpm@linux-
> foundation.org; lethal@linux-sh.org
> Subject: Re: [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and
> machine.
>
> Hello,
>
> On 11/23/2010 08:16 AM, Inki Dae wrote:
> > clock.c
> > - added dsim clock gate.
> >
> > dev-dsim.c
> > - platform and machine specific definitions.
> > Now just supports only MACHINE GONI so for other machines,
> > mipi_1_1v_name and mipi_1_8v_name values should be changed to
> > proper regulator name at each machine file.
> >
> > setup-dsim.c
> > - platform specific definitions.
> >
> > Signed-off-by: Inki Dae<inki.dae@samsung.com>
> > Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > ---
> > arch/arm/mach-s5pv210/Kconfig | 11 ++
> > arch/arm/mach-s5pv210/Makefile | 2 +
> > arch/arm/mach-s5pv210/clock.c | 6 +
> > arch/arm/mach-s5pv210/dev-dsim.c | 149
> +++++++++++++++++++++++
> > arch/arm/mach-s5pv210/include/mach/map.h | 4 +
> > arch/arm/mach-s5pv210/include/mach/regs-clock.h | 3 +-
> > arch/arm/mach-s5pv210/mach-goni.c | 12 ++
> > arch/arm/mach-s5pv210/setup-dsim.c | 144
> ++++++++++++++++++++++
> > arch/arm/plat-s5p/Kconfig | 5 +
> > 9 files changed, 335 insertions(+), 1 deletions(-)
> > create mode 100644 arch/arm/mach-s5pv210/dev-dsim.c
> > create mode 100644 arch/arm/mach-s5pv210/setup-dsim.c
> >
> > diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-
> s5pv210/Kconfig
> > index 862f239..447701f 100644
> > --- a/arch/arm/mach-s5pv210/Kconfig
> > +++ b/arch/arm/mach-s5pv210/Kconfig
> > @@ -53,6 +53,12 @@ config S5PV210_SETUP_SDHCI_GPIO
> > help
> > Common setup code for SDHCI gpio.
> >
> > +config S5PV210_SETUP_DSIM
> > + bool
> > + depends on REGULATOR
> > + help
> > + Common setup code for MIPI-DSIM.
> > +
> > menu "S5PC110 Machines"
> >
> > config MACH_AQUILA
> > @@ -68,6 +74,7 @@ config MACH_AQUILA
> > select S5P_DEV_ONENAND
> > select S5PV210_SETUP_FB_24BPP
> > select S5PV210_SETUP_SDHCI
> > + select S5PV210_SETUP_DSIM
> > help
> > Machine support for the Samsung Aquila target based on S5PC110
> SoC
> >
> > @@ -86,12 +93,14 @@ config MACH_GONI
> > select S3C_DEV_I2C2
> > select S3C_DEV_USB_HSOTG
> > select S5P_DEV_ONENAND
> > + select S5P_DEV_DSIM
> > select SAMSUNG_DEV_KEYPAD
> > select S5PV210_SETUP_FB_24BPP
> > select S5PV210_SETUP_I2C1
> > select S5PV210_SETUP_I2C2
> > select S5PV210_SETUP_KEYPAD
> > select S5PV210_SETUP_SDHCI
> > + select S5PV210_SETUP_DSIM
> > help
> > Machine support for Samsung GONI board
> > S5PC110(MCP) is one of package option of S5PV210
> > @@ -107,6 +116,7 @@ config MACH_SMDKC110
> > select S5PV210_SETUP_I2C1
> > select S5PV210_SETUP_I2C2
> > select S5PV210_SETUP_IDE
> > + select S5PV210_SETUP_DSIM
> > help
> > Machine support for Samsung SMDKC110
> > S5PC110(MCP) is one of package option of S5PV210
> > @@ -135,6 +145,7 @@ config MACH_SMDKV210
> > select S5PV210_SETUP_IDE
> > select S5PV210_SETUP_KEYPAD
> > select S5PV210_SETUP_SDHCI
> > + select S5PV210_SETUP_DSIM
> > help
> > Machine support for Samsung SMDKV210
> >
> > diff --git a/arch/arm/mach-s5pv210/Makefile b/arch/arm/mach-
> s5pv210/Makefile
> > index ff1a0db..f6a6443 100644
> > --- a/arch/arm/mach-s5pv210/Makefile
> > +++ b/arch/arm/mach-s5pv210/Makefile
> > @@ -29,6 +29,7 @@ obj-$(CONFIG_MACH_TORBRECK) += mach-torbreck.o
> >
> > obj-y += dev-audio.o
> > obj-$(CONFIG_S3C64XX_DEV_SPI) += dev-spi.o
> > +obj-$(CONFIG_S5P_DEV_DSIM) += dev-dsim.o
> >
> > obj-$(CONFIG_S5PV210_SETUP_FB_24BPP) += setup-fb-24bpp.o
> > obj-$(CONFIG_S5PV210_SETUP_I2C1) += setup-i2c1.o
> > @@ -37,3 +38,4 @@ obj-$(CONFIG_S5PV210_SETUP_IDE) += setup-ide.o
> > obj-$(CONFIG_S5PV210_SETUP_KEYPAD) += setup-keypad.o
> > obj-$(CONFIG_S5PV210_SETUP_SDHCI) += setup-sdhci.o
> > obj-$(CONFIG_S5PV210_SETUP_SDHCI_GPIO) += setup-sdhci-gpio.o
> > +obj-$(CONFIG_S5PV210_SETUP_DSIM) += setup-dsim.o
> > diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-
> s5pv210/clock.c
> > index 019c3a6..9cbf244 100644
> > --- a/arch/arm/mach-s5pv210/clock.c
> > +++ b/arch/arm/mach-s5pv210/clock.c
> > @@ -347,6 +347,12 @@ static struct clk init_clocks_disable[] = {
> > .enable = s5pv210_clk_ip0_ctrl,
> > .ctrlbit = (1<< 26),
> > }, {
> > + .name = "dsim",
> > + .id = -1,
> > + .parent =&clk_hclk_dsys.clk,
> > + .enable = s5pv210_clk_ip1_ctrl,
> > + .ctrlbit = (1<<2),
> > + }, {
> > .name = "otg",
> > .id = -1,
> > .parent =&clk_hclk_psys.clk,
> > diff --git a/arch/arm/mach-s5pv210/dev-dsim.c b/arch/arm/mach-
> s5pv210/dev-dsim.c
> > new file mode 100644
> > index 0000000..3f43d72
> > --- /dev/null
> > +++ b/arch/arm/mach-s5pv210/dev-dsim.c
> > @@ -0,0 +1,149 @@
> > +/* linux/arch/arm/plat-s5pc11x/dev-dsim.c
> > + *
> > + * Copyright 2009 Samsung Electronics
> > + * InKi Dae<inki.dae@samsung.com>
> > + *
> > + * S5PC1XX series device definition for MIPI-DSIM
> > + *
> > + * 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.
> > +*/
> > +
> > +#include<linux/kernel.h>
> > +#include<linux/string.h>
> > +#include<linux/platform_device.h>
> > +#include<linux/regulator/consumer.h>
> > +#include<linux/fb.h>
> > +
> > +#include<mach/map.h>
> > +#include<mach/irqs.h>
> > +
> > +#include<plat/devs.h>
> > +#include<plat/cpu.h>
> > +#include<plat/fb.h>
> > +
> > +#include<plat/dsim.h>
> > +#include<plat/mipi_ddi.h>
> > +
> > +static struct dsim_config dsim_info = {
> > + /* main frame fifo auto flush at VSYNC pulse */
> > + .auto_flush = false,
> > + .eot_disable = false,
> > +
> > + .auto_vertical_cnt = false,
> > + .hse = false,
> > + .hfp = false,
> > + .hbp = false,
> > + .hsa = false,
>
> Since static struct members are implicitly initialized with constant
> value 0 do you really need all that initializations to 'false'?
>
You are right, don't need.
I will remove this.
> > +
> > + .e_no_data_lane = DSIM_DATA_LANE_2,
> > + .e_byte_clk = DSIM_PLL_OUT_DIV8,
> > +
> > + /*
> > + * =====================> > + * | P | M | S | MHz |
> > + * -------------------------------------------
> > + * | 3 | 100 | 3 | 100 |
> > + * | 3 | 100 | 2 | 200 |
> > + * | 3 | 63 | 1 | 252 |
> > + * | 4 | 100 | 1 | 300 |
> > + * | 4 | 110 | 1 | 330 |
> > + * | 12 | 350 | 1 | 350 |
> > + * | 3 | 100 | 1 | 400 |
> > + * | 4 | 150 | 1 | 450 |
> > + * | 3 | 118 | 1 | 472 |
> > + * | 12 | 250 | 0 | 500 |
> > + * | 4 | 100 | 0 | 600 |
> > + * | 3 | 81 | 0 | 648 |
> > + * | 3 | 88 | 0 | 704 |
> > + * | 3 | 90 | 0 | 720 |
> > + * | 3 | 100 | 0 | 800 |
> > + * | 12 | 425 | 0 | 850 |
> > + * | 4 | 150 | 0 | 900 |
> > + * | 12 | 475 | 0 | 950 |
> > + * | 6 | 250 | 0 | 1000 |
> > + * -------------------------------------------
> > + */
> > +
> > + /* 472MHz: LSI Recommended */
> > + .p = 3,
> > + .m = 118,
> > + .s = 1,
> > +
> > + /* D-PHY PLL stable time spec :min = 200usec ~ max 400usec */
> > + .pll_stable_time = 400,
> > +
> > + .esc_clk = 10 * 1000000, /* escape clk : 10MHz */
> > +
> > + /* stop state holding counter after bta change count 0 ~ 0xfff */
> > + .stop_holding_cnt = 0x0f,
> > + .bta_timeout = 0xff, /* bta timeout 0 ~ 0xff */
> > + .rx_timeout = 0xffff, /* lp rx timeout 0 ~ 0xffff */
> > +
> > + .e_lane_swap = DSIM_NO_CHANGE,
> > +};
> > +
> > +/* define ddi platform data based on MIPI-DSI. */
> > +static struct mipi_ddi_platform_data mipi_ddi_pd = {
> > + .cmd_write = s5p_dsim_wr_data,
> > + .cmd_read = NULL,
> > + .get_dsim_frame_done = s5p_dsim_get_frame_done_status,
> > + .clear_dsim_frame_done = s5p_dsim_clear_frame_done,
> > + .change_dsim_transfer_mode = s5p_dsim_change_transfer_mode,
> > + .get_fb_frame_done = NULL,
> > + .trigger = NULL,
> No need to initialize to NULL.
> > +};
> > +
> > +static struct dsim_lcd_config dsim_lcd_info = {
> > + .e_interface = DSIM_COMMAND,
> > + .parameter[DSI_VIRTUAL_CH_ID] = (unsigned int)DSIM_VIRTUAL_CH_0,
> > + .parameter[DSI_FORMAT] = (unsigned int)DSIM_24BPP_888,
> > + .parameter[DSI_VIDEO_MODE_SEL] = (unsigned int)DSIM_BURST,
>
> Wouldn't it be cleaner to define struct members of relevant type and
> name in place of the array and avoid casting?
>
You saying make sense, but parameter array could have different enumeration types.
> > + .mipi_ddi_pd = (void *)&mipi_ddi_pd,
> > +};
> > +
> > +static struct resource s5p_dsim_resource[] = {
> > + [0] = {
> > + .start = S5PV210_PA_DSI,
> > + .end = S5PV210_PA_DSI + S5PV210_SZ_DSI - 1,
> > + .flags = IORESOURCE_MEM,
> > + },
> > + [1] = {
> > + .start = IRQ_MIPIDSI,
> > + .end = IRQ_MIPIDSI,
> > + .flags = IORESOURCE_IRQ,
> > + },
> > +};
> > +
> > +static struct s5p_platform_dsim dsim_platform_data = {
> > + .clk_name = "dsim",
> > + .mipi_1_1v_name = "vmipi_1.1v",
> > + .mipi_1_8v_name = "vmipi_1.8v",
>
> You could avoid passing regulator's names in here. All you need to do is
> defining regulator supplies properly (see further comments below).
> Creating well known (e.g. as defined in the datasheet) regulator supply
> names in the actual driver file (s5p-dsim.c) should be enough.
> The regulator API can be used to match a regulator and its consumer.
>
Regulator's name could be changed according to machine so I think should be set to Machine specific file.
> > + .dsim_info =&dsim_info,
> > + .dsim_lcd_info =&dsim_lcd_info,
> > +
> > + .mipi_power = s5p_dsim_mipi_power,
> > + .part_reset = s5p_dsim_part_reset,
> > + .init_d_phy = s5p_dsim_init_d_phy,
> > + .get_fb_frame_done = NULL,
> > + .trigger = NULL,
> > +
> > + .platform_rev = 1,
> > +
> > + /*
> > + * the stable time of needing to write data on SFR
> > + * when the mipi mode becomes LP mode.
> > + */
> > + .delay_for_stabilization = 600,
> > +};
> > +
> > +struct platform_device s5p_device_dsim = {
> > + .name = "s5p-dsim",
> > + .id = 0,
> > + .num_resources = ARRAY_SIZE(s5p_dsim_resource),
> > + .resource = s5p_dsim_resource,
> > + .dev = {
> > + .platform_data = (void *)&dsim_platform_data,
> > + },
> > +};
> > diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-
> s5pv210/include/mach/map.h
> > index 861d7fe..1ad2dad 100644
> > --- a/arch/arm/mach-s5pv210/include/mach/map.h
> > +++ b/arch/arm/mach-s5pv210/include/mach/map.h
> > @@ -69,6 +69,10 @@
> >
> > #define S5PV210_PA_FB (0xF8000000)
> >
> > +/* MIPI-DSI */
> > +#define S5PV210_PA_DSI (0xFA500000)
> > +#define S5PV210_SZ_DSI SZ_1M
>
> How about removing S5PV210_SZ_DSI and directly using SZ_1M in dev-dsim.c
> ? Also, 1MB is probably excessive.
>
> > +
> > #define S5PV210_PA_FIMC0 (0xFB200000)
> > #define S5PV210_PA_FIMC1 (0xFB300000)
> > #define S5PV210_PA_FIMC2 (0xFB400000)
> > diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> > index ebaabe0..c8b9366 100644
> > --- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> > +++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> > @@ -196,7 +196,8 @@
> > #define S5P_OTHERS_USB_SIG_MASK (1<< 16)
> >
> > /* MIPI */
> > -#define S5P_MIPI_DPHY_EN (3)
> > +#define S5P_MIPI_DPHY_EN (3<< 0)
> > +#define S5P_MIPI_M_RESETN (1<< 1)
> >
> > /* S5P_DAC_CONTROL */
> > #define S5P_DAC_ENABLE (1)
> > diff --git a/arch/arm/mach-s5pv210/mach-goni.c b/arch/arm/mach-
> s5pv210/mach-goni.c
> > index b1dcf96..500cc1b 100644
> > --- a/arch/arm/mach-s5pv210/mach-goni.c
> > +++ b/arch/arm/mach-s5pv210/mach-goni.c
> > @@ -269,10 +269,18 @@ static void __init goni_tsp_init(void)
> > /* MAX8998 regulators */
> > #if defined(CONFIG_REGULATOR_MAX8998) ||
> defined(CONFIG_REGULATOR_MAX8998_MODULE)
> >
> > +static struct regulator_consumer_supply goni_ldo3_consumers[] = {
> > + REGULATOR_SUPPLY("vmipi_1.1v", ""),
> > +};
>
> You could just do:
> REGULATOR_SUPPLY("vmipi_1.1v", "s5p-dsim.0"),
>
> The second argument is used to match your consumer device with the
> relevant regulator supply.
>
Ok, it would be corrected.
> Similar could be done in other machine's board setup files and there is
> no need to pass anything in the platform data.
>
> Then in your driver probe you might just do:
>
> ... = regulator_get(&pdev->dev, "vmipi_1.1v");
>
I mentioned before.
> > +
> > static struct regulator_consumer_supply goni_ldo5_consumers[] = {
> > REGULATOR_SUPPLY("vmmc", "s3c-sdhci.0"),
> > };
> >
> > +static struct regulator_consumer_supply goni_ldo7_consumers[] = {
> > + REGULATOR_SUPPLY("vmipi_1.8v", ""),
> > +};
>
> Ditto.
>
> > +
> > static struct regulator_init_data goni_ldo2_data = {
> > .constraints = {
> > .name = "VALIVE_1.1V",
> > @@ -294,6 +302,8 @@ static struct regulator_init_data goni_ldo3_data = {
> > .apply_uV = 1,
> > .always_on = 1,
> > },
> > + .num_consumer_supplies = ARRAY_SIZE(goni_ldo3_consumers),
> > + .consumer_supplies = goni_ldo3_consumers,
> > };
> >
> > static struct regulator_init_data goni_ldo4_data = {
> > @@ -333,6 +343,8 @@ static struct regulator_init_data goni_ldo7_data = {
> > .apply_uV = 1,
> > .always_on = 1,
> > },
> > + .num_consumer_supplies = ARRAY_SIZE(goni_ldo7_consumers),
> > + .consumer_supplies = goni_ldo7_consumers,
> > };
> >
> > static struct regulator_init_data goni_ldo8_data = {
> > diff --git a/arch/arm/mach-s5pv210/setup-dsim.c b/arch/arm/mach-
> s5pv210/setup-dsim.c
> > new file mode 100644
> > index 0000000..874efa0
> > --- /dev/null
> > +++ b/arch/arm/mach-s5pv210/setup-dsim.c
> > @@ -0,0 +1,144 @@
> > +/*
> > + * S5PC110 MIPI-DSIM driver.
> > + *
> > + * Author: InKi Dae<inki.dae@samsung.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + */
> > +#include<linux/kernel.h>
> > +#include<linux/string.h>
> > +#include<linux/io.h>
> > +#include<linux/err.h>
> > +#include<linux/platform_device.h>
> > +#include<linux/clk.h>
> > +#include<linux/regulator/consumer.h>
> > +
> > +#include<mach/map.h>
> > +#include<mach/regs-clock.h>
> > +
> > +#include<plat/dsim.h>
> > +#include<plat/clock.h>
> > +#include<plat/regs-dsim.h>
>
> > +
> > +static int s5p_dsim_enable_d_phy(struct dsim_global *dsim, unsigned int
> enable)
> > +{
> > + unsigned int reg;
> > +
> > + WARN_ON(dsim = NULL);
> > +
> > + reg = readl(S5P_MIPI_CONTROL)& ~(1<< 0);
> > + reg |= (enable<< 0);
> > + writel(reg, S5P_MIPI_CONTROL);
>
> You may want to use __raw_readl(), __raw_writel here instead
> since the addresses used are statically remapped.
>
Ok, it would be corrected.
> > +
> > + dev_dbg(dsim->dev, "%s : %x\n", __func__, reg);
> > +
> > + return 0;
> > +}
> > +
> > +static int s5p_dsim_enable_dsi_master(struct dsim_global *dsim,
> > + unsigned int enable)
> > +{
> > + unsigned int reg;
> > +
> > + WARN_ON(dsim = NULL);
>
> Issuing a warning when "dsim" is NULL and then crashing on a dereference
> of it seems rather pointless to me. But it might be just me.
>
Yes, it's just for debugging. It would be removed.
> > +
> > + reg = readl(S5P_MIPI_CONTROL)& ~(1<< 2);
> > + reg |= (enable<< 2);
> > + writel(reg, S5P_MIPI_CONTROL);
> > +
> > + dev_dbg(dsim->dev, "%s : %x\n", __func__, reg);
> > +
> > + return 0;
> > +}
> > +
> > +int s5p_dsim_part_reset(struct dsim_global *dsim)
> > +{
> > + WARN_ON(dsim = NULL);
> > +
> > + writel(S5P_MIPI_M_RESETN, S5P_MIPI_PHY_CON0);
> > +
> > + dev_dbg(dsim->dev, "%s\n", __func__);
> > +
> > + return 0;
> > +}
> > +
> > +int s5p_dsim_init_d_phy(struct dsim_global *dsim)
> > +{
> > + WARN_ON(dsim = NULL);
> > +
> > + /**
> > + * DPHY and Master block must be enabled at the system
> initialization
> > + * step before data access from/to DPHY begins.
> > + */
> > + s5p_dsim_enable_d_phy(dsim, 1);
> > +
> > + s5p_dsim_enable_dsi_master(dsim, 1);
>
> It looks like two separate functions which are just changing different
> bits in the same IO register are created and then these functions are
> used only here. I think it would be worth to make it more compact. We
> should be trying try to keep the platform device helpers as lean as
> possible.
>
Ok, it would be modified more compact.
> > +
> > + dev_dbg(dsim->dev, "%s\n", __func__);
> > +
> > + return 0;
> > +}
> > +
> > +int s5p_dsim_mipi_power(struct dsim_global *dsim, struct regulator
> *p_mipi_1_1v,
> > + struct regulator *p_mipi_1_8v, unsigned int enable)
> > +{
> > + int ret = -1;
> > +
> > + WARN_ON(dsim = NULL);
> > +
> > + if (IS_ERR(p_mipi_1_1v) || IS_ERR(p_mipi_1_8v)) {
> > + dev_err(dsim->dev, "p_mipi_1_1v or p_mipi_1_8v is NULL.\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (enable) {
> > + if (p_mipi_1_1v)
> > + ret = regulator_enable(p_mipi_1_1v);
> > +
> > + if (ret< 0) {
> > + dev_err(dsim->dev,
> > + "failed to enable regulator mipi_1_1v.\n");
> > + return ret;
> > + }
> > +
> > + if (p_mipi_1_8v)
> > + ret = regulator_enable(p_mipi_1_8v);
> > +
> > + if (ret< 0) {
> > + dev_err(dsim->dev,
> > + "failed to enable regulator mipi_1_8v.\n");
> > + return ret;
> > + }
> > + } else {
> > + if (p_mipi_1_1v)
> > + ret = regulator_force_disable(p_mipi_1_1v);
> > + if (ret< 0) {
> > + dev_err(dsim->dev,
> > + "failed to disable regulator mipi_1_1v.\n");
> > + return ret;
> > + }
> > +
> > + if (p_mipi_1_8v)
> > + ret = regulator_force_disable(p_mipi_1_8v);
> > + if (ret< 0) {
> > + dev_err(dsim->dev,
> > + "failed to disable regulator mipi_1_8v.\n");
> > + return ret;
> > + }
> > + }
> > +
> > + return ret;
> > +}
>
> This function seem to be called only in the driver and it uses driver's
> (private?) data, so can you explain what is the purpose of having it in
> arch?
> I suspect all the above regulator handling code could well be moved to
> the driver. Am I missing something?
>
As I said above, regulator's name is specific to machine so it would be got by driver's probe and then controlled by driver.
Note that some machine doesn’t use regulator(gpio instead of regulator)
> > diff --git a/arch/arm/plat-s5p/Kconfig b/arch/arm/plat-s5p/Kconfig
> > index 65dbfa8..73a34e0 100644
> > --- a/arch/arm/plat-s5p/Kconfig
> > +++ b/arch/arm/plat-s5p/Kconfig
> > @@ -56,3 +56,8 @@ config S5P_DEV_ONENAND
> > bool
> > help
> > Compile in platform device definition for OneNAND controller
> > +
> > +config S5P_DEV_DSIM
> > + bool
> > + help
> > + Compile in platform device definitions for MIPI-DSI
>
>
> --
> Regards,
> Sylwester
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and machine.
2010-11-30 1:30 ` Inki Dae
@ 2010-11-30 22:57 ` Sylwester Nawrocki
0 siblings, 0 replies; 5+ messages in thread
From: Sylwester Nawrocki @ 2010-11-30 22:57 UTC (permalink / raw)
To: linux-arm-kernel
Hi Inki,
On 11/30/2010 02:30 AM, Inki Dae wrote:
> Hello, Sylwester.
> Long time no see. :)
>
> Thank you for your comments.
>
>> -----Original Message-----
>> From: Sylwester Nawrocki [mailto:spnlinux@gmail.com]
>> Sent: Sunday, November 28, 2010 2:07 AM
>> To: Inki Dae
>> Cc: linux-fbdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> kyungmin.park@samsung.com; kgene.kim@samsung.com; akpm@linux-
>> foundation.org; lethal@linux-sh.org
>> Subject: Re: [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and
>> machine.
>>
>> Hello,
>>
>> On 11/23/2010 08:16 AM, Inki Dae wrote:
>>> clock.c
>>> - added dsim clock gate.
>>>
>>> dev-dsim.c
>>> - platform and machine specific definitions.
>>> Now just supports only MACHINE GONI so for other machines,
>>> mipi_1_1v_name and mipi_1_8v_name values should be changed to
>>> proper regulator name at each machine file.
>>>
>>> setup-dsim.c
>>> - platform specific definitions.
>>>
>>> Signed-off-by: Inki Dae<inki.dae@samsung.com>
>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>> ---
>>> arch/arm/mach-s5pv210/Kconfig | 11 ++
>>> arch/arm/mach-s5pv210/Makefile | 2 +
>>> arch/arm/mach-s5pv210/clock.c | 6 +
>>> arch/arm/mach-s5pv210/dev-dsim.c | 149
>> +++++++++++++++++++++++
>>> arch/arm/mach-s5pv210/include/mach/map.h | 4 +
>>> arch/arm/mach-s5pv210/include/mach/regs-clock.h | 3 +-
>>> arch/arm/mach-s5pv210/mach-goni.c | 12 ++
>>> arch/arm/mach-s5pv210/setup-dsim.c | 144
>> ++++++++++++++++++++++
>>> arch/arm/plat-s5p/Kconfig | 5 +
>>> 9 files changed, 335 insertions(+), 1 deletions(-)
>>> create mode 100644 arch/arm/mach-s5pv210/dev-dsim.c
>>> create mode 100644 arch/arm/mach-s5pv210/setup-dsim.c
>>>
[snip]
>>> diff --git a/arch/arm/mach-s5pv210/dev-dsim.c b/arch/arm/mach-
>> s5pv210/dev-dsim.c
[snip]
>>> +
>>> +static struct s5p_platform_dsim dsim_platform_data = {
>>> + .clk_name = "dsim",
>>> + .mipi_1_1v_name = "vmipi_1.1v",
>>> + .mipi_1_8v_name = "vmipi_1.8v",
>>
>> You could avoid passing regulator's names in here. All you need to do is
>> defining regulator supplies properly (see further comments below).
>> Creating well known (e.g. as defined in the datasheet) regulator supply
>> names in the actual driver file (s5p-dsim.c) should be enough.
>> The regulator API can be used to match a regulator and its consumer.
>>
> Regulator's name could be changed according to machine so I think should
> be set to Machine specific file.
First of all it is just my suggestion, please feel free to ignore it.
Regulator names are mostly different across machines but it is
the *"regulator consumer supply"* names that are used for lookup in
calls like regulator_get(). In the setup code of each machine that
wants to use mipi-dsi you could insert an entry dedicated to your
device into the consumers array of specific regulator.
On the other hand, if the number of regulators needed varies across
machines then using platform data structure for the names might be
a solution. But that doesn't look like your case.
>>> + .dsim_info =&dsim_info,
>>> + .dsim_lcd_info =&dsim_lcd_info,
>>> +
>>> + .mipi_power = s5p_dsim_mipi_power,
>>> + .part_reset = s5p_dsim_part_reset,
>>> + .init_d_phy = s5p_dsim_init_d_phy,
>>> + .get_fb_frame_done = NULL,
>>> + .trigger = NULL,
>>> +
>>> + .platform_rev = 1,
>>> +
>>> + /*
>>> + * the stable time of needing to write data on SFR
>>> + * when the mipi mode becomes LP mode.
>>> + */
>>> + .delay_for_stabilization = 600,
>>> +};
>>> +
>>> +struct platform_device s5p_device_dsim = {
>>> + .name = "s5p-dsim",
>>> + .id = 0,
>>> + .num_resources = ARRAY_SIZE(s5p_dsim_resource),
>>> + .resource = s5p_dsim_resource,
>>> + .dev = {
>>> + .platform_data = (void *)&dsim_platform_data,
>>> + },
>>> +};
>>> diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-
>> s5pv210/include/mach/map.h
>>> index 861d7fe..1ad2dad 100644
>>> --- a/arch/arm/mach-s5pv210/include/mach/map.h
>>> +++ b/arch/arm/mach-s5pv210/include/mach/map.h
>>> @@ -69,6 +69,10 @@
>>>
>>> #define S5PV210_PA_FB (0xF8000000)
>>>
>>> +/* MIPI-DSI */
>>> +#define S5PV210_PA_DSI (0xFA500000)
>>> +#define S5PV210_SZ_DSI SZ_1M
>>
>> How about removing S5PV210_SZ_DSI and directly using SZ_1M in dev-dsim.c
>> ? Also, 1MB is probably excessive.
>>
>>> +
>>> #define S5PV210_PA_FIMC0 (0xFB200000)
>>> #define S5PV210_PA_FIMC1 (0xFB300000)
>>> #define S5PV210_PA_FIMC2 (0xFB400000)
>>> diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
>> b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
>>> index ebaabe0..c8b9366 100644
>>> --- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
>>> +++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
>>> @@ -196,7 +196,8 @@
>>> #define S5P_OTHERS_USB_SIG_MASK (1<< 16)
>>>
>>> /* MIPI */
>>> -#define S5P_MIPI_DPHY_EN (3)
>>> +#define S5P_MIPI_DPHY_EN (3<< 0)
>>> +#define S5P_MIPI_M_RESETN (1<< 1)
>>>
>>> /* S5P_DAC_CONTROL */
>>> #define S5P_DAC_ENABLE (1)
>>> diff --git a/arch/arm/mach-s5pv210/mach-goni.c b/arch/arm/mach-
>> s5pv210/mach-goni.c
>>> index b1dcf96..500cc1b 100644
>>> --- a/arch/arm/mach-s5pv210/mach-goni.c
>>> +++ b/arch/arm/mach-s5pv210/mach-goni.c
>>> @@ -269,10 +269,18 @@ static void __init goni_tsp_init(void)
>>> /* MAX8998 regulators */
>>> #if defined(CONFIG_REGULATOR_MAX8998) ||
>> defined(CONFIG_REGULATOR_MAX8998_MODULE)
>>>
>>> +static struct regulator_consumer_supply goni_ldo3_consumers[] = {
>>> + REGULATOR_SUPPLY("vmipi_1.1v", ""),
>>> +};
>>
>> You could just do:
>> REGULATOR_SUPPLY("vmipi_1.1v", "s5p-dsim.0"),
>>
>> The second argument is used to match your consumer device with the
>> relevant regulator supply.
>>
> Ok, it would be corrected.
>
>> Similar could be done in other machine's board setup files and there is
>> no need to pass anything in the platform data.
>>
>> Then in your driver probe you might just do:
>>
>> ... = regulator_get(&pdev->dev, "vmipi_1.1v");
>>
> I mentioned before.
>>> +
>>> static struct regulator_consumer_supply goni_ldo5_consumers[] = {
>>> REGULATOR_SUPPLY("vmmc", "s3c-sdhci.0"),
>>> };
>>>
>>> +static struct regulator_consumer_supply goni_ldo7_consumers[] = {
>>> + REGULATOR_SUPPLY("vmipi_1.8v", ""),
>>> +};
>>
>> Ditto.
>>
>>> +
>>> static struct regulator_init_data goni_ldo2_data = {
>>> .constraints = {
>>> .name = "VALIVE_1.1V",
>>> @@ -294,6 +302,8 @@ static struct regulator_init_data goni_ldo3_data = {
>>> .apply_uV = 1,
>>> .always_on = 1,
>>> },
>>> + .num_consumer_supplies = ARRAY_SIZE(goni_ldo3_consumers),
>>> + .consumer_supplies = goni_ldo3_consumers,
>>> };
>>>
>>> static struct regulator_init_data goni_ldo4_data = {
>>> @@ -333,6 +343,8 @@ static struct regulator_init_data goni_ldo7_data = {
>>> .apply_uV = 1,
>>> .always_on = 1,
>>> },
>>> + .num_consumer_supplies = ARRAY_SIZE(goni_ldo7_consumers),
>>> + .consumer_supplies = goni_ldo7_consumers,
>>> };
>>>
>>> static struct regulator_init_data goni_ldo8_data = {
>>> diff --git a/arch/arm/mach-s5pv210/setup-dsim.c b/arch/arm/mach-
>> s5pv210/setup-dsim.c
>>> new file mode 100644
>>> index 0000000..874efa0
>>> --- /dev/null
>>> +++ b/arch/arm/mach-s5pv210/setup-dsim.c
>>> @@ -0,0 +1,144 @@
[snip]
>>> +
>>> +int s5p_dsim_mipi_power(struct dsim_global *dsim, struct regulator
>> *p_mipi_1_1v,
>>> + struct regulator *p_mipi_1_8v, unsigned int enable)
>>> +{
>>> + int ret = -1;
>>> +
>>> + WARN_ON(dsim = NULL);
>>> +
>>> + if (IS_ERR(p_mipi_1_1v) || IS_ERR(p_mipi_1_8v)) {
>>> + dev_err(dsim->dev, "p_mipi_1_1v or p_mipi_1_8v is NULL.\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (enable) {
>>> + if (p_mipi_1_1v)
>>> + ret = regulator_enable(p_mipi_1_1v);
>>> +
>>> + if (ret< 0) {
>>> + dev_err(dsim->dev,
>>> + "failed to enable regulator mipi_1_1v.\n");
>>> + return ret;
>>> + }
>>> +
>>> + if (p_mipi_1_8v)
>>> + ret = regulator_enable(p_mipi_1_8v);
>>> +
>>> + if (ret< 0) {
>>> + dev_err(dsim->dev,
>>> + "failed to enable regulator mipi_1_8v.\n");
>>> + return ret;
>>> + }
>>> + } else {
>>> + if (p_mipi_1_1v)
>>> + ret = regulator_force_disable(p_mipi_1_1v);
>>> + if (ret< 0) {
>>> + dev_err(dsim->dev,
>>> + "failed to disable regulator mipi_1_1v.\n");
>>> + return ret;
>>> + }
>>> +
>>> + if (p_mipi_1_8v)
>>> + ret = regulator_force_disable(p_mipi_1_8v);
>>> + if (ret< 0) {
>>> + dev_err(dsim->dev,
>>> + "failed to disable regulator mipi_1_8v.\n");
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + return ret;
>>> +}
>>
>> This function seem to be called only in the driver and it uses driver's
>> (private?) data, so can you explain what is the purpose of having it in
>> arch?
>> I suspect all the above regulator handling code could well be moved to
>> the driver. Am I missing something?
>>
> As I said above, regulator's name is specific to machine so it would be
> got by driver's probe and then controlled by driver.
Unless you plan to use mipi_power callback outside of the driver I
don't see a good reason for having it here.
Also you are not behaving nice by using regulator_force_disable().
IIRC it should be used only as a last resort, in critical situations.
If the mipi-dsi subsystem shares regulator with other device you will
be cutting off other device's power anytime you are disabling supply
of mipi-csi. Just imagine that this "other device" is the cpu core..
> Note that some machine doesn’t use regulator(gpio instead of regulator)
For those you could define the fixed voltage regulator if appropriate
and you would also get at the same time the reference counting.
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and machine.
2010-11-23 7:16 [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and machine Inki Dae
2010-11-27 17:07 ` Sylwester Nawrocki
2010-11-30 1:30 ` Inki Dae
@ 2010-12-01 2:29 ` Inki Dae
2 siblings, 0 replies; 5+ messages in thread
From: Inki Dae @ 2010-12-01 2:29 UTC (permalink / raw)
To: linux-fbdev
Hi Sylwester.
Thank you for your comments.
> -----Original Message-----
> From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-
> owner@vger.kernel.org] On Behalf Of Sylwester Nawrocki
> Sent: Wednesday, December 01, 2010 7:57 AM
> To: Inki Dae
> Cc: linux-fbdev@vger.kernel.org; kgene.kim@samsung.com;
> kyungmin.park@samsung.com; lethal@linux-sh.org; akpm@linux-foundation.org;
> linux-arm-kernel@lists.infradead.org; 'Sylwester Nawrocki'
> Subject: Re: [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and
> machine.
>
> Hi Inki,
>
> On 11/30/2010 02:30 AM, Inki Dae wrote:
> > Hello, Sylwester.
> > Long time no see. :)
> >
> > Thank you for your comments.
> >
> >> -----Original Message-----
> >> From: Sylwester Nawrocki [mailto:spnlinux@gmail.com]
> >> Sent: Sunday, November 28, 2010 2:07 AM
> >> To: Inki Dae
> >> Cc: linux-fbdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> kyungmin.park@samsung.com; kgene.kim@samsung.com; akpm@linux-
> >> foundation.org; lethal@linux-sh.org
> >> Subject: Re: [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and
> >> machine.
> >>
> >> Hello,
> >>
> >> On 11/23/2010 08:16 AM, Inki Dae wrote:
> >>> clock.c
> >>> - added dsim clock gate.
> >>>
> >>> dev-dsim.c
> >>> - platform and machine specific definitions.
> >>> Now just supports only MACHINE GONI so for other machines,
> >>> mipi_1_1v_name and mipi_1_8v_name values should be changed to
> >>> proper regulator name at each machine file.
> >>>
> >>> setup-dsim.c
> >>> - platform specific definitions.
> >>>
> >>> Signed-off-by: Inki Dae<inki.dae@samsung.com>
> >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >>> ---
> >>> arch/arm/mach-s5pv210/Kconfig | 11 ++
> >>> arch/arm/mach-s5pv210/Makefile | 2 +
> >>> arch/arm/mach-s5pv210/clock.c | 6 +
> >>> arch/arm/mach-s5pv210/dev-dsim.c | 149
> >> +++++++++++++++++++++++
> >>> arch/arm/mach-s5pv210/include/mach/map.h | 4 +
> >>> arch/arm/mach-s5pv210/include/mach/regs-clock.h | 3 +-
> >>> arch/arm/mach-s5pv210/mach-goni.c | 12 ++
> >>> arch/arm/mach-s5pv210/setup-dsim.c | 144
> >> ++++++++++++++++++++++
> >>> arch/arm/plat-s5p/Kconfig | 5 +
> >>> 9 files changed, 335 insertions(+), 1 deletions(-)
> >>> create mode 100644 arch/arm/mach-s5pv210/dev-dsim.c
> >>> create mode 100644 arch/arm/mach-s5pv210/setup-dsim.c
> >>>
> [snip]
> >>> diff --git a/arch/arm/mach-s5pv210/dev-dsim.c b/arch/arm/mach-
> >> s5pv210/dev-dsim.c
> [snip]
> >>> +
> >>> +static struct s5p_platform_dsim dsim_platform_data = {
> >>> + .clk_name = "dsim",
> >>> + .mipi_1_1v_name = "vmipi_1.1v",
> >>> + .mipi_1_8v_name = "vmipi_1.8v",
> >>
> >> You could avoid passing regulator's names in here. All you need to do
> is
> >> defining regulator supplies properly (see further comments below).
> >> Creating well known (e.g. as defined in the datasheet) regulator supply
> >> names in the actual driver file (s5p-dsim.c) should be enough.
> >> The regulator API can be used to match a regulator and its consumer.
> >>
> > Regulator's name could be changed according to machine so I think should
> > be set to Machine specific file.
>
> First of all it is just my suggestion, please feel free to ignore it.
>
> Regulator names are mostly different across machines but it is
> the *"regulator consumer supply"* names that are used for lookup in
> calls like regulator_get(). In the setup code of each machine that
> wants to use mipi-dsi you could insert an entry dedicated to your
> device into the consumers array of specific regulator.
>
> On the other hand, if the number of regulators needed varies across
> machines then using platform data structure for the names might be
> a solution. But that doesn't look like your case.
>
If it removes regulator consumer name from platform data then s5p_dsim_mipi_power() below should be added to machine code. In this case, every when mipi-dsi driver is ported to other machines regulator consumer name in s5p_dsim_mipi_power() should be modified properly in other words, s5p_dsim_mipi_power() couldn't be used commonly. if platform data includes regulator consumer name then it is done just only changing consumer name to machine specific and also s5p_dsim_mipi_power() could be used commonly. If I missed things then please feel free to give me your opinion.
> >>> + .dsim_info =&dsim_info,
> >>> + .dsim_lcd_info =&dsim_lcd_info,
> >>> +
> >>> + .mipi_power = s5p_dsim_mipi_power,
> >>> + .part_reset = s5p_dsim_part_reset,
> >>> + .init_d_phy = s5p_dsim_init_d_phy,
> >>> + .get_fb_frame_done = NULL,
> >>> + .trigger = NULL,
> >>> +
> >>> + .platform_rev = 1,
> >>> +
> >>> + /*
> >>> + * the stable time of needing to write data on SFR
> >>> + * when the mipi mode becomes LP mode.
> >>> + */
> >>> + .delay_for_stabilization = 600,
> >>> +};
> >>> +
> >>> +struct platform_device s5p_device_dsim = {
> >>> + .name = "s5p-dsim",
> >>> + .id = 0,
> >>> + .num_resources = ARRAY_SIZE(s5p_dsim_resource),
> >>> + .resource = s5p_dsim_resource,
> >>> + .dev = {
> >>> + .platform_data = (void *)&dsim_platform_data,
> >>> + },
> >>> +};
> >>> diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-
> >> s5pv210/include/mach/map.h
> >>> index 861d7fe..1ad2dad 100644
> >>> --- a/arch/arm/mach-s5pv210/include/mach/map.h
> >>> +++ b/arch/arm/mach-s5pv210/include/mach/map.h
> >>> @@ -69,6 +69,10 @@
> >>>
> >>> #define S5PV210_PA_FB (0xF8000000)
> >>>
> >>> +/* MIPI-DSI */
> >>> +#define S5PV210_PA_DSI (0xFA500000)
> >>> +#define S5PV210_SZ_DSI SZ_1M
> >>
> >> How about removing S5PV210_SZ_DSI and directly using SZ_1M in dev-
> dsim.c
> >> ? Also, 1MB is probably excessive.
> >>
> >>> +
> >>> #define S5PV210_PA_FIMC0 (0xFB200000)
> >>> #define S5PV210_PA_FIMC1 (0xFB300000)
> >>> #define S5PV210_PA_FIMC2 (0xFB400000)
> >>> diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> >> b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> >>> index ebaabe0..c8b9366 100644
> >>> --- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> >>> +++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
> >>> @@ -196,7 +196,8 @@
> >>> #define S5P_OTHERS_USB_SIG_MASK (1<< 16)
> >>>
> >>> /* MIPI */
> >>> -#define S5P_MIPI_DPHY_EN (3)
> >>> +#define S5P_MIPI_DPHY_EN (3<< 0)
> >>> +#define S5P_MIPI_M_RESETN (1<< 1)
> >>>
> >>> /* S5P_DAC_CONTROL */
> >>> #define S5P_DAC_ENABLE (1)
> >>> diff --git a/arch/arm/mach-s5pv210/mach-goni.c b/arch/arm/mach-
> >> s5pv210/mach-goni.c
> >>> index b1dcf96..500cc1b 100644
> >>> --- a/arch/arm/mach-s5pv210/mach-goni.c
> >>> +++ b/arch/arm/mach-s5pv210/mach-goni.c
> >>> @@ -269,10 +269,18 @@ static void __init goni_tsp_init(void)
> >>> /* MAX8998 regulators */
> >>> #if defined(CONFIG_REGULATOR_MAX8998) ||
> >> defined(CONFIG_REGULATOR_MAX8998_MODULE)
> >>>
> >>> +static struct regulator_consumer_supply goni_ldo3_consumers[] = {
> >>> + REGULATOR_SUPPLY("vmipi_1.1v", ""),
> >>> +};
> >>
> >> You could just do:
> >> REGULATOR_SUPPLY("vmipi_1.1v", "s5p-dsim.0"),
> >>
> >> The second argument is used to match your consumer device with the
> >> relevant regulator supply.
> >>
> > Ok, it would be corrected.
> >
> >> Similar could be done in other machine's board setup files and there is
> >> no need to pass anything in the platform data.
> >>
> >> Then in your driver probe you might just do:
> >>
> >> ... = regulator_get(&pdev->dev, "vmipi_1.1v");
> >>
> > I mentioned before.
> >>> +
> >>> static struct regulator_consumer_supply goni_ldo5_consumers[] = {
> >>> REGULATOR_SUPPLY("vmmc", "s3c-sdhci.0"),
> >>> };
> >>>
> >>> +static struct regulator_consumer_supply goni_ldo7_consumers[] = {
> >>> + REGULATOR_SUPPLY("vmipi_1.8v", ""),
> >>> +};
> >>
> >> Ditto.
> >>
> >>> +
> >>> static struct regulator_init_data goni_ldo2_data = {
> >>> .constraints = {
> >>> .name = "VALIVE_1.1V",
> >>> @@ -294,6 +302,8 @@ static struct regulator_init_data goni_ldo3_data > {
> >>> .apply_uV = 1,
> >>> .always_on = 1,
> >>> },
> >>> + .num_consumer_supplies = ARRAY_SIZE(goni_ldo3_consumers),
> >>> + .consumer_supplies = goni_ldo3_consumers,
> >>> };
> >>>
> >>> static struct regulator_init_data goni_ldo4_data = {
> >>> @@ -333,6 +343,8 @@ static struct regulator_init_data goni_ldo7_data > {
> >>> .apply_uV = 1,
> >>> .always_on = 1,
> >>> },
> >>> + .num_consumer_supplies = ARRAY_SIZE(goni_ldo7_consumers),
> >>> + .consumer_supplies = goni_ldo7_consumers,
> >>> };
> >>>
> >>> static struct regulator_init_data goni_ldo8_data = {
> >>> diff --git a/arch/arm/mach-s5pv210/setup-dsim.c b/arch/arm/mach-
> >> s5pv210/setup-dsim.c
> >>> new file mode 100644
> >>> index 0000000..874efa0
> >>> --- /dev/null
> >>> +++ b/arch/arm/mach-s5pv210/setup-dsim.c
> >>> @@ -0,0 +1,144 @@
> [snip]
> >>> +
> >>> +int s5p_dsim_mipi_power(struct dsim_global *dsim, struct regulator
> >> *p_mipi_1_1v,
> >>> + struct regulator *p_mipi_1_8v, unsigned int enable)
> >>> +{
> >>> + int ret = -1;
> >>> +
> >>> + WARN_ON(dsim = NULL);
> >>> +
> >>> + if (IS_ERR(p_mipi_1_1v) || IS_ERR(p_mipi_1_8v)) {
> >>> + dev_err(dsim->dev, "p_mipi_1_1v or p_mipi_1_8v is NULL.\n");
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + if (enable) {
> >>> + if (p_mipi_1_1v)
> >>> + ret = regulator_enable(p_mipi_1_1v);
> >>> +
> >>> + if (ret< 0) {
> >>> + dev_err(dsim->dev,
> >>> + "failed to enable regulator mipi_1_1v.\n");
> >>> + return ret;
> >>> + }
> >>> +
> >>> + if (p_mipi_1_8v)
> >>> + ret = regulator_enable(p_mipi_1_8v);
> >>> +
> >>> + if (ret< 0) {
> >>> + dev_err(dsim->dev,
> >>> + "failed to enable regulator mipi_1_8v.\n");
> >>> + return ret;
> >>> + }
> >>> + } else {
> >>> + if (p_mipi_1_1v)
> >>> + ret = regulator_force_disable(p_mipi_1_1v);
> >>> + if (ret< 0) {
> >>> + dev_err(dsim->dev,
> >>> + "failed to disable regulator mipi_1_1v.\n");
> >>> + return ret;
> >>> + }
> >>> +
> >>> + if (p_mipi_1_8v)
> >>> + ret = regulator_force_disable(p_mipi_1_8v);
> >>> + if (ret< 0) {
> >>> + dev_err(dsim->dev,
> >>> + "failed to disable regulator mipi_1_8v.\n");
> >>> + return ret;
> >>> + }
> >>> + }
> >>> +
> >>> + return ret;
> >>> +}
> >>
> >> This function seem to be called only in the driver and it uses driver's
> >> (private?) data, so can you explain what is the purpose of having it in
> >> arch?
> >> I suspect all the above regulator handling code could well be moved to
> >> the driver. Am I missing something?
> >>
> > As I said above, regulator's name is specific to machine so it would be
> > got by driver's probe and then controlled by driver.
>
> Unless you plan to use mipi_power callback outside of the driver I
> don't see a good reason for having it here.
>
> Also you are not behaving nice by using regulator_force_disable().
> IIRC it should be used only as a last resort, in critical situations.
> If the mipi-dsi subsystem shares regulator with other device you will
> be cutting off other device's power anytime you are disabling supply
> of mipi-csi. Just imagine that this "other device" is the cpu core..
>
> > Note that some machine doesn’t use regulator(gpio instead of regulator)
>
> For those you could define the fixed voltage regulator if appropriate
> and you would also get at the same time the reference counting.
>
Your pointing out is good. Definitely it's my mistake. Using regulator_force_disable() is critical. I will correct it. And also as you said, gpio could be used by regulator framework with fixed voltage regulator.
>
> --
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-12-01 2:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-23 7:16 [PATCH 3/4] S5PC110: add MIPI-DSI support for platform and machine Inki Dae
2010-11-27 17:07 ` Sylwester Nawrocki
2010-11-30 1:30 ` Inki Dae
2010-11-30 22:57 ` Sylwester Nawrocki
2010-12-01 2:29 ` Inki Dae
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).