Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1650065 > unrolled thread
| Started by | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| First post | 2017-05-25 02:40 +0200 |
| Last post | 2017-06-02 02:50 +0200 |
| Articles | 17 on this page of 57 — 7 participants |
Back to article view | Back to linux.kernel
[PATCH v7 00/34] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 05/34] ARM: dts: imx6qdl: Add compatible, clocks, irqs to MIPI CSI-2 node Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 21/34] media: imx: Add Capture Device Interface Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 25/34] media: imx: Add MIPI CSI-2 Receiver subdev driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 13/34] ARM: dts: imx6-sabreauto: add pinctrl for gpt input capture Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 31/34] media: imx: csi: add frame size/interval enumeration Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 14/34] ARM: dts: imx6-sabreauto: add the ADV7180 video decoder Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 27/34] media: imx: csi: add support for bayer formats Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 08/34] ARM: dts: imx6qdl-sabrelite: remove erratum ERR006687 workaround Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 28/34] media: imx: csi: increase burst size for YUV formats Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 30/34] media: imx: csi: add sink selection rectangles Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 09/34] ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 19/34] media: Add userspace header file for i.MX Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 12/34] ARM: dts: imx6-sabreauto: add reset-gpios property for max7310_b Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 06/34] ARM: dts: imx6qdl: Add video multiplexers, mipi_csi, and their connections Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 15/34] add mux and video interface bridge entity functions Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
Re: [PATCH v7 15/34] add mux and video interface bridge entity functions Hans Verkuil <hverkuil@xs4all.nl> - 2017-05-29 15:40 +0200
Re: [PATCH v7 15/34] add mux and video interface bridge entity functions Hans Verkuil <hverkuil@xs4all.nl> - 2017-05-29 16:00 +0200
Re: [PATCH v7 15/34] add mux and video interface bridge entity functions Philipp Zabel <p.zabel@pengutronix.de> - 2017-05-29 16:00 +0200
[PATCH v7 29/34] media: imx: csi: add frame skipping support Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 04/34] ARM: dts: imx6qdl: add multiplexer controls Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 26/34] ARM: imx_v6_v7_defconfig: Enable staging video4linux drivers Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 01/34] dt-bindings: Add bindings for video-multiplexer device Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 23/34] media: imx: Add VDIC subdev driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 18/34] platform: video-mux: include temporary mmio-mux support Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
Re: [PATCH v7 18/34] platform: video-mux: include temporary mmio-mux support kbuild test robot <lkp@intel.com> - 2017-05-25 10:00 +0200
[PATCH] platform: video-mux: fix ptr_ret.cocci warnings kbuild test robot <lkp@intel.com> - 2017-05-25 10:00 +0200
[PATCH v7 17/34] platform: add video-multiplexer subdevice driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 34/34] media: imx: Drop warning upon multiple S_STREAM disable calls Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:40 +0200
[PATCH v7 02/34] [media] dt-bindings: Add bindings for i.MX media driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:50 +0200
[PATCH v7 10/34] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-25 02:50 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-05-29 15:50 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-05-29 16:20 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-05-29 16:30 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-05-29 17:30 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Sakari Ailus <sakari.ailus@iki.fi> - 2017-05-29 17:40 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-05-29 18:30 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-29 20:20 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Pavel Machek <pavel@ucw.cz> - 2017-05-31 22:20 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-05-29 19:30 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-29 19:30 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-05-30 00:00 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Sakari Ailus <sakari.ailus@iki.fi> - 2017-05-30 09:00 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-06-03 20:10 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-06-04 20:10 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-07 14:40 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Pavel Machek <pavel@ucw.cz> - 2017-05-31 22:00 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-01 10:30 +0200
exposure vs. exposure_absolute was Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Pavel Machek <pavel@ucw.cz> - 2017-06-01 10:50 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-06-03 21:40 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Pavel Machek <pavel@ucw.cz> - 2017-06-03 22:00 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-04 00:00 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Pavel Machek <pavel@ucw.cz> - 2017-06-04 00:20 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-06-04 06:50 +0200
Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-07 14:40 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Tim Harvey <tharvey@gateworks.com> - 2017-06-02 02:30 +0200
Re: [PATCH v7 00/34] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-06-02 02:50 +0200
Page 3 of 3 — ← Prev page 1 2 [3]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-05-29 19:30 +0200 |
| Message-ID | <tMwEp-37j-5@gated-at.bofh.it> |
| In reply to | #1652535 |
Hi Hans, thanks for the reply... On 05/29/2017 06:46 AM, Hans Verkuil wrote: > Hi Steve, > > On 05/25/2017 02:29 AM, Steve Longerbeam wrote: >> In version 7: >> >> > > What is the status as of v7? > > From what I can tell patch 2/34 needs an Ack from Rob Herring, Yes still missing that Ack. I think the issue is likely the Synopsys DW mipi csi-2 bindings. Someone earlier noted that there is another driver under devel for this Synopsys core, with another set of bindings. But it was determined that in fact this is a different device with a different register set. From what I remember of dealing with Synopsys cores in the past, these cores are highly configurable using their coreBuilder tools. So while the other device might stem from the same initial core from Synopsys, it was probably built with different design parameters compared to the core that exists in the i.MX6. So in essence it is a different device. > patches > 4-14 are out of scope for the media subsystem, Ok. I did submit patches 4-14 to the right set of folks. Should I just drop this set in the next submission if they have not changed? > patches 20-25 and 27-34 > are all staging (so fine to be merged from my point of view). > > I'm not sure if patch 26 (defconfig) should be applied while the imx > driver is in staging. I would suggest that this patch is moved to the end > of the series. Ok. > > That leaves patches 15-19. I replied to patch 15 with a comment, patches > 16-18 look good to me, although patches 17 and 18 should be combined > to one > patch since patch 17 won't compile otherwise. Any idea when the > multiplexer is > expected to be merged? (just curious) Philipp replied separately. > > I would really like to get this merged for 4.13, so did I miss anything? > From what I can tell it is really just an Ack for patch 2/34. Agreed. Steve > > >> >> >> Marek Vasut (1): >> media: imx: Drop warning upon multiple S_STREAM disable calls >> >> Philipp Zabel (9): >> dt-bindings: Add bindings for video-multiplexer device >> ARM: dts: imx6qdl: add multiplexer controls >> ARM: dts: imx6qdl: Add video multiplexers, mipi_csi, and their >> connections >> add mux and video interface bridge entity functions >> platform: add video-multiplexer subdevice driver >> platform: video-mux: include temporary mmio-mux support >> media: imx: csi: increase burst size for YUV formats >> media: imx: csi: add frame skipping support >> media: imx: csi: add sink selection rectangles >> >> Russell King (3): >> media: imx: csi: add support for bayer formats >> media: imx: csi: add frame size/interval enumeration >> media: imx: capture: add frame sizes/interval enumeration >> >> Steve Longerbeam (21): >> [media] dt-bindings: Add bindings for i.MX media driver >> [media] dt/bindings: Add bindings for OV5640 >> ARM: dts: imx6qdl: Add compatible, clocks, irqs to MIPI CSI-2 node >> ARM: dts: imx6qdl: add capture-subsystem device >> ARM: dts: imx6qdl-sabrelite: remove erratum ERR006687 workaround >> ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors >> ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors >> ARM: dts: imx6-sabreauto: create i2cmux for i2c3 >> ARM: dts: imx6-sabreauto: add reset-gpios property for max7310_b >> ARM: dts: imx6-sabreauto: add pinctrl for gpt input capture >> ARM: dts: imx6-sabreauto: add the ADV7180 video decoder >> [media] add Omnivision OV5640 sensor driver >> media: Add userspace header file for i.MX >> media: Add i.MX media core driver >> media: imx: Add Capture Device Interface >> media: imx: Add CSI subdev driver >> media: imx: Add VDIC subdev driver >> media: imx: Add IC subdev drivers >> media: imx: Add MIPI CSI-2 Receiver subdev driver >> ARM: imx_v6_v7_defconfig: Enable staging video4linux drivers >> media: imx: set and propagate default field, colorimetry >> >> .../devicetree/bindings/media/i2c/ov5640.txt | 45 + >> Documentation/devicetree/bindings/media/imx.txt | 74 + >> .../devicetree/bindings/media/video-mux.txt | 60 + >> Documentation/media/uapi/mediactl/media-types.rst | 22 + >> Documentation/media/v4l-drivers/imx.rst | 590 ++++++ >> arch/arm/boot/dts/imx6dl-sabrelite.dts | 5 + >> arch/arm/boot/dts/imx6dl-sabresd.dts | 5 + >> arch/arm/boot/dts/imx6dl.dtsi | 189 ++ >> arch/arm/boot/dts/imx6q-sabrelite.dts | 5 + >> arch/arm/boot/dts/imx6q-sabresd.dts | 5 + >> arch/arm/boot/dts/imx6q.dtsi | 125 ++ >> arch/arm/boot/dts/imx6qdl-sabreauto.dtsi | 144 +- >> arch/arm/boot/dts/imx6qdl-sabrelite.dtsi | 152 +- >> arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 114 +- >> arch/arm/boot/dts/imx6qdl.dtsi | 20 +- >> arch/arm/configs/imx_v6_v7_defconfig | 11 + >> drivers/media/i2c/Kconfig | 9 + >> drivers/media/i2c/Makefile | 1 + >> drivers/media/i2c/ov5640.c | 2224 >> ++++++++++++++++++++ >> drivers/media/platform/Kconfig | 6 + >> drivers/media/platform/Makefile | 2 + >> drivers/media/platform/video-mux.c | 357 ++++ >> drivers/staging/media/Kconfig | 2 + >> drivers/staging/media/Makefile | 1 + >> drivers/staging/media/imx/Kconfig | 20 + >> drivers/staging/media/imx/Makefile | 12 + >> drivers/staging/media/imx/TODO | 15 + >> drivers/staging/media/imx/imx-ic-common.c | 113 + >> drivers/staging/media/imx/imx-ic-prp.c | 514 +++++ >> drivers/staging/media/imx/imx-ic-prpencvf.c | 1309 ++++++++++++ >> drivers/staging/media/imx/imx-ic.h | 38 + >> drivers/staging/media/imx/imx-media-capture.c | 775 +++++++ >> drivers/staging/media/imx/imx-media-csi.c | 1842 >> ++++++++++++++++ >> drivers/staging/media/imx/imx-media-dev.c | 665 ++++++ >> drivers/staging/media/imx/imx-media-fim.c | 463 ++++ >> drivers/staging/media/imx/imx-media-internal-sd.c | 349 +++ >> drivers/staging/media/imx/imx-media-of.c | 268 +++ >> drivers/staging/media/imx/imx-media-utils.c | 896 ++++++++ >> drivers/staging/media/imx/imx-media-vdic.c | 1009 +++++++++ >> drivers/staging/media/imx/imx-media.h | 326 +++ >> drivers/staging/media/imx/imx6-mipi-csi2.c | 697 ++++++ >> include/linux/imx-media.h | 27 + >> include/media/imx.h | 15 + >> include/uapi/linux/media.h | 6 + >> include/uapi/linux/v4l2-controls.h | 4 + >> 45 files changed, 13504 insertions(+), 27 deletions(-) >> create mode 100644 >> Documentation/devicetree/bindings/media/i2c/ov5640.txt >> create mode 100644 Documentation/devicetree/bindings/media/imx.txt >> create mode 100644 >> Documentation/devicetree/bindings/media/video-mux.txt >> create mode 100644 Documentation/media/v4l-drivers/imx.rst >> create mode 100644 drivers/media/i2c/ov5640.c >> create mode 100644 drivers/media/platform/video-mux.c >> create mode 100644 drivers/staging/media/imx/Kconfig >> create mode 100644 drivers/staging/media/imx/Makefile >> create mode 100644 drivers/staging/media/imx/TODO >> create mode 100644 drivers/staging/media/imx/imx-ic-common.c >> create mode 100644 drivers/staging/media/imx/imx-ic-prp.c >> create mode 100644 drivers/staging/media/imx/imx-ic-prpencvf.c >> create mode 100644 drivers/staging/media/imx/imx-ic.h >> create mode 100644 drivers/staging/media/imx/imx-media-capture.c >> create mode 100644 drivers/staging/media/imx/imx-media-csi.c >> create mode 100644 drivers/staging/media/imx/imx-media-dev.c >> create mode 100644 drivers/staging/media/imx/imx-media-fim.c >> create mode 100644 drivers/staging/media/imx/imx-media-internal-sd.c >> create mode 100644 drivers/staging/media/imx/imx-media-of.c >> create mode 100644 drivers/staging/media/imx/imx-media-utils.c >> create mode 100644 drivers/staging/media/imx/imx-media-vdic.c >> create mode 100644 drivers/staging/media/imx/imx-media.h >> create mode 100644 drivers/staging/media/imx/imx6-mipi-csi2.c >> create mode 100644 include/linux/imx-media.h >> create mode 100644 include/media/imx.h >> >
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-05-30 00:00 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tMARI-61W-9@gated-at.bofh.it> |
| In reply to | #1650065 |
Hi Sakari,
On 05/29/2017 08:55 AM, Sakari Ailus wrote:
> Hi Steve,
>
> A few comments below.
>
> On Wed, May 24, 2017 at 05:29:31PM -0700, Steve Longerbeam wrote:
>> This driver is based on ov5640_mipi.c from Freescale imx_3.10.17_1.0.0_beta
>> branch, modified heavily to bring forward to latest interfaces and code
>> cleanup.
>>
>> Signed-off-by: Steve Longerbeam<steve_longerbeam@mentor.com>
>> ---
>> drivers/media/i2c/Kconfig | 9 +
>> drivers/media/i2c/Makefile | 1 +
>> drivers/media/i2c/ov5640.c | 2224 ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 2234 insertions(+)
>> create mode 100644 drivers/media/i2c/ov5640.c
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index fd181c9..ff082a7 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -539,6 +539,15 @@ config VIDEO_OV2659
>> To compile this driver as a module, choose M here: the
>> module will be called ov2659.
>>
>> +config VIDEO_OV5640
>> + tristate "OmniVision OV5640 sensor support"
>> + depends on OF
>> + depends on GPIOLIB && VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
>> + depends on MEDIA_CAMERA_SUPPORT
>> + ---help---
>> + This is a Video4Linux2 sensor-level driver for the Omnivision
>> + OV5640 camera sensor with a MIPI CSI-2 interface.
>> +
>> config VIDEO_OV5645
>> tristate "OmniVision OV5645 sensor support"
>> depends on OF
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index 62323ec..dc6b0c4 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
>> obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
>> obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
>> obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
>> +obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
>> obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
>> obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
>> obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> new file mode 100644
>> index 0000000..2a032bc
>> --- /dev/null
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -0,0 +1,2224 @@
>> +/*
>> + * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. All Rights Reserved.
>> + * Copyright (C) 2014-2017 Mentor Graphics Inc.
>> + *
>> + * 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; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clkdev.h>
>> +#include <linux/ctype.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/i2c.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <media/v4l2-async.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-of.h>
> Could you rebase this on the V4L2 fwnode patchset here, please?
>
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=v4l2-acpi>
Once the fwnode patchset hits mediatree, then yes it can be
converted along with all the others under media/i2c.
> <snip>
>
>> +
>> +static int ov5640_write_reg16(struct ov5640_dev *sensor, u16 reg, u16 val)
>> +{
>> + int ret;
>> +
>> + ret = ov5640_write_reg(sensor, reg, val >> 8);
>> + if (ret)
>> + return ret;
>> +
>> + return ov5640_write_reg(sensor, reg + 1, val & 0xff);
> Does the sensor datasheet suggest doing this?
Why would the datasheet suggest or not suggest such things?
Coding details like this don't belong in the datasheet.
> Making the write in two
> transactions will make it non-atomic that could be an issue in some corner
> cases.
It's called everywhere under the same device mutex.
> <snip>
>> +
>> +static int ov5640_set_gain(struct ov5640_dev *sensor, int auto_gain)
>> +{
>> + struct ov5640_ctrls *ctrls = &sensor->ctrls;
>> +
>> + if (ctrls->auto_gain->is_new) {
>> + ov5640_mod_reg(sensor, OV5640_REG_AEC_PK_MANUAL,
>> + BIT(1), ctrls->auto_gain->val ? 0 : BIT(1));
> You're generally silently ignoring all I²C access errors. Is that
> intentional?
Yeah, this driver is much cleaned up from the original, but there are
still some issues like this. The register access errors are really only
being paid attention to during s_power() when loading the initial
register set, which is enough at least to catch a non-existent chip
or basic i2c bus or other hardware issues. But I should work on
catching all access errors. This is something I did in an earlier rev
but I used a questionable short-cut to make it easier to implement.
I'll just have to catch every case one by one.
> <snip>
>
>> +
>> +static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>> + struct ov5640_dev *sensor = to_ov5640_dev(sd);
>> + int ret = 0;
>> +
>> + mutex_lock(&sensor->lock);
> Could you use the same lock for the controls as you use for the rest? Just
> setting handler->lock after handler init does the trick.
Can you please rephrase, I don't follow. "same lock for the controls as
you use for the rest" - there's only one device lock owned by this driver
and I am already using that same lock.
> <snip>
>> +
>> +static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> + struct ov5640_dev *sensor = to_ov5640_dev(sd);
>> + int ret = 0;
>> +
>> + mutex_lock(&sensor->lock);
>> +
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> + if (sd->entity.stream_count > 1)
> The entity stream_count isn't connected to the number of times s_stream(sd,
> true) is called. Please remove the check.
It's incremented by media_pipeline_start(), even if the entity is already
a member of the given pipeline.
I added this check because in imx-media, the ov5640 can be streaming
concurrently to multiple video capture devices, and each capture device
calls
media_pipeline_start() at stream on, which increments the entity stream
count.
So if one capture device issues a stream off while others are still
streaming,
ov5640 should remain at stream on. So the entity stream count is being
used as a streaming usage counter. Is there a better way to do this? Should
I use a private stream use counter instead?
> <snip>
>
>> +
>> +free_ctrls:
>> + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
>> +entity_cleanup:
>> + mutex_destroy(&sensor->lock);
>> + media_entity_cleanup(&sensor->sd.entity);
>> + regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
> Should this still be here?
>
>> + return ret;
>> +}
>> +
>> +static int ov5640_remove(struct i2c_client *client)
>> +{
>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> + struct ov5640_dev *sensor = to_ov5640_dev(sd);
>> +
>> + regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
> Ditto.
I don't understand. regulator_bulk_disable() is still needed, am I missing
something?
Steve
[toc] | [prev] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-05-30 09:00 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tMJih-3J1-3@gated-at.bofh.it> |
| In reply to | #1652730 |
Hi Steve,
On Mon, May 29, 2017 at 02:50:34PM -0700, Steve Longerbeam wrote:
> ><snip>
> >
> >>+
> >>+static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
> >>+{
> >>+ struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> >>+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
> >>+ int ret = 0;
> >>+
> >>+ mutex_lock(&sensor->lock);
> >Could you use the same lock for the controls as you use for the rest? Just
> >setting handler->lock after handler init does the trick.
>
> Can you please rephrase, I don't follow. "same lock for the controls as
> you use for the rest" - there's only one device lock owned by this driver
> and I am already using that same lock.
There's another in the control handler. You could use your own lock for the
control handler as well.
>
>
> ><snip>
> >>+
> >>+static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
> >>+{
> >>+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
> >>+ int ret = 0;
> >>+
> >>+ mutex_lock(&sensor->lock);
> >>+
> >>+#if defined(CONFIG_MEDIA_CONTROLLER)
> >>+ if (sd->entity.stream_count > 1)
> >The entity stream_count isn't connected to the number of times s_stream(sd,
> >true) is called. Please remove the check.
>
> It's incremented by media_pipeline_start(), even if the entity is already
> a member of the given pipeline.
>
> I added this check because in imx-media, the ov5640 can be streaming
> concurrently to multiple video capture devices, and each capture device
> calls
> media_pipeline_start() at stream on, which increments the entity stream
> count.
>
> So if one capture device issues a stream off while others are still
> streaming,
> ov5640 should remain at stream on. So the entity stream count is being
> used as a streaming usage counter. Is there a better way to do this? Should
> I use a private stream use counter instead?
Different drivers may use media_pipeline_start() in different ways. Stream
control shouldn't depend on that count. This could cause issues in using the
driver with other ISP / receiver drivers.
I think it should be enough to move the check to the imx driver in this
case.
>
>
>
> ><snip>
> >
> >>+
> >>+free_ctrls:
> >>+ v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> >>+entity_cleanup:
> >>+ mutex_destroy(&sensor->lock);
> >>+ media_entity_cleanup(&sensor->sd.entity);
> >>+ regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
> >Should this still be here?
> >
> >>+ return ret;
> >>+}
> >>+
> >>+static int ov5640_remove(struct i2c_client *client)
> >>+{
> >>+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >>+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
> >>+
> >>+ regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
> >Ditto.
>
> I don't understand. regulator_bulk_disable() is still needed, am I missing
> something?
You still need to enable it first. I don't see that being done in probe. As
the driver implements the s_power() op, I don't see a need for powering the
device on at probe time (and conversely off at remove time).
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-06-03 20:10 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tOlER-1US-7@gated-at.bofh.it> |
| In reply to | #1652868 |
Hi Sakari,
On 05/29/2017 11:56 PM, Sakari Ailus wrote:
> Hi Steve,
>
> On Mon, May 29, 2017 at 02:50:34PM -0700, Steve Longerbeam wrote:
>>> <snip>
>>>
>>>> +
>>>> +static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>>>> +{
>>>> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>>>> + struct ov5640_dev *sensor = to_ov5640_dev(sd);
>>>> + int ret = 0;
>>>> +
>>>> + mutex_lock(&sensor->lock);
>>> Could you use the same lock for the controls as you use for the rest? Just
>>> setting handler->lock after handler init does the trick.
>>
>> Can you please rephrase, I don't follow. "same lock for the controls as
>> you use for the rest" - there's only one device lock owned by this driver
>> and I am already using that same lock.
>
> There's another in the control handler. You could use your own lock for the
> control handler as well.
I still don't understand.
>
>>
>>
>>> <snip>
>>>> +
>>>> +static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>>>> +{
>>>> + struct ov5640_dev *sensor = to_ov5640_dev(sd);
>>>> + int ret = 0;
>>>> +
>>>> + mutex_lock(&sensor->lock);
>>>> +
>>>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>>>> + if (sd->entity.stream_count > 1)
>>> The entity stream_count isn't connected to the number of times s_stream(sd,
>>> true) is called. Please remove the check.
>>
>> It's incremented by media_pipeline_start(), even if the entity is already
>> a member of the given pipeline.
>>
>> I added this check because in imx-media, the ov5640 can be streaming
>> concurrently to multiple video capture devices, and each capture device
>> calls
>> media_pipeline_start() at stream on, which increments the entity stream
>> count.
>>
>> So if one capture device issues a stream off while others are still
>> streaming,
>> ov5640 should remain at stream on. So the entity stream count is being
>> used as a streaming usage counter. Is there a better way to do this? Should
>> I use a private stream use counter instead?
>
> Different drivers may use media_pipeline_start() in different ways. Stream
> control shouldn't depend on that count. This could cause issues in using the
> driver with other ISP / receiver drivers.
>
> I think it should be enough to move the check to the imx driver in this
> case.
I will remove this check.
>>>> +
>>>> +static int ov5640_remove(struct i2c_client *client)
>>>> +{
>>>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>> + struct ov5640_dev *sensor = to_ov5640_dev(sd);
>>>> +
>>>> + regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
>>> Ditto.
>>
>> I don't understand. regulator_bulk_disable() is still needed, am I missing
>> something?
>
> You still need to enable it first. I don't see that being done in probe. As
> the driver implements the s_power() op, I don't see a need for powering the
> device on at probe time (and conversely off at remove time).
Oh you're right, it must have been left over from a previous revision
I guess. Yes, regulator_bulk_enable|disable() is only called in
ov5640_set_power(). I'll remove regulator_bulk_disable() from
probe/remove.
Steve
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-06-04 20:10 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tOI8q-fs-11@gated-at.bofh.it> |
| In reply to | #1656883 |
On 06/03/2017 11:02 AM, Steve Longerbeam wrote:
> Hi Sakari,
>
>
> On 05/29/2017 11:56 PM, Sakari Ailus wrote:
>> Hi Steve,
>>
>> On Mon, May 29, 2017 at 02:50:34PM -0700, Steve Longerbeam wrote:
>>>> <snip>
>>>>
>>>>> +
>>>>> +static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>> +{
>>>>> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>>>>> + struct ov5640_dev *sensor = to_ov5640_dev(sd);
>>>>> + int ret = 0;
>>>>> +
>>>>> + mutex_lock(&sensor->lock);
>>>> Could you use the same lock for the controls as you use for the
>>>> rest? Just
>>>> setting handler->lock after handler init does the trick.
>>>
>>> Can you please rephrase, I don't follow. "same lock for the controls as
>>> you use for the rest" - there's only one device lock owned by this
>>> driver
>>> and I am already using that same lock.
>>
>> There's another in the control handler. You could use your own lock
>> for the
>> control handler as well.
>
> I still don't understand.
>
Hi Sakari, sorry I see what you are referring to now. The lock
in 'struct v4l2_ctrl_handler' can be overridden by a caller's own
lock. Yes that's a good idea, I'll do that.
Steve
[toc] | [prev] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-06-07 14:40 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tPIpI-6D4-35@gated-at.bofh.it> |
| In reply to | #1657128 |
On Sun, Jun 04, 2017 at 11:00:14AM -0700, Steve Longerbeam wrote:
>
>
> On 06/03/2017 11:02 AM, Steve Longerbeam wrote:
> >Hi Sakari,
> >
> >
> >On 05/29/2017 11:56 PM, Sakari Ailus wrote:
> >>Hi Steve,
> >>
> >>On Mon, May 29, 2017 at 02:50:34PM -0700, Steve Longerbeam wrote:
> >>>><snip>
> >>>>
> >>>>>+
> >>>>>+static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
> >>>>>+{
> >>>>>+ struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> >>>>>+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
> >>>>>+ int ret = 0;
> >>>>>+
> >>>>>+ mutex_lock(&sensor->lock);
> >>>>Could you use the same lock for the controls as you use for the
> >>>>rest? Just
> >>>>setting handler->lock after handler init does the trick.
> >>>
> >>>Can you please rephrase, I don't follow. "same lock for the controls as
> >>>you use for the rest" - there's only one device lock owned by this
> >>>driver
> >>>and I am already using that same lock.
> >>
> >>There's another in the control handler. You could use your own lock for
> >>the
> >>control handler as well.
> >
> >I still don't understand.
> >
>
> Hi Sakari, sorry I see what you are referring to now. The lock
> in 'struct v4l2_ctrl_handler' can be overridden by a caller's own
> lock. Yes that's a good idea, I'll do that.
Ack, good! :-)
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | Pavel Machek <pavel@ucw.cz> |
|---|---|
| Date | 2017-05-31 22:00 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tNhWG-Ge-19@gated-at.bofh.it> |
| In reply to | #1650065 |
[Multipart message — attachments visible in raw view] — view raw
Hi! > +/* min/typical/max system clock (xclk) frequencies */ > +#define OV5640_XCLK_MIN 6000000 > +#define OV5640_XCLK_MAX 24000000 > + > +/* > + * FIXME: there is no subdev API to set the MIPI CSI-2 > + * virtual channel yet, so this is hardcoded for now. > + */ > +#define OV5640_MIPI_VC 1 Can the FIXME be fixed? > +/* > + * image size under 1280 * 960 are SUBSAMPLING -> Image > + * image size upper 1280 * 960 are SCALING above? > +/* > + * FIXME: all of these register tables are likely filled with > + * entries that set the register to their power-on default values, > + * and which are otherwise not touched by this driver. Those entries > + * should be identified and removed to speed register load time > + * over i2c. > + */ load->loading? Can the FIXME be fixed? > + /* Auto/manual exposure */ > + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops, > + V4L2_CID_EXPOSURE_AUTO, > + V4L2_EXPOSURE_MANUAL, 0, > + V4L2_EXPOSURE_AUTO); > + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, > + V4L2_CID_EXPOSURE_ABSOLUTE, > + 0, 65535, 1, 0); Is exposure_absolute supposed to be in microseconds...? > + /* Auto/manual gain */ > + ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN, > + 0, 1, 1, 1); > + ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, > + 0, 1023, 1, 0); > + > + ctrls->saturation = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION, > + 0, 255, 1, 64); > + ctrls->hue = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HUE, > + 0, 359, 1, 0); > + ctrls->contrast = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, > + 0, 255, 1, 0); > + ctrls->test_pattern = > + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(test_pattern_menu) - 1, > + 0, 0, test_pattern_menu); > + It is good to see sensor that has autogain/etc. I'm emulating them in v4l-utils, and hardware that supports it is a good argument. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[toc] | [prev] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-06-01 10:30 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tNtEu-9b-3@gated-at.bofh.it> |
| In reply to | #1654596 |
Hi Pavel, On Wed, May 31, 2017 at 09:58:21PM +0200, Pavel Machek wrote: > Hi! > > > +/* min/typical/max system clock (xclk) frequencies */ > > +#define OV5640_XCLK_MIN 6000000 > > +#define OV5640_XCLK_MAX 24000000 > > + > > +/* > > + * FIXME: there is no subdev API to set the MIPI CSI-2 > > + * virtual channel yet, so this is hardcoded for now. > > + */ > > +#define OV5640_MIPI_VC 1 > > Can the FIXME be fixed? Yes, but it's quite a bit of work. It makes sense to use a static virtual channel for now. A patchset which is however incomplete can be found here: <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=vc> For what it's worth, all other devices use virtual channel zero for image data and so should this one. > > > +/* > > + * image size under 1280 * 960 are SUBSAMPLING > > -> Image > > > + * image size upper 1280 * 960 are SCALING > > above? > > > +/* > > + * FIXME: all of these register tables are likely filled with > > + * entries that set the register to their power-on default values, > > + * and which are otherwise not touched by this driver. Those entries > > + * should be identified and removed to speed register load time > > + * over i2c. > > + */ > > load->loading? Can the FIXME be fixed? > > > + /* Auto/manual exposure */ > > + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops, > > + V4L2_CID_EXPOSURE_AUTO, > > + V4L2_EXPOSURE_MANUAL, 0, > > + V4L2_EXPOSURE_AUTO); > > + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, > > + V4L2_CID_EXPOSURE_ABSOLUTE, > > + 0, 65535, 1, 0); > > Is exposure_absolute supposed to be in microseconds...? Yes. OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO. Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too. Ideally we should have only one control for exposure. -- Regards, Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | Pavel Machek <pavel@ucw.cz> |
|---|---|
| Date | 2017-06-01 10:50 +0200 |
| Subject | exposure vs. exposure_absolute was Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tNtXQ-fI-9@gated-at.bofh.it> |
| In reply to | #1654919 |
[Multipart message — attachments visible in raw view] — view raw
Hi! > > > + /* Auto/manual exposure */ > > > + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops, > > > + V4L2_CID_EXPOSURE_AUTO, > > > + V4L2_EXPOSURE_MANUAL, 0, > > > + V4L2_EXPOSURE_AUTO); > > > + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, > > > + V4L2_CID_EXPOSURE_ABSOLUTE, > > > + 0, 65535, 1, 0); > > > > Is exposure_absolute supposed to be in microseconds...? > > Yes. OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO. > Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too. > > Ideally we should have only one control for exposure. No. N-o. No no no. NO! No. N-o. NONONO. No. NooooooooooOOO!!!!!!!!!!!!! Sorry, no. Userspace needs to know exposure times. It is not so important for a webcam, but it is mandatory for digital camera. As it gets darker, autogain wants to scale exposure to cca 1/100 sec, then it wants to scale gain up to maximum, and only then it wants to continue scaling exposure. (Threshold will be shorter in "sports" mode, perhaps 1/300sec?) Plus, we want user to be able to manually set exposure parameters. So... _this_ driver probably should use V4L2_CID_EXPOSURE. (If the units are not known). But in general we'd prefer drivers using V4L2_CID_EXPOSURE_ABSOLUTE. Your car has speedometer calibrated in km/h or mph, not in "% of max", right? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-06-03 21:40 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tOn3X-2HQ-13@gated-at.bofh.it> |
| In reply to | #1654919 |
On 06/01/2017 01:26 AM, Sakari Ailus wrote: > Hi Pavel, > > On Wed, May 31, 2017 at 09:58:21PM +0200, Pavel Machek wrote: >> Hi! >> >>> +/* min/typical/max system clock (xclk) frequencies */ >>> +#define OV5640_XCLK_MIN 6000000 >>> +#define OV5640_XCLK_MAX 24000000 >>> + >>> +/* >>> + * FIXME: there is no subdev API to set the MIPI CSI-2 >>> + * virtual channel yet, so this is hardcoded for now. >>> + */ >>> +#define OV5640_MIPI_VC 1 >> >> Can the FIXME be fixed? > > Yes, but it's quite a bit of work. It makes sense to use a static virtual > channel for now. A patchset which is however incomplete can be found here: > > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=vc> > > For what it's worth, all other devices use virtual channel zero for image > data and so should this one. Actually no. The CSI2IPU gasket in i.MX6 quad sends virtual channel 0 streams to IPU1-CSI0. But input to IPU1-CSI0 is also muxed with parallel bus cameras. So if vc0 were chosen instead, platforms that support parallel cameras to IPU1-CSI0 (SabreLite, SabreSD) would not be able to use them concurrently with a MIPI CSI-2 source to IPU1-CSI1. So I prefer to use static channel 1 to support those platforms. I could convert this to a module parameter however, until a virtual channel selection subdev API becomes available, at which point that would have to be stripped. > >> >>> +/* >>> + * image size under 1280 * 960 are SUBSAMPLING >> >> -> Image >> >>> + * image size upper 1280 * 960 are SCALING >> >> above? >> done. >>> +/* >>> + * FIXME: all of these register tables are likely filled with >>> + * entries that set the register to their power-on default values, >>> + * and which are otherwise not touched by this driver. Those entries >>> + * should be identified and removed to speed register load time >>> + * over i2c. >>> + */ >> >> load->loading? Can the FIXME be fixed? That's a lot of work, and risky work at that. If someone could take this on (strip out power-on default values from the tables), I'd be grateful, but I don't have the time. For now at least, these registers sets work fine. >> >>> + /* Auto/manual exposure */ >>> + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops, >>> + V4L2_CID_EXPOSURE_AUTO, >>> + V4L2_EXPOSURE_MANUAL, 0, >>> + V4L2_EXPOSURE_AUTO); >>> + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, >>> + V4L2_CID_EXPOSURE_ABSOLUTE, >>> + 0, 65535, 1, 0); >> >> Is exposure_absolute supposed to be in microseconds...? > > Yes. According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units. OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO. > Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too. Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not taking 100 usec units, so unit-less is better. Steve
[toc] | [prev] | [next] | [standalone]
| From | Pavel Machek <pavel@ucw.cz> |
|---|---|
| Date | 2017-06-03 22:00 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tOnnj-2O9-1@gated-at.bofh.it> |
| In reply to | #1656913 |
[Multipart message — attachments visible in raw view] — view raw
Hi! > >>>+ /* Auto/manual exposure */ > >>>+ ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops, > >>>+ V4L2_CID_EXPOSURE_AUTO, > >>>+ V4L2_EXPOSURE_MANUAL, 0, > >>>+ V4L2_EXPOSURE_AUTO); > >>>+ ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, > >>>+ V4L2_CID_EXPOSURE_ABSOLUTE, > >>>+ 0, 65535, 1, 0); > >> > >>Is exposure_absolute supposed to be in microseconds...? > > > >Yes. > > According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units. > > OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO. > >Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too. > > Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not > taking 100 usec units, so unit-less is better. Thanks. If you know the units, it would be of course better to use right units... Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[toc] | [prev] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-06-04 00:00 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tOpfs-3WE-3@gated-at.bofh.it> |
| In reply to | #1656917 |
On Sat, Jun 03, 2017 at 09:51:39PM +0200, Pavel Machek wrote: > Hi! > > > >>>+ /* Auto/manual exposure */ > > >>>+ ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops, > > >>>+ V4L2_CID_EXPOSURE_AUTO, > > >>>+ V4L2_EXPOSURE_MANUAL, 0, > > >>>+ V4L2_EXPOSURE_AUTO); > > >>>+ ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, > > >>>+ V4L2_CID_EXPOSURE_ABSOLUTE, > > >>>+ 0, 65535, 1, 0); > > >> > > >>Is exposure_absolute supposed to be in microseconds...? > > > > > >Yes. > > > > According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units. > > > > OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO. > > >Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too. > > > > Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not > > taking 100 usec units, so unit-less is better. > > Thanks. If you know the units, it would be of course better to use > right units... Steve: what's the unit in this case? Is it lines or something else? Pavel: we do need to make sure the user space will be able to know the unit, too. It's rather a case with a number of controls: the unit is known but there's no API to convey it to the user. The exposure is a bit special, too: granularity matters a lot on small values. On most other controls it does not. -- Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | Pavel Machek <pavel@ucw.cz> |
|---|---|
| Date | 2017-06-04 00:20 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tOpyN-4jB-3@gated-at.bofh.it> |
| In reply to | #1656938 |
[Multipart message — attachments visible in raw view] — view raw
Hi! > > > According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units. > > > > > > OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO. > > > >Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too. > > > > > > Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not > > > taking 100 usec units, so unit-less is better. > > > > Thanks. If you know the units, it would be of course better to use > > right units... > > Steve: what's the unit in this case? Is it lines or something else? > > Pavel: we do need to make sure the user space will be able to know the unit, > too. It's rather a case with a number of controls: the unit is known but > there's no API to convey it to the user. > > The exposure is a bit special, too: granularity matters a lot on small > values. On most other controls it does not. Yeah. Basically problem with exposure is that the control is logarithmic; by using linear scale we got too much resolution at long times and too little resolution at short times. (Plus, 100 usec ... n900 can do times _way_ shorter than that.) Anyway, even u32 gives us enough range, but I so the linear/log confusion does not matter. But it would be nicer if values were in 10 usec or usec, not 100 usec... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-06-04 06:50 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tOvEd-8h9-3@gated-at.bofh.it> |
| In reply to | #1656938 |
On 06/03/2017 02:57 PM, Sakari Ailus wrote: > On Sat, Jun 03, 2017 at 09:51:39PM +0200, Pavel Machek wrote: >> Hi! >> >>>>>> + /* Auto/manual exposure */ >>>>>> + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops, >>>>>> + V4L2_CID_EXPOSURE_AUTO, >>>>>> + V4L2_EXPOSURE_MANUAL, 0, >>>>>> + V4L2_EXPOSURE_AUTO); >>>>>> + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, >>>>>> + V4L2_CID_EXPOSURE_ABSOLUTE, >>>>>> + 0, 65535, 1, 0); >>>>> >>>>> Is exposure_absolute supposed to be in microseconds...? >>>> >>>> Yes. >>> >>> According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units. >>> >>> OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO. >>>> Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too. >>> >>> Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not >>> taking 100 usec units, so unit-less is better. >> >> Thanks. If you know the units, it would be of course better to use >> right units... > > Steve: what's the unit in this case? Is it lines or something else? Yes, the register interface for exposure takes lines*16. Maybe converting from seconds to lines is as simple as framerate * height * seconds. But I'm not sure about that. Steve > > Pavel: we do need to make sure the user space will be able to know the unit, > too. It's rather a case with a number of controls: the unit is known but > there's no API to convey it to the user. > > The exposure is a bit special, too: granularity matters a lot on small > values. On most other controls it does not. >
[toc] | [prev] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-06-07 14:40 +0200 |
| Subject | Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver |
| Message-ID | <tPIpJ-6D4-39@gated-at.bofh.it> |
| In reply to | #1656990 |
On Sat, Jun 03, 2017 at 09:46:36PM -0700, Steve Longerbeam wrote: > > > On 06/03/2017 02:57 PM, Sakari Ailus wrote: > >On Sat, Jun 03, 2017 at 09:51:39PM +0200, Pavel Machek wrote: > >>Hi! > >> > >>>>>>+ /* Auto/manual exposure */ > >>>>>>+ ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops, > >>>>>>+ V4L2_CID_EXPOSURE_AUTO, > >>>>>>+ V4L2_EXPOSURE_MANUAL, 0, > >>>>>>+ V4L2_EXPOSURE_AUTO); > >>>>>>+ ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, > >>>>>>+ V4L2_CID_EXPOSURE_ABSOLUTE, > >>>>>>+ 0, 65535, 1, 0); > >>>>> > >>>>>Is exposure_absolute supposed to be in microseconds...? > >>>> > >>>>Yes. > >>> > >>>According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units. > >>> > >>> OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO. > >>>>Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too. > >>> > >>>Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not > >>>taking 100 usec units, so unit-less is better. > >> > >>Thanks. If you know the units, it would be of course better to use > >>right units... > > > >Steve: what's the unit in this case? Is it lines or something else? > > Yes, the register interface for exposure takes lines*16. > > Maybe converting from seconds to lines is as simple as > framerate * height * seconds. But I'm not sure about that. The smiapp and a few other drivers are using lines. One option could be to use lines as the unit and have step as 16. Then the hblank + vblank controls will be needed, too, to enable the user to at least figure out the conversion to Si units. -- Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | Tim Harvey <tharvey@gateworks.com> |
|---|---|
| Date | 2017-06-02 02:30 +0200 |
| Message-ID | <tNIDw-1yO-13@gated-at.bofh.it> |
| In reply to | #1650065 |
On Wed, May 24, 2017 at 5:29 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
> In version 7:
>
> - video-mux: switched to Philipp's latest video-mux driver and updated
> bindings docs, that makes use of the mmio-mux framework.
>
> - mmio-mux: includes Philipp's temporary patch that adds mmio-mux support
> to video-mux driver, until mux framework is merged.
>
> - mmio-mux: updates to device tree from Philipp that define the i.MX6 mux
> devices and modifies the video-mux device to become a consumer of the
> video mmio-mux.
>
> - minor updates to Documentation/media/v4l-drivers/imx.rst.
>
> - ov5640: do nothing if entity stream count is greater than 1 in
> ov5640_s_stream().
>
> - Previous versions of this driver had not tested the ability to enable
> multiple independent streams, for instance enabling multiple output
> pads from the imx6-mipi-csi2 subdevice, or enabling both prpenc and
> prpvf outputs. Marek Vasut tested this support and reported issues
> with it.
>
> v4l2_pipeline_inherit_controls() used the media graph walk APIs, but
> that walks both sink and source pads, so if there are multiple paths
> enabled to video capture devices, controls would be added to the wrong
> video capture device, and no controls added to the other enabled
> capture devices.
>
> These issues have been fixed. Control inheritance works correctly now
> even with multiple enabled capture paths, and (for example)
> simultaneous capture from prpenc and prpvf works also, and each with
> independent scaling, CSC, and controls. For example prpenc can be
> capturing with a 90 degree rotation, while prpvf is capturing with
> vertical flip.
>
> So the v4l2_pipeline_inherit_controls() patch has been dropped. The
> new version of control inheritance could be made generically available,
> but it would be more involved to incorporate it into v4l2-core.
>
> - A new function imx_media_fill_default_mbus_fields() is added to setup
> colorimetry at sink pads, and these are propagated to source pads.
>
> - Ensure that the current sink and source rectangles meet alignment
> restrictions before applying a new rotation control setting in
> prp-enc/vf subdevices.
>
> - Chain the s_stream() subdev calls instead of implementing a custom
> stream on/off function that attempts to call a fixed set of subdevices
> in a pipeline in the correct order. This also simplifies imx6-mipi-csi2
> subdevice, since the correct MIPI CSI-2 startup sequence can be
> enforced completely in s_stream(), and s_power() is no longer
> required. This also paves the way for more arbitrary OF graphs
> external to the i.MX6.
>
> - Converted the v4l2_subdev and media_entity ops structures to const.
>
Hi Steve,
I've applied adv7180 device-tree config for the Gateworks ventana
boards on top of your imx-media-staging-md-v15 github branch but am
not able to get it to work.
Here's my device-tree patch that adds adv7180 to the GW54xx connected
to IPU2_CSI1:
--- a/arch/arm/boot/dts/imx6q-gw54xx.dts
+++ b/arch/arm/boot/dts/imx6q-gw54xx.dts
@@ -18,6 +18,76 @@
compatible = "gw,imx6q-gw54xx", "gw,ventana", "fsl,imx6q";
};
+&i2c3 {
+ adv7180: camera@20 {
+ compatible = "adi,adv7180";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_adv7180>;
+ reg = <0x20>;
+ powerdown-gpios = <&gpio3 31 GPIO_ACTIVE_LOW>;
+ interrupt-parent = <&gpio3>;
+ interrupts = <30 GPIO_ACTIVE_LOW>;
+ inputs = <0x00 0x01 0x02>;
+ input-names = "ADV7180 Composite on Ain1",
+ "ADV7180 Composite on Ain2",
+ "ADV7180 Composite on Ain3";
+
+ port {
+ adv7180_to_ipu2_csi1_mux: endpoint {
+ remote-endpoint =
<&ipu2_csi1_mux_from_parallel_sensor>;
+ bus-width = <8>;
+ };
+ };
+ };
+};
+
+&ipu2_csi1_from_ipu2_csi1_mux {
+ bus-width = <8>;
+};
+
+&ipu2_csi1_mux_from_parallel_sensor {
+ remote-endpoint = <&adv7180_to_ipu2_csi1_mux>;
+ bus-width = <8>;
+};
+
+&ipu2_csi1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_ipu2_csi1>;
+
+ /* enable frame interval monitor on this port */
+ fim {
+ status = "okay";
+ };
+};
+
&sata {
status = "okay";
};
+
+&iomuxc {
+ video {
+ pinctrl_adv7180: adv7180grp {
+ fsl,pins = <
+ MX6QDL_PAD_EIM_D30__GPIO3_IO30
0x0001b0b0
+ MX6QDL_PAD_EIM_D31__GPIO3_IO31
0x4001b0b0
+ >;
+ };
+
+ pinctrl_ipu2_csi1: ipu2_csi1grp { /* IPU2_CSI1: 8-bit input */
+ fsl,pins = <
+ MX6QDL_PAD_EIM_EB2__IPU2_CSI1_DATA19 0x1b0b0
+ MX6QDL_PAD_EIM_D16__IPU2_CSI1_DATA18 0x1b0b0
+ MX6QDL_PAD_EIM_D18__IPU2_CSI1_DATA17 0x1b0b0
+ MX6QDL_PAD_EIM_D19__IPU2_CSI1_DATA16 0x1b0b0
+ MX6QDL_PAD_EIM_D20__IPU2_CSI1_DATA15 0x1b0b0
+ MX6QDL_PAD_EIM_D26__IPU2_CSI1_DATA14 0x1b0b0
+ MX6QDL_PAD_EIM_D27__IPU2_CSI1_DATA13 0x1b0b0
+ MX6QDL_PAD_EIM_A17__IPU2_CSI1_DATA12 0x1b0b0
+ MX6QDL_PAD_EIM_D29__IPU2_CSI1_VSYNC 0x1b0b0
+ MX6QDL_PAD_EIM_EB3__IPU2_CSI1_HSYNC 0x1b0b0
+ MX6QDL_PAD_EIM_A16__IPU2_CSI1_PIXCLK 0x1b0b0
+ >;
+ };
+ };
+};
+
Here's my userspace test commands:
media-ctl -r # reset all links
export outputfmt="UYVY2X8/720x480"
# Setup links (ADV7180 IPU2_CSI1)
media-ctl -l '"adv7180 2-0020":0 -> "ipu2_csi1_mux":1[1]'
media-ctl -l '"ipu2_csi1_mux":2 -> "ipu2_csi1":0[1]'
media-ctl -l '"ipu2_csi1":1 -> "ipu2_vdic":0[1]'
media-ctl -l '"ipu2_vdic":2 -> "ipu2_ic_prp":0[1]'
media-ctl -l '"ipu2_ic_prp":2 -> "ipu2_ic_prpvf":0[1]'
media-ctl -l '"ipu2_ic_prpvf":1 -> "ipu2_ic_prpvf capture":0[1]'
# Configure pads
media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480]"
media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480 field:interlaced]"
media-ctl -V "'ipu2_csi1':1 [fmt:UYVY2X8/720x480 field:interlaced]"
media-ctl -V "'ipu2_vdic':2 [fmt:UYVY2X8/720x480 field:none]"
media-ctl -V "'ipu2_ic_prp':2 [fmt:AYUV32/720x480 field:none]"
media-ctl -V "'ipu2_ic_prpvf':1 [fmt:$outputfmt field:none]"
^^^^ no errors up to this point; streaming can now begin on
'ipu2_ic_prpvf capture'
# select input
v4l2-ctl --device /dev/video3 -i0 # 0=AIN1 1=AIN2 2=AIN3
VIDIOC_S_INPUT: failed: Inappropriate ioctl for device
^^^^ /sys/class/video4linux/v4l-subdev2/name is 'ipu2_ic_prpvf
capture' - is this not right?
# select any supported YUV or RGB pixelformat on the capture device node
v4l2-ctl --device /dev/video3
--set-fmt-video=width=720,height=480,pixelformat=UYVY
v4l2-ctl --device /dev/video3 --stream-mmap --stream-to=/x.raw
--stream-count=1 # capture single raw-frame
[ 904.870444] ipu2_ic_prpvf: EOF timeout
VIDIOC_DQBUF: failed: Input/output error
[ 905.910702] ipu2_ic_prpvf: wait last EOF timeout
^^^^ not getting any frames
The last patchset of yours I had running on this board was your v3
patchset - any ideas?
As it looks like things have settled down with this patchset and it
sounds like it will get merged for 4.13 I'm going to start working on
a driver for the tda1997x HDMI receiver which is also on this board
connected to IPU1_CSI0.
Thanks,
Tim
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-06-02 02:50 +0200 |
| Message-ID | <tNIWR-1Hb-7@gated-at.bofh.it> |
| In reply to | #1655819 |
Hi Tim, On 06/01/2017 05:25 PM, Tim Harvey wrote: > > > Hi Steve, > > I've applied adv7180 device-tree config for the Gateworks ventana > boards on top of your imx-media-staging-md-v15 github branch but am > not able to get it to work. > > Here's my device-tree patch that adds adv7180 to the GW54xx connected > to IPU2_CSI1: > --- a/arch/arm/boot/dts/imx6q-gw54xx.dts > +++ b/arch/arm/boot/dts/imx6q-gw54xx.dts I haven't studied your device-tree in detail yet, I'll try to have a better look this weekend. <snip> > > > Here's my userspace test commands: > > media-ctl -r # reset all links > export outputfmt="UYVY2X8/720x480" > # Setup links (ADV7180 IPU2_CSI1) > media-ctl -l '"adv7180 2-0020":0 -> "ipu2_csi1_mux":1[1]' > media-ctl -l '"ipu2_csi1_mux":2 -> "ipu2_csi1":0[1]' > media-ctl -l '"ipu2_csi1":1 -> "ipu2_vdic":0[1]' > media-ctl -l '"ipu2_vdic":2 -> "ipu2_ic_prp":0[1]' > media-ctl -l '"ipu2_ic_prp":2 -> "ipu2_ic_prpvf":0[1]' > media-ctl -l '"ipu2_ic_prpvf":1 -> "ipu2_ic_prpvf capture":0[1]' > # Configure pads > media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480]" > media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480 field:interlaced]" > media-ctl -V "'ipu2_csi1':1 [fmt:UYVY2X8/720x480 field:interlaced]" > media-ctl -V "'ipu2_vdic':2 [fmt:UYVY2X8/720x480 field:none]" > media-ctl -V "'ipu2_ic_prp':2 [fmt:AYUV32/720x480 field:none]" > media-ctl -V "'ipu2_ic_prpvf':1 [fmt:$outputfmt field:none]" > ^^^^ no errors up to this point; streaming can now begin on > 'ipu2_ic_prpvf capture' > > # select input > v4l2-ctl --device /dev/video3 -i0 # 0=AIN1 1=AIN2 2=AIN3 > VIDIOC_S_INPUT: failed: Inappropriate ioctl for device > ^^^^ /sys/class/video4linux/v4l-subdev2/name is 'ipu2_ic_prpvf > capture' - is this not right? Support for setting sensor inputs from the main video capture nodes was long ago removed. Sorry about that, but there were objections to reaching across the media graph to make this happen. Until a VIDIOC_SUBDEV_S_INPUT is added to v4l2, you will just need to send your analog signal to whichever ADV7180 input is active. > > # select any supported YUV or RGB pixelformat on the capture device node > v4l2-ctl --device /dev/video3 > --set-fmt-video=width=720,height=480,pixelformat=UYVY > v4l2-ctl --device /dev/video3 --stream-mmap --stream-to=/x.raw > --stream-count=1 # capture single raw-frame > [ 904.870444] ipu2_ic_prpvf: EOF timeout > VIDIOC_DQBUF: failed: Input/output error > [ 905.910702] ipu2_ic_prpvf: wait last EOF timeout > ^^^^ not getting any frames > > The last patchset of yours I had running on this board was your v3 > patchset - any ideas? Beyond maybe the input selection issue above, not really, your pipeline config looks correct. I have a script for the ADV7180 VDIC -> prpvf pipeline on IPU1 for the SabreAuto, I will send to you separately to see if that helps. > > As it looks like things have settled down with this patchset and it > sounds like it will get merged for 4.13 I'm going to start working on > a driver for the tda1997x HDMI receiver which is also on this board > connected to IPU1_CSI0. Awesome, thanks. Steve
[toc] | [prev] | [standalone]
Page 3 of 3 — ← Prev page 1 2 [3]
Back to top | Article view | linux.kernel
csiph-web