Re: i.MX6: can't capture on MIPI-CSI2 with DS90UB954

[Vladimir Zapolskiy]

Hi Jean-Michel,

On 10/30/2018 06:41 PM, Jean-Michel Hautbois wrote:
> Hi there,
> 
> I am using the i.MX6D from Digi (connect core 6 sbc) with a mailine
> kernel (well, 4.14 right now) and have an issue with mipi-csi2
> capture.
> First I will give brief explanation of my setup, and then I will
> detail the issue.
> I have a camera sensor (OV2732, but could be any other sensor)
> connected on a DS90UB953 FPD-Link III serializer.
> Then a coax cable propagates the signal to a DS90UB954 FPD-Link III
> deserializer.
> 
> The DS90UB954 has the ability to work in a pattern generation mode,
> and I will use it for the rest of the discussion.
> It is an I²C device, and I have written a basic driver (for the moment
> ;)) in order to make it visible on the imx6-mipi-csi2 bus as a camera
> sensor.
> I can give an access to the driver if necessary.

It's sort of indirectly related, anyway, I utterly hope that the
generic IC drivers will be ready and accepted for v4.21, see 
https://lwn.net/ml/devicetree/20181008211205.2900-1...@mleia.com/

Adding more ICs and cell devices to the framework is appreciated, 
in the queue there are DS90UB913, DS90Ux929, DS90Ux947, DS90UB964.

Steve, in case if you're unaware, that's FYI also.

--
Best wishes,
Vladimir

Re: [PATCH v4 2/5] media: dt: bindings: Add binding for NVIDIA Tegra Video Decoder Engine

[Vladimir Zapolskiy]

Hi Dmitry,

On 10/20/2017 12:34 AM, Dmitry Osipenko wrote:
> Add binding documentation for the Video Decoder Engine which is found
> on NVIDIA Tegra20/30/114/124/132 SoC's.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  .../devicetree/bindings/media/nvidia,tegra-vde.txt | 55 
> ++
>  1 file changed, 55 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt 
> b/Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
> new file mode 100644
> index ..470237ed6fe5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
> @@ -0,0 +1,55 @@
> +NVIDIA Tegra Video Decoder Engine
> +
> +Required properties:
> +- compatible : Must contain one of the following values:
> +   - "nvidia,tegra20-vde"
> +   - "nvidia,tegra30-vde"
> +   - "nvidia,tegra114-vde"
> +   - "nvidia,tegra124-vde"
> +   - "nvidia,tegra132-vde"
> +- reg : Must contain an entry for each entry in reg-names.
> +- reg-names : Must include the following entries:
> +  - sxe
> +  - bsev
> +  - mbe
> +  - ppe
> +  - mce
> +  - tfe
> +  - ppb
> +  - vdma
> +  - frameid

I've already mentioned it in my review of the driver code, but the
version from v3 with a single region is more preferable.

Also it implies that "reg-names" property will be removed.

> +- iram : Must contain phandle to the mmio-sram device node that represents
> + IRAM region used by VDE.
> +- interrupts : Must contain an entry for each entry in interrupt-names.
> +- interrupt-names : Must include the following entries:
> +  - sync-token
> +  - bsev
> +  - sxe
> +- clocks : Must include the following entries:
> +  - vde
> +- resets : Must include the following entries:
> +  - vde
> +
> +Example:
> +
> +video-codec@6001a000 {
> + compatible = "nvidia,tegra20-vde";
> + reg = <0x6001a000 0x1000 /* Syntax Engine */
> +0x6001b000 0x1000 /* Video Bitstream Engine */
> +0x6001c000  0x100 /* Macroblock Engine */
> +0x6001c200  0x100 /* Post-processing Engine */
> +0x6001c400  0x100 /* Motion Compensation Engine */
> +0x6001c600  0x100 /* Transform Engine */
> +0x6001c800  0x100 /* Pixel prediction block */
> +0x6001ca00  0x100 /* Video DMA */
> +0x6001d800  0x300 /* Video frame controls */>;
> + reg-names = "sxe", "bsev", "mbe", "ppe", "mce",
> + "tfe", "ppb", "vdma", "frameid";
> + iram = <_pool>; /* IRAM region */
> + interrupts = , /* Sync token interrupt 
> */
> +  , /* BSE-V interrupt */
> +  ; /* SXE interrupt */
> + interrupt-names = "sync-token", "bsev", "sxe";
> + clocks = <_car TEGRA20_CLK_VDE>;
> + resets = <_car 61>;
> +};
> 

--
With best wishes,
Vladimir

Re: [PATCH v4 1/5] ARM: tegra: Add device tree node to describe IRAM

[Vladimir Zapolskiy]

Hi Dmitry,

On 10/20/2017 12:34 AM, Dmitry Osipenko wrote:
> From: Vladimir Zapolskiy <v...@mleia.com>
> 
> All Tegra SoCs contain 256KiB IRAM, which is used to store CPU resume code
> and by hardware engines like a video decoder.
> 
> Signed-off-by: Vladimir Zapolskiy <v...@mleia.com>

Please add also your own closing "Signed-off-by" tag, please reference
to "Developer's Certificate of Origin 1.1", point (c), it is found in
Documentation/process/submitting-patches.rst

> ---
>  arch/arm/boot/dts/tegra114.dtsi | 8 
>  arch/arm/boot/dts/tegra124.dtsi | 8 
>  arch/arm/boot/dts/tegra20.dtsi  | 8 
>  arch/arm/boot/dts/tegra30.dtsi  | 8 

My assumption is that Thierry would prefer to get 4 separate patches,
one for each platform, please split the patch.

Also thanks for your time and your efforts applied to push my occasional
change, please feel free to take your own authorship for 3 out of 4 patches.

>  4 files changed, 32 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> index 8932ea3afd5f..13f6087790c8 100644
> --- a/arch/arm/boot/dts/tegra114.dtsi
> +++ b/arch/arm/boot/dts/tegra114.dtsi
> @@ -10,6 +10,14 @@
>   compatible = "nvidia,tegra114";
>   interrupt-parent = <>;
>  
> + iram@4000 {
> + compatible = "mmio-sram";

Unfortunately Thierry hasn't yet replied, but my assumption is that
the list of compatibles should be extended with one more SoC specific
value like

compatible = "nvidia,tegra114-sysram", "mmio-sram";

I'm not sure, if Tegra maintainers want to see a new compatible
described in Documentation/devicetree/bindings.

> + reg = <0x4000 0x4>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x4000 0x4>;
> + };
> +
>   host1x@5000 {
>   compatible = "nvidia,tegra114-host1x", "simple-bus";
>   reg = <0x5000 0x00028000>;
> diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
> index 8baf00b89efb..a3585ed82646 100644
> --- a/arch/arm/boot/dts/tegra124.dtsi
> +++ b/arch/arm/boot/dts/tegra124.dtsi

The considerations from above are applicable to the rest of
the touched platforms.

--
With best wishes,
Vladimir

Re: [PATCH v4 3/5] staging: Introduce NVIDIA Tegra video decoder driver

[Vladimir Zapolskiy]

Hi Dmitry,

I'll add just a couple of minor comments, in general the code looks
very good.

On 10/20/2017 12:34 AM, Dmitry Osipenko wrote:
> NVIDIA Tegra20/30/114/124/132 SoC's have video decoder engine that
> supports standard set of video formats like H.264 / MPEG-4 / WMV / VC1.
> Currently implemented decoding of CAVLC H.264 on Tegra20 only.
> 
> Signed-off-by: Dmitry Osipenko 

[snip]

> +++ b/drivers/staging/tegra-vde/uapi.h
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright (C) 2016-2017 Dmitry Osipenko 
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.

>From the specified MODULE_LICENSE("GPL") I'd rather expect to see a reference
to GPLv2+ license in the header, and here the text resembles MIT license only.

I understand that it is a UAPI header file and it may happen that different
rules are applied to this kind of sources, hopefully Greg can give the right
directions.

In general you may avoid the headache with the custom UAPI, if you reuse
V4L2 interfaces, if I remember correctly drivers/media/platform/coda does it.
Also from my point of view the custom UAPI is the only reason why the driver
is pushed to the staging folder.

[snip]

> +struct tegra_vde {
> + void __iomem *sxe;
> + void __iomem *bsev;
> + void __iomem *mbe;
> + void __iomem *ppe;
> + void __iomem *mce;
> + void __iomem *tfe;
> + void __iomem *ppb;
> + void __iomem *vdma;
> + void __iomem *frameid;

Please find a comment in tegra_vde_probe() function regarding
devm_ioremap_resource() calls.

> + struct mutex lock;
> + struct miscdevice miscdev;
> + struct reset_control *rst;
> + struct gen_pool *iram_pool;
> + struct completion decode_completion;
> + struct clk *clk;
> + dma_addr_t iram_lists_addr;
> + u32 *iram;
> +};

[snip]

> +static int tegra_vde_wait_bsev(struct tegra_vde *vde, bool wait_dma)
> +{
> + struct device *dev = vde->miscdev.parent;
> + u32 value;
> + int err;
> +
> + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value,
> +  !(value & BIT(2)), 1, 100);
> + if (err) {
> + dev_err(dev, "BSEV unknown bit timeout\n");
> + return err;
> + }
> +
> + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value,
> +  (value & BSE_ICMDQUE_EMPTY), 1, 100);
> + if (err) {
> + dev_err(dev, "BSEV ICMDQUE flush timeout\n");
> + return err;
> + }
> +
> + if (!wait_dma)
> + return 0;
> +
> + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value,
> +  !(value & BSE_DMA_BUSY), 1, 100);
> + if (err) {
> + dev_err(dev, "BSEV DMA timeout\n");
> + return err;
> + }
> +
> + return 0;

if (err)
dev_err(dev, "BSEV DMA timeout\n");

return err;

is two lines shorter.

> +}
> +

[snip]

> +static int tegra_vde_attach_dmabufs_to_frame(struct device *dev,
> + struct video_frame *frame,
> + struct tegra_vde_h264_frame *source,
> + enum dma_data_direction dma_dir,
> + bool baseline_profile,
> + size_t csize)
> +{
> + int err;
> +
> + err = tegra_vde_attach_dmabuf(dev, source->y_fd,
> +   source->y_offset, csize * 4,
> +   >y_dmabuf_attachment,
> +   >y_addr,
> +   >y_sgt,
> +   NULL, dma_dir);
> + if (err)
> + return err;
> +
> + err = tegra_vde_attach_dmabuf(dev, 

Re: [PATCH v3 2/2] ARM: dts: tegra20: Add video decoder node

[Vladimir Zapolskiy]

Hello Dmitry,

On 10/11/2017 11:08 PM, Dmitry Osipenko wrote:
> Add a device node for the video decoder engine found on Tegra20.
> 
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> ---
>  arch/arm/boot/dts/tegra20.dtsi | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 7c85f97f72ea..1b5d54b6c0cb 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -249,6 +249,23 @@
>   */
>   };
>  
> + vde@6001a000 {
> + compatible = "nvidia,tegra20-vde";
> + reg = <0x6001a000 0x3D00/* VDE registers */
> +0x4400 0x3FC00>; /* IRAM region */

this notation of a used region in IRAM is non-standard and potentially it
may lead to conflicts for IRAM resource between users.

My proposal is to add a valid device tree node to describe an IRAM region
firstly, then reserve a subregion in it by using a new "iram" property.

8<
From: Vladimir Zapolskiy <v...@mleia.com>
Date: Thu, 12 Oct 2017 10:25:45 +0300
Subject: [PATCH] ARM: tegra: add device tree node to describe IRAM on Tegra20

All Tegra20 SoCs contain 256KiB IRAM, which is used to store
resume code and by a video decoder engine.

Signed-off-by: Vladimir Zapolskiy <v...@mleia.com>
---
 arch/arm/boot/dts/tegra20.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 7c85f97f72ea..fd2843c90920 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -9,6 +9,14 @@
compatible = "nvidia,tegra20";
interrupt-parent = <>;
 
+   iram@4000 {
+   compatible = "mmio-sram";
+   reg = <0x4000 0x4>;
+   #address-cells = <1>;
+   #size-cells = <1>;
+   ranges = <0 0x4000 0x4>;
+   };
+
host1x@5000 {
compatible = "nvidia,tegra20-host1x", "simple-bus";
reg = <0x5000 0x00024000>;
8<

Please add the change above to your next version of the series, or
if you wish I can send it separately for review by Thierry.

After applying that change you do define a region in IRAM for the exclusive
usage by a video decoder engine and add an 'iram' property:

8<
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index fd2843c90920..5133fbac2185 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -15,6 +15,11 @@
#address-cells = <1>;
#size-cells = <1>;
ranges = <0 0x4000 0x4>;
+
+   vde_pool: vde {
+   reg = <0x400 0x3fc00>;
+   pool;
+   };
};
 
host1x@5000 {
[snip]

+   vde@6001a000 {
+   compatible = "nvidia,tegra20-vde";
+   reg = <0x6001a000 0x3d00>;  /* VDE registers */
+   iram = <_pool>; /* IRAM region */
[snip]
8<

And finally in the driver you'll use genalloc API to access the IRAM
region, for that you can find ready examples in the kernel source code.

--
With best wishes,
Vladimir

Re: [PATCH v8 00/34] i.MX Media Driver

[Vladimir Zapolskiy]

On 06/10/2017 02:26 AM, Hans Verkuil wrote:
> On 10/06/17 01:16, Steve Longerbeam wrote:
>>
>>
>> On 06/07/2017 12:02 PM, Hans Verkuil wrote:
>>> We're still waiting for an Ack for patch 02/34, right?
>>>
>>
>> Hi Hans, Rub has provided an Ack for patch 2.
>>
>>> Other than that everything is ready AFAICT.
>>>
>>
>> But as Pavel pointed out, in fact we are missing many
>> Acks still, for all of the dts source changes (patches
>> 4-14), as well as really everything else (imx-media staging
>> driver patches).
> 
> No Acks needed for the staging part. It's staging, so not held
> to the same standards as non-staging parts. That doesn't mean
> Acks aren't welcome, of course.

Acks are wanted for particular i.MX DTS changes including device
tree binding descriptions.

Shawn, please bless the series.

> 
> You don't need Greg's Ack for staging/media either, patches there
> go in via us (generally at least) and we handle those, not Greg.
> 

--
With best wishes,
Vladimir

Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Vladimir Zapolskiy]

Hi Steve,

On 03/28/2017 03:40 AM, Steve Longerbeam wrote:
> From: Philipp Zabel 
> 
> This driver can handle SoC internal and external video bus multiplexers,
> controlled either by register bit fields or by a GPIO. The subdevice
> passes through frame interval and mbus configuration of the active input
> to the output side.
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Philipp Zabel 
> 
> - fixed a cut error in vidsw_remove(): v4l2_async_register_subdev()
>   should be unregister.
> 
> - added media_entity_cleanup() to vidsw_remove().
> 
> - added missing MODULE_DEVICE_TABLE().
>   Suggested-by: Javier Martinez Canillas 
> 
> - there was a line left over from a previous iteration that negated
>   the new way of determining the pad count just before it which
>   has been removed (num_pads = of_get_child_count(np)).
> 
> - removed [gs]_frame_interval ops. timeperframe is not used anywhwere
>   in this subdev, and currently it has no control over frame rate.
> 
> - add link_validate to media_entity_operations.
> 
> - moved devicetree binding doc to a separate commit.
> 
> - Philipp Zabel has developed a set of patches that allow adding
>   to the subdev async notifier waiting list using a chaining method
>   from the async registered callbacks (v4l2_of_subdev_registered()
>   and the prep patches for that). For now, I've removed the use of
>   v4l2_of_subdev_registered() for the vidmux driver's registered
>   callback. This doesn't affect the functionality of this driver,
>   but allows for it to be merged now, before adding the chaining
>   support.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  drivers/media/platform/Kconfig |   8 +
>  drivers/media/platform/Makefile|   2 +
>  drivers/media/platform/video-multiplexer.c | 451 
> +
>  3 files changed, 461 insertions(+)
>  create mode 100644 drivers/media/platform/video-multiplexer.c
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index ab0bb48..c9b8d9c 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -74,6 +74,14 @@ config VIDEO_M32R_AR_M64278
> To compile this driver as a module, choose M here: the
> module will be called arv.
>  
> +config VIDEO_MULTIPLEXER
> + tristate "Video Multiplexer"
> + depends on VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER

+ depends on OF

> + help
> +   This driver provides support for SoC internal N:1 video bus
> +   multiplexers controlled by register bitfields as well as external
> +   2:1 video multiplexers controlled by a single GPIO.
> +

[snip]

> +static int vidsw_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct of_endpoint endpoint;
> + struct device_node *ep;
> + struct reg_field field;
> + struct vidsw *vidsw;
> + struct regmap *map;
> + unsigned int num_pads;
> + int ret;
> +
> + vidsw = devm_kzalloc(>dev, sizeof(*vidsw), GFP_KERNEL);
> + if (!vidsw)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, vidsw);
> +
> + v4l2_subdev_init(>subdev, _subdev_ops);
> + snprintf(vidsw->subdev.name, sizeof(vidsw->subdev.name), "%s",
> + np->name);

 or oops here   ^.

> + vidsw->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + vidsw->subdev.dev = >dev;
> +
> + /*
> +  * The largest numbered port is the output port. It determines
> +  * total number of pads
> +  */
> + num_pads = 0;
> + for_each_endpoint_of_node(np, ep) {
> + of_graph_parse_endpoint(ep, );
> + num_pads = max(num_pads, endpoint.port + 1);
> + }
> +
> + if (num_pads < 2) {
> + dev_err(>dev, "Not enough ports %d\n", num_pads);
> + return -EINVAL;
> + }

This unveils another runtime dependency on OF.

> +
> + ret = of_get_reg_field(np, );
> + if (ret == 0) {
> + map = syscon_node_to_regmap(np->parent);
> + if (!map) {
> + dev_err(>dev, "Failed to get syscon register 
> map\n");
> + return PTR_ERR(map);
> + }
> +
> + vidsw->field = devm_regmap_field_alloc(>dev, map, field);
> + if (IS_ERR(vidsw->field)) {
> + dev_err(>dev, "Failed to allocate regmap 
> field\n");
> + return PTR_ERR(vidsw->field);
> + }
> +
> + regmap_field_read(vidsw->field, >active);
> + } else {
> + if (num_pads > 3) {
> + dev_err(>dev, "Too many ports %d\n", num_pads);
> + return -EINVAL;
> + }
> +
> + vidsw->gpio = devm_gpiod_get(>dev, NULL, GPIOD_OUT_LOW);
> + if (IS_ERR(vidsw->gpio)) {
> +  

Re: [PATCH v10 1/2] Documentation: DT: Add OV5647 bindings

[Vladimir Zapolskiy]

Hi Ramiro,

On 03/06/2017 01:16 PM, Ramiro Oliveira wrote:
> Create device tree bindings documentation.
> 
> Signed-off-by: Ramiro Oliveira <roliv...@synopsys.com>

The device tree binding description looks perfect from my perspective.

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

--
With best wishes,
Vladimir

Re: [PATCH v10 2/2] media: i2c: Add support for OV5647 sensor.

[Vladimir Zapolskiy]

Hi Ramiro,

On 03/06/2017 01:16 PM, Ramiro Oliveira wrote:
> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
> and RAW 10 output formats, and MIPI CSI-2 interface.
> 
> The driver adds support for 640x480 RAW 8.
> 
> Signed-off-by: Ramiro Oliveira <roliv...@synopsys.com>

All updates are fine, thank you. Feel free to add my

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

> ---
>  MAINTAINERS|   7 +
>  drivers/media/i2c/Kconfig  |  11 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/ov5647.c | 636 
> +

I see this version has 100 LoC less in comparison to v8, good result.

[snip]

> +
> +static const struct v4l2_subdev_pad_ops ov5647_subdev_pad_ops = {
> + .enum_mbus_code =   ov5647_enum_mbus_code,

Nitpicking, above it's better to swap tab and space symbols around '='.

> +};
> +
> +static const struct v4l2_subdev_ops ov5647_subdev_ops = {
> + .core   = _subdev_core_ops,
> + .video  = _subdev_video_ops,
> + .pad= _subdev_pad_ops,
> +};
> +

--
With best wishes,
Vladimir

Re: [PATCH v5 00/39] i.MX Media Driver

[Vladimir Zapolskiy]

On 03/19/2017 04:22 PM, Russell King - ARM Linux wrote:
> On Sun, Mar 19, 2017 at 02:21:10PM +, Russell King - ARM Linux wrote:
>> There's a good reason why I dumped a full debug log using GST_DEBUG=*:9,
>> analysed it for the cause of the failure, and tried several different
>> pipelines, including the standard bayer2rgb plugin.
>>
>> Please don't blame this on random stuff after analysis of the logs _and_
>> reading the appropriate plugin code has shown where the problem is.  I
>> know gstreamer can be very complex, but it's very possible to analyse
>> the cause of problems and pin them down with detailed logs in conjunction
>> with the source code.
> 
> Oh, and the proof of correct analysis is that fixing the kernel capture
> driver to enumerate the frame sizes and intervals fixes the issue, even
> with bayer2rgbneon being used.
> 
> Therefore, there is _no way_ what so ever that it could be caused by that
> plugin.
> 

Hey, no blaming of the unknown to me bayer2rgbneon element from my side,
I've just asked an innocent question, thanks for reply. I failed to find
the source code of the plugin, I was interested to compare its performance
and features with mine in-house NEON powered RGGB/BGGR to RGB24 GStreamer
conversion element, which is written years ago. My question was offtopic.

--
With best wishes,
Vladimir

Re: [PATCH v5 00/39] i.MX Media Driver

[Vladimir Zapolskiy]

Hi Russell,

On 03/18/2017 10:43 PM, Russell King - ARM Linux wrote:
> On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
>> Can you share your gstreamer pipeline? For now, until
>> VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
>> does not attempt to specify a frame rate. I use the attached
>> script for testing, which works for me.
> 
> It's nothing more than
> 
>   gst-launch-1.0 -v v4l2src !  ! xvimagesink
> 
> in my case, the conversions are bayer2rgbneon.  However, this only shows
> you the frame rate negotiated on the pads (which is actually good enough
> to show the issue.)

I'm sorry for potential offtopic, but is bayer2rgbneon element found in
any officially supported by GStreamer plugin? Can it be a point of
failure?

--
With best wishes,
Vladimir

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

[Vladimir Zapolskiy]

On 02/22/2017 12:51 PM, Ramiro Oliveira wrote:
> Hi Zakari,
> 
> On 2/21/2017 8:36 PM, Vladimir Zapolskiy wrote:
>> Hi Ramiro,
>>
>> On 02/21/2017 06:42 PM, Ramiro Oliveira wrote:
>>> Hi Vladimir,
>>>
>>> Thank you for your feedback
>>>
>>> On 2/21/2017 3:54 PM, Vladimir Zapolskiy wrote:
>>>> Hi Ramiro,
>>>>
>>>> please find some review comments below.
>>>>
>>>> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
>>>>> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
>>>>> and RAW 10 output formats, and MIPI CSI-2 interface.
>>>>>
>>>>> The driver adds support for 640x480 RAW 8.
>>>>>
>>>>> Signed-off-by: Ramiro Oliveira <roliv...@synopsys.com>
>>>>> ---
>>>>
>>>> [snip]
>>>>
>>>>> +
>>>>> +struct ov5647 {
>>>>> + struct v4l2_subdev  sd;
>>>>> + struct media_padpad;
>>>>> + struct mutexlock;
>>>>> + struct v4l2_mbus_framefmt   format;
>>>>> + unsigned intwidth;
>>>>> + unsigned intheight;
>>>>> + int power_count;
>>>>> + struct clk  *xclk;
>>>>> + /* External clock frequency currently supported is 30MHz */
>>>>> + u32 xclk_freq;
>>>>
>>>> See a comment about 25MHz vs 30MHz below.
>>>>
>>>> Also I assume you can remove 'xclk_freq' from the struct fields,
>>>> it can be replaced by a local variable.
>>>>
>>>
>>> I'll do that.
>>>
>>>>> +};
>>>>
>>>> [snip]
>>>>
>>>>> +
>>>>> +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
>>>>> +{
>>>>> + int ret;
>>>>> + unsigned char data_w[2] = { reg >> 8, reg & 0xff };
>>>>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>>> +
>>>>> + ret = i2c_master_send(client, data_w, 2);
>>>>> + if (ret < 0) {
>>>>> + dev_dbg(>dev, "%s: i2c read error, reg: %x\n",
>>>>
>>>> s/i2c read error/i2c write error/
>>>>
>>>
>>> I'm not sure I understand what you mean.
>>
>> That's a sed expression for string substitution. Here you do 
>> i2c_master_send()
>> but dev_dbg() comment says "i2c read error". It's a simple copy-paste typo 
>> to fix.
>>
> 
> Ohh... now I see. I'll change it.
> 
>>>>> + __func__, reg);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>
>> [snip]
>>
>>>>> +
>>>>> +static int sensor_power(struct v4l2_subdev *sd, int on)
>>
>> On the caller's side (functions ov5647_open() and ov5647_close()) the second
>> argument of the function is of 'bool' type, however .s_power callback from
>> struct v4l2_subdev_core_ops (see include/media/v4l2-subdev.h) defines it as
>> 'int'.
>>
>> It's just a nitpicking, please feel free to ignore the comment above or
>> please consider to change the arguments on callers' side to integers.
>>
>> Also you may consider to add 'ov5647_' prefix to the function name to
>> distinguish it from a potentially added in future sensor_power() function,
>> the original name sounds too generic.
>>
> 
> OK. I'll add the prefix and change the variable type from int to bool.
> 

Just to eliminate any potential misunderstanding, if you consider to reuse
the current sensor_power() function, please change variables from bool to int
on a caller's side, the signature of the function shall not be changed to
match .s_power type.

>>>>> +{
>>>>> + int ret;
>>>>> + struct ov5647 *ov5647 = to_state(sd);
>>>>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>>> +
>>>>> + ret = 0;
>>>>> + mutex_lock(>lock);
>>>>> +
>>>>> + if (on && !ov5647->power_count) {
>>>>> + dev_dbg(>dev, "OV5647 power on\n");
>>>>> +
>>>>> + clk_set_rate(ov5647->xclk, ov5647->xclk_freq);
>>>>
>>>> Now clk_set_rate() is redundan

Re: [PATCH v9 1/2] Add OV5647 device tree documentation

[Vladimir Zapolskiy]

Hi Ramiro,

On 02/22/2017 12:57 PM, Ramiro Oliveira wrote:
> Hi Vladimir
> 
> On 2/21/2017 10:37 PM, Vladimir Zapolskiy wrote:
>> Hi Sakari,
>>
>> On 02/21/2017 11:48 PM, Sakari Ailus wrote:
>>> Hi, Vladimir!
>>>
>>> How do you do? :-)
>>
>> deferring execution of boring tasks by doing code review :)
>>
>>> On Tue, Feb 21, 2017 at 10:48:09PM +0200, Vladimir Zapolskiy wrote:
>>>> Hi Ramiro,
>>>>
>>>> On 02/21/2017 10:13 PM, Ramiro Oliveira wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> Thank you for your feedback
>>>>>
>>>>> On 2/21/2017 3:58 PM, Vladimir Zapolskiy wrote:
>>>>>> Hi Ramiro,
>>>>>>
>>>>>> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
>>>>>>> Create device tree bindings documentation.
>>>>>>>
>>>>>>> Signed-off-by: Ramiro Oliveira <roliv...@synopsys.com>
>>>>>>> Acked-by: Rob Herring <r...@kernel.org>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/media/i2c/ov5647.txt   | 35 
>>>>>>> ++
>>>>>>>  1 file changed, 35 insertions(+)
>>>>>>>  create mode 100644 
>>>>>>> Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt 
>>>>>>> b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>>>>> new file mode 100644
>>>>>>> index ..31956426d3b9
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>>>>> @@ -0,0 +1,35 @@
>>>>>>> +Omnivision OV5647 raw image sensor
>>>>>>> +-
>>>>>>> +
>>>>>>> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data 
>>>>>>> interfaces
>>>>>>> +and CCI (I2C compatible) control bus.
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +
>>>>>>> +- compatible   : "ovti,ov5647".
>>>>>>> +- reg  : I2C slave address of the sensor.
>>>>>>> +- clocks   : Reference to the xclk clock.
>>>>>>
>>>>>> Is "xclk" clock a pixel clock or something else?
>>>>>>
>>>>>
>>>>> It's an external oscillator.
>>>>
>>>> hmm, I suppose a clock of any type could serve as a clock for the sensor.
>>>> It can be an external oscillator on a particular board, or it can be
>>>> something else on another board.
>>>
>>> Any clock source could be used I presume.
>>>
>>
>> That's exactly my point, and it is a reason to rename "xclk" to something
>> more generic.
>>
> 
> xclk it's the name being used in the camera datasheet, but I can change it to
> something more generic
> 

Ah, if the name comes from the sensor datasheet, then it should be okay
to keep it.

>>>>
>>>> Can you please describe what for does ov5647 sensor need this clock, what
>>>> is its function?
>>>
>>> Camera modules (sensors) quite commonly require an external clock as they do
>>> not have an oscillator on their own. A lot of devices under
>>> Documentation/devicetree/bindings/media/i2c/ have similar properties.
>>>
>>
>> So, what should be a better replacement of "xclk" in the description above?
>>
>> E.g.
>>
>> - clocks : Phandle to a device supply clock.
>>
>>>>
>>>>>
>>>>>>> +- clock-names  : Should be "xclk".
>>
>> We got an agreement that "clock-names" property is removed, nevertheless
>> if it is added back, is should not be "xclk".
>>
>>>>>>
>>>>>> You can remove this property, because there is only one source clock.
>>>>>>
>>>>>
>>>>> Ok.
>>>>>
>>>>>>> +- clock-frequency  : Frequency of the xclk clock.
>>>>>>
>>>>>> And after the last updates in the driver this property can be removed as 
>>>>>&g

Re: [PATCH v9 1/2] Add OV5647 device tree documentation

[Vladimir Zapolskiy]

Hi Sakari,

On 02/21/2017 11:48 PM, Sakari Ailus wrote:
> Hi, Vladimir!
> 
> How do you do? :-)

deferring execution of boring tasks by doing code review :)

> On Tue, Feb 21, 2017 at 10:48:09PM +0200, Vladimir Zapolskiy wrote:
>> Hi Ramiro,
>>
>> On 02/21/2017 10:13 PM, Ramiro Oliveira wrote:
>>> Hi Vladimir,
>>>
>>> Thank you for your feedback
>>>
>>> On 2/21/2017 3:58 PM, Vladimir Zapolskiy wrote:
>>>> Hi Ramiro,
>>>>
>>>> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
>>>>> Create device tree bindings documentation.
>>>>>
>>>>> Signed-off-by: Ramiro Oliveira <roliv...@synopsys.com>
>>>>> Acked-by: Rob Herring <r...@kernel.org>
>>>>> ---
>>>>>  .../devicetree/bindings/media/i2c/ov5647.txt   | 35 
>>>>> ++
>>>>>  1 file changed, 35 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt 
>>>>> b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>>> new file mode 100644
>>>>> index ..31956426d3b9
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>>> @@ -0,0 +1,35 @@
>>>>> +Omnivision OV5647 raw image sensor
>>>>> +-
>>>>> +
>>>>> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data 
>>>>> interfaces
>>>>> +and CCI (I2C compatible) control bus.
>>>>> +
>>>>> +Required properties:
>>>>> +
>>>>> +- compatible : "ovti,ov5647".
>>>>> +- reg: I2C slave address of the sensor.
>>>>> +- clocks : Reference to the xclk clock.
>>>>
>>>> Is "xclk" clock a pixel clock or something else?
>>>>
>>>
>>> It's an external oscillator.
>>
>> hmm, I suppose a clock of any type could serve as a clock for the sensor.
>> It can be an external oscillator on a particular board, or it can be
>> something else on another board.
> 
> Any clock source could be used I presume.
> 

That's exactly my point, and it is a reason to rename "xclk" to something
more generic.

>>
>> Can you please describe what for does ov5647 sensor need this clock, what
>> is its function?
> 
> Camera modules (sensors) quite commonly require an external clock as they do
> not have an oscillator on their own. A lot of devices under
> Documentation/devicetree/bindings/media/i2c/ have similar properties.
> 

So, what should be a better replacement of "xclk" in the description above?

E.g.

- clocks: Phandle to a device supply clock.

>>
>>>
>>>>> +- clock-names: Should be "xclk".

We got an agreement that "clock-names" property is removed, nevertheless
if it is added back, is should not be "xclk".

>>>>
>>>> You can remove this property, because there is only one source clock.
>>>>
>>>
>>> Ok.
>>>
>>>>> +- clock-frequency: Frequency of the xclk clock.
>>>>
>>>> And after the last updates in the driver this property can be removed as 
>>>> well.
>>>>
>>>
>>> But I'm still using clk_get_rate in the driver, if I remove the frequency 
>>> here
>>> the probing will fail.
>>>
>>
>> I doubt it, there should be no connection between a custom "clock-frequency"
>> device tree property in a clock consumer device node and clk_get_rate() 
>> function
>> from the CCF, which takes a clock provider as its argument.
> 
> The purpose is to make sure the clock frequency is really usable for the
> device, in this particular case the driver can work with one particular
> frequency.
> 
> That said, the driver does not appear to use the property at the moment. It
> should.
> 
> It'd be good to verify that the rate matches: clk_set_rate() is not
> guaranteed to produce the requested clock rate, and the driver could
> conceivably be updated with support for more frequencies. There are
> typically a few frequencies that a SoC such a sensor is connected can
> support, and 25 MHz is not one of the common frequencies. With this
> property, the frequency would be always there explicitly.
> 

I can provide my arguments given at v8 review time again, since I don't
see a contradiction with my older comments.

Briefly "clock-frequency" as a device tree property on a consumer side
can be considered as redundant, because there is a mechanism to specify
a wanted clock frequency on a clock provider side right in a board DTB.

So, the clock frequency set up is delegated to CCF, and when any other
than 25 MHz frequencies are got supported, that's only the matter of
driver updates, DTBs won't be touched.

--
With best wishes,
Vladimir

Re: [PATCH v9 1/2] Add OV5647 device tree documentation

[Vladimir Zapolskiy]

Hi Ramiro,

On 02/21/2017 10:13 PM, Ramiro Oliveira wrote:
> Hi Vladimir,
> 
> Thank you for your feedback
> 
> On 2/21/2017 3:58 PM, Vladimir Zapolskiy wrote:
>> Hi Ramiro,
>>
>> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
>>> Create device tree bindings documentation.
>>>
>>> Signed-off-by: Ramiro Oliveira <roliv...@synopsys.com>
>>> Acked-by: Rob Herring <r...@kernel.org>
>>> ---
>>>  .../devicetree/bindings/media/i2c/ov5647.txt   | 35 
>>> ++
>>>  1 file changed, 35 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt 
>>> b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>> new file mode 100644
>>> index ..31956426d3b9
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>> @@ -0,0 +1,35 @@
>>> +Omnivision OV5647 raw image sensor
>>> +-
>>> +
>>> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
>>> +and CCI (I2C compatible) control bus.
>>> +
>>> +Required properties:
>>> +
>>> +- compatible   : "ovti,ov5647".
>>> +- reg  : I2C slave address of the sensor.
>>> +- clocks   : Reference to the xclk clock.
>>
>> Is "xclk" clock a pixel clock or something else?
>>
> 
> It's an external oscillator.

hmm, I suppose a clock of any type could serve as a clock for the sensor.
It can be an external oscillator on a particular board, or it can be
something else on another board.

Can you please describe what for does ov5647 sensor need this clock, what
is its function?

> 
>>> +- clock-names  : Should be "xclk".
>>
>> You can remove this property, because there is only one source clock.
>>
> 
> Ok.
> 
>>> +- clock-frequency  : Frequency of the xclk clock.
>>
>> And after the last updates in the driver this property can be removed as 
>> well.
>>
> 
> But I'm still using clk_get_rate in the driver, if I remove the frequency here
> the probing will fail.
> 

I doubt it, there should be no connection between a custom "clock-frequency"
device tree property in a clock consumer device node and clk_get_rate() function
from the CCF, which takes a clock provider as its argument.

>>> +
>>> +The common video interfaces bindings (see video-interfaces.txt) should be
>>> +used to specify link to the image data receiver. The OV5647 device
>>> +node should contain one 'port' child node with an 'endpoint' subnode.
>>> +
>>> +Example:
>>> +
>>> +   i2c@2000 {
>>> +   ...
>>> +   ov: camera@36 {
>>> +   compatible = "ovti,ov5647";
>>> +   reg = <0x36>;
>>> +   clocks = <_clk>;
>>> +   clock-names = "xclk";
>>> +   clock-frequency = <2500>;
>>
>> When you remove two unused properties, please don't forget to update the
>> example.
>>
> 
> Ok.
> 
>>> +   port {
>>> +   camera_1: endpoint {
>>> +   remote-endpoint = <_ep1>;
>>> +   };
>>> +   };
>>> +   };
>>> +   };
>>>
>>

--
With best wishes,
Vladimir

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

[Vladimir Zapolskiy]

Hi Ramiro,

On 02/21/2017 06:42 PM, Ramiro Oliveira wrote:
> Hi Vladimir,
> 
> Thank you for your feedback
> 
> On 2/21/2017 3:54 PM, Vladimir Zapolskiy wrote:
>> Hi Ramiro,
>>
>> please find some review comments below.
>>
>> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
>>> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
>>> and RAW 10 output formats, and MIPI CSI-2 interface.
>>>
>>> The driver adds support for 640x480 RAW 8.
>>>
>>> Signed-off-by: Ramiro Oliveira <roliv...@synopsys.com>
>>> ---
>>
>> [snip]
>>
>>> +
>>> +struct ov5647 {
>>> +   struct v4l2_subdev  sd;
>>> +   struct media_padpad;
>>> +   struct mutexlock;
>>> +   struct v4l2_mbus_framefmt   format;
>>> +   unsigned intwidth;
>>> +   unsigned intheight;
>>> +   int power_count;
>>> +   struct clk  *xclk;
>>> +   /* External clock frequency currently supported is 30MHz */
>>> +   u32 xclk_freq;
>>
>> See a comment about 25MHz vs 30MHz below.
>>
>> Also I assume you can remove 'xclk_freq' from the struct fields,
>> it can be replaced by a local variable.
>>
> 
> I'll do that.
> 
>>> +};
>>
>> [snip]
>>
>>> +
>>> +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
>>> +{
>>> +   int ret;
>>> +   unsigned char data_w[2] = { reg >> 8, reg & 0xff };
>>> +   struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> +
>>> +   ret = i2c_master_send(client, data_w, 2);
>>> +   if (ret < 0) {
>>> +   dev_dbg(>dev, "%s: i2c read error, reg: %x\n",
>>
>> s/i2c read error/i2c write error/
>>
> 
> I'm not sure I understand what you mean.

That's a sed expression for string substitution. Here you do i2c_master_send()
but dev_dbg() comment says "i2c read error". It's a simple copy-paste typo to 
fix.

>>> +   __func__, reg);
>>> +   return ret;
>>> +   }
>>> +

[snip]

>>> +
>>> +static int sensor_power(struct v4l2_subdev *sd, int on)

On the caller's side (functions ov5647_open() and ov5647_close()) the second
argument of the function is of 'bool' type, however .s_power callback from
struct v4l2_subdev_core_ops (see include/media/v4l2-subdev.h) defines it as
'int'.

It's just a nitpicking, please feel free to ignore the comment above or
please consider to change the arguments on callers' side to integers.

Also you may consider to add 'ov5647_' prefix to the function name to
distinguish it from a potentially added in future sensor_power() function,
the original name sounds too generic.

>>> +{
>>> +   int ret;
>>> +   struct ov5647 *ov5647 = to_state(sd);
>>> +   struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> +
>>> +   ret = 0;
>>> +   mutex_lock(>lock);
>>> +
>>> +   if (on && !ov5647->power_count) {
>>> +   dev_dbg(>dev, "OV5647 power on\n");
>>> +
>>> +   clk_set_rate(ov5647->xclk, ov5647->xclk_freq);
>>
>> Now clk_set_rate() is redundant, please remove it.
>>
>> If once it is needed again, please move it to the .probe function, so
>> it is called only once in the runtime.
>>
> 
> Ok. I'll remove it for now.
> 
>>> +
>>> +   ret = clk_prepare_enable(ov5647->xclk);
>>
>> I wonder would it be possible to unload the driver or to unbind the device
>> and leave the clock unintentionally enabled? If yes, then this is a bug.
>>
> 
> You're saying that if the driver was unloaded and the clock was left enabled
> when the driver was loaded again this line would cause an error?

Not exactly, here I saw a potential resource leak, namely a potentially left
prepared/enabled clock.

> 
> Should I disable the clock when the driver is removed?
> 

The driver (and framework) shall guarantee that when it is detached from
device(s) (e.g. by unloading "ov5647" kernel module or unbinding ov5647 device),
all acquired resources are released.

But in this particular case most probably I've been overly alert, I believe
that V4L2 framework correcly handles device power states, so please ignore my
comment.

To add something valuable to the review, could you please confirm that
ov5647_subdev_internal_ops data is in use by the driver?

E.g. shouldn't it be registered by

  sd->internal_ops = _subdev_internal_ops;

before calling v4l2_async_register_subdev(sd) ?

--
With best wishes,
Vladimir

Re: [PATCH v9 1/2] Add OV5647 device tree documentation

[Vladimir Zapolskiy]

Hi Ramiro,

On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
> Create device tree bindings documentation.
> 
> Signed-off-by: Ramiro Oliveira 
> Acked-by: Rob Herring 
> ---
>  .../devicetree/bindings/media/i2c/ov5647.txt   | 35 
> ++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt 
> b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
> new file mode 100644
> index ..31956426d3b9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
> @@ -0,0 +1,35 @@
> +Omnivision OV5647 raw image sensor
> +-
> +
> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
> +and CCI (I2C compatible) control bus.
> +
> +Required properties:
> +
> +- compatible : "ovti,ov5647".
> +- reg: I2C slave address of the sensor.
> +- clocks : Reference to the xclk clock.

Is "xclk" clock a pixel clock or something else?

> +- clock-names: Should be "xclk".

You can remove this property, because there is only one source clock.

> +- clock-frequency: Frequency of the xclk clock.

And after the last updates in the driver this property can be removed as well.

> +
> +The common video interfaces bindings (see video-interfaces.txt) should be
> +used to specify link to the image data receiver. The OV5647 device
> +node should contain one 'port' child node with an 'endpoint' subnode.
> +
> +Example:
> +
> + i2c@2000 {
> + ...
> + ov: camera@36 {
> + compatible = "ovti,ov5647";
> + reg = <0x36>;
> + clocks = <_clk>;
> + clock-names = "xclk";
> + clock-frequency = <2500>;

When you remove two unused properties, please don't forget to update the
example.

> + port {
> + camera_1: endpoint {
> + remote-endpoint = <_ep1>;
> + };
> + };
> + };
> + };
> 

--
With best wishes,
Vladimir

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

[Vladimir Zapolskiy]

Hi Ramiro,

please find some review comments below.

On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
> and RAW 10 output formats, and MIPI CSI-2 interface.
> 
> The driver adds support for 640x480 RAW 8.
> 
> Signed-off-by: Ramiro Oliveira 
> ---

[snip]

> +
> +struct ov5647 {
> + struct v4l2_subdev  sd;
> + struct media_padpad;
> + struct mutexlock;
> + struct v4l2_mbus_framefmt   format;
> + unsigned intwidth;
> + unsigned intheight;
> + int power_count;
> + struct clk  *xclk;
> + /* External clock frequency currently supported is 30MHz */
> + u32 xclk_freq;

See a comment about 25MHz vs 30MHz below.

Also I assume you can remove 'xclk_freq' from the struct fields,
it can be replaced by a local variable.

> +};

[snip]

> +
> +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
> +{
> + int ret;
> + unsigned char data_w[2] = { reg >> 8, reg & 0xff };
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ret = i2c_master_send(client, data_w, 2);
> + if (ret < 0) {
> + dev_dbg(>dev, "%s: i2c read error, reg: %x\n",

s/i2c read error/i2c write error/

> + __func__, reg);
> + return ret;
> + }
> +
> + ret = i2c_master_recv(client, val, 1);
> + if (ret < 0)
> + dev_dbg(>dev, "%s: i2c read error, reg: %x\n",
> + __func__, reg);
> +
> + return ret;
> +

Please remove the empty line above.

> +}
> +
> +static int ov5647_write_array(struct v4l2_subdev *sd,
> + struct regval_list *regs, int array_size)
> +{
> + int i = 0, ret;

Assignment of 'i' on declaration is not needed, please remove.

> +
> + for (i = 0; i < array_size; i++) {
> + ret = ov5647_write(sd, regs[i].addr, regs[i].data);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
> +{
> + u8 channel_id;
> + int ret;
> +
> + ret = ov5647_read(sd, 0x4814, _id);
> + if (ret < 0)
> + return ret;
> +
> + channel_id &= ~(3 << 6);
> + return ov5647_write(sd, 0x4814, channel_id | (channel << 6));
> +}
> +
> +static int ov5647_stream_on(struct v4l2_subdev *sd)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ov5647_write(sd, 0x4202, 0x00);

Should you add a check of the returned value?

> +
> + dev_dbg(>dev, "Stream on");

I would suggest to remove dev_dbg(), because ftrace will report to a user,
when this function is called.

Also dev_dbg() in the middle of two I2C transfers in a row looks as being
placed improperly.

> +
> + return ov5647_write(sd, 0x300D, 0x00);
> +}
> +
> +static int ov5647_stream_off(struct v4l2_subdev *sd)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ov5647_write(sd, 0x4202, 0x0f);

Should you add a check of the returned value?

> +
> + dev_dbg(>dev, "Stream off");

I would suggest to remove dev_dbg(), because ftrace will report to a user,
when this function is called.

Also dev_dbg() in the middle of two I2C transfers in a row looks as being
placed improperly.

> +
> + return ov5647_write(sd, 0x300D, 0x01);
> +}
> +
> +static int set_sw_standby(struct v4l2_subdev *sd, bool standby)
> +{
> + int ret;
> + u8 rdval;
> +
> + ret = ov5647_read(sd, 0x0100, );
> + if (ret < 0)
> + return ret;
> +
> + if (standby)
> + rdval &= ~0x01;
> + else
> + rdval |= 0x01;
> +
> + return ov5647_write(sd, 0x0100, rdval);
> +}
> +
> +static int __sensor_init(struct v4l2_subdev *sd)
> +{
> + int ret;
> + u8 resetval;
> + u8 rdval;

It could be possible to put declarations of 'resetval' and 'rdval' on the same 
line.

> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + dev_dbg(>dev, "sensor init\n");
> +
> + ret = ov5647_read(sd, 0x0100, );
> + if (ret < 0)
> + return ret;
> +
> + ret = ov5647_write_array(sd, ov5647_640x480,
> + ARRAY_SIZE(ov5647_640x480));
> + if (ret < 0) {
> + dev_err(>dev, "write sensor default regs error\n");
> + return ret;
> + }
> +
> + ret = ov5647_set_virtual_channel(sd, 0);
> + if (ret < 0)
> + return ret;
> +
> + ret = ov5647_read(sd, 0x0100, );
> + if (ret < 0)
> + return ret;
> +
> + if (!(resetval & 0x01)) {
> + dev_err(>dev, "Device was in SW standby");
> + ret = ov5647_write(sd, 0x0100, 0x01);
> + if (ret < 0)
> + 

Re: [PATCH v8 2/2] Add support for OV5647 sensor.

[Vladimir Zapolskiy]

Hi Ramiro,

On 02/13/2017 09:14 PM, Ramiro Oliveira wrote:
> Hi Vladimir,
> 
> Thank you for your feedback.
> 
> On 2/13/2017 12:21 PM, Vladimir Zapolskiy wrote:
>> Hello Ramiro,
>>
>> On 02/13/2017 01:25 PM, Ramiro Oliveira wrote:
>>> Modes supported:
>>>  - 640x480 RAW 8
>>>

[snip]

>>> +static int ov5647_probe(struct i2c_client *client,
>>> +   const struct i2c_device_id *id)
>>> +{
>>> +   struct device *dev = >dev;
>>> +   struct ov5647 *sensor;
>>> +   int ret;
>>> +   struct v4l2_subdev *sd;
>>> +
>>> +   dev_info(>dev, "Installing OmniVision OV5647 camera driver\n");
>>
>> Please remove the informational line, it will pollute the kernel log for no
>> good reason.
>>
> 
> Is it okay if I change it to debug?
> 

Most probably here it is okay to change it to dev_dbg(), however

1) please note that ftrace functionality should provide all the magic for you,
   and by the way this makes dev_dbg() calls from ov5647_write(), ov5647_read(),
   ov5647_stream_on(), ov5647_stream_off() and __sensor_init() all redundant,

2) please move the informational message to the end of the .probe function,
   right before returning success to avoid duplicates on deferred re-probing:

dev_dbg(>dev, "OmniVision OV5647 camera driver probed\n");
  
[snip]

>>> +
>>> +static const struct i2c_device_id ov5647_id[] = {
>>> +   { "ov5647", 0 },
>>> +   { }
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, ov5647_id);
>>> +
>>> +#if IS_ENABLED(CONFIG_OF)
>>
>> From Kconfig the driver depends on OF.
>>
> 
> You're right. Do you think I should remove the dependency in Kconfig or the
> check here?
> 

Let see...

I've been able to locate only one place where OF dependency is utterly needed:

ret = of_property_read_u32(dev->of_node, "clock-frequency",
>xclk_freq);
if (ret) {
dev_err(dev, "could not get xclk frequency\n");
return ret;
}

It might be preferred to change this snippet of code into something like one
below:

sensor->xclk_freq = clk_get_rate(sensor->xclk);
if (sensor->xclk_freq != 3000) {
dev_err(dev, "Unsupported clock frequency: %lu\n",
sensor->xclk_freq);
return -EINVAL;
}

Then

1) in case of an OF platform "xclk" clock frequency can be set directly
   in a board DTS file, please reference to clock-bindings.txt,
2) next you can drop 'clock-frequency' property from the sensor bindings,
3) you can drop 'depends on OF' from drivers/media/i2c/Kconfig,
4) you can drop 'clk_set_rate()' from sensor_power().

The proposed code is slightly more flexible, at least the change gives
a little chance to run the driver successfully on a non-OF platform,
otherwise it is known in advance that the driver is unusable on any
non-OF platforms and thus an explicit CONFIG_OF build dependency makes
sense.

>>> +static const struct of_device_id ov5647_of_match[] = {
>>> +   { .compatible = "ovti,ov5647" },
>>> +   { /* sentinel */ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ov5647_of_match);
>>> +#endif
>>> +
>>> +/**
>>> + * @short i2c driver structure
>>> + */
>>> +static struct i2c_driver ov5647_driver = {
>>> +   .driver = {
>>> +   .of_match_table = of_match_ptr(ov5647_of_match),
>>
>> Same comment as above, from Kconfig the driver depends on OF.
>>
> 
> I'm sorry but I'm not understanding what you're trying to say.
> 

You may take a look at of_match_ptr() macro definition from include/linux/of.h:

#ifdef CONFIG_OF
...
#define of_match_ptr(_ptr)  (_ptr)
...
#else
...
#define of_match_ptr(_ptr)  NULL
...
#endif

Hence if the code compilation always depends on the enabled CONFIG_OF option,
then of_match_ptr() macro should be dropped:

.of_match_table = ov5647_of_match,

>>> +   .owner  = THIS_MODULE,
>>
>> .owner is set by the core, please remove it.
>>
> 
> Ok.
> 
>>> +   .name   = "ov5647",
>>
>> May be .name = SENSOR_NAME, ?
>>
>> Otherwise SENSOR_NAME macro is unused and it should be removed.
>>
> 
> I'll change it to .name = SENSOR_NAME,
> 
>>> +   },
>>> +   .probe  = ov5647_probe,
>>> +   .remove = ov5647_remove,
>>> +   .id_table   = ov5647_id,
>>> +};
>>> +
>>> +module_i2c_driver(ov5647_driver);
>>> +
>>> +MODULE_AUTHOR("Ramiro Oliveira <roliv...@synopsys.com>");
>>> +MODULE_DESCRIPTION("A low-level driver for OmniVision ov5647 sensors");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>

--
With best wishes,
Vladimir

Re: [PATCH v8 2/2] Add support for OV5647 sensor.

[Vladimir Zapolskiy]

Hello Ramiro,

On 02/13/2017 01:25 PM, Ramiro Oliveira wrote:
> Modes supported:
>  - 640x480 RAW 8
> 

It is a pretty short commit message, please consider to write a couple
of words about the sensor.

> Signed-off-by: Ramiro Oliveira 
> ---

[snip]

> +
> +struct cfg_array {
> + struct regval_list *regs;
> + int size;
> +};

struct cfg_array is apparently unused.

> +
> +/**
> + * @short I2C Write operation
> + * @param[in] i2c_client I2C client
> + * @param[in] reg register address
> + * @param[in] val value to write
> + * @return Error code
> + */
> +static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
> +{

The comment contents is probably outdated, because it does not describe
the function input arguments properly.

> + int ret;
> + unsigned char data[3] = { reg >> 8, reg & 0xff, val};
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ret = i2c_master_send(client, data, 3);
> + if (ret != 3) {
> + dev_dbg(>dev, "%s: i2c write error, reg: %x\n",
> + __func__, reg);
> + return ret < 0 ? ret : -EIO;

Here i2c_master_send() returns only to a negative error or '3', thus
the check is redundant.

Please do 'return ret';

> + }
> + return 0;
> +}
> +
> +/**
> + * @short I2C Read operation
> + * @param[in] i2c_client I2C client
> + * @param[in] reg register address
> + * @param[out] val value read
> + * @return Error code
> + */
> +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
> +{

Same as above, the comment must be updated.

> + int ret;
> + unsigned char data_w[2] = { reg >> 8, reg & 0xff };
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ret = i2c_master_send(client, data_w, 2);
> +
> + if (ret < 2) {
> + dev_dbg(>dev, "%s: i2c read error, reg: %x\n",
> + __func__, reg);
> + return ret < 0 ? ret : -EIO;


Here i2c_master_send() returns only to a negative error or '2', thus
the check is redundant.

Please do 'return ret';

> + }
> +
> + ret = i2c_master_recv(client, val, 1);
> +
> + if (ret < 1) {
> + dev_dbg(>dev, "%s: i2c read error, reg: %x\n",
> + __func__, reg);
> + return ret < 0 ? ret : -EIO;

Here i2c_master_recv() returns only to a negative error or '1', thus
the check is redundant.

Please do 'return ret';

> + }
> + return 0;
> +}
> +
> +static int ov5647_write_array(struct v4l2_subdev *sd,
> + struct regval_list *regs, int array_size)
> +{
> + int i = 0;
> + int ret = 0;

Please don't assign a value to 'ret' and please do declarations on a single 
line.

> +
> + if (!regs)
> + return -EINVAL;

!regs is a dead code check, please remove it.

> +
> + while (i < array_size) {
> + ret = ov5647_write(sd, regs->addr, regs->data);
> + if (ret < 0)
> + return ret;
> + i++;
> + regs++;
> + }

Please do a for-loop, it will save two lines of code:

for (i = 0; i < array_size; i++) {
ret = ov5647_write(sd, regs[i].addr, regs[i].data);
if (ret < 0)
return ret;
}

> + return 0;
> +}
> +
> +static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
> +{
> + u8 channel_id;
> + int ret = 0;

Please don't assign a value to 'ret' on declaration here.

> +
> + ret = ov5647_read(sd, 0x4814, _id);
> + if (ret < 0)
> + return ret;
> +
> + channel_id &= ~(3 << 6);
> + return ov5647_write(sd, 0x4814, channel_id | (channel << 6));
> +}
> +
> +static int ov5647_stream_on(struct v4l2_subdev *sd)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ov5647_write(sd, 0x4202, 0x00);
> +
> + dev_dbg(>dev, "Stream on");
> +
> + return ov5647_write(sd, 0x300D, 0x00);
> +}
> +
> +static int ov5647_stream_off(struct v4l2_subdev *sd)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ov5647_write(sd, 0x4202, 0x0f);
> +
> + dev_dbg(>dev, "Stream off");
> +
> + return ov5647_write(sd, 0x300D, 0x01);
> +}
> +
> +/**
> + * @short Set SW standby
> + * @param[in] sd v4l2 sd
> + * @param[in] stanby standby mode status (on or off)
> + * @return Error code
> + */
> +static int set_sw_standby(struct v4l2_subdev *sd, bool standby)
> +{
> + int ret;
> + unsigned char rdval;

ov5647_read() return value type is 'u8', please change it here and
everywhere else in the code.

> +
> + ret = ov5647_read(sd, 0x0100, );
> + if (ret != 0)

if (ret < 0)

> + return ret;
> +
> + if (standby)
> + rdval &= 0xfe;

Here 'rdval &= ~0x01' would be preferred to emphasize symmetry.

> + else
> + rdval |= 0x01;
> +
> + return ov5647_write(sd, 0x0100, rdval);
> +}
> +
> +/**
> + * 

Re: [PATCH v2 15/19] media: imx: Add MIPI CSI-2 Receiver subdev driver

[Vladimir Zapolskiy]

On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> Adds MIPI CSI-2 Receiver subdev driver. This subdev is required
> for sensors with a MIPI CSI2 interface.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  drivers/staging/media/imx/Makefile|   1 +
>  drivers/staging/media/imx/imx-mipi-csi2.c | 509 
> ++
>  2 files changed, 510 insertions(+)
>  create mode 100644 drivers/staging/media/imx/imx-mipi-csi2.c
> 
> diff --git a/drivers/staging/media/imx/Makefile 
> b/drivers/staging/media/imx/Makefile
> index fe9e992..0decef7 100644
> --- a/drivers/staging/media/imx/Makefile
> +++ b/drivers/staging/media/imx/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-ic.o
>  obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-csi.o
>  obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-smfc.o
>  obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-camif.o
> +obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-mipi-csi2.o
> diff --git a/drivers/staging/media/imx/imx-mipi-csi2.c 
> b/drivers/staging/media/imx/imx-mipi-csi2.c
> new file mode 100644
> index 000..84df16e
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-mipi-csi2.c
> @@ -0,0 +1,509 @@
> +/*
> + * MIPI CSI-2 Receiver Subdev for Freescale i.MX5/6 SOC.
> + *
> + * Copyright (c) 2012-2014 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Please sort the list of headers alphabetically.

> +#include 

Why do you need to include this header?

> +#include 
> +#include "imx-media.h"
> +

[snip]

> +static int imxcsi2_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct imxcsi2_dev *csi2 = sd_to_dev(sd);
> + int i, ret = 0;
> +
> + if (!csi2->src_sd)
> + return -EPIPE;
> + for (i = 0; i < CSI2_NUM_SRC_PADS; i++) {
> + if (csi2->sink_sd[i])
> + break;
> + }
> + if (i >= CSI2_NUM_SRC_PADS)
> + return -EPIPE;
> +
> + v4l2_info(sd, "stream %s\n", enable ? "ON" : "OFF");
> +
> + if (enable && !csi2->stream_on) {
> + clk_prepare_enable(csi2->pix_clk);

It can complicate the design for you, but in general clk_prepare_enable()
can return an error.

> + ret = imxcsi2_dphy_wait(csi2);
> + if (ret)
> + clk_disable_unprepare(csi2->pix_clk);
> + } else if (!enable && csi2->stream_on) {
> + clk_disable_unprepare(csi2->pix_clk);
> + }
> +
> + if (!ret)
> + csi2->stream_on = enable;
> + return ret;
> +}
> +

[snip]

> +
> +static int imxcsi2_parse_endpoints(struct imxcsi2_dev *csi2)
> +{
> + struct device_node *node = csi2->dev->of_node;
> + struct device_node *epnode;
> + struct v4l2_of_endpoint ep;
> + int ret = 0;
> +
> + epnode = of_graph_get_next_endpoint(node, NULL);
> + if (!epnode) {
> + v4l2_err(>sd, "failed to get endpoint node\n");
> + return -EINVAL;
> + }
> +
> + v4l2_of_parse_endpoint(epnode, );

Do of_node_put(epnode) here and remove 'out' goto label.

> + if (ep.bus_type != V4L2_MBUS_CSI2) {
> + v4l2_err(>sd, "invalid bus type, must be MIPI CSI2\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + csi2->bus = ep.bus.mipi_csi2;
> +
> + v4l2_info(>sd, "data lanes: %d\n", csi2->bus.num_data_lanes);
> + v4l2_info(>sd, "flags: 0x%08x\n", csi2->bus.flags);
> +out:
> + of_node_put(epnode);
> + return ret;
> +}
> +

[snip]

> +static const struct of_device_id imxcsi2_dt_ids[] = {
> + { .compatible = "fsl,imx-mipi-csi2", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imxcsi2_dt_ids);
> +
> +static struct platform_driver imxcsi2_driver = {
> + .driver = {
> + .name = DEVICE_NAME,
> + .owner = THIS_MODULE,

Please drop .owner assignment.

> + .of_match_table = imxcsi2_dt_ids,
> + },
> + .probe = imxcsi2_probe,
> + .remove = imxcsi2_remove,
> +};
> +
> +module_platform_driver(imxcsi2_driver);
> +
> +MODULE_DESCRIPTION("i.MX5/6 MIPI CSI-2 Receiver driver");
> +MODULE_AUTHOR("Steve Longerbeam ");
> +MODULE_LICENSE("GPL");
> +
> 

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/19] media: imx: Add Camera Interface subdev driver

[Vladimir Zapolskiy]

On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> This is the camera interface driver that provides the v4l2
> user interface. Frames can be received from various sources:
> 
> - directly from SMFC for capturing unconverted images directly from
>   camera sensors.
> 
> - from the IC pre-process encode task.
> 
> - from the IC pre-process viewfinder task.
> 
> - from the IC post-process task.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  drivers/staging/media/imx/Makefile|2 +-
>  drivers/staging/media/imx/imx-camif.c | 1010 
> +
>  2 files changed, 1011 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/staging/media/imx/imx-camif.c
> 
> diff --git a/drivers/staging/media/imx/Makefile 
> b/drivers/staging/media/imx/Makefile
> index d2a962c..fe9e992 100644
> --- a/drivers/staging/media/imx/Makefile
> +++ b/drivers/staging/media/imx/Makefile
> @@ -8,4 +8,4 @@ obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-ic.o
>  
>  obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-csi.o
>  obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-smfc.o
> -
> +obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-camif.o

obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-camif.o imx-csi.o imx-smfc.o

as an option.

> diff --git a/drivers/staging/media/imx/imx-camif.c 
> b/drivers/staging/media/imx/imx-camif.c
> new file mode 100644
> index 000..3cf167e
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-camif.c
> @@ -0,0 +1,1010 @@
> +/*
> + * Video Camera Capture Subdev for Freescale i.MX5/6 SOC
> + *
> + * Copyright (c) 2012-2016 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Please sort the list of headers alphabetically.

> +#include 
> +#include 
> +#include "imx-media.h"
> +
> +#define DEVICE_NAME "imx-media-camif"

I would propose to drop this macro.

> +
> +#define CAMIF_NUM_PADS 2
> +
> +#define CAMIF_DQ_TIMEOUT5000

Add a comment about time unit?

> +
> +struct camif_priv;
> +

This is a leftover apparently.

> +struct camif_priv {
> + struct device *dev;
> + struct video_devicevfd;
> + struct media_pipeline  mp;
> + struct imx_media_dev  *md;

[snip]

> +static int camif_probe(struct platform_device *pdev)
> +{
> + struct imx_media_internal_sd_platformdata *pdata;
> + struct camif_priv *priv;
> + struct video_device *vfd;
> + int ret;
> +
> + priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> + priv->dev = >dev;
> +
> + pdata = priv->dev->platform_data;
> +
> + mutex_init(>mutex);
> + spin_lock_init(>q_lock);
> +
> + v4l2_subdev_init(>sd, _subdev_ops);
> + v4l2_set_subdevdata(>sd, priv);
> + priv->sd.internal_ops = _internal_ops;
> + priv->sd.entity.ops = _entity_ops;
> + priv->sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> + priv->sd.dev = >dev;
> + priv->sd.owner = THIS_MODULE;
> + /* get our group id and camif id */
> + priv->sd.grp_id = pdata->grp_id;
> + priv->id = (pdata->grp_id >> IMX_MEDIA_GRP_ID_CAMIF_BIT) - 1;
> + strncpy(priv->sd.name, pdata->sd_name, sizeof(priv->sd.name));
> + snprintf(camif_videodev.name, sizeof(camif_videodev.name),
> +  "%s devnode", pdata->sd_name);
> +
> + priv->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> +
> + vfd = >vfd;
> + *vfd = camif_videodev;
> + vfd->lock = >mutex;
> + vfd->queue = >buffer_queue;
> +
> + video_set_drvdata(vfd, priv);
> +
> + v4l2_ctrl_handler_init(>ctrl_hdlr, 0);
> +
> + ret = v4l2_async_register_subdev(>sd);
> + if (ret)
> + goto free_ctrls;
> +
> + return 0;
> +free_ctrls:
> + v4l2_ctrl_handler_free(>ctrl_hdlr);
> + return ret;

A shorter version:

if (ret)
v4l2_ctrl_handler_free(>ctrl_hdlr);

return ret;

> +}
> +
> +static int camif_remove(struct platform_device *pdev)
> +{
> + struct camif_priv *priv =
> + (struct camif_priv *)platform_get_drvdata(pdev);
> +
> + v4l2_ctrl_handler_free(>ctrl_hdlr);
> + v4l2_async_unregister_subdev(>sd);
> + media_entity_cleanup(>sd.entity);
> + v4l2_device_unregister_subdev(>sd);
> +
> + return 0;
> +}
> +
> +static const struct platform_device_id camif_ids[] = {
> + { .name = DEVICE_NAME },
> + { },
> +};
> +MODULE_DEVICE_TABLE(platform, camif_ids);
> +
> +static struct platform_driver imx_camif_driver = {
> + .probe  = camif_probe,
> + 

Re: [PATCH v2 13/19] media: imx: Add IC subdev drivers

[Vladimir Zapolskiy]

On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> This is a set of three media entity subdevice drivers for the i.MX
> Image Converter. The i.MX IC module contains three independent
> "tasks":
> 
> - Pre-processing Encode task: video frames are routed directly from
>   the CSI and can be scaled, color-space converted, and rotated.
>   Scaled output is limited to 1024x1024 resolution. Output frames
>   are routed to the camera interface entities (camif).
> 
> - Pre-processing Viewfinder task: this task can perform the same
>   conversions as the pre-process encode task, but in addition can
>   be used for hardware motion compensated deinterlacing. Frames can
>   come either directly from the CSI or from the SMFC entities (memory
>   buffers via IDMAC channels). Scaled output is limited to 1024x1024
>   resolution. Output frames can be routed to various sinks including
>   the post-processing task entities.
> 
> - Post-processing task: same conversions as pre-process encode. However
>   this entity sends frames to the i.MX IPU image converter which supports
>   image tiling, which allows scaled output up to 4096x4096 resolution.
>   Output frames can be routed to the camera interfaces.
> 
> Signed-off-by: Steve Longerbeam 
> ---

[snip]

> +static int imx_ic_probe(struct platform_device *pdev)
> +{
> + struct imx_media_internal_sd_platformdata *pdata;
> + struct imx_ic_priv *priv;
> + int ret;
> +
> + priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, >sd);
> + priv->dev = >dev;
> +
> + /* get our ipu_id, grp_id and IC task id */
> + pdata = priv->dev->platform_data;
> + priv->ipu_id = pdata->ipu_id;
> + switch (pdata->grp_id) {
> + case IMX_MEDIA_GRP_ID_IC_PRPENC:
> + priv->task_id = IC_TASK_ENCODER;
> + break;
> + case IMX_MEDIA_GRP_ID_IC_PRPVF:
> + priv->task_id = IC_TASK_VIEWFINDER;
> + break;
> + case IMX_MEDIA_GRP_ID_IC_PP0...IMX_MEDIA_GRP_ID_IC_PP3:
> + priv->task_id = IC_TASK_POST_PROCESSOR;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + v4l2_subdev_init(>sd, ic_ops[priv->task_id]->subdev_ops);
> + v4l2_set_subdevdata(>sd, priv);
> + priv->sd.internal_ops = ic_ops[priv->task_id]->internal_ops;
> + priv->sd.entity.ops = ic_ops[priv->task_id]->entity_ops;
> + priv->sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
> + priv->sd.dev = >dev;
> + priv->sd.owner = THIS_MODULE;
> + priv->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> + priv->sd.grp_id = pdata->grp_id;
> + strncpy(priv->sd.name, pdata->sd_name, sizeof(priv->sd.name));
> +
> + ret = ic_ops[priv->task_id]->init(priv);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_async_register_subdev(>sd);
> + if (ret)
> + goto remove;
> +
> + return 0;
> +remove:
> + ic_ops[priv->task_id]->remove(priv);
> + return ret;

if (ret)
ic_ops[priv->task_id]->remove(priv);

return ret;

as an alternative.

[snip]

> +static const struct platform_device_id imx_ic_ids[] = {
> + { .name = "imx-ipuv3-ic" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(platform, imx_ic_ids);
> +
> +static struct platform_driver imx_ic_driver = {
> + .probe = imx_ic_probe,
> + .remove = imx_ic_remove,
> + .id_table = imx_ic_ids,
> + .driver = {
> + .name = "imx-ipuv3-ic",
> + .owner = THIS_MODULE,

Please drop .owner assignment.

> + },
> +};
> +module_platform_driver(imx_ic_driver);
> +
> +MODULE_DESCRIPTION("i.MX IC subdev driver");
> +MODULE_AUTHOR("Steve Longerbeam ");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:imx-ipuv3-ic");
> diff --git a/drivers/staging/media/imx/imx-ic-pp.c 
> b/drivers/staging/media/imx/imx-ic-pp.c
> new file mode 100644
> index 000..5ef0581
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-ic-pp.c
> @@ -0,0 +1,636 @@
> +/*
> + * V4L2 IC Post-Processor Subdev for Freescale i.MX5/6 SOC
> + *
> + * Copyright (c) 2014-2016 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Please sort the list of headers alphabetically.

> +#include 
> +#include 
> +#include "imx-media.h"
> +#include "imx-ic.h"
> +

[snip]

> +
> +static int pp_start(struct pp_priv *priv)
> +{
> + struct imx_ic_priv *ic_priv = priv->ic_priv;
> + struct ipu_image image_in, 

Re: [PATCH v2 12/19] media: imx: Add SMFC subdev driver

[Vladimir Zapolskiy]

On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> This is a media entity subdevice driver for the i.MX Sensor Multi-FIFO
> Controller module. Video frames are received from the CSI and can
> be routed to various sinks including the i.MX Image Converter for
> scaling, color-space conversion, motion compensated deinterlacing,
> and image rotation.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  drivers/staging/media/imx/Makefile   |   1 +
>  drivers/staging/media/imx/imx-smfc.c | 739 
> +++
>  2 files changed, 740 insertions(+)
>  create mode 100644 drivers/staging/media/imx/imx-smfc.c
> 
> diff --git a/drivers/staging/media/imx/Makefile 
> b/drivers/staging/media/imx/Makefile
> index 133672a..3559d7b 100644
> --- a/drivers/staging/media/imx/Makefile
> +++ b/drivers/staging/media/imx/Makefile
> @@ -5,4 +5,5 @@ obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media.o
>  obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-common.o
>  
>  obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-csi.o
> +obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-smfc.o

May be

obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-csi.o imx-smfc.o

>  
> diff --git a/drivers/staging/media/imx/imx-smfc.c 
> b/drivers/staging/media/imx/imx-smfc.c
> new file mode 100644
> index 000..565048c
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-smfc.c
> @@ -0,0 +1,739 @@
> +/*
> + * V4L2 Capture SMFC Subdev for Freescale i.MX5/6 SOC
> + *
> + * This subdevice handles capture of raw/unconverted video frames
> + * from the CSI, directly to memory via the Sensor Multi-FIFO Controller.
> + *
> + * Copyright (c) 2012-2016 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Please sort the list of headers alphabetically.

> +#include 
> +#include "imx-media.h"
> +

[snip]

> +static irqreturn_t imx_smfc_eof_interrupt(int irq, void *dev_id)
> +{
> + struct imx_smfc_priv *priv = dev_id;
> + struct imx_media_dma_buf *done, *next;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>irqlock, flags);

spin_lock(>irqlock) should be sufficient.

> +
> + if (priv->last_eof) {
> + complete(>last_eof_comp);
> + priv->last_eof = false;
> + goto unlock;
> + }
> +
> + /* inform CSI of this EOF so it can monitor frame intervals */
> + v4l2_subdev_call(priv->src_sd, core, interrupt_service_routine,
> +  0, NULL);
> +
> + done = imx_media_dma_buf_get_active(priv->out_ring);
> + /* give the completed buffer to the sink  */
> + if (!WARN_ON(!done))
> + imx_media_dma_buf_done(done, IMX_MEDIA_BUF_STATUS_DONE);
> +
> + /* priv->next buffer is now the active one */
> + imx_media_dma_buf_set_active(priv->next);
> +
> + /* bump the EOF timeout timer */
> + mod_timer(>eof_timeout_timer,
> +   jiffies + msecs_to_jiffies(IMX_MEDIA_EOF_TIMEOUT));
> +
> + if (ipu_idmac_buffer_is_ready(priv->smfc_ch, priv->ipu_buf_num))
> + ipu_idmac_clear_buffer(priv->smfc_ch, priv->ipu_buf_num);
> +
> + /* get next queued buffer */
> + next = imx_media_dma_buf_get_next_queued(priv->out_ring);
> +
> + ipu_cpmem_set_buffer(priv->smfc_ch, priv->ipu_buf_num, next->phys);
> + ipu_idmac_select_buffer(priv->smfc_ch, priv->ipu_buf_num);
> +
> + /* toggle IPU double-buffer index */
> + priv->ipu_buf_num ^= 1;
> + priv->next = next;
> +
> +unlock:
> + spin_unlock_irqrestore(>irqlock, flags);
> + return IRQ_HANDLED;
> +}
> +

[snip]

> +
> +static const struct platform_device_id imx_smfc_ids[] = {
> + { .name = "imx-ipuv3-smfc" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(platform, imx_smfc_ids);
> +
> +static struct platform_driver imx_smfc_driver = {
> + .probe = imx_smfc_probe,
> + .remove = imx_smfc_remove,
> + .id_table = imx_smfc_ids,
> + .driver = {
> + .name = "imx-ipuv3-smfc",
> + .owner = THIS_MODULE,

You can drop owner assignment.

> + },
> +};
> +module_platform_driver(imx_smfc_driver);
> +
> +MODULE_DESCRIPTION("i.MX SMFC subdev driver");
> +MODULE_AUTHOR("Steve Longerbeam ");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:imx-ipuv3-smfc");
> 

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/19] media: imx: Add CSI subdev driver

[Vladimir Zapolskiy]

On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> This is a media entity subdevice for the i.MX Camera
> Serial Interface module.
> 
> Signed-off-by: Steve Longerbeam 
> ---

[snip]

> diff --git a/drivers/staging/media/imx/imx-csi.c 
> b/drivers/staging/media/imx/imx-csi.c
> new file mode 100644
> index 000..975eafb
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-csi.c
> @@ -0,0 +1,638 @@
> +/*
> + * V4L2 Capture CSI Subdev for Freescale i.MX5/6 SOC
> + *
> + * Copyright (c) 2014-2016 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Please add the headers alphabetically ordered.

> +#include 
> +#include "imx-media.h"
> +
> +#define CSI_NUM_PADS 2
> +
> +struct csi_priv {
> + struct device *dev;
> + struct ipu_soc *ipu;
> + struct imx_media_dev *md;
> + struct v4l2_subdev sd;
> + struct media_pad pad[CSI_NUM_PADS];
> + struct v4l2_mbus_framefmt format_mbus[CSI_NUM_PADS];
> + struct v4l2_mbus_config sensor_mbus_cfg;
> + struct v4l2_rect crop;
> + struct ipu_csi *csi;
> + int csi_id;
> + int input_pad;
> + int output_pad;
> + bool power_on;  /* power is on */
> + bool stream_on; /* streaming is on */
> +
> + /* the sink for the captured frames */
> + struct v4l2_subdev *sink_sd;
> + enum ipu_csi_dest dest;
> + struct v4l2_subdev *src_sd;
> +
> + struct v4l2_ctrl_handler ctrl_hdlr;
> + struct imx_media_fim *fim;
> +
> + /* the attached sensor at stream on */
> + struct imx_media_subdev *sensor;
> +};
> +
> +static inline struct csi_priv *sd_to_dev(struct v4l2_subdev *sdev)
> +{
> + return container_of(sdev, struct csi_priv, sd);
> +}
> +
> +/* Update the CSI whole sensor and active windows */
> +static int csi_setup(struct csi_priv *priv)
> +{
> + struct v4l2_mbus_framefmt infmt;
> +
> + ipu_csi_set_window(priv->csi, >crop);
> +
> + /*
> +  * the ipu-csi doesn't understand ALTERNATE, but it only
> +  * needs to know whether the stream is interlaced, so set
> +  * to INTERLACED if infmt field is ALTERNATE.
> +  */
> + infmt = priv->format_mbus[priv->input_pad];
> + if (infmt.field == V4L2_FIELD_ALTERNATE)
> + infmt.field = V4L2_FIELD_INTERLACED;
> +
> + ipu_csi_init_interface(priv->csi, >sensor_mbus_cfg, );
> +
> + ipu_csi_set_dest(priv->csi, priv->dest);
> +
> + ipu_csi_dump(priv->csi);
> +
> + return 0;
> +}
> +
> +static int csi_start(struct csi_priv *priv)
> +{
> + int ret;
> +
> + if (!priv->sensor) {
> + v4l2_err(>sd, "no sensor attached\n");
> + return -EINVAL;
> + }
> +
> + ret = csi_setup(priv);
> + if (ret)
> + return ret;
> +
> + /* start the frame interval monitor */
> + ret = imx_media_fim_set_stream(priv->fim, priv->sensor, true);
> + if (ret)
> + return ret;
> +
> + ret = ipu_csi_enable(priv->csi);
> + if (ret) {
> + v4l2_err(>sd, "CSI enable error: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;

if (ret)
v4l2_err(>sd, "CSI enable error: %d\n", ret);

return ret;

> +}
> +
> +static void csi_stop(struct csi_priv *priv)
> +{
> + /* stop the frame interval monitor */
> + imx_media_fim_set_stream(priv->fim, priv->sensor, false);
> +
> + ipu_csi_disable(priv->csi);
> +}
> +
> +static int csi_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct csi_priv *priv = v4l2_get_subdevdata(sd);
> + int ret = 0;
> +
> + if (!priv->src_sd || !priv->sink_sd)
> + return -EPIPE;
> +
> + v4l2_info(sd, "stream %s\n", enable ? "ON" : "OFF");
> +
> + if (enable && !priv->stream_on)
> + ret = csi_start(priv);
> + else if (!enable && priv->stream_on)
> + csi_stop(priv);
> +
> + if (!ret)
> + priv->stream_on = enable;
> + return ret;
> +}
> +
> +static int csi_s_power(struct v4l2_subdev *sd, int on)
> +{
> + struct csi_priv *priv = v4l2_get_subdevdata(sd);
> + int ret = 0;
> +
> + v4l2_info(sd, "power %s\n", on ? "ON" : "OFF");
> +
> + if (on != priv->power_on)
> + ret = imx_media_fim_set_power(priv->fim, on);
> +
> + if (!ret)
> + priv->power_on = on;
> + return ret;
> +}
> +
> +static int csi_link_setup(struct media_entity *entity,
> +   const struct media_pad *local,
> +   const struct media_pad *remote, u32 flags)
> +{
> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> + struct csi_priv *priv = v4l2_get_subdevdata(sd);
> + struct v4l2_subdev 

Re: [PATCH v2 10/19] media: Add i.MX media core driver

[Vladimir Zapolskiy]

Hi Steve,

On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> Add the core media driver for i.MX SOC.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  Documentation/devicetree/bindings/media/imx.txt   | 205 +

v2 was sent before getting Rob's review comments, but still they
should be addressed in v3.

Also I would suggest to separate device tree binding documentation
change and place it as the first patch in the series, this should
make the following DTS changes valid.

>  Documentation/media/v4l-drivers/imx.rst   | 430 ++
>  drivers/staging/media/Kconfig |   2 +
>  drivers/staging/media/Makefile|   1 +
>  drivers/staging/media/imx/Kconfig |   8 +
>  drivers/staging/media/imx/Makefile|   6 +
>  drivers/staging/media/imx/TODO|  18 +
>  drivers/staging/media/imx/imx-media-common.c  | 985 
> ++
>  drivers/staging/media/imx/imx-media-dev.c | 479 +++
>  drivers/staging/media/imx/imx-media-fim.c | 509 +++
>  drivers/staging/media/imx/imx-media-internal-sd.c | 457 ++
>  drivers/staging/media/imx/imx-media-of.c  | 291 +++
>  drivers/staging/media/imx/imx-media-of.h  |  25 +
>  drivers/staging/media/imx/imx-media.h | 299 +++
>  include/media/imx.h   |  15 +
>  include/uapi/Kbuild   |   1 +
>  include/uapi/linux/v4l2-controls.h|   4 +
>  include/uapi/media/Kbuild |   2 +
>  include/uapi/media/imx.h  |  30 +

Probably Greg should ack the UAPI changes, you may consider
to split them into a separate patch.

>  19 files changed, 3767 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/imx.txt
>  create mode 100644 Documentation/media/v4l-drivers/imx.rst
>  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-media-common.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-of.h
>  create mode 100644 drivers/staging/media/imx/imx-media.h
>  create mode 100644 include/media/imx.h
>  create mode 100644 include/uapi/media/Kbuild
>  create mode 100644 include/uapi/media/imx.h
> 

[snip]

> +
> +struct imx_media_subdev *
> +imx_media_find_subdev_by_sd(struct imx_media_dev *imxmd,
> + struct v4l2_subdev *sd)
> +{
> + struct imx_media_subdev *imxsd;
> + int i, ret = -ENODEV;
> +
> + for (i = 0; i < imxmd->num_subdevs; i++) {
> + imxsd = >subdev[i];
> + if (sd == imxsd->sd) {

This can be simplifed:

...

if (sd == imxsd->sd)
return imxsd;
}

return ERR_PTR(-ENODEV);

> + ret = 0;
> + break;
> + }
> + }
> +
> + return ret ? ERR_PTR(ret) : imxsd;
> +}
> +EXPORT_SYMBOL_GPL(imx_media_find_subdev_by_sd);
> +
> +struct imx_media_subdev *
> +imx_media_find_subdev_by_id(struct imx_media_dev *imxmd, u32 grp_id)
> +{
> + struct imx_media_subdev *imxsd;
> + int i, ret = -ENODEV;
> +
> + for (i = 0; i < imxmd->num_subdevs; i++) {
> + imxsd = >subdev[i];
> + if (imxsd->sd && imxsd->sd->grp_id == grp_id) {
> + ret = 0;
> + break;

This can be simplifed:

...

if (imxsd->sd && imxsd->sd->grp_id == grp_i)
return imxsd;
}

return ERR_PTR(-ENODEV);

> + }
> + }
> +
> + return ret ? ERR_PTR(ret) : imxsd;
> +}
> +EXPORT_SYMBOL_GPL(imx_media_find_subdev_by_id);
> +

[snip]

> diff --git a/drivers/staging/media/imx/imx-media-dev.c 
> b/drivers/staging/media/imx/imx-media-dev.c
> new file mode 100644
> index 000..8d22730
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -0,0 +1,479 @@
> +/*
> + * V4L2 Media Controller Driver for Freescale i.MX5/6 SOC
> + *
> + * Copyright (c) 2016 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Please sort out the list of headers alphabetically.

> +#include 
> +#include 
> +#include "imx-media.h"
> +#include 

Re: [PATCH v2 09/19] ARM: dts: imx6-sabreauto: add the ADV7180 video decoder

[Vladimir Zapolskiy]

On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> Enables the ADV7180 decoder sensor. The ADV7180 connects to the
> parallel-bus mux input on ipu1_csi0_mux.
> 
> On the sabreauto, two analog video inputs are routed to the ADV7180,
> composite on Ain1, and composite on Ain3. Those inputs are defined
> via inputs and input-names under the ADV7180 node. The ADV7180 power
> pin is via max7310_b port expander.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  arch/arm/boot/dts/imx6qdl-sabreauto.dtsi | 56 
> 
>  1 file changed, 56 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi 
> b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> index 83ac2ff..30ee378 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> @@ -147,10 +147,42 @@
>   gpio-controller;
>   #gpio-cells = <2>;
>   };
> +
> + camera: adv7180@21 {

adv7180: camera@21

> + compatible = "adi,adv7180";
> + reg = <0x21>;
> + powerdown-gpios = <_b 2 
> GPIO_ACTIVE_LOW>;
> + interrupt-parent = <>;
> + interrupts = <27 0x8>;
> + inputs = <0x00 0x02>;
> + input-names = "ADV7180 Composite on Ain1",
> + "ADV7180 Composite on Ain3";
> +
> + port {
> + adv7180_to_ipu1_csi0_mux: endpoint {
> + remote-endpoint = 
> <_csi0_mux_from_parallel_sensor>;
> + bus-width = <8>;
> + };
> + };
> + };
>   };
>   };
>  };
>  
> +_csi0_from_ipu1_csi0_mux {
> + bus-width = <8>;
> +};
> +
> +_csi0_mux_from_parallel_sensor {
> + remote-endpoint = <_to_ipu1_csi0_mux>;
> + bus-width = <8>;
> +};
> +
> +_csi0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <_ipu1_csi0>;
> +};
> +
>   {
>   assigned-clocks = < IMX6QDL_PLL4_BYPASS_SRC>,
> < IMX6QDL_PLL4_BYPASS>,
> @@ -451,6 +483,30 @@
>   >;
>   };
>  
> + pinctrl_ipu1_csi0: ipu1grp-csi0 {

Please rename node name to ipu1csi0grp.

> + fsl,pins = <
> + MX6QDL_PAD_CSI0_DAT4__IPU1_CSI0_DATA04   
> 0x8000
> + MX6QDL_PAD_CSI0_DAT5__IPU1_CSI0_DATA05   
> 0x8000
> + MX6QDL_PAD_CSI0_DAT6__IPU1_CSI0_DATA06   
> 0x8000
> + MX6QDL_PAD_CSI0_DAT7__IPU1_CSI0_DATA07   
> 0x8000
> + MX6QDL_PAD_CSI0_DAT8__IPU1_CSI0_DATA08   
> 0x8000
> + MX6QDL_PAD_CSI0_DAT9__IPU1_CSI0_DATA09   
> 0x8000
> + MX6QDL_PAD_CSI0_DAT10__IPU1_CSI0_DATA10  
> 0x8000
> + MX6QDL_PAD_CSI0_DAT11__IPU1_CSI0_DATA11  
> 0x8000
> + MX6QDL_PAD_CSI0_DAT12__IPU1_CSI0_DATA12  
> 0x8000
> + MX6QDL_PAD_CSI0_DAT13__IPU1_CSI0_DATA13  
> 0x8000
> + MX6QDL_PAD_CSI0_DAT14__IPU1_CSI0_DATA14  
> 0x8000
> + MX6QDL_PAD_CSI0_DAT15__IPU1_CSI0_DATA15  
> 0x8000
> + MX6QDL_PAD_CSI0_DAT16__IPU1_CSI0_DATA16  
> 0x8000
> + MX6QDL_PAD_CSI0_DAT17__IPU1_CSI0_DATA17  
> 0x8000
> + MX6QDL_PAD_CSI0_DAT18__IPU1_CSI0_DATA18  
> 0x8000
> + MX6QDL_PAD_CSI0_DAT19__IPU1_CSI0_DATA19  
> 0x8000
> + MX6QDL_PAD_CSI0_PIXCLK__IPU1_CSI0_PIXCLK 
> 0x8000
> + MX6QDL_PAD_CSI0_MCLK__IPU1_CSI0_HSYNC
> 0x8000
> + MX6QDL_PAD_CSI0_VSYNC__IPU1_CSI0_VSYNC   
> 0x8000
> + >;
> + };
> +
>   pinctrl_pwm3: pwm1grp {
>   fsl,pins = <
>   MX6QDL_PAD_SD4_DAT1__PWM3_OUT   0x1b0b1
> 

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 05/19] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors

[Vladimir Zapolskiy]

On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> Enables the OV5642 parallel-bus sensor, and the OV5640 MIPI CSI-2 sensor.
> 
> The OV5642 connects to the parallel-bus mux input port on ipu1_csi0_mux.
> 
> The OV5640 connects to the input port on the MIPI CSI-2 receiver on
> mipi_csi. It is set to transmit over MIPI virtual channel 1.
> 
> Until the OV5652 sensor module compatible with the SabreSD becomes
> available for testing, the ov5642 node is currently disabled.
> 
> Signed-off-by: Steve Longerbeam 
> ---

[snip]

> +
> + camera: ov5642@3c {

ov5642: camera@3c

> + compatible = "ovti,ov5642";
> + pinctrl-names = "default";
> + pinctrl-0 = <_ov5642>;
> + clocks = < IMX6QDL_CLK_CKO>;
> + clock-names = "xclk";
> + reg = <0x3c>;
> + xclk = <2400>;
> + DOVDD-supply = <_reg>; /* 1.8v */
> + AVDD-supply = <_reg>;  /* 2.8v, rev C board is VGEN3
> + rev B board is VGEN5 */
> + DVDD-supply = <_reg>;  /* 1.5v*/
> + pwdn-gpios = < 16 GPIO_ACTIVE_HIGH>; /* SD1_DAT0 */
> + reset-gpios = < 17 GPIO_ACTIVE_LOW>; /* SD1_DAT1 */

Comments about SD1_* pad names are redundant.

> + status = "disabled";

Why is it disabled here?

> +
> + port {
> + ov5642_to_ipu1_csi0_mux: endpoint {
> + remote-endpoint = 
> <_csi0_mux_from_parallel_sensor>;
> + bus-width = <8>;
> + hsync-active = <1>;
> + vsync-active = <1>;
> + };
> + };
> + };
>  };
>  
>   {
> @@ -322,6 +376,34 @@
>   };
>   };
>   };
> +
> + mipi_camera: ov5640@3c {

ov5640: camera@3c

> + compatible = "ovti,ov5640_mipi";
> + pinctrl-names = "default";
> + pinctrl-0 = <_ov5640>;
> + reg = <0x3c>;
> + clocks = < IMX6QDL_CLK_CKO>;
> + clock-names = "xclk";
> + xclk = <2400>;
> + DOVDD-supply = <_reg>; /* 1.8v */
> + AVDD-supply = <_reg>;  /* 2.8v, rev C board is VGEN3
> + rev B board is VGEN5 */
> + DVDD-supply = <_reg>;  /* 1.5v*/
> + pwdn-gpios = < 19 GPIO_ACTIVE_HIGH>; /* SD1_DAT2 */
> + reset-gpios = < 20 GPIO_ACTIVE_LOW>; /* SD1_CLK */

Comments about SD1_* pad names are redundant.

> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ov5640_to_mipi_csi: endpoint@1 {
> + reg = <1>;
> + remote-endpoint = <_csi_from_mipi_sensor>;
> + data-lanes = <0 1>;
> + clock-lanes = <2>;
> + };
> + };
> + };
>  };
>  
>   {
> @@ -426,6 +508,36 @@
>   >;
>   };
>  
> + pinctrl_ov5640: ov5640grp {
> + fsl,pins = <
> + MX6QDL_PAD_SD1_DAT2__GPIO1_IO19 0x8000
> + MX6QDL_PAD_SD1_CLK__GPIO1_IO20  0x8000
> + >;
> + };
> +
> + pinctrl_ov5642: ov5642grp {
> + fsl,pins = <
> + MX6QDL_PAD_SD1_DAT0__GPIO1_IO16 0x8000
> + MX6QDL_PAD_SD1_DAT1__GPIO1_IO17 0x8000
> + >;
> + };
> +
> + pinctrl_ipu1_csi0: ipu1grp-csi0 {

Please rename the node name to ipu1csi0grp.

Please add new pin control groups preserving the alphanimerical order.

> + fsl,pins = <
> + MX6QDL_PAD_CSI0_DAT12__IPU1_CSI0_DATA12
> 0x8000
> + MX6QDL_PAD_CSI0_DAT13__IPU1_CSI0_DATA13
> 0x8000
> + MX6QDL_PAD_CSI0_DAT14__IPU1_CSI0_DATA14
> 0x8000
> + MX6QDL_PAD_CSI0_DAT15__IPU1_CSI0_DATA15
> 0x8000
> + MX6QDL_PAD_CSI0_DAT16__IPU1_CSI0_DATA16
> 0x8000
> + MX6QDL_PAD_CSI0_DAT17__IPU1_CSI0_DATA17
> 0x8000
> + MX6QDL_PAD_CSI0_DAT18__IPU1_CSI0_DATA18
> 0x8000
> + MX6QDL_PAD_CSI0_DAT19__IPU1_CSI0_DATA19
> 0x8000
> + MX6QDL_PAD_CSI0_PIXCLK__IPU1_CSI0_PIXCLK   
> 0x8000
> + MX6QDL_PAD_CSI0_MCLK__IPU1_CSI0_HSYNC  
> 0x8000
> + MX6QDL_PAD_CSI0_VSYNC__IPU1_CSI0_VSYNC 
> 0x8000
> + >;
> + };
> +
>   pinctrl_pcie: pciegrp {
> 

Re: [PATCH v2 04/19] ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors

[Vladimir Zapolskiy]

Hi Steve,

On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> Enables the OV5642 parallel-bus sensor, and the OV5640 MIPI CSI-2 sensor.
> Both hang off the same i2c2 bus, so they require different (and non-
> default) i2c slave addresses.
> 
> The OV5642 connects to the parallel-bus mux input port on ipu1_csi0_mux.
> 
> The OV5640 connects to the input port on the MIPI CSI-2 receiver on
> mipi_csi. It is set to transmit over MIPI virtual channel 1.
> 
> Note there is a pin conflict with GPIO6. This pin functions as a power
> input pin to the OV5642, but ENET uses it as the h/w workaround for
> erratum ERR006687, to wake-up the ARM cores on normal RX and TX packet
> done events (see 6261c4c8). So workaround 6261c4c8 is reverted here to
> support the OV5642, and the "fsl,err006687-workaround-present" boolean
> also must be removed. The result is that the CPUidle driver will no longer
> allow entering the deep idle states on the sabrelite.

For me it sounds like a candidate of its own separate change.

> 
> Signed-off-by: Steve Longerbeam 
> ---

[snip]

>   {
>   pinctrl-names = "default";
>   pinctrl-0 = <_audmux>;
> @@ -271,9 +298,6 @@
>   txd1-skew-ps = <0>;
>   txd2-skew-ps = <0>;
>   txd3-skew-ps = <0>;
> - interrupts-extended = < 6 IRQ_TYPE_LEVEL_HIGH>,
> -   < 0 119 IRQ_TYPE_LEVEL_HIGH>;

Like you say in the commit message this is a partial revert of 6261c4c8f13e
("ARM: dts: imx6qdl-sabrelite: use GPIO_6 for FEC interrupt.")

> - fsl,err006687-workaround-present;

This is a partial revert of a28eeb43ee57 ("ARM: dts: imx6: tag boards that
have the HW workaround for ERR006687").

The change should be split and reviewed separately in my opinion, also
cc Gary Bisson  for SabreLite changes.

>   status = "okay";
>  };
>  
> @@ -302,6 +326,52 @@
>   pinctrl-names = "default";
>   pinctrl-0 = <_i2c2>;
>   status = "okay";
> +
> + camera: ov5642@42 {
> + compatible = "ovti,ov5642";
> + pinctrl-names = "default";
> + pinctrl-0 = <_ov5642>;
> + clocks = < IMX6QDL_CLK_CKO2>;
> + clock-names = "xclk";
> + reg = <0x42>;
> + xclk = <2400>;
> + reset-gpios = < 8 GPIO_ACTIVE_LOW>;
> + pwdn-gpios = < 6 GPIO_ACTIVE_HIGH>;
> + gp-gpios = < 16 GPIO_ACTIVE_HIGH>;
> +
> + port {
> + ov5642_to_ipu1_csi0_mux: endpoint {
> + remote-endpoint = 
> <_csi0_mux_from_parallel_sensor>;
> + bus-width = <8>;
> + hsync-active = <1>;
> + vsync-active = <1>;
> + };
> + };
> + };
> +
> + mipi_camera: ov5640@40 {

Please reorder device nodes by address value, also according to ePAPR
node names should be generic, labels can be specific:

ov5640: camera@40 {
...
};

ov5642: camera@42 {
...
};

> + compatible = "ovti,ov5640_mipi";
> + pinctrl-names = "default";
> + pinctrl-0 = <_ov5640>;
> + clocks = <_xclk>;
> + clock-names = "xclk";
> + reg = <0x40>;
> + xclk = <2200>;
> + reset-gpios = < 5 GPIO_ACTIVE_LOW>; /* NANDF_D5 */
> + pwdn-gpios = < 9 GPIO_ACTIVE_HIGH>; /* NANDF_WP_B */
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ov5640_to_mipi_csi: endpoint@1 {
> + reg = <1>;
> + remote-endpoint = <_csi_from_mipi_sensor>;
> + data-lanes = <0 1>;
> + clock-lanes = <2>;
> + };
> + };
> + };
>  };
>  
>   {
> @@ -374,7 +444,6 @@
>   MX6QDL_PAD_RGMII_RX_CTL__RGMII_RX_CTL   0x1b030
>   /* Phy reset */
>   MX6QDL_PAD_EIM_D23__GPIO3_IO23  0x000b0
> - MX6QDL_PAD_GPIO_6__ENET_IRQ 0x000b1

Yep.

>   >;
>   };
>  
> @@ -449,6 +518,39 @@
>   >;
>   };
>  
> + pinctrl_ov5642: ov5642grp {
> + fsl,pins = <
> + MX6QDL_PAD_SD1_DAT0__GPIO1_IO16 0x8000
> + MX6QDL_PAD_GPIO_6__GPIO1_IO06   0x8000
> + MX6QDL_PAD_GPIO_8__GPIO1_IO08   0x8000
> + MX6QDL_PAD_GPIO_3__CCM_CLKO20x8000
> + >;
> + };
> +
> + pinctrl_ipu1_csi0: ipu1grp-csi0 {

Please rename node name to ipu1csi0grp.

> + fsl,pins = <
> +   

Re: [PATCH v2 1/8] i2c-mux: add common core data for every mux instance

[Vladimir Zapolskiy]

Hi Peter,

On 24.03.2016 13:05, Peter Rosin wrote:
> Hi Vladimir,
> 
> On 2016-03-24 10:50, Vladimir Zapolskiy wrote:
>> Hi Peter,
>>
>> On 05.01.2016 17:57, Peter Rosin wrote:
>>> From: Peter Rosin <p...@axentia.se>
>>>
>>> The initial core mux structure starts off small with only the parent
>>> adapter pointer, which all muxes have, and a priv pointer for mux
>>> driver private data.
>>>
>>> Add i2c_mux_alloc function to unify the creation of a mux.
>>>
>>> Where appropriate, pass around the mux core structure instead of the
>>> parent adapter or the driver private data.
>>>
>>> Remove the parent adapter pointer from the driver private data for all
>>> mux drivers.
>>>
>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>>
>> is it still under review? If yes, please find one question from me below :)
> 
> Yes, the series is still under review/testing, with an update planned in a
> week or so.
> 
>> [snip]
>>
>>> @@ -196,21 +195,21 @@ static int i2c_arbitrator_probe(struct 
>>> platform_device *pdev)
>>> dev_err(dev, "Cannot parse i2c-parent\n");
>>> return -EINVAL;
>>> }
>>> -   arb->parent = of_get_i2c_adapter_by_node(parent_np);
>>> +   muxc->parent = of_find_i2c_adapter_by_node(parent_np);
>>
>> why do you prefer here to use "unlocked" version of API?
>>
>> Foe example would it be safe/possible to unload an I2C bus device driver
>> module or unbind I2C device itself in runtime?
> 
> I think you ask why I change from of_get_i2c_... to of_find_i2c_..., and that
> change was not intentional. It was the result of a bad merge during an early
> rebase.
> 
> Does that cover it?
> 

Yep, thank you for clarification, please account this in v3.

I'll try to find some time to review the whole changeset carefully,
in fact I briefly reviewed it two months ago, but I didn't find
anything obviously wrong that time.

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/8] i2c-mux: add common core data for every mux instance

[Vladimir Zapolskiy]

Hi Peter,

On 05.01.2016 17:57, Peter Rosin wrote:
> From: Peter Rosin 
> 
> The initial core mux structure starts off small with only the parent
> adapter pointer, which all muxes have, and a priv pointer for mux
> driver private data.
> 
> Add i2c_mux_alloc function to unify the creation of a mux.
> 
> Where appropriate, pass around the mux core structure instead of the
> parent adapter or the driver private data.
> 
> Remove the parent adapter pointer from the driver private data for all
> mux drivers.
> 
> Signed-off-by: Peter Rosin 

is it still under review? If yes, please find one question from me below :)

[snip]

> @@ -196,21 +195,21 @@ static int i2c_arbitrator_probe(struct platform_device 
> *pdev)
>   dev_err(dev, "Cannot parse i2c-parent\n");
>   return -EINVAL;
>   }
> - arb->parent = of_get_i2c_adapter_by_node(parent_np);
> + muxc->parent = of_find_i2c_adapter_by_node(parent_np);

why do you prefer here to use "unlocked" version of API?

Foe example would it be safe/possible to unload an I2C bus device driver
module or unbind I2C device itself in runtime?

>   of_node_put(parent_np);
> - if (!arb->parent) {
> + if (!muxc->parent) {
>   dev_err(dev, "Cannot find parent bus\n");
>   return -EPROBE_DEFER;
>   }
>  
>   /* Actually add the mux adapter */
> - arb->child = i2c_add_mux_adapter(arb->parent, dev, arb, 0, 0, 0,
> + arb->child = i2c_add_mux_adapter(muxc, dev, arb, 0, 0, 0,
>i2c_arbitrator_select,
>i2c_arbitrator_deselect);
>   if (!arb->child) {
>   dev_err(dev, "Failed to add adapter\n");
>   ret = -ENODEV;
> - i2c_put_adapter(arb->parent);
> + i2c_put_adapter(muxc->parent);
>   }
>  
>   return ret;

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: rc: remove unneeded mutex in rc_register_device

[Vladimir Zapolskiy]

On 16.03.2016 20:53, Heiner Kallweit wrote:
> Access to dev->initialized is atomic, therefore we don't have to
> protect it with a mutex.

Mutexes are used to split the code to mutually exclusive execution blocks,
so not arguing about the apparently correct change itself I want to
emphasize that the given explanation of the change in the commit message is
wrong. Atomic access does not cancel a specific care about execution
ordering.

Indirectly it applies to ("rc-core: allow calling rc_open with device not
initialized"), where "initialized" bool property was changed to atomic_t
type --- this (sub-)change is just useless.

Please grasp the topic and reword the commit message.

> Signed-off-by: Heiner Kallweit 
> ---
>  drivers/media/rc/rc-main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 4e9bbe7..68541b1 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1492,9 +1492,7 @@ int rc_register_device(struct rc_dev *dev)
>   }
>  
>   /* Allow the RC sysfs nodes to be accessible */
> - mutex_lock(>lock);
>   atomic_set(>initialized, 1);
> - mutex_unlock(>lock);
>  
>   IR_dprintk(1, "Registered rc%u (driver: %s, remote: %s, mode %s)\n",
>  dev->minor,
> 

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: gstreamer and v4l2

[Vladimir Zapolskiy]

On 15.03.2016 12:28, Antonio Ospite wrote:
> On Tue, 15 Mar 2016 09:10:59 +0200
> Ran Shalit  wrote:
> 
>> Hello,
>>
>> This is a bit offtopic, so I will understand if you rather not discuss 
>> that...
>>
>> I am trying to use gstreamer with v4l2 vivi device,
>> I first check the capabilities with
>>
>> gst-launch-1.0 --gst-debug=v4l2src:5 v4l2src device="/dev/video0" !
>> fakesink 2>&1
>>
>> and it gives many capabilities such as the following:
>>
>> video/x-raw-yuv, format=(string)YUY2, framerate=(fraction)[1/1000,
>> 1000/1], width=(int) 640, height=(int)180, interlaced=(boolean)true
>>
> 
> A cleaner way to enumerate capabilities of a video device in GStreamer
> is like that:
> 
>   gst-device-monitor-1.0 Video
> 
> on Debian distributions gst-device-monitor-1.0 is in the
> gstreamer1.0-plugins-base-apps package.

No, you add some redundant GStreamer app instead of using just
GStreamer framework internals, this is not a cleaner way.

>> So I tried to run as following:
>>
>> gst-launch-0.10 v4l2src device="/dev/video0" !
>> video/x-raw,width=640,height=180,framerate=30 ! autovideosink

According to the received caps use

* video/x-raw-yuv
* framerate=30/1
* and start from fakesink

>>
>> But it keeps giving me auto negotiation error -4.
>> Trying to give other values did not help neither.
> 
> BTW, the need for videoconvert is more likely because of the pixelformat
> rather than the frame dimensions.
> 

--
With best wishes,
Vladimir

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] media: i2c/adp1653: fix check of devm_gpiod_get() error code

[Vladimir Zapolskiy]

The devm_gpiod_get() function returns either a valid pointer to
struct gpio_desc or ERR_PTR() error value, check for NULL is bogus.

Signed-off-by: Vladimir Zapolskiy <v...@mleia.com>
---
 drivers/media/i2c/adp1653.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index fb7ed73..9e1731c 100644
--- a/drivers/media/i2c/adp1653.c
+++ b/drivers/media/i2c/adp1653.c
@@ -466,9 +466,9 @@ static int adp1653_of_init(struct i2c_client *client,
of_node_put(child);
 
pd->enable_gpio = devm_gpiod_get(>dev, "enable", GPIOD_OUT_LOW);
-   if (!pd->enable_gpio) {
+   if (IS_ERR(pd->enable_gpio)) {
dev_err(>dev, "Error getting GPIO\n");
-   return -EINVAL;
+   return PTR_ERR(pd->enable_gpio);
}
 
return 0;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] v4l2-ctrls: remove unclaimed v4l2_ctrl_add_ctrl() interface

[Vladimir Zapolskiy]

v4l2_ctrl_add_ctrl() interface has no users since its introduction in
commit 0996517cf8ea ("V4L/DVB: v4l2: Add new control handling framework")
and its functionality is covered by v4l2_ctrl_new() and derivative
interfaces, so it is safe to remove the interface from the kernel.

Signed-off-by: Vladimir Zapolskiy <v...@mleia.com>
---
 Documentation/video4linux/v4l2-controls.txt |  1 -
 drivers/media/v4l2-core/v4l2-ctrls.c| 16 
 include/media/v4l2-ctrls.h  | 12 
 3 files changed, 29 deletions(-)

diff --git a/Documentation/video4linux/v4l2-controls.txt 
b/Documentation/video4linux/v4l2-controls.txt
index 5517db6..5e759ca 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -647,7 +647,6 @@ Or you can add specific controls to a handler:
volume = v4l2_ctrl_new_std(_ctrl_handler, , 
V4L2_CID_AUDIO_VOLUME, ...);
v4l2_ctrl_new_std(_ctrl_handler, , V4L2_CID_BRIGHTNESS, ...);
v4l2_ctrl_new_std(_ctrl_handler, , V4L2_CID_CONTRAST, ...);
-   v4l2_ctrl_add_ctrl(_ctrl_handler, volume);
 
 What you should not do is make two identical controls for two handlers.
 For example:
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
b/drivers/media/v4l2-core/v4l2-ctrls.c
index b6b7dcc..3b14485 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2198,22 +2198,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct 
v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
 
-/* Add a control from another handler to this handler */
-struct v4l2_ctrl *v4l2_ctrl_add_ctrl(struct v4l2_ctrl_handler *hdl,
- struct v4l2_ctrl *ctrl)
-{
-   if (hdl == NULL || hdl->error)
-   return NULL;
-   if (ctrl == NULL) {
-   handler_set_err(hdl, -EINVAL);
-   return NULL;
-   }
-   if (ctrl->handler == hdl)
-   return ctrl;
-   return handler_new_ref(hdl, ctrl) ? NULL : ctrl;
-}
-EXPORT_SYMBOL(v4l2_ctrl_add_ctrl);
-
 /* Add the controls from another handler to our own. */
 int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
  struct v4l2_ctrl_handler *add,
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index da6fe98..0bc9b35 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -535,18 +535,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct 
v4l2_ctrl_handler *hdl,
u32 id, u8 max, u8 def, const s64 *qmenu_int);
 
 /**
- * v4l2_ctrl_add_ctrl() - Add a control from another handler to this handler.
- * @hdl:   The control handler.
- * @ctrl:  The control to add.
- *
- * It will return NULL if it was unable to add the control reference.
- * If the control already belonged to the handler, then it will do
- * nothing and just return @ctrl.
- */
-struct v4l2_ctrl *v4l2_ctrl_add_ctrl(struct v4l2_ctrl_handler *hdl,
- struct v4l2_ctrl *ctrl);
-
-/**
  * v4l2_ctrl_add_handler() - Add all controls from handler @add to
  * handler @hdl.
  * @hdl:   The control handler.
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] genalloc: add name arg to gen_pool_get() and devm_gen_pool_create()

[Vladimir Zapolskiy]

This change modifies gen_pool_get() and devm_gen_pool_create() client
interfaces adding one more argument name of a gen_pool object.

Due to implementation gen_pool_get() is capable to retrieve only one
gen_pool associated with a device even if multiple gen_pools are created,
fortunately right at the moment it is sufficient for the clients, hence
provide NULL as a valid argument on both producer devm_gen_pool_create()
and consumer gen_pool_get() sides.

Because only one created gen_pool per device is addressable, explicitly
add a restriction to devm_gen_pool_create() to create only one gen_pool
per device, this implies two possible error codes returned by the
function, account it on client side (only misc/sram).  This completes
client side changes related to genalloc updates.

Signed-off-by: Vladimir Zapolskiy vladimir_zapols...@mentor.com
Cc: Philipp Zabel p.za...@pengutronix.de
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Russell King li...@arm.linux.org.uk
Cc: Nicolas Ferre nicolas.fe...@atmel.com
Cc: Alexandre Belloni alexandre.bell...@free-electrons.com
Cc: Jean-Christophe Plagniol-Villard plagn...@jcrosoft.com
Cc: Shawn Guo shawn...@kernel.org
Cc: Sascha Hauer ker...@pengutronix.de
Cc: Mauro Carvalho Chehab mche...@osg.samsung.com
Cc: Arnd Bergmann a...@arndb.de
Cc: Dinh Nguyen dingu...@opensource.altera.com
---

Changes from v1 to v2:
* Added the same change in gen_pool_get() argument list
  to account a recently added client in arm/mach-socfpga/pm.c

 arch/arm/mach-at91/pm.c   |  2 +-
 arch/arm/mach-imx/pm-imx5.c   |  2 +-
 arch/arm/mach-imx/pm-imx6.c   |  2 +-
 arch/arm/mach-socfpga/pm.c|  2 +-
 drivers/media/platform/coda/coda-common.c |  2 +-
 drivers/misc/sram.c   |  8 ++---
 include/linux/genalloc.h  |  4 +--
 lib/genalloc.c| 49 ++-
 8 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index e24df77..e65e9db 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -369,7 +369,7 @@ static void __init at91_pm_sram_init(void)
return;
}
 
-   sram_pool = gen_pool_get(pdev-dev);
+   sram_pool = gen_pool_get(pdev-dev, NULL);
if (!sram_pool) {
pr_warn(%s: sram pool unavailable!\n, __func__);
return;
diff --git a/arch/arm/mach-imx/pm-imx5.c b/arch/arm/mach-imx/pm-imx5.c
index 1885676..532d4b0 100644
--- a/arch/arm/mach-imx/pm-imx5.c
+++ b/arch/arm/mach-imx/pm-imx5.c
@@ -297,7 +297,7 @@ static int __init imx_suspend_alloc_ocram(
goto put_node;
}
 
-   ocram_pool = gen_pool_get(pdev-dev);
+   ocram_pool = gen_pool_get(pdev-dev, NULL);
if (!ocram_pool) {
pr_warn(%s: ocram pool unavailable!\n, __func__);
ret = -ENODEV;
diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index 93ecf55..8ff8fc0 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -451,7 +451,7 @@ static int __init imx6q_suspend_init(const struct 
imx6_pm_socdata *socdata)
goto put_node;
}
 
-   ocram_pool = gen_pool_get(pdev-dev);
+   ocram_pool = gen_pool_get(pdev-dev, NULL);
if (!ocram_pool) {
pr_warn(%s: ocram pool unavailable!\n, __func__);
ret = -ENODEV;
diff --git a/arch/arm/mach-socfpga/pm.c b/arch/arm/mach-socfpga/pm.c
index 6a4199f..c378ab0 100644
--- a/arch/arm/mach-socfpga/pm.c
+++ b/arch/arm/mach-socfpga/pm.c
@@ -56,7 +56,7 @@ static int socfpga_setup_ocram_self_refresh(void)
goto put_node;
}
 
-   ocram_pool = gen_pool_get(pdev-dev);
+   ocram_pool = gen_pool_get(pdev-dev, NULL);
if (!ocram_pool) {
pr_warn(%s: ocram pool unavailable!\n, __func__);
ret = -ENODEV;
diff --git a/drivers/media/platform/coda/coda-common.c 
b/drivers/media/platform/coda/coda-common.c
index 58f6548..284ac4c 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2157,7 +2157,7 @@ static int coda_probe(struct platform_device *pdev)
/* Get IRAM pool from device tree or platform data */
pool = of_gen_pool_get(np, iram, 0);
if (!pool  pdata)
-   pool = gen_pool_get(pdata-iram_dev);
+   pool = gen_pool_get(pdata-iram_dev, NULL);
if (!pool) {
dev_err(pdev-dev, iram pool not available\n);
return -ENOMEM;
diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 15c33cc..431e1dd 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -186,10 +186,10 @@ static int sram_probe(struct platform_device *pdev)
if (IS_ERR(sram-virt_base))
return PTR_ERR(sram-virt_base);
 
-   sram-pool = devm_gen_pool_create(sram-dev

[PATCH v2 0/2] genalloc: add support of multiple gen_pools per device

[Vladimir Zapolskiy]

This is continuation of development of a proposed earlier
functionality to have multiple gen_pools per device, see
https://lkml.org/lkml/2015/6/8/588 discussion.

The main difference with the originally sent change is that instead of
adding two new interfaces gen_pool_get_named() and
devm_gen_pool_create_named(), it is considered better to change
signatures of the existing exported functions, because
devm_gen_pool_create() may be misused -- it is permitted to create
several gen_pool objects assigned to the same device and
gen_pool_get() returns only the first found object from device devres
data due to missing devres_match helper, fortunately currently
existing genalloc clients are not tainted -- at the moment the sole
user of devm_gen_pool_create() is drivers/misc/sram.c.

The change is split into two parts:
- update gen_pool_get() and devm_gen_pool_create() interfaces
  on client side
- implement proper handling of new gen_pool name argument
  and extend of_gen_pool_get() functionality based on it

Both changes are backward compatible in sense of functionality.

The lib/genalloc.c changes are based on two patches from -mm tree:
- 
http://ozlabs.org/~akpm/mmots/broken-out/genalloc-rename-dev_get_gen_pool-to-gen_pool_get.patch
- 
http://ozlabs.org/~akpm/mmots/broken-out/genalloc-rename-of_get_named_gen_pool-to-of_gen_pool_get.patch

The client side changes are based on linux-next/master branch.

Changes from v1 to v2:
- addressed Andrew's valuable review comments, namely
-- devm_gen_pool_create() is aware of attempts to register ambiguously
   addressed gen_pool objects and now it returns ERR_PTR() on error,
-- memory management to store gen_pool name literal is done on
   genalloc side,
-- replaced -1 with NUMA_NO_NODE in nid argument description,
- minor updates in of_gen_pool_get() change to respect OF_DYNAMIC
- instead of adding two new functions the existing functions
  gen_pool_get() (at91, imx5m, imx6 and CODA driver clients) and
  devm_gen_pool_create() (sram client) are updated.

Vladimir Zapolskiy (2):
  genalloc: add name arg to gen_pool_get() and devm_gen_pool_create()
  genalloc: add support of multiple gen_pools per device

 arch/arm/mach-at91/pm.c   |  2 +-
 arch/arm/mach-imx/pm-imx5.c   |  2 +-
 arch/arm/mach-imx/pm-imx6.c   |  2 +-
 drivers/media/platform/coda/coda-common.c |  2 +-
 drivers/misc/sram.c   |  8 +--
 include/linux/genalloc.h  |  6 +-
 lib/genalloc.c| 99 +++
 7 files changed, 85 insertions(+), 36 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] genalloc: add name arg to gen_pool_get() and devm_gen_pool_create()

[Vladimir Zapolskiy]

This change modifies gen_pool_get() and devm_gen_pool_create()
client interfaces adding one more argument name of a gen_pool
object.

Due to implementation gen_pool_get() is capable to retrieve only one
gen_pool associated with a device even if multiple gen_pools are
created, fortunately right at the moment it is sufficient for the
clients, hence provide NULL as a valid argument on both producer
devm_gen_pool_create() and consumer gen_pool_get() sides.

Because only one created gen_pool per device is addressable,
explicitly add a restriction to devm_gen_pool_create() to create only
one gen_pool per device, this implies two possible error codes
returned by the function, account it on client side (only misc/sram).
This completes client side changes related to genalloc updates.

Signed-off-by: Vladimir Zapolskiy vladimir_zapols...@mentor.com
---
 arch/arm/mach-at91/pm.c   |  2 +-
 arch/arm/mach-imx/pm-imx5.c   |  2 +-
 arch/arm/mach-imx/pm-imx6.c   |  2 +-
 drivers/media/platform/coda/coda-common.c |  2 +-
 drivers/misc/sram.c   |  8 ++---
 include/linux/genalloc.h  |  4 +--
 lib/genalloc.c| 49 ++-
 7 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index e24df77..e65e9db 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -369,7 +369,7 @@ static void __init at91_pm_sram_init(void)
return;
}
 
-   sram_pool = gen_pool_get(pdev-dev);
+   sram_pool = gen_pool_get(pdev-dev, NULL);
if (!sram_pool) {
pr_warn(%s: sram pool unavailable!\n, __func__);
return;
diff --git a/arch/arm/mach-imx/pm-imx5.c b/arch/arm/mach-imx/pm-imx5.c
index 1885676..532d4b0 100644
--- a/arch/arm/mach-imx/pm-imx5.c
+++ b/arch/arm/mach-imx/pm-imx5.c
@@ -297,7 +297,7 @@ static int __init imx_suspend_alloc_ocram(
goto put_node;
}
 
-   ocram_pool = gen_pool_get(pdev-dev);
+   ocram_pool = gen_pool_get(pdev-dev, NULL);
if (!ocram_pool) {
pr_warn(%s: ocram pool unavailable!\n, __func__);
ret = -ENODEV;
diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index 93ecf55..8ff8fc0 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -451,7 +451,7 @@ static int __init imx6q_suspend_init(const struct 
imx6_pm_socdata *socdata)
goto put_node;
}
 
-   ocram_pool = gen_pool_get(pdev-dev);
+   ocram_pool = gen_pool_get(pdev-dev, NULL);
if (!ocram_pool) {
pr_warn(%s: ocram pool unavailable!\n, __func__);
ret = -ENODEV;
diff --git a/drivers/media/platform/coda/coda-common.c 
b/drivers/media/platform/coda/coda-common.c
index 58f6548..284ac4c 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2157,7 +2157,7 @@ static int coda_probe(struct platform_device *pdev)
/* Get IRAM pool from device tree or platform data */
pool = of_gen_pool_get(np, iram, 0);
if (!pool  pdata)
-   pool = gen_pool_get(pdata-iram_dev);
+   pool = gen_pool_get(pdata-iram_dev, NULL);
if (!pool) {
dev_err(pdev-dev, iram pool not available\n);
return -ENOMEM;
diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 15c33cc..431e1dd 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -186,10 +186,10 @@ static int sram_probe(struct platform_device *pdev)
if (IS_ERR(sram-virt_base))
return PTR_ERR(sram-virt_base);
 
-   sram-pool = devm_gen_pool_create(sram-dev,
- ilog2(SRAM_GRANULARITY), -1);
-   if (!sram-pool)
-   return -ENOMEM;
+   sram-pool = devm_gen_pool_create(sram-dev, ilog2(SRAM_GRANULARITY),
+ NUMA_NO_NODE, NULL);
+   if (IS_ERR(sram-pool))
+   return PTR_ERR(sram-pool);
 
ret = sram_reserve_regions(sram, res);
if (ret)
diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 5383bb1..6afa65e 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -118,8 +118,8 @@ extern unsigned long gen_pool_best_fit(unsigned long *map, 
unsigned long size,
unsigned long start, unsigned int nr, void *data);
 
 extern struct gen_pool *devm_gen_pool_create(struct device *dev,
-   int min_alloc_order, int nid);
-extern struct gen_pool *gen_pool_get(struct device *dev);
+   int min_alloc_order, int nid, const char *name);
+extern struct gen_pool *gen_pool_get(struct device *dev, const char *name);
 
 bool addr_in_gen_pool(struct gen_pool *pool, unsigned long start,
size_t size);
diff --git a/lib/genalloc.c b/lib/genalloc.c
index

[PATCH 2/2] genalloc: rename of_get_named_gen_pool() to of_gen_pool_get()

[Vladimir Zapolskiy]

To be consistent with other kernel interface namings, rename
of_get_named_gen_pool() to of_gen_pool_get(). In the original
function name _named suffix references to a device tree property,
which contains a phandle to a device and the corresponding
device driver is assumed to register a gen_pool object.

Due to a weak relation and to avoid any confusion (e.g. in future
possible scenario if gen_pool objects are named) the suffix is
removed.

Signed-off-by: Vladimir Zapolskiy vladimir_zapols...@mentor.com
---
 drivers/dma/mmp_tdma.c| 2 +-
 drivers/media/platform/coda/coda-common.c | 2 +-
 include/linux/genalloc.h  | 4 ++--
 lib/genalloc.c| 6 +++---
 sound/core/memalloc.c | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
index 449e785..e683761 100644
--- a/drivers/dma/mmp_tdma.c
+++ b/drivers/dma/mmp_tdma.c
@@ -657,7 +657,7 @@ static int mmp_tdma_probe(struct platform_device *pdev)
INIT_LIST_HEAD(tdev-device.channels);
 
if (pdev-dev.of_node)
-   pool = of_get_named_gen_pool(pdev-dev.of_node, asram, 0);
+   pool = of_gen_pool_get(pdev-dev.of_node, asram, 0);
else
pool = sram_get_gpool(asram);
if (!pool) {
diff --git a/drivers/media/platform/coda/coda-common.c 
b/drivers/media/platform/coda/coda-common.c
index 6e640c0..58f6548 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2155,7 +2155,7 @@ static int coda_probe(struct platform_device *pdev)
}
 
/* Get IRAM pool from device tree or platform data */
-   pool = of_get_named_gen_pool(np, iram, 0);
+   pool = of_gen_pool_get(np, iram, 0);
if (!pool  pdata)
pool = gen_pool_get(pdata-iram_dev);
if (!pool) {
diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 015d170..5383bb1 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -125,10 +125,10 @@ bool addr_in_gen_pool(struct gen_pool *pool, unsigned 
long start,
size_t size);
 
 #ifdef CONFIG_OF
-extern struct gen_pool *of_get_named_gen_pool(struct device_node *np,
+extern struct gen_pool *of_gen_pool_get(struct device_node *np,
const char *propname, int index);
 #else
-static inline struct gen_pool *of_get_named_gen_pool(struct device_node *np,
+static inline struct gen_pool *of_gen_pool_get(struct device_node *np,
const char *propname, int index)
 {
return NULL;
diff --git a/lib/genalloc.c b/lib/genalloc.c
index 948e92c..daf0afb 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -620,7 +620,7 @@ EXPORT_SYMBOL_GPL(gen_pool_get);
 
 #ifdef CONFIG_OF
 /**
- * of_get_named_gen_pool - find a pool by phandle property
+ * of_gen_pool_get - find a pool by phandle property
  * @np: device node
  * @propname: property name containing phandle(s)
  * @index: index into the phandle array
@@ -629,7 +629,7 @@ EXPORT_SYMBOL_GPL(gen_pool_get);
  * address of the device tree node pointed at by the phandle property,
  * or NULL if not found.
  */
-struct gen_pool *of_get_named_gen_pool(struct device_node *np,
+struct gen_pool *of_gen_pool_get(struct device_node *np,
const char *propname, int index)
 {
struct platform_device *pdev;
@@ -644,5 +644,5 @@ struct gen_pool *of_get_named_gen_pool(struct device_node 
*np,
return NULL;
return gen_pool_get(pdev-dev);
 }
-EXPORT_SYMBOL_GPL(of_get_named_gen_pool);
+EXPORT_SYMBOL_GPL(of_gen_pool_get);
 #endif /* CONFIG_OF */
diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
index 082509e..f05cb6a 100644
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -124,7 +124,7 @@ static void snd_malloc_dev_iram(struct snd_dma_buffer 
*dmab, size_t size)
dmab-addr = 0;
 
if (dev-of_node)
-   pool = of_get_named_gen_pool(dev-of_node, iram, 0);
+   pool = of_gen_pool_get(dev-of_node, iram, 0);
 
if (!pool)
return;
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] genalloc: rename dev_get_gen_pool() to gen_pool_get()

[Vladimir Zapolskiy]

To be consistent with other genalloc interface namings, rename
dev_get_gen_pool() to gen_pool_get(). The original omitted dev_
prefix is removed, since it points to argument type of the function,
and so it does not bring any useful information.

Signed-off-by: Vladimir Zapolskiy vladimir_zapols...@mentor.com
---
 arch/arm/mach-at91/pm.c   | 2 +-
 arch/arm/mach-imx/pm-imx5.c   | 2 +-
 arch/arm/mach-imx/pm-imx6.c   | 2 +-
 drivers/media/platform/coda/coda-common.c | 2 +-
 include/linux/genalloc.h  | 2 +-
 lib/genalloc.c| 8 
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 1e18476..e24df77 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -369,7 +369,7 @@ static void __init at91_pm_sram_init(void)
return;
}
 
-   sram_pool = dev_get_gen_pool(pdev-dev);
+   sram_pool = gen_pool_get(pdev-dev);
if (!sram_pool) {
pr_warn(%s: sram pool unavailable!\n, __func__);
return;
diff --git a/arch/arm/mach-imx/pm-imx5.c b/arch/arm/mach-imx/pm-imx5.c
index 0309ccd..1885676 100644
--- a/arch/arm/mach-imx/pm-imx5.c
+++ b/arch/arm/mach-imx/pm-imx5.c
@@ -297,7 +297,7 @@ static int __init imx_suspend_alloc_ocram(
goto put_node;
}
 
-   ocram_pool = dev_get_gen_pool(pdev-dev);
+   ocram_pool = gen_pool_get(pdev-dev);
if (!ocram_pool) {
pr_warn(%s: ocram pool unavailable!\n, __func__);
ret = -ENODEV;
diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index b01650d..93ecf55 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -451,7 +451,7 @@ static int __init imx6q_suspend_init(const struct 
imx6_pm_socdata *socdata)
goto put_node;
}
 
-   ocram_pool = dev_get_gen_pool(pdev-dev);
+   ocram_pool = gen_pool_get(pdev-dev);
if (!ocram_pool) {
pr_warn(%s: ocram pool unavailable!\n, __func__);
ret = -ENODEV;
diff --git a/drivers/media/platform/coda/coda-common.c 
b/drivers/media/platform/coda/coda-common.c
index 6d6e0ca..6e640c0 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2157,7 +2157,7 @@ static int coda_probe(struct platform_device *pdev)
/* Get IRAM pool from device tree or platform data */
pool = of_get_named_gen_pool(np, iram, 0);
if (!pool  pdata)
-   pool = dev_get_gen_pool(pdata-iram_dev);
+   pool = gen_pool_get(pdata-iram_dev);
if (!pool) {
dev_err(pdev-dev, iram pool not available\n);
return -ENOMEM;
diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 1ccaab4..015d170 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -119,7 +119,7 @@ extern unsigned long gen_pool_best_fit(unsigned long *map, 
unsigned long size,
 
 extern struct gen_pool *devm_gen_pool_create(struct device *dev,
int min_alloc_order, int nid);
-extern struct gen_pool *dev_get_gen_pool(struct device *dev);
+extern struct gen_pool *gen_pool_get(struct device *dev);
 
 bool addr_in_gen_pool(struct gen_pool *pool, unsigned long start,
size_t size);
diff --git a/lib/genalloc.c b/lib/genalloc.c
index d214866..948e92c 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -602,12 +602,12 @@ struct gen_pool *devm_gen_pool_create(struct device *dev, 
int min_alloc_order,
 EXPORT_SYMBOL(devm_gen_pool_create);
 
 /**
- * dev_get_gen_pool - Obtain the gen_pool (if any) for a device
+ * gen_pool_get - Obtain the gen_pool (if any) for a device
  * @dev: device to retrieve the gen_pool from
  *
  * Returns the gen_pool for the device if one is present, or NULL.
  */
-struct gen_pool *dev_get_gen_pool(struct device *dev)
+struct gen_pool *gen_pool_get(struct device *dev)
 {
struct gen_pool **p = devres_find(dev, devm_gen_pool_release, NULL,
NULL);
@@ -616,7 +616,7 @@ struct gen_pool *dev_get_gen_pool(struct device *dev)
return NULL;
return *p;
 }
-EXPORT_SYMBOL_GPL(dev_get_gen_pool);
+EXPORT_SYMBOL_GPL(gen_pool_get);
 
 #ifdef CONFIG_OF
 /**
@@ -642,7 +642,7 @@ struct gen_pool *of_get_named_gen_pool(struct device_node 
*np,
of_node_put(np_pool);
if (!pdev)
return NULL;
-   return dev_get_gen_pool(pdev-dev);
+   return gen_pool_get(pdev-dev);
 }
 EXPORT_SYMBOL_GPL(of_get_named_gen_pool);
 #endif /* CONFIG_OF */
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] genalloc: rename dev_get_gen_pool() and of_get_named_gen_pool()

[Vladimir Zapolskiy]

Trivial nonfunctional change initially based on discussion
https://lkml.org/lkml/2015/6/8/588

Worth to mention that instead of the assumed new name
dev_gen_pool_get(), this change attempts to be more close to other
in-kernel interfaces and new function name is just gen_pool_get().

The change is based and tested on linux-next.

Vladimir Zapolskiy (2):
  genalloc: rename dev_get_gen_pool() to gen_pool_get()
  genalloc: rename of_get_named_gen_pool() to of_gen_pool_get()

 arch/arm/mach-at91/pm.c   |  2 +-
 arch/arm/mach-imx/pm-imx5.c   |  2 +-
 arch/arm/mach-imx/pm-imx6.c   |  2 +-
 drivers/dma/mmp_tdma.c|  2 +-
 drivers/media/platform/coda/coda-common.c |  4 ++--
 include/linux/genalloc.h  |  6 +++---
 lib/genalloc.c| 14 +++---
 sound/core/memalloc.c |  2 +-
 8 files changed, 17 insertions(+), 17 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] staging: imx-drm: document internal HDMI I2C master controller DT binding

[Vladimir Zapolskiy]

Hi Andy,

thank you for joining the discussion.

On 02.12.2014 08:36, Andy Yan wrote:
 Hi Vladimir:
 
   I am working on convert imx-hdmi to dw_hdmi now:
   https://lkml.org/lkml/2014/12/1/190
   I also have a plan to use  the internal HDMI I2C master under the I2c 
 framework,
 and I also have a patch to do this work. So glad to see your work.
   Please also Cc meand@rock-chips.com and zubair.kakak...@imgtec.com,
 maybe Zubair also have interests on your future patch.
 On 2014年12月02日 00:22, Philipp Zabel wrote:
 Hi Vladimir,

 [Added Andy Yan to Cc:, because imx-hdmi-dw-hdmi]

 Am Montag, den 01.12.2014, 17:39 +0200 schrieb Vladimir Zapolskiy:
 On 01.12.2014 17:11, Philipp Zabel wrote:
 Am Montag, den 01.12.2014, 16:54 +0200 schrieb Vladimir Zapolskiy:
 Hi Philipp and Shawn,

 On 15.11.2014 19:49, Vladimir Zapolskiy wrote:
 Provide information about how to bind internal iMX6Q/DL HDMI DDC I2C
 master controller. The property is set as optional one, because iMX6
 HDMI DDC bus may be represented by one of general purpose I2C busses
 found on SoC.

 Signed-off-by: Vladimir Zapolskiy vladimir_zapols...@mentor.com
 Cc: Wolfram Sang w...@the-dreams.de
 Cc: Philipp Zabel p.za...@pengutronix.de
 Cc: Shawn Guo shawn@linaro.org
 Cc: devicet...@vger.kernel.org
 Cc: linux-media@vger.kernel.org
 Cc: linux-arm-ker...@lists.infradead.org
 Cc: linux-...@vger.kernel.org
 ---
   Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt |   10 
 +-
   1 file changed, 9 insertions(+), 1 deletion(-)

 diff --git a/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt 
 b/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
 index 1b756cf..43c8924 100644
 --- a/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
 +++ b/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
 @@ -10,6 +10,8 @@ Required properties:
- #address-cells : should be 1
- #size-cells : should be 0
- compatible : should be fsl,imx6q-hdmi or fsl,imx6dl-hdmi.
 +   If internal HDMI DDC I2C master controller is supposed to be used,
 +   then simple-bus should be added to compatible value.
- gpr : should be gpr.
  The phandle points to the iomuxc-gpr region containing the HDMI
  multiplexer control register.
 @@ -22,6 +24,7 @@ Required properties:
   
   Optional properties:
- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
 + - ddc: internal HDMI DDC I2C master controller
   
   example:
   
 @@ -32,7 +35,7 @@ example:
   hdmi: hdmi@012 {
   #address-cells = 1;
   #size-cells = 0;
 -compatible = fsl,imx6q-hdmi;
 +compatible = fsl,imx6q-hdmi, simple-bus;
   reg = 0x0012 0x9000;
   interrupts = 0 115 0x04;
   gpr = gpr;
 @@ -40,6 +43,11 @@ example:
   clock-names = iahb, isfr;
   ddc-i2c-bus = i2c2;
   
 +hdmi_ddc: ddc {
 +compatible = fsl,imx6q-hdmi-ddc;
 +status = disabled;
 +};
 +
   port@0 {
   reg = 0;
   

 knowing in advance that I2C framework lacks a graceful support of non
 fully compliant I2C devices, do you have any objections to the proposed
 iMX HDMI DTS change?
 I'm not sure about this. Have you seen drm: Decouple EDID parsing from
 I2C adapter? I feel like in the absence of a ddc-i2c-bus property the
 imx-hdmi/dw-hdmi driver should try to use the internal HDMI DDC I2C
 master controller, bypassing the I2C framework altogether.

 My idea is exactly not to bypass the I2C framework, briefly the
 rationale is that
 * it allows to reuse I2C UAPI/tools naturally applied to the internal
 iMX HDMI DDC bus,
 * it allows to use iMX HDMI DDC bus as an additional feature-limited I2C
 bus on SoC (who knows, I absolutely won't be surprised, if anyone needs
 it on practice),
 * if an HDMI controller supports an external I2C bus, the integration
 with HDMI DDC bus driver based on I2C framework is seamless.

 However I agree that the selected approach may look odd, the question is
 if the oddness comes from the technical side or from the fact that
 nobody has done it before this way.

 I'm open to any critique, if the proposal of creating an I2C bus from
 HDMI DDC bus is lame, then I suppose the shared iMX HDMI DDC bus driver
 should be converted to something formless and internally used by
 imx-hdmi. The negative side-effects of such a change from my point of
 view are
 * more or less natural modularity is lost,
 * a number of I2C framework API/functions should be copy-pasted to the
 updated HDMI DDC bus driver to support a subset of I2C read/write
 transactions.
 If Wolfram is happy to accomodate such feature limited, 'I2C master'
 devices in i2c/drivers/busses in principle, I won't disagree.

 But then it should be abstracted properly. The dw-hdmi-tx core on i.MX6
 has the DDC I2C master register

Re: [PATCH 1/3] staging: imx-drm: document internal HDMI I2C master controller DT binding

[Vladimir Zapolskiy]

Hi Philipp and Shawn,

On 15.11.2014 19:49, Vladimir Zapolskiy wrote:
 Provide information about how to bind internal iMX6Q/DL HDMI DDC I2C
 master controller. The property is set as optional one, because iMX6
 HDMI DDC bus may be represented by one of general purpose I2C busses
 found on SoC.
 
 Signed-off-by: Vladimir Zapolskiy vladimir_zapols...@mentor.com
 Cc: Wolfram Sang w...@the-dreams.de
 Cc: Philipp Zabel p.za...@pengutronix.de
 Cc: Shawn Guo shawn@linaro.org
 Cc: devicet...@vger.kernel.org
 Cc: linux-media@vger.kernel.org
 Cc: linux-arm-ker...@lists.infradead.org
 Cc: linux-...@vger.kernel.org
 ---
  Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt |   10 +-
  1 file changed, 9 insertions(+), 1 deletion(-)
 
 diff --git a/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt 
 b/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
 index 1b756cf..43c8924 100644
 --- a/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
 +++ b/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
 @@ -10,6 +10,8 @@ Required properties:
   - #address-cells : should be 1
   - #size-cells : should be 0
   - compatible : should be fsl,imx6q-hdmi or fsl,imx6dl-hdmi.
 +   If internal HDMI DDC I2C master controller is supposed to be used,
 +   then simple-bus should be added to compatible value.
   - gpr : should be gpr.
 The phandle points to the iomuxc-gpr region containing the HDMI
 multiplexer control register.
 @@ -22,6 +24,7 @@ Required properties:
  
  Optional properties:
   - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
 + - ddc: internal HDMI DDC I2C master controller
  
  example:
  
 @@ -32,7 +35,7 @@ example:
  hdmi: hdmi@012 {
  #address-cells = 1;
  #size-cells = 0;
 -compatible = fsl,imx6q-hdmi;
 +compatible = fsl,imx6q-hdmi, simple-bus;
  reg = 0x0012 0x9000;
  interrupts = 0 115 0x04;
  gpr = gpr;
 @@ -40,6 +43,11 @@ example:
  clock-names = iahb, isfr;
  ddc-i2c-bus = i2c2;
  
 +hdmi_ddc: ddc {
 +compatible = fsl,imx6q-hdmi-ddc;
 +status = disabled;
 +};
 +
  port@0 {
  reg = 0;
  
 

knowing in advance that I2C framework lacks a graceful support of non
fully compliant I2C devices, do you have any objections to the proposed
iMX HDMI DTS change?

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] staging: imx-drm: document internal HDMI I2C master controller DT binding

[Vladimir Zapolskiy]

On 01.12.2014 17:11, Philipp Zabel wrote:
 Am Montag, den 01.12.2014, 16:54 +0200 schrieb Vladimir Zapolskiy:
 Hi Philipp and Shawn,

 On 15.11.2014 19:49, Vladimir Zapolskiy wrote:
 Provide information about how to bind internal iMX6Q/DL HDMI DDC I2C
 master controller. The property is set as optional one, because iMX6
 HDMI DDC bus may be represented by one of general purpose I2C busses
 found on SoC.

 Signed-off-by: Vladimir Zapolskiy vladimir_zapols...@mentor.com
 Cc: Wolfram Sang w...@the-dreams.de
 Cc: Philipp Zabel p.za...@pengutronix.de
 Cc: Shawn Guo shawn@linaro.org
 Cc: devicet...@vger.kernel.org
 Cc: linux-media@vger.kernel.org
 Cc: linux-arm-ker...@lists.infradead.org
 Cc: linux-...@vger.kernel.org
 ---
  Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt |   10 
 +-
  1 file changed, 9 insertions(+), 1 deletion(-)

 diff --git a/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt 
 b/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
 index 1b756cf..43c8924 100644
 --- a/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
 +++ b/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
 @@ -10,6 +10,8 @@ Required properties:
   - #address-cells : should be 1
   - #size-cells : should be 0
   - compatible : should be fsl,imx6q-hdmi or fsl,imx6dl-hdmi.
 +   If internal HDMI DDC I2C master controller is supposed to be used,
 +   then simple-bus should be added to compatible value.
   - gpr : should be gpr.
 The phandle points to the iomuxc-gpr region containing the HDMI
 multiplexer control register.
 @@ -22,6 +24,7 @@ Required properties:
  
  Optional properties:
   - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
 + - ddc: internal HDMI DDC I2C master controller
  
  example:
  
 @@ -32,7 +35,7 @@ example:
  hdmi: hdmi@012 {
  #address-cells = 1;
  #size-cells = 0;
 -compatible = fsl,imx6q-hdmi;
 +compatible = fsl,imx6q-hdmi, simple-bus;
  reg = 0x0012 0x9000;
  interrupts = 0 115 0x04;
  gpr = gpr;
 @@ -40,6 +43,11 @@ example:
  clock-names = iahb, isfr;
  ddc-i2c-bus = i2c2;
  
 +hdmi_ddc: ddc {
 +compatible = fsl,imx6q-hdmi-ddc;
 +status = disabled;
 +};
 +
  port@0 {
  reg = 0;
  


 knowing in advance that I2C framework lacks a graceful support of non
 fully compliant I2C devices, do you have any objections to the proposed
 iMX HDMI DTS change?
 
 I'm not sure about this. Have you seen drm: Decouple EDID parsing from
 I2C adapter? I feel like in the absence of a ddc-i2c-bus property the
 imx-hdmi/dw-hdmi driver should try to use the internal HDMI DDC I2C
 master controller, bypassing the I2C framework altogether.
 

My idea is exactly not to bypass the I2C framework, briefly the
rationale is that
* it allows to reuse I2C UAPI/tools naturally applied to the internal
iMX HDMI DDC bus,
* it allows to use iMX HDMI DDC bus as an additional feature-limited I2C
bus on SoC (who knows, I absolutely won't be surprised, if anyone needs
it on practice),
* if an HDMI controller supports an external I2C bus, the integration
with HDMI DDC bus driver based on I2C framework is seamless.

However I agree that the selected approach may look odd, the question is
if the oddness comes from the technical side or from the fact that
nobody has done it before this way.

I'm open to any critique, if the proposal of creating an I2C bus from
HDMI DDC bus is lame, then I suppose the shared iMX HDMI DDC bus driver
should be converted to something formless and internally used by
imx-hdmi. The negative side-effects of such a change from my point of
view are
* more or less natural modularity is lost,
* a number of I2C framework API/functions should be copy-pasted to the
updated HDMI DDC bus driver to support a subset of I2C read/write
transactions.

--
With best wishes,
Vladimir

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] staging: imx-drm: document internal HDMI I2C master controller DT binding

[Vladimir Zapolskiy]

Provide information about how to bind internal iMX6Q/DL HDMI DDC I2C
master controller. The property is set as optional one, because iMX6
HDMI DDC bus may be represented by one of general purpose I2C busses
found on SoC.

Signed-off-by: Vladimir Zapolskiy vladimir_zapols...@mentor.com
Cc: Wolfram Sang w...@the-dreams.de
Cc: Philipp Zabel p.za...@pengutronix.de
Cc: Shawn Guo shawn@linaro.org
Cc: devicet...@vger.kernel.org
Cc: linux-media@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-...@vger.kernel.org
---
 Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt |   10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt 
b/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
index 1b756cf..43c8924 100644
--- a/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
+++ b/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
@@ -10,6 +10,8 @@ Required properties:
  - #address-cells : should be 1
  - #size-cells : should be 0
  - compatible : should be fsl,imx6q-hdmi or fsl,imx6dl-hdmi.
+   If internal HDMI DDC I2C master controller is supposed to be used,
+   then simple-bus should be added to compatible value.
  - gpr : should be gpr.
The phandle points to the iomuxc-gpr region containing the HDMI
multiplexer control register.
@@ -22,6 +24,7 @@ Required properties:
 
 Optional properties:
  - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
+ - ddc: internal HDMI DDC I2C master controller
 
 example:
 
@@ -32,7 +35,7 @@ example:
 hdmi: hdmi@012 {
 #address-cells = 1;
 #size-cells = 0;
-compatible = fsl,imx6q-hdmi;
+compatible = fsl,imx6q-hdmi, simple-bus;
 reg = 0x0012 0x9000;
 interrupts = 0 115 0x04;
 gpr = gpr;
@@ -40,6 +43,11 @@ example:
 clock-names = iahb, isfr;
 ddc-i2c-bus = i2c2;
 
+hdmi_ddc: ddc {
+compatible = fsl,imx6q-hdmi-ddc;
+status = disabled;
+};
+
 port@0 {
 reg = 0;
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html