devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
@ 2013-06-25 14:21 Sylwester Nawrocki
  2013-06-25 14:21 ` [PATCH v2 2/5] ARM: dts: Add MIPI PHY node to exynos4.dtsi Sylwester Nawrocki
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: Sylwester Nawrocki @ 2013-06-25 14:21 UTC (permalink / raw)
  To: linux-arm-kernel, linux-samsung-soc
  Cc: kishon, linux-media, kyungmin.park, balbi, t.figa,
	devicetree-discuss, kgene.kim, dh09.lee, jg1.han, inki.dae,
	plagnioj, linux-fbdev, Sylwester Nawrocki

Add a PHY provider driver for the Samsung S5P/Exynos SoC MIPI CSI-2
receiver and MIPI DSI transmitter DPHYs.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v1:
 - enabled build as module and with CONFIG_OF disabled
 - added phy_id enum,
 - of_address_to_resource() replaced with platform_get_resource(),
 - adapted to changes in the PHY API v7, v8 - added phy labels,
 - added MODULE_DEVICE_TABLE() entry,
 - driver file renamed to phy-exynos-mipi-video.c,
 - changed DT compatible string to "samsung,s5pv210-mipi-video-phy",
 - corrected the compatible property's description.
---
 .../phy/samsung,s5pv210-mipi-video-phy.txt         |   14 ++
 drivers/phy/Kconfig                                |    9 +
 drivers/phy/Makefile                               |    3 +-
 drivers/phy/phy-exynos-mipi-video.c                |  173 ++++++++++++++++++++
 4 files changed, 198 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/phy/samsung,s5pv210-mipi-video-phy.txt
 create mode 100644 drivers/phy/phy-exynos-mipi-video.c

diff --git a/Documentation/devicetree/bindings/phy/samsung,s5pv210-mipi-video-phy.txt b/Documentation/devicetree/bindings/phy/samsung,s5pv210-mipi-video-phy.txt
new file mode 100644
index 0000000..5ff208c
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/samsung,s5pv210-mipi-video-phy.txt
@@ -0,0 +1,14 @@
+Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY
+-------------------------------------------------
+
+Required properties:
+- compatible : should be "samsung,s5pv210-mipi-video-phy";
+- reg : offset and length of the MIPI DPHY register set;
+- #phy-cells : from the generic phy bindings, must be 1;
+
+For "samsung,s5pv210-mipi-video-phy" compatible PHYs the second cell in
+the PHY specifier identifies the PHY and its meaning is as follows:
+  0 - MIPI CSIS 0,
+  1 - MIPI DSIM 0,
+  2 - MIPI CSIS 1,
+  3 - MIPI DSIM 1.
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 5f85909..6f446d0 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -11,3 +11,12 @@ menuconfig GENERIC_PHY
 	  devices present in the kernel. This layer will have the generic
 	  API by which phy drivers can create PHY using the phy framework and
 	  phy users can obtain reference to the PHY.
+
+if GENERIC_PHY
+
+config PHY_EXYNOS_MIPI_VIDEO
+	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
+	help
+	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung
+	  S5P and EXYNOS SoCs.
+endif
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 9e9560f..71d8841 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -2,4 +2,5 @@
 # Makefile for the phy drivers.
 #
 
-obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
+obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
diff --git a/drivers/phy/phy-exynos-mipi-video.c b/drivers/phy/phy-exynos-mipi-video.c
new file mode 100644
index 0000000..074a623
--- /dev/null
+++ b/drivers/phy/phy-exynos-mipi-video.c
@@ -0,0 +1,173 @@
+/*
+ * Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
+ *
+ * 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/delay.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+/* MIPI_PHYn_CONTROL register offset: n = 0..1 */
+#define EXYNOS_MIPI_PHY_CONTROL(n)	((n) * 4)
+#define EXYNOS_MIPI_PHY_ENABLE		(1 << 0)
+#define EXYNOS_MIPI_PHY_SRESETN		(1 << 1)
+#define EXYNOS_MIPI_PHY_MRESETN		(1 << 2)
+#define EXYNOS_MIPI_PHY_RESET_MASK	(3 << 1)
+
+enum phy_id {
+	PHY_CSIS0,
+	PHY_DSIM0,
+	PHY_CSIS1,
+	PHY_DSIM1,
+	NUM_PHYS
+};
+
+struct exynos_video_phy {
+	spinlock_t slock;
+	struct phy *phys[NUM_PHYS];
+	void __iomem *regs;
+};
+
+static int __set_phy_state(struct exynos_video_phy *state,
+				enum phy_id id, unsigned int on)
+{
+	void __iomem *addr;
+	unsigned long flags;
+	u32 reg, reset;
+
+	if (WARN_ON(id > NUM_PHYS))
+		return -EINVAL;
+
+	addr = state->regs + EXYNOS_MIPI_PHY_CONTROL(id / 2);
+
+	if (id == PHY_DSIM0 || id == PHY_DSIM1)
+		reset = EXYNOS_MIPI_PHY_MRESETN;
+	else
+		reset = EXYNOS_MIPI_PHY_SRESETN;
+
+	spin_lock_irqsave(&state->slock, flags);
+	reg = readl(addr);
+	if (on)
+		reg |= reset;
+	else
+		reg &= ~reset;
+	writel(reg, addr);
+
+	/* Clear ENABLE bit only if MRESETN, SRESETN bits are not set. */
+	if (on)
+		reg |= EXYNOS_MIPI_PHY_ENABLE;
+	else if (!(reg & EXYNOS_MIPI_PHY_RESET_MASK))
+		reg &= ~EXYNOS_MIPI_PHY_ENABLE;
+
+	writel(reg, addr);
+	spin_unlock_irqrestore(&state->slock, flags);
+
+	pr_debug("%s(): id: %d, on: %d, addr: %#x, base: %#x\n",
+		 __func__, id, on, (u32)addr, (u32)state->regs);
+
+	return 0;
+}
+
+static int exynos_video_phy_power_on(struct phy *phy)
+{
+	struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
+	return __set_phy_state(state, phy->id, 1);
+}
+
+static int exynos_video_phy_power_off(struct phy *phy)
+{
+	struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
+	return __set_phy_state(state, phy->id, 0);
+}
+
+static struct phy *exynos_video_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	struct exynos_video_phy *state = dev_get_drvdata(dev);
+
+	if (WARN_ON(args->args[0] > NUM_PHYS))
+		return NULL;
+
+	return state->phys[args->args[0]];
+}
+
+static struct phy_ops exynos_video_phy_ops = {
+	.power_on	= exynos_video_phy_power_on,
+	.power_off	= exynos_video_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int exynos_video_phy_probe(struct platform_device *pdev)
+{
+	struct exynos_video_phy *state;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct phy_provider *phy_provider;
+	int i;
+
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	state->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(state->regs))
+		return PTR_ERR(state->regs);
+
+	dev_set_drvdata(dev, state);
+
+	phy_provider = devm_of_phy_provider_register(dev,
+					exynos_video_phy_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	for (i = 0; i < NUM_PHYS; i++) {
+		char label[8];
+
+		snprintf(label, sizeof(label), "%s.%d",
+				i == PHY_DSIM0 || i == PHY_DSIM1 ?
+				"dsim" : "csis", i / 2);
+
+		state->phys[i] = devm_phy_create(dev, i, &exynos_video_phy_ops,
+								label, state);
+		if (IS_ERR(state->phys[i])) {
+			dev_err(dev, "failed to create PHY %s\n", label);
+			return PTR_ERR(state->phys[i]);
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id exynos_video_phy_of_match[] = {
+	{ .compatible = "samsung,s5pv210-mipi-video-phy" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, exynos_video_phy_of_match);
+
+static struct platform_driver exynos_video_phy_driver = {
+	.probe	= exynos_video_phy_probe,
+	.driver = {
+		.of_match_table	= exynos_video_phy_of_match,
+		.name  = "exynos-mipi-video-phy",
+		.owner = THIS_MODULE,
+	}
+};
+module_platform_driver(exynos_video_phy_driver);
+
+MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC MIPI CSI-2/DSI PHY driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 2/5] ARM: dts: Add MIPI PHY node to exynos4.dtsi
  2013-06-25 14:21 [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs Sylwester Nawrocki
@ 2013-06-25 14:21 ` Sylwester Nawrocki
  2013-06-25 14:21 ` [PATCH v2 3/5] video: exynos_mipi_dsim: Use generic PHY driver Sylwester Nawrocki
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 24+ messages in thread
From: Sylwester Nawrocki @ 2013-06-25 14:21 UTC (permalink / raw)
  To: linux-arm-kernel, linux-samsung-soc
  Cc: kishon, linux-media, kyungmin.park, balbi, t.figa,
	devicetree-discuss, kgene.kim, dh09.lee, jg1.han, inki.dae,
	plagnioj, linux-fbdev, Sylwester Nawrocki

Add PHY provider node for the MIPI CSIS and MIPI DSIM PHYs.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4.dtsi |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index 4d61120..9542088 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -98,6 +98,12 @@
 		reg = <0x10010000 0x400>;
 	};
 
+	mipi_phy: video-phy@10020710 {
+		compatible = "samsung,s5pv210-mipi-video-phy";
+		reg = <0x10020710 8>;
+		#phy-cells = <1>;
+	};
+
 	camera {
 		compatible = "samsung,fimc", "simple-bus";
 		status = "disabled";
@@ -147,6 +153,8 @@
 			interrupts = <0 78 0>;
 			bus-width = <4>;
 			samsung,power-domain = <&pd_cam>;
+			phys = <&mipi_phy 0>;
+			phy-names = "csis";
 			status = "disabled";
 		};
 
@@ -156,6 +164,8 @@
 			interrupts = <0 80 0>;
 			bus-width = <2>;
 			samsung,power-domain = <&pd_cam>;
+			phys = <&mipi_phy 2>;
+			phy-names = "csis";
 			status = "disabled";
 		};
 	};
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 3/5] video: exynos_mipi_dsim: Use generic PHY driver
  2013-06-25 14:21 [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs Sylwester Nawrocki
  2013-06-25 14:21 ` [PATCH v2 2/5] ARM: dts: Add MIPI PHY node to exynos4.dtsi Sylwester Nawrocki
@ 2013-06-25 14:21 ` Sylwester Nawrocki
       [not found]   ` <1372170110-12993-3-git-send-email-s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
  2013-06-25 14:21 ` [PATCH v2 4/5] exynos4-is: Use generic MIPI CSIS " Sylwester Nawrocki
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: Sylwester Nawrocki @ 2013-06-25 14:21 UTC (permalink / raw)
  To: linux-arm-kernel, linux-samsung-soc
  Cc: kishon, linux-media, kyungmin.park, balbi, t.figa,
	devicetree-discuss, kgene.kim, dh09.lee, jg1.han, inki.dae,
	plagnioj, linux-fbdev, Sylwester Nawrocki

Use the generic PHY API instead of the platform callback to control
the MIPI DSIM DPHY. The 'phy_label' field is added to the platform
data structure to allow PHY lookup on non-dt platforms.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/video/exynos/exynos_mipi_dsi.c |   18 +++++++++---------
 include/video/exynos_mipi_dsim.h       |    6 ++++--
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
index 32e5406..1f96004 100644
--- a/drivers/video/exynos/exynos_mipi_dsi.c
+++ b/drivers/video/exynos/exynos_mipi_dsi.c
@@ -156,8 +156,7 @@ static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
 		exynos_mipi_regulator_enable(dsim);
 
 		/* enable MIPI-DSI PHY. */
-		if (dsim->pd->phy_enable)
-			dsim->pd->phy_enable(pdev, true);
+		phy_power_on(dsim->phy);
 
 		clk_enable(dsim->clock);
 
@@ -373,6 +372,10 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	dsim->phy = devm_phy_get(&pdev->dev, dsim_pd->phy_label);
+	if (IS_ERR(dsim->phy))
+		return PTR_ERR(dsim->phy);
+
 	dsim->clock = devm_clk_get(&pdev->dev, "dsim0");
 	if (IS_ERR(dsim->clock)) {
 		dev_err(&pdev->dev, "failed to get dsim clock source\n");
@@ -439,8 +442,7 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
 	exynos_mipi_regulator_enable(dsim);
 
 	/* enable MIPI-DSI PHY. */
-	if (dsim->pd->phy_enable)
-		dsim->pd->phy_enable(pdev, true);
+	phy_power_on(dsim->phy);
 
 	exynos_mipi_update_cfg(dsim);
 
@@ -504,9 +506,8 @@ static int exynos_mipi_dsi_suspend(struct device *dev)
 	if (client_drv && client_drv->suspend)
 		client_drv->suspend(client_dev);
 
-	/* enable MIPI-DSI PHY. */
-	if (dsim->pd->phy_enable)
-		dsim->pd->phy_enable(pdev, false);
+	/* disable MIPI-DSI PHY. */
+	phy_power_off(dsim->phy);
 
 	clk_disable(dsim->clock);
 
@@ -536,8 +537,7 @@ static int exynos_mipi_dsi_resume(struct device *dev)
 	exynos_mipi_regulator_enable(dsim);
 
 	/* enable MIPI-DSI PHY. */
-	if (dsim->pd->phy_enable)
-		dsim->pd->phy_enable(pdev, true);
+	phy_power_on(dsim->phy);
 
 	clk_enable(dsim->clock);
 
diff --git a/include/video/exynos_mipi_dsim.h b/include/video/exynos_mipi_dsim.h
index 89dc88a..fd69beb 100644
--- a/include/video/exynos_mipi_dsim.h
+++ b/include/video/exynos_mipi_dsim.h
@@ -216,6 +216,7 @@ struct mipi_dsim_config {
  *	automatically.
  * @e_clk_src: select byte clock source.
  * @pd: pointer to MIPI-DSI driver platform data.
+ * @phy: pointer to the generic PHY
  */
 struct mipi_dsim_device {
 	struct device			*dev;
@@ -236,6 +237,7 @@ struct mipi_dsim_device {
 	bool				suspended;
 
 	struct mipi_dsim_platform_data	*pd;
+	struct phy			*phy;
 };
 
 /*
@@ -248,7 +250,7 @@ struct mipi_dsim_device {
  * @enabled: indicate whether mipi controller got enabled or not.
  * @lcd_panel_info: pointer for lcd panel specific structure.
  *	this structure specifies width, height, timing and polarity and so on.
- * @phy_enable: pointer to a callback controlling D-PHY enable/reset
+ * @phy_label: the generic PHY label
  */
 struct mipi_dsim_platform_data {
 	char				lcd_panel_name[PANEL_NAME_SIZE];
@@ -257,7 +259,7 @@ struct mipi_dsim_platform_data {
 	unsigned int			enabled;
 	void				*lcd_panel_info;
 
-	int (*phy_enable)(struct platform_device *pdev, bool on);
+	const char 			*phy_label;
 };
 
 /*
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 4/5] exynos4-is: Use generic MIPI CSIS PHY driver
  2013-06-25 14:21 [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs Sylwester Nawrocki
  2013-06-25 14:21 ` [PATCH v2 2/5] ARM: dts: Add MIPI PHY node to exynos4.dtsi Sylwester Nawrocki
  2013-06-25 14:21 ` [PATCH v2 3/5] video: exynos_mipi_dsim: Use generic PHY driver Sylwester Nawrocki
@ 2013-06-25 14:21 ` Sylwester Nawrocki
  2013-06-25 15:09   ` Felipe Balbi
  2013-06-25 14:21 ` [PATCH v2 5/5] ARM: Samsung: Remove MIPI PHY setup code Sylwester Nawrocki
  2013-06-25 15:06 ` [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs Felipe Balbi
  4 siblings, 1 reply; 24+ messages in thread
From: Sylwester Nawrocki @ 2013-06-25 14:21 UTC (permalink / raw)
  To: linux-arm-kernel, linux-samsung-soc
  Cc: kishon, linux-media, kyungmin.park, balbi, t.figa,
	devicetree-discuss, kgene.kim, dh09.lee, jg1.han, inki.dae,
	plagnioj, linux-fbdev, Sylwester Nawrocki

Use the generic PHY API instead of the platform callback to control
the MIPI CSIS DPHY. The 'phy_label' field is added to the platform
data structure to allow PHY lookup on non-dt platforms

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/exynos4-is/mipi-csis.c |   16 +++++++++++++---
 include/linux/platform_data/mipi-csis.h       |   11 ++---------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index a2eda9d..8436254 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -20,6 +20,7 @@
 #include <linux/memory.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_data/mipi-csis.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -167,6 +168,7 @@ struct csis_pktbuf {
  * @sd: v4l2_subdev associated with CSIS device instance
  * @index: the hardware instance index
  * @pdev: CSIS platform device
+ * @phy: pointer to the CSIS generic PHY
  * @regs: mmaped I/O registers memory
  * @supplies: CSIS regulator supplies
  * @clock: CSIS clocks
@@ -189,6 +191,8 @@ struct csis_state {
 	struct v4l2_subdev sd;
 	u8 index;
 	struct platform_device *pdev;
+	struct phy *phy;
+	const char *phy_label;
 	void __iomem *regs;
 	struct regulator_bulk_data supplies[CSIS_NUM_SUPPLIES];
 	struct clk *clock[NUM_CSIS_CLOCKS];
@@ -726,6 +730,7 @@ static int s5pcsis_get_platform_data(struct platform_device *pdev,
 	state->index = max(0, pdev->id);
 	state->max_num_lanes = state->index ? CSIS1_MAX_LANES :
 					      CSIS0_MAX_LANES;
+	state->phy_label = pdata->phy_label;
 	return 0;
 }
 
@@ -763,8 +768,9 @@ static int s5pcsis_parse_dt(struct platform_device *pdev,
 					"samsung,csis-wclk");
 
 	state->num_lanes = endpoint.bus.mipi_csi2.num_data_lanes;
-
 	of_node_put(node);
+
+	state->phy_label = "csis";
 	return 0;
 }
 #else
@@ -800,6 +806,10 @@ static int s5pcsis_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	state->phy = devm_phy_get(dev, state->phy_label);
+	if (IS_ERR(state->phy))
+		return PTR_ERR(state->phy);
+
 	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	state->regs = devm_ioremap_resource(dev, mem_res);
 	if (IS_ERR(state->regs))
@@ -893,7 +903,7 @@ static int s5pcsis_pm_suspend(struct device *dev, bool runtime)
 	mutex_lock(&state->lock);
 	if (state->flags & ST_POWERED) {
 		s5pcsis_stop_stream(state);
-		ret = s5p_csis_phy_enable(state->index, false);
+		ret = phy_power_off(state->phy);
 		if (ret)
 			goto unlock;
 		ret = regulator_bulk_disable(CSIS_NUM_SUPPLIES,
@@ -929,7 +939,7 @@ static int s5pcsis_pm_resume(struct device *dev, bool runtime)
 					    state->supplies);
 		if (ret)
 			goto unlock;
-		ret = s5p_csis_phy_enable(state->index, true);
+		ret = phy_power_on(state->phy);
 		if (!ret) {
 			state->flags |= ST_POWERED;
 		} else {
diff --git a/include/linux/platform_data/mipi-csis.h b/include/linux/platform_data/mipi-csis.h
index bf34e17..9214317 100644
--- a/include/linux/platform_data/mipi-csis.h
+++ b/include/linux/platform_data/mipi-csis.h
@@ -17,21 +17,14 @@
  * @wclk_source: CSI wrapper clock selection: 0 - bus clock, 1 - ext. SCLK_CAM
  * @lanes:       number of data lanes used
  * @hs_settle:   HS-RX settle time
+ * @phy_label:	 the generic PHY label
  */
 struct s5p_platform_mipi_csis {
 	unsigned long clk_rate;
 	u8 wclk_source;
 	u8 lanes;
 	u8 hs_settle;
+	const char *phy_label;
 };
 
-/**
- * s5p_csis_phy_enable - global MIPI-CSI receiver D-PHY control
- * @id:     MIPI-CSIS harware instance index (0...1)
- * @on:     true to enable D-PHY and deassert its reset
- *          false to disable D-PHY
- * @return: 0 on success, or negative error code on failure
- */
-int s5p_csis_phy_enable(int id, bool on);
-
 #endif /* __PLAT_SAMSUNG_MIPI_CSIS_H_ */
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 5/5] ARM: Samsung: Remove MIPI PHY setup code
  2013-06-25 14:21 [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs Sylwester Nawrocki
                   ` (2 preceding siblings ...)
  2013-06-25 14:21 ` [PATCH v2 4/5] exynos4-is: Use generic MIPI CSIS " Sylwester Nawrocki
@ 2013-06-25 14:21 ` Sylwester Nawrocki
  2013-06-25 15:09   ` Felipe Balbi
  2013-06-25 15:06 ` [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs Felipe Balbi
  4 siblings, 1 reply; 24+ messages in thread
From: Sylwester Nawrocki @ 2013-06-25 14:21 UTC (permalink / raw)
  To: linux-arm-kernel, linux-samsung-soc
  Cc: kishon, linux-media, kyungmin.park, balbi, t.figa,
	devicetree-discuss, kgene.kim, dh09.lee, jg1.han, inki.dae,
	plagnioj, linux-fbdev, Sylwester Nawrocki

Generic PHY drivers are used to handle the MIPI CSIS and MIPI DSIM
DPHYs so we can remove now unused code at arch/arm/plat-samsung.
In case there is any board file for S5PV210 platforms using MIPI
CSIS/DSIM (not any upstream currently) it should use the generic
PHY API to bind the PHYs to respective PHY consumer drivers.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v1:
 - removed S5P_SETUP_MIPIPHY symbol completely from
   arch/arm/plat-samsung/Kconfig
---
 arch/arm/mach-exynos/include/mach/regs-pmu.h    |    5 --
 arch/arm/mach-s5pv210/include/mach/regs-clock.h |    4 --
 arch/arm/plat-samsung/Kconfig                   |    5 --
 arch/arm/plat-samsung/Makefile                  |    1 -
 arch/arm/plat-samsung/setup-mipiphy.c           |   60 -----------------------
 5 files changed, 75 deletions(-)
 delete mode 100644 arch/arm/plat-samsung/setup-mipiphy.c

diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h b/arch/arm/mach-exynos/include/mach/regs-pmu.h
index 57344b7..2cdb63e 100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -44,11 +44,6 @@
 #define S5P_DAC_PHY_CONTROL			S5P_PMUREG(0x070C)
 #define S5P_DAC_PHY_ENABLE			(1 << 0)
 
-#define S5P_MIPI_DPHY_CONTROL(n)		S5P_PMUREG(0x0710 + (n) * 4)
-#define S5P_MIPI_DPHY_ENABLE			(1 << 0)
-#define S5P_MIPI_DPHY_SRESETN			(1 << 1)
-#define S5P_MIPI_DPHY_MRESETN			(1 << 2)
-
 #define S5P_INFORM0				S5P_PMUREG(0x0800)
 #define S5P_INFORM1				S5P_PMUREG(0x0804)
 #define S5P_INFORM2				S5P_PMUREG(0x0808)
diff --git a/arch/arm/mach-s5pv210/include/mach/regs-clock.h b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
index 032de66..e345584 100644
--- a/arch/arm/mach-s5pv210/include/mach/regs-clock.h
+++ b/arch/arm/mach-s5pv210/include/mach/regs-clock.h
@@ -147,10 +147,6 @@
 #define S5P_HDMI_PHY_CONTROL	S5P_CLKREG(0xE804)
 #define S5P_USB_PHY_CONTROL	S5P_CLKREG(0xE80C)
 #define S5P_DAC_PHY_CONTROL	S5P_CLKREG(0xE810)
-#define S5P_MIPI_DPHY_CONTROL(x) S5P_CLKREG(0xE814)
-#define S5P_MIPI_DPHY_ENABLE	(1 << 0)
-#define S5P_MIPI_DPHY_SRESETN	(1 << 1)
-#define S5P_MIPI_DPHY_MRESETN	(1 << 2)
 
 #define S5P_INFORM0		S5P_CLKREG(0xF000)
 #define S5P_INFORM1		S5P_CLKREG(0xF004)
diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
index b21d9d5..60f6337 100644
--- a/arch/arm/plat-samsung/Kconfig
+++ b/arch/arm/plat-samsung/Kconfig
@@ -388,11 +388,6 @@ config S3C24XX_PWM
 	  Support for exporting the PWM timer blocks via the pwm device
 	  system
 
-config S5P_SETUP_MIPIPHY
-	bool
-	help
-	  Compile in common setup code for MIPI-CSIS and MIPI-DSIM devices
-
 config S3C_SETUP_CAMIF
 	bool
 	help
diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
index 5d7f839..0db786e 100644
--- a/arch/arm/plat-samsung/Makefile
+++ b/arch/arm/plat-samsung/Makefile
@@ -38,7 +38,6 @@ obj-$(CONFIG_S5P_DEV_UART)	+= s5p-dev-uart.o
 obj-$(CONFIG_SAMSUNG_DEV_BACKLIGHT)	+= dev-backlight.o
 
 obj-$(CONFIG_S3C_SETUP_CAMIF)	+= setup-camif.o
-obj-$(CONFIG_S5P_SETUP_MIPIPHY)	+= setup-mipiphy.o
 
 # DMA support
 
diff --git a/arch/arm/plat-samsung/setup-mipiphy.c b/arch/arm/plat-samsung/setup-mipiphy.c
deleted file mode 100644
index 66df315..0000000
--- a/arch/arm/plat-samsung/setup-mipiphy.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
- * Copyright (C) 2011 Samsung Electronics Co., Ltd.
- *
- * S5P - Helper functions for MIPI-CSIS and MIPI-DSIM D-PHY control
- *
- * 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/export.h>
-#include <linux/kernel.h>
-#include <linux/platform_device.h>
-#include <linux/io.h>
-#include <linux/spinlock.h>
-#include <mach/regs-clock.h>
-
-static int __s5p_mipi_phy_control(int id, bool on, u32 reset)
-{
-	static DEFINE_SPINLOCK(lock);
-	void __iomem *addr;
-	unsigned long flags;
-	u32 cfg;
-
-	id = max(0, id);
-	if (id > 1)
-		return -EINVAL;
-
-	addr = S5P_MIPI_DPHY_CONTROL(id);
-
-	spin_lock_irqsave(&lock, flags);
-
-	cfg = __raw_readl(addr);
-	cfg = on ? (cfg | reset) : (cfg & ~reset);
-	__raw_writel(cfg, addr);
-
-	if (on) {
-		cfg |= S5P_MIPI_DPHY_ENABLE;
-	} else if (!(cfg & (S5P_MIPI_DPHY_SRESETN |
-			    S5P_MIPI_DPHY_MRESETN) & ~reset)) {
-		cfg &= ~S5P_MIPI_DPHY_ENABLE;
-	}
-
-	__raw_writel(cfg, addr);
-	spin_unlock_irqrestore(&lock, flags);
-
-	return 0;
-}
-
-int s5p_csis_phy_enable(int id, bool on)
-{
-	return __s5p_mipi_phy_control(id, on, S5P_MIPI_DPHY_SRESETN);
-}
-EXPORT_SYMBOL(s5p_csis_phy_enable);
-
-int s5p_dsim_phy_enable(struct platform_device *pdev, bool on)
-{
-	return __s5p_mipi_phy_control(pdev->id, on, S5P_MIPI_DPHY_MRESETN);
-}
-EXPORT_SYMBOL(s5p_dsim_phy_enable);
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-25 14:21 [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs Sylwester Nawrocki
                   ` (3 preceding siblings ...)
  2013-06-25 14:21 ` [PATCH v2 5/5] ARM: Samsung: Remove MIPI PHY setup code Sylwester Nawrocki
@ 2013-06-25 15:06 ` Felipe Balbi
  2013-06-25 17:44   ` Sylwester Nawrocki
  2013-06-26 15:00   ` Sylwester Nawrocki
  4 siblings, 2 replies; 24+ messages in thread
From: Felipe Balbi @ 2013-06-25 15:06 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: linux-arm-kernel, linux-samsung-soc, kishon, linux-media,
	kyungmin.park, balbi, t.figa, devicetree-discuss, kgene.kim,
	dh09.lee, jg1.han, inki.dae, plagnioj, linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 4608 bytes --]

Hi,

On Tue, Jun 25, 2013 at 04:21:46PM +0200, Sylwester Nawrocki wrote:
> +enum phy_id {
> +	PHY_CSIS0,
> +	PHY_DSIM0,
> +	PHY_CSIS1,
> +	PHY_DSIM1,
> +	NUM_PHYS

please prepend these with EXYNOS_PHY_ or EXYNOS_MIPI_PHY_

> +struct exynos_video_phy {
> +	spinlock_t slock;
> +	struct phy *phys[NUM_PHYS];

more than one phy ? This means you should instantiate driver multiple
drivers. Each phy id should call probe again.

> +static int __set_phy_state(struct exynos_video_phy *state,
> +				enum phy_id id, unsigned int on)
> +{
> +	void __iomem *addr;
> +	unsigned long flags;
> +	u32 reg, reset;
> +
> +	if (WARN_ON(id > NUM_PHYS))
> +		return -EINVAL;

you don't want to do this, actually. It'll bug you everytime you want to
add another phy ID :-)

> +	addr = state->regs + EXYNOS_MIPI_PHY_CONTROL(id / 2);
> +
> +	if (id == PHY_DSIM0 || id == PHY_DSIM1)
> +		reset = EXYNOS_MIPI_PHY_MRESETN;
> +	else
> +		reset = EXYNOS_MIPI_PHY_SRESETN;
> +
> +	spin_lock_irqsave(&state->slock, flags);
> +	reg = readl(addr);
> +	if (on)
> +		reg |= reset;
> +	else
> +		reg &= ~reset;
> +	writel(reg, addr);
> +
> +	/* Clear ENABLE bit only if MRESETN, SRESETN bits are not set. */
> +	if (on)
> +		reg |= EXYNOS_MIPI_PHY_ENABLE;
> +	else if (!(reg & EXYNOS_MIPI_PHY_RESET_MASK))
> +		reg &= ~EXYNOS_MIPI_PHY_ENABLE;
> +
> +	writel(reg, addr);
> +	spin_unlock_irqrestore(&state->slock, flags);
> +
> +	pr_debug("%s(): id: %d, on: %d, addr: %#x, base: %#x\n",
> +		 __func__, id, on, (u32)addr, (u32)state->regs);

use dev_dbg() instead.

> +
> +	return 0;
> +}
> +
> +static int exynos_video_phy_power_on(struct phy *phy)
> +{
> +	struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);

looks like we should have phy_get_drvdata() helper :-) Kishon ?

> +static struct phy *exynos_video_phy_xlate(struct device *dev,
> +					struct of_phandle_args *args)
> +{
> +	struct exynos_video_phy *state = dev_get_drvdata(dev);
> +
> +	if (WARN_ON(args->args[0] > NUM_PHYS))
> +		return NULL;

please remove this check.

> +	return state->phys[args->args[0]];

and your xlate is 'wrong'.

> +}
> +
> +static struct phy_ops exynos_video_phy_ops = {
> +	.power_on	= exynos_video_phy_power_on,
> +	.power_off	= exynos_video_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static int exynos_video_phy_probe(struct platform_device *pdev)
> +{
> +	struct exynos_video_phy *state;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct phy_provider *phy_provider;
> +	int i;
> +
> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	state->regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(state->regs))
> +		return PTR_ERR(state->regs);
> +
> +	dev_set_drvdata(dev, state);

you can use platform_set_drvdata(pdev, state);

same thing though, no strong feelings.

> +	phy_provider = devm_of_phy_provider_register(dev,
> +					exynos_video_phy_xlate);
> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
> +	for (i = 0; i < NUM_PHYS; i++) {
> +		char label[8];
> +
> +		snprintf(label, sizeof(label), "%s.%d",
> +				i == PHY_DSIM0 || i == PHY_DSIM1 ?
> +				"dsim" : "csis", i / 2);
> +
> +		state->phys[i] = devm_phy_create(dev, i, &exynos_video_phy_ops,
> +								label, state);
> +		if (IS_ERR(state->phys[i])) {
> +			dev_err(dev, "failed to create PHY %s\n", label);
> +			return PTR_ERR(state->phys[i]);
> +		}
> +	}

this really doesn't look correct to me. It looks like you have multiple
PHYs, one for each ID. So your probe should be called for each PHY ID
and you have several phy_providers too.

> +static const struct of_device_id exynos_video_phy_of_match[] = {
> +	{ .compatible = "samsung,s5pv210-mipi-video-phy" },

and this should contain all PHY IDs:

	{ .compatible = "samsung,s5pv210-mipi-video-dsim0-phy",
		.data = (const void *) DSIM0, },
	{ .compatible = "samsung,s5pv210-mipi-video-dsim1-phy",
		.data = (const void *) DSIM1, },
	{ .compatible = "samsung,s5pv210-mipi-video-csi0-phy"
		.data = (const void *) CSI0, },
	{ .compatible = "samsung,s5pv210-mipi-video-csi1-phy"
		.data = (const void *) CSI1, },

then on your probe you can fetch that data field and use it as phy->id.

> +static struct platform_driver exynos_video_phy_driver = {
> +	.probe	= exynos_video_phy_probe,

you *must* provide a remove method. drivers with NULL remove are
non-removable :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 3/5] video: exynos_mipi_dsim: Use generic PHY driver
       [not found]   ` <1372170110-12993-3-git-send-email-s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2013-06-25 15:08     ` Felipe Balbi
  0 siblings, 0 replies; 24+ messages in thread
From: Felipe Balbi @ 2013-06-25 15:08 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	jg1.han-Sze3O3UU22JBDgjK7y7TUQ, dh09.lee-Sze3O3UU22JBDgjK7y7TUQ,
	balbi-l0cyMroinI0, kishon-l0cyMroinI0,
	inki.dae-Sze3O3UU22JBDgjK7y7TUQ,
	kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ,
	kgene.kim-Sze3O3UU22JBDgjK7y7TUQ,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-media-u79uwXL29TY76Z2rM5mHXA


[-- Attachment #1.1: Type: text/plain, Size: 558 bytes --]

Hi,

On Tue, Jun 25, 2013 at 04:21:48PM +0200, Sylwester Nawrocki wrote:
> Use the generic PHY API instead of the platform callback to control
> the MIPI DSIM DPHY. The 'phy_label' field is added to the platform
> data structure to allow PHY lookup on non-dt platforms.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

this is awesome :-)

Acked-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>

-- 
balbi

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 4/5] exynos4-is: Use generic MIPI CSIS PHY driver
  2013-06-25 14:21 ` [PATCH v2 4/5] exynos4-is: Use generic MIPI CSIS " Sylwester Nawrocki
@ 2013-06-25 15:09   ` Felipe Balbi
  0 siblings, 0 replies; 24+ messages in thread
From: Felipe Balbi @ 2013-06-25 15:09 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: linux-arm-kernel, linux-samsung-soc, kishon, linux-media,
	kyungmin.park, balbi, t.figa, devicetree-discuss, kgene.kim,
	dh09.lee, jg1.han, inki.dae, plagnioj, linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 449 bytes --]

On Tue, Jun 25, 2013 at 04:21:49PM +0200, Sylwester Nawrocki wrote:
> Use the generic PHY API instead of the platform callback to control
> the MIPI CSIS DPHY. The 'phy_label' field is added to the platform
> data structure to allow PHY lookup on non-dt platforms
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Acked-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 5/5] ARM: Samsung: Remove MIPI PHY setup code
  2013-06-25 14:21 ` [PATCH v2 5/5] ARM: Samsung: Remove MIPI PHY setup code Sylwester Nawrocki
@ 2013-06-25 15:09   ` Felipe Balbi
  0 siblings, 0 replies; 24+ messages in thread
From: Felipe Balbi @ 2013-06-25 15:09 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: linux-arm-kernel, linux-samsung-soc, kishon, linux-media,
	kyungmin.park, balbi, t.figa, devicetree-discuss, kgene.kim,
	dh09.lee, jg1.han, inki.dae, plagnioj, linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

On Tue, Jun 25, 2013 at 04:21:50PM +0200, Sylwester Nawrocki wrote:
> Generic PHY drivers are used to handle the MIPI CSIS and MIPI DSIM
> DPHYs so we can remove now unused code at arch/arm/plat-samsung.
> In case there is any board file for S5PV210 platforms using MIPI
> CSIS/DSIM (not any upstream currently) it should use the generic
> PHY API to bind the PHYs to respective PHY consumer drivers.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

pretty cool stuff

Acked-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-25 15:06 ` [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs Felipe Balbi
@ 2013-06-25 17:44   ` Sylwester Nawrocki
  2013-06-25 18:05     ` Tomasz Figa
  2013-06-25 20:54     ` Felipe Balbi
  2013-06-26 15:00   ` Sylwester Nawrocki
  1 sibling, 2 replies; 24+ messages in thread
From: Sylwester Nawrocki @ 2013-06-25 17:44 UTC (permalink / raw)
  To: balbi
  Cc: linux-arm-kernel, linux-samsung-soc, kishon, linux-media,
	kyungmin.park, t.figa, devicetree-discuss, kgene.kim, dh09.lee,
	jg1.han, inki.dae, plagnioj, linux-fbdev

Hi Felipe,

Thanks for the review.

On 06/25/2013 05:06 PM, Felipe Balbi wrote:
> On Tue, Jun 25, 2013 at 04:21:46PM +0200, Sylwester Nawrocki wrote:
>> +enum phy_id {
>> +	PHY_CSIS0,
>> +	PHY_DSIM0,
>> +	PHY_CSIS1,
>> +	PHY_DSIM1,
>> +	NUM_PHYS
> 
> please prepend these with EXYNOS_PHY_ or EXYNOS_MIPI_PHY_

OK, will fix that.

>> +struct exynos_video_phy {
>> +	spinlock_t slock;
>> +	struct phy *phys[NUM_PHYS];
> 
> more than one phy ? This means you should instantiate driver multiple
> drivers. Each phy id should call probe again.

Why ? This single PHY _provider_ can well handle multiple PHYs.
I don't see a good reason to further complicate this driver like
this. Please note that MIPI-CSIS 0 and MIPI DSIM 0 share MMIO
register, so does MIPI CSIS 1 and MIPI DSIM 1. There are only 2
registers for those 4 PHYs. I could have the involved object
multiplied, but it would have been just a waste of resources
with no difference to the PHY consumers.

>> +static int __set_phy_state(struct exynos_video_phy *state,
>> +				enum phy_id id, unsigned int on)
>> +{
>> +	void __iomem *addr;
>> +	unsigned long flags;
>> +	u32 reg, reset;
>> +
>> +	if (WARN_ON(id > NUM_PHYS))
>> +		return -EINVAL;
> 
> you don't want to do this, actually. It'll bug you everytime you want to
> add another phy ID :-)

If there is an SoC with more PHYs enum phy_id would be extended, and this
part would not need to be touched. OTOH @id cannot be normally greater
than NUM_PHYS. I think I'll drop that then.

>> +	addr = state->regs + EXYNOS_MIPI_PHY_CONTROL(id / 2);
>> +
>> +	if (id == PHY_DSIM0 || id == PHY_DSIM1)
>> +		reset = EXYNOS_MIPI_PHY_MRESETN;
>> +	else
>> +		reset = EXYNOS_MIPI_PHY_SRESETN;
>> +
>> +	spin_lock_irqsave(&state->slock, flags);
>> +	reg = readl(addr);
>> +	if (on)
>> +		reg |= reset;
>> +	else
>> +		reg &= ~reset;
>> +	writel(reg, addr);
>> +
>> +	/* Clear ENABLE bit only if MRESETN, SRESETN bits are not set. */
>> +	if (on)
>> +		reg |= EXYNOS_MIPI_PHY_ENABLE;
>> +	else if (!(reg & EXYNOS_MIPI_PHY_RESET_MASK))
>> +		reg &= ~EXYNOS_MIPI_PHY_ENABLE;
>> +
>> +	writel(reg, addr);
>> +	spin_unlock_irqrestore(&state->slock, flags);
>> +
>> +	pr_debug("%s(): id: %d, on: %d, addr: %#x, base: %#x\n",
>> +		 __func__, id, on, (u32)addr, (u32)state->regs);
> 
> use dev_dbg() instead.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int exynos_video_phy_power_on(struct phy *phy)
>> +{
>> +	struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
> 
> looks like we should have phy_get_drvdata() helper :-) Kishon ?

Indeed, that might be useful.

>> +static struct phy *exynos_video_phy_xlate(struct device *dev,
>> +					struct of_phandle_args *args)
>> +{
>> +	struct exynos_video_phy *state = dev_get_drvdata(dev);
>> +
>> +	if (WARN_ON(args->args[0] > NUM_PHYS))
>> +		return NULL;
> 
> please remove this check.

args->args[0] comes from DT as the PHY id and there is nothing
preventing it from being greater or equal to the state->phys[]
array length, unless I'm missing something. Actually it should
have been 'if (args->args[0] >= NUM_PHYS)'.

>> +	return state->phys[args->args[0]];
> 
> and your xlate is 'wrong'.

What exactly is wrong here ?

>> +}
>> +
>> +static struct phy_ops exynos_video_phy_ops = {
>> +	.power_on	= exynos_video_phy_power_on,
>> +	.power_off	= exynos_video_phy_power_off,
>> +	.owner		= THIS_MODULE,
>> +};
>> +
>> +static int exynos_video_phy_probe(struct platform_device *pdev)
>> +{
>> +	struct exynos_video_phy *state;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	struct phy_provider *phy_provider;
>> +	int i;
>> +
>> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
>> +	if (!state)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> +	state->regs = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(state->regs))
>> +		return PTR_ERR(state->regs);
>> +
>> +	dev_set_drvdata(dev, state);
> 
> you can use platform_set_drvdata(pdev, state);

I had it in the previous version, but changed for symmetry with
dev_set_drvdata(). I guess those could be replaced with
phy_{get, set}_drvdata as you suggested.

> same thing though, no strong feelings.
> 
>> +	phy_provider = devm_of_phy_provider_register(dev,
>> +					exynos_video_phy_xlate);
>> +	if (IS_ERR(phy_provider))
>> +		return PTR_ERR(phy_provider);
>> +
>> +	for (i = 0; i < NUM_PHYS; i++) {
>> +		char label[8];
>> +
>> +		snprintf(label, sizeof(label), "%s.%d",
>> +				i == PHY_DSIM0 || i == PHY_DSIM1 ?
>> +				"dsim" : "csis", i / 2);
>> +
>> +		state->phys[i] = devm_phy_create(dev, i, &exynos_video_phy_ops,
>> +								label, state);
>> +		if (IS_ERR(state->phys[i])) {
>> +			dev_err(dev, "failed to create PHY %s\n", label);
>> +			return PTR_ERR(state->phys[i]);
>> +		}
>> +	}
> 
> this really doesn't look correct to me. It looks like you have multiple
> PHYs, one for each ID. So your probe should be called for each PHY ID
> and you have several phy_providers too.

Yes, multiple PHY objects, but a single provider. There is no need
whatsoever for multiple PHY providers.

>> +static const struct of_device_id exynos_video_phy_of_match[] = {
>> +	{ .compatible = "samsung,s5pv210-mipi-video-phy" },
> 
> and this should contain all PHY IDs:
> 
> 	{ .compatible = "samsung,s5pv210-mipi-video-dsim0-phy",
> 		.data = (const void *) DSIM0, },
> 	{ .compatible = "samsung,s5pv210-mipi-video-dsim1-phy",
> 		.data = (const void *) DSIM1, },
> 	{ .compatible = "samsung,s5pv210-mipi-video-csi0-phy"
> 		.data = (const void *) CSI0, },
> 	{ .compatible = "samsung,s5pv210-mipi-video-csi1-phy"
> 		.data = (const void *) CSI1, },
> 
> then on your probe you can fetch that data field and use it as phy->id.

This looks wrong to me, it doesn't look like a right usage of 'compatible'
property. MIPI-CSIS0/MIPI-DSIM0, MIPI-CSIS1/MIPI-DSIM1 are identical pairs,
so one compatible property would need to be used for them. We don't use
different compatible strings for different instances of same device.
And MIPI DSIM and MIPI CSIS share one MMIO register, so they need to be
handled by one provider, to synchronize accesses. That's one of the main
reasons I turned to the generic PHY framework for those devices.

>> +static struct platform_driver exynos_video_phy_driver = {
>> +	.probe	= exynos_video_phy_probe,
> 
> you *must* provide a remove method. drivers with NULL remove are
> non-removable :-)

Oops, my bad. I've forgotten to update this, after enabling build
as module. Will update and test that. It will be an empty callback
though.


Thanks,
Sylwester

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-25 17:44   ` Sylwester Nawrocki
@ 2013-06-25 18:05     ` Tomasz Figa
  2013-06-25 20:54     ` Felipe Balbi
  1 sibling, 0 replies; 24+ messages in thread
From: Tomasz Figa @ 2013-06-25 18:05 UTC (permalink / raw)
  To: Sylwester Nawrocki, balbi
  Cc: linux-arm-kernel, linux-samsung-soc, kishon, linux-media,
	kyungmin.park, t.figa, devicetree-discuss, kgene.kim, dh09.lee,
	jg1.han, inki.dae, plagnioj, linux-fbdev

Hi Sylwester, Felipe,

On Tuesday 25 of June 2013 19:44:52 Sylwester Nawrocki wrote:
> Hi Felipe,
> 
> Thanks for the review.
> 
> On 06/25/2013 05:06 PM, Felipe Balbi wrote:
> > On Tue, Jun 25, 2013 at 04:21:46PM +0200, Sylwester Nawrocki wrote:
> >> +enum phy_id {
> >> +	PHY_CSIS0,
> >> +	PHY_DSIM0,
> >> +	PHY_CSIS1,
> >> +	PHY_DSIM1,
> >> +	NUM_PHYS
> > 
> > please prepend these with EXYNOS_PHY_ or EXYNOS_MIPI_PHY_
> 
> OK, will fix that.
> 
> >> +struct exynos_video_phy {
> >> +	spinlock_t slock;
> >> +	struct phy *phys[NUM_PHYS];
> > 
> > more than one phy ? This means you should instantiate driver multiple
> > drivers. Each phy id should call probe again.
> 
> Why ? This single PHY _provider_ can well handle multiple PHYs.
> I don't see a good reason to further complicate this driver like
> this. Please note that MIPI-CSIS 0 and MIPI DSIM 0 share MMIO
> register, so does MIPI CSIS 1 and MIPI DSIM 1. There are only 2
> registers for those 4 PHYs. I could have the involved object
> multiplied, but it would have been just a waste of resources
> with no difference to the PHY consumers.

IMHO one driver instance should represent one instance of IP block. Since 
this is a single IP block containing multiple PHYs with shared control 
interface, I think Sylwester did the right thing.

[snip]
> >> +static struct phy *exynos_video_phy_xlate(struct device *dev,
> >> +					struct of_phandle_args *args)
> >> +{
> >> +	struct exynos_video_phy *state = dev_get_drvdata(dev);
> >> +
> >> +	if (WARN_ON(args->args[0] > NUM_PHYS))
> >> +		return NULL;
> > 
> > please remove this check.
> 
> args->args[0] comes from DT as the PHY id and there is nothing
> preventing it from being greater or equal to the state->phys[]
> array length, unless I'm missing something. Actually it should
> have been 'if (args->args[0] >= NUM_PHYS)'.

The xlate() callback gets directly whatever parsed from device tree, so it 
is possible for an out of range value to get here and so this check is 
valid. However I think it should rather return an ERR_PTR, not NULL. See 
of_phy_get().

> >> +	return state->phys[args->args[0]];
> > 
> > and your xlate is 'wrong'.
> 
> What exactly is wrong here ?

Felipe, could you elaborate a bit more on this? I can't find any serious 
problems with this code.

[snip]
> >> +	phy_provider = devm_of_phy_provider_register(dev,
> >> +					exynos_video_phy_xlate);
> >> +	if (IS_ERR(phy_provider))
> >> +		return PTR_ERR(phy_provider);
> >> +
> >> +	for (i = 0; i < NUM_PHYS; i++) {
> >> +		char label[8];
> >> +
> >> +		snprintf(label, sizeof(label), "%s.%d",
> >> +				i == PHY_DSIM0 || i == PHY_DSIM1 ?
> >> +				"dsim" : "csis", i / 2);
> >> +
> >> +		state->phys[i] = devm_phy_create(dev, i, 
&exynos_video_phy_ops,
> >> +								label, 
state);
> >> +		if (IS_ERR(state->phys[i])) {
> >> +			dev_err(dev, "failed to create PHY %s\n", label);
> >> +			return PTR_ERR(state->phys[i]);
> >> +		}
> >> +	}
> > 
> > this really doesn't look correct to me. It looks like you have
> > multiple
> > PHYs, one for each ID. So your probe should be called for each PHY ID
> > and you have several phy_providers too.
> 
> Yes, multiple PHY objects, but a single provider. There is no need
> whatsoever for multiple PHY providers.

The whole concept of whatever-provider is to allow managing multiple 
objects by one parent object, like one clock provider for the whole clock 
controller, one interrupt controller object for all interrupts of an 
interrupt controller block, etc.

This is why a phandle has args, to allow addressing subobjects inside a 
provider.

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-25 17:44   ` Sylwester Nawrocki
  2013-06-25 18:05     ` Tomasz Figa
@ 2013-06-25 20:54     ` Felipe Balbi
  2013-06-25 21:47       ` Sylwester Nawrocki
  2013-06-26 11:21       ` Kishon Vijay Abraham I
  1 sibling, 2 replies; 24+ messages in thread
From: Felipe Balbi @ 2013-06-25 20:54 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: balbi, linux-arm-kernel, linux-samsung-soc, kishon, linux-media,
	kyungmin.park, t.figa, devicetree-discuss, kgene.kim, dh09.lee,
	jg1.han, inki.dae, plagnioj, linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 3653 bytes --]

Hi,

On Tue, Jun 25, 2013 at 07:44:52PM +0200, Sylwester Nawrocki wrote:
> >> +struct exynos_video_phy {
> >> +	spinlock_t slock;
> >> +	struct phy *phys[NUM_PHYS];
> > 
> > more than one phy ? This means you should instantiate driver multiple
> > drivers. Each phy id should call probe again.
> 
> Why ? This single PHY _provider_ can well handle multiple PHYs.
> I don't see a good reason to further complicate this driver like
> this. Please note that MIPI-CSIS 0 and MIPI DSIM 0 share MMIO
> register, so does MIPI CSIS 1 and MIPI DSIM 1. There are only 2
> registers for those 4 PHYs. I could have the involved object
> multiplied, but it would have been just a waste of resources
> with no difference to the PHY consumers.

alright, I misunderstood your code then. When I looked over your id
usage I missed the "/2" part and assumed that you would have separate
EXYNOS_MIPI_PHY_CONTROL() register for each ;-)

My bad, you can disregard the other comments.

> >> +static int exynos_video_phy_probe(struct platform_device *pdev)
> >> +{
> >> +	struct exynos_video_phy *state;
> >> +	struct device *dev = &pdev->dev;
> >> +	struct resource *res;
> >> +	struct phy_provider *phy_provider;
> >> +	int i;
> >> +
> >> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> >> +	if (!state)
> >> +		return -ENOMEM;
> >> +
> >> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +
> >> +	state->regs = devm_ioremap_resource(dev, res);
> >> +	if (IS_ERR(state->regs))
> >> +		return PTR_ERR(state->regs);
> >> +
> >> +	dev_set_drvdata(dev, state);
> > 
> > you can use platform_set_drvdata(pdev, state);
> 
> I had it in the previous version, but changed for symmetry with
> dev_set_drvdata(). I guess those could be replaced with
> phy_{get, set}_drvdata as you suggested.

hmm, you do need to set the drvdata() to the phy object, but also to the
pdev object (should you need it on a suspend/resume callback, for
instance). Those are separate struct device instances.

> >> +static const struct of_device_id exynos_video_phy_of_match[] = {
> >> +	{ .compatible = "samsung,s5pv210-mipi-video-phy" },
> > 
> > and this should contain all PHY IDs:
> > 
> > 	{ .compatible = "samsung,s5pv210-mipi-video-dsim0-phy",
> > 		.data = (const void *) DSIM0, },
> > 	{ .compatible = "samsung,s5pv210-mipi-video-dsim1-phy",
> > 		.data = (const void *) DSIM1, },
> > 	{ .compatible = "samsung,s5pv210-mipi-video-csi0-phy"
> > 		.data = (const void *) CSI0, },
> > 	{ .compatible = "samsung,s5pv210-mipi-video-csi1-phy"
> > 		.data = (const void *) CSI1, },
> > 
> > then on your probe you can fetch that data field and use it as phy->id.
> 
> This looks wrong to me, it doesn't look like a right usage of 'compatible'
> property. MIPI-CSIS0/MIPI-DSIM0, MIPI-CSIS1/MIPI-DSIM1 are identical pairs,
> so one compatible property would need to be used for them. We don't use
> different compatible strings for different instances of same device.
> And MIPI DSIM and MIPI CSIS share one MMIO register, so they need to be
> handled by one provider, to synchronize accesses. That's one of the main
> reasons I turned to the generic PHY framework for those devices.

understood :-)

> >> +static struct platform_driver exynos_video_phy_driver = {
> >> +	.probe	= exynos_video_phy_probe,
> > 
> > you *must* provide a remove method. drivers with NULL remove are
> > non-removable :-)
> 
> Oops, my bad. I've forgotten to update this, after enabling build
> as module. Will update and test that. It will be an empty callback
> though.

right, because of devm.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-25 20:54     ` Felipe Balbi
@ 2013-06-25 21:47       ` Sylwester Nawrocki
  2013-06-26  5:23         ` Felipe Balbi
  2013-06-26 11:21       ` Kishon Vijay Abraham I
  1 sibling, 1 reply; 24+ messages in thread
From: Sylwester Nawrocki @ 2013-06-25 21:47 UTC (permalink / raw)
  To: balbi
  Cc: Sylwester Nawrocki, linux-arm-kernel, linux-samsung-soc, kishon,
	linux-media, kyungmin.park, t.figa, devicetree-discuss, kgene.kim,
	dh09.lee, jg1.han, inki.dae, plagnioj, linux-fbdev

Hi,

On 06/25/2013 10:54 PM, Felipe Balbi wrote:
>>>> +static int exynos_video_phy_probe(struct platform_device *pdev)
>>>> >  >>  +{
>>>> >  >>  +	struct exynos_video_phy *state;
>>>> >  >>  +	struct device *dev =&pdev->dev;
>>>> >  >>  +	struct resource *res;
>>>> >  >>  +	struct phy_provider *phy_provider;
>>>> >  >>  +	int i;
>>>> >  >>  +
>>>> >  >>  +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
>>>> >  >>  +	if (!state)
>>>> >  >>  +		return -ENOMEM;
>>>> >  >>  +
>>>> >  >>  +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> >  >>  +
>>>> >  >>  +	state->regs = devm_ioremap_resource(dev, res);
>>>> >  >>  +	if (IS_ERR(state->regs))
>>>> >  >>  +		return PTR_ERR(state->regs);
>>>> >  >>  +
>>>> >  >>  +	dev_set_drvdata(dev, state);
>>> >  >
>>> >  >  you can use platform_set_drvdata(pdev, state);
>> >
>> >  I had it in the previous version, but changed for symmetry with
>> >  dev_set_drvdata(). I guess those could be replaced with
>> >  phy_{get, set}_drvdata as you suggested.
>
> hmm, you do need to set the drvdata() to the phy object, but also to the
> pdev object (should you need it on a suspend/resume callback, for
> instance). Those are separate struct device instances.

Indeed, I somehow confused phy->dev with with phy->dev.parent. I'm going
to just drop the above call, since the pdev drvdata is currently not
referenced anywhere.


Thanks,
Sylwester

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-25 21:47       ` Sylwester Nawrocki
@ 2013-06-26  5:23         ` Felipe Balbi
  0 siblings, 0 replies; 24+ messages in thread
From: Felipe Balbi @ 2013-06-26  5:23 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: balbi, Sylwester Nawrocki, linux-arm-kernel, linux-samsung-soc,
	kishon, linux-media, kyungmin.park, t.figa, devicetree-discuss,
	kgene.kim, dh09.lee, jg1.han, inki.dae, plagnioj, linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

On Tue, Jun 25, 2013 at 11:47:13PM +0200, Sylwester Nawrocki wrote:
> Hi,
> 
> On 06/25/2013 10:54 PM, Felipe Balbi wrote:
> >>>>+static int exynos_video_phy_probe(struct platform_device *pdev)
> >>>>>  >>  +{
> >>>>>  >>  +	struct exynos_video_phy *state;
> >>>>>  >>  +	struct device *dev =&pdev->dev;
> >>>>>  >>  +	struct resource *res;
> >>>>>  >>  +	struct phy_provider *phy_provider;
> >>>>>  >>  +	int i;
> >>>>>  >>  +
> >>>>>  >>  +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> >>>>>  >>  +	if (!state)
> >>>>>  >>  +		return -ENOMEM;
> >>>>>  >>  +
> >>>>>  >>  +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>>>>  >>  +
> >>>>>  >>  +	state->regs = devm_ioremap_resource(dev, res);
> >>>>>  >>  +	if (IS_ERR(state->regs))
> >>>>>  >>  +		return PTR_ERR(state->regs);
> >>>>>  >>  +
> >>>>>  >>  +	dev_set_drvdata(dev, state);
> >>>>  >
> >>>>  >  you can use platform_set_drvdata(pdev, state);
> >>>
> >>>  I had it in the previous version, but changed for symmetry with
> >>>  dev_set_drvdata(). I guess those could be replaced with
> >>>  phy_{get, set}_drvdata as you suggested.
> >
> >hmm, you do need to set the drvdata() to the phy object, but also to the
> >pdev object (should you need it on a suspend/resume callback, for
> >instance). Those are separate struct device instances.
> 
> Indeed, I somehow confused phy->dev with with phy->dev.parent. I'm going
> to just drop the above call, since the pdev drvdata is currently not
> referenced anywhere.

ok cool :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-25 20:54     ` Felipe Balbi
  2013-06-25 21:47       ` Sylwester Nawrocki
@ 2013-06-26 11:21       ` Kishon Vijay Abraham I
  2013-06-26 12:03         ` Sylwester Nawrocki
  1 sibling, 1 reply; 24+ messages in thread
From: Kishon Vijay Abraham I @ 2013-06-26 11:21 UTC (permalink / raw)
  To: balbi
  Cc: Sylwester Nawrocki, linux-arm-kernel, linux-samsung-soc,
	linux-media, kyungmin.park, t.figa, devicetree-discuss, kgene.kim,
	dh09.lee, jg1.han, inki.dae, plagnioj, linux-fbdev

Hi,

On Wednesday 26 June 2013 02:24 AM, Felipe Balbi wrote:
> Hi,
>
> On Tue, Jun 25, 2013 at 07:44:52PM +0200, Sylwester Nawrocki wrote:
>>>> +struct exynos_video_phy {
>>>> +	spinlock_t slock;
>>>> +	struct phy *phys[NUM_PHYS];
>>>
>>> more than one phy ? This means you should instantiate driver multiple
>>> drivers. Each phy id should call probe again.
>>
>> Why ? This single PHY _provider_ can well handle multiple PHYs.
>> I don't see a good reason to further complicate this driver like
>> this. Please note that MIPI-CSIS 0 and MIPI DSIM 0 share MMIO
>> register, so does MIPI CSIS 1 and MIPI DSIM 1. There are only 2
>> registers for those 4 PHYs. I could have the involved object
>> multiplied, but it would have been just a waste of resources
>> with no difference to the PHY consumers.
>
> alright, I misunderstood your code then. When I looked over your id
> usage I missed the "/2" part and assumed that you would have separate
> EXYNOS_MIPI_PHY_CONTROL() register for each ;-)
>
> My bad, you can disregard the other comments.
>
>>>> +static int exynos_video_phy_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct exynos_video_phy *state;
>>>> +	struct device *dev = &pdev->dev;
>>>> +	struct resource *res;
>>>> +	struct phy_provider *phy_provider;
>>>> +	int i;
>>>> +
>>>> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
>>>> +	if (!state)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +
>>>> +	state->regs = devm_ioremap_resource(dev, res);
>>>> +	if (IS_ERR(state->regs))
>>>> +		return PTR_ERR(state->regs);
>>>> +
>>>> +	dev_set_drvdata(dev, state);
>>>
>>> you can use platform_set_drvdata(pdev, state);
>>
>> I had it in the previous version, but changed for symmetry with
>> dev_set_drvdata(). I guess those could be replaced with
>> phy_{get, set}_drvdata as you suggested.

right. currently I was setting dev_set_drvdata of phy (core) device
in phy-core.c and the corresponding dev_get_drvdata in phy provider driver
which is little confusing.
So I'll add phy_set_drvdata and phy_get_drvdata in phy.h (as suggested by
Felipe) to be used by phy provider drivers. So after creating the PHY, the
phy provider should use phy_set_drvdata and in phy_ops, it can use
phy_get_drvdata. (I'll remove the dev_set_drvdata in phy_create).

This also means _void *priv_ in phy_create is useless. So I'll be removing
_priv_ from phy_create.

Thanks
Kishon

>
> hmm, you do need to set the drvdata() to the phy object, but also to the
> pdev object (should you need it on a suspend/resume callback, for
> instance). Those are separate struct device instances.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-26 11:21       ` Kishon Vijay Abraham I
@ 2013-06-26 12:03         ` Sylwester Nawrocki
  2013-06-26 12:22           ` Felipe Balbi
  2013-06-26 12:48           ` Kishon Vijay Abraham I
  0 siblings, 2 replies; 24+ messages in thread
From: Sylwester Nawrocki @ 2013-06-26 12:03 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: balbi, linux-arm-kernel, linux-samsung-soc, linux-media,
	kyungmin.park, t.figa, devicetree-discuss, kgene.kim, dh09.lee,
	jg1.han, inki.dae, plagnioj, linux-fbdev

On 06/26/2013 01:21 PM, Kishon Vijay Abraham I wrote:
>>>>> +static int exynos_video_phy_probe(struct platform_device *pdev)
>>>>> >>>> +{
>>>>> >>>> +	struct exynos_video_phy *state;
>>>>> >>>> +	struct device *dev = &pdev->dev;
>>>>> >>>> +	struct resource *res;
>>>>> >>>> +	struct phy_provider *phy_provider;
>>>>> >>>> +	int i;
>>>>> >>>> +
>>>>> >>>> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
>>>>> >>>> +	if (!state)
>>>>> >>>> +		return -ENOMEM;
>>>>> >>>> +
>>>>> >>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> >>>> +
>>>>> >>>> +	state->regs = devm_ioremap_resource(dev, res);
>>>>> >>>> +	if (IS_ERR(state->regs))
>>>>> >>>> +		return PTR_ERR(state->regs);
>>>>> >>>> +
>>>>> >>>> +	dev_set_drvdata(dev, state);
>>>> >>>
>>>> >>> you can use platform_set_drvdata(pdev, state);
>>> >>
>>> >> I had it in the previous version, but changed for symmetry with
>>> >> dev_set_drvdata(). I guess those could be replaced with
>>> >> phy_{get, set}_drvdata as you suggested.
>
> right. currently I was setting dev_set_drvdata of phy (core) device
> in phy-core.c and the corresponding dev_get_drvdata in phy provider driver
> which is little confusing.
> So I'll add phy_set_drvdata and phy_get_drvdata in phy.h (as suggested by
> Felipe) to be used by phy provider drivers. So after creating the PHY, the
> phy provider should use phy_set_drvdata and in phy_ops, it can use
> phy_get_drvdata. (I'll remove the dev_set_drvdata in phy_create).
> 
> This also means _void *priv_ in phy_create is useless. So I'll be removing
> _priv_ from phy_create.

Yeah, sounds good. Then in the phy ops phy_get_drvdata(&phy->dev) would
be used and in a custom of_xlate dev_get_drvdata(dev) (assuming the phy
provider sets drvdata on its device beforehand).

I can't see any races from runtime PM with this approach.

Regards,
Sylwester

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-26 12:03         ` Sylwester Nawrocki
@ 2013-06-26 12:22           ` Felipe Balbi
  2013-06-26 12:48             ` Kishon Vijay Abraham I
  2013-06-26 12:48           ` Kishon Vijay Abraham I
  1 sibling, 1 reply; 24+ messages in thread
From: Felipe Balbi @ 2013-06-26 12:22 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Kishon Vijay Abraham I, balbi, linux-arm-kernel,
	linux-samsung-soc, linux-media, kyungmin.park, t.figa,
	devicetree-discuss, kgene.kim, dh09.lee, jg1.han, inki.dae,
	plagnioj, linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

On Wed, Jun 26, 2013 at 02:03:42PM +0200, Sylwester Nawrocki wrote:
> On 06/26/2013 01:21 PM, Kishon Vijay Abraham I wrote:
> >>>>> +static int exynos_video_phy_probe(struct platform_device *pdev)
> >>>>> >>>> +{
> >>>>> >>>> +	struct exynos_video_phy *state;
> >>>>> >>>> +	struct device *dev = &pdev->dev;
> >>>>> >>>> +	struct resource *res;
> >>>>> >>>> +	struct phy_provider *phy_provider;
> >>>>> >>>> +	int i;
> >>>>> >>>> +
> >>>>> >>>> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> >>>>> >>>> +	if (!state)
> >>>>> >>>> +		return -ENOMEM;
> >>>>> >>>> +
> >>>>> >>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>>>> >>>> +
> >>>>> >>>> +	state->regs = devm_ioremap_resource(dev, res);
> >>>>> >>>> +	if (IS_ERR(state->regs))
> >>>>> >>>> +		return PTR_ERR(state->regs);
> >>>>> >>>> +
> >>>>> >>>> +	dev_set_drvdata(dev, state);
> >>>> >>>
> >>>> >>> you can use platform_set_drvdata(pdev, state);
> >>> >>
> >>> >> I had it in the previous version, but changed for symmetry with
> >>> >> dev_set_drvdata(). I guess those could be replaced with
> >>> >> phy_{get, set}_drvdata as you suggested.
> >
> > right. currently I was setting dev_set_drvdata of phy (core) device
> > in phy-core.c and the corresponding dev_get_drvdata in phy provider driver
> > which is little confusing.
> > So I'll add phy_set_drvdata and phy_get_drvdata in phy.h (as suggested by
> > Felipe) to be used by phy provider drivers. So after creating the PHY, the
> > phy provider should use phy_set_drvdata and in phy_ops, it can use
> > phy_get_drvdata. (I'll remove the dev_set_drvdata in phy_create).
> > 
> > This also means _void *priv_ in phy_create is useless. So I'll be removing
> > _priv_ from phy_create.
> 
> Yeah, sounds good. Then in the phy ops phy_get_drvdata(&phy->dev) would

phy_get_drvdata(phy);

accessing the dev pointer will be done inside the helper :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-26 12:03         ` Sylwester Nawrocki
  2013-06-26 12:22           ` Felipe Balbi
@ 2013-06-26 12:48           ` Kishon Vijay Abraham I
  1 sibling, 0 replies; 24+ messages in thread
From: Kishon Vijay Abraham I @ 2013-06-26 12:48 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: balbi, linux-arm-kernel, linux-samsung-soc, linux-media,
	kyungmin.park, t.figa, devicetree-discuss, kgene.kim, dh09.lee,
	jg1.han, inki.dae, plagnioj, linux-fbdev

On Wednesday 26 June 2013 05:33 PM, Sylwester Nawrocki wrote:
> On 06/26/2013 01:21 PM, Kishon Vijay Abraham I wrote:
>>>>>> +static int exynos_video_phy_probe(struct platform_device *pdev)
>>>>>>>>>> +{
>>>>>>>>>> +	struct exynos_video_phy *state;
>>>>>>>>>> +	struct device *dev = &pdev->dev;
>>>>>>>>>> +	struct resource *res;
>>>>>>>>>> +	struct phy_provider *phy_provider;
>>>>>>>>>> +	int i;
>>>>>>>>>> +
>>>>>>>>>> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
>>>>>>>>>> +	if (!state)
>>>>>>>>>> +		return -ENOMEM;
>>>>>>>>>> +
>>>>>>>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>>>>>> +
>>>>>>>>>> +	state->regs = devm_ioremap_resource(dev, res);
>>>>>>>>>> +	if (IS_ERR(state->regs))
>>>>>>>>>> +		return PTR_ERR(state->regs);
>>>>>>>>>> +
>>>>>>>>>> +	dev_set_drvdata(dev, state);
>>>>>>>>
>>>>>>>> you can use platform_set_drvdata(pdev, state);
>>>>>>
>>>>>> I had it in the previous version, but changed for symmetry with
>>>>>> dev_set_drvdata(). I guess those could be replaced with
>>>>>> phy_{get, set}_drvdata as you suggested.
>>
>> right. currently I was setting dev_set_drvdata of phy (core) device
>> in phy-core.c and the corresponding dev_get_drvdata in phy provider driver
>> which is little confusing.
>> So I'll add phy_set_drvdata and phy_get_drvdata in phy.h (as suggested by
>> Felipe) to be used by phy provider drivers. So after creating the PHY, the
>> phy provider should use phy_set_drvdata and in phy_ops, it can use
>> phy_get_drvdata. (I'll remove the dev_set_drvdata in phy_create).
>>
>> This also means _void *priv_ in phy_create is useless. So I'll be removing
>> _priv_ from phy_create.
>
> Yeah, sounds good. Then in the phy ops phy_get_drvdata(&phy->dev) would
> be used and in a custom of_xlate dev_get_drvdata(dev) (assuming the phy
> provider sets drvdata on its device beforehand).

thats correct. btw when you send the next version just have MODULE_LICENSE set
to GPL v2. Apart from that this patch looks good to me.

Thanks
Kishon

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-26 12:22           ` Felipe Balbi
@ 2013-06-26 12:48             ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 24+ messages in thread
From: Kishon Vijay Abraham I @ 2013-06-26 12:48 UTC (permalink / raw)
  To: balbi
  Cc: Sylwester Nawrocki, linux-arm-kernel, linux-samsung-soc,
	linux-media, kyungmin.park, t.figa, devicetree-discuss, kgene.kim,
	dh09.lee, jg1.han, inki.dae, plagnioj, linux-fbdev

On Wednesday 26 June 2013 05:52 PM, Felipe Balbi wrote:
> On Wed, Jun 26, 2013 at 02:03:42PM +0200, Sylwester Nawrocki wrote:
>> On 06/26/2013 01:21 PM, Kishon Vijay Abraham I wrote:
>>>>>>> +static int exynos_video_phy_probe(struct platform_device *pdev)
>>>>>>>>>>> +{
>>>>>>>>>>> +	struct exynos_video_phy *state;
>>>>>>>>>>> +	struct device *dev = &pdev->dev;
>>>>>>>>>>> +	struct resource *res;
>>>>>>>>>>> +	struct phy_provider *phy_provider;
>>>>>>>>>>> +	int i;
>>>>>>>>>>> +
>>>>>>>>>>> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
>>>>>>>>>>> +	if (!state)
>>>>>>>>>>> +		return -ENOMEM;
>>>>>>>>>>> +
>>>>>>>>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>>>>>>> +
>>>>>>>>>>> +	state->regs = devm_ioremap_resource(dev, res);
>>>>>>>>>>> +	if (IS_ERR(state->regs))
>>>>>>>>>>> +		return PTR_ERR(state->regs);
>>>>>>>>>>> +
>>>>>>>>>>> +	dev_set_drvdata(dev, state);
>>>>>>>>>
>>>>>>>>> you can use platform_set_drvdata(pdev, state);
>>>>>>>
>>>>>>> I had it in the previous version, but changed for symmetry with
>>>>>>> dev_set_drvdata(). I guess those could be replaced with
>>>>>>> phy_{get, set}_drvdata as you suggested.
>>>
>>> right. currently I was setting dev_set_drvdata of phy (core) device
>>> in phy-core.c and the corresponding dev_get_drvdata in phy provider driver
>>> which is little confusing.
>>> So I'll add phy_set_drvdata and phy_get_drvdata in phy.h (as suggested by
>>> Felipe) to be used by phy provider drivers. So after creating the PHY, the
>>> phy provider should use phy_set_drvdata and in phy_ops, it can use
>>> phy_get_drvdata. (I'll remove the dev_set_drvdata in phy_create).
>>>
>>> This also means _void *priv_ in phy_create is useless. So I'll be removing
>>> _priv_ from phy_create.
>>
>> Yeah, sounds good. Then in the phy ops phy_get_drvdata(&phy->dev) would
>
> phy_get_drvdata(phy);
>
> accessing the dev pointer will be done inside the helper :-)

right :-)

-Kishon

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-25 15:06 ` [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs Felipe Balbi
  2013-06-25 17:44   ` Sylwester Nawrocki
@ 2013-06-26 15:00   ` Sylwester Nawrocki
  2013-06-27  6:17     ` Felipe Balbi
  1 sibling, 1 reply; 24+ messages in thread
From: Sylwester Nawrocki @ 2013-06-26 15:00 UTC (permalink / raw)
  To: balbi
  Cc: linux-arm-kernel, linux-samsung-soc, kishon, linux-media,
	kyungmin.park, t.figa, devicetree-discuss, kgene.kim, dh09.lee,
	jg1.han, inki.dae, plagnioj, linux-fbdev

Hi,

On 06/25/2013 05:06 PM, Felipe Balbi wrote:
>> +static struct platform_driver exynos_video_phy_driver = {
>> > +	.probe	= exynos_video_phy_probe,
>
> you *must* provide a remove method. drivers with NULL remove are
> non-removable :-)

Actually the remove() callback can be NULL, it's just missing module_exit
function that makes a module not unloadable.

--
Regards,
Sylwester

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-26 15:00   ` Sylwester Nawrocki
@ 2013-06-27  6:17     ` Felipe Balbi
  2013-06-27  7:47       ` Andrzej Hajda
  2013-06-27  8:49       ` Russell King - ARM Linux
  0 siblings, 2 replies; 24+ messages in thread
From: Felipe Balbi @ 2013-06-27  6:17 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: linux-fbdev, linux-samsung-soc, t.figa, jg1.han, dh09.lee, balbi,
	kishon, inki.dae, kyungmin.park, kgene.kim, plagnioj,
	devicetree-discuss, linux-arm-kernel, linux-media


[-- Attachment #1.1: Type: text/plain, Size: 991 bytes --]

On Wed, Jun 26, 2013 at 05:00:34PM +0200, Sylwester Nawrocki wrote:
> Hi,
> 
> On 06/25/2013 05:06 PM, Felipe Balbi wrote:
> >> +static struct platform_driver exynos_video_phy_driver = {
> >> > +	.probe	= exynos_video_phy_probe,
> >
> > you *must* provide a remove method. drivers with NULL remove are
> > non-removable :-)
> 
> Actually the remove() callback can be NULL, it's just missing module_exit
> function that makes a module not unloadable.

look at the implementation of platform_drv_remove():

 499 static int platform_drv_remove(struct device *_dev)
 500 {
 501         struct platform_driver *drv = to_platform_driver(_dev->driver);
 502         struct platform_device *dev = to_platform_device(_dev);
 503         int ret;
 504 
 505         ret = drv->remove(dev);
 506         if (ACPI_HANDLE(_dev))
 507                 acpi_dev_pm_detach(_dev, true);
 508 
 509         return ret;
 510 }

that's not a conditional call right :-)

-- 
balbi

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-27  6:17     ` Felipe Balbi
@ 2013-06-27  7:47       ` Andrzej Hajda
  2013-06-27  7:51         ` Felipe Balbi
  2013-06-27  8:49       ` Russell King - ARM Linux
  1 sibling, 1 reply; 24+ messages in thread
From: Andrzej Hajda @ 2013-06-27  7:47 UTC (permalink / raw)
  To: balbi
  Cc: Sylwester Nawrocki, linux-fbdev, linux-samsung-soc, t.figa,
	jg1.han, dh09.lee, kishon, inki.dae, kyungmin.park, kgene.kim,
	plagnioj, devicetree-discuss, linux-arm-kernel, linux-media

Hi Felipe,

On 06/27/2013 08:17 AM, Felipe Balbi wrote:
> On Wed, Jun 26, 2013 at 05:00:34PM +0200, Sylwester Nawrocki wrote:
>> Hi,
>>
>> On 06/25/2013 05:06 PM, Felipe Balbi wrote:
>>>> +static struct platform_driver exynos_video_phy_driver = {
>>>>> +	.probe	= exynos_video_phy_probe,
>>>
>>> you *must* provide a remove method. drivers with NULL remove are
>>> non-removable :-)
>>
>> Actually the remove() callback can be NULL, it's just missing module_exit
>> function that makes a module not unloadable.
> 
> look at the implementation of platform_drv_remove():
> 
>  499 static int platform_drv_remove(struct device *_dev)
>  500 {
>  501         struct platform_driver *drv = to_platform_driver(_dev->driver);
>  502         struct platform_device *dev = to_platform_device(_dev);
>  503         int ret;
>  504 
>  505         ret = drv->remove(dev);
>  506         if (ACPI_HANDLE(_dev))
>  507                 acpi_dev_pm_detach(_dev, true);
>  508 
>  509         return ret;
>  510 }
> 
> that's not a conditional call right :-)

It is conditional, just condition check is in different place:

int platform_driver_register(struct platform_driver *drv)
{
	(...)
	if (drv->remove)
		drv->driver.remove = platform_drv_remove;
	(...)
}


Regards
Andrzej

> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-27  7:47       ` Andrzej Hajda
@ 2013-06-27  7:51         ` Felipe Balbi
  0 siblings, 0 replies; 24+ messages in thread
From: Felipe Balbi @ 2013-06-27  7:51 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: balbi, Sylwester Nawrocki, linux-fbdev, linux-samsung-soc, t.figa,
	jg1.han, dh09.lee, kishon, inki.dae, kyungmin.park, kgene.kim,
	plagnioj, devicetree-discuss, linux-arm-kernel, linux-media

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

On Thu, Jun 27, 2013 at 09:47:47AM +0200, Andrzej Hajda wrote:
> Hi Felipe,
> 
> On 06/27/2013 08:17 AM, Felipe Balbi wrote:
> > On Wed, Jun 26, 2013 at 05:00:34PM +0200, Sylwester Nawrocki wrote:
> >> Hi,
> >>
> >> On 06/25/2013 05:06 PM, Felipe Balbi wrote:
> >>>> +static struct platform_driver exynos_video_phy_driver = {
> >>>>> +	.probe	= exynos_video_phy_probe,
> >>>
> >>> you *must* provide a remove method. drivers with NULL remove are
> >>> non-removable :-)
> >>
> >> Actually the remove() callback can be NULL, it's just missing module_exit
> >> function that makes a module not unloadable.
> > 
> > look at the implementation of platform_drv_remove():
> > 
> >  499 static int platform_drv_remove(struct device *_dev)
> >  500 {
> >  501         struct platform_driver *drv = to_platform_driver(_dev->driver);
> >  502         struct platform_device *dev = to_platform_device(_dev);
> >  503         int ret;
> >  504 
> >  505         ret = drv->remove(dev);
> >  506         if (ACPI_HANDLE(_dev))
> >  507                 acpi_dev_pm_detach(_dev, true);
> >  508 
> >  509         return ret;
> >  510 }
> > 
> > that's not a conditional call right :-)
> 
> It is conditional, just condition check is in different place:
> 
> int platform_driver_register(struct platform_driver *drv)
> {
> 	(...)
> 	if (drv->remove)
> 		drv->driver.remove = platform_drv_remove;
> 	(...)
> }

good point :-) thanks. I'll go ack your driver now

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
  2013-06-27  6:17     ` Felipe Balbi
  2013-06-27  7:47       ` Andrzej Hajda
@ 2013-06-27  8:49       ` Russell King - ARM Linux
  1 sibling, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2013-06-27  8:49 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Sylwester Nawrocki, linux-fbdev, linux-samsung-soc, t.figa,
	jg1.han, dh09.lee, kishon, inki.dae, kyungmin.park, kgene.kim,
	plagnioj, devicetree-discuss, linux-arm-kernel, linux-media

On Thu, Jun 27, 2013 at 09:17:13AM +0300, Felipe Balbi wrote:
> On Wed, Jun 26, 2013 at 05:00:34PM +0200, Sylwester Nawrocki wrote:
> > Hi,
> > 
> > On 06/25/2013 05:06 PM, Felipe Balbi wrote:
> > >> +static struct platform_driver exynos_video_phy_driver = {
> > >> > +	.probe	= exynos_video_phy_probe,
> > >
> > > you *must* provide a remove method. drivers with NULL remove are
> > > non-removable :-)
> > 
> > Actually the remove() callback can be NULL, it's just missing module_exit
> > function that makes a module not unloadable.
> 
> look at the implementation of platform_drv_remove():
> 
>  499 static int platform_drv_remove(struct device *_dev)
>  500 {
>  501         struct platform_driver *drv = to_platform_driver(_dev->driver);
>  502         struct platform_device *dev = to_platform_device(_dev);
>  503         int ret;
>  504 
>  505         ret = drv->remove(dev);
>  506         if (ACPI_HANDLE(_dev))
>  507                 acpi_dev_pm_detach(_dev, true);
>  508 
>  509         return ret;
>  510 }
> 
> that's not a conditional call right :-)

Wrong.

        if (drv->remove)
                drv->driver.remove = platform_drv_remove;

The function you quote will only be used if drv->remove is non-NULL.
You do not need to provide a remove method.

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2013-06-27  8:49 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-25 14:21 [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs Sylwester Nawrocki
2013-06-25 14:21 ` [PATCH v2 2/5] ARM: dts: Add MIPI PHY node to exynos4.dtsi Sylwester Nawrocki
2013-06-25 14:21 ` [PATCH v2 3/5] video: exynos_mipi_dsim: Use generic PHY driver Sylwester Nawrocki
     [not found]   ` <1372170110-12993-3-git-send-email-s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-06-25 15:08     ` Felipe Balbi
2013-06-25 14:21 ` [PATCH v2 4/5] exynos4-is: Use generic MIPI CSIS " Sylwester Nawrocki
2013-06-25 15:09   ` Felipe Balbi
2013-06-25 14:21 ` [PATCH v2 5/5] ARM: Samsung: Remove MIPI PHY setup code Sylwester Nawrocki
2013-06-25 15:09   ` Felipe Balbi
2013-06-25 15:06 ` [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs Felipe Balbi
2013-06-25 17:44   ` Sylwester Nawrocki
2013-06-25 18:05     ` Tomasz Figa
2013-06-25 20:54     ` Felipe Balbi
2013-06-25 21:47       ` Sylwester Nawrocki
2013-06-26  5:23         ` Felipe Balbi
2013-06-26 11:21       ` Kishon Vijay Abraham I
2013-06-26 12:03         ` Sylwester Nawrocki
2013-06-26 12:22           ` Felipe Balbi
2013-06-26 12:48             ` Kishon Vijay Abraham I
2013-06-26 12:48           ` Kishon Vijay Abraham I
2013-06-26 15:00   ` Sylwester Nawrocki
2013-06-27  6:17     ` Felipe Balbi
2013-06-27  7:47       ` Andrzej Hajda
2013-06-27  7:51         ` Felipe Balbi
2013-06-27  8:49       ` Russell King - ARM Linux

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