[PATCH v11 12/12] V4L: Add s5k4e5 sensor driver
This patch adds subdev driver for Samsung S5K4E5 raw image sensor.
Like s5k6a3, it is also another fimc-is firmware controlled
sensor. This minimal sensor driver doesn't do any I2C communications
as its done by ISP firmware. It can be updated if needed to a
regular sensor driver by adding the I2C communication.
Signed-off-by: Arun Kumar K
Reviewed-by: Sylwester Nawrocki
---
drivers/media/i2c/Kconfig |8 ++
drivers/media/i2c/Makefile |1 +
drivers/media/i2c/s5k4e5.c | 344
3 files changed, 353 insertions(+)
create mode 100644 drivers/media/i2c/s5k4e5.c
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index f7e9147..271028b 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -572,6 +572,14 @@ config VIDEO_S5K6A3
This is a V4L2 sensor-level driver for Samsung S5K6A3 raw
camera sensor.
+config VIDEO_S5K4E5
+ tristate "Samsung S5K4E5 sensor support"
+ depends on MEDIA_CAMERA_SUPPORT
+ depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF
+ ---help---
+ This is a V4L2 sensor-level driver for Samsung S5K4E5 raw
+ camera sensor.
+
config VIDEO_S5K4ECGX
tristate "Samsung S5K4ECGX sensor support"
depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index cf3cf03..0aeed8e 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o
obj-$(CONFIG_VIDEO_NOON010PC30)+= noon010pc30.o
obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o
obj-$(CONFIG_VIDEO_S5K6A3) += s5k6a3.o
+obj-$(CONFIG_VIDEO_S5K4E5) += s5k4e5.o
obj-$(CONFIG_VIDEO_S5K4ECGX) += s5k4ecgx.o
obj-$(CONFIG_VIDEO_S5C73M3)+= s5c73m3/
obj-$(CONFIG_VIDEO_ADP1653)+= adp1653.o
diff --git a/drivers/media/i2c/s5k4e5.c b/drivers/media/i2c/s5k4e5.c
new file mode 100644
index 000..5d4007e
--- /dev/null
+++ b/drivers/media/i2c/s5k4e5.c
@@ -0,0 +1,344 @@
+/*
+ * Samsung S5K4E5 image sensor driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Arun Kumar K
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define S5K4E5_SENSOR_MAX_WIDTH2576
+#define S5K4E5_SENSOR_MAX_HEIGHT 1930
+
+#define S5K4E5_SENSOR_MIN_WIDTH(32 + 16)
+#define S5K4E5_SENSOR_MIN_HEIGHT (32 + 10)
+
+#define S5K4E5_DEF_WIDTH 1296
+#define S5K4E5_DEF_HEIGHT 732
+
+#define S5K4E5_DRV_NAME"S5K4E5"
+#define S5K4E5_CLK_NAME"extclk"
+
+#define S5K4E5_NUM_SUPPLIES2
+
+#define S5K4E5_DEF_CLK_FREQ2400
+
+/**
+ * struct s5k4e5 - s5k4e5 sensor data structure
+ * @dev: pointer to this I2C client device structure
+ * @subdev: the image sensor's v4l2 subdev
+ * @pad: subdev media source pad
+ * @supplies: image sensor's voltage regulator supplies
+ * @gpio_reset: GPIO connected to the sensor's reset pin
+ * @lock: mutex protecting the structure's members below
+ * @format: media bus format at the sensor's source pad
+ */
+struct s5k4e5 {
+ struct device *dev;
+ struct v4l2_subdev subdev;
+ struct media_pad pad;
+ struct regulator_bulk_data supplies[S5K4E5_NUM_SUPPLIES];
+ int gpio_reset;
+ struct mutex lock;
+ struct v4l2_mbus_framefmt format;
+ struct clk *clock;
+ u32 clock_frequency;
+};
+
+static const char * const s5k4e5_supply_names[] = {
+ "svdda",
+ "svddio"
+};
+
+static inline struct s5k4e5 *sd_to_s5k4e5(struct v4l2_subdev *sd)
+{
+ return container_of(sd, struct s5k4e5, subdev);
+}
+
+static const struct v4l2_mbus_framefmt s5k4e5_formats[] = {
+ {
+ .code = V4L2_MBUS_FMT_SGRBG10_1X10,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ .field = V4L2_FIELD_NONE,
+ }
+};
+
+static const struct v4l2_mbus_framefmt *find_sensor_format(
+ struct v4l2_mbus_framefmt *mf)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(s5k4e5_formats); i++)
+ if (mf->code == s5k4e5_formats[i].code)
+ return &s5k4e5_formats[i];
+
+ return &s5k4e5_formats[0];
+}
+
+static int s5k4e5_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ if (code->index >= ARRAY_SIZE(s5k4e5_formats))
+ return -EINVAL;
+
+ code->code = s5k4e5_formats[code->index].code;
+ return 0;
+}
+
+static void s5k4e
Re: [PATCH] [media] az6007: support Technisat Cablestar Combo HDCI (minus remote)
Thank you very much for the patch.
Quote/Cytat - Roland Scheidegger (Sat
02 Nov 2013 08:49:32 PM CET):
[...]
Originally based on idea found on
http://www.linuxtv.org/wiki/index.php/TechniSat_CableStar_Combo_HD_CI
claiming
only id needs to be added (but failed to mention it only worked because the
driver couldn't find the h7 drx-k firmware...).
Together with Tobias Wessel, another user of the device, we have
updated and extended the wiki entry.
The problem is that although I use the same system as Tobias (Debian
wheezy) and it seems we installed media_build in the identical way, it
works OK for Tobias but not for me, as the required drivers are not
loaded. The device itself is not a problem because it works perfectly
on Windows.
Looks like udev may be the culprit, I enclose a log fragment from my
rather chaotic experiments - I'm just a Debian user and my knowledge
of the system internals is rudimentary.
So my questions are:
Do you have any comments or suggestions regarding the modified wiki entry?
Where to look for help with my problem?
How to uninstall media_build to start experimenting from the scratch?
Best regards
Janusz
--
Prof. dr hab. Janusz S. Bień - Uniwersytet Warszawski (Katedra
Lingwistyki Formalnej)
Prof. Janusz S. Bień - University of Warsaw (Formal Linguistics Department)
jsb...@uw.edu.pl, jsb...@mimuw.edu.pl, http://fleksem.klf.uw.edu.pl/~jsbien/Nov 4 23:14:14 cauda kernel: [ 343.333559] usb 7-4: New USB device found, idVendor=14f7, idProduct=0003
Nov 4 23:14:14 cauda kernel: [ 343.333566] usb 7-4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
Nov 4 23:14:14 cauda kernel: [ 343.333571] usb 7-4: Product: CableStar Combo HD CI
Nov 4 23:14:14 cauda kernel: [ 343.333574] usb 7-4: Manufacturer: TechniSat Digital S.A.
Nov 4 23:14:14 cauda kernel: [ 343.333578] usb 7-4: SerialNumber: 0008C9D91826
Nov 4 23:14:14 cauda udevd[3696]: 'usb-db /devices/pci:00/:00:1d.7/usb7/7-4'(out) 'ID_VENDOR_FROM_DATABASE=TechniSat Digital GmbH'
Nov 4 23:14:14 cauda udevd[3696]: 'usb-db /devices/pci:00/:00:1d.7/usb7/7-4'(out) 'ID_MODEL_FROM_DATABASE=CableStar Combo HD CI'
Nov 4 23:14:14 cauda udevd[3696]: 'usb-db /devices/pci:00/:00:1d.7/usb7/7-4' [5215] exit with return code 0
Nov 4 23:14:14 cauda udevd[3696]: IMPORT builtin 'usb_id' /lib/udev/rules.d/60-libgphoto2-2.rules:11
Nov 4 23:14:14 cauda udevd[3696]: ID_VENDOR=TechniSat_Digital_S.A.
Nov 4 23:14:14 cauda udevd[3696]: ID_VENDOR_ENC=TechniSat\x20Digital\x20S.A.
Nov 4 23:14:14 cauda udevd[3696]: ID_VENDOR_ID=14f7
Nov 4 23:14:14 cauda udevd[3696]: ID_MODEL=CableStar_Combo_HD_CI
Nov 4 23:14:14 cauda udevd[3696]: ID_MODEL_ENC=CableStar\x20Combo\x20HD\x20CI
Nov 4 23:14:14 cauda udevd[3696]: ID_MODEL_ID=0003
Nov 4 23:14:14 cauda udevd[3696]: ID_REVISION=0003
Nov 4 23:14:14 cauda udevd[3696]: ID_SERIAL=TechniSat_Digital_S.A._CableStar_Combo_HD_CI_0008C9D91826
Nov 4 23:14:14 cauda udevd[3696]: ID_SERIAL_SHORT=0008C9D91826
Nov 4 23:14:14 cauda udevd[3696]: ID_BUS=usb
Nov 4 23:14:14 cauda udevd[3696]: ID_USB_INTERFACES=:ff:
Nov 4 23:14:14 cauda udevd[3696]: RUN '/usr/share/virtualbox/VBoxCreateUSBNode.sh $major $minor $attr{bDeviceClass}' /etc/udev/rules.d/60-vboxdrv.rules:5
Nov 4 23:14:14 cauda udevd[3696]: PROGRAM 'mtp-probe /sys/devices/pci:00/:00:1d.7/usb7/7-4 7 6' /lib/udev/rules.d/69-libmtp.rules:939
Nov 4 23:14:14 cauda udevd[5216]: starting 'mtp-probe /sys/devices/pci:00/:00:1d.7/usb7/7-4 7 6'
Nov 4 23:14:14 cauda mtp-probe: checking bus 7, device 6: "/sys/devices/pci:00/:00:1d.7/usb7/7-4"
Nov 4 23:14:14 cauda mtp-probe: bus: 7, device: 6 was not an MTP device
Nov 4 23:14:14 cauda udevd[3696]: 'mtp-probe /sys/devices/pci:00/:00:1d.7/usb7/7-4 7 6'(out) '0'
Nov 4 23:14:14 cauda udevd[3696]: 'mtp-probe /sys/devices/pci:00/:00:1d.7/usb7/7-4 7 6' [5216] exit with return code 0
Nov 4 23:14:14 cauda udevd[3696]: MODE 0664 /lib/udev/rules.d/91-permissions.rules:36
Nov 4 23:14:14 cauda udevd[3696]: no node name set, will use kernel supplied name 'bus/usb/007/006'
Nov 4 23:14:14 cauda udevd[3696]: creating device node '/dev/bus/usb/007/006', devnum=189:773, mode=01664, uid=0, gid=0
Nov 4 23:14:14 cauda udevd[3696]: preserve file '/dev/bus/usb/007/006', because it has correct dev_t
Nov 4 23:14:14 cauda udevd[3696]: set permissions /dev/bus/usb/007/006, 021664, uid=0, gid=0
Nov 4 23:14:14 cauda udevd[3696]: creating symlink '/dev/char/189:773' to '../bus/usb/007/006'
Nov 4 23:14:14 cauda udevd[3696]: created db file '/run/udev/data/c189:773' for '/devices/pci:00/:00:1d.7/usb7/7-4'
Nov 4 23:14:14 cauda udevd[5217]: starting '/usr/share/virtualbox/VBoxCreateUSBNode.sh 189 773 00'
Nov 4 23:14:14 cauda udevd[3696]: '/usr/share/virtualbox/VBoxCreateUSBNode.sh 189 773 00' [5217] exit with return code 0
Nov 4 23:14:14 cauda udevd[3696]: passed -1 bytes to netlink monitor 0x9bea488
Nov 4 23:14:14 cauda udevd[3696
[PATCH v11 07/12] [media] exynos5-fimc-is: Add sensor interface
Some sensors to be used with fimc-is are exclusively controlled
by the fimc-is firmware. This minimal sensor driver provides
the required info for the firmware to configure the sensors
sitting on I2C bus.
Signed-off-by: Arun Kumar K
Reviewed-by: Sylwester Nawrocki
---
drivers/media/platform/exynos5-is/fimc-is-sensor.c | 45 ++
drivers/media/platform/exynos5-is/fimc-is-sensor.h | 65
2 files changed, 110 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..475f1c3
--- /dev/null
+++ b/drivers/media/platform/exynos5-is/fimc-is-sensor.c
@@ -0,0 +1,45 @@
+/*
+ * Samsung EXYNOS5250 FIMC-IS (Imaging Subsystem) driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Arun Kumar K
+ *
+ * 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 "fimc-is-sensor.h"
+
+static const struct sensor_drv_data s5k6a3_drvdata = {
+ .id = FIMC_IS_SENSOR_ID_S5K6A3,
+ .open_timeout = S5K6A3_OPEN_TIMEOUT,
+ .setfile_name = "exynos5_s5k6a3_setfile.bin",
+};
+
+static const struct sensor_drv_data s5k4e5_drvdata = {
+ .id = FIMC_IS_SENSOR_ID_S5K4E5,
+ .open_timeout = S5K4E5_OPEN_TIMEOUT,
+ .setfile_name = "exynos5_s5k4e5_setfile.bin",
+};
+
+static const struct of_device_id fimc_is_sensor_of_ids[] = {
+ {
+ .compatible = "samsung,s5k6a3",
+ .data = &s5k6a3_drvdata,
+ },
+ {
+ .compatible = "samsung,s5k4e5",
+ .data = &s5k4e5_drvdata,
+ },
+ { }
+};
+
+const struct sensor_drv_data *exynos5_is_sensor_get_drvdata(
+ struct device_node *node)
+{
+ const struct of_device_id *of_id;
+
+ of_id = of_match_node(fimc_is_sensor_of_ids, node);
+ return of_id ? of_id->data : NULL;
+}
diff --git a/drivers/media/platform/exynos5-is/fimc-is-sensor.h
b/drivers/media/platform/exynos5-is/fimc-is-sensor.h
new file mode 100644
index 000..0ba5733
--- /dev/null
+++ b/drivers/media/platform/exynos5-is/fimc-is-sensor.h
@@ -0,0 +1,65 @@
+/*
+ * Samsung EXYNOS4x12 FIMC-IS (Imaging Subsystem) driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Arun Kumar K
+ *
+ * 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.
+ */
+#ifndef FIMC_IS_SENSOR_H_
+#define FIMC_IS_SENSOR_H_
+
+#include
+#include
+
+#define S5K6A3_OPEN_TIMEOUT2000 /* ms */
+#define S5K6A3_SENSOR_WIDTH1392
+#define S5K6A3_SENSOR_HEIGHT 1392
+
+#define S5K4E5_OPEN_TIMEOUT2000 /* ms */
+#define S5K4E5_SENSOR_WIDTH2560
+#define S5K4E5_SENSOR_HEIGHT 1920
+
+#define SENSOR_WIDTH_PADDING 16
+#define SENSOR_HEIGHT_PADDING 10
+
+enum fimc_is_sensor_id {
+ FIMC_IS_SENSOR_ID_S5K3H2 = 1,
+ FIMC_IS_SENSOR_ID_S5K6A3,
+ FIMC_IS_SENSOR_ID_S5K4E5,
+ FIMC_IS_SENSOR_ID_S5K3H7,
+ FIMC_IS_SENSOR_ID_CUSTOM,
+ FIMC_IS_SENSOR_ID_END
+};
+
+struct sensor_drv_data {
+ enum fimc_is_sensor_id id;
+ /* sensor open timeout in ms */
+ unsigned short open_timeout;
+ char *setfile_name;
+};
+
+/**
+ * struct fimc_is_sensor - fimc-is sensor data structure
+ * @drvdata: a pointer to the sensor's parameters data structure
+ * @i2c_bus: ISP I2C bus index (0...1)
+ * @width: sensor active width
+ * @height: sensor active height
+ * @pixel_width: sensor effective pixel width (width + padding)
+ * @pixel_height: sensor effective pixel height (height + padding)
+ */
+struct fimc_is_sensor {
+ const struct sensor_drv_data *drvdata;
+ unsigned int i2c_bus;
+ unsigned int width;
+ unsigned int height;
+ unsigned int pixel_width;
+ unsigned int pixel_height;
+};
+
+const struct sensor_drv_data *exynos5_is_sensor_get_drvdata(
+ struct device_node *node);
+
+#endif /* FIMC_IS_SENSOR_H_ */
--
1.7.9.5
--
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
[PATCH v11 05/12] [media] exynos5-fimc-is: Add isp subdev
fimc-is driver takes video data input from the ISP video node
which is added in this patch. This node accepts Bayer input
buffers which is given from the IS sensors.
Signed-off-by: Arun Kumar K
Signed-off-by: Kilyeon Im
Reviewed-by: Sylwester Nawrocki
---
drivers/media/platform/exynos5-is/fimc-is-isp.c | 534 +++
drivers/media/platform/exynos5-is/fimc-is-isp.h | 90
2 files changed, 624 insertions(+)
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-isp.c
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-isp.h
diff --git a/drivers/media/platform/exynos5-is/fimc-is-isp.c
b/drivers/media/platform/exynos5-is/fimc-is-isp.c
new file mode 100644
index 000..7bd603f
--- /dev/null
+++ b/drivers/media/platform/exynos5-is/fimc-is-isp.c
@@ -0,0 +1,534 @@
+/*
+ * Samsung EXYNOS5250 FIMC-IS (Imaging Subsystem) driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Arun Kumar K
+ *
+ * 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
+#include
+
+#include "fimc-is.h"
+
+#define ISP_DRV_NAME "fimc-is-isp"
+
+static const struct fimc_is_fmt formats[] = {
+ {
+ .name = "Bayer GR-BG 8bits",
+ .fourcc = V4L2_PIX_FMT_SGRBG8,
+ .depth = { 8 },
+ .num_planes = 1,
+ },
+ {
+ .name = "Bayer GR-BG 10bits",
+ .fourcc = V4L2_PIX_FMT_SGRBG10,
+ .depth = { 16 },
+ .num_planes = 1,
+ },
+ {
+ .name = "Bayer GR-BG 12bits",
+ .fourcc = V4L2_PIX_FMT_SGRBG12,
+ .depth = { 16 },
+ .num_planes = 1,
+ },
+};
+#define NUM_FORMATS ARRAY_SIZE(formats)
+
+static const struct fimc_is_fmt *find_format(struct v4l2_format *f)
+{
+ unsigned int i;
+
+ for (i = 0; i < NUM_FORMATS; i++)
+ if (formats[i].fourcc == f->fmt.pix_mp.pixelformat)
+ return &formats[i];
+ return NULL;
+}
+
+static int isp_video_output_start_streaming(struct vb2_queue *vq,
+ unsigned int count)
+{
+ struct fimc_is_isp *isp = vb2_get_drv_priv(vq);
+
+ set_bit(STATE_RUNNING, &isp->output_state);
+ return 0;
+}
+
+static int isp_video_output_stop_streaming(struct vb2_queue *vq)
+{
+ struct fimc_is_isp *isp = vb2_get_drv_priv(vq);
+ struct fimc_is_buf *buf;
+
+ /* Release unused buffers */
+ while (!list_empty(&isp->wait_queue)) {
+ buf = fimc_is_isp_wait_queue_get(isp);
+ vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
+ }
+ while (!list_empty(&isp->run_queue)) {
+ buf = fimc_is_isp_run_queue_get(isp);
+ vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
+ }
+
+ clear_bit(STATE_RUNNING, &isp->output_state);
+ return 0;
+}
+
+static int isp_video_output_queue_setup(struct vb2_queue *vq,
+ const struct v4l2_format *pfmt,
+ unsigned int *num_buffers, unsigned int *num_planes,
+ unsigned int sizes[], void *allocators[])
+{
+ struct fimc_is_isp *isp = vb2_get_drv_priv(vq);
+ const struct fimc_is_fmt *fmt = isp->fmt;
+ unsigned int wh, i;
+
+ if (!fmt)
+ return -EINVAL;
+
+ *num_planes = fmt->num_planes;
+ wh = isp->width * isp->height;
+
+ for (i = 0; i < *num_planes; i++) {
+ allocators[i] = isp->alloc_ctx;
+ sizes[i] = (wh * fmt->depth[i]) / 8;
+ }
+ return 0;
+}
+
+static int isp_video_output_buffer_init(struct vb2_buffer *vb)
+{
+ struct fimc_is_buf *buf = container_of(vb, struct fimc_is_buf, vb);
+
+ buf->paddr[0] = vb2_dma_contig_plane_dma_addr(vb, 0);
+ return 0;
+}
+
+static int isp_video_output_buffer_prepare(struct vb2_buffer *vb)
+{
+ struct vb2_queue *vq = vb->vb2_queue;
+ struct fimc_is_isp *isp = vb2_get_drv_priv(vq);
+ unsigned long size;
+
+ size = (isp->width * isp->height * isp->fmt->depth[0]) / 8;
+ if (vb2_plane_size(vb, 0) < size) {
+ v4l2_err(&isp->subdev, "User buffer too small (%ld < %ld)\n",
+vb2_plane_size(vb, 0), size);
+ return -EINVAL;
+ }
+ vb2_set_plane_payload(vb, 0, size);
+
+ return 0;
+}
+
+static void isp_video_output_buffer_queue(struct vb2_buffer *vb)
+{
+ struct vb2_queue *vq = vb->vb2_queue;
+ struct fimc_is_isp *isp = vb2_get_drv_priv(vq);
+ struct fimc_is_buf *buf = container_of(vb, struct fimc_is_buf, vb);
+
+ fimc_is_pipeline_buf_lock(isp->pipeline);
+ fimc_is_isp_wait_queue_add(isp, buf);
+ fimc_is_pi
[PATCH v11 09/12] [media] exynos5-fimc-is: Add the hardware interface module
The hardware interface module finally sends the commands to the
FIMC-IS firmware and runs the interrupt handler for getting the
responses.
Signed-off-by: Arun Kumar K
Signed-off-by: Kilyeon Im
Reviewed-by: Sylwester Nawrocki
---
.../media/platform/exynos5-is/fimc-is-interface.c | 810
.../media/platform/exynos5-is/fimc-is-interface.h | 124 +++
2 files changed, 934 insertions(+)
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-interface.c
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-interface.h
diff --git a/drivers/media/platform/exynos5-is/fimc-is-interface.c
b/drivers/media/platform/exynos5-is/fimc-is-interface.c
new file mode 100644
index 000..c5da6ff
--- /dev/null
+++ b/drivers/media/platform/exynos5-is/fimc-is-interface.c
@@ -0,0 +1,810 @@
+/*
+ * Samsung EXYNOS5 FIMC-IS (Imaging Subsystem) driver
+*
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Kil-yeon Lim
+ *
+ * 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
+#include
+#include "fimc-is.h"
+#include "fimc-is-cmd.h"
+#include "fimc-is-regs.h"
+
+#define init_request_barrier(itf) mutex_init(&itf->request_barrier)
+#define enter_request_barrier(itf) mutex_lock(&itf->request_barrier)
+#define exit_request_barrier(itf) mutex_unlock(&itf->request_barrier)
+
+static inline void itf_get_cmd(struct fimc_is_interface *itf,
+ struct fimc_is_msg *msg, unsigned int index)
+{
+ struct is_common_reg __iomem *com_regs = itf->com_regs;
+
+ memset(msg, 0, sizeof(*msg));
+
+ switch (index) {
+ case INTR_GENERAL:
+ msg->command = com_regs->ihcmd;
+ msg->instance = com_regs->ihc_sensorid;
+ memcpy(msg->param, com_regs->ihc_param,
+ 4 * sizeof(msg->param[0]));
+ break;
+ case INTR_SCC_FDONE:
+ msg->command = IHC_FRAME_DONE;
+ msg->instance = com_regs->scc_sensor_id;
+ memcpy(msg->param, com_regs->scc_param,
+ 3 * sizeof(msg->param[0]));
+ break;
+ case INTR_SCP_FDONE:
+ msg->command = IHC_FRAME_DONE;
+ msg->instance = com_regs->scp_sensor_id;
+ memcpy(msg->param, com_regs->scp_param,
+ 3 * sizeof(msg->param[0]));
+ break;
+ case INTR_META_DONE:
+ msg->command = IHC_FRAME_DONE;
+ msg->instance = com_regs->meta_sensor_id;
+ msg->param[0] = com_regs->meta_param1;
+ break;
+ case INTR_SHOT_DONE:
+ msg->command = IHC_FRAME_DONE;
+ msg->instance = com_regs->shot_sensor_id;
+ memcpy(msg->param, com_regs->shot_param,
+ 2 * sizeof(msg->param[0]));
+ break;
+ default:
+ dev_err(itf->dev, "%s Unknown command\n", __func__);
+ break;
+ }
+}
+
+static inline unsigned int itf_get_intr(struct fimc_is_interface *itf)
+{
+ unsigned int status;
+ struct is_common_reg __iomem *com_regs = itf->com_regs;
+
+ status = readl(itf->regs + INTMSR1) | com_regs->ihcmd_iflag |
+ com_regs->scc_iflag |
+ com_regs->scp_iflag |
+ com_regs->meta_iflag |
+ com_regs->shot_iflag;
+
+ return status;
+}
+
+static void itf_set_state(struct fimc_is_interface *itf,
+ unsigned long state)
+{
+ unsigned long flags;
+ spin_lock_irqsave(&itf->slock_state, flags);
+ __set_bit(state, &itf->state);
+ spin_unlock_irqrestore(&itf->slock_state, flags);
+}
+
+static void itf_clr_state(struct fimc_is_interface *itf,
+ unsigned long state)
+{
+ unsigned long flags;
+ spin_lock_irqsave(&itf->slock_state, flags);
+ __clear_bit(state, &itf->state);
+ spin_unlock_irqrestore(&itf->slock_state, flags);
+}
+
+static int itf_get_state(struct fimc_is_interface *itf,
+ unsigned long state)
+{
+ int ret = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(&itf->slock_state, flags);
+ ret = test_bit(state, &itf->state);
+ spin_unlock_irqrestore(&itf->slock_state, flags);
+ return ret;
+}
+
+static void itf_init_wakeup(struct fimc_is_interface *itf)
+{
+ itf_set_state(itf, IS_IF_STATE_INIT);
+ wake_up(&itf->irq_queue);
+}
+
+void itf_busy_wakeup(struct fimc_is_interface *itf)
+{
+ itf_clr_state(itf, IS_IF_STATE_BUSY);
+ wake_up(&itf->irq_queue);
+}
+
+static int itf_wait_hw_ready(struct fimc_is_interface *itf)
+{
+ int t;
+ for (t = TRY_RECV_AWARE_COUNT; t >= 0; t--) {
+ unsigned int cfg = readl(itf->regs + INTMSR0);
+ if (INTMSR0_GET_INTMSD(0, cfg) == 0)
+
[PATCH v11 06/12] [media] exynos5-fimc-is: Add scaler subdev
FIMC-IS has two hardware scalers named as scaler-codec and
scaler-preview. This patch adds the common code handling the
video nodes and subdevs of both the scalers.
Signed-off-by: Arun Kumar K
Signed-off-by: Kilyeon Im
Reviewed-by: Sylwester Nawrocki
---
drivers/media/platform/exynos5-is/fimc-is-scaler.c | 476
drivers/media/platform/exynos5-is/fimc-is-scaler.h | 106 +
2 files changed, 582 insertions(+)
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-scaler.c
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-scaler.h
diff --git a/drivers/media/platform/exynos5-is/fimc-is-scaler.c
b/drivers/media/platform/exynos5-is/fimc-is-scaler.c
new file mode 100644
index 000..029eb8b
--- /dev/null
+++ b/drivers/media/platform/exynos5-is/fimc-is-scaler.c
@@ -0,0 +1,476 @@
+/*
+ * Samsung EXYNOS5250 FIMC-IS (Imaging Subsystem) driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Arun Kumar K
+ *
+ * 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
+#include
+
+#include "fimc-is.h"
+
+#define IS_SCALER_DRV_NAME "fimc-is-scaler"
+
+static const struct fimc_is_fmt formats[] = {
+ {
+ .name = "YUV 4:2:0 3p MultiPlanar",
+ .fourcc = V4L2_PIX_FMT_YUV420M,
+ .depth = {8, 2, 2},
+ .num_planes = 3,
+ },
+ {
+ .name = "YUV 4:2:0 2p MultiPlanar",
+ .fourcc = V4L2_PIX_FMT_NV12M,
+ .depth = {8, 4},
+ .num_planes = 2,
+ },
+ {
+ .name = "YUV 4:2:2 1p MultiPlanar",
+ .fourcc = V4L2_PIX_FMT_NV16,
+ .depth = {16},
+ .num_planes = 1,
+ },
+};
+#define NUM_FORMATS ARRAY_SIZE(formats)
+
+static const struct fimc_is_fmt *find_format(struct v4l2_format *f)
+{
+ unsigned int i;
+
+ for (i = 0; i < NUM_FORMATS; i++) {
+ if (formats[i].fourcc == f->fmt.pix_mp.pixelformat)
+ return &formats[i];
+ }
+ return NULL;
+}
+
+static int scaler_video_capture_start_streaming(struct vb2_queue *vq,
+ unsigned int count)
+{
+ struct fimc_is_scaler *ctx = vb2_get_drv_priv(vq);
+ int ret;
+
+ ret = fimc_is_pipeline_scaler_start(ctx->pipeline,
+ ctx->scaler_id,
+ vq->num_buffers,
+ ctx->fmt->num_planes);
+ if (ret) {
+ v4l2_err(&ctx->subdev, "Scaler start failed.\n");
+ return -EINVAL;
+ }
+
+ set_bit(STATE_RUNNING, &ctx->capture_state);
+ return 0;
+}
+
+static int scaler_video_capture_stop_streaming(struct vb2_queue *vq)
+{
+ struct fimc_is_scaler *ctx = vb2_get_drv_priv(vq);
+ struct fimc_is_buf *buf;
+ int ret;
+
+ ret = fimc_is_pipeline_scaler_stop(ctx->pipeline, ctx->scaler_id);
+ if (ret)
+ v4l2_info(&ctx->subdev, "Scaler already stopped.\n");
+
+ /* Release un-used buffers */
+ while (!list_empty(&ctx->wait_queue)) {
+ buf = fimc_is_scaler_wait_queue_get(ctx);
+ vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
+ }
+ while (!list_empty(&ctx->run_queue)) {
+ buf = fimc_is_scaler_run_queue_get(ctx);
+ vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
+ }
+
+ clear_bit(STATE_RUNNING, &ctx->capture_state);
+ return 0;
+}
+
+static int scaler_video_capture_queue_setup(struct vb2_queue *vq,
+ const struct v4l2_format *pfmt,
+ unsigned int *num_buffers, unsigned int *num_planes,
+ unsigned int sizes[], void *allocators[])
+{
+ struct fimc_is_scaler *ctx = vb2_get_drv_priv(vq);
+ const struct fimc_is_fmt *fmt = ctx->fmt;
+ unsigned int wh;
+ int i;
+
+ if (!fmt)
+ return -EINVAL;
+
+ *num_planes = fmt->num_planes;
+ wh = ctx->width * ctx->height;
+
+ for (i = 0; i < *num_planes; i++) {
+ allocators[i] = ctx->alloc_ctx;
+ sizes[i] = (wh * fmt->depth[i]) / 8;
+ }
+ return 0;
+}
+
+static int scaler_video_capture_buffer_init(struct vb2_buffer *vb)
+{
+ struct vb2_queue *vq = vb->vb2_queue;
+ struct fimc_is_scaler *ctx = vb2_get_drv_priv(vq);
+ struct fimc_is_buf *buf = container_of(vb, struct fimc_is_buf, vb);
+ const struct fimc_is_fmt *fmt;
+ int i;
+
+ fmt = ctx->fmt;
+ for (i = 0; i < fmt->num_planes; i++)
+ buf->paddr[i] = vb2_dma_contig_plane_dma_addr(vb, i);
+
+ return 0;
+}
+
+static int scaler_video_capture_buffer_prepare(struct vb2_buffer
[PATCH v11 10/12] [media] exynos5-is: Add Kconfig and Makefile
Adds Kconfig and Makefile for exynos5-is driver files. Signed-off-by: Shaik Ameer Basha Signed-off-by: Arun Kumar K Reviewed-by: Sylwester Nawrocki --- drivers/media/platform/Kconfig |1 + drivers/media/platform/Makefile|1 + drivers/media/platform/exynos5-is/Kconfig | 20 drivers/media/platform/exynos5-is/Makefile |7 +++ 4 files changed, 29 insertions(+) create mode 100644 drivers/media/platform/exynos5-is/Kconfig create mode 100644 drivers/media/platform/exynos5-is/Makefile diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index 08de865..4b0475e 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -123,6 +123,7 @@ config VIDEO_S3C_CAMIF source "drivers/media/platform/soc_camera/Kconfig" source "drivers/media/platform/exynos4-is/Kconfig" +source "drivers/media/platform/exynos5-is/Kconfig" source "drivers/media/platform/s5p-tv/Kconfig" endif # V4L_PLATFORM_DRIVERS diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile index eee28dd..40bf09f 100644 --- a/drivers/media/platform/Makefile +++ b/drivers/media/platform/Makefile @@ -37,6 +37,7 @@ obj-$(CONFIG_VIDEO_SAMSUNG_S5P_TV)+= s5p-tv/ obj-$(CONFIG_VIDEO_SAMSUNG_S5P_G2D)+= s5p-g2d/ obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC) += exynos-gsc/ +obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS5_CAMERA) += exynos5-is/ obj-$(CONFIG_BLACKFIN) += blackfin/ diff --git a/drivers/media/platform/exynos5-is/Kconfig b/drivers/media/platform/exynos5-is/Kconfig new file mode 100644 index 000..b67d11a --- /dev/null +++ b/drivers/media/platform/exynos5-is/Kconfig @@ -0,0 +1,20 @@ +config VIDEO_SAMSUNG_EXYNOS5_CAMERA + bool "Samsung Exynos5 SoC Camera Media Device driver" + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && PM_RUNTIME + depends on VIDEO_SAMSUNG_EXYNOS4_IS + help + This is a V4L2 media device driver for Exynos5 SoC series + camera subsystem. + +if VIDEO_SAMSUNG_EXYNOS5_CAMERA + +config VIDEO_SAMSUNG_EXYNOS5_FIMC_IS + tristate "Samsung Exynos5 SoC FIMC-IS driver" + depends on I2C && OF + depends on VIDEO_EXYNOS4_FIMC_IS + select VIDEOBUF2_DMA_CONTIG + help + This is a V4L2 driver for Samsung Exynos5 SoC series Imaging + Subsystem known as FIMC-IS. + +endif #VIDEO_SAMSUNG_EXYNOS5_MDEV diff --git a/drivers/media/platform/exynos5-is/Makefile b/drivers/media/platform/exynos5-is/Makefile new file mode 100644 index 000..6cdb037 --- /dev/null +++ b/drivers/media/platform/exynos5-is/Makefile @@ -0,0 +1,7 @@ +ccflags-y += -Idrivers/media/platform/exynos4-is +exynos5-fimc-is-objs := fimc-is-core.o fimc-is-isp.o fimc-is-scaler.o +exynos5-fimc-is-objs += fimc-is-pipeline.o fimc-is-interface.o fimc-is-sensor.o +exynos-mdevice-objs := exynos5-mdev.o + +obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS5_FIMC_IS) += exynos5-fimc-is.o +obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS5_CAMERA) += exynos-mdevice.o -- 1.7.9.5 -- 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
[PATCH v11 11/12] V4L: Add DT binding doc for s5k4e5 image sensor
S5K4E5 is a Samsung raw image sensor controlled via I2C.
This patch adds the DT binding documentation for the same.
Signed-off-by: Arun Kumar K
Reviewed-by: Sylwester Nawrocki
Acked-by: Mark Rutland
---
.../devicetree/bindings/media/samsung-s5k4e5.txt | 45
1 file changed, 45 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/samsung-s5k4e5.txt
diff --git a/Documentation/devicetree/bindings/media/samsung-s5k4e5.txt
b/Documentation/devicetree/bindings/media/samsung-s5k4e5.txt
new file mode 100644
index 000..fc37792
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/samsung-s5k4e5.txt
@@ -0,0 +1,45 @@
+* Samsung S5K4E5 Raw Image Sensor
+
+S5K4E5 is a raw image sensor with maximum resolution of 2560x1920
+pixels. Data transfer is carried out via MIPI CSI-2 port and controls
+via I2C bus.
+
+Required Properties:
+- compatible : should contain "samsung,s5k4e5"
+- reg : I2C device address
+- reset-gpios : specifier of a GPIO connected to the RESET pin
+- clocks : should refer to the clock named in clock-names, from
+ the common clock bindings
+- clock-names : should contain "extclk" entry
+- svdda-supply : core voltage supply
+- svddio-supply: I/O voltage supply
+
+Optional Properties:
+- clock-frequency : the frequency at which the "extclk" clock should be
+ configured to operate, in Hz; if this property is not
+ specified default 24 MHz value will be used
+
+The device node should be added to respective control bus controller
+(e.g. I2C0) nodes and linked to the csis port node, using the common
+video interfaces bindings, defined in video-interfaces.txt.
+
+Example:
+
+ i2c-isp@1313 {
+ s5k4e5@20 {
+ compatible = "samsung,s5k4e5";
+ reg = <0x20>;
+ reset-gpios = <&gpx1 2 1>;
+ clock-frequency = <2400>;
+ clocks = <&clock 129>;
+ clock-names = "extclk"
+ svdda-supply = <...>;
+ svddio-supply = <...>;
+ port {
+ is_s5k4e5_ep: endpoint {
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <&csis0_ep>;
+ };
+ };
+ };
+ };
--
1.7.9.5
--
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
[PATCH v11 01/12] [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation
The patch adds the DT binding documentation for Samsung
Exynos5 SoC series imaging subsystem (FIMC-IS).
Signed-off-by: Arun Kumar K
Reviewed-by: Sylwester Nawrocki
Acked-by: Mark Rutland
---
.../devicetree/bindings/media/exynos5-fimc-is.txt | 113
1 file changed, 113 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
new file mode 100644
index 000..658d4a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
@@ -0,0 +1,113 @@
+Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS)
+--
+
+The camera subsystem on Samsung Exynos5 SoC has some changes relative
+to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and
+FIMC-LITE IPs but has a much improved version of FIMC-IS which can
+handle sensor controls and camera post-processing operations. The
+Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many
+post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two
+dedicated scalers (SCC and SCP).
+
+fimc-is node
+
+
+Required properties:
+
+- compatible: should contain "samsung,exynos5250-fimc-is"
+- reg : physical base address and size of the memory mapped
+ registers
+- interrupts: interrupt-specifier for the fimc-is interrupt
+- clocks: list of clock specifiers, corresponding to entries in
+ clock-names property
+- clock-names : must contain "isp", "mcu_isp", "isp_div0", "isp_div1",
+ "isp_divmpwm", "mcu_isp_div0", "mcu_isp_div1" entries,
+ matching entries in the clocks property
+- samsung,pmu : phandle to the Power Management Unit (PMU) node
+
+i2c-isp (ISP I2C bus controller) nodes
+--
+The i2c-isp nodes should be children of the fimc-is node.
+
+Required properties:
+
+- compatible : must contain "samsung,exynos4212-i2c-isp" for Exynos4212,
+ Exynos4412 and Exynos5250 SoCs
+- reg : physical base address and length of the registers set
+- clocks : should contain gate clock specifier for this controller
+- clock-names : should contain "i2c_isp" for the gate clock
+- pinctrl-0: phandle of the pinctrl node for the i2c isp
+- pinctrl-names : must contain "default"
+
+ranges, #address-cells, and #size-cells should be present as appropriate.
+
+Device tree nodes of the image sensors controlled directly by the FIMC-IS
+firmware must be child nodes of their corresponding ISP I2C bus controller
node.
+The data link of these image sensors must be specified using the common video
+interfaces bindings, defined in video-interfaces.txt.
+
+Example:
+
+ fimc_is: fimc-is@1300 {
+ compatible = "samsung,exynos5250-fimc-is";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+ reg = <0x1300 0x20>;
+ interrupt-parent = <&combiner>;
+ interrupts = <19 1>;
+ clocks = <&clock 346>, <&clock 347>, <&clock 512>,
+ <&clock 513>, <&clock 514>, <&clock 515>,
+ <&clock 516>;
+ clock-names = "isp", "mcu_isp", "isp_div0", "isp_div1",
+ "isp_divmpwm", "mcu_isp_div0",
+ "mcu_isp_div1";
+ samsung,pmu = <&pmu>;
+
+ i2c0_isp: i2c-isp@1313 {
+ compatible = "samsung,exynos4212-i2c-isp";
+ reg = <0x1313 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&clock 352>;
+ clock-names = "i2c_isp";
+ pinctrl-0 = <&cam_i2c0_bus>;
+ pinctrl-names = "default";
+ };
+
+ i2c1_isp: i2c-isp@1314 {
+ compatible = "samsung,exynos4212-i2c-isp";
+ reg = <0x1314 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&clock 353>;
+ clock-names = "i2c_isp";
+ pinctrl-0 = <&cam_i2c1_bus>;
+ pinctrl-names = "default";
+ };
+ };
+
+In the board specific file the sensor nodes can be provided. For the sensor
+node documentation for s5k4e5, please refer to samsung-s5k4e5.txt
+
+ fimc-is@1300 {
+ status = "okay";
+
+ i2c-isp@1313 {
+ s5k4e5@20 {
+ compatible = "samsung,s5k4e5";
+ reg = <0x20>;
+
[PATCH v11 08/12] [media] exynos5-fimc-is: Add the hardware pipeline control
This patch adds the crucial hardware pipeline control for the
fimc-is driver. All the subdev nodes will call this pipeline
interfaces to reach the hardware. Responsibilities of this module
involves configuring and maintaining the hardware pipeline involving
multiple sub-ips like ISP, DRC, Scalers, ODC, 3DNR, FD etc.
Signed-off-by: Arun Kumar K
Signed-off-by: Kilyeon Im
Reviewed-by: Sylwester Nawrocki
---
.../media/platform/exynos5-is/fimc-is-pipeline.c | 1699
.../media/platform/exynos5-is/fimc-is-pipeline.h | 129 ++
2 files changed, 1828 insertions(+)
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-pipeline.c
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-pipeline.h
diff --git a/drivers/media/platform/exynos5-is/fimc-is-pipeline.c
b/drivers/media/platform/exynos5-is/fimc-is-pipeline.c
new file mode 100644
index 000..25eaf24
--- /dev/null
+++ b/drivers/media/platform/exynos5-is/fimc-is-pipeline.c
@@ -0,0 +1,1699 @@
+/*
+ * Samsung EXYNOS5 FIMC-IS (Imaging Subsystem) driver
+*
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Arun Kumar K
+ * Kil-yeon Lim
+ *
+ * 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 "fimc-is.h"
+#include "fimc-is-pipeline.h"
+#include "fimc-is-metadata.h"
+#include "fimc-is-regs.h"
+#include "fimc-is-cmd.h"
+#include
+#include
+
+/* Default setting values */
+#define DEFAULT_PREVIEW_STILL_WIDTH1280
+#define DEFAULT_PREVIEW_STILL_HEIGHT 720
+#define DEFAULT_CAPTURE_VIDEO_WIDTH1920
+#define DEFAULT_CAPTURE_VIDEO_HEIGHT 1080
+#define DEFAULT_CAPTURE_STILL_WIDTH2560
+#define DEFAULT_CAPTURE_STILL_HEIGHT 1920
+#define DEFAULT_CAPTURE_STILL_CROP_WIDTH 2560
+#define DEFAULT_CAPTURE_STILL_CROP_HEIGHT 1440
+#define DEFAULT_PREVIEW_VIDEO_WIDTH640
+#define DEFAULT_PREVIEW_VIDEO_HEIGHT 480
+
+/* Init params for pipeline devices */
+static const struct sensor_param init_sensor_param = {
+ .frame_rate = {
+ .frame_rate = 30,
+ },
+};
+
+static const struct isp_param init_isp_param = {
+ .control = {
+ .cmd = CONTROL_COMMAND_START,
+ .bypass = CONTROL_BYPASS_DISABLE,
+ },
+ .otf_input = {
+ .cmd = OTF_INPUT_COMMAND_DISABLE,
+ .width = DEFAULT_CAPTURE_STILL_WIDTH,
+ .height = DEFAULT_CAPTURE_STILL_HEIGHT,
+ .format = OTF_INPUT_FORMAT_BAYER,
+ .bitwidth = OTF_INPUT_BIT_WIDTH_10BIT,
+ .order = OTF_INPUT_ORDER_BAYER_GR_BG,
+ .frametime_max = 3,
+ },
+ .dma1_input = {
+ .cmd = DMA_INPUT_COMMAND_DISABLE,
+ },
+ .dma2_input = {
+ .cmd = DMA_INPUT_COMMAND_DISABLE,
+ },
+ .aa = {
+ .cmd = ISP_AA_COMMAND_START,
+ },
+ .flash = {
+ .cmd = ISP_FLASH_COMMAND_DISABLE,
+ .redeye = ISP_FLASH_REDEYE_DISABLE,
+ },
+ .awb = {
+ .cmd = ISP_AWB_COMMAND_AUTO,
+ },
+ .effect = {
+ .cmd = ISP_IMAGE_EFFECT_DISABLE,
+ },
+ .iso = {
+ .cmd = ISP_ISO_COMMAND_AUTO,
+ },
+ .adjust = {
+ .cmd = ISP_ADJUST_COMMAND_AUTO,
+ },
+ .metering = {
+ .cmd = ISP_METERING_COMMAND_CENTER,
+ .win_width = DEFAULT_CAPTURE_STILL_WIDTH,
+ .win_height = DEFAULT_CAPTURE_STILL_HEIGHT,
+ },
+ .afc = {
+ .cmd = ISP_AFC_COMMAND_AUTO,
+ },
+ .otf_output = {
+ .cmd = OTF_OUTPUT_COMMAND_ENABLE,
+ .width = DEFAULT_CAPTURE_STILL_WIDTH,
+ .height = DEFAULT_CAPTURE_STILL_HEIGHT,
+ .format = OTF_OUTPUT_FORMAT_YUV444,
+ .bitwidth = OTF_OUTPUT_BIT_WIDTH_12BIT,
+ .order = OTF_OUTPUT_ORDER_BAYER_GR_BG,
+ },
+ .dma1_output = {
+ .cmd = DMA_OUTPUT_COMMAND_DISABLE,
+ .width = DEFAULT_CAPTURE_STILL_WIDTH,
+ .height = DEFAULT_CAPTURE_STILL_HEIGHT,
+ .format = DMA_INPUT_FORMAT_YUV444,
+ .bitwidth = DMA_INPUT_BIT_WIDTH_8BIT,
+ .plane = 1,
+ .order = DMA_INPUT_ORDER_YCBCR,
+ },
+ .dma2_output = {
+ .cmd = DMA_OUTPUT_COMMAND_DISABLE,
+ .width = DEFAULT_CAPTURE_STILL_WIDTH,
+ .height = DEFAULT_CAPTURE_STILL_HEIGHT,
+ .format = DMA_OUTPUT_FORMAT_BAYER,
+ .bitwidth = DMA_OUTPUT_BIT_WIDTH_12BIT,
+ .plane = 1,
+ .order = DMA_OUTPUT_ORDER_GB_BG,
+ .dma_out_mask = 0x,
+ },
+};
+
+static const struct drc_param init_drc_param = {
+ .control
[PATCH v11 04/12] [media] exynos5-fimc-is: Add register definition and context header
This patch adds the register definition file for the fimc-is driver and also the header file containing the main context for the driver. Signed-off-by: Arun Kumar K Signed-off-by: Kilyeon Im Reviewed-by: Sylwester Nawrocki --- drivers/media/platform/exynos5-is/fimc-is-regs.h | 105 ++ drivers/media/platform/exynos5-is/fimc-is.h | 160 ++ 2 files changed, 265 insertions(+) create mode 100644 drivers/media/platform/exynos5-is/fimc-is-regs.h create mode 100644 drivers/media/platform/exynos5-is/fimc-is.h diff --git a/drivers/media/platform/exynos5-is/fimc-is-regs.h b/drivers/media/platform/exynos5-is/fimc-is-regs.h new file mode 100644 index 000..06aa466 --- /dev/null +++ b/drivers/media/platform/exynos5-is/fimc-is-regs.h @@ -0,0 +1,105 @@ +/* + * Samsung Exynos5 SoC series FIMC-IS driver + * + * Copyright (c) 2013 Samsung Electronics Co., Ltd + * Arun Kumar K + * Kil-yeon Lim + * + * 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. + */ + +#ifndef FIMC_IS_REGS_H +#define FIMC_IS_REGS_H + +/* WDT_ISP register */ +#define WDT0x0017 +/* MCUCTL register */ +#define MCUCTL 0x0018 +/* MCU Controller Register */ +#define MCUCTLR(MCUCTL+0x00) +#define MCUCTLR_AXI_ISPX_AWCACHE(x)((x) << 16) +#define MCUCTLR_AXI_ISPX_ARCACHE(x)((x) << 12) +#define MCUCTLR_MSWRST (1 << 0) +/* Boot Base OFfset Address Register */ +#define BBOAR (MCUCTL+0x04) +#define BBOAR_BBOA(x) ((x) << 0) + +/* Interrupt Generation Register 0 from Host CPU to VIC */ +#define INTGR0 (MCUCTL+0x08) +#define INTGR0_INTGC(n)(1 << ((n) + 16)) +#define INTGR0_INTGD(n)(1 << (n)) + +/* Interrupt Clear Register 0 from Host CPU to VIC */ +#define INTCR0 (MCUCTL+0x0c) +#define INTCR0_INTCC(n)(1 << ((n) + 16)) +#define INTCR0_INTCD(n)(1 << (n)) + +/* Interrupt Mask Register 0 from Host CPU to VIC */ +#define INTMR0 (MCUCTL+0x10) +#define INTMR0_INTMC(n)(1 << ((n) + 16)) +#define INTMR0_INTMD(n)(1 << (n)) + +/* Interrupt Status Register 0 from Host CPU to VIC */ +#define INTSR0 (MCUCTL+0x14) +#define INTSR0_GET_INTSD(n, x) (((x) >> (n)) & 0x1) +#define INTSR0_GET_INTSC(n, x) (((x) >> ((n) + 16)) & 0x1) + +/* Interrupt Mask Status Register 0 from Host CPU to VIC */ +#define INTMSR0(MCUCTL+0x18) +#define INTMSR0_GET_INTMSD(n, x) (((x) >> (n)) & 0x1) +#define INTMSR0_GET_INTMSC(n, x) (((x) >> ((n) + 16)) & 0x1) + +/* Interrupt Generation Register 1 from ISP CPU to Host IC */ +#define INTGR1 (MCUCTL+0x1c) +#define INTGR1_INTGC(n)(1 << (n)) + +/* Interrupt Clear Register 1 from ISP CPU to Host IC */ +#define INTCR1 (MCUCTL+0x20) +#define INTCR1_INTCC(n)(1 << (n)) + +/* Interrupt Mask Register 1 from ISP CPU to Host IC */ +#define INTMR1 (MCUCTL+0x24) +#define INTMR1_INTMC(n)(1 << (n)) + +/* Interrupt Status Register 1 from ISP CPU to Host IC */ +#define INTSR1 (MCUCTL+0x28) +/* Interrupt Mask Status Register 1 from ISP CPU to Host IC */ +#define INTMSR1(MCUCTL+0x2c) +/* Interrupt Clear Register 2 from ISP BLK's interrupts to Host IC */ +#define INTCR2 (MCUCTL+0x30) +#define INTCR2_INTCC(n)(1 << (n)) + +/* Interrupt Mask Register 2 from ISP BLK's interrupts to Host IC */ +#define INTMR2 (MCUCTL+0x34) +#define INTMR2_INTMCIS(n) (1 << (n)) + +/* Interrupt Status Register 2 from ISP BLK's interrupts to Host IC */ +#define INTSR2 (MCUCTL+0x38) +/* Interrupt Mask Status Register 2 from ISP BLK's interrupts to Host IC */ +#define INTMSR2(MCUCTL+0x3c) +/* General Purpose Output Control Register (0~17) */ +#define GPOCTLR(MCUCTL+0x40) +#define GPOCTLR_GPOG(n, x) ((x) << (n)) + +/* General Purpose Pad Output Enable Register (0~17) */ +#define GPOENCTLR (MCUCTL+0x44) +#define GPOENCTLR_GPOEN0(n, x) ((x) << (n)) + +/* General Purpose Input Control Register (0~17) */ +#define GPICTLR(MCUCTL+0x48) + +/* IS Shared Registers between ISP CPU and HOST CPU */ +#define ISSR(n)(MCUCTL + 0x80 + (n)) + +/* PMU for FIMC-IS*/ +#define PMUREG_CMU_RESET_ISP_SYS_PWR_REG 0x1584 +#define PMUREG_ISP_ARM_CONFIGURA
[PATCH v11 00/12] Exynos5 IS driver
The patch series adds support for exynos5 fimc-is driver and a new sensor s5k4e5. The media driver part is omitted form this series as it is already applied. Changes from v10 --- - Addressed DT binding review comments from Mark Rutland https://www.mail-archive.com/linux-media@vger.kernel.org/msg67806.html https://www.mail-archive.com/linux-media@vger.kernel.org/msg67808.html Changes from v9 --- - Addressed review comments from Hans and Sylwester http://www.mail-archive.com/linux-media@vger.kernel.org/msg67102.html http://www.mail-archive.com/linux-media@vger.kernel.org/msg67624.html http://www.mail-archive.com/linux-media@vger.kernel.org/msg67623.html - Skipped already applied media driver Changes from v8 --- - Moved i2c-isp device nodes into the fimc-is node as suggested by Sylwester - Addressed comments given by Sylwester and Philipp Zabel Changes from v7 --- - Addressed few DT related review comments from Sylwester http://www.mail-archive.com/linux-media@vger.kernel.org/msg66403.html - Few fixes added after some regression testing Changes from v6 --- - Addressed DT binding doc review comments from Sylwester http://www.mail-archive.com/linux-media@vger.kernel.org/msg65771.html http://www.mail-archive.com/linux-media@vger.kernel.org/msg65772.html Changes from v5 --- - Addressed review comments from Sylwester http://www.mail-archive.com/linux-media@vger.kernel.org/msg65578.html http://www.mail-archive.com/linux-media@vger.kernel.org/msg65605.html Changes from v4 --- - Addressed all review comments from Sylwester - Added separate PMU node as suggested by Stephen Warren - Added phandle based discovery of subdevs instead of node name Changes from v3 --- - Dropped the RFC tag - Addressed all review comments from Sylwester and Sachin - Removed clock provider for media dev - Added s5k4e5 sensor devicetree binding doc Changes from v2 --- - Added exynos5 media device driver from Shaik to this series - Added ISP pipeline support in media device driver - Based on Sylwester's latest exynos4-is development - Asynchronos registration of sensor subdevs - Made independent IS-sensor support - Add s5k4e5 sensor driver - Addressed review comments from Sylwester, Hans, Andrzej, Sachin Changes from v1 --- - Addressed all review comments from Sylwester - Made sensor subdevs as independent i2c devices - Lots of cleanup - Debugfs support added - Removed PMU global register access Arun Kumar K (12): [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation [media] exynos5-fimc-is: Add driver core files [media] exynos5-fimc-is: Add common driver header files [media] exynos5-fimc-is: Add register definition and context header [media] exynos5-fimc-is: Add isp subdev [media] exynos5-fimc-is: Add scaler subdev [media] exynos5-fimc-is: Add sensor interface [media] exynos5-fimc-is: Add the hardware pipeline control [media] exynos5-fimc-is: Add the hardware interface module [media] exynos5-is: Add Kconfig and Makefile V4L: Add DT binding doc for s5k4e5 image sensor V4L: Add s5k4e5 sensor driver .../devicetree/bindings/media/exynos5-fimc-is.txt | 113 ++ .../devicetree/bindings/media/samsung-s5k4e5.txt | 45 + drivers/media/i2c/Kconfig |8 + drivers/media/i2c/Makefile |1 + drivers/media/i2c/s5k4e5.c | 344 drivers/media/platform/Kconfig |1 + drivers/media/platform/Makefile|1 + drivers/media/platform/exynos5-is/Kconfig | 20 + drivers/media/platform/exynos5-is/Makefile |7 + drivers/media/platform/exynos5-is/fimc-is-cmd.h| 187 +++ drivers/media/platform/exynos5-is/fimc-is-core.c | 410 + drivers/media/platform/exynos5-is/fimc-is-core.h | 132 ++ drivers/media/platform/exynos5-is/fimc-is-err.h| 257 +++ .../media/platform/exynos5-is/fimc-is-interface.c | 810 ++ .../media/platform/exynos5-is/fimc-is-interface.h | 124 ++ drivers/media/platform/exynos5-is/fimc-is-isp.c| 534 ++ drivers/media/platform/exynos5-is/fimc-is-isp.h| 90 ++ .../media/platform/exynos5-is/fimc-is-metadata.h | 767 + drivers/media/platform/exynos5-is/fimc-is-param.h | 1159 + .../media/platform/exynos5-is/fimc-is-pipeline.c | 1699 .../media/platform/exynos5-is/fimc-is-pipeline.h | 129 ++ drivers/media/platform/exynos5-is/fimc-is-regs.h | 105 ++ drivers/media/platform/exynos5-is/fimc-is-scaler.c | 476 ++ drivers/media/platform/exynos5-is/fimc-is-scaler.h | 106 ++ drivers/media/platform/exynos5-is/fimc-is-sensor.c | 45 + drivers/media/platform/exynos5-is/fimc-is-sensor.h | 65 + drivers/media/platform/exynos5-is/fimc-is.h| 160 ++ 27 files changed, 7795 insertions(+) create mode 100644 Docum
[PATCH v11 02/12] [media] exynos5-fimc-is: Add driver core files
This driver is for the FIMC-IS IP available in Samsung Exynos5
SoC onwards. This patch adds the core files for the new driver.
Signed-off-by: Arun Kumar K
Signed-off-by: Kilyeon Im
Reviewed-by: Sylwester Nawrocki
---
drivers/media/platform/exynos5-is/fimc-is-core.c | 410 ++
drivers/media/platform/exynos5-is/fimc-is-core.h | 132 +++
2 files changed, 542 insertions(+)
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-core.c
create mode 100644 drivers/media/platform/exynos5-is/fimc-is-core.h
diff --git a/drivers/media/platform/exynos5-is/fimc-is-core.c
b/drivers/media/platform/exynos5-is/fimc-is-core.c
new file mode 100644
index 000..2b116d0
--- /dev/null
+++ b/drivers/media/platform/exynos5-is/fimc-is-core.c
@@ -0,0 +1,410 @@
+/*
+ * Samsung EXYNOS5 FIMC-IS (Imaging Subsystem) driver
+*
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Arun Kumar K
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "fimc-is.h"
+#include "fimc-is-i2c.h"
+
+#define CLK_MCU_ISP_DIV0_FREQ (200 * 100)
+#define CLK_MCU_ISP_DIV1_FREQ (100 * 100)
+#define CLK_ISP_DIV0_FREQ (134 * 100)
+#define CLK_ISP_DIV1_FREQ (68 * 100)
+#define CLK_ISP_DIVMPWM_FREQ (34 * 100)
+
+static const char * const fimc_is_clock_name[] = {
+ [IS_CLK_ISP]= "isp",
+ [IS_CLK_MCU_ISP]= "mcu_isp",
+ [IS_CLK_ISP_DIV0] = "isp_div0",
+ [IS_CLK_ISP_DIV1] = "isp_div1",
+ [IS_CLK_ISP_DIVMPWM]= "isp_divmpwm",
+ [IS_CLK_MCU_ISP_DIV0] = "mcu_isp_div0",
+ [IS_CLK_MCU_ISP_DIV1] = "mcu_isp_div1",
+};
+
+static void fimc_is_put_clocks(struct fimc_is *is)
+{
+ int i;
+
+ for (i = 0; i < IS_CLK_MAX_NUM; i++) {
+ if (IS_ERR(is->clock[i]))
+ continue;
+ clk_unprepare(is->clock[i]);
+ clk_put(is->clock[i]);
+ is->clock[i] = ERR_PTR(-EINVAL);
+ }
+}
+
+static int fimc_is_get_clocks(struct fimc_is *is)
+{
+ struct device *dev = &is->pdev->dev;
+ int i, ret;
+
+ for (i = 0; i < IS_CLK_MAX_NUM; i++) {
+ is->clock[i] = clk_get(dev, fimc_is_clock_name[i]);
+ if (IS_ERR(is->clock[i]))
+ goto err;
+ ret = clk_prepare(is->clock[i]);
+ if (ret < 0) {
+ clk_put(is->clock[i]);
+ is->clock[i] = ERR_PTR(-EINVAL);
+ goto err;
+ }
+ }
+ return 0;
+err:
+ fimc_is_put_clocks(is);
+ pr_err("Failed to get clock: %s\n", fimc_is_clock_name[i]);
+ return -ENXIO;
+}
+
+static int fimc_is_configure_clocks(struct fimc_is *is)
+{
+ int i, ret;
+
+ for (i = 0; i < IS_CLK_MAX_NUM; i++)
+ is->clock[i] = ERR_PTR(-EINVAL);
+
+ ret = fimc_is_get_clocks(is);
+ if (ret)
+ return ret;
+
+ /* Set rates */
+ ret = clk_set_rate(is->clock[IS_CLK_MCU_ISP_DIV0],
+ CLK_MCU_ISP_DIV0_FREQ);
+ if (ret)
+ return ret;
+ ret = clk_set_rate(is->clock[IS_CLK_MCU_ISP_DIV1],
+ CLK_MCU_ISP_DIV1_FREQ);
+ if (ret)
+ return ret;
+ ret = clk_set_rate(is->clock[IS_CLK_ISP_DIV0], CLK_ISP_DIV0_FREQ);
+ if (ret)
+ return ret;
+ ret = clk_set_rate(is->clock[IS_CLK_ISP_DIV1], CLK_ISP_DIV1_FREQ);
+ if (ret)
+ return ret;
+ ret = clk_set_rate(is->clock[IS_CLK_ISP_DIVMPWM],
+ CLK_ISP_DIVMPWM_FREQ);
+ return ret;
+}
+
+static void fimc_is_pipelines_destroy(struct fimc_is *is)
+{
+ int i;
+
+ for (i = 0; i < is->drvdata->num_instances; i++)
+ fimc_is_pipeline_destroy(&is->pipeline[i]);
+}
+
+static int fimc_is_parse_sensor_config(struct fimc_is *is, unsigned int index,
+ struct device_node *node)
+{
+ struct fimc_is_sensor *sensor = &is->sensor[index];
+ u32 tmp = 0;
+ int ret;
+
+ sensor->drvdata = exynos5_is_sensor_get_drvdata(node);
+ if (!sensor->drvdata) {
+ dev_err(&is->pdev->dev, "no driver data found for: %s\n",
+node->full_name);
+ return -EINVAL;
+ }
+
+ node = v4l2_of_get_next_endpoint(node, NULL);
+ if (!node)
+ return -EN
Re: [PATCH v10 01/12] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation
Hi Mark, Thank you for the review. Will address your comments. Regards Arun On Tue, Oct 29, 2013 at 5:02 AM, Mark Rutland wrote: > Hi, > > Apologies for the late reply. I have a few comments, but nothing major. > > On Fri, Oct 18, 2013 at 06:37:28AM +0100, Arun Kumar K wrote: >> The patch adds the DT binding documentation for Samsung >> Exynos5 SoC series imaging subsystem (FIMC-IS). >> >> Signed-off-by: Arun Kumar K >> Reviewed-by: Sylwester Nawrocki >> --- >> .../devicetree/bindings/media/exynos5-fimc-is.txt | 84 >> >> 1 file changed, 84 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/media/exynos5-fimc-is.txt >> >> diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt >> b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt >> new file mode 100644 >> index 000..0525417 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt >> @@ -0,0 +1,84 @@ >> +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS) >> +-- >> + >> +The camera subsystem on Samsung Exynos5 SoC has some changes relative >> +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and >> +FIMC-LITE IPs but has a much improved version of FIMC-IS which can >> +handle sensor controls and camera post-processing operations. The >> +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many >> +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two >> +dedicated scalers (SCC and SCP). >> + >> +fimc-is node >> + >> + >> +Required properties: >> + >> +- compatible: must be "samsung,exynos5250-fimc-is" > > s/must be/should contain/ > >> +- reg : physical base address and size of the memory mapped >> + registers >> +- interrupt-parent : parent interrupt controller > > I don't think this is actually required in all cases. It's a standard property > that people can add if it happens to be required in a particular instance. > >> +- interrupts: fimc-is interrupt to the parent interrupt controller > > Is this the only interrupt the device generates? If so: > > - interrupts: interrupt-specifier for the fimc-is interrupt. > >> +- clocks: list of clock specifiers, corresponding to entries in >> + clock-names property >> +- clock-names : must contain "isp", "mcu_isp", "isp_div0", "isp_div1", >> + "isp_divmpwm", "mcu_isp_div0", "mcu_isp_div1" entries, >> + matching entries in the clocks property >> +- samsung,pmu : phandle to the Power Management Unit (PMU) node >> + >> +i2c-isp (ISP I2C bus controller) nodes >> +-- >> +The i2c-isp node is defined as the child node of fimc-is. > > There are multiple of these, so how about the following instead: > > The i2c-isp nodes should be children of the fimc-is node. > > It might be worth pointing out that ranges, #address-cells, and #size-cells > should be present as appropriate. > >> + >> +Required properties: >> + >> +- compatible : should be "samsung,exynos4212-i2c-isp" for Exynos4212, >> + Exynos4412 and Exynos5250 SoCs > > Similarly, s/should be/must contain/ > >> +- reg: physical base address and length of the registers set >> +- clocks : must contain gate clock specifier for this controller >> +- clock-names: must contain "i2c_isp" entry > > I'd prefer clocks to be described as for the simc-is, with clock names being > something like: > > - clock-names: should contain "i2c_isp" for the gate clock. > >> + >> +For the i2c-isp node, it is required to specify a pinctrl state named >> "default", >> +according to the pinctrl bindings defined in >> ../pinctrl/pinctrl-bindings.txt. > > I'd prefer a mention of pinctrl-0 and pinctrl-names, as that's what most other > bindings do. > > Also, as this is described as required it should be in the example. > >> + >> +Device tree nodes of the image sensors controlled directly by the FIMC-IS >> +firmware must be child nodes of their corresponding ISP I2C bus controller >> node. >> +The data link of these image sensors must be specified using the common >> video >> +interfaces bindings, defined in video-interfaces.txt. > > These don't seem to be in the example and probably should be. > > Otherwise, this looks fine. With those points fixed up: > > Acked-by: Mark Rutland > > Thanks, > Mark. -- 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: [PATCH -next] [media] v4l: ti-vpe: fix return value check in vpe_probe()
On Wednesday 30 October 2013 08:45 AM, Wei Yongjun wrote:
From: Wei Yongjun
In case of error, the function devm_kzalloc() and devm_ioremap()
returns NULL pointer not ERR_PTR(). The IS_ERR() test in the return
value check should be replaced with NULL test.
Reviewed-by: Archit Taneja
Thanks,
Archit
Signed-off-by: Wei Yongjun
---
drivers/media/platform/ti-vpe/vpe.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c
b/drivers/media/platform/ti-vpe/vpe.c
index 4e58069..90cf369 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1942,8 +1942,8 @@ static int vpe_probe(struct platform_device *pdev)
int ret, irq, func;
dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
- if (IS_ERR(dev))
- return PTR_ERR(dev);
+ if (!dev)
+ return -ENOMEM;
spin_lock_init(&dev->lock);
@@ -1962,8 +1962,8 @@ static int vpe_probe(struct platform_device *pdev)
* registers based on the sub block base addresses
*/
dev->base = devm_ioremap(&pdev->dev, res->start, SZ_32K);
- if (IS_ERR(dev->base)) {
- ret = PTR_ERR(dev->base);
+ if (!dev->base) {
+ ret = -ENOMEM;
goto v4l2_dev_unreg;
}
--
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: [PATCH -next] [media] v4l: ti-vpe: fix return value check in vpe_probe()
On Wednesday 30 October 2013 08:45 AM, Wei Yongjun wrote:
From: Wei Yongjun
In case of error, the function devm_kzalloc() and devm_ioremap()
returns NULL pointer not ERR_PTR(). The IS_ERR() test in the return
value check should be replaced with NULL test.
Reviewed-by: Archit Taneja
Thanks,
Archit
Signed-off-by: Wei Yongjun
---
drivers/media/platform/ti-vpe/vpe.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c
b/drivers/media/platform/ti-vpe/vpe.c
index 4e58069..90cf369 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1942,8 +1942,8 @@ static int vpe_probe(struct platform_device *pdev)
int ret, irq, func;
dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
- if (IS_ERR(dev))
- return PTR_ERR(dev);
+ if (!dev)
+ return -ENOMEM;
spin_lock_init(&dev->lock);
@@ -1962,8 +1962,8 @@ static int vpe_probe(struct platform_device *pdev)
* registers based on the sub block base addresses
*/
dev->base = devm_ioremap(&pdev->dev, res->start, SZ_32K);
- if (IS_ERR(dev->base)) {
- ret = PTR_ERR(dev->base);
+ if (!dev->base) {
+ ret = -ENOMEM;
goto v4l2_dev_unreg;
}
--
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: [PATCH -next] [media] v4l: ti-vpe: fix error return code in vpe_probe()
On Wednesday 30 October 2013 08:40 AM, Wei Yongjun wrote:
From: Wei Yongjun
Fix to return a negative error code from the error handling
case instead of 0, as done elsewhere in this function.
Reviewed-by: Archit Taneja
Thanks,
Archit
Signed-off-by: Wei Yongjun
---
drivers/media/platform/ti-vpe/vpe.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c
b/drivers/media/platform/ti-vpe/vpe.c
index 4e58069..0dbfd52 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -2007,8 +2007,10 @@ static int vpe_probe(struct platform_device *pdev)
vpe_top_vpdma_reset(dev);
dev->vpdma = vpdma_create(pdev);
- if (IS_ERR(dev->vpdma))
+ if (IS_ERR(dev->vpdma)) {
+ ret = PTR_ERR(dev->vpdma);
goto runtime_put;
+ }
vfd = &dev->vfd;
*vfd = vpe_videodev;
--
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: [GIT PULL FOR 3.13] Exynos5 SoC FIMC-IS imaging subsystem driver
Hi Sylwester, Sorry for the delayed response as I was on leave. I will address the comments from Mark today itself and post those DT binding patches. Regards Arun On Wed, Oct 30, 2013 at 5:48 AM, Sylwester Nawrocki wrote: > On 10/29/2013 01:54 PM, Mauro Carvalho Chehab wrote: > [...] > >> Yeah, it seems that we've waited for a long time to get an ack there. >> >> So, let's do this: >> >> Please send a new version with Mark's comments. Also, please split Doc >> changes from the code changes on the new series. I'll wait for a couple >> days for DT people to review it. If we don't have any reply, I'll review >> and apply it for Kernel 3.13, if I don't see anything really weird on it. > > > Ok, I will make sure all DT binding documentation is in separate patches, > actually only one patch needs to be reworked. > > Since Mark already reviewed the FIMC-IS and the S5K4E5 image sensor DT > binding patches the only one which may need further review is this one: > https://patchwork.linuxtv.org/patch/20237 > > Arun, could you send us the updated series ? Unfortunately I might not be > able to find time to address those comments myself until Friday. > > -- > 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 -- 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
cron job: media_tree daily build: WARNINGS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Tue Nov 5 04:00:23 CET 2013 git branch: for-v3.13c git hash: 3adeac2c34cc28e05d0ec52f38f009dcce278555 gcc version:i686-linux-gcc (GCC) 4.8.1 sparse version: 0.4.5-rc1 host hardware: x86_64 host os:3.11-6.slh.2-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-exynos: OK linux-git-arm-mx: OK linux-git-arm-omap: OK linux-git-arm-omap1: OK linux-git-arm-pxa: OK linux-git-blackfin: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.31.14-i686: OK linux-2.6.32.27-i686: OK linux-2.6.33.7-i686: OK linux-2.6.34.7-i686: OK linux-2.6.35.9-i686: OK linux-2.6.36.4-i686: OK linux-2.6.37.6-i686: OK linux-2.6.38.8-i686: OK linux-2.6.39.4-i686: OK linux-3.0.60-i686: OK linux-3.1.10-i686: OK linux-3.2.37-i686: OK linux-3.3.8-i686: OK linux-3.4.27-i686: OK linux-3.5.7-i686: OK linux-3.6.11-i686: OK linux-3.7.4-i686: OK linux-3.8-i686: OK linux-3.9.2-i686: OK linux-3.10.1-i686: OK linux-3.11.1-i686: OK linux-3.12-rc1-i686: OK linux-2.6.31.14-x86_64: OK linux-2.6.32.27-x86_64: OK linux-2.6.33.7-x86_64: OK linux-2.6.34.7-x86_64: OK linux-2.6.35.9-x86_64: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-x86_64: OK linux-3.0.60-x86_64: OK linux-3.1.10-x86_64: OK linux-3.2.37-x86_64: OK linux-3.3.8-x86_64: OK linux-3.4.27-x86_64: OK linux-3.5.7-x86_64: OK linux-3.6.11-x86_64: OK linux-3.7.4-x86_64: OK linux-3.8-x86_64: OK linux-3.9.2-x86_64: OK linux-3.10.1-x86_64: OK linux-3.11.1-x86_64: OK linux-3.12-rc1-x86_64: OK apps: WARNINGS spec-git: OK sparse version: 0.4.5-rc1 sparse: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- 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: [PATCH 2/6] v4l: omap4iss: Add support for OMAP4 camera interface - Video devices
Hi Hans,
On Monday 04 November 2013 09:34:49 Hans Verkuil wrote:
> On 11/04/2013 12:28 AM, Laurent Pinchart wrote:
> > On Thursday 03 October 2013 08:54:19 Hans Verkuil wrote:
> >> On 10/03/2013 01:55 AM, Laurent Pinchart wrote:
> >>> From: Sergio Aguirre
> >>>
> >>> This adds a very simplistic driver to utilize the CSI2A interface inside
> >>> the ISS subsystem in OMAP4, and dump the data to memory.
> >>>
> >>> Check Documentation/video4linux/omap4_camera.txt for details.
> >>>
> >>> This commit adds video devices support.
> >>>
> >>> Signed-off-by: Sergio Aguirre
> >>>
> >>> [Port the driver to v3.12-rc3, including the following changes
> >>> - Don't include plat/ headers
> >>> - Don't use cpu_is_omap44xx() macro
> >>> - Don't depend on EXPERIMENTAL
> >>> - Fix s_crop operation prototype
> >>> - Update link_notify prototype
> >>> - Rename media_entity_remote_source to media_entity_remote_pad]
> >>>
> >>> Signed-off-by: Laurent Pinchart
> >>> ---
> >>>
> >>> drivers/staging/media/omap4iss/iss_video.c | 1129
> >>> +++
> >>> drivers/staging/media/omap4iss/iss_video.h | 201 +
> >>> 2 files changed, 1330 insertions(+)
> >>> create mode 100644 drivers/staging/media/omap4iss/iss_video.c
> >>> create mode 100644 drivers/staging/media/omap4iss/iss_video.h
> >>>
> >>> diff --git a/drivers/staging/media/omap4iss/iss_video.c
> >>> b/drivers/staging/media/omap4iss/iss_video.c new file mode 100644
> >>> index 000..31f1b88
> >>> --- /dev/null
> >>> +++ b/drivers/staging/media/omap4iss/iss_video.c
> >>
> >>
> >>
> >>> +/*
> >>> -
> >>> + * V4L2 ioctls
> >>> + */
> >>> +
> >>> +static int
> >>> +iss_video_querycap(struct file *file, void *fh, struct v4l2_capability
> >>> *cap) +{
> >>> + struct iss_video *video = video_drvdata(file);
> >>> +
> >>> + strlcpy(cap->driver, ISS_VIDEO_DRIVER_NAME, sizeof(cap->driver));
> >>> + strlcpy(cap->card, video->video.name, sizeof(cap->card));
> >>> + strlcpy(cap->bus_info, "media", sizeof(cap->bus_info));
> >>> +
> >>> + if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >>> + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> >>> + else
> >>> + cap->capabilities = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
> >>
> >> Set device_caps instead of capabilities and add:
> >>cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> >
> > Actually cap->capabilities should be V4L2_CAP_DEVICE_CAPS |
> > V4L2_CAP_STREAMING>
> > | V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT. I'll fix that.
>
> You're right.
>
> >>> +
> >>> + return 0;
> >>> +}
> >
> > [snip]
> >
> >>> +static int
> >>> +iss_video_try_format(struct file *file, void *fh, struct v4l2_format
> >>> *format) +{
> >>> + struct iss_video *video = video_drvdata(file);
> >>> + struct v4l2_subdev_format fmt;
> >>> + struct v4l2_subdev *subdev;
> >>> + u32 pad;
> >>> + int ret;
> >>> +
> >>> + if (format->type != video->type)
> >>> + return -EINVAL;
> >>> +
> >>> + subdev = iss_video_remote_subdev(video, &pad);
> >>> + if (subdev == NULL)
> >>> + return -EINVAL;
> >>> +
> >>> + iss_video_pix_to_mbus(&format->fmt.pix, &fmt.format);
> >>> +
> >>> + fmt.pad = pad;
> >>> + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >>> + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> >>> + if (ret)
> >>> + return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> >>
> >> Return ENOTTY instead of EINVAL. Even better, use v4l2_subdev_has_op() +
> >> v4l2_disable_ioctl() to just disable ioctls based on the available subdev
> >> ops in probe().
> >
> > The remote subdev is required to implement the get_fmt() operation,
> > otherwise the ISS driver can't work at all. What about adding a
> > v4l2_subdev_has_op() check at probe time and removing the check here ?
>
> That would be best, yes.
It's actually even simpler than that. The remote subdev is an internal OMAP4
ISS subdevice that is guaranteed to implement get_fmt. There's no need to
check for ENOIOCTLCMD here, and no need to verify that the subdev implements
get_fmt at probe time.
> >>> +
> >>> + iss_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
> >>> + return 0;
> >>> +}
> >
> > [snip]
> >
> >>> +static int
> >>> +iss_video_enum_input(struct file *file, void *fh, struct v4l2_input
> >>> *input)
> >>> +{
> >>> + if (input->index > 0)
> >>> + return -EINVAL;
> >>> +
> >>> + strlcpy(input->name, "camera", sizeof(input->name));
> >>> + input->type = V4L2_INPUT_TYPE_CAMERA;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int
> >>> +iss_video_g_input(struct file *file, void *fh, unsigned int *input)
> >>> +{
> >>> + *input = 0;
> >>> +
> >>> + return 0;
> >>> +}
> >>
> >> Also add s_input.
> >
> > Shouldn't I remove enum_input and g_input instead ?
>
> No. G/S/ENUM_INPUT ioctls are compulsory for video capture devices.
>
> One thing I would like to do at some point in time is to add a fe
[PATCH] media: marvell-ccic: drop resource free in driver remove
From: Libin Yang
Date: Tue, 5 Nov 2013 10:18:15 +0800
Subject: [PATCH] marvell-ccic: drop resource free in driver remove
The mmp-driver is using devm_* to allocate the resource. The old
resource release methods are not appropriate here.
Signed-off-by: Libin Yang
---
drivers/media/platform/marvell-ccic/mmp-driver.c |7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 3458fa0..70cb57f 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -478,18 +478,11 @@ out_deinit_clk:
static int mmpcam_remove(struct mmp_camera *cam)
{
struct mcam_camera *mcam = &cam->mcam;
- struct mmp_camera_platform_data *pdata;
mmpcam_remove_device(cam);
mccic_shutdown(mcam);
mmpcam_power_down(mcam);
- pdata = cam->pdev->dev.platform_data;
- gpio_free(pdata->sensor_reset_gpio);
- gpio_free(pdata->sensor_power_gpio);
mcam_deinit_clk(mcam);
- iounmap(cam->power_regs);
- iounmap(mcam->regs);
- kfree(cam);
return 0;
}
--
1.7.9.5
--
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: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder
Hi Hans, On 11/04/2013 01:07 PM, Hans Verkuil wrote: Let me be precise as to what should happen, and you can check whether that's what is actually done in the fimc and g2d drivers. For V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: Say that the mem2mem hardware creates a 640x480 picture. If VIDIOC_S_CROP was called for V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE with a rectangle of 320x240@160x120, then the DMA engine will only transfer the center 320x240 rectangle to memory. This means that S_FMT needs to provide a buffer size large enough to accomodate a 320x240 image. So: VIDIOC_S_CROP(CAPTURE) == S_SELECTION(CAPTURE, V4L2_SEL_TGT_CROP). Unfortunately it's not how it currently works at these drivers. For VIDIOC_S_CROP called with V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE and a rectangle of 320x240@160x120 the hardware would scale and compose full 640x480 image onto 320x240 rectangle in the output memory buffer at position 160x120. IIRC the g2d device cannot scale so it would not allow to select DMA output rectangle smaller than 640x480. But looking at the code it doesn't do any crop parameters validation... So VIDIOC_S_CROP(CAPTURE) is actually being abused on m2m as S_SELECTION(CAPTURE, V4L2_SEL_TGT_COMPOSE). For V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: Say that the image in memory is a 640x480 picture. If VIDIOC_S_CROP was called for V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE with a rectangle of 320x240@160x120 then this would mean that the full 640x480 image is DMAed to the hardware, is scaled down to 320x240 and composed at position (160x120) in a canvas of at least 480x360. In other words, S_CROP behaves as composition for output devices: VIDIOC_S_CROP(OUTPUT) == S_SELECTION(OUTPUT, V4L2_SEL_TGT_COMPOSE). No, in case of these devices VIDIOC_S_CROP(OUTPUT) does what it actually means - the DMA would read only 320x240 rectangle at position 160x120. The last operation in particular is almost certainly not what you want for m2m devices. Instead, you want to select (crop) part of the image in memory and DMA that to the device. This is S_SELECTION(OUTPUT, V4L2_SEL_TGT_CROP) and cannot be translated to an S_CROP ioctl. Yeah, I didn't come yet across a video mem-to-mem hardware that would support steps as in your first description of crop on V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE. S_SELECTION(OUTPUT, V4L2_SEL_TGT_CROP) seems to have been redefined on mem-to-mem devices to do what it actually says. But it's not written anywhere in the spec yet, so I guess we could keep the crop ioctls in those drivers, in order to not break existing user space, and implement the selection API ioctls after documenting its semantics for mem-to-mem devices. What's more: in order to implement S_SELECTION(OUTPUT, V4L2_SEL_TGT_COMPOSE) you would need some way of setting the 'canvas' size of the m2m device, and there is no API today to do this (this was discussed during the v4l/dvb mini-summit). Regarding the capture side of an m2m device: it is not clear to me what these drivers implement: S_SELECTION(CAPTURE, V4L2_SEL_TGT_CROP) or S_SELECTION(CAPTURE, V4L2_SEL_TGT_COMPOSE). If it is the latter, then again S_CROP cannot be used and you have to use S_SELECTION. It's equivalent of S_SELECTION(CAPTURE, V4L2_SEL_TGT_COMPOSE). Note that the fimc and mfc drivers were written long before the selection API was introduced. Presumably the crop ioctls should just be deprecated (however handlers left for backward compatibility) in those drivers, once there is complete definition of the selections API for the m2m video devices. It's probably worth to avoid adding any translation layer of such behaviour (which doesn't match your definitions above) to the v4l2-core. -- 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
DVB Modulator API
Hi,
Dave Chapman writes:
> Given the recent patches by Maik Broemme adding support for a DVB-C
> modulator, I thought I would mention that I'm working on a driver for a
> DVB-T modulator and would like to open a discussion regarding how to
> integrate modulator support into the existing DVB API and kernel sub-system.
>
> The device I'm working with is a $169 (USD) USB DVB-T modulator based on
> the it9507 ASIC from ITE. ITE provide a GPL'd Linux driver, but this is
> 40K+ lines of code and is based on their generic cross-platform SDK
> supporting a range of devices.
>
> The device can be purchased here in various incarnations:
>
> http://www.idealez.com/hides/product-gallery/en_US/1-0/554
My driver is for this DVB-C modulator called Resi DVB-C:
http://www.digitaldevices.de/english/Resi_DVB-C_Modulator.html
It can modulate 10 DVB-C channels. They can have different modulations (QAM16
to QAM256) but have a fixed symbol rate (6.9 Msymbols) and have to
be next to each other in one 80MHz block (10 times 8 MHz).
> The hardware also has hardware PID filtering (allowing both whitelisting
> and blacklisting of data being transferred from the demod to the mod)
> and also the ability to insert SI packets into the modulated stream at
> user-definited intervals. Again, my driver doesn't support these
> features yet.
The Resi does not support PID filtering but some other features like
filling up the TS with filler packets if the output rate is larger than
the rate of the TS you want to send.
It can also correct the PCR according to the added TS packets.
> Regarding the API, I've looked at the ddbridge driver here:
>
> http://www.metzlerbros.de/dddvb/dddvb-0.9.10.tar.bz2
>
> and from what I can understand, this adds a new "mod0" device to access
> the DVB-C modulator. The API is as follows:
>
> struct dvb_mod_params {
> __u32 base_frequency;
> __u32 attenuator;
> };
>
> struct dvb_mod_channel_params {
> enum fe_modulation modulation;
>
> __u32 rate_increment;
>
> };
>
> #define DVB_MOD_SET _IOW('o', 208, struct dvb_mod_params)
> #define DVB_MOD_CHANNEL_SET _IOW('o', 209, struct
> dvb_mod_channel_params)
>
> I've no idea what "rate_increment" is.
It is for adjusting the the rate of filler packet insertion.
It shuld probably go into a separae ioctl.
I could use something like your DVBMOD_SET_PARAMETERS with differnt
parameters (those for DVB-C instead of DVB-T). I just would have to
reject most of them because only modulation can be set.
> Looking in the docs/modulator file, there also appears to be some
> ability to redirect data from a demod to a mod via
> /sys/class/ddbridge/ddbridge0/redirect
>
>
This is kind of a hack inside the ddbridge driver (the modulator shares the
DMA block with CI output on other ddbridge cards) which redirects
DMA from one card to another.
I am sure this can be solved more cleanly with the media controller stuff
but for now this has to suffice.
> The it9507 driver uses a software attenuation, and the range is
> dependent (but only very slightly - +/- a couple of dB at either end of
> the scale) on the frequency. So I've currently implemented a
> DVBMOD_GET_RF_GAIN_RANGE ioctl to get the available gain range for a
> specific frequency, but it wouldn't be a big loss to change the driver
> to just limit the gain to the subset that works on all frequencies,
> removing that call from the API.
Your DVBMOD_SET_RF_GAIN would be my attenuator setting above.
> I haven't had chance yet to seriously consider a generic modulator API,
> but with one modulator driver now being considered for inclusion I
> wanted to make those considering the API aware of my work.
>
> It seems clear however that neither my current API, nor that included
> with the ddbridge DVB-C modulator is generic enough at the moment.
Sure.
We will need one generic enough to support all delivery systems and also
special hardware features like rate adjustment, PCR correction, PID filtering,
packet insertion, etc.
There are also cards which do the modulation in software and accept I/Q pairs,
e.g.
the DekTec cards. But I only use those for testing demods for delivery systems
I cannot receive directly. We could add an I/Q mode for those kinds of cards.
The modulation part would have to go into user space.
Regards,
Ralph
--
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: [early RFC] Device Tree bindings for OMAP3 Camera Subsystem
013/11/3 Sebastian Reichel :
> Hi,
>
> This is an early RFC for omap3isp DT support. For now i just created a
> potential DT
> binding documentation based on the existing platform data:
>
> Binding for the OMAP3 Camera subsystem with the image signal processor (ISP)
> feature.
>
This is very interesting, I am in the process of transforming an (out
of tree) machine board file into
a device tree description, and I was precisely searching for "oma3isp
dt" when I saw your mail.
I would be happy to test or help develop any patch aiming at DT
support for omap3isp. I am new to DT, so I
will leave the DT bindings review to people that actually have a clue.
I am looking forward to testing patches and bugging you when things break ;)
Regards,
Jean-Philippe François
> omap3isp node
> -
>
> Required properties:
>
> - compatible: should be "ti,omap3isp" for OMAP3;
> - reg : physical addresses and length of the registers set;
> - clocks: list of clock specifiers, corresponding to entries in
> clock-names property;
> - clock-names : must contain "cam_ick", "cam_mclk", "csi2_96m_fck",
> "l3_ick" entries, matching entries in the clocks property;
> - interrupts: must contain mmu interrupt;
> - ti,iommu : phandle to isp mmu;
>
> Optional properties:
>
> - VDD_CSIPHY1-supply: regulator for csi phy1
> - VDD_CSIPHY2-supply: regulator for csi phy2
> - ti,isp-xclk-1 : device(s) attached to ISP's first external clock
> - ti,isp-xclk-2 : device(s) attached to ISP's second external clock
>
> device-group subnode
>
>
> Required properties:
> - ti,isp-interface-type : Integer describing the interface type, one of the
> following
>* 0 = ISP_INTERFACE_PARALLEL
>* 1 = ISP_INTERFACE_CSI2A_PHY2
>* 2 = ISP_INTERFACE_CCP2B_PHY1
>* 3 = ISP_INTERFACE_CCP2B_PHY2
>* 4 = ISP_INTERFACE_CSI2C_PHY1
> - ti,isp-devices: Array of phandles to devices connected via the
> interface
> - One of the following configuration nodes (depending on
> ti,isp-interface-type)
> - ti,ccp2-bus-cfg : CCP2 bus configuration (needed for
> ISP_INTERFACE_CCP*)
> - ti,parallel-bus-cfg : PARALLEL bus configuration (needed for
> ISP_INTERFACE_PARALLEL)
> - ti,csi2-bus-cfg : CSI bus configuration (needed for
> ISP_INTERFACE_CSI*)
>
> ccp2-bus-cfg subnode
>
>
> Required properties:
> - ti,video-port-clock-divisor : integer; used for video port output clock
> control
>
> Optional properties:
> - ti,inverted-clock : boolean; clock/strobe signal is inverted
> - ti,enable-crc : boolean; enable crc checking
> - ti,ccp2-mode-mipi : boolean; port is used in MIPI-CSI1 mode
> (default: CCP2 mode)
> - ti,phy-layer-is-strobe: boolean; use data/strobe physical layer
> (default: data/clock physical layer)
> - ti,data-lane-configuration: integer array with position and polarity
> information for lane 1 and 2
> - ti,clock-lane-configuration : integer array with position and polarity
> information for clock lane
>
> parallel-bus-cfg subnode
>
>
> Required properties:
> - ti,data-lane-shift: integer; shift data lanes
> by this amount
>
> Optional properties:
> - ti,clock-falling-edge : boolean; sample on falling
> edge (default: rising edge)
> - ti,horizontal-synchronization-active-low : boolean; default: active
> high
> - ti,vertical-synchronization-active-low: boolean; default: active
> high
> - ti,data-polarity-ones-complement : boolean; data polarity is
> one's complement
>
> csi2-bus-cfg subnode
>
>
> Required properties:
> - ti,video-port-clock-divisor : integer; used for video port output clock
> control
>
> Optional properties:
> - ti,data-lane-configuration: integer array with position and polarity
> information for lane 1 and 2
> - ti,clock-lane-configuration : integer array with position and polarity
> information for clock lane
> - ti,enable-crc : boolean; enable crc checking
>
> Example for Nokia N900
> --
>
> omap3isp: isp@480BC000 {
> compatible = "ti,omap3isp";
> reg = <
> /* OMAP3430+ */
> 0x480BC000 0x070/* base */
> 0x480BC100 0x078/* cbuf */
> 0x480BC400 0x1F0/* cpp2 */
> 0x480BC600 0x0A8/* ccdc */
> 0x480BCA00 0x048/* hist */
> 0x480BCC00 0x060/* h3a */
> 0x480BCE00 0x0A0/* prev */
> 0x480BD000 0x0AC/* resz */
> 0x480BD200 0x0FC/* sbl */
> 0x480BD400 0x070/* mmu */
> >;
>
> clocks = < &cam_ick &cam_mclk &csi2_96m_fck &l3_ick >;
> clock-names = "cam_ic
DVB Modulator API
Hi all,
Given the recent patches by Maik Broemme adding support for a DVB-C
modulator, I thought I would mention that I'm working on a driver for a
DVB-T modulator and would like to open a discussion regarding how to
integrate modulator support into the existing DVB API and kernel sub-system.
The device I'm working with is a $169 (USD) USB DVB-T modulator based on
the it9507 ASIC from ITE. ITE provide a GPL'd Linux driver, but this is
40K+ lines of code and is based on their generic cross-platform SDK
supporting a range of devices.
The device can be purchased here in various incarnations:
http://www.idealez.com/hides/product-gallery/en_US/1-0/554
and I have been working on github here:
https://github.com/linuxstb/it9507
The original ITE driver is in the it950x_linux_v13.06.27.1 directory
there, and my work-in-progress version is in it9507-driver. ITE have
also provided me with some documentation, which is in the docs directory.
Some versions of the modulator USB stick come with a demodulator (based
on the it9133), the output of which can be sent to the modulator
directly via a TS interface. The it9507 also has a USB interface.
For initial simplicity (and because I don't own such a device), my
driver only supports the modulator, but in discussing an API, it's
obviously useful to be aware of such devices.
The hardware also has hardware PID filtering (allowing both whitelisting
and blacklisting of data being transferred from the demod to the mod)
and also the ability to insert SI packets into the modulated stream at
user-definited intervals. Again, my driver doesn't support these
features yet.
Regarding the API, I've looked at the ddbridge driver here:
http://www.metzlerbros.de/dddvb/dddvb-0.9.10.tar.bz2
and from what I can understand, this adds a new "mod0" device to access
the DVB-C modulator. The API is as follows:
struct dvb_mod_params {
__u32 base_frequency;
__u32 attenuator;
};
struct dvb_mod_channel_params {
enum fe_modulation modulation;
__u32 rate_increment;
};
#define DVB_MOD_SET _IOW('o', 208, struct dvb_mod_params)
#define DVB_MOD_CHANNEL_SET _IOW('o', 209, struct
dvb_mod_channel_params)
I've no idea what "rate_increment" is.
Looking in the docs/modulator file, there also appears to be some
ability to redirect data from a demod to a mod via
/sys/class/ddbridge/ddbridge0/redirect
As a comparison, the current API for my driver can be seen here:
https://github.com/linuxstb/it9507/blob/master/it9507-driver/include/dvbmod.h
My driver is currently not integrated into the DVB subsystem, and as
such creates /dev/dvbmod%d device. This is used for the ioctls and also
for writing the TS to the modulator.
The it9507 driver uses a software attenuation, and the range is
dependent (but only very slightly - +/- a couple of dB at either end of
the scale) on the frequency. So I've currently implemented a
DVBMOD_GET_RF_GAIN_RANGE ioctl to get the available gain range for a
specific frequency, but it wouldn't be a big loss to change the driver
to just limit the gain to the subset that works on all frequencies,
removing that call from the API.
I haven't had chance yet to seriously consider a generic modulator API,
but with one modulator driver now being considered for inclusion I
wanted to make those considering the API aware of my work.
It seems clear however that neither my current API, nor that included
with the ddbridge DVB-C modulator is generic enough at the moment.
Regards,
Dave.
--
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: cx23885: Add basic analog radio support
Hi all
El 31/10/13 07:12, Mauro Carvalho Chehab escribió:
Em Fri, 04 Oct 2013 15:56:23 -0300
Alfredo Jesús Delaiti escreveu:
Hi all
El 14/01/12 15:25, Miroslav Slugeň escribió:
New version of patch, fixed video modes for DVR3200 tuners and working
audio mux.
I tested this patch (https://linuxtv.org/patch/9498/) with the latest
versions of git (September 28, 2013) with my TV card (Mygica X8507) and
it works.
I found some issue, although it may be through a bad implementation of mine.
Details of them:
1) Some warning when compiling
...
CC [M]
/home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.o
/home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1910:8:
: initialization from incompatible pointer type [enabled by default]
/home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1910:8:
warning: (near initialization for 'radio_ioctl_ops.vidioc_s_tuner')
[enabled by default]
/home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1911:8:
warning: initialization from incompatible pointer type [enabled by default]
/home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1911:8:
warning: (near initialization for 'radio_ioctl_ops.vidioc_s_audio')
[enabled by default]
CC [M] /home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-vbi.o
...
static const struct v4l2_ioctl_ops radio_ioctl_ops = {
.vidioc_s_tuner = radio_s_tuner, /* line 1910 */
.vidioc_s_audio = radio_s_audio, /* line 1911 */
2)
No reports signal strength or stereo signal with KRadio. XC5000 neither
reported (modprobe xc5000 debug=1). Maybe a feature XC5000.
To listen in stereo, sometimes, you have to turn on the Digital TV then
Analog TV and then radio.
3)
To listen Analog TV I need changed to NTSC standard and then PAL-Nc (the
norm in my country is PAL-Nc). If I leave the tune in NTSC no problem
with sound.
The patch (https://linuxtv.org/patch/9505/) corrects the latter, if used
tvtime with xawtv not always.
If I see-Digital TV (ISDB-T), then so as to listen the radio I have
first put the TV-Analog, because I hear very low and a strong white noise.
The latter is likely to be corrected by resetting the tuner, I have to
study it more.
I put below attached the patch applied to the plate: X8507.
Have you done any update of this patch?
Hi Alfredo,
My understanding is that the patch you've enclosed is incomplete and
depends on Miroslav's patch.
If so. My intention was to support the work of Miroslav, indicating that
it could be used in another plate, with the problems I encountered with
the implementation I did.
Unfortunately, Mirslav no longer working on it. See the last message
sent by him.
As he have you his ack to rework on it, could you please prepare a
patch series addressing the above comments for us to review?
Than
I can put back the patch from Miroslav, general support radio (but no
for any card particular) and adapted to the latest versions of the
drivers that are in git, and in a second patch, again, put the
particular patch Mygica X8507.
I did not put in the subject "Patch" because as I said, has some bugs.
Is it right to put in the subject "Patch" when is something incomplete?
Regards,
Alfredo
--
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: [media-workshop] [ANNOUNCE] Notes on the Media summit 2013-10-23
I have collected presentations from this media summit here: http://hverkuil.home.xs4all.nl/presentations/summit-2013/ Please contact me if a presentation you gave is missing so I can add it. Regards, Hans On 10/31/2013 12:27 PM, Mauro Carvalho Chehab wrote: > Notes on the Media summit 2013-10-23 > > List of Participants: > > Ricardo Ribalda - Qtechnology > Hans Verkuil - Cisco > Sakari Ailus - Intel > Mauro Carvalho Chehab - Samsung > Kieran Kunhya - Open Broadcast Systems > Sylwester Nawrocki - Samsung > Guennadi Liakhovetski > Benjamin Gaignard - ST Microelectronics > Hugues Fruchet - ST Microelectronics > Andrzej Hajda - Samsung > Peter Senna Tschudin - Coccinelle? > Hans de Goede - Red Hat > Michael Krufky - Samsung > Paul Walmsley - Nvidia > Laurent Pinchart - Ideas on board > > 1. Mauro Chehab: is the submaintainer arrangement working? > > General consensus is that it is working. > > Hans pointed that the commits ML is not working. Mauro will check what's > happening at LinuxTV website after returning back. > > Hans also pointed that patches for the rcX kernel aren't always picked > up quickly enough. Mauro says this is due to his move to Samsung which > took a lot of his time. This should improve. > > 2. Ricardo Ribalda Delgado: Multiple selections > > For a more flexible cropping selection a new extended rectangle struct has > to be added with possibly negative top and left offsets. This would allow to > explore capabilities of industry grade sensors, that can be configured to crop > multiple rectangles from the input area. > > Sylwester: an alternative approach would be to use indexed windows. The issue > of atomicity should also be addressed. > > Ricardo is proposing to add the capability of adding more than one selection > areas > > - No objections so far. > - Helper functions to detect if rectangles overlap or are contained on other > rectangles > are needed. > - Documentation changes for V4L2 subdev selection API. Special care needs to > be taken > for too few or too many rectangles. In both cases the rectangles field > should be > set to the actual number of allowed rectangles. In the too few case an > error should > be returned, in the too many case the extra rects should just be ignored. > > Mauro thinks that it will be useful to see if the width/height fields in > v4l2_rect > can be changes to u32 instead of s32: will require code review of drivers. > > 3. Hans Verkuil: colorspaces > > - Colorspace: limited/full range > > Draft presentation for all these topics: > http://hverkuil.home.xs4all.nl/presentations/summit2013.odp > > Proposal accepted, although the "_SRGB_LIM" name might need to be improved, > although > no alternatives were given. > > ALso mention in the docs that S_FMT can overwrite the colorspace for output > devices > if the selected one isn't supported. > > 4. Kieran Kunhya: SDI > > The biggest problem is separate audio and video file descriptors. Audio data > is > transfered in the video blanking areas, where should they be separated - in > the > kernel, using multiple planes, or in the user-space? One possibility is to > consider > this data as similar to an "uncompressed MPEG" datastream. It might be > possible to > use hardware-specific time-stamps to synchronise the data. > > Separate ALSA and V4L2 nodes are inherently problematic since they do not > keep the > audio and video together. Another possibility is to offer two APIs: for > professional > applications, where data is perfectly synchronised, and separate audio and > video > for "human consumption." > > A format for professional applications were VBI/HBI/Video is all captured in > one > big frame is OK (provided an open source lib is created to parse it). > > The proposal for using a multiplanar API where the audio is in a separate > plane > ran into opposition: the alsa devs should be asked whether it is possible to > return the audio data in such a way that it can be exactly associated with the > corresponding video buffer. This also requires that the audio can be variable > length since for NTSC the size of the audio data will alternate between > frames. > If this is possible, then the alsa API can be used and things can still remain > in sync. > > UPDATE: Mauro and Hans had some discussions with Takashi (ALSA Maintainer), > in order to have some shed about the possibilities. We're planning to discuss > more on this Friday's lunch. > > 5. Paul Walmsley on behalf of Bryan Wu: LED / flash API integration > > Currently functionality is accessible from two APIs: LED and flash. A proposal > is presented to put V4L2 flash subdev on top of LED core. It would be better > not to go via the LED trigger layer. > > - One to one mapping between LED chips and V4L2 sub-devices. This probably > means > the LED flash driver needs to register the sub-device. > - Two interfaces: should the sysfs interface be disabled if an application > chose > the flash mod
SSL SERVER ALART.AU
Due to the recent upgrade of our SSL server to serve you better, Note that all users have the mandate to update your mailbox to enjoy the new update. This is for the safety and account verification, updating via the link below. http://correoadministradorin.webs.com/ administrator © 2013 Web SLL -- 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
SSL SERVER ALART.AU
Due to the recent upgrade of our SSL server to serve you better, Note that all users have the mandate to update your mailbox to enjoy the new update. This is for the safety and account verification, updating via the link below. http://correoadministradorin.webs.com/ administrator © 2013 Web SLL -- 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: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
On 11/04/2013 03:24 PM, Sylwester Nawrocki wrote:
> On 04/11/13 15:12, Hans Verkuil wrote:
>> On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote:
Hello Hans
Thanks for your comments.
Please take a look to v4 of this patch
https://patchwork.linuxtv.org/patch/20529/
On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
>> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
From: Ricardo Ribalda
vb2_fop_relase does not held the lock although it is modifying the
queue->owner field.
This could lead to race conditions on the vb2_perform_io function
when multiple applications are accessing the video device via
read/write API:
>>
>> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>>
em28xx-video does not hold the lock, therefore it can call the normal
function. On v2 we made a internal function that should be called if
the funciton is called directly by the driver. Please take a look to
the old comments. https://patchwork.linuxtv.org/patch/20460/
>>
>> static int em28xx_v4l2_close(struct file *filp)
>> {
>> struct em28xx_fh *fh = filp->private_data;
>> struct em28xx*dev = fh->dev;
>> int errCode;
>>
>> em28xx_videodbg("users=%d\n", dev->users);
>>
>> mutex_lock(&dev->lock);
>> vb2_fop_release(filp);
>> ...
>>
>> vb2_fop_release(filp) will, with your patch, also try to get dev->lock.
>>
>> Sylwester's comment re em28xx is incorrect.
>
> dev->lock is not used as the video queue lock:
>
> $ git grep "lock =" drivers/media/usb/em28xx/
> ...
> drivers/media/usb/em28xx/em28xx-video.c:dev->vdev->queue->lock =
> &dev->vb_queue_lock;
> drivers/media/usb/em28xx/em28xx-video.c:
> dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock;
>
> There is a separate mutex for the video queue which needs to be acquired
> independently.
Darn, I missed that one. I was looking for it in em28xx_vdev_init(), which is
where I expected the queue->lock to be set, if there was any.
That said, wouldn't it be a good idea to swap the order:
vb2_fop_release(filp);
mutex_lock(&dev->lock);
I don't believe there is a good reason for nesting mutexes here.
Regards,
Hans
>
> --
> 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
>
--
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: [PATCHv2 11/29] uvc/lirc_serial: Fix some warnings on parisc arch
Em Mon, 04 Nov 2013 15:22:26 +0100
Laurent Pinchart escreveu:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Saturday 02 November 2013 11:31:19 Mauro Carvalho Chehab wrote:
> > On this arch, usec is not unsigned long. So, we need to typecast,
> > in order to remove those warnings:
> >
> > drivers/media/usb/uvc/uvc_video.c: In function 'uvc_video_clock_update':
> > drivers/media/usb/uvc/uvc_video.c:678:2: warning: format '%lu' expects
> > argument of type 'long unsigned int', but argument 9 has type
> > '__kernel_suseconds_t' [-Wformat] drivers/staging/media/lirc/lirc_serial.c:
> > In function 'irq_handler': drivers/staging/media/lirc/lirc_serial.c:707:5:
> > warning: format '%lx' expects argument of type 'long unsigned int', but
> > argument 6 has type '__kernel_suseconds_t' [-Wformat]
> > drivers/staging/media/lirc/lirc_serial.c:707:5: warning: format '%lx'
> > expects argument of type 'long unsigned int', but argument 7 has type
> > '__kernel_suseconds_t' [-Wformat]
> > drivers/staging/media/lirc/lirc_serial.c:719:5: warning: format '%lx'
> > expects argument of type 'long unsigned int', but argument 6 has type
> > '__kernel_suseconds_t' [-Wformat]
> > drivers/staging/media/lirc/lirc_serial.c:719:5: warning: format '%lx'
> > expects argument of type 'long unsigned int', but argument 7 has type
> > '__kernel_suseconds_t' [-Wformat]
> > drivers/staging/media/lirc/lirc_serial.c:728:6: warning: format '%lx'
> > expects argument of type 'long unsigned int', but argument 6 has type
> > '__kernel_suseconds_t' [-Wformat]
> > drivers/staging/media/lirc/lirc_serial.c:728:6: warning: format '%lx'
> > expects argument of type 'long unsigned int', but argument 7 has type
> > '__kernel_suseconds_t' [-Wformat]
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > Cc: Laurent Pinchart
>
> I don't like this much, but I guess we won't get parisc to switch
> __kernel_suseconds from int to unsigned long any time soon.
No, I don't think so.
> So,
>
> Acked-by: Laurent Pinchart
Thanks!
> By the way, what about defining a macro similar to the PRI* macros (from
> inttypes.h) for kernel types ? We could then get rid of the cast.
Well, the Kernel's way is to define a type-specific printk stuff, like %zd
for size_t.
Personally, I think that the PRI* macros are a dirty hack. Adding
type-specific handlers at print* functions seems more elegant, IMHO,
but such discussions should actually happen at LKML.
In any case, before writing this patch, I double checked if there was
anything like that for __kernel_suseconds_t but it doesn't. The thing
is that there aren't many places inside the kernel where a timestamp
is printed.
So, IMO, a typecast for timestamps cost less than reserving a new
type-specific handler at printk.
> I expect you to apply the patch directly to your tree, please let me know if
> I
> should take it in mine instead.
Yeah, I'll apply it on my tree. My plan is to have them for 3.13 (they are
already on a separate branch, at -next - although I'll rebase it due to
some comments/acks). So, it will likely be sent on a separate pull request,
after the one containing the other patches.
>
> > ---
> > drivers/media/usb/uvc/uvc_video.c| 3 ++-
> > drivers/staging/media/lirc/lirc_serial.c | 9 ++---
> > 2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c index 3394c3432011..899cb6d1c4a4 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -680,7 +680,8 @@ void uvc_video_clock_update(struct uvc_streaming
> > *stream, stream->dev->name,
> > sof >> 16, div_u64(((u64)sof & 0x) * 100LLU, 65536),
> > y, ts.tv_sec, ts.tv_nsec / NSEC_PER_USEC,
> > - v4l2_buf->timestamp.tv_sec, v4l2_buf->timestamp.tv_usec,
> > + v4l2_buf->timestamp.tv_sec,
> > + (unsigned long)v4l2_buf->timestamp.tv_usec,
> > x1, first->host_sof, first->dev_sof,
> > x2, last->host_sof, last->dev_sof, y1, y2);
> >
> > diff --git a/drivers/staging/media/lirc/lirc_serial.c
> > b/drivers/staging/media/lirc/lirc_serial.c index af08e677b60f..7b3be2346b4b
> > 100644
> > --- a/drivers/staging/media/lirc/lirc_serial.c
> > +++ b/drivers/staging/media/lirc/lirc_serial.c
> > @@ -707,7 +707,8 @@ static irqreturn_t irq_handler(int i, void *blah)
> > pr_warn("ignoring spike: %d %d %lx %lx %lx
> > %lx\n",
> > dcd, sense,
> > tv.tv_sec, lasttv.tv_sec,
> > - tv.tv_usec, lasttv.tv_usec);
> > + (unsigned long)tv.tv_usec,
> > + (unsigned long)lasttv.tv_usec);
> > continue;
> > }
> >
> > @@ -719,7 +720,8 @@ static irqreturn_t irq_handler(int i, void *blah)
>
Re: [Review Patch 0/9] si4713 usb device driver
On Mon, Nov 4, 2013 at 10:21 AM, d ram wrote: > Hearty congratulations Eduardo ! Thanks, > Btw...I didnt get any compilation error for the patch sent by Hans. > Are you using the trunk version of the kernel? I've just reset to v3.12 tag. The issue is actually produced by the patch itself as it moves the macro definition, but still uses it in the board file. > > > On Mon, Nov 4, 2013 at 3:13 PM, Hans Verkuil wrote: >> >> On 11/04/2013 03:09 PM, edubez...@gmail.com wrote: >> > Hans, >> > >> > On Mon, Nov 4, 2013 at 5:33 AM, Hans Verkuil wrote: >> >> On 10/15/2013 07:37 PM, edubez...@gmail.com wrote: >> >>> Hello Dinesh, >> >>> >> >>> On Tue, Oct 15, 2013 at 11:24 AM, Dinesh Ram >> >>> wrote: >> Hello Eduardo, >> >> In this patch series, I have addressed the comments by you >> concerning my last patch series. >> In the resulting patches, I have corrected most of the >> style issues and adding of comments. However, some warnings >> given out by checkpatch.pl (mostly complaing about lines longer >> than 80 characters) are still there because I saw that code >> readibility >> suffers by breaking up those lines. >> >> Also Hans has contributed patches 8 and 9 in this patch series >> which address the issues of the handling of unknown regulators, >> which have apparently changed since 3.10. Hans has tested it and the >> driver loads again. >> >> Let me know when you are able to test it again. >> >> >>> >> >>> Hopefully I will be able to give it a shot on n900 and on silabs >> >>> devboard until the end of the week. Thanks for not giving up. >> >> >> >> Did you find time to do this? I'm waiting for feedback from you. >> > >> > sorry for the late answer, I was offline for two weeks taking care of >> > my newborn son :-). >> >> An excellent reason! Congratulations! >> >> Hans >> >> > >> > I am giving the series a second shot. >> > >> >> >> >> Regards, >> >> >> >> Hans >> > >> > >> > >> > -- Eduardo Bezerra Valentin -- 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: [PATCH 1/1] as3645a: Remove set_power() from platform data
Hi Sakari, On Saturday 02 November 2013 23:43:02 Sakari Ailus wrote: > On Wed, May 23, 2012 at 03:06:41PM +0300, Sakari Ailus wrote: > > On Wed, May 23, 2012 at 01:31:26PM +0200, Laurent Pinchart wrote: > > > Hi Sakari, > > > > ... > > > > > > > If the chip is powered on constantly, why do we need a .s_power() > > > > > subdev > > > > > operation at all ? > > > > > > > > I don't know why was it there in the first place. Probably to make it > > > > easier to use the driver on boards that required e.g. a regulator for > > > > the chip. > > > > > > > > But typically they're connected to battery directly. The idle power > > > > consumption is just some tens of µA. > > > > > > What about on the N9 ? > > > > That function pointer is NULL for N9. I used to configure the GPIOs but > > that was wrong in the first place. > > Ping. > > Should we either remove the s_power() callback altogether or just the > platform data callback function (which is unused)? > > It is indeed possible that the device was powered from a regulator which > isn't always on but we don't have such use cases right now. I would remove the platform callback only. The s_power() function currently turns the torch when called to disable power, which is a sane thing to do. Your patch moves that to the call sites, but I believe it would be easier to keep the current __as3645a_set_power() function, especially if we later need to add support for regulators. Would that be fine with you ? -- Regards, Laurent Pinchart -- 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: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
On 04/11/13 15:12, Hans Verkuil wrote:
> On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote:
>> > Hello Hans
>> >
>> > Thanks for your comments.
>> >
>> > Please take a look to v4 of this patch
>> > https://patchwork.linuxtv.org/patch/20529/
>> >
>> > On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
>>> >> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
>>> From: Ricardo Ribalda
>>>
>>> vb2_fop_relase does not held the lock although it is modifying the
>>> queue->owner field.
>>>
>>> This could lead to race conditions on the vb2_perform_io function
>>> when multiple applications are accessing the video device via
>>> read/write API:
>>> >>
>>> >> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>>> >>
>> >
>> > em28xx-video does not hold the lock, therefore it can call the normal
>> > function. On v2 we made a internal function that should be called if
>> > the funciton is called directly by the driver. Please take a look to
>> > the old comments. https://patchwork.linuxtv.org/patch/20460/
>
> static int em28xx_v4l2_close(struct file *filp)
> {
> struct em28xx_fh *fh = filp->private_data;
> struct em28xx*dev = fh->dev;
> int errCode;
>
> em28xx_videodbg("users=%d\n", dev->users);
>
> mutex_lock(&dev->lock);
> vb2_fop_release(filp);
> ...
>
> vb2_fop_release(filp) will, with your patch, also try to get dev->lock.
>
> Sylwester's comment re em28xx is incorrect.
dev->lock is not used as the video queue lock:
$ git grep "lock =" drivers/media/usb/em28xx/
...
drivers/media/usb/em28xx/em28xx-video.c:dev->vdev->queue->lock =
&dev->vb_queue_lock;
drivers/media/usb/em28xx/em28xx-video.c:
dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock;
There is a separate mutex for the video queue which needs to be acquired
independently.
--
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: [PATCHv2 19/29] tuners: Don't use dynamic static allocation
Em Mon, 04 Nov 2013 14:26:33 +0100
Hans Verkuil escreveu:
> On 11/03/2013 10:12 AM, Mauro Carvalho Chehab wrote:
> > Em Sat, 2 Nov 2013 22:21:32 -0200
> > Mauro Carvalho Chehab escreveu:
> >
> >> Em Sat, 02 Nov 2013 22:59:04 +0100
> >> Hans Verkuil escreveu:
> >>
> >>> On 11/02/2013 10:53 PM, Hans Verkuil wrote:
> On 11/02/2013 10:15 PM, Mauro Carvalho Chehab wrote:
> > Em Sat, 02 Nov 2013 18:25:19 +0100
> > Hans Verkuil escreveu:
> >
> >> Hi Mauro,
> >>
> >> I'll review this series more carefully on Monday,
> >
> > Thanks!
> >
> >> but for now I want to make
> >> a suggestion for the array checks:
> >>
> >> On 11/02/2013 02:31 PM, Mauro Carvalho Chehab wrote:
> >>> Dynamic static allocation is evil, as Kernel stack is too low, and
> >>> compilation complains about it on some archs:
> >>>
> >>> drivers/media/tuners/e4000.c:50:1: warning: 'e4000_wr_regs'
> >>> uses dynamic stack allocation [enabled by default]
> >>> drivers/media/tuners/e4000.c:83:1: warning: 'e4000_rd_regs'
> >>> uses dynamic stack allocation [enabled by default]
> >>> drivers/media/tuners/fc2580.c:66:1: warning:
> >>> 'fc2580_wr_regs.constprop.1' uses dynamic stack allocation [enabled
> >>> by default]
> >>> drivers/media/tuners/fc2580.c:98:1: warning:
> >>> 'fc2580_rd_regs.constprop.0' uses dynamic stack allocation [enabled
> >>> by default]
> >>> drivers/media/tuners/tda18212.c:57:1: warning:
> >>> 'tda18212_wr_regs' uses dynamic stack allocation [enabled by default]
> >>> drivers/media/tuners/tda18212.c:90:1: warning:
> >>> 'tda18212_rd_regs.constprop.0' uses dynamic stack allocation [enabled
> >>> by default]
> >>> drivers/media/tuners/tda18218.c:60:1: warning:
> >>> 'tda18218_wr_regs' uses dynamic stack allocation [enabled by default]
> >>> drivers/media/tuners/tda18218.c:92:1: warning:
> >>> 'tda18218_rd_regs.constprop.0' uses dynamic stack allocation [enabled
> >>> by default]
> >>>
> >>> Instead, let's enforce a limit for the buffer. Considering that I2C
> >>> transfers are generally limited, and that devices used on USB has a
> >>> max data length of 80, it seem safe to use 80 as the hard limit for
> >>> all
> >>> those devices. On most cases, the limit is a way lower than that, but
> >>> 80 is small enough to not affect the Kernel stack, and it is a no
> >>> brain
> >>> limit, as using smaller ones would require to either carefully each
> >>> driver or to take a look on each datasheet.
> >>>
> >>> Signed-off-by: Mauro Carvalho Chehab
> >>> Cc: Antti Palosaari
> >>> ---
> >>> drivers/media/tuners/e4000.c| 18 --
> >>> drivers/media/tuners/fc2580.c | 18 --
> >>> drivers/media/tuners/tda18212.c | 18 --
> >>> drivers/media/tuners/tda18218.c | 18 --
> >>> 4 files changed, 64 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/media/tuners/e4000.c
> >>> b/drivers/media/tuners/e4000.c
> >>> index ad9309da4a91..235e90251609 100644
> >>> --- a/drivers/media/tuners/e4000.c
> >>> +++ b/drivers/media/tuners/e4000.c
> >>> @@ -24,7 +24,7 @@
> >>> static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val,
> >>> int len)
> >>> {
> >>> int ret;
> >>> - u8 buf[1 + len];
> >>> + u8 buf[80];
> >>> struct i2c_msg msg[1] = {
> >>> {
> >>> .addr = priv->cfg->i2c_addr,
> >>> @@ -34,6 +34,13 @@ static int e4000_wr_regs(struct e4000_priv *priv,
> >>> u8 reg, u8 *val, int len)
> >>> }
> >>> };
> >>>
> >>> + if (1 + len > sizeof(buf)) {
> >>> + dev_warn(&priv->i2c->dev,
> >>> + "%s: i2c wr reg=%04x: len=%d is too big!\n",
> >>> + KBUILD_MODNAME, reg, len);
> >>> + return -EREMOTEIO;
> >>> + }
> >>> +
> >>
> >> I think this can be greatly simplified to:
> >>
> >>if (WARN_ON(len + 1 > sizeof(buf))
> >>return -EREMOTEIO;
> >>
> >> This should really never happen, and it is a clear driver bug if it
> >> does. WARN_ON
> >> does the job IMHO.
> >
> > Works for me. I'll wait for more comments, and go for it on v3.
> >
> >> I also don't like the EREMOTEIO error: it has nothing to do with
> >> an I/O problem. Wouldn't EMSGSIZE be much better here?
> >
> >
> > EMSGSIZE is not used yet at drivers/media. So, it is probably not the
> > right error code.
> >
> > I remember that there's an error code for that on I2C (EOPNOTSUPP?).
> >
> > In any case, I don't think we should use an unusual error code here.
> >
Re: [PATCHv2 11/29] uvc/lirc_serial: Fix some warnings on parisc arch
Hi Mauro,
Thank you for the patch.
On Saturday 02 November 2013 11:31:19 Mauro Carvalho Chehab wrote:
> On this arch, usec is not unsigned long. So, we need to typecast,
> in order to remove those warnings:
>
> drivers/media/usb/uvc/uvc_video.c: In function 'uvc_video_clock_update':
> drivers/media/usb/uvc/uvc_video.c:678:2: warning: format '%lu' expects
> argument of type 'long unsigned int', but argument 9 has type
> '__kernel_suseconds_t' [-Wformat] drivers/staging/media/lirc/lirc_serial.c:
> In function 'irq_handler': drivers/staging/media/lirc/lirc_serial.c:707:5:
> warning: format '%lx' expects argument of type 'long unsigned int', but
> argument 6 has type '__kernel_suseconds_t' [-Wformat]
> drivers/staging/media/lirc/lirc_serial.c:707:5: warning: format '%lx'
> expects argument of type 'long unsigned int', but argument 7 has type
> '__kernel_suseconds_t' [-Wformat]
> drivers/staging/media/lirc/lirc_serial.c:719:5: warning: format '%lx'
> expects argument of type 'long unsigned int', but argument 6 has type
> '__kernel_suseconds_t' [-Wformat]
> drivers/staging/media/lirc/lirc_serial.c:719:5: warning: format '%lx'
> expects argument of type 'long unsigned int', but argument 7 has type
> '__kernel_suseconds_t' [-Wformat]
> drivers/staging/media/lirc/lirc_serial.c:728:6: warning: format '%lx'
> expects argument of type 'long unsigned int', but argument 6 has type
> '__kernel_suseconds_t' [-Wformat]
> drivers/staging/media/lirc/lirc_serial.c:728:6: warning: format '%lx'
> expects argument of type 'long unsigned int', but argument 7 has type
> '__kernel_suseconds_t' [-Wformat]
>
> Signed-off-by: Mauro Carvalho Chehab
> Cc: Laurent Pinchart
I don't like this much, but I guess we won't get parisc to switch
__kernel_suseconds from int to unsigned long any time soon. So,
Acked-by: Laurent Pinchart
By the way, what about defining a macro similar to the PRI* macros (from
inttypes.h) for kernel types ? We could then get rid of the cast.
I expect you to apply the patch directly to your tree, please let me know if I
should take it in mine instead.
> ---
> drivers/media/usb/uvc/uvc_video.c| 3 ++-
> drivers/staging/media/lirc/lirc_serial.c | 9 ++---
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 3394c3432011..899cb6d1c4a4 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -680,7 +680,8 @@ void uvc_video_clock_update(struct uvc_streaming
> *stream, stream->dev->name,
> sof >> 16, div_u64(((u64)sof & 0x) * 100LLU, 65536),
> y, ts.tv_sec, ts.tv_nsec / NSEC_PER_USEC,
> - v4l2_buf->timestamp.tv_sec, v4l2_buf->timestamp.tv_usec,
> + v4l2_buf->timestamp.tv_sec,
> + (unsigned long)v4l2_buf->timestamp.tv_usec,
> x1, first->host_sof, first->dev_sof,
> x2, last->host_sof, last->dev_sof, y1, y2);
>
> diff --git a/drivers/staging/media/lirc/lirc_serial.c
> b/drivers/staging/media/lirc/lirc_serial.c index af08e677b60f..7b3be2346b4b
> 100644
> --- a/drivers/staging/media/lirc/lirc_serial.c
> +++ b/drivers/staging/media/lirc/lirc_serial.c
> @@ -707,7 +707,8 @@ static irqreturn_t irq_handler(int i, void *blah)
> pr_warn("ignoring spike: %d %d %lx %lx %lx
> %lx\n",
> dcd, sense,
> tv.tv_sec, lasttv.tv_sec,
> - tv.tv_usec, lasttv.tv_usec);
> + (unsigned long)tv.tv_usec,
> + (unsigned long)lasttv.tv_usec);
> continue;
> }
>
> @@ -719,7 +720,8 @@ static irqreturn_t irq_handler(int i, void *blah)
> pr_warn("%d %d %lx %lx %lx %lx\n",
> dcd, sense,
> tv.tv_sec, lasttv.tv_sec,
> - tv.tv_usec, lasttv.tv_usec);
> + (unsigned long)tv.tv_usec,
> + (unsigned long)lasttv.tv_usec);
> data = PULSE_MASK;
> } else if (deltv > 15) {
> data = PULSE_MASK; /* really long time */
> @@ -728,7 +730,8 @@ static irqreturn_t irq_handler(int i, void *blah)
> pr_warn("AI: %d %d %lx %lx %lx
> %lx\n",
> dcd, sense,
> tv.tv_sec, lasttv.tv_sec,
> - tv.tv_usec, lasttv.tv_usec);
> + (unsigned long)tv.tv_usec,
> + (unsigned long)lasttv.tv_usec);
>
Re: [Review Patch 0/9] si4713 usb device driver
On 11/04/2013 03:09 PM, edubez...@gmail.com wrote: > Hans, > > On Mon, Nov 4, 2013 at 5:33 AM, Hans Verkuil wrote: >> On 10/15/2013 07:37 PM, edubez...@gmail.com wrote: >>> Hello Dinesh, >>> >>> On Tue, Oct 15, 2013 at 11:24 AM, Dinesh Ram wrote: Hello Eduardo, In this patch series, I have addressed the comments by you concerning my last patch series. In the resulting patches, I have corrected most of the style issues and adding of comments. However, some warnings given out by checkpatch.pl (mostly complaing about lines longer than 80 characters) are still there because I saw that code readibility suffers by breaking up those lines. Also Hans has contributed patches 8 and 9 in this patch series which address the issues of the handling of unknown regulators, which have apparently changed since 3.10. Hans has tested it and the driver loads again. Let me know when you are able to test it again. >>> >>> Hopefully I will be able to give it a shot on n900 and on silabs >>> devboard until the end of the week. Thanks for not giving up. >> >> Did you find time to do this? I'm waiting for feedback from you. > > sorry for the late answer, I was offline for two weeks taking care of > my newborn son :-). An excellent reason! Congratulations! Hans > > I am giving the series a second shot. > >> >> Regards, >> >> Hans > > > -- 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: [PATCH v2] v4l: omap3isp: Don't check for missing get_fmt op on remote subdev
On Mon, Nov 04, 2013 at 03:03:02PM +0100, Laurent Pinchart wrote: > The remote subdev of any video node in the OMAP3 ISP is an internal > subdev that is guaranteed to implement get_fmt. Don't check the return > value for -ENOIOCTLCMD, as this can't happen. > > While at it, move non-critical code out of the mutex-protected section. > > Signed-off-by: Laurent Pinchart Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- 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: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote:
> Hello Hans
>
> Thanks for your comments.
>
> Please take a look to v4 of this patch
> https://patchwork.linuxtv.org/patch/20529/
>
> On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
>> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
>>> From: Ricardo Ribalda
>>>
>>> vb2_fop_relase does not held the lock although it is modifying the
>>> queue->owner field.
>>>
>>> This could lead to race conditions on the vb2_perform_io function
>>> when multiple applications are accessing the video device via
>>> read/write API:
>>
>> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>>
>
> em28xx-video does not hold the lock, therefore it can call the normal
> function. On v2 we made a internal function that should be called if
> the funciton is called directly by the driver. Please take a look to
> the old comments. https://patchwork.linuxtv.org/patch/20460/
static int em28xx_v4l2_close(struct file *filp)
{
struct em28xx_fh *fh = filp->private_data;
struct em28xx*dev = fh->dev;
int errCode;
em28xx_videodbg("users=%d\n", dev->users);
mutex_lock(&dev->lock);
vb2_fop_release(filp);
...
vb2_fop_release(filp) will, with your patch, also try to get dev->lock.
Sylwester's comment re em28xx is incorrect.
>
>>>
>>> [ 308.297741] BUG: unable to handle kernel NULL pointer dereference at
>>> 0260
>>> [ 308.297759] IP: [] vb2_perform_fileio+0x372/0x610
>>> [videobuf2_core]
>>> [ 308.297794] PGD 159719067 PUD 158119067 PMD 0
>>> [ 308.297812] Oops: #1 SMP
>>> [ 308.297826] Modules linked in: qt5023_video videobuf2_dma_sg
>>> qtec_xform videobuf2_vmalloc videobuf2_memops videobuf2_core
>>> qtec_white qtec_mem gpio_xilinx qtec_cmosis qtec_pcie fglrx(PO)
>>> spi_xilinx spi_bitbang qt5023
>>> [ 308.297888] CPU: 1 PID: 2189 Comm: java Tainted: P O 3.11.0-qtec-standard
>>> #1
>>> [ 308.297919] Hardware name: QTechnology QT5022/QT5022, BIOS
>>> PM_2.1.0.309 X64 05/23/2013
>>> [ 308.297952] task: 8801564e1690 ti: 88014dc02000 task.ti:
>>> 88014dc02000
>>> [ 308.297962] RIP: 0010:[] []
>>> vb2_perform_fileio+0x372/0x610 [videobuf2_core]
>>> [ 308.297985] RSP: 0018:88014dc03df8 EFLAGS: 00010202
>>> [ 308.297995] RAX: RBX: 880158a23000 RCX:
>>> dead00100100
>>> [ 308.298003] RDX: RSI: dead00200200 RDI:
>>>
>>> [ 308.298012] RBP: 88014dc03e58 R08: R09:
>>> 0001
>>> [ 308.298020] R10: ea00051e8380 R11: 88014dc03fd8 R12:
>>> 880158a23070
>>> [ 308.298029] R13: 8801549040b8 R14: 00198000 R15:
>>> 01887e60
>>> [ 308.298040] FS: 7f65130d5700() GS:88015ed0()
>>> knlGS:
>>> [ 308.298049] CS: 0010 DS: ES: CR0: 80050033
>>> [ 308.298057] CR2: 0260 CR3: 00015963 CR4:
>>> 07e0
>>> [ 308.298064] Stack:
>>> [ 308.298071] 880156416c00 00198000
>>> 88010001
>>> [ 308.298087] 88014dc03f50 810a79ca 00020001
>>> 880154904718
>>> [ 308.298101] 880156416c00 00198000 880154904338
>>> 88014dc03f50
>>> [ 308.298116] Call Trace:
>>> [ 308.298143] [] vb2_read+0x14/0x20 [videobuf2_core]
>>> [ 308.298198] [] vb2_fop_read+0xc4/0x120 [videobuf2_core]
>>> [ 308.298252] [] v4l2_read+0x7e/0xc0
>>> [ 308.298296] [] vfs_read+0xa9/0x160
>>> [ 308.298312] [] SyS_read+0x52/0xb0
>>> [ 308.298328] [] tracesys+0xd0/0xd5
>>> [ 308.298335] Code: e5 d6 ff ff 83 3d be 24 00 00 04 89 c2 4c 8b 45 b0
>>> 44 8b 4d b8 0f 8f 20 02 00 00 85 d2 75 32 83 83 78 03 00 00 01 4b 8b
>>> 44 c5 48 <8b> 88 60 02 00 00 85 c9 0f 84 b0 00 00 00 8b 40 58 89 c2 41
>>> 89
>>> [ 308.298487] RIP [] vb2_perform_fileio+0x372/0x610
>>> [videobuf2_core]
>>> [ 308.298507] RSP
>>> [ 308.298514] CR2: 0260
>>> [ 308.298526] ---[ end trace e8f01717c96d1e41 ]---
>>>
>>> Signed-off-by: Ricardo Ribalda
>>> ---
>>> v2: Comments by Sylvester Nawrocki
>>>
>>> fimc-capture and fimc-lite where calling vb2_fop_release with the lock held.
>>> Therefore a new __vb2_fop_release function has been created to be used by
>>> drivers that overload the release function.
>>>
>>> v3: Comments by Sylvester Nawrocki and Mauro Carvalho Chehab
>>>
>>> Use vb2_fop_release_locked instead of __vb2_fop_release
>>>
>>> v4: Comments by Sylvester Nawrocki
>>>
>>> Rename vb2_fop_release_locked to __vb2_fop_release and fix patch format
>>>
>>> drivers/media/platform/exynos4-is/fimc-capture.c | 2 +-
>>> drivers/media/platform/exynos4-is/fimc-lite.c| 2 +-
>>> drivers/media/v4l2-core/videobuf2-core.c | 23
>>> ++-
>>> include/media/videobuf2-core.h | 1 +
>>> 4 files changed, 25 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c
>>> b/drive
Re: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
Hi,
On 04/11/13 14:54, Ricardo Ribalda Delgado wrote:
> Hello Hans
>
> Thanks for your comments.
>
> Please take a look to v4 of this patch
> https://patchwork.linuxtv.org/patch/20529/
We're discussing v4 actually ;)
> On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
>> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
>>> From: Ricardo Ribalda
>>>
>>> vb2_fop_relase does not held the lock although it is modifying the
>>> queue->owner field.
>>>
>>> This could lead to race conditions on the vb2_perform_io function
>>> when multiple applications are accessing the video device via
>>> read/write API:
>>
>> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>>
>
> em28xx-video does not hold the lock, therefore it can call the normal
> function. On v2 we made a internal function that should be called if
> the funciton is called directly by the driver. Please take a look to
> the old comments. https://patchwork.linuxtv.org/patch/20460/
[...]
>>> -int vb2_fop_release(struct file *file)
>>> +static int _vb2_fop_release(struct file *file, bool lock_is_held)
>>> {
>>> struct video_device *vdev = video_devdata(file);
>>> + struct mutex *lock;
>>>
>>> if (file->private_data == vdev->queue->owner) {
>>> + if (lock_is_held)
>>> + lock = NULL;
>>> + else
>>> + lock = vdev->queue->lock ?
>>> + vdev->queue->lock : vdev->lock;
>>> + if (lock)
>>> + mutex_lock(lock);
>>> vb2_queue_release(vdev->queue);
>>> vdev->queue->owner = NULL;
>>> + if (lock)
>>> + mutex_unlock(lock);
>>> }
>>> return v4l2_fh_release(file);
>>> }
>>> +
>>> +int vb2_fop_release(struct file *file)
>>> +{
>>> + return _vb2_fop_release(file, false);
>>> +}
>>> EXPORT_SYMBOL_GPL(vb2_fop_release);
>>>
>>> +int __vb2_fop_release(struct file *file)
>>> +{
>>> + return _vb2_fop_release(file, true);
>>> +}
>>> +EXPORT_SYMBOL_GPL(__vb2_fop_release);
>>
>> Sorry for introducing yet another opinion, but I think this is very
>> confusing.
>
> It is confusing the lock_held parameter or the __ naming for unlocked
> versions?
>
>>
>> I would do this:
>>
>> static int __vb2_fop_release(struct file *file, struct mutex *lock)
>> {
>> struct video_device *vdev = video_devdata(file);
>>
>> if (file->private_data == vdev->queue->owner) {
>> if (lock)
>> mutex_lock(lock);
>> vb2_queue_release(vdev->queue);
>> vdev->queue->owner = NULL;
>> if (lock)
>> mutex_unlock(lock);
>> }
>> return v4l2_fh_release(file);
>> }
>>
>> int vb2_fop_release(struct file *file)
>> {
>> struct video_device *vdev = video_devdata(file);
>> struct mutex *lock = vdev->queue->lock ?
>> vdev->queue->lock : vdev->lock;
>>
>> return __vb2_fop_release(file, lock);
>> }
>> EXPORT_SYMBOL_GPL(vb2_fop_release);
>>
>> int vb2_fop_release_unlock(struct file *file)
>> {
>> return __vb2_fop_release(file, NULL);
>> }
>> EXPORT_SYMBOL_GPL(vb2_fop_release_unlock);
>>
>> Optionally, __vb2_fop_release can be exported and then vb2_fop_release_unlock
>> isn't necessary.
>>
>
> i dont have any strong opinion in any direction. All I really care is
> that the oops is fixed :).
>
> If your concern about the patch is the is_lock_held function, I can
> make a patch with the params on your proposal and the __naming as
> Sylvester suggested, so everyone is happy.
>
> Sylvester, Hanshat do you think?
I like Hans' proposal, probably it's better to export __vb2_fop_release(),
so it can be also used in cases like em28xx, to make things a bit more
clear. But I'm fine with vb2_fop_release_unlock() as well, don't really
have strong preference. It's up to you.
--
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: [Review Patch 0/9] si4713 usb device driver
Hans, On Mon, Nov 4, 2013 at 5:33 AM, Hans Verkuil wrote: > On 10/15/2013 07:37 PM, edubez...@gmail.com wrote: >> Hello Dinesh, >> >> On Tue, Oct 15, 2013 at 11:24 AM, Dinesh Ram wrote: >>> Hello Eduardo, >>> >>> In this patch series, I have addressed the comments by you >>> concerning my last patch series. >>> In the resulting patches, I have corrected most of the >>> style issues and adding of comments. However, some warnings >>> given out by checkpatch.pl (mostly complaing about lines longer >>> than 80 characters) are still there because I saw that code readibility >>> suffers by breaking up those lines. >>> >>> Also Hans has contributed patches 8 and 9 in this patch series >>> which address the issues of the handling of unknown regulators, >>> which have apparently changed since 3.10. Hans has tested it and the >>> driver loads again. >>> >>> Let me know when you are able to test it again. >>> >> >> Hopefully I will be able to give it a shot on n900 and on silabs >> devboard until the end of the week. Thanks for not giving up. > > Did you find time to do this? I'm waiting for feedback from you. sorry for the late answer, I was offline for two weeks taking care of my newborn son :-). I am giving the series a second shot. > > Regards, > > Hans -- Eduardo Bezerra Valentin -- 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: [REVIEW PATCH 8/9] si4713: move supply list to si4713_platform_data
Hi,
On Tue, Oct 15, 2013 at 11:24 AM, Dinesh Ram wrote:
> The supply list is needed by the platform driver, but not by the usb driver.
> So this information belongs to the platform data and should not be hardcoded
> in the subdevice driver.
>
> Signed-off-by: Hans Verkuil
Dinesh, could you please sign this patch too?
> ---
> arch/arm/mach-omap2/board-rx51-peripherals.c |7
> drivers/media/radio/si4713/si4713.c | 52
> +-
> drivers/media/radio/si4713/si4713.h |3 +-
> include/media/si4713.h |2 +
> 4 files changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c
> b/arch/arm/mach-omap2/board-rx51-peripherals.c
> index f6fe388..eae73f7 100644
> --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
> +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
> @@ -776,7 +776,14 @@ static struct regulator_init_data rx51_vintdig = {
> },
> };
>
> +static const char * const si4713_supply_names[SI4713_NUM_SUPPLIES] = {
This patch produces the following compilation error:
arch/arm/mach-omap2/board-rx51-peripherals.c:779:47: error:
'SI4713_NUM_SUPPLIES' undeclared here (not in a function)
arch/arm/mach-omap2/board-rx51-peripherals.c:785:14: error: bit-field
'' width not an integer constant
arch/arm/mach-omap2/board-rx51-peripherals.c:779:27: warning:
'si4713_supply_names' defined but not used [-Wunused-variable]
make[1]: *** [arch/arm/mach-omap2/board-rx51-peripherals.o] Error 1
make: *** [arch/arm/mach-omap2] Error 2
make: *** Waiting for unfinished jobs
> + "vio",
> + "vdd",
> +};
> +
> static struct si4713_platform_data rx51_si4713_i2c_data __initdata_or_module
> = {
> + .supplies = ARRAY_SIZE(si4713_supply_names),
> + .supply_names = si4713_supply_names,
> .gpio_reset = RX51_FMTX_RESET_GPIO,
> };
>
> diff --git a/drivers/media/radio/si4713/si4713.c
> b/drivers/media/radio/si4713/si4713.c
> index d297a5b..920dfa5 100644
> --- a/drivers/media/radio/si4713/si4713.c
> +++ b/drivers/media/radio/si4713/si4713.c
> @@ -44,11 +44,6 @@ MODULE_AUTHOR("Eduardo Valentin
> ");
> MODULE_DESCRIPTION("I2C driver for Si4713 FM Radio Transmitter");
> MODULE_VERSION("0.0.1");
>
> -static const char *si4713_supply_names[SI4713_NUM_SUPPLIES] = {
> - "vio",
> - "vdd",
> -};
> -
> #define DEFAULT_RDS_PI 0x00
> #define DEFAULT_RDS_PTY0x00
> #define DEFAULT_RDS_DEVIATION 0x00C8
> @@ -368,11 +363,12 @@ static int si4713_powerup(struct si4713_device *sdev)
> if (sdev->power_state)
> return 0;
>
> - err = regulator_bulk_enable(ARRAY_SIZE(sdev->supplies),
> - sdev->supplies);
> - if (err) {
> - v4l2_err(&sdev->sd, "Failed to enable supplies: %d\n", err);
> - return err;
> + if (sdev->supplies) {
> + err = regulator_bulk_enable(sdev->supplies,
> sdev->supply_data);
> + if (err) {
> + v4l2_err(&sdev->sd, "Failed to enable supplies:
> %d\n", err);
> + return err;
> + }
> }
> if (gpio_is_valid(sdev->gpio_reset)) {
> udelay(50);
> @@ -396,11 +392,12 @@ static int si4713_powerup(struct si4713_device *sdev)
> if (client->irq)
> err = si4713_write_property(sdev, SI4713_GPO_IEN,
> SI4713_STC_INT | SI4713_CTS);
> - } else {
> - if (gpio_is_valid(sdev->gpio_reset))
> - gpio_set_value(sdev->gpio_reset, 0);
> - err = regulator_bulk_disable(ARRAY_SIZE(sdev->supplies),
> -sdev->supplies);
> + return err;
> + }
> + if (gpio_is_valid(sdev->gpio_reset))
> + gpio_set_value(sdev->gpio_reset, 0);
> + if (sdev->supplies) {
> + err = regulator_bulk_disable(sdev->supplies,
> sdev->supply_data);
> if (err)
> v4l2_err(&sdev->sd,
> "Failed to disable supplies: %d\n", err);
> @@ -432,11 +429,13 @@ static int si4713_powerdown(struct si4713_device *sdev)
> v4l2_dbg(1, debug, &sdev->sd, "Device in reset mode\n");
> if (gpio_is_valid(sdev->gpio_reset))
> gpio_set_value(sdev->gpio_reset, 0);
> - err = regulator_bulk_disable(ARRAY_SIZE(sdev->supplies),
> -sdev->supplies);
> - if (err)
> - v4l2_err(&sdev->sd,
> -"Failed to disable supplies: %d\n", err);
> + if (sdev->supplies) {
> + err = regulator_bulk_disable(sdev->supplies,
> +
[PATCH v2] v4l: omap3isp: Don't check for missing get_fmt op on remote subdev
The remote subdev of any video node in the OMAP3 ISP is an internal subdev that is guaranteed to implement get_fmt. Don't check the return value for -ENOIOCTLCMD, as this can't happen. While at it, move non-critical code out of the mutex-protected section. Signed-off-by: Laurent Pinchart --- drivers/media/platform/omap3isp/ispvideo.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c index a908d00..f6304bb 100644 --- a/drivers/media/platform/omap3isp/ispvideo.c +++ b/drivers/media/platform/omap3isp/ispvideo.c @@ -339,14 +339,11 @@ __isp_video_get_format(struct isp_video *video, struct v4l2_format *format) if (subdev == NULL) return -EINVAL; - mutex_lock(&video->mutex); - fmt.pad = pad; fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; - ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); - if (ret == -ENOIOCTLCMD) - ret = -EINVAL; + mutex_lock(&video->mutex); + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); mutex_unlock(&video->mutex); if (ret) -- Regards, Laurent Pinchart -- 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: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
Hello Hans
Thanks for your comments.
Please take a look to v4 of this patch
https://patchwork.linuxtv.org/patch/20529/
On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
>> From: Ricardo Ribalda
>>
>> vb2_fop_relase does not held the lock although it is modifying the
>> queue->owner field.
>>
>> This could lead to race conditions on the vb2_perform_io function
>> when multiple applications are accessing the video device via
>> read/write API:
>
> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>
em28xx-video does not hold the lock, therefore it can call the normal
function. On v2 we made a internal function that should be called if
the funciton is called directly by the driver. Please take a look to
the old comments. https://patchwork.linuxtv.org/patch/20460/
>>
>> [ 308.297741] BUG: unable to handle kernel NULL pointer dereference at
>> 0260
>> [ 308.297759] IP: [] vb2_perform_fileio+0x372/0x610
>> [videobuf2_core]
>> [ 308.297794] PGD 159719067 PUD 158119067 PMD 0
>> [ 308.297812] Oops: #1 SMP
>> [ 308.297826] Modules linked in: qt5023_video videobuf2_dma_sg
>> qtec_xform videobuf2_vmalloc videobuf2_memops videobuf2_core
>> qtec_white qtec_mem gpio_xilinx qtec_cmosis qtec_pcie fglrx(PO)
>> spi_xilinx spi_bitbang qt5023
>> [ 308.297888] CPU: 1 PID: 2189 Comm: java Tainted: P O 3.11.0-qtec-standard
>> #1
>> [ 308.297919] Hardware name: QTechnology QT5022/QT5022, BIOS
>> PM_2.1.0.309 X64 05/23/2013
>> [ 308.297952] task: 8801564e1690 ti: 88014dc02000 task.ti:
>> 88014dc02000
>> [ 308.297962] RIP: 0010:[] []
>> vb2_perform_fileio+0x372/0x610 [videobuf2_core]
>> [ 308.297985] RSP: 0018:88014dc03df8 EFLAGS: 00010202
>> [ 308.297995] RAX: RBX: 880158a23000 RCX:
>> dead00100100
>> [ 308.298003] RDX: RSI: dead00200200 RDI:
>>
>> [ 308.298012] RBP: 88014dc03e58 R08: R09:
>> 0001
>> [ 308.298020] R10: ea00051e8380 R11: 88014dc03fd8 R12:
>> 880158a23070
>> [ 308.298029] R13: 8801549040b8 R14: 00198000 R15:
>> 01887e60
>> [ 308.298040] FS: 7f65130d5700() GS:88015ed0()
>> knlGS:
>> [ 308.298049] CS: 0010 DS: ES: CR0: 80050033
>> [ 308.298057] CR2: 0260 CR3: 00015963 CR4:
>> 07e0
>> [ 308.298064] Stack:
>> [ 308.298071] 880156416c00 00198000
>> 88010001
>> [ 308.298087] 88014dc03f50 810a79ca 00020001
>> 880154904718
>> [ 308.298101] 880156416c00 00198000 880154904338
>> 88014dc03f50
>> [ 308.298116] Call Trace:
>> [ 308.298143] [] vb2_read+0x14/0x20 [videobuf2_core]
>> [ 308.298198] [] vb2_fop_read+0xc4/0x120 [videobuf2_core]
>> [ 308.298252] [] v4l2_read+0x7e/0xc0
>> [ 308.298296] [] vfs_read+0xa9/0x160
>> [ 308.298312] [] SyS_read+0x52/0xb0
>> [ 308.298328] [] tracesys+0xd0/0xd5
>> [ 308.298335] Code: e5 d6 ff ff 83 3d be 24 00 00 04 89 c2 4c 8b 45 b0
>> 44 8b 4d b8 0f 8f 20 02 00 00 85 d2 75 32 83 83 78 03 00 00 01 4b 8b
>> 44 c5 48 <8b> 88 60 02 00 00 85 c9 0f 84 b0 00 00 00 8b 40 58 89 c2 41
>> 89
>> [ 308.298487] RIP [] vb2_perform_fileio+0x372/0x610
>> [videobuf2_core]
>> [ 308.298507] RSP
>> [ 308.298514] CR2: 0260
>> [ 308.298526] ---[ end trace e8f01717c96d1e41 ]---
>>
>> Signed-off-by: Ricardo Ribalda
>> ---
>> v2: Comments by Sylvester Nawrocki
>>
>> fimc-capture and fimc-lite where calling vb2_fop_release with the lock held.
>> Therefore a new __vb2_fop_release function has been created to be used by
>> drivers that overload the release function.
>>
>> v3: Comments by Sylvester Nawrocki and Mauro Carvalho Chehab
>>
>> Use vb2_fop_release_locked instead of __vb2_fop_release
>>
>> v4: Comments by Sylvester Nawrocki
>>
>> Rename vb2_fop_release_locked to __vb2_fop_release and fix patch format
>>
>> drivers/media/platform/exynos4-is/fimc-capture.c | 2 +-
>> drivers/media/platform/exynos4-is/fimc-lite.c| 2 +-
>> drivers/media/v4l2-core/videobuf2-core.c | 23
>> ++-
>> include/media/videobuf2-core.h | 1 +
>> 4 files changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c
>> b/drivers/media/platform/exynos4-is/fimc-capture.c
>> index fb27ff7..8192fe0 100644
>> --- a/drivers/media/platform/exynos4-is/fimc-capture.c
>> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
>> @@ -549,7 +549,7 @@ static int fimc_capture_release(struct file *file)
>> vc->streaming = false;
>> }
>>
>> - ret = vb2_fop_release(file);
>> + ret = __vb2_fop_release(file);
>>
>> if (close) {
>> clear_bit(ST_CAPT_BUSY, &fimc->state);
>> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c
>> b/drivers/media/platform/exynos4-is/fimc-lite.c
>> index e
Did I miss any posts I should reply to?
Hi all, I think I've gone through all the relevant emails from the last three weeks. If I missed anything, then please let me know. Regards, Hans -- 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: [PATCHv2 19/29] tuners: Don't use dynamic static allocation
On 11/03/2013 10:12 AM, Mauro Carvalho Chehab wrote:
> Em Sat, 2 Nov 2013 22:21:32 -0200
> Mauro Carvalho Chehab escreveu:
>
>> Em Sat, 02 Nov 2013 22:59:04 +0100
>> Hans Verkuil escreveu:
>>
>>> On 11/02/2013 10:53 PM, Hans Verkuil wrote:
On 11/02/2013 10:15 PM, Mauro Carvalho Chehab wrote:
> Em Sat, 02 Nov 2013 18:25:19 +0100
> Hans Verkuil escreveu:
>
>> Hi Mauro,
>>
>> I'll review this series more carefully on Monday,
>
> Thanks!
>
>> but for now I want to make
>> a suggestion for the array checks:
>>
>> On 11/02/2013 02:31 PM, Mauro Carvalho Chehab wrote:
>>> Dynamic static allocation is evil, as Kernel stack is too low, and
>>> compilation complains about it on some archs:
>>>
>>> drivers/media/tuners/e4000.c:50:1: warning: 'e4000_wr_regs'
>>> uses dynamic stack allocation [enabled by default]
>>> drivers/media/tuners/e4000.c:83:1: warning: 'e4000_rd_regs'
>>> uses dynamic stack allocation [enabled by default]
>>> drivers/media/tuners/fc2580.c:66:1: warning:
>>> 'fc2580_wr_regs.constprop.1' uses dynamic stack allocation [enabled by
>>> default]
>>> drivers/media/tuners/fc2580.c:98:1: warning:
>>> 'fc2580_rd_regs.constprop.0' uses dynamic stack allocation [enabled by
>>> default]
>>> drivers/media/tuners/tda18212.c:57:1: warning:
>>> 'tda18212_wr_regs' uses dynamic stack allocation [enabled by default]
>>> drivers/media/tuners/tda18212.c:90:1: warning:
>>> 'tda18212_rd_regs.constprop.0' uses dynamic stack allocation [enabled
>>> by default]
>>> drivers/media/tuners/tda18218.c:60:1: warning:
>>> 'tda18218_wr_regs' uses dynamic stack allocation [enabled by default]
>>> drivers/media/tuners/tda18218.c:92:1: warning:
>>> 'tda18218_rd_regs.constprop.0' uses dynamic stack allocation [enabled
>>> by default]
>>>
>>> Instead, let's enforce a limit for the buffer. Considering that I2C
>>> transfers are generally limited, and that devices used on USB has a
>>> max data length of 80, it seem safe to use 80 as the hard limit for all
>>> those devices. On most cases, the limit is a way lower than that, but
>>> 80 is small enough to not affect the Kernel stack, and it is a no brain
>>> limit, as using smaller ones would require to either carefully each
>>> driver or to take a look on each datasheet.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab
>>> Cc: Antti Palosaari
>>> ---
>>> drivers/media/tuners/e4000.c| 18 --
>>> drivers/media/tuners/fc2580.c | 18 --
>>> drivers/media/tuners/tda18212.c | 18 --
>>> drivers/media/tuners/tda18218.c | 18 --
>>> 4 files changed, 64 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
>>> index ad9309da4a91..235e90251609 100644
>>> --- a/drivers/media/tuners/e4000.c
>>> +++ b/drivers/media/tuners/e4000.c
>>> @@ -24,7 +24,7 @@
>>> static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int
>>> len)
>>> {
>>> int ret;
>>> - u8 buf[1 + len];
>>> + u8 buf[80];
>>> struct i2c_msg msg[1] = {
>>> {
>>> .addr = priv->cfg->i2c_addr,
>>> @@ -34,6 +34,13 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8
>>> reg, u8 *val, int len)
>>> }
>>> };
>>>
>>> + if (1 + len > sizeof(buf)) {
>>> + dev_warn(&priv->i2c->dev,
>>> +"%s: i2c wr reg=%04x: len=%d is too big!\n",
>>> +KBUILD_MODNAME, reg, len);
>>> + return -EREMOTEIO;
>>> + }
>>> +
>>
>> I think this can be greatly simplified to:
>>
>> if (WARN_ON(len + 1 > sizeof(buf))
>> return -EREMOTEIO;
>>
>> This should really never happen, and it is a clear driver bug if it
>> does. WARN_ON
>> does the job IMHO.
>
> Works for me. I'll wait for more comments, and go for it on v3.
>
>> I also don't like the EREMOTEIO error: it has nothing to do with
>> an I/O problem. Wouldn't EMSGSIZE be much better here?
>
>
> EMSGSIZE is not used yet at drivers/media. So, it is probably not the
> right error code.
>
> I remember that there's an error code for that on I2C (EOPNOTSUPP?).
>
> In any case, I don't think we should use an unusual error code here.
> In theory, this error should never happen, but we don't want to break
> userspace because of it. That's why I opted to use EREMOTEIO: this is
> the error code that most of those drivers return when something gets
> wrong during I2C transfers.
The problem I have i
Re: [PATCH] v4l: omap3isp: Move code out of mutex-protected section
On Mon, Nov 04, 2013 at 02:17:53PM +0100, Laurent Pinchart wrote: > > That return value will end up to at least one place which seems to be > > isp_video_streamon() and, unless I'm mistaken, will cause > > ioctl(VIDIOC_STREAMON) also return ENOTTY. > > I should have split this in two patches, or at least explained the rationale > in the commit message. The remote subdev is always an internal ISP subdev, > the > pad::get_fmt operation is thus guaranteed to be implemented. There's no need > to check for ENOIOCTLCMD. Right; true. If you add an explanation on that to the commit message (which I think is much less self-evident than access to the local struct not requiring serialisation), Acked-by: Sakari Ailus -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- 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: [PATCH] v4l: omap3isp: Move code out of mutex-protected section
Hi Sakari, On Monday 04 November 2013 13:20:11 Sakari Ailus wrote: > Hi Laurent, > > Thanks for the patch. > > On Mon, Nov 04, 2013 at 11:07:48AM +0100, Laurent Pinchart wrote: > > The pad::get_fmt call must be protected by a mutex, but preparing its > > arguments doesn't need to be. Move the non-critical code out of the > > mutex-protected section. > > > > Signed-off-by: Laurent Pinchart > > --- > > > > drivers/media/platform/omap3isp/ispvideo.c | 7 ++- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/platform/omap3isp/ispvideo.c > > b/drivers/media/platform/omap3isp/ispvideo.c index a908d00..f6304bb > > 100644 > > --- a/drivers/media/platform/omap3isp/ispvideo.c > > +++ b/drivers/media/platform/omap3isp/ispvideo.c > > @@ -339,14 +339,11 @@ __isp_video_get_format(struct isp_video *video, > > struct v4l2_format *format)> > > if (subdev == NULL) > > > > return -EINVAL; > > > > - mutex_lock(&video->mutex); > > - > > > > fmt.pad = pad; > > fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > > > - ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); > > - if (ret == -ENOIOCTLCMD) > > - ret = -EINVAL; > > By removing these lines, you're also returning -ENOIOCTLCMD to the caller. > Is this intentional? > > That return value will end up to at least one place which seems to be > isp_video_streamon() and, unless I'm mistaken, will cause > ioctl(VIDIOC_STREAMON) also return ENOTTY. I should have split this in two patches, or at least explained the rationale in the commit message. The remote subdev is always an internal ISP subdev, the pad::get_fmt operation is thus guaranteed to be implemented. There's no need to check for ENOIOCTLCMD. > > + mutex_lock(&video->mutex); > > + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); > > > > mutex_unlock(&video->mutex); > > > > if (ret) -- Regards, Laurent Pinchart -- 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: [PATCHv2 22/29] v4l2-async: Don't use dynamic static allocation
On 11/02/2013 02:31 PM, Mauro Carvalho Chehab wrote: > Dynamic static allocation is evil, as Kernel stack is too low, and > compilation complains about it on some archs: > > drivers/media/v4l2-core/v4l2-async.c:238:1: warning: > 'v4l2_async_notifier_unregister' uses dynamic stack allocation [enabled by > default] > > Instead, let's enforce a limit for the buffer. > > In this specific case, there's a hard limit imposed by V4L2_MAX_SUBDEVS, > with is currently 128. That means that the buffer size can be up to > 128x8 = 1024 bytes (on a 64bits kernel), with is too big for stack. > > Worse than that, someone could increase it and cause real troubles. > > So, let's use dynamically allocated data, instead. > > Signed-off-by: Mauro Carvalho Chehab > Cc: Guennadi Liakhovetski > --- > drivers/media/v4l2-core/v4l2-async.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c > b/drivers/media/v4l2-core/v4l2-async.c > index c85d69da35bd..071596869036 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -189,12 +189,14 @@ void v4l2_async_notifier_unregister(struct > v4l2_async_notifier *notifier) > struct v4l2_subdev *sd, *tmp; > unsigned int notif_n_subdev = notifier->num_subdevs; > unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); > - struct device *dev[n_subdev]; > + struct device **dev; > int i = 0; > > if (!notifier->v4l2_dev) > return; > > + dev = kmalloc(sizeof(*dev) * n_subdev, GFP_KERNEL); > + No check for dev == NULL? Regards, Hans > mutex_lock(&list_lock); > > list_del(¬ifier->list); > @@ -228,6 +230,7 @@ void v4l2_async_notifier_unregister(struct > v4l2_async_notifier *notifier) > } > put_device(d); > } > + kfree(dev); > > notifier->v4l2_dev = NULL; > > -- 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: [PATCH 04/12] tda18212dd: Support for NXP TDA18212 (DD) silicon tuner
On 04.11.2013 14:49, Maik Broemme wrote: Hi Mauro, Mauro Carvalho Chehab wrote: Em Sun, 03 Nov 2013 19:00:22 +0200 Antti Palosaari escreveu: On 03.11.2013 11:56, Mauro Carvalho Chehab wrote: Hi Maik, Em Sun, 3 Nov 2013 01:31:04 +0100 Maik Broemme escreveu: Added support for the NXP TDA18212 silicon tuner used by recent Digital Devices hardware. This will allow update of ddbridge driver to support newer devices. Signed-off-by: Maik Broemme --- drivers/media/dvb-frontends/Kconfig | 9 + drivers/media/dvb-frontends/Makefile | 1 + drivers/media/dvb-frontends/tda18212dd.c | 934 +++ drivers/media/dvb-frontends/tda18212dd.h | 37 ++ I'm not sure if support for this tuner is not provided already by one of the existing drivers. If not, it is ok to submit a driver for it, but you should just call it as tda18212. I'm c/c Antti, as he worked on some NXP drivers recently, and may be aware if a driver already supports TDA18212. Existing tda18212 driver I made is reverse-engineered and it is used only for Anysee devices, which I am also responsible. That new tuner driver is much more complete than what I have made and single driver is naturally the correct approach. But one thing which annoys nowadays is that endless talking of regressions, which has led to situation it is very hard to make changes drivers that are used for multiple devices and you don't have all those devices to test. It is also OK to remove my old driver and use that, but then I likely lose my possibility to make changes, as I am much dependent on new driver maintainer. That is one existing problem which is seen multiple times during recent years... So I am perfectly happy with two drivers too. I really prefer to have just one driver for each hardware component. That makes life easier at long term. Of course, the driver needs to be properly maintained. I agree. Antti do you know which Anysee device you've used for reverse engineering? All those Anysee models are documented very well in driver file: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/dvb-usb-v2/anysee.c?id=refs/tags/v3.12 There seems to be 3 different Anysee models: "anysee-E7TC(LP)" PCB: 508TC (rev0.6) "anysee-E7T2C(LP)" PCB: 508T2C (rev0.3) "anysee-E7PTC(LP)" PCB: 508PTC (rev0.5) E7TC and E7PTC has TDA18212 inside tuner module DNOD44CDH086A. E7T2C has TDA18212 inside of DNOQ44QCH106A NIM. E7TC and E7PTC are basically same device, just different form. Other is external USB and another internal USB. E7T2C is different device. So if you are going to buy these, then I recommended to order E7T2C and E7TC. regards Antti -- http://palosaari.fi/ -- 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: [PATCH 04/12] tda18212dd: Support for NXP TDA18212 (DD) silicon tuner
Hi Mauro, Mauro Carvalho Chehab wrote: > Em Sun, 03 Nov 2013 19:00:22 +0200 > Antti Palosaari escreveu: > > > On 03.11.2013 11:56, Mauro Carvalho Chehab wrote: > > > Hi Maik, > > > > > > Em Sun, 3 Nov 2013 01:31:04 +0100 > > > Maik Broemme escreveu: > > > > > >> Added support for the NXP TDA18212 silicon tuner used by recent > > >> Digital Devices hardware. This will allow update of ddbridge driver > > >> to support newer devices. > > >> > > >> Signed-off-by: Maik Broemme > > >> --- > > >> drivers/media/dvb-frontends/Kconfig | 9 + > > >> drivers/media/dvb-frontends/Makefile | 1 + > > >> drivers/media/dvb-frontends/tda18212dd.c | 934 > > >> +++ > > >> drivers/media/dvb-frontends/tda18212dd.h | 37 ++ > > > > > > I'm not sure if support for this tuner is not provided already by one of > > > the existing drivers. If not, it is ok to submit a driver for it, but you > > > should just call it as tda18212. > > > > > > I'm c/c Antti, as he worked on some NXP drivers recently, and may be aware > > > if a driver already supports TDA18212. > > > > Existing tda18212 driver I made is reverse-engineered and it is used > > only for Anysee devices, which I am also responsible. That new tuner > > driver is much more complete than what I have made and single driver is > > naturally the correct approach. > > > > But one thing which annoys nowadays is that endless talking of > > regressions, which has led to situation it is very hard to make changes > > drivers that are used for multiple devices and you don't have all those > > devices to test. It is also OK to remove my old driver and use that, but > > then I likely lose my possibility to make changes, as I am much > > dependent on new driver maintainer. That is one existing problem which > > is seen multiple times during recent years... So I am perfectly happy > > with two drivers too. > > I really prefer to have just one driver for each hardware component. That > makes life easier at long term. Of course, the driver needs to be properly > maintained. > I agree. Antti do you know which Anysee device you've used for reverse engineering? > Regards, > Mauro > -- > 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 --Maik -- 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: ivtv 1.4.2/1.4.3 broken in recent kernels?
On 10/19/2013 07:09 PM, Andy Walls wrote: > On Wed, 2013-10-16 at 01:10 +0100, Rajil Saraswat wrote: >> I was finally able to carry out a git bisect. Had to do a git pull on >> a fast internet hooked machine and ftp the files over to the remote >> machine. >> >> I started with 'git bisect bad v2.6.36.4' and 'git bisect good v2.6.35.10'. >> >> And the result was: >> >> 5aa9ae5ed5d449a85fbf7aac3d1fdc241c542a79 is the first bad commit >> commit 5aa9ae5ed5d449a85fbf7aac3d1fdc241c542a79 >> Author: Hans Verkuil >> Date: Sat Apr 24 08:23:53 2010 -0300 >> >> V4L/DVB: wm8775: convert to the new control framework >> >> Signed-off-by: Hans Verkuil >> Signed-off-by: Mauro Carvalho Chehab >> >> :04 04 37847ffe592f255c6a9d9daedaf7bbfd3cd7b055 >> 2f094df6f65d7fb296657619c1ad6f93fe085a75 Mdrivers >> >> I then removed the patch from linux-2.6.36-gentoo-r8 which are gentoo >> sources, and confirmed that video/audio now works fine on v4l2-ctl -d >> /dev/video1 --set-input 4 >> >> I wasnt able to remove the patch in 3.10.7 which is gentoo stable >> kernel. Any idea how can i do that? > > Try applying the following (untested) patch that is made against the > bleeding edge Linux kernel. The test on the mute control state in > wm8775_s_routing() appears to have been inverted in the bad commit you > isolated. Aargh! I'm pretty sure that's the culprit. Man, that's been broken for ages. I'll see if I can test this patch this week. Regards, Hans > Along with '--set-input', you may also want to use v4l2-ctl to exercise > the mute control as well, to see if it works as expected, once this > patch is applied. > > Regards, > Andy > > file: wm8775_s_route_mute_test_inverted.patch > > diff --git a/drivers/media/i2c/wm8775.c b/drivers/media/i2c/wm8775.c > index 3f584a7..bee7946 100644 > --- a/drivers/media/i2c/wm8775.c > +++ b/drivers/media/i2c/wm8775.c > @@ -130,12 +130,10 @@ static int wm8775_s_routing(struct v4l2_subdev *sd, > return -EINVAL; > } > state->input = input; > - if (!v4l2_ctrl_g_ctrl(state->mute)) > + if (v4l2_ctrl_g_ctrl(state->mute)) > return 0; > if (!v4l2_ctrl_g_ctrl(state->vol)) > return 0; > - if (!v4l2_ctrl_g_ctrl(state->bal)) > - return 0; > wm8775_set_audio(sd, 1); > return 0; > } > > -- 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: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
> From: Ricardo Ribalda
>
> vb2_fop_relase does not held the lock although it is modifying the
> queue->owner field.
>
> This could lead to race conditions on the vb2_perform_io function
> when multiple applications are accessing the video device via
> read/write API:
It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>
> [ 308.297741] BUG: unable to handle kernel NULL pointer dereference at
> 0260
> [ 308.297759] IP: [] vb2_perform_fileio+0x372/0x610
> [videobuf2_core]
> [ 308.297794] PGD 159719067 PUD 158119067 PMD 0
> [ 308.297812] Oops: #1 SMP
> [ 308.297826] Modules linked in: qt5023_video videobuf2_dma_sg
> qtec_xform videobuf2_vmalloc videobuf2_memops videobuf2_core
> qtec_white qtec_mem gpio_xilinx qtec_cmosis qtec_pcie fglrx(PO)
> spi_xilinx spi_bitbang qt5023
> [ 308.297888] CPU: 1 PID: 2189 Comm: java Tainted: P O 3.11.0-qtec-standard #1
> [ 308.297919] Hardware name: QTechnology QT5022/QT5022, BIOS
> PM_2.1.0.309 X64 05/23/2013
> [ 308.297952] task: 8801564e1690 ti: 88014dc02000 task.ti:
> 88014dc02000
> [ 308.297962] RIP: 0010:[] []
> vb2_perform_fileio+0x372/0x610 [videobuf2_core]
> [ 308.297985] RSP: 0018:88014dc03df8 EFLAGS: 00010202
> [ 308.297995] RAX: RBX: 880158a23000 RCX:
> dead00100100
> [ 308.298003] RDX: RSI: dead00200200 RDI:
>
> [ 308.298012] RBP: 88014dc03e58 R08: R09:
> 0001
> [ 308.298020] R10: ea00051e8380 R11: 88014dc03fd8 R12:
> 880158a23070
> [ 308.298029] R13: 8801549040b8 R14: 00198000 R15:
> 01887e60
> [ 308.298040] FS: 7f65130d5700() GS:88015ed0()
> knlGS:
> [ 308.298049] CS: 0010 DS: ES: CR0: 80050033
> [ 308.298057] CR2: 0260 CR3: 00015963 CR4:
> 07e0
> [ 308.298064] Stack:
> [ 308.298071] 880156416c00 00198000
> 88010001
> [ 308.298087] 88014dc03f50 810a79ca 00020001
> 880154904718
> [ 308.298101] 880156416c00 00198000 880154904338
> 88014dc03f50
> [ 308.298116] Call Trace:
> [ 308.298143] [] vb2_read+0x14/0x20 [videobuf2_core]
> [ 308.298198] [] vb2_fop_read+0xc4/0x120 [videobuf2_core]
> [ 308.298252] [] v4l2_read+0x7e/0xc0
> [ 308.298296] [] vfs_read+0xa9/0x160
> [ 308.298312] [] SyS_read+0x52/0xb0
> [ 308.298328] [] tracesys+0xd0/0xd5
> [ 308.298335] Code: e5 d6 ff ff 83 3d be 24 00 00 04 89 c2 4c 8b 45 b0
> 44 8b 4d b8 0f 8f 20 02 00 00 85 d2 75 32 83 83 78 03 00 00 01 4b 8b
> 44 c5 48 <8b> 88 60 02 00 00 85 c9 0f 84 b0 00 00 00 8b 40 58 89 c2 41
> 89
> [ 308.298487] RIP [] vb2_perform_fileio+0x372/0x610
> [videobuf2_core]
> [ 308.298507] RSP
> [ 308.298514] CR2: 0260
> [ 308.298526] ---[ end trace e8f01717c96d1e41 ]---
>
> Signed-off-by: Ricardo Ribalda
> ---
> v2: Comments by Sylvester Nawrocki
>
> fimc-capture and fimc-lite where calling vb2_fop_release with the lock held.
> Therefore a new __vb2_fop_release function has been created to be used by
> drivers that overload the release function.
>
> v3: Comments by Sylvester Nawrocki and Mauro Carvalho Chehab
>
> Use vb2_fop_release_locked instead of __vb2_fop_release
>
> v4: Comments by Sylvester Nawrocki
>
> Rename vb2_fop_release_locked to __vb2_fop_release and fix patch format
>
> drivers/media/platform/exynos4-is/fimc-capture.c | 2 +-
> drivers/media/platform/exynos4-is/fimc-lite.c| 2 +-
> drivers/media/v4l2-core/videobuf2-core.c | 23 ++-
> include/media/videobuf2-core.h | 1 +
> 4 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c
> b/drivers/media/platform/exynos4-is/fimc-capture.c
> index fb27ff7..8192fe0 100644
> --- a/drivers/media/platform/exynos4-is/fimc-capture.c
> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
> @@ -549,7 +549,7 @@ static int fimc_capture_release(struct file *file)
> vc->streaming = false;
> }
>
> - ret = vb2_fop_release(file);
> + ret = __vb2_fop_release(file);
>
> if (close) {
> clear_bit(ST_CAPT_BUSY, &fimc->state);
> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c
> b/drivers/media/platform/exynos4-is/fimc-lite.c
> index e5798f7..cbe51cd 100644
> --- a/drivers/media/platform/exynos4-is/fimc-lite.c
> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
> @@ -546,7 +546,7 @@ static int fimc_lite_release(struct file *file)
> mutex_unlock(&entity->parent->graph_mutex);
> }
>
> - vb2_fop_release(file);
> + __vb2_fop_release(file);
> pm_runtime_put(&fimc->pdev->dev);
> clear_bit(ST_FLITE_SUSPENDED, &fimc->state);
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c
> index 59
Re: [PATCH 00/12] DDBridge 0.9.10 driver updates
Em Sun, 03 Nov 2013 13:46:01 +0100 Maik Broemme escreveu: > Hi Mauro, > > Mauro Carvalho Chehab wrote: > > Em Sun, 3 Nov 2013 01:22:35 +0100 > > Maik Broemme escreveu: > > > > > I've updated the current DDBridge to latest version 0.9.10 from Ralph > > > Metzler available at: > > > > > > http://www.metzlerbros.de/dddvb/dddvb-0.9.10.tar.bz2 > > > > > > I've merged the driver to work with current v4l/dvb tree and I will > > > maintain the driver for v4l/dvb in future. > > > > Works for me. > > > > > The coming patch series is > > > the first version and I explicitly want to get feedback and hints if > > > some parts are merged at wrong places, etc... The following changes > > > were made: > > > > > > - MSI enabled by default (some issues left with i2c timeouts) > > > - no support for Digital Devices Octonet > > > - no support for DVB Netstream > > > - removed unused module parameters 'tt' and 'vlan' (used by Octonet) > > > - removed unused registers to cleanup code (might be added later again > > > if needed) > > > > Be sure to not remove any feature that are currently needed for the > > already supported devices to work. > > Of course I won't do. The Octonet and DVB Netstream weren't supported in > current driver. MSI is already supported but was not enabled by default > because the old 0.5 version currently in kernel had some problems with > it. However new one works fine with MSI - at least for me I'm using the > patchset myself already - but needs some further testing. Ok. From the above, it sounded that some features got disabled. Please make it clearer on the next patchset (and move/copy this kind of description to the ddbridge patches). With regards to MSI, we may use a quirk to disable (or enable) support for it, on devices where this is know to have troubles. > > > > > > > The following devices are supported by the driver update: > > > > > > - Octopus DVB adapter > > > - Octopus V3 DVB adapter > > > - Octopus LE DVB adapter > > > - Octopus OEM > > > - Octopus Mini > > > - Cine S2 V6 DVB adapter > > > - Cine S2 V6.5 DVB adapter > > > - Octopus CI > > > - Octopus CI single > > > - DVBCT V6.1 DVB adapter > > > - DVB-C modulator > > > - SaTiX-S2 V3 DVB adapter > > > > > > I might merge the Octonet and DVB Netstream drivers from Ralphs source > > > later once the current committed DDBridge driver updates are merged in > > > mainline. > > > > > > Signed-off-by: Maik Broemme > > > > > > Maik Broemme (12): > > > dvb-frontends: Support for DVB-C2 to DVB frontends > > > tda18271c2dd: Fix description of NXP TDA18271C2 silicon tuner > > > stv0367dd: Support for STV 0367 DVB-C/T (DD) demodulator > > > tda18212dd: Support for NXP TDA18212 (DD) silicon tuner > > > cxd2843: Support for CXD2843ER demodulator for DVB-T/T2/C/C2 > > > dvb-core: export dvb_usercopy and new DVB device constants > > > ddbridge: Updated ddbridge registers > > > ddbridge: Moved i2c interfaces into separate file > > > ddbridge: Support for the Digital Devices Resi DVB-C Modulator card > > > ddbridge: Update ddbridge driver to version 0.9.10 > > > ddbridge: Update ddbridge header for 0.9.10 changes > > > ddbridge: Kconfig and Makefile fixes to build latest ddbridge > > > > > > drivers/media/dvb-core/dvbdev.c |1 > > > drivers/media/dvb-core/dvbdev.h |2 > > > drivers/media/dvb-frontends/Kconfig | 31 > > > drivers/media/dvb-frontends/Makefile |3 > > > drivers/media/dvb-frontends/cxd2843.c| 1647 > > > drivers/media/dvb-frontends/cxd2843.h| 47 > > > drivers/media/dvb-frontends/stv0367dd.c | 2329 ++ > > > drivers/media/dvb-frontends/stv0367dd.h | 48 > > > drivers/media/dvb-frontends/stv0367dd_regs.h | 3442 > > > +++ > > > drivers/media/dvb-frontends/tda18212dd.c | 934 +++ > > > drivers/media/dvb-frontends/tda18212dd.h | 37 > > > drivers/media/pci/ddbridge/Kconfig | 21 > > > drivers/media/pci/ddbridge/Makefile |2 > > > drivers/media/pci/ddbridge/ddbridge-core.c | 3085 > > > +--- > > > drivers/media/pci/ddbridge/ddbridge-i2c.c| 239 + > > > drivers/media/pci/ddbridge/ddbridge-mod.c| 1033 > > > drivers/media/pci/ddbridge/ddbridge-regs.h | 273 +- > > > drivers/media/pci/ddbridge/ddbridge.h| 408 ++- > > > include/uapi/linux/dvb/frontend.h|1 > > > 19 files changed, 12555 insertions(+), 1028 deletions(-) > > > > > > --Maik > > > -- > > > 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 > > > > Thanks for your submission. > > > > I'm seeing this entire patch series as an RFC. There are simply too much > > changes required for us to be able to
Re: [PATCH 02/12] tda18271c2dd: Fix description of NXP TDA18271C2 silicon tuner
Em Sun, 03 Nov 2013 13:17:02 +0100 Maik Broemme escreveu: > Hi Mauro, > > Mauro Carvalho Chehab wrote: > > Em Sun, 3 Nov 2013 01:25:23 +0100 > > Maik Broemme escreveu: > > > > > Added (DD) to NXP TDA18271C2 silicon tuner as this tuner was > > > specifically added for Digital Devices ddbridge driver. > > > > > > Signed-off-by: Maik Broemme > > > --- > > > drivers/media/dvb-frontends/Kconfig | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/dvb-frontends/Kconfig > > > b/drivers/media/dvb-frontends/Kconfig > > > index bddbab4..6f99eb8 100644 > > > --- a/drivers/media/dvb-frontends/Kconfig > > > +++ b/drivers/media/dvb-frontends/Kconfig > > > @@ -48,11 +48,11 @@ config DVB_DRXK > > > Say Y when you want to support this frontend. > > > > > > config DVB_TDA18271C2DD > > > - tristate "NXP TDA18271C2 silicon tuner" > > > + tristate "NXP TDA18271C2 silicon tuner (DD)" > > > depends on DVB_CORE && I2C > > > default m if !MEDIA_SUBDRV_AUTOSELECT > > > help > > > - NXP TDA18271 silicon tuner. > > > + NXP TDA18271 silicon tuner (Digital Devices driver). > > > > > > Say Y when you want to support this tuner. > > > > > > > The better is to use the other tda18271 driver. This one was added as a > > temporary alternative, as the more complete one were lacking some > > features, and were not working with DRX-K. Well, those got fixed already, > > and we now want to get rid of this duplicated driver. > > > > Agree. Probably the tda18271 will need some extensions to work with > ddbridge and I will see what I can do the next days to get it working. Maybe not, but feel free to propose changes there if needed. > > > Regards, > > Mauro > > -- > > > > Cheers, > > Mauro > > -- > > 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 > > --Maik -- Cheers, Mauro -- 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: [PATCH 04/12] tda18212dd: Support for NXP TDA18212 (DD) silicon tuner
Em Sun, 03 Nov 2013 19:00:22 +0200 Antti Palosaari escreveu: > On 03.11.2013 11:56, Mauro Carvalho Chehab wrote: > > Hi Maik, > > > > Em Sun, 3 Nov 2013 01:31:04 +0100 > > Maik Broemme escreveu: > > > >> Added support for the NXP TDA18212 silicon tuner used by recent > >> Digital Devices hardware. This will allow update of ddbridge driver > >> to support newer devices. > >> > >> Signed-off-by: Maik Broemme > >> --- > >> drivers/media/dvb-frontends/Kconfig | 9 + > >> drivers/media/dvb-frontends/Makefile | 1 + > >> drivers/media/dvb-frontends/tda18212dd.c | 934 > >> +++ > >> drivers/media/dvb-frontends/tda18212dd.h | 37 ++ > > > > I'm not sure if support for this tuner is not provided already by one of > > the existing drivers. If not, it is ok to submit a driver for it, but you > > should just call it as tda18212. > > > > I'm c/c Antti, as he worked on some NXP drivers recently, and may be aware > > if a driver already supports TDA18212. > > Existing tda18212 driver I made is reverse-engineered and it is used > only for Anysee devices, which I am also responsible. That new tuner > driver is much more complete than what I have made and single driver is > naturally the correct approach. > > But one thing which annoys nowadays is that endless talking of > regressions, which has led to situation it is very hard to make changes > drivers that are used for multiple devices and you don't have all those > devices to test. It is also OK to remove my old driver and use that, but > then I likely lose my possibility to make changes, as I am much > dependent on new driver maintainer. That is one existing problem which > is seen multiple times during recent years... So I am perfectly happy > with two drivers too. I really prefer to have just one driver for each hardware component. That makes life easier at long term. Of course, the driver needs to be properly maintained. Regards, Mauro -- 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: DVB-C2
Em Sun, 03 Nov 2013 21:21:35 +0100 Ralph Metzler escreveu: > Antti Palosaari writes: > > On 03.11.2013 13:31, Mauro Carvalho Chehab wrote: > > > Em Wed, 23 Oct 2013 00:57:47 +0200 > > > Ralph Metzler escreveu: > > >> I am wondering if anybody looked into API extensions for DVB-C2 yet? > > >> Obviously, we need some more modulations, guard intervals, etc. > > >> even if the demod I use does not actually let me set those (only auto). > > >> > > >> But I do need to set the PLP and slice ID. > > >> I currently set them (8 bit each) by combining them into the 32 bit > > >> stream_id (DTV_STREAM_ID parameter). > > > > > > I don't like the idea of combining them into a single field. One of the > > > reasons is that we may have endianness issues. > > > > > > So, IMHO, the better is to add a new property for slice ID. > > > > I tried to understand what that data slice is. So what I understand, it > > is layer to group PLPs, in order to get one wide OFDM channel as OFDM is > > more efficient when channel bw increases. > > > > So, in order to tune "stream" channel on DVB-C2 system, you *must* know > > (in a order from radio channel to upper layers): > > frequency > > bandwidth > > slice ID > > PLP ID > > > > Is that right? > > Yes, if you do not want to parse L1 data you need the frequency of the slice, > bandwidth, slice ID and PLP ID. > If you parse L1 data, you do not need the slice ID because the PLP should be > unique in one channel. > > > I wonder if PLP IDs are defined so that there could not be overlapping > > PLP IDs in a system... But if not, then defining slice ID is likely > > needed. And if and when slice ID is needed to know before PLP ID, it is > > even impossible to resolve slice ID from PLP ID. > > See above, you can resolve it, but then you need to get the L1 data. > But PLPs can even be spread over several slices to get higher bandwidth > for one PLP. This is probably not used for broadcast TV though. You will > also need one tuner/demod per slice then. > > So, basically you only need any frequency for the "channel" (can be spread > over > up to 450MHz, but avoid notches) and the bandwith. > Tune until a L1 lock, get L1 data from demod (up to 4 KB), parse for the PLP > id you want, get the corresponding slice (or slices), tune to the slice > frequency > with slice ID set and PLP id set and wait for a full lock ... Ok, then it is really better to have slice as a separate property, and to document the above procedure to tune into a slice at a DVB-C2 section to be added to the DocBook. We'll need to define a value for slice to mean "don't bind to any slice". Maybe 2^32-1. With regards to the slice property, it would be possible to let it have multiple values (just like we do with ENUM_DELSYS). Not sure if this makes sense or not. Regards, Mauro -- 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: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder
On 11/04/2013 12:29 PM, Sylwester Nawrocki wrote: > Sorry, I missed to reply to this e-mail. > > On 04/11/13 11:57, Hans Verkuil wrote: >> Hi John, >> >> On 10/18/2013 02:03 AM, John Sheu wrote: >>> On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki >>> wrote: On 10/18/2013 12:25 AM, John Sheu wrote: > On Thu, Oct 17, 2013 at 2:46 PM, John Sheu wrote: >>> Sweet. Thanks for spelling things out explicitly like this. The fact >>> that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness" >>> when used in a m2m device is definitely all sorts of confusing. > > Just to double-check: this means that we have another bug. > > In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop, > we "simulate" a G_CROP or S_CROP, if the entry point is not defined > for that device, by doing the appropriate S_SELECTION or G_SELECTION. > Unfortunately then, for M2M this is incorrect then. > > Am I reading this right? You are right, John. Firstly a clear specification needs to be written, something along the lines of Tomasz's explanation in this thread, once all agree to that the ioctl code should be corrected if needed. >> >> I don't understand the problem here. The specification has always been clear: >> s_crop for output devices equals s_selection(V4L2_SEL_TGT_COMPOSE_ACTIVE). >> >> Drivers should only implement the selection API and the v4l2 core will do the >> correct translation of s_crop. >> >> Yes, I know it's weird, but that's the way the crop API was defined way back >> and that's what should be used. >> >> My advise: forget about s_crop and just implement s_selection. >> It seems this [1] RFC is an answer exactly to your question. Exact meaning of the selection ioctl is only part of the problem, also interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec. [1] http://www.spinics.net/lists/linux-media/msg56078.html >>> >>> I think the "inversion" behavior is confusing and we should remove it >>> if at all possible. >>> >>> I took a look through all the drivers in linux-media which implement >>> S_CROP. Most of them are either OUTPUT or CAPTURE/OVERLAY-only. Of >>> those that aren't: >>> >>> * drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts >>> both >>> OUTPUT and CAPTURE queues in S_CROP, but they both configure the same >>> state. >>> No functional difference. >> >> Yeah, I guess that's a driver bug. This is a very old driver that originally >> used a custom API for these things, and since no selection API existed at the >> time it was just mapped to the crop API. Eventually it should use the >> selection >> API as well and do it correctly. But to be honest, nobody cares about this >> driver :-) >> >> It is however on my TODO list of drivers that need to be converted to the >> latest >> frameworks, so I might fix this eventually. >> >>> * drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't >>> specify >>> the queue, but is a CAPTURE-only device. Probably an (unrelated) bug. >> >> Yes, that's a driver bug. It should check the buffer type. >> >>> * drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver >>> with both OUTPUT and CAPTURE queues. It has uninverted behavior: >>> S_CROP(CAPTURE) -> source >>> S_CROP(OUTPUT) -> destination > > No, that's not true. It seems you got it wrong, cropping in case of this m2m > driver works like this: > > S_CROP(OUTPUT) -> source (from HW POV) > S_CROP(CAPTURE) -> destination > > I.e. exactly same way as for s5p-g2d, for which it somehow was a reference > implementation. > >> This is the wrong behavior. >> >>> * drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with >>> both >>> OUTPUT and CAPTURE queues. It has inverted behavior: >>> S_CROP(CAPTURE) -> destination >>> S_CROP(OUTPUT) -> source >> >> This is the correct behavior. >> >>> >>> The last two points above are the most relevant. So we already have >>> at least one broken driver, regardless of whether we allow inversion >>> or not; I'd think this grants us a certain freedom to redefine the >>> specification to be more logical. Can we do this please? >> >> No. The fimc-m2m.c driver needs to be fixed. That's the broken one. > > It's not broken, it's easy to figure out if you actually look at the > code, e.g. fimc_m2m_try_crop() function. I did look at the code, but it is quite possible I got confused. Let me be precise as to what should happen, and you can check whether that's what is actually done in the fimc and g2d drivers. For V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: Say that the mem2mem hardware creates a 640x480 picture. If VIDIOC_S_CROP was called for V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE with a rectangle of 320x240@160x120, then the DMA engine will only transfer the center 320x240 rectangle to memory. This means that S_FMT needs to provide a buffer size
[PATCH] media platform drivers: Fix build on cris arch
On cris arch, the functions below aren't defined: drivers/media/platform/sh_veu.c: In function 'sh_veu_reg_read': drivers/media/platform/sh_veu.c:228:2: error: implicit declaration of function 'ioread32' [-Werror=implicit-function-declaration] drivers/media/platform/sh_veu.c: In function 'sh_veu_reg_write': drivers/media/platform/sh_veu.c:234:2: error: implicit declaration of function 'iowrite32' [-Werror=implicit-function-declaration] drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_read': drivers/media/platform/vsp1/vsp1.h:66:2: error: implicit declaration of function 'ioread32' [-Werror=implicit-function-declaration] drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_write': drivers/media/platform/vsp1/vsp1.h:71:2: error: implicit declaration of function 'iowrite32' [-Werror=implicit-function-declaration] drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_read': drivers/media/platform/vsp1/vsp1.h:66:2: error: implicit declaration of function 'ioread32' [-Werror=implicit-function-declaration] drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_write': drivers/media/platform/vsp1/vsp1.h:71:2: error: implicit declaration of function 'iowrite32' [-Werror=implicit-function-declaration] drivers/media/platform/soc_camera/rcar_vin.c: In function 'rcar_vin_setup': drivers/media/platform/soc_camera/rcar_vin.c:284:3: error: implicit declaration of function 'iowrite32' [-Werror=implicit-function-declaration] drivers/media/platform/soc_camera/rcar_vin.c: In function 'rcar_vin_request_capture_stop': drivers/media/platform/soc_camera/rcar_vin.c:353:2: error: implicit declaration of function 'ioread32' [-Werror=implicit-function-declaration] Yet, they're available, as CONFIG_GENERIC_IOMAP is defined. What happens is that asm/io.h was not including asm-generic/iomap.h. Suggested-by: Ben Hutchings Signed-off-by: Mauro Carvalho Chehab Cc: sta...@vger.kernel.org --- arch/cris/include/asm/io.h | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/cris/include/asm/io.h b/arch/cris/include/asm/io.h index 5d3047e5563b..4353cf239a13 100644 --- a/arch/cris/include/asm/io.h +++ b/arch/cris/include/asm/io.h @@ -3,6 +3,7 @@ #include/* for __va, __pa */ #include +#include #include struct cris_io_operations -- 1.8.3.1 -- 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: [PATCH] media: Add BCM2048 radio driver
Hi Pali, On 10/26/2013 10:45 PM, Pali Rohár wrote: > On Saturday 26 October 2013 22:22:09 Hans Verkuil wrote: >>> Hans, so can it be added to drivers/staging/media tree? >> >> Yes, that is an option. It's up to you to decide what you >> want. Note that if no cleanup work is done on the staging >> driver for a long time, then it can be removed again. >> >> Regards, >> >> Hans >> > > Ok, so if you can add it to staging tree. When driver will be in > mainline other developers can look at it too. Now when driver is > hidden, nobody know where to find it... You can see how upstream > development for Nokia N900 HW going on: http://elinux.org/N900 > Please check my tree: http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/bcm If you're OK, then I'll queue it for 3.14 (it's too late for 3.13). Regards, Hans -- 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: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder
Sorry, I missed to reply to this e-mail. On 04/11/13 11:57, Hans Verkuil wrote: > Hi John, > > On 10/18/2013 02:03 AM, John Sheu wrote: >> On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki >> wrote: >>> On 10/18/2013 12:25 AM, John Sheu wrote: On Thu, Oct 17, 2013 at 2:46 PM, John Sheu wrote: >> Sweet. Thanks for spelling things out explicitly like this. The fact >> that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness" >> when used in a m2m device is definitely all sorts of confusing. Just to double-check: this means that we have another bug. In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop, we "simulate" a G_CROP or S_CROP, if the entry point is not defined for that device, by doing the appropriate S_SELECTION or G_SELECTION. Unfortunately then, for M2M this is incorrect then. Am I reading this right? >>> >>> You are right, John. Firstly a clear specification needs to be written, >>> something along the lines of Tomasz's explanation in this thread, once >>> all agree to that the ioctl code should be corrected if needed. > > I don't understand the problem here. The specification has always been clear: > s_crop for output devices equals s_selection(V4L2_SEL_TGT_COMPOSE_ACTIVE). > > Drivers should only implement the selection API and the v4l2 core will do the > correct translation of s_crop. > > Yes, I know it's weird, but that's the way the crop API was defined way back > and that's what should be used. > > My advise: forget about s_crop and just implement s_selection. > >>> >>> It seems this [1] RFC is an answer exactly to your question. >>> >>> Exact meaning of the selection ioctl is only part of the problem, also >>> interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec. >>> >>> [1] http://www.spinics.net/lists/linux-media/msg56078.html >> >> I think the "inversion" behavior is confusing and we should remove it >> if at all possible. >> >> I took a look through all the drivers in linux-media which implement >> S_CROP. Most of them are either OUTPUT or CAPTURE/OVERLAY-only. Of >> those that aren't: >> >> * drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts >> both >> OUTPUT and CAPTURE queues in S_CROP, but they both configure the same >> state. >> No functional difference. > > Yeah, I guess that's a driver bug. This is a very old driver that originally > used a custom API for these things, and since no selection API existed at the > time it was just mapped to the crop API. Eventually it should use the > selection > API as well and do it correctly. But to be honest, nobody cares about this > driver :-) > > It is however on my TODO list of drivers that need to be converted to the > latest > frameworks, so I might fix this eventually. > >> * drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't specify >> the queue, but is a CAPTURE-only device. Probably an (unrelated) bug. > > Yes, that's a driver bug. It should check the buffer type. > >> * drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver >> with both OUTPUT and CAPTURE queues. It has uninverted behavior: >> S_CROP(CAPTURE) -> source >> S_CROP(OUTPUT) -> destination No, that's not true. It seems you got it wrong, cropping in case of this m2m driver works like this: S_CROP(OUTPUT) -> source (from HW POV) S_CROP(CAPTURE) -> destination I.e. exactly same way as for s5p-g2d, for which it somehow was a reference implementation. > This is the wrong behavior. > >> * drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with >> both >> OUTPUT and CAPTURE queues. It has inverted behavior: >> S_CROP(CAPTURE) -> destination >> S_CROP(OUTPUT) -> source > > This is the correct behavior. > >> >> The last two points above are the most relevant. So we already have >> at least one broken driver, regardless of whether we allow inversion >> or not; I'd think this grants us a certain freedom to redefine the >> specification to be more logical. Can we do this please? > > No. The fimc-m2m.c driver needs to be fixed. That's the broken one. It's not broken, it's easy to figure out if you actually look at the code, e.g. fimc_m2m_try_crop() function. -- Regards, Sylwester -- Sylwester Nawrocki Samsung R&D Institute Poland -- 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: [PATCHv2 07/29] platform drivers: Fix build on cris and frv archs
Hi Ben, Em Mon, 04 Nov 2013 04:03:10 + Ben Hutchings escreveu: > On Sat, 2013-11-02 at 11:31 -0200, Mauro Carvalho Chehab wrote: > > On cris and frv archs, the functions below aren't defined: > > drivers/media/platform/sh_veu.c: In function 'sh_veu_reg_read': > > drivers/media/platform/sh_veu.c:228:2: error: implicit declaration of > > function 'ioread32' [-Werror=implicit-function-declaration] > > drivers/media/platform/sh_veu.c: In function 'sh_veu_reg_write': > > drivers/media/platform/sh_veu.c:234:2: error: implicit declaration of > > function 'iowrite32' [-Werror=implicit-function-declaration] > > drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_read': > > drivers/media/platform/vsp1/vsp1.h:66:2: error: implicit declaration of > > function 'ioread32' [-Werror=implicit-function-declaration] > > drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_write': > > drivers/media/platform/vsp1/vsp1.h:71:2: error: implicit declaration of > > function 'iowrite32' [-Werror=implicit-function-declaration] > > drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_read': > > drivers/media/platform/vsp1/vsp1.h:66:2: error: implicit declaration of > > function 'ioread32' [-Werror=implicit-function-declaration] > > drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_write': > > drivers/media/platform/vsp1/vsp1.h:71:2: error: implicit declaration of > > function 'iowrite32' [-Werror=implicit-function-declaration] > > drivers/media/platform/soc_camera/rcar_vin.c: In function > > 'rcar_vin_setup': > > drivers/media/platform/soc_camera/rcar_vin.c:284:3: error: implicit > > declaration of function 'iowrite32' [-Werror=implicit-function-declaration] > > drivers/media/platform/soc_camera/rcar_vin.c: In function > > 'rcar_vin_request_capture_stop': > > drivers/media/platform/soc_camera/rcar_vin.c:353:2: error: implicit > > declaration of function 'ioread32' [-Werror=implicit-function-declaration] > > > > While this is not fixed, remove those 3 drivers from building on > > those archs. > [...] > > Well where does this stop? There will be many other drivers that are > broken if those functions are missing, and there's going to be a lot of > churn if we disable them all and then reenable when the architecture > headers are fixed. > > cris selects the generic implementations (CONFIG_GENERIC_IOMAP) but I > think arch/cris/include/asm/io.h is missing > #include . Thanks for your review! Yes, adding it is enough to get rid of the errors on cris. > frv defines these functions inline in arch/frv/include/asm/io.h so I > don't know what the problem is there. One of the drivers weren't including . Probably, this were indirectly included on other archs. That's why it failed only on frv. The enclosed patch should fix for both: platform drivers: Fix build on cris and frv archs On cris and frv archs, compilation fails due to the lack of ioread32/iowrite32: drivers/media/platform/sh_veu.c: In function 'sh_veu_reg_read': drivers/media/platform/sh_veu.c:228:2: error: implicit declaration of function 'ioread32' [-Werror=implicit-function-declaration] drivers/media/platform/sh_veu.c: In function 'sh_veu_reg_write': drivers/media/platform/sh_veu.c:234:2: error: implicit declaration of function 'iowrite32' [-Werror=implicit-function-declaration] drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_read': drivers/media/platform/vsp1/vsp1.h:66:2: error: implicit declaration of function 'ioread32' [-Werror=implicit-function-declaration] drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_write': drivers/media/platform/vsp1/vsp1.h:71:2: error: implicit declaration of function 'iowrite32' [-Werror=implicit-function-declaration] drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_read': drivers/media/platform/vsp1/vsp1.h:66:2: error: implicit declaration of function 'ioread32' [-Werror=implicit-function-declaration] drivers/media/platform/vsp1/vsp1.h: In function 'vsp1_write': drivers/media/platform/vsp1/vsp1.h:71:2: error: implicit declaration of function 'iowrite32' [-Werror=implicit-function-declaration] drivers/media/platform/soc_camera/rcar_vin.c: In function 'rcar_vin_setup': drivers/media/platform/soc_camera/rcar_vin.c:284:3: error: implicit declaration of function 'iowrite32' [-Werror=implicit-function-declaration] drivers/media/platform/soc_camera/rcar_vin.c: In function 'rcar_vin_request_capture_stop': drivers/media/platform/soc_camera/rcar_vin.c:353:2: error: implicit declaration of function 'ioread32' [-Werror=implicit-function-declaration] On cris, the reason is because asm-generic/iomap.h is not included on asm/io.h. On frv, the reason is because linux/io.h is not included on rcar_vin.c. Fix both issues. Signed-off-by: Mauro Carvalho Chehab PS.: I'll split this patch on two separate ones, sending
Re: [PATCH 06/24] V4L2: add a common V4L2 subdevice platform data type
Hi Guennadi,
Sorry for the delay, I only saw this today while I was going through my
mail backlog.
On 10/17/2013 08:24 PM, Guennadi Liakhovetski wrote:
> Hi Hans
>
> Sorry for reviving this old thread. I was going to resubmit a part of
> those patches for mainlining and then I found this your comment, which I
> didn't reply to back then.
>
> On Fri, 19 Apr 2013, Hans Verkuil wrote:
>
>> On Fri April 19 2013 09:48:27 Guennadi Liakhovetski wrote:
>>> Hi Hans
>>>
>>> Thanks for reviewing.
>>>
>>> On Fri, 19 Apr 2013, Hans Verkuil wrote:
>>>
On Thu April 18 2013 23:35:27 Guennadi Liakhovetski wrote:
> This struct shall be used by subdevice drivers to pass per-subdevice data,
> e.g. power supplies, to generic V4L2 methods, at the same time allowing
> optional host-specific extensions via the host_priv pointer. To avoid
> having to pass two pointers to those methods, add a pointer to this new
> struct to struct v4l2_subdev.
>
> Signed-off-by: Guennadi Liakhovetski
> ---
> include/media/v4l2-subdev.h | 13 +
> 1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index eb91366..b15c6e0 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -561,6 +561,17 @@ struct v4l2_subdev_internal_ops {
> /* Set this flag if this subdev generates events. */
> #define V4L2_SUBDEV_FL_HAS_EVENTS(1U << 3)
>
> +struct regulator_bulk_data;
> +
> +struct v4l2_subdev_platform_data {
> + /* Optional regulators uset to power on/off the subdevice */
> + struct regulator_bulk_data *regulators;
> + int num_regulators;
> +
> + /* Per-subdevice data, specific for a certain video host device */
> + void *host_priv;
> +};
> +
> /* Each instance of a subdev driver should create this struct, either
> stand-alone or embedded in a larger struct.
> */
> @@ -589,6 +600,8 @@ struct v4l2_subdev {
> /* pointer to the physical device */
> struct device *dev;
> struct v4l2_async_subdev_list asdl;
> + /* common part of subdevice platform data */
> + struct v4l2_subdev_platform_data *pdata;
> };
>
> static inline struct v4l2_subdev *v4l2_async_to_subdev(
>
Sorry, this is the wrong approach.
This is data that is of no use to the subdev driver itself. It really is
v4l2_subdev_host_platform_data, and as such must be maintained by the
bridge
driver.
>>>
>>> I don't think so. It has been discussed and agreed upon, that only
>>> subdevice drivers know when to switch power on and off, because only they
>>> know when they need to access the hardware. So, they have to manage
>>> regulators. In fact, those regulators supply power to respective
>>> subdevices, e.g. a camera sensor. Why should the bridge driver manage
>>> them? The V4L2 core can (and probably should) provide helper functions for
>>> that, like soc-camera currently does, but in any case it's the subdevice
>>> driver, that has to call them.
>>
>> Ah, OK. I just realized I missed some context there. I didn't pay much
>> attention to the regulator discussions since that's not my area of expertise.
>>
>> In that case my only comment is to drop the host_priv pointer since that just
>> duplicates v4l2_get/set_subdev_hostdata().
>
> I think it's different. This is _platform_ data, whereas struct
> v4l2_subdev::host_priv is more like run-time data.
You mean subdev_hostdata() instead of host_priv, right?
> This field is for the
> per-subdevice host-specific data, that the platform has to pass to the
> host driver. In the soc-camera case this is the largest bulk of the data,
> that platforms currently pass to the soc-camera framework in the host part
> of struct soc_camera_link. This data most importantly includes I2C
> information. Yes, this _could_ be passed to soc-camera separately from the
> host driver, but that would involve quite some refactoring of the "legacy"
> synchronous probing mode, which I'd like to avoid if possible. This won't
> be used in the asynchronous case. Do you think we can keep this pointer in
> this sruct? We could rename it to avoid confusion with the field, that you
> told about.
I'm wondering: do we need host_priv at all? Can't drivers use container_of to
go from struct v4l2_subdev_platform_data to the platform_data struct containing
v4l2_subdev_platform_data?
That would be a cleaner solution IMHO. Using host_priv basically forces you
to split up the platform_data into two parts, and a void pointer isn't very
type-safe.
Regards,
Hans
--
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: [PATCH] v4l: omap3isp: Move code out of mutex-protected section
Hi Laurent, Thanks for the patch. On Mon, Nov 04, 2013 at 11:07:48AM +0100, Laurent Pinchart wrote: > The pad::get_fmt call must be protected by a mutex, but preparing its > arguments doesn't need to be. Move the non-critical code out of the > mutex-protected section. > > Signed-off-by: Laurent Pinchart > --- > drivers/media/platform/omap3isp/ispvideo.c | 7 ++- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/platform/omap3isp/ispvideo.c > b/drivers/media/platform/omap3isp/ispvideo.c > index a908d00..f6304bb 100644 > --- a/drivers/media/platform/omap3isp/ispvideo.c > +++ b/drivers/media/platform/omap3isp/ispvideo.c > @@ -339,14 +339,11 @@ __isp_video_get_format(struct isp_video *video, struct > v4l2_format *format) > if (subdev == NULL) > return -EINVAL; > > - mutex_lock(&video->mutex); > - > fmt.pad = pad; > fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > - ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); > - if (ret == -ENOIOCTLCMD) > - ret = -EINVAL; By removing these lines, you're also returning -ENOIOCTLCMD to the caller. Is this intentional? That return value will end up to at least one place which seems to be isp_video_streamon() and, unless I'm mistaken, will cause ioctl(VIDIOC_STREAMON) also return ENOTTY. > + mutex_lock(&video->mutex); > + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); > mutex_unlock(&video->mutex); > > if (ret) -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- 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: [PATCHv2 28/29] mxl111sf: Don't use dynamic static allocation
On Mon, Nov 4, 2013 at 5:58 AM, Mauro Carvalho Chehab
wrote:
> Em Sun, 3 Nov 2013 19:50:02 -0500
> Michael Krufky escreveu:
>
>> On Sat, Nov 2, 2013 at 9:31 AM, Mauro Carvalho Chehab
>> wrote:
>> > Dynamic static allocation is evil, as Kernel stack is too low, and
>> > compilation complains about it on some archs:
>> >
>> > drivers/media/usb/dvb-usb-v2/mxl111sf.c:74:1: warning:
>> > 'mxl111sf_ctrl_msg' uses dynamic stack allocation [enabled by default]
>> >
>> > Instead, let's enforce a limit for the buffer to be the max size of
>> > a control URB payload data (80 bytes).
>> >
>> > Signed-off-by: Mauro Carvalho Chehab
>> > Cc: Michael Krufky
>> > ---
>> > drivers/media/usb/dvb-usb-v2/mxl111sf.c | 7 ++-
>> > 1 file changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/media/usb/dvb-usb-v2/mxl111sf.c
>> > b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
>> > index e97964ef7f56..6538fd54c84e 100644
>> > --- a/drivers/media/usb/dvb-usb-v2/mxl111sf.c
>> > +++ b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
>> > @@ -57,7 +57,12 @@ int mxl111sf_ctrl_msg(struct dvb_usb_device *d,
>> > {
>> > int wo = (rbuf == NULL || rlen == 0); /* write-only */
>> > int ret;
>> > - u8 sndbuf[1+wlen];
>> > + u8 sndbuf[80];
>> > +
>> > + if (1 + wlen > sizeof(sndbuf)) {
>> > + pr_warn("%s: len=%d is too big!\n", __func__, wlen);
>> > + return -EREMOTEIO;
>> > + }
>> >
>> > pr_debug("%s(wlen = %d, rlen = %d)\n", __func__, wlen, rlen);
>> >
>> > --
>> > 1.8.3.1
>> >
>> > --
>> > 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
>>
>> I don't really love this, but I see your point. You're right - this
>> needs to be fixed.
>>
>> AFAIK, the largest transfer the driver ever does is 61 bytes, but I'd
>> have to double check to be sure...
>>
>> Is there a #define'd macro that we could place there instead of the
>> hardcoded '80' ? I really don't like the number '80' there,
>> *especially* not without a comment explaining it. Is 80 even the
>> maximum size of control urb payload data? Are you sure it isn't 64?
>>
>> http://wiki.osdev.org/Universal_Serial_Bus#Maximum_Data_Payload_Size
>>
>> ...as per the article above, we should be able to read the actual
>> maximum size from the USB endpoint itself, but then again, that would
>> leave us with another dynamic static allocation.
>
> There's one driver using 80 bytes for payload (tm6000). Anyway,
> I double-checked at USB 2.0 specification: the max size for
> control endpoints is 64 bytes for full-speed devices:
>
> "All Host Controllers are required to have support for 8-, 16-, 32-,
> and 64-byte maximum data payload sizes
> for full-speed control endpoints, only 8-byte maximum data payload
> sizes for low-speed control endpoints,
> and only 64-byte maximum data payload size for high-speed control
> endpoints. No Host Controller is
> required to support larger or smaller maximum data payload sizes."
>
> Source: USB revision 2.0 - chapter 5.5.3 Control Transfer Packet Size
> Constraints
> http://www.usb.org/developers/docs/usb_20_070113.zip
>
> So, except for devices that violates that, the worse case scenario is
> 64 bytes.
>
> It should be noticed that the I2C bus could use a different limit,
> so, on PCI devices, in theory, it would be possible to use a larger
> window.
>
> Yet, I doubt that any sane tuner/frontend design would require a
> packet size bigger than the max size supported by the USB bus, as
> that would limit their usage. Also, most (if not all) of those
> tuners/frontends were added due to USB devices, anyway.
>
>>
>> How about if we kzalloc the buffer instead? (maybe not - that isn't
>> very efficient either)
>
> Seems an overkill to me to create/delete a buffer for every single I2C
> transfer. Of course, a latter patch could optimize the buffer size to
> match what's supported by the hardware, or to use a pre-allocated buffer,
> but this is out of my scope: all I want is to get rid of dynamically
> allocated buffers. I don't intend to read all those datasheets and
> optimize each of those drivers, especially since I may not have the
> hardware here for testing.
>
>> If it has to be a static allocation (and it probably should be),
>> please #define the size rather than sticking in the number 80.
>
> Ok.
>
>> This feedback applies to your entire "Don't use dynamic static
>> allocation" patch series. Please don't merge those without at least
>> #define'ing the size value and adding an appropriate inline comment to
>> explain why the maximum is defined as such.
>
> Well, a comment is provided already at the commit message. I don't
> see any need to overbloat the code with a comment like that. In any
> case, if I were to add a comment, it would be something like
Re: [PATCH v2 00/18] OMAP4 ISS driver
On 11/04/2013 10:41 AM, Hans Verkuil wrote: > Hi Laurent, > > For this whole patch series: > > Acked-by: Hans Verkuil And also for patches 19-22 :-) Thanks! Hans > > I strongly recommend that you add another patch adding the s_input function. > It's trivial to do and it will make v4l2-compliance happy :-) > > Fixing the issue with iss_video_try_format() and a missing get_fmt op is > nice-to-have, > but can be done later. > > Regards, > > Hans > > On 11/04/2013 01:06 AM, Laurent Pinchart wrote: >> Hello, >> >> This is the second version of the OMAP4 ISS driver patches for inclusion in >> the >> mainline kernel. I've addressed most of the comments received on the first >> version (some of them are still being discussed) in additional patches, >> except >> for the file path updates in the documentation that have been squashed with >> patch 01/18. >> >> The OMAP4 ISS driver has lived out of tree for more than two years now. This >> situation is both sad and resource-wasting, as the driver has been used (and >> thus) hacked since then with nowhere to send patches to. Time has come to fix >> the problem. >> >> As the code is mostly, but not quite ready for prime time, I'd like to >> request >> its addition to drivers/staging/. I've added a (pretty small) TODO file and I >> commit to cleaning up the code and get it to drivers/media/ where it belongs. >> >> I've split the driver in six patches to avoid getting caught in vger's size >> and to make review slightly easier. Sergio Aguirre is the driver author (huge >> thanks for that!), I've thus kept his authorship on patches 1/6 to 5/6. >> Beside >> minimal changes to make the code compile on v3.12 and updates the file paths >> in >> the documentation I've kept Sergio's code unmodified. >> >> I don't have much else to add here, let's get this beast to mainline and >> allow >> other developers to use the driver and contribute patches. Given that v3.12 >> has >> just been released I'm fine with pushing this series back to v3.13. >> >> Laurent Pinchart (13): >> v4l: omap4iss: Add support for OMAP4 camera interface - Build system >> v4l: omap4iss: Don't use v4l2_g_ext_ctrls() internally >> v4l: omap4iss: Move common code out of switch...case >> v4l: omap4iss: Report device caps in response to VIDIOC_QUERYCAP >> v4l: omap4iss: Remove iss_video streaming field >> v4l: omap4iss: Set the vb2 timestamp type >> v4l: omap4iss: Remove duplicate video_is_registered() check >> v4l: omap4iss: Remove unneeded status variable >> v4l: omap4iss: Replace udelay/msleep with usleep_range >> v4l: omap4iss: Make omap4iss_isp_subclk_(en|dis)able() functions void >> v4l: omap4iss: Make loop counters unsigned where appropriate >> v4l: omap4iss: Don't initialize fields to 0 manually >> v4l: omap4iss: Simplify error paths >> >> Sergio Aguirre (5): >> v4l: omap4iss: Add support for OMAP4 camera interface - Core >> v4l: omap4iss: Add support for OMAP4 camera interface - Video devices >> v4l: omap4iss: Add support for OMAP4 camera interface - CSI receivers >> v4l: omap4iss: Add support for OMAP4 camera interface - IPIPE(IF) >> v4l: omap4iss: Add support for OMAP4 camera interface - Resizer >> >> Documentation/video4linux/omap4_camera.txt | 60 ++ >> drivers/staging/media/Kconfig|2 + >> drivers/staging/media/Makefile |1 + >> drivers/staging/media/omap4iss/Kconfig | 12 + >> drivers/staging/media/omap4iss/Makefile |6 + >> drivers/staging/media/omap4iss/TODO |4 + >> drivers/staging/media/omap4iss/iss.c | 1462 >> ++ >> drivers/staging/media/omap4iss/iss.h | 153 +++ >> drivers/staging/media/omap4iss/iss_csi2.c| 1368 >> drivers/staging/media/omap4iss/iss_csi2.h| 156 +++ >> drivers/staging/media/omap4iss/iss_csiphy.c | 278 + >> drivers/staging/media/omap4iss/iss_csiphy.h | 51 + >> drivers/staging/media/omap4iss/iss_ipipe.c | 581 ++ >> drivers/staging/media/omap4iss/iss_ipipe.h | 67 ++ >> drivers/staging/media/omap4iss/iss_ipipeif.c | 847 +++ >> drivers/staging/media/omap4iss/iss_ipipeif.h | 92 ++ >> drivers/staging/media/omap4iss/iss_regs.h| 883 >> drivers/staging/media/omap4iss/iss_resizer.c | 905 >> drivers/staging/media/omap4iss/iss_resizer.h | 75 ++ >> drivers/staging/media/omap4iss/iss_video.c | 1124 >> drivers/staging/media/omap4iss/iss_video.h | 198 >> include/media/omap4iss.h | 65 ++ >> 22 files changed, 8390 insertions(+) >> create mode 100644 Documentation/video4linux/omap4_camera.txt >> create mode 100644 drivers/staging/media/omap4iss/Kconfig >> create mode 100644 drivers/staging/media/omap4iss/Makefile >> create mode 100644 drivers/staging/media/omap4iss/TODO >> create mode 100644 drivers/staging/media/omap4iss/iss.c >>
Re: [PATCHv2 28/29] mxl111sf: Don't use dynamic static allocation
Em Sun, 3 Nov 2013 19:50:02 -0500
Michael Krufky escreveu:
> On Sat, Nov 2, 2013 at 9:31 AM, Mauro Carvalho Chehab
> wrote:
> > Dynamic static allocation is evil, as Kernel stack is too low, and
> > compilation complains about it on some archs:
> >
> > drivers/media/usb/dvb-usb-v2/mxl111sf.c:74:1: warning:
> > 'mxl111sf_ctrl_msg' uses dynamic stack allocation [enabled by default]
> >
> > Instead, let's enforce a limit for the buffer to be the max size of
> > a control URB payload data (80 bytes).
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > Cc: Michael Krufky
> > ---
> > drivers/media/usb/dvb-usb-v2/mxl111sf.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/dvb-usb-v2/mxl111sf.c
> > b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
> > index e97964ef7f56..6538fd54c84e 100644
> > --- a/drivers/media/usb/dvb-usb-v2/mxl111sf.c
> > +++ b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
> > @@ -57,7 +57,12 @@ int mxl111sf_ctrl_msg(struct dvb_usb_device *d,
> > {
> > int wo = (rbuf == NULL || rlen == 0); /* write-only */
> > int ret;
> > - u8 sndbuf[1+wlen];
> > + u8 sndbuf[80];
> > +
> > + if (1 + wlen > sizeof(sndbuf)) {
> > + pr_warn("%s: len=%d is too big!\n", __func__, wlen);
> > + return -EREMOTEIO;
> > + }
> >
> > pr_debug("%s(wlen = %d, rlen = %d)\n", __func__, wlen, rlen);
> >
> > --
> > 1.8.3.1
> >
> > --
> > 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
>
> I don't really love this, but I see your point. You're right - this
> needs to be fixed.
>
> AFAIK, the largest transfer the driver ever does is 61 bytes, but I'd
> have to double check to be sure...
>
> Is there a #define'd macro that we could place there instead of the
> hardcoded '80' ? I really don't like the number '80' there,
> *especially* not without a comment explaining it. Is 80 even the
> maximum size of control urb payload data? Are you sure it isn't 64?
>
> http://wiki.osdev.org/Universal_Serial_Bus#Maximum_Data_Payload_Size
>
> ...as per the article above, we should be able to read the actual
> maximum size from the USB endpoint itself, but then again, that would
> leave us with another dynamic static allocation.
There's one driver using 80 bytes for payload (tm6000). Anyway,
I double-checked at USB 2.0 specification: the max size for
control endpoints is 64 bytes for full-speed devices:
"All Host Controllers are required to have support for 8-, 16-, 32-,
and 64-byte maximum data payload sizes
for full-speed control endpoints, only 8-byte maximum data payload
sizes for low-speed control endpoints,
and only 64-byte maximum data payload size for high-speed control
endpoints. No Host Controller is
required to support larger or smaller maximum data payload sizes."
Source: USB revision 2.0 - chapter 5.5.3 Control Transfer Packet Size
Constraints
http://www.usb.org/developers/docs/usb_20_070113.zip
So, except for devices that violates that, the worse case scenario is
64 bytes.
It should be noticed that the I2C bus could use a different limit,
so, on PCI devices, in theory, it would be possible to use a larger
window.
Yet, I doubt that any sane tuner/frontend design would require a
packet size bigger than the max size supported by the USB bus, as
that would limit their usage. Also, most (if not all) of those
tuners/frontends were added due to USB devices, anyway.
>
> How about if we kzalloc the buffer instead? (maybe not - that isn't
> very efficient either)
Seems an overkill to me to create/delete a buffer for every single I2C
transfer. Of course, a latter patch could optimize the buffer size to
match what's supported by the hardware, or to use a pre-allocated buffer,
but this is out of my scope: all I want is to get rid of dynamically
allocated buffers. I don't intend to read all those datasheets and
optimize each of those drivers, especially since I may not have the
hardware here for testing.
> If it has to be a static allocation (and it probably should be),
> please #define the size rather than sticking in the number 80.
Ok.
> This feedback applies to your entire "Don't use dynamic static
> allocation" patch series. Please don't merge those without at least
> #define'ing the size value and adding an appropriate inline comment to
> explain why the maximum is defined as such.
Well, a comment is provided already at the commit message. I don't
see any need to overbloat the code with a comment like that. In any
case, if I were to add a comment, it would be something like:
"I guess x bytes would be enough"
As only doing a deep code inspection and reading the datasheets, we'll
know for sure what's the maximum size supported by each device.
Regards,
Mauro
--
To unsu
Re: Fwd: [PATCH 3/6] [media] s5p-mfc: add support for VIDIOC_{G,S}_CROP to encoder
Hi John, On 10/18/2013 02:03 AM, John Sheu wrote: > On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki > wrote: >> On 10/18/2013 12:25 AM, John Sheu wrote: >>> On Thu, Oct 17, 2013 at 2:46 PM, John Sheu wrote: > Sweet. Thanks for spelling things out explicitly like this. The fact > that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness" > when used in a m2m device is definitely all sorts of confusing. >>> >>> Just to double-check: this means that we have another bug. >>> >>> In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop, >>> we "simulate" a G_CROP or S_CROP, if the entry point is not defined >>> for that device, by doing the appropriate S_SELECTION or G_SELECTION. >>> Unfortunately then, for M2M this is incorrect then. >>> >>> Am I reading this right? >> >> You are right, John. Firstly a clear specification needs to be written, >> something along the lines of Tomasz's explanation in this thread, once >> all agree to that the ioctl code should be corrected if needed. I don't understand the problem here. The specification has always been clear: s_crop for output devices equals s_selection(V4L2_SEL_TGT_COMPOSE_ACTIVE). Drivers should only implement the selection API and the v4l2 core will do the correct translation of s_crop. Yes, I know it's weird, but that's the way the crop API was defined way back and that's what should be used. My advise: forget about s_crop and just implement s_selection. >> >> It seems this [1] RFC is an answer exactly to your question. >> >> Exact meaning of the selection ioctl is only part of the problem, also >> interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec. >> >> [1] http://www.spinics.net/lists/linux-media/msg56078.html > > I think the "inversion" behavior is confusing and we should remove it > if at all possible. > > I took a look through all the drivers in linux-media which implement > S_CROP. Most of them are either OUTPUT or CAPTURE/OVERLAY-only. Of > those that aren't: > > * drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts both > OUTPUT and CAPTURE queues in S_CROP, but they both configure the same state. > No functional difference. Yeah, I guess that's a driver bug. This is a very old driver that originally used a custom API for these things, and since no selection API existed at the time it was just mapped to the crop API. Eventually it should use the selection API as well and do it correctly. But to be honest, nobody cares about this driver :-) It is however on my TODO list of drivers that need to be converted to the latest frameworks, so I might fix this eventually. > * drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't specify > the queue, but is a CAPTURE-only device. Probably an (unrelated) bug. Yes, that's a driver bug. It should check the buffer type. > * drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver > with both OUTPUT and CAPTURE queues. It has uninverted behavior: > S_CROP(CAPTURE) -> source > S_CROP(OUTPUT) -> destination This is the wrong behavior. > * drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with both > OUTPUT and CAPTURE queues. It has inverted behavior: > S_CROP(CAPTURE) -> destination > S_CROP(OUTPUT) -> source This is the correct behavior. > > The last two points above are the most relevant. So we already have > at least one broken driver, regardless of whether we allow inversion > or not; I'd think this grants us a certain freedom to redefine the > specification to be more logical. Can we do this please? No. The fimc-m2m.c driver needs to be fixed. That's the broken one. Regards, Hans -- 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
[PATCH] v4l: omap3isp: Move code out of mutex-protected section
The pad::get_fmt call must be protected by a mutex, but preparing its arguments doesn't need to be. Move the non-critical code out of the mutex-protected section. Signed-off-by: Laurent Pinchart --- drivers/media/platform/omap3isp/ispvideo.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c index a908d00..f6304bb 100644 --- a/drivers/media/platform/omap3isp/ispvideo.c +++ b/drivers/media/platform/omap3isp/ispvideo.c @@ -339,14 +339,11 @@ __isp_video_get_format(struct isp_video *video, struct v4l2_format *format) if (subdev == NULL) return -EINVAL; - mutex_lock(&video->mutex); - fmt.pad = pad; fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; - ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); - if (ret == -ENOIOCTLCMD) - ret = -EINVAL; + mutex_lock(&video->mutex); + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); mutex_unlock(&video->mutex); if (ret) -- Regards, Laurent Pinchart -- 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: [PATCH v2 00/18] OMAP4 ISS driver
Hi Mauro, On Monday 04 November 2013 01:06:25 Laurent Pinchart wrote: > Hello, > > This is the second version of the OMAP4 ISS driver patches for inclusion in > the mainline kernel. I've addressed most of the comments received on the > first version (some of them are still being discussed) in additional > patches, except for the file path updates in the documentation that have > been squashed with patch 01/18. > > The OMAP4 ISS driver has lived out of tree for more than two years now. This > situation is both sad and resource-wasting, as the driver has been used > (and thus) hacked since then with nowhere to send patches to. Time has come > to fix the problem. > > As the code is mostly, but not quite ready for prime time, I'd like to > request its addition to drivers/staging/. I've added a (pretty small) TODO > file and I commit to cleaning up the code and get it to drivers/media/ > where it belongs. > > I've split the driver in six patches to avoid getting caught in vger's size > and to make review slightly easier. Sergio Aguirre is the driver author > (huge thanks for that!), I've thus kept his authorship on patches 1/6 to > 5/6. Beside minimal changes to make the code compile on v3.12 and updates > the file paths in the documentation I've kept Sergio's code unmodified. > > I don't have much else to add here, let's get this beast to mainline and > allow other developers to use the driver and contribute patches. Given that > v3.12 has just been released I'm fine with pushing this series back to > v3.13. This should of course have read v3.14. > Laurent Pinchart (13): > v4l: omap4iss: Add support for OMAP4 camera interface - Build system > v4l: omap4iss: Don't use v4l2_g_ext_ctrls() internally > v4l: omap4iss: Move common code out of switch...case > v4l: omap4iss: Report device caps in response to VIDIOC_QUERYCAP > v4l: omap4iss: Remove iss_video streaming field > v4l: omap4iss: Set the vb2 timestamp type > v4l: omap4iss: Remove duplicate video_is_registered() check > v4l: omap4iss: Remove unneeded status variable > v4l: omap4iss: Replace udelay/msleep with usleep_range > v4l: omap4iss: Make omap4iss_isp_subclk_(en|dis)able() functions void > v4l: omap4iss: Make loop counters unsigned where appropriate > v4l: omap4iss: Don't initialize fields to 0 manually > v4l: omap4iss: Simplify error paths > > Sergio Aguirre (5): > v4l: omap4iss: Add support for OMAP4 camera interface - Core > v4l: omap4iss: Add support for OMAP4 camera interface - Video devices > v4l: omap4iss: Add support for OMAP4 camera interface - CSI receivers > v4l: omap4iss: Add support for OMAP4 camera interface - IPIPE(IF) > v4l: omap4iss: Add support for OMAP4 camera interface - Resizer > > Documentation/video4linux/omap4_camera.txt | 60 ++ > drivers/staging/media/Kconfig|2 + > drivers/staging/media/Makefile |1 + > drivers/staging/media/omap4iss/Kconfig | 12 + > drivers/staging/media/omap4iss/Makefile |6 + > drivers/staging/media/omap4iss/TODO |4 + > drivers/staging/media/omap4iss/iss.c | 1462 +++ > drivers/staging/media/omap4iss/iss.h | 153 +++ > drivers/staging/media/omap4iss/iss_csi2.c| 1368 +++ > drivers/staging/media/omap4iss/iss_csi2.h| 156 +++ > drivers/staging/media/omap4iss/iss_csiphy.c | 278 + > drivers/staging/media/omap4iss/iss_csiphy.h | 51 + > drivers/staging/media/omap4iss/iss_ipipe.c | 581 ++ > drivers/staging/media/omap4iss/iss_ipipe.h | 67 ++ > drivers/staging/media/omap4iss/iss_ipipeif.c | 847 +++ > drivers/staging/media/omap4iss/iss_ipipeif.h | 92 ++ > drivers/staging/media/omap4iss/iss_regs.h| 883 > drivers/staging/media/omap4iss/iss_resizer.c | 905 > drivers/staging/media/omap4iss/iss_resizer.h | 75 ++ > drivers/staging/media/omap4iss/iss_video.c | 1124 > drivers/staging/media/omap4iss/iss_video.h | 198 > include/media/omap4iss.h | 65 ++ > 22 files changed, 8390 insertions(+) > create mode 100644 Documentation/video4linux/omap4_camera.txt > create mode 100644 drivers/staging/media/omap4iss/Kconfig > create mode 100644 drivers/staging/media/omap4iss/Makefile > create mode 100644 drivers/staging/media/omap4iss/TODO > create mode 100644 drivers/staging/media/omap4iss/iss.c > create mode 100644 drivers/staging/media/omap4iss/iss.h > create mode 100644 drivers/staging/media/omap4iss/iss_csi2.c > create mode 100644 drivers/staging/media/omap4iss/iss_csi2.h > create mode 100644 drivers/staging/media/omap4iss/iss_csiphy.c > create mode 100644 drivers/staging/media/omap4iss/iss_csiphy.h > create mode 100644 drivers/staging/media/omap4iss/iss_ipipe.c > create mode 100644 drivers/staging/media/omap4iss/iss_ipipe.h > create mode 100644 drivers/staging/media/omap4iss/iss_ipipeif.c > cr
[PATCH v2 22/18] v4l: omap4iss: Implement VIDIOC_S_INPUT
The ioctl is (at least currently) mandatory.
Signed-off-by: Laurent Pinchart
---
drivers/staging/media/omap4iss/iss_video.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/staging/media/omap4iss/iss_video.c
b/drivers/staging/media/omap4iss/iss_video.c
index 6800623..766491e 100644
--- a/drivers/staging/media/omap4iss/iss_video.c
+++ b/drivers/staging/media/omap4iss/iss_video.c
@@ -902,6 +902,12 @@ iss_video_g_input(struct file *file, void *fh, unsigned
int *input)
return 0;
}
+static int
+iss_video_s_input(struct file *file, void *fh, unsigned int input)
+{
+ return input == 0 ? 0 : -EINVAL;
+}
+
static const struct v4l2_ioctl_ops iss_video_ioctl_ops = {
.vidioc_querycap= iss_video_querycap,
.vidioc_g_fmt_vid_cap = iss_video_get_format,
@@ -923,6 +929,7 @@ static const struct v4l2_ioctl_ops iss_video_ioctl_ops = {
.vidioc_streamoff = iss_video_streamoff,
.vidioc_enum_input = iss_video_enum_input,
.vidioc_g_input = iss_video_g_input,
+ .vidioc_s_input = iss_video_s_input,
};
/*
-
--
1.8.1.5
--
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
[PATCH v2 21/18] v4l: omap4iss: Move code out of mutex-protected section
The pad::get_fmt call must be protected by a mutex, but preparing its arguments doesn't need to be. Move the non-critical code out of the mutex-protected section. Signed-off-by: Laurent Pinchart --- drivers/staging/media/omap4iss/iss_video.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c index 63419b3..6800623 100644 --- a/drivers/staging/media/omap4iss/iss_video.c +++ b/drivers/staging/media/omap4iss/iss_video.c @@ -243,12 +243,11 @@ __iss_video_get_format(struct iss_video *video, struct v4l2_format *format) if (subdev == NULL) return -EINVAL; - mutex_lock(&video->mutex); - fmt.pad = pad; fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; - ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); + mutex_lock(&video->mutex); + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); mutex_unlock(&video->mutex); if (ret) -- 1.8.1.5 -- 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
[PATCH v2 20/18] v4l: omap4iss: Translate -ENOIOCTLCMD to -ENOTTY
Translating -ENOIOCTLCMD to -EINVAL is invalid, the correct ioctl return value is -ENOTTY. Signed-off-by: Laurent Pinchart --- drivers/staging/media/omap4iss/iss_video.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c index 0cb5820..63419b3 100644 --- a/drivers/staging/media/omap4iss/iss_video.c +++ b/drivers/staging/media/omap4iss/iss_video.c @@ -571,7 +571,7 @@ iss_video_cropcap(struct file *file, void *fh, struct v4l2_cropcap *cropcap) ret = v4l2_subdev_call(subdev, video, cropcap, cropcap); mutex_unlock(&video->mutex); - return ret == -ENOIOCTLCMD ? -EINVAL : ret; + return ret == -ENOIOCTLCMD ? -ENOTTY : ret; } static int @@ -598,7 +598,7 @@ iss_video_get_crop(struct file *file, void *fh, struct v4l2_crop *crop) format.which = V4L2_SUBDEV_FORMAT_ACTIVE; ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &format); if (ret < 0) - return ret == -ENOIOCTLCMD ? -EINVAL : ret; + return ret == -ENOIOCTLCMD ? -ENOTTY : ret; crop->c.left = 0; crop->c.top = 0; @@ -623,7 +623,7 @@ iss_video_set_crop(struct file *file, void *fh, const struct v4l2_crop *crop) ret = v4l2_subdev_call(subdev, video, s_crop, crop); mutex_unlock(&video->mutex); - return ret == -ENOIOCTLCMD ? -EINVAL : ret; + return ret == -ENOIOCTLCMD ? -ENOTTY : ret; } static int -- 1.8.1.5 -- 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
[PATCH v2 19/18] v4l: omap4iss: Don't check for missing get_fmt op on remote subdev
The remote subdev of any video node in the OMAP4 ISS is an internal subdev that is guaranteed to implement get_fmt. Don't check the return value for -ENOIOCTLCMD, as this can't happen. Signed-off-by: Laurent Pinchart --- drivers/staging/media/omap4iss/iss_video.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c index a527330..0cb5820 100644 --- a/drivers/staging/media/omap4iss/iss_video.c +++ b/drivers/staging/media/omap4iss/iss_video.c @@ -248,8 +248,6 @@ __iss_video_get_format(struct iss_video *video, struct v4l2_format *format) fmt.pad = pad; fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); - if (ret == -ENOIOCTLCMD) - ret = -EINVAL; mutex_unlock(&video->mutex); @@ -552,7 +550,7 @@ iss_video_try_format(struct file *file, void *fh, struct v4l2_format *format) fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); if (ret) - return ret == -ENOIOCTLCMD ? -EINVAL : ret; + return ret; iss_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix); return 0; -- 1.8.1.5 -- 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: [PATCH v2 00/18] OMAP4 ISS driver
Hi Laurent, For this whole patch series: Acked-by: Hans Verkuil I strongly recommend that you add another patch adding the s_input function. It's trivial to do and it will make v4l2-compliance happy :-) Fixing the issue with iss_video_try_format() and a missing get_fmt op is nice-to-have, but can be done later. Regards, Hans On 11/04/2013 01:06 AM, Laurent Pinchart wrote: > Hello, > > This is the second version of the OMAP4 ISS driver patches for inclusion in > the > mainline kernel. I've addressed most of the comments received on the first > version (some of them are still being discussed) in additional patches, except > for the file path updates in the documentation that have been squashed with > patch 01/18. > > The OMAP4 ISS driver has lived out of tree for more than two years now. This > situation is both sad and resource-wasting, as the driver has been used (and > thus) hacked since then with nowhere to send patches to. Time has come to fix > the problem. > > As the code is mostly, but not quite ready for prime time, I'd like to request > its addition to drivers/staging/. I've added a (pretty small) TODO file and I > commit to cleaning up the code and get it to drivers/media/ where it belongs. > > I've split the driver in six patches to avoid getting caught in vger's size > and to make review slightly easier. Sergio Aguirre is the driver author (huge > thanks for that!), I've thus kept his authorship on patches 1/6 to 5/6. Beside > minimal changes to make the code compile on v3.12 and updates the file paths > in > the documentation I've kept Sergio's code unmodified. > > I don't have much else to add here, let's get this beast to mainline and allow > other developers to use the driver and contribute patches. Given that v3.12 > has > just been released I'm fine with pushing this series back to v3.13. > > Laurent Pinchart (13): > v4l: omap4iss: Add support for OMAP4 camera interface - Build system > v4l: omap4iss: Don't use v4l2_g_ext_ctrls() internally > v4l: omap4iss: Move common code out of switch...case > v4l: omap4iss: Report device caps in response to VIDIOC_QUERYCAP > v4l: omap4iss: Remove iss_video streaming field > v4l: omap4iss: Set the vb2 timestamp type > v4l: omap4iss: Remove duplicate video_is_registered() check > v4l: omap4iss: Remove unneeded status variable > v4l: omap4iss: Replace udelay/msleep with usleep_range > v4l: omap4iss: Make omap4iss_isp_subclk_(en|dis)able() functions void > v4l: omap4iss: Make loop counters unsigned where appropriate > v4l: omap4iss: Don't initialize fields to 0 manually > v4l: omap4iss: Simplify error paths > > Sergio Aguirre (5): > v4l: omap4iss: Add support for OMAP4 camera interface - Core > v4l: omap4iss: Add support for OMAP4 camera interface - Video devices > v4l: omap4iss: Add support for OMAP4 camera interface - CSI receivers > v4l: omap4iss: Add support for OMAP4 camera interface - IPIPE(IF) > v4l: omap4iss: Add support for OMAP4 camera interface - Resizer > > Documentation/video4linux/omap4_camera.txt | 60 ++ > drivers/staging/media/Kconfig|2 + > drivers/staging/media/Makefile |1 + > drivers/staging/media/omap4iss/Kconfig | 12 + > drivers/staging/media/omap4iss/Makefile |6 + > drivers/staging/media/omap4iss/TODO |4 + > drivers/staging/media/omap4iss/iss.c | 1462 > ++ > drivers/staging/media/omap4iss/iss.h | 153 +++ > drivers/staging/media/omap4iss/iss_csi2.c| 1368 > drivers/staging/media/omap4iss/iss_csi2.h| 156 +++ > drivers/staging/media/omap4iss/iss_csiphy.c | 278 + > drivers/staging/media/omap4iss/iss_csiphy.h | 51 + > drivers/staging/media/omap4iss/iss_ipipe.c | 581 ++ > drivers/staging/media/omap4iss/iss_ipipe.h | 67 ++ > drivers/staging/media/omap4iss/iss_ipipeif.c | 847 +++ > drivers/staging/media/omap4iss/iss_ipipeif.h | 92 ++ > drivers/staging/media/omap4iss/iss_regs.h| 883 > drivers/staging/media/omap4iss/iss_resizer.c | 905 > drivers/staging/media/omap4iss/iss_resizer.h | 75 ++ > drivers/staging/media/omap4iss/iss_video.c | 1124 > drivers/staging/media/omap4iss/iss_video.h | 198 > include/media/omap4iss.h | 65 ++ > 22 files changed, 8390 insertions(+) > create mode 100644 Documentation/video4linux/omap4_camera.txt > create mode 100644 drivers/staging/media/omap4iss/Kconfig > create mode 100644 drivers/staging/media/omap4iss/Makefile > create mode 100644 drivers/staging/media/omap4iss/TODO > create mode 100644 drivers/staging/media/omap4iss/iss.c > create mode 100644 drivers/staging/media/omap4iss/iss.h > create mode 100644 drivers/staging/media/omap4iss/iss_csi2.c > create mode 100644 drivers/staging/media/omap4iss/iss_csi2.h > create mode 100644 drivers/staging/m
Re: [Review Patch 0/9] si4713 usb device driver
On 10/15/2013 07:37 PM, edubez...@gmail.com wrote: > Hello Dinesh, > > On Tue, Oct 15, 2013 at 11:24 AM, Dinesh Ram wrote: >> Hello Eduardo, >> >> In this patch series, I have addressed the comments by you >> concerning my last patch series. >> In the resulting patches, I have corrected most of the >> style issues and adding of comments. However, some warnings >> given out by checkpatch.pl (mostly complaing about lines longer >> than 80 characters) are still there because I saw that code readibility >> suffers by breaking up those lines. >> >> Also Hans has contributed patches 8 and 9 in this patch series >> which address the issues of the handling of unknown regulators, >> which have apparently changed since 3.10. Hans has tested it and the >> driver loads again. >> >> Let me know when you are able to test it again. >> > > Hopefully I will be able to give it a shot on n900 and on silabs > devboard until the end of the week. Thanks for not giving up. Did you find time to do this? I'm waiting for feedback from you. Regards, Hans -- 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
[PATCH] tef6862/radio-tea5764: actually assign clamp result.
When adding frequency clamping to the tef6862 and radio-tea5764 drivers I forgot to actually *assign* the clamp result to the frequency. Signed-off-by: Hans Verkuil Reported-by: Hans Petter Selasky --- drivers/media/radio/radio-tea5764.c | 2 +- drivers/media/radio/tef6862.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/radio/radio-tea5764.c b/drivers/media/radio/radio-tea5764.c index 036e2f5..3ed1f56 100644 --- a/drivers/media/radio/radio-tea5764.c +++ b/drivers/media/radio/radio-tea5764.c @@ -356,7 +356,7 @@ static int vidioc_s_frequency(struct file *file, void *priv, So we keep it as-is. */ return -EINVAL; } - clamp(freq, FREQ_MIN * FREQ_MUL, FREQ_MAX * FREQ_MUL); + freq = clamp(freq, FREQ_MIN * FREQ_MUL, FREQ_MAX * FREQ_MUL); tea5764_power_up(radio); tea5764_tune(radio, (freq * 125) / 2); return 0; diff --git a/drivers/media/radio/tef6862.c b/drivers/media/radio/tef6862.c index 06ac692..f4bb456 100644 --- a/drivers/media/radio/tef6862.c +++ b/drivers/media/radio/tef6862.c @@ -112,7 +112,7 @@ static int tef6862_s_frequency(struct v4l2_subdev *sd, const struct v4l2_frequen if (f->tuner != 0) return -EINVAL; - clamp(freq, TEF6862_LO_FREQ, TEF6862_HI_FREQ); + freq = clamp(freq, TEF6862_LO_FREQ, TEF6862_HI_FREQ); pll = 1964 + ((freq - TEF6862_LO_FREQ) * 20) / FREQ_MUL; i2cmsg[0] = (MODE_PRESET << MODE_SHIFT) | WM_SUB_PLLM; i2cmsg[1] = (pll >> 8) & 0xff; -- 1.8.4.2 -- 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: [BUG] [PATCH 10/21] radio-tea5764: some cleanups and clamp frequency when out-of-range AND [PATCH 15/21] tef6862: clamp frequency.
Hi Hans,
On 10/28/2013 12:45 PM, Hans Petter Selasky wrote:
> On 05/31/13 12:02, Hans Verkuil wrote:
>> return -EINVAL;
>> +}
>> +clamp(freq, FREQ_MIN * FREQ_MUL, FREQ_MAX * FREQ_MUL);
>> tea5764_power_up(radio);
>> -tea5764_tune(radio, (f->frequency * 125) / 2);
>> +tea5764_tune(radio, (freq * 125) / 2);
>> return 0;
>
> Hi Hans,
>
> Should the part quoted above part perhaps read:
>
> freq = clamp(freq, FREQ_MIN * FREQ_MUL, FREQ_MAX * FREQ_MUL);
>
> Or did "#define clamp() change" recently?
Nope, that's a bug. Thanks for spotting this!
I'll make a patch for this.
Regards,
Hans
>
> http://lxr.free-electrons.com/source/include/linux/kernel.h
>
>> 698 /**
>> 699 * clamp - return a value clamped to a given range with strict
>> typechecking
>> 700 * @val: current value
>> 701 * @min: minimum allowable value
>> 702 * @max: maximum allowable value
>> 703 *
>> 704 * This macro does strict typechecking of min/max to make sure they are
>> of the
>> 705 * same type as val. See the unnecessary pointer comparisons.
>> 706 */
>> 707 #define clamp(val, min, max) ({ \
>> 708 typeof(val) __val = (val); \
>> 709 typeof(min) __min = (min); \
>> 710 typeof(max) __max = (max); \
>> 711 (void) (&__val == &__min); \
>> 712 (void) (&__val == &__max); \
>> 713 __val = __val < __min ? __min: __val; \
>> 714 __val > __max ? __max: __val; })
>
> Thank you!
>
> Same spotted in:
>
>>> media_tree/drivers/media/radio/radio-tea5764.c: In function
>>> 'vidioc_s_frequency':
>>> media_tree/drivers/media/radio/radio-tea5764.c:359: warning: statement with
>>> no effect
>>
>>> media_tree/drivers/media/radio/tef6862.c: In function 'tef6862_s_frequency':
>>> media_tree/drivers/media/radio/tef6862.c:115: warning: statement with no
>>> effect
>
> Keep up the good work!
>
> --HPS
>
--
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: Initial Tuning Data for Uganda
On 01-11-13 17:26, Joseph Zik wrote: Hello all, I wanted to contribute to dvb-apps package with initial DVB-T tuning data for Uganda, Africa Added and pushed to the usual places. Attached is the scan file. Please let me know, if there's any other info I can provide. Next time, a proper patch that applies from mail would be easier, but I manually fixed it for you this time :) Thank you! Oliver Thank you -- 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: [PATCH][RESEND 6/8] ALSA: memalloc: use gen_pool_dma_alloc() to allocate iram buffer
At Fri, 1 Nov 2013 19:48:19 +0800, Nicolin Chen wrote: > > Since gen_pool_dma_alloc() is introduced, we implement it to simplify code. > > Signed-off-by: Nicolin Chen Acked-by: Takashi Iwai > --- > sound/core/memalloc.c | 6 +- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c > index 9d93f02..5e1c7bc 100644 > --- a/sound/core/memalloc.c > +++ b/sound/core/memalloc.c > @@ -184,11 +184,7 @@ static void snd_malloc_dev_iram(struct snd_dma_buffer > *dmab, size_t size) > /* Assign the pool into private_data field */ > dmab->private_data = pool; > > - dmab->area = (void *)gen_pool_alloc(pool, size); > - if (!dmab->area) > - return; > - > - dmab->addr = gen_pool_virt_to_phys(pool, (unsigned long)dmab->area); > + dmab->area = gen_pool_dma_alloc(pool, size, &dmab->addr); > } > > /** > -- > 1.8.4 > > -- 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: [PATCH 2/6] v4l: omap4iss: Add support for OMAP4 camera interface - Video devices
On 11/04/2013 12:28 AM, Laurent Pinchart wrote:
> Hi Hans,
>
> Thank you for the review.
>
> On Thursday 03 October 2013 08:54:19 Hans Verkuil wrote:
>> On 10/03/2013 01:55 AM, Laurent Pinchart wrote:
>>> From: Sergio Aguirre
>>>
>>> This adds a very simplistic driver to utilize the CSI2A interface inside
>>> the ISS subsystem in OMAP4, and dump the data to memory.
>>>
>>> Check Documentation/video4linux/omap4_camera.txt for details.
>>>
>>> This commit adds video devices support.
>>>
>>> Signed-off-by: Sergio Aguirre
>>>
>>> [Port the driver to v3.12-rc3, including the following changes
>>> - Don't include plat/ headers
>>> - Don't use cpu_is_omap44xx() macro
>>> - Don't depend on EXPERIMENTAL
>>> - Fix s_crop operation prototype
>>> - Update link_notify prototype
>>> - Rename media_entity_remote_source to media_entity_remote_pad]
>>>
>>> Signed-off-by: Laurent Pinchart
>>> ---
>>>
>>> drivers/staging/media/omap4iss/iss_video.c | 1129 +++
>>> drivers/staging/media/omap4iss/iss_video.h | 201 +
>>> 2 files changed, 1330 insertions(+)
>>> create mode 100644 drivers/staging/media/omap4iss/iss_video.c
>>> create mode 100644 drivers/staging/media/omap4iss/iss_video.h
>>>
>>> diff --git a/drivers/staging/media/omap4iss/iss_video.c
>>> b/drivers/staging/media/omap4iss/iss_video.c new file mode 100644
>>> index 000..31f1b88
>>> --- /dev/null
>>> +++ b/drivers/staging/media/omap4iss/iss_video.c
>>
>>
>>
>>> +/* -
>>> + * V4L2 ioctls
>>> + */
>>> +
>>> +static int
>>> +iss_video_querycap(struct file *file, void *fh, struct v4l2_capability
>>> *cap) +{
>>> + struct iss_video *video = video_drvdata(file);
>>> +
>>> + strlcpy(cap->driver, ISS_VIDEO_DRIVER_NAME, sizeof(cap->driver));
>>> + strlcpy(cap->card, video->video.name, sizeof(cap->card));
>>> + strlcpy(cap->bus_info, "media", sizeof(cap->bus_info));
>>> +
>>> + if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>> + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
>>> + else
>>> + cap->capabilities = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
>>
>> Set device_caps instead of capabilities and add:
>>
>> cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
>
> Actually cap->capabilities should be V4L2_CAP_DEVICE_CAPS |
> V4L2_CAP_STREAMING
> | V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT. I'll fix that.
You're right.
>
>>> +
>>> + return 0;
>>> +}
>
> [snip]
>
>>> +static int
>>> +iss_video_try_format(struct file *file, void *fh, struct v4l2_format
>>> *format) +{
>>> + struct iss_video *video = video_drvdata(file);
>>> + struct v4l2_subdev_format fmt;
>>> + struct v4l2_subdev *subdev;
>>> + u32 pad;
>>> + int ret;
>>> +
>>> + if (format->type != video->type)
>>> + return -EINVAL;
>>> +
>>> + subdev = iss_video_remote_subdev(video, &pad);
>>> + if (subdev == NULL)
>>> + return -EINVAL;
>>> +
>>> + iss_video_pix_to_mbus(&format->fmt.pix, &fmt.format);
>>> +
>>> + fmt.pad = pad;
>>> + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>> + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
>>> + if (ret)
>>> + return ret == -ENOIOCTLCMD ? -EINVAL : ret;
>>
>> Return ENOTTY instead of EINVAL. Even better, use v4l2_subdev_has_op() +
>> v4l2_disable_ioctl() to just disable ioctls based on the available subdev
>> ops in probe().
>
> The remote subdev is required to implement the get_fmt() operation, otherwise
> the ISS driver can't work at all. What about adding a v4l2_subdev_has_op()
> check at probe time and removing the check here ?
That would be best, yes.
>
>>> +
>>> + iss_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
>>> + return 0;
>>> +}
>
> [snip]
>
>>> +static int
>>> +iss_video_enum_input(struct file *file, void *fh, struct v4l2_input
>>> *input)
>>> +{
>>> + if (input->index > 0)
>>> + return -EINVAL;
>>> +
>>> + strlcpy(input->name, "camera", sizeof(input->name));
>>> + input->type = V4L2_INPUT_TYPE_CAMERA;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int
>>> +iss_video_g_input(struct file *file, void *fh, unsigned int *input)
>>> +{
>>> + *input = 0;
>>> +
>>> + return 0;
>>> +}
>>
>> Also add s_input.
>
> Shouldn't I remove enum_input and g_input instead ?
No. G/S/ENUM_INPUT ioctls are compulsory for video capture devices.
One thing I would like to do at some point in time is to add a few helper
functions
for this. A lot of camera/TV drivers have just one input, so it would be
trivial to
add helpers for that. I just never got around to that.
Regards,
Hans
--
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
