Re: [RFC v2 05/10] exynos5-fimc-is: Adds the sensor subdev
On 07/17/2013 06:55 AM, Arun Kumar K wrote: On Wed, Jul 17, 2013 at 3:33 AM, Sylwester Nawrocki sylvester.nawro...@gmail.com wrote: On 07/09/2013 02:04 PM, Arun Kumar K wrote: On Wed, Jun 26, 2013 at 12:57 PM, Hans Verkuilhverk...@xs4all.nl wrote: On Fri May 31 2013 15:03:23 Arun Kumar K wrote: FIMC-IS uses certain sensors which are exclusively controlled from the IS firmware. This patch adds the sensor subdev for the fimc-is sensors. Signed-off-by: Arun Kumar Karun...@samsung.com Signed-off-by: Kilyeon Imkilyeon...@samsung.com Not surprisingly I really hate the idea of sensor drivers that are tied to a specific SoC, since it completely destroys the reusability of such drivers. Yes agree to it. I understand that you have little choice to do something special here, but I was wondering whether there is a way of keeping things as generic as possible. I'm just brainstorming here, but as far as I can see this driver is basically a partial sensor driver: it handles the clock, the format negotiation and power management. Any sensor driver needs that. What would be nice is if the fmic specific parts are replaced by callbacks into the bridge driver using v4l2_subdev_notify(). The platform data (or DT) can also state if this sensor is firmware controlled or not. If not, then the missing bits can be implemented in the future by someone who needs that. That way the driver itself remains independent from fimc. And existing sensor drivers can be adapted to be usable with fimc as well by adding support for the notify callback. Would a scheme along those lines work? Yes this should make the implementation very generic. Will check the feasibility of this approach. Is I suggested earlier, you likely could do without this call back to the FIMC-IS from within the sensor subdev. Look at your call chain right now: /dev/video? media-dev-driversensor-subdev FIMC-IS | | | | | VIDIOC_STREAMON | | | |# s_stream()| | | #--# pipeline_open() | | | # | | | # pipeline_start() | | | # | | | | | Couldn't you move pipeline_open(), pipeline_start() to s_stream handler of the ISP subdev ? It is currently empty. The media device driver could call s_stream on the ISP subdev each time it sees s_stream request on the sensor subdev. And you wouldn't need any hacks to get the pipeline pointer in the sensor subdev. Then it would be something like: /dev/video? media-dev-driversensor-subdev FIMC-IS-ISP-subdev | | | | | VIDIOC_STREAMON | | | |# s_stream()| | | #--| | | # s_stream()| | | #---+# pipeline_open() | | | # pipeline_start() | | | # I suppose pipeline_open() is better candidate for the s_power callback. It just needs to be ensured at the media device level the subdev operations sequences are correct. It can be done this way. But my intention of putting these calls in the sensor subdev was to use the sensor subdev independent of isp subdev. This is for the usecase where the pipeline will only contain is-sensor -- mipi-csis -- fimc-lite --- memory This way you can capture the bayer rgb data from sensor without using any isp components at all. The second pipeline which is isp -- scc -- scp can be used for processing the sensor data and can be created and used if needed. In the method you mentioned, the isp subdev has to be used even when it is not part of the pipeline. Is that allowed? If its allowed as per media pipeline guidelines, then this definitely is a better approach. Please suggest on this. Sure, I'm aware of those two relatively separate pipelines. s_power, s_stream callbacks belong to the kernel so I don't think it would be an issue to do it as I described. Please note s_power, s_stream are normally reference counted. Alternatively you could create a separate subdev for the FIMC-IS firmware interface. Such subdev wouldn't be exposing device node and would be used by the media pipeline controller driver to ensure proper hardware configuration sequences. I don't know the Exynos5 FIMC-IS firmware architecture very well so I'm not sure if it is worth to create such a separate subdev as the firmware interface obstruction layer ;) I guess we could do only with subdevs that are
Re: [RFC v2 05/10] exynos5-fimc-is: Adds the sensor subdev
Hi Sylwester, On Wed, Jul 17, 2013 at 7:44 PM, Sylwester Nawrocki s.nawro...@samsung.com wrote: On 07/17/2013 06:55 AM, Arun Kumar K wrote: On Wed, Jul 17, 2013 at 3:33 AM, Sylwester Nawrocki sylvester.nawro...@gmail.com wrote: On 07/09/2013 02:04 PM, Arun Kumar K wrote: On Wed, Jun 26, 2013 at 12:57 PM, Hans Verkuilhverk...@xs4all.nl wrote: On Fri May 31 2013 15:03:23 Arun Kumar K wrote: FIMC-IS uses certain sensors which are exclusively controlled from the IS firmware. This patch adds the sensor subdev for the fimc-is sensors. Signed-off-by: Arun Kumar Karun...@samsung.com Signed-off-by: Kilyeon Imkilyeon...@samsung.com Not surprisingly I really hate the idea of sensor drivers that are tied to a specific SoC, since it completely destroys the reusability of such drivers. Yes agree to it. I understand that you have little choice to do something special here, but I was wondering whether there is a way of keeping things as generic as possible. I'm just brainstorming here, but as far as I can see this driver is basically a partial sensor driver: it handles the clock, the format negotiation and power management. Any sensor driver needs that. What would be nice is if the fmic specific parts are replaced by callbacks into the bridge driver using v4l2_subdev_notify(). The platform data (or DT) can also state if this sensor is firmware controlled or not. If not, then the missing bits can be implemented in the future by someone who needs that. That way the driver itself remains independent from fimc. And existing sensor drivers can be adapted to be usable with fimc as well by adding support for the notify callback. Would a scheme along those lines work? Yes this should make the implementation very generic. Will check the feasibility of this approach. Is I suggested earlier, you likely could do without this call back to the FIMC-IS from within the sensor subdev. Look at your call chain right now: /dev/video? media-dev-driversensor-subdev FIMC-IS | | | | | VIDIOC_STREAMON | | | |# s_stream()| | | #--# pipeline_open() | | | # | | | # pipeline_start() | | | # | | | | | Couldn't you move pipeline_open(), pipeline_start() to s_stream handler of the ISP subdev ? It is currently empty. The media device driver could call s_stream on the ISP subdev each time it sees s_stream request on the sensor subdev. And you wouldn't need any hacks to get the pipeline pointer in the sensor subdev. Then it would be something like: /dev/video? media-dev-driversensor-subdev FIMC-IS-ISP-subdev | | | | | VIDIOC_STREAMON | | | |# s_stream()| | | #--| | | # s_stream()| | | #---+# pipeline_open() | | | # pipeline_start() | | | # I suppose pipeline_open() is better candidate for the s_power callback. It just needs to be ensured at the media device level the subdev operations sequences are correct. It can be done this way. But my intention of putting these calls in the sensor subdev was to use the sensor subdev independent of isp subdev. This is for the usecase where the pipeline will only contain is-sensor -- mipi-csis -- fimc-lite --- memory This way you can capture the bayer rgb data from sensor without using any isp components at all. The second pipeline which is isp -- scc -- scp can be used for processing the sensor data and can be created and used if needed. In the method you mentioned, the isp subdev has to be used even when it is not part of the pipeline. Is that allowed? If its allowed as per media pipeline guidelines, then this definitely is a better approach. Please suggest on this. Sure, I'm aware of those two relatively separate pipelines. s_power, s_stream callbacks belong to the kernel so I don't think it would be an issue to do it as I described. Please note s_power, s_stream are normally reference counted. Alternatively you could create a separate subdev for the FIMC-IS firmware interface. Such subdev wouldn't be exposing device node and would be used by the media pipeline controller driver to ensure proper hardware configuration sequences. I don't know the Exynos5 FIMC-IS firmware architecture very well so I'm not sure if it is worth to create such a separate
Re: [RFC v2 05/10] exynos5-fimc-is: Adds the sensor subdev
Hi Arun, On 07/09/2013 02:04 PM, Arun Kumar K wrote: On Wed, Jun 26, 2013 at 12:57 PM, Hans Verkuilhverk...@xs4all.nl wrote: On Fri May 31 2013 15:03:23 Arun Kumar K wrote: FIMC-IS uses certain sensors which are exclusively controlled from the IS firmware. This patch adds the sensor subdev for the fimc-is sensors. Signed-off-by: Arun Kumar Karun...@samsung.com Signed-off-by: Kilyeon Imkilyeon...@samsung.com Not surprisingly I really hate the idea of sensor drivers that are tied to a specific SoC, since it completely destroys the reusability of such drivers. Yes agree to it. I understand that you have little choice to do something special here, but I was wondering whether there is a way of keeping things as generic as possible. I'm just brainstorming here, but as far as I can see this driver is basically a partial sensor driver: it handles the clock, the format negotiation and power management. Any sensor driver needs that. What would be nice is if the fmic specific parts are replaced by callbacks into the bridge driver using v4l2_subdev_notify(). The platform data (or DT) can also state if this sensor is firmware controlled or not. If not, then the missing bits can be implemented in the future by someone who needs that. That way the driver itself remains independent from fimc. And existing sensor drivers can be adapted to be usable with fimc as well by adding support for the notify callback. Would a scheme along those lines work? Yes this should make the implementation very generic. Will check the feasibility of this approach. Is I suggested earlier, you likely could do without this call back to the FIMC-IS from within the sensor subdev. Look at your call chain right now: /dev/video? media-dev-driversensor-subdev FIMC-IS | | | | | VIDIOC_STREAMON | | | |# s_stream()| | | #--# pipeline_open() | | | # | | | # pipeline_start() | | | # | | | | | Couldn't you move pipeline_open(), pipeline_start() to s_stream handler of the ISP subdev ? It is currently empty. The media device driver could call s_stream on the ISP subdev each time it sees s_stream request on the sensor subdev. And you wouldn't need any hacks to get the pipeline pointer in the sensor subdev. Then it would be something like: /dev/video? media-dev-driversensor-subdev FIMC-IS-ISP-subdev | | | | | VIDIOC_STREAMON | | | |# s_stream()| | | #--| | | # s_stream()| | | #---+# pipeline_open() | | | # pipeline_start() | | | # I suppose pipeline_open() is better candidate for the s_power callback. It just needs to be ensured at the media device level the subdev operations sequences are correct. -- Regards, 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 v2 05/10] exynos5-fimc-is: Adds the sensor subdev
Hi Sylwester, On Wed, Jul 17, 2013 at 3:33 AM, Sylwester Nawrocki sylvester.nawro...@gmail.com wrote: Hi Arun, On 07/09/2013 02:04 PM, Arun Kumar K wrote: On Wed, Jun 26, 2013 at 12:57 PM, Hans Verkuilhverk...@xs4all.nl wrote: On Fri May 31 2013 15:03:23 Arun Kumar K wrote: FIMC-IS uses certain sensors which are exclusively controlled from the IS firmware. This patch adds the sensor subdev for the fimc-is sensors. Signed-off-by: Arun Kumar Karun...@samsung.com Signed-off-by: Kilyeon Imkilyeon...@samsung.com Not surprisingly I really hate the idea of sensor drivers that are tied to a specific SoC, since it completely destroys the reusability of such drivers. Yes agree to it. I understand that you have little choice to do something special here, but I was wondering whether there is a way of keeping things as generic as possible. I'm just brainstorming here, but as far as I can see this driver is basically a partial sensor driver: it handles the clock, the format negotiation and power management. Any sensor driver needs that. What would be nice is if the fmic specific parts are replaced by callbacks into the bridge driver using v4l2_subdev_notify(). The platform data (or DT) can also state if this sensor is firmware controlled or not. If not, then the missing bits can be implemented in the future by someone who needs that. That way the driver itself remains independent from fimc. And existing sensor drivers can be adapted to be usable with fimc as well by adding support for the notify callback. Would a scheme along those lines work? Yes this should make the implementation very generic. Will check the feasibility of this approach. Is I suggested earlier, you likely could do without this call back to the FIMC-IS from within the sensor subdev. Look at your call chain right now: /dev/video? media-dev-driversensor-subdev FIMC-IS | | | | | VIDIOC_STREAMON | | | |# s_stream()| | | #--# pipeline_open() | | | # | | | # pipeline_start() | | | # | | | | | Couldn't you move pipeline_open(), pipeline_start() to s_stream handler of the ISP subdev ? It is currently empty. The media device driver could call s_stream on the ISP subdev each time it sees s_stream request on the sensor subdev. And you wouldn't need any hacks to get the pipeline pointer in the sensor subdev. Then it would be something like: /dev/video? media-dev-driversensor-subdev FIMC-IS-ISP-subdev | | | | | VIDIOC_STREAMON | | | |# s_stream()| | | #--| | | # s_stream()| | | #---+# pipeline_open() | | | # pipeline_start() | | | # I suppose pipeline_open() is better candidate for the s_power callback. It just needs to be ensured at the media device level the subdev operations sequences are correct. It can be done this way. But my intention of putting these calls in the sensor subdev was to use the sensor subdev independent of isp subdev. This is for the usecase where the pipeline will only contain is-sensor -- mipi-csis -- fimc-lite --- memory This way you can capture the bayer rgb data from sensor without using any isp components at all. The second pipeline which is isp -- scc -- scp can be used for processing the sensor data and can be created and used if needed. In the method you mentioned, the isp subdev has to be used even when it is not part of the pipeline. Is that allowed? If its allowed as per media pipeline guidelines, then this definitely is a better approach. Please suggest on this. 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 v2 05/10] exynos5-fimc-is: Adds the sensor subdev
Hi Sylwester,
Thank you for the review.
On Fri, Jun 21, 2013 at 4:34 AM, Sylwester Nawrocki
sylvester.nawro...@gmail.com wrote:
On 05/31/2013 03:03 PM, Arun Kumar K wrote:
FIMC-IS uses certain sensors which are exclusively controlled
from the IS firmware. This patch adds the sensor subdev for the
fimc-is sensors.
Signed-off-by: Arun Kumar Karun...@samsung.com
Signed-off-by: Kilyeon Imkilyeon...@samsung.com
---
drivers/media/platform/exynos5-is/fimc-is-sensor.c | 463
drivers/media/platform/exynos5-is/fimc-is-sensor.h | 168 +++
2 files changed, 631 insertions(+)
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-sensor.c
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-sensor.h
diff --git a/drivers/media/platform/exynos5-is/fimc-is-sensor.c
b/drivers/media/platform/exynos5-is/fimc-is-sensor.c
new file mode 100644
index 000..b8fb834
--- /dev/null
+++ b/drivers/media/platform/exynos5-is/fimc-is-sensor.c
@@ -0,0 +1,463 @@
+/*
+ * Samsung EXYNOS5250 FIMC-IS (Imaging Subsystem) driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Arun Kumar Karun...@samsung.com
+ * Kil-yeon Limkilyeon...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#includelinux/gpio.h
+#includelinux/of_gpio.h
+#includelinux/i2c.h
+#includelinux/of.h
+#includelinux/of_platform.h
+#includemedia/v4l2-of.h
+#include fimc-is-sensor.h
+#include fimc-is.h
+
+#define DRIVER_NAME fimc-is-sensor
+
+static char *sensor_clock_name[] = {
+ [SCLK_BAYER]= sclk_bayer,
+ [SCLK_CAM0] = sclk_cam0,
+ [SCLK_CAM1] = sclk_cam1,
+};
+
+/* Sensor supported formats */
+static struct v4l2_mbus_framefmt sensor_formats[FIMC_IS_MAX_SENSORS] = {
+ [SENSOR_S5K4E5] = {
+ .width = SENSOR_4E5_WIDTH + 16,
+ .height = SENSOR_4E5_HEIGHT + 10,
+ .code = V4L2_MBUS_FMT_SGRBG10_1X10,
+ .field = V4L2_FIELD_NONE,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ },
+ [SENSOR_S5K6A3] = {
+ .width = SENSOR_6A3_WIDTH + 16,
+ .height = SENSOR_6A3_HEIGHT + 10,
+ .code = V4L2_MBUS_FMT_SGRBG10_1X10,
+ .field = V4L2_FIELD_NONE,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ },
+};
+
+static struct fimc_is_sensor *sd_to_fimc_is_sensor(struct v4l2_subdev
*sd)
+{
+ return container_of(sd, struct fimc_is_sensor, subdev);
+}
+
+static void sensor_clk_put(struct fimc_is_sensor *sensor)
+{
+ int i;
+
+ for (i = 0; i SCLK_MAX_NUM; i++) {
+ if (IS_ERR(sensor-clock[i]))
+ continue;
+ clk_unprepare(sensor-clock[i]);
+ clk_put(sensor-clock[i]);
+ sensor-clock[i] = ERR_PTR(-EINVAL);
+ }
+}
+
+static int sensor_clk_init(struct fimc_is_sensor *sensor)
+{
+ int i, ret;
+
+ /* Get CAM clocks */
+ for (i = 0; i SCLK_MAX_NUM; i++) {
+ sensor-clock[i] = clk_get(NULL, sensor_clock_name[i]);
+ if (IS_ERR(sensor-clock[i]))
+ goto err;
+ ret = clk_prepare(sensor-clock[i]);
+ if (ret 0) {
+ clk_put(sensor-clock[i]);
+ sensor-clock[i] = ERR_PTR(-EINVAL);
+ goto err;
+ }
+ }
+
+ /* Set clock rates */
+ ret = clk_set_rate(sensor-clock[SCLK_CAM0], 24 * 100);
+ ret |= clk_set_rate(sensor-clock[SCLK_BAYER], 24 * 100);
Please don't obfuscate the return value.
Ok
+ if (ret) {
+ pr_err(Failed to set cam clock rates\n);
+ goto err;
+ }
+ return 0;
+err:
+ sensor_clk_put(sensor);
+ pr_err(Failed to init sensor clock\n);
+ return -ENXIO;
+}
+
+static int sensor_clk_enable(struct fimc_is_sensor *sensor)
+{
+ int ret = 0, i;
+
+ for (i = 0; i SCLK_MAX_NUM; i++) {
+ ret = clk_enable(sensor-clock[i]);
+ if (ret)
+ return ret;
+ }
Oh, so you enable all clocks in this driver ? Is it really flexible
enough ? What if one of these clocks is connected to some external
sensor with an ISP ? Are this clocks going to be managed/exposed by
the media device driver as well ?
I was hoping that this driver can handle all its clocks as exposed
from the DT. I can see that in exynos4, the media device enables the
cam clocks. In exynos5, there are multiple frequency settings
possible on these clocks depending on modes of operation.
So isn't it better that the sensor driver
Re: [RFC v2 05/10] exynos5-fimc-is: Adds the sensor subdev
Hi Hans, Thanks for the review. On Wed, Jun 26, 2013 at 12:57 PM, Hans Verkuil hverk...@xs4all.nl wrote: On Fri May 31 2013 15:03:23 Arun Kumar K wrote: FIMC-IS uses certain sensors which are exclusively controlled from the IS firmware. This patch adds the sensor subdev for the fimc-is sensors. Signed-off-by: Arun Kumar K arun...@samsung.com Signed-off-by: Kilyeon Im kilyeon...@samsung.com Not surprisingly I really hate the idea of sensor drivers that are tied to a specific SoC, since it completely destroys the reusability of such drivers. Yes agree to it. I understand that you have little choice to do something special here, but I was wondering whether there is a way of keeping things as generic as possible. I'm just brainstorming here, but as far as I can see this driver is basically a partial sensor driver: it handles the clock, the format negotiation and power management. Any sensor driver needs that. What would be nice is if the fmic specific parts are replaced by callbacks into the bridge driver using v4l2_subdev_notify(). The platform data (or DT) can also state if this sensor is firmware controlled or not. If not, then the missing bits can be implemented in the future by someone who needs that. That way the driver itself remains independent from fimc. And existing sensor drivers can be adapted to be usable with fimc as well by adding support for the notify callback. Would a scheme along those lines work? Yes this should make the implementation very generic. Will check the feasibility of this approach. Thanks 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 v2 05/10] exynos5-fimc-is: Adds the sensor subdev
On Fri May 31 2013 15:03:23 Arun Kumar K wrote:
FIMC-IS uses certain sensors which are exclusively controlled
from the IS firmware. This patch adds the sensor subdev for the
fimc-is sensors.
Signed-off-by: Arun Kumar K arun...@samsung.com
Signed-off-by: Kilyeon Im kilyeon...@samsung.com
Not surprisingly I really hate the idea of sensor drivers that are tied to
a specific SoC, since it completely destroys the reusability of such drivers.
I understand that you have little choice to do something special here, but
I was wondering whether there is a way of keeping things as generic as
possible.
I'm just brainstorming here, but as far as I can see this driver is basically
a partial sensor driver: it handles the clock, the format negotiation and
power management. Any sensor driver needs that.
What would be nice is if the fmic specific parts are replaced by callbacks
into the bridge driver using v4l2_subdev_notify().
The platform data (or DT) can also state if this sensor is firmware controlled
or not. If not, then the missing bits can be implemented in the future by
someone who needs that.
That way the driver itself remains independent from fimc.
And existing sensor drivers can be adapted to be usable with fimc as well by
adding support for the notify callback.
Would a scheme along those lines work?
Regards,
Hans
---
drivers/media/platform/exynos5-is/fimc-is-sensor.c | 463
drivers/media/platform/exynos5-is/fimc-is-sensor.h | 168 +++
2 files changed, 631 insertions(+)
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-sensor.c
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-sensor.h
diff --git a/drivers/media/platform/exynos5-is/fimc-is-sensor.c
b/drivers/media/platform/exynos5-is/fimc-is-sensor.c
new file mode 100644
index 000..b8fb834
--- /dev/null
+++ b/drivers/media/platform/exynos5-is/fimc-is-sensor.c
@@ -0,0 +1,463 @@
+/*
+ * Samsung EXYNOS5250 FIMC-IS (Imaging Subsystem) driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Arun Kumar K arun...@samsung.com
+ * Kil-yeon Lim kilyeon...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/gpio.h
+#include linux/of_gpio.h
+#include linux/i2c.h
+#include linux/of.h
+#include linux/of_platform.h
+#include media/v4l2-of.h
+#include fimc-is-sensor.h
+#include fimc-is.h
+
+#define DRIVER_NAME fimc-is-sensor
+
+static char *sensor_clock_name[] = {
+ [SCLK_BAYER]= sclk_bayer,
+ [SCLK_CAM0] = sclk_cam0,
+ [SCLK_CAM1] = sclk_cam1,
+};
+
+/* Sensor supported formats */
+static struct v4l2_mbus_framefmt sensor_formats[FIMC_IS_MAX_SENSORS] = {
+ [SENSOR_S5K4E5] = {
+ .width = SENSOR_4E5_WIDTH + 16,
+ .height = SENSOR_4E5_HEIGHT + 10,
+ .code = V4L2_MBUS_FMT_SGRBG10_1X10,
+ .field = V4L2_FIELD_NONE,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ },
+ [SENSOR_S5K6A3] = {
+ .width = SENSOR_6A3_WIDTH + 16,
+ .height = SENSOR_6A3_HEIGHT + 10,
+ .code = V4L2_MBUS_FMT_SGRBG10_1X10,
+ .field = V4L2_FIELD_NONE,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ },
+};
+
+static struct fimc_is_sensor *sd_to_fimc_is_sensor(struct v4l2_subdev *sd)
+{
+ return container_of(sd, struct fimc_is_sensor, subdev);
+}
+
+static void sensor_clk_put(struct fimc_is_sensor *sensor)
+{
+ int i;
+
+ for (i = 0; i SCLK_MAX_NUM; i++) {
+ if (IS_ERR(sensor-clock[i]))
+ continue;
+ clk_unprepare(sensor-clock[i]);
+ clk_put(sensor-clock[i]);
+ sensor-clock[i] = ERR_PTR(-EINVAL);
+ }
+}
+
+static int sensor_clk_init(struct fimc_is_sensor *sensor)
+{
+ int i, ret;
+
+ /* Get CAM clocks */
+ for (i = 0; i SCLK_MAX_NUM; i++) {
+ sensor-clock[i] = clk_get(NULL, sensor_clock_name[i]);
+ if (IS_ERR(sensor-clock[i]))
+ goto err;
+ ret = clk_prepare(sensor-clock[i]);
+ if (ret 0) {
+ clk_put(sensor-clock[i]);
+ sensor-clock[i] = ERR_PTR(-EINVAL);
+ goto err;
+ }
+ }
+
+ /* Set clock rates */
+ ret = clk_set_rate(sensor-clock[SCLK_CAM0], 24 * 100);
+ ret |= clk_set_rate(sensor-clock[SCLK_BAYER], 24 * 100);
+ if (ret) {
+ pr_err(Failed to set cam clock rates\n);
+ goto err;
+ }
+ return 0;
+err:
+ sensor_clk_put(sensor);
+ pr_err(Failed to init sensor clock\n);
+ return -ENXIO;
+}
+
+static int
Re: [RFC v2 05/10] exynos5-fimc-is: Adds the sensor subdev
On 05/31/2013 03:03 PM, Arun Kumar K wrote:
FIMC-IS uses certain sensors which are exclusively controlled
from the IS firmware. This patch adds the sensor subdev for the
fimc-is sensors.
Signed-off-by: Arun Kumar Karun...@samsung.com
Signed-off-by: Kilyeon Imkilyeon...@samsung.com
---
drivers/media/platform/exynos5-is/fimc-is-sensor.c | 463
drivers/media/platform/exynos5-is/fimc-is-sensor.h | 168 +++
2 files changed, 631 insertions(+)
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-sensor.c
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-sensor.h
diff --git a/drivers/media/platform/exynos5-is/fimc-is-sensor.c
b/drivers/media/platform/exynos5-is/fimc-is-sensor.c
new file mode 100644
index 000..b8fb834
--- /dev/null
+++ b/drivers/media/platform/exynos5-is/fimc-is-sensor.c
@@ -0,0 +1,463 @@
+/*
+ * Samsung EXYNOS5250 FIMC-IS (Imaging Subsystem) driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Arun Kumar Karun...@samsung.com
+ * Kil-yeon Limkilyeon...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#includelinux/gpio.h
+#includelinux/of_gpio.h
+#includelinux/i2c.h
+#includelinux/of.h
+#includelinux/of_platform.h
+#includemedia/v4l2-of.h
+#include fimc-is-sensor.h
+#include fimc-is.h
+
+#define DRIVER_NAME fimc-is-sensor
+
+static char *sensor_clock_name[] = {
+ [SCLK_BAYER]= sclk_bayer,
+ [SCLK_CAM0] = sclk_cam0,
+ [SCLK_CAM1] = sclk_cam1,
+};
+
+/* Sensor supported formats */
+static struct v4l2_mbus_framefmt sensor_formats[FIMC_IS_MAX_SENSORS] = {
+ [SENSOR_S5K4E5] = {
+ .width = SENSOR_4E5_WIDTH + 16,
+ .height = SENSOR_4E5_HEIGHT + 10,
+ .code = V4L2_MBUS_FMT_SGRBG10_1X10,
+ .field = V4L2_FIELD_NONE,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ },
+ [SENSOR_S5K6A3] = {
+ .width = SENSOR_6A3_WIDTH + 16,
+ .height = SENSOR_6A3_HEIGHT + 10,
+ .code = V4L2_MBUS_FMT_SGRBG10_1X10,
+ .field = V4L2_FIELD_NONE,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ },
+};
+
+static struct fimc_is_sensor *sd_to_fimc_is_sensor(struct v4l2_subdev *sd)
+{
+ return container_of(sd, struct fimc_is_sensor, subdev);
+}
+
+static void sensor_clk_put(struct fimc_is_sensor *sensor)
+{
+ int i;
+
+ for (i = 0; i SCLK_MAX_NUM; i++) {
+ if (IS_ERR(sensor-clock[i]))
+ continue;
+ clk_unprepare(sensor-clock[i]);
+ clk_put(sensor-clock[i]);
+ sensor-clock[i] = ERR_PTR(-EINVAL);
+ }
+}
+
+static int sensor_clk_init(struct fimc_is_sensor *sensor)
+{
+ int i, ret;
+
+ /* Get CAM clocks */
+ for (i = 0; i SCLK_MAX_NUM; i++) {
+ sensor-clock[i] = clk_get(NULL, sensor_clock_name[i]);
+ if (IS_ERR(sensor-clock[i]))
+ goto err;
+ ret = clk_prepare(sensor-clock[i]);
+ if (ret 0) {
+ clk_put(sensor-clock[i]);
+ sensor-clock[i] = ERR_PTR(-EINVAL);
+ goto err;
+ }
+ }
+
+ /* Set clock rates */
+ ret = clk_set_rate(sensor-clock[SCLK_CAM0], 24 * 100);
+ ret |= clk_set_rate(sensor-clock[SCLK_BAYER], 24 * 100);
Please don't obfuscate the return value.
+ if (ret) {
+ pr_err(Failed to set cam clock rates\n);
+ goto err;
+ }
+ return 0;
+err:
+ sensor_clk_put(sensor);
+ pr_err(Failed to init sensor clock\n);
+ return -ENXIO;
+}
+
+static int sensor_clk_enable(struct fimc_is_sensor *sensor)
+{
+ int ret = 0, i;
+
+ for (i = 0; i SCLK_MAX_NUM; i++) {
+ ret = clk_enable(sensor-clock[i]);
+ if (ret)
+ return ret;
+ }
Oh, so you enable all clocks in this driver ? Is it really flexible
enough ? What if one of these clocks is connected to some external
sensor with an ISP ? Are this clocks going to be managed/exposed by
the media device driver as well ?
+ return ret;
+}
+
+static void sensor_clk_disable(struct fimc_is_sensor *sensor)
+{
+ int i;
+
+ for (i = 0; i SCLK_MAX_NUM; i++)
+ clk_disable(sensor-clock[i]);
+}
+
+static int sensor_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ struct fimc_is_sensor *sensor = sd_to_fimc_is_sensor(sd);
+ struct fimc_is_sensor_drv_data *sdata = sensor-drvdata;
+
+ if (!code)
+
Re: [RFC v2 05/10] exynos5-fimc-is: Adds the sensor subdev
Hi Sachin,
+ if (ret 0) {
+ pr_err(Pipeline already opened.\n);
+ return -EBUSY;
why not propogate 'ret'? Same for other instances below.
Yes it can be done. Will change it.
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 v2 05/10] exynos5-fimc-is: Adds the sensor subdev
On 31 May 2013 18:33, Arun Kumar K arun...@samsung.com wrote:
FIMC-IS uses certain sensors which are exclusively controlled
from the IS firmware. This patch adds the sensor subdev for the
fimc-is sensors.
Signed-off-by: Arun Kumar K arun...@samsung.com
Signed-off-by: Kilyeon Im kilyeon...@samsung.com
---
drivers/media/platform/exynos5-is/fimc-is-sensor.c | 463
drivers/media/platform/exynos5-is/fimc-is-sensor.h | 168 +++
2 files changed, 631 insertions(+)
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-sensor.c
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-sensor.h
[snip]
+static int sensor_s_stream(struct v4l2_subdev *sd, int enable)
+{
+ struct fimc_is_sensor *sensor = sd_to_fimc_is_sensor(sd);
+ int ret;
+
+ if (enable) {
+ pr_debug(Stream ON\n);
+ /* Open pipeline */
+ ret = fimc_is_pipeline_open(sensor-pipeline, sensor);
+ if (ret 0) {
+ pr_err(Pipeline already opened.\n);
+ return -EBUSY;
why not propogate 'ret'? Same for other instances below.
+ }
+
+ /* Start IS pipeline */
+ ret = fimc_is_pipeline_start(sensor-pipeline);
+ if (ret 0) {
+ pr_err(Pipeline start failed.\n);
+ return -EINVAL;
+ }
+ } else {
+ pr_debug(Stream OFF\n);
+ /* Stop IS pipeline */
+ ret = fimc_is_pipeline_stop(sensor-pipeline);
+ if (ret 0) {
+ pr_err(Pipeline stop failed.\n);
+ return -EINVAL;
+ }
+
+ /* Close pipeline */
+ ret = fimc_is_pipeline_close(sensor-pipeline);
+ if (ret 0) {
+ pr_err(Pipeline close failed\n);
+ return -EBUSY;
+ }
+ }
+
+ return 0;
+}
+
[snip]
+
+static int fimc_is_sensor_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = client-dev;
+ struct fimc_is_sensor *sensor;
+ const struct of_device_id *of_id;
+ struct v4l2_subdev *sd;
+ int gpio, ret;
+ unsigned int sensor_id;
+
+ sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+ if (!sensor)
+ return -ENOMEM;
+
+ sensor-gpio_reset = -EINVAL;
+
+ gpio = of_get_gpio_flags(dev-of_node, 0, NULL);
+ if (gpio_is_valid(gpio)) {
+ ret = gpio_request_one(gpio, GPIOF_OUT_INIT_LOW, DRIVER_NAME);
+ if (ret 0)
+ return ret;
+ }
+ pr_err(GPIO Request success : %d, gpio);
+ sensor-gpio_reset = gpio;
+
+ of_id = of_match_node(fimc_is_sensor_of_match, dev-of_node);
+ if (!of_id) {
+ ret = -ENODEV;
+ goto err_gpio;
+ }
+
+ sensor-drvdata = (struct fimc_is_sensor_drv_data *) of_id-data;
+ sensor-dev = dev;
+
+ /* Get FIMC-IS context */
+ ret = sensor_parse_dt(sensor, dev-of_node);
+ if (ret) {
+ pr_err(Unable to obtain IS context\n);
+ ret = -ENODEV;
Why not propagate 'ret' itself?
--
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
[RFC v2 05/10] exynos5-fimc-is: Adds the sensor subdev
FIMC-IS uses certain sensors which are exclusively controlled
from the IS firmware. This patch adds the sensor subdev for the
fimc-is sensors.
Signed-off-by: Arun Kumar K arun...@samsung.com
Signed-off-by: Kilyeon Im kilyeon...@samsung.com
---
drivers/media/platform/exynos5-is/fimc-is-sensor.c | 463
drivers/media/platform/exynos5-is/fimc-is-sensor.h | 168 +++
2 files changed, 631 insertions(+)
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-sensor.c
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-sensor.h
diff --git a/drivers/media/platform/exynos5-is/fimc-is-sensor.c
b/drivers/media/platform/exynos5-is/fimc-is-sensor.c
new file mode 100644
index 000..b8fb834
--- /dev/null
+++ b/drivers/media/platform/exynos5-is/fimc-is-sensor.c
@@ -0,0 +1,463 @@
+/*
+ * Samsung EXYNOS5250 FIMC-IS (Imaging Subsystem) driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Arun Kumar K arun...@samsung.com
+ * Kil-yeon Lim kilyeon...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/gpio.h
+#include linux/of_gpio.h
+#include linux/i2c.h
+#include linux/of.h
+#include linux/of_platform.h
+#include media/v4l2-of.h
+#include fimc-is-sensor.h
+#include fimc-is.h
+
+#define DRIVER_NAME fimc-is-sensor
+
+static char *sensor_clock_name[] = {
+ [SCLK_BAYER]= sclk_bayer,
+ [SCLK_CAM0] = sclk_cam0,
+ [SCLK_CAM1] = sclk_cam1,
+};
+
+/* Sensor supported formats */
+static struct v4l2_mbus_framefmt sensor_formats[FIMC_IS_MAX_SENSORS] = {
+ [SENSOR_S5K4E5] = {
+ .width = SENSOR_4E5_WIDTH + 16,
+ .height = SENSOR_4E5_HEIGHT + 10,
+ .code = V4L2_MBUS_FMT_SGRBG10_1X10,
+ .field = V4L2_FIELD_NONE,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ },
+ [SENSOR_S5K6A3] = {
+ .width = SENSOR_6A3_WIDTH + 16,
+ .height = SENSOR_6A3_HEIGHT + 10,
+ .code = V4L2_MBUS_FMT_SGRBG10_1X10,
+ .field = V4L2_FIELD_NONE,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ },
+};
+
+static struct fimc_is_sensor *sd_to_fimc_is_sensor(struct v4l2_subdev *sd)
+{
+ return container_of(sd, struct fimc_is_sensor, subdev);
+}
+
+static void sensor_clk_put(struct fimc_is_sensor *sensor)
+{
+ int i;
+
+ for (i = 0; i SCLK_MAX_NUM; i++) {
+ if (IS_ERR(sensor-clock[i]))
+ continue;
+ clk_unprepare(sensor-clock[i]);
+ clk_put(sensor-clock[i]);
+ sensor-clock[i] = ERR_PTR(-EINVAL);
+ }
+}
+
+static int sensor_clk_init(struct fimc_is_sensor *sensor)
+{
+ int i, ret;
+
+ /* Get CAM clocks */
+ for (i = 0; i SCLK_MAX_NUM; i++) {
+ sensor-clock[i] = clk_get(NULL, sensor_clock_name[i]);
+ if (IS_ERR(sensor-clock[i]))
+ goto err;
+ ret = clk_prepare(sensor-clock[i]);
+ if (ret 0) {
+ clk_put(sensor-clock[i]);
+ sensor-clock[i] = ERR_PTR(-EINVAL);
+ goto err;
+ }
+ }
+
+ /* Set clock rates */
+ ret = clk_set_rate(sensor-clock[SCLK_CAM0], 24 * 100);
+ ret |= clk_set_rate(sensor-clock[SCLK_BAYER], 24 * 100);
+ if (ret) {
+ pr_err(Failed to set cam clock rates\n);
+ goto err;
+ }
+ return 0;
+err:
+ sensor_clk_put(sensor);
+ pr_err(Failed to init sensor clock\n);
+ return -ENXIO;
+}
+
+static int sensor_clk_enable(struct fimc_is_sensor *sensor)
+{
+ int ret = 0, i;
+
+ for (i = 0; i SCLK_MAX_NUM; i++) {
+ ret = clk_enable(sensor-clock[i]);
+ if (ret)
+ return ret;
+ }
+ return ret;
+}
+
+static void sensor_clk_disable(struct fimc_is_sensor *sensor)
+{
+ int i;
+
+ for (i = 0; i SCLK_MAX_NUM; i++)
+ clk_disable(sensor-clock[i]);
+}
+
+static int sensor_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ struct fimc_is_sensor *sensor = sd_to_fimc_is_sensor(sd);
+ struct fimc_is_sensor_drv_data *sdata = sensor-drvdata;
+
+ if (!code)
+ return -EINVAL;
+
+ code-code = sensor_formats[sdata-sensor_id].code;
+ return 0;
+}
+
+static int sensor_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_format *fmt)
+{
+ struct fimc_is_sensor *sensor = sd_to_fimc_is_sensor(sd);
