* [PATCH v4 00/10] Device tree support for Exynos SoC camera subsystem
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-media
Cc: kyungmin.park, kgene.kim, swarren, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel,
Sylwester Nawrocki
This is an updated patch series adding initial support for the Exynos4
SoC series camera subsystem. It is based on the video interfaces common
device tree bindings and depends on patch series [1]. The full source
tree can be browsed at [2].
I've dropped the RFC tag from this series and I'm going to continue with
adding support for asynchronous sensor subdevs registration and the
FIMC-IS on top of it. Unless there are further changes in the bindings.
Still, any comments are welcome.
The changes are listed in each patch, if any.
This series likely won't make it to v3.9. It would be nice, but there
seems not to be much interest in getting [1] in the mainline and the
reviews are relatively slow.
Any Ack/nack from the device tree maintainers are welcome, so I could
finally have this patch set merged and carry on with other, even more
interesting things. :)
Thanks,
Sylwester
[1] http://www.spinics.net/lists/linux-media/msg59471.html
[2] http://git.linuxtv.org/snawrocki/samsung.git/exynos_fimc
Sylwester Nawrocki (10):
s5p-csis: Add device tree support
s5p-fimc: Add device tree support for FIMC devices
s5p-fimc: Add device tree support for FIMC-LITE devices
s5p-fimc: Add device tree support for the main media device driver
s5p-fimc: Add device tree based sensors registration
s5p-fimc: Use pinctrl API for camera ports configuration
ARM: dts: Add camera to node exynos4.dtsi
ARM: dts: Add ISP power domain node for Exynos4x12
ARM: dts: Add FIMC and MIPI CSIS device nodes for Exynos4x12
ARM: dts: Correct camera pinctrl nodes for Exynos4x12 SoCs
.../devicetree/bindings/media/soc/samsung-fimc.txt | 183 +++++++++++
.../bindings/media/soc/samsung-mipi-csis.txt | 82 +++++
arch/arm/boot/dts/exynos4.dtsi | 64 ++++
arch/arm/boot/dts/exynos4x12-pinctrl.dtsi | 26 +-
arch/arm/boot/dts/exynos4x12.dtsi | 52 ++++
drivers/media/platform/s5p-fimc/fimc-capture.c | 2 +-
drivers/media/platform/s5p-fimc/fimc-core.c | 94 ++++--
drivers/media/platform/s5p-fimc/fimc-lite.c | 65 ++--
drivers/media/platform/s5p-fimc/fimc-mdevice.c | 319 ++++++++++++++++++--
drivers/media/platform/s5p-fimc/fimc-mdevice.h | 4 +
drivers/media/platform/s5p-fimc/mipi-csis.c | 154 +++++++---
drivers/media/platform/s5p-fimc/mipi-csis.h | 1 +
include/media/s5p_fimc.h | 17 ++
13 files changed, 936 insertions(+), 127 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
--
1.7.9.5
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 00/10] Device tree support for Exynos SoC camera subsystem
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-arm-kernel
This is an updated patch series adding initial support for the Exynos4
SoC series camera subsystem. It is based on the video interfaces common
device tree bindings and depends on patch series [1]. The full source
tree can be browsed at [2].
I've dropped the RFC tag from this series and I'm going to continue with
adding support for asynchronous sensor subdevs registration and the
FIMC-IS on top of it. Unless there are further changes in the bindings.
Still, any comments are welcome.
The changes are listed in each patch, if any.
This series likely won't make it to v3.9. It would be nice, but there
seems not to be much interest in getting [1] in the mainline and the
reviews are relatively slow.
Any Ack/nack from the device tree maintainers are welcome, so I could
finally have this patch set merged and carry on with other, even more
interesting things. :)
Thanks,
Sylwester
[1] http://www.spinics.net/lists/linux-media/msg59471.html
[2] http://git.linuxtv.org/snawrocki/samsung.git/exynos_fimc
Sylwester Nawrocki (10):
s5p-csis: Add device tree support
s5p-fimc: Add device tree support for FIMC devices
s5p-fimc: Add device tree support for FIMC-LITE devices
s5p-fimc: Add device tree support for the main media device driver
s5p-fimc: Add device tree based sensors registration
s5p-fimc: Use pinctrl API for camera ports configuration
ARM: dts: Add camera to node exynos4.dtsi
ARM: dts: Add ISP power domain node for Exynos4x12
ARM: dts: Add FIMC and MIPI CSIS device nodes for Exynos4x12
ARM: dts: Correct camera pinctrl nodes for Exynos4x12 SoCs
.../devicetree/bindings/media/soc/samsung-fimc.txt | 183 +++++++++++
.../bindings/media/soc/samsung-mipi-csis.txt | 82 +++++
arch/arm/boot/dts/exynos4.dtsi | 64 ++++
arch/arm/boot/dts/exynos4x12-pinctrl.dtsi | 26 +-
arch/arm/boot/dts/exynos4x12.dtsi | 52 ++++
drivers/media/platform/s5p-fimc/fimc-capture.c | 2 +-
drivers/media/platform/s5p-fimc/fimc-core.c | 94 ++++--
drivers/media/platform/s5p-fimc/fimc-lite.c | 65 ++--
drivers/media/platform/s5p-fimc/fimc-mdevice.c | 319 ++++++++++++++++++--
drivers/media/platform/s5p-fimc/fimc-mdevice.h | 4 +
drivers/media/platform/s5p-fimc/mipi-csis.c | 154 +++++++---
drivers/media/platform/s5p-fimc/mipi-csis.h | 1 +
include/media/s5p_fimc.h | 17 ++
13 files changed, 936 insertions(+), 127 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
--
1.7.9.5
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 01/10] s5p-csis: Add device tree support
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-01 19:09 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-media
Cc: kyungmin.park, kgene.kim, swarren, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel,
Sylwester Nawrocki
s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
device. This patch support for binding the driver to the MIPI-CSIS
devices instantiated from device tree and for parsing all SoC and
board specific properties.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v3:
- dropped 'bus-width' property, hard coded the number of supported
data lanes per device also in dt case, it can be derived from the
compatible property if required;
- "samsung,s5pv210-csis" renamed to "samsung,exynos3110-csis",
updated the bindings description.
---
.../bindings/media/soc/samsung-mipi-csis.txt | 82 +++++++++++
drivers/media/platform/s5p-fimc/mipi-csis.c | 154 +++++++++++++++-----
drivers/media/platform/s5p-fimc/mipi-csis.h | 1 +
3 files changed, 202 insertions(+), 35 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
diff --git a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
new file mode 100644
index 0000000..c142272
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
@@ -0,0 +1,82 @@
+Samsung S5P/EXYNOS SoC MIPI-CSI2 receiver (MIPI CSIS)
+-----------------------------------------------------
+
+Required properties:
+
+- compatible : "samsung,exynos3110-csis" for Exynos3110 (S5PC110, S5PV210)
+ SoCs;
+ "samsung,exynos4210-csis" for Exynos4210 (S5PC210, S5PV310)
+ and later SoCs;
+- reg : physical base address and size of the device memory mapped
+ registers;
+- interrupts : should contain MIPI CSIS interrupt; the format of the
+ interrupt specifier depends on the interrupt controller;
+- vddio-supply : MIPI CSIS I/O and PLL voltage supply (e.g. 1.8V);
+- vddcore-supply : MIPI CSIS Core voltage supply (e.g. 1.1V).
+
+Optional properties:
+
+- clock-frequency : The IP's main (system bus) clock frequency in Hz, default
+ value when this property is not specified is 166 MHz;
+- samsung,csis-wclk : CSI-2 wrapper clock selection. If this property is present
+ external clock from CMU will be used, if not bus clock will
+ be selected.
+
+The device node should contain one 'port' child node with one child 'endpoint'
+node, as outlined in the common media bindings specification. See
+Documentation/devicetree/bindings/media/video-interfaces.txt for details.
+The following are properties specific to these nodes.
+
+port node
+---------
+
+- reg : (required) must be 3 for camera C input (CSIS0) or 4 for
+ camera D input (CSIS1);
+
+endpoint node
+-------------
+
+- data-lanes : (required) an array specifying active physical MIPI-CSI2
+ data input lanes and their mapping to logical lanes; the
+ array's content is unused, only its length is meaningful;
+
+- samsung,csis-hs-settle : (optional) differential receiver (HS-RX) settle time;
+
+
+Example:
+
+ reg0: regulator@0 {
+ };
+
+ reg1: regulator@1 {
+ };
+
+/* SoC properties */
+
+ aliases {
+ csis0 = &csis_0;
+ };
+
+ csis_0: csis@11880000 {
+ compatible = "samsung,exynos4210-csis";
+ reg = <0x11880000 0x1000>;
+ interrupts = <0 78 0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+
+/* Board properties */
+
+ csis_0: csis@11880000 {
+ clock-frequency = <166000000>;
+ vddio-supply = <®0>;
+ vddcore-supply = <®1>;
+ port {
+ reg = <3>; /* 3 - CSIS0, 4 - CSIS1 */
+ csis0_ep: endpoint {
+ remote-endpoint = <...>;
+ data-lanes = <1>, <2>;
+ samsung,csis-hs-settle = <12>;
+ };
+ };
+ };
diff --git a/drivers/media/platform/s5p-fimc/mipi-csis.c b/drivers/media/platform/s5p-fimc/mipi-csis.c
index 0f5104c..b906bfe 100644
--- a/drivers/media/platform/s5p-fimc/mipi-csis.c
+++ b/drivers/media/platform/s5p-fimc/mipi-csis.c
@@ -19,12 +19,14 @@
#include <linux/kernel.h>
#include <linux/memory.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/videodev2.h>
+#include <media/v4l2-of.h>
#include <media/v4l2-subdev.h>
#include <linux/platform_data/mipi-csis.h>
#include "mipi-csis.h"
@@ -113,6 +115,7 @@ static char *csi_clock_name[] = {
[CSIS_CLK_GATE] = "csis",
};
#define NUM_CSIS_CLOCKS ARRAY_SIZE(csi_clock_name)
+#define DEFAULT_SCLK_CSIS_FREQ 166000000UL
static const char * const csis_supply_name[] = {
"vddcore", /* CSIS Core (1.0V, 1.1V or 1.2V) suppply */
@@ -167,6 +170,11 @@ struct csis_pktbuf {
* @clock: CSIS clocks
* @irq: requested s5p-mipi-csis irq number
* @flags: the state variable for power and streaming control
+ * @clock_frequency: device bus clock frequency
+ * @hs_settle: HS-RX settle time
+ * @num_lanes: number of MIPI-CSI data lanes used
+ * @max_num_lanes: maximum number of MIPI-CSI data lanes supported
+ * @wclk_ext: CSI wrapper clock: 0 - bus clock, 1 - external SCLK_CAM
* @csis_fmt: current CSIS pixel format
* @format: common media bus format for the source and sink pad
* @slock: spinlock protecting structure members below
@@ -184,6 +192,13 @@ struct csis_state {
struct clk *clock[NUM_CSIS_CLOCKS];
int irq;
u32 flags;
+
+ u32 clk_frequency;
+ u32 hs_settle;
+ u32 num_lanes;
+ u32 max_num_lanes;
+ u8 wclk_ext;
+
const struct csis_pix_format *csis_fmt;
struct v4l2_mbus_framefmt format;
@@ -273,7 +288,6 @@ static void s5pcsis_reset(struct csis_state *state)
static void s5pcsis_system_enable(struct csis_state *state, int on)
{
- struct s5p_platform_mipi_csis *pdata = state->pdev->dev.platform_data;
u32 val, mask;
val = s5pcsis_read(state, S5PCSIS_CTRL);
@@ -286,7 +300,7 @@ static void s5pcsis_system_enable(struct csis_state *state, int on)
val = s5pcsis_read(state, S5PCSIS_DPHYCTRL);
val &= ~S5PCSIS_DPHYCTRL_ENABLE;
if (on) {
- mask = (1 << (pdata->lanes + 1)) - 1;
+ mask = (1 << (state->num_lanes + 1)) - 1;
val |= (mask & S5PCSIS_DPHYCTRL_ENABLE);
}
s5pcsis_write(state, S5PCSIS_DPHYCTRL, val);
@@ -321,15 +335,14 @@ static void s5pcsis_set_hsync_settle(struct csis_state *state, int settle)
static void s5pcsis_set_params(struct csis_state *state)
{
- struct s5p_platform_mipi_csis *pdata = state->pdev->dev.platform_data;
u32 val;
val = s5pcsis_read(state, S5PCSIS_CONFIG);
- val = (val & ~S5PCSIS_CFG_NR_LANE_MASK) | (pdata->lanes - 1);
+ val = (val & ~S5PCSIS_CFG_NR_LANE_MASK) | (state->num_lanes - 1);
s5pcsis_write(state, S5PCSIS_CONFIG, val);
__s5pcsis_set_format(state);
- s5pcsis_set_hsync_settle(state, pdata->hs_settle);
+ s5pcsis_set_hsync_settle(state, state->hs_settle);
val = s5pcsis_read(state, S5PCSIS_CTRL);
if (state->csis_fmt->data_alignment == 32)
@@ -338,7 +351,7 @@ static void s5pcsis_set_params(struct csis_state *state)
val &= ~S5PCSIS_CTRL_ALIGN_32BIT;
val &= ~S5PCSIS_CTRL_WCLK_EXTCLK;
- if (pdata->wclk_source)
+ if (state->wclk_ext)
val |= S5PCSIS_CTRL_WCLK_EXTCLK;
s5pcsis_write(state, S5PCSIS_CTRL, val);
@@ -701,54 +714,114 @@ static irqreturn_t s5pcsis_irq_handler(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static int s5pcsis_get_platform_data(struct platform_device *pdev,
+ struct csis_state *state)
+{
+ struct s5p_platform_mipi_csis *pdata = pdev->dev.platform_data;
+
+ if (pdata == NULL) {
+ dev_err(&pdev->dev, "Platform data not specified\n");
+ return -EINVAL;
+ }
+
+ state->clk_frequency = pdata->clk_rate;
+ state->num_lanes = pdata->lanes;
+ state->hs_settle = pdata->hs_settle;
+ state->index = max(0, pdev->id);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static int s5pcsis_parse_dt(struct platform_device *pdev,
+ struct csis_state *state)
+{
+ struct device_node *node = pdev->dev.of_node;
+ struct v4l2_of_endpoint endpoint;
+ int ret;
+
+ state->index = of_alias_get_id(node, "csis");
+ if (state->index < 0 || state->index >= CSIS_MAX_ENTITIES)
+ return -EINVAL;
+
+ if (of_property_read_u32(node, "clock-frequency",
+ &state->clk_frequency))
+ state->clk_frequency = DEFAULT_SCLK_CSIS_FREQ;
+ /*
+ * Get endpoint node. We care only about first child
+ * nodes since these are the only ones available.
+ */
+ while ((node = of_get_next_child(node, NULL))) {
+ if (!of_node_cmp(node->name, "endpoint"))
+ break;
+ of_node_put(node);
+ };
+ if (!node)
+ return -EINVAL;
+ of_property_read_u32(node, "samsung,csis-hs-settle",
+ &state->hs_settle);
+ state->wclk_ext = of_property_read_bool(node,
+ "samsung,csis-wclk");
+ ret = v4l2_of_parse_mipi_csi2(node, &endpoint);
+ state->num_lanes = endpoint.mbus.mipi_csi2.num_data_lanes;
+
+ of_node_put(node);
+ return 0;
+}
+#else
+#define s5pcsis_parse_dt(pdev, state) (-ENOSYS)
+#endif
+
static int s5pcsis_probe(struct platform_device *pdev)
{
- struct s5p_platform_mipi_csis *pdata;
+ struct device *dev = &pdev->dev;
struct resource *mem_res;
struct csis_state *state;
int ret = -ENOMEM;
int i;
- state = devm_kzalloc(&pdev->dev, sizeof(*state), GFP_KERNEL);
+ state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
if (!state)
return -ENOMEM;
mutex_init(&state->lock);
spin_lock_init(&state->slock);
-
state->pdev = pdev;
- state->index = max(0, pdev->id);
- pdata = pdev->dev.platform_data;
- if (pdata == NULL) {
- dev_err(&pdev->dev, "Platform data not fully specified\n");
- return -EINVAL;
- }
+ if (dev->of_node)
+ ret = s5pcsis_parse_dt(pdev, state);
+ else
+ ret = s5pcsis_get_platform_data(pdev, state);
+ if (ret < 0)
+ return ret;
+ if (state->index == 1)
+ state->max_num_lanes = CSIS1_MAX_LANES;
+ else
+ state->max_num_lanes = CSIS0_MAX_LANES;
- if ((state->index == 1 && pdata->lanes > CSIS1_MAX_LANES) ||
- pdata->lanes > CSIS0_MAX_LANES) {
- dev_err(&pdev->dev, "Unsupported number of data lanes: %d\n",
- pdata->lanes);
+ if (state->num_lanes == 0 || state->num_lanes > state->max_num_lanes) {
+ dev_err(dev, "Unsupported number of data lanes: %d (max. %d)\n",
+ state->num_lanes, state->max_num_lanes);
return -EINVAL;
}
mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- state->regs = devm_request_and_ioremap(&pdev->dev, mem_res);
+ state->regs = devm_request_and_ioremap(dev, mem_res);
if (state->regs == NULL) {
- dev_err(&pdev->dev, "Failed to request and remap io memory\n");
+ dev_err(dev, "Failed to request and remap io memory\n");
return -ENXIO;
}
state->irq = platform_get_irq(pdev, 0);
if (state->irq < 0) {
- dev_err(&pdev->dev, "Failed to get irq\n");
+ dev_err(dev, "Failed to get irq\n");
return state->irq;
}
for (i = 0; i < CSIS_NUM_SUPPLIES; i++)
state->supplies[i].supply = csis_supply_name[i];
- ret = devm_regulator_bulk_get(&pdev->dev, CSIS_NUM_SUPPLIES,
+ ret = devm_regulator_bulk_get(dev, CSIS_NUM_SUPPLIES,
state->supplies);
if (ret)
return ret;
@@ -757,11 +830,11 @@ static int s5pcsis_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
- if (pdata->clk_rate)
+ if (state->clk_frequency)
ret = clk_set_rate(state->clock[CSIS_CLK_MUX],
- pdata->clk_rate);
+ state->clk_frequency);
else
- dev_WARN(&pdev->dev, "No clock frequency specified!\n");
+ dev_WARN(dev, "No clock frequency specified!\n");
if (ret < 0)
goto e_clkput;
@@ -769,16 +842,17 @@ static int s5pcsis_probe(struct platform_device *pdev)
if (ret < 0)
goto e_clkput;
- ret = devm_request_irq(&pdev->dev, state->irq, s5pcsis_irq_handler,
- 0, dev_name(&pdev->dev), state);
+ ret = devm_request_irq(dev, state->irq, s5pcsis_irq_handler,
+ 0, dev_name(dev), state);
if (ret) {
- dev_err(&pdev->dev, "Interrupt request failed\n");
+ dev_err(dev, "Interrupt request failed\n");
goto e_clkdis;
}
v4l2_subdev_init(&state->sd, &s5pcsis_subdev_ops);
state->sd.owner = THIS_MODULE;
- strlcpy(state->sd.name, dev_name(&pdev->dev), sizeof(state->sd.name));
+ snprintf(state->sd.name, sizeof(state->sd.name), "%s.%d",
+ CSIS_SUBDEV_NAME, state->index);
state->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
state->csis_fmt = &s5pcsis_formats[0];
@@ -798,10 +872,12 @@ static int s5pcsis_probe(struct platform_device *pdev)
/* .. and a pointer to the subdev. */
platform_set_drvdata(pdev, &state->sd);
-
memcpy(state->events, s5pcsis_events, sizeof(state->events));
+ pm_runtime_enable(dev);
- pm_runtime_enable(&pdev->dev);
+ dev_info(&pdev->dev, "lanes: %d, hs_settle: %d, wclk: %d, freq: %u\n",
+ state->num_lanes, state->hs_settle, state->wclk_ext,
+ state->clk_frequency);
return 0;
e_clkdis:
@@ -925,13 +1001,21 @@ static const struct dev_pm_ops s5pcsis_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(s5pcsis_suspend, s5pcsis_resume)
};
+static const struct of_device_id s5pcsis_of_match[] __devinitconst = {
+ { .compatible = "samsung,exynos3110-csis" },
+ { .compatible = "samsung,exynos4210-csis" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, s5pcsis_of_match);
+
static struct platform_driver s5pcsis_driver = {
.probe = s5pcsis_probe,
.remove = s5pcsis_remove,
.driver = {
- .name = CSIS_DRIVER_NAME,
- .owner = THIS_MODULE,
- .pm = &s5pcsis_pm_ops,
+ .of_match_table = s5pcsis_of_match,
+ .name = CSIS_DRIVER_NAME,
+ .owner = THIS_MODULE,
+ .pm = &s5pcsis_pm_ops,
},
};
diff --git a/drivers/media/platform/s5p-fimc/mipi-csis.h b/drivers/media/platform/s5p-fimc/mipi-csis.h
index 2709286..28c11c4 100644
--- a/drivers/media/platform/s5p-fimc/mipi-csis.h
+++ b/drivers/media/platform/s5p-fimc/mipi-csis.h
@@ -11,6 +11,7 @@
#define S5P_MIPI_CSIS_H_
#define CSIS_DRIVER_NAME "s5p-mipi-csis"
+#define CSIS_SUBDEV_NAME CSIS_DRIVER_NAME
#define CSIS_MAX_ENTITIES 2
#define CSIS0_MAX_LANES 4
#define CSIS1_MAX_LANES 2
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 01/10] s5p-csis: Add device tree support
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-arm-kernel
s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
device. This patch support for binding the driver to the MIPI-CSIS
devices instantiated from device tree and for parsing all SoC and
board specific properties.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v3:
- dropped 'bus-width' property, hard coded the number of supported
data lanes per device also in dt case, it can be derived from the
compatible property if required;
- "samsung,s5pv210-csis" renamed to "samsung,exynos3110-csis",
updated the bindings description.
---
.../bindings/media/soc/samsung-mipi-csis.txt | 82 +++++++++++
drivers/media/platform/s5p-fimc/mipi-csis.c | 154 +++++++++++++++-----
drivers/media/platform/s5p-fimc/mipi-csis.h | 1 +
3 files changed, 202 insertions(+), 35 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
diff --git a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
new file mode 100644
index 0000000..c142272
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
@@ -0,0 +1,82 @@
+Samsung S5P/EXYNOS SoC MIPI-CSI2 receiver (MIPI CSIS)
+-----------------------------------------------------
+
+Required properties:
+
+- compatible : "samsung,exynos3110-csis" for Exynos3110 (S5PC110, S5PV210)
+ SoCs;
+ "samsung,exynos4210-csis" for Exynos4210 (S5PC210, S5PV310)
+ and later SoCs;
+- reg : physical base address and size of the device memory mapped
+ registers;
+- interrupts : should contain MIPI CSIS interrupt; the format of the
+ interrupt specifier depends on the interrupt controller;
+- vddio-supply : MIPI CSIS I/O and PLL voltage supply (e.g. 1.8V);
+- vddcore-supply : MIPI CSIS Core voltage supply (e.g. 1.1V).
+
+Optional properties:
+
+- clock-frequency : The IP's main (system bus) clock frequency in Hz, default
+ value when this property is not specified is 166 MHz;
+- samsung,csis-wclk : CSI-2 wrapper clock selection. If this property is present
+ external clock from CMU will be used, if not bus clock will
+ be selected.
+
+The device node should contain one 'port' child node with one child 'endpoint'
+node, as outlined in the common media bindings specification. See
+Documentation/devicetree/bindings/media/video-interfaces.txt for details.
+The following are properties specific to these nodes.
+
+port node
+---------
+
+- reg : (required) must be 3 for camera C input (CSIS0) or 4 for
+ camera D input (CSIS1);
+
+endpoint node
+-------------
+
+- data-lanes : (required) an array specifying active physical MIPI-CSI2
+ data input lanes and their mapping to logical lanes; the
+ array's content is unused, only its length is meaningful;
+
+- samsung,csis-hs-settle : (optional) differential receiver (HS-RX) settle time;
+
+
+Example:
+
+ reg0: regulator at 0 {
+ };
+
+ reg1: regulator at 1 {
+ };
+
+/* SoC properties */
+
+ aliases {
+ csis0 = &csis_0;
+ };
+
+ csis_0: csis at 11880000 {
+ compatible = "samsung,exynos4210-csis";
+ reg = <0x11880000 0x1000>;
+ interrupts = <0 78 0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+
+/* Board properties */
+
+ csis_0: csis at 11880000 {
+ clock-frequency = <166000000>;
+ vddio-supply = <®0>;
+ vddcore-supply = <®1>;
+ port {
+ reg = <3>; /* 3 - CSIS0, 4 - CSIS1 */
+ csis0_ep: endpoint {
+ remote-endpoint = <...>;
+ data-lanes = <1>, <2>;
+ samsung,csis-hs-settle = <12>;
+ };
+ };
+ };
diff --git a/drivers/media/platform/s5p-fimc/mipi-csis.c b/drivers/media/platform/s5p-fimc/mipi-csis.c
index 0f5104c..b906bfe 100644
--- a/drivers/media/platform/s5p-fimc/mipi-csis.c
+++ b/drivers/media/platform/s5p-fimc/mipi-csis.c
@@ -19,12 +19,14 @@
#include <linux/kernel.h>
#include <linux/memory.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/videodev2.h>
+#include <media/v4l2-of.h>
#include <media/v4l2-subdev.h>
#include <linux/platform_data/mipi-csis.h>
#include "mipi-csis.h"
@@ -113,6 +115,7 @@ static char *csi_clock_name[] = {
[CSIS_CLK_GATE] = "csis",
};
#define NUM_CSIS_CLOCKS ARRAY_SIZE(csi_clock_name)
+#define DEFAULT_SCLK_CSIS_FREQ 166000000UL
static const char * const csis_supply_name[] = {
"vddcore", /* CSIS Core (1.0V, 1.1V or 1.2V) suppply */
@@ -167,6 +170,11 @@ struct csis_pktbuf {
* @clock: CSIS clocks
* @irq: requested s5p-mipi-csis irq number
* @flags: the state variable for power and streaming control
+ * @clock_frequency: device bus clock frequency
+ * @hs_settle: HS-RX settle time
+ * @num_lanes: number of MIPI-CSI data lanes used
+ * @max_num_lanes: maximum number of MIPI-CSI data lanes supported
+ * @wclk_ext: CSI wrapper clock: 0 - bus clock, 1 - external SCLK_CAM
* @csis_fmt: current CSIS pixel format
* @format: common media bus format for the source and sink pad
* @slock: spinlock protecting structure members below
@@ -184,6 +192,13 @@ struct csis_state {
struct clk *clock[NUM_CSIS_CLOCKS];
int irq;
u32 flags;
+
+ u32 clk_frequency;
+ u32 hs_settle;
+ u32 num_lanes;
+ u32 max_num_lanes;
+ u8 wclk_ext;
+
const struct csis_pix_format *csis_fmt;
struct v4l2_mbus_framefmt format;
@@ -273,7 +288,6 @@ static void s5pcsis_reset(struct csis_state *state)
static void s5pcsis_system_enable(struct csis_state *state, int on)
{
- struct s5p_platform_mipi_csis *pdata = state->pdev->dev.platform_data;
u32 val, mask;
val = s5pcsis_read(state, S5PCSIS_CTRL);
@@ -286,7 +300,7 @@ static void s5pcsis_system_enable(struct csis_state *state, int on)
val = s5pcsis_read(state, S5PCSIS_DPHYCTRL);
val &= ~S5PCSIS_DPHYCTRL_ENABLE;
if (on) {
- mask = (1 << (pdata->lanes + 1)) - 1;
+ mask = (1 << (state->num_lanes + 1)) - 1;
val |= (mask & S5PCSIS_DPHYCTRL_ENABLE);
}
s5pcsis_write(state, S5PCSIS_DPHYCTRL, val);
@@ -321,15 +335,14 @@ static void s5pcsis_set_hsync_settle(struct csis_state *state, int settle)
static void s5pcsis_set_params(struct csis_state *state)
{
- struct s5p_platform_mipi_csis *pdata = state->pdev->dev.platform_data;
u32 val;
val = s5pcsis_read(state, S5PCSIS_CONFIG);
- val = (val & ~S5PCSIS_CFG_NR_LANE_MASK) | (pdata->lanes - 1);
+ val = (val & ~S5PCSIS_CFG_NR_LANE_MASK) | (state->num_lanes - 1);
s5pcsis_write(state, S5PCSIS_CONFIG, val);
__s5pcsis_set_format(state);
- s5pcsis_set_hsync_settle(state, pdata->hs_settle);
+ s5pcsis_set_hsync_settle(state, state->hs_settle);
val = s5pcsis_read(state, S5PCSIS_CTRL);
if (state->csis_fmt->data_alignment == 32)
@@ -338,7 +351,7 @@ static void s5pcsis_set_params(struct csis_state *state)
val &= ~S5PCSIS_CTRL_ALIGN_32BIT;
val &= ~S5PCSIS_CTRL_WCLK_EXTCLK;
- if (pdata->wclk_source)
+ if (state->wclk_ext)
val |= S5PCSIS_CTRL_WCLK_EXTCLK;
s5pcsis_write(state, S5PCSIS_CTRL, val);
@@ -701,54 +714,114 @@ static irqreturn_t s5pcsis_irq_handler(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static int s5pcsis_get_platform_data(struct platform_device *pdev,
+ struct csis_state *state)
+{
+ struct s5p_platform_mipi_csis *pdata = pdev->dev.platform_data;
+
+ if (pdata == NULL) {
+ dev_err(&pdev->dev, "Platform data not specified\n");
+ return -EINVAL;
+ }
+
+ state->clk_frequency = pdata->clk_rate;
+ state->num_lanes = pdata->lanes;
+ state->hs_settle = pdata->hs_settle;
+ state->index = max(0, pdev->id);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static int s5pcsis_parse_dt(struct platform_device *pdev,
+ struct csis_state *state)
+{
+ struct device_node *node = pdev->dev.of_node;
+ struct v4l2_of_endpoint endpoint;
+ int ret;
+
+ state->index = of_alias_get_id(node, "csis");
+ if (state->index < 0 || state->index >= CSIS_MAX_ENTITIES)
+ return -EINVAL;
+
+ if (of_property_read_u32(node, "clock-frequency",
+ &state->clk_frequency))
+ state->clk_frequency = DEFAULT_SCLK_CSIS_FREQ;
+ /*
+ * Get endpoint node. We care only about first child
+ * nodes since these are the only ones available.
+ */
+ while ((node = of_get_next_child(node, NULL))) {
+ if (!of_node_cmp(node->name, "endpoint"))
+ break;
+ of_node_put(node);
+ };
+ if (!node)
+ return -EINVAL;
+ of_property_read_u32(node, "samsung,csis-hs-settle",
+ &state->hs_settle);
+ state->wclk_ext = of_property_read_bool(node,
+ "samsung,csis-wclk");
+ ret = v4l2_of_parse_mipi_csi2(node, &endpoint);
+ state->num_lanes = endpoint.mbus.mipi_csi2.num_data_lanes;
+
+ of_node_put(node);
+ return 0;
+}
+#else
+#define s5pcsis_parse_dt(pdev, state) (-ENOSYS)
+#endif
+
static int s5pcsis_probe(struct platform_device *pdev)
{
- struct s5p_platform_mipi_csis *pdata;
+ struct device *dev = &pdev->dev;
struct resource *mem_res;
struct csis_state *state;
int ret = -ENOMEM;
int i;
- state = devm_kzalloc(&pdev->dev, sizeof(*state), GFP_KERNEL);
+ state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
if (!state)
return -ENOMEM;
mutex_init(&state->lock);
spin_lock_init(&state->slock);
-
state->pdev = pdev;
- state->index = max(0, pdev->id);
- pdata = pdev->dev.platform_data;
- if (pdata == NULL) {
- dev_err(&pdev->dev, "Platform data not fully specified\n");
- return -EINVAL;
- }
+ if (dev->of_node)
+ ret = s5pcsis_parse_dt(pdev, state);
+ else
+ ret = s5pcsis_get_platform_data(pdev, state);
+ if (ret < 0)
+ return ret;
+ if (state->index == 1)
+ state->max_num_lanes = CSIS1_MAX_LANES;
+ else
+ state->max_num_lanes = CSIS0_MAX_LANES;
- if ((state->index == 1 && pdata->lanes > CSIS1_MAX_LANES) ||
- pdata->lanes > CSIS0_MAX_LANES) {
- dev_err(&pdev->dev, "Unsupported number of data lanes: %d\n",
- pdata->lanes);
+ if (state->num_lanes == 0 || state->num_lanes > state->max_num_lanes) {
+ dev_err(dev, "Unsupported number of data lanes: %d (max. %d)\n",
+ state->num_lanes, state->max_num_lanes);
return -EINVAL;
}
mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- state->regs = devm_request_and_ioremap(&pdev->dev, mem_res);
+ state->regs = devm_request_and_ioremap(dev, mem_res);
if (state->regs == NULL) {
- dev_err(&pdev->dev, "Failed to request and remap io memory\n");
+ dev_err(dev, "Failed to request and remap io memory\n");
return -ENXIO;
}
state->irq = platform_get_irq(pdev, 0);
if (state->irq < 0) {
- dev_err(&pdev->dev, "Failed to get irq\n");
+ dev_err(dev, "Failed to get irq\n");
return state->irq;
}
for (i = 0; i < CSIS_NUM_SUPPLIES; i++)
state->supplies[i].supply = csis_supply_name[i];
- ret = devm_regulator_bulk_get(&pdev->dev, CSIS_NUM_SUPPLIES,
+ ret = devm_regulator_bulk_get(dev, CSIS_NUM_SUPPLIES,
state->supplies);
if (ret)
return ret;
@@ -757,11 +830,11 @@ static int s5pcsis_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
- if (pdata->clk_rate)
+ if (state->clk_frequency)
ret = clk_set_rate(state->clock[CSIS_CLK_MUX],
- pdata->clk_rate);
+ state->clk_frequency);
else
- dev_WARN(&pdev->dev, "No clock frequency specified!\n");
+ dev_WARN(dev, "No clock frequency specified!\n");
if (ret < 0)
goto e_clkput;
@@ -769,16 +842,17 @@ static int s5pcsis_probe(struct platform_device *pdev)
if (ret < 0)
goto e_clkput;
- ret = devm_request_irq(&pdev->dev, state->irq, s5pcsis_irq_handler,
- 0, dev_name(&pdev->dev), state);
+ ret = devm_request_irq(dev, state->irq, s5pcsis_irq_handler,
+ 0, dev_name(dev), state);
if (ret) {
- dev_err(&pdev->dev, "Interrupt request failed\n");
+ dev_err(dev, "Interrupt request failed\n");
goto e_clkdis;
}
v4l2_subdev_init(&state->sd, &s5pcsis_subdev_ops);
state->sd.owner = THIS_MODULE;
- strlcpy(state->sd.name, dev_name(&pdev->dev), sizeof(state->sd.name));
+ snprintf(state->sd.name, sizeof(state->sd.name), "%s.%d",
+ CSIS_SUBDEV_NAME, state->index);
state->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
state->csis_fmt = &s5pcsis_formats[0];
@@ -798,10 +872,12 @@ static int s5pcsis_probe(struct platform_device *pdev)
/* .. and a pointer to the subdev. */
platform_set_drvdata(pdev, &state->sd);
-
memcpy(state->events, s5pcsis_events, sizeof(state->events));
+ pm_runtime_enable(dev);
- pm_runtime_enable(&pdev->dev);
+ dev_info(&pdev->dev, "lanes: %d, hs_settle: %d, wclk: %d, freq: %u\n",
+ state->num_lanes, state->hs_settle, state->wclk_ext,
+ state->clk_frequency);
return 0;
e_clkdis:
@@ -925,13 +1001,21 @@ static const struct dev_pm_ops s5pcsis_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(s5pcsis_suspend, s5pcsis_resume)
};
+static const struct of_device_id s5pcsis_of_match[] __devinitconst = {
+ { .compatible = "samsung,exynos3110-csis" },
+ { .compatible = "samsung,exynos4210-csis" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, s5pcsis_of_match);
+
static struct platform_driver s5pcsis_driver = {
.probe = s5pcsis_probe,
.remove = s5pcsis_remove,
.driver = {
- .name = CSIS_DRIVER_NAME,
- .owner = THIS_MODULE,
- .pm = &s5pcsis_pm_ops,
+ .of_match_table = s5pcsis_of_match,
+ .name = CSIS_DRIVER_NAME,
+ .owner = THIS_MODULE,
+ .pm = &s5pcsis_pm_ops,
},
};
diff --git a/drivers/media/platform/s5p-fimc/mipi-csis.h b/drivers/media/platform/s5p-fimc/mipi-csis.h
index 2709286..28c11c4 100644
--- a/drivers/media/platform/s5p-fimc/mipi-csis.h
+++ b/drivers/media/platform/s5p-fimc/mipi-csis.h
@@ -11,6 +11,7 @@
#define S5P_MIPI_CSIS_H_
#define CSIS_DRIVER_NAME "s5p-mipi-csis"
+#define CSIS_SUBDEV_NAME CSIS_DRIVER_NAME
#define CSIS_MAX_ENTITIES 2
#define CSIS0_MAX_LANES 4
#define CSIS1_MAX_LANES 2
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-01 19:09 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-media
Cc: kyungmin.park, kgene.kim, swarren, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel,
Sylwester Nawrocki
This patch adds support for FIMC devices instantiated from devicetree
for S5PV210 and Exynos4 SoCs. The FIMC IP features include colorspace
conversion and scaling (mem-to-mem) and parallel/MIPI CSI2 bus video
capture interface.
Multiple SoC revisions specific parameters are defined statically in
the driver and are used for both dt and non-dt. The driver's static
data is selected based on the compatible property. Previously the
platform device name was used to match driver data and a specific
SoC/IP version.
Aliases are used to determine an index of the IP which is essential
for linking FIMC IP with other entities, like MIPI-CSIS (the MIPI
CSI-2 bus frontend) or FIMC-LITE and FIMC-IS ISP.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v3:
- added optional clock-frequency property to specify local bus
(FIMCn_LCLK) clock frequency
---
.../devicetree/bindings/media/soc/samsung-fimc.txt | 72 +++++++++++++++
drivers/media/platform/s5p-fimc/fimc-capture.c | 2 +-
drivers/media/platform/s5p-fimc/fimc-core.c | 93 +++++++++++++-------
3 files changed, 135 insertions(+), 32 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
new file mode 100644
index 0000000..916b2c3
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -0,0 +1,72 @@
+Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
+----------------------------------------------
+
+The Exynos Camera subsystem comprises of multiple sub-devices that are
+represented by separate platform devices. Some of the IPs come in different
+variants across the SoC revisions (FIMC) and some remain mostly unchanged
+(MIPI CSIS, FIMC-LITE).
+
+All those sub-subdevices are defined as parent nodes of the common device
+node, which also includes common properties of the whole subsystem not really
+specific to any single sub-device, like common camera port pins or external
+clocks for image sensors attached to the SoC.
+
+Common 'camera' node
+--------------------
+
+Required properties:
+
+- compatible : must be "samsung,fimc", "simple-bus"
+
+The 'camera' node must include at least one 'fimc' child node.
+
+
+'fimc' device nodes
+-------------------
+
+Required properties:
+
+- compatible : "samsung,s5pv210-fimc" for S5PV210,
+ "samsung,exynos4210-fimc" for Exynos4210,
+ "samsung,exynos4212-fimc" for Exynos4212/4412 SoCs;
+- reg : physical base address and size of the device memory mapped
+ registers;
+- interrupts : FIMC interrupt to the CPU should be described here;
+
+For every fimc node a numbered alias should be present in the aliases node.
+Aliases are of the form fimc<n>, where <n> is an integer (0...N) specifying
+the IP's instance index.
+
+Optional properties
+
+ - clock-frequency - maximum FIMC local clock (LCLK) frequency
+
+Example:
+
+ aliases {
+ csis0 = &csis_0;
+ fimc0 = &fimc_0;
+ };
+
+ camera {
+ compatible = "samsung,fimc", "simple-bus";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ status = "okay";
+
+ fimc_0: fimc@11800000 {
+ compatible = "samsung,exynos4210-fimc";
+ reg = <0x11800000 0x1000>;
+ interrupts = <0 85 0>;
+ status = "okay";
+ };
+
+ csis_0: csis@11880000 {
+ compatible = "samsung,exynos4210-csis";
+ reg = <0x11880000 0x1000>;
+ interrupts = <0 78 0>;
+ max-data-lanes = <4>;
+ };
+ };
+
+[1] Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
index f822b8e..bc84301 100644
--- a/drivers/media/platform/s5p-fimc/fimc-capture.c
+++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
@@ -1883,7 +1883,7 @@ int fimc_initialize_capture_subdev(struct fimc_dev *fimc)
v4l2_subdev_init(sd, &fimc_subdev_ops);
sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
- snprintf(sd->name, sizeof(sd->name), "FIMC.%d", fimc->pdev->id);
+ snprintf(sd->name, sizeof(sd->name), "FIMC.%d", fimc->id);
fimc->vid_cap.sd_pads[FIMC_SD_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
fimc->vid_cap.sd_pads[FIMC_SD_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
diff --git a/drivers/media/platform/s5p-fimc/fimc-core.c b/drivers/media/platform/s5p-fimc/fimc-core.c
index 29f7bb7..07ca0e0 100644
--- a/drivers/media/platform/s5p-fimc/fimc-core.c
+++ b/drivers/media/platform/s5p-fimc/fimc-core.c
@@ -21,6 +21,8 @@
#include <linux/pm_runtime.h>
#include <linux/list.h>
#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/slab.h>
#include <linux/clk.h>
#include <media/v4l2-ioctl.h>
@@ -863,45 +865,57 @@ static int fimc_m2m_resume(struct fimc_dev *fimc)
return 0;
}
+static const struct of_device_id fimc_of_match[];
+
static int fimc_probe(struct platform_device *pdev)
{
- const struct fimc_drvdata *drv_data = fimc_get_drvdata(pdev);
- struct s5p_platform_fimc *pdata;
+ struct fimc_drvdata *drv_data = NULL;
+ struct device *dev = &pdev->dev;
+ const struct of_device_id *of_id;
+ u32 lclk_freq = 0;
struct fimc_dev *fimc;
struct resource *res;
int ret = 0;
- if (pdev->id >= drv_data->num_entities) {
- dev_err(&pdev->dev, "Invalid platform device id: %d\n",
- pdev->id);
- return -EINVAL;
- }
-
- fimc = devm_kzalloc(&pdev->dev, sizeof(*fimc), GFP_KERNEL);
+ fimc = devm_kzalloc(dev, sizeof(*fimc), GFP_KERNEL);
if (!fimc)
return -ENOMEM;
- fimc->id = pdev->id;
+ if (dev->of_node) {
+ of_id = of_match_node(fimc_of_match, dev->of_node);
+ if (of_id)
+ drv_data = (struct fimc_drvdata *)of_id->data;
+ fimc->id = of_alias_get_id(dev->of_node, "fimc");
+
+ of_property_read_u32(dev->of_node, "clock-frequency",
+ &lclk_freq);
+ } else {
+ drv_data = fimc_get_drvdata(pdev);
+ fimc->id = pdev->id;
+ }
+
+ if (!drv_data || fimc->id < 0 || fimc->id >= drv_data->num_entities) {
+ dev_err(dev, "Invalid driver data or device index (%d)\n",
+ fimc->id);
+ return -EINVAL;
+ }
fimc->variant = drv_data->variant[fimc->id];
fimc->pdev = pdev;
- pdata = pdev->dev.platform_data;
- fimc->pdata = pdata;
-
init_waitqueue_head(&fimc->irq_queue);
spin_lock_init(&fimc->slock);
mutex_init(&fimc->lock);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- fimc->regs = devm_request_and_ioremap(&pdev->dev, res);
+ fimc->regs = devm_request_and_ioremap(dev, res);
if (fimc->regs == NULL) {
- dev_err(&pdev->dev, "Failed to obtain io memory\n");
+ dev_err(dev, "Failed to obtain io memory\n");
return -ENOENT;
}
res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "Failed to get IRQ resource\n");
+ dev_err(dev, "Failed to get IRQ resource\n");
return -ENXIO;
}
@@ -909,7 +923,10 @@ static int fimc_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = clk_set_rate(fimc->clock[CLK_BUS], drv_data->lclk_frequency);
+ if (lclk_freq == 0)
+ lclk_freq = drv_data->lclk_frequency;
+
+ ret = clk_set_rate(fimc->clock[CLK_BUS], lclk_freq);
if (ret < 0)
return ret;
@@ -917,10 +934,10 @@ static int fimc_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
- ret = devm_request_irq(&pdev->dev, res->start, fimc_irq_handler,
- 0, dev_name(&pdev->dev), fimc);
+ ret = devm_request_irq(dev, res->start, fimc_irq_handler,
+ 0, dev_name(dev), fimc);
if (ret) {
- dev_err(&pdev->dev, "failed to install irq (%d)\n", ret);
+ dev_err(dev, "failed to install irq (%d)\n", ret);
goto err_clk;
}
@@ -929,27 +946,26 @@ static int fimc_probe(struct platform_device *pdev)
goto err_clk;
platform_set_drvdata(pdev, fimc);
- pm_runtime_enable(&pdev->dev);
- ret = pm_runtime_get_sync(&pdev->dev);
+ pm_runtime_enable(dev);
+ ret = pm_runtime_get_sync(dev);
if (ret < 0)
goto err_sd;
/* Initialize contiguous memory allocator */
- fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
+ fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
if (IS_ERR(fimc->alloc_ctx)) {
ret = PTR_ERR(fimc->alloc_ctx);
goto err_pm;
}
- dev_dbg(&pdev->dev, "FIMC.%d registered successfully\n", fimc->id);
+ dev_dbg(dev, "FIMC.%d registered successfully\n", fimc->id);
- pm_runtime_put(&pdev->dev);
+ pm_runtime_put(dev);
return 0;
err_pm:
- pm_runtime_put(&pdev->dev);
+ pm_runtime_put(dev);
err_sd:
fimc_unregister_capture_subdev(fimc);
err_clk:
- clk_disable(fimc->clock[CLK_BUS]);
fimc_clk_put(fimc);
return ret;
}
@@ -1258,10 +1274,24 @@ static const struct platform_device_id fimc_driver_ids[] = {
.name = "exynos4x12-fimc",
.driver_data = (unsigned long)&fimc_drvdata_exynos4x12,
},
- {},
+ { },
};
MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
+static const struct of_device_id fimc_of_match[] = {
+ {
+ .compatible = "samsung,s5pv210-fimc",
+ .data = &fimc_drvdata_s5pv210,
+ }, {
+ .compatible = "samsung,exynos4210-fimc",
+ .data = &fimc_drvdata_exynos4210,
+ }, {
+ .compatible = "samsung,exynos4212-fimc",
+ .data = &fimc_drvdata_exynos4x12,
+ },
+ { /* sentinel */ },
+};
+
static const struct dev_pm_ops fimc_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(fimc_suspend, fimc_resume)
SET_RUNTIME_PM_OPS(fimc_runtime_suspend, fimc_runtime_resume, NULL)
@@ -1272,9 +1302,10 @@ static struct platform_driver fimc_driver = {
.remove = fimc_remove,
.id_table = fimc_driver_ids,
.driver = {
- .name = FIMC_MODULE_NAME,
- .owner = THIS_MODULE,
- .pm = &fimc_pm_ops,
+ .of_match_table = fimc_of_match,
+ .name = FIMC_MODULE_NAME,
+ .owner = THIS_MODULE,
+ .pm = &fimc_pm_ops,
}
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds support for FIMC devices instantiated from devicetree
for S5PV210 and Exynos4 SoCs. The FIMC IP features include colorspace
conversion and scaling (mem-to-mem) and parallel/MIPI CSI2 bus video
capture interface.
Multiple SoC revisions specific parameters are defined statically in
the driver and are used for both dt and non-dt. The driver's static
data is selected based on the compatible property. Previously the
platform device name was used to match driver data and a specific
SoC/IP version.
Aliases are used to determine an index of the IP which is essential
for linking FIMC IP with other entities, like MIPI-CSIS (the MIPI
CSI-2 bus frontend) or FIMC-LITE and FIMC-IS ISP.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v3:
- added optional clock-frequency property to specify local bus
(FIMCn_LCLK) clock frequency
---
.../devicetree/bindings/media/soc/samsung-fimc.txt | 72 +++++++++++++++
drivers/media/platform/s5p-fimc/fimc-capture.c | 2 +-
drivers/media/platform/s5p-fimc/fimc-core.c | 93 +++++++++++++-------
3 files changed, 135 insertions(+), 32 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
new file mode 100644
index 0000000..916b2c3
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -0,0 +1,72 @@
+Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
+----------------------------------------------
+
+The Exynos Camera subsystem comprises of multiple sub-devices that are
+represented by separate platform devices. Some of the IPs come in different
+variants across the SoC revisions (FIMC) and some remain mostly unchanged
+(MIPI CSIS, FIMC-LITE).
+
+All those sub-subdevices are defined as parent nodes of the common device
+node, which also includes common properties of the whole subsystem not really
+specific to any single sub-device, like common camera port pins or external
+clocks for image sensors attached to the SoC.
+
+Common 'camera' node
+--------------------
+
+Required properties:
+
+- compatible : must be "samsung,fimc", "simple-bus"
+
+The 'camera' node must include at least one 'fimc' child node.
+
+
+'fimc' device nodes
+-------------------
+
+Required properties:
+
+- compatible : "samsung,s5pv210-fimc" for S5PV210,
+ "samsung,exynos4210-fimc" for Exynos4210,
+ "samsung,exynos4212-fimc" for Exynos4212/4412 SoCs;
+- reg : physical base address and size of the device memory mapped
+ registers;
+- interrupts : FIMC interrupt to the CPU should be described here;
+
+For every fimc node a numbered alias should be present in the aliases node.
+Aliases are of the form fimc<n>, where <n> is an integer (0...N) specifying
+the IP's instance index.
+
+Optional properties
+
+ - clock-frequency - maximum FIMC local clock (LCLK) frequency
+
+Example:
+
+ aliases {
+ csis0 = &csis_0;
+ fimc0 = &fimc_0;
+ };
+
+ camera {
+ compatible = "samsung,fimc", "simple-bus";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ status = "okay";
+
+ fimc_0: fimc at 11800000 {
+ compatible = "samsung,exynos4210-fimc";
+ reg = <0x11800000 0x1000>;
+ interrupts = <0 85 0>;
+ status = "okay";
+ };
+
+ csis_0: csis at 11880000 {
+ compatible = "samsung,exynos4210-csis";
+ reg = <0x11880000 0x1000>;
+ interrupts = <0 78 0>;
+ max-data-lanes = <4>;
+ };
+ };
+
+[1] Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
index f822b8e..bc84301 100644
--- a/drivers/media/platform/s5p-fimc/fimc-capture.c
+++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
@@ -1883,7 +1883,7 @@ int fimc_initialize_capture_subdev(struct fimc_dev *fimc)
v4l2_subdev_init(sd, &fimc_subdev_ops);
sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
- snprintf(sd->name, sizeof(sd->name), "FIMC.%d", fimc->pdev->id);
+ snprintf(sd->name, sizeof(sd->name), "FIMC.%d", fimc->id);
fimc->vid_cap.sd_pads[FIMC_SD_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
fimc->vid_cap.sd_pads[FIMC_SD_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
diff --git a/drivers/media/platform/s5p-fimc/fimc-core.c b/drivers/media/platform/s5p-fimc/fimc-core.c
index 29f7bb7..07ca0e0 100644
--- a/drivers/media/platform/s5p-fimc/fimc-core.c
+++ b/drivers/media/platform/s5p-fimc/fimc-core.c
@@ -21,6 +21,8 @@
#include <linux/pm_runtime.h>
#include <linux/list.h>
#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/slab.h>
#include <linux/clk.h>
#include <media/v4l2-ioctl.h>
@@ -863,45 +865,57 @@ static int fimc_m2m_resume(struct fimc_dev *fimc)
return 0;
}
+static const struct of_device_id fimc_of_match[];
+
static int fimc_probe(struct platform_device *pdev)
{
- const struct fimc_drvdata *drv_data = fimc_get_drvdata(pdev);
- struct s5p_platform_fimc *pdata;
+ struct fimc_drvdata *drv_data = NULL;
+ struct device *dev = &pdev->dev;
+ const struct of_device_id *of_id;
+ u32 lclk_freq = 0;
struct fimc_dev *fimc;
struct resource *res;
int ret = 0;
- if (pdev->id >= drv_data->num_entities) {
- dev_err(&pdev->dev, "Invalid platform device id: %d\n",
- pdev->id);
- return -EINVAL;
- }
-
- fimc = devm_kzalloc(&pdev->dev, sizeof(*fimc), GFP_KERNEL);
+ fimc = devm_kzalloc(dev, sizeof(*fimc), GFP_KERNEL);
if (!fimc)
return -ENOMEM;
- fimc->id = pdev->id;
+ if (dev->of_node) {
+ of_id = of_match_node(fimc_of_match, dev->of_node);
+ if (of_id)
+ drv_data = (struct fimc_drvdata *)of_id->data;
+ fimc->id = of_alias_get_id(dev->of_node, "fimc");
+
+ of_property_read_u32(dev->of_node, "clock-frequency",
+ &lclk_freq);
+ } else {
+ drv_data = fimc_get_drvdata(pdev);
+ fimc->id = pdev->id;
+ }
+
+ if (!drv_data || fimc->id < 0 || fimc->id >= drv_data->num_entities) {
+ dev_err(dev, "Invalid driver data or device index (%d)\n",
+ fimc->id);
+ return -EINVAL;
+ }
fimc->variant = drv_data->variant[fimc->id];
fimc->pdev = pdev;
- pdata = pdev->dev.platform_data;
- fimc->pdata = pdata;
-
init_waitqueue_head(&fimc->irq_queue);
spin_lock_init(&fimc->slock);
mutex_init(&fimc->lock);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- fimc->regs = devm_request_and_ioremap(&pdev->dev, res);
+ fimc->regs = devm_request_and_ioremap(dev, res);
if (fimc->regs == NULL) {
- dev_err(&pdev->dev, "Failed to obtain io memory\n");
+ dev_err(dev, "Failed to obtain io memory\n");
return -ENOENT;
}
res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "Failed to get IRQ resource\n");
+ dev_err(dev, "Failed to get IRQ resource\n");
return -ENXIO;
}
@@ -909,7 +923,10 @@ static int fimc_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = clk_set_rate(fimc->clock[CLK_BUS], drv_data->lclk_frequency);
+ if (lclk_freq == 0)
+ lclk_freq = drv_data->lclk_frequency;
+
+ ret = clk_set_rate(fimc->clock[CLK_BUS], lclk_freq);
if (ret < 0)
return ret;
@@ -917,10 +934,10 @@ static int fimc_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
- ret = devm_request_irq(&pdev->dev, res->start, fimc_irq_handler,
- 0, dev_name(&pdev->dev), fimc);
+ ret = devm_request_irq(dev, res->start, fimc_irq_handler,
+ 0, dev_name(dev), fimc);
if (ret) {
- dev_err(&pdev->dev, "failed to install irq (%d)\n", ret);
+ dev_err(dev, "failed to install irq (%d)\n", ret);
goto err_clk;
}
@@ -929,27 +946,26 @@ static int fimc_probe(struct platform_device *pdev)
goto err_clk;
platform_set_drvdata(pdev, fimc);
- pm_runtime_enable(&pdev->dev);
- ret = pm_runtime_get_sync(&pdev->dev);
+ pm_runtime_enable(dev);
+ ret = pm_runtime_get_sync(dev);
if (ret < 0)
goto err_sd;
/* Initialize contiguous memory allocator */
- fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
+ fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
if (IS_ERR(fimc->alloc_ctx)) {
ret = PTR_ERR(fimc->alloc_ctx);
goto err_pm;
}
- dev_dbg(&pdev->dev, "FIMC.%d registered successfully\n", fimc->id);
+ dev_dbg(dev, "FIMC.%d registered successfully\n", fimc->id);
- pm_runtime_put(&pdev->dev);
+ pm_runtime_put(dev);
return 0;
err_pm:
- pm_runtime_put(&pdev->dev);
+ pm_runtime_put(dev);
err_sd:
fimc_unregister_capture_subdev(fimc);
err_clk:
- clk_disable(fimc->clock[CLK_BUS]);
fimc_clk_put(fimc);
return ret;
}
@@ -1258,10 +1274,24 @@ static const struct platform_device_id fimc_driver_ids[] = {
.name = "exynos4x12-fimc",
.driver_data = (unsigned long)&fimc_drvdata_exynos4x12,
},
- {},
+ { },
};
MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
+static const struct of_device_id fimc_of_match[] = {
+ {
+ .compatible = "samsung,s5pv210-fimc",
+ .data = &fimc_drvdata_s5pv210,
+ }, {
+ .compatible = "samsung,exynos4210-fimc",
+ .data = &fimc_drvdata_exynos4210,
+ }, {
+ .compatible = "samsung,exynos4212-fimc",
+ .data = &fimc_drvdata_exynos4x12,
+ },
+ { /* sentinel */ },
+};
+
static const struct dev_pm_ops fimc_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(fimc_suspend, fimc_resume)
SET_RUNTIME_PM_OPS(fimc_runtime_suspend, fimc_runtime_resume, NULL)
@@ -1272,9 +1302,10 @@ static struct platform_driver fimc_driver = {
.remove = fimc_remove,
.id_table = fimc_driver_ids,
.driver = {
- .name = FIMC_MODULE_NAME,
- .owner = THIS_MODULE,
- .pm = &fimc_pm_ops,
+ .of_match_table = fimc_of_match,
+ .name = FIMC_MODULE_NAME,
+ .owner = THIS_MODULE,
+ .pm = &fimc_pm_ops,
}
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 03/10] s5p-fimc: Add device tree support for FIMC-LITE devices
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-01 19:09 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-media
Cc: kyungmin.park, kgene.kim, swarren, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel,
Sylwester Nawrocki
This patch add support for binding the driver to FIMC-LITE devices
instantiated from the device tree.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
.../devicetree/bindings/media/soc/samsung-fimc.txt | 15 +++++
drivers/media/platform/s5p-fimc/fimc-lite.c | 65 ++++++++++++++------
2 files changed, 61 insertions(+), 19 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
index 916b2c3..2b932f2 100644
--- a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -41,6 +41,21 @@ Optional properties
- clock-frequency - maximum FIMC local clock (LCLK) frequency
+'fimc-lite' device nodes
+-----------------------
+
+Required properties:
+
+- compatible : should be "samsung,exynos4212-fimc" for Exynos4212 and
+ Exynos4412 SoCs;
+- reg : physical base address and size of the device memory mapped
+ registers;
+- interrupts : should contain FIMC-LITE interrupt;
+
+For every fimc-lite node a numbered alias should be present in the aliases
+node. Aliases are in form of fimc-lite<n>, where <n> is an integer (0...N)
+specifying the IP's instance index.
+
Example:
aliases {
diff --git a/drivers/media/platform/s5p-fimc/fimc-lite.c b/drivers/media/platform/s5p-fimc/fimc-lite.c
index e18babf..a0a70bc 100644
--- a/drivers/media/platform/s5p-fimc/fimc-lite.c
+++ b/drivers/media/platform/s5p-fimc/fimc-lite.c
@@ -17,6 +17,7 @@
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/types.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
@@ -1480,18 +1481,34 @@ static int fimc_lite_clk_get(struct fimc_lite *fimc)
return ret;
}
+static const struct of_device_id flite_of_match[];
+
static int fimc_lite_probe(struct platform_device *pdev)
{
- struct flite_drvdata *drv_data = fimc_lite_get_drvdata(pdev);
+ struct flite_drvdata *drv_data = NULL;
+ struct device *dev = &pdev->dev;
+ const struct of_device_id *of_id;
struct fimc_lite *fimc;
struct resource *res;
int ret;
- fimc = devm_kzalloc(&pdev->dev, sizeof(*fimc), GFP_KERNEL);
+ fimc = devm_kzalloc(dev, sizeof(*fimc), GFP_KERNEL);
if (!fimc)
return -ENOMEM;
- fimc->index = pdev->id;
+ if (dev->of_node) {
+ of_id = of_match_node(flite_of_match, dev->of_node);
+ if (of_id)
+ drv_data = (struct flite_drvdata *)of_id->data;
+ fimc->index = of_alias_get_id(dev->of_node, "fimc-lite");
+ } else {
+ drv_data = fimc_lite_get_drvdata(pdev);
+ fimc->index = pdev->id;
+ }
+
+ if (!drv_data || fimc->index < 0 || fimc->index >= FIMC_LITE_MAX_DEVS)
+ return -EINVAL;
+
fimc->variant = drv_data->variant[fimc->index];
fimc->pdev = pdev;
@@ -1500,15 +1517,15 @@ static int fimc_lite_probe(struct platform_device *pdev)
mutex_init(&fimc->lock);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- fimc->regs = devm_request_and_ioremap(&pdev->dev, res);
+ fimc->regs = devm_request_and_ioremap(dev, res);
if (fimc->regs == NULL) {
- dev_err(&pdev->dev, "Failed to obtain io memory\n");
+ dev_err(dev, "Failed to obtain io memory\n");
return -ENOENT;
}
res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "Failed to get IRQ resource\n");
+ dev_err(dev, "Failed to get IRQ resource\n");
return -ENXIO;
}
@@ -1516,10 +1533,10 @@ static int fimc_lite_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = devm_request_irq(&pdev->dev, res->start, flite_irq_handler,
- 0, dev_name(&pdev->dev), fimc);
+ ret = devm_request_irq(dev, res->start, flite_irq_handler,
+ 0, dev_name(dev), fimc);
if (ret) {
- dev_err(&pdev->dev, "Failed to install irq (%d)\n", ret);
+ dev_err(dev, "Failed to install irq (%d)\n", ret);
goto err_clk;
}
@@ -1529,23 +1546,23 @@ static int fimc_lite_probe(struct platform_device *pdev)
goto err_clk;
platform_set_drvdata(pdev, fimc);
- pm_runtime_enable(&pdev->dev);
- ret = pm_runtime_get_sync(&pdev->dev);
+ pm_runtime_enable(dev);
+ ret = pm_runtime_get_sync(dev);
if (ret < 0)
goto err_sd;
- fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
+ fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
if (IS_ERR(fimc->alloc_ctx)) {
ret = PTR_ERR(fimc->alloc_ctx);
goto err_pm;
}
- pm_runtime_put(&pdev->dev);
+ pm_runtime_put(dev);
- dev_dbg(&pdev->dev, "FIMC-LITE.%d registered successfully\n",
+ dev_dbg(dev, "FIMC-LITE.%d registered successfully\n",
fimc->index);
return 0;
err_pm:
- pm_runtime_put(&pdev->dev);
+ pm_runtime_put(dev);
err_sd:
fimc_lite_unregister_capture_subdev(fimc);
err_clk:
@@ -1636,6 +1653,12 @@ static int fimc_lite_remove(struct platform_device *pdev)
return 0;
}
+static const struct dev_pm_ops fimc_lite_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(fimc_lite_suspend, fimc_lite_resume)
+ SET_RUNTIME_PM_OPS(fimc_lite_runtime_suspend, fimc_lite_runtime_resume,
+ NULL)
+};
+
static struct flite_variant fimc_lite0_variant_exynos4 = {
.max_width = 8192,
.max_height = 8192,
@@ -1661,17 +1684,21 @@ static struct platform_device_id fimc_lite_driver_ids[] = {
};
MODULE_DEVICE_TABLE(platform, fimc_lite_driver_ids);
-static const struct dev_pm_ops fimc_lite_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(fimc_lite_suspend, fimc_lite_resume)
- SET_RUNTIME_PM_OPS(fimc_lite_runtime_suspend, fimc_lite_runtime_resume,
- NULL)
+static const struct of_device_id flite_of_match[] = {
+ {
+ .compatible = "samsung,exynos4212-fimc-lite",
+ .data = &fimc_lite_drvdata_exynos4,
+ },
+ { /* sentinel */ },
};
+MODULE_DEVICE_TABLE(of, flite_of_match);
static struct platform_driver fimc_lite_driver = {
.probe = fimc_lite_probe,
.remove = fimc_lite_remove,
.id_table = fimc_lite_driver_ids,
.driver = {
+ .of_match_table = flite_of_match,
.name = FIMC_LITE_DRV_NAME,
.owner = THIS_MODULE,
.pm = &fimc_lite_pm_ops,
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 03/10] s5p-fimc: Add device tree support for FIMC-LITE devices
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-arm-kernel
This patch add support for binding the driver to FIMC-LITE devices
instantiated from the device tree.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
.../devicetree/bindings/media/soc/samsung-fimc.txt | 15 +++++
drivers/media/platform/s5p-fimc/fimc-lite.c | 65 ++++++++++++++------
2 files changed, 61 insertions(+), 19 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
index 916b2c3..2b932f2 100644
--- a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -41,6 +41,21 @@ Optional properties
- clock-frequency - maximum FIMC local clock (LCLK) frequency
+'fimc-lite' device nodes
+-----------------------
+
+Required properties:
+
+- compatible : should be "samsung,exynos4212-fimc" for Exynos4212 and
+ Exynos4412 SoCs;
+- reg : physical base address and size of the device memory mapped
+ registers;
+- interrupts : should contain FIMC-LITE interrupt;
+
+For every fimc-lite node a numbered alias should be present in the aliases
+node. Aliases are in form of fimc-lite<n>, where <n> is an integer (0...N)
+specifying the IP's instance index.
+
Example:
aliases {
diff --git a/drivers/media/platform/s5p-fimc/fimc-lite.c b/drivers/media/platform/s5p-fimc/fimc-lite.c
index e18babf..a0a70bc 100644
--- a/drivers/media/platform/s5p-fimc/fimc-lite.c
+++ b/drivers/media/platform/s5p-fimc/fimc-lite.c
@@ -17,6 +17,7 @@
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/types.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
@@ -1480,18 +1481,34 @@ static int fimc_lite_clk_get(struct fimc_lite *fimc)
return ret;
}
+static const struct of_device_id flite_of_match[];
+
static int fimc_lite_probe(struct platform_device *pdev)
{
- struct flite_drvdata *drv_data = fimc_lite_get_drvdata(pdev);
+ struct flite_drvdata *drv_data = NULL;
+ struct device *dev = &pdev->dev;
+ const struct of_device_id *of_id;
struct fimc_lite *fimc;
struct resource *res;
int ret;
- fimc = devm_kzalloc(&pdev->dev, sizeof(*fimc), GFP_KERNEL);
+ fimc = devm_kzalloc(dev, sizeof(*fimc), GFP_KERNEL);
if (!fimc)
return -ENOMEM;
- fimc->index = pdev->id;
+ if (dev->of_node) {
+ of_id = of_match_node(flite_of_match, dev->of_node);
+ if (of_id)
+ drv_data = (struct flite_drvdata *)of_id->data;
+ fimc->index = of_alias_get_id(dev->of_node, "fimc-lite");
+ } else {
+ drv_data = fimc_lite_get_drvdata(pdev);
+ fimc->index = pdev->id;
+ }
+
+ if (!drv_data || fimc->index < 0 || fimc->index >= FIMC_LITE_MAX_DEVS)
+ return -EINVAL;
+
fimc->variant = drv_data->variant[fimc->index];
fimc->pdev = pdev;
@@ -1500,15 +1517,15 @@ static int fimc_lite_probe(struct platform_device *pdev)
mutex_init(&fimc->lock);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- fimc->regs = devm_request_and_ioremap(&pdev->dev, res);
+ fimc->regs = devm_request_and_ioremap(dev, res);
if (fimc->regs == NULL) {
- dev_err(&pdev->dev, "Failed to obtain io memory\n");
+ dev_err(dev, "Failed to obtain io memory\n");
return -ENOENT;
}
res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "Failed to get IRQ resource\n");
+ dev_err(dev, "Failed to get IRQ resource\n");
return -ENXIO;
}
@@ -1516,10 +1533,10 @@ static int fimc_lite_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = devm_request_irq(&pdev->dev, res->start, flite_irq_handler,
- 0, dev_name(&pdev->dev), fimc);
+ ret = devm_request_irq(dev, res->start, flite_irq_handler,
+ 0, dev_name(dev), fimc);
if (ret) {
- dev_err(&pdev->dev, "Failed to install irq (%d)\n", ret);
+ dev_err(dev, "Failed to install irq (%d)\n", ret);
goto err_clk;
}
@@ -1529,23 +1546,23 @@ static int fimc_lite_probe(struct platform_device *pdev)
goto err_clk;
platform_set_drvdata(pdev, fimc);
- pm_runtime_enable(&pdev->dev);
- ret = pm_runtime_get_sync(&pdev->dev);
+ pm_runtime_enable(dev);
+ ret = pm_runtime_get_sync(dev);
if (ret < 0)
goto err_sd;
- fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
+ fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
if (IS_ERR(fimc->alloc_ctx)) {
ret = PTR_ERR(fimc->alloc_ctx);
goto err_pm;
}
- pm_runtime_put(&pdev->dev);
+ pm_runtime_put(dev);
- dev_dbg(&pdev->dev, "FIMC-LITE.%d registered successfully\n",
+ dev_dbg(dev, "FIMC-LITE.%d registered successfully\n",
fimc->index);
return 0;
err_pm:
- pm_runtime_put(&pdev->dev);
+ pm_runtime_put(dev);
err_sd:
fimc_lite_unregister_capture_subdev(fimc);
err_clk:
@@ -1636,6 +1653,12 @@ static int fimc_lite_remove(struct platform_device *pdev)
return 0;
}
+static const struct dev_pm_ops fimc_lite_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(fimc_lite_suspend, fimc_lite_resume)
+ SET_RUNTIME_PM_OPS(fimc_lite_runtime_suspend, fimc_lite_runtime_resume,
+ NULL)
+};
+
static struct flite_variant fimc_lite0_variant_exynos4 = {
.max_width = 8192,
.max_height = 8192,
@@ -1661,17 +1684,21 @@ static struct platform_device_id fimc_lite_driver_ids[] = {
};
MODULE_DEVICE_TABLE(platform, fimc_lite_driver_ids);
-static const struct dev_pm_ops fimc_lite_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(fimc_lite_suspend, fimc_lite_resume)
- SET_RUNTIME_PM_OPS(fimc_lite_runtime_suspend, fimc_lite_runtime_resume,
- NULL)
+static const struct of_device_id flite_of_match[] = {
+ {
+ .compatible = "samsung,exynos4212-fimc-lite",
+ .data = &fimc_lite_drvdata_exynos4,
+ },
+ { /* sentinel */ },
};
+MODULE_DEVICE_TABLE(of, flite_of_match);
static struct platform_driver fimc_lite_driver = {
.probe = fimc_lite_probe,
.remove = fimc_lite_remove,
.id_table = fimc_lite_driver_ids,
.driver = {
+ .of_match_table = flite_of_match,
.name = FIMC_LITE_DRV_NAME,
.owner = THIS_MODULE,
.pm = &fimc_lite_pm_ops,
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 04/10] s5p-fimc: Add device tree support for the main media device driver
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-01 19:09 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-media
Cc: kyungmin.park, kgene.kim, swarren, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel,
Sylwester Nawrocki
This patch adds changes required for the main camera media device
driver to be initialized on systems instantiated from the device tree.
The platform devices corresponding to child nodes of the 'camera'
node are looked up and and registered as sub-devices to the common
media device. The main driver's probing is deferred if any of the
sub-device drivers is not yet initialized and ready.
An OF matching table is added for the main driver associated with
the 'camera' node.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/media/platform/s5p-fimc/fimc-core.c | 1 -
drivers/media/platform/s5p-fimc/fimc-mdevice.c | 77 +++++++++++++++++++++---
drivers/media/platform/s5p-fimc/fimc-mdevice.h | 4 ++
include/media/s5p_fimc.h | 1 +
4 files changed, 75 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/s5p-fimc/fimc-core.c b/drivers/media/platform/s5p-fimc/fimc-core.c
index 07ca0e0..ba743a5 100644
--- a/drivers/media/platform/s5p-fimc/fimc-core.c
+++ b/drivers/media/platform/s5p-fimc/fimc-core.c
@@ -1276,7 +1276,6 @@ static const struct platform_device_id fimc_driver_ids[] = {
},
{ },
};
-MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
static const struct of_device_id fimc_of_match[] = {
{
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index f49f6f1..c113734 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -17,6 +17,8 @@
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/types.h>
@@ -457,6 +459,51 @@ static int fimc_md_pdev_match(struct device *dev, void *data)
return 0;
}
+/* Register FIMC, FIMC-LITE and CSIS media entities */
+#ifdef CONFIG_OF
+static int fimc_md_register_of_platform_entities(struct fimc_md *fmd,
+ struct device_node *parent)
+{
+ struct device_node *node;
+ int ret = 0;
+
+ for_each_available_child_of_node(parent, node) {
+ struct platform_device *pdev;
+ int plat_entity = -1;
+
+ pdev = of_find_device_by_node(node);
+ if (!pdev)
+ continue;
+
+ /* If driver of any entity isn't ready try all again later. */
+ if (!strcmp(node->name, CSIS_OF_NODE_NAME))
+ plat_entity = IDX_CSIS;
+ else if (!strcmp(node->name, FIMC_LITE_OF_NODE_NAME))
+ plat_entity = IDX_FLITE;
+ else if (!strcmp(node->name, FIMC_OF_NODE_NAME))
+ plat_entity = IDX_FIMC;
+
+ if (plat_entity >= 0)
+ ret = fimc_md_register_platform_entity(fmd, pdev,
+ plat_entity);
+ put_device(&pdev->dev);
+ if (ret < 0)
+ break;
+
+ /* FIMC-IS child devices */
+ if (plat_entity == IDX_IS_ISP) {
+ ret = fimc_md_register_of_platform_entities(fmd, node);
+ if (ret < 0)
+ break;
+ }
+ }
+
+ return ret;
+}
+#else
+#define fimc_md_register_platform_entities(fmd) (-ENOSYS)
+#endif
+
static void fimc_md_unregister_entities(struct fimc_md *fmd)
{
int i;
@@ -948,8 +995,8 @@ static int fimc_md_probe(struct platform_device *pdev)
v4l2_dev = &fmd->v4l2_dev;
v4l2_dev->mdev = &fmd->media_dev;
v4l2_dev->notify = fimc_sensor_notify;
- snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s",
- dev_name(&pdev->dev));
+ strlcpy(v4l2_dev->name, "s5p-fimc-md", sizeof(v4l2_dev->name));
+
ret = v4l2_device_register(&pdev->dev, &fmd->v4l2_dev);
if (ret < 0) {
@@ -965,13 +1012,16 @@ static int fimc_md_probe(struct platform_device *pdev)
if (ret)
goto err_clk;
- fmd->user_subdev_api = false;
+ fmd->user_subdev_api = (dev->of_node != NULL);
/* Protect the media graph while we're registering entities */
mutex_lock(&fmd->media_dev.graph_mutex);
- ret = bus_for_each_dev(&platform_bus_type, NULL, fmd,
- fimc_md_pdev_match);
+ if (fmd->pdev->dev.of_node)
+ ret = fimc_md_register_of_platform_entities(fmd, dev->of_node);
+ else
+ ret = bus_for_each_dev(&platform_bus_type, NULL, fmd,
+ fimc_md_pdev_match);
if (ret)
goto err_unlock;
@@ -1019,12 +1069,25 @@ static int fimc_md_remove(struct platform_device *pdev)
return 0;
}
+static struct platform_device_id fimc_driver_ids[] __always_unused = {
+ { .name = "s5p-fimc-md" },
+ { },
+};
+MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
+
+static const struct of_device_id fimc_md_of_match[] __initconst = {
+ { .compatible = "samsung,fimc" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, fimc_md_of_match);
+
static struct platform_driver fimc_md_driver = {
.probe = fimc_md_probe,
.remove = fimc_md_remove,
.driver = {
- .name = "s5p-fimc-md",
- .owner = THIS_MODULE,
+ .of_match_table = fimc_md_of_match,
+ .name = "s5p-fimc-md",
+ .owner = THIS_MODULE,
}
};
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.h b/drivers/media/platform/s5p-fimc/fimc-mdevice.h
index 06b0d82..f3e0251 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.h
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.h
@@ -21,6 +21,10 @@
#include "fimc-lite.h"
#include "mipi-csis.h"
+#define FIMC_OF_NODE_NAME "fimc"
+#define FIMC_LITE_OF_NODE_NAME "fimc-lite"
+#define CSIS_OF_NODE_NAME "csis"
+
/* Group IDs of sensor, MIPI-CSIS, FIMC-LITE and the writeback subdevs. */
#define GRP_ID_SENSOR (1 << 8)
#define GRP_ID_FIMC_IS_SENSOR (1 << 9)
diff --git a/include/media/s5p_fimc.h b/include/media/s5p_fimc.h
index 28f3590..17fd2fa 100644
--- a/include/media/s5p_fimc.h
+++ b/include/media/s5p_fimc.h
@@ -81,6 +81,7 @@ enum fimc_subdev_index {
IDX_SENSOR,
IDX_CSIS,
IDX_FLITE,
+ IDX_IS_ISP,
IDX_FIMC,
IDX_MAX,
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 04/10] s5p-fimc: Add device tree support for the main media device driver
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds changes required for the main camera media device
driver to be initialized on systems instantiated from the device tree.
The platform devices corresponding to child nodes of the 'camera'
node are looked up and and registered as sub-devices to the common
media device. The main driver's probing is deferred if any of the
sub-device drivers is not yet initialized and ready.
An OF matching table is added for the main driver associated with
the 'camera' node.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/media/platform/s5p-fimc/fimc-core.c | 1 -
drivers/media/platform/s5p-fimc/fimc-mdevice.c | 77 +++++++++++++++++++++---
drivers/media/platform/s5p-fimc/fimc-mdevice.h | 4 ++
include/media/s5p_fimc.h | 1 +
4 files changed, 75 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/s5p-fimc/fimc-core.c b/drivers/media/platform/s5p-fimc/fimc-core.c
index 07ca0e0..ba743a5 100644
--- a/drivers/media/platform/s5p-fimc/fimc-core.c
+++ b/drivers/media/platform/s5p-fimc/fimc-core.c
@@ -1276,7 +1276,6 @@ static const struct platform_device_id fimc_driver_ids[] = {
},
{ },
};
-MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
static const struct of_device_id fimc_of_match[] = {
{
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index f49f6f1..c113734 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -17,6 +17,8 @@
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/types.h>
@@ -457,6 +459,51 @@ static int fimc_md_pdev_match(struct device *dev, void *data)
return 0;
}
+/* Register FIMC, FIMC-LITE and CSIS media entities */
+#ifdef CONFIG_OF
+static int fimc_md_register_of_platform_entities(struct fimc_md *fmd,
+ struct device_node *parent)
+{
+ struct device_node *node;
+ int ret = 0;
+
+ for_each_available_child_of_node(parent, node) {
+ struct platform_device *pdev;
+ int plat_entity = -1;
+
+ pdev = of_find_device_by_node(node);
+ if (!pdev)
+ continue;
+
+ /* If driver of any entity isn't ready try all again later. */
+ if (!strcmp(node->name, CSIS_OF_NODE_NAME))
+ plat_entity = IDX_CSIS;
+ else if (!strcmp(node->name, FIMC_LITE_OF_NODE_NAME))
+ plat_entity = IDX_FLITE;
+ else if (!strcmp(node->name, FIMC_OF_NODE_NAME))
+ plat_entity = IDX_FIMC;
+
+ if (plat_entity >= 0)
+ ret = fimc_md_register_platform_entity(fmd, pdev,
+ plat_entity);
+ put_device(&pdev->dev);
+ if (ret < 0)
+ break;
+
+ /* FIMC-IS child devices */
+ if (plat_entity == IDX_IS_ISP) {
+ ret = fimc_md_register_of_platform_entities(fmd, node);
+ if (ret < 0)
+ break;
+ }
+ }
+
+ return ret;
+}
+#else
+#define fimc_md_register_platform_entities(fmd) (-ENOSYS)
+#endif
+
static void fimc_md_unregister_entities(struct fimc_md *fmd)
{
int i;
@@ -948,8 +995,8 @@ static int fimc_md_probe(struct platform_device *pdev)
v4l2_dev = &fmd->v4l2_dev;
v4l2_dev->mdev = &fmd->media_dev;
v4l2_dev->notify = fimc_sensor_notify;
- snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s",
- dev_name(&pdev->dev));
+ strlcpy(v4l2_dev->name, "s5p-fimc-md", sizeof(v4l2_dev->name));
+
ret = v4l2_device_register(&pdev->dev, &fmd->v4l2_dev);
if (ret < 0) {
@@ -965,13 +1012,16 @@ static int fimc_md_probe(struct platform_device *pdev)
if (ret)
goto err_clk;
- fmd->user_subdev_api = false;
+ fmd->user_subdev_api = (dev->of_node != NULL);
/* Protect the media graph while we're registering entities */
mutex_lock(&fmd->media_dev.graph_mutex);
- ret = bus_for_each_dev(&platform_bus_type, NULL, fmd,
- fimc_md_pdev_match);
+ if (fmd->pdev->dev.of_node)
+ ret = fimc_md_register_of_platform_entities(fmd, dev->of_node);
+ else
+ ret = bus_for_each_dev(&platform_bus_type, NULL, fmd,
+ fimc_md_pdev_match);
if (ret)
goto err_unlock;
@@ -1019,12 +1069,25 @@ static int fimc_md_remove(struct platform_device *pdev)
return 0;
}
+static struct platform_device_id fimc_driver_ids[] __always_unused = {
+ { .name = "s5p-fimc-md" },
+ { },
+};
+MODULE_DEVICE_TABLE(platform, fimc_driver_ids);
+
+static const struct of_device_id fimc_md_of_match[] __initconst = {
+ { .compatible = "samsung,fimc" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, fimc_md_of_match);
+
static struct platform_driver fimc_md_driver = {
.probe = fimc_md_probe,
.remove = fimc_md_remove,
.driver = {
- .name = "s5p-fimc-md",
- .owner = THIS_MODULE,
+ .of_match_table = fimc_md_of_match,
+ .name = "s5p-fimc-md",
+ .owner = THIS_MODULE,
}
};
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.h b/drivers/media/platform/s5p-fimc/fimc-mdevice.h
index 06b0d82..f3e0251 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.h
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.h
@@ -21,6 +21,10 @@
#include "fimc-lite.h"
#include "mipi-csis.h"
+#define FIMC_OF_NODE_NAME "fimc"
+#define FIMC_LITE_OF_NODE_NAME "fimc-lite"
+#define CSIS_OF_NODE_NAME "csis"
+
/* Group IDs of sensor, MIPI-CSIS, FIMC-LITE and the writeback subdevs. */
#define GRP_ID_SENSOR (1 << 8)
#define GRP_ID_FIMC_IS_SENSOR (1 << 9)
diff --git a/include/media/s5p_fimc.h b/include/media/s5p_fimc.h
index 28f3590..17fd2fa 100644
--- a/include/media/s5p_fimc.h
+++ b/include/media/s5p_fimc.h
@@ -81,6 +81,7 @@ enum fimc_subdev_index {
IDX_SENSOR,
IDX_CSIS,
IDX_FLITE,
+ IDX_IS_ISP,
IDX_FIMC,
IDX_MAX,
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 05/10] s5p-fimc: Add device tree based sensors registration
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-01 19:09 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-media
Cc: kyungmin.park, kgene.kim, swarren, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel,
Sylwester Nawrocki
The sensor (I2C and/or SPI client) devices are instantiated by their
corresponding control bus drivers. Since the I2C client's master clock
is often provided by a video bus receiver (host interface) or other
than I2C/SPI controller device, the drivers of those client devices
are not accessing hardware in their driver's probe() callback. Instead,
after enabling clock, the host driver calls back into a sub-device
when it wants to activate them. This pattern is used by some in-tree
drivers and this patch also uses it for DT case. This patch is intended
as a first step for adding device tree support to the S5P/Exynos SoC
camera drivers. The second one is adding support for asynchronous
sub-devices registration and clock control from sub-device driver
level. The bindings shall not change when asynchronous probing support
is added.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
.../devicetree/bindings/media/soc/samsung-fimc.txt | 89 ++++++++
drivers/media/platform/s5p-fimc/fimc-mdevice.c | 231 +++++++++++++++++---
include/media/s5p_fimc.h | 16 ++
3 files changed, 311 insertions(+), 25 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
index 2b932f2..6b81ad1 100644
--- a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -56,6 +56,29 @@ For every fimc-lite node a numbered alias should be present in the aliases
node. Aliases are in form of fimc-lite<n>, where <n> is an integer (0...N)
specifying the IP's instance index.
+
+'parallel-ports' node
+---------------------
+
+This node should contain child 'port' nodes specifying active parallel video
+input ports. It includes camera A and camera B inputs. 'reg' property in the
+port nodes specifies data input - 0, 1 indicates input A, B respectively.
+
+Optional properties
+
+- samsung,camclk-out : specifies clock output for remote sensor,
+ 0 - CAM_A_CLKOUT, 1 - CAM_B_CLKOUT;
+
+Image sensor nodes
+------------------
+
+The sensor device nodes should be added as their control bus controller
+(e.g. I2C0) child nodes and linked to a port node in the csis or parallel-ports
+node, using common the common video interfaces bindings, i.e. port/endpoint
+node pairs. The implementation of this binding requires clock-frequency
+property to be present in the sensor device nodes.
+
+
Example:
aliases {
@@ -63,12 +86,68 @@ Example:
fimc0 = &fimc_0;
};
+ /* Parallel bus IF sensor */
+ i2c_0: i2c@13860000 {
+ s5k6aa: sensor@3c {
+ compatible = "samsung,s5k6aafx";
+ reg = <0x3c>;
+ vddio-supply = <...>;
+
+ clock-frequency = <24000000>;
+ clocks = <...>;
+ clock-names = "mclk";
+
+ port {
+ s5k6aa_ep: endpoint {
+ remote-endpoint = <&fimc0_ep>;
+ bus-width = <8>;
+ hsync-active = <0>;
+ hsync-active = <1>;
+ pclk-sample = <1>;
+ };
+ };
+ };
+ };
+
+ /* MIPI CSI-2 bus IF sensor */
+ s5c73m3: sensor@0x1a {
+ compatible = "samsung,s5c73m3";
+ reg = <0x1a>;
+ vddio-supply = <...>;
+
+ clock-frequency = <24000000>;
+ clocks = <...>;
+ clock-names = "mclk";
+
+ port {
+ s5c73m3_1: endpoint {
+ data-lanes = <1>, <2>, <3>, <4>;
+ remote-endpoint = <&csis0_ep>;
+ };
+ };
+ };
+
camera {
compatible = "samsung,fimc", "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
status = "okay";
+ /* parallel camera ports */
+ parallel-ports {
+ /* camera A input */
+ port@0 {
+ reg = <1>;
+ fimc0_ep: endpoint {
+ remote-endpoint = <&s5k6aa_ep>;
+ bus-width = <8>;
+ hsync-active = <0>;
+ hsync-active = <1>;
+ pclk-sample = <1>;
+ };
+ };
+ };
+
fimc_0: fimc@11800000 {
compatible = "samsung,exynos4210-fimc";
reg = <0x11800000 0x1000>;
@@ -81,6 +160,16 @@ Example:
reg = <0x11880000 0x1000>;
interrupts = <0 78 0>;
max-data-lanes = <4>;
+ /* camera C input */
+ port {
+ reg = <3>;
+ csis0_ep: endpoint {
+ remote-endpoint = <&s5c73m3_ep>;
+ data-lanes = <1>, <2>, <3>, <4>;
+ samsung,csis-hs-settle = <12>;
+ samsung,camclk-out = <0>;
+ };
+ };
};
};
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index c113734..2bb501f 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -19,11 +19,14 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_platform.h>
+#include <linux/of_device.h>
+#include <linux/of_i2c.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/types.h>
#include <linux/slab.h>
#include <media/v4l2-ctrls.h>
+#include <media/v4l2-of.h>
#include <media/media-device.h>
#include <media/s5p_fimc.h>
@@ -248,7 +251,7 @@ static struct v4l2_subdev *fimc_md_register_sensor(struct fimc_md *fmd,
sd->grp_id = GRP_ID_SENSOR;
v4l2_info(&fmd->v4l2_dev, "Registered sensor subdevice %s\n",
- s_info->pdata.board_info->type);
+ sd->name);
return sd;
}
@@ -260,17 +263,185 @@ static void fimc_md_unregister_sensor(struct v4l2_subdev *sd)
if (!client)
return;
v4l2_device_unregister_subdev(sd);
- adapter = client->adapter;
- i2c_unregister_device(client);
- if (adapter)
- i2c_put_adapter(adapter);
+
+ if (!client->dev.of_node) {
+ adapter = client->adapter;
+ i2c_unregister_device(client);
+ if (adapter)
+ i2c_put_adapter(adapter);
+ }
+}
+
+#ifdef CONFIG_OF
+/* Register I2C client subdev associated with @node. */
+static int fimc_md_of_add_sensor(struct fimc_md *fmd,
+ struct device_node *node, int index)
+{
+ struct fimc_sensor_info *si;
+ struct i2c_client *client;
+ struct v4l2_subdev *sd;
+ int ret;
+
+ if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor)))
+ return -EINVAL;
+ si = &fmd->sensor[index];
+
+ client = of_find_i2c_device_by_node(node);
+ if (!client)
+ return -EPROBE_DEFER;
+
+ device_lock(&client->dev);
+
+ if (!client->driver ||
+ !try_module_get(client->driver->driver.owner)) {
+ ret = -EPROBE_DEFER;
+ goto dev_put;
+ }
+
+ /* Enable sensor's master clock */
+ ret = __fimc_md_set_camclk(fmd, si, true);
+ if (ret < 0)
+ goto mod_put;
+ sd = i2c_get_clientdata(client);
+
+ ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
+ __fimc_md_set_camclk(fmd, si, false);
+ if (ret < 0)
+ goto mod_put;
+
+ v4l2_set_subdev_hostdata(sd, si);
+ sd->grp_id = GRP_ID_SENSOR;
+ si->subdev = sd;
+ v4l2_info(&fmd->v4l2_dev, "Registered sensor subdevice: %s (%d)\n",
+ sd->name, fmd->num_sensors);
+ fmd->num_sensors++;
+
+mod_put:
+ module_put(client->driver->driver.owner);
+dev_put:
+ device_unlock(&client->dev);
+ put_device(&client->dev);
+ return ret;
}
+/* Parse port node and register as a sub-device any sensor specified there. */
+static int fimc_md_parse_port_node(struct fimc_md *fmd,
+ struct device_node *port,
+ unsigned int index)
+{
+ struct device_node *rem, *endpoint;
+ struct fimc_source_info *pd;
+ struct v4l2_of_endpoint bus_cfg;
+ u32 tmp, reg = 0;
+ int ret;
+
+ if (WARN_ON(of_property_read_u32(port, "reg", ®) ||
+ reg >= FIMC_MAX_SENSORS))
+ return -EINVAL;
+
+ pd = &fmd->sensor[index].pdata;
+ pd->mux_id = (reg - 1) & 0x1;
+
+ /* Assume here a port node can have only one endpoint node. */
+ endpoint = of_get_next_child(port, NULL);
+ if (!endpoint)
+ return 0;
+
+ rem = v4l2_of_get_remote_port_parent(endpoint);
+ of_node_put(endpoint);
+ if (rem == NULL) {
+ v4l2_info(&fmd->v4l2_dev, "Remote device at %s not found\n",
+ endpoint->full_name);
+ return 0;
+ }
+ if (!of_property_read_u32(rem, "samsung,camclk-out", &tmp))
+ pd->clk_id = tmp;
+
+ if (!of_property_read_u32(rem, "clock-frequency", &tmp))
+ pd->clk_frequency = tmp;
+
+ if (pd->clk_frequency == 0) {
+ v4l2_err(&fmd->v4l2_dev, "Wrong clock frequency at node %s\n",
+ rem->full_name);
+ of_node_put(rem);
+ return -EINVAL;
+ }
+
+ if (fimc_input_is_parallel(reg)) {
+ v4l2_of_parse_parallel_bus(endpoint, &bus_cfg);
+ if (bus_cfg.mbus.type == V4L2_MBUS_PARALLEL)
+ pd->sensor_bus_type = FIMC_BUS_TYPE_ITU_601;
+ else
+ pd->sensor_bus_type = FIMC_BUS_TYPE_ITU_656;
+ pd->flags = bus_cfg.mbus.flags;
+ } else if (fimc_input_is_mipi_csi(reg)) {
+ /*
+ * MIPI CSI-2: only input mux selection
+ * and sensor's clock frequency is needed.
+ */
+ pd->sensor_bus_type = FIMC_BUS_TYPE_MIPI_CSI2;
+ } else {
+ v4l2_err(&fmd->v4l2_dev, "Wrong port id (%u) at node %s\n",
+ reg, rem->full_name);
+ }
+
+ ret = fimc_md_of_add_sensor(fmd, rem, index);
+ of_node_put(rem);
+
+ return ret;
+}
+
+/* Register all SoC external sub-devices */
+static int fimc_md_of_sensors_register(struct fimc_md *fmd,
+ struct device_node *np)
+{
+ struct device_node *parent = fmd->pdev->dev.of_node;
+ struct device_node *node, *ports;
+ int index = 0;
+ int ret;
+
+ /* Attach sensors linked to MIPI CSI-2 receivers */
+ for_each_available_child_of_node(parent, node) {
+ struct device_node *port;
+
+ if (of_node_cmp(node->name, "csis"))
+ continue;
+ /* The csis node can have only port subnode. */
+ port = of_get_next_child(node, NULL);
+ if (!port)
+ continue;
+
+ ret = fimc_md_parse_port_node(fmd, port, index);
+ if (ret < 0)
+ return ret;
+ index++;
+ }
+
+ /* Attach sensors listed in the parallel-ports node */
+ ports = of_get_child_by_name(parent, "parallel-ports");
+ if (!ports)
+ return 0;
+
+ for_each_child_of_node(ports, node) {
+ ret = fimc_md_parse_port_node(fmd, node, index);
+ if (ret < 0)
+ break;
+ index++;
+ }
+
+ return 0;
+}
+#else
+#define fimc_md_of_sensors_register(fmd, np) (-ENOSYS)
+#endif
+
static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
{
struct s5p_platform_fimc *pdata = fmd->pdev->dev.platform_data;
+ struct device_node *of_node = fmd->pdev->dev.of_node;
struct fimc_dev *fd = NULL;
- int num_clients, ret, i;
+ int num_clients = 0;
+ int ret, i;
/*
* Runtime resume one of the FIMC entities to make sure
@@ -281,34 +452,42 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
fd = fmd->fimc[i];
if (!fd)
return -ENXIO;
+
ret = pm_runtime_get_sync(&fd->pdev->dev);
if (ret < 0)
return ret;
- WARN_ON(pdata->num_clients > ARRAY_SIZE(fmd->sensor));
- num_clients = min_t(u32, pdata->num_clients, ARRAY_SIZE(fmd->sensor));
+ if (of_node) {
+ fmd->num_sensors = 0;
+ ret = fimc_md_of_sensors_register(fmd, of_node);
+ } else if (pdata) {
+ WARN_ON(pdata->num_clients > ARRAY_SIZE(fmd->sensor));
+ num_clients = min_t(u32, pdata->num_clients,
+ ARRAY_SIZE(fmd->sensor));
+ fmd->num_sensors = num_clients;
fmd->num_sensors = num_clients;
for (i = 0; i < num_clients; i++) {
struct v4l2_subdev *sd;
- fmd->sensor[i].pdata = pdata->source_info[i];
- ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], true);
- if (ret)
- break;
- sd = fimc_md_register_sensor(fmd, &fmd->sensor[i]);
- ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], false);
+ fmd->sensor[i].pdata = pdata->source_info[i];
+ ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], true);
+ if (ret)
+ break;
+ sd = fimc_md_register_sensor(fmd, &fmd->sensor[i]);
+ ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], false);
- if (!IS_ERR(sd)) {
+ if (IS_ERR(sd)) {
+ fmd->sensor[i].subdev = NULL;
+ ret = PTR_ERR(sd);
+ break;
+ }
fmd->sensor[i].subdev = sd;
- } else {
- fmd->sensor[i].subdev = NULL;
- ret = PTR_ERR(sd);
- break;
+ if (ret)
+ break;
}
- if (ret)
- break;
}
+
pm_runtime_put(&fd->pdev->dev);
return ret;
}
@@ -976,11 +1155,12 @@ static DEVICE_ATTR(subdev_conf_mode, S_IWUSR | S_IRUGO,
static int fimc_md_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct v4l2_device *v4l2_dev;
struct fimc_md *fmd;
int ret;
- fmd = devm_kzalloc(&pdev->dev, sizeof(*fmd), GFP_KERNEL);
+ fmd = devm_kzalloc(dev, sizeof(*fmd), GFP_KERNEL);
if (!fmd)
return -ENOMEM;
@@ -990,7 +1170,7 @@ static int fimc_md_probe(struct platform_device *pdev)
strlcpy(fmd->media_dev.model, "SAMSUNG S5P FIMC",
sizeof(fmd->media_dev.model));
fmd->media_dev.link_notify = fimc_md_link_notify;
- fmd->media_dev.dev = &pdev->dev;
+ fmd->media_dev.dev = dev;
v4l2_dev = &fmd->v4l2_dev;
v4l2_dev->mdev = &fmd->media_dev;
@@ -998,7 +1178,7 @@ static int fimc_md_probe(struct platform_device *pdev)
strlcpy(v4l2_dev->name, "s5p-fimc-md", sizeof(v4l2_dev->name));
- ret = v4l2_device_register(&pdev->dev, &fmd->v4l2_dev);
+ ret = v4l2_device_register(dev, &fmd->v4l2_dev);
if (ret < 0) {
v4l2_err(v4l2_dev, "Failed to register v4l2_device: %d\n", ret);
return ret;
@@ -1025,11 +1205,12 @@ static int fimc_md_probe(struct platform_device *pdev)
if (ret)
goto err_unlock;
- if (pdev->dev.platform_data) {
+ if (dev->platform_data || dev->of_node) {
ret = fimc_md_register_sensor_entities(fmd);
if (ret)
goto err_unlock;
}
+
ret = fimc_md_create_links(fmd);
if (ret)
goto err_unlock;
diff --git a/include/media/s5p_fimc.h b/include/media/s5p_fimc.h
index 17fd2fa..e2434bb 100644
--- a/include/media/s5p_fimc.h
+++ b/include/media/s5p_fimc.h
@@ -15,6 +15,19 @@
#include <media/media-entity.h>
/*
+ * Enumeration of data inputs to the camera subsystem.
+ */
+enum fimc_input {
+ FIMC_INPUT_PARALLEL_0 = 1,
+ FIMC_INPUT_PARALLEL_1,
+ FIMC_INPUT_MIPI_CSI2_0 = 3,
+ FIMC_INPUT_MIPI_CSI2_1,
+ FIMC_INPUT_WRITEBACK_A = 5,
+ FIMC_INPUT_WRITEBACK_B,
+ FIMC_INPUT_WRITEBACK_ISP = 5,
+};
+
+/*
* Enumeration of the FIMC data bus types.
*/
enum fimc_bus_type {
@@ -32,6 +45,9 @@ enum fimc_bus_type {
FIMC_BUS_TYPE_ISP_WRITEBACK = FIMC_BUS_TYPE_LCD_WRITEBACK_B,
};
+#define fimc_input_is_parallel(x) ((x) == 1 || (x) == 2)
+#define fimc_input_is_mipi_csi(x) ((x) == 3 || (x) == 4)
+
struct i2c_board_info;
/**
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 05/10] s5p-fimc: Add device tree based sensors registration
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-arm-kernel
The sensor (I2C and/or SPI client) devices are instantiated by their
corresponding control bus drivers. Since the I2C client's master clock
is often provided by a video bus receiver (host interface) or other
than I2C/SPI controller device, the drivers of those client devices
are not accessing hardware in their driver's probe() callback. Instead,
after enabling clock, the host driver calls back into a sub-device
when it wants to activate them. This pattern is used by some in-tree
drivers and this patch also uses it for DT case. This patch is intended
as a first step for adding device tree support to the S5P/Exynos SoC
camera drivers. The second one is adding support for asynchronous
sub-devices registration and clock control from sub-device driver
level. The bindings shall not change when asynchronous probing support
is added.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
.../devicetree/bindings/media/soc/samsung-fimc.txt | 89 ++++++++
drivers/media/platform/s5p-fimc/fimc-mdevice.c | 231 +++++++++++++++++---
include/media/s5p_fimc.h | 16 ++
3 files changed, 311 insertions(+), 25 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
index 2b932f2..6b81ad1 100644
--- a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -56,6 +56,29 @@ For every fimc-lite node a numbered alias should be present in the aliases
node. Aliases are in form of fimc-lite<n>, where <n> is an integer (0...N)
specifying the IP's instance index.
+
+'parallel-ports' node
+---------------------
+
+This node should contain child 'port' nodes specifying active parallel video
+input ports. It includes camera A and camera B inputs. 'reg' property in the
+port nodes specifies data input - 0, 1 indicates input A, B respectively.
+
+Optional properties
+
+- samsung,camclk-out : specifies clock output for remote sensor,
+ 0 - CAM_A_CLKOUT, 1 - CAM_B_CLKOUT;
+
+Image sensor nodes
+------------------
+
+The sensor device nodes should be added as their control bus controller
+(e.g. I2C0) child nodes and linked to a port node in the csis or parallel-ports
+node, using common the common video interfaces bindings, i.e. port/endpoint
+node pairs. The implementation of this binding requires clock-frequency
+property to be present in the sensor device nodes.
+
+
Example:
aliases {
@@ -63,12 +86,68 @@ Example:
fimc0 = &fimc_0;
};
+ /* Parallel bus IF sensor */
+ i2c_0: i2c at 13860000 {
+ s5k6aa: sensor at 3c {
+ compatible = "samsung,s5k6aafx";
+ reg = <0x3c>;
+ vddio-supply = <...>;
+
+ clock-frequency = <24000000>;
+ clocks = <...>;
+ clock-names = "mclk";
+
+ port {
+ s5k6aa_ep: endpoint {
+ remote-endpoint = <&fimc0_ep>;
+ bus-width = <8>;
+ hsync-active = <0>;
+ hsync-active = <1>;
+ pclk-sample = <1>;
+ };
+ };
+ };
+ };
+
+ /* MIPI CSI-2 bus IF sensor */
+ s5c73m3: sensor at 0x1a {
+ compatible = "samsung,s5c73m3";
+ reg = <0x1a>;
+ vddio-supply = <...>;
+
+ clock-frequency = <24000000>;
+ clocks = <...>;
+ clock-names = "mclk";
+
+ port {
+ s5c73m3_1: endpoint {
+ data-lanes = <1>, <2>, <3>, <4>;
+ remote-endpoint = <&csis0_ep>;
+ };
+ };
+ };
+
camera {
compatible = "samsung,fimc", "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
status = "okay";
+ /* parallel camera ports */
+ parallel-ports {
+ /* camera A input */
+ port at 0 {
+ reg = <1>;
+ fimc0_ep: endpoint {
+ remote-endpoint = <&s5k6aa_ep>;
+ bus-width = <8>;
+ hsync-active = <0>;
+ hsync-active = <1>;
+ pclk-sample = <1>;
+ };
+ };
+ };
+
fimc_0: fimc at 11800000 {
compatible = "samsung,exynos4210-fimc";
reg = <0x11800000 0x1000>;
@@ -81,6 +160,16 @@ Example:
reg = <0x11880000 0x1000>;
interrupts = <0 78 0>;
max-data-lanes = <4>;
+ /* camera C input */
+ port {
+ reg = <3>;
+ csis0_ep: endpoint {
+ remote-endpoint = <&s5c73m3_ep>;
+ data-lanes = <1>, <2>, <3>, <4>;
+ samsung,csis-hs-settle = <12>;
+ samsung,camclk-out = <0>;
+ };
+ };
};
};
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index c113734..2bb501f 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -19,11 +19,14 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_platform.h>
+#include <linux/of_device.h>
+#include <linux/of_i2c.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/types.h>
#include <linux/slab.h>
#include <media/v4l2-ctrls.h>
+#include <media/v4l2-of.h>
#include <media/media-device.h>
#include <media/s5p_fimc.h>
@@ -248,7 +251,7 @@ static struct v4l2_subdev *fimc_md_register_sensor(struct fimc_md *fmd,
sd->grp_id = GRP_ID_SENSOR;
v4l2_info(&fmd->v4l2_dev, "Registered sensor subdevice %s\n",
- s_info->pdata.board_info->type);
+ sd->name);
return sd;
}
@@ -260,17 +263,185 @@ static void fimc_md_unregister_sensor(struct v4l2_subdev *sd)
if (!client)
return;
v4l2_device_unregister_subdev(sd);
- adapter = client->adapter;
- i2c_unregister_device(client);
- if (adapter)
- i2c_put_adapter(adapter);
+
+ if (!client->dev.of_node) {
+ adapter = client->adapter;
+ i2c_unregister_device(client);
+ if (adapter)
+ i2c_put_adapter(adapter);
+ }
+}
+
+#ifdef CONFIG_OF
+/* Register I2C client subdev associated with @node. */
+static int fimc_md_of_add_sensor(struct fimc_md *fmd,
+ struct device_node *node, int index)
+{
+ struct fimc_sensor_info *si;
+ struct i2c_client *client;
+ struct v4l2_subdev *sd;
+ int ret;
+
+ if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor)))
+ return -EINVAL;
+ si = &fmd->sensor[index];
+
+ client = of_find_i2c_device_by_node(node);
+ if (!client)
+ return -EPROBE_DEFER;
+
+ device_lock(&client->dev);
+
+ if (!client->driver ||
+ !try_module_get(client->driver->driver.owner)) {
+ ret = -EPROBE_DEFER;
+ goto dev_put;
+ }
+
+ /* Enable sensor's master clock */
+ ret = __fimc_md_set_camclk(fmd, si, true);
+ if (ret < 0)
+ goto mod_put;
+ sd = i2c_get_clientdata(client);
+
+ ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
+ __fimc_md_set_camclk(fmd, si, false);
+ if (ret < 0)
+ goto mod_put;
+
+ v4l2_set_subdev_hostdata(sd, si);
+ sd->grp_id = GRP_ID_SENSOR;
+ si->subdev = sd;
+ v4l2_info(&fmd->v4l2_dev, "Registered sensor subdevice: %s (%d)\n",
+ sd->name, fmd->num_sensors);
+ fmd->num_sensors++;
+
+mod_put:
+ module_put(client->driver->driver.owner);
+dev_put:
+ device_unlock(&client->dev);
+ put_device(&client->dev);
+ return ret;
}
+/* Parse port node and register as a sub-device any sensor specified there. */
+static int fimc_md_parse_port_node(struct fimc_md *fmd,
+ struct device_node *port,
+ unsigned int index)
+{
+ struct device_node *rem, *endpoint;
+ struct fimc_source_info *pd;
+ struct v4l2_of_endpoint bus_cfg;
+ u32 tmp, reg = 0;
+ int ret;
+
+ if (WARN_ON(of_property_read_u32(port, "reg", ®) ||
+ reg >= FIMC_MAX_SENSORS))
+ return -EINVAL;
+
+ pd = &fmd->sensor[index].pdata;
+ pd->mux_id = (reg - 1) & 0x1;
+
+ /* Assume here a port node can have only one endpoint node. */
+ endpoint = of_get_next_child(port, NULL);
+ if (!endpoint)
+ return 0;
+
+ rem = v4l2_of_get_remote_port_parent(endpoint);
+ of_node_put(endpoint);
+ if (rem == NULL) {
+ v4l2_info(&fmd->v4l2_dev, "Remote device at %s not found\n",
+ endpoint->full_name);
+ return 0;
+ }
+ if (!of_property_read_u32(rem, "samsung,camclk-out", &tmp))
+ pd->clk_id = tmp;
+
+ if (!of_property_read_u32(rem, "clock-frequency", &tmp))
+ pd->clk_frequency = tmp;
+
+ if (pd->clk_frequency == 0) {
+ v4l2_err(&fmd->v4l2_dev, "Wrong clock frequency at node %s\n",
+ rem->full_name);
+ of_node_put(rem);
+ return -EINVAL;
+ }
+
+ if (fimc_input_is_parallel(reg)) {
+ v4l2_of_parse_parallel_bus(endpoint, &bus_cfg);
+ if (bus_cfg.mbus.type == V4L2_MBUS_PARALLEL)
+ pd->sensor_bus_type = FIMC_BUS_TYPE_ITU_601;
+ else
+ pd->sensor_bus_type = FIMC_BUS_TYPE_ITU_656;
+ pd->flags = bus_cfg.mbus.flags;
+ } else if (fimc_input_is_mipi_csi(reg)) {
+ /*
+ * MIPI CSI-2: only input mux selection
+ * and sensor's clock frequency is needed.
+ */
+ pd->sensor_bus_type = FIMC_BUS_TYPE_MIPI_CSI2;
+ } else {
+ v4l2_err(&fmd->v4l2_dev, "Wrong port id (%u) at node %s\n",
+ reg, rem->full_name);
+ }
+
+ ret = fimc_md_of_add_sensor(fmd, rem, index);
+ of_node_put(rem);
+
+ return ret;
+}
+
+/* Register all SoC external sub-devices */
+static int fimc_md_of_sensors_register(struct fimc_md *fmd,
+ struct device_node *np)
+{
+ struct device_node *parent = fmd->pdev->dev.of_node;
+ struct device_node *node, *ports;
+ int index = 0;
+ int ret;
+
+ /* Attach sensors linked to MIPI CSI-2 receivers */
+ for_each_available_child_of_node(parent, node) {
+ struct device_node *port;
+
+ if (of_node_cmp(node->name, "csis"))
+ continue;
+ /* The csis node can have only port subnode. */
+ port = of_get_next_child(node, NULL);
+ if (!port)
+ continue;
+
+ ret = fimc_md_parse_port_node(fmd, port, index);
+ if (ret < 0)
+ return ret;
+ index++;
+ }
+
+ /* Attach sensors listed in the parallel-ports node */
+ ports = of_get_child_by_name(parent, "parallel-ports");
+ if (!ports)
+ return 0;
+
+ for_each_child_of_node(ports, node) {
+ ret = fimc_md_parse_port_node(fmd, node, index);
+ if (ret < 0)
+ break;
+ index++;
+ }
+
+ return 0;
+}
+#else
+#define fimc_md_of_sensors_register(fmd, np) (-ENOSYS)
+#endif
+
static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
{
struct s5p_platform_fimc *pdata = fmd->pdev->dev.platform_data;
+ struct device_node *of_node = fmd->pdev->dev.of_node;
struct fimc_dev *fd = NULL;
- int num_clients, ret, i;
+ int num_clients = 0;
+ int ret, i;
/*
* Runtime resume one of the FIMC entities to make sure
@@ -281,34 +452,42 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
fd = fmd->fimc[i];
if (!fd)
return -ENXIO;
+
ret = pm_runtime_get_sync(&fd->pdev->dev);
if (ret < 0)
return ret;
- WARN_ON(pdata->num_clients > ARRAY_SIZE(fmd->sensor));
- num_clients = min_t(u32, pdata->num_clients, ARRAY_SIZE(fmd->sensor));
+ if (of_node) {
+ fmd->num_sensors = 0;
+ ret = fimc_md_of_sensors_register(fmd, of_node);
+ } else if (pdata) {
+ WARN_ON(pdata->num_clients > ARRAY_SIZE(fmd->sensor));
+ num_clients = min_t(u32, pdata->num_clients,
+ ARRAY_SIZE(fmd->sensor));
+ fmd->num_sensors = num_clients;
fmd->num_sensors = num_clients;
for (i = 0; i < num_clients; i++) {
struct v4l2_subdev *sd;
- fmd->sensor[i].pdata = pdata->source_info[i];
- ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], true);
- if (ret)
- break;
- sd = fimc_md_register_sensor(fmd, &fmd->sensor[i]);
- ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], false);
+ fmd->sensor[i].pdata = pdata->source_info[i];
+ ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], true);
+ if (ret)
+ break;
+ sd = fimc_md_register_sensor(fmd, &fmd->sensor[i]);
+ ret = __fimc_md_set_camclk(fmd, &fmd->sensor[i], false);
- if (!IS_ERR(sd)) {
+ if (IS_ERR(sd)) {
+ fmd->sensor[i].subdev = NULL;
+ ret = PTR_ERR(sd);
+ break;
+ }
fmd->sensor[i].subdev = sd;
- } else {
- fmd->sensor[i].subdev = NULL;
- ret = PTR_ERR(sd);
- break;
+ if (ret)
+ break;
}
- if (ret)
- break;
}
+
pm_runtime_put(&fd->pdev->dev);
return ret;
}
@@ -976,11 +1155,12 @@ static DEVICE_ATTR(subdev_conf_mode, S_IWUSR | S_IRUGO,
static int fimc_md_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct v4l2_device *v4l2_dev;
struct fimc_md *fmd;
int ret;
- fmd = devm_kzalloc(&pdev->dev, sizeof(*fmd), GFP_KERNEL);
+ fmd = devm_kzalloc(dev, sizeof(*fmd), GFP_KERNEL);
if (!fmd)
return -ENOMEM;
@@ -990,7 +1170,7 @@ static int fimc_md_probe(struct platform_device *pdev)
strlcpy(fmd->media_dev.model, "SAMSUNG S5P FIMC",
sizeof(fmd->media_dev.model));
fmd->media_dev.link_notify = fimc_md_link_notify;
- fmd->media_dev.dev = &pdev->dev;
+ fmd->media_dev.dev = dev;
v4l2_dev = &fmd->v4l2_dev;
v4l2_dev->mdev = &fmd->media_dev;
@@ -998,7 +1178,7 @@ static int fimc_md_probe(struct platform_device *pdev)
strlcpy(v4l2_dev->name, "s5p-fimc-md", sizeof(v4l2_dev->name));
- ret = v4l2_device_register(&pdev->dev, &fmd->v4l2_dev);
+ ret = v4l2_device_register(dev, &fmd->v4l2_dev);
if (ret < 0) {
v4l2_err(v4l2_dev, "Failed to register v4l2_device: %d\n", ret);
return ret;
@@ -1025,11 +1205,12 @@ static int fimc_md_probe(struct platform_device *pdev)
if (ret)
goto err_unlock;
- if (pdev->dev.platform_data) {
+ if (dev->platform_data || dev->of_node) {
ret = fimc_md_register_sensor_entities(fmd);
if (ret)
goto err_unlock;
}
+
ret = fimc_md_create_links(fmd);
if (ret)
goto err_unlock;
diff --git a/include/media/s5p_fimc.h b/include/media/s5p_fimc.h
index 17fd2fa..e2434bb 100644
--- a/include/media/s5p_fimc.h
+++ b/include/media/s5p_fimc.h
@@ -15,6 +15,19 @@
#include <media/media-entity.h>
/*
+ * Enumeration of data inputs to the camera subsystem.
+ */
+enum fimc_input {
+ FIMC_INPUT_PARALLEL_0 = 1,
+ FIMC_INPUT_PARALLEL_1,
+ FIMC_INPUT_MIPI_CSI2_0 = 3,
+ FIMC_INPUT_MIPI_CSI2_1,
+ FIMC_INPUT_WRITEBACK_A = 5,
+ FIMC_INPUT_WRITEBACK_B,
+ FIMC_INPUT_WRITEBACK_ISP = 5,
+};
+
+/*
* Enumeration of the FIMC data bus types.
*/
enum fimc_bus_type {
@@ -32,6 +45,9 @@ enum fimc_bus_type {
FIMC_BUS_TYPE_ISP_WRITEBACK = FIMC_BUS_TYPE_LCD_WRITEBACK_B,
};
+#define fimc_input_is_parallel(x) ((x) == 1 || (x) == 2)
+#define fimc_input_is_mipi_csi(x) ((x) == 3 || (x) == 4)
+
struct i2c_board_info;
/**
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 06/10] s5p-fimc: Use pinctrl API for camera ports configuration
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-01 19:09 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-media
Cc: kyungmin.park, kgene.kim, swarren, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel,
Sylwester Nawrocki
Before the camera ports can be used the pinmux needs to be configured
properly. This patch adds a function to set the camera ports pinctrl
to a default state within the media driver's probe().
The camera port(s) are then configured for the video bus operation.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Changes since v3:
- removed the "inactive" pinctrl state, it will be added later if required
---
.../devicetree/bindings/media/soc/samsung-fimc.txt | 7 +++++++
drivers/media/platform/s5p-fimc/fimc-mdevice.c | 9 +++++++++
2 files changed, 16 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
index 6b81ad1..3788305 100644
--- a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -18,6 +18,10 @@ Required properties:
- compatible : must be "samsung,fimc", "simple-bus"
+- pinctrl-names : pinctrl names for camera port pinmux control, at least
+ "default" needs to be specified.
+- pinctrl-0...N : pinctrl properties corresponding to pinctrl-names
+
The 'camera' node must include at least one 'fimc' child node.
@@ -133,6 +137,9 @@ Example:
#size-cells = <1>;
status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&cam_port_a_clk_active>;
+
/* parallel camera ports */
parallel-ports {
/* camera A input */
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index 2bb501f..6c2c9e3 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -21,6 +21,7 @@
#include <linux/of_platform.h>
#include <linux/of_device.h>
#include <linux/of_i2c.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/types.h>
@@ -1197,6 +1198,14 @@ static int fimc_md_probe(struct platform_device *pdev)
/* Protect the media graph while we're registering entities */
mutex_lock(&fmd->media_dev.graph_mutex);
+ if (dev->of_node) {
+ struct pinctrl *pctl = devm_pinctrl_get_select_default(dev);
+ if (IS_ERR(pctl)) {
+ ret = PTR_ERR(pctl);
+ goto err_unlock;
+ }
+ }
+
if (fmd->pdev->dev.of_node)
ret = fimc_md_register_of_platform_entities(fmd, dev->of_node);
else
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 06/10] s5p-fimc: Use pinctrl API for camera ports configuration
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-arm-kernel
Before the camera ports can be used the pinmux needs to be configured
properly. This patch adds a function to set the camera ports pinctrl
to a default state within the media driver's probe().
The camera port(s) are then configured for the video bus operation.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Changes since v3:
- removed the "inactive" pinctrl state, it will be added later if required
---
.../devicetree/bindings/media/soc/samsung-fimc.txt | 7 +++++++
drivers/media/platform/s5p-fimc/fimc-mdevice.c | 9 +++++++++
2 files changed, 16 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
index 6b81ad1..3788305 100644
--- a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
@@ -18,6 +18,10 @@ Required properties:
- compatible : must be "samsung,fimc", "simple-bus"
+- pinctrl-names : pinctrl names for camera port pinmux control, at least
+ "default" needs to be specified.
+- pinctrl-0...N : pinctrl properties corresponding to pinctrl-names
+
The 'camera' node must include at least one 'fimc' child node.
@@ -133,6 +137,9 @@ Example:
#size-cells = <1>;
status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&cam_port_a_clk_active>;
+
/* parallel camera ports */
parallel-ports {
/* camera A input */
diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index 2bb501f..6c2c9e3 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -21,6 +21,7 @@
#include <linux/of_platform.h>
#include <linux/of_device.h>
#include <linux/of_i2c.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/types.h>
@@ -1197,6 +1198,14 @@ static int fimc_md_probe(struct platform_device *pdev)
/* Protect the media graph while we're registering entities */
mutex_lock(&fmd->media_dev.graph_mutex);
+ if (dev->of_node) {
+ struct pinctrl *pctl = devm_pinctrl_get_select_default(dev);
+ if (IS_ERR(pctl)) {
+ ret = PTR_ERR(pctl);
+ goto err_unlock;
+ }
+ }
+
if (fmd->pdev->dev.of_node)
ret = fimc_md_register_of_platform_entities(fmd, dev->of_node);
else
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 07/10] ARM: dts: Add camera to node exynos4.dtsi
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-01 19:09 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-media
Cc: kyungmin.park, kgene.kim, swarren, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel,
Sylwester Nawrocki
This patch adds common FIMC device nodes for all Exynos4 SoCs.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/boot/dts/exynos4.dtsi | 64 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index e1347fc..75c388b 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -36,6 +36,12 @@
i2c5 = &i2c_5;
i2c6 = &i2c_6;
i2c7 = &i2c_7;
+ csis0 = &csis_0;
+ csis1 = &csis_1;
+ fimc0 = &fimc_0;
+ fimc1 = &fimc_1;
+ fimc2 = &fimc_2;
+ fimc3 = &fimc_3;
};
pd_mfc: mfc-power-domain@10023C40 {
@@ -82,6 +88,64 @@
reg = <0x10440000 0x1000>;
};
+ camera {
+ compatible = "samsung,fimc", "simple-bus";
+ status = "disabled";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ fimc_0: fimc@11800000 {
+ compatible = "samsung,exynos4210-fimc";
+ reg = <0x11800000 0x1000>;
+ interrupts = <0 84 0>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+
+ fimc_1: fimc@11810000 {
+ compatible = "samsung,exynos4210-fimc";
+ reg = <0x11810000 0x1000>;
+ interrupts = <0 85 0>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+
+ fimc_2: fimc@11820000 {
+ compatible = "samsung,exynos4210-fimc";
+ reg = <0x11820000 0x1000>;
+ interrupts = <0 86 0>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+
+ fimc_3: fimc@11830000 {
+ compatible = "samsung,exynos4210-fimc";
+ reg = <0x11830000 0x1000>;
+ interrupts = <0 87 0>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+
+ csis_0: csis@11880000 {
+ compatible = "samsung,exynos4210-csis";
+ reg = <0x11880000 0x4000>;
+ interrupts = <0 78 0>;
+ bus-width = <4>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+
+ csis_1: csis@11890000 {
+ compatible = "samsung,exynos4210-csis";
+ reg = <0x11890000 0x4000>;
+ interrupts = <0 80 0>;
+ bus-width = <2>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+ };
+
watchdog@10060000 {
compatible = "samsung,s3c2410-wdt";
reg = <0x10060000 0x100>;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 07/10] ARM: dts: Add camera to node exynos4.dtsi
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds common FIMC device nodes for all Exynos4 SoCs.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/boot/dts/exynos4.dtsi | 64 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index e1347fc..75c388b 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -36,6 +36,12 @@
i2c5 = &i2c_5;
i2c6 = &i2c_6;
i2c7 = &i2c_7;
+ csis0 = &csis_0;
+ csis1 = &csis_1;
+ fimc0 = &fimc_0;
+ fimc1 = &fimc_1;
+ fimc2 = &fimc_2;
+ fimc3 = &fimc_3;
};
pd_mfc: mfc-power-domain at 10023C40 {
@@ -82,6 +88,64 @@
reg = <0x10440000 0x1000>;
};
+ camera {
+ compatible = "samsung,fimc", "simple-bus";
+ status = "disabled";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ fimc_0: fimc at 11800000 {
+ compatible = "samsung,exynos4210-fimc";
+ reg = <0x11800000 0x1000>;
+ interrupts = <0 84 0>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+
+ fimc_1: fimc at 11810000 {
+ compatible = "samsung,exynos4210-fimc";
+ reg = <0x11810000 0x1000>;
+ interrupts = <0 85 0>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+
+ fimc_2: fimc at 11820000 {
+ compatible = "samsung,exynos4210-fimc";
+ reg = <0x11820000 0x1000>;
+ interrupts = <0 86 0>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+
+ fimc_3: fimc at 11830000 {
+ compatible = "samsung,exynos4210-fimc";
+ reg = <0x11830000 0x1000>;
+ interrupts = <0 87 0>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+
+ csis_0: csis at 11880000 {
+ compatible = "samsung,exynos4210-csis";
+ reg = <0x11880000 0x4000>;
+ interrupts = <0 78 0>;
+ bus-width = <4>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+
+ csis_1: csis at 11890000 {
+ compatible = "samsung,exynos4210-csis";
+ reg = <0x11890000 0x4000>;
+ interrupts = <0 80 0>;
+ bus-width = <2>;
+ samsung,power-domain = <&pd_cam>;
+ status = "disabled";
+ };
+ };
+
watchdog at 10060000 {
compatible = "samsung,s3c2410-wdt";
reg = <0x10060000 0x100>;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 08/10] ARM: dts: Add ISP power domain node for Exynos4x12
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-01 19:09 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-media
Cc: kyungmin.park, kgene.kim, swarren, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel,
Sylwester Nawrocki
The ISP power domain is a common power domain for fimc-lite
and fimc-is (ISP) devices.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/boot/dts/exynos4x12.dtsi | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index 179a62e..9c809b72 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -28,6 +28,11 @@
pinctrl3 = &pinctrl_3;
};
+ pd_isp: isp-power-domain@10023CA0 {
+ compatible = "samsung,exynos4210-pd";
+ reg = <0x10023CA0 0x20>;
+ };
+
combiner:interrupt-controller@10440000 {
interrupts = <0 0 0>, <0 1 0>, <0 2 0>, <0 3 0>,
<0 4 0>, <0 5 0>, <0 6 0>, <0 7 0>,
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 08/10] ARM: dts: Add ISP power domain node for Exynos4x12
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-arm-kernel
The ISP power domain is a common power domain for fimc-lite
and fimc-is (ISP) devices.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/boot/dts/exynos4x12.dtsi | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index 179a62e..9c809b72 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -28,6 +28,11 @@
pinctrl3 = &pinctrl_3;
};
+ pd_isp: isp-power-domain at 10023CA0 {
+ compatible = "samsung,exynos4210-pd";
+ reg = <0x10023CA0 0x20>;
+ };
+
combiner:interrupt-controller at 10440000 {
interrupts = <0 0 0>, <0 1 0>, <0 2 0>, <0 3 0>,
<0 4 0>, <0 5 0>, <0 6 0>, <0 7 0>,
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 09/10] ARM: dts: Add FIMC and MIPI CSIS device nodes for Exynos4x12
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-01 19:09 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-media
Cc: kyungmin.park, kgene.kim, swarren, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel,
Sylwester Nawrocki
Add common camera node and fimc nodes specific to Exynos4212 and
Exynos4412 SoCs. fimc-is is a node for the Exynos4x12 FIMC-IS
subsystem and fimc-lite nodes are created as its child nodes,
among others due to FIMC-LITE device dependencies on FIMC-IS
related clocks.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/boot/dts/exynos4x12.dtsi | 47 +++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index 9c809b72..59b2b8e 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -26,6 +26,8 @@
pinctrl1 = &pinctrl_1;
pinctrl2 = &pinctrl_2;
pinctrl3 = &pinctrl_3;
+ fimc-lite0 = &fimc_lite_0;
+ fimc-lite1 = &fimc_lite_1;
};
pd_isp: isp-power-domain@10023CA0 {
@@ -71,4 +73,49 @@
reg = <0x106E0000 0x1000>;
interrupts = <0 72 0>;
};
+
+ camera {
+ fimc_0: fimc@11800000 {
+ compatible = "samsung,exynos4212-fimc";
+ };
+
+ fimc_1: fimc@11810000 {
+ compatible = "samsung,exynos4212-fimc";
+ };
+
+ fimc_2: fimc@11820000 {
+ compatible = "samsung,exynos4212-fimc";
+ };
+
+ fimc_3: fimc@11830000 {
+ compatible = "samsung,exynos4212-fimc";
+ };
+
+ fimc_is: fimc-is@12000000 {
+ compatible = "samsung,exynos4212-fimc-is", "simple-bus";
+ reg = <0x12000000 0x260000>;
+ interrupts = <0 90 0>, <0 95 0>;
+ samsung,power-domain = <&pd_isp>;
+ status = "disabled";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ fimc_lite_0: fimc-lite@12390000 {
+ compatible = "samsung,exynos4212-fimc-lite";
+ reg = <0x12390000 0x1000>;
+ interrupts = <0 105 0>;
+ samsung,power-domain = <&pd_isp>;
+ status = "disabled";
+ };
+
+ fimc_lite_1: fimc-lite@123A0000 {
+ compatible = "samsung,exynos4212-fimc-lite";
+ reg = <0x123A0000 0x1000>;
+ interrupts = <0 106 0>;
+ samsung,power-domain = <&pd_isp>;
+ status = "disabled";
+ };
+ };
+ };
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 09/10] ARM: dts: Add FIMC and MIPI CSIS device nodes for Exynos4x12
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-arm-kernel
Add common camera node and fimc nodes specific to Exynos4212 and
Exynos4412 SoCs. fimc-is is a node for the Exynos4x12 FIMC-IS
subsystem and fimc-lite nodes are created as its child nodes,
among others due to FIMC-LITE device dependencies on FIMC-IS
related clocks.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/boot/dts/exynos4x12.dtsi | 47 +++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index 9c809b72..59b2b8e 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -26,6 +26,8 @@
pinctrl1 = &pinctrl_1;
pinctrl2 = &pinctrl_2;
pinctrl3 = &pinctrl_3;
+ fimc-lite0 = &fimc_lite_0;
+ fimc-lite1 = &fimc_lite_1;
};
pd_isp: isp-power-domain at 10023CA0 {
@@ -71,4 +73,49 @@
reg = <0x106E0000 0x1000>;
interrupts = <0 72 0>;
};
+
+ camera {
+ fimc_0: fimc at 11800000 {
+ compatible = "samsung,exynos4212-fimc";
+ };
+
+ fimc_1: fimc at 11810000 {
+ compatible = "samsung,exynos4212-fimc";
+ };
+
+ fimc_2: fimc at 11820000 {
+ compatible = "samsung,exynos4212-fimc";
+ };
+
+ fimc_3: fimc at 11830000 {
+ compatible = "samsung,exynos4212-fimc";
+ };
+
+ fimc_is: fimc-is at 12000000 {
+ compatible = "samsung,exynos4212-fimc-is", "simple-bus";
+ reg = <0x12000000 0x260000>;
+ interrupts = <0 90 0>, <0 95 0>;
+ samsung,power-domain = <&pd_isp>;
+ status = "disabled";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ fimc_lite_0: fimc-lite at 12390000 {
+ compatible = "samsung,exynos4212-fimc-lite";
+ reg = <0x12390000 0x1000>;
+ interrupts = <0 105 0>;
+ samsung,power-domain = <&pd_isp>;
+ status = "disabled";
+ };
+
+ fimc_lite_1: fimc-lite at 123A0000 {
+ compatible = "samsung,exynos4212-fimc-lite";
+ reg = <0x123A0000 0x1000>;
+ interrupts = <0 106 0>;
+ samsung,power-domain = <&pd_isp>;
+ status = "disabled";
+ };
+ };
+ };
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 10/10] ARM: dts: Correct camera pinctrl nodes for Exynos4x12 SoCs
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-01 19:09 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-media
Cc: kyungmin.park, kgene.kim, swarren, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel,
Sylwester Nawrocki
Add separate nodes for the CAMCLK pin and turn off pull-up on camera
ports A, B. The video bus pins and the clock output (CAMCLK) pin need
separate nodes since full camera port is not used in some configurations,
e.g. for MIPI CSI-2 bus on CAMCLK is required and data/clock signal
use separate dedicated pins.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Changes since v3:
- corrected camera port B part and removed entries for
"inactive" state
---
arch/arm/boot/dts/exynos4x12-pinctrl.dtsi | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi b/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi
index 8e6115a..4c3a2c3 100644
--- a/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi
+++ b/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi
@@ -401,15 +401,21 @@
samsung,pin-drv = <0>;
};
- cam_port_a: cam-port-a {
+ cam_port_a_io: cam-port-a-io {
samsung,pins = "gpj0-0", "gpj0-1", "gpj0-2", "gpj0-3",
"gpj0-4", "gpj0-5", "gpj0-6", "gpj0-7",
- "gpj1-0", "gpj1-1", "gpj1-2", "gpj1-3",
- "gpj1-4";
+ "gpj1-0", "gpj1-1", "gpj1-2", "gpj1-4";
samsung,pin-function = <2>;
- samsung,pin-pud = <3>;
+ samsung,pin-pud = <0>;
samsung,pin-drv = <0>;
};
+
+ cam_port_a_clk_active: cam-port-a-clk-active {
+ samsung,pins = "gpj1-3";
+ samsung,pin-function = <2>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <3>;
+ };
};
pinctrl@11000000 {
@@ -834,16 +840,22 @@
samsung,pin-drv = <0>;
};
- cam_port_b: cam-port-b {
+ cam_port_b_io: cam-port-b-io {
samsung,pins = "gpm0-0", "gpm0-1", "gpm0-2", "gpm0-3",
"gpm0-4", "gpm0-5", "gpm0-6", "gpm0-7",
- "gpm1-0", "gpm1-1", "gpm2-0", "gpm2-1",
- "gpm2-2";
+ "gpm1-0", "gpm1-1", "gpm2-0", "gpm2-1";
samsung,pin-function = <3>;
samsung,pin-pud = <3>;
samsung,pin-drv = <0>;
};
+ cam_port_b_clk_active: cam-port-b-clk-active {
+ samsung,pins = "gpm2-2";
+ samsung,pin-function = <3>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <3>;
+ };
+
eint0: ext-int0 {
samsung,pins = "gpx0-0";
samsung,pin-function = <0xf>;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 10/10] ARM: dts: Correct camera pinctrl nodes for Exynos4x12 SoCs
@ 2013-02-01 19:09 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-01 19:09 UTC (permalink / raw)
To: linux-arm-kernel
Add separate nodes for the CAMCLK pin and turn off pull-up on camera
ports A, B. The video bus pins and the clock output (CAMCLK) pin need
separate nodes since full camera port is not used in some configurations,
e.g. for MIPI CSI-2 bus on CAMCLK is required and data/clock signal
use separate dedicated pins.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Changes since v3:
- corrected camera port B part and removed entries for
"inactive" state
---
arch/arm/boot/dts/exynos4x12-pinctrl.dtsi | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi b/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi
index 8e6115a..4c3a2c3 100644
--- a/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi
+++ b/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi
@@ -401,15 +401,21 @@
samsung,pin-drv = <0>;
};
- cam_port_a: cam-port-a {
+ cam_port_a_io: cam-port-a-io {
samsung,pins = "gpj0-0", "gpj0-1", "gpj0-2", "gpj0-3",
"gpj0-4", "gpj0-5", "gpj0-6", "gpj0-7",
- "gpj1-0", "gpj1-1", "gpj1-2", "gpj1-3",
- "gpj1-4";
+ "gpj1-0", "gpj1-1", "gpj1-2", "gpj1-4";
samsung,pin-function = <2>;
- samsung,pin-pud = <3>;
+ samsung,pin-pud = <0>;
samsung,pin-drv = <0>;
};
+
+ cam_port_a_clk_active: cam-port-a-clk-active {
+ samsung,pins = "gpj1-3";
+ samsung,pin-function = <2>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <3>;
+ };
};
pinctrl at 11000000 {
@@ -834,16 +840,22 @@
samsung,pin-drv = <0>;
};
- cam_port_b: cam-port-b {
+ cam_port_b_io: cam-port-b-io {
samsung,pins = "gpm0-0", "gpm0-1", "gpm0-2", "gpm0-3",
"gpm0-4", "gpm0-5", "gpm0-6", "gpm0-7",
- "gpm1-0", "gpm1-1", "gpm2-0", "gpm2-1",
- "gpm2-2";
+ "gpm1-0", "gpm1-1", "gpm2-0", "gpm2-1";
samsung,pin-function = <3>;
samsung,pin-pud = <3>;
samsung,pin-drv = <0>;
};
+ cam_port_b_clk_active: cam-port-b-clk-active {
+ samsung,pins = "gpm2-2";
+ samsung,pin-function = <3>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <3>;
+ };
+
eint0: ext-int0 {
samsung,pins = "gpx0-0";
samsung,pin-function = <0xf>;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v4 01/10] s5p-csis: Add device tree support
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-06 23:36 ` Stephen Warren
-1 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-06 23:36 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: linux-media, kyungmin.park, kgene.kim, rob.herring,
prabhakar.lad, devicetree-discuss, linux-samsung-soc,
linux-arm-kernel
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
> device. This patch support for binding the driver to the MIPI-CSIS
> devices instantiated from device tree and for parsing all SoC and
> board specific properties.
> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
> +Optional properties:
> +
> +- clock-frequency : The IP's main (system bus) clock frequency in Hz, default
> + value when this property is not specified is 166 MHz;
Shouldn't this be a "clocks" property, so that the driver can call
clk_get(), clk_prepare_enable(), clk_get_rate(), etc. on it?
Other than that this binding seems fine to me at a quick glance.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 01/10] s5p-csis: Add device tree support
@ 2013-02-06 23:36 ` Stephen Warren
0 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-06 23:36 UTC (permalink / raw)
To: linux-arm-kernel
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
> device. This patch support for binding the driver to the MIPI-CSIS
> devices instantiated from device tree and for parsing all SoC and
> board specific properties.
> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
> +Optional properties:
> +
> +- clock-frequency : The IP's main (system bus) clock frequency in Hz, default
> + value when this property is not specified is 166 MHz;
Shouldn't this be a "clocks" property, so that the driver can call
clk_get(), clk_prepare_enable(), clk_get_rate(), etc. on it?
Other than that this binding seems fine to me at a quick glance.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-06 23:40 ` Stephen Warren
-1 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-06 23:40 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: linux-media, kyungmin.park, kgene.kim, rob.herring,
prabhakar.lad, devicetree-discuss, linux-samsung-soc,
linux-arm-kernel
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
> This patch adds support for FIMC devices instantiated from devicetree
> for S5PV210 and Exynos4 SoCs. The FIMC IP features include colorspace
> conversion and scaling (mem-to-mem) and parallel/MIPI CSI2 bus video
> capture interface.
>
> Multiple SoC revisions specific parameters are defined statically in
> the driver and are used for both dt and non-dt. The driver's static
> data is selected based on the compatible property. Previously the
> platform device name was used to match driver data and a specific
> SoC/IP version.
>
> Aliases are used to determine an index of the IP which is essential
> for linking FIMC IP with other entities, like MIPI-CSIS (the MIPI
> CSI-2 bus frontend) or FIMC-LITE and FIMC-IS ISP.
> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
> +----------------------------------------------
> +
> +The Exynos Camera subsystem comprises of multiple sub-devices that are
> +represented by separate platform devices. Some of the IPs come in different
"platform devices" is a rather Linux-centric term, and DT bindings
should be OS-agnostic. Perhaps use "device tree nodes" here?
> +variants across the SoC revisions (FIMC) and some remain mostly unchanged
> +(MIPI CSIS, FIMC-LITE).
> +
> +All those sub-subdevices are defined as parent nodes of the common device
s/parent nodes/child node/ I think?
> +For every fimc node a numbered alias should be present in the aliases node.
> +Aliases are of the form fimc<n>, where <n> is an integer (0...N) specifying
> +the IP's instance index.
Why? Isn't it up to the DT author whether they care if each fimc node is
assigned a specific identification v.s. whether identification is
assigned automatically?
> +Optional properties
> +
> + - clock-frequency - maximum FIMC local clock (LCLK) frequency
Again, I'd expect a clocks property here instead.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-06 23:40 ` Stephen Warren
0 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-06 23:40 UTC (permalink / raw)
To: linux-arm-kernel
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
> This patch adds support for FIMC devices instantiated from devicetree
> for S5PV210 and Exynos4 SoCs. The FIMC IP features include colorspace
> conversion and scaling (mem-to-mem) and parallel/MIPI CSI2 bus video
> capture interface.
>
> Multiple SoC revisions specific parameters are defined statically in
> the driver and are used for both dt and non-dt. The driver's static
> data is selected based on the compatible property. Previously the
> platform device name was used to match driver data and a specific
> SoC/IP version.
>
> Aliases are used to determine an index of the IP which is essential
> for linking FIMC IP with other entities, like MIPI-CSIS (the MIPI
> CSI-2 bus frontend) or FIMC-LITE and FIMC-IS ISP.
> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
> +----------------------------------------------
> +
> +The Exynos Camera subsystem comprises of multiple sub-devices that are
> +represented by separate platform devices. Some of the IPs come in different
"platform devices" is a rather Linux-centric term, and DT bindings
should be OS-agnostic. Perhaps use "device tree nodes" here?
> +variants across the SoC revisions (FIMC) and some remain mostly unchanged
> +(MIPI CSIS, FIMC-LITE).
> +
> +All those sub-subdevices are defined as parent nodes of the common device
s/parent nodes/child node/ I think?
> +For every fimc node a numbered alias should be present in the aliases node.
> +Aliases are of the form fimc<n>, where <n> is an integer (0...N) specifying
> +the IP's instance index.
Why? Isn't it up to the DT author whether they care if each fimc node is
assigned a specific identification v.s. whether identification is
assigned automatically?
> +Optional properties
> +
> + - clock-frequency - maximum FIMC local clock (LCLK) frequency
Again, I'd expect a clocks property here instead.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 05/10] s5p-fimc: Add device tree based sensors registration
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-06 23:42 ` Stephen Warren
-1 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-06 23:42 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: linux-media, kyungmin.park, kgene.kim, rob.herring,
prabhakar.lad, devicetree-discuss, linux-samsung-soc,
linux-arm-kernel
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
> The sensor (I2C and/or SPI client) devices are instantiated by their
> corresponding control bus drivers. Since the I2C client's master clock
> is often provided by a video bus receiver (host interface) or other
> than I2C/SPI controller device, the drivers of those client devices
> are not accessing hardware in their driver's probe() callback. Instead,
> after enabling clock, the host driver calls back into a sub-device
> when it wants to activate them. This pattern is used by some in-tree
> drivers and this patch also uses it for DT case. This patch is intended
> as a first step for adding device tree support to the S5P/Exynos SoC
> camera drivers. The second one is adding support for asynchronous
> sub-devices registration and clock control from sub-device driver
> level. The bindings shall not change when asynchronous probing support
> is added.
> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
> +The sensor device nodes should be added as their control bus controller
I think "as" should be "to"?
> +(e.g. I2C0) child nodes and linked to a port node in the csis or parallel-ports
> +node, using common the common video interfaces bindings, i.e. port/endpoint
> +node pairs. The implementation of this binding requires clock-frequency
> +property to be present in the sensor device nodes.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 05/10] s5p-fimc: Add device tree based sensors registration
@ 2013-02-06 23:42 ` Stephen Warren
0 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-06 23:42 UTC (permalink / raw)
To: linux-arm-kernel
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
> The sensor (I2C and/or SPI client) devices are instantiated by their
> corresponding control bus drivers. Since the I2C client's master clock
> is often provided by a video bus receiver (host interface) or other
> than I2C/SPI controller device, the drivers of those client devices
> are not accessing hardware in their driver's probe() callback. Instead,
> after enabling clock, the host driver calls back into a sub-device
> when it wants to activate them. This pattern is used by some in-tree
> drivers and this patch also uses it for DT case. This patch is intended
> as a first step for adding device tree support to the S5P/Exynos SoC
> camera drivers. The second one is adding support for asynchronous
> sub-devices registration and clock control from sub-device driver
> level. The bindings shall not change when asynchronous probing support
> is added.
> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
> +The sensor device nodes should be added as their control bus controller
I think "as" should be "to"?
> +(e.g. I2C0) child nodes and linked to a port node in the csis or parallel-ports
> +node, using common the common video interfaces bindings, i.e. port/endpoint
> +node pairs. The implementation of this binding requires clock-frequency
> +property to be present in the sensor device nodes.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 06/10] s5p-fimc: Use pinctrl API for camera ports configuration
2013-02-01 19:09 ` Sylwester Nawrocki
@ 2013-02-06 23:44 ` Stephen Warren
-1 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-06 23:44 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: linux-media, kyungmin.park, kgene.kim, rob.herring,
prabhakar.lad, devicetree-discuss, linux-samsung-soc,
linux-arm-kernel
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
> Before the camera ports can be used the pinmux needs to be configured
> properly. This patch adds a function to set the camera ports pinctrl
> to a default state within the media driver's probe().
> The camera port(s) are then configured for the video bus operation.
> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
> +- pinctrl-names : pinctrl names for camera port pinmux control, at least
> + "default" needs to be specified.
> +- pinctrl-0...N : pinctrl properties corresponding to pinctrl-names
> +
A reference to the binding document describing the pin control bindings
would be appropriate here. Given that reference, I'm not sure if
spelling out the property names makes sense since it feels a little like
duplication; an alternative might be simply:
The pinctrl bindings defined in ../../../pinctrl/pinctrl-bindings.txt
must be used to define a pinctrl state named "default".
However, this isn't a big deal; it's fine either way.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 06/10] s5p-fimc: Use pinctrl API for camera ports configuration
@ 2013-02-06 23:44 ` Stephen Warren
0 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-06 23:44 UTC (permalink / raw)
To: linux-arm-kernel
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
> Before the camera ports can be used the pinmux needs to be configured
> properly. This patch adds a function to set the camera ports pinctrl
> to a default state within the media driver's probe().
> The camera port(s) are then configured for the video bus operation.
> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
> +- pinctrl-names : pinctrl names for camera port pinmux control, at least
> + "default" needs to be specified.
> +- pinctrl-0...N : pinctrl properties corresponding to pinctrl-names
> +
A reference to the binding document describing the pin control bindings
would be appropriate here. Given that reference, I'm not sure if
spelling out the property names makes sense since it feels a little like
duplication; an alternative might be simply:
The pinctrl bindings defined in ../../../pinctrl/pinctrl-bindings.txt
must be used to define a pinctrl state named "default".
However, this isn't a big deal; it's fine either way.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 01/10] s5p-csis: Add device tree support
2013-02-06 23:36 ` Stephen Warren
@ 2013-02-08 22:29 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-08 22:29 UTC (permalink / raw)
To: Stephen Warren
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/07/2013 12:36 AM, Stephen Warren wrote:
> On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
>> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
>> device. This patch support for binding the driver to the MIPI-CSIS
>> devices instantiated from device tree and for parsing all SoC and
>> board specific properties.
>
>> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>
>> +Optional properties:
>> +
>> +- clock-frequency : The IP's main (system bus) clock frequency in Hz, default
>> + value when this property is not specified is 166 MHz;
>
> Shouldn't this be a "clocks" property, so that the driver can call
> clk_get(), clk_prepare_enable(), clk_get_rate(), etc. on it?
Hi Stephen,
Thanks for your review!
I also use "clocks" and "clock-names" property, but didn't specify
it here, because the Exynos SoCs clocks driver is not in the mainline yet.
There are two clocks the driver needs to be aware of. One of them needs
to have a specific parent clock set and the frequency set properly,
so when it is put into a data pipeline with other IPs there is no overflows
of their input/output FIFOs.
devfreq may change those frequencies if enabled, however its another topic.
I wanted to ensure the device has initially correct local clock frequency
set, with which it can operate.
--
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 01/10] s5p-csis: Add device tree support
@ 2013-02-08 22:29 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-08 22:29 UTC (permalink / raw)
To: linux-arm-kernel
On 02/07/2013 12:36 AM, Stephen Warren wrote:
> On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
>> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
>> device. This patch support for binding the driver to the MIPI-CSIS
>> devices instantiated from device tree and for parsing all SoC and
>> board specific properties.
>
>> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>
>> +Optional properties:
>> +
>> +- clock-frequency : The IP's main (system bus) clock frequency in Hz, default
>> + value when this property is not specified is 166 MHz;
>
> Shouldn't this be a "clocks" property, so that the driver can call
> clk_get(), clk_prepare_enable(), clk_get_rate(), etc. on it?
Hi Stephen,
Thanks for your review!
I also use "clocks" and "clock-names" property, but didn't specify
it here, because the Exynos SoCs clocks driver is not in the mainline yet.
There are two clocks the driver needs to be aware of. One of them needs
to have a specific parent clock set and the frequency set properly,
so when it is put into a data pipeline with other IPs there is no overflows
of their input/output FIFOs.
devfreq may change those frequencies if enabled, however its another topic.
I wanted to ensure the device has initially correct local clock frequency
set, with which it can operate.
--
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-06 23:40 ` Stephen Warren
@ 2013-02-08 23:16 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-08 23:16 UTC (permalink / raw)
To: Stephen Warren
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/07/2013 12:40 AM, Stephen Warren wrote:
>> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>
>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>> +----------------------------------------------
>> +
>> +The Exynos Camera subsystem comprises of multiple sub-devices that are
>> +represented by separate platform devices. Some of the IPs come in different
>
> "platform devices" is a rather Linux-centric term, and DT bindings
> should be OS-agnostic. Perhaps use "device tree nodes" here?
Indeed, thank you for the suggestion, I'll change it.
>> +variants across the SoC revisions (FIMC) and some remain mostly unchanged
>> +(MIPI CSIS, FIMC-LITE).
>> +
>> +All those sub-subdevices are defined as parent nodes of the common device
>
> s/parent nodes/child node/ I think?
Yeah, 'parent nodes' doesn't really make sense. Thanks for catching it.
>> +For every fimc node a numbered alias should be present in the aliases node.
>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N) specifying
>> +the IP's instance index.
>
> Why? Isn't it up to the DT author whether they care if each fimc node is
> assigned a specific identification v.s. whether identification is
> assigned automatically?
There are at least three different kinds of IPs that come in multiple
instances in an SoC. To activate data links between them each instance
needs to be clearly identified. There are also differences between
instances of same device. Hence it's important these aliases don't have
random values.
Some more details about the SoC can be found at [1]. The aliases are
also already used in the Exynos5 GScaler bindings [2] in a similar way.
>> +Optional properties
>> +
>> + - clock-frequency - maximum FIMC local clock (LCLK) frequency
>
> Again, I'd expect a clocks property here instead.
Please see my comment to patch 01/10. Analogously, I needed this clock
frequency to ensure reliable video data pipeline operation.
[1] http://tinyurl.com/anw9udm
[2] http://www.spinics.net/lists/arm-kernel/msg200036.html
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-08 23:16 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-08 23:16 UTC (permalink / raw)
To: linux-arm-kernel
On 02/07/2013 12:40 AM, Stephen Warren wrote:
>> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>
>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>> +----------------------------------------------
>> +
>> +The Exynos Camera subsystem comprises of multiple sub-devices that are
>> +represented by separate platform devices. Some of the IPs come in different
>
> "platform devices" is a rather Linux-centric term, and DT bindings
> should be OS-agnostic. Perhaps use "device tree nodes" here?
Indeed, thank you for the suggestion, I'll change it.
>> +variants across the SoC revisions (FIMC) and some remain mostly unchanged
>> +(MIPI CSIS, FIMC-LITE).
>> +
>> +All those sub-subdevices are defined as parent nodes of the common device
>
> s/parent nodes/child node/ I think?
Yeah, 'parent nodes' doesn't really make sense. Thanks for catching it.
>> +For every fimc node a numbered alias should be present in the aliases node.
>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N) specifying
>> +the IP's instance index.
>
> Why? Isn't it up to the DT author whether they care if each fimc node is
> assigned a specific identification v.s. whether identification is
> assigned automatically?
There are at least three different kinds of IPs that come in multiple
instances in an SoC. To activate data links between them each instance
needs to be clearly identified. There are also differences between
instances of same device. Hence it's important these aliases don't have
random values.
Some more details about the SoC can be found at [1]. The aliases are
also already used in the Exynos5 GScaler bindings [2] in a similar way.
>> +Optional properties
>> +
>> + - clock-frequency - maximum FIMC local clock (LCLK) frequency
>
> Again, I'd expect a clocks property here instead.
Please see my comment to patch 01/10. Analogously, I needed this clock
frequency to ensure reliable video data pipeline operation.
[1] http://tinyurl.com/anw9udm
[2] http://www.spinics.net/lists/arm-kernel/msg200036.html
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-08 23:16 ` Sylwester Nawrocki
@ 2013-02-08 23:21 ` Stephen Warren
-1 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-08 23:21 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>> diff --git
>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>
>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>> +----------------------------------------------
...
>>> +For every fimc node a numbered alias should be present in the
>>> aliases node.
>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>> specifying
>>> +the IP's instance index.
>>
>> Why? Isn't it up to the DT author whether they care if each fimc node is
>> assigned a specific identification v.s. whether identification is
>> assigned automatically?
>
> There are at least three different kinds of IPs that come in multiple
> instances in an SoC. To activate data links between them each instance
> needs to be clearly identified. There are also differences between
> instances of same device. Hence it's important these aliases don't have
> random values.
>
> Some more details about the SoC can be found at [1]. The aliases are
> also already used in the Exynos5 GScaler bindings [2] in a similar way.
Hmmm. I'd expect explicit DT properties to represent the
instance-specific "configuration", or even different compatible values.
Relying on the alias ID seems rather indirect; what if in e.g.
Exynos6/... the mapping from instance/alias ID to feature set changes.
With explicit DT properties, that'd just be a .dts change, whereas by
requiring alias IDs now, you'd need a driver change to support this.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-08 23:21 ` Stephen Warren
0 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-08 23:21 UTC (permalink / raw)
To: linux-arm-kernel
On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>> diff --git
>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>
>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>> +----------------------------------------------
...
>>> +For every fimc node a numbered alias should be present in the
>>> aliases node.
>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>> specifying
>>> +the IP's instance index.
>>
>> Why? Isn't it up to the DT author whether they care if each fimc node is
>> assigned a specific identification v.s. whether identification is
>> assigned automatically?
>
> There are at least three different kinds of IPs that come in multiple
> instances in an SoC. To activate data links between them each instance
> needs to be clearly identified. There are also differences between
> instances of same device. Hence it's important these aliases don't have
> random values.
>
> Some more details about the SoC can be found at [1]. The aliases are
> also already used in the Exynos5 GScaler bindings [2] in a similar way.
Hmmm. I'd expect explicit DT properties to represent the
instance-specific "configuration", or even different compatible values.
Relying on the alias ID seems rather indirect; what if in e.g.
Exynos6/... the mapping from instance/alias ID to feature set changes.
With explicit DT properties, that'd just be a .dts change, whereas by
requiring alias IDs now, you'd need a driver change to support this.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 05/10] s5p-fimc: Add device tree based sensors registration
2013-02-06 23:42 ` Stephen Warren
@ 2013-02-08 23:26 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-08 23:26 UTC (permalink / raw)
To: Stephen Warren
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/07/2013 12:42 AM, Stephen Warren wrote:
> On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
>> The sensor (I2C and/or SPI client) devices are instantiated by their
>> corresponding control bus drivers. Since the I2C client's master clock
>> is often provided by a video bus receiver (host interface) or other
>> than I2C/SPI controller device, the drivers of those client devices
>> are not accessing hardware in their driver's probe() callback. Instead,
>> after enabling clock, the host driver calls back into a sub-device
>> when it wants to activate them. This pattern is used by some in-tree
>> drivers and this patch also uses it for DT case. This patch is intended
>> as a first step for adding device tree support to the S5P/Exynos SoC
>> camera drivers. The second one is adding support for asynchronous
>> sub-devices registration and clock control from sub-device driver
>> level. The bindings shall not change when asynchronous probing support
>> is added.
>
>> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>
>> +The sensor device nodes should be added as their control bus controller
>
> I think "as" should be "to"?
Yes, something is wrong here. Hopefully this is more correct:
The sensor device nodes should be added to their control bus controller
(e.g. I2C0) nodes and linked to a port node in the csis or parallel-ports
>> +(e.g. I2C0) child nodes and linked to a port node in the csis or parallel-ports
>> +node, using common the common video interfaces bindings, i.e. port/endpoint
^^^^^^^^
And this needs correction too.
>> +node pairs. The implementation of this binding requires clock-frequency
>> +property to be present in the sensor device nodes.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 05/10] s5p-fimc: Add device tree based sensors registration
@ 2013-02-08 23:26 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-08 23:26 UTC (permalink / raw)
To: linux-arm-kernel
On 02/07/2013 12:42 AM, Stephen Warren wrote:
> On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
>> The sensor (I2C and/or SPI client) devices are instantiated by their
>> corresponding control bus drivers. Since the I2C client's master clock
>> is often provided by a video bus receiver (host interface) or other
>> than I2C/SPI controller device, the drivers of those client devices
>> are not accessing hardware in their driver's probe() callback. Instead,
>> after enabling clock, the host driver calls back into a sub-device
>> when it wants to activate them. This pattern is used by some in-tree
>> drivers and this patch also uses it for DT case. This patch is intended
>> as a first step for adding device tree support to the S5P/Exynos SoC
>> camera drivers. The second one is adding support for asynchronous
>> sub-devices registration and clock control from sub-device driver
>> level. The bindings shall not change when asynchronous probing support
>> is added.
>
>> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>
>> +The sensor device nodes should be added as their control bus controller
>
> I think "as" should be "to"?
Yes, something is wrong here. Hopefully this is more correct:
The sensor device nodes should be added to their control bus controller
(e.g. I2C0) nodes and linked to a port node in the csis or parallel-ports
>> +(e.g. I2C0) child nodes and linked to a port node in the csis or parallel-ports
>> +node, using common the common video interfaces bindings, i.e. port/endpoint
^^^^^^^^
And this needs correction too.
>> +node pairs. The implementation of this binding requires clock-frequency
>> +property to be present in the sensor device nodes.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 01/10] s5p-csis: Add device tree support
2013-02-08 22:29 ` Sylwester Nawrocki
@ 2013-02-08 23:27 ` Stephen Warren
-1 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-08 23:27 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/08/2013 03:29 PM, Sylwester Nawrocki wrote:
> On 02/07/2013 12:36 AM, Stephen Warren wrote:
>> On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
>>> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
>>> device. This patch support for binding the driver to the MIPI-CSIS
>>> devices instantiated from device tree and for parsing all SoC and
>>> board specific properties.
>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>> b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>>
>>> +Optional properties:
>>> +
>>> +- clock-frequency : The IP's main (system bus) clock frequency in
>>> Hz, default
>>> + value when this property is not specified is 166 MHz;
>>
>> Shouldn't this be a "clocks" property, so that the driver can call
>> clk_get(), clk_prepare_enable(), clk_get_rate(), etc. on it?
>
> Hi Stephen,
>
> Thanks for your review!
>
> I also use "clocks" and "clock-names" property, but didn't specify
> it here, because the Exynos SoCs clocks driver is not in the mainline yet.
I'm a bit sympathetic to this, since I've been trying to push Tegra
towards the common clock framework and describing clock connectivity in
DT, before adding new drivers that need clocks, specifically to avoid
this kind of situation.
The problem here is that if this gets merged now, then the clock driver
comes along later, you'll have to change this binding definition to
account for it, and DT bindings aren't supposed to be changed...
Do you have any clock driver at all upstream yet? Or, could you add a
dummy clock driver to satisfy the driver's clkl_get() needs? If you do,
you can always set up some AUXDATA so that clk_get() works for your
driver right now, and then remove that once the complete clock driver is
in place with full device tree support. That's how we've ended up
handling this for Tegra drivers.
Anyway, this is all mainly food-for-thought rather than an objection to
the patch; I'd leave that to Grant/Rob if applicable.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 01/10] s5p-csis: Add device tree support
@ 2013-02-08 23:27 ` Stephen Warren
0 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-08 23:27 UTC (permalink / raw)
To: linux-arm-kernel
On 02/08/2013 03:29 PM, Sylwester Nawrocki wrote:
> On 02/07/2013 12:36 AM, Stephen Warren wrote:
>> On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
>>> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
>>> device. This patch support for binding the driver to the MIPI-CSIS
>>> devices instantiated from device tree and for parsing all SoC and
>>> board specific properties.
>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>> b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>>
>>> +Optional properties:
>>> +
>>> +- clock-frequency : The IP's main (system bus) clock frequency in
>>> Hz, default
>>> + value when this property is not specified is 166 MHz;
>>
>> Shouldn't this be a "clocks" property, so that the driver can call
>> clk_get(), clk_prepare_enable(), clk_get_rate(), etc. on it?
>
> Hi Stephen,
>
> Thanks for your review!
>
> I also use "clocks" and "clock-names" property, but didn't specify
> it here, because the Exynos SoCs clocks driver is not in the mainline yet.
I'm a bit sympathetic to this, since I've been trying to push Tegra
towards the common clock framework and describing clock connectivity in
DT, before adding new drivers that need clocks, specifically to avoid
this kind of situation.
The problem here is that if this gets merged now, then the clock driver
comes along later, you'll have to change this binding definition to
account for it, and DT bindings aren't supposed to be changed...
Do you have any clock driver at all upstream yet? Or, could you add a
dummy clock driver to satisfy the driver's clkl_get() needs? If you do,
you can always set up some AUXDATA so that clk_get() works for your
driver right now, and then remove that once the complete clock driver is
in place with full device tree support. That's how we've ended up
handling this for Tegra drivers.
Anyway, this is all mainly food-for-thought rather than an objection to
the patch; I'd leave that to Grant/Rob if applicable.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 06/10] s5p-fimc: Use pinctrl API for camera ports configuration
2013-02-06 23:44 ` Stephen Warren
@ 2013-02-08 23:30 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-08 23:30 UTC (permalink / raw)
To: Stephen Warren
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/07/2013 12:44 AM, Stephen Warren wrote:
> On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
>> Before the camera ports can be used the pinmux needs to be configured
>> properly. This patch adds a function to set the camera ports pinctrl
>> to a default state within the media driver's probe().
>> The camera port(s) are then configured for the video bus operation.
>
>> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>
>> +- pinctrl-names : pinctrl names for camera port pinmux control, at least
>> + "default" needs to be specified.
>> +- pinctrl-0...N : pinctrl properties corresponding to pinctrl-names
>> +
>
> A reference to the binding document describing the pin control bindings
> would be appropriate here. Given that reference, I'm not sure if
> spelling out the property names makes sense since it feels a little like
> duplication; an alternative might be simply:
>
> The pinctrl bindings defined in ../../../pinctrl/pinctrl-bindings.txt
> must be used to define a pinctrl state named "default".
OK, I will add a reference to the pinctrl bindings instead.
> However, this isn't a big deal; it's fine either way.
--
Thanks.
Sylwester
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 06/10] s5p-fimc: Use pinctrl API for camera ports configuration
@ 2013-02-08 23:30 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-08 23:30 UTC (permalink / raw)
To: linux-arm-kernel
On 02/07/2013 12:44 AM, Stephen Warren wrote:
> On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
>> Before the camera ports can be used the pinmux needs to be configured
>> properly. This patch adds a function to set the camera ports pinctrl
>> to a default state within the media driver's probe().
>> The camera port(s) are then configured for the video bus operation.
>
>> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>
>> +- pinctrl-names : pinctrl names for camera port pinmux control, at least
>> + "default" needs to be specified.
>> +- pinctrl-0...N : pinctrl properties corresponding to pinctrl-names
>> +
>
> A reference to the binding document describing the pin control bindings
> would be appropriate here. Given that reference, I'm not sure if
> spelling out the property names makes sense since it feels a little like
> duplication; an alternative might be simply:
>
> The pinctrl bindings defined in ../../../pinctrl/pinctrl-bindings.txt
> must be used to define a pinctrl state named "default".
OK, I will add a reference to the pinctrl bindings instead.
> However, this isn't a big deal; it's fine either way.
--
Thanks.
Sylwester
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-08 23:21 ` Stephen Warren
@ 2013-02-09 0:05 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-09 0:05 UTC (permalink / raw)
To: Stephen Warren
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/09/2013 12:21 AM, Stephen Warren wrote:
> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>
>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>> +----------------------------------------------
> ...
>>>> +For every fimc node a numbered alias should be present in the
>>>> aliases node.
>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>>> specifying
>>>> +the IP's instance index.
>>>
>>> Why? Isn't it up to the DT author whether they care if each fimc node is
>>> assigned a specific identification v.s. whether identification is
>>> assigned automatically?
>>
>> There are at least three different kinds of IPs that come in multiple
>> instances in an SoC. To activate data links between them each instance
>> needs to be clearly identified. There are also differences between
>> instances of same device. Hence it's important these aliases don't have
>> random values.
>>
>> Some more details about the SoC can be found at [1]. The aliases are
>> also already used in the Exynos5 GScaler bindings [2] in a similar way.
>
> Hmmm. I'd expect explicit DT properties to represent the
> instance-specific "configuration", or even different compatible values.
> Relying on the alias ID seems rather indirect; what if in e.g.
> Exynos6/... the mapping from instance/alias ID to feature set changes.
> With explicit DT properties, that'd just be a .dts change, whereas by
> requiring alias IDs now, you'd need a driver change to support this.
In the initial version of this patch series I used cell-index property,
but then Grant pointed out in some other mail thread it should be
avoided. Hence I used the node aliases.
Different compatible values might not work, when for example there
are 3 IPs out of 4 of one type and the fourth one of another type.
It wouldn't even by really different types, just quirks/little
differences between them, e.g. no data path routed to one of other IPs.
Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
MIPI-CSIS needs to be written to the FIMC.2 data input control register.
Even though MIPI-CSIS.N are same in terms of hardware structure they still
need to be distinguished as separate instances.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-09 0:05 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-09 0:05 UTC (permalink / raw)
To: linux-arm-kernel
On 02/09/2013 12:21 AM, Stephen Warren wrote:
> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>
>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>> +----------------------------------------------
> ...
>>>> +For every fimc node a numbered alias should be present in the
>>>> aliases node.
>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>>> specifying
>>>> +the IP's instance index.
>>>
>>> Why? Isn't it up to the DT author whether they care if each fimc node is
>>> assigned a specific identification v.s. whether identification is
>>> assigned automatically?
>>
>> There are at least three different kinds of IPs that come in multiple
>> instances in an SoC. To activate data links between them each instance
>> needs to be clearly identified. There are also differences between
>> instances of same device. Hence it's important these aliases don't have
>> random values.
>>
>> Some more details about the SoC can be found at [1]. The aliases are
>> also already used in the Exynos5 GScaler bindings [2] in a similar way.
>
> Hmmm. I'd expect explicit DT properties to represent the
> instance-specific "configuration", or even different compatible values.
> Relying on the alias ID seems rather indirect; what if in e.g.
> Exynos6/... the mapping from instance/alias ID to feature set changes.
> With explicit DT properties, that'd just be a .dts change, whereas by
> requiring alias IDs now, you'd need a driver change to support this.
In the initial version of this patch series I used cell-index property,
but then Grant pointed out in some other mail thread it should be
avoided. Hence I used the node aliases.
Different compatible values might not work, when for example there
are 3 IPs out of 4 of one type and the fourth one of another type.
It wouldn't even by really different types, just quirks/little
differences between them, e.g. no data path routed to one of other IPs.
Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
MIPI-CSIS needs to be written to the FIMC.2 data input control register.
Even though MIPI-CSIS.N are same in terms of hardware structure they still
need to be distinguished as separate instances.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 01/10] s5p-csis: Add device tree support
2013-02-08 23:27 ` Stephen Warren
@ 2013-02-09 0:31 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-09 0:31 UTC (permalink / raw)
To: Stephen Warren
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/09/2013 12:27 AM, Stephen Warren wrote:
> On 02/08/2013 03:29 PM, Sylwester Nawrocki wrote:
>> On 02/07/2013 12:36 AM, Stephen Warren wrote:
>>> On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
>>>> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
>>>> device. This patch support for binding the driver to the MIPI-CSIS
>>>> devices instantiated from device tree and for parsing all SoC and
>>>> board specific properties.
>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>>> b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>>>
>>>> +Optional properties:
>>>> +
>>>> +- clock-frequency : The IP's main (system bus) clock frequency in
>>>> Hz, default
>>>> + value when this property is not specified is 166 MHz;
>>>
>>> Shouldn't this be a "clocks" property, so that the driver can call
>>> clk_get(), clk_prepare_enable(), clk_get_rate(), etc. on it?
>>
>> Hi Stephen,
>>
>> Thanks for your review!
>>
>> I also use "clocks" and "clock-names" property, but didn't specify
>> it here, because the Exynos SoCs clocks driver is not in the mainline yet.
>
> I'm a bit sympathetic to this, since I've been trying to push Tegra
> towards the common clock framework and describing clock connectivity in
> DT, before adding new drivers that need clocks, specifically to avoid
> this kind of situation.
>
> The problem here is that if this gets merged now, then the clock driver
> comes along later, you'll have to change this binding definition to
> account for it, and DT bindings aren't supposed to be changed...
Yes, I wasn't quite sure if this is a really serious problem or not. There
is already quite a few drivers for the Exynos SoC IPs that have DT support
and their bindings will need to be changed when the new clocks driver
gets upstreamed...
> Do you have any clock driver at all upstream yet? Or, could you add a
> dummy clock driver to satisfy the driver's clkl_get() needs? If you do,
> you can always set up some AUXDATA so that clk_get() works for your
> driver right now, and then remove that once the complete clock driver is
> in place with full device tree support. That's how we've ended up
> handling this for Tegra drivers.
Yes, there is the clocks support upstream, only not using the common clock
API. And I used indeed AUXDATA in v3 of these patches.
The Exynos common clock API based driver was supposed to be merged in v3.9,
but it seems it won't happen. These patches also won't make it to 3.9.
Then hopefully both appear in 3.10 together.
I will add the clock properties to relevant nodes, assuming the new clocks
driver is available.
> Anyway, this is all mainly food-for-thought rather than an objection to
> the patch; I'd leave that to Grant/Rob if applicable.
:-) OK, thanks for the suggestions.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 01/10] s5p-csis: Add device tree support
@ 2013-02-09 0:31 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-09 0:31 UTC (permalink / raw)
To: linux-arm-kernel
On 02/09/2013 12:27 AM, Stephen Warren wrote:
> On 02/08/2013 03:29 PM, Sylwester Nawrocki wrote:
>> On 02/07/2013 12:36 AM, Stephen Warren wrote:
>>> On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote:
>>>> s5p-csis is platform device driver for MIPI-CSI frontend to the FIMC
>>>> device. This patch support for binding the driver to the MIPI-CSIS
>>>> devices instantiated from device tree and for parsing all SoC and
>>>> board specific properties.
>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>>> b/Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt
>>>
>>>> +Optional properties:
>>>> +
>>>> +- clock-frequency : The IP's main (system bus) clock frequency in
>>>> Hz, default
>>>> + value when this property is not specified is 166 MHz;
>>>
>>> Shouldn't this be a "clocks" property, so that the driver can call
>>> clk_get(), clk_prepare_enable(), clk_get_rate(), etc. on it?
>>
>> Hi Stephen,
>>
>> Thanks for your review!
>>
>> I also use "clocks" and "clock-names" property, but didn't specify
>> it here, because the Exynos SoCs clocks driver is not in the mainline yet.
>
> I'm a bit sympathetic to this, since I've been trying to push Tegra
> towards the common clock framework and describing clock connectivity in
> DT, before adding new drivers that need clocks, specifically to avoid
> this kind of situation.
>
> The problem here is that if this gets merged now, then the clock driver
> comes along later, you'll have to change this binding definition to
> account for it, and DT bindings aren't supposed to be changed...
Yes, I wasn't quite sure if this is a really serious problem or not. There
is already quite a few drivers for the Exynos SoC IPs that have DT support
and their bindings will need to be changed when the new clocks driver
gets upstreamed...
> Do you have any clock driver at all upstream yet? Or, could you add a
> dummy clock driver to satisfy the driver's clkl_get() needs? If you do,
> you can always set up some AUXDATA so that clk_get() works for your
> driver right now, and then remove that once the complete clock driver is
> in place with full device tree support. That's how we've ended up
> handling this for Tegra drivers.
Yes, there is the clocks support upstream, only not using the common clock
API. And I used indeed AUXDATA in v3 of these patches.
The Exynos common clock API based driver was supposed to be merged in v3.9,
but it seems it won't happen. These patches also won't make it to 3.9.
Then hopefully both appear in 3.10 together.
I will add the clock properties to relevant nodes, assuming the new clocks
driver is available.
> Anyway, this is all mainly food-for-thought rather than an objection to
> the patch; I'd leave that to Grant/Rob if applicable.
:-) OK, thanks for the suggestions.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-09 0:05 ` Sylwester Nawrocki
@ 2013-02-09 0:32 ` Stephen Warren
-1 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-09 0:32 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>
>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>> +----------------------------------------------
>> ...
>>>>> +For every fimc node a numbered alias should be present in the
>>>>> aliases node.
>>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>>>> specifying
>>>>> +the IP's instance index.
>>>>
>>>> Why? Isn't it up to the DT author whether they care if each fimc
>>>> node is
>>>> assigned a specific identification v.s. whether identification is
>>>> assigned automatically?
>>>
>>> There are at least three different kinds of IPs that come in multiple
>>> instances in an SoC. To activate data links between them each instance
>>> needs to be clearly identified. There are also differences between
>>> instances of same device. Hence it's important these aliases don't have
>>> random values.
>>>
>>> Some more details about the SoC can be found at [1]. The aliases are
>>> also already used in the Exynos5 GScaler bindings [2] in a similar way.
>>
>> Hmmm. I'd expect explicit DT properties to represent the
>> instance-specific "configuration", or even different compatible values.
>> Relying on the alias ID seems rather indirect; what if in e.g.
>> Exynos6/... the mapping from instance/alias ID to feature set changes.
>> With explicit DT properties, that'd just be a .dts change, whereas by
>> requiring alias IDs now, you'd need a driver change to support this.
>
> In the initial version of this patch series I used cell-index property,
> but then Grant pointed out in some other mail thread it should be
> avoided. Hence I used the node aliases.
To me, using cell-index is semantically equivalent to using the alias ID.
> Different compatible values might not work, when for example there
> are 3 IPs out of 4 of one type and the fourth one of another type.
> It wouldn't even by really different types, just quirks/little
> differences between them, e.g. no data path routed to one of other IPs.
I was thinking of using feature-/quirk-oriented properties. For example,
if there's a port on 3 of the 4 devices to connect to some other IP
block, simply include a boolean property to indicate whether that port
is present. It would be in 3 of the nodes but not the 4th.
> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
> Even though MIPI-CSIS.N are same in terms of hardware structure they still
> need to be distinguished as separate instances.
Oh, so you're using the alias ID as the value to write into the FIMC.2
register for that. I'm not 100% familiar with aliases, but they seem
like a more user-oriented naming thing to me, whereas values for hooking
up intra-SoC routing are an unrelated namespace semantically, even if
the values happen to line up right now. Perhaps rather than a Boolean
property I mentioned above, use a custom property to indicate the ID
that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems
similar to using cell-index, my *guess* is that Grant's objection to
using cell-index was more based on re-using cell-index for something
other than its intended purpose than pushing you to use an alias ID
rather than a property.
After all, what happens in some later SoC where you have two different
types of module that feed into the common module, such that type A
sources have IDs 0..3 in the common module, and type B sources have IDs
4..7 in the common module - you wouldn't want to require alias ISs 4..7
for the type B DT nodes.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-09 0:32 ` Stephen Warren
0 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-09 0:32 UTC (permalink / raw)
To: linux-arm-kernel
On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>
>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>> +----------------------------------------------
>> ...
>>>>> +For every fimc node a numbered alias should be present in the
>>>>> aliases node.
>>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>>>> specifying
>>>>> +the IP's instance index.
>>>>
>>>> Why? Isn't it up to the DT author whether they care if each fimc
>>>> node is
>>>> assigned a specific identification v.s. whether identification is
>>>> assigned automatically?
>>>
>>> There are at least three different kinds of IPs that come in multiple
>>> instances in an SoC. To activate data links between them each instance
>>> needs to be clearly identified. There are also differences between
>>> instances of same device. Hence it's important these aliases don't have
>>> random values.
>>>
>>> Some more details about the SoC can be found at [1]. The aliases are
>>> also already used in the Exynos5 GScaler bindings [2] in a similar way.
>>
>> Hmmm. I'd expect explicit DT properties to represent the
>> instance-specific "configuration", or even different compatible values.
>> Relying on the alias ID seems rather indirect; what if in e.g.
>> Exynos6/... the mapping from instance/alias ID to feature set changes.
>> With explicit DT properties, that'd just be a .dts change, whereas by
>> requiring alias IDs now, you'd need a driver change to support this.
>
> In the initial version of this patch series I used cell-index property,
> but then Grant pointed out in some other mail thread it should be
> avoided. Hence I used the node aliases.
To me, using cell-index is semantically equivalent to using the alias ID.
> Different compatible values might not work, when for example there
> are 3 IPs out of 4 of one type and the fourth one of another type.
> It wouldn't even by really different types, just quirks/little
> differences between them, e.g. no data path routed to one of other IPs.
I was thinking of using feature-/quirk-oriented properties. For example,
if there's a port on 3 of the 4 devices to connect to some other IP
block, simply include a boolean property to indicate whether that port
is present. It would be in 3 of the nodes but not the 4th.
> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
> Even though MIPI-CSIS.N are same in terms of hardware structure they still
> need to be distinguished as separate instances.
Oh, so you're using the alias ID as the value to write into the FIMC.2
register for that. I'm not 100% familiar with aliases, but they seem
like a more user-oriented naming thing to me, whereas values for hooking
up intra-SoC routing are an unrelated namespace semantically, even if
the values happen to line up right now. Perhaps rather than a Boolean
property I mentioned above, use a custom property to indicate the ID
that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems
similar to using cell-index, my *guess* is that Grant's objection to
using cell-index was more based on re-using cell-index for something
other than its intended purpose than pushing you to use an alias ID
rather than a property.
After all, what happens in some later SoC where you have two different
types of module that feed into the common module, such that type A
sources have IDs 0..3 in the common module, and type B sources have IDs
4..7 in the common module - you wouldn't want to require alias ISs 4..7
for the type B DT nodes.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-09 0:32 ` Stephen Warren
@ 2013-02-09 22:29 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-09 22:29 UTC (permalink / raw)
To: Stephen Warren
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/09/2013 01:32 AM, Stephen Warren wrote:
> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>
>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>> +----------------------------------------------
>>> ...
>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>> aliases node.
>>>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>>>>> specifying
>>>>>> +the IP's instance index.
>>>>>
>>>>> Why? Isn't it up to the DT author whether they care if each fimc
>>>>> node is
>>>>> assigned a specific identification v.s. whether identification is
>>>>> assigned automatically?
>>>>
>>>> There are at least three different kinds of IPs that come in multiple
>>>> instances in an SoC. To activate data links between them each instance
>>>> needs to be clearly identified. There are also differences between
>>>> instances of same device. Hence it's important these aliases don't have
>>>> random values.
>>>>
>>>> Some more details about the SoC can be found at [1]. The aliases are
>>>> also already used in the Exynos5 GScaler bindings [2] in a similar way.
>>>
>>> Hmmm. I'd expect explicit DT properties to represent the
>>> instance-specific "configuration", or even different compatible values.
>>> Relying on the alias ID seems rather indirect; what if in e.g.
>>> Exynos6/... the mapping from instance/alias ID to feature set changes.
>>> With explicit DT properties, that'd just be a .dts change, whereas by
>>> requiring alias IDs now, you'd need a driver change to support this.
>>
>> In the initial version of this patch series I used cell-index property,
>> but then Grant pointed out in some other mail thread it should be
>> avoided. Hence I used the node aliases.
>
> To me, using cell-index is semantically equivalent to using the alias ID.
I can't see significant difference either. I just switched to what
seemed to be used for similar purpose.
>> Different compatible values might not work, when for example there
>> are 3 IPs out of 4 of one type and the fourth one of another type.
>> It wouldn't even by really different types, just quirks/little
>> differences between them, e.g. no data path routed to one of other IPs.
>
> I was thinking of using feature-/quirk-oriented properties. For example,
> if there's a port on 3 of the 4 devices to connect to some other IP
> block, simply include a boolean property to indicate whether that port
> is present. It would be in 3 of the nodes but not the 4th.
Yes, I could add several properties corresponding to all members of this
[3] data structure. But still it is needed to clearly identify the IP
block in a set of the hardware instances.
>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
>> Even though MIPI-CSIS.N are same in terms of hardware structure they still
>> need to be distinguished as separate instances.
>
> Oh, so you're using the alias ID as the value to write into the FIMC.2
> register for that. I'm not 100% familiar with aliases, but they seem
> like a more user-oriented naming thing to me, whereas values for hooking
> up intra-SoC routing are an unrelated namespace semantically, even if
> the values happen to line up right now. Perhaps rather than a Boolean
> property I mentioned above, use a custom property to indicate the ID
> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems
That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
blocks are immutably connected to the SoC camera physical MIPI CSI-2
interfaces, and the physical camera ports have fixed assignment to the
MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS
nodes. And their instance index that is required for the top level
driver which exposes topology and the routing capabilities to user space
could be restored from the reg property value by subtracting a fixed
offset.
Similarly an instance index of FIMC-LITE could be derived from the value
of reg property in a port node that links it to FIMC-IS ISP. I have been
omitting these port/endpoint nodes because it seemed unnecessary to model
explicitly those data paths. However it feels a bit overkill to add them
just for the sake of identifying the IP block instance
Still I can't really see a possibility to drop indexes for the FIMC nodes.
> similar to using cell-index, my *guess* is that Grant's objection to
> using cell-index was more based on re-using cell-index for something
> other than its intended purpose than pushing you to use an alias ID
> rather than a property.
The comments to a patch for some other driver I was referring to can be
found at [1]. My first patch series appeared significantly later [2].
I confused things a bit, sorry about that.
I can see aliases used in bindings of multiple devices: uart, spi, sound
interfaces, gpio, ... And all bindings seem to impose some rules on how
their aliases are created.
> After all, what happens in some later SoC where you have two different
> types of module that feed into the common module, such that type A
> sources have IDs 0..3 in the common module, and type B sources have IDs
> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
> for the type B DT nodes.
There is no need to write alias ID directly into registers, and actually
it doesn't really happen. But we need to know that, for example camera A
is connected to physical MIPI CSI-2 channel 0 and to capture video with
DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to
MIPI-CSIS 0.
The system registers, where a sort of camera/display glue logic is
configured also refer to FIMC devices explicitly by ID. So the driver
as a client of the system registers block/glue logic interface needs
to pass an information which FIMC H/W instance it wants to be e.g.
attached/detached to/from some data pipeline.
I still need to design some API for the camera system registers (glue
logic) block. This would be shared by the V4L2 and the DRM FIMC driver.
I thought about a couple of function calls specific to Exynos platform
that would be called directly by related drivers.
[1]
https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-January/012128.html
[2] http://www.spinics.net/lists/linux-media/msg48341.html
[3]
http://lxr.linux.no/#linux+v3.7.6/drivers/media/platform/s5p-fimc/fimc-core.h#L365
[4] http://tinyurl.com/arczzuo
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-09 22:29 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-09 22:29 UTC (permalink / raw)
To: linux-arm-kernel
On 02/09/2013 01:32 AM, Stephen Warren wrote:
> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>
>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>> +----------------------------------------------
>>> ...
>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>> aliases node.
>>>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>>>>> specifying
>>>>>> +the IP's instance index.
>>>>>
>>>>> Why? Isn't it up to the DT author whether they care if each fimc
>>>>> node is
>>>>> assigned a specific identification v.s. whether identification is
>>>>> assigned automatically?
>>>>
>>>> There are at least three different kinds of IPs that come in multiple
>>>> instances in an SoC. To activate data links between them each instance
>>>> needs to be clearly identified. There are also differences between
>>>> instances of same device. Hence it's important these aliases don't have
>>>> random values.
>>>>
>>>> Some more details about the SoC can be found at [1]. The aliases are
>>>> also already used in the Exynos5 GScaler bindings [2] in a similar way.
>>>
>>> Hmmm. I'd expect explicit DT properties to represent the
>>> instance-specific "configuration", or even different compatible values.
>>> Relying on the alias ID seems rather indirect; what if in e.g.
>>> Exynos6/... the mapping from instance/alias ID to feature set changes.
>>> With explicit DT properties, that'd just be a .dts change, whereas by
>>> requiring alias IDs now, you'd need a driver change to support this.
>>
>> In the initial version of this patch series I used cell-index property,
>> but then Grant pointed out in some other mail thread it should be
>> avoided. Hence I used the node aliases.
>
> To me, using cell-index is semantically equivalent to using the alias ID.
I can't see significant difference either. I just switched to what
seemed to be used for similar purpose.
>> Different compatible values might not work, when for example there
>> are 3 IPs out of 4 of one type and the fourth one of another type.
>> It wouldn't even by really different types, just quirks/little
>> differences between them, e.g. no data path routed to one of other IPs.
>
> I was thinking of using feature-/quirk-oriented properties. For example,
> if there's a port on 3 of the 4 devices to connect to some other IP
> block, simply include a boolean property to indicate whether that port
> is present. It would be in 3 of the nodes but not the 4th.
Yes, I could add several properties corresponding to all members of this
[3] data structure. But still it is needed to clearly identify the IP
block in a set of the hardware instances.
>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
>> Even though MIPI-CSIS.N are same in terms of hardware structure they still
>> need to be distinguished as separate instances.
>
> Oh, so you're using the alias ID as the value to write into the FIMC.2
> register for that. I'm not 100% familiar with aliases, but they seem
> like a more user-oriented naming thing to me, whereas values for hooking
> up intra-SoC routing are an unrelated namespace semantically, even if
> the values happen to line up right now. Perhaps rather than a Boolean
> property I mentioned above, use a custom property to indicate the ID
> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems
That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
blocks are immutably connected to the SoC camera physical MIPI CSI-2
interfaces, and the physical camera ports have fixed assignment to the
MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS
nodes. And their instance index that is required for the top level
driver which exposes topology and the routing capabilities to user space
could be restored from the reg property value by subtracting a fixed
offset.
Similarly an instance index of FIMC-LITE could be derived from the value
of reg property in a port node that links it to FIMC-IS ISP. I have been
omitting these port/endpoint nodes because it seemed unnecessary to model
explicitly those data paths. However it feels a bit overkill to add them
just for the sake of identifying the IP block instance
Still I can't really see a possibility to drop indexes for the FIMC nodes.
> similar to using cell-index, my *guess* is that Grant's objection to
> using cell-index was more based on re-using cell-index for something
> other than its intended purpose than pushing you to use an alias ID
> rather than a property.
The comments to a patch for some other driver I was referring to can be
found at [1]. My first patch series appeared significantly later [2].
I confused things a bit, sorry about that.
I can see aliases used in bindings of multiple devices: uart, spi, sound
interfaces, gpio, ... And all bindings seem to impose some rules on how
their aliases are created.
> After all, what happens in some later SoC where you have two different
> types of module that feed into the common module, such that type A
> sources have IDs 0..3 in the common module, and type B sources have IDs
> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
> for the type B DT nodes.
There is no need to write alias ID directly into registers, and actually
it doesn't really happen. But we need to know that, for example camera A
is connected to physical MIPI CSI-2 channel 0 and to capture video with
DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to
MIPI-CSIS 0.
The system registers, where a sort of camera/display glue logic is
configured also refer to FIMC devices explicitly by ID. So the driver
as a client of the system registers block/glue logic interface needs
to pass an information which FIMC H/W instance it wants to be e.g.
attached/detached to/from some data pipeline.
I still need to design some API for the camera system registers (glue
logic) block. This would be shared by the V4L2 and the DRM FIMC driver.
I thought about a couple of function calls specific to Exynos platform
that would be called directly by related drivers.
[1]
https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-January/012128.html
[2] http://www.spinics.net/lists/linux-media/msg48341.html
[3]
http://lxr.linux.no/#linux+v3.7.6/drivers/media/platform/s5p-fimc/fimc-core.h#L365
[4] http://tinyurl.com/arczzuo
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-09 22:29 ` Sylwester Nawrocki
@ 2013-02-09 22:52 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-09 22:52 UTC (permalink / raw)
To: Stephen Warren
Cc: Sylwester Nawrocki, Sylwester Nawrocki, linux-media,
kyungmin.park, kgene.kim, rob.herring, prabhakar.lad,
devicetree-discuss, linux-samsung-soc, linux-arm-kernel
On 02/09/2013 11:29 PM, Sylwester Nawrocki wrote:
>
>> After all, what happens in some later SoC where you have two different
>> types of module that feed into the common module, such that type A
>> sources have IDs 0..3 in the common module, and type B sources have IDs
>> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
>> for the type B DT nodes.
I forgot to add, any ID remapping could happen in the common module, if
it requires it. Type A and type B sources could have indexes 0...3 and
the common module could derive its configuration from the source ID *and*
the source type. The idea behind aliases was to identify each instance,
rather than providing an exact configuration data that the common module
could use.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-09 22:52 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-09 22:52 UTC (permalink / raw)
To: linux-arm-kernel
On 02/09/2013 11:29 PM, Sylwester Nawrocki wrote:
>
>> After all, what happens in some later SoC where you have two different
>> types of module that feed into the common module, such that type A
>> sources have IDs 0..3 in the common module, and type B sources have IDs
>> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
>> for the type B DT nodes.
I forgot to add, any ID remapping could happen in the common module, if
it requires it. Type A and type B sources could have indexes 0...3 and
the common module could derive its configuration from the source ID *and*
the source type. The idea behind aliases was to identify each instance,
rather than providing an exact configuration data that the common module
could use.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-09 22:29 ` Sylwester Nawrocki
@ 2013-02-11 21:50 ` Stephen Warren
-1 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-11 21:50 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote:
> On 02/09/2013 01:32 AM, Stephen Warren wrote:
>> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>
>>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>>> +----------------------------------------------
>>>> ...
>>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>>> aliases node.
>>>>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>>>>>> specifying
>>>>>>> +the IP's instance index.
...
>>> Different compatible values might not work, when for example there
>>> are 3 IPs out of 4 of one type and the fourth one of another type.
>>> It wouldn't even by really different types, just quirks/little
>>> differences between them, e.g. no data path routed to one of other IPs.
>>
>> I was thinking of using feature-/quirk-oriented properties. For example,
>> if there's a port on 3 of the 4 devices to connect to some other IP
>> block, simply include a boolean property to indicate whether that port
>> is present. It would be in 3 of the nodes but not the 4th.
>
> Yes, I could add several properties corresponding to all members of this
> [3] data structure. But still it is needed to clearly identify the IP
> block in a set of the hardware instances.
Why? What decisions will be made based on the identify of the IP block
instance that wouldn't be covered by DT properties that describe which
features/bugs/... the IP block instance has?
>>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>>> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
>>> Even though MIPI-CSIS.N are same in terms of hardware structure they
>>> still
>>> need to be distinguished as separate instances.
>>
>> Oh, so you're using the alias ID as the value to write into the FIMC.2
>> register for that. I'm not 100% familiar with aliases, but they seem
>> like a more user-oriented naming thing to me, whereas values for hooking
>> up intra-SoC routing are an unrelated namespace semantically, even if
>> the values happen to line up right now. Perhaps rather than a Boolean
>> property I mentioned above, use a custom property to indicate the ID
>> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems
>
> That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
> links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
> blocks are immutably connected to the SoC camera physical MIPI CSI-2
> interfaces, and the physical camera ports have fixed assignment to the
> MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS
> nodes. And their instance index that is required for the top level
> driver which exposes topology and the routing capabilities to user space
> could be restored from the reg property value by subtracting a fixed
> offset.
I suppose that would work. It feels a little indirect, and I think means
that the driver needs to go find some child node defining its end of
some link, then find the node representing the other end of the link,
then read properties out of that other node to find the value. That
seems a little unusual, but I guess it would work. I'm not sure of the
long-term implications of doing that kind of thing. You'd want to run
the idea past some DT maintainers/experts.
...
> I can see aliases used in bindings of multiple devices: uart, spi, sound
> interfaces, gpio, ... And all bindings seem to impose some rules on how
> their aliases are created.
Do you have specific examples? I really don't think the bindings should
be dictating the alias values.
>> After all, what happens in some later SoC where you have two different
>> types of module that feed into the common module, such that type A
>> sources have IDs 0..3 in the common module, and type B sources have IDs
>> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
>> for the type B DT nodes.
>
> There is no need to write alias ID directly into registers, and actually
> it doesn't really happen. But we need to know that, for example camera A
> is connected to physical MIPI CSI-2 channel 0 and to capture video with
> DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to
> MIPI-CSIS 0.
OK, so the IDs are selecting which register to write, or which mux
settings to access. That's pretty much semantically the same thing.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-11 21:50 ` Stephen Warren
0 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-11 21:50 UTC (permalink / raw)
To: linux-arm-kernel
On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote:
> On 02/09/2013 01:32 AM, Stephen Warren wrote:
>> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>
>>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>>> +----------------------------------------------
>>>> ...
>>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>>> aliases node.
>>>>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>>>>>> specifying
>>>>>>> +the IP's instance index.
...
>>> Different compatible values might not work, when for example there
>>> are 3 IPs out of 4 of one type and the fourth one of another type.
>>> It wouldn't even by really different types, just quirks/little
>>> differences between them, e.g. no data path routed to one of other IPs.
>>
>> I was thinking of using feature-/quirk-oriented properties. For example,
>> if there's a port on 3 of the 4 devices to connect to some other IP
>> block, simply include a boolean property to indicate whether that port
>> is present. It would be in 3 of the nodes but not the 4th.
>
> Yes, I could add several properties corresponding to all members of this
> [3] data structure. But still it is needed to clearly identify the IP
> block in a set of the hardware instances.
Why? What decisions will be made based on the identify of the IP block
instance that wouldn't be covered by DT properties that describe which
features/bugs/... the IP block instance has?
>>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>>> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
>>> Even though MIPI-CSIS.N are same in terms of hardware structure they
>>> still
>>> need to be distinguished as separate instances.
>>
>> Oh, so you're using the alias ID as the value to write into the FIMC.2
>> register for that. I'm not 100% familiar with aliases, but they seem
>> like a more user-oriented naming thing to me, whereas values for hooking
>> up intra-SoC routing are an unrelated namespace semantically, even if
>> the values happen to line up right now. Perhaps rather than a Boolean
>> property I mentioned above, use a custom property to indicate the ID
>> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems
>
> That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
> links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
> blocks are immutably connected to the SoC camera physical MIPI CSI-2
> interfaces, and the physical camera ports have fixed assignment to the
> MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS
> nodes. And their instance index that is required for the top level
> driver which exposes topology and the routing capabilities to user space
> could be restored from the reg property value by subtracting a fixed
> offset.
I suppose that would work. It feels a little indirect, and I think means
that the driver needs to go find some child node defining its end of
some link, then find the node representing the other end of the link,
then read properties out of that other node to find the value. That
seems a little unusual, but I guess it would work. I'm not sure of the
long-term implications of doing that kind of thing. You'd want to run
the idea past some DT maintainers/experts.
...
> I can see aliases used in bindings of multiple devices: uart, spi, sound
> interfaces, gpio, ... And all bindings seem to impose some rules on how
> their aliases are created.
Do you have specific examples? I really don't think the bindings should
be dictating the alias values.
>> After all, what happens in some later SoC where you have two different
>> types of module that feed into the common module, such that type A
>> sources have IDs 0..3 in the common module, and type B sources have IDs
>> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
>> for the type B DT nodes.
>
> There is no need to write alias ID directly into registers, and actually
> it doesn't really happen. But we need to know that, for example camera A
> is connected to physical MIPI CSI-2 channel 0 and to capture video with
> DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to
> MIPI-CSIS 0.
OK, so the IDs are selecting which register to write, or which mux
settings to access. That's pretty much semantically the same thing.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-11 21:50 ` Stephen Warren
@ 2013-02-12 22:39 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-12 22:39 UTC (permalink / raw)
To: Stephen Warren
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/11/2013 10:50 PM, Stephen Warren wrote:
> On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote:
>> On 02/09/2013 01:32 AM, Stephen Warren wrote:
>>> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>>>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>
>>>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>>>> +----------------------------------------------
>>>>> ...
>>>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>>>> aliases node.
>>>>>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>>>>>>> specifying
>>>>>>>> +the IP's instance index.
> ...
>>>> Different compatible values might not work, when for example there
>>>> are 3 IPs out of 4 of one type and the fourth one of another type.
>>>> It wouldn't even by really different types, just quirks/little
>>>> differences between them, e.g. no data path routed to one of other IPs.
>>>
>>> I was thinking of using feature-/quirk-oriented properties. For example,
>>> if there's a port on 3 of the 4 devices to connect to some other IP
>>> block, simply include a boolean property to indicate whether that port
>>> is present. It would be in 3 of the nodes but not the 4th.
>>
>> Yes, I could add several properties corresponding to all members of this
>> [3] data structure. But still it is needed to clearly identify the IP
>> block in a set of the hardware instances.
>
> Why? What decisions will be made based on the identify of the IP block
> instance that wouldn't be covered by DT properties that describe which
> features/bugs/... the IP block instance has?
The whole subsystem topology is exposed to user space through the Media
Controller API. Although the user space libraries/applications using
this driver are not much concerned how the hardware is represented
internally in the kernel, some properties of the media entities seen
in user space are derived from the hardware details, e.g. the media entity
names contain index of a corresponding IP block. Please see [1] for
an example of a topology exposed by the driver.
Since different H/W instances have different capabilities, user space
libraries/plugins can be coded to e.g. use one instance for video playback
post-processing and another for camera capture. The capabilities could be
also discovered with the V4L2 API to some level of detail. But still
assigning random entity names to the IP blocks has a potential of breaking
user space or causing some malfunctions.
Perhaps I should just use a custom properties like "samsung,fimc-id" ?
I tried to represent some intra-soc data routing details with our common
video interfaces bindings and it really looked like a lot of unnecessary
nodes, with 11 camera sub-device nodes required to cover a front a rear
facing camera. Some details can be just coded in the driver, especially
that newer SoCs will get a new driver anyway, since there are huge
differences between the media subsystem architecture across subsequent
SoC revisions.
[1] http://www.spinics.net/lists/linux-media/attachments/psPhA96YX70U.ps
>>>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>>>> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
>>>> Even though MIPI-CSIS.N are same in terms of hardware structure they
>>>> still
>>>> need to be distinguished as separate instances.
>>>
>>> Oh, so you're using the alias ID as the value to write into the FIMC.2
>>> register for that. I'm not 100% familiar with aliases, but they seem
>>> like a more user-oriented naming thing to me, whereas values for hooking
>>> up intra-SoC routing are an unrelated namespace semantically, even if
>>> the values happen to line up right now. Perhaps rather than a Boolean
>>> property I mentioned above, use a custom property to indicate the ID
>>> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems
>>
>> That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
>> links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
>> blocks are immutably connected to the SoC camera physical MIPI CSI-2
>> interfaces, and the physical camera ports have fixed assignment to the
>> MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS
>> nodes. And their instance index that is required for the top level
>> driver which exposes topology and the routing capabilities to user space
>> could be restored from the reg property value by subtracting a fixed
>> offset.
>
> I suppose that would work. It feels a little indirect, and I think means
> that the driver needs to go find some child node defining its end of
> some link, then find the node representing the other end of the link,
> then read properties out of that other node to find the value. That
> seems a little unusual, but I guess it would work. I'm not sure of the
> long-term implications of doing that kind of thing. You'd want to run
> the idea past some DT maintainers/experts.
It's a bit simpler than that. We would need only to look for the reg
property in a local port subnode. MIPI-CSIS correspond to physical MIPI
CSI-2 bus interface of an SoC, hence it has to have specific reg values
that identify each camera input interface.
csis { /* MIPI CSI-2 Slave */
...
port {
reg = <1>; /* e.g. MIPI-CSIS.1 */
csis_ep: endpoint {
remote-endpoint = <&img_sensor_ep>;
}
};
};
i2c-controller {
...
image-sensor { /* MIPI CSI-2 Master */
...
port {
img_sensor_ep: endpoint {
remote-endpoint = <&csis_ep>;
}
};
};
};
Naturally the image-sensor node represents a device external to an SoC.
> ...
>> I can see aliases used in bindings of multiple devices: uart, spi, sound
>> interfaces, gpio, ... And all bindings seem to impose some rules on how
>> their aliases are created.
>
> Do you have specific examples? I really don't think the bindings should
> be dictating the alias values.
I just grepped through the existing bindings documentation:
gpio/gpio-mxs.txt:Note: Each GPIO port should have an alias correctly
numbered in "aliases"
gpio/gpio-mxs.txt-node.
serial/fsl-imx-uart.txt:Note: Each uart controller should have an alias
correctly numbered
serial/fsl-imx-uart.txt:in "aliases" node.
mmc/synopsis-dw-mshc.txt:- All the MSHC controller nodes should be
represented in the aliases node using
mmc/synopsis-dw-mshc.txt: the following format 'mshc{n}' where n is a
unique number for the alias.
sound/mxs-saif.txt:Note: Each SAIF controller should have an alias
correctly numbered
sound/mxs-saif.txt:in "aliases" node.
spi/spi-samsung.txt:- All the SPI controller nodes should be represented
in the aliases node using
spi/spi-samsung.txt: the following format 'spi{n}' where n is a unique
number for the alias.
tty/serial/fsl-mxs-auart.txt:Note: Each auart port should have an alias
correctly numbered in "aliases"
tty/serial/fsl-mxs-auart.txt-node.
powerpc/fsl/mpic-msgr.txt: An alias should be created for every
message register block. They are not
powerpc/fsl/mpic-msgr.txt- required, though. However, a particular
implementation of this binding
powerpc/fsl/mpic-msgr.txt: may require aliases to be present.
Aliases are of the form
powerpc/fsl/mpic-msgr.txt- 'mpic-msgr-block<n>', where <n> is an
integer specifying the block's number.
powerpc/fsl/mpic-msgr.txt- Numbers shall start at 0.
I think "correctly numbered" in the above statements means there are some
specific rules on how the aliases are created, however those seem not
clearly communicated.
And there is a new patch series that allows I2C bus controller enumeration
by means of the aliases:
http://www.spinics.net/lists/arm-kernel/msg224162.html
>>> After all, what happens in some later SoC where you have two different
>>> types of module that feed into the common module, such that type A
>>> sources have IDs 0..3 in the common module, and type B sources have IDs
>>> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
>>> for the type B DT nodes.
>>
>> There is no need to write alias ID directly into registers, and actually
>> it doesn't really happen. But we need to know that, for example camera A
>> is connected to physical MIPI CSI-2 channel 0 and to capture video with
>> DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to
>> MIPI-CSIS 0.
>
> OK, so the IDs are selecting which register to write, or which mux
> settings to access. That's pretty much semantically the same thing.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-12 22:39 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-12 22:39 UTC (permalink / raw)
To: linux-arm-kernel
On 02/11/2013 10:50 PM, Stephen Warren wrote:
> On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote:
>> On 02/09/2013 01:32 AM, Stephen Warren wrote:
>>> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>>>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>
>>>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>>>> +----------------------------------------------
>>>>> ...
>>>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>>>> aliases node.
>>>>>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N)
>>>>>>>> specifying
>>>>>>>> +the IP's instance index.
> ...
>>>> Different compatible values might not work, when for example there
>>>> are 3 IPs out of 4 of one type and the fourth one of another type.
>>>> It wouldn't even by really different types, just quirks/little
>>>> differences between them, e.g. no data path routed to one of other IPs.
>>>
>>> I was thinking of using feature-/quirk-oriented properties. For example,
>>> if there's a port on 3 of the 4 devices to connect to some other IP
>>> block, simply include a boolean property to indicate whether that port
>>> is present. It would be in 3 of the nodes but not the 4th.
>>
>> Yes, I could add several properties corresponding to all members of this
>> [3] data structure. But still it is needed to clearly identify the IP
>> block in a set of the hardware instances.
>
> Why? What decisions will be made based on the identify of the IP block
> instance that wouldn't be covered by DT properties that describe which
> features/bugs/... the IP block instance has?
The whole subsystem topology is exposed to user space through the Media
Controller API. Although the user space libraries/applications using
this driver are not much concerned how the hardware is represented
internally in the kernel, some properties of the media entities seen
in user space are derived from the hardware details, e.g. the media entity
names contain index of a corresponding IP block. Please see [1] for
an example of a topology exposed by the driver.
Since different H/W instances have different capabilities, user space
libraries/plugins can be coded to e.g. use one instance for video playback
post-processing and another for camera capture. The capabilities could be
also discovered with the V4L2 API to some level of detail. But still
assigning random entity names to the IP blocks has a potential of breaking
user space or causing some malfunctions.
Perhaps I should just use a custom properties like "samsung,fimc-id" ?
I tried to represent some intra-soc data routing details with our common
video interfaces bindings and it really looked like a lot of unnecessary
nodes, with 11 camera sub-device nodes required to cover a front a rear
facing camera. Some details can be just coded in the driver, especially
that newer SoCs will get a new driver anyway, since there are huge
differences between the media subsystem architecture across subsequent
SoC revisions.
[1] http://www.spinics.net/lists/linux-media/attachments/psPhA96YX70U.ps
>>>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>>>> MIPI-CSIS needs to be written to the FIMC.2 data input control register.
>>>> Even though MIPI-CSIS.N are same in terms of hardware structure they
>>>> still
>>>> need to be distinguished as separate instances.
>>>
>>> Oh, so you're using the alias ID as the value to write into the FIMC.2
>>> register for that. I'm not 100% familiar with aliases, but they seem
>>> like a more user-oriented naming thing to me, whereas values for hooking
>>> up intra-SoC routing are an unrelated namespace semantically, even if
>>> the values happen to line up right now. Perhaps rather than a Boolean
>>> property I mentioned above, use a custom property to indicate the ID
>>> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems
>>
>> That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
>> links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
>> blocks are immutably connected to the SoC camera physical MIPI CSI-2
>> interfaces, and the physical camera ports have fixed assignment to the
>> MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS
>> nodes. And their instance index that is required for the top level
>> driver which exposes topology and the routing capabilities to user space
>> could be restored from the reg property value by subtracting a fixed
>> offset.
>
> I suppose that would work. It feels a little indirect, and I think means
> that the driver needs to go find some child node defining its end of
> some link, then find the node representing the other end of the link,
> then read properties out of that other node to find the value. That
> seems a little unusual, but I guess it would work. I'm not sure of the
> long-term implications of doing that kind of thing. You'd want to run
> the idea past some DT maintainers/experts.
It's a bit simpler than that. We would need only to look for the reg
property in a local port subnode. MIPI-CSIS correspond to physical MIPI
CSI-2 bus interface of an SoC, hence it has to have specific reg values
that identify each camera input interface.
csis { /* MIPI CSI-2 Slave */
...
port {
reg = <1>; /* e.g. MIPI-CSIS.1 */
csis_ep: endpoint {
remote-endpoint = <&img_sensor_ep>;
}
};
};
i2c-controller {
...
image-sensor { /* MIPI CSI-2 Master */
...
port {
img_sensor_ep: endpoint {
remote-endpoint = <&csis_ep>;
}
};
};
};
Naturally the image-sensor node represents a device external to an SoC.
> ...
>> I can see aliases used in bindings of multiple devices: uart, spi, sound
>> interfaces, gpio, ... And all bindings seem to impose some rules on how
>> their aliases are created.
>
> Do you have specific examples? I really don't think the bindings should
> be dictating the alias values.
I just grepped through the existing bindings documentation:
gpio/gpio-mxs.txt:Note: Each GPIO port should have an alias correctly
numbered in "aliases"
gpio/gpio-mxs.txt-node.
serial/fsl-imx-uart.txt:Note: Each uart controller should have an alias
correctly numbered
serial/fsl-imx-uart.txt:in "aliases" node.
mmc/synopsis-dw-mshc.txt:- All the MSHC controller nodes should be
represented in the aliases node using
mmc/synopsis-dw-mshc.txt: the following format 'mshc{n}' where n is a
unique number for the alias.
sound/mxs-saif.txt:Note: Each SAIF controller should have an alias
correctly numbered
sound/mxs-saif.txt:in "aliases" node.
spi/spi-samsung.txt:- All the SPI controller nodes should be represented
in the aliases node using
spi/spi-samsung.txt: the following format 'spi{n}' where n is a unique
number for the alias.
tty/serial/fsl-mxs-auart.txt:Note: Each auart port should have an alias
correctly numbered in "aliases"
tty/serial/fsl-mxs-auart.txt-node.
powerpc/fsl/mpic-msgr.txt: An alias should be created for every
message register block. They are not
powerpc/fsl/mpic-msgr.txt- required, though. However, a particular
implementation of this binding
powerpc/fsl/mpic-msgr.txt: may require aliases to be present.
Aliases are of the form
powerpc/fsl/mpic-msgr.txt- 'mpic-msgr-block<n>', where <n> is an
integer specifying the block's number.
powerpc/fsl/mpic-msgr.txt- Numbers shall start at 0.
I think "correctly numbered" in the above statements means there are some
specific rules on how the aliases are created, however those seem not
clearly communicated.
And there is a new patch series that allows I2C bus controller enumeration
by means of the aliases:
http://www.spinics.net/lists/arm-kernel/msg224162.html
>>> After all, what happens in some later SoC where you have two different
>>> types of module that feed into the common module, such that type A
>>> sources have IDs 0..3 in the common module, and type B sources have IDs
>>> 4..7 in the common module - you wouldn't want to require alias ISs 4..7
>>> for the type B DT nodes.
>>
>> There is no need to write alias ID directly into registers, and actually
>> it doesn't really happen. But we need to know that, for example camera A
>> is connected to physical MIPI CSI-2 channel 0 and to capture video with
>> DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to
>> MIPI-CSIS 0.
>
> OK, so the IDs are selecting which register to write, or which mux
> settings to access. That's pretty much semantically the same thing.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-12 22:39 ` Sylwester Nawrocki
@ 2013-02-13 20:42 ` Stephen Warren
-1 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-13 20:42 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/12/2013 03:39 PM, Sylwester Nawrocki wrote:
> On 02/11/2013 10:50 PM, Stephen Warren wrote:
>> On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote:
>>> On 02/09/2013 01:32 AM, Stephen Warren wrote:
>>>> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>>>>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>>>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>>
>>>>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>>>>> +----------------------------------------------
>>>>>> ...
>>>>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>>>>> aliases node.
>>>>>>>>> +Aliases are of the form fimc<n>, where<n> is an integer
>>>>>>>>> (0...N)
>>>>>>>>> specifying
>>>>>>>>> +the IP's instance index.
>> ...
>>>>> Different compatible values might not work, when for example there
>>>>> are 3 IPs out of 4 of one type and the fourth one of another type.
>>>>> It wouldn't even by really different types, just quirks/little
>>>>> differences between them, e.g. no data path routed to one of other
>>>>> IPs.
>>>>
>>>> I was thinking of using feature-/quirk-oriented properties. For
>>>> example,
>>>> if there's a port on 3 of the 4 devices to connect to some other IP
>>>> block, simply include a boolean property to indicate whether that port
>>>> is present. It would be in 3 of the nodes but not the 4th.
>>>
>>> Yes, I could add several properties corresponding to all members of this
>>> [3] data structure. But still it is needed to clearly identify the IP
>>> block in a set of the hardware instances.
>>
>> Why? What decisions will be made based on the identify of the IP block
>> instance that wouldn't be covered by DT properties that describe which
>> features/bugs/... the IP block instance has?
>
> The whole subsystem topology is exposed to user space through the Media
> Controller API.
OK, stable user-visible names are a reasonable use for device tree. I
still don't think you should use those user-visible IDs for making any
other kind of decision though.
>>>>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>>>>> MIPI-CSIS needs to be written to the FIMC.2 data input control
>>>>> register.
>>>>> Even though MIPI-CSIS.N are same in terms of hardware structure they
>>>>> still
>>>>> need to be distinguished as separate instances.
>>>>
>>>> Oh, so you're using the alias ID as the value to write into the FIMC.2
>>>> register for that. I'm not 100% familiar with aliases, but they seem
>>>> like a more user-oriented naming thing to me, whereas values for
>>>> hooking
>>>> up intra-SoC routing are an unrelated namespace semantically, even if
>>>> the values happen to line up right now. Perhaps rather than a Boolean
>>>> property I mentioned above, use a custom property to indicate the ID
>>>> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this
>>>> seems
>>>
>>> That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
>>> links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
>>> blocks are immutably connected to the SoC camera physical MIPI CSI-2
>>> interfaces, and the physical camera ports have fixed assignment to the
>>> MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS
>>> nodes. And their instance index that is required for the top level
>>> driver which exposes topology and the routing capabilities to user space
>>> could be restored from the reg property value by subtracting a fixed
>>> offset.
>>
>> I suppose that would work. It feels a little indirect, and I think means
>> that the driver needs to go find some child node defining its end of
>> some link, then find the node representing the other end of the link,
>> then read properties out of that other node to find the value. That
>> seems a little unusual, but I guess it would work. I'm not sure of the
>> long-term implications of doing that kind of thing. You'd want to run
>> the idea past some DT maintainers/experts.
>
> It's a bit simpler than that. We would need only to look for the reg
> property in a local port subnode. MIPI-CSIS correspond to physical MIPI
> CSI-2 bus interface of an SoC, hence it has to have specific reg values
> that identify each camera input interface.
Oh I see. I guess if a device is using its own node to determine its own
identify, that's reasonable.
I thought you were talking about a situation like:
FIMC <--> XXX
where FIMC wanted to determine what ID XXX knew that particular FIMC as.
>>> I can see aliases used in bindings of multiple devices: uart, spi, sound
>>> interfaces, gpio, ... And all bindings seem to impose some rules on how
>>> their aliases are created.
>>
>> Do you have specific examples? I really don't think the bindings should
>> be dictating the alias values.
>
> I just grepped through the existing bindings documentation:
...
> I think "correctly numbered" in the above statements means there are some
> specific rules on how the aliases are created, however those seem not
> clearly communicated.
A binding specifying that an alias must (or even should) exist for each
node seems odd to me. In the absence of an explicit rule for how to
determine the alias IDs to use, I think the rule would simply be that
the aliases must be unique?
> And there is a new patch series that allows I2C bus controller enumeration
> by means of the aliases:
>
> http://www.spinics.net/lists/arm-kernel/msg224162.html
That's not enumerating controllers by alias (they're still enumerated by
scanning the DT nodes for buses in the normal way). The change simply
assigns the bus ID of each controller from an alias; exactly what
aliases are for.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-13 20:42 ` Stephen Warren
0 siblings, 0 replies; 60+ messages in thread
From: Stephen Warren @ 2013-02-13 20:42 UTC (permalink / raw)
To: linux-arm-kernel
On 02/12/2013 03:39 PM, Sylwester Nawrocki wrote:
> On 02/11/2013 10:50 PM, Stephen Warren wrote:
>> On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote:
>>> On 02/09/2013 01:32 AM, Stephen Warren wrote:
>>>> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote:
>>>>> On 02/09/2013 12:21 AM, Stephen Warren wrote:
>>>>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote:
>>>>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote:
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt
>>>>>>>>
>>>>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC)
>>>>>>>>> +----------------------------------------------
>>>>>> ...
>>>>>>>>> +For every fimc node a numbered alias should be present in the
>>>>>>>>> aliases node.
>>>>>>>>> +Aliases are of the form fimc<n>, where<n> is an integer
>>>>>>>>> (0...N)
>>>>>>>>> specifying
>>>>>>>>> +the IP's instance index.
>> ...
>>>>> Different compatible values might not work, when for example there
>>>>> are 3 IPs out of 4 of one type and the fourth one of another type.
>>>>> It wouldn't even by really different types, just quirks/little
>>>>> differences between them, e.g. no data path routed to one of other
>>>>> IPs.
>>>>
>>>> I was thinking of using feature-/quirk-oriented properties. For
>>>> example,
>>>> if there's a port on 3 of the 4 devices to connect to some other IP
>>>> block, simply include a boolean property to indicate whether that port
>>>> is present. It would be in 3 of the nodes but not the 4th.
>>>
>>> Yes, I could add several properties corresponding to all members of this
>>> [3] data structure. But still it is needed to clearly identify the IP
>>> block in a set of the hardware instances.
>>
>> Why? What decisions will be made based on the identify of the IP block
>> instance that wouldn't be covered by DT properties that describe which
>> features/bugs/... the IP block instance has?
>
> The whole subsystem topology is exposed to user space through the Media
> Controller API.
OK, stable user-visible names are a reasonable use for device tree. I
still don't think you should use those user-visible IDs for making any
other kind of decision though.
>>>>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the
>>>>> MIPI-CSIS needs to be written to the FIMC.2 data input control
>>>>> register.
>>>>> Even though MIPI-CSIS.N are same in terms of hardware structure they
>>>>> still
>>>>> need to be distinguished as separate instances.
>>>>
>>>> Oh, so you're using the alias ID as the value to write into the FIMC.2
>>>> register for that. I'm not 100% familiar with aliases, but they seem
>>>> like a more user-oriented naming thing to me, whereas values for
>>>> hooking
>>>> up intra-SoC routing are an unrelated namespace semantically, even if
>>>> the values happen to line up right now. Perhaps rather than a Boolean
>>>> property I mentioned above, use a custom property to indicate the ID
>>>> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this
>>>> seems
>>>
>>> That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that
>>> links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP
>>> blocks are immutably connected to the SoC camera physical MIPI CSI-2
>>> interfaces, and the physical camera ports have fixed assignment to the
>>> MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS
>>> nodes. And their instance index that is required for the top level
>>> driver which exposes topology and the routing capabilities to user space
>>> could be restored from the reg property value by subtracting a fixed
>>> offset.
>>
>> I suppose that would work. It feels a little indirect, and I think means
>> that the driver needs to go find some child node defining its end of
>> some link, then find the node representing the other end of the link,
>> then read properties out of that other node to find the value. That
>> seems a little unusual, but I guess it would work. I'm not sure of the
>> long-term implications of doing that kind of thing. You'd want to run
>> the idea past some DT maintainers/experts.
>
> It's a bit simpler than that. We would need only to look for the reg
> property in a local port subnode. MIPI-CSIS correspond to physical MIPI
> CSI-2 bus interface of an SoC, hence it has to have specific reg values
> that identify each camera input interface.
Oh I see. I guess if a device is using its own node to determine its own
identify, that's reasonable.
I thought you were talking about a situation like:
FIMC <--> XXX
where FIMC wanted to determine what ID XXX knew that particular FIMC as.
>>> I can see aliases used in bindings of multiple devices: uart, spi, sound
>>> interfaces, gpio, ... And all bindings seem to impose some rules on how
>>> their aliases are created.
>>
>> Do you have specific examples? I really don't think the bindings should
>> be dictating the alias values.
>
> I just grepped through the existing bindings documentation:
...
> I think "correctly numbered" in the above statements means there are some
> specific rules on how the aliases are created, however those seem not
> clearly communicated.
A binding specifying that an alias must (or even should) exist for each
node seems odd to me. In the absence of an explicit rule for how to
determine the alias IDs to use, I think the rule would simply be that
the aliases must be unique?
> And there is a new patch series that allows I2C bus controller enumeration
> by means of the aliases:
>
> http://www.spinics.net/lists/arm-kernel/msg224162.html
That's not enumerating controllers by alias (they're still enumerated by
scanning the DT nodes for buses in the normal way). The change simply
assigns the bus ID of each controller from an alias; exactly what
aliases are for.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
2013-02-13 20:42 ` Stephen Warren
@ 2013-02-14 23:03 ` Sylwester Nawrocki
-1 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-14 23:03 UTC (permalink / raw)
To: Stephen Warren
Cc: Sylwester Nawrocki, linux-media, kyungmin.park, kgene.kim,
rob.herring, prabhakar.lad, devicetree-discuss,
linux-samsung-soc, linux-arm-kernel
On 02/13/2013 09:42 PM, Stephen Warren wrote:
> On 02/12/2013 03:39 PM, Sylwester Nawrocki wrote:
[...]
>> The whole subsystem topology is exposed to user space through the Media
>> Controller API.
>
> OK, stable user-visible names are a reasonable use for device tree. I
> still don't think you should use those user-visible IDs for making any
> other kind of decision though.
OK, I will update the bindings so all variant details are placed in the
device tree. Then the routing information would mostly be coming from the
device specific dt properties/the common media bindings and the state of
links between the media entities, set by the user.
>> It's a bit simpler than that. We would need only to look for the reg
>> property in a local port subnode. MIPI-CSIS correspond to physical MIPI
>> CSI-2 bus interface of an SoC, hence it has to have specific reg values
>> that identify each camera input interface.
>
> Oh I see. I guess if a device is using its own node to determine its own
> identify, that's reasonable.
OK, I'm going to post an updated patch series in a week or two.
> I thought you were talking about a situation like:
>
> FIMC <--> XXX
>
> where FIMC wanted to determine what ID XXX knew that particular FIMC as.
Ah, no. Sorry for the poor explanation. FIMC are on a sort if interconnect
bus and they can be attached to a single data source, even in parallel,
and the data source entity don't even need to be fully aware of it.
>>>> I can see aliases used in bindings of multiple devices: uart, spi, sound
>>>> interfaces, gpio, ... And all bindings seem to impose some rules on how
>>>> their aliases are created.
>>>
>>> Do you have specific examples? I really don't think the bindings should
>>> be dictating the alias values.
>>
>> I just grepped through the existing bindings documentation:
> ...
>> I think "correctly numbered" in the above statements means there are some
>> specific rules on how the aliases are created, however those seem not
>> clearly communicated.
>
> A binding specifying that an alias must (or even should) exist for each
> node seems odd to me. In the absence of an explicit rule for how to
> determine the alias IDs to use, I think the rule would simply be that
> the aliases must be unique?
I guess so. Inspecting of_alias_get_id() call sites tells us that most
drivers
just fail when alias is not present and only rarely it is not treated as an
error condition.
>> And there is a new patch series that allows I2C bus controller enumeration
>> by means of the aliases:
>>
>> http://www.spinics.net/lists/arm-kernel/msg224162.html
>
> That's not enumerating controllers by alias (they're still enumerated by
> scanning the DT nodes for buses in the normal way). The change simply
> assigns the bus ID of each controller from an alias; exactly what
> aliases are for.
OK, that clarifies a bit my understanding of the aliases.
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices
@ 2013-02-14 23:03 ` Sylwester Nawrocki
0 siblings, 0 replies; 60+ messages in thread
From: Sylwester Nawrocki @ 2013-02-14 23:03 UTC (permalink / raw)
To: linux-arm-kernel
On 02/13/2013 09:42 PM, Stephen Warren wrote:
> On 02/12/2013 03:39 PM, Sylwester Nawrocki wrote:
[...]
>> The whole subsystem topology is exposed to user space through the Media
>> Controller API.
>
> OK, stable user-visible names are a reasonable use for device tree. I
> still don't think you should use those user-visible IDs for making any
> other kind of decision though.
OK, I will update the bindings so all variant details are placed in the
device tree. Then the routing information would mostly be coming from the
device specific dt properties/the common media bindings and the state of
links between the media entities, set by the user.
>> It's a bit simpler than that. We would need only to look for the reg
>> property in a local port subnode. MIPI-CSIS correspond to physical MIPI
>> CSI-2 bus interface of an SoC, hence it has to have specific reg values
>> that identify each camera input interface.
>
> Oh I see. I guess if a device is using its own node to determine its own
> identify, that's reasonable.
OK, I'm going to post an updated patch series in a week or two.
> I thought you were talking about a situation like:
>
> FIMC <--> XXX
>
> where FIMC wanted to determine what ID XXX knew that particular FIMC as.
Ah, no. Sorry for the poor explanation. FIMC are on a sort if interconnect
bus and they can be attached to a single data source, even in parallel,
and the data source entity don't even need to be fully aware of it.
>>>> I can see aliases used in bindings of multiple devices: uart, spi, sound
>>>> interfaces, gpio, ... And all bindings seem to impose some rules on how
>>>> their aliases are created.
>>>
>>> Do you have specific examples? I really don't think the bindings should
>>> be dictating the alias values.
>>
>> I just grepped through the existing bindings documentation:
> ...
>> I think "correctly numbered" in the above statements means there are some
>> specific rules on how the aliases are created, however those seem not
>> clearly communicated.
>
> A binding specifying that an alias must (or even should) exist for each
> node seems odd to me. In the absence of an explicit rule for how to
> determine the alias IDs to use, I think the rule would simply be that
> the aliases must be unique?
I guess so. Inspecting of_alias_get_id() call sites tells us that most
drivers
just fail when alias is not present and only rarely it is not treated as an
error condition.
>> And there is a new patch series that allows I2C bus controller enumeration
>> by means of the aliases:
>>
>> http://www.spinics.net/lists/arm-kernel/msg224162.html
>
> That's not enumerating controllers by alias (they're still enumerated by
> scanning the DT nodes for buses in the normal way). The change simply
> assigns the bus ID of each controller from an alias; exactly what
> aliases are for.
OK, that clarifies a bit my understanding of the aliases.
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 60+ messages in thread
end of thread, other threads:[~2013-02-14 23:03 UTC | newest]
Thread overview: 60+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-01 19:09 [PATCH v4 00/10] Device tree support for Exynos SoC camera subsystem Sylwester Nawrocki
2013-02-01 19:09 ` Sylwester Nawrocki
2013-02-01 19:09 ` [PATCH v4 01/10] s5p-csis: Add device tree support Sylwester Nawrocki
2013-02-01 19:09 ` Sylwester Nawrocki
2013-02-06 23:36 ` Stephen Warren
2013-02-06 23:36 ` Stephen Warren
2013-02-08 22:29 ` Sylwester Nawrocki
2013-02-08 22:29 ` Sylwester Nawrocki
2013-02-08 23:27 ` Stephen Warren
2013-02-08 23:27 ` Stephen Warren
2013-02-09 0:31 ` Sylwester Nawrocki
2013-02-09 0:31 ` Sylwester Nawrocki
2013-02-01 19:09 ` [PATCH v4 02/10] s5p-fimc: Add device tree support for FIMC devices Sylwester Nawrocki
2013-02-01 19:09 ` Sylwester Nawrocki
2013-02-06 23:40 ` Stephen Warren
2013-02-06 23:40 ` Stephen Warren
2013-02-08 23:16 ` Sylwester Nawrocki
2013-02-08 23:16 ` Sylwester Nawrocki
2013-02-08 23:21 ` Stephen Warren
2013-02-08 23:21 ` Stephen Warren
2013-02-09 0:05 ` Sylwester Nawrocki
2013-02-09 0:05 ` Sylwester Nawrocki
2013-02-09 0:32 ` Stephen Warren
2013-02-09 0:32 ` Stephen Warren
2013-02-09 22:29 ` Sylwester Nawrocki
2013-02-09 22:29 ` Sylwester Nawrocki
2013-02-09 22:52 ` Sylwester Nawrocki
2013-02-09 22:52 ` Sylwester Nawrocki
2013-02-11 21:50 ` Stephen Warren
2013-02-11 21:50 ` Stephen Warren
2013-02-12 22:39 ` Sylwester Nawrocki
2013-02-12 22:39 ` Sylwester Nawrocki
2013-02-13 20:42 ` Stephen Warren
2013-02-13 20:42 ` Stephen Warren
2013-02-14 23:03 ` Sylwester Nawrocki
2013-02-14 23:03 ` Sylwester Nawrocki
2013-02-01 19:09 ` [PATCH v4 03/10] s5p-fimc: Add device tree support for FIMC-LITE devices Sylwester Nawrocki
2013-02-01 19:09 ` Sylwester Nawrocki
2013-02-01 19:09 ` [PATCH v4 04/10] s5p-fimc: Add device tree support for the main media device driver Sylwester Nawrocki
2013-02-01 19:09 ` Sylwester Nawrocki
2013-02-01 19:09 ` [PATCH v4 05/10] s5p-fimc: Add device tree based sensors registration Sylwester Nawrocki
2013-02-01 19:09 ` Sylwester Nawrocki
2013-02-06 23:42 ` Stephen Warren
2013-02-06 23:42 ` Stephen Warren
2013-02-08 23:26 ` Sylwester Nawrocki
2013-02-08 23:26 ` Sylwester Nawrocki
2013-02-01 19:09 ` [PATCH v4 06/10] s5p-fimc: Use pinctrl API for camera ports configuration Sylwester Nawrocki
2013-02-01 19:09 ` Sylwester Nawrocki
2013-02-06 23:44 ` Stephen Warren
2013-02-06 23:44 ` Stephen Warren
2013-02-08 23:30 ` Sylwester Nawrocki
2013-02-08 23:30 ` Sylwester Nawrocki
2013-02-01 19:09 ` [PATCH v4 07/10] ARM: dts: Add camera to node exynos4.dtsi Sylwester Nawrocki
2013-02-01 19:09 ` Sylwester Nawrocki
2013-02-01 19:09 ` [PATCH v4 08/10] ARM: dts: Add ISP power domain node for Exynos4x12 Sylwester Nawrocki
2013-02-01 19:09 ` Sylwester Nawrocki
2013-02-01 19:09 ` [PATCH v4 09/10] ARM: dts: Add FIMC and MIPI CSIS device nodes " Sylwester Nawrocki
2013-02-01 19:09 ` Sylwester Nawrocki
2013-02-01 19:09 ` [PATCH v4 10/10] ARM: dts: Correct camera pinctrl nodes for Exynos4x12 SoCs Sylwester Nawrocki
2013-02-01 19:09 ` Sylwester Nawrocki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.