[PATCH 1/3] media: ov2640: set default window and format code at probe time
Set default window and format code at probe time instead of always checking
if they have not been set yet when VIDIOC_SUBDEV_G_FMT ioctl is called.
This change simplifies the next patch (make VIDIOC_SUBDEV_G_FMT ioctl work
with V4L2_SUBDEV_FORMAT_TRY).
Cc: Sakari Ailus
Cc: Mauro Carvalho Chehab
Signed-off-by: Akinobu Mita
---
drivers/media/i2c/ov2640.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index d8e91bc..a07e6f2 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -842,9 +842,6 @@ static int ov2640_set_params(struct i2c_client *client,
u8 val;
int ret;
- if (!win)
- return -EINVAL;
-
switch (code) {
case MEDIA_BUS_FMT_RGB565_2X8_BE:
dev_dbg(>dev, "%s: Selected cfmt RGB565 BE", __func__);
@@ -929,10 +926,6 @@ static int ov2640_get_fmt(struct v4l2_subdev *sd,
if (format->pad)
return -EINVAL;
- if (!priv->win) {
- priv->win = ov2640_select_win(SVGA_WIDTH, SVGA_HEIGHT);
- priv->cfmt_code = MEDIA_BUS_FMT_UYVY8_2X8;
- }
mf->width = priv->win->width;
mf->height = priv->win->height;
@@ -1193,6 +1186,9 @@ static int ov2640_probe(struct i2c_client *client,
if (ret)
goto err_clk;
+ priv->win = ov2640_select_win(SVGA_WIDTH, SVGA_HEIGHT);
+ priv->cfmt_code = MEDIA_BUS_FMT_UYVY8_2X8;
+
v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
V4L2_SUBDEV_FL_HAS_EVENTS;
--
2.7.4
[PATCH 3/3] media: ov2640: set all mbus format field when G_FMT and S_FMT ioctls
This driver doesn't set all members of mbus format field when the
VIDIOC_SUBDEV_{S,G}_FMT ioctls are called.
This is detected by v4l2-compliance.
Cc: Sakari Ailus
Cc: Mauro Carvalho Chehab
Signed-off-by: Akinobu Mita
---
drivers/media/i2c/ov2640.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index 5f888f5..439c6da 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -938,6 +938,9 @@ static int ov2640_get_fmt(struct v4l2_subdev *sd,
mf->code= priv->cfmt_code;
mf->colorspace = V4L2_COLORSPACE_SRGB;
mf->field = V4L2_FIELD_NONE;
+ mf->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+ mf->quantization = V4L2_QUANTIZATION_DEFAULT;
+ mf->xfer_func = V4L2_XFER_FUNC_DEFAULT;
return 0;
}
@@ -964,6 +967,9 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
mf->field = V4L2_FIELD_NONE;
mf->colorspace = V4L2_COLORSPACE_SRGB;
+ mf->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+ mf->quantization = V4L2_QUANTIZATION_DEFAULT;
+ mf->xfer_func = V4L2_XFER_FUNC_DEFAULT;
switch (mf->code) {
case MEDIA_BUS_FMT_RGB565_2X8_BE:
--
2.7.4
[PATCH 2/3] media: ov2640: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
The VIDIOC_SUBDEV_G_FMT ioctl for this driver doesn't recognize
V4L2_SUBDEV_FORMAT_TRY and always works as if V4L2_SUBDEV_FORMAT_ACTIVE
is specified.
Cc: Sakari Ailus
Cc: Mauro Carvalho Chehab
Signed-off-by: Akinobu Mita
---
drivers/media/i2c/ov2640.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index a07e6f2..5f888f5 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -926,6 +926,12 @@ static int ov2640_get_fmt(struct v4l2_subdev *sd,
if (format->pad)
return -EINVAL;
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+ mf = v4l2_subdev_get_try_format(sd, cfg, 0);
+ format->format = *mf;
+
+ return 0;
+ }
mf->width = priv->win->width;
mf->height = priv->win->height;
@@ -992,6 +998,26 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
return ret;
}
+static int ov2640_init_cfg(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct ov2640_priv *priv = to_ov2640(client);
+ struct v4l2_mbus_framefmt *try_fmt =
+ v4l2_subdev_get_try_format(sd, cfg, 0);
+
+ try_fmt->width = priv->win->width;
+ try_fmt->height = priv->win->height;
+ try_fmt->code = priv->cfmt_code;
+ try_fmt->colorspace = V4L2_COLORSPACE_SRGB;
+ try_fmt->field = V4L2_FIELD_NONE;
+ try_fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+ try_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
+ try_fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
+
+ return 0;
+}
+
static int ov2640_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_mbus_code_enum *code)
@@ -1101,6 +1127,7 @@ static const struct v4l2_subdev_core_ops
ov2640_subdev_core_ops = {
};
static const struct v4l2_subdev_pad_ops ov2640_subdev_pad_ops = {
+ .init_cfg = ov2640_init_cfg,
.enum_mbus_code = ov2640_enum_mbus_code,
.get_selection = ov2640_get_selection,
.get_fmt= ov2640_get_fmt,
--
2.7.4
[PATCH 0/3] media: ov2640: fix two problems
This patch series contains two bugfixes and a preparatory change for ov2640 driver. Akinobu Mita (3): media: ov2640: set default window and format code at probe time media: ov2640: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY media: ov2640: set all mbus format field when G_FMT and S_FMT ioctls drivers/media/i2c/ov2640.c | 41 +++-- 1 file changed, 35 insertions(+), 6 deletions(-) Cc: Sakari Ailus Cc: Mauro Carvalho Chehab -- 2.7.4
cron job: media_tree daily build: ERRORS
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: Sat Dec 8 05:00:14 CET 2018 media-tree git hash:e159b6074c82fe31b79aad672e02fa204dbbc6d8 media_build git hash: 4b9237c73e29ea969f6a7b3d00030e14be50 v4l-utils git hash: 516595495957cbc18b578e6c1598bec21858b4e5 edid-decode git hash: 6def7bc83dfb0338632e06a8b14c93faa6af8879 gcc version:i686-linux-gcc (GCC) 8.2.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.18.0-3-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: ERRORS linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-3.10.108-i686: ERRORS linux-3.10.108-x86_64: ERRORS linux-3.11.10-i686: ERRORS linux-3.11.10-x86_64: ERRORS linux-3.12.74-i686: ERRORS linux-3.12.74-x86_64: ERRORS linux-3.13.11-i686: ERRORS linux-3.13.11-x86_64: ERRORS linux-3.14.79-i686: ERRORS linux-3.14.79-x86_64: ERRORS linux-3.15.10-i686: ERRORS linux-3.15.10-x86_64: ERRORS linux-3.16.57-i686: ERRORS linux-3.16.57-x86_64: ERRORS linux-3.17.8-i686: ERRORS linux-3.17.8-x86_64: ERRORS linux-3.18.123-i686: ERRORS linux-3.18.123-x86_64: ERRORS linux-3.19.8-i686: ERRORS linux-3.19.8-x86_64: ERRORS linux-4.0.9-i686: ERRORS linux-4.0.9-x86_64: ERRORS linux-4.1.52-i686: ERRORS linux-4.1.52-x86_64: ERRORS linux-4.2.8-i686: ERRORS linux-4.2.8-x86_64: ERRORS linux-4.3.6-i686: ERRORS linux-4.3.6-x86_64: ERRORS linux-4.4.159-i686: ERRORS linux-4.4.159-x86_64: ERRORS linux-4.5.7-i686: ERRORS linux-4.5.7-x86_64: ERRORS linux-4.6.7-i686: ERRORS linux-4.6.7-x86_64: ERRORS linux-4.7.10-i686: ERRORS linux-4.7.10-x86_64: ERRORS linux-4.8.17-i686: ERRORS linux-4.8.17-x86_64: ERRORS linux-4.9.131-i686: ERRORS linux-4.9.131-x86_64: ERRORS linux-4.10.17-i686: ERRORS linux-4.10.17-x86_64: ERRORS linux-4.11.12-i686: ERRORS linux-4.11.12-x86_64: ERRORS linux-4.12.14-i686: ERRORS linux-4.12.14-x86_64: ERRORS linux-4.13.16-i686: ERRORS linux-4.13.16-x86_64: ERRORS linux-4.14.74-i686: ERRORS linux-4.14.74-x86_64: ERRORS linux-4.15.18-i686: ERRORS linux-4.15.18-x86_64: ERRORS linux-4.16.18-i686: ERRORS linux-4.16.18-x86_64: ERRORS linux-4.17.19-i686: ERRORS linux-4.17.19-x86_64: ERRORS linux-4.18.12-i686: ERRORS linux-4.18.12-x86_64: ERRORS linux-4.19.1-i686: OK linux-4.19.1-x86_64: OK linux-4.20-rc1-i686: OK linux-4.20-rc1-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
[PATCH v5 1/1] media: rc: rcmm decoder
media: add support for RCMM infrared remote controls.
Signed-off-by: Patrick Lerda
---
MAINTAINERS| 5 +
drivers/media/rc/Kconfig | 7 ++
drivers/media/rc/Makefile | 1 +
drivers/media/rc/ir-rcmm-decoder.c | 164 +
drivers/media/rc/rc-core-priv.h| 5 +
drivers/media/rc/rc-main.c | 3 +
include/media/rc-map.h | 6 +-
include/uapi/linux/lirc.h | 2 +
tools/include/uapi/linux/lirc.h| 2 +
9 files changed, 193 insertions(+), 2 deletions(-)
create mode 100644 drivers/media/rc/ir-rcmm-decoder.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 3e9f1710ed13..80426d1faaba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16277,6 +16277,11 @@ M: David Härdeman
S: Maintained
F: drivers/media/rc/winbond-cir.c
+RCMM REMOTE CONTROLS DECODER
+M: Patrick Lerda
+S: Maintained
+F: drivers/media/rc/ir-rcmm-decoder.c
+
WINSYSTEMS EBC-C384 WATCHDOG DRIVER
M: William Breathitt Gray
L: linux-watch...@vger.kernel.org
diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index 8a216068a35a..43775ac74268 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -133,6 +133,13 @@ config IR_IMON_DECODER
remote control and you would like to use it with a raw IR
receiver, or if you wish to use an encoder to transmit this IR.
+config IR_RCMM_DECODER
+ tristate "Enable IR raw decoder for the RC-MM protocol"
+ depends on RC_CORE
+ help
+ Enable this option if you have IR with RC-MM protocol, and
+ if the IR is decoded in software
+
endif #RC_DECODERS
menuconfig RC_DEVICES
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index 92c163816849..48d23433b3c0 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_IR_SHARP_DECODER) += ir-sharp-decoder.o
obj-$(CONFIG_IR_MCE_KBD_DECODER) += ir-mce_kbd-decoder.o
obj-$(CONFIG_IR_XMP_DECODER) += ir-xmp-decoder.o
obj-$(CONFIG_IR_IMON_DECODER) += ir-imon-decoder.o
+obj-$(CONFIG_IR_RCMM_DECODER) += ir-rcmm-decoder.o
# stand-alone IR receivers/transmitters
obj-$(CONFIG_RC_ATI_REMOTE) += ati_remote.o
diff --git a/drivers/media/rc/ir-rcmm-decoder.c
b/drivers/media/rc/ir-rcmm-decoder.c
new file mode 100644
index ..a3c09885da5f
--- /dev/null
+++ b/drivers/media/rc/ir-rcmm-decoder.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0+
+// ir-rcmm-decoder.c - A decoder for the RCMM IR protocol
+//
+// Copyright (C) 2018 by Patrick Lerda
+
+#include "rc-core-priv.h"
+#include
+#include
+
+#define RCMM_UNIT 17 /* nanosecs */
+#define RCMM_PREFIX_PULSE 41 /* 16.6*2.5 */
+#define RCMM_PULSE_027 /* 16.6*(1+2/3) */
+#define RCMM_PULSE_144 /* 16.6*(2+2/3) */
+#define RCMM_PULSE_261 /* 16.6*(3+2/3) */
+#define RCMM_PULSE_378 /* 16.6*(4+2/3) */
+
+enum rcmm_state {
+ STATE_INACTIVE,
+ STATE_LOW,
+ STATE_BUMP,
+ STATE_VALUE,
+ STATE_FINISHED,
+};
+
+static bool rcmm_mode(struct rcmm_dec *data)
+{
+ return !((0x000c & data->bits) == 0x000c);
+}
+
+/**
+ * ir_rcmm_decode() - Decode one RCMM pulse or space
+ * @dev: the struct rc_dev descriptor of the device
+ * @ev:the struct ir_raw_event descriptor of the pulse/space
+ *
+ * This function returns -EINVAL if the pulse violates the state machine
+ */
+static int ir_rcmm_decode(struct rc_dev *dev, struct ir_raw_event ev)
+{
+ struct rcmm_dec *data = >raw->rcmm;
+ u32 scancode;
+ u8 toggle;
+ int value;
+
+ if (!(dev->enabled_protocols & RC_PROTO_BIT_RCMM))
+ return 0;
+
+ if (!is_timing_event(ev)) {
+ if (ev.reset)
+ data->state = STATE_INACTIVE;
+ return 0;
+ }
+
+ if (ev.duration > RCMM_PULSE_3 + RCMM_UNIT)
+ goto out;
+
+ switch (data->state) {
+ case STATE_INACTIVE:
+ if (!ev.pulse)
+ break;
+
+ if (!eq_margin(ev.duration, RCMM_PREFIX_PULSE, RCMM_UNIT / 2))
+ break;
+
+ data->state = STATE_LOW;
+ data->count = 0;
+ data->bits = 0;
+ return 0;
+
+ case STATE_LOW:
+ if (ev.pulse)
+ break;
+
+ if (!eq_margin(ev.duration, RCMM_PULSE_0, RCMM_UNIT / 2))
+ break;
+
+ data->state = STATE_BUMP;
+ return 0;
+
+ case STATE_BUMP:
+ if (!ev.pulse)
+ break;
+
+ if (!eq_margin(ev.duration, RCMM_UNIT, RCMM_UNIT / 2))
+ break;
+
+ data->state = STATE_VALUE;
+
Re: [PATCHv3 1/1] Add ir-rcmm-driver
Hi Patrick, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linuxtv-media/master] [also build test WARNING on v4.20-rc5 next-20181207] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Patrick-LERDA/Add-ir-rcmm-driver/20181208-02 base: git://linuxtv.org/media_tree.git master reproduce: make htmldocs All warnings (new ones prefixed by >>): include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params ' include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params ' include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params ' include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params ' include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params ' include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params ' include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params ' include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params ' include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params ' net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.avg_ack_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info' kernel/rcu/tree.c:685: warning: Excess function parameter 'irq' description in 'rcu_nmi_exit' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.cb' not described in 'dma_buf' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.poll' not described in 'dma_buf' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.active' not described in 'dma_buf' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.cb' not described in 'dma_buf' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.poll' not described in 'dma_buf' include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.active' not described in 'dma_buf' include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array' include/linux/gpio/driver.h:375: warning: Function parameter or member 'init_valid_mask' not described in 'gpio_chip' include/linux/iio/hw-consumer.h:1: warning: no structured comments found include/linux/input/sparse-keymap.h:46: warning: Function parameter or memb
[GIT FIXES for 4.20] Fwnode parsing fix
Hi Mauro, Here's a patch that fixes clearing fwnode flags. The patch that broke it is only in 4.20. Please pull. The following changes since commit 078ab3ea2c3bb69cb989d52346fefa1246055e5b: media: Add a Kconfig option for the Request API (2018-12-05 13:07:43 -0500) are available in the git repository at: ssh://linuxtv.org/git/sailus/media_tree.git tags/fixes-4.20-1-sign for you to fetch changes up to 7add7222d1741afb448a386c3085f38da801fd50: media: v4l2-fwnode: Fix setting V4L2_MBUS_DATA_ACTIVE_HIGH/LOW flag (2018-12-08 00:27:15 +0200) clear correct fwnode flags Ondrej Jirman (1): media: v4l2-fwnode: Fix setting V4L2_MBUS_DATA_ACTIVE_HIGH/LOW flag drivers/media/v4l2-core/v4l2-fwnode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- Sakari Ailus
Re: [PATCH 1/2] media: pxa_camera: don't deferenciate a NULL pointer
Hi Mauro,
On Fri, Dec 07, 2018 at 08:07:54AM -0500, Mauro Carvalho Chehab wrote:
> As warned by smatch:
> drivers/media/platform/pxa_camera.c:2400 pxa_camera_probe() error: we
> previously assumed 'pcdev->pdata' could be null (see line 2397)
>
> It would be possible that neither DT nor platform data would be
> provided. This is a Kernel bug, so warn about that and bail.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/platform/pxa_camera.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/platform/pxa_camera.c
> b/drivers/media/platform/pxa_camera.c
> index 5f930560eb30..f91f8fd424c4 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -2396,6 +2396,9 @@ static int pxa_camera_probe(struct platform_device
> *pdev)
> pcdev->pdata = pdev->dev.platform_data;
> if (pdev->dev.of_node && !pcdev->pdata) {
> err = pxa_camera_pdata_from_dt(>dev, pcdev, >asd);
> + } else if (!pcdev->pdata) {
This fixes the issue, but the current checks remain a bit odd.
The driver seems to prefer platform data over OF. I wonder if that's
intentional or not.
In that case, I'd roughly write this as:
if (pcdev->pdata) {
...;
} else if (pdev->dev.of_node) {
...;
} else {
return -ENODEV;
}
I'm not sure WARN_ON(1) is necessary. A lot of drivers simply do it this
way without WARN_ON().
> + WARN_ON(1);
> + return -ENODEV;
> } else {
> pcdev->platform_flags = pcdev->pdata->flags;
> pcdev->mclk = pcdev->pdata->mclk_10khz * 1;
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v5 00/12] imx-media: Fixes for interlaced capture
Hi Hans, On 12/7/18 5:35 AM, Hans Verkuil wrote: Hi Steve, How to proceed with this w.r.t. the two gpu ipu patches? Are those going in first through the gpu tree? Or do they have to go in through our tree? There is only one remaining gpu ipu patch in this series that is awaiting an ack from Philipp: "gpu: ipu-csi: Swap fields according to input/output field types" I pinged him again. Philipp agreed to allow the two ipu patches in the series to be merged to media tree. Steve In that case I need Acks from whoever maintains that code. Regards, Hans On 10/17/2018 02:00 AM, Steve Longerbeam wrote: A set of patches that fixes some bugs with capturing from an interlaced source, and incompatibilites between IDMAC interlace interweaving and 4:2:0 data write reduction. History: v5: - Added a regression fix to allow empty endpoints to CSI (fix for imx6q SabreAuto). - Cleaned up some convoluted code in ipu_csi_init_interface(), suggested by Philipp Zabel. - Fixed a regression in csi_setup(), caught by Philipp. - Removed interweave_offset and replace with boolean interweave_swap, suggested by Philipp. - Make clear that it is IDMAC channel that does pixel reordering and interweave, not the CSI, in the imx.rst doc, caught by Philipp. v4: - rebased to latest media-tree master branch. - Make patch author and SoB email addresses the same. v3: - add support for/fix interweaved scan with YUV planar output. - fix bug in 4:2:0 U/V offset macros. - add patch that generalizes behavior of field swap in ipu_csi_init_interface(). - add support for interweaved scan with field order swap. Suggested by Philipp Zabel. - in v2, inteweave scan was determined using field types of CSI (and PRPENCVF) at the sink and source pads. In v3, this has been moved one hop downstream: interweave is now determined using field type at source pad, and field type selected at capture interface. Suggested by Philipp. - make sure to double CSI crop target height when input field type in alternate. - more updates to media driver doc to reflect above. v2: - update media driver doc. - enable idmac interweave only if input field is sequential/alternate, and output field is 'interlaced*'. - move field try logic out of *try_fmt and into separate function. - fix bug with resetting crop/compose rectangles. - add a patch that fixes a field order bug in VDIC indirect mode. - remove alternate field type from V4L2_FIELD_IS_SEQUENTIAL() macro Suggested-by: Nicolas Dufresne . - add macro V4L2_FIELD_IS_INTERLACED(). Steve Longerbeam (12): media: videodev2.h: Add more field helper macros gpu: ipu-csi: Swap fields according to input/output field types gpu: ipu-v3: Add planar support to interlaced scan media: imx: Fix field negotiation media: imx-csi: Input connections to CSI should be optional media: imx-csi: Double crop height for alternate fields at sink media: imx: interweave and odd-chroma-row skip are incompatible media: imx-csi: Allow skipping odd chroma rows for YVU420 media: imx: vdic: rely on VDIC for correct field order media: imx-csi: Move crop/compose reset after filling default mbus fields media: imx: Allow interweave with top/bottom lines swapped media: imx.rst: Update doc to reflect fixes to interlaced capture Documentation/media/v4l-drivers/imx.rst | 103 +++ drivers/gpu/ipu-v3/ipu-cpmem.c| 26 ++- drivers/gpu/ipu-v3/ipu-csi.c | 119 + drivers/staging/media/imx/imx-ic-prpencvf.c | 46 +++-- drivers/staging/media/imx/imx-media-capture.c | 14 ++ drivers/staging/media/imx/imx-media-csi.c | 168 +- drivers/staging/media/imx/imx-media-vdic.c| 12 +- include/uapi/linux/videodev2.h| 7 + include/video/imx-ipu-v3.h| 6 +- 9 files changed, 354 insertions(+), 147 deletions(-)
Re: VIVID/VIMC and media fuzzing
Hi Dmitry, On 10/31/18 9:49 AM, Helen Koike wrote: > Hi Dmitry, > > On 10/31/18 7:46 AM, Hans Verkuil wrote: >> On 10/30/2018 03:02 PM, Dmitry Vyukov wrote: >>> Hello Helen and linux-media, >>> >>> I've attended your talk "Shifting Media App Development into High >>> Gear" on OSS Summit last week and approached you with some questions >>> if/how this can be used for kernel testing. Thanks, turn out to be a >>> very useful talk! > > Great, I am glad it was useful :) > >>> >>> I am working on syzkaller/syzbot, continuous kernel fuzzing system: >>> https://github.com/google/syzkaller >>> https://github.com/google/syzkaller/blob/master/docs/syzbot.md >>> https://syzkaller.appspot.com >>> >>> After simply enabling CONFIG_VIDEO_VIMC, CONFIG_VIDEO_VIM2M, >>> CONFIG_VIDEO_VIVID, CONFIG_VIDEO_VICODEC syzbot has found 8 bugs in >>> media subsystem in just 24 hours: >>> >>> KASAN: use-after-free Read in vb2_mmap >>> https://groups.google.com/forum/#!msg/syzkaller-bugs/XGGH69jMWQ0/S8vfxgEmCgAJ >>> >>> KASAN: use-after-free Write in __vb2_cleanup_fileio >>> https://groups.google.com/forum/#!msg/syzkaller-bugs/qKKhsZVPo3o/P6AB2of2CQAJ >>> >>> WARNING in __vb2_queue_cancel >>> https://groups.google.com/forum/#!msg/syzkaller-bugs/S29GU_NtfPY/ZvAz8UDtCQAJ >>> >>> divide error in vivid_vid_cap_s_dv_timings >>> https://groups.google.com/forum/#!msg/syzkaller-bugs/GwF5zGBCfyg/wnuWmW_sCQAJ >> >> Should be fixed by https://patchwork.linuxtv.org/patch/52641/ >> >>> >>> KASAN: use-after-free Read in wake_up_if_idle >>> https://groups.google.com/forum/#!msg/syzkaller-bugs/aBWb_yV1kiI/sWQO63fkCQAJ >>> >>> KASAN: use-after-free Read in __vb2_perform_fileio >>> https://groups.google.com/forum/#!msg/syzkaller-bugs/MdFCZHz0LUQ/qSK_bFbcCQAJ >>> >>> INFO: task hung in vivid_stop_generating_vid_cap >>> https://groups.google.com/forum/#!msg/syzkaller-bugs/F_KFW6PVyTA/wTBeHLfTCQAJ >>> >>> KASAN: null-ptr-deref Write in kthread_stop >>> https://groups.google.com/forum/#!msg/syzkaller-bugs/u0AGnYvSlf4/fUiyfA_TCQAJ >> >> These last two should be fixed by https://patchwork.linuxtv.org/patch/52640/ >> >> Haven't figured out the others yet (hope to get back to that next week). >> >>> >>> Based on this I think if we put more effort into media fuzzing, it >>> will be able to find dozens more. >> >> Yeah, this is good stuff. Thank you for setting this up. > > Agreed, Dmitry thank you for doing this. > >> >>> >>> syzkaller needs descriptions of kernel interfaces to efficiently cover >>> a subsystem. For example, see: >>> https://github.com/google/syzkaller/blob/master/sys/linux/uinput.txt >>> Hopefully you can read it without much explanation, it basically >>> states that there is that node in /dev and here are ioctls and other >>> syscalls that are relevant for this device and here are types of >>> arguments and layout of involved data structures. >>> >>> Turned we actually have such descriptions for /dev/video* and >>> /dev/v4l-subdev*: >>> https://github.com/google/syzkaller/blob/master/sys/linux/video4linux.txt >>> But we don't have anything for /dev/media*, fuzzer merely knows that >>> it can open the device: >>> https://github.com/google/syzkaller/blob/12b38f22c18c6109a5cc1c0238d015eef121b9b7/sys/linux/sys.txt#L479 >>> and then it will just blindly execute completely random workload on >>> it, e.g. most likely it won't be able to come up with a proper complex >>> structure layout for some ioctls. And I am actually not completely >>> sure about completeness and coverage of video4linux.txt descriptions >>> too as they were contributed by somebody interested in android >>> testing. >> >> A quick look suggests that it is based on the 4.9 videodev2.h, which ain't >> too bad. There are some differences between the 4.20 videodev2.h and the >> 4.9, but not too many. >> >>> >>> I wonder if somebody knowledgeable in /dev/media interface be willing >>> to contribute additional descriptions? >> >> We'll have to wait for 4.20-rc1 to be released since there are important >> additions to the media API. I can probably come up with something, I'm >> just not sure when I get around to it. Ping me in three weeks time if you >> haven't heard from me. >> >>> >>> We also have code coverage reports with the coverage fuzzer achieved >>> so far. Here in the Cover column: >>> https://syzkaller.appspot.com/#managers >>> e.g. this one (but note this is a ~80MB html file): >>> https://storage.googleapis.com/syzkaller/cover/ci-upstream-kasan-gce-root.html >>> This can be used to assess e.g. v4l coverage. But I don't know what's >>> coverable in general from syscalls and what's coverable via the stub >>> drivers in particular. So some expertise from media developers would >>> be helpful too. >> >> The four virtual drivers should give pretty decent coverage of the core >> code. Are you able to test with a 32-bit syzkaller application on a 64-bit >> kernel as well? That way the compat32 code is tested. >> >>> >>> Do I understand it correctly that when a process opens
Re: [PATCH 2/2] media: drxk_hard: check if parameter is not NULL
On Fri, Dec 7, 2018 at 5:08 AM Mauro Carvalho Chehab
wrote:
>
> There is a smatch warning:
> drivers/media/dvb-frontends/drxk_hard.c:
> drivers/media/dvb-frontends/drxk_hard.c:1478 scu_command() error: we
> previously assumed 'parameter' could be null (see line 1467)
>
> Telling that parameter might be NULL. Well, it can't, due to the
> way the driver works, but it doesn't hurt to add a check, in order
> to shut up smatch.
eh, yeah this function is kind of odd; the early return conditions are
a little tricky, but I agree that this check doesn't hurt to add.
Reviewed-by: Nick Desaulniers
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/dvb-frontends/drxk_hard.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/drxk_hard.c
> b/drivers/media/dvb-frontends/drxk_hard.c
> index 84ac3f73f8fe..8ea1e45be710 100644
> --- a/drivers/media/dvb-frontends/drxk_hard.c
> +++ b/drivers/media/dvb-frontends/drxk_hard.c
> @@ -1474,9 +1474,11 @@ static int scu_command(struct drxk_state *state,
>
> /* assume that the command register is ready
> since it is checked afterwards */
> - for (ii = parameter_len - 1; ii >= 0; ii -= 1) {
> - buffer[cnt++] = (parameter[ii] & 0xFF);
> - buffer[cnt++] = ((parameter[ii] >> 8) & 0xFF);
> + if (parameter) {
> + for (ii = parameter_len - 1; ii >= 0; ii -= 1) {
> + buffer[cnt++] = (parameter[ii] & 0xFF);
> + buffer[cnt++] = ((parameter[ii] >> 8) & 0xFF);
> + }
> }
> buffer[cnt++] = (cmd & 0xFF);
> buffer[cnt++] = ((cmd >> 8) & 0xFF);
> --
> 2.19.2
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture
On 12/07/2018 03:30 PM, Mauro Carvalho Chehab wrote: > Em Thu, 6 Sep 2018 11:02:28 +0200 > Hans Verkuil escreveu: > >> Hi Philipp, >> >> It is much appreciated that this old RFC of mine is picked up again. >> I always wanted to get this in, but I never had a driver where it would >> make sense to do so. > > What's the status of this? Changes were requested, and there was some discussion. I'm basically waiting for an update. I've delegated it to me. Regards, Hans > > Hans, > As this is an old RFC from you, I'll delegate it to you at patchwork, > for you to track it. > > Regards, > Mauro > >> >> On 09/05/2018 07:09 PM, Philipp Zabel wrote: >>> For video capture it is the driver that reports the colorspace, >> >> add: "transfer function," >> >>> Y'CbCr/HSV encoding and quantization range used by the video, and there >>> is no way to request something different, even though many HDTV >>> receivers have some sort of colorspace conversion capabilities. >>> >>> For output video this feature already exists since the application >>> specifies this information for the video format it will send out, and >>> the transmitter will enable any available CSC if a format conversion has >>> to be performed in order to match the capabilities of the sink. >>> >>> For video capture we propose adding new pix_format flags: >>> V4L2_PIX_FMT_FLAG_CSC_COLORSPACE, V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC, >>> V4L2_PIX_FMT_FLAG_CSC_HSV_ENC, V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION, and >>> V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC. These are set by the driver to indicate >>> its conversion features. When set by the application, the driver will >>> interpret the colorspace, ycbcr_enc/hsv_enc, quantization and xfer_func >>> fields as the requested colorspace information and will attempt to do >>> the conversion it supports. >>> >>> Drivers do not have to actually look at the flags: if the flags are not >>> set, then the colorspace, ycbcr_enc and quantization fields are set to >>> the default values by the core, i.e. just pass on the received format >>> without conversion. >> >> Thinking about this some more, I don't think this is quite the right >> approach. >> Having userspace set these flags with S_FMT if they want to do explicit >> conversions makes sense, and that part we can keep. >> >> But to signal the capabilities I think should be done via new flags for >> VIDIOC_ENUM_FMT. Basically the same set of flags, but for the flags field >> of struct v4l2_fmtdesc. >> >> One thing that's not clear to me is what happens if userspace sets one or >> more flags and calls S_FMT for a driver that doesn't support this. Are the >> flags zeroed in that case upon return? I don't think so, but I think that >> is already true for the existing flag V4L2_PIX_FMT_FLAG_PREMUL_ALPHA. >> >> I wonder if V4L2_PIX_FMT_FLAG_PREMUL_ALPHA should also get an equivalent >> flag for v4l2_fmtdesc. >> >> Then we can just document that v4l2_format flags are only valid if they >> are also defined in v4l2_fmtdesc. >> >> Does anyone have better ideas for this? >> >> Regards, >> >> Hans >> >>> >>> Signed-off-by: Hans Verkuil >>> Signed-off-by: Philipp Zabel >>> --- >>> Changes since v1 [1] >>> - convert to rst >>> - split V4L2_PIX_FMT_FLAG_REQUEST_CSC into four separate flags for >>>colorspace, ycbcr_enc/hsv_enc, quantization, and xfer_func >>> - let driver set flags to indicate supported features >>> >>> [1] https://patchwork.linuxtv.org/patch/28847/ >>> --- >>> .../media/uapi/v4l/pixfmt-reserved.rst| 41 +++ >>> .../media/uapi/v4l/pixfmt-v4l2-mplane.rst | 16 ++-- >>> Documentation/media/uapi/v4l/pixfmt-v4l2.rst | 37 ++--- >>> drivers/media/v4l2-core/v4l2-ioctl.c | 12 ++ >>> include/uapi/linux/videodev2.h| 5 +++ >>> 5 files changed, 94 insertions(+), 17 deletions(-) >>> >>> diff --git a/Documentation/media/uapi/v4l/pixfmt-reserved.rst >>> b/Documentation/media/uapi/v4l/pixfmt-reserved.rst >>> index 38af1472a4b4..c1090027626c 100644 >>> --- a/Documentation/media/uapi/v4l/pixfmt-reserved.rst >>> +++ b/Documentation/media/uapi/v4l/pixfmt-reserved.rst >>> @@ -261,3 +261,44 @@ please make a proposal on the linux-media mailing list. >>> by RGBA values (128, 192, 255, 128), the same pixel described with >>> premultiplied colors would be described by RGBA values (64, 96, >>> 128, 128) >>> +* - ``V4L2_PIX_FMT_FLAG_CSC_COLORSPACE`` >>> + - 0x0002 >>> + - Set by the driver to indicate colorspace conversion support. Set >>> by the >>> + application to request conversion to the specified colorspace. It is >>> + only used for capture and is ignored for output streams. If set by the >>> + application, then request the driver to do colorspace conversion from >>> + the received colorspace to the requested colorspace by setting the >>> + ``colorspace`` field of struct :c:type:`v4l2_pix_format`. >>> +* - ``V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC`` >>> +
Re: [PATCH v2] [RFC v2] v4l2: add support for colorspace conversion for video capture
Em Thu, 6 Sep 2018 11:02:28 +0200 Hans Verkuil escreveu: > Hi Philipp, > > It is much appreciated that this old RFC of mine is picked up again. > I always wanted to get this in, but I never had a driver where it would > make sense to do so. What's the status of this? Hans, As this is an old RFC from you, I'll delegate it to you at patchwork, for you to track it. Regards, Mauro > > On 09/05/2018 07:09 PM, Philipp Zabel wrote: > > For video capture it is the driver that reports the colorspace, > > add: "transfer function," > > > Y'CbCr/HSV encoding and quantization range used by the video, and there > > is no way to request something different, even though many HDTV > > receivers have some sort of colorspace conversion capabilities. > > > > For output video this feature already exists since the application > > specifies this information for the video format it will send out, and > > the transmitter will enable any available CSC if a format conversion has > > to be performed in order to match the capabilities of the sink. > > > > For video capture we propose adding new pix_format flags: > > V4L2_PIX_FMT_FLAG_CSC_COLORSPACE, V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC, > > V4L2_PIX_FMT_FLAG_CSC_HSV_ENC, V4L2_PIX_FMT_FLAG_CSC_QUANTIZATION, and > > V4L2_PIX_FMT_FLAG_CSC_XFER_FUNC. These are set by the driver to indicate > > its conversion features. When set by the application, the driver will > > interpret the colorspace, ycbcr_enc/hsv_enc, quantization and xfer_func > > fields as the requested colorspace information and will attempt to do > > the conversion it supports. > > > > Drivers do not have to actually look at the flags: if the flags are not > > set, then the colorspace, ycbcr_enc and quantization fields are set to > > the default values by the core, i.e. just pass on the received format > > without conversion. > > Thinking about this some more, I don't think this is quite the right approach. > Having userspace set these flags with S_FMT if they want to do explicit > conversions makes sense, and that part we can keep. > > But to signal the capabilities I think should be done via new flags for > VIDIOC_ENUM_FMT. Basically the same set of flags, but for the flags field > of struct v4l2_fmtdesc. > > One thing that's not clear to me is what happens if userspace sets one or > more flags and calls S_FMT for a driver that doesn't support this. Are the > flags zeroed in that case upon return? I don't think so, but I think that > is already true for the existing flag V4L2_PIX_FMT_FLAG_PREMUL_ALPHA. > > I wonder if V4L2_PIX_FMT_FLAG_PREMUL_ALPHA should also get an equivalent > flag for v4l2_fmtdesc. > > Then we can just document that v4l2_format flags are only valid if they > are also defined in v4l2_fmtdesc. > > Does anyone have better ideas for this? > > Regards, > > Hans > > > > > Signed-off-by: Hans Verkuil > > Signed-off-by: Philipp Zabel > > --- > > Changes since v1 [1] > > - convert to rst > > - split V4L2_PIX_FMT_FLAG_REQUEST_CSC into four separate flags for > >colorspace, ycbcr_enc/hsv_enc, quantization, and xfer_func > > - let driver set flags to indicate supported features > > > > [1] https://patchwork.linuxtv.org/patch/28847/ > > --- > > .../media/uapi/v4l/pixfmt-reserved.rst| 41 +++ > > .../media/uapi/v4l/pixfmt-v4l2-mplane.rst | 16 ++-- > > Documentation/media/uapi/v4l/pixfmt-v4l2.rst | 37 ++--- > > drivers/media/v4l2-core/v4l2-ioctl.c | 12 ++ > > include/uapi/linux/videodev2.h| 5 +++ > > 5 files changed, 94 insertions(+), 17 deletions(-) > > > > diff --git a/Documentation/media/uapi/v4l/pixfmt-reserved.rst > > b/Documentation/media/uapi/v4l/pixfmt-reserved.rst > > index 38af1472a4b4..c1090027626c 100644 > > --- a/Documentation/media/uapi/v4l/pixfmt-reserved.rst > > +++ b/Documentation/media/uapi/v4l/pixfmt-reserved.rst > > @@ -261,3 +261,44 @@ please make a proposal on the linux-media mailing list. > > by RGBA values (128, 192, 255, 128), the same pixel described with > > premultiplied colors would be described by RGBA values (64, 96, > > 128, 128) > > +* - ``V4L2_PIX_FMT_FLAG_CSC_COLORSPACE`` > > + - 0x0002 > > + - Set by the driver to indicate colorspace conversion support. Set > > by the > > + application to request conversion to the specified colorspace. It is > > + only used for capture and is ignored for output streams. If set by the > > + application, then request the driver to do colorspace conversion from > > + the received colorspace to the requested colorspace by setting the > > + ``colorspace`` field of struct :c:type:`v4l2_pix_format`. > > +* - ``V4L2_PIX_FMT_FLAG_CSC_YCBCR_ENC`` > > + - 0x0004 > > + - Set by the driver to indicate Y'CbCr encoding conversion support. > > Set > > + by the application to request conversion to the specified Y'CbCr > > + encoding. It is only used for capture and is ignored for
Urgently need money? We can help you!
Urgently need money? We can help you! Are you by the current situation in trouble or threatens you in trouble? In this way, we give you the ability to take a new development. As a rich person I feel obliged to assist people who are struggling to give them a chance. Everyone deserved a second chance and since the Government fails, it will have to come from others. No amount is too crazy for us and the maturity we determine by mutual agreement. No surprises, no extra costs, but just the agreed amounts and nothing else. Don't wait any longer and comment on this post. Please specify the amount you want to borrow and we will contact you with all the possibilities. contact us today at stewarrt.l...@gmail.com
Re: [PATCH v9 00/13] media: staging/imx7: add i.MX7 media driver
Hi Hans and Dan, On Fri 07 Dec 2018 at 13:38, Dan Carpenter wrote: On Fri, Dec 07, 2018 at 01:44:00PM +0100, Hans Verkuil wrote: CHECK: Alignment should match open parenthesis #936: FILE: drivers/staging/media/imx/imx7-mipi-csis.c:921: + ret = v4l2_async_register_fwnode_subdev(mipi_sd, + sizeof(struct v4l2_async_subdev), _port, 1, Apparently the latest coding style is that alignment is more important than line length, although I personally do not agree. But since you need to respin in any case due to the wrong SPDX identifier you used you might as well take this into account. I added this in the cover letter: - some alignment parenthesis that were left as they are, to be more readable this ones were left as they are as they seem impossible to fit all rules. I hope you really do not mind about this checks. if you have a strong opinion about this let me know, otherwise I will leave this as it is. I'm pretty sure it complains about both equally. If you make fix one warning it will complain about the other. So you just have to pick which warning to not care about. Yeah, I agree with you, I normally like to avoid at all cost the line length issue. --- Cheers, Rui regards, dan carpenter
Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API
Em Fri, 7 Dec 2018 11:53:17 -0200 Mauro Carvalho Chehab escreveu: > Em Fri, 7 Dec 2018 14:27:48 +0100 > Hans Verkuil escreveu: > > > On 12/07/2018 01:42 PM, Mauro Carvalho Chehab wrote: > > > Em Fri, 7 Dec 2018 12:47:24 +0100 > > > Hans Verkuil escreveu: > > > > > >> On 12/07/2018 12:26 PM, Mauro Carvalho Chehab wrote: > > >>> Em Fri, 7 Dec 2018 10:09:04 +0100 > > >>> Hans Verkuil escreveu: > > >>> > > This patch selects MEDIA_CONTROLLER for all camera, analog TV and > > digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically. > > > > This will allow us to simplify drivers that currently have to add > > #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API > > to their code, since now this will always be available. > > > > The original intent of allowing these to be configured by the > > user was (I think) to save a bit of memory. > > >>> > > >>> No. The original intent was/is to be sure that adding the media > > >>> controller support won't be breaking existing working drivers. > > >> > > >> That doesn't make sense. If enabling this option would break existing > > >> drivers, then something is really wrong, isn't it? > > > > > > It is the opposite: disabling it should not break any driver that don't > > > depend on them to work. > > > > > >> > > >>> > > But as more and more > > drivers have a media controller and all regular distros already > > enable one or more of those drivers, the memory for the MC code is > > there anyway. > > > > Complexity has always been the bane of media drivers, so reducing > > complexity at the expense of a bit more memory (which is a rounding > > error compared to the amount of video buffer memory needed) is IMHO > > a good thing. > > > > Signed-off-by: Hans Verkuil > > --- > > diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig > > index 8add62a18293..56eb01cc8bb4 100644 > > --- a/drivers/media/Kconfig > > +++ b/drivers/media/Kconfig > > @@ -31,6 +31,7 @@ comment "Multimedia core support" > > # > > config MEDIA_CAMERA_SUPPORT > > bool "Cameras/video grabbers support" > > + select MEDIA_CONTROLLER > > ---help--- > > Enable support for webcams and video grabbers. > > > > @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT > > > > config MEDIA_ANALOG_TV_SUPPORT > > bool "Analog TV support" > > + select MEDIA_CONTROLLER > > ---help--- > > Enable analog TV support. > > > > @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT > > > > config MEDIA_DIGITAL_TV_SUPPORT > > bool "Digital TV support" > > + select MEDIA_CONTROLLER > > ---help--- > > Enable digital TV support. > > >>> > > >>> See my comments below. > > >>> > > > > @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig" > > > > config MEDIA_CONTROLLER > > bool "Media Controller API" > > - depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || > > MEDIA_DIGITAL_TV_SUPPORT > > ---help--- > > Enable the media controller API used to query media devices > > internal > > topology and configure it dynamically. > > >>> > > >>> I have split comments with regards to it. Yeah, nowadays media > > >>> controller > > >>> has becoming more important. Still, a lot of media drivers work fine > > >>> without them. > > >>> > > >>> Anyway, if we're willing to make it mandatory, better to just remove the > > >>> entire config option or to make it a silent one. > > >> > > >> Well, that assumes that the media controller will only be used by media > > >> drivers, and not alsa or anyone else who wants to experiment with the MC. > > >> > > >> I won't object to making it silent (since it does reflect the current > > >> situation), but since this functionality is not actually specific to > > >> media > > >> drivers I think that is a good case to be made to allow it to be selected > > >> manually. > > >> > > >>> > > @@ -119,16 +121,11 @@ config VIDEO_DEV > > tristate > > depends on MEDIA_SUPPORT > > depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || > > MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT > > + select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER > > default y > > > > config VIDEO_V4L2_SUBDEV_API > > - bool "V4L2 sub-device userspace API" > > - depends on VIDEO_DEV && MEDIA_CONTROLLER > > - ---help--- > > -Enables the V4L2 sub-device pad-level userspace API used to > > configure > > -video format, size and frame rate between hardware blocks. > > - > > -This API is mostly used by
Re: [PATCH v9 05/13] media: dt-bindings: add bindings for i.MX7 media driver
Hi Hans,
On Fri 07 Dec 2018 at 12:39, Hans Verkuil wrote:
On 11/22/2018 04:18 PM, Rui Miguel Silva wrote:
Add bindings documentation for i.MX7 media drivers.
The imx7 MIPI CSI2 and imx7 CMOS Sensor Interface.
Signed-off-by: Rui Miguel Silva
Reviewed-by: Rob Herring
Acked-by: Sakari Ailus
Please move this patch to the beginning of the series to avoid
checkpatch warnings:
WARNING: DT compatible string "fsl,imx7-csi" appears
un-documented -- check ./Documentation/devicetree/bindings/
#1378: FILE: drivers/staging/media/imx/imx7-media-csi.c:1336:
+ { .compatible = "fsl,imx7-csi" },
Will do, thanks for catching this one.
---
Cheers,
Rui
Thanks!
Hans
---
.../devicetree/bindings/media/imx7-csi.txt| 45 ++
.../bindings/media/imx7-mipi-csi2.txt | 90
+++
2 files changed, 135 insertions(+)
create mode 100644
Documentation/devicetree/bindings/media/imx7-csi.txt
create mode 100644
Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
diff --git
a/Documentation/devicetree/bindings/media/imx7-csi.txt
b/Documentation/devicetree/bindings/media/imx7-csi.txt
new file mode 100644
index ..3c07bc676bc3
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/imx7-csi.txt
@@ -0,0 +1,45 @@
+Freescale i.MX7 CMOS Sensor Interface
+=
+
+csi node
+
+
+This is device node for the CMOS Sensor Interface (CSI) which
enables the chip
+to connect directly to external CMOS image sensors.
+
+Required properties:
+
+- compatible: "fsl,imx7-csi";
+- reg : base address and length of the register set
for the device;
+- interrupts: should contain CSI interrupt;
+- clocks: list of clock specifiers, see
+
Documentation/devicetree/bindings/clock/clock-bindings.txt for
details;
+- clock-names : must contain "axi", "mclk" and "dcic"
entries, matching
+ entries in the clock property;
+
+The device node shall contain one 'port' child node with one
child 'endpoint'
+node, according to the bindings defined in:
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+In the following example a remote endpoint is a video
multiplexer.
+
+example:
+
+csi: csi@3071 {
+#address-cells = <1>;
+#size-cells = <0>;
+
+compatible = "fsl,imx7-csi";
+reg = <0x3071 0x1>;
+interrupts = IRQ_TYPE_LEVEL_HIGH>;
+clocks = < IMX7D_CLK_DUMMY>,
+<
IMX7D_CSI_MCLK_ROOT_CLK>,
+<
IMX7D_CLK_DUMMY>;
+clock-names = "axi", "mclk", "dcic";
+
+port {
+csi_from_csi_mux: endpoint {
+remote-endpoint =
<_mux_to_csi>;
+};
+};
+};
diff --git
a/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
b/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
new file mode 100644
index ..71fd74ed3ec8
--- /dev/null
+++
b/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
@@ -0,0 +1,90 @@
+Freescale i.MX7 Mipi CSI2
+=
+
+mipi_csi2 node
+--
+
+This is the device node for the MIPI CSI-2 receiver core in
i.MX7 SoC. It is
+compatible with previous version of Samsung D-phy.
+
+Required properties:
+
+- compatible: "fsl,imx7-mipi-csi2";
+- reg : base address and length of the register set
for the device;
+- interrupts: should contain MIPI CSIS interrupt;
+- clocks: list of clock specifiers, see
+
Documentation/devicetree/bindings/clock/clock-bindings.txt for
details;
+- clock-names : must contain "pclk", "wrap" and "phy"
entries, matching
+ entries in the clock property;
+- power-domains : a phandle to the power domain, see
+
Documentation/devicetree/bindings/power/power_domain.txt for
details.
+- reset-names : should include following entry "mrst";
+- resets: a list of phandle, should contain reset
entry of
+ reset-names;
+- phy-supply: from the generic phy bindings, a phandle to
a regulator that
+ provides power to MIPI CSIS core;
+
+Optional properties:
+
+- clock-frequency : The IP's main (system bus) clock frequency
in Hz, default
+ value when this property is not specified is
166 MHz;
+- fsl,csis-hs-settle : differential receiver (HS-RX) settle
time;
+
+The device node should contain two 'port' child nodes with one
child 'endpoint'
+node, according to the bindings defined in:
+ Documentation/devicetree/bindings/
media/video-interfaces.txt.
+ The following are properties specific to those nodes.
+
+port node
+-
+
+- reg : (required) can take
Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API
Em Fri, 7 Dec 2018 14:27:48 +0100 Hans Verkuil escreveu: > On 12/07/2018 01:42 PM, Mauro Carvalho Chehab wrote: > > Em Fri, 7 Dec 2018 12:47:24 +0100 > > Hans Verkuil escreveu: > > > >> On 12/07/2018 12:26 PM, Mauro Carvalho Chehab wrote: > >>> Em Fri, 7 Dec 2018 10:09:04 +0100 > >>> Hans Verkuil escreveu: > >>> > This patch selects MEDIA_CONTROLLER for all camera, analog TV and > digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically. > > This will allow us to simplify drivers that currently have to add > #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API > to their code, since now this will always be available. > > The original intent of allowing these to be configured by the > user was (I think) to save a bit of memory. > >>> > >>> No. The original intent was/is to be sure that adding the media > >>> controller support won't be breaking existing working drivers. > >> > >> That doesn't make sense. If enabling this option would break existing > >> drivers, then something is really wrong, isn't it? > > > > It is the opposite: disabling it should not break any driver that don't > > depend on them to work. > > > >> > >>> > But as more and more > drivers have a media controller and all regular distros already > enable one or more of those drivers, the memory for the MC code is > there anyway. > > Complexity has always been the bane of media drivers, so reducing > complexity at the expense of a bit more memory (which is a rounding > error compared to the amount of video buffer memory needed) is IMHO > a good thing. > > Signed-off-by: Hans Verkuil > --- > diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig > index 8add62a18293..56eb01cc8bb4 100644 > --- a/drivers/media/Kconfig > +++ b/drivers/media/Kconfig > @@ -31,6 +31,7 @@ comment "Multimedia core support" > # > config MEDIA_CAMERA_SUPPORT > bool "Cameras/video grabbers support" > +select MEDIA_CONTROLLER > ---help--- > Enable support for webcams and video grabbers. > > @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT > > config MEDIA_ANALOG_TV_SUPPORT > bool "Analog TV support" > +select MEDIA_CONTROLLER > ---help--- > Enable analog TV support. > > @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT > > config MEDIA_DIGITAL_TV_SUPPORT > bool "Digital TV support" > +select MEDIA_CONTROLLER > ---help--- > Enable digital TV support. > >>> > >>> See my comments below. > >>> > > @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig" > > config MEDIA_CONTROLLER > bool "Media Controller API" > -depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || > MEDIA_DIGITAL_TV_SUPPORT > ---help--- > Enable the media controller API used to query media devices > internal > topology and configure it dynamically. > >>> > >>> I have split comments with regards to it. Yeah, nowadays media controller > >>> has becoming more important. Still, a lot of media drivers work fine > >>> without them. > >>> > >>> Anyway, if we're willing to make it mandatory, better to just remove the > >>> entire config option or to make it a silent one. > >> > >> Well, that assumes that the media controller will only be used by media > >> drivers, and not alsa or anyone else who wants to experiment with the MC. > >> > >> I won't object to making it silent (since it does reflect the current > >> situation), but since this functionality is not actually specific to media > >> drivers I think that is a good case to be made to allow it to be selected > >> manually. > >> > >>> > @@ -119,16 +121,11 @@ config VIDEO_DEV > tristate > depends on MEDIA_SUPPORT > depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || > MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT > +select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER > default y > > config VIDEO_V4L2_SUBDEV_API > -bool "V4L2 sub-device userspace API" > -depends on VIDEO_DEV && MEDIA_CONTROLLER > ----help--- > - Enables the V4L2 sub-device pad-level userspace API used to > configure > - video format, size and frame rate between hardware blocks. > - > - This API is mostly used by camera interfaces in embedded > platforms. > +bool > >>> > >>> NACK. > >>> > >>> There is a very good reason why the subdev API is optional: there > >>> are drivers that use camera sensors but are not MC-centric. On those, > >>> the USB bridge
Re: [PATCH v9 01/13] media: staging/imx: refactor imx media device probe
Hi Hans, Thanks for the review. On Fri 07 Dec 2018 at 12:38, Hans Verkuil wrote: On 11/22/2018 04:18 PM, Rui Miguel Silva wrote: Refactor and move media device initialization code to a new common module, so it can be used by other devices, this will allow for example a near to introduce imx7 CSI driver, to use this media device. Signed-off-by: Rui Miguel Silva --- drivers/staging/media/imx/Makefile| 1 + .../staging/media/imx/imx-media-dev-common.c | 102 ++ drivers/staging/media/imx/imx-media-dev.c | 88 --- drivers/staging/media/imx/imx-media-of.c | 6 +- drivers/staging/media/imx/imx-media.h | 15 +++ 5 files changed, 141 insertions(+), 71 deletions(-) create mode 100644 drivers/staging/media/imx/imx-media-dev-common.c diff --git a/drivers/staging/media/imx/Makefile b/drivers/staging/media/imx/Makefile index 698a4210316e..a30b3033f9a3 100644 --- a/drivers/staging/media/imx/Makefile +++ b/drivers/staging/media/imx/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 imx-media-objs := imx-media-dev.o imx-media-internal-sd.o imx-media-of.o +imx-media-objs += imx-media-dev-common.o imx-media-common-objs := imx-media-utils.o imx-media-fim.o imx-media-ic-objs := imx-ic-common.o imx-ic-prp.o imx-ic-prpencvf.o diff --git a/drivers/staging/media/imx/imx-media-dev-common.c b/drivers/staging/media/imx/imx-media-dev-common.c new file mode 100644 index ..55fe94fb72f2 --- /dev/null +++ b/drivers/staging/media/imx/imx-media-dev-common.c @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: GPL This is an invalid SPDX license identifier. You probably want to use GPL-2.0. hrr... you are right, I will update it here and others. --- Cheers, Rui
Re: [PATCH v2] media: cedrus: don't initialize pointers with zero
Em Fri, 07 Dec 2018 14:21:44 +0100
Paul Kocialkowski escreveu:
> Hi,
>
> On Fri, 2018-12-07 at 08:03 -0500, Mauro Carvalho Chehab wrote:
> > A common mistake is to assume that initializing a var with:
> > struct foo f = { 0 };
> >
> > Would initialize a zeroed struct. Actually, what this does is
> > to initialize the first element of the struct to zero.
> >
> > According to C99 Standard 6.7.8.21:
> >
> > "If there are fewer initializers in a brace-enclosed
> > list than there are elements or members of an aggregate,
> > or fewer characters in a string literal used to initialize
> > an array of known size than there are elements in the array,
> > the remainder of the aggregate shall be initialized implicitly
> > the same as objects that have static storage duration."
> >
> > So, in practice, it could zero the entire struct, but, if the
> > first element is not an integer, it will produce warnings:
> >
> >
> > drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
> > warning: Using plain integer as NULL pointer
> >
> > drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
> > warning: Using plain integer as NULL pointer
> >
> > As the right initialization would be, instead:
> >
> > struct foo f = { NULL };
>
> Thanks for sharing these details, it's definitely interesting and good
> to know :)
Yeah, that's something that was bothering for quite a while, as I've
seen patches using either one of the ways. It took me a while to
do some research, and having it documented at the patch helps, as
we should now handle it the same way for similar stuff :-)
>
> > Another way to initialize it with gcc is to use:
> >
> > struct foo f = {};
> >
> > That seems to be a gcc extension, but clang also does the right thing,
> > and that's a clean way for doing it.
> >
> > Anyway, I decided to check upstream what's the most commonly pattern.
> > The "= {}" pattern has about 2000 entries:
> >
> > $ git grep -E "=\s*\{\s*\}"|wc -l
> > 1951
> >
> > The standard-C compliant pattern has about 2500 entries:
> >
> > $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l
> > 137
> > $ git grep -E "=\s*\{\s*0\s*\}"|wc -l
> > 2323
> >
> > Meaning that developers have split options on that.
> >
> > So, let's opt to the simpler form.
>
> Acked-by: Paul Kocialkowski
Applied, thanks!
>
> > Signed-off-by: Mauro Carvalho Chehab
> > ---
> > drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +-
> > drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > index b538eb0321d8..b7c918fa5fd1 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > @@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev,
> > struct cedrus_ctx *ctx)
> > memset(ctx->ctrls, 0, ctrl_size);
> >
> > for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
> > - struct v4l2_ctrl_config cfg = { 0 };
> > + struct v4l2_ctrl_config cfg = {};
> >
> > cfg.elem_size = cedrus_controls[i].elem_size;
> > cfg.id = cedrus_controls[i].id;
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > index e40180a33951..f10c25f5460e 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > @@ -26,7 +26,7 @@ void cedrus_device_run(void *priv)
> > {
> > struct cedrus_ctx *ctx = priv;
> > struct cedrus_dev *dev = ctx->dev;
> > - struct cedrus_run run = { 0 };
> > + struct cedrus_run run = {};
> > struct media_request *src_req;
> > unsigned long flags;
> >
Thanks,
Mauro
Re: [PATCH v9 00/13] media: staging/imx7: add i.MX7 media driver
On Fri, Dec 07, 2018 at 01:44:00PM +0100, Hans Verkuil wrote: > CHECK: Alignment should match open parenthesis > #936: FILE: drivers/staging/media/imx/imx7-mipi-csis.c:921: > + ret = v4l2_async_register_fwnode_subdev(mipi_sd, > + sizeof(struct v4l2_async_subdev), _port, > 1, > > Apparently the latest coding style is that alignment is more important than > line length, although I personally do not agree. But since you need to > respin in any case due to the wrong SPDX identifier you used you might as > well take this into account. I'm pretty sure it complains about both equally. If you make fix one warning it will complain about the other. So you just have to pick which warning to not care about. regards, dan carpenter
Re: [PATCH v5 00/12] imx-media: Fixes for interlaced capture
Hi Steve, How to proceed with this w.r.t. the two gpu ipu patches? Are those going in first through the gpu tree? Or do they have to go in through our tree? In that case I need Acks from whoever maintains that code. Regards, Hans On 10/17/2018 02:00 AM, Steve Longerbeam wrote: > A set of patches that fixes some bugs with capturing from an > interlaced source, and incompatibilites between IDMAC interlace > interweaving and 4:2:0 data write reduction. > > History: > v5: > - Added a regression fix to allow empty endpoints to CSI (fix for imx6q > SabreAuto). > - Cleaned up some convoluted code in ipu_csi_init_interface(), suggested > by Philipp Zabel. > - Fixed a regression in csi_setup(), caught by Philipp. > - Removed interweave_offset and replace with boolean interweave_swap, > suggested by Philipp. > - Make clear that it is IDMAC channel that does pixel reordering and > interweave, not the CSI, in the imx.rst doc, caught by Philipp. > > v4: > - rebased to latest media-tree master branch. > - Make patch author and SoB email addresses the same. > > v3: > - add support for/fix interweaved scan with YUV planar output. > - fix bug in 4:2:0 U/V offset macros. > - add patch that generalizes behavior of field swap in > ipu_csi_init_interface(). > - add support for interweaved scan with field order swap. > Suggested by Philipp Zabel. > - in v2, inteweave scan was determined using field types of > CSI (and PRPENCVF) at the sink and source pads. In v3, this > has been moved one hop downstream: interweave is now determined > using field type at source pad, and field type selected at > capture interface. Suggested by Philipp. > - make sure to double CSI crop target height when input field > type in alternate. > - more updates to media driver doc to reflect above. > > v2: > - update media driver doc. > - enable idmac interweave only if input field is sequential/alternate, > and output field is 'interlaced*'. > - move field try logic out of *try_fmt and into separate function. > - fix bug with resetting crop/compose rectangles. > - add a patch that fixes a field order bug in VDIC indirect mode. > - remove alternate field type from V4L2_FIELD_IS_SEQUENTIAL() macro > Suggested-by: Nicolas Dufresne . > - add macro V4L2_FIELD_IS_INTERLACED(). > > > Steve Longerbeam (12): > media: videodev2.h: Add more field helper macros > gpu: ipu-csi: Swap fields according to input/output field types > gpu: ipu-v3: Add planar support to interlaced scan > media: imx: Fix field negotiation > media: imx-csi: Input connections to CSI should be optional > media: imx-csi: Double crop height for alternate fields at sink > media: imx: interweave and odd-chroma-row skip are incompatible > media: imx-csi: Allow skipping odd chroma rows for YVU420 > media: imx: vdic: rely on VDIC for correct field order > media: imx-csi: Move crop/compose reset after filling default mbus > fields > media: imx: Allow interweave with top/bottom lines swapped > media: imx.rst: Update doc to reflect fixes to interlaced capture > > Documentation/media/v4l-drivers/imx.rst | 103 +++ > drivers/gpu/ipu-v3/ipu-cpmem.c| 26 ++- > drivers/gpu/ipu-v3/ipu-csi.c | 119 + > drivers/staging/media/imx/imx-ic-prpencvf.c | 46 +++-- > drivers/staging/media/imx/imx-media-capture.c | 14 ++ > drivers/staging/media/imx/imx-media-csi.c | 168 +- > drivers/staging/media/imx/imx-media-vdic.c| 12 +- > include/uapi/linux/videodev2.h| 7 + > include/video/imx-ipu-v3.h| 6 +- > 9 files changed, 354 insertions(+), 147 deletions(-) >
Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API
On 12/07/2018 01:42 PM, Mauro Carvalho Chehab wrote: > Em Fri, 7 Dec 2018 12:47:24 +0100 > Hans Verkuil escreveu: > >> On 12/07/2018 12:26 PM, Mauro Carvalho Chehab wrote: >>> Em Fri, 7 Dec 2018 10:09:04 +0100 >>> Hans Verkuil escreveu: >>> This patch selects MEDIA_CONTROLLER for all camera, analog TV and digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically. This will allow us to simplify drivers that currently have to add #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API to their code, since now this will always be available. The original intent of allowing these to be configured by the user was (I think) to save a bit of memory. >>> >>> No. The original intent was/is to be sure that adding the media >>> controller support won't be breaking existing working drivers. >> >> That doesn't make sense. If enabling this option would break existing >> drivers, then something is really wrong, isn't it? > > It is the opposite: disabling it should not break any driver that don't > depend on them to work. > >> >>> But as more and more drivers have a media controller and all regular distros already enable one or more of those drivers, the memory for the MC code is there anyway. Complexity has always been the bane of media drivers, so reducing complexity at the expense of a bit more memory (which is a rounding error compared to the amount of video buffer memory needed) is IMHO a good thing. Signed-off-by: Hans Verkuil --- diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig index 8add62a18293..56eb01cc8bb4 100644 --- a/drivers/media/Kconfig +++ b/drivers/media/Kconfig @@ -31,6 +31,7 @@ comment "Multimedia core support" # config MEDIA_CAMERA_SUPPORT bool "Cameras/video grabbers support" + select MEDIA_CONTROLLER ---help--- Enable support for webcams and video grabbers. @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT config MEDIA_ANALOG_TV_SUPPORT bool "Analog TV support" + select MEDIA_CONTROLLER ---help--- Enable analog TV support. @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT config MEDIA_DIGITAL_TV_SUPPORT bool "Digital TV support" + select MEDIA_CONTROLLER ---help--- Enable digital TV support. >>> >>> See my comments below. >>> @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig" config MEDIA_CONTROLLER bool "Media Controller API" - depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT ---help--- Enable the media controller API used to query media devices internal topology and configure it dynamically. >>> >>> I have split comments with regards to it. Yeah, nowadays media controller >>> has becoming more important. Still, a lot of media drivers work fine >>> without them. >>> >>> Anyway, if we're willing to make it mandatory, better to just remove the >>> entire config option or to make it a silent one. >> >> Well, that assumes that the media controller will only be used by media >> drivers, and not alsa or anyone else who wants to experiment with the MC. >> >> I won't object to making it silent (since it does reflect the current >> situation), but since this functionality is not actually specific to media >> drivers I think that is a good case to be made to allow it to be selected >> manually. >> >>> @@ -119,16 +121,11 @@ config VIDEO_DEV tristate depends on MEDIA_SUPPORT depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT + select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER default y config VIDEO_V4L2_SUBDEV_API - bool "V4L2 sub-device userspace API" - depends on VIDEO_DEV && MEDIA_CONTROLLER - ---help--- -Enables the V4L2 sub-device pad-level userspace API used to configure -video format, size and frame rate between hardware blocks. - -This API is mostly used by camera interfaces in embedded platforms. + bool >>> >>> NACK. >>> >>> There is a very good reason why the subdev API is optional: there >>> are drivers that use camera sensors but are not MC-centric. On those, >>> the USB bridge driver is responsible to setup the subdevice. >>> >>> This options helps to ensure that camera sensors used by such drivers >>> won't stop working because of the lack of the subdev-API. >> >> But they won't stop working if this is enabled. > > That's not the issue. I've seen (and nacked) several patches breaking > drivers by assuming that all init would happen via subdev API. > > By having this as an optional feature that can be disabled, developers > need to ensure that either the driver won't be built as a hole, if > no
Re: [PATCH] media: cedrus: don't initialize pointers with zero
On Fri, Dec 07, 2018 at 09:31:06AM -0200, Mauro Carvalho Chehab wrote:
> Em Fri, 7 Dec 2018 12:14:50 +0100
> Hans Verkuil escreveu:
>
> > On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote:
> > > A common mistake is to assume that initializing a var with:
> > > struct foo f = { 0 };
> > >
> > > Would initialize a zeroed struct. Actually, what this does is
> > > to initialize the first element of the struct to zero.
> > >
> > > According to C99 Standard 6.7.8.21:
> > >
> > > "If there are fewer initializers in a brace-enclosed
> > > list than there are elements or members of an aggregate,
> > > or fewer characters in a string literal used to initialize
> > > an array of known size than there are elements in the array,
> > > the remainder of the aggregate shall be initialized implicitly
> > > the same as objects that have static storage duration."
> > >
> > > So, in practice, it could zero the entire struct, but, if the
> > > first element is not an integer, it will produce warnings:
> > >
> > >
> > > drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
> > > warning: Using plain integer as NULL pointer
> > >
> > > drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
> > > warning: Using plain integer as NULL pointer
> > >
> > > A proper way to initialize it with gcc is to use:
> > >
> > > struct foo f = { };
> > >
> > > But that seems to be a gcc extension. So, I decided to check upstream
> >
> > No, this is not a gcc extension. It's part of the latest C standard.
>
> Sure? Where the C standard spec states that? I've been seeking for
> such info for a while, as '= {}' is also my personal preference.
>
> I tried to build the Kernel with clang, just to be sure that this
> won't cause issues with the clang support
My test says that clang works with {}.
I support this in Smatch as well.
regards,
dan carpenter
Re: [PATCH v2] media: cedrus: don't initialize pointers with zero
Hi,
On Fri, 2018-12-07 at 08:03 -0500, Mauro Carvalho Chehab wrote:
> A common mistake is to assume that initializing a var with:
> struct foo f = { 0 };
>
> Would initialize a zeroed struct. Actually, what this does is
> to initialize the first element of the struct to zero.
>
> According to C99 Standard 6.7.8.21:
>
> "If there are fewer initializers in a brace-enclosed
> list than there are elements or members of an aggregate,
> or fewer characters in a string literal used to initialize
> an array of known size than there are elements in the array,
> the remainder of the aggregate shall be initialized implicitly
> the same as objects that have static storage duration."
>
> So, in practice, it could zero the entire struct, but, if the
> first element is not an integer, it will produce warnings:
>
>
> drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
> warning: Using plain integer as NULL pointer
>
> drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
> warning: Using plain integer as NULL pointer
>
> As the right initialization would be, instead:
>
> struct foo f = { NULL };
Thanks for sharing these details, it's definitely interesting and good
to know :)
> Another way to initialize it with gcc is to use:
>
> struct foo f = {};
>
> That seems to be a gcc extension, but clang also does the right thing,
> and that's a clean way for doing it.
>
> Anyway, I decided to check upstream what's the most commonly pattern.
> The "= {}" pattern has about 2000 entries:
>
> $ git grep -E "=\s*\{\s*\}"|wc -l
> 1951
>
> The standard-C compliant pattern has about 2500 entries:
>
> $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l
> 137
> $ git grep -E "=\s*\{\s*0\s*\}"|wc -l
> 2323
>
> Meaning that developers have split options on that.
>
> So, let's opt to the simpler form.
Acked-by: Paul Kocialkowski
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +-
> drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
> b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index b538eb0321d8..b7c918fa5fd1 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct
> cedrus_ctx *ctx)
> memset(ctx->ctrls, 0, ctrl_size);
>
> for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
> - struct v4l2_ctrl_config cfg = { 0 };
> + struct v4l2_ctrl_config cfg = {};
>
> cfg.elem_size = cedrus_controls[i].elem_size;
> cfg.id = cedrus_controls[i].id;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> index e40180a33951..f10c25f5460e 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> @@ -26,7 +26,7 @@ void cedrus_device_run(void *priv)
> {
> struct cedrus_ctx *ctx = priv;
> struct cedrus_dev *dev = ctx->dev;
> - struct cedrus_run run = { 0 };
> + struct cedrus_run run = {};
> struct media_request *src_req;
> unsigned long flags;
>
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
[PATCH 1/2] media: pxa_camera: don't deferenciate a NULL pointer
As warned by smatch:
drivers/media/platform/pxa_camera.c:2400 pxa_camera_probe() error: we
previously assumed 'pcdev->pdata' could be null (see line 2397)
It would be possible that neither DT nor platform data would be
provided. This is a Kernel bug, so warn about that and bail.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/platform/pxa_camera.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/platform/pxa_camera.c
b/drivers/media/platform/pxa_camera.c
index 5f930560eb30..f91f8fd424c4 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -2396,6 +2396,9 @@ static int pxa_camera_probe(struct platform_device *pdev)
pcdev->pdata = pdev->dev.platform_data;
if (pdev->dev.of_node && !pcdev->pdata) {
err = pxa_camera_pdata_from_dt(>dev, pcdev, >asd);
+ } else if (!pcdev->pdata) {
+ WARN_ON(1);
+ return -ENODEV;
} else {
pcdev->platform_flags = pcdev->pdata->flags;
pcdev->mclk = pcdev->pdata->mclk_10khz * 1;
--
2.19.2
[PATCH 2/2] media: drxk_hard: check if parameter is not NULL
There is a smatch warning:
drivers/media/dvb-frontends/drxk_hard.c:
drivers/media/dvb-frontends/drxk_hard.c:1478 scu_command() error: we previously
assumed 'parameter' could be null (see line 1467)
Telling that parameter might be NULL. Well, it can't, due to the
way the driver works, but it doesn't hurt to add a check, in order
to shut up smatch.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/media/dvb-frontends/drxk_hard.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/media/dvb-frontends/drxk_hard.c
b/drivers/media/dvb-frontends/drxk_hard.c
index 84ac3f73f8fe..8ea1e45be710 100644
--- a/drivers/media/dvb-frontends/drxk_hard.c
+++ b/drivers/media/dvb-frontends/drxk_hard.c
@@ -1474,9 +1474,11 @@ static int scu_command(struct drxk_state *state,
/* assume that the command register is ready
since it is checked afterwards */
- for (ii = parameter_len - 1; ii >= 0; ii -= 1) {
- buffer[cnt++] = (parameter[ii] & 0xFF);
- buffer[cnt++] = ((parameter[ii] >> 8) & 0xFF);
+ if (parameter) {
+ for (ii = parameter_len - 1; ii >= 0; ii -= 1) {
+ buffer[cnt++] = (parameter[ii] & 0xFF);
+ buffer[cnt++] = ((parameter[ii] >> 8) & 0xFF);
+ }
}
buffer[cnt++] = (cmd & 0xFF);
buffer[cnt++] = ((cmd >> 8) & 0xFF);
--
2.19.2
[PATCH v2] media: cedrus: don't initialize pointers with zero
A common mistake is to assume that initializing a var with:
struct foo f = { 0 };
Would initialize a zeroed struct. Actually, what this does is
to initialize the first element of the struct to zero.
According to C99 Standard 6.7.8.21:
"If there are fewer initializers in a brace-enclosed
list than there are elements or members of an aggregate,
or fewer characters in a string literal used to initialize
an array of known size than there are elements in the array,
the remainder of the aggregate shall be initialized implicitly
the same as objects that have static storage duration."
So, in practice, it could zero the entire struct, but, if the
first element is not an integer, it will produce warnings:
drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
warning: Using plain integer as NULL pointer
drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
warning: Using plain integer as NULL pointer
As the right initialization would be, instead:
struct foo f = { NULL };
Another way to initialize it with gcc is to use:
struct foo f = {};
That seems to be a gcc extension, but clang also does the right thing,
and that's a clean way for doing it.
Anyway, I decided to check upstream what's the most commonly pattern.
The "= {}" pattern has about 2000 entries:
$ git grep -E "=\s*\{\s*\}"|wc -l
1951
The standard-C compliant pattern has about 2500 entries:
$ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l
137
$ git grep -E "=\s*\{\s*0\s*\}"|wc -l
2323
Meaning that developers have split options on that.
So, let's opt to the simpler form.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +-
drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
b/drivers/staging/media/sunxi/cedrus/cedrus.c
index b538eb0321d8..b7c918fa5fd1 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct
cedrus_ctx *ctx)
memset(ctx->ctrls, 0, ctrl_size);
for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
- struct v4l2_ctrl_config cfg = { 0 };
+ struct v4l2_ctrl_config cfg = {};
cfg.elem_size = cedrus_controls[i].elem_size;
cfg.id = cedrus_controls[i].id;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index e40180a33951..f10c25f5460e 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -26,7 +26,7 @@ void cedrus_device_run(void *priv)
{
struct cedrus_ctx *ctx = priv;
struct cedrus_dev *dev = ctx->dev;
- struct cedrus_run run = { 0 };
+ struct cedrus_run run = {};
struct media_request *src_req;
unsigned long flags;
--
2.19.2
Re: [PATCH] media: cetrus: return an error if alloc fails
Hi,
On Fri, 2018-12-07 at 06:13 -0500, Mauro Carvalho Chehab wrote:
> As warned by smatch:
>
> drivers/staging/media/sunxi/cedrus/cedrus.c:
> drivers/staging/media/sunxi/cedrus/cedrus.c:93 cedrus_init_ctrls() error:
> potential null dereference 'ctx->ctrls'. (kzalloc returns null)
>
> While here, remove the memset(), as kzalloc() already zeroes the
> struct.
Good catch, thanks for the patch!
> Signed-off-by: Mauro Carvalho Chehab
Acked-by: Paul Kocialkowski
> ---
> drivers/staging/media/sunxi/cedrus/cedrus.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
> b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index 44c45c687503..24b89cd2b692 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -72,7 +72,8 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct
> cedrus_ctx *ctx)
> ctrl_size = sizeof(ctrl) * CEDRUS_CONTROLS_COUNT + 1;
>
> ctx->ctrls = kzalloc(ctrl_size, GFP_KERNEL);
> - memset(ctx->ctrls, 0, ctrl_size);
> + if (!ctx->ctrls)
> + return -ENOMEM;
>
> for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
> struct v4l2_ctrl_config cfg = { NULL };
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
Re: [PATCHv3 1/1] Add ir-rcmm-driver
Hi Sean,
Sorry, I just checked the first paragraph.
I will send you a new release.
Thanks,
Patrick.
On 07/12/2018 11:12, Sean Young wrote:
Hi Patrick,
On Fri, Dec 07, 2018 at 10:57:21AM +0100, Patrick LERDA wrote:
Add support for RCMM infrared remote controls.
Signed-off-by: Patrick Lerda
Other than the Signed-off-by this looks exactly like the v2 version;
did you see my other comments on the v2 patch?
Thanks
Sean
---
drivers/media/rc/Kconfig | 10 ++
drivers/media/rc/Makefile | 1 +
drivers/media/rc/ir-rcmm-decoder.c | 185
+
drivers/media/rc/rc-core-priv.h| 5 +
drivers/media/rc/rc-main.c | 3 +
include/media/rc-map.h | 6 +-
include/uapi/linux/lirc.h | 1 +
tools/include/uapi/linux/lirc.h| 1 +
8 files changed, 210 insertions(+), 2 deletions(-)
create mode 100644 drivers/media/rc/ir-rcmm-decoder.c
diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index 1021c08a9ba4..b7e08324b874 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -133,6 +133,16 @@ config IR_IMON_DECODER
remote control and you would like to use it with a raw IR
receiver, or if you wish to use an encoder to transmit this IR.
+config IR_RCMM_DECODER
+ tristate "Enable IR raw decoder for the RC-MM protocol"
+ depends on RC_CORE
+ select BITREVERSE
+ default y
+
+ ---help---
+ Enable this option if you have IR with RC-MM protocol, and
+ if the IR is decoded in software
+
endif #RC_DECODERS
menuconfig RC_DEVICES
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index e0340d043fe8..fc4058013234 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_IR_SHARP_DECODER) += ir-sharp-decoder.o
obj-$(CONFIG_IR_MCE_KBD_DECODER) += ir-mce_kbd-decoder.o
obj-$(CONFIG_IR_XMP_DECODER) += ir-xmp-decoder.o
obj-$(CONFIG_IR_IMON_DECODER) += ir-imon-decoder.o
+obj-$(CONFIG_IR_RCMM_DECODER) += ir-rcmm-decoder.o
# stand-alone IR receivers/transmitters
obj-$(CONFIG_RC_ATI_REMOTE) += ati_remote.o
diff --git a/drivers/media/rc/ir-rcmm-decoder.c
b/drivers/media/rc/ir-rcmm-decoder.c
new file mode 100644
index ..94d5b70f7a0f
--- /dev/null
+++ b/drivers/media/rc/ir-rcmm-decoder.c
@@ -0,0 +1,185 @@
+/* ir-rcmm-decoder.c - A decoder for the RCMM IR protocol
+ *
+ * Copyright (C) 2016 by Patrick Lerda
+ *
+ * This program is free software; you can redistribute it and/or
modify
+ * it under the terms of the GNU General Public License as published
by
+ * the Free Software Foundation version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include "rc-core-priv.h"
+#include
+#include
+
+
+#define RCMM_UNIT 17 /* nanosecs */
+#define RCMM_0_NBITS 64
+#define RCMM_PREFIX_PULSE 41 /* 16.6*2.5 */
+#define RCMM_PULSE_027 /* 16.6*(1+2/3)
*/
+#define RCMM_PULSE_144 /* 16.6*(2+2/3)
*/
+#define RCMM_PULSE_261 /* 16.6*(3+2/3)
*/
+#define RCMM_PULSE_378 /* 16.6*(4+2/3)
*/
+#define RCMM_MODE_MASK 0x
+
+enum rcmm_state {
+ STATE_INACTIVE,
+ STATE_LOW,
+ STATE_BUMP,
+ STATE_VALUE,
+ STATE_FINISHED,
+};
+
+static bool rcmm_mode(struct rcmm_dec *data)
+{
+return !((0x000c & data->bits) == 0x000c);
+}
+
+/**
+ * ir_rcmm_decode() - Decode one RCMM pulse or space
+ * @dev: the struct rc_dev descriptor of the device
+ * @ev:the struct ir_raw_event descriptor of the pulse/space
+ *
+ * This function returns -EINVAL if the pulse violates the state
machine
+ */
+static int ir_rcmm_decode(struct rc_dev *dev, struct ir_raw_event ev)
+{
+ struct rcmm_dec *data = >raw->rcmm;
+ u32 scancode;
+ u8 toggle;
+
+ if (!(dev->enabled_protocols & RC_PROTO_BIT_RCMM))
+ return 0;
+
+ if (!is_timing_event(ev)) {
+ if (ev.reset)
+ data->state = STATE_INACTIVE;
+ return 0;
+ }
+
+ if (ev.duration > RCMM_PULSE_3 + RCMM_UNIT)
+ goto out;
+
+ switch (data->state) {
+
+ case STATE_INACTIVE:
+ if (!ev.pulse)
+ break;
+
+ /* Note: larger margin on first pulse since each RCMM_UNIT
+ is quite short and some hardware takes some time to
+ adjust to the signal */
+ if (!eq_margin(ev.duration, RCMM_PREFIX_PULSE, RCMM_UNIT/2))
+ break;
+
+ data->state = STATE_LOW;
+
Re: [PATCH] media: cedrus: don't initialize pointers with zero
Em Fri, 7 Dec 2018 12:27:09 +
Ian Arkver escreveu:
> On 07/12/2018 11:37, Hans Verkuil wrote:
> > On 12/07/2018 12:31 PM, Mauro Carvalho Chehab wrote:
> >> Em Fri, 7 Dec 2018 12:14:50 +0100
> >> Hans Verkuil escreveu:
> >>
> >>> On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote:
> A common mistake is to assume that initializing a var with:
> struct foo f = { 0 };
>
> Would initialize a zeroed struct. Actually, what this does is
> to initialize the first element of the struct to zero.
>
> According to C99 Standard 6.7.8.21:
>
> "If there are fewer initializers in a brace-enclosed
> list than there are elements or members of an aggregate,
> or fewer characters in a string literal used to initialize
> an array of known size than there are elements in the array,
> the remainder of the aggregate shall be initialized implicitly
> the same as objects that have static storage duration."
>
> So, in practice, it could zero the entire struct, but, if the
> first element is not an integer, it will produce warnings:
>
>
> drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
> warning: Using plain integer as NULL pointer
>
> drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
> warning: Using plain integer as NULL pointer
>
> A proper way to initialize it with gcc is to use:
>
> struct foo f = { };
>
> But that seems to be a gcc extension. So, I decided to check upstream
> >>>
> >>> No, this is not a gcc extension. It's part of the latest C standard.
> >>
> >> Sure? Where the C standard spec states that? I've been seeking for
> >> such info for a while, as '= {}' is also my personal preference.
> >
> > I believe it was added in C11, but I've loaned the book that stated
> > that to someone else, so I can't check.
>
> Sadly I don't see mention of empty initializer lists in section 6.7.9 of
> ISO/IEC 9899:2011, though I've only got a draft version.
Yeah, as far as I checked, this is not really part of the standard.
Depending on how you read:
"If there are fewer initializers in a brace-enclosed
list than there are elements or members of an aggregate,
or fewer characters in a string literal used to initialize
an array of known size than there are elements in the array,
the remainder of the aggregate shall be initialized implicitly
the same as objects that have static storage duration."
One may infere that a brace-enclosed list with zero elements
would fit, and "the remainder of the aggregate shall be
initialized implicitly".
I suspect that this is how gcc people interpreted it.
> I had a play with Compiler Explorer[1] and it seems like clang is OK
> with the {} form though just out of interest msvc isn't.
Yeah, I'm aware that msvc won't support it. Good to know that clang
does the right thing cleaning the struct.
To be realistic, we only really care with gcc at the Kernel side, as
clang upstream versions still can't build upstream Kernels, and
nobody uses any other compiler for the Kernel anymore. Yet, with
regards to clang, there's a push to let it to build the Kernel.
So, it seems wise to add something that would work for both.
Anyway, I'll post a version 2 of this patch using ={} and placing
some rationale on it.
> I didn't try
> exploring other command line options.
>
> [1] https://gcc.godbolt.org/z/XIDC7W
>
> Regards,
> Ian
> >
> > In any case, it's used everywhere:
> >
> > git grep '= *{ *}' drivers/
> >
> > So it is really pointless to *not* use it.
> >
> > Regards,
> >
> > Hans
> >
> >> I tried to build the Kernel with clang, just to be sure that this
> >> won't cause issues with the clang support, but, unfortunately, the
> >> clang compiler (not even the upstream version) is able to build
> >> the upstream Kernel yet, as it lacks asm-goto support (there is an
> >> OOT patch for llvm solving it - but it sounds too much effort for
> >> my time to have to build llvm from their sources just for a simple
> >> check like this).
> >>
> >>> It's used all over in the kernel, so please use {} instead of { NULL }.
> >>>
> >>> Personally I think {} is a fantastic invention and I wish C++ had it as
> >>> well.
> >>
> >> Fully agreed on that.
> >>
> >>>
> >>> Regards,
> >>>
> >>> Hans
> >>>
> what's the most commonly pattern. The gcc pattern has about 2000 entries:
>
> $ git grep -E "=\s*\{\s*\}"|wc -l
> 1951
>
> The standard-C compliant pattern has about 2500 entries:
>
> $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l
> 137
> $ git grep -E "=\s*\{\s*0\s*\}"|wc -l
> 2323
>
> So, let's initialize those structs with:
> = { NULL }
>
> Signed-off-by: Mauro Carvalho Chehab
Re: [PATCH v9 00/13] media: staging/imx7: add i.MX7 media driver
On 11/22/2018 04:18 PM, Rui Miguel Silva wrote:
> Hi,
> This series introduces the Media driver to work with the i.MX7 SoC. it uses
> the
> already existing imx media core drivers but since the i.MX7, contrary to
> i.MX5/6, do not have an IPU and because of that some changes in the imx media
> core are made along this series to make it support that case.
>
> This patches adds CSI and MIPI-CSI2 drivers for i.MX7, along with several
> configurations changes for this to work as a capture subsystem. Some bugs are
> also fixed along the line. And necessary documentation.
>
> For a more detailed view of the capture paths, pads links in the i.MX7 please
> take a look at the documentation in PATCH 10.
>
> The system used to test and develop this was the Warp7 board with an OV2680
> sensor, which output format is 10-bit bayer. So, only MIPI interface was
> tested, a scenario with an parallel input would nice to have.
I got a few checkpatch warnings about coding style:
CHECK: Alignment should match open parenthesis
#953: FILE: drivers/staging/media/imx/imx7-media-csi.c:911:
+static struct v4l2_mbus_framefmt *imx7_csi_get_format(struct imx7_csi *csi,
+ struct v4l2_subdev_pad_config *cfg,
CHECK: Alignment should match open parenthesis
#1341: FILE: drivers/staging/media/imx/imx7-media-csi.c:1299:
+ ret = v4l2_async_register_fwnode_subdev(>sd,
+ sizeof(struct v4l2_async_subdev),
CHECK: Lines should not end with a '('
#684: FILE: drivers/staging/media/imx/imx7-mipi-csis.c:669:
+static struct csis_pix_format const *mipi_csis_try_format(
CHECK: Alignment should match open parenthesis
#708: FILE: drivers/staging/media/imx/imx7-mipi-csis.c:693:
+static struct v4l2_mbus_framefmt *mipi_csis_get_format(struct csi_state *state,
+ struct v4l2_subdev_pad_config *cfg,
CHECK: Alignment should match open parenthesis
#936: FILE: drivers/staging/media/imx/imx7-mipi-csis.c:921:
+ ret = v4l2_async_register_fwnode_subdev(mipi_sd,
+ sizeof(struct v4l2_async_subdev), _port, 1,
Apparently the latest coding style is that alignment is more important than
line length, although I personally do not agree. But since you need to
respin in any case due to the wrong SPDX identifier you used you might as
well take this into account.
I was really hoping I could merge this, but the SPDX license issue killed it.
Regards,
Hans
>
>
> Bellow goes an example of the output of the pads and links and the output of
> v4l2-compliance testing.
>
> The v4l-utils version used is:
> v4l2-compliance SHA : 044d5ab7b0d02683070d01a369c73d462d7a0cee from Nov 19th
>
> The Media Driver fail some tests but this failures are coming from code out of
> scope of this series (imx-capture), and some from the sensor OV2680
> but that I think not related with the sensor driver but with the testing and
> core.
>
> The csi and mipi-csi entities pass all compliance tests.
>
> Cheers,
> Rui
>
> v8->v9:
> Hans Verkuil:
> - Fix issues detected by checkpatch strict, still some left:
> - bigger kconfig option description
> - some alignement parenthesis that were left as they are, to be more
> readable
> - added new patch (PATCH13) for Maintainers update
> - SPDX in documentation rst file
> Sakari Ailus:
> - remove pad check in csi, this is done by core already
> - destroy mutex in probe error path (add label)
> - swap order in driver release
> - initialize endpoint in stack
> - use clk_bulk
> kbuild test robot:
> - add the missing imx-media-dev-common.c in patch 1/13
> - remove OWNER of module csis
> Myself:
> - add MAINTAINERS entries - new patch
>
> v7->v8:
> Myself:
> - rebase to latest linux-next (s/V4L2_MBUS_CSI2/V4L2_MBUS_CSI2_DPHY/)
> - Rebuild and test with latest v4l2-compliance
> - add Sakari reviewed-by tag to dt-bindings
>
> v6->v7:
> Myself:
> - Clock patches removed from this version since they were already merged
> - Rebuild and test with the latest v4l2-compliance
> - Add patch to video-mux regarding bayer formats
> - remove reference to dependent patch serie (was already merged)
>
> Sakari Ailus:
> - add port and endpoint explanantions
> - fix some wording should -> shall
>
> v5->v6:
> Rob Herring:
> - rename power-domain node name from: pgc-power-domain to power-domain
> - change mux-control-cells to 0
> - remove bus-width from mipi bindings and dts
> - remove err... regarding clock names line
> - remove clk-settle from example
> - split mipi-csi2 and csi bindings per file
> - add OF graph description to CSI
>
> Philipp Zabel:
> - rework group IDs and rename them with an _IPU_ prefix, this allowed to
> remove
>the ipu_present flag need.
>
> v4->v5:
> Sakari Ailus:
> - fix remove of the capture entries in dts bindings in the right patch
>
> Stephen Boyd:
> - Send all series to clk list
>
> v3->v4:
> Philipp Zabel:
> -
Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API
Em Fri, 7 Dec 2018 12:47:24 +0100 Hans Verkuil escreveu: > On 12/07/2018 12:26 PM, Mauro Carvalho Chehab wrote: > > Em Fri, 7 Dec 2018 10:09:04 +0100 > > Hans Verkuil escreveu: > > > >> This patch selects MEDIA_CONTROLLER for all camera, analog TV and > >> digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically. > >> > >> This will allow us to simplify drivers that currently have to add > >> #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API > >> to their code, since now this will always be available. > >> > >> The original intent of allowing these to be configured by the > >> user was (I think) to save a bit of memory. > > > > No. The original intent was/is to be sure that adding the media > > controller support won't be breaking existing working drivers. > > That doesn't make sense. If enabling this option would break existing > drivers, then something is really wrong, isn't it? It is the opposite: disabling it should not break any driver that don't depend on them to work. > > > > >> But as more and more > >> drivers have a media controller and all regular distros already > >> enable one or more of those drivers, the memory for the MC code is > >> there anyway. > >> > >> Complexity has always been the bane of media drivers, so reducing > >> complexity at the expense of a bit more memory (which is a rounding > >> error compared to the amount of video buffer memory needed) is IMHO > >> a good thing. > >> > >> Signed-off-by: Hans Verkuil > >> --- > >> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig > >> index 8add62a18293..56eb01cc8bb4 100644 > >> --- a/drivers/media/Kconfig > >> +++ b/drivers/media/Kconfig > >> @@ -31,6 +31,7 @@ comment "Multimedia core support" > >> # > >> config MEDIA_CAMERA_SUPPORT > >>bool "Cameras/video grabbers support" > >> + select MEDIA_CONTROLLER > >>---help--- > >> Enable support for webcams and video grabbers. > >> > >> @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT > >> > >> config MEDIA_ANALOG_TV_SUPPORT > >>bool "Analog TV support" > >> + select MEDIA_CONTROLLER > >>---help--- > >> Enable analog TV support. > >> > >> @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT > >> > >> config MEDIA_DIGITAL_TV_SUPPORT > >>bool "Digital TV support" > >> + select MEDIA_CONTROLLER > >>---help--- > >> Enable digital TV support. > > > > See my comments below. > > > >> > >> @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig" > >> > >> config MEDIA_CONTROLLER > >>bool "Media Controller API" > >> - depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || > >> MEDIA_DIGITAL_TV_SUPPORT > >>---help--- > >> Enable the media controller API used to query media devices internal > >> topology and configure it dynamically. > > > > I have split comments with regards to it. Yeah, nowadays media controller > > has becoming more important. Still, a lot of media drivers work fine > > without them. > > > > Anyway, if we're willing to make it mandatory, better to just remove the > > entire config option or to make it a silent one. > > Well, that assumes that the media controller will only be used by media > drivers, and not alsa or anyone else who wants to experiment with the MC. > > I won't object to making it silent (since it does reflect the current > situation), but since this functionality is not actually specific to media > drivers I think that is a good case to be made to allow it to be selected > manually. > > > > >> @@ -119,16 +121,11 @@ config VIDEO_DEV > >>tristate > >>depends on MEDIA_SUPPORT > >>depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || > >> MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT > >> + select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER > >>default y > >> > >> config VIDEO_V4L2_SUBDEV_API > >> - bool "V4L2 sub-device userspace API" > >> - depends on VIDEO_DEV && MEDIA_CONTROLLER > >> - ---help--- > >> -Enables the V4L2 sub-device pad-level userspace API used to configure > >> -video format, size and frame rate between hardware blocks. > >> - > >> -This API is mostly used by camera interfaces in embedded platforms. > >> + bool > > > > NACK. > > > > There is a very good reason why the subdev API is optional: there > > are drivers that use camera sensors but are not MC-centric. On those, > > the USB bridge driver is responsible to setup the subdevice. > > > > This options helps to ensure that camera sensors used by such drivers > > won't stop working because of the lack of the subdev-API. > > But they won't stop working if this is enabled. That's not the issue. I've seen (and nacked) several patches breaking drivers by assuming that all init would happen via subdev API. By having this as an optional feature that can be disabled, developers need to ensure that either the driver won't be built as a hole, if no subdev API suport is enabled, or need to add the needed logic inside the sub-device
Re: [PATCH v9 05/13] media: dt-bindings: add bindings for i.MX7 media driver
On 11/22/2018 04:18 PM, Rui Miguel Silva wrote:
> Add bindings documentation for i.MX7 media drivers.
> The imx7 MIPI CSI2 and imx7 CMOS Sensor Interface.
>
> Signed-off-by: Rui Miguel Silva
> Reviewed-by: Rob Herring
> Acked-by: Sakari Ailus
Please move this patch to the beginning of the series to avoid
checkpatch warnings:
WARNING: DT compatible string "fsl,imx7-csi" appears un-documented -- check
./Documentation/devicetree/bindings/
#1378: FILE: drivers/staging/media/imx/imx7-media-csi.c:1336:
+ { .compatible = "fsl,imx7-csi" },
Thanks!
Hans
> ---
> .../devicetree/bindings/media/imx7-csi.txt| 45 ++
> .../bindings/media/imx7-mipi-csi2.txt | 90 +++
> 2 files changed, 135 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/imx7-csi.txt
> create mode 100644 Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
>
> diff --git a/Documentation/devicetree/bindings/media/imx7-csi.txt
> b/Documentation/devicetree/bindings/media/imx7-csi.txt
> new file mode 100644
> index ..3c07bc676bc3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/imx7-csi.txt
> @@ -0,0 +1,45 @@
> +Freescale i.MX7 CMOS Sensor Interface
> +=
> +
> +csi node
> +
> +
> +This is device node for the CMOS Sensor Interface (CSI) which enables the
> chip
> +to connect directly to external CMOS image sensors.
> +
> +Required properties:
> +
> +- compatible: "fsl,imx7-csi";
> +- reg : base address and length of the register set for the device;
> +- interrupts: should contain CSI interrupt;
> +- clocks: list of clock specifiers, see
> +Documentation/devicetree/bindings/clock/clock-bindings.txt for
> details;
> +- clock-names : must contain "axi", "mclk" and "dcic" entries, matching
> + entries in the clock property;
> +
> +The device node shall contain one 'port' child node with one child 'endpoint'
> +node, according to the bindings defined in:
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +In the following example a remote endpoint is a video multiplexer.
> +
> +example:
> +
> +csi: csi@3071 {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +compatible = "fsl,imx7-csi";
> +reg = <0x3071 0x1>;
> +interrupts = ;
> +clocks = < IMX7D_CLK_DUMMY>,
> +< IMX7D_CSI_MCLK_ROOT_CLK>,
> +< IMX7D_CLK_DUMMY>;
> +clock-names = "axi", "mclk", "dcic";
> +
> +port {
> +csi_from_csi_mux: endpoint {
> +remote-endpoint = <_mux_to_csi>;
> +};
> +};
> +};
> diff --git a/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
> b/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
> new file mode 100644
> index ..71fd74ed3ec8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/imx7-mipi-csi2.txt
> @@ -0,0 +1,90 @@
> +Freescale i.MX7 Mipi CSI2
> +=
> +
> +mipi_csi2 node
> +--
> +
> +This is the device node for the MIPI CSI-2 receiver core in i.MX7 SoC. It is
> +compatible with previous version of Samsung D-phy.
> +
> +Required properties:
> +
> +- compatible: "fsl,imx7-mipi-csi2";
> +- reg : base address and length of the register set for the device;
> +- interrupts: should contain MIPI CSIS interrupt;
> +- clocks: list of clock specifiers, see
> +Documentation/devicetree/bindings/clock/clock-bindings.txt for
> details;
> +- clock-names : must contain "pclk", "wrap" and "phy" entries, matching
> + entries in the clock property;
> +- power-domains : a phandle to the power domain, see
> + Documentation/devicetree/bindings/power/power_domain.txt for
> details.
> +- reset-names : should include following entry "mrst";
> +- resets: a list of phandle, should contain reset entry of
> + reset-names;
> +- phy-supply: from the generic phy bindings, a phandle to a regulator
> that
> + provides power to MIPI CSIS core;
> +
> +Optional properties:
> +
> +- clock-frequency : The IP's main (system bus) clock frequency in Hz, default
> + value when this property is not specified is 166 MHz;
> +- fsl,csis-hs-settle : differential receiver (HS-RX) settle time;
> +
> +The device node should contain two 'port' child nodes with one child
> 'endpoint'
> +node, according to the bindings defined in:
> + Documentation/devicetree/bindings/ media/video-interfaces.txt.
> + The following are properties specific to those nodes.
>
Re: [PATCH v9 01/13] media: staging/imx: refactor imx media device probe
On 11/22/2018 04:18 PM, Rui Miguel Silva wrote: > Refactor and move media device initialization code to a new common > module, so it can be used by other devices, this will allow for example > a near to introduce imx7 CSI driver, to use this media device. > > Signed-off-by: Rui Miguel Silva > --- > drivers/staging/media/imx/Makefile| 1 + > .../staging/media/imx/imx-media-dev-common.c | 102 ++ > drivers/staging/media/imx/imx-media-dev.c | 88 --- > drivers/staging/media/imx/imx-media-of.c | 6 +- > drivers/staging/media/imx/imx-media.h | 15 +++ > 5 files changed, 141 insertions(+), 71 deletions(-) > create mode 100644 drivers/staging/media/imx/imx-media-dev-common.c > > diff --git a/drivers/staging/media/imx/Makefile > b/drivers/staging/media/imx/Makefile > index 698a4210316e..a30b3033f9a3 100644 > --- a/drivers/staging/media/imx/Makefile > +++ b/drivers/staging/media/imx/Makefile > @@ -1,5 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0 > imx-media-objs := imx-media-dev.o imx-media-internal-sd.o imx-media-of.o > +imx-media-objs += imx-media-dev-common.o > imx-media-common-objs := imx-media-utils.o imx-media-fim.o > imx-media-ic-objs := imx-ic-common.o imx-ic-prp.o imx-ic-prpencvf.o > > diff --git a/drivers/staging/media/imx/imx-media-dev-common.c > b/drivers/staging/media/imx/imx-media-dev-common.c > new file mode 100644 > index ..55fe94fb72f2 > --- /dev/null > +++ b/drivers/staging/media/imx/imx-media-dev-common.c > @@ -0,0 +1,102 @@ > +// SPDX-License-Identifier: GPL This is an invalid SPDX license identifier. You probably want to use GPL-2.0. This happens in more patches, please check! Regards, Hans
[GIT PULL FOR v4.21] Various fixes/enhancements
Note: there are a few patches that combine bindings with code changes. But since these are older patches and the bindings have already been reviewed I am not going to require the author to split them up. That's a bit overkill. If new patches arrive that have this problem, then I will request this going forward. Regards, Hans The following changes since commit 3c28b91380dd1183347d32d87d820818031ebecf: media: stkwebcam: Bugfix for wrong return values (2018-12-05 14:10:48 -0500) are available in the Git repository at: git://linuxtv.org/hverkuil/media_tree.git tags/br-v4.21i for you to fetch changes up to 54efad597804e6846ab860e7c2af84af529c669c: media: cedrus: Add device-tree compatible and variant for A64 support (2018-12-07 13:12:34 +0100) Tag branch Akinobu Mita (1): media: video-i2c: support runtime PM Colin Ian King (2): media: pvrusb2: fix spelling mistake "statuss" -> "status" media: sun6i: fix spelling mistake "droped" -> "dropped" Dmitry Osipenko (1): media: staging: tegra-vde: Replace debug messages with trace points Ezequiel Garcia (1): v4l2-ioctl: Zero v4l2_plane_pix_format reserved fields Gabriel Francisco Mandaji (1): media: vivid: Improve timestamping Kelvin Lawson (1): media: venus: Support V4L2 QP parameters in Venus encoder Lubomir Rintel (1): marvell-ccic: trivial fix to the datasheet URL Luca Ceresoli (1): media: v4l2-subdev: document controls need _FL_HAS_DEVNODE Malathi Gottam (1): media: venus: add support for key frame Matt Ranostay (1): media: video-i2c: check if chip struct has set_power function Paul Kocialkowski (4): media: cedrus: Remove global IRQ spin lock from the driver dt-bindings: media: cedrus: Add compatibles for the A64 and H5 media: cedrus: Add device-tree compatible and variant for H5 support media: cedrus: Add device-tree compatible and variant for A64 support Philipp Zabel (2): media: v4l2: clarify H.264 loop filter offset controls media: coda: fix H.264 deblocking filter controls Rob Herring (2): media: Use of_node_name_eq for node name comparisons staging: media: imx: Use of_node_name_eq for node name comparisons Sergei Shtylyov (2): rcar-csi2: add R8A77980 support rcar-vin: add R8A77980 support Todor Tomov (1): MAINTAINERS: Change Todor Tomov's email address Vivek Gautam (1): media: venus: core: Set dma maximum segment size Documentation/devicetree/bindings/media/cedrus.txt| 2 + Documentation/devicetree/bindings/media/rcar_vin.txt | 1 + Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt | 1 + Documentation/media/uapi/v4l/extended-controls.rst| 6 ++ MAINTAINERS | 2 +- drivers/media/i2c/video-i2c.c | 153 +- drivers/media/platform/coda/coda-bit.c| 19 ++-- drivers/media/platform/coda/coda-common.c | 15 ++- drivers/media/platform/coda/coda.h| 6 +- drivers/media/platform/coda/coda_regs.h | 2 +- drivers/media/platform/exynos4-is/media-dev.c | 12 +-- drivers/media/platform/marvell-ccic/cafe-driver.c | 2 +- drivers/media/platform/qcom/venus/core.c | 8 ++ drivers/media/platform/qcom/venus/venc.c | 19 drivers/media/platform/qcom/venus/venc_ctrls.c| 19 +++- drivers/media/platform/rcar-vin/rcar-core.c | 32 ++ drivers/media/platform/rcar-vin/rcar-csi2.c | 11 ++ drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c | 4 +- drivers/media/platform/ti-vpe/cal.c | 4 +- drivers/media/platform/vivid/vivid-core.h | 3 + drivers/media/platform/vivid/vivid-kthread-cap.c | 51 ++--- drivers/media/platform/vivid/vivid-vbi-cap.c | 4 - drivers/media/platform/xilinx/xilinx-tpg.c| 2 +- drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 2 +- drivers/media/v4l2-core/v4l2-fwnode.c | 6 +- drivers/media/v4l2-core/v4l2-ioctl.c | 10 ++ drivers/staging/media/imx/imx-media-of.c | 2 +- drivers/staging/media/sunxi/cedrus/cedrus.c | 17 ++- drivers/staging/media/sunxi/cedrus/cedrus.h | 2 - drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 9 -- drivers/staging/media/sunxi/cedrus/cedrus_hw.c| 13 +-- drivers/staging/media/sunxi/cedrus/cedrus_video.c | 5 -
Re: [PATCH] media: cedrus: don't initialize pointers with zero
On 07/12/2018 11:37, Hans Verkuil wrote:
On 12/07/2018 12:31 PM, Mauro Carvalho Chehab wrote:
Em Fri, 7 Dec 2018 12:14:50 +0100
Hans Verkuil escreveu:
On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote:
A common mistake is to assume that initializing a var with:
struct foo f = { 0 };
Would initialize a zeroed struct. Actually, what this does is
to initialize the first element of the struct to zero.
According to C99 Standard 6.7.8.21:
"If there are fewer initializers in a brace-enclosed
list than there are elements or members of an aggregate,
or fewer characters in a string literal used to initialize
an array of known size than there are elements in the array,
the remainder of the aggregate shall be initialized implicitly
the same as objects that have static storage duration."
So, in practice, it could zero the entire struct, but, if the
first element is not an integer, it will produce warnings:
drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
warning: Using plain integer as NULL pointer
drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
warning: Using plain integer as NULL pointer
A proper way to initialize it with gcc is to use:
struct foo f = { };
But that seems to be a gcc extension. So, I decided to check upstream
No, this is not a gcc extension. It's part of the latest C standard.
Sure? Where the C standard spec states that? I've been seeking for
such info for a while, as '= {}' is also my personal preference.
I believe it was added in C11, but I've loaned the book that stated
that to someone else, so I can't check.
Sadly I don't see mention of empty initializer lists in section 6.7.9 of
ISO/IEC 9899:2011, though I've only got a draft version.
I had a play with Compiler Explorer[1] and it seems like clang is OK
with the {} form though just out of interest msvc isn't. I didn't try
exploring other command line options.
[1] https://gcc.godbolt.org/z/XIDC7W
Regards,
Ian
In any case, it's used everywhere:
git grep '= *{ *}' drivers/
So it is really pointless to *not* use it.
Regards,
Hans
I tried to build the Kernel with clang, just to be sure that this
won't cause issues with the clang support, but, unfortunately, the
clang compiler (not even the upstream version) is able to build
the upstream Kernel yet, as it lacks asm-goto support (there is an
OOT patch for llvm solving it - but it sounds too much effort for
my time to have to build llvm from their sources just for a simple
check like this).
It's used all over in the kernel, so please use {} instead of { NULL }.
Personally I think {} is a fantastic invention and I wish C++ had it as
well.
Fully agreed on that.
Regards,
Hans
what's the most commonly pattern. The gcc pattern has about 2000 entries:
$ git grep -E "=\s*\{\s*\}"|wc -l
1951
The standard-C compliant pattern has about 2500 entries:
$ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l
137
$ git grep -E "=\s*\{\s*0\s*\}"|wc -l
2323
So, let's initialize those structs with:
= { NULL }
Signed-off-by: Mauro Carvalho Chehab
---
drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +-
drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
b/drivers/staging/media/sunxi/cedrus/cedrus.c
index b538eb0321d8..44c45c687503 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct
cedrus_ctx *ctx)
memset(ctx->ctrls, 0, ctrl_size);
for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
- struct v4l2_ctrl_config cfg = { 0 };
+ struct v4l2_ctrl_config cfg = { NULL };
cfg.elem_size = cedrus_controls[i].elem_size;
cfg.id = cedrus_controls[i].id;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index e40180a33951..4099a42dba2d 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -26,7 +26,7 @@ void cedrus_device_run(void *priv)
{
struct cedrus_ctx *ctx = priv;
struct cedrus_dev *dev = ctx->dev;
- struct cedrus_run run = { 0 };
+ struct cedrus_run run = { NULL };
struct media_request *src_req;
unsigned long flags;
Thanks,
Mauro
Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API
On 12/07/2018 12:26 PM, Mauro Carvalho Chehab wrote: > Em Fri, 7 Dec 2018 10:09:04 +0100 > Hans Verkuil escreveu: > >> This patch selects MEDIA_CONTROLLER for all camera, analog TV and >> digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically. >> >> This will allow us to simplify drivers that currently have to add >> #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API >> to their code, since now this will always be available. >> >> The original intent of allowing these to be configured by the >> user was (I think) to save a bit of memory. > > No. The original intent was/is to be sure that adding the media > controller support won't be breaking existing working drivers. That doesn't make sense. If enabling this option would break existing drivers, then something is really wrong, isn't it? > >> But as more and more >> drivers have a media controller and all regular distros already >> enable one or more of those drivers, the memory for the MC code is >> there anyway. >> >> Complexity has always been the bane of media drivers, so reducing >> complexity at the expense of a bit more memory (which is a rounding >> error compared to the amount of video buffer memory needed) is IMHO >> a good thing. >> >> Signed-off-by: Hans Verkuil >> --- >> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig >> index 8add62a18293..56eb01cc8bb4 100644 >> --- a/drivers/media/Kconfig >> +++ b/drivers/media/Kconfig >> @@ -31,6 +31,7 @@ comment "Multimedia core support" >> # >> config MEDIA_CAMERA_SUPPORT >> bool "Cameras/video grabbers support" >> +select MEDIA_CONTROLLER >> ---help--- >>Enable support for webcams and video grabbers. >> >> @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT >> >> config MEDIA_ANALOG_TV_SUPPORT >> bool "Analog TV support" >> +select MEDIA_CONTROLLER >> ---help--- >>Enable analog TV support. >> >> @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT >> >> config MEDIA_DIGITAL_TV_SUPPORT >> bool "Digital TV support" >> +select MEDIA_CONTROLLER >> ---help--- >>Enable digital TV support. > > See my comments below. > >> >> @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig" >> >> config MEDIA_CONTROLLER >> bool "Media Controller API" >> -depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || >> MEDIA_DIGITAL_TV_SUPPORT >> ---help--- >>Enable the media controller API used to query media devices internal >>topology and configure it dynamically. > > I have split comments with regards to it. Yeah, nowadays media controller > has becoming more important. Still, a lot of media drivers work fine > without them. > > Anyway, if we're willing to make it mandatory, better to just remove the > entire config option or to make it a silent one. Well, that assumes that the media controller will only be used by media drivers, and not alsa or anyone else who wants to experiment with the MC. I won't object to making it silent (since it does reflect the current situation), but since this functionality is not actually specific to media drivers I think that is a good case to be made to allow it to be selected manually. > >> @@ -119,16 +121,11 @@ config VIDEO_DEV >> tristate >> depends on MEDIA_SUPPORT >> depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || >> MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT >> +select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER >> default y >> >> config VIDEO_V4L2_SUBDEV_API >> -bool "V4L2 sub-device userspace API" >> -depends on VIDEO_DEV && MEDIA_CONTROLLER >> ----help--- >> - Enables the V4L2 sub-device pad-level userspace API used to configure >> - video format, size and frame rate between hardware blocks. >> - >> - This API is mostly used by camera interfaces in embedded platforms. >> +bool > > NACK. > > There is a very good reason why the subdev API is optional: there > are drivers that use camera sensors but are not MC-centric. On those, > the USB bridge driver is responsible to setup the subdevice. > > This options helps to ensure that camera sensors used by such drivers > won't stop working because of the lack of the subdev-API. But they won't stop working if this is enabled. This option is used as a dependency by drivers that require this functionality, but enabling it will never break other drivers that don't need this. Those drivers simply won't use it. Also note that it is the bridge driver that controls whether or not the v4l-subdevX devices are created. If the bridge driver doesn't explicitly enable it AND the subdev driver explicitly supports it, those devices will not be created. Regards, Hans > >> >> source "drivers/media/v4l2-core/Kconfig" >> > > > > Thanks, > Mauro >
[PATCH] media: imx214: don't de-reference a NULL pointer
As warned by smatch: drivers/media/i2c/imx214.c:591 imx214_set_format() warn: variable dereferenced before check 'format' (see line 589) It turns that the code at imx214_set_format() has support for being called with the format being NULL. I've no idea why, as it is only called internally with the pointer set, and via subdev API (with should also set it). Also, the entire logic there depends on having format != NULL, so just remove the bogus broken support for a null format. Signed-off-by: Mauro Carvalho Chehab --- drivers/media/i2c/imx214.c | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c index ec3d1b855f62..b046a26219a4 100644 --- a/drivers/media/i2c/imx214.c +++ b/drivers/media/i2c/imx214.c @@ -588,12 +588,10 @@ static int imx214_set_format(struct v4l2_subdev *sd, __crop = __imx214_get_pad_crop(imx214, cfg, format->pad, format->which); - if (format) - mode = v4l2_find_nearest_size(imx214_modes, - ARRAY_SIZE(imx214_modes), width, height, - format->format.width, format->format.height); - else - mode = _modes[0]; + mode = v4l2_find_nearest_size(imx214_modes, + ARRAY_SIZE(imx214_modes), width, height, + format->format.width, + format->format.height); __crop->width = mode->width; __crop->height = mode->height; -- 2.19.2
Re: [PATCH] media: cedrus: don't initialize pointers with zero
On 12/07/2018 12:31 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 7 Dec 2018 12:14:50 +0100
> Hans Verkuil escreveu:
>
>> On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote:
>>> A common mistake is to assume that initializing a var with:
>>> struct foo f = { 0 };
>>>
>>> Would initialize a zeroed struct. Actually, what this does is
>>> to initialize the first element of the struct to zero.
>>>
>>> According to C99 Standard 6.7.8.21:
>>>
>>> "If there are fewer initializers in a brace-enclosed
>>> list than there are elements or members of an aggregate,
>>> or fewer characters in a string literal used to initialize
>>> an array of known size than there are elements in the array,
>>> the remainder of the aggregate shall be initialized implicitly
>>> the same as objects that have static storage duration."
>>>
>>> So, in practice, it could zero the entire struct, but, if the
>>> first element is not an integer, it will produce warnings:
>>>
>>>
>>> drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
>>> warning: Using plain integer as NULL pointer
>>>
>>> drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
>>> warning: Using plain integer as NULL pointer
>>>
>>> A proper way to initialize it with gcc is to use:
>>>
>>> struct foo f = { };
>>>
>>> But that seems to be a gcc extension. So, I decided to check upstream
>>
>> No, this is not a gcc extension. It's part of the latest C standard.
>
> Sure? Where the C standard spec states that? I've been seeking for
> such info for a while, as '= {}' is also my personal preference.
I believe it was added in C11, but I've loaned the book that stated
that to someone else, so I can't check.
In any case, it's used everywhere:
git grep '= *{ *}' drivers/
So it is really pointless to *not* use it.
Regards,
Hans
> I tried to build the Kernel with clang, just to be sure that this
> won't cause issues with the clang support, but, unfortunately, the
> clang compiler (not even the upstream version) is able to build
> the upstream Kernel yet, as it lacks asm-goto support (there is an
> OOT patch for llvm solving it - but it sounds too much effort for
> my time to have to build llvm from their sources just for a simple
> check like this).
>
>> It's used all over in the kernel, so please use {} instead of { NULL }.
>>
>> Personally I think {} is a fantastic invention and I wish C++ had it as
>> well.
>
> Fully agreed on that.
>
>>
>> Regards,
>>
>> Hans
>>
>>> what's the most commonly pattern. The gcc pattern has about 2000 entries:
>>>
>>> $ git grep -E "=\s*\{\s*\}"|wc -l
>>> 1951
>>>
>>> The standard-C compliant pattern has about 2500 entries:
>>>
>>> $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l
>>> 137
>>> $ git grep -E "=\s*\{\s*0\s*\}"|wc -l
>>> 2323
>>>
>>> So, let's initialize those structs with:
>>> = { NULL }
>>>
>>> Signed-off-by: Mauro Carvalho Chehab
>>> ---
>>> drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +-
>>> drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> b/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> index b538eb0321d8..44c45c687503 100644
>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> @@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev,
>>> struct cedrus_ctx *ctx)
>>> memset(ctx->ctrls, 0, ctrl_size);
>>>
>>> for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
>>> - struct v4l2_ctrl_config cfg = { 0 };
>>> + struct v4l2_ctrl_config cfg = { NULL };
>>>
>>> cfg.elem_size = cedrus_controls[i].elem_size;
>>> cfg.id = cedrus_controls[i].id;
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
>>> b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
>>> index e40180a33951..4099a42dba2d 100644
>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
>>> @@ -26,7 +26,7 @@ void cedrus_device_run(void *priv)
>>> {
>>> struct cedrus_ctx *ctx = priv;
>>> struct cedrus_dev *dev = ctx->dev;
>>> - struct cedrus_run run = { 0 };
>>> + struct cedrus_run run = { NULL };
>>> struct media_request *src_req;
>>> unsigned long flags;
>>>
>>>
>>
>
>
>
> Thanks,
> Mauro
>
Re: [PATCH] media: cedrus: don't initialize pointers with zero
Em Fri, 7 Dec 2018 12:14:50 +0100
Hans Verkuil escreveu:
> On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote:
> > A common mistake is to assume that initializing a var with:
> > struct foo f = { 0 };
> >
> > Would initialize a zeroed struct. Actually, what this does is
> > to initialize the first element of the struct to zero.
> >
> > According to C99 Standard 6.7.8.21:
> >
> > "If there are fewer initializers in a brace-enclosed
> > list than there are elements or members of an aggregate,
> > or fewer characters in a string literal used to initialize
> > an array of known size than there are elements in the array,
> > the remainder of the aggregate shall be initialized implicitly
> > the same as objects that have static storage duration."
> >
> > So, in practice, it could zero the entire struct, but, if the
> > first element is not an integer, it will produce warnings:
> >
> >
> > drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
> > warning: Using plain integer as NULL pointer
> >
> > drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
> > warning: Using plain integer as NULL pointer
> >
> > A proper way to initialize it with gcc is to use:
> >
> > struct foo f = { };
> >
> > But that seems to be a gcc extension. So, I decided to check upstream
>
> No, this is not a gcc extension. It's part of the latest C standard.
Sure? Where the C standard spec states that? I've been seeking for
such info for a while, as '= {}' is also my personal preference.
I tried to build the Kernel with clang, just to be sure that this
won't cause issues with the clang support, but, unfortunately, the
clang compiler (not even the upstream version) is able to build
the upstream Kernel yet, as it lacks asm-goto support (there is an
OOT patch for llvm solving it - but it sounds too much effort for
my time to have to build llvm from their sources just for a simple
check like this).
> It's used all over in the kernel, so please use {} instead of { NULL }.
>
> Personally I think {} is a fantastic invention and I wish C++ had it as
> well.
Fully agreed on that.
>
> Regards,
>
> Hans
>
> > what's the most commonly pattern. The gcc pattern has about 2000 entries:
> >
> > $ git grep -E "=\s*\{\s*\}"|wc -l
> > 1951
> >
> > The standard-C compliant pattern has about 2500 entries:
> >
> > $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l
> > 137
> > $ git grep -E "=\s*\{\s*0\s*\}"|wc -l
> > 2323
> >
> > So, let's initialize those structs with:
> > = { NULL }
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > ---
> > drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +-
> > drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > index b538eb0321d8..44c45c687503 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > @@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev,
> > struct cedrus_ctx *ctx)
> > memset(ctx->ctrls, 0, ctrl_size);
> >
> > for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
> > - struct v4l2_ctrl_config cfg = { 0 };
> > + struct v4l2_ctrl_config cfg = { NULL };
> >
> > cfg.elem_size = cedrus_controls[i].elem_size;
> > cfg.id = cedrus_controls[i].id;
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > index e40180a33951..4099a42dba2d 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > @@ -26,7 +26,7 @@ void cedrus_device_run(void *priv)
> > {
> > struct cedrus_ctx *ctx = priv;
> > struct cedrus_dev *dev = ctx->dev;
> > - struct cedrus_run run = { 0 };
> > + struct cedrus_run run = { NULL };
> > struct media_request *src_req;
> > unsigned long flags;
> >
> >
>
Thanks,
Mauro
Re: [PATCH 4/5 RESEND] si470x-i2c: Add optional reset-gpio support
Adding the actual author :-)
Regards,
Hans
On 12/07/2018 12:25 PM, Michael Nazzareno Trimarchi wrote:
> Hi
>
> On Fri, Dec 7, 2018 at 12:12 PM Hans Verkuil wrote:
>>
>> Subject: [PATCH 4/5] si470x-i2c: Add optional reset-gpio support
>> Date: Wed, 5 Dec 2018 16:47:49 +0100
>> From: Paweł Chmiel
>> To: mche...@kernel.org, robh...@kernel.org, mark.rutl...@arm.com
>> CC: hverk...@xs4all.nl, fischerdougl...@gmail.com, keesc...@chromium.org,
>> linux-media@vger.kernel.org, linux-ker...@vger.kernel.org,
>> devicet...@vger.kernel.org, Paweł Chmiel
>>
>> If reset-gpio is defined, use it to bring device out of reset.
>> Without this, it's not possible to access si470x registers.
>>
>> Signed-off-by: Paweł Chmiel
>> ---
>> For some reason this patch was not picked up by patchwork. Resending to see
>> if
>> it is picked up now.
>> ---
>> drivers/media/radio/si470x/radio-si470x-i2c.c | 15 +++
>> drivers/media/radio/si470x/radio-si470x.h | 1 +
>> 2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/media/radio/si470x/radio-si470x-i2c.c
>> b/drivers/media/radio/si470x/radio-si470x-i2c.c
>> index a7ac09c55188..15eea2b2c90f 100644
>> --- a/drivers/media/radio/si470x/radio-si470x-i2c.c
>> +++ b/drivers/media/radio/si470x/radio-si470x-i2c.c
>> @@ -28,6 +28,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include "radio-si470x.h"
>> @@ -392,6 +393,17 @@ static int si470x_i2c_probe(struct i2c_client *client,
>> radio->videodev.release = video_device_release_empty;
>> video_set_drvdata(>videodev, radio);
>> + radio->gpio_reset = devm_gpiod_get_optional(>dev, "reset",
>> + GPIOD_OUT_LOW);
>> + if (IS_ERR(radio->gpio_reset)) {
>> + retval = PTR_ERR(radio->gpio_reset);
>> + dev_err(>dev, "Failed to request gpio: %d\n",
>> retval);
>> + goto err_all;
>> + }
>> +
>> + if (radio->gpio_reset)
>> + gpiod_set_value(radio->gpio_reset, 1);
>> +
>> /* power up : need 110ms */
>> radio->registers[POWERCFG] = POWERCFG_ENABLE;
>> if (si470x_set_register(radio, POWERCFG) < 0) {
>> @@ -478,6 +490,9 @@ static int si470x_i2c_remove(struct i2c_client *client)
>> video_unregister_device(>videodev);
>> + if (radio->gpio_reset)
>> + gpiod_set_value(radio->gpio_reset, 0);
>
> I have a question for you. If the gpio is the last of the bank
> acquired for this cpu, when you put to 0, then the gpio will
> be free on remove and the clock of the logic will be deactivated so I
> think that you don't have any
> garantee that the state will be 0
>
> Michael
>
>> +
>> return 0;
>> }
>> diff --git a/drivers/media/radio/si470x/radio-si470x.h
>> b/drivers/media/radio/si470x/radio-si470x.h
>> index 35fa0f3bbdd2..6fd6a399cb77 100644
>> --- a/drivers/media/radio/si470x/radio-si470x.h
>> +++ b/drivers/media/radio/si470x/radio-si470x.h
>> @@ -189,6 +189,7 @@ struct si470x_device {
>> #if IS_ENABLED(CONFIG_I2C_SI470X)
>> struct i2c_client *client;
>> + struct gpio_desc *gpio_reset;
>> #endif
>> };
>> -- 2.17.1
>>
>
>
Re: [PATCH v2 2/2] media: video-i2c: add Melexis MLX90640 thermal camera support
On 11/22/2018 04:52 AM, Matt Ranostay wrote:
> Add initial support for MLX90640 thermal cameras which output an 32x24
> greyscale pixel image along with 2 rows of coefficent data.
>
> Because of this the data outputed is really 32x26 and needs the two rows
> removed after using the coefficent information to generate processed
> images in userspace.
>
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Matt Ranostay
> ---
> .../bindings/media/i2c/melexis,mlx90640.txt | 20
> drivers/media/i2c/Kconfig | 1 +
> drivers/media/i2c/video-i2c.c | 110 +-
> 3 files changed, 130 insertions(+), 1 deletion(-)
> create mode 100644
> Documentation/devicetree/bindings/media/i2c/melexis,mlx90640.txt
This patch needs to be split up in two: one for the bindings, one for
the driver changes.
Once the bindings for the v3 of this patch are accepted, I can merge this.
Note that I am in the process of merging '[PATCH v3] media: video-i2c: check if
chip
struct has set_power function', so it shouldn't be necessary to repost that
patch.
Regards,
Hans
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/melexis,mlx90640.txt
> b/Documentation/devicetree/bindings/media/i2c/melexis,mlx90640.txt
> new file mode 100644
> index ..060d2b7a5893
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/melexis,mlx90640.txt
> @@ -0,0 +1,20 @@
> +* Melexis MLX90640 FIR Sensor
> +
> +Melexis MLX90640 FIR sensor support which allows recording of thermal data
> +with 32x24 resolution excluding 2 lines of coefficient data that is used by
> +userspace to render processed frames.
> +
> +Required Properties:
> + - compatible : Must be "melexis,mlx90640"
> + - reg : i2c address of the device
> +
> +Example:
> +
> + i2c0@1c22000 {
> + ...
> + mlx90640@33 {
> + compatible = "melexis,mlx90640";
> + reg = <0x33>;
> + };
> + ...
> + };
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 66bbbec487ec..24342f283f2a 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1097,6 +1097,7 @@ config VIDEO_I2C
> Enable the I2C transport video support which supports the
> following:
> * Panasonic AMG88xx Grid-Eye Sensors
> +* Melexis MLX90640 Thermal Cameras
>
> To compile this driver as a module, choose M here: the
> module will be called video-i2c
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index a64e1a725a20..d58f40334e8b 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -6,6 +6,7 @@
> *
> * Supported:
> * - Panasonic AMG88xx Grid-Eye Sensors
> + * - Melexis MLX90640 Thermal Cameras
> */
>
> #include
> @@ -18,6 +19,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -66,12 +68,26 @@ static const struct v4l2_frmsize_discrete amg88xx_size = {
> .height = 8,
> };
>
> +static const struct v4l2_fmtdesc mlx90640_format = {
> + .pixelformat = V4L2_PIX_FMT_Y16_BE,
> +};
> +
> +static const struct v4l2_frmsize_discrete mlx90640_size = {
> + .width = 32,
> + .height = 26, /* 24 lines of pixel data + 2 lines of processing data */
> +};
> +
> static const struct regmap_config amg88xx_regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> .max_register = 0xff
> };
>
> +static const struct regmap_config mlx90640_regmap_config = {
> + .reg_bits = 16,
> + .val_bits = 16,
> +};
> +
> struct video_i2c_chip {
> /* video dimensions */
> const struct v4l2_fmtdesc *format;
> @@ -88,6 +104,7 @@ struct video_i2c_chip {
> unsigned int bpp;
>
> const struct regmap_config *regmap_config;
> + struct nvmem_config *nvmem_config;
>
> /* setup function */
> int (*setup)(struct video_i2c_data *data);
> @@ -102,6 +119,22 @@ struct video_i2c_chip {
> int (*hwmon_init)(struct video_i2c_data *data);
> };
>
> +static int mlx90640_nvram_read(void *priv, unsigned int offset, void *val,
> + size_t bytes)
> +{
> + struct video_i2c_data *data = priv;
> +
> + return regmap_bulk_read(data->regmap, 0x2400 + offset, val, bytes);
> +}
> +
> +static struct nvmem_config mlx90640_nvram_config = {
> + .name = "mlx90640_nvram",
> + .word_size = 2,
> + .stride = 1,
> + .size = 1664,
> + .reg_read = mlx90640_nvram_read,
> +};
> +
> /* Power control register */
> #define AMG88XX_REG_PCTL 0x00
> #define AMG88XX_PCTL_NORMAL 0x00
> @@ -122,12 +155,23 @@ struct video_i2c_chip {
> /* Temperature register */
> #define AMG88XX_REG_T01L 0x80
>
> +/* Control register */
> +#define MLX90640_REG_CTL10x800d
> +#define MLX90640_REG_CTL1_MASK 0x0380
> +#define MLX90640_REG_CTL1_MASK_SHIFT 7
Re: [RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API
Em Fri, 7 Dec 2018 10:09:04 +0100 Hans Verkuil escreveu: > This patch selects MEDIA_CONTROLLER for all camera, analog TV and > digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically. > > This will allow us to simplify drivers that currently have to add > #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API > to their code, since now this will always be available. > > The original intent of allowing these to be configured by the > user was (I think) to save a bit of memory. No. The original intent was/is to be sure that adding the media controller support won't be breaking existing working drivers. > But as more and more > drivers have a media controller and all regular distros already > enable one or more of those drivers, the memory for the MC code is > there anyway. > > Complexity has always been the bane of media drivers, so reducing > complexity at the expense of a bit more memory (which is a rounding > error compared to the amount of video buffer memory needed) is IMHO > a good thing. > > Signed-off-by: Hans Verkuil > --- > diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig > index 8add62a18293..56eb01cc8bb4 100644 > --- a/drivers/media/Kconfig > +++ b/drivers/media/Kconfig > @@ -31,6 +31,7 @@ comment "Multimedia core support" > # > config MEDIA_CAMERA_SUPPORT > bool "Cameras/video grabbers support" > + select MEDIA_CONTROLLER > ---help--- > Enable support for webcams and video grabbers. > > @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT > > config MEDIA_ANALOG_TV_SUPPORT > bool "Analog TV support" > + select MEDIA_CONTROLLER > ---help--- > Enable analog TV support. > > @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT > > config MEDIA_DIGITAL_TV_SUPPORT > bool "Digital TV support" > + select MEDIA_CONTROLLER > ---help--- > Enable digital TV support. See my comments below. > > @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig" > > config MEDIA_CONTROLLER > bool "Media Controller API" > - depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || > MEDIA_DIGITAL_TV_SUPPORT > ---help--- > Enable the media controller API used to query media devices internal > topology and configure it dynamically. I have split comments with regards to it. Yeah, nowadays media controller has becoming more important. Still, a lot of media drivers work fine without them. Anyway, if we're willing to make it mandatory, better to just remove the entire config option or to make it a silent one. > @@ -119,16 +121,11 @@ config VIDEO_DEV > tristate > depends on MEDIA_SUPPORT > depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || > MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT > + select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER > default y > > config VIDEO_V4L2_SUBDEV_API > - bool "V4L2 sub-device userspace API" > - depends on VIDEO_DEV && MEDIA_CONTROLLER > - ---help--- > - Enables the V4L2 sub-device pad-level userspace API used to configure > - video format, size and frame rate between hardware blocks. > - > - This API is mostly used by camera interfaces in embedded platforms. > + bool NACK. There is a very good reason why the subdev API is optional: there are drivers that use camera sensors but are not MC-centric. On those, the USB bridge driver is responsible to setup the subdevice. This options helps to ensure that camera sensors used by such drivers won't stop working because of the lack of the subdev-API. > > source "drivers/media/v4l2-core/Kconfig" > Thanks, Mauro
Re: [PATCH 4/5 RESEND] si470x-i2c: Add optional reset-gpio support
Hi
On Fri, Dec 7, 2018 at 12:12 PM Hans Verkuil wrote:
>
> Subject: [PATCH 4/5] si470x-i2c: Add optional reset-gpio support
> Date: Wed, 5 Dec 2018 16:47:49 +0100
> From: Paweł Chmiel
> To: mche...@kernel.org, robh...@kernel.org, mark.rutl...@arm.com
> CC: hverk...@xs4all.nl, fischerdougl...@gmail.com, keesc...@chromium.org,
> linux-media@vger.kernel.org, linux-ker...@vger.kernel.org,
> devicet...@vger.kernel.org, Paweł Chmiel
>
> If reset-gpio is defined, use it to bring device out of reset.
> Without this, it's not possible to access si470x registers.
>
> Signed-off-by: Paweł Chmiel
> ---
> For some reason this patch was not picked up by patchwork. Resending to see if
> it is picked up now.
> ---
> drivers/media/radio/si470x/radio-si470x-i2c.c | 15 +++
> drivers/media/radio/si470x/radio-si470x.h | 1 +
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/media/radio/si470x/radio-si470x-i2c.c
> b/drivers/media/radio/si470x/radio-si470x-i2c.c
> index a7ac09c55188..15eea2b2c90f 100644
> --- a/drivers/media/radio/si470x/radio-si470x-i2c.c
> +++ b/drivers/media/radio/si470x/radio-si470x-i2c.c
> @@ -28,6 +28,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include "radio-si470x.h"
> @@ -392,6 +393,17 @@ static int si470x_i2c_probe(struct i2c_client *client,
> radio->videodev.release = video_device_release_empty;
> video_set_drvdata(>videodev, radio);
> + radio->gpio_reset = devm_gpiod_get_optional(>dev, "reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(radio->gpio_reset)) {
> + retval = PTR_ERR(radio->gpio_reset);
> + dev_err(>dev, "Failed to request gpio: %d\n", retval);
> + goto err_all;
> + }
> +
> + if (radio->gpio_reset)
> + gpiod_set_value(radio->gpio_reset, 1);
> +
> /* power up : need 110ms */
> radio->registers[POWERCFG] = POWERCFG_ENABLE;
> if (si470x_set_register(radio, POWERCFG) < 0) {
> @@ -478,6 +490,9 @@ static int si470x_i2c_remove(struct i2c_client *client)
> video_unregister_device(>videodev);
> + if (radio->gpio_reset)
> + gpiod_set_value(radio->gpio_reset, 0);
I have a question for you. If the gpio is the last of the bank
acquired for this cpu, when you put to 0, then the gpio will
be free on remove and the clock of the logic will be deactivated so I
think that you don't have any
garantee that the state will be 0
Michael
> +
> return 0;
> }
> diff --git a/drivers/media/radio/si470x/radio-si470x.h
> b/drivers/media/radio/si470x/radio-si470x.h
> index 35fa0f3bbdd2..6fd6a399cb77 100644
> --- a/drivers/media/radio/si470x/radio-si470x.h
> +++ b/drivers/media/radio/si470x/radio-si470x.h
> @@ -189,6 +189,7 @@ struct si470x_device {
> #if IS_ENABLED(CONFIG_I2C_SI470X)
> struct i2c_client *client;
> + struct gpio_desc *gpio_reset;
> #endif
> };
> -- 2.17.1
>
--
| Michael Nazzareno Trimarchi Amarula Solutions BV |
| COO - Founder Cruquiuskade 47 |
| +31(0)851119172 Amsterdam 1018 AM NL |
| [`as] http://www.amarulasolutions.com |
Re: [PATCH] media: cedrus: don't initialize pointers with zero
On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote:
> A common mistake is to assume that initializing a var with:
> struct foo f = { 0 };
>
> Would initialize a zeroed struct. Actually, what this does is
> to initialize the first element of the struct to zero.
>
> According to C99 Standard 6.7.8.21:
>
> "If there are fewer initializers in a brace-enclosed
> list than there are elements or members of an aggregate,
> or fewer characters in a string literal used to initialize
> an array of known size than there are elements in the array,
> the remainder of the aggregate shall be initialized implicitly
> the same as objects that have static storage duration."
>
> So, in practice, it could zero the entire struct, but, if the
> first element is not an integer, it will produce warnings:
>
>
> drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
> warning: Using plain integer as NULL pointer
>
> drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
> warning: Using plain integer as NULL pointer
>
> A proper way to initialize it with gcc is to use:
>
> struct foo f = { };
>
> But that seems to be a gcc extension. So, I decided to check upstream
No, this is not a gcc extension. It's part of the latest C standard.
It's used all over in the kernel, so please use {} instead of { NULL }.
Personally I think {} is a fantastic invention and I wish C++ had it as
well.
Regards,
Hans
> what's the most commonly pattern. The gcc pattern has about 2000 entries:
>
> $ git grep -E "=\s*\{\s*\}"|wc -l
> 1951
>
> The standard-C compliant pattern has about 2500 entries:
>
> $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l
> 137
> $ git grep -E "=\s*\{\s*0\s*\}"|wc -l
> 2323
>
> So, let's initialize those structs with:
>= { NULL }
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +-
> drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
> b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index b538eb0321d8..44c45c687503 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct
> cedrus_ctx *ctx)
> memset(ctx->ctrls, 0, ctrl_size);
>
> for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
> - struct v4l2_ctrl_config cfg = { 0 };
> + struct v4l2_ctrl_config cfg = { NULL };
>
> cfg.elem_size = cedrus_controls[i].elem_size;
> cfg.id = cedrus_controls[i].id;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> index e40180a33951..4099a42dba2d 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> @@ -26,7 +26,7 @@ void cedrus_device_run(void *priv)
> {
> struct cedrus_ctx *ctx = priv;
> struct cedrus_dev *dev = ctx->dev;
> - struct cedrus_run run = { 0 };
> + struct cedrus_run run = { NULL };
> struct media_request *src_req;
> unsigned long flags;
>
>
[PATCH] media: cetrus: return an error if alloc fails
As warned by smatch:
drivers/staging/media/sunxi/cedrus/cedrus.c:
drivers/staging/media/sunxi/cedrus/cedrus.c:93 cedrus_init_ctrls() error:
potential null dereference 'ctx->ctrls'. (kzalloc returns null)
While here, remove the memset(), as kzalloc() already zeroes the
struct.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/staging/media/sunxi/cedrus/cedrus.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 44c45c687503..24b89cd2b692 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -72,7 +72,8 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct
cedrus_ctx *ctx)
ctrl_size = sizeof(ctrl) * CEDRUS_CONTROLS_COUNT + 1;
ctx->ctrls = kzalloc(ctrl_size, GFP_KERNEL);
- memset(ctx->ctrls, 0, ctrl_size);
+ if (!ctx->ctrls)
+ return -ENOMEM;
for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
struct v4l2_ctrl_config cfg = { NULL };
--
2.19.2
[PATCH 4/5 RESEND] si470x-i2c: Add optional reset-gpio support
Subject: [PATCH 4/5] si470x-i2c: Add optional reset-gpio support
Date: Wed, 5 Dec 2018 16:47:49 +0100
From: Paweł Chmiel
To: mche...@kernel.org, robh...@kernel.org, mark.rutl...@arm.com
CC: hverk...@xs4all.nl, fischerdougl...@gmail.com, keesc...@chromium.org,
linux-media@vger.kernel.org, linux-ker...@vger.kernel.org,
devicet...@vger.kernel.org, Paweł Chmiel
If reset-gpio is defined, use it to bring device out of reset.
Without this, it's not possible to access si470x registers.
Signed-off-by: Paweł Chmiel
---
For some reason this patch was not picked up by patchwork. Resending to see if
it is picked up now.
---
drivers/media/radio/si470x/radio-si470x-i2c.c | 15 +++
drivers/media/radio/si470x/radio-si470x.h | 1 +
2 files changed, 16 insertions(+)
diff --git a/drivers/media/radio/si470x/radio-si470x-i2c.c
b/drivers/media/radio/si470x/radio-si470x-i2c.c
index a7ac09c55188..15eea2b2c90f 100644
--- a/drivers/media/radio/si470x/radio-si470x-i2c.c
+++ b/drivers/media/radio/si470x/radio-si470x-i2c.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include "radio-si470x.h"
@@ -392,6 +393,17 @@ static int si470x_i2c_probe(struct i2c_client *client,
radio->videodev.release = video_device_release_empty;
video_set_drvdata(>videodev, radio);
+ radio->gpio_reset = devm_gpiod_get_optional(>dev, "reset",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(radio->gpio_reset)) {
+ retval = PTR_ERR(radio->gpio_reset);
+ dev_err(>dev, "Failed to request gpio: %d\n", retval);
+ goto err_all;
+ }
+
+ if (radio->gpio_reset)
+ gpiod_set_value(radio->gpio_reset, 1);
+
/* power up : need 110ms */
radio->registers[POWERCFG] = POWERCFG_ENABLE;
if (si470x_set_register(radio, POWERCFG) < 0) {
@@ -478,6 +490,9 @@ static int si470x_i2c_remove(struct i2c_client *client)
video_unregister_device(>videodev);
+ if (radio->gpio_reset)
+ gpiod_set_value(radio->gpio_reset, 0);
+
return 0;
}
diff --git a/drivers/media/radio/si470x/radio-si470x.h
b/drivers/media/radio/si470x/radio-si470x.h
index 35fa0f3bbdd2..6fd6a399cb77 100644
--- a/drivers/media/radio/si470x/radio-si470x.h
+++ b/drivers/media/radio/si470x/radio-si470x.h
@@ -189,6 +189,7 @@ struct si470x_device {
#if IS_ENABLED(CONFIG_I2C_SI470X)
struct i2c_client *client;
+ struct gpio_desc *gpio_reset;
#endif
};
-- 2.17.1
[PATCH] media: cedrus: don't initialize pointers with zero
A common mistake is to assume that initializing a var with:
struct foo f = { 0 };
Would initialize a zeroed struct. Actually, what this does is
to initialize the first element of the struct to zero.
According to C99 Standard 6.7.8.21:
"If there are fewer initializers in a brace-enclosed
list than there are elements or members of an aggregate,
or fewer characters in a string literal used to initialize
an array of known size than there are elements in the array,
the remainder of the aggregate shall be initialized implicitly
the same as objects that have static storage duration."
So, in practice, it could zero the entire struct, but, if the
first element is not an integer, it will produce warnings:
drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
warning: Using plain integer as NULL pointer
drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
warning: Using plain integer as NULL pointer
A proper way to initialize it with gcc is to use:
struct foo f = { };
But that seems to be a gcc extension. So, I decided to check upstream
what's the most commonly pattern. The gcc pattern has about 2000 entries:
$ git grep -E "=\s*\{\s*\}"|wc -l
1951
The standard-C compliant pattern has about 2500 entries:
$ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l
137
$ git grep -E "=\s*\{\s*0\s*\}"|wc -l
2323
So, let's initialize those structs with:
= { NULL }
Signed-off-by: Mauro Carvalho Chehab
---
drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +-
drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
b/drivers/staging/media/sunxi/cedrus/cedrus.c
index b538eb0321d8..44c45c687503 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct
cedrus_ctx *ctx)
memset(ctx->ctrls, 0, ctrl_size);
for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
- struct v4l2_ctrl_config cfg = { 0 };
+ struct v4l2_ctrl_config cfg = { NULL };
cfg.elem_size = cedrus_controls[i].elem_size;
cfg.id = cedrus_controls[i].id;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index e40180a33951..4099a42dba2d 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -26,7 +26,7 @@ void cedrus_device_run(void *priv)
{
struct cedrus_ctx *ctx = priv;
struct cedrus_dev *dev = ctx->dev;
- struct cedrus_run run = { 0 };
+ struct cedrus_run run = { NULL };
struct media_request *src_req;
unsigned long flags;
--
2.19.2
Re: [PATCHv3 1/1] Add ir-rcmm-driver
Hi Patrick,
On Fri, Dec 07, 2018 at 10:57:21AM +0100, Patrick LERDA wrote:
> Add support for RCMM infrared remote controls.
>
> Signed-off-by: Patrick Lerda
Other than the Signed-off-by this looks exactly like the v2 version;
did you see my other comments on the v2 patch?
Thanks
Sean
> ---
> drivers/media/rc/Kconfig | 10 ++
> drivers/media/rc/Makefile | 1 +
> drivers/media/rc/ir-rcmm-decoder.c | 185 +
> drivers/media/rc/rc-core-priv.h| 5 +
> drivers/media/rc/rc-main.c | 3 +
> include/media/rc-map.h | 6 +-
> include/uapi/linux/lirc.h | 1 +
> tools/include/uapi/linux/lirc.h| 1 +
> 8 files changed, 210 insertions(+), 2 deletions(-)
> create mode 100644 drivers/media/rc/ir-rcmm-decoder.c
>
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index 1021c08a9ba4..b7e08324b874 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -133,6 +133,16 @@ config IR_IMON_DECODER
> remote control and you would like to use it with a raw IR
> receiver, or if you wish to use an encoder to transmit this IR.
>
> +config IR_RCMM_DECODER
> + tristate "Enable IR raw decoder for the RC-MM protocol"
> + depends on RC_CORE
> + select BITREVERSE
> + default y
> +
> + ---help---
> +Enable this option if you have IR with RC-MM protocol, and
> +if the IR is decoded in software
> +
> endif #RC_DECODERS
>
> menuconfig RC_DEVICES
> diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
> index e0340d043fe8..fc4058013234 100644
> --- a/drivers/media/rc/Makefile
> +++ b/drivers/media/rc/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_IR_SHARP_DECODER) += ir-sharp-decoder.o
> obj-$(CONFIG_IR_MCE_KBD_DECODER) += ir-mce_kbd-decoder.o
> obj-$(CONFIG_IR_XMP_DECODER) += ir-xmp-decoder.o
> obj-$(CONFIG_IR_IMON_DECODER) += ir-imon-decoder.o
> +obj-$(CONFIG_IR_RCMM_DECODER) += ir-rcmm-decoder.o
>
> # stand-alone IR receivers/transmitters
> obj-$(CONFIG_RC_ATI_REMOTE) += ati_remote.o
> diff --git a/drivers/media/rc/ir-rcmm-decoder.c
> b/drivers/media/rc/ir-rcmm-decoder.c
> new file mode 100644
> index ..94d5b70f7a0f
> --- /dev/null
> +++ b/drivers/media/rc/ir-rcmm-decoder.c
> @@ -0,0 +1,185 @@
> +/* ir-rcmm-decoder.c - A decoder for the RCMM IR protocol
> + *
> + * Copyright (C) 2016 by Patrick Lerda
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include "rc-core-priv.h"
> +#include
> +#include
> +
> +
> +#define RCMM_UNIT17 /* nanosecs */
> +#define RCMM_0_NBITS 64
> +#define RCMM_PREFIX_PULSE41 /* 16.6*2.5 */
> +#define RCMM_PULSE_027 /* 16.6*(1+2/3) */
> +#define RCMM_PULSE_144 /* 16.6*(2+2/3) */
> +#define RCMM_PULSE_261 /* 16.6*(3+2/3) */
> +#define RCMM_PULSE_378 /* 16.6*(4+2/3) */
> +#define RCMM_MODE_MASK 0x
> +
> +enum rcmm_state {
> + STATE_INACTIVE,
> + STATE_LOW,
> + STATE_BUMP,
> + STATE_VALUE,
> + STATE_FINISHED,
> +};
> +
> +static bool rcmm_mode(struct rcmm_dec *data)
> +{
> +return !((0x000c & data->bits) == 0x000c);
> +}
> +
> +/**
> + * ir_rcmm_decode() - Decode one RCMM pulse or space
> + * @dev: the struct rc_dev descriptor of the device
> + * @ev: the struct ir_raw_event descriptor of the pulse/space
> + *
> + * This function returns -EINVAL if the pulse violates the state machine
> + */
> +static int ir_rcmm_decode(struct rc_dev *dev, struct ir_raw_event ev)
> +{
> + struct rcmm_dec *data = >raw->rcmm;
> + u32 scancode;
> + u8 toggle;
> +
> + if (!(dev->enabled_protocols & RC_PROTO_BIT_RCMM))
> + return 0;
> +
> + if (!is_timing_event(ev)) {
> + if (ev.reset)
> + data->state = STATE_INACTIVE;
> + return 0;
> + }
> +
> + if (ev.duration > RCMM_PULSE_3 + RCMM_UNIT)
> + goto out;
> +
> + switch (data->state) {
> +
> + case STATE_INACTIVE:
> + if (!ev.pulse)
> + break;
> +
> + /* Note: larger margin on first pulse since each RCMM_UNIT
> +is quite short and some hardware takes some time to
> +adjust to the signal */
> + if (!eq_margin(ev.duration, RCMM_PREFIX_PULSE, RCMM_UNIT/2))
> + break;
> +
> + data->state =
[PATCHv3 1/1] Add ir-rcmm-driver
Add support for RCMM infrared remote controls.
Signed-off-by: Patrick Lerda
---
drivers/media/rc/Kconfig | 10 ++
drivers/media/rc/Makefile | 1 +
drivers/media/rc/ir-rcmm-decoder.c | 185 +
drivers/media/rc/rc-core-priv.h| 5 +
drivers/media/rc/rc-main.c | 3 +
include/media/rc-map.h | 6 +-
include/uapi/linux/lirc.h | 1 +
tools/include/uapi/linux/lirc.h| 1 +
8 files changed, 210 insertions(+), 2 deletions(-)
create mode 100644 drivers/media/rc/ir-rcmm-decoder.c
diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index 1021c08a9ba4..b7e08324b874 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -133,6 +133,16 @@ config IR_IMON_DECODER
remote control and you would like to use it with a raw IR
receiver, or if you wish to use an encoder to transmit this IR.
+config IR_RCMM_DECODER
+ tristate "Enable IR raw decoder for the RC-MM protocol"
+ depends on RC_CORE
+ select BITREVERSE
+ default y
+
+ ---help---
+ Enable this option if you have IR with RC-MM protocol, and
+ if the IR is decoded in software
+
endif #RC_DECODERS
menuconfig RC_DEVICES
diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
index e0340d043fe8..fc4058013234 100644
--- a/drivers/media/rc/Makefile
+++ b/drivers/media/rc/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_IR_SHARP_DECODER) += ir-sharp-decoder.o
obj-$(CONFIG_IR_MCE_KBD_DECODER) += ir-mce_kbd-decoder.o
obj-$(CONFIG_IR_XMP_DECODER) += ir-xmp-decoder.o
obj-$(CONFIG_IR_IMON_DECODER) += ir-imon-decoder.o
+obj-$(CONFIG_IR_RCMM_DECODER) += ir-rcmm-decoder.o
# stand-alone IR receivers/transmitters
obj-$(CONFIG_RC_ATI_REMOTE) += ati_remote.o
diff --git a/drivers/media/rc/ir-rcmm-decoder.c
b/drivers/media/rc/ir-rcmm-decoder.c
new file mode 100644
index ..94d5b70f7a0f
--- /dev/null
+++ b/drivers/media/rc/ir-rcmm-decoder.c
@@ -0,0 +1,185 @@
+/* ir-rcmm-decoder.c - A decoder for the RCMM IR protocol
+ *
+ * Copyright (C) 2016 by Patrick Lerda
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include "rc-core-priv.h"
+#include
+#include
+
+
+#define RCMM_UNIT 17 /* nanosecs */
+#define RCMM_0_NBITS 64
+#define RCMM_PREFIX_PULSE 41 /* 16.6*2.5 */
+#define RCMM_PULSE_027 /* 16.6*(1+2/3) */
+#define RCMM_PULSE_144 /* 16.6*(2+2/3) */
+#define RCMM_PULSE_261 /* 16.6*(3+2/3) */
+#define RCMM_PULSE_378 /* 16.6*(4+2/3) */
+#define RCMM_MODE_MASK 0x
+
+enum rcmm_state {
+ STATE_INACTIVE,
+ STATE_LOW,
+ STATE_BUMP,
+ STATE_VALUE,
+ STATE_FINISHED,
+};
+
+static bool rcmm_mode(struct rcmm_dec *data)
+{
+return !((0x000c & data->bits) == 0x000c);
+}
+
+/**
+ * ir_rcmm_decode() - Decode one RCMM pulse or space
+ * @dev: the struct rc_dev descriptor of the device
+ * @ev:the struct ir_raw_event descriptor of the pulse/space
+ *
+ * This function returns -EINVAL if the pulse violates the state machine
+ */
+static int ir_rcmm_decode(struct rc_dev *dev, struct ir_raw_event ev)
+{
+ struct rcmm_dec *data = >raw->rcmm;
+ u32 scancode;
+ u8 toggle;
+
+ if (!(dev->enabled_protocols & RC_PROTO_BIT_RCMM))
+ return 0;
+
+ if (!is_timing_event(ev)) {
+ if (ev.reset)
+ data->state = STATE_INACTIVE;
+ return 0;
+ }
+
+ if (ev.duration > RCMM_PULSE_3 + RCMM_UNIT)
+ goto out;
+
+ switch (data->state) {
+
+ case STATE_INACTIVE:
+ if (!ev.pulse)
+ break;
+
+ /* Note: larger margin on first pulse since each RCMM_UNIT
+ is quite short and some hardware takes some time to
+ adjust to the signal */
+ if (!eq_margin(ev.duration, RCMM_PREFIX_PULSE, RCMM_UNIT/2))
+ break;
+
+ data->state = STATE_LOW;
+ data->count = 0;
+ data->bits = 0;
+ return 0;
+
+ case STATE_LOW:
+ if (ev.pulse)
+ break;
+
+ /* Note: larger margin on first pulse since each RCMM_UNIT
+ is quite short and some hardware takes some time to
+ adjust to the signal */
+
Re: [PATCH] media: venus: Add support for H265 controls required by gstreamer V4L2 H265 module
Hi Kelvin, Thanks for the patch! On 11/30/18 7:31 PM, Kelvin Lawson wrote: > Add support for V4L2_CID_MPEG_VIDEO_HEVC_PROFILE and > V4L2_CID_MPEG_VIDEO_HEVC_LEVEL controls required by gstreamer V4L2 H265 > encoder module. > > Signed-off-by: Kelvin Lawson > --- > drivers/media/platform/qcom/venus/venc_ctrls.c | 19 +++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c > b/drivers/media/platform/qcom/venus/venc_ctrls.c > index 45910172..ad1a4d8 100644 > --- a/drivers/media/platform/qcom/venus/venc_ctrls.c > +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c > @@ -101,6 +101,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_MPEG_VIDEO_H264_PROFILE: > ctr->profile.h264 = ctrl->val; > break; > + case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE: > + ctr->profile.hevc = ctrl->val; > + break; > case V4L2_CID_MPEG_VIDEO_VP8_PROFILE: > ctr->profile.vpx = ctrl->val; > break; > @@ -110,6 +113,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_MPEG_VIDEO_H264_LEVEL: > ctr->level.h264 = ctrl->val; > break; > + case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL: > + ctr->level.hevc = ctrl->val; > + break; > case V4L2_CID_MPEG_VIDEO_H264_I_FRAME_QP: > ctr->h264_i_qp = ctrl->val; > break; > @@ -217,6 +223,19 @@ int venc_ctrl_init(struct venus_inst *inst) > 0, V4L2_MPEG_VIDEO_MPEG4_LEVEL_0); > > v4l2_ctrl_new_std_menu(>ctrl_handler, _ctrl_ops, > + V4L2_CID_MPEG_VIDEO_HEVC_PROFILE, > + V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN_10, > + ~((1 << V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN) | > + (1 << V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN_STILL_PICTURE) | > + (1 << V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN_10)), > + V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN); > + > + v4l2_ctrl_new_std_menu(>ctrl_handler, _ctrl_ops, > + V4L2_CID_MPEG_VIDEO_HEVC_LEVEL, > + V4L2_MPEG_VIDEO_HEVC_LEVEL_6_2, > + 0, V4L2_MPEG_VIDEO_HEVC_LEVEL_1); > + > + v4l2_ctrl_new_std_menu(>ctrl_handler, _ctrl_ops, you are adding two new menu controls but forgot to increment the count of controls in v4l2_ctrl_handler_init(). Can you resend with that fixed? > V4L2_CID_MPEG_VIDEO_H264_PROFILE, > V4L2_MPEG_VIDEO_H264_PROFILE_MULTIVIEW_HIGH, > ~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) | > -- regards, Stan
Re: [PATCH v2] media: venus: Support V4L2 QP parameters in Venus encoder
Hi Kelvin, Thanks for the patch! On 11/30/18 2:07 AM, Kelvin Lawson wrote: > Support V4L2 QP parameters in Venus encoder: > * V4L2_CID_MPEG_VIDEO_H264_I_FRAME_QP > * V4L2_CID_MPEG_VIDEO_H264_B_FRAME_QP > * V4L2_CID_MPEG_VIDEO_H264_MIN_QP > * V4L2_CID_MPEG_VIDEO_H264_MAX_QP > > Signed-off-by: Kelvin Lawson > --- > drivers/media/platform/qcom/venus/venc.c | 19 +++ > 1 file changed, 19 insertions(+) Acked-by: Stanimir Varbanov -- regards, Stan
[RFC PATCH] media/Kconfig: always enable MEDIA_CONTROLLER and VIDEO_V4L2_SUBDEV_API
This patch selects MEDIA_CONTROLLER for all camera, analog TV and digital TV drivers and selects VIDEO_V4L2_SUBDEV_API automatically. This will allow us to simplify drivers that currently have to add #ifdef CONFIG_MEDIA_CONTROLLER or #ifdef VIDEO_V4L2_SUBDEV_API to their code, since now this will always be available. The original intent of allowing these to be configured by the user was (I think) to save a bit of memory. But as more and more drivers have a media controller and all regular distros already enable one or more of those drivers, the memory for the MC code is there anyway. Complexity has always been the bane of media drivers, so reducing complexity at the expense of a bit more memory (which is a rounding error compared to the amount of video buffer memory needed) is IMHO a good thing. Signed-off-by: Hans Verkuil --- diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig index 8add62a18293..56eb01cc8bb4 100644 --- a/drivers/media/Kconfig +++ b/drivers/media/Kconfig @@ -31,6 +31,7 @@ comment "Multimedia core support" # config MEDIA_CAMERA_SUPPORT bool "Cameras/video grabbers support" + select MEDIA_CONTROLLER ---help--- Enable support for webcams and video grabbers. @@ -38,6 +39,7 @@ config MEDIA_CAMERA_SUPPORT config MEDIA_ANALOG_TV_SUPPORT bool "Analog TV support" + select MEDIA_CONTROLLER ---help--- Enable analog TV support. @@ -50,6 +52,7 @@ config MEDIA_ANALOG_TV_SUPPORT config MEDIA_DIGITAL_TV_SUPPORT bool "Digital TV support" + select MEDIA_CONTROLLER ---help--- Enable digital TV support. @@ -95,7 +98,6 @@ source "drivers/media/cec/Kconfig" config MEDIA_CONTROLLER bool "Media Controller API" - depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT ---help--- Enable the media controller API used to query media devices internal topology and configure it dynamically. @@ -119,16 +121,11 @@ config VIDEO_DEV tristate depends on MEDIA_SUPPORT depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT || MEDIA_SDR_SUPPORT + select VIDEO_V4L2_SUBDEV_API if MEDIA_CONTROLLER default y config VIDEO_V4L2_SUBDEV_API - bool "V4L2 sub-device userspace API" - depends on VIDEO_DEV && MEDIA_CONTROLLER - ---help--- - Enables the V4L2 sub-device pad-level userspace API used to configure - video format, size and frame rate between hardware blocks. - - This API is mostly used by camera interfaces in embedded platforms. + bool source "drivers/media/v4l2-core/Kconfig"
Re: Invite for IRC meeting: Re: [PATCHv4 01/10] videodev2.h: add tag support
Hi Hans, On Fri, Dec 7, 2018 at 12:08 AM Hans Verkuil wrote: > > Mauro raised a number of objections on irc regarding tags: > > https://linuxtv.org/irc/irclogger_log/media-maint?date=2018-12-06,Thu > > I would like to setup an irc meeting to discuss this and come to a > conclusion, since we need to decide this soon since this is critical > for stateless codec support. > > Unfortunately timezone-wise this is a bit of a nightmare. I think > that at least Mauro, myself and Tomasz Figa should be there, so UTC-2, > UTC+1 and UTC+9 (if I got that right). > > I propose 9 AM UTC which I think will work for everyone except Nicolas. > Any day next week works for me, and (for now) as well for Mauro. Let's pick > Monday to start with, and if you want to join in, then let me know. If that > day doesn't work for you, let me know what other days next week do work for > you. Monday (or any other day next week) 9AM UTC should work for me! Thanks, Alex.
