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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] mtd: nand: Rename nand.h into rawnand.h

[Vladimir Zapolskiy]

Hi Boris,

On 04.08.2017 18:29, Boris Brezillon wrote:
> We are planning to share more code between different NAND based
> devices (SPI NAND, OneNAND and raw NANDs), but before doing that
> we need to move the existing include/linux/mtd/nand.h file into
> include/linux/mtd/rawnand.h so we can later create a nand.h header
> containing all common structure and function prototypes.
> 
> Signed-off-by: Boris Brezillon <boris.brezil...@free-electrons.com>
> Signed-off-by: Peter Pan <peterpand...@micron.com>
> Cc: Jonathan Corbet <cor...@lwn.net>
> Cc: Sekhar Nori <nsek...@ti.com>
> Cc: Kevin Hilman <khil...@kernel.org>
> Cc: Jason Cooper <ja...@lakedaemon.net>
> Cc: Andrew Lunn <and...@lunn.ch>
> Cc: Sebastian Hesselbarth <sebastian.hesselba...@gmail.com>
> Cc: Gregory Clement <gregory.clem...@free-electrons.com>
> Cc: Hartley Sweeten <hswee...@visionengravers.com>
> Cc: Alexander Sverdlin <alexander.sverd...@gmail.com>
> Cc: Shawn Guo <shawn...@kernel.org>
> Cc: Sascha Hauer <ker...@pengutronix.de>
> Cc: Fabio Estevam <fabio.este...@nxp.com>
> Cc: Imre Kaloz <ka...@openwrt.org>
> Cc: Krzysztof Halasa <khal...@piap.pl>
> Cc: Eric Miao <eric.y.m...@gmail.com>
> Cc: Haojian Zhuang <haojian.zhu...@gmail.com>
> Cc: Aaro Koskinen <aaro.koski...@iki.fi>
> Cc: Tony Lindgren <t...@atomide.com>
> Cc: Alexander Clouter <a...@digriz.org.uk>
> Cc: Daniel Mack <dan...@zonque.org>
> Cc: Robert Jarzmik <robert.jarz...@free.fr>
> Cc: Marek Vasut <marek.va...@gmail.com>
> Cc: Kukjin Kim <kg...@kernel.org>
> Cc: Krzysztof Kozlowski <k...@kernel.org>
> Cc: Simtec Linux Team <li...@simtec.co.uk>
> Cc: Steven Miao <real...@gmail.com>
> Cc: Mikael Starvik <star...@axis.com>
> Cc: Jesper Nilsson <jesper.nils...@axis.com>
> Cc: Ralf Baechle <r...@linux-mips.org>
> Cc: Yoshinori Sato <ys...@users.sourceforge.jp>
> Cc: Rich Felker <dal...@libc.org>
> Cc: Wenyou Yang <wenyou.y...@atmel.com>
> Cc: Josh Wu <rainyfeel...@outlook.com>
> Cc: Kamal Dasu <kdasu.k...@gmail.com>
> Cc: Masahiro Yamada <yamada.masah...@socionext.com>
> Cc: Han Xu <han...@nxp.com>
> Cc: Harvey Hunt <harveyhuntne...@gmail.com>
> Cc: Vladimir Zapolskiy <v...@mleia.com>
> Cc: Sylvain Lemieux <slemieux.t...@gmail.com>
> Cc: Matthias Brugger <matthias@gmail.com>
> Cc: Wan ZongShun <mcuos@gmail.com>
> Cc: Neil Armstrong <narmstr...@baylibre.com>
> Cc: Ezequiel Garcia <ezequiel.gar...@free-electrons.com>
> Cc: Maxim Levitsky <maximlevit...@gmail.com>
> Cc: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
> Cc: Stefan Agner <ste...@agner.ch>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> Cc: linux-...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-o...@vger.kernel.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: adi-buildroot-de...@lists.sourceforge.net
> Cc: linux-cris-ker...@axis.com
> Cc: linux-m...@linux-mips.org
> Cc: linux...@vger.kernel.org
> Cc: bcm-kernel-feedback-l...@broadcom.com
> Cc: linux-media...@lists.infradead.org
> Cc: linux-ox...@lists.tuxfamily.org
> Cc: linuxppc-...@lists.ozlabs.org
> Cc: de...@driverdev.osuosl.org
> ---
> Hi All,
> 
> Sorry for the huge Cc list, but I'd like to collect as much acks as
> possible for this patch which is actually part of a bigger series [1].
> 
> Note that there's nothing complicated here, it's just a mechanical
> s/nand\.h/rawnand\.h/ replacement, but it impacts several architectures,
> the doc and staging directories.
> 
> Regards,
> 
> Boris
> 
> [1]https://lwn.net/Articles/723694/
> ---

[snip]

>  drivers/mtd/nand/lpc32xx_mlc.c  | 2 +-
>  drivers/mtd/nand/lpc32xx_slc.c  | 2 +-

For LPC32xx drivers

Acked-by: Vladimir Zapolskiy <v...@mleia.com>

--
With best wishes,
Vladimir
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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 = <
> +   

[PATCH] staging: android: ion_test: fix check of platform_device_register_simple() error code

[Vladimir Zapolskiy]

On error platform_device_register_simple() returns ERR_PTR() value,
check for NULL always fails. The change corrects the check itself and
propagates the returned error upwards.

Fixes: 81fb0b901397 ("staging: android: ion_test: unregister the platform 
device")
Signed-off-by: Vladimir Zapolskiy <v...@mleia.com>
---
 drivers/staging/android/ion/ion_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/ion/ion_test.c 
b/drivers/staging/android/ion/ion_test.c
index da34bc12..83a3af0 100644
--- a/drivers/staging/android/ion/ion_test.c
+++ b/drivers/staging/android/ion/ion_test.c
@@ -285,8 +285,8 @@ static int __init ion_test_init(void)
 {
ion_test_pdev = platform_device_register_simple("ion-test",
-1, NULL, 0);
-   if (!ion_test_pdev)
-   return -ENODEV;
+   if (IS_ERR(ion_test_pdev))
+   return PTR_ERR(ion_test_pdev);
 
return platform_driver_probe(_test_platform_driver, ion_test_probe);
 }
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: wilc1000: fix check of kthread_run() return value

[Vladimir Zapolskiy]

Hi Julian,

On 10.03.2016 02:24, Julian Calaby wrote:
> Hi Vladimir,
> 
> On Thu, Mar 10, 2016 at 11:02 AM, Vladimir Zapolskiy <v...@mleia.com> wrote:
>> Hi Julian,
>>
>> On 10.03.2016 01:42, Julian Calaby wrote:
>>> Hi Vladimir,
>>>
>>> On Thu, Mar 10, 2016 at 10:30 AM, Vladimir Zapolskiy <v...@mleia.com> wrote:
>>>> Hi Julian,
>>>>
>>>> On 10.03.2016 01:27, Julian Calaby wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> On Thu, Mar 10, 2016 at 10:13 AM, Vladimir Zapolskiy <v...@mleia.com> 
>>>>> wrote:
>>>>>> The kthread_run() function returns either a valid task_struct or
>>>>>> ERR_PTR() value, check for NULL is invalid. The change fixes potential
>>>>>> oops, e.g. in OOM situation.
>>>>>>
>>>>>> Signed-off-by: Vladimir Zapolskiy <v...@mleia.com>
>>>>>> ---
>>>>>>  drivers/staging/wilc1000/linux_wlan.c | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/wilc1000/linux_wlan.c 
>>>>>> b/drivers/staging/wilc1000/linux_wlan.c
>>>>>> index 54fe9d7..5077c30 100644
>>>>>> --- a/drivers/staging/wilc1000/linux_wlan.c
>>>>>> +++ b/drivers/staging/wilc1000/linux_wlan.c
>>>>>> @@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct 
>>>>>> net_device *dev)
>>>>>> PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
>>>>>> wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
>>>>>>  "K_TXQ_TASK");
>>>>>> -   if (!wilc->txq_thread) {
>>>>>> +   if (IS_ERR(wilc->txq_thread)) {
>>>>>> PRINT_ER("couldn't create TXQ thread\n");
>>>>>> wilc->close = 0;
>>>>>> -   return -ENOBUFS;
>>>>>> +   return PTR_ERR(wilc->txq_thread);
>>>>>
>>>>> Are you sure changing the error returned is correct? Do all the
>>>>> callers of wlan_initialize_threads() handle the full range of errors
>>>>> from kthread_run()?
>>>>
>>>> Have you checked the driver?
>>>
>>> I'm making sure you have. It's possible that there's a good reason why
>>> this returns -ENOBUFS I want to know that you've at least considered
>>> that possibility.
>>
>> You have my confirmation, I've checked the call stack before publishing
>> this fix.
> 
> Awesome.
> 
>>>> This function is called once on initialization, the check on the upper 
>>>> layer
>>>> has "if (ret < 0) goto exit_badly;" form.
>>>
>>> And practically everything in the chain up to net_device_ops uses the
>>> same error handling scheme so it's probably fine.
>>
>> dev_open()
>>   __dev_open()
>> wilc_mac_open()
>>   wilc1000_wlan_init()
>> wlan_initialize_threads()
>>
>> Oh, why kernel threads within a driver are init'ed/destroyed on
>> each device up/down state transition?
> 
> You'll have to ask the driver developers. I believe this was a cross
> platform driver that is currently being Linux-ised, so I'm guessing
> this is some artefact of that.
> 
>>> You should also document this change in the commit message.
>>
>> The change is documented in the commit message, take a look. But I didn't
>> add "I swear it does not break anything" ;)
> 
> You
> 1. corrected the test in the if statement
> 2. changed the return value from -ENOBUFS
> in your patch, however you only documented the first part.

Agree, it makes sense.

> I would have expected a commit message along the lines of:
> 
> >8
> 
> The kthread_run() function returns either a valid task_struct or
> ERR_PTR() value, so the check for NULL is invalid.
> 
> Also return the error from kthread_run() instead of -ENOBUFS.
> 
> 8<
> 

Before publishing v2 let see if driver maintainers have something else
to add, or may be it is better to preserve -ENOBUFS, or may be they are
so kind to update the commit message on patch application.

Julian, thanks for review.

--
With best wishes,
Vladimir
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: wilc1000: fix check of kthread_run() return value

[Vladimir Zapolskiy]

Hi Julian,

On 10.03.2016 01:42, Julian Calaby wrote:
> Hi Vladimir,
> 
> On Thu, Mar 10, 2016 at 10:30 AM, Vladimir Zapolskiy <v...@mleia.com> wrote:
>> Hi Julian,
>>
>> On 10.03.2016 01:27, Julian Calaby wrote:
>>> Hi Vladimir,
>>>
>>> On Thu, Mar 10, 2016 at 10:13 AM, Vladimir Zapolskiy <v...@mleia.com> wrote:
>>>> The kthread_run() function returns either a valid task_struct or
>>>> ERR_PTR() value, check for NULL is invalid. The change fixes potential
>>>> oops, e.g. in OOM situation.
>>>>
>>>> Signed-off-by: Vladimir Zapolskiy <v...@mleia.com>
>>>> ---
>>>>  drivers/staging/wilc1000/linux_wlan.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/wilc1000/linux_wlan.c 
>>>> b/drivers/staging/wilc1000/linux_wlan.c
>>>> index 54fe9d7..5077c30 100644
>>>> --- a/drivers/staging/wilc1000/linux_wlan.c
>>>> +++ b/drivers/staging/wilc1000/linux_wlan.c
>>>> @@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct net_device 
>>>> *dev)
>>>> PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
>>>> wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
>>>>  "K_TXQ_TASK");
>>>> -   if (!wilc->txq_thread) {
>>>> +   if (IS_ERR(wilc->txq_thread)) {
>>>> PRINT_ER("couldn't create TXQ thread\n");
>>>> wilc->close = 0;
>>>> -   return -ENOBUFS;
>>>> +   return PTR_ERR(wilc->txq_thread);
>>>
>>> Are you sure changing the error returned is correct? Do all the
>>> callers of wlan_initialize_threads() handle the full range of errors
>>> from kthread_run()?
>>
>> Have you checked the driver?
> 
> I'm making sure you have. It's possible that there's a good reason why
> this returns -ENOBUFS I want to know that you've at least considered
> that possibility.

You have my confirmation, I've checked the call stack before publishing
this fix.

>> This function is called once on initialization, the check on the upper layer
>> has "if (ret < 0) goto exit_badly;" form.
> 
> And practically everything in the chain up to net_device_ops uses the
> same error handling scheme so it's probably fine.

dev_open()
  __dev_open()
wilc_mac_open()
  wilc1000_wlan_init()
wlan_initialize_threads()

Oh, why kernel threads within a driver are init'ed/destroyed on
each device up/down state transition?

> You should also document this change in the commit message.

The change is documented in the commit message, take a look. But I didn't
add "I swear it does not break anything" ;)

--
With best wishes,
Vladimir
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: wilc1000: fix check of kthread_run() return value

[Vladimir Zapolskiy]

Hi Julian,

On 10.03.2016 01:27, Julian Calaby wrote:
> Hi Vladimir,
> 
> On Thu, Mar 10, 2016 at 10:13 AM, Vladimir Zapolskiy <v...@mleia.com> wrote:
>> The kthread_run() function returns either a valid task_struct or
>> ERR_PTR() value, check for NULL is invalid. The change fixes potential
>> oops, e.g. in OOM situation.
>>
>> Signed-off-by: Vladimir Zapolskiy <v...@mleia.com>
>> ---
>>  drivers/staging/wilc1000/linux_wlan.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/wilc1000/linux_wlan.c 
>> b/drivers/staging/wilc1000/linux_wlan.c
>> index 54fe9d7..5077c30 100644
>> --- a/drivers/staging/wilc1000/linux_wlan.c
>> +++ b/drivers/staging/wilc1000/linux_wlan.c
>> @@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct net_device 
>> *dev)
>> PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
>> wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
>>  "K_TXQ_TASK");
>> -   if (!wilc->txq_thread) {
>> +   if (IS_ERR(wilc->txq_thread)) {
>> PRINT_ER("couldn't create TXQ thread\n");
>> wilc->close = 0;
>> -   return -ENOBUFS;
>> +   return PTR_ERR(wilc->txq_thread);
> 
> Are you sure changing the error returned is correct? Do all the
> callers of wlan_initialize_threads() handle the full range of errors
> from kthread_run()?

Have you checked the driver?

This function is called once on initialization, the check on the upper layer
has "if (ret < 0) goto exit_badly;" form.

--
With best wishes,
Vladimir
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: wilc1000: fix check of kthread_run() return value

[Vladimir Zapolskiy]

The kthread_run() function returns either a valid task_struct or
ERR_PTR() value, check for NULL is invalid. The change fixes potential
oops, e.g. in OOM situation.

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

diff --git a/drivers/staging/wilc1000/linux_wlan.c 
b/drivers/staging/wilc1000/linux_wlan.c
index 54fe9d7..5077c30 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct net_device *dev)
PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
 "K_TXQ_TASK");
-   if (!wilc->txq_thread) {
+   if (IS_ERR(wilc->txq_thread)) {
PRINT_ER("couldn't create TXQ thread\n");
wilc->close = 0;
-   return -ENOBUFS;
+   return PTR_ERR(wilc->txq_thread);
}
down(>txq_thread_started);
 
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel