Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <eb6488f6-330f-d42c-679a-393ac899e98f@ideasonboard.com>
Date:   Wed, 26 Jan 2022 15:54:58 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [RFC PATCH v2 4/7] media: bcm2835-unicam: Add support for for
 CCP2/CSI2 camera interface
Content-Language: en-US
From:   Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     dave.stevenson@raspberrypi.com, devicetree@vger.kernel.org,
        kernel-list@raspberrypi.com, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
        linux-rpi-kernel@lists.infradead.org, lukasz@jany.st,
        mchehab@kernel.org, naush@raspberrypi.com, robh@kernel.org,
        tomi.valkeinen@ideasonboard.com, Maxime Ripard <maxime@cerno.tech>
References: <20220121081810.155500-1-jeanmichel.hautbois@ideasonboard.com>
 <20220121081810.155500-5-jeanmichel.hautbois@ideasonboard.com>
 <YeySvkxEfpzTj+pC@pendragon.ideasonboard.com>
 <0212af6d-a5de-05bb-161b-4271692dee59@ideasonboard.com>
In-Reply-To: <0212af6d-a5de-05bb-161b-4271692dee59@ideasonboard.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

+CC Maxime :-)

On 26/01/2022 15:21, Jean-Michel Hautbois wrote:
> Hi Laurent, thanks for the review !
> 
> A question for Dave below :-)
> 
> On 23/01/2022 00:26, Laurent Pinchart wrote:
>> Hi Jean-Michel,
>>
>> Thank you for the patch.
>>
>> Dave, Naush, there are a few questions for you below.
>>
>> On Fri, Jan 21, 2022 at 09:18:07AM +0100, Jean-Michel Hautbois wrote:
>>> Add driver for the Unicam camera receiver block on BCM283x processors.
>>> It is represented as two video device nodes: unicam-image and
>>> unicam-embedded which are connected to an internal subdev (named
>>> unicam-subdev) in order to manage streams routing.
>>>
>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>>> Signed-off-by: Jean-Michel Hautbois 
>>> <jeanmichel.hautbois@ideasonboard.com>
>>>
>>> ---
>>> in v2: Remove the unicam_{info,debug,error} macros and use
>>> dev_dbg/dev_err instead.
>>> ---
>>>   MAINTAINERS                                   |    1 +
>>>   drivers/media/platform/Kconfig                |    1 +
>>>   drivers/media/platform/Makefile               |    2 +
>>>   drivers/media/platform/bcm2835/Kconfig        |   21 +
>>>   drivers/media/platform/bcm2835/Makefile       |    3 +
>>>   .../media/platform/bcm2835/bcm2835-unicam.c   | 2678 +++++++++++++++++
>>>   .../media/platform/bcm2835/vc4-regs-unicam.h  |  253 ++
>>>   7 files changed, 2959 insertions(+)
>>>   create mode 100644 drivers/media/platform/bcm2835/Kconfig
>>>   create mode 100644 drivers/media/platform/bcm2835/Makefile
>>>   create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
>>>   create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 07f238fd5ff9..b17bb533e007 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -3684,6 +3684,7 @@ M:    Raspberry Pi Kernel Maintenance 
>>> <kernel-list@raspberrypi.com>
>>>   L:    linux-media@vger.kernel.org
>>>   S:    Maintained
>>>   F:    Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
>>> +F:    drivers/media/platform/bcm2835/
>>>   BROADCOM BCM47XX MIPS ARCHITECTURE
>>>   M:    Hauke Mehrtens <hauke@hauke-m.de>
>>> diff --git a/drivers/media/platform/Kconfig 
>>> b/drivers/media/platform/Kconfig
>>> index 9fbdba0fd1e7..033b0358fbb8 100644
>>> --- a/drivers/media/platform/Kconfig
>>> +++ b/drivers/media/platform/Kconfig
>>> @@ -170,6 +170,7 @@ source "drivers/media/platform/am437x/Kconfig"
>>>   source "drivers/media/platform/xilinx/Kconfig"
>>>   source "drivers/media/platform/rcar-vin/Kconfig"
>>>   source "drivers/media/platform/atmel/Kconfig"
>>> +source "drivers/media/platform/bcm2835/Kconfig"
>>>   source "drivers/media/platform/sunxi/Kconfig"
>>>   config VIDEO_TI_CAL
>>> diff --git a/drivers/media/platform/Makefile 
>>> b/drivers/media/platform/Makefile
>>> index 19bcbced7382..689e64857db1 100644
>>> --- a/drivers/media/platform/Makefile
>>> +++ b/drivers/media/platform/Makefile
>>> @@ -85,6 +85,8 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)        += qcom/camss/
>>>   obj-$(CONFIG_VIDEO_QCOM_VENUS)        += qcom/venus/
>>> +obj-y                    += bcm2835/
>>
>> One day it would be nice to clean the media Kconfig and Makefile
>> infrastructure to use a consistent style for all drivers. One can always
>> dream :-)
>>
>>> +
>>>   obj-y                    += sunxi/
>>>   obj-$(CONFIG_VIDEO_MESON_GE2D)        += meson/ge2d/
>>> diff --git a/drivers/media/platform/bcm2835/Kconfig 
>>> b/drivers/media/platform/bcm2835/Kconfig
>>> new file mode 100644
>>> index 000000000000..bd1370199650
>>> --- /dev/null
>>> +++ b/drivers/media/platform/bcm2835/Kconfig
>>> @@ -0,0 +1,21 @@
>>> +# Broadcom VideoCore4 V4L2 camera support
>>> +
>>> +config VIDEO_BCM2835_UNICAM
>>> +    tristate "Broadcom BCM283x/BCM271x Unicam video capture driver"
>>> +    depends on VIDEO_V4L2
>>> +    depends on ARCH_BCM2835 || COMPILE_TEST
>>
>> I would put the arch dependency first, but that may be just me.
>>
>>> +    select VIDEO_V4L2_SUBDEV_API
>>> +    select MEDIA_CONTROLLER
>>> +    select VIDEOBUF2_DMA_CONTIG
>>> +    select V4L2_FWNODE
>>
>> And sort there alphabetically.
>>
>>> +    help
>>> +      Say Y here to enable support for the BCM283x/BCM271x CSI-2 
>>> receiver.
>>> +      This is a V4L2 driver that controls the CSI-2 receiver directly,
>>> +      independently from the VC4 firmware.
>>> +      This driver is mutually exclusive with the use of 
>>> bcm2835-camera. The
>>> +      firmware will disable all access to the peripheral from within 
>>> the
>>> +      firmware if it finds a DT node using it, and bcm2835-camera will
>>> +      therefore fail to probe.
>>
>> Dave, Naush, what should we do with the bcm2835-camera driver in staging
>> ? Do we need to keep it to support older Pi models ?
>>
>>> +
>>> +      To compile this driver as a module, choose M here. The module 
>>> will be
>>> +      called bcm2835-unicam.
>>> diff --git a/drivers/media/platform/bcm2835/Makefile 
>>> b/drivers/media/platform/bcm2835/Makefile
>>> new file mode 100644
>>> index 000000000000..a98aba03598a
>>> --- /dev/null
>>> +++ b/drivers/media/platform/bcm2835/Makefile
>>> @@ -0,0 +1,3 @@
>>> +# Makefile for BCM2835 Unicam driver
>>> +
>>> +obj-$(CONFIG_VIDEO_BCM2835_UNICAM) += bcm2835-unicam.o
>>> diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c 
>>> b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>>> new file mode 100644
>>> index 000000000000..7636496be00a
>>> --- /dev/null
>>> +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>>> @@ -0,0 +1,2678 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * BCM283x / BCM271x Unicam Capture Driver
>>> + *
>>> + * Copyright (C) 2017-2020 - Raspberry Pi (Trading) Ltd.
>>> + *
>>> + * Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> + *
>>> + * Based on TI am437x driver by
>>> + *   Benoit Parrot <bparrot@ti.com>
>>> + *   Lad, Prabhakar <prabhakar.csengg@gmail.com>
>>> + *
>>> + * and TI CAL camera interface driver by
>>> + *    Benoit Parrot <bparrot@ti.com>
>>> + *
>>> + *
>>> + * There are two camera drivers in the kernel for BCM283x - this one
>>> + * and bcm2835-camera (currently in staging).
>>> + *
>>> + * This driver directly controls the Unicam peripheral - there is no
>>> + * involvement with the VideoCore firmware. Unicam receives CSI-2 or
>>> + * CCP2 data and writes it into SDRAM.
>>> + * The only potential processing options are to repack Bayer data 
>>> into an
>>> + * alternate format, and applying windowing.
>>> + * The repacking does not shift the data, so can repack 
>>> V4L2_PIX_FMT_Sxxxx10P
>>> + * to V4L2_PIX_FMT_Sxxxx10, or V4L2_PIX_FMT_Sxxxx12P to 
>>> V4L2_PIX_FMT_Sxxxx12,
>>> + * but not generically up to V4L2_PIX_FMT_Sxxxx16. The driver will 
>>> add both
>>> + * formats where the relevant formats are defined, and will 
>>> automatically
>>> + * configure the repacking as required.
>>> + * Support for windowing may be added later.
>>> + *
>>> + * It should be possible to connect this driver to any sensor with a
>>> + * suitable output interface and V4L2 subdevice driver.
>>> + *
>>> + * bcm2835-camera uses the VideoCore firmware to control the sensor,
>>> + * Unicam, ISP, and all tuner control loops. Fully processed frames are
>>> + * delivered to the driver by the firmware. It only has sensor drivers
>>> + * for Omnivision OV5647, and Sony IMX219 sensors.
>>> + *
>>> + * The two drivers are mutually exclusive for the same Unicam instance.
>>> + * The VideoCore firmware checks the device tree configuration 
>>> during boot.
>>> + * If it finds device tree nodes called csi0 or csi1 it will block the
>>> + * firmware from accessing the peripheral, and bcm2835-camera will
>>> + * not be able to stream data.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/device.h>
>>> +#include <linux/dma-mapping.h>
>>> +#include <linux/err.h>
>>> +#include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/of_graph.h>
>>> +#include <linux/pinctrl/consumer.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/uaccess.h>
>>> +#include <linux/videodev2.h>
>>> +
>>> +#include <media/v4l2-common.h>
>>> +#include <media/v4l2-ctrls.h>
>>> +#include <media/v4l2-dev.h>
>>> +#include <media/v4l2-device.h>
>>> +#include <media/v4l2-dv-timings.h>
>>> +#include <media/v4l2-event.h>
>>> +#include <media/v4l2-ioctl.h>
>>> +#include <media/v4l2-fwnode.h>
>>> +#include <media/videobuf2-dma-contig.h>
>>> +
>>> +#include <media/v4l2-async.h>
>>> +#define v4l2_async_nf_add_subdev __v4l2_async_nf_add_subdev
>>
>> Don't do that ! It's the kind of pattern that a malicious actor would
>> use to obfuscate code and get vulnerabilities merged.
>>
>>> +
>>> +#include "vc4-regs-unicam.h"
>>> +
>>> +#define UNICAM_MODULE_NAME    "unicam"
>>> +#define UNICAM_VERSION        "0.1.0"
>>
>> I doubt the version would ever be incremented, so let's just drop it.
>>
>>> +
>>> +static int debug;
>>> +module_param(debug, int, 0644);
>>> +MODULE_PARM_DESC(debug, "Debug level 0-3");
>>
>> Not used.
>>
>>> +
>>> +/*
>>> + * Unicam must request a minimum of 250Mhz from the VPU clock.
>>> + * Otherwise the input FIFOs overrun and cause image corruption.
>>> + */
>>> +#define MIN_VPU_CLOCK_RATE (250 * 1000 * 1000)
>>> +/*
>>> + * To protect against a dodgy sensor driver never returning an error 
>>> from
>>> + * enum_mbus_code, set a maximum index value to be used.
>>> + */
>>> +#define MAX_ENUM_MBUS_CODE    128
>>> +
>>> +/*
>>> + * Stride is a 16 bit register, but also has to be a multiple of 32.
>>> + */
>>> +#define BPL_ALIGNMENT        32
>>> +#define MAX_BYTESPERLINE    ((1 << 16) - BPL_ALIGNMENT)
>>> +/*
>>> + * Max width is therefore determined by the max stride divided by
>>> + * the number of bits per pixel. Take 32bpp as a
>>> + * worst case.
>>> + * No imposed limit on the height, so adopt a square image for want
>>> + * of anything better.
>>> + */
>>> +#define MAX_WIDTH        (MAX_BYTESPERLINE / 4)
>>> +#define MAX_HEIGHT        MAX_WIDTH
>>> +/* Define a nominal minimum image size */
>>> +#define MIN_WIDTH        16
>>> +#define MIN_HEIGHT        16
>>> +/* Default size of the embedded buffer */
>>> +#define UNICAM_EMBEDDED_SIZE    16384
>>> +
>>> +/*
>>> + * Size of the dummy buffer. Can be any size really, but the DMA
>>> + * allocation works in units of page sizes.
>>> + */
>>> +#define DUMMY_BUF_SIZE        (PAGE_SIZE)
>>
>> No need for parentheses.
>>
>> Some of those macros have toogeneric names and may result in namespace
>> clashes. Please prefix them all with UNICAM_.
>>
>>> +
>>> +#define UNICAM_SD_PAD_SINK        0
>>> +#define UNICAM_SD_PAD_FIRST_SOURCE    1
>>
>> I'd name the source pads explicitly.
>>
>> #define UNICAM_SD_PAD_SOURCE_IMAGE    1
>> #define UNICAM_SD_PAD_SOURCE_METADATA    2
>>
>> (or META instead of METADATA, up to you, as long as we use consistent
>> naming through the driver).
>>
>>> +#define UNICAM_SD_NUM_SOURCE_PADS    2
>>> +#define UNICAM_SD_NUM_PADS        (1 + UNICAM_SD_NUM_SOURCE_PADS)
>>> +
>>> +static inline bool unicam_sd_pad_is_sink(u32 pad)
>>> +{
>>> +    /* Camera RX has 1 sink pad, and N source pads */
>>> +    return pad == 0;
>>> +}
>>> +
>>> +static inline bool unicam_sd_pad_is_source(u32 pad)
>>> +{
>>> +    /* Camera RX has 1 sink pad, and N source pads */
>>> +    return pad >= UNICAM_SD_PAD_FIRST_SOURCE &&
>>> +           pad <= UNICAM_SD_NUM_SOURCE_PADS;
>>
>>     return pad != UNICAM_SD_PAD_SINK;
>>
>> and drop UNICAM_SD_PAD_FIRST_SOURCE and UNICAM_SD_NUM_SOURCE_PADS.
>>
>>> +}
>>> +
>>> +enum node_types {
>>
>> enum unicam_node_type {
>>
>>> +    IMAGE_NODE,
>>> +    METADATA_NODE,
>>> +    MAX_NODES
>>> +};
>>
>> UNICAM_ prefixes please.
>>
>>> +
>>> +#define MASK_CS_DEFAULT        BIT(V4L2_COLORSPACE_DEFAULT)
>>
>> This and several of the macros below are not used.
>>
>>> +#define MASK_CS_SMPTE170M    BIT(V4L2_COLORSPACE_SMPTE170M)
>>> +#define MASK_CS_SMPTE240M    BIT(V4L2_COLORSPACE_SMPTE240M)
>>> +#define MASK_CS_REC709        BIT(V4L2_COLORSPACE_REC709)
>>> +#define MASK_CS_BT878        BIT(V4L2_COLORSPACE_BT878)
>>> +#define MASK_CS_470_M        BIT(V4L2_COLORSPACE_470_SYSTEM_M)
>>> +#define MASK_CS_470_BG        BIT(V4L2_COLORSPACE_470_SYSTEM_BG)
>>> +#define MASK_CS_JPEG        BIT(V4L2_COLORSPACE_JPEG)
>>> +#define MASK_CS_SRGB        BIT(V4L2_COLORSPACE_SRGB)
>>> +#define MASK_CS_OPRGB        BIT(V4L2_COLORSPACE_OPRGB)
>>> +#define MASK_CS_BT2020        BIT(V4L2_COLORSPACE_BT2020)
>>> +#define MASK_CS_RAW        BIT(V4L2_COLORSPACE_RAW)
>>> +#define MASK_CS_DCI_P3        BIT(V4L2_COLORSPACE_DCI_P3)
>>> +
>>> +#define MAX_COLORSPACE        32
>>> +
>>> +/*
>>> + * struct unicam_fmt - Unicam media bus format information
>>> + * @pixelformat: V4L2 pixel format FCC identifier. 0 if n/a.
>>> + * @repacked_fourcc: V4L2 pixel format FCC identifier if the data is 
>>> expanded
>>> + * out to 16bpp. 0 if n/a.
>>> + * @code: V4L2 media bus format code.
>>> + * @depth: Bits per pixel as delivered from the source.
>>> + * @csi_dt: CSI data type.
>>> + * @valid_colorspaces: Bitmask of valid colorspaces so that the 
>>> Media Controller
>>> + *        centric try_fmt can validate the colorspace and pass
>>> + *        v4l2-compliance.
>>
>> I'd actually drop colorspace support completely. Unicam doesn't affect
>> the colorspace. It will output whatever it receives from the sensor. You
>> can accept any colorspace on the sink pad of the subdev, and just
>> propagate that to the source pad.
>>
>> If that causes v4l2-compliance failures, then we have a problem either
>> in the sensor drivers, or in v4l2-compliance.
>>
>>> + * @check_variants: Flag to denote that there are multiple mediabus 
>>> formats
>>> + *        still in the list that could match this V4L2 format.
>>> + * @mc_skip: Media Controller shouldn't list this format via 
>>> ENUM_FMT as it is
>>> + *        a duplicate of an earlier format.
>>> + * @metadata_fmt: This format only applies to the metadata pad.
>>> + */
>>> +struct unicam_fmt {
>>> +    u32    fourcc;
>>> +    u32    repacked_fourcc;
>>> +    u32    code;
>>> +    u8    depth;
>>> +    u8    csi_dt;
>>> +    u32    valid_colorspaces;
>>> +    u8    check_variants:1;
>>> +    u8    mc_skip:1;
>>> +    u8    metadata_fmt:1;
>>> +};
>>> +
>>> +static const struct unicam_fmt formats[] = {
>>> +    /* YUV Formats */
>>> +    {
>>> +        .fourcc        = V4L2_PIX_FMT_YUYV,
>>> +        .code        = MEDIA_BUS_FMT_YUYV8_2X8,
>>
>> The convention for CSI-2 buses is to used the _1X16 media bus codes for
>> YUV. You can drop the first four entries, and drop the check_variants
>> and mc_skip fields.
>>
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x1e,
>>> +        .check_variants = 1,
>>> +        .valid_colorspaces = MASK_CS_SMPTE170M | MASK_CS_REC709 |
>>> +                     MASK_CS_JPEG,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_UYVY,
>>> +        .code        = MEDIA_BUS_FMT_UYVY8_2X8,
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x1e,
>>> +        .check_variants = 1,
>>> +        .valid_colorspaces = MASK_CS_SMPTE170M | MASK_CS_REC709 |
>>> +                     MASK_CS_JPEG,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_YVYU,
>>> +        .code        = MEDIA_BUS_FMT_YVYU8_2X8,
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x1e,
>>> +        .check_variants = 1,
>>> +        .valid_colorspaces = MASK_CS_SMPTE170M | MASK_CS_REC709 |
>>> +                     MASK_CS_JPEG,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_VYUY,
>>> +        .code        = MEDIA_BUS_FMT_VYUY8_2X8,
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x1e,
>>> +        .check_variants = 1,
>>> +        .valid_colorspaces = MASK_CS_SMPTE170M | MASK_CS_REC709 |
>>> +                     MASK_CS_JPEG,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_YUYV,
>>> +        .code        = MEDIA_BUS_FMT_YUYV8_1X16,
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x1e,
>>> +        .mc_skip    = 1,
>>> +        .valid_colorspaces = MASK_CS_SMPTE170M | MASK_CS_REC709 |
>>> +                     MASK_CS_JPEG,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_UYVY,
>>> +        .code        = MEDIA_BUS_FMT_UYVY8_1X16,
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x1e,
>>> +        .mc_skip    = 1,
>>> +        .valid_colorspaces = MASK_CS_SMPTE170M | MASK_CS_REC709 |
>>> +                     MASK_CS_JPEG,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_YVYU,
>>> +        .code        = MEDIA_BUS_FMT_YVYU8_1X16,
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x1e,
>>> +        .mc_skip    = 1,
>>> +        .valid_colorspaces = MASK_CS_SMPTE170M | MASK_CS_REC709 |
>>> +                     MASK_CS_JPEG,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_VYUY,
>>> +        .code        = MEDIA_BUS_FMT_VYUY8_1X16,
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x1e,
>>> +        .mc_skip    = 1,
>>> +        .valid_colorspaces = MASK_CS_SMPTE170M | MASK_CS_REC709 |
>>> +                     MASK_CS_JPEG,
>>> +    }, {
>>> +    /* RGB Formats */
>>> +        .fourcc        = V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */
>>> +        .code        = MEDIA_BUS_FMT_RGB565_2X8_LE,
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x22,
>>> +        .valid_colorspaces = MASK_CS_SRGB,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_RGB565X, /* rrrrrggg gggbbbbb */
>>> +        .code        = MEDIA_BUS_FMT_RGB565_2X8_BE,
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x22,
>>> +        .valid_colorspaces = MASK_CS_SRGB,
>>
>> CSI-2 specifies the RGB565 format unambiguously, there are no little
>> endian of big endian variants. Let's drop one of the two entries, and
>> use MEDIA_BUS_FMT_RGB565_1X15. Dave, Naush, what is the correct pixel
>> format for this, is data layed out in big endian or little endian ?
>>
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_RGB555, /* gggbbbbb arrrrrgg */
>>> +        .code        = MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE,
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x21,
>>> +        .valid_colorspaces = MASK_CS_SRGB,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_RGB555X, /* arrrrrgg gggbbbbb */
>>> +        .code        = MEDIA_BUS_FMT_RGB555_2X8_PADHI_BE,
>>> +        .depth        = 16,
>>> +        .csi_dt        = 0x21,
>>> +        .valid_colorspaces = MASK_CS_SRGB,
>>
>> Same here, but we need to define a new MEDIA_BUS_FMT_RGB555_1X16 format.
>> The CSI-2 RGB555 data type has the padding bit between the green and
>> blue components, so I wonder if Unicam maps that to one of
>> V4L2_PIX_FMT_RGB555 or V4L2_PIX_FMT_RGB555X, or if a new pixel format is
>> needed too.
>>
>> Another option would be to drop RGB555 support until someone needs it
>> (with a comment in the table to tell the entry is missing).
>>
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_RGB24, /* rgb */
>>> +        .code        = MEDIA_BUS_FMT_RGB888_1X24,
>>> +        .depth        = 24,
>>> +        .csi_dt        = 0x24,
>>> +        .valid_colorspaces = MASK_CS_SRGB,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_BGR24, /* bgr */
>>> +        .code        = MEDIA_BUS_FMT_BGR888_1X24,
>>> +        .depth        = 24,
>>> +        .csi_dt        = 0x24,
>>> +        .valid_colorspaces = MASK_CS_SRGB,
>>
>> This is possibly more tricky, as CSI-2 defined RGB888 as being
>> transmitted in BGR order, but sensors could possibly also support RGB as
>> a non-standard extension (the same may CSI-2 standardizes on UYVY, but
>> many sensors can swap components). We can keep both entry I suppose.
>>
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_RGB32, /* argb */
>>> +        .code        = MEDIA_BUS_FMT_ARGB8888_1X32,
>>> +        .depth        = 32,
>>> +        .csi_dt        = 0x0,
>>> +        .valid_colorspaces = MASK_CS_SRGB,
>>
>> There's no 32-bit RGB in CSI-2, should this be dropped ?
>>
>>> +    }, {
>>> +    /* Bayer Formats */
>>> +        .fourcc        = V4L2_PIX_FMT_SBGGR8,
>>> +        .code        = MEDIA_BUS_FMT_SBGGR8_1X8,
>>> +        .depth        = 8,
>>> +        .csi_dt        = 0x2a,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SGBRG8,
>>> +        .code        = MEDIA_BUS_FMT_SGBRG8_1X8,
>>> +        .depth        = 8,
>>> +        .csi_dt        = 0x2a,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SGRBG8,
>>> +        .code        = MEDIA_BUS_FMT_SGRBG8_1X8,
>>> +        .depth        = 8,
>>> +        .csi_dt        = 0x2a,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SRGGB8,
>>> +        .code        = MEDIA_BUS_FMT_SRGGB8_1X8,
>>> +        .depth        = 8,
>>> +        .csi_dt        = 0x2a,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SBGGR10P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SBGGR10,
>>> +        .code        = MEDIA_BUS_FMT_SBGGR10_1X10,
>>> +        .depth        = 10,
>>> +        .csi_dt        = 0x2b,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SGBRG10P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SGBRG10,
>>> +        .code        = MEDIA_BUS_FMT_SGBRG10_1X10,
>>> +        .depth        = 10,
>>> +        .csi_dt        = 0x2b,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SGRBG10P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SGRBG10,
>>> +        .code        = MEDIA_BUS_FMT_SGRBG10_1X10,
>>> +        .depth        = 10,
>>> +        .csi_dt        = 0x2b,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SRGGB10P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SRGGB10,
>>> +        .code        = MEDIA_BUS_FMT_SRGGB10_1X10,
>>> +        .depth        = 10,
>>> +        .csi_dt        = 0x2b,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SBGGR12P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SBGGR12,
>>> +        .code        = MEDIA_BUS_FMT_SBGGR12_1X12,
>>> +        .depth        = 12,
>>> +        .csi_dt        = 0x2c,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SGBRG12P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SGBRG12,
>>> +        .code        = MEDIA_BUS_FMT_SGBRG12_1X12,
>>> +        .depth        = 12,
>>> +        .csi_dt        = 0x2c,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SGRBG12P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SGRBG12,
>>> +        .code        = MEDIA_BUS_FMT_SGRBG12_1X12,
>>> +        .depth        = 12,
>>> +        .csi_dt        = 0x2c,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SRGGB12P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SRGGB12,
>>> +        .code        = MEDIA_BUS_FMT_SRGGB12_1X12,
>>> +        .depth        = 12,
>>> +        .csi_dt        = 0x2c,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SBGGR14P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SBGGR14,
>>> +        .code        = MEDIA_BUS_FMT_SBGGR14_1X14,
>>> +        .depth        = 14,
>>> +        .csi_dt        = 0x2d,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SGBRG14P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SGBRG14,
>>> +        .code        = MEDIA_BUS_FMT_SGBRG14_1X14,
>>> +        .depth        = 14,
>>> +        .csi_dt        = 0x2d,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SGRBG14P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SGRBG14,
>>> +        .code        = MEDIA_BUS_FMT_SGRBG14_1X14,
>>> +        .depth        = 14,
>>> +        .csi_dt        = 0x2d,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_SRGGB14P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_SRGGB14,
>>> +        .code        = MEDIA_BUS_FMT_SRGGB14_1X14,
>>> +        .depth        = 14,
>>> +        .csi_dt        = 0x2d,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +    /*
>>> +     * 16 bit Bayer formats could be supported, but there is no CSI2
>>> +     * data_type defined for raw 16, and no sensors that produce it at
>>> +     * present.
>>> +     */
>>
>> RAW16 is now defined in CSI-2, but that can be handled later.
>>
>>> +
>>> +    /* Greyscale formats */
>>> +        .fourcc        = V4L2_PIX_FMT_GREY,
>>> +        .code        = MEDIA_BUS_FMT_Y8_1X8,
>>> +        .depth        = 8,
>>> +        .csi_dt        = 0x2a,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_Y10P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_Y10,
>>> +        .code        = MEDIA_BUS_FMT_Y10_1X10,
>>> +        .depth        = 10,
>>> +        .csi_dt        = 0x2b,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_Y12P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_Y12,
>>> +        .code        = MEDIA_BUS_FMT_Y12_1X12,
>>> +        .depth        = 12,
>>> +        .csi_dt        = 0x2c,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    }, {
>>> +        .fourcc        = V4L2_PIX_FMT_Y14P,
>>> +        .repacked_fourcc = V4L2_PIX_FMT_Y14,
>>> +        .code        = MEDIA_BUS_FMT_Y14_1X14,
>>> +        .depth        = 14,
>>> +        .csi_dt        = 0x2d,
>>> +        .valid_colorspaces = MASK_CS_RAW,
>>> +    },
>>> +    /* Embedded data format */
>>> +    {
>>> +        .fourcc        = V4L2_META_FMT_8,
>>> +        .code        = MEDIA_BUS_FMT_METADATA_8,
>>> +        .depth        = 8,
>>> +        .metadata_fmt    = 1,
>>> +    }
>>> +};
>>
>> Let's reorder the code a bit to improve readability:
>>
>> - Macro definitions
>> - Structure definitions (with the container_of wrappers following the
>>    respective structure)
>> - Misc helper functions (from is_metadata_node() to reg_write_field())
>> - Format data table and helper functions (from find_format_by_code() to
>>    unicam_calc_format_size_bpl())
>> - Hardware handling:
>>    - From unicam_wr_dma_addr() to unicam_isr()
>>    - From unicam_set_packing_config() to unicam_disable()
>> - V4L2 subdev (ops & initialization & registration)
>> - vb2 queue opq
>> - V4L2 video device (ops & initialization & registration)
>> - Probe
>>
>> I recommend adding a comment header in front of each section after the
>> structure definitions with a distinctive visual appearance, to delimit
>> code blocks very clearly. I usually use the following style:
>>
>> /* 
>> ----------------------------------------------------------------------------- 
>>
>>   * V4L2 Subdev Operations
>>   */
>>
>>> +
>>> +struct unicam_buffer {
>>> +    struct vb2_v4l2_buffer vb;
>>> +    struct list_head list;
>>> +};
>>> +
>>> +static inline struct unicam_buffer *to_unicam_buffer(struct 
>>> vb2_buffer *vb)
>>> +{
>>> +    return container_of(vb, struct unicam_buffer, vb.vb2_buf);
>>> +}
>>> +
>>> +struct unicam_node {
>>> +    bool registered;
>>> +    int open;
>>
>> Not used. Please go through all structure fields and delete the ones
>> that are not used.
>>
>>> +    bool streaming;
>>> +    unsigned int node_id;
>>> +    /* Source pad id on the sensor for this node */
>>> +    unsigned int src_pad_id;
>>> +    /* Pointer pointing to current v4l2_buffer */
>>> +    struct unicam_buffer *cur_frm;
>>> +    /* Pointer pointing to next v4l2_buffer */
>>> +    struct unicam_buffer *next_frm;
>>> +    /* video capture */
>>> +    const struct unicam_fmt *fmt;
>>> +    /* Used to store current pixel format */
>>> +    struct v4l2_format v_fmt;
>>> +    /* Used to store current mbus frame format */
>>> +    struct v4l2_mbus_framefmt m_fmt;
>>> +    /* Buffer queue used in video-buf */
>>> +    struct vb2_queue buffer_queue;
>>> +    /* Queue of filled frames */
>>> +    struct list_head dma_queue;
>>> +    /* IRQ lock for DMA queue */
>>> +    spinlock_t dma_queue_lock;
>>> +    /* lock used to access this structure */
>>> +    struct mutex lock;
>>> +    /* Identifies video device for this channel */
>>> +    struct video_device video_dev;
>>> +    /* Pointer to the parent handle */
>>> +    struct unicam_device *dev;
>>> +    struct media_pad pad;
>>> +    unsigned int embedded_lines;
>>> +    struct media_pipeline pipe;
>>> +    /*
>>> +     * Dummy buffer intended to be used by unicam
>>> +     * if we have no other queued buffers to swap to.
>>> +     */
>>> +    void *dummy_buf_cpu_addr;
>>> +    dma_addr_t dummy_buf_dma_addr;
>>> +};
>>> +
>>> +struct unicam_device {
>>> +    struct kref kref;
>>> +
>>> +    /* V4l2 specific parameters */
>>> +    struct v4l2_async_subdev asd;
>>> +
>>> +    /* peripheral base address */
>>> +    void __iomem *base;
>>> +    /* clock gating base address */
>>> +    void __iomem *clk_gate_base;
>>> +    /* lp clock handle */
>>> +    struct clk *clock;
>>> +    /* vpu clock handle */
>>> +    struct clk *vpu_clock;
>>> +    /* vpu clock request */
>>> +    struct clk_request *vpu_req;
>>
>> Not used (and that may be a problem).
> 
> In the original linux-rpi tree, there is this portion of code in 
> unicam_start_streaming:
> 
> dev->vpu_req = clk_request_start(dev->vpu_clock, MIN_VPU_CLOCK_RATE);
> if (!dev->vpu_req) {
>      unicam_err(dev, "failed to set up VPU clock\n");
>      goto error_pipeline;
> }
> 
> ret = clk_prepare_enable(dev->vpu_clock);
> if (ret) {
>      unicam_err(dev, "Failed to enable VPU clock: %d\n", ret);
>      goto error_pipeline;
> }
> 
> And this is used as this because it depends on the non-merged series 
> "clk: [PATCH v2 0/3] clk: Implement a clock request API" [1]
> 
> [1]: https://lore.kernel.org/all/20210914093515.260031-1-maxime@cerno.tech/
> 
> That's why I modified the code and called:
> clk_set_min_rate(dev->vpu_clock, UNICAM_MIN_VPU_CLOCK_RATE);
> 
> Dave, is it ok or do we need absolutely the clock request API ?
> 
> JM
> 
>>
>>> +    /* clock status for error handling */
>>> +    bool clocks_enabled;
>>> +    /* V4l2 device */
>>> +    struct v4l2_device v4l2_dev;
>>> +    struct media_device mdev;
>>> +
>>> +    /* parent device */
>>> +    struct platform_device *pdev;
>>
>> The pdev field is only used to access pdev.dev, so I'd replace it with
>> the struct device.
>>
>>> +    /* subdevice async Notifier */
>>> +    struct v4l2_async_notifier notifier;
>>> +    unsigned int sequence;
>>> +
>>> +    /* ptr to  sub device */
>>> +    struct v4l2_subdev *sensor;
>>> +    /* Pad config for the sensor */
>>> +    struct v4l2_subdev_state *sensor_state;
>>> +
>>> +    /* Internal subdev */
>>> +    struct v4l2_subdev sd;
>>> +    struct media_pad pads[UNICAM_SD_NUM_PADS];
>>> +
>>> +    bool streaming;
>>
>> "streaming" is ambiguous, it's not clear it relates to the subdev. I'd
>> group all the fields related to the subdev in a structure:
>>
>>     struct {
>>         struct v4l2_subdev sd;
>>         struct media_pad pads[UNICAM_SD_NUM_PADS];
>>         bool streaming;
>>     } subdev;
>>
>> (there may be more, I haven't checked)
>>
>>> +
>>> +    enum v4l2_mbus_type bus_type;
>>> +    /*
>>> +     * Stores bus.mipi_csi2.flags for CSI2 sensors, or
>>> +     * bus.mipi_csi1.strobe for CCP2.
>>> +     */
>>> +    unsigned int bus_flags;
>>> +    unsigned int max_data_lanes;
>>> +    unsigned int active_data_lanes;
>>> +    bool sensor_embedded_data;
>>> +
>>> +    struct unicam_node node[MAX_NODES];
>>> +    struct v4l2_ctrl_handler ctrl_handler;
>>> +
>>> +    bool mc_api;
>>> +};
>>> +
>>> +static inline struct unicam_device *
>>> +to_unicam_device(struct v4l2_device *v4l2_dev)
>>
>> I'd rename this to v4l2_device_to_unicam_device(), as there's another
>> container_of wrapper.
>>
>>> +{
>>> +    return container_of(v4l2_dev, struct unicam_device, v4l2_dev);
>>> +}
>>> +
>>> +static inline struct unicam_device *
>>> +sd_to_unicam_device(struct v4l2_subdev *sd)
>>> +{
>>> +    return container_of(sd, struct unicam_device, sd);
>>> +}
>>> +
>>> +static inline bool is_metadata_node(struct unicam_node *node)
>>> +{
>>> +    return node->video_dev.device_caps & V4L2_CAP_META_CAPTURE;
>>> +}
>>> +
>>> +static inline bool is_image_node(struct unicam_node *node)
>>> +{
>>> +    return node->video_dev.device_caps & V4L2_CAP_VIDEO_CAPTURE;
>>> +}
>>> +
>>> +/* Hardware access */
>>> +static inline void clk_write(struct unicam_device *dev, u32 val)
>>> +{
>>> +    writel(val | 0x5a000000, dev->clk_gate_base);
>>> +}
>>
>> The name of this function and the functions below are too generic. Add a
>> unicam_ prefix.
>>
>>> +
>>> +static inline u32 reg_read(struct unicam_device *dev, u32 offset)
>>> +{
>>> +    return readl(dev->base + offset);
>>> +}
>>> +
>>> +static inline void reg_write(struct unicam_device *dev, u32 offset, 
>>> u32 val)
>>> +{
>>> +    writel(val, dev->base + offset);
>>> +}
>>> +
>>> +static inline int get_field(u32 value, u32 mask)
>>> +{
>>> +    return (value & mask) >> __ffs(mask);
>>> +}
>>> +
>>> +static inline void set_field(u32 *valp, u32 field, u32 mask)
>>> +{
>>> +    u32 val = *valp;
>>> +
>>> +    val &= ~mask;
>>> +    val |= (field << __ffs(mask)) & mask;
>>> +    *valp = val;
>>> +}
>>> +
>>> +static inline u32 reg_read_field(struct unicam_device *dev, u32 offset,
>>> +                 u32 mask)
>>
>> Not used.
>>
>>> +{
>>> +    return get_field(reg_read(dev, offset), mask);
>>> +}
>>> +
>>> +static inline void reg_write_field(struct unicam_device *dev, u32 
>>> offset,
>>> +                   u32 field, u32 mask)
>>> +{
>>> +    u32 val = reg_read(dev, offset);
>>> +
>>> +    set_field(&val, field, mask);
>>> +    reg_write(dev, offset, val);
>>> +}
>>
>> I'm really not a bit fan of the read-modify-update patterns, more often
>> than not they can be replaced by direct writes. No need to fix this
>> though, it's ok.
>>
>>> +
>>> +/* Power management functions */
>>> +static inline int unicam_runtime_get(struct unicam_device *dev)
>>> +{
>>> +    return pm_runtime_get_sync(&dev->pdev->dev);
>>
>> Use pm_runtime_resume_and_get() instead, as the error handling is broken
>> in the driver otherwise.
>>
>>> +}
>>> +
>>> +static inline void unicam_runtime_put(struct unicam_device *dev)
>>> +{
>>> +    pm_runtime_put_sync(&dev->pdev->dev);
>>> +}
>>
>> Drop these wrappers and call the functions directly.
>>
>>> +
>>> +/* Format setup functions */
>>> +static const struct unicam_fmt *find_format_by_code(u32 code)
>>> +{
>>> +    unsigned int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(formats); i++) {
>>> +        if (formats[i].code == code)
>>> +            return &formats[i];
>>> +    }
>>> +
>>> +    return NULL;
>>> +}
>>> +
>>> +static const struct unicam_fmt *find_format_by_fourcc(u32 fourcc)
>>> +{
>>> +    unsigned int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(formats); ++i) {
>>> +        if (formats[i].fourcc == fourcc)
>>> +            return &formats[i];
>>> +    }
>>> +
>>> +    return NULL;
>>> +}
>>> +
>>> +static unsigned int bytes_per_line(u32 width, const struct 
>>> unicam_fmt *fmt,
>>> +                   u32 v4l2_fourcc)
>>> +{
>>> +    if (v4l2_fourcc == fmt->repacked_fourcc)
>>> +        /* Repacking always goes to 16bpp */
>>> +        return ALIGN(width << 1, BPL_ALIGNMENT);
>>> +    else
>>> +        return ALIGN((width * fmt->depth) >> 3, BPL_ALIGNMENT);
>>> +}
>>> +
>>> +static int unicam_calc_format_size_bpl(struct unicam_device *dev,
>>> +                       const struct unicam_fmt *fmt,
>>> +                       struct v4l2_format *f)
>>> +{
>>> +    unsigned int min_bytesperline;
>>> +
>>> +    v4l_bound_align_image(&f->fmt.pix.width, MIN_WIDTH, MAX_WIDTH, 2,
>>> +                  &f->fmt.pix.height, MIN_HEIGHT, MAX_HEIGHT, 0,
>>> +                  0);
>>> +
>>> +    min_bytesperline = bytes_per_line(f->fmt.pix.width, fmt,
>>> +                      f->fmt.pix.pixelformat);
>>> +
>>> +    if (f->fmt.pix.bytesperline > min_bytesperline &&
>>> +        f->fmt.pix.bytesperline <= MAX_BYTESPERLINE)
>>> +        f->fmt.pix.bytesperline = ALIGN(f->fmt.pix.bytesperline,
>>> +                        BPL_ALIGNMENT);
>>> +    else
>>> +        f->fmt.pix.bytesperline = min_bytesperline;
>>> +
>>> +    f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
>>> +
>>> +    dev_dbg(dev->v4l2_dev.dev, "%s: fourcc: %08X size: %dx%d bpl:%d 
>>> img_size:%d\n",
>>> +        __func__,
>>> +        f->fmt.pix.pixelformat,
>>> +        f->fmt.pix.width, f->fmt.pix.height,
>>> +        f->fmt.pix.bytesperline, f->fmt.pix.sizeimage);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void unicam_wr_dma_addr(struct unicam_device *dev, dma_addr_t 
>>> dmaaddr,
>>> +                   unsigned int buffer_size, int node_id)
>>> +{
>>> +    dma_addr_t endaddr = dmaaddr + buffer_size;
>>> +
>>> +    if (node_id == IMAGE_NODE) {
>>> +        reg_write(dev, UNICAM_IBSA0, dmaaddr);
>>> +        reg_write(dev, UNICAM_IBEA0, endaddr);
>>> +    } else {
>>> +        reg_write(dev, UNICAM_DBSA0, dmaaddr);
>>> +        reg_write(dev, UNICAM_DBEA0, endaddr);
>>> +    }
>>> +}
>>> +
>>> +static unsigned int unicam_get_lines_done(struct unicam_device *dev)
>>> +{
>>> +    dma_addr_t start_addr, cur_addr;
>>> +    unsigned int stride = 
>>> dev->node[IMAGE_NODE].v_fmt.fmt.pix.bytesperline;
>>> +    struct unicam_buffer *frm = dev->node[IMAGE_NODE].cur_frm;
>>> +
>>> +    if (!frm)
>>> +        return 0;
>>> +
>>> +    start_addr = vb2_dma_contig_plane_dma_addr(&frm->vb.vb2_buf, 0);
>>> +    cur_addr = reg_read(dev, UNICAM_IBWP);
>>> +    return (unsigned int)(cur_addr - start_addr) / stride;
>>> +}
>>> +
>>> +static void unicam_schedule_next_buffer(struct unicam_node *node)
>>> +{
>>> +    struct unicam_device *dev = node->dev;
>>> +    struct unicam_buffer *buf;
>>> +    unsigned int size;
>>> +    dma_addr_t addr;
>>> +
>>> +    buf = list_first_entry(&node->dma_queue, struct unicam_buffer, 
>>> list);
>>> +    node->next_frm = buf;
>>> +    list_del(&buf->list);
>>> +
>>> +    addr = vb2_dma_contig_plane_dma_addr(&buf->vb.vb2_buf, 0);
>>> +    if (is_image_node(node)) {
>>> +        size = node->v_fmt.fmt.pix.sizeimage;
>>> +        unicam_wr_dma_addr(dev, addr, size, IMAGE_NODE);
>>> +    } else {
>>> +        size = node->v_fmt.fmt.meta.buffersize;
>>> +        unicam_wr_dma_addr(dev, addr, size, METADATA_NODE);
>>> +    }
>>> +}
>>> +
>>> +static void unicam_schedule_dummy_buffer(struct unicam_node *node)
>>> +{
>>> +    struct unicam_device *dev = node->dev;
>>> +    int node_id = is_image_node(node) ? IMAGE_NODE : METADATA_NODE;
>>> +
>>> +    dev_dbg(dev->v4l2_dev.dev, "Scheduling dummy buffer for node 
>>> %d\n", node_id);
>>> +
>>> +    unicam_wr_dma_addr(dev, node->dummy_buf_dma_addr, DUMMY_BUF_SIZE,
>>> +               node_id);
>>> +    node->next_frm = NULL;
>>> +}
>>> +
>>> +static void unicam_process_buffer_complete(struct unicam_node *node,
>>> +                       unsigned int sequence)
>>> +{
>>> +    node->cur_frm->vb.field = node->m_fmt.field;
>>> +    node->cur_frm->vb.sequence = sequence;
>>> +
>>> +    vb2_buffer_done(&node->cur_frm->vb.vb2_buf, VB2_BUF_STATE_DONE);
>>> +}
>>> +
>>> +static void unicam_queue_event_sof(struct unicam_device *unicam)
>>> +{
>>> +    struct v4l2_event event = {
>>> +        .type = V4L2_EVENT_FRAME_SYNC,
>>> +        .u.frame_sync.frame_sequence = unicam->sequence,
>>> +    };
>>> +
>>> +    v4l2_event_queue(&unicam->node[IMAGE_NODE].video_dev, &event);
>>> +}
>>> +
>>> +/*
>>> + * unicam_isr : ISR handler for unicam capture
>>> + * @irq: irq number
>>> + * @dev_id: dev_id ptr
>>> + *
>>> + * It changes status of the captured buffer, takes next buffer from 
>>> the queue
>>> + * and sets its address in unicam registers
>>> + */
>>> +static irqreturn_t unicam_isr(int irq, void *dev)
>>> +{
>>> +    struct unicam_device *unicam = dev;
>>> +    unsigned int lines_done = unicam_get_lines_done(dev);
>>> +    unsigned int sequence = unicam->sequence;
>>> +    unsigned int i;
>>> +    u32 ista, sta;
>>> +    bool fe;
>>> +    u64 ts;
>>> +
>>> +    sta = reg_read(unicam, UNICAM_STA);
>>> +    /* Write value back to clear the interrupts */
>>> +    reg_write(unicam, UNICAM_STA, sta);
>>> +
>>> +    ista = reg_read(unicam, UNICAM_ISTA);
>>> +    /* Write value back to clear the interrupts */
>>> +    reg_write(unicam, UNICAM_ISTA, ista);
>>> +
>>> +    dev_dbg(unicam->v4l2_dev.dev, "ISR: ISTA: 0x%X, STA: 0x%X, 
>>> sequence %d, lines done %d",
>>> +        ista, sta, sequence, lines_done);
>>> +
>>> +    if (!(sta & (UNICAM_IS | UNICAM_PI0)))
>>> +        return IRQ_HANDLED;
>>> +
>>> +    /*
>>> +     * Look for either the Frame End interrupt or the Packet Capture 
>>> status
>>> +     * to signal a frame end.
>>> +     */
>>> +    fe = (ista & UNICAM_FEI || sta & UNICAM_PI0);
>>
>> No need for the outer parentheses.
>>
>>> +
>>> +    /*
>>> +     * We must run the frame end handler first. If we have a valid 
>>> next_frm
>>> +     * and we get a simultaneout FE + FS interrupt, running the FS 
>>> handler
>>> +     * first would null out the next_frm ptr and we would have lost the
>>> +     * buffer forever.
>>> +     */
>>> +    if (fe) {
>>> +        /*
>>> +         * Ensure we have swapped buffers already as we can't
>>> +         * stop the peripheral. If no buffer is available, use a
>>> +         * dummy buffer to dump out frames until we get a new buffer
>>> +         * to use.
>>> +         */
>>> +        for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
>>> +            if (!unicam->node[i].streaming)
>>> +                continue;
>>> +
>>> +            /*
>>> +             * If cur_frm == next_frm, it means we have not had
>>> +             * a chance to swap buffers, likely due to having
>>> +             * multiple interrupts occurring simultaneously (like FE
>>> +             * + FS + LS). In this case, we cannot signal the buffer
>>> +             * as complete, as the HW will reuse that buffer.
>>> +             */
>>> +            if (unicam->node[i].cur_frm &&
>>> +                unicam->node[i].cur_frm != unicam->node[i].next_frm)
>>> +                unicam_process_buffer_complete(&unicam->node[i],
>>> +                                   sequence);
>>> +            unicam->node[i].cur_frm = unicam->node[i].next_frm;
>>> +        }
>>> +        unicam->sequence++;
>>> +    }
>>> +
>>> +    if (ista & UNICAM_FSI) {
>>> +        /*
>>> +         * Timestamp is to be when the first data byte was captured,
>>> +         * aka frame start.
>>> +         */
>>> +        ts = ktime_get_ns();
>>> +        for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
>>> +            if (!unicam->node[i].streaming)
>>> +                continue;
>>> +
>>> +            if (unicam->node[i].cur_frm)
>>> +                unicam->node[i].cur_frm->vb.vb2_buf.timestamp =
>>> +                                ts;
>>> +            else
>>> +                dev_dbg(unicam->v4l2_dev.dev, "ISR: [%d] Dropping 
>>> frame, buffer not available at FS\n",
>>> +                    i);
>>> +            /*
>>> +             * Set the next frame output to go to a dummy frame
>>> +             * if we have not managed to obtain another frame
>>> +             * from the queue.
>>> +             */
>>> +            unicam_schedule_dummy_buffer(&unicam->node[i]);
>>> +        }
>>> +
>>> +        unicam_queue_event_sof(unicam);
>>> +    }
>>> +
>>> +    /*
>>> +     * Cannot swap buffer at frame end, there may be a race condition
>>> +     * where the HW does not actually swap it if the new frame has
>>> +     * already started.
>>> +     */
>>> +    if (ista & (UNICAM_FSI | UNICAM_LCI) && !fe) {
>>> +        for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
>>> +            if (!unicam->node[i].streaming)
>>> +                continue;
>>> +
>>> +            spin_lock(&unicam->node[i].dma_queue_lock);
>>> +            if (!list_empty(&unicam->node[i].dma_queue) &&
>>> +                !unicam->node[i].next_frm)
>>> +                unicam_schedule_next_buffer(&unicam->node[i]);
>>> +            spin_unlock(&unicam->node[i].dma_queue_lock);
>>> +        }
>>> +    }
>>> +
>>> +    if (reg_read(unicam, UNICAM_ICTL) & UNICAM_FCM) {
>>> +        /* Switch out of trigger mode if selected */
>>> +        reg_write_field(unicam, UNICAM_ICTL, 1, UNICAM_TFC);
>>> +        reg_write_field(unicam, UNICAM_ICTL, 0, UNICAM_FCM);
>>> +    }
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +/* V4L2 Common IOCTLs */
>>> +static int unicam_querycap(struct file *file, void *priv,
>>> +               struct v4l2_capability *cap)
>>> +{
>>> +    struct unicam_node *node = video_drvdata(file);
>>> +    struct unicam_device *dev = node->dev;
>>> +
>>> +    strscpy(cap->driver, UNICAM_MODULE_NAME, sizeof(cap->driver));
>>> +    strscpy(cap->card, UNICAM_MODULE_NAME, sizeof(cap->card));
>>> +
>>> +    snprintf(cap->bus_info, sizeof(cap->bus_info),
>>> +         "platform:%s", dev_name(&dev->pdev->dev));
>>> +
>>> +    cap->capabilities |= V4L2_CAP_VIDEO_CAPTURE | 
>>> V4L2_CAP_META_CAPTURE;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int unicam_log_status(struct file *file, void *fh)
>>> +{
>>> +    struct unicam_node *node = video_drvdata(file);
>>> +    struct unicam_device *dev = node->dev;
>>> +    u32 reg;
>>> +
>>> +    /* status for sub devices */
>>> +    v4l2_device_call_all(&dev->v4l2_dev, 0, core, log_status);
>>
>> I'm tempted to drop this, bit I won't insist.
>>
>>> +
>>> +    dev_info(dev->v4l2_dev.dev, "-----Receiver status-----\n");
>>> +    dev_info(dev->v4l2_dev.dev, "V4L2 width/height:   %ux%u\n",
>>> +         node->v_fmt.fmt.pix.width, node->v_fmt.fmt.pix.height);
>>> +    dev_info(dev->v4l2_dev.dev, "Mediabus format:     %08x\n", 
>>> node->fmt->code);
>>> +    dev_info(dev->v4l2_dev.dev, "V4L2 format:         %08x\n",
>>> +         node->v_fmt.fmt.pix.pixelformat);
>>> +    reg = reg_read(dev, UNICAM_IPIPE);
>>> +    dev_info(dev->v4l2_dev.dev, "Unpacking/packing:   %u / %u\n",
>>> +         get_field(reg, UNICAM_PUM_MASK),
>>> +         get_field(reg, UNICAM_PPM_MASK));
>>> +    dev_info(dev->v4l2_dev.dev, "----Live data----\n");
>>> +    dev_info(dev->v4l2_dev.dev, "Programmed stride:   %4u\n",
>>> +         reg_read(dev, UNICAM_IBLS));
>>> +    dev_info(dev->v4l2_dev.dev, "Detected resolution: %ux%u\n",
>>> +         reg_read(dev, UNICAM_IHSTA),
>>> +         reg_read(dev, UNICAM_IVSTA));
>>> +    dev_info(dev->v4l2_dev.dev, "Write pointer:       %08x\n",
>>> +         reg_read(dev, UNICAM_IBWP));
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int unicam_subscribe_event(struct v4l2_fh *fh,
>>> +                  const struct v4l2_event_subscription *sub)
>>> +{
>>> +    switch (sub->type) {
>>> +    case V4L2_EVENT_FRAME_SYNC:
>>> +        return v4l2_event_subscribe(fh, sub, 2, NULL);
>>> +    case V4L2_EVENT_SOURCE_CHANGE:
>>> +        return v4l2_event_subscribe(fh, sub, 4, NULL);
>>
>> The drive doesn't generate V4L2_EVENT_SOURCE_CHANGE events, drop it.
>>
>>> +    }
>>> +
>>> +    return v4l2_ctrl_subscribe_event(fh, sub);
>>> +}
>>> +
>>> +static void unicam_notify(struct v4l2_subdev *sd,
>>> +              unsigned int notification, void *arg)
>>> +{
>>> +    struct unicam_device *dev = to_unicam_device(sd->v4l2_dev);
>>
>> Use sd_to_unicam_device().
>>
>>> +
>>> +    switch (notification) {
>>> +    case V4L2_DEVICE_NOTIFY_EVENT:
>>> +        v4l2_event_queue(&dev->node[IMAGE_NODE].video_dev, arg);
>>> +        break;
>>> +    default:
>>> +        break;
>>> +    }
>>> +}
>>
>> Not needed, drop it.
>>
>>> +
>>> +/* V4L2 Media Controller Centric IOCTLs */
>>> +
>>> +static int unicam_mc_enum_fmt_vid_cap(struct file *file, void  *priv,
>>> +                      struct v4l2_fmtdesc *f)
>>
>> Drop "mc" in all function names as that's the only option now.
>>
>>> +{
>>> +    int i, j;
>>
>> Never negative, can be unsigned int.
>>
>> j is not a loop counter but the format index, I'd rename it to index to
>> make it clearer.
>>
>>> +
>>> +    for (i = 0, j = 0; i < ARRAY_SIZE(formats); i++) {
>>> +        if (f->mbus_code && formats[i].code != f->mbus_code)
>>> +            continue;
>>> +        if (formats[i].mc_skip || formats[i].metadata_fmt)
>>> +            continue;
>>> +
>>> +        if (formats[i].fourcc) {
>>
>> All entries have a fourcc so you can drop this check.
>>
>>> +            if (j == f->index) {
>>> +                f->pixelformat = formats[i].fourcc;
>>> +                f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>
>> Not needed, the type is an input parameter. Same below.
>>
>>> +                return 0;
>>> +            }
>>> +            j++;
>>> +        }
>>> +        if (formats[i].repacked_fourcc) {
>>> +            if (j == f->index) {
>>> +                f->pixelformat = formats[i].repacked_fourcc;
>>> +                f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +                return 0;
>>> +            }
>>> +            j++;
>>> +        }
>>> +    }
>>> +
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static int unicam_mc_g_fmt_vid_cap(struct file *file, void *priv,
>>> +                   struct v4l2_format *f)
>>> +{
>>> +    struct unicam_node *node = video_drvdata(file);
>>> +
>>> +    if (!is_image_node(node))
>>> +        return -EINVAL;
>>
>> Can this happen ?
>>
>>> +
>>> +    *f = node->v_fmt;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void unicam_mc_try_fmt(struct unicam_node *node, struct 
>>> v4l2_format *f,
>>> +                  const struct unicam_fmt **ret_fmt)
>>> +{
>>> +    struct v4l2_pix_format *v4l2_format = &f->fmt.pix;
>>> +    struct unicam_device *dev = node->dev;
>>> +    const struct unicam_fmt *fmt;
>>> +    int is_rgb;
>>
>> bool.
>>
>>> +
>>> +    /*
>>> +     * Default to the first format if the requested pixel format 
>>> code isn't
>>> +     * supported.
>>> +     */
>>> +    fmt = find_format_by_fourcc(v4l2_format->pixelformat);
>>> +    if (!fmt) {
>>> +        fmt = &formats[0];
>>> +        v4l2_format->pixelformat = fmt->fourcc;
>>> +    }
>>> +
>>> +    unicam_calc_format_size_bpl(dev, fmt, f);
>>> +
>>> +    if (v4l2_format->field == V4L2_FIELD_ANY)
>>> +        v4l2_format->field = V4L2_FIELD_NONE;
>>> +
>>> +    if (v4l2_format->colorspace >= MAX_COLORSPACE ||
>>> +        !(fmt->valid_colorspaces & (1 << v4l2_format->colorspace))) {
>>> +        v4l2_format->colorspace = __ffs(fmt->valid_colorspaces);
>>> +
>>> +        v4l2_format->xfer_func =
>>> +            V4L2_MAP_XFER_FUNC_DEFAULT(v4l2_format->colorspace);
>>> +        v4l2_format->ycbcr_enc =
>>> +            V4L2_MAP_YCBCR_ENC_DEFAULT(v4l2_format->colorspace);
>>> +        is_rgb = v4l2_format->colorspace == V4L2_COLORSPACE_SRGB;
>>> +        v4l2_format->quantization =
>>> +            V4L2_MAP_QUANTIZATION_DEFAULT(is_rgb,
>>> +                              v4l2_format->colorspace,
>>> +                              v4l2_format->ycbcr_enc);
>>> +    }
>>
>> Drop this, we can accept any colorspace produced by the sensor.
>>
>>> +
>>> +    if (ret_fmt)
>>> +        *ret_fmt = fmt;
>>
>> How about returning the format from the function instead ?
>>
>>> +
>>> +    dev_dbg(dev->v4l2_dev.dev, "%s: %08x %ux%u (bytesperline %u 
>>> sizeimage %u)\n",
>>> +        __func__, v4l2_format->pixelformat,
>>> +        v4l2_format->width, v4l2_format->height,
>>> +        v4l2_format->bytesperline, v4l2_format->sizeimage);
>>> +}
>>> +
>>> +static int unicam_mc_try_fmt_vid_cap(struct file *file, void *priv,
>>> +                     struct v4l2_format *f)
>>> +{
>>> +    struct unicam_node *node = video_drvdata(file);
>>> +
>>> +    unicam_mc_try_fmt(node, f, NULL);
>>> +    return 0;
>>> +}
>>> +
>>> +static int unicam_mc_s_fmt_vid_cap(struct file *file, void *priv,
>>> +                   struct v4l2_format *f)
>>> +{
>>> +    struct unicam_node *node = video_drvdata(file);
>>> +    struct unicam_device *dev = node->dev;
>>> +    const struct unicam_fmt *fmt;
>>> +
>>> +    if (vb2_is_busy(&node->buffer_queue)) {
>>> +        dev_dbg(dev->v4l2_dev.dev, "%s device busy\n", __func__);
>>> +        return -EBUSY;
>>> +    }
>>> +
>>> +    unicam_mc_try_fmt(node, f, &fmt);
>>> +
>>> +    node->v_fmt = *f;
>>> +    node->fmt = fmt;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int unicam_mc_enum_framesizes(struct file *file, void *fh,
>>> +                     struct v4l2_frmsizeenum *fsize)
>>> +{
>>> +    struct unicam_node *node = video_drvdata(file);
>>> +    struct unicam_device *dev = node->dev;
>>> +
>>> +    if (fsize->index > 0)
>>> +        return -EINVAL;
>>> +
>>> +    if (!find_format_by_fourcc(fsize->pixel_format)) {
>>> +        dev_dbg(dev->v4l2_dev.dev, "Invalid pixel format 0x%08x\n",
>>> +            fsize->pixel_format);
>>
>> I'd drop this message.
>>
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
>>> +    fsize->stepwise.min_width = MIN_WIDTH;
>>> +    fsize->stepwise.max_width = MAX_WIDTH;
>>> +    fsize->stepwise.step_width = 1;
>>> +    fsize->stepwise.min_height = MIN_HEIGHT;
>>> +    fsize->stepwise.max_height = MAX_HEIGHT;
>>> +    fsize->stepwise.step_height = 1;
>>
>> Is this valid for the metadata node too ?
>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int unicam_mc_enum_fmt_meta_cap(struct file *file, void  *priv,
>>> +                       struct v4l2_fmtdesc *f)
>>> +{
>>> +    struct unicam_node *node = video_drvdata(file);
>>> +    int i, j;
>>> +
>>> +    if (!is_metadata_node(node))
>>> +        return -EINVAL;
>>
>> Same comments as for video format enumeration.
>>
>>> +
>>> +    for (i = 0, j = 0; i < ARRAY_SIZE(formats); i++) {
>>> +        if (f->mbus_code && formats[i].code != f->mbus_code)
>>> +            continue;
>>> +        if (!formats[i].metadata_fmt)
>>> +            continue;
>>> +
>>> +        if (formats[i].fourcc) {
>>> +            if (j == f->index) {
>>> +                f->pixelformat = formats[i].fourcc;
>>> +                f->type = V4L2_BUF_TYPE_META_CAPTURE;
>>> +                return 0;
>>> +            }
>>> +            j++;
>>> +        }
>>> +    }
>>> +
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static int unicam_mc_g_fmt_meta_cap(struct file *file, void *priv,
>>> +                    struct v4l2_format *f)
>>> +{
>>> +    struct unicam_node *node = video_drvdata(file);
>>> +
>>> +    if (!is_metadata_node(node))
>>> +        return -EINVAL;
>>
>> Can this happen ? Same below.
>>
>>> +
>>> +    *f = node->v_fmt;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int unicam_mc_try_fmt_meta_cap(struct file *file, void *priv,
>>> +                      struct v4l2_format *f)
>>> +{
>>> +    struct unicam_node *node = video_drvdata(file);
>>> +
>>> +    if (!is_metadata_node(node))
>>> +        return -EINVAL;
>>> +
>>> +    f->fmt.meta.dataformat = V4L2_META_FMT_8;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int unicam_mc_s_fmt_meta_cap(struct file *file, void *priv,
>>> +                    struct v4l2_format *f)
>>> +{
>>> +    struct unicam_node *node = video_drvdata(file);
>>> +
>>> +    if (!is_metadata_node(node))
>>> +        return -EINVAL;
>>> +
>>> +    unicam_mc_try_fmt_meta_cap(file, priv, f);
>>> +
>>> +    node->v_fmt = *f;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct v4l2_ioctl_ops unicam_mc_ioctl_ops = {
>>> +    .vidioc_querycap      = unicam_querycap,
>>> +    .vidioc_enum_fmt_vid_cap  = unicam_mc_enum_fmt_vid_cap,
>>> +    .vidioc_g_fmt_vid_cap     = unicam_mc_g_fmt_vid_cap,
>>> +    .vidioc_try_fmt_vid_cap   = unicam_mc_try_fmt_vid_cap,
>>> +    .vidioc_s_fmt_vid_cap     = unicam_mc_s_fmt_vid_cap,
>>> +
>>> +    .vidioc_enum_fmt_meta_cap    = unicam_mc_enum_fmt_meta_cap,
>>> +    .vidioc_g_fmt_meta_cap        = unicam_mc_g_fmt_meta_cap,
>>> +    .vidioc_try_fmt_meta_cap    = unicam_mc_try_fmt_meta_cap,
>>> +    .vidioc_s_fmt_meta_cap        = unicam_mc_s_fmt_meta_cap,
>>> +
>>> +    .vidioc_enum_framesizes   = unicam_mc_enum_framesizes,
>>> +    .vidioc_reqbufs       = vb2_ioctl_reqbufs,
>>> +    .vidioc_create_bufs   = vb2_ioctl_create_bufs,
>>> +    .vidioc_prepare_buf   = vb2_ioctl_prepare_buf,
>>> +    .vidioc_querybuf      = vb2_ioctl_querybuf,
>>> +    .vidioc_qbuf          = vb2_ioctl_qbuf,
>>> +    .vidioc_dqbuf         = vb2_ioctl_dqbuf,
>>> +    .vidioc_expbuf        = vb2_ioctl_expbuf,
>>> +    .vidioc_streamon      = vb2_ioctl_streamon,
>>> +    .vidioc_streamoff     = vb2_ioctl_streamoff,
>>> +
>>> +    .vidioc_log_status        = unicam_log_status,
>>> +    .vidioc_subscribe_event        = unicam_subscribe_event,
>>> +    .vidioc_unsubscribe_event    = v4l2_event_unsubscribe,
>>> +};
>>> +
>>> +static const struct media_entity_operations unicam_mc_entity_ops = {
>>> +    .link_validate = v4l2_subdev_link_validate,
>>> +    .has_route = v4l2_subdev_has_route,
>>> +};
>>
>> I don't think this is needed for the video nodes.
>>
>>> +
>>> +/* videobuf2 Operations */
>>> +
>>> +static int unicam_queue_setup(struct vb2_queue *vq,
>>> +                  unsigned int *nbuffers,
>>> +                  unsigned int *nplanes,
>>> +                  unsigned int sizes[],
>>> +                  struct device *alloc_devs[])
>>> +{
>>> +    struct unicam_node *node = vb2_get_drv_priv(vq);
>>> +    struct unicam_device *dev = node->dev;
>>> +    unsigned int size = is_image_node(node) ?
>>> +                node->v_fmt.fmt.pix.sizeimage :
>>> +                node->v_fmt.fmt.meta.buffersize;
>>> +
>>> +    if (vq->num_buffers + *nbuffers < 3)
>>> +        *nbuffers = 3 - vq->num_buffers;
>>> +
>>> +    if (*nplanes) {
>>> +        if (sizes[0] < size) {
>>> +            dev_err(dev->v4l2_dev.dev, "sizes[0] %i < size %u\n", 
>>> sizes[0],
>>> +                size);
>>
>> This can be a debug message, let's not give a way to unprivileged
>> userspace to flood the kernel log.
>>
>>> +            return -EINVAL;
>>> +        }
>>> +        size = sizes[0];
>>> +    }
>>> +
>>> +    *nplanes = 1;
>>> +    sizes[0] = size;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int unicam_buffer_prepare(struct vb2_buffer *vb)
>>> +{
>>> +    struct unicam_node *node = vb2_get_drv_priv(vb->vb2_queue);
>>> +    struct unicam_device *dev = node->dev;
>>> +    struct unicam_buffer *buf = to_unicam_buffer(vb);
>>> +    unsigned long size;
>>> +
>>> +    if (WARN_ON(!node->fmt))
>>> +        return -EINVAL;
>>> +
>>> +    size = is_image_node(node) ? node->v_fmt.fmt.pix.sizeimage :
>>> +                     node->v_fmt.fmt.meta.buffersize;
>>> +    if (vb2_plane_size(vb, 0) < size) {
>>> +        dev_err(dev->v4l2_dev.dev, "data will not fit into plane 
>>> (%lu < %lu)\n",
>>> +            vb2_plane_size(vb, 0), size);
>>
>> Debug message here too.
>>
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    vb2_set_plane_payload(&buf->vb.vb2_buf, 0, size);
>>> +    return 0;
>>> +}
>>> +
>>> +static void unicam_buffer_queue(struct vb2_buffer *vb)
>>> +{
>>> +    struct unicam_node *node = vb2_get_drv_priv(vb->vb2_queue);
>>> +    struct unicam_buffer *buf = to_unicam_buffer(vb);
>>> +    unsigned long flags;
>>> +
>>> +    spin_lock_irqsave(&node->dma_queue_lock, flags);
>>
>> Small optimization, as this is guaranteed to be called with interrupts
>> enabled, you can use the spin_lock_irq() variant.
>>
>>> +    list_add_tail(&buf->list, &node->dma_queue);
>>> +    spin_unlock_irqrestore(&node->dma_queue_lock, flags);
>>> +}
>>> +
>>> +static void unicam_set_packing_config(struct unicam_device *dev)
>>> +{
>>> +    u32 pack, unpack;
>>> +    u32 val;
>>> +
>>> +    if (dev->node[IMAGE_NODE].v_fmt.fmt.pix.pixelformat ==
>>> +        dev->node[IMAGE_NODE].fmt->fourcc) {
>>> +        unpack = UNICAM_PUM_NONE;
>>> +        pack = UNICAM_PPM_NONE;
>>> +    } else {
>>> +        switch (dev->node[IMAGE_NODE].fmt->depth) {
>>> +        case 8:
>>> +            unpack = UNICAM_PUM_UNPACK8;
>>> +            break;
>>> +        case 10:
>>> +            unpack = UNICAM_PUM_UNPACK10;
>>> +            break;
>>> +        case 12:
>>> +            unpack = UNICAM_PUM_UNPACK12;
>>> +            break;
>>> +        case 14:
>>> +            unpack = UNICAM_PUM_UNPACK14;
>>> +            break;
>>> +        case 16:
>>> +            unpack = UNICAM_PUM_UNPACK16;
>>> +            break;
>>> +        default:
>>> +            unpack = UNICAM_PUM_NONE;
>>> +            break;
>>> +        }
>>> +
>>> +        /* Repacking is always to 16bpp */
>>> +        pack = UNICAM_PPM_PACK16;
>>> +    }
>>> +
>>> +    val = 0;
>>> +    set_field(&val, unpack, UNICAM_PUM_MASK);
>>> +    set_field(&val, pack, UNICAM_PPM_MASK);
>>> +    reg_write(dev, UNICAM_IPIPE, val);
>>> +}
>>> +
>>> +static void unicam_cfg_image_id(struct unicam_device *dev)
>>> +{
>>> +    if (dev->bus_type == V4L2_MBUS_CSI2_DPHY) {
>>> +        /* CSI2 mode, hardcode VC 0 for now. */
>>> +        reg_write(dev, UNICAM_IDI0,
>>> +              (0 << 6) | dev->node[IMAGE_NODE].fmt->csi_dt);
>>> +    } else {
>>> +        /* CCP2 mode */
>>> +        reg_write(dev, UNICAM_IDI0,
>>> +              0x80 | dev->node[IMAGE_NODE].fmt->csi_dt);
>>> +    }
>>> +}
>>> +
>>> +static void unicam_enable_ed(struct unicam_device *dev)
>>> +{
>>> +    u32 val = reg_read(dev, UNICAM_DCS);
>>> +
>>> +    set_field(&val, 2, UNICAM_EDL_MASK);
>>> +    /* Do not wrap at the end of the embedded data buffer */
>>> +    set_field(&val, 0, UNICAM_DBOB);
>>> +
>>> +    reg_write(dev, UNICAM_DCS, val);
>>> +}
>>> +
>>> +static void unicam_start_rx(struct unicam_device *dev, dma_addr_t 
>>> *addr)
>>> +{
>>> +    int line_int_freq = dev->node[IMAGE_NODE].v_fmt.fmt.pix.height 
>>> >> 2;
>>> +    unsigned int size, i;
>>> +    u32 val;
>>> +
>>> +    if (line_int_freq < 128)
>>> +        line_int_freq = 128;
>>> +
>>> +    /* Enable lane clocks */
>>> +    val = 1;
>>> +    for (i = 0; i < dev->active_data_lanes; i++)
>>> +        val = val << 2 | 1;
>>> +    clk_write(dev, val);
>>> +
>>> +    /* Basic init */
>>> +    reg_write(dev, UNICAM_CTRL, UNICAM_MEM);
>>> +
>>> +    /* Enable analogue control, and leave in reset. */
>>> +    val = UNICAM_AR;
>>> +    set_field(&val, 7, UNICAM_CTATADJ_MASK);
>>> +    set_field(&val, 7, UNICAM_PTATADJ_MASK);
>>> +    reg_write(dev, UNICAM_ANA, val);
>>> +    usleep_range(1000, 2000);
>>> +
>>> +    /* Come out of reset */
>>> +    reg_write_field(dev, UNICAM_ANA, 0, UNICAM_AR);
>>> +
>>> +    /* Peripheral reset */
>>> +    reg_write_field(dev, UNICAM_CTRL, 1, UNICAM_CPR);
>>> +    reg_write_field(dev, UNICAM_CTRL, 0, UNICAM_CPR);
>>> +
>>> +    reg_write_field(dev, UNICAM_CTRL, 0, UNICAM_CPE);
>>> +
>>> +    /* Enable Rx control. */
>>> +    val = reg_read(dev, UNICAM_CTRL);
>>> +    if (dev->bus_type == V4L2_MBUS_CSI2_DPHY) {
>>> +        set_field(&val, UNICAM_CPM_CSI2, UNICAM_CPM_MASK);
>>> +        set_field(&val, UNICAM_DCM_STROBE, UNICAM_DCM_MASK);
>>> +    } else {
>>> +        set_field(&val, UNICAM_CPM_CCP2, UNICAM_CPM_MASK);
>>> +        set_field(&val, dev->bus_flags, UNICAM_DCM_MASK);
>>> +    }
>>> +    /* Packet framer timeout */
>>> +    set_field(&val, 0xf, UNICAM_PFT_MASK);
>>> +    set_field(&val, 128, UNICAM_OET_MASK);
>>> +    reg_write(dev, UNICAM_CTRL, val);
>>> +
>>> +    reg_write(dev, UNICAM_IHWIN, 0);
>>> +    reg_write(dev, UNICAM_IVWIN, 0);
>>> +
>>> +    /* AXI bus access QoS setup */
>>> +    val = reg_read(dev, UNICAM_PRI);
>>> +    set_field(&val, 0, UNICAM_BL_MASK);
>>> +    set_field(&val, 0, UNICAM_BS_MASK);
>>> +    set_field(&val, 0xe, UNICAM_PP_MASK);
>>> +    set_field(&val, 8, UNICAM_NP_MASK);
>>> +    set_field(&val, 2, UNICAM_PT_MASK);
>>> +    set_field(&val, 1, UNICAM_PE);
>>> +    reg_write(dev, UNICAM_PRI, val);
>>> +
>>> +    reg_write_field(dev, UNICAM_ANA, 0, UNICAM_DDL);
>>> +
>>> +    /* Always start in trigger frame capture mode (UNICAM_FCM set) */
>>> +    val = UNICAM_FSIE | UNICAM_FEIE | UNICAM_FCM | UNICAM_IBOB;
>>> +    set_field(&val, line_int_freq, UNICAM_LCIE_MASK);
>>> +    reg_write(dev, UNICAM_ICTL, val);
>>> +    reg_write(dev, UNICAM_STA, UNICAM_STA_MASK_ALL);
>>> +    reg_write(dev, UNICAM_ISTA, UNICAM_ISTA_MASK_ALL);
>>> +
>>> +    /* tclk_term_en */
>>> +    reg_write_field(dev, UNICAM_CLT, 2, UNICAM_CLT1_MASK);
>>> +    /* tclk_settle */
>>> +    reg_write_field(dev, UNICAM_CLT, 6, UNICAM_CLT2_MASK);
>>> +    /* td_term_en */
>>> +    reg_write_field(dev, UNICAM_DLT, 2, UNICAM_DLT1_MASK);
>>> +    /* ths_settle */
>>> +    reg_write_field(dev, UNICAM_DLT, 6, UNICAM_DLT2_MASK);
>>> +    /* trx_enable */
>>> +    reg_write_field(dev, UNICAM_DLT, 0, UNICAM_DLT3_MASK);
>>> +
>>> +    reg_write_field(dev, UNICAM_CTRL, 0, UNICAM_SOE);
>>> +
>>> +    /* Packet compare setup - required to avoid missing frame ends */
>>> +    val = 0;
>>> +    set_field(&val, 1, UNICAM_PCE);
>>> +    set_field(&val, 1, UNICAM_GI);
>>> +    set_field(&val, 1, UNICAM_CPH);
>>> +    set_field(&val, 0, UNICAM_PCVC_MASK);
>>> +    set_field(&val, 1, UNICAM_PCDT_MASK);
>>> +    reg_write(dev, UNICAM_CMP0, val);
>>> +
>>> +    /* Enable clock lane and set up terminations */
>>> +    val = 0;
>>> +    if (dev->bus_type == V4L2_MBUS_CSI2_DPHY) {
>>> +        /* CSI2 */
>>> +        set_field(&val, 1, UNICAM_CLE);
>>> +        set_field(&val, 1, UNICAM_CLLPE);
>>> +        if (dev->bus_flags & V4L2_MBUS_CSI2_CONTINUOUS_CLOCK) {
>>> +            set_field(&val, 1, UNICAM_CLTRE);
>>> +            set_field(&val, 1, UNICAM_CLHSE);
>>> +        }
>>> +    } else {
>>> +        /* CCP2 */
>>> +        set_field(&val, 1, UNICAM_CLE);
>>> +        set_field(&val, 1, UNICAM_CLHSE);
>>> +        set_field(&val, 1, UNICAM_CLTRE);
>>> +    }
>>> +    reg_write(dev, UNICAM_CLK, val);
>>> +
>>> +    /*
>>> +     * Enable required data lanes with appropriate terminations.
>>> +     * The same value needs to be written to UNICAM_DATn registers for
>>> +     * the active lanes, and 0 for inactive ones.
>>> +     */
>>> +    val = 0;
>>> +    if (dev->bus_type == V4L2_MBUS_CSI2_DPHY) {
>>> +        /* CSI2 */
>>> +        set_field(&val, 1, UNICAM_DLE);
>>> +        set_field(&val, 1, UNICAM_DLLPE);
>>> +        if (dev->bus_flags & V4L2_MBUS_CSI2_CONTINUOUS_CLOCK) {
>>> +            set_field(&val, 1, UNICAM_DLTRE);
>>> +            set_field(&val, 1, UNICAM_DLHSE);
>>> +        }
>>> +    } else {
>>> +        /* CCP2 */
>>> +        set_field(&val, 1, UNICAM_DLE);
>>> +        set_field(&val, 1, UNICAM_DLHSE);
>>> +        set_field(&val, 1, UNICAM_DLTRE);
>>> +    }
>>> +    reg_write(dev, UNICAM_DAT0, val);
>>> +
>>> +    if (dev->active_data_lanes == 1)
>>> +        val = 0;
>>> +    reg_write(dev, UNICAM_DAT1, val);
>>> +
>>> +    if (dev->max_data_lanes > 2) {
>>> +        /*
>>> +         * Registers UNICAM_DAT2 and UNICAM_DAT3 only valid if the
>>> +         * instance supports more than 2 data lanes.
>>> +         */
>>> +        if (dev->active_data_lanes == 2)
>>> +            val = 0;
>>> +        reg_write(dev, UNICAM_DAT2, val);
>>> +
>>> +        if (dev->active_data_lanes == 3)
>>> +            val = 0;
>>> +        reg_write(dev, UNICAM_DAT3, val);
>>> +    }
>>> +
>>> +    reg_write(dev, UNICAM_IBLS,
>>> +          dev->node[IMAGE_NODE].v_fmt.fmt.pix.bytesperline);
>>> +    size = dev->node[IMAGE_NODE].v_fmt.fmt.pix.sizeimage;
>>> +    unicam_wr_dma_addr(dev, addr[IMAGE_NODE], size, IMAGE_NODE);
>>> +    unicam_set_packing_config(dev);
>>> +    unicam_cfg_image_id(dev);
>>> +
>>> +    val = reg_read(dev, UNICAM_MISC);
>>> +    set_field(&val, 1, UNICAM_FL0);
>>> +    set_field(&val, 1, UNICAM_FL1);
>>> +    reg_write(dev, UNICAM_MISC, val);
>>> +
>>> +    if (dev->node[METADATA_NODE].streaming && 
>>> dev->sensor_embedded_data) {
>>> +        dev_dbg(dev->v4l2_dev.dev, "enable metadata dma\n");
>>> +        size = dev->node[METADATA_NODE].v_fmt.fmt.meta.buffersize;
>>> +        unicam_enable_ed(dev);
>>> +        unicam_wr_dma_addr(dev, addr[METADATA_NODE], size, 
>>> METADATA_NODE);
>>> +    }
>>> +
>>> +    /* Enable peripheral */
>>> +    reg_write_field(dev, UNICAM_CTRL, 1, UNICAM_CPE);
>>> +
>>> +    /* Load image pointers */
>>> +    reg_write_field(dev, UNICAM_ICTL, 1, UNICAM_LIP_MASK);
>>> +
>>> +    /* Load embedded data buffer pointers if needed */
>>> +    if (dev->node[METADATA_NODE].streaming && 
>>> dev->sensor_embedded_data)
>>> +        reg_write_field(dev, UNICAM_DCS, 1, UNICAM_LDP);
>>> +
>>> +    /*
>>> +     * Enable trigger only for the first frame to
>>> +     * sync correctly to the FS from the source.
>>> +     */
>>> +    reg_write_field(dev, UNICAM_ICTL, 1, UNICAM_TFC);
>>> +}
>>> +
>>> +static void unicam_disable(struct unicam_device *dev)
>>> +{
>>> +    /* Analogue lane control disable */
>>> +    reg_write_field(dev, UNICAM_ANA, 1, UNICAM_DDL);
>>> +
>>> +    /* Stop the output engine */
>>> +    reg_write_field(dev, UNICAM_CTRL, 1, UNICAM_SOE);
>>> +
>>> +    /* Disable the data lanes. */
>>> +    reg_write(dev, UNICAM_DAT0, 0);
>>> +    reg_write(dev, UNICAM_DAT1, 0);
>>> +
>>> +    if (dev->max_data_lanes > 2) {
>>> +        reg_write(dev, UNICAM_DAT2, 0);
>>> +        reg_write(dev, UNICAM_DAT3, 0);
>>> +    }
>>> +
>>> +    /* Peripheral reset */
>>> +    reg_write_field(dev, UNICAM_CTRL, 1, UNICAM_CPR);
>>> +    usleep_range(50, 100);
>>> +    reg_write_field(dev, UNICAM_CTRL, 0, UNICAM_CPR);
>>> +
>>> +    /* Disable peripheral */
>>> +    reg_write_field(dev, UNICAM_CTRL, 0, UNICAM_CPE);
>>> +
>>> +    /* Clear ED setup */
>>> +    reg_write(dev, UNICAM_DCS, 0);
>>> +
>>> +    /* Disable all lane clocks */
>>> +    clk_write(dev, 0);
>>> +}
>>> +
>>> +static void unicam_return_buffers(struct unicam_node *node,
>>> +                  enum vb2_buffer_state state)
>>> +{
>>> +    struct unicam_buffer *buf, *tmp;
>>> +    unsigned long flags;
>>> +
>>> +    spin_lock_irqsave(&node->dma_queue_lock, flags);
>>
>> Same here, spin_lock_irq() would do. But isn't this function meant to be
>> called with the hardware stopped ? Shouldn't it then be safe to drop the
>> spinlock completely ?
>>
>>> +    list_for_each_entry_safe(buf, tmp, &node->dma_queue, list) {
>>> +        list_del(&buf->list);
>>> +        vb2_buffer_done(&buf->vb.vb2_buf, state);
>>> +    }
>>> +
>>> +    if (node->cur_frm)
>>> +        vb2_buffer_done(&node->cur_frm->vb.vb2_buf,
>>> +                state);
>>> +    if (node->next_frm && node->cur_frm != node->next_frm)
>>> +        vb2_buffer_done(&node->next_frm->vb.vb2_buf,
>>> +                state);
>>> +
>>> +    node->cur_frm = NULL;
>>> +    node->next_frm = NULL;
>>> +    spin_unlock_irqrestore(&node->dma_queue_lock, flags);
>>> +}
>>> +
>>> +static int unicam_video_check_format(struct unicam_node *node)
>>> +{
>>> +    const struct v4l2_mbus_framefmt *format;
>>> +    struct media_pad *remote_pad;
>>> +    struct v4l2_subdev_state *state;
>>> +    int ret = 0;
>>> +
>>> +    remote_pad = media_entity_remote_pad(&node->pad);
>>> +    if (!remote_pad)
>>> +        return -ENODEV;
>>
>> This could be done in unicam_async_complete() and cached in the
>> unicam_device structure, the same way we cache the sensor subdev
>> pointer.
>>
>>> +
>>> +    state = v4l2_subdev_lock_active_state(node->dev->sensor);
>>
>> That's not correct, the video node is connected to the subdev, not to
>> the sensor.
>>
>>> +
>>> +    format = v4l2_state_get_stream_format(state,
>>> +                          remote_pad->index, 0);
>>
>> This holds on a single line.
>>
>>> +    if (!format) {
>>> +        ret = -EINVAL;
>>> +        goto out;
>>> +    }
>>> +
>>> +    if (node->fmt->code != format->code ||
>>> +        node->v_fmt.fmt.pix.height != format->height ||
>>> +        node->v_fmt.fmt.pix.width != format->width ||
>>> +        node->v_fmt.fmt.pix.field != format->field) {
>>
>> A debug message that prints the configuration on both sides could be
>> useful for debugging here.
>>
>>> +        ret = -EPIPE;
>>> +        goto out;
>>> +    }
>>> +
>>> +out:
>>> +    v4l2_subdev_unlock_state(state);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int unicam_start_streaming(struct vb2_queue *vq, unsigned int 
>>> count)
>>> +{
>>> +    struct unicam_node *node = vb2_get_drv_priv(vq);
>>> +    struct unicam_device *dev = node->dev;
>>> +    dma_addr_t buffer_addr[MAX_NODES] = { 0 };
>>> +    struct unicam_buffer *buf;
>>> +    unsigned long flags;
>>> +    unsigned int i;
>>> +    int ret;
>>> +
>>> +    node->streaming = true;
>>
>> Locking seems to be completely missing from the driver. I'll comment
>> more on that on v3, it will be easier after all the other cleanups.
>>
>>> +
>>> +    dev->sequence = 0;
>>> +    ret = unicam_runtime_get(dev);
>>> +    if (ret < 0) {
>>> +        dev_dbg(dev->v4l2_dev.dev, "unicam_runtime_get failed\n");
>>
>> This is really not supposed to happen, dev_err() is appropriate.
>>
>>> +        goto err_streaming;
>>> +    }
>>> +
>>> +    ret = media_pipeline_start(node->video_dev.entity.pads, 
>>> &node->pipe);
>>> +    if (ret < 0) {
>>> +        dev_err(dev->v4l2_dev.dev, "Failed to start media pipeline: 
>>> %d\n", ret);
>>
>> dev_dbg() here too.
>>
>>> +        goto err_pm_put;
>>> +    }
>>> +
>>> +    dev->active_data_lanes = dev->max_data_lanes;
>>
>> You need to get the number of data lanes from the sensor subdev and
>> validate it to ensure it doesn't exceed the maximum.
>>
>>> +
>>> +    unicam_video_check_format(node);
>>
>> Where's the return value check ?
>>
>>> +
>>> +    dev_dbg(dev->v4l2_dev.dev, "Running with %u data lanes\n",
>>> +        dev->active_data_lanes);
>>> +
>>> +    ret = clk_set_min_rate(dev->vpu_clock, MIN_VPU_CLOCK_RATE);
>>> +    if (ret) {
>>> +        dev_err(dev->v4l2_dev.dev, "failed to set up VPU clock\n");
>>> +        goto error_pipeline;
>>> +    }
>>> +
>>> +    ret = clk_prepare_enable(dev->vpu_clock);
>>> +    if (ret) {
>>> +        dev_err(dev->v4l2_dev.dev, "Failed to enable VPU clock: 
>>> %d\n", ret);
>>> +        goto error_pipeline;
>>> +    }
>>> +
>>> +    ret = clk_set_rate(dev->clock, 100 * 1000 * 1000);
>>> +    if (ret) {
>>> +        dev_err(dev->v4l2_dev.dev, "failed to set up CSI clock\n");
>>> +        goto err_vpu_clock;
>>> +    }
>>> +
>>> +    ret = clk_prepare_enable(dev->clock);
>>> +    if (ret) {
>>> +        dev_err(dev->v4l2_dev.dev, "Failed to enable CSI clock: 
>>> %d\n", ret);
>>> +        goto err_vpu_clock;
>>> +    }
>>> +
>>> +    dev_dbg(dev->v4l2_dev.dev, "node %s\n", node->video_dev.name);
>>> +
>>> +    spin_lock_irqsave(&node->dma_queue_lock, flags);
>>> +    buf = list_first_entry(&node->dma_queue,
>>> +                   struct unicam_buffer, list);
>>
>> This holds on a single line.
>>
>>> +    node->cur_frm = buf;
>>> +    node->next_frm = buf;
>>> +    list_del(&buf->list);
>>> +    spin_unlock_irqrestore(&node->dma_queue_lock, flags);
>>> +
>>> +    buffer_addr[i] =
>>> +        vb2_dma_contig_plane_dma_addr(&buf->vb.vb2_buf, 0);
>>
>> This too.
>>
>>> +
>>> +    unicam_start_rx(dev, buffer_addr);
>>> +
>>> +    ret = v4l2_subdev_call(&dev->sd, video, s_stream, 1);
>>> +    if (ret < 0) {
>>> +        dev_err(dev->v4l2_dev.dev, "stream on failed in subdev\n");
>>> +        goto err_disable_unicam;
>>> +    }
>>
>> Do I understand correctly that all this will run twice, once when
>> VIDIOC_STREAMON is called on the image node, and once when it's called
>> on the metadata node ? That doesn't seem right. The stop_streaming
>> function is affected similarly.
>>
>>> +
>>> +    dev->clocks_enabled = true;
>>> +    return 0;
>>> +
>>> +err_disable_unicam:
>>> +    unicam_disable(dev);
>>> +    clk_disable_unprepare(dev->clock);
>>> +err_vpu_clock:
>>> +    if (clk_set_min_rate(dev->vpu_clock, 0))
>>> +        dev_err(dev->v4l2_dev.dev, "failed to reset the VPU clock\n");
>>> +    clk_disable_unprepare(dev->vpu_clock);
>>> +error_pipeline:
>>> +    media_pipeline_stop(node->video_dev.entity.pads);
>>> +err_pm_put:
>>> +    unicam_runtime_put(dev);
>>> +err_streaming:
>>> +    unicam_return_buffers(node, VB2_BUF_STATE_QUEUED);
>>> +    node->streaming = false;
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static void unicam_stop_streaming(struct vb2_queue *vq)
>>> +{
>>> +    struct unicam_node *node = vb2_get_drv_priv(vq);
>>> +    struct unicam_device *dev = node->dev;
>>> +
>>> +    node->streaming = false;
>>> +    /*
>>> +     * Stop streaming the sensor and disable the peripheral.
>>> +     * We cannot continue streaming embedded data with the
>>> +     * image pad disabled.
>>> +     */
>>> +    if (v4l2_subdev_call(&dev->sd, video, s_stream, 0) < 0)
>>> +        dev_err(dev->v4l2_dev.dev, "stream off failed in subdev\n");
>>> +
>>> +    unicam_disable(dev);
>>> +
>>> +    media_pipeline_stop(node->video_dev.entity.pads);
>>> +
>>> +    if (dev->clocks_enabled) {
>>
>> .stop_streaming() can only be called after a successful
>> .start_streaming() call, so you can drop the clocks_enabled field.
>>
>>> +        if (clk_set_min_rate(dev->vpu_clock, 0))
>>> +            dev_err(dev->v4l2_dev.dev, "failed to reset the VPU 
>>> clock\n");
>>> +        clk_disable_unprepare(dev->vpu_clock);
>>> +        clk_disable_unprepare(dev->clock);
>>> +        dev->clocks_enabled = false;
>>> +    }
>>> +    unicam_runtime_put(dev);
>>> +
>>> +    if (is_metadata_node(node)) {
>>> +        /*
>>> +         * Allow the hardware to spin in the dummy buffer.
>>> +         * This is only really needed if the embedded data pad is
>>> +         * disabled before the image pad.
>>> +         */
>>> +        unicam_wr_dma_addr(dev, node->dummy_buf_dma_addr,
>>> +                   DUMMY_BUF_SIZE, METADATA_NODE);
>>> +    }
>>> +
>>> +    /* Clear all queued buffers for the node */
>>> +    unicam_return_buffers(node, VB2_BUF_STATE_ERROR);
>>> +}
>>> +
>>> +static const struct vb2_ops unicam_video_qops = {
>>> +    .wait_prepare        = vb2_ops_wait_prepare,
>>> +    .wait_finish        = vb2_ops_wait_finish,
>>> +    .queue_setup        = unicam_queue_setup,
>>> +    .buf_prepare        = unicam_buffer_prepare,
>>> +    .buf_queue        = unicam_buffer_queue,
>>> +    .start_streaming    = unicam_start_streaming,
>>> +    .stop_streaming        = unicam_stop_streaming,
>>> +};
>>> +
>>> +/* unicam capture driver file operations */
>>> +static const struct v4l2_file_operations unicam_fops = {
>>> +    .owner        = THIS_MODULE,
>>> +    .open           = v4l2_fh_open,
>>> +    .release        = vb2_fop_release,
>>> +    .poll        = vb2_fop_poll,
>>> +    .unlocked_ioctl = video_ioctl2, /* V4L2 ioctl handler */
>>
>> Drop the comment.
>>
>>> +    .mmap           = vb2_fop_mmap,
>>> +};
>>> +
>>> +static int
>>> +unicam_async_bound(struct v4l2_async_notifier *notifier,
>>> +           struct v4l2_subdev *subdev,
>>> +           struct v4l2_async_subdev *asd)
>>> +{
>>> +    struct unicam_device *unicam = 
>>> to_unicam_device(notifier->v4l2_dev);
>>> +
>>> +    if (unicam->sensor) {
>>> +        dev_info(unicam->v4l2_dev.dev, "subdev %s (Already set!!)",
>>> +             subdev->name);
>>> +        return 0;
>>> +    }
>>
>> This can't happen as we register a single asd only.
>>
>>> +
>>> +    unicam->sensor = subdev;
>>> +    dev_dbg(unicam->v4l2_dev.dev, "Using sensor %s for capture\n", 
>>> subdev->name);
>>
>> Line wrap.
>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void unicam_release(struct kref *kref)
>>> +{
>>> +    struct unicam_device *unicam =
>>> +        container_of(kref, struct unicam_device, kref);
>>> +
>>> +    v4l2_ctrl_handler_free(&unicam->ctrl_handler);
>>> +    media_device_cleanup(&unicam->mdev);
>>> +
>>> +    if (unicam->sensor_state)
>>> +        __v4l2_subdev_state_free(unicam->sensor_state);
>>> +
>>> +    kfree(unicam);
>>> +}
>>> +
>>> +static void unicam_put(struct unicam_device *unicam)
>>> +{
>>> +    kref_put(&unicam->kref, unicam_release);
>>> +}
>>> +
>>> +static void unicam_get(struct unicam_device *unicam)
>>> +{
>>> +    kref_get(&unicam->kref);
>>> +}
>>> +
>>> +static void unicam_node_release(struct video_device *vdev)
>>> +{
>>> +    struct unicam_node *node = video_get_drvdata(vdev);
>>> +
>>> +    unicam_put(node->dev);
>>> +}
>>> +
>>> +static void unicam_mc_set_default_format(struct unicam_node *node)
>>> +{
>>> +    dev_dbg(node->dev->v4l2_dev.dev, "Set default format for %s 
>>> node\n",
>>
>> node->dev->dev should do, no need to go through v4l2_dev here. But I'd
>> drop the message, I don't think it brings much value.
>>
>>> +        node->node_id == IMAGE_NODE ? "image" : "metadata");
>>> +
>>> +    if (is_image_node(node)) {
>>> +        struct v4l2_pix_format *pix_fmt = &node->v_fmt.fmt.pix;
>>> +
>>> +        pix_fmt->width = 640;
>>> +        pix_fmt->height = 480;
>>> +        pix_fmt->field = V4L2_FIELD_NONE;
>>> +        pix_fmt->colorspace = V4L2_COLORSPACE_SRGB;
>>> +        pix_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>> +        pix_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
>>> +        pix_fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
>>> +        pix_fmt->pixelformat = formats[0].fourcc;
>>> +        unicam_calc_format_size_bpl(node->dev, &formats[0],
>>> +                        &node->v_fmt);
>>> +        node->v_fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +
>>> +        node->fmt = &formats[0];
>>> +    } else {
>>> +        const struct unicam_fmt *fmt;
>>> +
>>> +        /* Fix this node format as embedded data. */
>>> +        fmt = find_format_by_code(MEDIA_BUS_FMT_METADATA_8);
>>> +        node->v_fmt.fmt.meta.dataformat = fmt->fourcc;
>>> +        node->fmt = fmt;
>>
>> Move this to the end to group all v_fmt initialization together.
>>
>>> +        node->v_fmt.fmt.meta.buffersize = UNICAM_EMBEDDED_SIZE;
>>> +        node->embedded_lines = 1;
>>
>> This belongs to the called.
>>
>>> +        node->v_fmt.type = V4L2_BUF_TYPE_META_CAPTURE;
>>> +    }
>>> +}
>>> +
>>> +static int register_node(struct unicam_device *unicam, struct 
>>> unicam_node *node,
>>> +             enum v4l2_buf_type type)
>>
>> Replace the type parameter with a enum unicam_node_type, that will look
>> cleaner in
>>
>>> +{
>>> +    struct video_device *vdev;
>>> +    struct vb2_queue *q;
>>> +    int ret;
>>> +
>>> +    node->dev = unicam;
>>> +    node->node_id = type == V4L2_BUF_TYPE_VIDEO_CAPTURE ?
>>> +                IMAGE_NODE :
>>> +                METADATA_NODE;
>>> +
>>> +    spin_lock_init(&node->dma_queue_lock);
>>> +    mutex_init(&node->lock);
>>> +
>>> +    vdev = &node->video_dev;
>>> +    /*
>>> +     * If the sensor subdevice has any controls, associate the node
>>> +     *  with the ctrl handler to allow access from userland.
>>> +     */
>>> +    if (!list_empty(&unicam->ctrl_handler.ctrls))
>>> +        vdev->ctrl_handler = &unicam->ctrl_handler;
>>
>> Drop this, and move the vdev assignment below, just before it gets used.
>>
>>     /* Initialize the videobuf2 queue. */
>>> +
>>> +    q = &node->buffer_queue;
>>> +    q->type = type;
>>> +    q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_READ;
>>
>> Drop VB2_READ, that shouldn't be used.
>>
>>> +    q->drv_priv = node;
>>> +    q->ops = &unicam_video_qops;
>>> +    q->mem_ops = &vb2_dma_contig_memops;
>>> +    q->buf_struct_size = sizeof(struct unicam_buffer);
>>> +    q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>>> +    q->lock = &node->lock;
>>> +    q->min_buffers_needed = 1;
>>> +    q->dev = &unicam->pdev->dev;
>>> +
>>> +    ret = vb2_queue_init(q);
>>> +    if (ret) {
>>> +        dev_err(unicam->v4l2_dev.dev, "vb2_queue_init() failed\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    INIT_LIST_HEAD(&node->dma_queue);
>>
>> Move this to the beginning of the function with the rest of the node
>> initialization.
>>
>>     /* Initialize the video device. */
>>
>>> +
>>> +    vdev->release = unicam_node_release;
>>> +    vdev->fops = &unicam_fops;
>>> +    vdev->ioctl_ops = &unicam_mc_ioctl_ops;
>>> +    vdev->v4l2_dev = &unicam->v4l2_dev;
>>> +    vdev->vfl_dir = VFL_DIR_RX;
>>> +    vdev->queue = q;
>>> +    vdev->lock = &node->lock;
>>> +    vdev->device_caps = (type == V4L2_BUF_TYPE_VIDEO_CAPTURE) ?
>>
>> No need for parentheses.
>>
>>> +                V4L2_CAP_VIDEO_CAPTURE : V4L2_CAP_META_CAPTURE;
>>> +    vdev->device_caps |= V4L2_CAP_READWRITE | V4L2_CAP_STREAMING | 
>>> V4L2_CAP_IO_MC;
>>
>> Drop V4L2_CAP_READWRITE.
>>
>>> +    vdev->entity.ops = &unicam_mc_entity_ops;
>>> +
>>> +    /* Define the device names */
>>> +    snprintf(vdev->name, sizeof(vdev->name), "%s-%s", 
>>> UNICAM_MODULE_NAME,
>>> +         type == V4L2_BUF_TYPE_VIDEO_CAPTURE ? "image" : "embedded");
>>> +
>>> +    video_set_drvdata(vdev, node);
>>> +    if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>> +        vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
>>> +    node->pad.flags = MEDIA_PAD_FL_SINK;
>>> +    media_entity_pads_init(&vdev->entity, 1, &node->pad);
>>
>> Error check.
>>
>>> +
>>> +    node->dummy_buf_cpu_addr = dma_alloc_coherent(&unicam->pdev->dev,
>>> +                              DUMMY_BUF_SIZE,
>>> +                              &node->dummy_buf_dma_addr,
>>> +                              GFP_KERNEL);
>>> +    if (!node->dummy_buf_cpu_addr) {
>>> +        dev_err(unicam->v4l2_dev.dev, "Unable to allocate dummy 
>>> buffer.\n");
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>>> +    if (ret) {
>>> +        dev_err(unicam->v4l2_dev.dev, "Unable to register video 
>>> device %s\n",
>>> +            vdev->name);
>>> +        return ret;
>>> +    }
>>> +
>>> +    /*
>>> +     * Acquire a reference to unicam, which will be released when 
>>> the video
>>> +     * device will be unregistered and userspace will have closed 
>>> all open
>>> +     * file handles.
>>> +     */
>>> +    unicam_get(unicam);
>>
>> This is error-prone. References should be acquired when stored, so this
>> should be moved to the beginning of the function, and written as
>>
>>     node->dev = unicam_get(unicam);
>>
>> (you'll need to modify the function prototype to return the pointer).
>> You can then drop the comment. You will also to add error handling to
>> this function, as the reference will be released by
>> unicam_node_release(), which is called only after successful
>> registration of the video device. All error paths before registration
>> will thus need to release the reference (possibly with a goto
>> err_unicam_put)..
>>
>>> +    node->registered = true;
>>> +
>>> +    ret = media_create_pad_link(&unicam->sd.entity,
>>> +                    node->src_pad_id,
>>
>> unicam->sd is the unicam sd, node->src_pad_id is documented as being the
>> source pad of the sensor. This is wrong.
>>
>>> +                    &node->video_dev.entity,
>>> +                    0,
>>> +                    MEDIA_LNK_FL_ENABLED |
>>> +                    MEDIA_LNK_FL_IMMUTABLE);
>>> +    if (ret)
>>> +        dev_err(unicam->v4l2_dev.dev, "Unable to create pad link for 
>>> %s",
>>> +            unicam->sensor->name);
>>
>>         return ret;
>>     }
>>
>>> +
>>> +    unicam_mc_set_default_format(node);
>>
>> Call this before registering the video device, otherwise it can race
>> with userspace.
>>
>>> +
>>> +    return ret;
>>
>>     return 0;
>>
>>> +}
>>> +
>>> +static void unregister_nodes(struct unicam_device *unicam)
>>> +{
>>> +    unsigned int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
>>> +        struct unicam_node *node = &unicam->node[i];
>>> +
>>> +        if (node->dummy_buf_cpu_addr) {
>>> +            dma_free_coherent(&unicam->pdev->dev, DUMMY_BUF_SIZE,
>>> +                      node->dummy_buf_cpu_addr,
>>> +                      node->dummy_buf_dma_addr);
>>> +        }
>>
>> No need for curly braces.
>>
>>> +
>>> +        if (node->registered) {
>>> +            node->registered = false;
>>> +            video_unregister_device(&node->video_dev);
>>
>> Swap the two lines.
>>
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +static int unicam_async_complete(struct v4l2_async_notifier *notifier)
>>> +{
>>> +    struct unicam_device *unicam = 
>>> to_unicam_device(notifier->v4l2_dev);
>>> +    unsigned int i, source_pads = 0;
>>> +    int ret;
>>> +    static struct lock_class_key key;
>>> +
>>> +    unicam->v4l2_dev.notify = unicam_notify;
>>> +
>>> +    unicam->sensor_state = __v4l2_subdev_state_alloc(unicam->sensor,
>>> +                             "unicam:state->lock",
>>> +                             &key);
>>
>> You never use sensor_state.
>>
>>> +    if (!unicam->sensor_state)
>>> +        return -ENOMEM;
>>> +
>>> +    for (i = 0; i < unicam->sd.entity.num_pads; i++) {
>>> +        if (unicam->sd.entity.pads[i].flags & MEDIA_PAD_FL_SOURCE) {
>>> +            if (source_pads < MAX_NODES) {
>>> +                unicam->node[source_pads].src_pad_id = i;
>>> +                dev_dbg(unicam->v4l2_dev.dev, "source pad %u is 
>>> index %u\n",
>>> +                    source_pads, i);
>>> +            }
>>> +            source_pads++;
>>> +        }
>>> +    }
>>> +    if (!source_pads) {
>>> +        dev_err(unicam->v4l2_dev.dev, "No source pads on sensor.\n");
>>> +        goto unregister;
>>> +    }
>>
>> You're not looking at the sensor here, unicam->sd is the unicam subdev.
>> Furthermore, you're hardcoding the source pad of the sensor to 0 below
>> when creating the link. And in any case, that's not how it's supposed to
>> be done, a source subdev could have multiple source pads, you need to
>> identify the correct one from the device tree.
>>
>> The right way to handle this is to replace the call to
>> media_create_pad_link() with v4l2_create_fwnode_links_to_pad(). This can
>> be done in the .bound() handler.
>>
>>> +
>>> +    ret = register_node(unicam, &unicam->node[IMAGE_NODE],
>>> +                V4L2_BUF_TYPE_VIDEO_CAPTURE);
>>
>> Replace the type parameter with a enum unicam_node_type, so this will
>> become
>>
>>     ret = register_node(unicam, &unicam->node[IMAGE_NODE], IMAGE_NODE);
>>
>> which is less error-prone. You could even drop the second parameters:
>>
>>     ret = register_node(unicam, IMAGE_NODE);
>>
>>> +    if (ret) {
>>> +        dev_err(unicam->v4l2_dev.dev, "Unable to register image 
>>> video device.\n");
>>> +        goto unregister;
>>> +    }
>>> +
>>> +    /* \todo: check before :-) */
>>> +    unicam->sensor_embedded_data = true;
>>
>> Check what ? In any case I don't think this belongs here.
>>
>>> +
>>> +    ret = register_node(unicam, &unicam->node[METADATA_NODE],
>>> +                V4L2_BUF_TYPE_META_CAPTURE);
>>> +    if (ret) {
>>> +        dev_err(unicam->v4l2_dev.dev, "Unable to register metadata 
>>> video device.\n");
>>> +        goto unregister;
>>> +    }
>>> +
>>> +    ret = media_create_pad_link(&unicam->sensor->entity,
>>> +                    0,
>>> +                    &unicam->sd.entity,
>>> +                    UNICAM_SD_PAD_SINK,
>>> +                    MEDIA_LNK_FL_ENABLED |
>>> +                    MEDIA_LNK_FL_IMMUTABLE);
>>> +    if (ret)
>>> +        dev_err(unicam->v4l2_dev.dev, "Unable to create pad link for 
>>> %s",
>>> +            unicam->sensor->name);
>>> +
>>> +    ret = v4l2_device_register_subdev_nodes(&unicam->v4l2_dev);
>>> +    if (ret) {
>>> +        dev_err(unicam->v4l2_dev.dev, "Unable to register subdev 
>>> nodes.\n");
>>> +        goto unregister;
>>> +    }
>>> +
>>> +    /*
>>> +     * Release the initial reference, all references are now owned 
>>> by the
>>> +     * video devices.
>>> +     */
>>> +    unicam_put(unicam);
>>> +    return 0;
>>> +
>>> +unregister:
>>> +    unregister_nodes(unicam);
>>> +    unicam_put(unicam);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static const struct v4l2_async_notifier_operations unicam_async_ops = {
>>> +    .bound = unicam_async_bound,
>>> +    .complete = unicam_async_complete,
>>> +};
>>> +
>>> +static int of_unicam_connect_subdevs(struct unicam_device *dev)
>>> +{
>>> +    struct platform_device *pdev = dev->pdev;
>>> +    struct v4l2_fwnode_endpoint ep = { };
>>> +    struct device_node *ep_node;
>>> +    struct device_node *sensor_node;
>>> +    unsigned int lane;
>>> +    int ret = -EINVAL;
>>> +
>>> +    if (of_property_read_u32(pdev->dev.of_node, "num-data-lanes",
>>> +                 &dev->max_data_lanes) < 0) {
>>> +        dev_err(dev->v4l2_dev.dev, "number of data lanes not set\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /* Get the local endpoint and remote device. */
>>> +    ep_node = of_graph_get_next_endpoint(pdev->dev.of_node, NULL);
>>
>> Use of_graph_get_endpoint_by_regs(node, 0, -1).
>>
>>> +    if (!ep_node) {
>>> +        dev_dbg(dev->v4l2_dev.dev, "can't get next endpoint\n");
>>
>>         dev_dbg(dev->v4l2_dev.dev, "No endpoint\n");
>>
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    dev_dbg(dev->v4l2_dev.dev, "ep_node is %pOF\n", ep_node);
>>
>> You can drop this.
>>
>>> +
>>> +    sensor_node = of_graph_get_remote_port_parent(ep_node);
>>> +    if (!sensor_node) {
>>> +        dev_dbg(dev->v4l2_dev.dev, "can't get remote parent\n");
>>> +        goto cleanup_exit;
>>> +    }
>>> +
>>> +    dev_dbg(dev->v4l2_dev.dev, "found subdevice %pOF\n", sensor_node);
>>
>> There's no subdev yet, we're dealing with DT here.
>>
>>     dev_dbg(dev->v4l2_dev.dev, "found source device %pOF\n", 
>> sensor_node);
>>
>> But I would actually drop all this. The sensor_node is only used in
>> debug or error messages, so you could remove it and use ep_node in those
>> messages instead. Or even better, drop the "subdevice %pOF: " prefix in
>> the messages, that will make the code simpler.
>>
>>> +
>>> +    /* Parse the local endpoint and validate its configuration. */
>>> +    v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_node), &ep);
>>
>> Use v4l2_fwnode_endpoint_alloc_parse() and check for errors (and update
>> the error cleanup path accordingly).
>>
>>> +
>>> +    dev_dbg(dev->v4l2_dev.dev, "parsed local endpoint, bus_type %u\n",
>>> +        ep.bus_type);
>>> +
>>> +    dev->bus_type = ep.bus_type;
>>> +
>>> +    switch (ep.bus_type) {
>>> +    case V4L2_MBUS_CSI2_DPHY:
>>> +        switch (ep.bus.mipi_csi2.num_data_lanes) {
>>> +        case 1:
>>> +        case 2:
>>> +        case 4:
>>> +            break;
>>> +
>>> +        default:
>>> +            dev_err(dev->v4l2_dev.dev, "subdevice %pOF: %u data 
>>> lanes not supported\n",
>>> +                sensor_node,
>>> +                ep.bus.mipi_csi2.num_data_lanes);
>>> +            goto cleanup_exit;
>>> +        }
>>> +
>>> +        for (lane = 0; lane < ep.bus.mipi_csi2.num_data_lanes; 
>>> lane++) {
>>> +            if (ep.bus.mipi_csi2.data_lanes[lane] != lane + 1) {
>>> +                dev_err(dev->v4l2_dev.dev, "subdevice %pOF: data 
>>> lanes reordering not supported\n",
>>> +                    sensor_node);
>>> +                goto cleanup_exit;
>>> +            }
>>> +        }
>>> +
>>> +        if (ep.bus.mipi_csi2.num_data_lanes > dev->max_data_lanes) {
>>> +            dev_err(dev->v4l2_dev.dev, "subdevice requires %u data 
>>> lanes when %u are supported\n",
>>
>> s/subdevice/endpoint/
>>
>>> +                ep.bus.mipi_csi2.num_data_lanes,
>>> +                dev->max_data_lanes);
>>> +        }
>>> +
>>> +        dev->max_data_lanes = ep.bus.mipi_csi2.num_data_lanes;
>>> +        dev->bus_flags = ep.bus.mipi_csi2.flags;
>>> +
>>> +        break;
>>> +
>>> +    case V4L2_MBUS_CCP2:
>>> +        if (ep.bus.mipi_csi1.clock_lane != 0 ||
>>> +            ep.bus.mipi_csi1.data_lane != 1) {
>>> +            dev_err(dev->v4l2_dev.dev, "subdevice %pOF: unsupported 
>>> lanes configuration\n",
>>> +                sensor_node);
>>> +            goto cleanup_exit;
>>> +        }
>>> +
>>> +        dev->max_data_lanes = 1;
>>> +        dev->bus_flags = ep.bus.mipi_csi1.strobe;
>>> +        break;
>>
>> I wonder if this driver will ever be used with a CCP2 sensor.
>>
>>> +
>>> +    default:
>>> +        /* Unsupported bus type */
>>> +        dev_err(dev->v4l2_dev.dev, "subdevice %pOF: unsupported bus 
>>> type %u\n",
>>> +            sensor_node, ep.bus_type);
>>> +        goto cleanup_exit;
>>> +    }
>>> +
>>> +    dev_dbg(dev->v4l2_dev.dev, "subdevice %pOF: %s bus, %u data 
>>> lanes, flags=0x%08x\n",
>>> +        sensor_node,
>>> +        dev->bus_type == V4L2_MBUS_CSI2_DPHY ? "CSI-2" : "CCP2",
>>> +        dev->max_data_lanes, dev->bus_flags);
>>> +
>>> +    /* Initialize and register the async notifier. */
>>> +    v4l2_async_nf_init(&dev->notifier);
>>> +    dev->notifier.ops = &unicam_async_ops;
>>> +
>>> +    dev->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>>> +    dev->asd.match.fwnode = 
>>> fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep_node));
>>> +    ret = v4l2_async_nf_add_subdev(&dev->notifier, &dev->asd);
>>
>> The asd must be dynamically allocated as stated by the documentation of
>> __v4l2_async_nf_add_subdev(). You can use
>> v4l2_async_nf_add_fwnode_remote() to simplify this.
>>
>>> +    if (ret) {
>>> +        dev_err(dev->v4l2_dev.dev, "Error adding subdevice: %d\n", 
>>> ret);
>>> +        goto cleanup_exit;
>>> +    }
>>> +
>>> +    ret = v4l2_async_nf_register(&dev->v4l2_dev, &dev->notifier);
>>> +    if (ret) {
>>> +        dev_err(dev->v4l2_dev.dev, "Error registering async 
>>> notifier: %d\n", ret);
>>> +        ret = -EINVAL;
>>> +    }
>>> +
>>> +cleanup_exit:
>>> +    of_node_put(sensor_node);
>>> +    of_node_put(ep_node);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int bcm2835_media_dev_init(struct unicam_device *unicam,
>>> +                  struct platform_device *pdev)
>>> +{
>>> +    int ret = 0;
>>> +
>>> +    unicam->mdev.dev = &pdev->dev;
>>> +    strscpy(unicam->mdev.model, UNICAM_MODULE_NAME,
>>> +        sizeof(unicam->mdev.model));
>>> +    strscpy(unicam->mdev.serial, "", sizeof(unicam->mdev.serial));
>>> +    snprintf(unicam->mdev.bus_info, sizeof(unicam->mdev.bus_info),
>>> +         "platform:%s", dev_name(&pdev->dev));
>>> +    unicam->mdev.hw_revision = 0;
>>> +
>>> +    media_device_init(&unicam->mdev);
>>> +
>>> +    unicam->v4l2_dev.mdev = &unicam->mdev;
>>> +
>>> +    ret = v4l2_device_register(&pdev->dev, &unicam->v4l2_dev);
>>
>>     return v4l2_device_register(&pdev->dev, &unicam->v4l2_dev);
>>
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +/* Internal subdev */
>>> +
>>> +static int _unicam_subdev_set_routing(struct v4l2_subdev *sd,
>>
>> __ is more conventional.
>>
>>> +                      struct v4l2_subdev_state *state,
>>> +                      struct v4l2_subdev_krouting *routing)
>>> +{
>>> +    static const struct v4l2_mbus_framefmt format = {
>>> +        .width = 640,
>>> +        .height = 480,
>>> +        .code = MEDIA_BUS_FMT_UYVY8_2X8,
>>> +        .field = V4L2_FIELD_NONE,
>>> +        .colorspace = V4L2_COLORSPACE_SRGB,
>>> +        .ycbcr_enc = V4L2_YCBCR_ENC_601,
>>> +        .quantization = V4L2_QUANTIZATION_LIM_RANGE,
>>> +        .xfer_func = V4L2_XFER_FUNC_SRGB,
>>> +        .flags = 0,
>>> +    };
>>
>> As this should match the values in unicam_mc_set_default_format(), can
>> you move this structure out of the function, and use it to initialize
>> the pix_fmt fields in unicam_mc_set_default_format() ?
>>
>>> +    int ret;
>>> +
>>> +    ret = v4l2_subdev_routing_validate_1_to_1(routing);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    v4l2_subdev_lock_state(state);
>>> +
>>> +    ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, 
>>> &format);
>>> +
>>> +    v4l2_subdev_unlock_state(state);
>>> +
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int unicam_subdev_set_routing(struct v4l2_subdev *sd,
>>> +                     struct v4l2_subdev_state *state,
>>> +                     enum v4l2_subdev_format_whence which,
>>> +                     struct v4l2_subdev_krouting *routing)
>>> +{
>>> +    struct unicam_device *unicam = sd_to_unicam_device(sd);
>>> +
>>> +    if (which == V4L2_SUBDEV_FORMAT_ACTIVE && unicam->streaming)
>>> +        return -EBUSY;
>>> +
>>> +    return _unicam_subdev_set_routing(sd, state, routing);
>>> +}
>>> +
>>> +static int unicam_subdev_init_cfg(struct v4l2_subdev *sd,
>>> +                  struct v4l2_subdev_state *state)
>>> +{
>>> +    struct v4l2_subdev_route routes[] = {
>>> +        {
>>> +            .sink_pad = 0,
>>> +            .sink_stream = 0,
>>> +            .source_pad = 1,
>>
>> You have macros you can use for the pad numbers.
>>
>>> +            .source_stream = 0,
>>> +            .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
>>> +        },
>>> +    };
>>> +
>>> +    struct v4l2_subdev_krouting routing = {
>>> +        .num_routes = ARRAY_SIZE(routes),
>>> +        .routes = routes,
>>> +    };
>>> +
>>> +    /* Initialize routing to single route to the fist source pad */
>>> +    return _unicam_subdev_set_routing(sd, state, &routing);
>>> +}
>>> +
>>> +static int unicam_subdev_enum_mbus_code(struct v4l2_subdev *sd,
>>> +                    struct v4l2_subdev_state *state,
>>> +                    struct v4l2_subdev_mbus_code_enum *code)
>>> +{
>>> +    int ret = 0;
>>> +
>>> +    v4l2_subdev_lock_state(state);
>>> +
>>> +    /* No transcoding, source and sink codes must match. */
>>> +    if (unicam_sd_pad_is_source(code->pad)) {
>>> +        struct v4l2_mbus_framefmt *fmt;
>>> +
>>> +        if (code->index > 0) {
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +
>>> +        fmt = v4l2_subdev_state_get_opposite_stream_format(state,
>>> +                                   code->pad,
>>> +                                   code->stream);
>>> +        if (!fmt) {
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +
>>> +        code->code = fmt->code;
>>> +    } else {
>>> +        if (code->index >= ARRAY_SIZE(formats)) {
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +
>>> +        code->code = formats[code->index].code;
>>> +    }
>>> +
>>> +out:
>>> +    v4l2_subdev_unlock_state(state);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int unicam_subdev_start_streaming(struct unicam_device *unicam)
>>> +{
>>> +    const struct v4l2_subdev_krouting *routing;
>>> +    struct v4l2_subdev_state *state;
>>> +    int ret = 0;
>>
>> No need to initialize ret to 0.
>>
>>> +
>>> +    state = v4l2_subdev_lock_active_state(&unicam->sd);
>>> +
>>> +    routing = &state->routing;
>>> +
>>> +    v4l2_subdev_unlock_state(state);
>>
>> As a piece of conceptual art, maybe, but as useful code, I have some
>> doubts.
>>
>>> +
>>> +    unicam->streaming = true;
>>> +
>>> +    ret = v4l2_subdev_call(unicam->sensor, video, s_stream, 1);
>>> +    if (ret) {
>>> +        v4l2_subdev_call(unicam->sensor, video, s_stream, 0);
>>
>> If .s_stream(1) fails, why do you need to call .s_stream(0) ?
>>
>>> +        unicam->streaming = false;
>>
>> Drop this, and move the unicam->streaming = true line below.
>>
>>> +        return ret;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int unicam_subdev_stop_streaming(struct unicam_device *unicam)
>>> +{
>>> +    v4l2_subdev_call(unicam->sensor, video, s_stream, 0);
>>> +
>>> +    unicam->streaming = false;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int unicam_subdev_s_stream(struct v4l2_subdev *sd, int enable)
>>> +{
>>> +    struct unicam_device *unicam = sd_to_unicam_device(sd);
>>> +
>>> +    if (enable)
>>> +        return unicam_subdev_start_streaming(unicam);
>>> +    else
>>> +        return unicam_subdev_stop_streaming(unicam);
>>> +    return 0;
>>
>> Drop the last line.
>>
>>> +}
>>> +
>>> +static int unicam_subdev_set_pad_format(struct v4l2_subdev *sd,
>>> +                    struct v4l2_subdev_state *state,
>>> +                    struct v4l2_subdev_format *format)
>>> +{
>>> +    struct unicam_device *unicam = sd_to_unicam_device(sd);
>>> +    struct v4l2_mbus_framefmt *fmt;
>>> +    int ret = 0;
>>> +
>>> +    if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && 
>>> unicam->streaming)
>>> +        return -EBUSY;
>>> +
>>> +    /* No transcoding, source and sink formats must match. */
>>> +    if (unicam_sd_pad_is_source(format->pad))
>>> +        return v4l2_subdev_get_fmt(sd, state, format);
>>> +
>>> +    // TODO: implement fmt validation
>>
>> That's a candidate for v3.
>>
>>> +
>>> +    v4l2_subdev_lock_state(state);
>>> +
>>> +    fmt = v4l2_state_get_stream_format(state, format->pad, 
>>> format->stream);
>>> +    if (!fmt) {
>>> +        ret = -EINVAL;
>>> +        goto out;
>>> +    }
>>> +
>>> +    *fmt = format->format;
>>> +
>>> +    fmt = v4l2_subdev_state_get_opposite_stream_format(state, 
>>> format->pad,
>>> +                               format->stream);
>>> +    if (!fmt) {
>>> +        ret = -EINVAL;
>>> +        goto out;
>>> +    }
>>> +
>>> +    *fmt = format->format;
>>
>> Let's avoid applying the format on one pad to then fail on the other
>> pad.
>>
>>     sink_format = v4l2_state_get_stream_format(state, format->pad,
>>                            format->stream);
>>     source_format = v4l2_subdev_state_get_opposite_stream_format(state,
>>                                      format->pad,
>>                                      format->stream);
>>     if (!sink_format || !source_format) {
>>         ret = -EINVAL;
>>         goto out;
>>     }
>>
>>     *sink_format = format->format;
>>     *source_format = format->format;
>>
>>> +
>>> +out:
>>> +    v4l2_subdev_unlock_state(state);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int unicam_subdev_enum_frame_size(struct v4l2_subdev *sd,
>>> +                     struct v4l2_subdev_state *state,
>>> +                     struct v4l2_subdev_frame_size_enum *fse)
>>> +{
>>> +    const struct unicam_fmt *fmtinfo;
>>> +    int ret = 0;
>>> +
>>> +    if (fse->index > 0)
>>> +        return -EINVAL;
>>> +
>>> +    v4l2_subdev_lock_state(state);
>>> +
>>> +    /* No transcoding, source and sink formats must match. */
>>> +    if (unicam_sd_pad_is_source(fse->pad)) {
>>> +        struct v4l2_mbus_framefmt *fmt;
>>> +
>>> +        fmt = v4l2_subdev_state_get_opposite_stream_format(state,
>>> +                                   fse->pad,
>>> +                                   fse->stream);
>>> +
>>
>> You can drop this blank line.
>>
>>> +        if (!fmt) {
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +
>>> +        if (fse->code != fmt->code) {
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +
>>> +        fse->min_width = fmt->width;
>>> +        fse->max_width = fmt->width;
>>> +        fse->min_height = fmt->height;
>>> +        fse->max_height = fmt->height;
>>> +    } else {
>>> +        fmtinfo = find_format_by_code(fse->code);
>>> +        if (!fmtinfo) {
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +
>>> +        fse->min_width = MIN_WIDTH * 8 / ALIGN(fmtinfo->depth, 8);
>>> +        fse->max_width = MAX_WIDTH * 8 / ALIGN(fmtinfo->depth, 8);
>>> +        fse->min_height = MIN_HEIGHT;
>>> +        fse->max_height = MAX_HEIGHT;
>>> +    }
>>> +
>>> +out:
>>> +    v4l2_subdev_unlock_state(state);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static const struct v4l2_subdev_video_ops unicam_subdev_video_ops = {
>>> +    .s_stream    = unicam_subdev_s_stream,
>>> +};
>>> +
>>> +static const struct v4l2_subdev_pad_ops unicam_subdev_pad_ops = {
>>> +    .init_cfg        = unicam_subdev_init_cfg,
>>> +    .enum_mbus_code        = unicam_subdev_enum_mbus_code,
>>> +    .get_fmt        = v4l2_subdev_get_fmt,
>>> +    .set_fmt        = unicam_subdev_set_pad_format,
>>> +    .set_routing        = unicam_subdev_set_routing,
>>> +    .enum_frame_size    = unicam_subdev_enum_frame_size,
>>> +};
>>> +
>>> +static const struct v4l2_subdev_ops unicam_subdev_ops = {
>>> +    .video        = &unicam_subdev_video_ops,
>>> +    .pad        = &unicam_subdev_pad_ops,
>>> +};
>>> +
>>> +static struct media_entity_operations unicam_subdev_media_ops = {
>>
>> static const
>>
>>> +    .link_validate = v4l2_subdev_link_validate,
>>> +    .has_route = v4l2_subdev_has_route,
>>> +};
>>> +
>>> +static int unicam_probe(struct platform_device *pdev)
>>> +{
>>> +    struct unicam_device *unicam;
>>> +    int ret = 0;
>>> +    int i;
>>> +
>>> +    unicam = kzalloc(sizeof(*unicam), GFP_KERNEL);
>>> +    if (!unicam)
>>> +        return -ENOMEM;
>>> +
>>> +    kref_init(&unicam->kref);
>>> +    unicam->pdev = pdev;
>>> +
>>> +    unicam->base = devm_platform_ioremap_resource(pdev, 0);
>>> +    if (IS_ERR(unicam->base)) {
>>> +        dev_err(unicam->v4l2_dev.dev, "Failed to get main io block\n");
>>
>> Let's use unicam->dev to access the struct device, especially given that
>> unicam->v4l2_dev is only initialized below when calling
>> bcm2835_media_dev_init().
>>
>> This message can actually br dropped, devm_platform_ioremap_resource()
>> already prints an error.
>>
>>> +        ret = PTR_ERR(unicam->base);
>>> +        goto err_unicam_put;
>>> +    }
>>> +
>>> +    unicam->clk_gate_base = devm_platform_ioremap_resource(pdev, 1);
>>> +    if (IS_ERR(unicam->clk_gate_base)) {
>>> +        dev_err(unicam->v4l2_dev.dev, "Failed to get 2nd io block\n");
>>
>> Same here.
>>
>>> +        ret = PTR_ERR(unicam->clk_gate_base);
>>> +        goto err_unicam_put;
>>> +    }
>>> +
>>> +    unicam->clock = devm_clk_get(&pdev->dev, "lp");
>>> +    if (IS_ERR(unicam->clock)) {
>>> +        dev_err(unicam->v4l2_dev.dev, "Failed to get lp clock\n");
>>> +        ret = PTR_ERR(unicam->clock);
>>> +        goto err_unicam_put;
>>> +    }
>>> +
>>> +    unicam->vpu_clock = devm_clk_get(&pdev->dev, "vpu");
>>> +    if (IS_ERR(unicam->vpu_clock)) {
>>> +        dev_err(unicam->v4l2_dev.dev, "Failed to get vpu clock\n");
>>> +        ret = PTR_ERR(unicam->vpu_clock);
>>> +        goto err_unicam_put;
>>> +    }
>>
>> Could the clock bulk API help here ?
>>
>>> +
>>> +    ret = platform_get_irq(pdev, 0);
>>> +    if (ret <= 0) {
>>> +        dev_err(&pdev->dev, "No IRQ resource\n");
>>> +        ret = -EINVAL;
>>> +        goto err_unicam_put;
>>> +    }
>>> +
>>> +    ret = devm_request_irq(&pdev->dev, ret, unicam_isr, 0,
>>> +                   "unicam_capture0", unicam);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "Unable to request interrupt\n");
>>> +        ret = -EINVAL;
>>> +        goto err_unicam_put;
>>> +    }
>>> +
>>> +    ret = bcm2835_media_dev_init(unicam, pdev);
>>> +    if (ret) {
>>> +        dev_err(unicam->v4l2_dev.dev,
>>> +            "Unable to register v4l2 device.\n");
>>> +        goto err_unicam_put;
>>> +    }
>>> +
>>> +    ret = media_device_register(&unicam->mdev);
>>> +    if (ret < 0) {
>>> +        dev_err(unicam->v4l2_dev.dev,
>>> +            "Unable to register media-controller device.\n");
>>> +        goto err_v4l2_unregister;
>>> +    }
>>> +
>>> +    /* Reserve space for the controls */
>>> +    ret = v4l2_ctrl_handler_init(&unicam->ctrl_handler, 16);
>>> +    if (ret < 0)
>>> +        goto err_media_unregister;
>>
>> The control handler is unused, drop it.
>>
>>> +
>>> +    /* set the driver data in platform device */
>>> +    platform_set_drvdata(pdev, unicam);
>>> +
>>> +    dev_dbg(unicam->v4l2_dev.dev, "Initialize internal subdev");
>>
>> You can drop this.
>>
>>> +    v4l2_subdev_init(&unicam->sd, &unicam_subdev_ops);
>>> +    v4l2_set_subdevdata(&unicam->sd, unicam);
>>> +    unicam->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>>> +    unicam->sd.dev = &pdev->dev;
>>> +    unicam->sd.owner = THIS_MODULE;
>>> +    unicam->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | 
>>> V4L2_SUBDEV_FL_MULTIPLEXED;
>>> +    snprintf(unicam->sd.name, sizeof(unicam->sd.name), 
>>> "unicam-subdev");
>>> +
>>> +    unicam->pads[UNICAM_SD_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>>> +
>>> +    for (i = UNICAM_SD_PAD_FIRST_SOURCE; i < UNICAM_SD_NUM_PADS; ++i)
>>> +        unicam->pads[i].flags = MEDIA_PAD_FL_SOURCE;
>>
>>     unicam->pads[UNICAM_SD_PAD_SOURCE_IMAGE].flags = MEDIA_PAD_FL_SOURCE;
>>     unicam->pads[UNICAM_SD_PAD_SOURCE_META].flags = MEDIA_PAD_FL_SOURCE;
>>
>>> +    unicam->sd.entity.ops = &unicam_subdev_media_ops;
>>> +    ret = media_entity_pads_init(&unicam->sd.entity,
>>> +                     ARRAY_SIZE(unicam->pads), unicam->pads);
>>> +    if (ret) {
>>> +        dev_err(unicam->v4l2_dev.dev, "Could not init media 
>>> controller for subdev");
>>> +        goto err_subdev_unregister;
>>> +    }
>>> +
>>> +    ret = v4l2_subdev_init_finalize(&unicam->sd);
>>> +    if (ret) {
>>> +        dev_err(unicam->v4l2_dev.dev, "Could not finalize init for 
>>> subdev");
>>> +        goto err_entity_cleanup;
>>> +    }
>>> +
>>> +    ret = v4l2_device_register_subdev(&unicam->v4l2_dev, &unicam->sd);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "Failed to register internal subdev\n");
>>> +        goto err_subdev_unregister;
>>> +    }
>>
>> Let's split initialization and registration of the subdev to a separate
>> function, placed above with the subdev operations.
>>
>>> +
>>> +    ret = of_unicam_connect_subdevs(unicam);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "Failed to connect subdevs\n");
>>> +        goto err_subdev_unregister;
>>> +    }
>>> +
>>> +    /* Enable the block power domain */
>>> +    pm_runtime_enable(&pdev->dev);
>>> +
>>> +    return 0;
>>> +
>>> +err_subdev_unregister:
>>> +    v4l2_subdev_cleanup(&unicam->sd);
>>> +err_entity_cleanup:
>>> +    media_entity_cleanup(&unicam->sd.entity);
>>> +err_media_unregister:
>>> +    media_device_unregister(&unicam->mdev);
>>> +err_v4l2_unregister:
>>> +    v4l2_device_unregister(&unicam->v4l2_dev);
>>> +err_unicam_put:
>>> +    unicam_put(unicam);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int unicam_remove(struct platform_device *pdev)
>>> +{
>>> +    struct unicam_device *unicam = platform_get_drvdata(pdev);
>>> +
>>> +    v4l2_async_nf_unregister(&unicam->notifier);
>>> +    v4l2_device_unregister(&unicam->v4l2_dev);
>>> +    media_device_unregister(&unicam->mdev);
>>> +    unregister_nodes(unicam);
>>
>> I'm a bit worried there could be race conditions with userspace here.
>> For instance, calling v4l2_async_nf_unregister() will result in
>> v4l2_device_unregister_subdev() being called on the sensor subdev, which
>> may race with userspace starting capture on a video node. The following
>> order would, I think be safer:
>>
>>     unregister_nodes(unicam);
>>     v4l2_device_unregister(&unicam->v4l2_dev);
>>     media_device_unregister(&unicam->mdev);
>>     v4l2_async_nf_unregister(&unicam->notifier);
>>
>> But this will cause a problem, when unregistering device nodes, the last
>> reference to unicam would be dropped. I think you could drop the calls
>> to unicam_put() from unicam_async_complete(), and add a unicam_put()
>> call here. Dave, does that sound good to you ?
>>
>>> +
>>> +    pm_runtime_disable(&pdev->dev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct of_device_id unicam_of_match[] = {
>>> +    { .compatible = "brcm,bcm2835-unicam", },
>>> +    { /* sentinel */ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, unicam_of_match);
>>> +
>>> +static struct platform_driver unicam_driver = {
>>> +    .probe        = unicam_probe,
>>> +    .remove        = unicam_remove,
>>> +    .driver = {
>>> +        .name    = UNICAM_MODULE_NAME,
>>> +        .of_match_table = of_match_ptr(unicam_of_match),
>>> +    },
>>> +};
>>> +
>>> +module_platform_driver(unicam_driver);
>>> +
>>> +MODULE_AUTHOR("Dave Stevenson <dave.stevenson@raspberrypi.com>");
>>> +MODULE_DESCRIPTION("BCM2835 Unicam driver");
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_VERSION(UNICAM_VERSION);
>>> diff --git a/drivers/media/platform/bcm2835/vc4-regs-unicam.h 
>>> b/drivers/media/platform/bcm2835/vc4-regs-unicam.h
>>> new file mode 100644
>>> index 000000000000..ae059a171d0f
>>> --- /dev/null
>>> +++ b/drivers/media/platform/bcm2835/vc4-regs-unicam.h
>>> @@ -0,0 +1,253 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +/*
>>> + * Copyright (C) 2017-2020 Raspberry Pi Trading.
>>> + * Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> + */
>>> +
>>> +#ifndef VC4_REGS_UNICAM_H
>>> +#define VC4_REGS_UNICAM_H
>>> +
>>> +/*
>>> + * The following values are taken from files found within the code drop
>>> + * made by Broadcom for the BCM21553 Graphics Driver, predominantly in
>>> + * brcm_usrlib/dag/vmcsx/vcinclude/hardware_vc4.h.
>>> + * They have been modified to be only the register offset.
>>> + */
>>> +#define UNICAM_CTRL    0x000
>>> +#define UNICAM_STA    0x004
>>> +#define UNICAM_ANA    0x008
>>> +#define UNICAM_PRI    0x00c
>>> +#define UNICAM_CLK    0x010
>>> +#define UNICAM_CLT    0x014
>>> +#define UNICAM_DAT0    0x018
>>> +#define UNICAM_DAT1    0x01c
>>> +#define UNICAM_DAT2    0x020
>>> +#define UNICAM_DAT3    0x024
>>> +#define UNICAM_DLT    0x028
>>> +#define UNICAM_CMP0    0x02c
>>> +#define UNICAM_CMP1    0x030
>>> +#define UNICAM_CAP0    0x034
>>> +#define UNICAM_CAP1    0x038
>>> +#define UNICAM_ICTL    0x100
>>> +#define UNICAM_ISTA    0x104
>>> +#define UNICAM_IDI0    0x108
>>> +#define UNICAM_IPIPE    0x10c
>>> +#define UNICAM_IBSA0    0x110
>>> +#define UNICAM_IBEA0    0x114
>>> +#define UNICAM_IBLS    0x118
>>> +#define UNICAM_IBWP    0x11c
>>> +#define UNICAM_IHWIN    0x120
>>> +#define UNICAM_IHSTA    0x124
>>> +#define UNICAM_IVWIN    0x128
>>> +#define UNICAM_IVSTA    0x12c
>>> +#define UNICAM_ICC    0x130
>>> +#define UNICAM_ICS    0x134
>>> +#define UNICAM_IDC    0x138
>>> +#define UNICAM_IDPO    0x13c
>>> +#define UNICAM_IDCA    0x140
>>> +#define UNICAM_IDCD    0x144
>>> +#define UNICAM_IDS    0x148
>>> +#define UNICAM_DCS    0x200
>>> +#define UNICAM_DBSA0    0x204
>>> +#define UNICAM_DBEA0    0x208
>>> +#define UNICAM_DBWP    0x20c
>>> +#define UNICAM_DBCTL    0x300
>>> +#define UNICAM_IBSA1    0x304
>>> +#define UNICAM_IBEA1    0x308
>>> +#define UNICAM_IDI1    0x30c
>>> +#define UNICAM_DBSA1    0x310
>>> +#define UNICAM_DBEA1    0x314
>>> +#define UNICAM_MISC    0x400
>>
>> Please add one tab before the value, to match the indentation of the
>> rest of the file.
>>
>>> +
>>> +/*
>>> + * The following bitmasks are from the kernel released by Broadcom
>>> + * for Android - https://android.googlesource.com/kernel/bcm/
>>> + * The Rhea, Hawaii, and Java chips all contain the same VideoCore4
>>> + * Unicam block as BCM2835, as defined in eg
>>> + * arch/arm/mach-rhea/include/mach/rdb_A0/brcm_rdb_cam.h and similar.
>>> + * Values reworked to use the kernel BIT and GENMASK macros.
>>> + *
>>> + * Some of the bit mnenomics have been amended to match the datasheet.
>>> + */
>>> +/* UNICAM_CTRL Register */
>>> +#define UNICAM_CPE        BIT(0)
>>> +#define UNICAM_MEM        BIT(1)
>>> +#define UNICAM_CPR        BIT(2)
>>> +#define UNICAM_CPM_MASK        GENMASK(3, 3)
>>> +#define UNICAM_CPM_CSI2        0
>>> +#define UNICAM_CPM_CCP2        1
>>> +#define UNICAM_SOE        BIT(4)
>>> +#define UNICAM_DCM_MASK        GENMASK(5, 5)
>>> +#define UNICAM_DCM_STROBE    0
>>> +#define UNICAM_DCM_DATA        1
>>> +#define UNICAM_SLS        BIT(6)
>>> +#define UNICAM_PFT_MASK        GENMASK(11, 8)
>>> +#define UNICAM_OET_MASK        GENMASK(20, 12)
>>> +
>>> +/* UNICAM_STA Register */
>>> +#define UNICAM_SYN        BIT(0)
>>> +#define UNICAM_CS        BIT(1)
>>> +#define UNICAM_SBE        BIT(2)
>>> +#define UNICAM_PBE        BIT(3)
>>> +#define UNICAM_HOE        BIT(4)
>>> +#define UNICAM_PLE        BIT(5)
>>> +#define UNICAM_SSC        BIT(6)
>>> +#define UNICAM_CRCE        BIT(7)
>>> +#define UNICAM_OES        BIT(8)
>>> +#define UNICAM_IFO        BIT(9)
>>> +#define UNICAM_OFO        BIT(10)
>>> +#define UNICAM_BFO        BIT(11)
>>> +#define UNICAM_DL        BIT(12)
>>> +#define UNICAM_PS        BIT(13)
>>> +#define UNICAM_IS        BIT(14)
>>> +#define UNICAM_PI0        BIT(15)
>>> +#define UNICAM_PI1        BIT(16)
>>> +#define UNICAM_FSI_S        BIT(17)
>>> +#define UNICAM_FEI_S        BIT(18)
>>> +#define UNICAM_LCI_S        BIT(19)
>>> +#define UNICAM_BUF0_RDY        BIT(20)
>>> +#define UNICAM_BUF0_NO        BIT(21)
>>> +#define UNICAM_BUF1_RDY        BIT(22)
>>> +#define UNICAM_BUF1_NO        BIT(23)
>>> +#define UNICAM_DI        BIT(24)
>>> +
>>> +#define UNICAM_STA_MASK_ALL \
>>> +        (UNICAM_DL + \
>>> +        UNICAM_SBE + \
>>> +        UNICAM_PBE + \
>>> +        UNICAM_HOE + \
>>> +        UNICAM_PLE + \
>>> +        UNICAM_SSC + \
>>> +        UNICAM_CRCE + \
>>> +        UNICAM_IFO + \
>>> +        UNICAM_OFO + \
>>> +        UNICAM_PS + \
>>> +        UNICAM_PI0 + \
>>> +        UNICAM_PI1)
>>
>> I'd use | instead of + to combine bits. Same below.
>>
>>> +
>>> +/* UNICAM_ANA Register */
>>> +#define UNICAM_APD        BIT(0)
>>> +#define UNICAM_BPD        BIT(1)
>>> +#define UNICAM_AR        BIT(2)
>>> +#define UNICAM_DDL        BIT(3)
>>> +#define UNICAM_CTATADJ_MASK    GENMASK(7, 4)
>>> +#define UNICAM_PTATADJ_MASK    GENMASK(11, 8)
>>> +
>>> +/* UNICAM_PRI Register */
>>> +#define UNICAM_PE        BIT(0)
>>> +#define UNICAM_PT_MASK        GENMASK(2, 1)
>>> +#define UNICAM_NP_MASK        GENMASK(7, 4)
>>> +#define UNICAM_PP_MASK        GENMASK(11, 8)
>>> +#define UNICAM_BS_MASK        GENMASK(15, 12)
>>> +#define UNICAM_BL_MASK        GENMASK(17, 16)
>>> +
>>> +/* UNICAM_CLK Register */
>>> +#define UNICAM_CLE        BIT(0)
>>> +#define UNICAM_CLPD        BIT(1)
>>> +#define UNICAM_CLLPE        BIT(2)
>>> +#define UNICAM_CLHSE        BIT(3)
>>> +#define UNICAM_CLTRE        BIT(4)
>>> +#define UNICAM_CLAC_MASK    GENMASK(8, 5)
>>> +#define UNICAM_CLSTE        BIT(29)
>>> +
>>> +/* UNICAM_CLT Register */
>>> +#define UNICAM_CLT1_MASK    GENMASK(7, 0)
>>> +#define UNICAM_CLT2_MASK    GENMASK(15, 8)
>>> +
>>> +/* UNICAM_DATn Registers */
>>> +#define UNICAM_DLE        BIT(0)
>>> +#define UNICAM_DLPD        BIT(1)
>>> +#define UNICAM_DLLPE        BIT(2)
>>> +#define UNICAM_DLHSE        BIT(3)
>>> +#define UNICAM_DLTRE        BIT(4)
>>> +#define UNICAM_DLSM        BIT(5)
>>> +#define UNICAM_DLFO        BIT(28)
>>> +#define UNICAM_DLSTE        BIT(29)
>>> +
>>> +#define UNICAM_DAT_MASK_ALL (UNICAM_DLSTE + UNICAM_DLFO)
>>
>> This also needs an indentation fix, as well as UNICAM_ISTA_MASK_ALL.
>>
>>> +
>>> +/* UNICAM_DLT Register */
>>> +#define UNICAM_DLT1_MASK    GENMASK(7, 0)
>>> +#define UNICAM_DLT2_MASK    GENMASK(15, 8)
>>> +#define UNICAM_DLT3_MASK    GENMASK(23, 16)
>>> +
>>> +/* UNICAM_ICTL Register */
>>> +#define UNICAM_FSIE        BIT(0)
>>> +#define UNICAM_FEIE        BIT(1)
>>> +#define UNICAM_IBOB        BIT(2)
>>> +#define UNICAM_FCM        BIT(3)
>>> +#define UNICAM_TFC        BIT(4)
>>> +#define UNICAM_LIP_MASK        GENMASK(6, 5)
>>> +#define UNICAM_LCIE_MASK    GENMASK(28, 16)
>>> +
>>> +/* UNICAM_IDI0/1 Register */
>>> +#define UNICAM_ID0_MASK        GENMASK(7, 0)
>>> +#define UNICAM_ID1_MASK        GENMASK(15, 8)
>>> +#define UNICAM_ID2_MASK        GENMASK(23, 16)
>>> +#define UNICAM_ID3_MASK        GENMASK(31, 24)
>>> +
>>> +/* UNICAM_ISTA Register */
>>> +#define UNICAM_FSI        BIT(0)
>>> +#define UNICAM_FEI        BIT(1)
>>> +#define UNICAM_LCI        BIT(2)
>>> +
>>> +#define UNICAM_ISTA_MASK_ALL (UNICAM_FSI + UNICAM_FEI + UNICAM_LCI)
>>> +
>>> +/* UNICAM_IPIPE Register */
>>> +#define UNICAM_PUM_MASK        GENMASK(2, 0)
>>> +        /* Unpacking modes */
>>> +        #define UNICAM_PUM_NONE        0
>>> +        #define UNICAM_PUM_UNPACK6    1
>>> +        #define UNICAM_PUM_UNPACK7    2
>>> +        #define UNICAM_PUM_UNPACK8    3
>>> +        #define UNICAM_PUM_UNPACK10    4
>>> +        #define UNICAM_PUM_UNPACK12    5
>>> +        #define UNICAM_PUM_UNPACK14    6
>>> +        #define UNICAM_PUM_UNPACK16    7
>>> +#define UNICAM_DDM_MASK        GENMASK(6, 3)
>>> +#define UNICAM_PPM_MASK        GENMASK(9, 7)
>>> +        /* Packing modes */
>>> +        #define UNICAM_PPM_NONE        0
>>> +        #define UNICAM_PPM_PACK8    1
>>> +        #define UNICAM_PPM_PACK10    2
>>> +        #define UNICAM_PPM_PACK12    3
>>> +        #define UNICAM_PPM_PACK14    4
>>> +        #define UNICAM_PPM_PACK16    5
>>
>> I'd drop the leading tab.
>>
>>> +#define UNICAM_DEM_MASK        GENMASK(11, 10)
>>> +#define UNICAM_DEBL_MASK    GENMASK(14, 12)
>>> +#define UNICAM_ICM_MASK        GENMASK(16, 15)
>>> +#define UNICAM_IDM_MASK        GENMASK(17, 17)
>>> +
>>> +/* UNICAM_ICC Register */
>>> +#define UNICAM_ICFL_MASK    GENMASK(4, 0)
>>> +#define UNICAM_ICFH_MASK    GENMASK(9, 5)
>>> +#define UNICAM_ICST_MASK    GENMASK(12, 10)
>>> +#define UNICAM_ICLT_MASK    GENMASK(15, 13)
>>> +#define UNICAM_ICLL_MASK    GENMASK(31, 16)
>>> +
>>> +/* UNICAM_DCS Register */
>>> +#define UNICAM_DIE        BIT(0)
>>> +#define UNICAM_DIM        BIT(1)
>>> +#define UNICAM_DBOB        BIT(3)
>>> +#define UNICAM_FDE        BIT(4)
>>> +#define UNICAM_LDP        BIT(5)
>>> +#define UNICAM_EDL_MASK        GENMASK(15, 8)
>>> +
>>> +/* UNICAM_DBCTL Register */
>>> +#define UNICAM_DBEN        BIT(0)
>>> +#define UNICAM_BUF0_IE        BIT(1)
>>> +#define UNICAM_BUF1_IE        BIT(2)
>>> +
>>> +/* UNICAM_CMP[0,1] register */
>>> +#define UNICAM_PCE        BIT(31)
>>> +#define UNICAM_GI        BIT(9)
>>> +#define UNICAM_CPH        BIT(8)
>>> +#define UNICAM_PCVC_MASK    GENMASK(7, 6)
>>> +#define UNICAM_PCDT_MASK    GENMASK(5, 0)
>>> +
>>> +/* UNICAM_MISC register */
>>> +#define UNICAM_FL0        BIT(6)
>>> +#define UNICAM_FL1        BIT(9)
>>> +
>>> +#endif
>>