Hi Sakari Answering one of the review comments On Tue, 15 Sep 2020 at 08:04, Sakari Ailus <sakari.ailus@xxxxxx> wrote: > > Hi Laurent, > > Thanks for the patch, and my apologies for the late review. Please do cc me > for v3. > > After a quick look I can already say this is the cleanest Unicam driver > I've ever seen. But please also see my detailed comments below. > > On Mon, May 04, 2020 at 12:25:41PM +0300, Laurent Pinchart wrote: > > From: Naushir Patuck <naush@xxxxxxxxxxxxxxx> > > > > Add a driver for the Unicam camera receiver block on BCM283x processors. > > Compared to the bcm2835-camera driver present in staging, this driver > > handles the Unicam block only (CSI-2 receiver), and doesn't depend on > > the VC4 firmware running on the VPU. > > > > The commit is made up of a series of changes cherry-picked from the > > rpi-5.4.y branch of https://github.com/raspberrypi/linux/ with > > additional enhancements, forward-ported to the mainline kernel. > > > > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > Signed-off-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > --- > > Changes since v1: > > > > - Re-fetch mbus code from subdev on a g_fmt call > > - Group all ioctl disabling together > > - Fix reference counting in unicam_open > > - Add support for VIDIOC_[S|G]_SELECTION > > --- > > MAINTAINERS | 7 + > > drivers/media/platform/Kconfig | 1 + > > drivers/media/platform/Makefile | 2 + > > drivers/media/platform/bcm2835/Kconfig | 15 + > > drivers/media/platform/bcm2835/Makefile | 3 + > > .../media/platform/bcm2835/bcm2835-unicam.c | 2825 +++++++++++++++++ > > .../media/platform/bcm2835/vc4-regs-unicam.h | 253 ++ > > 7 files changed, 3106 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 b816a453b10e..edf5b8d9c98a 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -3341,6 +3341,13 @@ N: bcm113* > > N: bcm216* > > N: kona > > > > +BROADCOM BCM2835 CAMERA DRIVER > > +M: Raspberry Pi Kernel Maintenance <kernel-list@xxxxxxxxxxxxxxx> > > +L: linux-media@xxxxxxxxxxxxxxx > > +S: Maintained > > +F: drivers/media/platform/bcm2835/ > > +F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml > > + > > BROADCOM BCM47XX MIPS ARCHITECTURE > > M: Hauke Mehrtens <hauke@xxxxxxxxxx> > > M: Rafał Miłecki <zajec5@xxxxxxxxx> > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > > index e01bbb9dd1c1..98721a4e0be1 100644 > > --- a/drivers/media/platform/Kconfig > > +++ b/drivers/media/platform/Kconfig > > @@ -146,6 +146,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 d13db96e3015..a425e4d2e3f3 100644 > > --- a/drivers/media/platform/Makefile > > +++ b/drivers/media/platform/Makefile > > @@ -98,4 +98,6 @@ obj-y += meson/ > > > > obj-y += cros-ec-cec/ > > > > +obj-y += bcm2835/ > > + > > obj-y += sunxi/ > > diff --git a/drivers/media/platform/bcm2835/Kconfig b/drivers/media/platform/bcm2835/Kconfig > > new file mode 100644 > > index 000000000000..ec46e3ef053c > > --- /dev/null > > +++ b/drivers/media/platform/bcm2835/Kconfig > > @@ -0,0 +1,15 @@ > > +# Broadcom VideoCore4 V4L2 camera support > > + > > +config VIDEO_BCM2835_UNICAM > > + tristate "Broadcom BCM2835 Unicam video capture driver" > > + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER > > + depends on ARCH_BCM2835 || COMPILE_TEST > > + select VIDEOBUF2_DMA_CONTIG > > + select V4L2_FWNODE > > + help > > + Say Y here to enable support for the BCM2835 CSI-2 receiver. This is a > > + V4L2 driver that controls the CSI-2 receiver directly, independently > > + from the VC4 firmware. > > \o/ > > > + > > + 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..2e9387cbc1e0 > > --- /dev/null > > +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c > > @@ -0,0 +1,2825 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * BCM2835 Unicam Capture Driver > > + * > > + * Copyright (C) 2017-2020 - Raspberry Pi (Trading) Ltd. > > + * > > + * Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > + * > > + * Based on TI am437x driver by > > + * Benoit Parrot <bparrot@xxxxxx> > > + * Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> > > + * > > + * and TI CAL camera interface driver by > > + * Benoit Parrot <bparrot@xxxxxx> > > + * > > + * > > + * 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 "vc4-regs-unicam.h" > > + > > +#define UNICAM_MODULE_NAME "unicam" > > +#define UNICAM_VERSION "0.1.0" > > + > > +static int debug; > > +module_param(debug, int, 0644); > > +MODULE_PARM_DESC(debug, "Debug level 0-3"); > > + > > +#define unicam_dbg(level, dev, fmt, arg...) \ > > + v4l2_dbg(level, debug, &(dev)->v4l2_dev, fmt, ##arg) > > +#define unicam_info(dev, fmt, arg...) \ > > + v4l2_info(&(dev)->v4l2_dev, fmt, ##arg) > > +#define unicam_err(dev, fmt, arg...) \ > > + v4l2_err(&(dev)->v4l2_dev, fmt, ##arg) > > I'd just use dev_*() macros in new drivers unless you really need something > else. It's configurable. Up to you... > > > + > > +/* > > + * 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 > > Hmm. > > Is it Unicam driver's job to work around sensor driver bugs? That same bug > would hit user space, too. I'd say such a sensor driver must be fixed, and > not just worked around here and there. Yes, it would hit userspace too. However the issue from Unicam's side is that in order to support and enumerate both the packed and unpacked version of each Bayer or mono format (eg RGGB10 and RGGB10P), unicam_enum_fmt_vid_cap loops until either it reaches the requested index (and it only counts supported formats), or the subdev returns an error. If the subdev has a bug such that it doesn't check index and return an error, and the last format isn't supported, then there is no way of breaking that loop. Being stuck in a loop within the kernel, whether the fault of the subdev or Unicam, is surely a bad thing, hence some defensive programming. Yes, I was bitten by this, but I don't recall which driver it was that I was working on at the time. > > + > > +/* > > + * 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 8192 > > + > > +/* > > + * 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) > > + > > +enum pad_types { > > + IMAGE_PAD, > > + METADATA_PAD, > > + MAX_NODES > > +}; > > + > > +/* > > + * 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. > > + * @check_variants: Flag to denote that there are multiple mediabus formats > > + * still in the list that could match this V4L2 format. > > + */ > > +struct unicam_fmt { > > + u32 fourcc; > > + u32 repacked_fourcc; > > + u32 code; > > + u8 depth; > > + u8 csi_dt; > > + u8 check_variants; > > +}; > > + > > +static const struct unicam_fmt formats[] = { > > + /* YUV Formats */ > > + { > > + .fourcc = V4L2_PIX_FMT_YUYV, > > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > > + .depth = 16, > > + .csi_dt = 0x1e, > > + .check_variants = 1, > > + }, { > > + .fourcc = V4L2_PIX_FMT_UYVY, > > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > > + .depth = 16, > > + .csi_dt = 0x1e, > > + .check_variants = 1, > > + }, { > > + .fourcc = V4L2_PIX_FMT_YVYU, > > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > > + .depth = 16, > > + .csi_dt = 0x1e, > > + .check_variants = 1, > > + }, { > > + .fourcc = V4L2_PIX_FMT_VYUY, > > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > > + .depth = 16, > > + .csi_dt = 0x1e, > > + .check_variants = 1, > > + }, { > > + .fourcc = V4L2_PIX_FMT_YUYV, > > + .code = MEDIA_BUS_FMT_YUYV8_1X16, > > + .depth = 16, > > + .csi_dt = 0x1e, > > + }, { > > + .fourcc = V4L2_PIX_FMT_UYVY, > > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > > + .depth = 16, > > + .csi_dt = 0x1e, > > + }, { > > + .fourcc = V4L2_PIX_FMT_YVYU, > > + .code = MEDIA_BUS_FMT_YVYU8_1X16, > > + .depth = 16, > > + .csi_dt = 0x1e, > > + }, { > > + .fourcc = V4L2_PIX_FMT_VYUY, > > + .code = MEDIA_BUS_FMT_VYUY8_1X16, > > + .depth = 16, > > + .csi_dt = 0x1e, > > + }, { > > + /* RGB Formats */ > > + .fourcc = V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */ > > + .code = MEDIA_BUS_FMT_RGB565_2X8_LE, > > + .depth = 16, > > + .csi_dt = 0x22, > > + }, { > > + .fourcc = V4L2_PIX_FMT_RGB565X, /* rrrrrggg gggbbbbb */ > > + .code = MEDIA_BUS_FMT_RGB565_2X8_BE, > > + .depth = 16, > > + .csi_dt = 0x22 > > + }, { > > + .fourcc = V4L2_PIX_FMT_RGB555, /* gggbbbbb arrrrrgg */ > > + .code = MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE, > > + .depth = 16, > > + .csi_dt = 0x21, > > + }, { > > + .fourcc = V4L2_PIX_FMT_RGB555X, /* arrrrrgg gggbbbbb */ > > + .code = MEDIA_BUS_FMT_RGB555_2X8_PADHI_BE, > > + .depth = 16, > > + .csi_dt = 0x21, > > + }, { > > + .fourcc = V4L2_PIX_FMT_RGB24, /* rgb */ > > + .code = MEDIA_BUS_FMT_RGB888_1X24, > > + .depth = 24, > > + .csi_dt = 0x24, > > + }, { > > + .fourcc = V4L2_PIX_FMT_BGR24, /* bgr */ > > + .code = MEDIA_BUS_FMT_BGR888_1X24, > > + .depth = 24, > > + .csi_dt = 0x24, > > + }, { > > + .fourcc = V4L2_PIX_FMT_RGB32, /* argb */ > > + .code = MEDIA_BUS_FMT_ARGB8888_1X32, > > + .depth = 32, > > + .csi_dt = 0x0, > > + }, { > > + /* Bayer Formats */ > > + .fourcc = V4L2_PIX_FMT_SBGGR8, > > + .code = MEDIA_BUS_FMT_SBGGR8_1X8, > > + .depth = 8, > > + .csi_dt = 0x2a, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SGBRG8, > > + .code = MEDIA_BUS_FMT_SGBRG8_1X8, > > + .depth = 8, > > + .csi_dt = 0x2a, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SGRBG8, > > + .code = MEDIA_BUS_FMT_SGRBG8_1X8, > > + .depth = 8, > > + .csi_dt = 0x2a, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SRGGB8, > > + .code = MEDIA_BUS_FMT_SRGGB8_1X8, > > + .depth = 8, > > + .csi_dt = 0x2a, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SBGGR10P, > > + .repacked_fourcc = V4L2_PIX_FMT_SBGGR10, > > + .code = MEDIA_BUS_FMT_SBGGR10_1X10, > > + .depth = 10, > > + .csi_dt = 0x2b, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SGBRG10P, > > + .repacked_fourcc = V4L2_PIX_FMT_SGBRG10, > > + .code = MEDIA_BUS_FMT_SGBRG10_1X10, > > + .depth = 10, > > + .csi_dt = 0x2b, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SGRBG10P, > > + .repacked_fourcc = V4L2_PIX_FMT_SGRBG10, > > + .code = MEDIA_BUS_FMT_SGRBG10_1X10, > > + .depth = 10, > > + .csi_dt = 0x2b, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SRGGB10P, > > + .repacked_fourcc = V4L2_PIX_FMT_SRGGB10, > > + .code = MEDIA_BUS_FMT_SRGGB10_1X10, > > + .depth = 10, > > + .csi_dt = 0x2b, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SBGGR12P, > > + .repacked_fourcc = V4L2_PIX_FMT_SBGGR12, > > + .code = MEDIA_BUS_FMT_SBGGR12_1X12, > > + .depth = 12, > > + .csi_dt = 0x2c, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SGBRG12P, > > + .repacked_fourcc = V4L2_PIX_FMT_SGBRG12, > > + .code = MEDIA_BUS_FMT_SGBRG12_1X12, > > + .depth = 12, > > + .csi_dt = 0x2c, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SGRBG12P, > > + .repacked_fourcc = V4L2_PIX_FMT_SGRBG12, > > + .code = MEDIA_BUS_FMT_SGRBG12_1X12, > > + .depth = 12, > > + .csi_dt = 0x2c, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SRGGB12P, > > + .repacked_fourcc = V4L2_PIX_FMT_SRGGB12, > > + .code = MEDIA_BUS_FMT_SRGGB12_1X12, > > + .depth = 12, > > + .csi_dt = 0x2c, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SBGGR14P, > > + .code = MEDIA_BUS_FMT_SBGGR14_1X14, > > + .depth = 14, > > + .csi_dt = 0x2d, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SGBRG14P, > > + .code = MEDIA_BUS_FMT_SGBRG14_1X14, > > + .depth = 14, > > + .csi_dt = 0x2d, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SGRBG14P, > > + .code = MEDIA_BUS_FMT_SGRBG14_1X14, > > + .depth = 14, > > + .csi_dt = 0x2d, > > + }, { > > + .fourcc = V4L2_PIX_FMT_SRGGB14P, > > + .code = MEDIA_BUS_FMT_SRGGB14_1X14, > > + .depth = 14, > > + .csi_dt = 0x2d, > > + }, { > > + /* > > + * 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. > > + */ > > + > > + /* Greyscale formats */ > > + .fourcc = V4L2_PIX_FMT_GREY, > > + .code = MEDIA_BUS_FMT_Y8_1X8, > > + .depth = 8, > > + .csi_dt = 0x2a, > > + }, { > > + .fourcc = V4L2_PIX_FMT_Y10P, > > + .repacked_fourcc = V4L2_PIX_FMT_Y10, > > + .code = MEDIA_BUS_FMT_Y10_1X10, > > + .depth = 10, > > + .csi_dt = 0x2b, > > + }, { > > + /* NB There is no packed V4L2 fourcc for this format. */ > > + .repacked_fourcc = V4L2_PIX_FMT_Y12, > > + .code = MEDIA_BUS_FMT_Y12_1X12, > > + .depth = 12, > > + .csi_dt = 0x2c, > > + }, > > + /* Embedded data format */ > > + { > > + .fourcc = V4L2_META_FMT_SENSOR_DATA, > > + .code = MEDIA_BUS_FMT_SENSOR_DATA, > > + .depth = 8, > > + } > > +}; > > + > > +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; > > + bool streaming; > > + unsigned int 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; > > + /* > > + * 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; > > + /* clock handle */ > > + struct clk *clock; > > + /* V4l2 device */ > > + struct v4l2_device v4l2_dev; > > + struct media_device mdev; > > + > > + /* parent device */ > > + struct platform_device *pdev; > > + /* 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_pad_config *sensor_config; > > + > > + 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; > > +}; > > + > > +static inline struct unicam_device * > > +to_unicam_device(struct v4l2_device *v4l2_dev) > > +{ > > + return container_of(v4l2_dev, struct unicam_device, v4l2_dev); > > +} > > + > > +/* Hardware access */ > > +static inline void clk_write(struct unicam_device *dev, u32 val) > > +{ > > + writel(val | 0x5a000000, dev->clk_gate_base); > > +} > > + > > +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) > > +{ > > + 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); > > +} > > + > > +/* Power management functions */ > > +static inline int unicam_runtime_get(struct unicam_device *dev) > > +{ > > + return pm_runtime_get_sync(&dev->pdev->dev); > > +} > > + > > +static inline void unicam_runtime_put(struct unicam_device *dev) > > +{ > > + pm_runtime_put_sync(&dev->pdev->dev); > > +} > > Please don't add such wrappers. > > > + > > +/* 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 int check_mbus_format(struct unicam_device *dev, > > + const struct unicam_fmt *format) > > +{ > > + unsigned int i; > > + int ret = 0; > > + > > + for (i = 0; !ret && i < MAX_ENUM_MBUS_CODE; i++) { > > + struct v4l2_subdev_mbus_code_enum mbus_code = { > > + .index = i, > > + .pad = IMAGE_PAD, > > + .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > + }; > > + > > + ret = v4l2_subdev_call(dev->sensor, pad, enum_mbus_code, > > + NULL, &mbus_code); > > + > > + if (!ret && mbus_code.code == format->code) > > + return 1; > > + } > > + > > + return 0; > > +} > > + > > +static const struct unicam_fmt *find_format_by_pix(struct unicam_device *dev, > > + u32 pixelformat) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > > + if (formats[i].fourcc == pixelformat || > > + formats[i].repacked_fourcc == pixelformat) { > > + if (formats[i].check_variants && > > + !check_mbus_format(dev, &formats[i])) > > + continue; > > + return &formats[i]; > > + } > > + } > > + > > + return NULL; > > +} > > + > > +static inline 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 __subdev_get_format(struct unicam_device *dev, > > + struct v4l2_mbus_framefmt *fmt, int pad_id) > > +{ > > + struct v4l2_subdev_format sd_fmt = { > > + .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > + .pad = pad_id > > + }; > > + int ret; > > + > > + ret = v4l2_subdev_call(dev->sensor, pad, get_fmt, dev->sensor_config, > > + &sd_fmt); > > + if (ret < 0) > > + return ret; > > + > > + *fmt = sd_fmt.format; > > + > > + unicam_dbg(1, dev, "%s %dx%d code:%04x\n", __func__, > > + fmt->width, fmt->height, fmt->code); > > + > > + return 0; > > +} > > + > > +static int __subdev_set_format(struct unicam_device *dev, > > + struct v4l2_mbus_framefmt *fmt, int pad_id) > > +{ > > + struct v4l2_subdev_format sd_fmt = { > > + .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > + .pad = pad_id > > + }; > > + int ret; > > + > > + sd_fmt.format = *fmt; > > + > > + ret = v4l2_subdev_call(dev->sensor, pad, set_fmt, dev->sensor_config, > > + &sd_fmt); > > + if (ret < 0) > > + return ret; > > + > > + *fmt = sd_fmt.format; > > + > > + if (pad_id == IMAGE_PAD) > > + unicam_dbg(1, dev, "%s %dx%d code:%04x\n", __func__, fmt->width, > > + fmt->height, fmt->code); > > + else > > + unicam_dbg(1, dev, "%s Embedded data code:%04x\n", __func__, > > + sd_fmt.format.code); > > + > > + return 0; > > +} > > + > > +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; > > + > > + unicam_dbg(3, 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 int unicam_reset_format(struct unicam_node *node) > > +{ > > + struct unicam_device *dev = node->dev; > > + struct v4l2_mbus_framefmt mbus_fmt; > > + int ret; > > + > > + if (dev->sensor_embedded_data || node->pad_id != METADATA_PAD) { > > + ret = __subdev_get_format(dev, &mbus_fmt, node->pad_id); > > + if (ret) { > > + unicam_err(dev, "Failed to get_format - ret %d\n", ret); > > + return ret; > > + } > > + > > + if (mbus_fmt.code != node->fmt->code) { > > + unicam_err(dev, "code mismatch - fmt->code %08x, mbus_fmt.code %08x\n", > > + node->fmt->code, mbus_fmt.code); > > + return ret; > > + } > > + } > > + > > + if (node->pad_id == IMAGE_PAD) { > > + v4l2_fill_pix_format(&node->v_fmt.fmt.pix, &mbus_fmt); > > + node->v_fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > > + unicam_calc_format_size_bpl(dev, node->fmt, &node->v_fmt); > > + } else { > > + node->v_fmt.type = V4L2_BUF_TYPE_META_CAPTURE; > > + node->v_fmt.fmt.meta.dataformat = V4L2_META_FMT_SENSOR_DATA; > > + if (dev->sensor_embedded_data) { > > + node->v_fmt.fmt.meta.buffersize = > > + mbus_fmt.width * mbus_fmt.height; > > + node->embedded_lines = mbus_fmt.height; > > + } else { > > + node->v_fmt.fmt.meta.buffersize = UNICAM_EMBEDDED_SIZE; > > + node->embedded_lines = 1; > > + } > > + } > > + > > + node->m_fmt = mbus_fmt; > > + return 0; > > +} > > + > > +static void unicam_wr_dma_addr(struct unicam_device *dev, dma_addr_t dmaaddr, > > + unsigned int buffer_size, int pad_id) > > +{ > > + dma_addr_t endaddr = dmaaddr + buffer_size; > > + > > + /* > > + * dmaaddr and endaddr should be a 32-bit address with the top two bits > > + * set to 0x3 to signify uncached access through the Videocore memory > > + * controller. > > + */ > > + WARN_ON((dmaaddr >> 30) != 0x3 || (endaddr >> 30) != 0x3); > > + > > + if (pad_id == IMAGE_PAD) { > > + 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 inline unsigned int unicam_get_lines_done(struct unicam_device *dev) > > +{ > > + dma_addr_t start_addr, cur_addr; > > + unsigned int stride = dev->node[IMAGE_PAD].v_fmt.fmt.pix.bytesperline; > > + struct unicam_buffer *frm = dev->node[IMAGE_PAD].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 inline 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); > > + size = (node->pad_id == IMAGE_PAD) ? > > + node->v_fmt.fmt.pix.sizeimage : > > + node->v_fmt.fmt.meta.buffersize; > > + > > + unicam_wr_dma_addr(dev, addr, size, node->pad_id); > > +} > > + > > +static inline void unicam_schedule_dummy_buffer(struct unicam_node *node) > > +{ > > + struct unicam_device *dev = node->dev; > > + > > + unicam_dbg(3, dev, "Scheduling dummy buffer for node %d\n", > > + node->pad_id); > > + > > + unicam_wr_dma_addr(dev, node->dummy_buf_dma_addr, DUMMY_BUF_SIZE, > > + node->pad_id); > > + node->next_frm = NULL; > > +} > > + > > +static inline 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 bool unicam_all_nodes_streaming(struct unicam_device *dev) > > +{ > > + bool ret; > > + > > + ret = dev->node[IMAGE_PAD].open && dev->node[IMAGE_PAD].streaming; > > + ret &= !dev->node[METADATA_PAD].open || > > + dev->node[METADATA_PAD].streaming; > > + return ret; > > +} > > + > > +static bool unicam_all_nodes_disabled(struct unicam_device *dev) > > +{ > > + return !dev->node[IMAGE_PAD].streaming && > > + !dev->node[METADATA_PAD].streaming; > > +} > > + > > +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_PAD].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; > > + u64 ts; > > + > > + /* > > + * Don't service interrupts if not streaming. > > + * Avoids issues if the VPU should enable the > > + * peripheral without the kernel knowing (that > > + * shouldn't happen, but causes issues if it does). > > + */ > > + if (unicam_all_nodes_disabled(unicam)) > > + return IRQ_NONE; > > + > > + 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); > > + > > + unicam_dbg(3, unicam, "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; > > + > > + /* > > + * 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 (ista & UNICAM_FEI || sta & UNICAM_PI0) { > > + /* > > + * 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 (unicam->node[i].cur_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; > > + /* > > + * 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) && !(ista & UNICAM_FEI)) { > > + 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; > > +} > > + > > +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; > > + > > + strlcpy(cap->driver, UNICAM_MODULE_NAME, sizeof(cap->driver)); > > + strlcpy(cap->card, UNICAM_MODULE_NAME, sizeof(cap->card)); > > strscpy()? > > > + > > + 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; > > Please also fill cap->device_caps. > > > + > > + return 0; > > +} > > + > > +static int unicam_enum_fmt_vid_cap(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > +{ > > + struct unicam_node *node = video_drvdata(file); > > + struct unicam_device *dev = node->dev; > > + unsigned int index = 0; > > + unsigned int i; > > + int ret = 0; > > + > > + if (node->pad_id != IMAGE_PAD) > > + return -EINVAL; > > + > > + for (i = 0; !ret && i < MAX_ENUM_MBUS_CODE; i++) { > > + struct v4l2_subdev_mbus_code_enum mbus_code = { > > + .index = i, > > + .pad = IMAGE_PAD, > > + .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > + }; > > + const struct unicam_fmt *fmt; > > + > > + ret = v4l2_subdev_call(dev->sensor, pad, enum_mbus_code, > > + NULL, &mbus_code); > > + if (ret < 0) { > > + unicam_dbg(2, dev, > > + "subdev->enum_mbus_code idx %d returned %d - index invalid\n", > > + i, ret); > > + return -EINVAL; > > + } > > + > > + fmt = find_format_by_code(mbus_code.code); > > + if (fmt) { > > + if (fmt->fourcc) { > > + if (index == f->index) { > > + f->pixelformat = fmt->fourcc; > > + break; > > + } > > + index++; > > + } > > + if (fmt->repacked_fourcc) { > > + if (index == f->index) { > > + f->pixelformat = fmt->repacked_fourcc; > > + break; > > + } > > + index++; > > + } > > + } > > + } Following on from the above comment, we should check if (i >= MAX_ENUM_MBUS_CODE) return -EINVAL; rather than always returning 0 from here. Possibly add a WARN_ONCE as well to flag the subdev bug? Dave
