| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <eb6488f6-330f-d42c-679a-393ac899e98f@ideasonboard.com>
Date: Wed, 26 Jan 2022 15:54:58 +0100
From: Jean-Michel Hautbois <jeanmichel.hautbois@...asonboard.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: dave.stevenson@...pberrypi.com, devicetree@...r.kernel.org,
kernel-list@...pberrypi.com, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
linux-rpi-kernel@...ts.infradead.org, lukasz@...y.st,
mchehab@...nel.org, naush@...pberrypi.com, robh@...nel.org,
tomi.valkeinen@...asonboard.com, Maxime Ripard <maxime@...no.tech>
Subject: Re: [RFC PATCH v2 4/7] media: bcm2835-unicam: Add support for for
CCP2/CSI2 camera interface
+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@...pberrypi.com>
>>> Signed-off-by: Naushir Patuck <naush@...pberrypi.com>
>>> Signed-off-by: Jean-Michel Hautbois
>>> <jeanmichel.hautbois@...asonboard.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@...pberrypi.com>
>>> L: linux-media@...r.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@...ke-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@...pberrypi.com>
>>> + *
>>> + * Based on TI am437x driver by
>>> + * Benoit Parrot <bparrot@...com>
>>> + * Lad, Prabhakar <prabhakar.csengg@...il.com>
>>> + *
>>> + * and TI CAL camera interface driver by
>>> + * Benoit Parrot <bparrot@...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@...pberrypi.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@...pberrypi.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
>>
Powered by blists - more mailing lists