Re: [RFC v3 01/13] [media] exynos5-is: Adding media device driver for exynos5
Hi Sylwester,
Thank you for the review.
Will address all your review comments.
Some responses below:
[snip]
+
+static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
+{
+ struct device_node *of_node = fmd-pdev-dev.of_node;
+ int ret;
+
+ /*
+* Runtime resume one of the FIMC entities to make sure
+* the sclk_cam clocks are not globally disabled.
It's a bit mysterious to me, is this requirement still valid on Exynos5 ?
I glanced over the Exynos5250 datasheet and there seem to be no sclk_cam?
clocks dependency on any of GScaler clocks. Maybe you don't need a clock
provider in this driver, perhaps sensor drivers could use sclk_cam clocks
directly, assigned through dts ?
Yes these clocks can be directly exposed via dt.
I will drop clock provider from this driver.
[snip]
+/*
+ * The peripheral sensor clock management.
+ */
+static void fimc_md_put_clocks(struct fimc_md *fmd)
+{
+ int i = FIMC_MAX_CAMCLKS;
+
+ while (--i= 0) {
+ if (IS_ERR(fmd-camclk[i].clock))
+ continue;
+ clk_put(fmd-camclk[i].clock);
+ fmd-camclk[i].clock = ERR_PTR(-EINVAL);
+ }
Please double check if you need this sclk_cam clocks handling. We could
simply add a requirement that this driver supports only sensor subdevs
through the v4l2-async API and which controls their clock themselves.
sclk_cam* handling can be removed and be done from respective
sensors. But I think the sclk_bayer handling needs to be retained in the
media driver.
+}
+
+static int fimc_md_get_clocks(struct fimc_md *fmd)
+{
+ struct device *dev = NULL;
+ char clk_name[32];
+ struct clk *clock;
+ int i, ret = 0;
+
+ for (i = 0; i FIMC_MAX_CAMCLKS; i++)
+ fmd-camclk[i].clock = ERR_PTR(-EINVAL);
+
+ if (fmd-pdev-dev.of_node)
+ dev =fmd-pdev-dev;
+
+ for (i = 0; i SCLK_BAYER; i++) {
+ snprintf(clk_name, sizeof(clk_name), sclk_cam%u, i);
+ clock = clk_get(dev, clk_name);
+
+ if (IS_ERR(clock)) {
+ dev_err(fmd-pdev-dev, Failed to get clock:
%s\n,
+ clk_name);
+ ret = PTR_ERR(clock);
+ break;
+ }
+ fmd-camclk[i].clock = clock;
+ }
+ if (ret)
+ fimc_md_put_clocks(fmd);
+
+ /* Prepare bayer clk */
+ clock = clk_get(dev, sclk_bayer);
+
+ if (IS_ERR(clock)) {
+ dev_err(fmd-pdev-dev, Failed to get clock: %s\n,
+ clk_name);
Wrong error message.
+ ret = PTR_ERR(clock);
+ goto err_exit;
+ }
+ ret = clk_prepare(clock);
+ if (ret 0) {
+ clk_put(clock);
+ fmd-camclk[SCLK_BAYER].clock = ERR_PTR(-EINVAL);
+ goto err_exit;
+ }
+ fmd-camclk[SCLK_BAYER].clock = clock;
Could you explain a bit how is this SCLK_BAYER clock used ? Is it routed
to external image sensor, or is it used only inside an SoC ?
It is not defined properly in the manual, but I suppose its the bus clock
for the bayer rgb data bus. So for proper sensor functionality, we need this
sclk_bayer in addition to the external sensor clks (sclk_cam*). Isn't
exynos5 media driver is the best place to handle such clocks?
+ return 0;
+err_exit:
+ fimc_md_put_clocks(fmd);
+ return ret;
+}
+
Regards
Arun
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 01/13] [media] exynos5-is: Adding media device driver for exynos5
Hi Sachin,
Thank you for the review.
On Mon, Aug 5, 2013 at 10:51 AM, Sachin Kamat sachin.ka...@linaro.org wrote:
On 2 August 2013 20:32, Arun Kumar K arun...@samsung.com wrote:
From: Shaik Ameer Basha shaik.am...@samsung.com
This patch adds support for media device for EXYNOS5 SoCs.
The current media device supports the following ips to connect
through the media controller framework.
* MIPI-CSIS
Support interconnection(subdev interface) between devices
* FIMC-LITE
Support capture interface from device(Sensor, MIPI-CSIS) to memory
Support interconnection(subdev interface) between devices
* FIMC-IS
Camera post-processing IP having multiple sub-nodes.
G-Scaler will be added later to the current media device.
The media device creates two kinds of pipelines for connecting
the above mentioned IPs.
The pipeline0 is uses Sensor, MIPI-CSIS and FIMC-LITE which captures
image data and dumps to memory.
Pipeline1 uses FIMC-IS components for doing post-processing
operations on the captured image and give scaled YUV output.
Pipeline0
++ +---+ +---+ ++
| Sensor | -- | MIPI-CSIS | -- | FIMC-LITE | -- | Memory |
++ +---+ +---+ ++
Pipeline1
++ ++ +---+ +---+
| Memory | -- | ISP | -- |SCC| -- |SCP|
++ ++ +---+ +---+
Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
Signed-off-by: Arun Kumar K arun...@samsung.com
[snip]
+
+Common 'camera' node
+
+
+Required properties:
+
+- compatible : must be samsung,exynos5-fimc, simple-bus
I am not sure if this point was discusssed during the previous
versions. samsung,exynos5-fimc seems a bit generic.
The compatible string should generally point to a specific SoC (the
first one to have this IP), something like samsung,exynos5250-fimc.
Yes will make it samsung,exynos5250-fimc
+- clocks : list of clock specifiers, corresponding to entries in
+ the clock-names property;
+- clock-names : must contain sclk_cam0, sclk_cam1 entries,
+ matching entries in the clocks property.
+
[snip]
+Example:
+
+ aliases {
+ fimc-lite0 = fimc_lite_0
+ };
+
+ /* Parallel bus IF sensor */
+ i2c_0: i2c@1386 {
+ s5k6aa: sensor@3c {
+ compatible = samsung,s5k6aafx;
+ reg = 0x3c;
+ vddio-supply = ...;
+
+ clock-frequency = 2400;
+ clocks = ...;
+ clock-names = mclk;
+
+ port {
+ s5k6aa_ep: endpoint {
+ remote-endpoint = fimc0_ep;
+ bus-width = 8;
+ hsync-active = 0;
+ vsync-active = 1;
+ pclk-sample = 1;
+ };
+ };
+ };
+ };
+
+ /* MIPI CSI-2 bus IF sensor */
+ s5c73m3: sensor@0x1a {
0x not needed.
Ok.
Regards
Arun
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 01/13] [media] exynos5-is: Adding media device driver for exynos5
Hi Arun,
On 08/05/2013 12:06 PM, Arun Kumar K wrote:
Hi Sylwester,
Thank you for the review.
Will address all your review comments.
Some responses below:
Thanks, it took me a while to review this nearly 10k of code.
But fortunately I could spent some more time at the computer
this weekend ;)
[snip]
+
+static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
+{
+ struct device_node *of_node = fmd-pdev-dev.of_node;
+ int ret;
+
+ /*
+* Runtime resume one of the FIMC entities to make sure
+* the sclk_cam clocks are not globally disabled.
It's a bit mysterious to me, is this requirement still valid on Exynos5 ?
I glanced over the Exynos5250 datasheet and there seem to be no sclk_cam?
clocks dependency on any of GScaler clocks. Maybe you don't need a clock
provider in this driver, perhaps sensor drivers could use sclk_cam clocks
directly, assigned through dts ?
Yes these clocks can be directly exposed via dt.
I will drop clock provider from this driver.
That's great, this patch set won't depend then on the proper clock
deregistration support in the common clock framework.
+/*
+ * The peripheral sensor clock management.
+ */
+static void fimc_md_put_clocks(struct fimc_md *fmd)
+{
+ int i = FIMC_MAX_CAMCLKS;
+
+ while (--i= 0) {
+ if (IS_ERR(fmd-camclk[i].clock))
+ continue;
+ clk_put(fmd-camclk[i].clock);
+ fmd-camclk[i].clock = ERR_PTR(-EINVAL);
+ }
Please double check if you need this sclk_cam clocks handling. We could
simply add a requirement that this driver supports only sensor subdevs
through the v4l2-async API and which controls their clock themselves.
sclk_cam* handling can be removed and be done from respective
sensors. But I think the sclk_bayer handling needs to be retained in the
media driver.
Yes, that was my understanding as well.
+}
+
+static int fimc_md_get_clocks(struct fimc_md *fmd)
+{
+ struct device *dev = NULL;
+ char clk_name[32];
+ struct clk *clock;
+ int i, ret = 0;
+
+ for (i = 0; i FIMC_MAX_CAMCLKS; i++)
+ fmd-camclk[i].clock = ERR_PTR(-EINVAL);
+
+ if (fmd-pdev-dev.of_node)
+ dev =fmd-pdev-dev;
+
+ for (i = 0; i SCLK_BAYER; i++) {
+ snprintf(clk_name, sizeof(clk_name), sclk_cam%u, i);
+ clock = clk_get(dev, clk_name);
+
+ if (IS_ERR(clock)) {
+ dev_err(fmd-pdev-dev, Failed to get clock:
%s\n,
+ clk_name);
+ ret = PTR_ERR(clock);
+ break;
+ }
+ fmd-camclk[i].clock = clock;
+ }
+ if (ret)
+ fimc_md_put_clocks(fmd);
+
+ /* Prepare bayer clk */
+ clock = clk_get(dev, sclk_bayer);
+
+ if (IS_ERR(clock)) {
+ dev_err(fmd-pdev-dev, Failed to get clock: %s\n,
+ clk_name);
Wrong error message.
+ ret = PTR_ERR(clock);
+ goto err_exit;
+ }
+ ret = clk_prepare(clock);
+ if (ret 0) {
+ clk_put(clock);
+ fmd-camclk[SCLK_BAYER].clock = ERR_PTR(-EINVAL);
+ goto err_exit;
+ }
+ fmd-camclk[SCLK_BAYER].clock = clock;
Could you explain a bit how is this SCLK_BAYER clock used ? Is it routed
to external image sensor, or is it used only inside an SoC ?
It is not defined properly in the manual, but I suppose its the bus clock
for the bayer rgb data bus. So for proper sensor functionality, we need this
sclk_bayer in addition to the external sensor clks (sclk_cam*). Isn't
exynos5 media driver is the best place to handle such clocks?
I see, I think it's the right place. I was just curious what this clock
was exactly. It an SoC requires it internally then it is correct to handle
it as you do now. And if it happens that it controls the CAM_BAY_MCLK
output clock of the camera bay, then it could be exposed as the master
clock for the device attached to that physical camera port. In that case
the SCLK_CAM bayer clock could be gated conditionally in the media device
driver, depending on the data bus interface used by a remote image data
source device. But that's mostly speculations. I'm fine with associating
this clock with the media device, especially as far as the DT binding is
concerned.
--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 01/13] [media] exynos5-is: Adding media device driver for exynos5
On 2 August 2013 20:32, Arun Kumar K arun...@samsung.com wrote:
From: Shaik Ameer Basha shaik.am...@samsung.com
This patch adds support for media device for EXYNOS5 SoCs.
The current media device supports the following ips to connect
through the media controller framework.
* MIPI-CSIS
Support interconnection(subdev interface) between devices
* FIMC-LITE
Support capture interface from device(Sensor, MIPI-CSIS) to memory
Support interconnection(subdev interface) between devices
* FIMC-IS
Camera post-processing IP having multiple sub-nodes.
G-Scaler will be added later to the current media device.
The media device creates two kinds of pipelines for connecting
the above mentioned IPs.
The pipeline0 is uses Sensor, MIPI-CSIS and FIMC-LITE which captures
image data and dumps to memory.
Pipeline1 uses FIMC-IS components for doing post-processing
operations on the captured image and give scaled YUV output.
Pipeline0
++ +---+ +---+ ++
| Sensor | -- | MIPI-CSIS | -- | FIMC-LITE | -- | Memory |
++ +---+ +---+ ++
Pipeline1
++ ++ +---+ +---+
| Memory | -- | ISP | -- |SCC| -- |SCP|
++ ++ +---+ +---+
Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
Signed-off-by: Arun Kumar K arun...@samsung.com
[snip]
+
+Common 'camera' node
+
+
+Required properties:
+
+- compatible : must be samsung,exynos5-fimc, simple-bus
I am not sure if this point was discusssed during the previous
versions. samsung,exynos5-fimc seems a bit generic.
The compatible string should generally point to a specific SoC (the
first one to have this IP), something like samsung,exynos5250-fimc.
+- clocks : list of clock specifiers, corresponding to entries in
+ the clock-names property;
+- clock-names : must contain sclk_cam0, sclk_cam1 entries,
+ matching entries in the clocks property.
+
[snip]
+Example:
+
+ aliases {
+ fimc-lite0 = fimc_lite_0
+ };
+
+ /* Parallel bus IF sensor */
+ i2c_0: i2c@1386 {
+ s5k6aa: sensor@3c {
+ compatible = samsung,s5k6aafx;
+ reg = 0x3c;
+ vddio-supply = ...;
+
+ clock-frequency = 2400;
+ clocks = ...;
+ clock-names = mclk;
+
+ port {
+ s5k6aa_ep: endpoint {
+ remote-endpoint = fimc0_ep;
+ bus-width = 8;
+ hsync-active = 0;
+ vsync-active = 1;
+ pclk-sample = 1;
+ };
+ };
+ };
+ };
+
+ /* MIPI CSI-2 bus IF sensor */
+ s5c73m3: sensor@0x1a {
0x not needed.
--
With warm regards,
Sachin
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 01/13] [media] exynos5-is: Adding media device driver for exynos5
On 08/02/2013 05:02 PM, Arun Kumar K wrote:
From: Shaik Ameer Bashashaik.am...@samsung.com
This patch adds support for media device for EXYNOS5 SoCs.
The current media device supports the following ips to connect
through the media controller framework.
* MIPI-CSIS
Support interconnection(subdev interface) between devices
* FIMC-LITE
Support capture interface from device(Sensor, MIPI-CSIS) to memory
Support interconnection(subdev interface) between devices
* FIMC-IS
Camera post-processing IP having multiple sub-nodes.
G-Scaler will be added later to the current media device.
The media device creates two kinds of pipelines for connecting
the above mentioned IPs.
The pipeline0 is uses Sensor, MIPI-CSIS and FIMC-LITE which captures
image data and dumps to memory.
Pipeline1 uses FIMC-IS components for doing post-processing
operations on the captured image and give scaled YUV output.
Pipeline0
++ +---+ +---+ ++
| Sensor | -- | MIPI-CSIS | -- | FIMC-LITE | -- | Memory |
++ +---+ +---+ ++
Pipeline1
++ ++ +---+ +---+
| Memory | -- | ISP | -- |SCC| -- |SCP|
++ ++ +---+ +---+
Signed-off-by: Shaik Ameer Bashashaik.am...@samsung.com
Signed-off-by: Arun Kumar Karun...@samsung.com
---
.../devicetree/bindings/media/exynos5-mdev.txt | 153 ++
drivers/media/platform/exynos5-is/exynos5-mdev.c | 1471
drivers/media/platform/exynos5-is/exynos5-mdev.h | 199 +++
3 files changed, 1823 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/exynos5-mdev.txt
create mode 100644 drivers/media/platform/exynos5-is/exynos5-mdev.c
create mode 100644 drivers/media/platform/exynos5-is/exynos5-mdev.h
diff --git a/Documentation/devicetree/bindings/media/exynos5-mdev.txt
b/Documentation/devicetree/bindings/media/exynos5-mdev.txt
new file mode 100644
index 000..d7d419b
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/exynos5-mdev.txt
@@ -0,0 +1,153 @@
+Samsung EXYNOS5 SoC Camera Subsystem (FIMC)
I guess (FIMC) could be omitted.
+--
+
+The Exynos5 SoC Camera subsystem comprises of multiple sub-devices
+represented by separate device tree nodes. Currently this includes: FIMC-LITE,
+MIPI CSIS and FIMC-IS.
+
+The sub-subdevices are defined as child nodes of the common 'camera' node which
+also includes common properties of the whole subsystem not really specific to
+any single sub-device, like common camera port pins or the CAMCLK clock outputs
+for external image sensors attached to an SoC.
+
+Common 'camera' node
+
+
+Required properties:
+
+- compatible : must be samsung,exynos5-fimc, simple-bus
+- clocks : list of clock specifiers, corresponding to entries in
+ the clock-names property;
+- clock-names : must contain sclk_cam0, sclk_cam1 entries,
+ matching entries in the clocks property.
+
+The pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt must be used
+to define a required pinctrl state named default and optional pinctrl states:
+idle, active-a, active-b. These optional states can be used to switch the
+camera port pinmux at runtime. The idle state should configure both the
camera
+ports A and B into high impedance state, especially the CAMCLK clock output
+should be inactive. For the active-a state the camera port A must be
activated
+and the port B deactivated and for the state active-b it should be the other
+way around.
+
+The 'camera' node must include at least one 'fimc-lite' child node.
+
+'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;
This property is not needed any more, once the common clock bindings are
used.
+Image sensor nodes
+--
+
+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 the parallel-ports node,
+using the common video interfaces bindings, defined in video-interfaces.txt.
+The implementation of this bindings requires clock-frequency property to be
+present in the sensor device nodes.
+
+Example:
+
+ aliases {
+ fimc-lite0 =fimc_lite_0
+ };
+
+ /* Parallel bus IF sensor */
+ i2c_0: i2c@1386 {
+ s5k6aa: sensor@3c {
+ compatible = samsung,s5k6aafx;
+ reg =0x3c;
+ vddio-supply =...;
+
+
[RFC v3 01/13] [media] exynos5-is: Adding media device driver for exynos5
From: Shaik Ameer Basha shaik.am...@samsung.com
This patch adds support for media device for EXYNOS5 SoCs.
The current media device supports the following ips to connect
through the media controller framework.
* MIPI-CSIS
Support interconnection(subdev interface) between devices
* FIMC-LITE
Support capture interface from device(Sensor, MIPI-CSIS) to memory
Support interconnection(subdev interface) between devices
* FIMC-IS
Camera post-processing IP having multiple sub-nodes.
G-Scaler will be added later to the current media device.
The media device creates two kinds of pipelines for connecting
the above mentioned IPs.
The pipeline0 is uses Sensor, MIPI-CSIS and FIMC-LITE which captures
image data and dumps to memory.
Pipeline1 uses FIMC-IS components for doing post-processing
operations on the captured image and give scaled YUV output.
Pipeline0
++ +---+ +---+ ++
| Sensor | -- | MIPI-CSIS | -- | FIMC-LITE | -- | Memory |
++ +---+ +---+ ++
Pipeline1
++ ++ +---+ +---+
| Memory | -- | ISP | -- |SCC| -- |SCP|
++ ++ +---+ +---+
Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
Signed-off-by: Arun Kumar K arun...@samsung.com
---
.../devicetree/bindings/media/exynos5-mdev.txt | 153 ++
drivers/media/platform/exynos5-is/exynos5-mdev.c | 1471
drivers/media/platform/exynos5-is/exynos5-mdev.h | 199 +++
3 files changed, 1823 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/exynos5-mdev.txt
create mode 100644 drivers/media/platform/exynos5-is/exynos5-mdev.c
create mode 100644 drivers/media/platform/exynos5-is/exynos5-mdev.h
diff --git a/Documentation/devicetree/bindings/media/exynos5-mdev.txt
b/Documentation/devicetree/bindings/media/exynos5-mdev.txt
new file mode 100644
index 000..d7d419b
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/exynos5-mdev.txt
@@ -0,0 +1,153 @@
+Samsung EXYNOS5 SoC Camera Subsystem (FIMC)
+--
+
+The Exynos5 SoC Camera subsystem comprises of multiple sub-devices
+represented by separate device tree nodes. Currently this includes: FIMC-LITE,
+MIPI CSIS and FIMC-IS.
+
+The sub-subdevices are defined as child nodes of the common 'camera' node which
+also includes common properties of the whole subsystem not really specific to
+any single sub-device, like common camera port pins or the CAMCLK clock outputs
+for external image sensors attached to an SoC.
+
+Common 'camera' node
+
+
+Required properties:
+
+- compatible : must be samsung,exynos5-fimc, simple-bus
+- clocks : list of clock specifiers, corresponding to entries in
+ the clock-names property;
+- clock-names : must contain sclk_cam0, sclk_cam1 entries,
+ matching entries in the clocks property.
+
+The pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt must be used
+to define a required pinctrl state named default and optional pinctrl states:
+idle, active-a, active-b. These optional states can be used to switch the
+camera port pinmux at runtime. The idle state should configure both the
camera
+ports A and B into high impedance state, especially the CAMCLK clock output
+should be inactive. For the active-a state the camera port A must be
activated
+and the port B deactivated and for the state active-b it should be the other
+way around.
+
+The 'camera' node must include at least one 'fimc-lite' child node.
+
+'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 to their control bus controller (e.g.
+I2C0) nodes and linked to a port node in the csis or the parallel-ports node,
+using the common video interfaces bindings, defined in video-interfaces.txt.
+The implementation of this bindings requires clock-frequency property to be
+present in the sensor device nodes.
+
+Example:
+
+ aliases {
+ fimc-lite0 = fimc_lite_0
+ };
+
+ /* Parallel bus IF sensor */
+ i2c_0: i2c@1386 {
+ s5k6aa: sensor@3c {
+ compatible = samsung,s5k6aafx;
+ reg = 0x3c;
+ vddio-supply = ...;
+
+ clock-frequency = 2400;
+ clocks = ...;
+ clock-names = mclk;
+
+ port {
+
