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:

quoted

Hello, Sylwester.
Long time no see. :)

Thank you for your comments.

quoted

-----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:

quoted

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

+++++++++++++++++++++++

quoted

   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

++++++++++++++++++++++

quoted

   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]

quoted

quoted

quoted

diff --git a/arch/arm/mach-s5pv210/dev-dsim.c b/arch/arm/mach-

s5pv210/dev-dsim.c

[snip]

quoted

quoted

quoted

+
+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

quoted

quoted

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.

quoted

quoted

quoted

+	.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

quoted

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

quoted

quoted

? Also, 1MB is probably excessive.

quoted

+
   #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

quoted

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

quoted

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)

quoted

+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.

quoted

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.

quoted

quoted

+
   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.

quoted

+
   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

quoted

new file mode 100644
index 0000000..874efa0

--- /dev/null
+++ b/arch/arm/mach-s5pv210/setup-dsim.c

@@ -0,0 +1,144 @@

[snip]

quoted

quoted

quoted

+
+int s5p_dsim_mipi_power(struct dsim_global *dsim, struct regulator

*p_mipi_1_1v,

quoted

+	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..

quoted

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