Add bindings documentation for Microchip CSI2 Demultiplexer controller.
CSI2DC is a demultiplexer from Synopsys IDI interface specification to
parallel interface connection or direct memory access.
Signed-off-by: Eugen Hristev <[email protected]>
---
Changes in v3:
- Removed some text from description, as it was explained in the schema
- fixed other things as per Rob's review
- moved some text inside the schema, like the clock description
Changes in v2:
- fixed warnings reported by dt_binding_check
.../bindings/media/microchip,csi2dc.yaml | 174 ++++++++++++++++++
1 file changed, 174 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
diff --git a/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml b/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
new file mode 100644
index 000000000000..b4c1b8800a3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
@@ -0,0 +1,174 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/microchip,csi2dc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip CSI2 Demux Controller (CSI2DC)
+
+maintainers:
+ - Eugen Hristev <[email protected]>
+
+description:
+ CSI2DC - Camera Serial Interface 2 Demux Controller
+
+ CSI2DC is a hardware block that receives incoming data from an IDI interface
+ and filters packets based on their data type and virtual channel identifier,
+ then converts the byte stream into a cross clock domain to a pixel stream
+ to a parallel interface that can be read by a sensor controller.
+
+ CSI2DC provides two pipes, one video pipe and one data pipe. Video pipe
+ is connected to a sensor controller and the data pipe is accessible
+ as a DMA slave port to a DMA controller.
+
+ CSI2DC supports a single 'port' node as a source pad with Synopsys 32-bit
+ IDI interface. The connected endpoint must be a IDI interface compatible
+ device (like Synopsys CSI2HOST) , that can provide 32-bit IDI interface
+ connection as sink pad.
+ For media entity and endpoints please refer to the bindings defined in
+ Documentation/devicetree/bindings/media/video-interfaces.txt.
+ For Synopsys IDI interface please refer to
+ Documentation/devicetree/bindings/media/snps,dw-csi-plat.txt
+
+ CSI2DC supports one 'port' node as sink pad with parallel interface. This is
+ called video pipe.
+ This port has an 'endpoint' can then be used as a source pad for another
+ controller (next in pipeline).
+ Please refer to the bindings defined in
+ Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+ CSI2DC also supports direct access to the data through AHB, via DMA channel,
+ called data pipe.
+ Because of this, the sink 'port' child node (second) is not mandatory.
+ If the sink 'port' child node is missing, only data pipe is available.
+
+properties:
+ compatible:
+ const: microchip,sama7g5-csi2dc
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 2
+
+ clock-names:
+ description:
+ CSI2DC must have two clocks to function correctly. One clock is the
+ peripheral clock for the inside functionality of the hardware block.
+ This is named 'pclk'. The second clock must be the cross domain clock,
+ in which CSI2DC will perform clock crossing. This clock must be fed
+ by the next controller in pipeline, which usually is a sensor controller.
+ Normally this clock should be given by this sensor controller who
+ is also a clock source. This clock is named 'scck', sensor controller clock.
+ items:
+ - const: pclk
+ - const: scck
+
+ microchip,clk-gated:
+ type: boolean
+ description:
+ If present, indicates that the clock is gated.
+ Otherwise, the clock is free-running.
+
+ microchip,inter-line-delay:
+ allOf:
+ - $ref: /schemas/types.yaml#/definitions/uint32
+ - minimum: 1
+ - maximum: 16
+ default: 16
+ description:
+ Indicates how many clock cycles should be introduced between each line.
+
+ port@0:
+ type: object
+ description:
+ Input port node, single endpoint describing the input pad.
+
+ properties:
+ reg:
+ const: 0
+
+ endpoint:
+ type: object
+
+ properties:
+ remote-endpoint: true
+
+ required:
+ - remote-endpoint
+
+ additionalProperties: false
+
+ additionalProperties: false
+
+ port@1:
+ type: object
+ description:
+ Output port node, single endpoint, describing the output pad.
+
+ properties:
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ reg:
+ const: 1
+
+ patternProperties:
+ "^endpoint@[0-3]$":
+ type: object
+
+ properties:
+ reg:
+ enum: [0, 1, 2, 3]
+ description: virtual channel for the endpoint
+
+ remote-endpoint: true
+
+ required:
+ - remote-endpoint
+ - reg
+
+ additionalProperties: false
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - port@0
+
+examples:
+ - |
+ csi2dc@e1404000 {
+ compatible = "microchip,sama7g5-csi2dc";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0xe1404000 0x500>;
+ clocks = <&pclk>, <&scck>;
+ clock-names = "pclk", "scck";
+
+ port@0 {
+ reg = <0>; /* must be 0, first child port */
+ csi2dc_in: endpoint { /* input from IDI interface */
+ remote-endpoint = <&csi2host_out>;
+ };
+ };
+
+ port@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>; /* must be 1, second child port */
+ csi2dc_out: endpoint@2 {
+ reg = <2>; /* virtual channel identifier */
+ remote-endpoint = <&xisc_in>; /* output to sensor controller */
+ };
+ };
+ };
+
+...
--
2.25.1
Microchip CSI2DC (CSI2 Demultiplexer Controller) is a misc bridge device
that converts a byte stream in IDI Synopsys format (coming from a CSI2HOST)
to a pixel stream that can be captured by a sensor controller.
Signed-off-by: Eugen Hristev <[email protected]>
---
Changes in v2:
- moved driver to platform/atmel
- fixed minor things as per Sakari's review
- still some things from v2 review are not yet addressed, to be followed up
drivers/media/platform/atmel/Kconfig | 13 +
drivers/media/platform/atmel/Makefile | 1 +
.../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++
3 files changed, 714 insertions(+)
create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.c
diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig
index 1850fe7f9360..80bbddcc2504 100644
--- a/drivers/media/platform/atmel/Kconfig
+++ b/drivers/media/platform/atmel/Kconfig
@@ -21,3 +21,16 @@ config VIDEO_ATMEL_ISI
help
This module makes the ATMEL Image Sensor Interface available
as a v4l2 device.
+
+config VIDEO_MICROCHIP_CSI2DC
+ tristate "Microchip CSI2 Demux Controller"
+ depends on VIDEO_V4L2 && COMMON_CLK && OF
+ depends on ARCH_AT91 || COMPILE_TEST
+ select MEDIA_CONTROLLER
+ select VIDEO_V4L2_SUBDEV_API
+ select V4L2_FWNODE
+ help
+ CSI2 Demux Controller driver. CSI2DC is a helper chip
+ that converts IDI interface byte stream to a parallel pixel stream.
+ It supports various RAW formats as input.
+ Performs clock domain crossing between hardware blocks.
diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
index 2dba38994a70..8af7c5b534c3 100644
--- a/drivers/media/platform/atmel/Makefile
+++ b/drivers/media/platform/atmel/Makefile
@@ -3,3 +3,4 @@ atmel-isc-objs = atmel-sama5d2-isc.o atmel-isc-base.o
obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
+obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o
diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c
new file mode 100644
index 000000000000..97d07a80bbf7
--- /dev/null
+++ b/drivers/media/platform/atmel/microchip-csi2dc.c
@@ -0,0 +1,700 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Microchip CSI2 Demux Controller (CSI2DC) driver
+ *
+ * Copyright (C) 2018-2020 Microchip Technology, Inc.
+ *
+ * Author: Eugen Hristev <[email protected]>
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/videodev2.h>
+
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+#include <media/videobuf2-dma-contig.h>
+
+/* Global configuration register */
+#define CSI2DC_GCFG 0x0
+
+/* MIPI sensor pixel clock is free running */
+#define CSI2DC_GCFG_MIPIFRN BIT(0)
+/* Output waveform inter-line minimum delay */
+#define CSI2DC_GCFG_HLC(v) ((v) << 4)
+#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4)
+
+/* Global control register */
+#define CSI2DC_GCTLR 0x04
+#define CSI2DC_GCTLR_SWRST BIT(0)
+
+/* Global status register */
+#define CSI2DC_GS 0x08
+
+/* SSP interrupt status register */
+#define CSI2DC_SSPIS 0x28
+/* Pipe update register */
+#define CSI2DC_PU 0xC0
+/* Video pipe attributes update */
+#define CSI2DC_PU_VP BIT(0)
+
+/* Pipe update status register */
+#define CSI2DC_PUS 0xC4
+
+/* Video pipeline enable register */
+#define CSI2DC_VPE 0xF8
+#define CSI2DC_VPE_ENABLE BIT(0)
+
+/* Video pipeline configuration register */
+#define CSI2DC_VPCFG 0xFC
+/* Data type */
+#define CSI2DC_VPCFG_DT(v) ((v) << 0)
+#define CSI2DC_VPCFG_DT_MASK GENMASK(5, 0)
+/* Virtual channel identifier */
+#define CSI2DC_VPCFG_VC(v) ((v) << 6)
+#define CSI2DC_VPCFG_VC_MASK GENMASK(7, 6)
+/* Decompression enable */
+#define CSI2DC_VPCFG_DE BIT(8)
+/* Decoder mode */
+#define CSI2DC_VPCFG_DM(v) ((v) << 9)
+#define CSI2DC_VPCFG_DM_DECODER8TO12 0
+/* Decoder predictor 2 selection */
+#define CSI2DC_VPCFG_DP2 BIT(12)
+/* Recommended memory storage */
+#define CSI2DC_VPCFG_RMS BIT(13)
+/* Post adjustment */
+#define CSI2DC_VPCFG_PA BIT(14)
+
+/* Video pipeline column register */
+#define CSI2DC_VPCOL 0x100
+/* Column number */
+#define CSI2DC_VPCOL_COL(v) ((v) << 0)
+#define CSI2DC_VPCOL_COL_MASK GENMASK(15, 0)
+
+/* Video pipeline row register */
+#define CSI2DC_VPROW 0x104
+/* Row number */
+#define CSI2DC_VPROW_ROW(v) ((v) << 0)
+#define CSI2DC_VPROW_ROW_MASK GENMASK(15, 0)
+
+/* Version register */
+#define CSI2DC_VERSION 0x1FC
+
+/* register read/write helpers */
+#define csi2dc_readl(st, reg) readl_relaxed((st)->base + (reg))
+#define csi2dc_writel(st, reg, val) writel_relaxed((val), (st)->base + (reg))
+
+/* supported RAW data types */
+#define CSI2DC_DT_RAW6 0x28
+#define CSI2DC_DT_RAW7 0x29
+#define CSI2DC_DT_RAW8 0x2A
+#define CSI2DC_DT_RAW10 0x2B
+#define CSI2DC_DT_RAW12 0x2C
+#define CSI2DC_DT_RAW14 0x2D
+
+struct csi2dc_format {
+ u32 mbus_code;
+ u32 dt;
+};
+
+static struct csi2dc_format csi2dc_formats_list[] = {
+ {
+ .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
+ .dt = CSI2DC_DT_RAW10,
+ },
+};
+
+enum mipi_csi_pads {
+ CSI2DC_PAD_SINK = 0,
+ CSI2DC_PAD_SOURCE = 1,
+ CSI2DC_PADS_NUM = 2,
+};
+
+struct csi2dc_device {
+ void __iomem *base;
+ struct v4l2_subdev csi2dc_sd;
+ struct device *dev;
+ struct v4l2_device v4l2_dev;
+ struct clk *pclk;
+ struct clk *scck;
+
+ bool video_pipe;
+
+ u32 num_fmts;
+ struct csi2dc_format **formats;
+
+ struct csi2dc_format *cur_fmt;
+ struct csi2dc_format *try_fmt;
+
+ struct media_pad pads[CSI2DC_PADS_NUM];
+
+ bool clk_gated;
+ u32 inter_line_delay;
+ u32 vc;
+
+ struct v4l2_async_subdev *asd;
+ struct v4l2_async_notifier notifier;
+
+ struct v4l2_subdev *input_sd;
+ bool completed;
+};
+
+static void csi2dc_vp_update(struct csi2dc_device *csi2dc)
+{
+ u32 vp;
+
+ vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;
+ vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;
+ vp &= ~CSI2DC_VPCFG_DE;
+ vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);
+ vp &= ~CSI2DC_VPCFG_DP2;
+ vp &= ~CSI2DC_VPCFG_RMS;
+ vp |= CSI2DC_VPCFG_PA;
+
+ csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);
+ csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);
+ csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);
+}
+
+static inline struct csi2dc_device *
+csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)
+{
+ return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);
+}
+
+static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
+
+ if (code->index >= csi2dc->num_fmts)
+ return -EINVAL;
+
+ code->code = csi2dc->formats[code->index]->mbus_code;
+ return 0;
+}
+
+static int csi2dc_try_fmt(struct v4l2_subdev *csi2dc_sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *req_fmt)
+{
+ struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
+ struct csi2dc_format *fmt;
+
+ if (!csi2dc->completed) {
+ dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
+ return 0;
+ }
+
+ for (fmt = csi2dc->formats[0]; fmt; fmt++)
+ if (req_fmt->format.code == fmt->mbus_code)
+ csi2dc->try_fmt = fmt;
+
+ /* in case we could not find the desired format, default to something */
+ if (!csi2dc->try_fmt ||
+ req_fmt->format.code != csi2dc->try_fmt->mbus_code) {
+ csi2dc->try_fmt = csi2dc->formats[0];
+ req_fmt->format.code = csi2dc->formats[0]->mbus_code;
+ }
+
+ return v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
+}
+
+static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *req_fmt)
+{
+ struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
+ int ret;
+
+ if (!csi2dc->completed) {
+ dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
+ return 0;
+ }
+
+ csi2dc_try_fmt(csi2dc_sd, cfg, req_fmt);
+
+ if (csi2dc->try_fmt->mbus_code != req_fmt->format.code) {
+ dev_dbg(csi2dc->dev, "CSI2DC unsupported format 0x%x\n",
+ req_fmt->format.code);
+ return -EINVAL;
+ }
+
+ ret = v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
+ if (ret) {
+ dev_err(csi2dc->dev, "input subdev failed %d\n", ret);
+ return ret;
+ }
+
+ csi2dc->cur_fmt = csi2dc->try_fmt;
+ /* update video pipe */
+ csi2dc_vp_update(csi2dc);
+
+ dev_dbg(csi2dc->dev, "CSI2DC new format: 0x%x\n", req_fmt->format.code);
+ return 0;
+}
+
+static int csi2dc_formats_init(struct csi2dc_device *csi2dc)
+{
+ int i, j;
+ struct csi2dc_format *fmt = &csi2dc_formats_list[0];
+
+ csi2dc->num_fmts = 1;
+
+ csi2dc->formats = devm_kcalloc(csi2dc->dev, csi2dc->num_fmts,
+ sizeof(*csi2dc->formats), GFP_KERNEL);
+
+ for (i = 0, j = 0; i < ARRAY_SIZE(csi2dc_formats_list); i++, fmt++)
+ if (fmt->mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10)
+ csi2dc->formats[j++] = fmt;
+ return 0;
+}
+
+static int csi2dc_s_power(struct v4l2_subdev *csi2dc_sd, int on)
+{
+ struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
+ int ret = 0;
+
+ if (!csi2dc->completed) {
+ dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
+ return 0;
+ }
+
+ if (on)
+ ret = clk_prepare_enable(csi2dc->scck);
+ else
+ clk_disable_unprepare(csi2dc->scck);
+ if (ret)
+ dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret);
+
+ /* if powering up, deassert reset line */
+ if (on)
+ csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);
+
+ ret = v4l2_subdev_call(csi2dc->input_sd, core, s_power, on);
+
+ /* if powering down, assert reset line */
+ if (!on)
+ csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST);
+
+ return ret;
+}
+
+static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)
+{
+ struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
+
+ if (!csi2dc->completed) {
+ dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
+ return 0;
+ }
+
+ return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable);
+}
+
+static int csi2dc_g_frame_interval(struct v4l2_subdev *csi2dc_sd,
+ struct v4l2_subdev_frame_interval *interval)
+{
+ struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
+
+ if (!csi2dc->completed) {
+ dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
+ return 0;
+ }
+
+ return v4l2_subdev_call(csi2dc->input_sd, video, g_frame_interval,
+ interval);
+}
+
+static int csi2dc_s_frame_interval(struct v4l2_subdev *csi2dc_sd,
+ struct v4l2_subdev_frame_interval *interval)
+{
+ struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
+
+ if (!csi2dc->completed) {
+ dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
+ return 0;
+ }
+
+ return v4l2_subdev_call(csi2dc->input_sd, video, s_frame_interval,
+ interval);
+}
+
+static int csi2dc_enum_frame_size(struct v4l2_subdev *csi2dc_sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
+
+ if (!csi2dc->completed) {
+ dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
+ return 0;
+ }
+
+ return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_size, cfg,
+ fse);
+}
+
+static int csi2dc_enum_frame_interval(struct v4l2_subdev *csi2dc_sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_interval_enum *fie)
+{
+ struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
+
+ if (!csi2dc->completed) {
+ dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
+ return 0;
+ }
+
+ return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_interval, cfg,
+ fie);
+}
+
+static const struct v4l2_subdev_core_ops csi2dc_core_ops = {
+ .s_power = csi2dc_s_power,
+};
+
+static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {
+ .enum_mbus_code = csi2dc_enum_mbus_code,
+ .set_fmt = csi2dc_set_fmt,
+ .enum_frame_size = csi2dc_enum_frame_size,
+ .enum_frame_interval = csi2dc_enum_frame_interval,
+};
+
+static const struct v4l2_subdev_video_ops csi2dc_video_ops = {
+ .s_stream = csi2dc_s_stream,
+ .g_frame_interval = csi2dc_g_frame_interval,
+ .s_frame_interval = csi2dc_s_frame_interval,
+};
+
+static const struct v4l2_subdev_ops csi2dc_subdev_ops = {
+ .core = &csi2dc_core_ops,
+ .pad = &csi2dc_pad_ops,
+ .video = &csi2dc_video_ops,
+};
+
+static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *subdev,
+ struct v4l2_async_subdev *asd)
+{
+ struct csi2dc_device *csi2dc = container_of(notifier->v4l2_dev,
+ struct csi2dc_device, v4l2_dev);
+ csi2dc->input_sd = subdev;
+
+ return 0;
+}
+
+static int csi2dc_async_complete(struct v4l2_async_notifier *notifier)
+{
+ struct csi2dc_device *csi2dc =
+ container_of(notifier->v4l2_dev, struct csi2dc_device, v4l2_dev);
+ int ret;
+
+ ret = v4l2_device_register_subdev_nodes(&csi2dc->v4l2_dev);
+ if (ret < 0) {
+ v4l2_err(&csi2dc->v4l2_dev, "failed to register subdev nodes\n");
+ return ret;
+ }
+
+ csi2dc_writel(csi2dc, CSI2DC_GCFG,
+ (CSI2DC_GCFG_HLC(csi2dc->inter_line_delay) &
+ CSI2DC_GCFG_HLC_MASK) |
+ (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));
+
+ csi2dc_writel(csi2dc, CSI2DC_VPCOL,
+ CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK);
+ csi2dc_writel(csi2dc, CSI2DC_VPROW,
+ CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK);
+
+ csi2dc->completed = true;
+
+ return ret;
+}
+
+static const struct v4l2_async_notifier_operations csi2dc_async_ops = {
+ .bound = csi2dc_async_bound,
+ .complete = csi2dc_async_complete,
+};
+
+static void csi2dc_cleanup_notifier(struct csi2dc_device *csi2dc)
+{
+ v4l2_async_notifier_unregister(&csi2dc->notifier);
+ v4l2_async_notifier_cleanup(&csi2dc->notifier);
+}
+
+static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,
+ struct device_node *input_parent)
+{
+ int ret;
+
+ v4l2_async_notifier_init(&csi2dc->notifier);
+
+ csi2dc->asd = kzalloc(sizeof(*csi2dc->asd), GFP_KERNEL);
+ if (!csi2dc->asd)
+ return -ENOMEM;
+
+ csi2dc->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+ csi2dc->asd->match.fwnode = of_fwnode_handle(input_parent);
+
+ ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd);
+ if (ret) {
+ dev_err(csi2dc->dev, "failed to add async notifier.\n");
+ v4l2_async_notifier_cleanup(&csi2dc->notifier);
+ goto csi2dc_prepare_notifier_err;
+ }
+
+ csi2dc->notifier.ops = &csi2dc_async_ops;
+
+ ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev,
+ &csi2dc->notifier);
+
+ if (ret) {
+ dev_err(csi2dc->dev, "fail to register async notifier.\n");
+ goto csi2dc_prepare_notifier_err;
+ }
+
+csi2dc_prepare_notifier_err:
+ of_node_put(input_parent);
+
+ return ret;
+}
+
+static int csi2dc_of_parse(struct csi2dc_device *csi2dc,
+ struct device_node *of_node)
+{
+ struct device_node *input_node, *sink_node, *input_parent;
+ struct v4l2_fwnode_endpoint input_endpoint = { 0 }, sink_endpoint = { 0 };
+ int ret;
+
+ input_node = of_graph_get_next_endpoint(of_node, NULL);
+
+ if (!input_node) {
+ dev_err(csi2dc->dev, "missing port node at %s, input node is mandatory.\n",
+ of_node->full_name);
+ return -EINVAL;
+ }
+
+ ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node),
+ &input_endpoint);
+
+ if (ret) {
+ dev_err(csi2dc->dev, "endpoint not defined at %s\n",
+ of_node->full_name);
+ return ret;
+ }
+
+ input_parent = of_graph_get_remote_port_parent(input_node);
+ if (!input_parent) {
+ dev_err(csi2dc->dev, "could not get input node's parent node.\n");
+ return -EINVAL;
+ }
+
+ sink_node = of_graph_get_next_endpoint(of_node, input_node);
+
+ if (sink_node)
+ ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(sink_node),
+ &sink_endpoint);
+
+ if (!sink_node || ret) {
+ dev_info(csi2dc->dev, "missing sink node at %s, data pipe available only.\n",
+ of_node->full_name);
+ } else {
+ csi2dc->video_pipe = true;
+ csi2dc->vc = sink_endpoint.base.id;
+
+ dev_dbg(csi2dc->dev, "block %s %d.%d->%d.%d video pipe VC %d\n",
+ of_node->full_name, input_endpoint.base.port,
+ input_endpoint.base.id, sink_endpoint.base.port,
+ sink_endpoint.base.id, csi2dc->vc);
+ }
+
+ csi2dc->clk_gated = of_property_read_bool(of_node,
+ "microchip,clk-gated");
+
+ dev_dbg(csi2dc->dev, "%s clock\n",
+ csi2dc->clk_gated ? "Gated" : "Free running");
+
+ ret = of_property_read_u32(of_node, "microchip,inter-line-delay",
+ &csi2dc->inter_line_delay);
+
+ if (ret || csi2dc->inter_line_delay > 16 ||
+ csi2dc->inter_line_delay == 0) {
+ dev_dbg(csi2dc->dev, "assuming inter line delay = 16 clocks");
+ csi2dc->inter_line_delay = 16;
+ ret = 0;
+ }
+ /* hardware automatically adds 1 */
+ csi2dc->inter_line_delay--;
+
+ /* prepare async notifier for subdevice completion */
+
+ of_node_put(sink_node);
+ of_node_put(input_node);
+
+ return csi2dc_prepare_notifier(csi2dc, input_parent);
+}
+
+static int csi2dc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct csi2dc_device *csi2dc;
+ struct resource *res = NULL;
+ int ret = 0;
+
+ csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);
+ if (!csi2dc)
+ return -ENOMEM;
+
+ csi2dc->dev = dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+
+ csi2dc->base = devm_ioremap_resource(dev, res);
+
+ if (IS_ERR(csi2dc->base)) {
+ dev_err(dev, "base address not set\n");
+ return PTR_ERR(csi2dc->base);
+ }
+
+ csi2dc->pclk = devm_clk_get(dev, "pclk");
+ if (IS_ERR(csi2dc->pclk)) {
+ ret = PTR_ERR(csi2dc->pclk);
+ dev_err(dev, "failed to get pclk: %d\n", ret);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(csi2dc->pclk);
+ if (ret) {
+ dev_err(dev, "failed to enable pclk: %d\n", ret);
+ return ret;
+ }
+
+ csi2dc->scck = devm_clk_get(dev, "scck");
+ if (IS_ERR(csi2dc->scck)) {
+ ret = PTR_ERR(csi2dc->scck);
+ dev_err(dev, "failed to get scck: %d\n", ret);
+ goto csi2dc_clk_fail;
+ }
+
+ ret = v4l2_device_register(dev, &csi2dc->v4l2_dev);
+ if (ret) {
+ dev_err(dev, "unable to register v4l2 device.\n");
+ goto csi2dc_clk_fail;
+ }
+
+ v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);
+
+ csi2dc->csi2dc_sd.owner = THIS_MODULE;
+ csi2dc->csi2dc_sd.dev = dev;
+ snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),
+ "CSI2DC.0");
+
+ csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+ csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+ csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+
+ ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, CSI2DC_PADS_NUM,
+ csi2dc->pads);
+ if (ret < 0) {
+ dev_err(dev, "media entity init failed\n");
+ goto csi2dc_probe_entity_err;
+ }
+
+ v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev);
+
+ platform_set_drvdata(pdev, &csi2dc->csi2dc_sd);
+
+ ret = csi2dc_of_parse(csi2dc, dev->of_node);
+ if (ret)
+ goto csi2dc_probe_entity_err;
+
+ ret = csi2dc_formats_init(csi2dc);
+ if (ret)
+ goto csi2dc_probe_error;
+
+ ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);
+ if (ret)
+ goto csi2dc_probe_error;
+
+ pr_info("Microchip CSI2DC version %x\n",
+ csi2dc_readl(csi2dc, CSI2DC_VERSION));
+
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+ pm_request_idle(dev);
+
+ return 0;
+
+csi2dc_probe_error:
+ v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);
+ csi2dc_cleanup_notifier(csi2dc);
+
+csi2dc_probe_entity_err:
+ media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
+ v4l2_device_unregister(&csi2dc->v4l2_dev);
+csi2dc_clk_fail:
+ clk_disable_unprepare(csi2dc->pclk);
+ return ret;
+}
+
+static int csi2dc_remove(struct platform_device *pdev)
+{
+ struct v4l2_subdev *csi2dc_sd = platform_get_drvdata(pdev);
+ struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
+
+ pm_runtime_disable(&pdev->dev);
+ csi2dc_cleanup_notifier(csi2dc);
+ media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
+ v4l2_device_unregister(&csi2dc->v4l2_dev);
+ clk_disable_unprepare(csi2dc->pclk);
+
+ return 0;
+}
+
+static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)
+{
+ return 0;
+}
+
+static int __maybe_unused csi2dc_runtime_resume(struct device *dev)
+{
+ return 0;
+}
+
+static const struct dev_pm_ops csi2dc_dev_pm_ops = {
+ SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)
+};
+
+static const struct of_device_id csi2dc_of_match[] = {
+ { .compatible = "microchip,sama7g5-csi2dc" },
+ { }
+};
+
+MODULE_DEVICE_TABLE(of, csi2dc_of_match);
+
+static struct platform_driver csi2dc_driver = {
+ .probe = csi2dc_probe,
+ .remove = csi2dc_remove,
+ .driver = {
+ .name = "microchip-csi2dc",
+ .pm = &csi2dc_dev_pm_ops,
+ .of_match_table = of_match_ptr(csi2dc_of_match),
+ },
+};
+
+module_platform_driver(csi2dc_driver);
+
+MODULE_AUTHOR("Eugen Hristev <[email protected]>");
+MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver");
+MODULE_LICENSE("GPL v2");
+MODULE_SUPPORTED_DEVICE("video");
--
2.25.1
Add Microchip CSI2DC driver in the list.
Signed-off-by: Eugen Hristev <[email protected]>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index deaafb617361..a07d3f5597a6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11375,6 +11375,13 @@ L: [email protected] (moderated for non-subscribers)
S: Supported
F: sound/soc/atmel
+MICROCHIP CSI2DC DRIVER
+M: Eugen Hristev <[email protected]>
+L: [email protected]
+S: Supported
+F: Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
+F: drivers/media/platform/atmel/microchip-csi2dc.c
+
MICROCHIP ECC DRIVER
M: Tudor Ambarus <[email protected]>
L: [email protected]
--
2.25.1
On Wed, 26 Aug 2020 09:51:40 +0300, Eugen Hristev wrote:
> Add bindings documentation for Microchip CSI2 Demultiplexer controller.
>
> CSI2DC is a demultiplexer from Synopsys IDI interface specification to
> parallel interface connection or direct memory access.
>
> Signed-off-by: Eugen Hristev <[email protected]>
> ---
> Changes in v3:
> - Removed some text from description, as it was explained in the schema
> - fixed other things as per Rob's review
> - moved some text inside the schema, like the clock description
>
> Changes in v2:
> - fixed warnings reported by dt_binding_check
>
> .../bindings/media/microchip,csi2dc.yaml | 174 ++++++++++++++++++
> 1 file changed, 174 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
>
Reviewed-by: Rob Herring <[email protected]>
Hi Eugen,
Thanks for the update.
On Wed, Aug 26, 2020 at 09:51:41AM +0300, Eugen Hristev wrote:
> Microchip CSI2DC (CSI2 Demultiplexer Controller) is a misc bridge device
> that converts a byte stream in IDI Synopsys format (coming from a CSI2HOST)
> to a pixel stream that can be captured by a sensor controller.
>
> Signed-off-by: Eugen Hristev <[email protected]>
> ---
> Changes in v2:
> - moved driver to platform/atmel
> - fixed minor things as per Sakari's review
> - still some things from v2 review are not yet addressed, to be followed up
>
> drivers/media/platform/atmel/Kconfig | 13 +
> drivers/media/platform/atmel/Makefile | 1 +
> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++
> 3 files changed, 714 insertions(+)
> create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.c
>
> diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig
> index 1850fe7f9360..80bbddcc2504 100644
> --- a/drivers/media/platform/atmel/Kconfig
> +++ b/drivers/media/platform/atmel/Kconfig
> @@ -21,3 +21,16 @@ config VIDEO_ATMEL_ISI
> help
> This module makes the ATMEL Image Sensor Interface available
> as a v4l2 device.
> +
> +config VIDEO_MICROCHIP_CSI2DC
> + tristate "Microchip CSI2 Demux Controller"
> + depends on VIDEO_V4L2 && COMMON_CLK && OF
> + depends on ARCH_AT91 || COMPILE_TEST
> + select MEDIA_CONTROLLER
> + select VIDEO_V4L2_SUBDEV_API
> + select V4L2_FWNODE
> + help
> + CSI2 Demux Controller driver. CSI2DC is a helper chip
> + that converts IDI interface byte stream to a parallel pixel stream.
> + It supports various RAW formats as input.
> + Performs clock domain crossing between hardware blocks.
> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
> index 2dba38994a70..8af7c5b534c3 100644
> --- a/drivers/media/platform/atmel/Makefile
> +++ b/drivers/media/platform/atmel/Makefile
> @@ -3,3 +3,4 @@ atmel-isc-objs = atmel-sama5d2-isc.o atmel-isc-base.o
>
> obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o
> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c
> new file mode 100644
> index 000000000000..97d07a80bbf7
> --- /dev/null
> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c
> @@ -0,0 +1,700 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Microchip CSI2 Demux Controller (CSI2DC) driver
> + *
> + * Copyright (C) 2018-2020 Microchip Technology, Inc.
> + *
> + * Author: Eugen Hristev <[email protected]>
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/videobuf2-dma-contig.h>
> +
> +/* Global configuration register */
> +#define CSI2DC_GCFG 0x0
> +
> +/* MIPI sensor pixel clock is free running */
> +#define CSI2DC_GCFG_MIPIFRN BIT(0)
> +/* Output waveform inter-line minimum delay */
> +#define CSI2DC_GCFG_HLC(v) ((v) << 4)
> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4)
> +
> +/* Global control register */
> +#define CSI2DC_GCTLR 0x04
> +#define CSI2DC_GCTLR_SWRST BIT(0)
> +
> +/* Global status register */
> +#define CSI2DC_GS 0x08
> +
> +/* SSP interrupt status register */
> +#define CSI2DC_SSPIS 0x28
> +/* Pipe update register */
> +#define CSI2DC_PU 0xC0
> +/* Video pipe attributes update */
> +#define CSI2DC_PU_VP BIT(0)
> +
> +/* Pipe update status register */
> +#define CSI2DC_PUS 0xC4
> +
> +/* Video pipeline enable register */
> +#define CSI2DC_VPE 0xF8
> +#define CSI2DC_VPE_ENABLE BIT(0)
> +
> +/* Video pipeline configuration register */
> +#define CSI2DC_VPCFG 0xFC
> +/* Data type */
> +#define CSI2DC_VPCFG_DT(v) ((v) << 0)
> +#define CSI2DC_VPCFG_DT_MASK GENMASK(5, 0)
> +/* Virtual channel identifier */
> +#define CSI2DC_VPCFG_VC(v) ((v) << 6)
> +#define CSI2DC_VPCFG_VC_MASK GENMASK(7, 6)
> +/* Decompression enable */
> +#define CSI2DC_VPCFG_DE BIT(8)
> +/* Decoder mode */
> +#define CSI2DC_VPCFG_DM(v) ((v) << 9)
> +#define CSI2DC_VPCFG_DM_DECODER8TO12 0
> +/* Decoder predictor 2 selection */
> +#define CSI2DC_VPCFG_DP2 BIT(12)
> +/* Recommended memory storage */
> +#define CSI2DC_VPCFG_RMS BIT(13)
> +/* Post adjustment */
> +#define CSI2DC_VPCFG_PA BIT(14)
> +
> +/* Video pipeline column register */
> +#define CSI2DC_VPCOL 0x100
> +/* Column number */
> +#define CSI2DC_VPCOL_COL(v) ((v) << 0)
> +#define CSI2DC_VPCOL_COL_MASK GENMASK(15, 0)
> +
> +/* Video pipeline row register */
> +#define CSI2DC_VPROW 0x104
> +/* Row number */
> +#define CSI2DC_VPROW_ROW(v) ((v) << 0)
> +#define CSI2DC_VPROW_ROW_MASK GENMASK(15, 0)
> +
> +/* Version register */
> +#define CSI2DC_VERSION 0x1FC
> +
> +/* register read/write helpers */
> +#define csi2dc_readl(st, reg) readl_relaxed((st)->base + (reg))
> +#define csi2dc_writel(st, reg, val) writel_relaxed((val), (st)->base + (reg))
> +
> +/* supported RAW data types */
> +#define CSI2DC_DT_RAW6 0x28
> +#define CSI2DC_DT_RAW7 0x29
> +#define CSI2DC_DT_RAW8 0x2A
> +#define CSI2DC_DT_RAW10 0x2B
> +#define CSI2DC_DT_RAW12 0x2C
> +#define CSI2DC_DT_RAW14 0x2D
> +
> +struct csi2dc_format {
> + u32 mbus_code;
> + u32 dt;
> +};
> +
> +static struct csi2dc_format csi2dc_formats_list[] = {
const
> + {
> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> + .dt = CSI2DC_DT_RAW10,
> + },
> +};
> +
> +enum mipi_csi_pads {
> + CSI2DC_PAD_SINK = 0,
> + CSI2DC_PAD_SOURCE = 1,
> + CSI2DC_PADS_NUM = 2,
> +};
> +
> +struct csi2dc_device {
> + void __iomem *base;
> + struct v4l2_subdev csi2dc_sd;
> + struct device *dev;
> + struct v4l2_device v4l2_dev;
This device is a bridge; the V4L2 device should be registered by the master
driver, i.e. the one handling DMA and registering the video nodes. Please
drop this.
> + struct clk *pclk;
> + struct clk *scck;
> +
> + bool video_pipe;
> +
> + u32 num_fmts;
> + struct csi2dc_format **formats;
> +
> + struct csi2dc_format *cur_fmt;
> + struct csi2dc_format *try_fmt;
> +
> + struct media_pad pads[CSI2DC_PADS_NUM];
> +
> + bool clk_gated;
> + u32 inter_line_delay;
> + u32 vc;
> +
> + struct v4l2_async_subdev *asd;
> + struct v4l2_async_notifier notifier;
> +
> + struct v4l2_subdev *input_sd;
> + bool completed;
> +};
> +
> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc)
> +{
> + u32 vp;
> +
> + vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;
> + vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;
> + vp &= ~CSI2DC_VPCFG_DE;
> + vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);
> + vp &= ~CSI2DC_VPCFG_DP2;
> + vp &= ~CSI2DC_VPCFG_RMS;
> + vp |= CSI2DC_VPCFG_PA;
> +
> + csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);
> + csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);
> + csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);
> +}
> +
> +static inline struct csi2dc_device *
> +csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)
> +{
> + return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);
> +}
> +
> +static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> +
> + if (code->index >= csi2dc->num_fmts)
> + return -EINVAL;
> +
> + code->code = csi2dc->formats[code->index]->mbus_code;
> + return 0;
> +}
> +
> +static int csi2dc_try_fmt(struct v4l2_subdev *csi2dc_sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *req_fmt)
> +{
> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> + struct csi2dc_format *fmt;
> +
> + if (!csi2dc->completed) {
> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> + return 0;
> + }
> +
> + for (fmt = csi2dc->formats[0]; fmt; fmt++)
> + if (req_fmt->format.code == fmt->mbus_code)
> + csi2dc->try_fmt = fmt;
> +
> + /* in case we could not find the desired format, default to something */
> + if (!csi2dc->try_fmt ||
> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) {
> + csi2dc->try_fmt = csi2dc->formats[0];
> + req_fmt->format.code = csi2dc->formats[0]->mbus_code;
> + }
> +
> + return v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
> +}
> +
> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *req_fmt)
> +{
> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> + int ret;
> +
> + if (!csi2dc->completed) {
> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> + return 0;
> + }
> +
> + csi2dc_try_fmt(csi2dc_sd, cfg, req_fmt);
> +
> + if (csi2dc->try_fmt->mbus_code != req_fmt->format.code) {
> + dev_dbg(csi2dc->dev, "CSI2DC unsupported format 0x%x\n",
> + req_fmt->format.code);
Please adjust the format into a supported one instead.
> + return -EINVAL;
> + }
> +
> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
> + if (ret) {
> + dev_err(csi2dc->dev, "input subdev failed %d\n", ret);
> + return ret;
> + }
> +
> + csi2dc->cur_fmt = csi2dc->try_fmt;
> + /* update video pipe */
> + csi2dc_vp_update(csi2dc);
> +
> + dev_dbg(csi2dc->dev, "CSI2DC new format: 0x%x\n", req_fmt->format.code);
> + return 0;
> +}
> +
> +static int csi2dc_formats_init(struct csi2dc_device *csi2dc)
> +{
> + int i, j;
> + struct csi2dc_format *fmt = &csi2dc_formats_list[0];
> +
> + csi2dc->num_fmts = 1;
> +
> + csi2dc->formats = devm_kcalloc(csi2dc->dev, csi2dc->num_fmts,
> + sizeof(*csi2dc->formats), GFP_KERNEL);
> +
> + for (i = 0, j = 0; i < ARRAY_SIZE(csi2dc_formats_list); i++, fmt++)
> + if (fmt->mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10)
> + csi2dc->formats[j++] = fmt;
> + return 0;
> +}
> +
> +static int csi2dc_s_power(struct v4l2_subdev *csi2dc_sd, int on)
> +{
> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> + int ret = 0;
> +
> + if (!csi2dc->completed) {
> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> + return 0;
> + }
> +
> + if (on)
> + ret = clk_prepare_enable(csi2dc->scck);
> + else
> + clk_disable_unprepare(csi2dc->scck);
> + if (ret)
> + dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret);
> +
> + /* if powering up, deassert reset line */
> + if (on)
> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);
> +
> + ret = v4l2_subdev_call(csi2dc->input_sd, core, s_power, on);
> +
> + /* if powering down, assert reset line */
> + if (!on)
> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST);
These go to the runtime resume and suspend callbacks.
> +
> + return ret;
> +}
> +
> +static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)
> +{
> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> +
> + if (!csi2dc->completed) {
> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> + return 0;
> + }
> +
> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable);
> +}
> +
> +static int csi2dc_g_frame_interval(struct v4l2_subdev *csi2dc_sd,
> + struct v4l2_subdev_frame_interval *interval)
> +{
> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> +
> + if (!csi2dc->completed) {
> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> + return 0;
> + }
> +
> + return v4l2_subdev_call(csi2dc->input_sd, video, g_frame_interval,
> + interval);
> +}
> +
> +static int csi2dc_s_frame_interval(struct v4l2_subdev *csi2dc_sd,
> + struct v4l2_subdev_frame_interval *interval)
> +{
> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> +
> + if (!csi2dc->completed) {
> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> + return 0;
> + }
> +
> + return v4l2_subdev_call(csi2dc->input_sd, video, s_frame_interval,
> + interval);
If your driver is used with a MC-enabled master driver, you can drop all
the v4l2_subdev_call() macros apart from the ones related to s_stream
operation.
If not, then you can drop the HAS_DEVNODE flag from the subdev flags. But
I'd try to avoid this route if possible.
> +}
> +
> +static int csi2dc_enum_frame_size(struct v4l2_subdev *csi2dc_sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> +
> + if (!csi2dc->completed) {
> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> + return 0;
> + }
> +
> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_size, cfg,
> + fse);
> +}
> +
> +static int csi2dc_enum_frame_interval(struct v4l2_subdev *csi2dc_sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_interval_enum *fie)
> +{
> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> +
> + if (!csi2dc->completed) {
> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> + return 0;
> + }
> +
> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_interval, cfg,
> + fie);
> +}
> +
> +static const struct v4l2_subdev_core_ops csi2dc_core_ops = {
> + .s_power = csi2dc_s_power,
Please use runtime PM instead.
> +};
> +
> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {
> + .enum_mbus_code = csi2dc_enum_mbus_code,
> + .set_fmt = csi2dc_set_fmt,
> + .enum_frame_size = csi2dc_enum_frame_size,
> + .enum_frame_interval = csi2dc_enum_frame_interval,
> +};
> +
> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = {
> + .s_stream = csi2dc_s_stream,
> + .g_frame_interval = csi2dc_g_frame_interval,
> + .s_frame_interval = csi2dc_s_frame_interval,
> +};
> +
> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = {
> + .core = &csi2dc_core_ops,
> + .pad = &csi2dc_pad_ops,
> + .video = &csi2dc_video_ops,
> +};
> +
> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct csi2dc_device *csi2dc = container_of(notifier->v4l2_dev,
> + struct csi2dc_device, v4l2_dev);
> + csi2dc->input_sd = subdev;
> +
> + return 0;
> +}
> +
> +static int csi2dc_async_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct csi2dc_device *csi2dc =
> + container_of(notifier->v4l2_dev, struct csi2dc_device, v4l2_dev);
Over 80 characters per line. Please check the rest of the driver, too.
> + int ret;
> +
> + ret = v4l2_device_register_subdev_nodes(&csi2dc->v4l2_dev);
> + if (ret < 0) {
> + v4l2_err(&csi2dc->v4l2_dev, "failed to register subdev nodes\n");
> + return ret;
> + }
> +
> + csi2dc_writel(csi2dc, CSI2DC_GCFG,
> + (CSI2DC_GCFG_HLC(csi2dc->inter_line_delay) &
> + CSI2DC_GCFG_HLC_MASK) |
> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));
> +
> + csi2dc_writel(csi2dc, CSI2DC_VPCOL,
> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK);
> + csi2dc_writel(csi2dc, CSI2DC_VPROW,
> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK);
> +
> + csi2dc->completed = true;
> +
> + return ret;
> +}
> +
> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = {
> + .bound = csi2dc_async_bound,
> + .complete = csi2dc_async_complete,
> +};
> +
> +static void csi2dc_cleanup_notifier(struct csi2dc_device *csi2dc)
> +{
> + v4l2_async_notifier_unregister(&csi2dc->notifier);
> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
> +}
> +
> +static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,
> + struct device_node *input_parent)
> +{
> + int ret;
> +
> + v4l2_async_notifier_init(&csi2dc->notifier);
> +
> + csi2dc->asd = kzalloc(sizeof(*csi2dc->asd), GFP_KERNEL);
> + if (!csi2dc->asd)
> + return -ENOMEM;
> +
> + csi2dc->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> + csi2dc->asd->match.fwnode = of_fwnode_handle(input_parent);
> +
> + ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd);
> + if (ret) {
> + dev_err(csi2dc->dev, "failed to add async notifier.\n");
> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
> + goto csi2dc_prepare_notifier_err;
> + }
> +
> + csi2dc->notifier.ops = &csi2dc_async_ops;
> +
> + ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev,
> + &csi2dc->notifier);
> +
> + if (ret) {
> + dev_err(csi2dc->dev, "fail to register async notifier.\n");
> + goto csi2dc_prepare_notifier_err;
> + }
> +
> +csi2dc_prepare_notifier_err:
> + of_node_put(input_parent);
> +
> + return ret;
> +}
> +
> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc,
> + struct device_node *of_node)
> +{
> + struct device_node *input_node, *sink_node, *input_parent;
> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, sink_endpoint = { 0 };
> + int ret;
> +
> + input_node = of_graph_get_next_endpoint(of_node, NULL);
> +
> + if (!input_node) {
> + dev_err(csi2dc->dev, "missing port node at %s, input node is mandatory.\n",
> + of_node->full_name);
> + return -EINVAL;
> + }
> +
> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node),
> + &input_endpoint);
> +
> + if (ret) {
> + dev_err(csi2dc->dev, "endpoint not defined at %s\n",
> + of_node->full_name);
> + return ret;
> + }
> +
> + input_parent = of_graph_get_remote_port_parent(input_node);
> + if (!input_parent) {
> + dev_err(csi2dc->dev, "could not get input node's parent node.\n");
> + return -EINVAL;
> + }
> +
> + sink_node = of_graph_get_next_endpoint(of_node, input_node);
> +
> + if (sink_node)
> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(sink_node),
> + &sink_endpoint);
> +
> + if (!sink_node || ret) {
> + dev_info(csi2dc->dev, "missing sink node at %s, data pipe available only.\n",
> + of_node->full_name);
> + } else {
> + csi2dc->video_pipe = true;
> + csi2dc->vc = sink_endpoint.base.id;
> +
> + dev_dbg(csi2dc->dev, "block %s %d.%d->%d.%d video pipe VC %d\n",
> + of_node->full_name, input_endpoint.base.port,
> + input_endpoint.base.id, sink_endpoint.base.port,
> + sink_endpoint.base.id, csi2dc->vc);
> + }
> +
> + csi2dc->clk_gated = of_property_read_bool(of_node,
> + "microchip,clk-gated");
> +
> + dev_dbg(csi2dc->dev, "%s clock\n",
> + csi2dc->clk_gated ? "Gated" : "Free running");
> +
> + ret = of_property_read_u32(of_node, "microchip,inter-line-delay",
> + &csi2dc->inter_line_delay);
> +
> + if (ret || csi2dc->inter_line_delay > 16 ||
> + csi2dc->inter_line_delay == 0) {
> + dev_dbg(csi2dc->dev, "assuming inter line delay = 16 clocks");
> + csi2dc->inter_line_delay = 16;
> + ret = 0;
> + }
> + /* hardware automatically adds 1 */
> + csi2dc->inter_line_delay--;
> +
> + /* prepare async notifier for subdevice completion */
> +
> + of_node_put(sink_node);
> + of_node_put(input_node);
> +
> + return csi2dc_prepare_notifier(csi2dc, input_parent);
> +}
> +
> +static int csi2dc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct csi2dc_device *csi2dc;
> + struct resource *res = NULL;
> + int ret = 0;
> +
> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);
> + if (!csi2dc)
> + return -ENOMEM;
> +
> + csi2dc->dev = dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + csi2dc->base = devm_ioremap_resource(dev, res);
> +
> + if (IS_ERR(csi2dc->base)) {
> + dev_err(dev, "base address not set\n");
> + return PTR_ERR(csi2dc->base);
> + }
> +
> + csi2dc->pclk = devm_clk_get(dev, "pclk");
> + if (IS_ERR(csi2dc->pclk)) {
> + ret = PTR_ERR(csi2dc->pclk);
> + dev_err(dev, "failed to get pclk: %d\n", ret);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(csi2dc->pclk);
> + if (ret) {
> + dev_err(dev, "failed to enable pclk: %d\n", ret);
> + return ret;
> + }
> +
> + csi2dc->scck = devm_clk_get(dev, "scck");
> + if (IS_ERR(csi2dc->scck)) {
> + ret = PTR_ERR(csi2dc->scck);
> + dev_err(dev, "failed to get scck: %d\n", ret);
> + goto csi2dc_clk_fail;
> + }
> +
> + ret = v4l2_device_register(dev, &csi2dc->v4l2_dev);
> + if (ret) {
> + dev_err(dev, "unable to register v4l2 device.\n");
> + goto csi2dc_clk_fail;
> + }
> +
> + v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);
> +
> + csi2dc->csi2dc_sd.owner = THIS_MODULE;
> + csi2dc->csi2dc_sd.dev = dev;
> + snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),
> + "CSI2DC.0");
> +
> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +
> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, CSI2DC_PADS_NUM,
> + csi2dc->pads);
> + if (ret < 0) {
> + dev_err(dev, "media entity init failed\n");
> + goto csi2dc_probe_entity_err;
> + }
> +
> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev);
> +
> + platform_set_drvdata(pdev, &csi2dc->csi2dc_sd);
> +
> + ret = csi2dc_of_parse(csi2dc, dev->of_node);
> + if (ret)
> + goto csi2dc_probe_entity_err;
> +
> + ret = csi2dc_formats_init(csi2dc);
> + if (ret)
> + goto csi2dc_probe_error;
> +
> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);
> + if (ret)
> + goto csi2dc_probe_error;
> +
> + pr_info("Microchip CSI2DC version %x\n",
> + csi2dc_readl(csi2dc, CSI2DC_VERSION));
dev_info(), please. Or just dev_dbg().
> +
> + pm_runtime_set_active(dev);
> + pm_runtime_enable(dev);
> + pm_request_idle(dev);
> +
> + return 0;
> +
> +csi2dc_probe_error:
> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);
> + csi2dc_cleanup_notifier(csi2dc);
> +
> +csi2dc_probe_entity_err:
> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
> + v4l2_device_unregister(&csi2dc->v4l2_dev);
> +csi2dc_clk_fail:
> + clk_disable_unprepare(csi2dc->pclk);
> + return ret;
> +}
> +
> +static int csi2dc_remove(struct platform_device *pdev)
> +{
> + struct v4l2_subdev *csi2dc_sd = platform_get_drvdata(pdev);
> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> +
> + pm_runtime_disable(&pdev->dev);
> + csi2dc_cleanup_notifier(csi2dc);
> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
> + v4l2_device_unregister(&csi2dc->v4l2_dev);
> + clk_disable_unprepare(csi2dc->pclk);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {
> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)
> +};
> +
> +static const struct of_device_id csi2dc_of_match[] = {
> + { .compatible = "microchip,sama7g5-csi2dc" },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(of, csi2dc_of_match);
> +
> +static struct platform_driver csi2dc_driver = {
> + .probe = csi2dc_probe,
> + .remove = csi2dc_remove,
> + .driver = {
> + .name = "microchip-csi2dc",
> + .pm = &csi2dc_dev_pm_ops,
> + .of_match_table = of_match_ptr(csi2dc_of_match),
> + },
> +};
> +
> +module_platform_driver(csi2dc_driver);
> +
> +MODULE_AUTHOR("Eugen Hristev <[email protected]>");
> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_SUPPORTED_DEVICE("video");
Hmm. This line needs to be more specific. I'd just drop it.
--
Kind regards,
Sakari Ailus
On 31.08.2020 11:50, Sakari Ailus wrote:
> Hi Eugen,
>
> Thanks for the update.
Hi Sakari,
Thanks for reviewing, but, could you please help me understand your
review below ?
>
> On Wed, Aug 26, 2020 at 09:51:41AM +0300, Eugen Hristev wrote:
>> Microchip CSI2DC (CSI2 Demultiplexer Controller) is a misc bridge device
>> that converts a byte stream in IDI Synopsys format (coming from a CSI2HOST)
>> to a pixel stream that can be captured by a sensor controller.
>>
>> Signed-off-by: Eugen Hristev <[email protected]>
>> ---
>> Changes in v2:
>> - moved driver to platform/atmel
>> - fixed minor things as per Sakari's review
>> - still some things from v2 review are not yet addressed, to be followed up
>>
>> drivers/media/platform/atmel/Kconfig | 13 +
>> drivers/media/platform/atmel/Makefile | 1 +
>> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++
>> 3 files changed, 714 insertions(+)
>> create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.c
>>
>> diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig
>> index 1850fe7f9360..80bbddcc2504 100644
>> --- a/drivers/media/platform/atmel/Kconfig
>> +++ b/drivers/media/platform/atmel/Kconfig
>> @@ -21,3 +21,16 @@ config VIDEO_ATMEL_ISI
>> help
>> This module makes the ATMEL Image Sensor Interface available
>> as a v4l2 device.
>> +
>> +config VIDEO_MICROCHIP_CSI2DC
>> + tristate "Microchip CSI2 Demux Controller"
>> + depends on VIDEO_V4L2 && COMMON_CLK && OF
>> + depends on ARCH_AT91 || COMPILE_TEST
>> + select MEDIA_CONTROLLER
>> + select VIDEO_V4L2_SUBDEV_API
>> + select V4L2_FWNODE
>> + help
>> + CSI2 Demux Controller driver. CSI2DC is a helper chip
>> + that converts IDI interface byte stream to a parallel pixel stream.
>> + It supports various RAW formats as input.
>> + Performs clock domain crossing between hardware blocks.
>> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
>> index 2dba38994a70..8af7c5b534c3 100644
>> --- a/drivers/media/platform/atmel/Makefile
>> +++ b/drivers/media/platform/atmel/Makefile
>> @@ -3,3 +3,4 @@ atmel-isc-objs = atmel-sama5d2-isc.o atmel-isc-base.o
>>
>> obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
>> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o
>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c
>> new file mode 100644
>> index 000000000000..97d07a80bbf7
>> --- /dev/null
>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c
>> @@ -0,0 +1,700 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Microchip CSI2 Demux Controller (CSI2DC) driver
>> + *
>> + * Copyright (C) 2018-2020 Microchip Technology, Inc.
>> + *
>> + * Author: Eugen Hristev <[email protected]>
>> + *
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/videodev2.h>
>> +
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-fwnode.h>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/videobuf2-dma-contig.h>
>> +
>> +/* Global configuration register */
>> +#define CSI2DC_GCFG 0x0
>> +
>> +/* MIPI sensor pixel clock is free running */
>> +#define CSI2DC_GCFG_MIPIFRN BIT(0)
>> +/* Output waveform inter-line minimum delay */
>> +#define CSI2DC_GCFG_HLC(v) ((v) << 4)
>> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4)
>> +
>> +/* Global control register */
>> +#define CSI2DC_GCTLR 0x04
>> +#define CSI2DC_GCTLR_SWRST BIT(0)
>> +
>> +/* Global status register */
>> +#define CSI2DC_GS 0x08
>> +
>> +/* SSP interrupt status register */
>> +#define CSI2DC_SSPIS 0x28
>> +/* Pipe update register */
>> +#define CSI2DC_PU 0xC0
>> +/* Video pipe attributes update */
>> +#define CSI2DC_PU_VP BIT(0)
>> +
>> +/* Pipe update status register */
>> +#define CSI2DC_PUS 0xC4
>> +
>> +/* Video pipeline enable register */
>> +#define CSI2DC_VPE 0xF8
>> +#define CSI2DC_VPE_ENABLE BIT(0)
>> +
>> +/* Video pipeline configuration register */
>> +#define CSI2DC_VPCFG 0xFC
>> +/* Data type */
>> +#define CSI2DC_VPCFG_DT(v) ((v) << 0)
>> +#define CSI2DC_VPCFG_DT_MASK GENMASK(5, 0)
>> +/* Virtual channel identifier */
>> +#define CSI2DC_VPCFG_VC(v) ((v) << 6)
>> +#define CSI2DC_VPCFG_VC_MASK GENMASK(7, 6)
>> +/* Decompression enable */
>> +#define CSI2DC_VPCFG_DE BIT(8)
>> +/* Decoder mode */
>> +#define CSI2DC_VPCFG_DM(v) ((v) << 9)
>> +#define CSI2DC_VPCFG_DM_DECODER8TO12 0
>> +/* Decoder predictor 2 selection */
>> +#define CSI2DC_VPCFG_DP2 BIT(12)
>> +/* Recommended memory storage */
>> +#define CSI2DC_VPCFG_RMS BIT(13)
>> +/* Post adjustment */
>> +#define CSI2DC_VPCFG_PA BIT(14)
>> +
>> +/* Video pipeline column register */
>> +#define CSI2DC_VPCOL 0x100
>> +/* Column number */
>> +#define CSI2DC_VPCOL_COL(v) ((v) << 0)
>> +#define CSI2DC_VPCOL_COL_MASK GENMASK(15, 0)
>> +
>> +/* Video pipeline row register */
>> +#define CSI2DC_VPROW 0x104
>> +/* Row number */
>> +#define CSI2DC_VPROW_ROW(v) ((v) << 0)
>> +#define CSI2DC_VPROW_ROW_MASK GENMASK(15, 0)
>> +
>> +/* Version register */
>> +#define CSI2DC_VERSION 0x1FC
>> +
>> +/* register read/write helpers */
>> +#define csi2dc_readl(st, reg) readl_relaxed((st)->base + (reg))
>> +#define csi2dc_writel(st, reg, val) writel_relaxed((val), (st)->base + (reg))
>> +
>> +/* supported RAW data types */
>> +#define CSI2DC_DT_RAW6 0x28
>> +#define CSI2DC_DT_RAW7 0x29
>> +#define CSI2DC_DT_RAW8 0x2A
>> +#define CSI2DC_DT_RAW10 0x2B
>> +#define CSI2DC_DT_RAW12 0x2C
>> +#define CSI2DC_DT_RAW14 0x2D
>> +
>> +struct csi2dc_format {
>> + u32 mbus_code;
>> + u32 dt;
>> +};
>> +
>> +static struct csi2dc_format csi2dc_formats_list[] = {
>
> const
Elements from this array are being selected in a dynamic list, and
pointed from a non-const array of selected working formats (at subdevice
completion time). If I make this const , I will get compiler warnings
when assigning to the non-const array.
What solution do you suggest for this ?
>
>> + {
>> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
>> + .dt = CSI2DC_DT_RAW10,
>> + },
>> +};
>> +
>> +enum mipi_csi_pads {
>> + CSI2DC_PAD_SINK = 0,
>> + CSI2DC_PAD_SOURCE = 1,
>> + CSI2DC_PADS_NUM = 2,
>> +};
>> +
>> +struct csi2dc_device {
>> + void __iomem *base;
>> + struct v4l2_subdev csi2dc_sd;
>> + struct device *dev;
>> + struct v4l2_device v4l2_dev;
>
> This device is a bridge; the V4L2 device should be registered by the master
> driver, i.e. the one handling DMA and registering the video nodes. Please
> drop this.
>
>> + struct clk *pclk;
>> + struct clk *scck;
>> +
>> + bool video_pipe;
>> +
>> + u32 num_fmts;
>> + struct csi2dc_format **formats;
>> +
>> + struct csi2dc_format *cur_fmt;
>> + struct csi2dc_format *try_fmt;
>> +
>> + struct media_pad pads[CSI2DC_PADS_NUM];
>> +
>> + bool clk_gated;
>> + u32 inter_line_delay;
>> + u32 vc;
>> +
>> + struct v4l2_async_subdev *asd;
>> + struct v4l2_async_notifier notifier;
>> +
>> + struct v4l2_subdev *input_sd;
>> + bool completed;
>> +};
>> +
>> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc)
>> +{
>> + u32 vp;
>> +
>> + vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;
>> + vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;
>> + vp &= ~CSI2DC_VPCFG_DE;
>> + vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);
>> + vp &= ~CSI2DC_VPCFG_DP2;
>> + vp &= ~CSI2DC_VPCFG_RMS;
>> + vp |= CSI2DC_VPCFG_PA;
>> +
>> + csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);
>> + csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);
>> + csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);
>> +}
>> +
>> +static inline struct csi2dc_device *
>> +csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)
>> +{
>> + return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);
>> +}
>> +
>> +static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>> +
>> + if (code->index >= csi2dc->num_fmts)
>> + return -EINVAL;
>> +
>> + code->code = csi2dc->formats[code->index]->mbus_code;
>> + return 0;
>> +}
>> +
>> +static int csi2dc_try_fmt(struct v4l2_subdev *csi2dc_sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_format *req_fmt)
>> +{
>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>> + struct csi2dc_format *fmt;
>> +
>> + if (!csi2dc->completed) {
>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>> + return 0;
>> + }
>> +
>> + for (fmt = csi2dc->formats[0]; fmt; fmt++)
>> + if (req_fmt->format.code == fmt->mbus_code)
>> + csi2dc->try_fmt = fmt;
>> +
>> + /* in case we could not find the desired format, default to something */
>> + if (!csi2dc->try_fmt ||
>> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) {
>> + csi2dc->try_fmt = csi2dc->formats[0];
>> + req_fmt->format.code = csi2dc->formats[0]->mbus_code;
>> + }
>> +
>> + return v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
>> +}
>> +
>> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_format *req_fmt)
>> +{
>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>> + int ret;
>> +
>> + if (!csi2dc->completed) {
>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>> + return 0;
>> + }
>> +
>> + csi2dc_try_fmt(csi2dc_sd, cfg, req_fmt);
>> +
>> + if (csi2dc->try_fmt->mbus_code != req_fmt->format.code) {
>> + dev_dbg(csi2dc->dev, "CSI2DC unsupported format 0x%x\n",
>> + req_fmt->format.code);
>
> Please adjust the format into a supported one instead.
I thought that only the 'try_fmt' calls should adapt to something that
works. It is the same case for set_fmt ? This function cannot return
invalid format ?
>
>> + return -EINVAL;
>> + }
>> +
>> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
>> + if (ret) {
>> + dev_err(csi2dc->dev, "input subdev failed %d\n", ret);
>> + return ret;
>> + }
>> +
>> + csi2dc->cur_fmt = csi2dc->try_fmt;
>> + /* update video pipe */
>> + csi2dc_vp_update(csi2dc);
>> +
>> + dev_dbg(csi2dc->dev, "CSI2DC new format: 0x%x\n", req_fmt->format.code);
>> + return 0;
>> +}
>> +
>> +static int csi2dc_formats_init(struct csi2dc_device *csi2dc)
>> +{
>> + int i, j;
>> + struct csi2dc_format *fmt = &csi2dc_formats_list[0];
>> +
>> + csi2dc->num_fmts = 1;
>> +
>> + csi2dc->formats = devm_kcalloc(csi2dc->dev, csi2dc->num_fmts,
>> + sizeof(*csi2dc->formats), GFP_KERNEL);
>> +
>> + for (i = 0, j = 0; i < ARRAY_SIZE(csi2dc_formats_list); i++, fmt++)
>> + if (fmt->mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10)
>> + csi2dc->formats[j++] = fmt;
>> + return 0;
>> +}
>> +
>> +static int csi2dc_s_power(struct v4l2_subdev *csi2dc_sd, int on)
>> +{
>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>> + int ret = 0;
>> +
>> + if (!csi2dc->completed) {
>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>> + return 0;
>> + }
>> +
>> + if (on)
>> + ret = clk_prepare_enable(csi2dc->scck);
>> + else
>> + clk_disable_unprepare(csi2dc->scck);
>> + if (ret)
>> + dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret);
>> +
>> + /* if powering up, deassert reset line */
>> + if (on)
>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);
>> +
>> + ret = v4l2_subdev_call(csi2dc->input_sd, core, s_power, on);
>> +
>> + /* if powering down, assert reset line */
>> + if (!on)
>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST);
>
> These go to the runtime resume and suspend callbacks.
Okay I will update
>
>> +
>> + return ret;
>> +}
>> +
>> +static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)
>> +{
>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>> +
>> + if (!csi2dc->completed) {
>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>> + return 0;
>> + }
>> +
>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable);
>> +}
>> +
>> +static int csi2dc_g_frame_interval(struct v4l2_subdev *csi2dc_sd,
>> + struct v4l2_subdev_frame_interval *interval)
>> +{
>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>> +
>> + if (!csi2dc->completed) {
>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>> + return 0;
>> + }
>> +
>> + return v4l2_subdev_call(csi2dc->input_sd, video, g_frame_interval,
>> + interval);
>> +}
>> +
>> +static int csi2dc_s_frame_interval(struct v4l2_subdev *csi2dc_sd,
>> + struct v4l2_subdev_frame_interval *interval)
>> +{
>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>> +
>> + if (!csi2dc->completed) {
>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>> + return 0;
>> + }
>> +
>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_frame_interval,
>> + interval);
>
> If your driver is used with a MC-enabled master driver, you can drop all
> the v4l2_subdev_call() macros apart from the ones related to s_stream
> operation.
And the set frame interval for example, will it be sent to the sensor
automatically ?
I noticed that such operations on the master driver were not propagated
to the underlying subdevice. How does the master device 'know' how to
call the input_sd ? this input_sd is not referenced anywhere in the
master driver.
>
> If not, then you can drop the HAS_DEVNODE flag from the subdev flags. But
> I'd try to avoid this route if possible.
>
>> +}
>> +
>> +static int csi2dc_enum_frame_size(struct v4l2_subdev *csi2dc_sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_frame_size_enum *fse)
>> +{
>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>> +
>> + if (!csi2dc->completed) {
>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>> + return 0;
>> + }
>> +
>> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_size, cfg,
>> + fse);
>> +}
>> +
>> +static int csi2dc_enum_frame_interval(struct v4l2_subdev *csi2dc_sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_frame_interval_enum *fie)
>> +{
>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>> +
>> + if (!csi2dc->completed) {
>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>> + return 0;
>> + }
>> +
>> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_interval, cfg,
>> + fie);
>> +}
>> +
>> +static const struct v4l2_subdev_core_ops csi2dc_core_ops = {
>> + .s_power = csi2dc_s_power,
>
> Please use runtime PM instead.
>
>> +};
>> +
>> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {
>> + .enum_mbus_code = csi2dc_enum_mbus_code,
>> + .set_fmt = csi2dc_set_fmt,
>> + .enum_frame_size = csi2dc_enum_frame_size,
>> + .enum_frame_interval = csi2dc_enum_frame_interval,
>> +};
>> +
>> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = {
>> + .s_stream = csi2dc_s_stream,
>> + .g_frame_interval = csi2dc_g_frame_interval,
>> + .s_frame_interval = csi2dc_s_frame_interval,
>> +};
>> +
>> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = {
>> + .core = &csi2dc_core_ops,
>> + .pad = &csi2dc_pad_ops,
>> + .video = &csi2dc_video_ops,
>> +};
>> +
>> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,
>> + struct v4l2_subdev *subdev,
>> + struct v4l2_async_subdev *asd)
>> +{
>> + struct csi2dc_device *csi2dc = container_of(notifier->v4l2_dev,
>> + struct csi2dc_device, v4l2_dev);
>> + csi2dc->input_sd = subdev;
>> +
>> + return 0;
>> +}
>> +
>> +static int csi2dc_async_complete(struct v4l2_async_notifier *notifier)
>> +{
>> + struct csi2dc_device *csi2dc =
>> + container_of(notifier->v4l2_dev, struct csi2dc_device, v4l2_dev);
>
> Over 80 characters per line. Please check the rest of the driver, too.
>
>> + int ret;
>> +
>> + ret = v4l2_device_register_subdev_nodes(&csi2dc->v4l2_dev);
>> + if (ret < 0) {
>> + v4l2_err(&csi2dc->v4l2_dev, "failed to register subdev nodes\n");
>> + return ret;
>> + }
>> +
>> + csi2dc_writel(csi2dc, CSI2DC_GCFG,
>> + (CSI2DC_GCFG_HLC(csi2dc->inter_line_delay) &
>> + CSI2DC_GCFG_HLC_MASK) |
>> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));
>> +
>> + csi2dc_writel(csi2dc, CSI2DC_VPCOL,
>> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK);
>> + csi2dc_writel(csi2dc, CSI2DC_VPROW,
>> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK);
>> +
>> + csi2dc->completed = true;
>> +
>> + return ret;
>> +}
>> +
>> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = {
>> + .bound = csi2dc_async_bound,
>> + .complete = csi2dc_async_complete,
>> +};
>> +
>> +static void csi2dc_cleanup_notifier(struct csi2dc_device *csi2dc)
>> +{
>> + v4l2_async_notifier_unregister(&csi2dc->notifier);
>> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
>> +}
>> +
>> +static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,
>> + struct device_node *input_parent)
>> +{
>> + int ret;
>> +
>> + v4l2_async_notifier_init(&csi2dc->notifier);
>> +
>> + csi2dc->asd = kzalloc(sizeof(*csi2dc->asd), GFP_KERNEL);
>> + if (!csi2dc->asd)
>> + return -ENOMEM;
>> +
>> + csi2dc->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
>> + csi2dc->asd->match.fwnode = of_fwnode_handle(input_parent);
>> +
>> + ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd);
>> + if (ret) {
>> + dev_err(csi2dc->dev, "failed to add async notifier.\n");
>> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
>> + goto csi2dc_prepare_notifier_err;
>> + }
>> +
>> + csi2dc->notifier.ops = &csi2dc_async_ops;
>> +
>> + ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev,
>> + &csi2dc->notifier);
>> +
>> + if (ret) {
>> + dev_err(csi2dc->dev, "fail to register async notifier.\n");
>> + goto csi2dc_prepare_notifier_err;
>> + }
>> +
>> +csi2dc_prepare_notifier_err:
>> + of_node_put(input_parent);
>> +
>> + return ret;
>> +}
>> +
>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc,
>> + struct device_node *of_node)
>> +{
>> + struct device_node *input_node, *sink_node, *input_parent;
>> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, sink_endpoint = { 0 };
>> + int ret;
>> +
>> + input_node = of_graph_get_next_endpoint(of_node, NULL);
>> +
>> + if (!input_node) {
>> + dev_err(csi2dc->dev, "missing port node at %s, input node is mandatory.\n",
>> + of_node->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node),
>> + &input_endpoint);
>> +
>> + if (ret) {
>> + dev_err(csi2dc->dev, "endpoint not defined at %s\n",
>> + of_node->full_name);
>> + return ret;
>> + }
>> +
>> + input_parent = of_graph_get_remote_port_parent(input_node);
>> + if (!input_parent) {
>> + dev_err(csi2dc->dev, "could not get input node's parent node.\n");
>> + return -EINVAL;
>> + }
>> +
>> + sink_node = of_graph_get_next_endpoint(of_node, input_node);
>> +
>> + if (sink_node)
>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(sink_node),
>> + &sink_endpoint);
>> +
>> + if (!sink_node || ret) {
>> + dev_info(csi2dc->dev, "missing sink node at %s, data pipe available only.\n",
>> + of_node->full_name);
>> + } else {
>> + csi2dc->video_pipe = true;
>> + csi2dc->vc = sink_endpoint.base.id;
>> +
>> + dev_dbg(csi2dc->dev, "block %s %d.%d->%d.%d video pipe VC %d\n",
>> + of_node->full_name, input_endpoint.base.port,
>> + input_endpoint.base.id, sink_endpoint.base.port,
>> + sink_endpoint.base.id, csi2dc->vc);
>> + }
>> +
>> + csi2dc->clk_gated = of_property_read_bool(of_node,
>> + "microchip,clk-gated");
>> +
>> + dev_dbg(csi2dc->dev, "%s clock\n",
>> + csi2dc->clk_gated ? "Gated" : "Free running");
>> +
>> + ret = of_property_read_u32(of_node, "microchip,inter-line-delay",
>> + &csi2dc->inter_line_delay);
>> +
>> + if (ret || csi2dc->inter_line_delay > 16 ||
>> + csi2dc->inter_line_delay == 0) {
>> + dev_dbg(csi2dc->dev, "assuming inter line delay = 16 clocks");
>> + csi2dc->inter_line_delay = 16;
>> + ret = 0;
>> + }
>> + /* hardware automatically adds 1 */
>> + csi2dc->inter_line_delay--;
>> +
>> + /* prepare async notifier for subdevice completion */
>> +
>> + of_node_put(sink_node);
>> + of_node_put(input_node);
>> +
>> + return csi2dc_prepare_notifier(csi2dc, input_parent);
>> +}
>> +
>> +static int csi2dc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct csi2dc_device *csi2dc;
>> + struct resource *res = NULL;
>> + int ret = 0;
>> +
>> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);
>> + if (!csi2dc)
>> + return -ENOMEM;
>> +
>> + csi2dc->dev = dev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return -EINVAL;
>> +
>> + csi2dc->base = devm_ioremap_resource(dev, res);
>> +
>> + if (IS_ERR(csi2dc->base)) {
>> + dev_err(dev, "base address not set\n");
>> + return PTR_ERR(csi2dc->base);
>> + }
>> +
>> + csi2dc->pclk = devm_clk_get(dev, "pclk");
>> + if (IS_ERR(csi2dc->pclk)) {
>> + ret = PTR_ERR(csi2dc->pclk);
>> + dev_err(dev, "failed to get pclk: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = clk_prepare_enable(csi2dc->pclk);
>> + if (ret) {
>> + dev_err(dev, "failed to enable pclk: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + csi2dc->scck = devm_clk_get(dev, "scck");
>> + if (IS_ERR(csi2dc->scck)) {
>> + ret = PTR_ERR(csi2dc->scck);
>> + dev_err(dev, "failed to get scck: %d\n", ret);
>> + goto csi2dc_clk_fail;
>> + }
>> +
>> + ret = v4l2_device_register(dev, &csi2dc->v4l2_dev);
>> + if (ret) {
>> + dev_err(dev, "unable to register v4l2 device.\n");
>> + goto csi2dc_clk_fail;
>> + }
>> +
>> + v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);
>> +
>> + csi2dc->csi2dc_sd.owner = THIS_MODULE;
>> + csi2dc->csi2dc_sd.dev = dev;
>> + snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),
>> + "CSI2DC.0");
>> +
>> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
>> +
>> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, CSI2DC_PADS_NUM,
>> + csi2dc->pads);
>> + if (ret < 0) {
>> + dev_err(dev, "media entity init failed\n");
>> + goto csi2dc_probe_entity_err;
>> + }
>> +
>> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev);
>> +
>> + platform_set_drvdata(pdev, &csi2dc->csi2dc_sd);
>> +
>> + ret = csi2dc_of_parse(csi2dc, dev->of_node);
>> + if (ret)
>> + goto csi2dc_probe_entity_err;
>> +
>> + ret = csi2dc_formats_init(csi2dc);
>> + if (ret)
>> + goto csi2dc_probe_error;
>> +
>> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);
>> + if (ret)
>> + goto csi2dc_probe_error;
>> +
>> + pr_info("Microchip CSI2DC version %x\n",
>> + csi2dc_readl(csi2dc, CSI2DC_VERSION));
>
> dev_info(), please. Or just dev_dbg().
>
>> +
>> + pm_runtime_set_active(dev);
>> + pm_runtime_enable(dev);
>> + pm_request_idle(dev);
>> +
>> + return 0;
>> +
>> +csi2dc_probe_error:
>> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);
>> + csi2dc_cleanup_notifier(csi2dc);
>> +
>> +csi2dc_probe_entity_err:
>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>> + v4l2_device_unregister(&csi2dc->v4l2_dev);
>> +csi2dc_clk_fail:
>> + clk_disable_unprepare(csi2dc->pclk);
>> + return ret;
>> +}
>> +
>> +static int csi2dc_remove(struct platform_device *pdev)
>> +{
>> + struct v4l2_subdev *csi2dc_sd = platform_get_drvdata(pdev);
>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>> +
>> + pm_runtime_disable(&pdev->dev);
>> + csi2dc_cleanup_notifier(csi2dc);
>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>> + v4l2_device_unregister(&csi2dc->v4l2_dev);
>> + clk_disable_unprepare(csi2dc->pclk);
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)
>> +{
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev)
>> +{
>> + return 0;
>> +}
>> +
>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {
>> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)
>> +};
>> +
>> +static const struct of_device_id csi2dc_of_match[] = {
>> + { .compatible = "microchip,sama7g5-csi2dc" },
>> + { }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, csi2dc_of_match);
>> +
>> +static struct platform_driver csi2dc_driver = {
>> + .probe = csi2dc_probe,
>> + .remove = csi2dc_remove,
>> + .driver = {
>> + .name = "microchip-csi2dc",
>> + .pm = &csi2dc_dev_pm_ops,
>> + .of_match_table = of_match_ptr(csi2dc_of_match),
>> + },
>> +};
>> +
>> +module_platform_driver(csi2dc_driver);
>> +
>> +MODULE_AUTHOR("Eugen Hristev <[email protected]>");
>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_SUPPORTED_DEVICE("video");
>
> Hmm. This line needs to be more specific. I'd just drop it.
>
> --
> Kind regards,
>
> Sakari Ailus
>
On 07.09.2020 12:16, Eugen Hristev - M18282 wrote:
> On 31.08.2020 11:50, Sakari Ailus wrote:
>
>> Hi Eugen,
>>
>> Thanks for the update.
>
> Hi Sakari,
>
> Thanks for reviewing, but, could you please help me understand your
> review below ?
>
Gentle ping
Thanks
>>
>> On Wed, Aug 26, 2020 at 09:51:41AM +0300, Eugen Hristev wrote:
>>> Microchip CSI2DC (CSI2 Demultiplexer Controller) is a misc bridge device
>>> that converts a byte stream in IDI Synopsys format (coming from a CSI2HOST)
>>> to a pixel stream that can be captured by a sensor controller.
>>>
>>> Signed-off-by: Eugen Hristev <[email protected]>
>>> ---
>>> Changes in v2:
>>> - moved driver to platform/atmel
>>> - fixed minor things as per Sakari's review
>>> - still some things from v2 review are not yet addressed, to be followed up
>>>
>>> drivers/media/platform/atmel/Kconfig | 13 +
>>> drivers/media/platform/atmel/Makefile | 1 +
>>> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++
>>> 3 files changed, 714 insertions(+)
>>> create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.c
>>>
>>> diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig
>>> index 1850fe7f9360..80bbddcc2504 100644
>>> --- a/drivers/media/platform/atmel/Kconfig
>>> +++ b/drivers/media/platform/atmel/Kconfig
>>> @@ -21,3 +21,16 @@ config VIDEO_ATMEL_ISI
>>> help
>>> This module makes the ATMEL Image Sensor Interface available
>>> as a v4l2 device.
>>> +
>>> +config VIDEO_MICROCHIP_CSI2DC
>>> + tristate "Microchip CSI2 Demux Controller"
>>> + depends on VIDEO_V4L2 && COMMON_CLK && OF
>>> + depends on ARCH_AT91 || COMPILE_TEST
>>> + select MEDIA_CONTROLLER
>>> + select VIDEO_V4L2_SUBDEV_API
>>> + select V4L2_FWNODE
>>> + help
>>> + CSI2 Demux Controller driver. CSI2DC is a helper chip
>>> + that converts IDI interface byte stream to a parallel pixel stream.
>>> + It supports various RAW formats as input.
>>> + Performs clock domain crossing between hardware blocks.
>>> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
>>> index 2dba38994a70..8af7c5b534c3 100644
>>> --- a/drivers/media/platform/atmel/Makefile
>>> +++ b/drivers/media/platform/atmel/Makefile
>>> @@ -3,3 +3,4 @@ atmel-isc-objs = atmel-sama5d2-isc.o atmel-isc-base.o
>>>
>>> obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>>> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
>>> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o
>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c
>>> new file mode 100644
>>> index 000000000000..97d07a80bbf7
>>> --- /dev/null
>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c
>>> @@ -0,0 +1,700 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Microchip CSI2 Demux Controller (CSI2DC) driver
>>> + *
>>> + * Copyright (C) 2018-2020 Microchip Technology, Inc.
>>> + *
>>> + * Author: Eugen Hristev <[email protected]>
>>> + *
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_graph.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/videodev2.h>
>>> +
>>> +#include <media/v4l2-device.h>
>>> +#include <media/v4l2-fwnode.h>
>>> +#include <media/v4l2-subdev.h>
>>> +#include <media/videobuf2-dma-contig.h>
>>> +
>>> +/* Global configuration register */
>>> +#define CSI2DC_GCFG 0x0
>>> +
>>> +/* MIPI sensor pixel clock is free running */
>>> +#define CSI2DC_GCFG_MIPIFRN BIT(0)
>>> +/* Output waveform inter-line minimum delay */
>>> +#define CSI2DC_GCFG_HLC(v) ((v) << 4)
>>> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4)
>>> +
>>> +/* Global control register */
>>> +#define CSI2DC_GCTLR 0x04
>>> +#define CSI2DC_GCTLR_SWRST BIT(0)
>>> +
>>> +/* Global status register */
>>> +#define CSI2DC_GS 0x08
>>> +
>>> +/* SSP interrupt status register */
>>> +#define CSI2DC_SSPIS 0x28
>>> +/* Pipe update register */
>>> +#define CSI2DC_PU 0xC0
>>> +/* Video pipe attributes update */
>>> +#define CSI2DC_PU_VP BIT(0)
>>> +
>>> +/* Pipe update status register */
>>> +#define CSI2DC_PUS 0xC4
>>> +
>>> +/* Video pipeline enable register */
>>> +#define CSI2DC_VPE 0xF8
>>> +#define CSI2DC_VPE_ENABLE BIT(0)
>>> +
>>> +/* Video pipeline configuration register */
>>> +#define CSI2DC_VPCFG 0xFC
>>> +/* Data type */
>>> +#define CSI2DC_VPCFG_DT(v) ((v) << 0)
>>> +#define CSI2DC_VPCFG_DT_MASK GENMASK(5, 0)
>>> +/* Virtual channel identifier */
>>> +#define CSI2DC_VPCFG_VC(v) ((v) << 6)
>>> +#define CSI2DC_VPCFG_VC_MASK GENMASK(7, 6)
>>> +/* Decompression enable */
>>> +#define CSI2DC_VPCFG_DE BIT(8)
>>> +/* Decoder mode */
>>> +#define CSI2DC_VPCFG_DM(v) ((v) << 9)
>>> +#define CSI2DC_VPCFG_DM_DECODER8TO12 0
>>> +/* Decoder predictor 2 selection */
>>> +#define CSI2DC_VPCFG_DP2 BIT(12)
>>> +/* Recommended memory storage */
>>> +#define CSI2DC_VPCFG_RMS BIT(13)
>>> +/* Post adjustment */
>>> +#define CSI2DC_VPCFG_PA BIT(14)
>>> +
>>> +/* Video pipeline column register */
>>> +#define CSI2DC_VPCOL 0x100
>>> +/* Column number */
>>> +#define CSI2DC_VPCOL_COL(v) ((v) << 0)
>>> +#define CSI2DC_VPCOL_COL_MASK GENMASK(15, 0)
>>> +
>>> +/* Video pipeline row register */
>>> +#define CSI2DC_VPROW 0x104
>>> +/* Row number */
>>> +#define CSI2DC_VPROW_ROW(v) ((v) << 0)
>>> +#define CSI2DC_VPROW_ROW_MASK GENMASK(15, 0)
>>> +
>>> +/* Version register */
>>> +#define CSI2DC_VERSION 0x1FC
>>> +
>>> +/* register read/write helpers */
>>> +#define csi2dc_readl(st, reg) readl_relaxed((st)->base + (reg))
>>> +#define csi2dc_writel(st, reg, val) writel_relaxed((val), (st)->base + (reg))
>>> +
>>> +/* supported RAW data types */
>>> +#define CSI2DC_DT_RAW6 0x28
>>> +#define CSI2DC_DT_RAW7 0x29
>>> +#define CSI2DC_DT_RAW8 0x2A
>>> +#define CSI2DC_DT_RAW10 0x2B
>>> +#define CSI2DC_DT_RAW12 0x2C
>>> +#define CSI2DC_DT_RAW14 0x2D
>>> +
>>> +struct csi2dc_format {
>>> + u32 mbus_code;
>>> + u32 dt;
>>> +};
>>> +
>>> +static struct csi2dc_format csi2dc_formats_list[] = {
>>
>> const
>
> Elements from this array are being selected in a dynamic list, and
> pointed from a non-const array of selected working formats (at subdevice
> completion time). If I make this const , I will get compiler warnings
> when assigning to the non-const array.
> What solution do you suggest for this ?
>
>
>>
>>> + {
>>> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
>>> + .dt = CSI2DC_DT_RAW10,
>>> + },
>>> +};
>>> +
>>> +enum mipi_csi_pads {
>>> + CSI2DC_PAD_SINK = 0,
>>> + CSI2DC_PAD_SOURCE = 1,
>>> + CSI2DC_PADS_NUM = 2,
>>> +};
>>> +
>>> +struct csi2dc_device {
>>> + void __iomem *base;
>>> + struct v4l2_subdev csi2dc_sd;
>>> + struct device *dev;
>>> + struct v4l2_device v4l2_dev;
>>
>> This device is a bridge; the V4L2 device should be registered by the master
>> driver, i.e. the one handling DMA and registering the video nodes. Please
>> drop this.
>>
>>> + struct clk *pclk;
>>> + struct clk *scck;
>>> +
>>> + bool video_pipe;
>>> +
>>> + u32 num_fmts;
>>> + struct csi2dc_format **formats;
>>> +
>>> + struct csi2dc_format *cur_fmt;
>>> + struct csi2dc_format *try_fmt;
>>> +
>>> + struct media_pad pads[CSI2DC_PADS_NUM];
>>> +
>>> + bool clk_gated;
>>> + u32 inter_line_delay;
>>> + u32 vc;
>>> +
>>> + struct v4l2_async_subdev *asd;
>>> + struct v4l2_async_notifier notifier;
>>> +
>>> + struct v4l2_subdev *input_sd;
>>> + bool completed;
>>> +};
>>> +
>>> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc)
>>> +{
>>> + u32 vp;
>>> +
>>> + vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;
>>> + vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;
>>> + vp &= ~CSI2DC_VPCFG_DE;
>>> + vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);
>>> + vp &= ~CSI2DC_VPCFG_DP2;
>>> + vp &= ~CSI2DC_VPCFG_RMS;
>>> + vp |= CSI2DC_VPCFG_PA;
>>> +
>>> + csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);
>>> + csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);
>>> + csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);
>>> +}
>>> +
>>> +static inline struct csi2dc_device *
>>> +csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)
>>> +{
>>> + return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);
>>> +}
>>> +
>>> +static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,
>>> + struct v4l2_subdev_pad_config *cfg,
>>> + struct v4l2_subdev_mbus_code_enum *code)
>>> +{
>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>> +
>>> + if (code->index >= csi2dc->num_fmts)
>>> + return -EINVAL;
>>> +
>>> + code->code = csi2dc->formats[code->index]->mbus_code;
>>> + return 0;
>>> +}
>>> +
>>> +static int csi2dc_try_fmt(struct v4l2_subdev *csi2dc_sd,
>>> + struct v4l2_subdev_pad_config *cfg,
>>> + struct v4l2_subdev_format *req_fmt)
>>> +{
>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>> + struct csi2dc_format *fmt;
>>> +
>>> + if (!csi2dc->completed) {
>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>> + return 0;
>>> + }
>>> +
>>> + for (fmt = csi2dc->formats[0]; fmt; fmt++)
>>> + if (req_fmt->format.code == fmt->mbus_code)
>>> + csi2dc->try_fmt = fmt;
>>> +
>>> + /* in case we could not find the desired format, default to something */
>>> + if (!csi2dc->try_fmt ||
>>> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) {
>>> + csi2dc->try_fmt = csi2dc->formats[0];
>>> + req_fmt->format.code = csi2dc->formats[0]->mbus_code;
>>> + }
>>> +
>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
>>> +}
>>> +
>>> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,
>>> + struct v4l2_subdev_pad_config *cfg,
>>> + struct v4l2_subdev_format *req_fmt)
>>> +{
>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>> + int ret;
>>> +
>>> + if (!csi2dc->completed) {
>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>> + return 0;
>>> + }
>>> +
>>> + csi2dc_try_fmt(csi2dc_sd, cfg, req_fmt);
>>> +
>>> + if (csi2dc->try_fmt->mbus_code != req_fmt->format.code) {
>>> + dev_dbg(csi2dc->dev, "CSI2DC unsupported format 0x%x\n",
>>> + req_fmt->format.code);
>>
>> Please adjust the format into a supported one instead.
>
> I thought that only the 'try_fmt' calls should adapt to something that
> works. It is the same case for set_fmt ? This function cannot return
> invalid format ?
>>
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
>>> + if (ret) {
>>> + dev_err(csi2dc->dev, "input subdev failed %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + csi2dc->cur_fmt = csi2dc->try_fmt;
>>> + /* update video pipe */
>>> + csi2dc_vp_update(csi2dc);
>>> +
>>> + dev_dbg(csi2dc->dev, "CSI2DC new format: 0x%x\n", req_fmt->format.code);
>>> + return 0;
>>> +}
>>> +
>>> +static int csi2dc_formats_init(struct csi2dc_device *csi2dc)
>>> +{
>>> + int i, j;
>>> + struct csi2dc_format *fmt = &csi2dc_formats_list[0];
>>> +
>>> + csi2dc->num_fmts = 1;
>>> +
>>> + csi2dc->formats = devm_kcalloc(csi2dc->dev, csi2dc->num_fmts,
>>> + sizeof(*csi2dc->formats), GFP_KERNEL);
>>> +
>>> + for (i = 0, j = 0; i < ARRAY_SIZE(csi2dc_formats_list); i++, fmt++)
>>> + if (fmt->mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10)
>>> + csi2dc->formats[j++] = fmt;
>>> + return 0;
>>> +}
>>> +
>>> +static int csi2dc_s_power(struct v4l2_subdev *csi2dc_sd, int on)
>>> +{
>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>> + int ret = 0;
>>> +
>>> + if (!csi2dc->completed) {
>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>> + return 0;
>>> + }
>>> +
>>> + if (on)
>>> + ret = clk_prepare_enable(csi2dc->scck);
>>> + else
>>> + clk_disable_unprepare(csi2dc->scck);
>>> + if (ret)
>>> + dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret);
>>> +
>>> + /* if powering up, deassert reset line */
>>> + if (on)
>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);
>>> +
>>> + ret = v4l2_subdev_call(csi2dc->input_sd, core, s_power, on);
>>> +
>>> + /* if powering down, assert reset line */
>>> + if (!on)
>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST);
>>
>> These go to the runtime resume and suspend callbacks.
>
> Okay I will update
>
>>
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)
>>> +{
>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>> +
>>> + if (!csi2dc->completed) {
>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>> + return 0;
>>> + }
>>> +
>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable);
>>> +}
>>> +
>>> +static int csi2dc_g_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>> + struct v4l2_subdev_frame_interval *interval)
>>> +{
>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>> +
>>> + if (!csi2dc->completed) {
>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>> + return 0;
>>> + }
>>> +
>>> + return v4l2_subdev_call(csi2dc->input_sd, video, g_frame_interval,
>>> + interval);
>>> +}
>>> +
>>> +static int csi2dc_s_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>> + struct v4l2_subdev_frame_interval *interval)
>>> +{
>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>> +
>>> + if (!csi2dc->completed) {
>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>> + return 0;
>>> + }
>>> +
>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_frame_interval,
>>> + interval);
>>
>> If your driver is used with a MC-enabled master driver, you can drop all
>> the v4l2_subdev_call() macros apart from the ones related to s_stream
>> operation.
>
> And the set frame interval for example, will it be sent to the sensor
> automatically ?
> I noticed that such operations on the master driver were not propagated
> to the underlying subdevice. How does the master device 'know' how to
> call the input_sd ? this input_sd is not referenced anywhere in the
> master driver.
>
>>
>> If not, then you can drop the HAS_DEVNODE flag from the subdev flags. But
>> I'd try to avoid this route if possible.
>>
>>> +}
>>> +
>>> +static int csi2dc_enum_frame_size(struct v4l2_subdev *csi2dc_sd,
>>> + struct v4l2_subdev_pad_config *cfg,
>>> + struct v4l2_subdev_frame_size_enum *fse)
>>> +{
>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>> +
>>> + if (!csi2dc->completed) {
>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>> + return 0;
>>> + }
>>> +
>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_size, cfg,
>>> + fse);
>>> +}
>>> +
>>> +static int csi2dc_enum_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>> + struct v4l2_subdev_pad_config *cfg,
>>> + struct v4l2_subdev_frame_interval_enum *fie)
>>> +{
>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>> +
>>> + if (!csi2dc->completed) {
>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>> + return 0;
>>> + }
>>> +
>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_interval, cfg,
>>> + fie);
>>> +}
>>> +
>>> +static const struct v4l2_subdev_core_ops csi2dc_core_ops = {
>>> + .s_power = csi2dc_s_power,
>>
>> Please use runtime PM instead.
>>
>>> +};
>>> +
>>> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {
>>> + .enum_mbus_code = csi2dc_enum_mbus_code,
>>> + .set_fmt = csi2dc_set_fmt,
>>> + .enum_frame_size = csi2dc_enum_frame_size,
>>> + .enum_frame_interval = csi2dc_enum_frame_interval,
>>> +};
>>> +
>>> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = {
>>> + .s_stream = csi2dc_s_stream,
>>> + .g_frame_interval = csi2dc_g_frame_interval,
>>> + .s_frame_interval = csi2dc_s_frame_interval,
>>> +};
>>> +
>>> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = {
>>> + .core = &csi2dc_core_ops,
>>> + .pad = &csi2dc_pad_ops,
>>> + .video = &csi2dc_video_ops,
>>> +};
>>> +
>>> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,
>>> + struct v4l2_subdev *subdev,
>>> + struct v4l2_async_subdev *asd)
>>> +{
>>> + struct csi2dc_device *csi2dc = container_of(notifier->v4l2_dev,
>>> + struct csi2dc_device, v4l2_dev);
>>> + csi2dc->input_sd = subdev;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int csi2dc_async_complete(struct v4l2_async_notifier *notifier)
>>> +{
>>> + struct csi2dc_device *csi2dc =
>>> + container_of(notifier->v4l2_dev, struct csi2dc_device, v4l2_dev);
>>
>> Over 80 characters per line. Please check the rest of the driver, too.
>>
>>> + int ret;
>>> +
>>> + ret = v4l2_device_register_subdev_nodes(&csi2dc->v4l2_dev);
>>> + if (ret < 0) {
>>> + v4l2_err(&csi2dc->v4l2_dev, "failed to register subdev nodes\n");
>>> + return ret;
>>> + }
>>> +
>>> + csi2dc_writel(csi2dc, CSI2DC_GCFG,
>>> + (CSI2DC_GCFG_HLC(csi2dc->inter_line_delay) &
>>> + CSI2DC_GCFG_HLC_MASK) |
>>> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));
>>> +
>>> + csi2dc_writel(csi2dc, CSI2DC_VPCOL,
>>> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK);
>>> + csi2dc_writel(csi2dc, CSI2DC_VPROW,
>>> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK);
>>> +
>>> + csi2dc->completed = true;
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = {
>>> + .bound = csi2dc_async_bound,
>>> + .complete = csi2dc_async_complete,
>>> +};
>>> +
>>> +static void csi2dc_cleanup_notifier(struct csi2dc_device *csi2dc)
>>> +{
>>> + v4l2_async_notifier_unregister(&csi2dc->notifier);
>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
>>> +}
>>> +
>>> +static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,
>>> + struct device_node *input_parent)
>>> +{
>>> + int ret;
>>> +
>>> + v4l2_async_notifier_init(&csi2dc->notifier);
>>> +
>>> + csi2dc->asd = kzalloc(sizeof(*csi2dc->asd), GFP_KERNEL);
>>> + if (!csi2dc->asd)
>>> + return -ENOMEM;
>>> +
>>> + csi2dc->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
>>> + csi2dc->asd->match.fwnode = of_fwnode_handle(input_parent);
>>> +
>>> + ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd);
>>> + if (ret) {
>>> + dev_err(csi2dc->dev, "failed to add async notifier.\n");
>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
>>> + goto csi2dc_prepare_notifier_err;
>>> + }
>>> +
>>> + csi2dc->notifier.ops = &csi2dc_async_ops;
>>> +
>>> + ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev,
>>> + &csi2dc->notifier);
>>> +
>>> + if (ret) {
>>> + dev_err(csi2dc->dev, "fail to register async notifier.\n");
>>> + goto csi2dc_prepare_notifier_err;
>>> + }
>>> +
>>> +csi2dc_prepare_notifier_err:
>>> + of_node_put(input_parent);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc,
>>> + struct device_node *of_node)
>>> +{
>>> + struct device_node *input_node, *sink_node, *input_parent;
>>> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, sink_endpoint = { 0 };
>>> + int ret;
>>> +
>>> + input_node = of_graph_get_next_endpoint(of_node, NULL);
>>> +
>>> + if (!input_node) {
>>> + dev_err(csi2dc->dev, "missing port node at %s, input node is mandatory.\n",
>>> + of_node->full_name);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node),
>>> + &input_endpoint);
>>> +
>>> + if (ret) {
>>> + dev_err(csi2dc->dev, "endpoint not defined at %s\n",
>>> + of_node->full_name);
>>> + return ret;
>>> + }
>>> +
>>> + input_parent = of_graph_get_remote_port_parent(input_node);
>>> + if (!input_parent) {
>>> + dev_err(csi2dc->dev, "could not get input node's parent node.\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + sink_node = of_graph_get_next_endpoint(of_node, input_node);
>>> +
>>> + if (sink_node)
>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(sink_node),
>>> + &sink_endpoint);
>>> +
>>> + if (!sink_node || ret) {
>>> + dev_info(csi2dc->dev, "missing sink node at %s, data pipe available only.\n",
>>> + of_node->full_name);
>>> + } else {
>>> + csi2dc->video_pipe = true;
>>> + csi2dc->vc = sink_endpoint.base.id;
>>> +
>>> + dev_dbg(csi2dc->dev, "block %s %d.%d->%d.%d video pipe VC %d\n",
>>> + of_node->full_name, input_endpoint.base.port,
>>> + input_endpoint.base.id, sink_endpoint.base.port,
>>> + sink_endpoint.base.id, csi2dc->vc);
>>> + }
>>> +
>>> + csi2dc->clk_gated = of_property_read_bool(of_node,
>>> + "microchip,clk-gated");
>>> +
>>> + dev_dbg(csi2dc->dev, "%s clock\n",
>>> + csi2dc->clk_gated ? "Gated" : "Free running");
>>> +
>>> + ret = of_property_read_u32(of_node, "microchip,inter-line-delay",
>>> + &csi2dc->inter_line_delay);
>>> +
>>> + if (ret || csi2dc->inter_line_delay > 16 ||
>>> + csi2dc->inter_line_delay == 0) {
>>> + dev_dbg(csi2dc->dev, "assuming inter line delay = 16 clocks");
>>> + csi2dc->inter_line_delay = 16;
>>> + ret = 0;
>>> + }
>>> + /* hardware automatically adds 1 */
>>> + csi2dc->inter_line_delay--;
>>> +
>>> + /* prepare async notifier for subdevice completion */
>>> +
>>> + of_node_put(sink_node);
>>> + of_node_put(input_node);
>>> +
>>> + return csi2dc_prepare_notifier(csi2dc, input_parent);
>>> +}
>>> +
>>> +static int csi2dc_probe(struct platform_device *pdev)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct csi2dc_device *csi2dc;
>>> + struct resource *res = NULL;
>>> + int ret = 0;
>>> +
>>> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);
>>> + if (!csi2dc)
>>> + return -ENOMEM;
>>> +
>>> + csi2dc->dev = dev;
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + if (!res)
>>> + return -EINVAL;
>>> +
>>> + csi2dc->base = devm_ioremap_resource(dev, res);
>>> +
>>> + if (IS_ERR(csi2dc->base)) {
>>> + dev_err(dev, "base address not set\n");
>>> + return PTR_ERR(csi2dc->base);
>>> + }
>>> +
>>> + csi2dc->pclk = devm_clk_get(dev, "pclk");
>>> + if (IS_ERR(csi2dc->pclk)) {
>>> + ret = PTR_ERR(csi2dc->pclk);
>>> + dev_err(dev, "failed to get pclk: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + ret = clk_prepare_enable(csi2dc->pclk);
>>> + if (ret) {
>>> + dev_err(dev, "failed to enable pclk: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + csi2dc->scck = devm_clk_get(dev, "scck");
>>> + if (IS_ERR(csi2dc->scck)) {
>>> + ret = PTR_ERR(csi2dc->scck);
>>> + dev_err(dev, "failed to get scck: %d\n", ret);
>>> + goto csi2dc_clk_fail;
>>> + }
>>> +
>>> + ret = v4l2_device_register(dev, &csi2dc->v4l2_dev);
>>> + if (ret) {
>>> + dev_err(dev, "unable to register v4l2 device.\n");
>>> + goto csi2dc_clk_fail;
>>> + }
>>> +
>>> + v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);
>>> +
>>> + csi2dc->csi2dc_sd.owner = THIS_MODULE;
>>> + csi2dc->csi2dc_sd.dev = dev;
>>> + snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),
>>> + "CSI2DC.0");
>>> +
>>> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>>> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>>> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
>>> +
>>> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, CSI2DC_PADS_NUM,
>>> + csi2dc->pads);
>>> + if (ret < 0) {
>>> + dev_err(dev, "media entity init failed\n");
>>> + goto csi2dc_probe_entity_err;
>>> + }
>>> +
>>> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev);
>>> +
>>> + platform_set_drvdata(pdev, &csi2dc->csi2dc_sd);
>>> +
>>> + ret = csi2dc_of_parse(csi2dc, dev->of_node);
>>> + if (ret)
>>> + goto csi2dc_probe_entity_err;
>>> +
>>> + ret = csi2dc_formats_init(csi2dc);
>>> + if (ret)
>>> + goto csi2dc_probe_error;
>>> +
>>> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);
>>> + if (ret)
>>> + goto csi2dc_probe_error;
>>> +
>>> + pr_info("Microchip CSI2DC version %x\n",
>>> + csi2dc_readl(csi2dc, CSI2DC_VERSION));
>>
>> dev_info(), please. Or just dev_dbg().
>>
>>> +
>>> + pm_runtime_set_active(dev);
>>> + pm_runtime_enable(dev);
>>> + pm_request_idle(dev);
>>> +
>>> + return 0;
>>> +
>>> +csi2dc_probe_error:
>>> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);
>>> + csi2dc_cleanup_notifier(csi2dc);
>>> +
>>> +csi2dc_probe_entity_err:
>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>>> + v4l2_device_unregister(&csi2dc->v4l2_dev);
>>> +csi2dc_clk_fail:
>>> + clk_disable_unprepare(csi2dc->pclk);
>>> + return ret;
>>> +}
>>> +
>>> +static int csi2dc_remove(struct platform_device *pdev)
>>> +{
>>> + struct v4l2_subdev *csi2dc_sd = platform_get_drvdata(pdev);
>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>> +
>>> + pm_runtime_disable(&pdev->dev);
>>> + csi2dc_cleanup_notifier(csi2dc);
>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>>> + v4l2_device_unregister(&csi2dc->v4l2_dev);
>>> + clk_disable_unprepare(csi2dc->pclk);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {
>>> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)
>>> +};
>>> +
>>> +static const struct of_device_id csi2dc_of_match[] = {
>>> + { .compatible = "microchip,sama7g5-csi2dc" },
>>> + { }
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(of, csi2dc_of_match);
>>> +
>>> +static struct platform_driver csi2dc_driver = {
>>> + .probe = csi2dc_probe,
>>> + .remove = csi2dc_remove,
>>> + .driver = {
>>> + .name = "microchip-csi2dc",
>>> + .pm = &csi2dc_dev_pm_ops,
>>> + .of_match_table = of_match_ptr(csi2dc_of_match),
>>> + },
>>> +};
>>> +
>>> +module_platform_driver(csi2dc_driver);
>>> +
>>> +MODULE_AUTHOR("Eugen Hristev <[email protected]>");
>>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_SUPPORTED_DEVICE("video");
>>
>> Hmm. This line needs to be more specific. I'd just drop it.
>>
>> --
>> Kind regards,
>>
>> Sakari Ailus
>>
>
Hi Eugen,
My apologies for the late reply.
On Mon, Sep 07, 2020 at 09:16:57AM +0000, [email protected] wrote:
> On 31.08.2020 11:50, Sakari Ailus wrote:
>
> > Hi Eugen,
> >
> > Thanks for the update.
>
> Hi Sakari,
>
> Thanks for reviewing, but, could you please help me understand your
> review below ?
>
> >
> > On Wed, Aug 26, 2020 at 09:51:41AM +0300, Eugen Hristev wrote:
> >> Microchip CSI2DC (CSI2 Demultiplexer Controller) is a misc bridge device
> >> that converts a byte stream in IDI Synopsys format (coming from a CSI2HOST)
> >> to a pixel stream that can be captured by a sensor controller.
> >>
> >> Signed-off-by: Eugen Hristev <[email protected]>
> >> ---
> >> Changes in v2:
> >> - moved driver to platform/atmel
> >> - fixed minor things as per Sakari's review
> >> - still some things from v2 review are not yet addressed, to be followed up
> >>
> >> drivers/media/platform/atmel/Kconfig | 13 +
> >> drivers/media/platform/atmel/Makefile | 1 +
> >> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++
> >> 3 files changed, 714 insertions(+)
> >> create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.c
> >>
> >> diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig
> >> index 1850fe7f9360..80bbddcc2504 100644
> >> --- a/drivers/media/platform/atmel/Kconfig
> >> +++ b/drivers/media/platform/atmel/Kconfig
> >> @@ -21,3 +21,16 @@ config VIDEO_ATMEL_ISI
> >> help
> >> This module makes the ATMEL Image Sensor Interface available
> >> as a v4l2 device.
> >> +
> >> +config VIDEO_MICROCHIP_CSI2DC
> >> + tristate "Microchip CSI2 Demux Controller"
> >> + depends on VIDEO_V4L2 && COMMON_CLK && OF
> >> + depends on ARCH_AT91 || COMPILE_TEST
> >> + select MEDIA_CONTROLLER
> >> + select VIDEO_V4L2_SUBDEV_API
> >> + select V4L2_FWNODE
> >> + help
> >> + CSI2 Demux Controller driver. CSI2DC is a helper chip
> >> + that converts IDI interface byte stream to a parallel pixel stream.
> >> + It supports various RAW formats as input.
> >> + Performs clock domain crossing between hardware blocks.
> >> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
> >> index 2dba38994a70..8af7c5b534c3 100644
> >> --- a/drivers/media/platform/atmel/Makefile
> >> +++ b/drivers/media/platform/atmel/Makefile
> >> @@ -3,3 +3,4 @@ atmel-isc-objs = atmel-sama5d2-isc.o atmel-isc-base.o
> >>
> >> obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
> >> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
> >> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o
> >> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c
> >> new file mode 100644
> >> index 000000000000..97d07a80bbf7
> >> --- /dev/null
> >> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c
> >> @@ -0,0 +1,700 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Microchip CSI2 Demux Controller (CSI2DC) driver
> >> + *
> >> + * Copyright (C) 2018-2020 Microchip Technology, Inc.
> >> + *
> >> + * Author: Eugen Hristev <[email protected]>
> >> + *
> >> + */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_graph.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/videodev2.h>
> >> +
> >> +#include <media/v4l2-device.h>
> >> +#include <media/v4l2-fwnode.h>
> >> +#include <media/v4l2-subdev.h>
> >> +#include <media/videobuf2-dma-contig.h>
> >> +
> >> +/* Global configuration register */
> >> +#define CSI2DC_GCFG 0x0
> >> +
> >> +/* MIPI sensor pixel clock is free running */
> >> +#define CSI2DC_GCFG_MIPIFRN BIT(0)
> >> +/* Output waveform inter-line minimum delay */
> >> +#define CSI2DC_GCFG_HLC(v) ((v) << 4)
> >> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4)
> >> +
> >> +/* Global control register */
> >> +#define CSI2DC_GCTLR 0x04
> >> +#define CSI2DC_GCTLR_SWRST BIT(0)
> >> +
> >> +/* Global status register */
> >> +#define CSI2DC_GS 0x08
> >> +
> >> +/* SSP interrupt status register */
> >> +#define CSI2DC_SSPIS 0x28
> >> +/* Pipe update register */
> >> +#define CSI2DC_PU 0xC0
> >> +/* Video pipe attributes update */
> >> +#define CSI2DC_PU_VP BIT(0)
> >> +
> >> +/* Pipe update status register */
> >> +#define CSI2DC_PUS 0xC4
> >> +
> >> +/* Video pipeline enable register */
> >> +#define CSI2DC_VPE 0xF8
> >> +#define CSI2DC_VPE_ENABLE BIT(0)
> >> +
> >> +/* Video pipeline configuration register */
> >> +#define CSI2DC_VPCFG 0xFC
> >> +/* Data type */
> >> +#define CSI2DC_VPCFG_DT(v) ((v) << 0)
> >> +#define CSI2DC_VPCFG_DT_MASK GENMASK(5, 0)
> >> +/* Virtual channel identifier */
> >> +#define CSI2DC_VPCFG_VC(v) ((v) << 6)
> >> +#define CSI2DC_VPCFG_VC_MASK GENMASK(7, 6)
> >> +/* Decompression enable */
> >> +#define CSI2DC_VPCFG_DE BIT(8)
> >> +/* Decoder mode */
> >> +#define CSI2DC_VPCFG_DM(v) ((v) << 9)
> >> +#define CSI2DC_VPCFG_DM_DECODER8TO12 0
> >> +/* Decoder predictor 2 selection */
> >> +#define CSI2DC_VPCFG_DP2 BIT(12)
> >> +/* Recommended memory storage */
> >> +#define CSI2DC_VPCFG_RMS BIT(13)
> >> +/* Post adjustment */
> >> +#define CSI2DC_VPCFG_PA BIT(14)
> >> +
> >> +/* Video pipeline column register */
> >> +#define CSI2DC_VPCOL 0x100
> >> +/* Column number */
> >> +#define CSI2DC_VPCOL_COL(v) ((v) << 0)
> >> +#define CSI2DC_VPCOL_COL_MASK GENMASK(15, 0)
> >> +
> >> +/* Video pipeline row register */
> >> +#define CSI2DC_VPROW 0x104
> >> +/* Row number */
> >> +#define CSI2DC_VPROW_ROW(v) ((v) << 0)
> >> +#define CSI2DC_VPROW_ROW_MASK GENMASK(15, 0)
> >> +
> >> +/* Version register */
> >> +#define CSI2DC_VERSION 0x1FC
> >> +
> >> +/* register read/write helpers */
> >> +#define csi2dc_readl(st, reg) readl_relaxed((st)->base + (reg))
> >> +#define csi2dc_writel(st, reg, val) writel_relaxed((val), (st)->base + (reg))
> >> +
> >> +/* supported RAW data types */
> >> +#define CSI2DC_DT_RAW6 0x28
> >> +#define CSI2DC_DT_RAW7 0x29
> >> +#define CSI2DC_DT_RAW8 0x2A
> >> +#define CSI2DC_DT_RAW10 0x2B
> >> +#define CSI2DC_DT_RAW12 0x2C
> >> +#define CSI2DC_DT_RAW14 0x2D
> >> +
> >> +struct csi2dc_format {
> >> + u32 mbus_code;
> >> + u32 dt;
> >> +};
> >> +
> >> +static struct csi2dc_format csi2dc_formats_list[] = {
> >
> > const
>
> Elements from this array are being selected in a dynamic list, and
> pointed from a non-const array of selected working formats (at subdevice
> completion time). If I make this const , I will get compiler warnings
> when assigning to the non-const array.
> What solution do you suggest for this ?
You'll need to make the other instances const, too. They do not need to be
modified I presume?
>
>
> >
> >> + {
> >> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> >> + .dt = CSI2DC_DT_RAW10,
> >> + },
> >> +};
> >> +
> >> +enum mipi_csi_pads {
> >> + CSI2DC_PAD_SINK = 0,
> >> + CSI2DC_PAD_SOURCE = 1,
> >> + CSI2DC_PADS_NUM = 2,
> >> +};
> >> +
> >> +struct csi2dc_device {
> >> + void __iomem *base;
> >> + struct v4l2_subdev csi2dc_sd;
> >> + struct device *dev;
> >> + struct v4l2_device v4l2_dev;
> >
> > This device is a bridge; the V4L2 device should be registered by the master
> > driver, i.e. the one handling DMA and registering the video nodes. Please
> > drop this.
> >
> >> + struct clk *pclk;
> >> + struct clk *scck;
> >> +
> >> + bool video_pipe;
> >> +
> >> + u32 num_fmts;
> >> + struct csi2dc_format **formats;
> >> +
> >> + struct csi2dc_format *cur_fmt;
> >> + struct csi2dc_format *try_fmt;
> >> +
> >> + struct media_pad pads[CSI2DC_PADS_NUM];
> >> +
> >> + bool clk_gated;
> >> + u32 inter_line_delay;
> >> + u32 vc;
> >> +
> >> + struct v4l2_async_subdev *asd;
> >> + struct v4l2_async_notifier notifier;
> >> +
> >> + struct v4l2_subdev *input_sd;
> >> + bool completed;
> >> +};
> >> +
> >> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc)
> >> +{
> >> + u32 vp;
> >> +
> >> + vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;
> >> + vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;
> >> + vp &= ~CSI2DC_VPCFG_DE;
> >> + vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);
> >> + vp &= ~CSI2DC_VPCFG_DP2;
> >> + vp &= ~CSI2DC_VPCFG_RMS;
> >> + vp |= CSI2DC_VPCFG_PA;
> >> +
> >> + csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);
> >> + csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);
> >> + csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);
> >> +}
> >> +
> >> +static inline struct csi2dc_device *
> >> +csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)
> >> +{
> >> + return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);
> >> +}
> >> +
> >> +static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,
> >> + struct v4l2_subdev_pad_config *cfg,
> >> + struct v4l2_subdev_mbus_code_enum *code)
> >> +{
> >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> >> +
> >> + if (code->index >= csi2dc->num_fmts)
> >> + return -EINVAL;
> >> +
> >> + code->code = csi2dc->formats[code->index]->mbus_code;
> >> + return 0;
> >> +}
> >> +
> >> +static int csi2dc_try_fmt(struct v4l2_subdev *csi2dc_sd,
> >> + struct v4l2_subdev_pad_config *cfg,
> >> + struct v4l2_subdev_format *req_fmt)
> >> +{
> >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> >> + struct csi2dc_format *fmt;
> >> +
> >> + if (!csi2dc->completed) {
> >> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> >> + return 0;
> >> + }
> >> +
> >> + for (fmt = csi2dc->formats[0]; fmt; fmt++)
> >> + if (req_fmt->format.code == fmt->mbus_code)
> >> + csi2dc->try_fmt = fmt;
> >> +
> >> + /* in case we could not find the desired format, default to something */
> >> + if (!csi2dc->try_fmt ||
> >> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) {
> >> + csi2dc->try_fmt = csi2dc->formats[0];
> >> + req_fmt->format.code = csi2dc->formats[0]->mbus_code;
> >> + }
> >> +
> >> + return v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
> >> +}
> >> +
> >> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,
> >> + struct v4l2_subdev_pad_config *cfg,
> >> + struct v4l2_subdev_format *req_fmt)
> >> +{
> >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> >> + int ret;
> >> +
> >> + if (!csi2dc->completed) {
> >> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> >> + return 0;
> >> + }
> >> +
> >> + csi2dc_try_fmt(csi2dc_sd, cfg, req_fmt);
> >> +
> >> + if (csi2dc->try_fmt->mbus_code != req_fmt->format.code) {
> >> + dev_dbg(csi2dc->dev, "CSI2DC unsupported format 0x%x\n",
> >> + req_fmt->format.code);
> >
> > Please adjust the format into a supported one instead.
>
> I thought that only the 'try_fmt' calls should adapt to something that
> works. It is the same case for set_fmt ? This function cannot return
> invalid format ?
Correct. set_fmt should always return with a valid format.
Perhaps merging csi2dc_try_fmt() to this function would help? It's not used
elsewhere. It seems you're doing some redundant checks here, too.
> >
> >> + return -EINVAL;
> >> + }
> >> +
> >> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
> >> + if (ret) {
> >> + dev_err(csi2dc->dev, "input subdev failed %d\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + csi2dc->cur_fmt = csi2dc->try_fmt;
> >> + /* update video pipe */
> >> + csi2dc_vp_update(csi2dc);
> >> +
> >> + dev_dbg(csi2dc->dev, "CSI2DC new format: 0x%x\n", req_fmt->format.code);
> >> + return 0;
> >> +}
> >> +
> >> +static int csi2dc_formats_init(struct csi2dc_device *csi2dc)
> >> +{
> >> + int i, j;
> >> + struct csi2dc_format *fmt = &csi2dc_formats_list[0];
> >> +
> >> + csi2dc->num_fmts = 1;
> >> +
> >> + csi2dc->formats = devm_kcalloc(csi2dc->dev, csi2dc->num_fmts,
> >> + sizeof(*csi2dc->formats), GFP_KERNEL);
> >> +
> >> + for (i = 0, j = 0; i < ARRAY_SIZE(csi2dc_formats_list); i++, fmt++)
> >> + if (fmt->mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10)
> >> + csi2dc->formats[j++] = fmt;
> >> + return 0;
> >> +}
> >> +
> >> +static int csi2dc_s_power(struct v4l2_subdev *csi2dc_sd, int on)
> >> +{
> >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> >> + int ret = 0;
> >> +
> >> + if (!csi2dc->completed) {
> >> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> >> + return 0;
> >> + }
> >> +
> >> + if (on)
> >> + ret = clk_prepare_enable(csi2dc->scck);
> >> + else
> >> + clk_disable_unprepare(csi2dc->scck);
> >> + if (ret)
> >> + dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret);
> >> +
> >> + /* if powering up, deassert reset line */
> >> + if (on)
> >> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);
> >> +
> >> + ret = v4l2_subdev_call(csi2dc->input_sd, core, s_power, on);
> >> +
> >> + /* if powering down, assert reset line */
> >> + if (!on)
> >> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST);
> >
> > These go to the runtime resume and suspend callbacks.
>
> Okay I will update
>
> >
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)
> >> +{
> >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> >> +
> >> + if (!csi2dc->completed) {
> >> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> >> + return 0;
> >> + }
> >> +
> >> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable);
> >> +}
> >> +
> >> +static int csi2dc_g_frame_interval(struct v4l2_subdev *csi2dc_sd,
> >> + struct v4l2_subdev_frame_interval *interval)
> >> +{
> >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> >> +
> >> + if (!csi2dc->completed) {
> >> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> >> + return 0;
> >> + }
> >> +
> >> + return v4l2_subdev_call(csi2dc->input_sd, video, g_frame_interval,
> >> + interval);
> >> +}
> >> +
> >> +static int csi2dc_s_frame_interval(struct v4l2_subdev *csi2dc_sd,
> >> + struct v4l2_subdev_frame_interval *interval)
> >> +{
> >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> >> +
> >> + if (!csi2dc->completed) {
> >> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> >> + return 0;
> >> + }
> >> +
> >> + return v4l2_subdev_call(csi2dc->input_sd, video, s_frame_interval,
> >> + interval);
> >
> > If your driver is used with a MC-enabled master driver, you can drop all
> > the v4l2_subdev_call() macros apart from the ones related to s_stream
> > operation.
>
> And the set frame interval for example, will it be sent to the sensor
> automatically ?
> I noticed that such operations on the master driver were not propagated
> to the underlying subdevice. How does the master device 'know' how to
> call the input_sd ? this input_sd is not referenced anywhere in the
> master driver.
That's correct. In MC-centric drivers it's the responsibility of the user
space to configure the entire pipeline.
As your device is a CSI-2 to parallel (?) converter, it may well be part of
a complex pipeline that the master driver could not know about.
I remember there was an existing stm parallel receiver that was tightly
coupled by a CSI-2 to parallel IP block where the V4L2-centric model was
used. But virtually all devices that are not just simple sensor + receiver
with DMA are MC-centric.
>
> >
> > If not, then you can drop the HAS_DEVNODE flag from the subdev flags. But
> > I'd try to avoid this route if possible.
> >
> >> +}
> >> +
> >> +static int csi2dc_enum_frame_size(struct v4l2_subdev *csi2dc_sd,
> >> + struct v4l2_subdev_pad_config *cfg,
> >> + struct v4l2_subdev_frame_size_enum *fse)
> >> +{
> >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> >> +
> >> + if (!csi2dc->completed) {
> >> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> >> + return 0;
> >> + }
> >> +
> >> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_size, cfg,
> >> + fse);
> >> +}
> >> +
> >> +static int csi2dc_enum_frame_interval(struct v4l2_subdev *csi2dc_sd,
> >> + struct v4l2_subdev_pad_config *cfg,
> >> + struct v4l2_subdev_frame_interval_enum *fie)
> >> +{
> >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> >> +
> >> + if (!csi2dc->completed) {
> >> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
> >> + return 0;
> >> + }
> >> +
> >> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_interval, cfg,
> >> + fie);
> >> +}
> >> +
> >> +static const struct v4l2_subdev_core_ops csi2dc_core_ops = {
> >> + .s_power = csi2dc_s_power,
> >
> > Please use runtime PM instead.
> >
> >> +};
> >> +
> >> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {
> >> + .enum_mbus_code = csi2dc_enum_mbus_code,
> >> + .set_fmt = csi2dc_set_fmt,
> >> + .enum_frame_size = csi2dc_enum_frame_size,
> >> + .enum_frame_interval = csi2dc_enum_frame_interval,
> >> +};
> >> +
> >> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = {
> >> + .s_stream = csi2dc_s_stream,
> >> + .g_frame_interval = csi2dc_g_frame_interval,
> >> + .s_frame_interval = csi2dc_s_frame_interval,
> >> +};
> >> +
> >> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = {
> >> + .core = &csi2dc_core_ops,
> >> + .pad = &csi2dc_pad_ops,
> >> + .video = &csi2dc_video_ops,
> >> +};
> >> +
> >> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,
> >> + struct v4l2_subdev *subdev,
> >> + struct v4l2_async_subdev *asd)
> >> +{
> >> + struct csi2dc_device *csi2dc = container_of(notifier->v4l2_dev,
> >> + struct csi2dc_device, v4l2_dev);
> >> + csi2dc->input_sd = subdev;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int csi2dc_async_complete(struct v4l2_async_notifier *notifier)
> >> +{
> >> + struct csi2dc_device *csi2dc =
> >> + container_of(notifier->v4l2_dev, struct csi2dc_device, v4l2_dev);
> >
> > Over 80 characters per line. Please check the rest of the driver, too.
> >
> >> + int ret;
> >> +
> >> + ret = v4l2_device_register_subdev_nodes(&csi2dc->v4l2_dev);
> >> + if (ret < 0) {
> >> + v4l2_err(&csi2dc->v4l2_dev, "failed to register subdev nodes\n");
> >> + return ret;
> >> + }
> >> +
> >> + csi2dc_writel(csi2dc, CSI2DC_GCFG,
> >> + (CSI2DC_GCFG_HLC(csi2dc->inter_line_delay) &
> >> + CSI2DC_GCFG_HLC_MASK) |
> >> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));
> >> +
> >> + csi2dc_writel(csi2dc, CSI2DC_VPCOL,
> >> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK);
> >> + csi2dc_writel(csi2dc, CSI2DC_VPROW,
> >> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK);
> >> +
> >> + csi2dc->completed = true;
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = {
> >> + .bound = csi2dc_async_bound,
> >> + .complete = csi2dc_async_complete,
> >> +};
> >> +
> >> +static void csi2dc_cleanup_notifier(struct csi2dc_device *csi2dc)
> >> +{
> >> + v4l2_async_notifier_unregister(&csi2dc->notifier);
> >> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
> >> +}
> >> +
> >> +static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,
> >> + struct device_node *input_parent)
> >> +{
> >> + int ret;
> >> +
> >> + v4l2_async_notifier_init(&csi2dc->notifier);
> >> +
> >> + csi2dc->asd = kzalloc(sizeof(*csi2dc->asd), GFP_KERNEL);
> >> + if (!csi2dc->asd)
> >> + return -ENOMEM;
> >> +
> >> + csi2dc->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> >> + csi2dc->asd->match.fwnode = of_fwnode_handle(input_parent);
> >> +
> >> + ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd);
> >> + if (ret) {
> >> + dev_err(csi2dc->dev, "failed to add async notifier.\n");
> >> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
> >> + goto csi2dc_prepare_notifier_err;
> >> + }
> >> +
> >> + csi2dc->notifier.ops = &csi2dc_async_ops;
> >> +
> >> + ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev,
> >> + &csi2dc->notifier);
> >> +
> >> + if (ret) {
> >> + dev_err(csi2dc->dev, "fail to register async notifier.\n");
> >> + goto csi2dc_prepare_notifier_err;
> >> + }
> >> +
> >> +csi2dc_prepare_notifier_err:
> >> + of_node_put(input_parent);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc,
> >> + struct device_node *of_node)
> >> +{
> >> + struct device_node *input_node, *sink_node, *input_parent;
> >> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, sink_endpoint = { 0 };
> >> + int ret;
> >> +
> >> + input_node = of_graph_get_next_endpoint(of_node, NULL);
> >> +
> >> + if (!input_node) {
> >> + dev_err(csi2dc->dev, "missing port node at %s, input node is mandatory.\n",
> >> + of_node->full_name);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node),
> >> + &input_endpoint);
> >> +
> >> + if (ret) {
> >> + dev_err(csi2dc->dev, "endpoint not defined at %s\n",
> >> + of_node->full_name);
> >> + return ret;
> >> + }
> >> +
> >> + input_parent = of_graph_get_remote_port_parent(input_node);
> >> + if (!input_parent) {
> >> + dev_err(csi2dc->dev, "could not get input node's parent node.\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + sink_node = of_graph_get_next_endpoint(of_node, input_node);
> >> +
> >> + if (sink_node)
> >> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(sink_node),
> >> + &sink_endpoint);
> >> +
> >> + if (!sink_node || ret) {
> >> + dev_info(csi2dc->dev, "missing sink node at %s, data pipe available only.\n",
> >> + of_node->full_name);
> >> + } else {
> >> + csi2dc->video_pipe = true;
> >> + csi2dc->vc = sink_endpoint.base.id;
> >> +
> >> + dev_dbg(csi2dc->dev, "block %s %d.%d->%d.%d video pipe VC %d\n",
> >> + of_node->full_name, input_endpoint.base.port,
> >> + input_endpoint.base.id, sink_endpoint.base.port,
> >> + sink_endpoint.base.id, csi2dc->vc);
> >> + }
> >> +
> >> + csi2dc->clk_gated = of_property_read_bool(of_node,
> >> + "microchip,clk-gated");
> >> +
> >> + dev_dbg(csi2dc->dev, "%s clock\n",
> >> + csi2dc->clk_gated ? "Gated" : "Free running");
> >> +
> >> + ret = of_property_read_u32(of_node, "microchip,inter-line-delay",
> >> + &csi2dc->inter_line_delay);
> >> +
> >> + if (ret || csi2dc->inter_line_delay > 16 ||
> >> + csi2dc->inter_line_delay == 0) {
> >> + dev_dbg(csi2dc->dev, "assuming inter line delay = 16 clocks");
> >> + csi2dc->inter_line_delay = 16;
> >> + ret = 0;
> >> + }
> >> + /* hardware automatically adds 1 */
> >> + csi2dc->inter_line_delay--;
> >> +
> >> + /* prepare async notifier for subdevice completion */
> >> +
> >> + of_node_put(sink_node);
> >> + of_node_put(input_node);
> >> +
> >> + return csi2dc_prepare_notifier(csi2dc, input_parent);
> >> +}
> >> +
> >> +static int csi2dc_probe(struct platform_device *pdev)
> >> +{
> >> + struct device *dev = &pdev->dev;
> >> + struct csi2dc_device *csi2dc;
> >> + struct resource *res = NULL;
> >> + int ret = 0;
> >> +
> >> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);
> >> + if (!csi2dc)
> >> + return -ENOMEM;
> >> +
> >> + csi2dc->dev = dev;
> >> +
> >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> + if (!res)
> >> + return -EINVAL;
> >> +
> >> + csi2dc->base = devm_ioremap_resource(dev, res);
> >> +
> >> + if (IS_ERR(csi2dc->base)) {
> >> + dev_err(dev, "base address not set\n");
> >> + return PTR_ERR(csi2dc->base);
> >> + }
> >> +
> >> + csi2dc->pclk = devm_clk_get(dev, "pclk");
> >> + if (IS_ERR(csi2dc->pclk)) {
> >> + ret = PTR_ERR(csi2dc->pclk);
> >> + dev_err(dev, "failed to get pclk: %d\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + ret = clk_prepare_enable(csi2dc->pclk);
> >> + if (ret) {
> >> + dev_err(dev, "failed to enable pclk: %d\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + csi2dc->scck = devm_clk_get(dev, "scck");
> >> + if (IS_ERR(csi2dc->scck)) {
> >> + ret = PTR_ERR(csi2dc->scck);
> >> + dev_err(dev, "failed to get scck: %d\n", ret);
> >> + goto csi2dc_clk_fail;
> >> + }
> >> +
> >> + ret = v4l2_device_register(dev, &csi2dc->v4l2_dev);
> >> + if (ret) {
> >> + dev_err(dev, "unable to register v4l2 device.\n");
> >> + goto csi2dc_clk_fail;
> >> + }
> >> +
> >> + v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);
> >> +
> >> + csi2dc->csi2dc_sd.owner = THIS_MODULE;
> >> + csi2dc->csi2dc_sd.dev = dev;
> >> + snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),
> >> + "CSI2DC.0");
> >> +
> >> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> >> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> >> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> >> +
> >> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, CSI2DC_PADS_NUM,
> >> + csi2dc->pads);
> >> + if (ret < 0) {
> >> + dev_err(dev, "media entity init failed\n");
> >> + goto csi2dc_probe_entity_err;
> >> + }
> >> +
> >> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev);
> >> +
> >> + platform_set_drvdata(pdev, &csi2dc->csi2dc_sd);
> >> +
> >> + ret = csi2dc_of_parse(csi2dc, dev->of_node);
> >> + if (ret)
> >> + goto csi2dc_probe_entity_err;
> >> +
> >> + ret = csi2dc_formats_init(csi2dc);
> >> + if (ret)
> >> + goto csi2dc_probe_error;
> >> +
> >> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);
> >> + if (ret)
> >> + goto csi2dc_probe_error;
> >> +
> >> + pr_info("Microchip CSI2DC version %x\n",
> >> + csi2dc_readl(csi2dc, CSI2DC_VERSION));
> >
> > dev_info(), please. Or just dev_dbg().
> >
> >> +
> >> + pm_runtime_set_active(dev);
> >> + pm_runtime_enable(dev);
> >> + pm_request_idle(dev);
> >> +
> >> + return 0;
> >> +
> >> +csi2dc_probe_error:
> >> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);
> >> + csi2dc_cleanup_notifier(csi2dc);
> >> +
> >> +csi2dc_probe_entity_err:
> >> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
> >> + v4l2_device_unregister(&csi2dc->v4l2_dev);
> >> +csi2dc_clk_fail:
> >> + clk_disable_unprepare(csi2dc->pclk);
> >> + return ret;
> >> +}
> >> +
> >> +static int csi2dc_remove(struct platform_device *pdev)
> >> +{
> >> + struct v4l2_subdev *csi2dc_sd = platform_get_drvdata(pdev);
> >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
> >> +
> >> + pm_runtime_disable(&pdev->dev);
> >> + csi2dc_cleanup_notifier(csi2dc);
> >> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
> >> + v4l2_device_unregister(&csi2dc->v4l2_dev);
> >> + clk_disable_unprepare(csi2dc->pclk);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {
> >> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)
> >> +};
> >> +
> >> +static const struct of_device_id csi2dc_of_match[] = {
> >> + { .compatible = "microchip,sama7g5-csi2dc" },
> >> + { }
> >> +};
> >> +
> >> +MODULE_DEVICE_TABLE(of, csi2dc_of_match);
> >> +
> >> +static struct platform_driver csi2dc_driver = {
> >> + .probe = csi2dc_probe,
> >> + .remove = csi2dc_remove,
> >> + .driver = {
> >> + .name = "microchip-csi2dc",
> >> + .pm = &csi2dc_dev_pm_ops,
> >> + .of_match_table = of_match_ptr(csi2dc_of_match),
> >> + },
> >> +};
> >> +
> >> +module_platform_driver(csi2dc_driver);
> >> +
> >> +MODULE_AUTHOR("Eugen Hristev <[email protected]>");
> >> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver");
> >> +MODULE_LICENSE("GPL v2");
> >> +MODULE_SUPPORTED_DEVICE("video");
> >
> > Hmm. This line needs to be more specific. I'd just drop it.
> >
> > --
> > Kind regards,
> >
> > Sakari Ailus
> >
>
--
Regards,
Sakari Ailus
Hi Eugen,
Thank you for the patch.
On Wed, Aug 26, 2020 at 09:51:40AM +0300, Eugen Hristev wrote:
> Add bindings documentation for Microchip CSI2 Demultiplexer controller.
>
> CSI2DC is a demultiplexer from Synopsys IDI interface specification to
> parallel interface connection or direct memory access.
>
> Signed-off-by: Eugen Hristev <[email protected]>
> ---
> Changes in v3:
> - Removed some text from description, as it was explained in the schema
> - fixed other things as per Rob's review
> - moved some text inside the schema, like the clock description
>
> Changes in v2:
> - fixed warnings reported by dt_binding_check
>
> .../bindings/media/microchip,csi2dc.yaml | 174 ++++++++++++++++++
> 1 file changed, 174 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml b/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
> new file mode 100644
> index 000000000000..b4c1b8800a3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
> @@ -0,0 +1,174 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/microchip,csi2dc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip CSI2 Demux Controller (CSI2DC)
> +
> +maintainers:
> + - Eugen Hristev <[email protected]>
> +
> +description:
> + CSI2DC - Camera Serial Interface 2 Demux Controller
> +
> + CSI2DC is a hardware block that receives incoming data from an IDI interface
> + and filters packets based on their data type and virtual channel identifier,
> + then converts the byte stream into a cross clock domain to a pixel stream
> + to a parallel interface that can be read by a sensor controller.
> +
> + CSI2DC provides two pipes, one video pipe and one data pipe. Video pipe
> + is connected to a sensor controller and the data pipe is accessible
> + as a DMA slave port to a DMA controller.
> +
> + CSI2DC supports a single 'port' node as a source pad with Synopsys 32-bit
> + IDI interface. The connected endpoint must be a IDI interface compatible
> + device (like Synopsys CSI2HOST) , that can provide 32-bit IDI interface
> + connection as sink pad.
> + For media entity and endpoints please refer to the bindings defined in
> + Documentation/devicetree/bindings/media/video-interfaces.txt.
> + For Synopsys IDI interface please refer to
> + Documentation/devicetree/bindings/media/snps,dw-csi-plat.txt
> +
> + CSI2DC supports one 'port' node as sink pad with parallel interface. This is
> + called video pipe.
> + This port has an 'endpoint' can then be used as a source pad for another
> + controller (next in pipeline).
> + Please refer to the bindings defined in
> + Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> + CSI2DC also supports direct access to the data through AHB, via DMA channel,
> + called data pipe.
> + Because of this, the sink 'port' child node (second) is not mandatory.
> + If the sink 'port' child node is missing, only data pipe is available.
> +
> +properties:
> + compatible:
> + const: microchip,sama7g5-csi2dc
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 2
> +
> + clock-names:
> + description:
> + CSI2DC must have two clocks to function correctly. One clock is the
> + peripheral clock for the inside functionality of the hardware block.
> + This is named 'pclk'. The second clock must be the cross domain clock,
> + in which CSI2DC will perform clock crossing. This clock must be fed
> + by the next controller in pipeline, which usually is a sensor controller.
> + Normally this clock should be given by this sensor controller who
> + is also a clock source. This clock is named 'scck', sensor controller clock.
> + items:
> + - const: pclk
> + - const: scck
> +
> + microchip,clk-gated:
> + type: boolean
> + description:
> + If present, indicates that the clock is gated.
> + Otherwise, the clock is free-running.
I don't think this belongs to the DT bindings, it should instead be
queried from the source subdev at runtime.
> +
> + microchip,inter-line-delay:
> + allOf:
> + - $ref: /schemas/types.yaml#/definitions/uint32
> + - minimum: 1
> + - maximum: 16
> + default: 16
> + description:
> + Indicates how many clock cycles should be introduced between each line.
This also sounds like a configuration parameter. How does one compute
the right value for this ?
> +
> + port@0:
> + type: object
> + description:
> + Input port node, single endpoint describing the input pad.
> +
> + properties:
> + reg:
> + const: 0
> +
> + endpoint:
> + type: object
> +
> + properties:
> + remote-endpoint: true
> +
> + required:
> + - remote-endpoint
> +
> + additionalProperties: false
> +
> + additionalProperties: false
> +
> + port@1:
> + type: object
> + description:
> + Output port node, single endpoint, describing the output pad.
> +
> + properties:
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> + reg:
> + const: 1
> +
> + patternProperties:
> + "^endpoint@[0-3]$":
> + type: object
> +
> + properties:
> + reg:
> + enum: [0, 1, 2, 3]
> + description: virtual channel for the endpoint
The virtual channel used by the source is also something that needs to
be queried from the source at runtime, it doesn't belong to this
binding.
> +
> + remote-endpoint: true
> +
> + required:
> + - remote-endpoint
> + - reg
> +
> + additionalProperties: false
> +
> + additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - port@0
> +
> +examples:
> + - |
> + csi2dc@e1404000 {
> + compatible = "microchip,sama7g5-csi2dc";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0xe1404000 0x500>;
> + clocks = <&pclk>, <&scck>;
> + clock-names = "pclk", "scck";
> +
> + port@0 {
> + reg = <0>; /* must be 0, first child port */
> + csi2dc_in: endpoint { /* input from IDI interface */
> + remote-endpoint = <&csi2host_out>;
> + };
> + };
> +
> + port@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>; /* must be 1, second child port */
> + csi2dc_out: endpoint@2 {
> + reg = <2>; /* virtual channel identifier */
> + remote-endpoint = <&xisc_in>; /* output to sensor controller */
> + };
> + };
> + };
> +
> +...
--
Regards,
Laurent Pinchart
On 09.10.2020 17:58, Sakari Ailus wrote:
> Hi Eugen,
>
> My apologies for the late reply.
>
Hi,
Thank you for replying,
> On Mon, Sep 07, 2020 at 09:16:57AM +0000, [email protected] wrote:
>> On 31.08.2020 11:50, Sakari Ailus wrote:
>>
>>> Hi Eugen,
>>>
>>> Thanks for the update.
>>
>> Hi Sakari,
>>
>> Thanks for reviewing, but, could you please help me understand your
>> review below ?
>>
>>>
>>> On Wed, Aug 26, 2020 at 09:51:41AM +0300, Eugen Hristev wrote:
>>>> Microchip CSI2DC (CSI2 Demultiplexer Controller) is a misc bridge device
>>>> that converts a byte stream in IDI Synopsys format (coming from a CSI2HOST)
>>>> to a pixel stream that can be captured by a sensor controller.
>>>>
>>>> Signed-off-by: Eugen Hristev <[email protected]>
>>>> ---
>>>> Changes in v2:
>>>> - moved driver to platform/atmel
>>>> - fixed minor things as per Sakari's review
>>>> - still some things from v2 review are not yet addressed, to be followed up
>>>>
>>>> drivers/media/platform/atmel/Kconfig | 13 +
>>>> drivers/media/platform/atmel/Makefile | 1 +
>>>> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++
>>>> 3 files changed, 714 insertions(+)
>>>> create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.c
>>>>
>>>> diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig
>>>> index 1850fe7f9360..80bbddcc2504 100644
>>>> --- a/drivers/media/platform/atmel/Kconfig
>>>> +++ b/drivers/media/platform/atmel/Kconfig
>>>> @@ -21,3 +21,16 @@ config VIDEO_ATMEL_ISI
>>>> help
>>>> This module makes the ATMEL Image Sensor Interface available
>>>> as a v4l2 device.
>>>> +
>>>> +config VIDEO_MICROCHIP_CSI2DC
>>>> + tristate "Microchip CSI2 Demux Controller"
>>>> + depends on VIDEO_V4L2 && COMMON_CLK && OF
>>>> + depends on ARCH_AT91 || COMPILE_TEST
>>>> + select MEDIA_CONTROLLER
>>>> + select VIDEO_V4L2_SUBDEV_API
>>>> + select V4L2_FWNODE
>>>> + help
>>>> + CSI2 Demux Controller driver. CSI2DC is a helper chip
>>>> + that converts IDI interface byte stream to a parallel pixel stream.
>>>> + It supports various RAW formats as input.
>>>> + Performs clock domain crossing between hardware blocks.
>>>> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
>>>> index 2dba38994a70..8af7c5b534c3 100644
>>>> --- a/drivers/media/platform/atmel/Makefile
>>>> +++ b/drivers/media/platform/atmel/Makefile
>>>> @@ -3,3 +3,4 @@ atmel-isc-objs = atmel-sama5d2-isc.o atmel-isc-base.o
>>>>
>>>> obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>>>> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
>>>> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o
>>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c
>>>> new file mode 100644
>>>> index 000000000000..97d07a80bbf7
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c
>>>> @@ -0,0 +1,700 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Microchip CSI2 Demux Controller (CSI2DC) driver
>>>> + *
>>>> + * Copyright (C) 2018-2020 Microchip Technology, Inc.
>>>> + *
>>>> + * Author: Eugen Hristev <[email protected]>
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_graph.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +#include <linux/videodev2.h>
>>>> +
>>>> +#include <media/v4l2-device.h>
>>>> +#include <media/v4l2-fwnode.h>
>>>> +#include <media/v4l2-subdev.h>
>>>> +#include <media/videobuf2-dma-contig.h>
>>>> +
>>>> +/* Global configuration register */
>>>> +#define CSI2DC_GCFG 0x0
>>>> +
>>>> +/* MIPI sensor pixel clock is free running */
>>>> +#define CSI2DC_GCFG_MIPIFRN BIT(0)
>>>> +/* Output waveform inter-line minimum delay */
>>>> +#define CSI2DC_GCFG_HLC(v) ((v) << 4)
>>>> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4)
>>>> +
>>>> +/* Global control register */
>>>> +#define CSI2DC_GCTLR 0x04
>>>> +#define CSI2DC_GCTLR_SWRST BIT(0)
>>>> +
>>>> +/* Global status register */
>>>> +#define CSI2DC_GS 0x08
>>>> +
>>>> +/* SSP interrupt status register */
>>>> +#define CSI2DC_SSPIS 0x28
>>>> +/* Pipe update register */
>>>> +#define CSI2DC_PU 0xC0
>>>> +/* Video pipe attributes update */
>>>> +#define CSI2DC_PU_VP BIT(0)
>>>> +
>>>> +/* Pipe update status register */
>>>> +#define CSI2DC_PUS 0xC4
>>>> +
>>>> +/* Video pipeline enable register */
>>>> +#define CSI2DC_VPE 0xF8
>>>> +#define CSI2DC_VPE_ENABLE BIT(0)
>>>> +
>>>> +/* Video pipeline configuration register */
>>>> +#define CSI2DC_VPCFG 0xFC
>>>> +/* Data type */
>>>> +#define CSI2DC_VPCFG_DT(v) ((v) << 0)
>>>> +#define CSI2DC_VPCFG_DT_MASK GENMASK(5, 0)
>>>> +/* Virtual channel identifier */
>>>> +#define CSI2DC_VPCFG_VC(v) ((v) << 6)
>>>> +#define CSI2DC_VPCFG_VC_MASK GENMASK(7, 6)
>>>> +/* Decompression enable */
>>>> +#define CSI2DC_VPCFG_DE BIT(8)
>>>> +/* Decoder mode */
>>>> +#define CSI2DC_VPCFG_DM(v) ((v) << 9)
>>>> +#define CSI2DC_VPCFG_DM_DECODER8TO12 0
>>>> +/* Decoder predictor 2 selection */
>>>> +#define CSI2DC_VPCFG_DP2 BIT(12)
>>>> +/* Recommended memory storage */
>>>> +#define CSI2DC_VPCFG_RMS BIT(13)
>>>> +/* Post adjustment */
>>>> +#define CSI2DC_VPCFG_PA BIT(14)
>>>> +
>>>> +/* Video pipeline column register */
>>>> +#define CSI2DC_VPCOL 0x100
>>>> +/* Column number */
>>>> +#define CSI2DC_VPCOL_COL(v) ((v) << 0)
>>>> +#define CSI2DC_VPCOL_COL_MASK GENMASK(15, 0)
>>>> +
>>>> +/* Video pipeline row register */
>>>> +#define CSI2DC_VPROW 0x104
>>>> +/* Row number */
>>>> +#define CSI2DC_VPROW_ROW(v) ((v) << 0)
>>>> +#define CSI2DC_VPROW_ROW_MASK GENMASK(15, 0)
>>>> +
>>>> +/* Version register */
>>>> +#define CSI2DC_VERSION 0x1FC
>>>> +
>>>> +/* register read/write helpers */
>>>> +#define csi2dc_readl(st, reg) readl_relaxed((st)->base + (reg))
>>>> +#define csi2dc_writel(st, reg, val) writel_relaxed((val), (st)->base + (reg))
>>>> +
>>>> +/* supported RAW data types */
>>>> +#define CSI2DC_DT_RAW6 0x28
>>>> +#define CSI2DC_DT_RAW7 0x29
>>>> +#define CSI2DC_DT_RAW8 0x2A
>>>> +#define CSI2DC_DT_RAW10 0x2B
>>>> +#define CSI2DC_DT_RAW12 0x2C
>>>> +#define CSI2DC_DT_RAW14 0x2D
>>>> +
>>>> +struct csi2dc_format {
>>>> + u32 mbus_code;
>>>> + u32 dt;
>>>> +};
>>>> +
>>>> +static struct csi2dc_format csi2dc_formats_list[] = {
>>>
>>> const
>>
>> Elements from this array are being selected in a dynamic list, and
>> pointed from a non-const array of selected working formats (at subdevice
>> completion time). If I make this const , I will get compiler warnings
>> when assigning to the non-const array.
>> What solution do you suggest for this ?
>
> You'll need to make the other instances const, too. They do not need to be
> modified I presume?
That's the problem I am trying to explain. I have a dynamic (initially
empty) array that is filled with elements from this array which yes, is
constant, but if I qualify it as const, all const elements 'cannot be
assigned to a non-const array' and if I make the dynamic array as const,
then well, I cannot modify it.
>
>>
>>
>>>
>>>> + {
>>>> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
>>>> + .dt = CSI2DC_DT_RAW10,
>>>> + },
>>>> +};
>>>> +
>>>> +enum mipi_csi_pads {
>>>> + CSI2DC_PAD_SINK = 0,
>>>> + CSI2DC_PAD_SOURCE = 1,
>>>> + CSI2DC_PADS_NUM = 2,
>>>> +};
>>>> +
>>>> +struct csi2dc_device {
>>>> + void __iomem *base;
>>>> + struct v4l2_subdev csi2dc_sd;
>>>> + struct device *dev;
>>>> + struct v4l2_device v4l2_dev;
>>>
>>> This device is a bridge; the V4L2 device should be registered by the master
>>> driver, i.e. the one handling DMA and registering the video nodes. Please
>>> drop this.
>>>
>>>> + struct clk *pclk;
>>>> + struct clk *scck;
>>>> +
>>>> + bool video_pipe;
>>>> +
>>>> + u32 num_fmts;
>>>> + struct csi2dc_format **formats;
>>>> +
>>>> + struct csi2dc_format *cur_fmt;
>>>> + struct csi2dc_format *try_fmt;
>>>> +
>>>> + struct media_pad pads[CSI2DC_PADS_NUM];
>>>> +
>>>> + bool clk_gated;
>>>> + u32 inter_line_delay;
>>>> + u32 vc;
>>>> +
>>>> + struct v4l2_async_subdev *asd;
>>>> + struct v4l2_async_notifier notifier;
>>>> +
>>>> + struct v4l2_subdev *input_sd;
>>>> + bool completed;
>>>> +};
>>>> +
>>>> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc)
>>>> +{
>>>> + u32 vp;
>>>> +
>>>> + vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;
>>>> + vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;
>>>> + vp &= ~CSI2DC_VPCFG_DE;
>>>> + vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);
>>>> + vp &= ~CSI2DC_VPCFG_DP2;
>>>> + vp &= ~CSI2DC_VPCFG_RMS;
>>>> + vp |= CSI2DC_VPCFG_PA;
>>>> +
>>>> + csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);
>>>> + csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);
>>>> + csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);
>>>> +}
>>>> +
>>>> +static inline struct csi2dc_device *
>>>> +csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)
>>>> +{
>>>> + return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);
>>>> +}
>>>> +
>>>> +static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,
>>>> + struct v4l2_subdev_pad_config *cfg,
>>>> + struct v4l2_subdev_mbus_code_enum *code)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> +
>>>> + if (code->index >= csi2dc->num_fmts)
>>>> + return -EINVAL;
>>>> +
>>>> + code->code = csi2dc->formats[code->index]->mbus_code;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int csi2dc_try_fmt(struct v4l2_subdev *csi2dc_sd,
>>>> + struct v4l2_subdev_pad_config *cfg,
>>>> + struct v4l2_subdev_format *req_fmt)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> + struct csi2dc_format *fmt;
>>>> +
>>>> + if (!csi2dc->completed) {
>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>> + return 0;
>>>> + }
>>>> +
>>>> + for (fmt = csi2dc->formats[0]; fmt; fmt++)
>>>> + if (req_fmt->format.code == fmt->mbus_code)
>>>> + csi2dc->try_fmt = fmt;
>>>> +
>>>> + /* in case we could not find the desired format, default to something */
>>>> + if (!csi2dc->try_fmt ||
>>>> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) {
>>>> + csi2dc->try_fmt = csi2dc->formats[0];
>>>> + req_fmt->format.code = csi2dc->formats[0]->mbus_code;
>>>> + }
>>>> +
>>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
>>>> +}
>>>> +
>>>> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,
>>>> + struct v4l2_subdev_pad_config *cfg,
>>>> + struct v4l2_subdev_format *req_fmt)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> + int ret;
>>>> +
>>>> + if (!csi2dc->completed) {
>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>> + return 0;
>>>> + }
>>>> +
>>>> + csi2dc_try_fmt(csi2dc_sd, cfg, req_fmt);
>>>> +
>>>> + if (csi2dc->try_fmt->mbus_code != req_fmt->format.code) {
>>>> + dev_dbg(csi2dc->dev, "CSI2DC unsupported format 0x%x\n",
>>>> + req_fmt->format.code);
>>>
>>> Please adjust the format into a supported one instead.
>>
>> I thought that only the 'try_fmt' calls should adapt to something that
>> works. It is the same case for set_fmt ? This function cannot return
>> invalid format ?
>
> Correct. set_fmt should always return with a valid format.
>
> Perhaps merging csi2dc_try_fmt() to this function would help? It's not used
> elsewhere. It seems you're doing some redundant checks here, too.
But this would mean that this driver will not have a pure 'try_fmt' and
what will happen on try attempts from the master driver ?
>
>>>
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
>>>> + if (ret) {
>>>> + dev_err(csi2dc->dev, "input subdev failed %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + csi2dc->cur_fmt = csi2dc->try_fmt;
>>>> + /* update video pipe */
>>>> + csi2dc_vp_update(csi2dc);
>>>> +
>>>> + dev_dbg(csi2dc->dev, "CSI2DC new format: 0x%x\n", req_fmt->format.code);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int csi2dc_formats_init(struct csi2dc_device *csi2dc)
>>>> +{
>>>> + int i, j;
>>>> + struct csi2dc_format *fmt = &csi2dc_formats_list[0];
>>>> +
>>>> + csi2dc->num_fmts = 1;
>>>> +
>>>> + csi2dc->formats = devm_kcalloc(csi2dc->dev, csi2dc->num_fmts,
>>>> + sizeof(*csi2dc->formats), GFP_KERNEL);
>>>> +
>>>> + for (i = 0, j = 0; i < ARRAY_SIZE(csi2dc_formats_list); i++, fmt++)
>>>> + if (fmt->mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10)
>>>> + csi2dc->formats[j++] = fmt;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int csi2dc_s_power(struct v4l2_subdev *csi2dc_sd, int on)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> + int ret = 0;
>>>> +
>>>> + if (!csi2dc->completed) {
>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>> + return 0;
>>>> + }
>>>> +
>>>> + if (on)
>>>> + ret = clk_prepare_enable(csi2dc->scck);
>>>> + else
>>>> + clk_disable_unprepare(csi2dc->scck);
>>>> + if (ret)
>>>> + dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret);
>>>> +
>>>> + /* if powering up, deassert reset line */
>>>> + if (on)
>>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);
>>>> +
>>>> + ret = v4l2_subdev_call(csi2dc->input_sd, core, s_power, on);
>>>> +
>>>> + /* if powering down, assert reset line */
>>>> + if (!on)
>>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST);
>>>
>>> These go to the runtime resume and suspend callbacks.
>>
>> Okay I will update
Another question is what happens when CONFIG_PM is not used, thus we do
not have runtime resume and suspend callbacks ?
>>
>>>
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> +
>>>> + if (!csi2dc->completed) {
>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>> + return 0;
>>>> + }
>>>> +
>>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable);
>>>> +}
>>>> +
>>>> +static int csi2dc_g_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>>> + struct v4l2_subdev_frame_interval *interval)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> +
>>>> + if (!csi2dc->completed) {
>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>> + return 0;
>>>> + }
>>>> +
>>>> + return v4l2_subdev_call(csi2dc->input_sd, video, g_frame_interval,
>>>> + interval);
>>>> +}
>>>> +
>>>> +static int csi2dc_s_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>>> + struct v4l2_subdev_frame_interval *interval)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> +
>>>> + if (!csi2dc->completed) {
>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>> + return 0;
>>>> + }
>>>> +
>>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_frame_interval,
>>>> + interval);
>>>
>>> If your driver is used with a MC-enabled master driver, you can drop all
>>> the v4l2_subdev_call() macros apart from the ones related to s_stream
>>> operation.
>>
>> And the set frame interval for example, will it be sent to the sensor
>> automatically ?
>> I noticed that such operations on the master driver were not propagated
>> to the underlying subdevice. How does the master device 'know' how to
>> call the input_sd ? this input_sd is not referenced anywhere in the
>> master driver.
>
> That's correct. In MC-centric drivers it's the responsibility of the user
> space to configure the entire pipeline.
>
> As your device is a CSI-2 to parallel (?) converter, it may well be part of
> a complex pipeline that the master driver could not know about.
>
> I remember there was an existing stm parallel receiver that was tightly
> coupled by a CSI-2 to parallel IP block where the V4L2-centric model was
> used. But virtually all devices that are not just simple sensor + receiver
> with DMA are MC-centric.
Without these calls to the subdevice, the subdevice (csi2host and
sensor) are not aware of anything configured on the master driver. How
are such settings sent to the sensor ?
for example v4l2-ctl --list-formats-ext
# v4l2-ctl -d /dev/video0 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
Type: Video Capture
[0]: 'AR12' (16-bit ARGB 4-4-4-4)
Size: Discrete 3280x2464
Size: Discrete 1920x1080
Size: Discrete 1640x1232
[1]: 'AR15' (16-bit ARGB 1-5-5-5)
Size: Discrete 3280x2464
Size: Discrete 1920x1080
Size: Discrete 1640x1232
[2]: 'RGBP' (16-bit RGB 5-6-5)
Size: Discrete 3280x2464
Size: Discrete 1920x1080
Size: Discrete 1640x1232
All this information is obtained because of the enum_frame_size callback
to the subdevice
If I remove these,
# v4l2-ctl -d /dev/video0 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
Type: Video Capture
[0]: 'AR12' (16-bit ARGB 4-4-4-4)
[1]: 'AR15' (16-bit ARGB 1-5-5-5)
[2]: 'RGBP' (16-bit RGB 5-6-5)
[3]: 'AR24' (32-bit BGRA 8-8-8-8)
[4]: 'XR24' (32-bit BGRX 8-8-8-8)
[5]: 'YU12' (Planar YUV 4:2:0)
[6]: 'UYVY' (UYVY 4:2:2)
[7]: 'VYUY' (VYUY 4:2:2)
[8]: 'YUYV' (YUYV 4:2:2)
[9]: '422P' (Planar YUV 4:2:2)
[10]: 'GREY' (8-bit Greyscale)
[11]: 'Y10 ' (10-bit Greyscale)
[12]: 'Y16 ' (16-bit Greyscale)
[13]: 'RG10' (10-bit Bayer RGRG/GBGB)
All the frame information is no longer available.
So how can I obtain this information if I remove these callbacks that
you say they should be removed ?
>
>>
>>>
>>> If not, then you can drop the HAS_DEVNODE flag from the subdev flags. But
>>> I'd try to avoid this route if possible.
>>>
>>>> +}
>>>> +
>>>> +static int csi2dc_enum_frame_size(struct v4l2_subdev *csi2dc_sd,
>>>> + struct v4l2_subdev_pad_config *cfg,
>>>> + struct v4l2_subdev_frame_size_enum *fse)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> +
>>>> + if (!csi2dc->completed) {
>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>> + return 0;
>>>> + }
>>>> +
>>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_size, cfg,
>>>> + fse);
>>>> +}
>>>> +
>>>> +static int csi2dc_enum_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>>> + struct v4l2_subdev_pad_config *cfg,
>>>> + struct v4l2_subdev_frame_interval_enum *fie)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> +
>>>> + if (!csi2dc->completed) {
>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>> + return 0;
>>>> + }
>>>> +
>>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_interval, cfg,
>>>> + fie);
>>>> +}
>>>> +
>>>> +static const struct v4l2_subdev_core_ops csi2dc_core_ops = {
>>>> + .s_power = csi2dc_s_power,
>>>
>>> Please use runtime PM instead.
What happens if we do not have runtime PM ? (CONFIG_PM=n)
>>>
>>>> +};
>>>> +
>>>> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {
>>>> + .enum_mbus_code = csi2dc_enum_mbus_code,
>>>> + .set_fmt = csi2dc_set_fmt,
>>>> + .enum_frame_size = csi2dc_enum_frame_size,
>>>> + .enum_frame_interval = csi2dc_enum_frame_interval,
>>>> +};
>>>> +
>>>> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = {
>>>> + .s_stream = csi2dc_s_stream,
>>>> + .g_frame_interval = csi2dc_g_frame_interval,
>>>> + .s_frame_interval = csi2dc_s_frame_interval,
>>>> +};
>>>> +
>>>> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = {
>>>> + .core = &csi2dc_core_ops,
>>>> + .pad = &csi2dc_pad_ops,
>>>> + .video = &csi2dc_video_ops,
>>>> +};
>>>> +
>>>> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,
>>>> + struct v4l2_subdev *subdev,
>>>> + struct v4l2_async_subdev *asd)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = container_of(notifier->v4l2_dev,
>>>> + struct csi2dc_device, v4l2_dev);
>>>> + csi2dc->input_sd = subdev;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int csi2dc_async_complete(struct v4l2_async_notifier *notifier)
>>>> +{
>>>> + struct csi2dc_device *csi2dc =
>>>> + container_of(notifier->v4l2_dev, struct csi2dc_device, v4l2_dev);
>>>
>>> Over 80 characters per line. Please check the rest of the driver, too.
>>>
>>>> + int ret;
>>>> +
>>>> + ret = v4l2_device_register_subdev_nodes(&csi2dc->v4l2_dev);
>>>> + if (ret < 0) {
>>>> + v4l2_err(&csi2dc->v4l2_dev, "failed to register subdev nodes\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + csi2dc_writel(csi2dc, CSI2DC_GCFG,
>>>> + (CSI2DC_GCFG_HLC(csi2dc->inter_line_delay) &
>>>> + CSI2DC_GCFG_HLC_MASK) |
>>>> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));
>>>> +
>>>> + csi2dc_writel(csi2dc, CSI2DC_VPCOL,
>>>> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK);
>>>> + csi2dc_writel(csi2dc, CSI2DC_VPROW,
>>>> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK);
>>>> +
>>>> + csi2dc->completed = true;
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = {
>>>> + .bound = csi2dc_async_bound,
>>>> + .complete = csi2dc_async_complete,
>>>> +};
>>>> +
>>>> +static void csi2dc_cleanup_notifier(struct csi2dc_device *csi2dc)
>>>> +{
>>>> + v4l2_async_notifier_unregister(&csi2dc->notifier);
>>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
>>>> +}
>>>> +
>>>> +static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,
>>>> + struct device_node *input_parent)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + v4l2_async_notifier_init(&csi2dc->notifier);
>>>> +
>>>> + csi2dc->asd = kzalloc(sizeof(*csi2dc->asd), GFP_KERNEL);
>>>> + if (!csi2dc->asd)
>>>> + return -ENOMEM;
>>>> +
>>>> + csi2dc->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
>>>> + csi2dc->asd->match.fwnode = of_fwnode_handle(input_parent);
>>>> +
>>>> + ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd);
>>>> + if (ret) {
>>>> + dev_err(csi2dc->dev, "failed to add async notifier.\n");
>>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
>>>> + goto csi2dc_prepare_notifier_err;
>>>> + }
>>>> +
>>>> + csi2dc->notifier.ops = &csi2dc_async_ops;
>>>> +
>>>> + ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev,
>>>> + &csi2dc->notifier);
>>>> +
>>>> + if (ret) {
>>>> + dev_err(csi2dc->dev, "fail to register async notifier.\n");
>>>> + goto csi2dc_prepare_notifier_err;
>>>> + }
>>>> +
>>>> +csi2dc_prepare_notifier_err:
>>>> + of_node_put(input_parent);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc,
>>>> + struct device_node *of_node)
>>>> +{
>>>> + struct device_node *input_node, *sink_node, *input_parent;
>>>> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, sink_endpoint = { 0 };
>>>> + int ret;
>>>> +
>>>> + input_node = of_graph_get_next_endpoint(of_node, NULL);
>>>> +
>>>> + if (!input_node) {
>>>> + dev_err(csi2dc->dev, "missing port node at %s, input node is mandatory.\n",
>>>> + of_node->full_name);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node),
>>>> + &input_endpoint);
>>>> +
>>>> + if (ret) {
>>>> + dev_err(csi2dc->dev, "endpoint not defined at %s\n",
>>>> + of_node->full_name);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + input_parent = of_graph_get_remote_port_parent(input_node);
>>>> + if (!input_parent) {
>>>> + dev_err(csi2dc->dev, "could not get input node's parent node.\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + sink_node = of_graph_get_next_endpoint(of_node, input_node);
>>>> +
>>>> + if (sink_node)
>>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(sink_node),
>>>> + &sink_endpoint);
>>>> +
>>>> + if (!sink_node || ret) {
>>>> + dev_info(csi2dc->dev, "missing sink node at %s, data pipe available only.\n",
>>>> + of_node->full_name);
>>>> + } else {
>>>> + csi2dc->video_pipe = true;
>>>> + csi2dc->vc = sink_endpoint.base.id;
>>>> +
>>>> + dev_dbg(csi2dc->dev, "block %s %d.%d->%d.%d video pipe VC %d\n",
>>>> + of_node->full_name, input_endpoint.base.port,
>>>> + input_endpoint.base.id, sink_endpoint.base.port,
>>>> + sink_endpoint.base.id, csi2dc->vc);
>>>> + }
>>>> +
>>>> + csi2dc->clk_gated = of_property_read_bool(of_node,
>>>> + "microchip,clk-gated");
>>>> +
>>>> + dev_dbg(csi2dc->dev, "%s clock\n",
>>>> + csi2dc->clk_gated ? "Gated" : "Free running");
>>>> +
>>>> + ret = of_property_read_u32(of_node, "microchip,inter-line-delay",
>>>> + &csi2dc->inter_line_delay);
>>>> +
>>>> + if (ret || csi2dc->inter_line_delay > 16 ||
>>>> + csi2dc->inter_line_delay == 0) {
>>>> + dev_dbg(csi2dc->dev, "assuming inter line delay = 16 clocks");
>>>> + csi2dc->inter_line_delay = 16;
>>>> + ret = 0;
>>>> + }
>>>> + /* hardware automatically adds 1 */
>>>> + csi2dc->inter_line_delay--;
>>>> +
>>>> + /* prepare async notifier for subdevice completion */
>>>> +
>>>> + of_node_put(sink_node);
>>>> + of_node_put(input_node);
>>>> +
>>>> + return csi2dc_prepare_notifier(csi2dc, input_parent);
>>>> +}
>>>> +
>>>> +static int csi2dc_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct device *dev = &pdev->dev;
>>>> + struct csi2dc_device *csi2dc;
>>>> + struct resource *res = NULL;
>>>> + int ret = 0;
>>>> +
>>>> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);
>>>> + if (!csi2dc)
>>>> + return -ENOMEM;
>>>> +
>>>> + csi2dc->dev = dev;
>>>> +
>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> + if (!res)
>>>> + return -EINVAL;
>>>> +
>>>> + csi2dc->base = devm_ioremap_resource(dev, res);
>>>> +
>>>> + if (IS_ERR(csi2dc->base)) {
>>>> + dev_err(dev, "base address not set\n");
>>>> + return PTR_ERR(csi2dc->base);
>>>> + }
>>>> +
>>>> + csi2dc->pclk = devm_clk_get(dev, "pclk");
>>>> + if (IS_ERR(csi2dc->pclk)) {
>>>> + ret = PTR_ERR(csi2dc->pclk);
>>>> + dev_err(dev, "failed to get pclk: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = clk_prepare_enable(csi2dc->pclk);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to enable pclk: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + csi2dc->scck = devm_clk_get(dev, "scck");
>>>> + if (IS_ERR(csi2dc->scck)) {
>>>> + ret = PTR_ERR(csi2dc->scck);
>>>> + dev_err(dev, "failed to get scck: %d\n", ret);
>>>> + goto csi2dc_clk_fail;
>>>> + }
>>>> +
>>>> + ret = v4l2_device_register(dev, &csi2dc->v4l2_dev);
>>>> + if (ret) {
>>>> + dev_err(dev, "unable to register v4l2 device.\n");
>>>> + goto csi2dc_clk_fail;
>>>> + }
>>>> +
>>>> + v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);
>>>> +
>>>> + csi2dc->csi2dc_sd.owner = THIS_MODULE;
>>>> + csi2dc->csi2dc_sd.dev = dev;
>>>> + snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),
>>>> + "CSI2DC.0");
>>>> +
>>>> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>>> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>>>> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>>>> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
>>>> +
>>>> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, CSI2DC_PADS_NUM,
>>>> + csi2dc->pads);
>>>> + if (ret < 0) {
>>>> + dev_err(dev, "media entity init failed\n");
>>>> + goto csi2dc_probe_entity_err;
>>>> + }
>>>> +
>>>> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev);
>>>> +
>>>> + platform_set_drvdata(pdev, &csi2dc->csi2dc_sd);
>>>> +
>>>> + ret = csi2dc_of_parse(csi2dc, dev->of_node);
>>>> + if (ret)
>>>> + goto csi2dc_probe_entity_err;
>>>> +
>>>> + ret = csi2dc_formats_init(csi2dc);
>>>> + if (ret)
>>>> + goto csi2dc_probe_error;
>>>> +
>>>> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);
>>>> + if (ret)
>>>> + goto csi2dc_probe_error;
>>>> +
>>>> + pr_info("Microchip CSI2DC version %x\n",
>>>> + csi2dc_readl(csi2dc, CSI2DC_VERSION));
>>>
>>> dev_info(), please. Or just dev_dbg().
>>>
>>>> +
>>>> + pm_runtime_set_active(dev);
>>>> + pm_runtime_enable(dev);
>>>> + pm_request_idle(dev);
>>>> +
>>>> + return 0;
>>>> +
>>>> +csi2dc_probe_error:
>>>> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);
>>>> + csi2dc_cleanup_notifier(csi2dc);
>>>> +
>>>> +csi2dc_probe_entity_err:
>>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>>>> + v4l2_device_unregister(&csi2dc->v4l2_dev);
>>>> +csi2dc_clk_fail:
>>>> + clk_disable_unprepare(csi2dc->pclk);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int csi2dc_remove(struct platform_device *pdev)
>>>> +{
>>>> + struct v4l2_subdev *csi2dc_sd = platform_get_drvdata(pdev);
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> +
>>>> + pm_runtime_disable(&pdev->dev);
>>>> + csi2dc_cleanup_notifier(csi2dc);
>>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>>>> + v4l2_device_unregister(&csi2dc->v4l2_dev);
>>>> + clk_disable_unprepare(csi2dc->pclk);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {
>>>> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)
>>>> +};
>>>> +
>>>> +static const struct of_device_id csi2dc_of_match[] = {
>>>> + { .compatible = "microchip,sama7g5-csi2dc" },
>>>> + { }
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(of, csi2dc_of_match);
>>>> +
>>>> +static struct platform_driver csi2dc_driver = {
>>>> + .probe = csi2dc_probe,
>>>> + .remove = csi2dc_remove,
>>>> + .driver = {
>>>> + .name = "microchip-csi2dc",
>>>> + .pm = &csi2dc_dev_pm_ops,
>>>> + .of_match_table = of_match_ptr(csi2dc_of_match),
>>>> + },
>>>> +};
>>>> +
>>>> +module_platform_driver(csi2dc_driver);
>>>> +
>>>> +MODULE_AUTHOR("Eugen Hristev <[email protected]>");
>>>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> +MODULE_SUPPORTED_DEVICE("video");
>>>
>>> Hmm. This line needs to be more specific. I'd just drop it.
>>>
>>> --
>>> Kind regards,
>>>
>>> Sakari Ailus
>>>
>>
>
> --
> Regards,
>
> Sakari Ailus
>
On 11.10.2020 00:17, Laurent Pinchart wrote:
> Hi Eugen,
>
> Thank you for the patch.
Hi,
Thank you for your review,
>
> On Wed, Aug 26, 2020 at 09:51:40AM +0300, Eugen Hristev wrote:
>> Add bindings documentation for Microchip CSI2 Demultiplexer controller.
>>
>> CSI2DC is a demultiplexer from Synopsys IDI interface specification to
>> parallel interface connection or direct memory access.
>>
>> Signed-off-by: Eugen Hristev <[email protected]>
>> ---
>> Changes in v3:
>> - Removed some text from description, as it was explained in the schema
>> - fixed other things as per Rob's review
>> - moved some text inside the schema, like the clock description
>>
>> Changes in v2:
>> - fixed warnings reported by dt_binding_check
>>
>> .../bindings/media/microchip,csi2dc.yaml | 174 ++++++++++++++++++
>> 1 file changed, 174 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml b/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
>> new file mode 100644
>> index 000000000000..b4c1b8800a3b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
>> @@ -0,0 +1,174 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/microchip,csi2dc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip CSI2 Demux Controller (CSI2DC)
>> +
>> +maintainers:
>> + - Eugen Hristev <[email protected]>
>> +
>> +description:
>> + CSI2DC - Camera Serial Interface 2 Demux Controller
>> +
>> + CSI2DC is a hardware block that receives incoming data from an IDI interface
>> + and filters packets based on their data type and virtual channel identifier,
>> + then converts the byte stream into a cross clock domain to a pixel stream
>> + to a parallel interface that can be read by a sensor controller.
>> +
>> + CSI2DC provides two pipes, one video pipe and one data pipe. Video pipe
>> + is connected to a sensor controller and the data pipe is accessible
>> + as a DMA slave port to a DMA controller.
>> +
>> + CSI2DC supports a single 'port' node as a source pad with Synopsys 32-bit
>> + IDI interface. The connected endpoint must be a IDI interface compatible
>> + device (like Synopsys CSI2HOST) , that can provide 32-bit IDI interface
>> + connection as sink pad.
>> + For media entity and endpoints please refer to the bindings defined in
>> + Documentation/devicetree/bindings/media/video-interfaces.txt.
>> + For Synopsys IDI interface please refer to
>> + Documentation/devicetree/bindings/media/snps,dw-csi-plat.txt
>> +
>> + CSI2DC supports one 'port' node as sink pad with parallel interface. This is
>> + called video pipe.
>> + This port has an 'endpoint' can then be used as a source pad for another
>> + controller (next in pipeline).
>> + Please refer to the bindings defined in
>> + Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> + CSI2DC also supports direct access to the data through AHB, via DMA channel,
>> + called data pipe.
>> + Because of this, the sink 'port' child node (second) is not mandatory.
>> + If the sink 'port' child node is missing, only data pipe is available.
>> +
>> +properties:
>> + compatible:
>> + const: microchip,sama7g5-csi2dc
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + clocks:
>> + maxItems: 2
>> +
>> + clock-names:
>> + description:
>> + CSI2DC must have two clocks to function correctly. One clock is the
>> + peripheral clock for the inside functionality of the hardware block.
>> + This is named 'pclk'. The second clock must be the cross domain clock,
>> + in which CSI2DC will perform clock crossing. This clock must be fed
>> + by the next controller in pipeline, which usually is a sensor controller.
>> + Normally this clock should be given by this sensor controller who
>> + is also a clock source. This clock is named 'scck', sensor controller clock.
>> + items:
>> + - const: pclk
>> + - const: scck
>> +
>> + microchip,clk-gated:
>> + type: boolean
>> + description:
>> + If present, indicates that the clock is gated.
>> + Otherwise, the clock is free-running.
>
> I don't think this belongs to the DT bindings, it should instead be
> queried from the source subdev at runtime.
If this should be queried, do you know what is the v4l2 mechanism to
query such information ?
The subdevice is connected through a port interface to this device, so
it was natural for me to fully describe the interface in the devicetree
port description
>
>> +
>> + microchip,inter-line-delay:
>> + allOf:
>> + - $ref: /schemas/types.yaml#/definitions/uint32
>> + - minimum: 1
>> + - maximum: 16
>> + default: 16
>> + description:
>> + Indicates how many clock cycles should be introduced between each line.
>
> This also sounds like a configuration parameter. How does one compute
> the right value for this ?
I think this is a delay that can be added inside the hardware block,
depending on the interface speed and bandwidth. I will try to understand
more details from the hardware design and come back with a more detailed
answer.
>
>> +
>> + port@0:
>> + type: object
>> + description:
>> + Input port node, single endpoint describing the input pad.
>> +
>> + properties:
>> + reg:
>> + const: 0
>> +
>> + endpoint:
>> + type: object
>> +
>> + properties:
>> + remote-endpoint: true
>> +
>> + required:
>> + - remote-endpoint
>> +
>> + additionalProperties: false
>> +
>> + additionalProperties: false
>> +
>> + port@1:
>> + type: object
>> + description:
>> + Output port node, single endpoint, describing the output pad.
>> +
>> + properties:
>> + '#address-cells':
>> + const: 1
>> +
>> + '#size-cells':
>> + const: 0
>> +
>> + reg:
>> + const: 1
>> +
>> + patternProperties:
>> + "^endpoint@[0-3]$":
>> + type: object
>> +
>> + properties:
>> + reg:
>> + enum: [0, 1, 2, 3]
>> + description: virtual channel for the endpoint
>
> The virtual channel used by the source is also something that needs to
> be queried from the source at runtime, it doesn't belong to this
> binding.
The same question as for the gated clock configuration. How can we use
v4l2 subdevice API to obtain such information from the subdevice? And if
the subdevice does not offer such information ?
Thanks again,
Eugen
>
>> +
>> + remote-endpoint: true
>> +
>> + required:
>> + - remote-endpoint
>> + - reg
>> +
>> + additionalProperties: false
>> +
>> + additionalProperties: false
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - clocks
>> + - clock-names
>> + - port@0
>> +
>> +examples:
>> + - |
>> + csi2dc@e1404000 {
>> + compatible = "microchip,sama7g5-csi2dc";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <0xe1404000 0x500>;
>> + clocks = <&pclk>, <&scck>;
>> + clock-names = "pclk", "scck";
>> +
>> + port@0 {
>> + reg = <0>; /* must be 0, first child port */
>> + csi2dc_in: endpoint { /* input from IDI interface */
>> + remote-endpoint = <&csi2host_out>;
>> + };
>> + };
>> +
>> + port@1 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <1>; /* must be 1, second child port */
>> + csi2dc_out: endpoint@2 {
>> + reg = <2>; /* virtual channel identifier */
>> + remote-endpoint = <&xisc_in>; /* output to sensor controller */
>> + };
>> + };
>> + };
>> +
>> +...
>
> --
> Regards,
>
> Laurent Pinchart
>
Hello,
just my 2 cents, as I've noticed this patch skimming through the
list
On Mon, Oct 12, 2020 at 07:19:43AM +0000, [email protected] wrote:
> On 11.10.2020 00:17, Laurent Pinchart wrote:
> > Hi Eugen,
> >
> > Thank you for the patch.
>
> Hi,
>
> Thank you for your review,
>
> >
> > On Wed, Aug 26, 2020 at 09:51:40AM +0300, Eugen Hristev wrote:
> >> Add bindings documentation for Microchip CSI2 Demultiplexer controller.
> >>
> >> CSI2DC is a demultiplexer from Synopsys IDI interface specification to
> >> parallel interface connection or direct memory access.
> >>
> >> Signed-off-by: Eugen Hristev <[email protected]>
> >> ---
> >> Changes in v3:
> >> - Removed some text from description, as it was explained in the schema
> >> - fixed other things as per Rob's review
> >> - moved some text inside the schema, like the clock description
> >>
> >> Changes in v2:
> >> - fixed warnings reported by dt_binding_check
> >>
> >> .../bindings/media/microchip,csi2dc.yaml | 174 ++++++++++++++++++
> >> 1 file changed, 174 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml b/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
> >> new file mode 100644
> >> index 000000000000..b4c1b8800a3b
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
> >> @@ -0,0 +1,174 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/media/microchip,csi2dc.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Microchip CSI2 Demux Controller (CSI2DC)
> >> +
> >> +maintainers:
> >> + - Eugen Hristev <[email protected]>
> >> +
> >> +description:
> >> + CSI2DC - Camera Serial Interface 2 Demux Controller
> >> +
> >> + CSI2DC is a hardware block that receives incoming data from an IDI interface
> >> + and filters packets based on their data type and virtual channel identifier,
> >> + then converts the byte stream into a cross clock domain to a pixel stream
> >> + to a parallel interface that can be read by a sensor controller.
> >> +
> >> + CSI2DC provides two pipes, one video pipe and one data pipe. Video pipe
> >> + is connected to a sensor controller and the data pipe is accessible
> >> + as a DMA slave port to a DMA controller.
> >> +
> >> + CSI2DC supports a single 'port' node as a source pad with Synopsys 32-bit
> >> + IDI interface. The connected endpoint must be a IDI interface compatible
> >> + device (like Synopsys CSI2HOST) , that can provide 32-bit IDI interface
> >> + connection as sink pad.
> >> + For media entity and endpoints please refer to the bindings defined in
> >> + Documentation/devicetree/bindings/media/video-interfaces.txt.
> >> + For Synopsys IDI interface please refer to
> >> + Documentation/devicetree/bindings/media/snps,dw-csi-plat.txt
> >> +
> >> + CSI2DC supports one 'port' node as sink pad with parallel interface. This is
> >> + called video pipe.
> >> + This port has an 'endpoint' can then be used as a source pad for another
> >> + controller (next in pipeline).
> >> + Please refer to the bindings defined in
> >> + Documentation/devicetree/bindings/media/video-interfaces.txt.
> >> +
> >> + CSI2DC also supports direct access to the data through AHB, via DMA channel,
> >> + called data pipe.
> >> + Because of this, the sink 'port' child node (second) is not mandatory.
> >> + If the sink 'port' child node is missing, only data pipe is available.
> >> +
> >> +properties:
> >> + compatible:
> >> + const: microchip,sama7g5-csi2dc
> >> +
> >> + reg:
> >> + maxItems: 1
> >> +
> >> + clocks:
> >> + maxItems: 2
> >> +
> >> + clock-names:
> >> + description:
> >> + CSI2DC must have two clocks to function correctly. One clock is the
> >> + peripheral clock for the inside functionality of the hardware block.
> >> + This is named 'pclk'. The second clock must be the cross domain clock,
> >> + in which CSI2DC will perform clock crossing. This clock must be fed
> >> + by the next controller in pipeline, which usually is a sensor controller.
> >> + Normally this clock should be given by this sensor controller who
> >> + is also a clock source. This clock is named 'scck', sensor controller clock.
> >> + items:
> >> + - const: pclk
> >> + - const: scck
> >> +
> >> + microchip,clk-gated:
> >> + type: boolean
> >> + description:
> >> + If present, indicates that the clock is gated.
> >> + Otherwise, the clock is free-running.
> >
> > I don't think this belongs to the DT bindings, it should instead be
> > queried from the source subdev at runtime.
>
> If this should be queried, do you know what is the v4l2 mechanism to
> query such information ?
> The subdevice is connected through a port interface to this device, so
> it was natural for me to fully describe the interface in the devicetree
> port description
>
Is this property describing the CSI-2 clock continuous, non-continuous
mode configuration, or did I mis-interpreted it ?
We added support for retrieving run-time configuration of the media
bus with the get_mbus_config pad operations recently. Among the
configuration flags for MBUS_CSI2_DPHY there are inded CONTINUOUS and
NON_CONTINUOUS clock flags.
> >
> >> +
> >> + microchip,inter-line-delay:
> >> + allOf:
> >> + - $ref: /schemas/types.yaml#/definitions/uint32
> >> + - minimum: 1
> >> + - maximum: 16
> >> + default: 16
> >> + description:
> >> + Indicates how many clock cycles should be introduced between each line.
> >
> > This also sounds like a configuration parameter. How does one compute
> > the right value for this ?
>
> I think this is a delay that can be added inside the hardware block,
> depending on the interface speed and bandwidth. I will try to understand
> more details from the hardware design and come back with a more detailed
> answer.
>
> >
> >> +
> >> + port@0:
> >> + type: object
> >> + description:
> >> + Input port node, single endpoint describing the input pad.
> >> +
> >> + properties:
> >> + reg:
> >> + const: 0
> >> +
> >> + endpoint:
> >> + type: object
> >> +
> >> + properties:
> >> + remote-endpoint: true
> >> +
> >> + required:
> >> + - remote-endpoint
> >> +
> >> + additionalProperties: false
> >> +
> >> + additionalProperties: false
> >> +
> >> + port@1:
> >> + type: object
> >> + description:
> >> + Output port node, single endpoint, describing the output pad.
> >> +
> >> + properties:
> >> + '#address-cells':
> >> + const: 1
> >> +
> >> + '#size-cells':
> >> + const: 0
> >> +
> >> + reg:
> >> + const: 1
> >> +
> >> + patternProperties:
> >> + "^endpoint@[0-3]$":
> >> + type: object
> >> +
> >> + properties:
> >> + reg:
> >> + enum: [0, 1, 2, 3]
> >> + description: virtual channel for the endpoint
> >
> > The virtual channel used by the source is also something that needs to
> > be queried from the source at runtime, it doesn't belong to this
> > binding.
>
> The same question as for the gated clock configuration. How can we use
> v4l2 subdevice API to obtain such information from the subdevice? And if
> the subdevice does not offer such information ?
I think the subdev driver should be instrumented to report it instead of
hard-coding the information in DT which should be otherwise updated
depending on which sensor is connected to the board. Does it make
sense to you ?
Cheers
j
>
> Thanks again,
>
> Eugen
> >
> >> +
> >> + remote-endpoint: true
> >> +
> >> + required:
> >> + - remote-endpoint
> >> + - reg
> >> +
> >> + additionalProperties: false
> >> +
> >> + additionalProperties: false
> >> +
> >> +required:
> >> + - compatible
> >> + - reg
> >> + - clocks
> >> + - clock-names
> >> + - port@0
> >> +
> >> +examples:
> >> + - |
> >> + csi2dc@e1404000 {
> >> + compatible = "microchip,sama7g5-csi2dc";
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> + reg = <0xe1404000 0x500>;
> >> + clocks = <&pclk>, <&scck>;
> >> + clock-names = "pclk", "scck";
> >> +
> >> + port@0 {
> >> + reg = <0>; /* must be 0, first child port */
> >> + csi2dc_in: endpoint { /* input from IDI interface */
> >> + remote-endpoint = <&csi2host_out>;
> >> + };
> >> + };
> >> +
> >> + port@1 {
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> + reg = <1>; /* must be 1, second child port */
> >> + csi2dc_out: endpoint@2 {
> >> + reg = <2>; /* virtual channel identifier */
> >> + remote-endpoint = <&xisc_in>; /* output to sensor controller */
> >> + };
> >> + };
> >> + };
> >> +
> >> +...
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
>
On 12.10.2020 10:15, Eugen Hristev - M18282 wrote:
> On 09.10.2020 17:58, Sakari Ailus wrote:
>
>> Hi Eugen,
>>
>> My apologies for the late reply.
>>
>
> Hi,
>
> Thank you for replying,
>
>> On Mon, Sep 07, 2020 at 09:16:57AM +0000, [email protected] wrote:
>>> On 31.08.2020 11:50, Sakari Ailus wrote:
>>>
>>>> Hi Eugen,
>>>>
>>>> Thanks for the update.
>>>
>>> Hi Sakari,
>>>
>>> Thanks for reviewing, but, could you please help me understand your
>>> review below ?
>>>
>>>>
>>>> On Wed, Aug 26, 2020 at 09:51:41AM +0300, Eugen Hristev wrote:
>>>>> Microchip CSI2DC (CSI2 Demultiplexer Controller) is a misc bridge device
>>>>> that converts a byte stream in IDI Synopsys format (coming from a CSI2HOST)
>>>>> to a pixel stream that can be captured by a sensor controller.
>>>>>
>>>>> Signed-off-by: Eugen Hristev <[email protected]>
>>>>> ---
>>>>> Changes in v2:
>>>>> - moved driver to platform/atmel
>>>>> - fixed minor things as per Sakari's review
>>>>> - still some things from v2 review are not yet addressed, to be followed up
>>>>>
>>>>> drivers/media/platform/atmel/Kconfig | 13 +
>>>>> drivers/media/platform/atmel/Makefile | 1 +
>>>>> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++
>>>>> 3 files changed, 714 insertions(+)
>>>>> create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.c
>>>>>
>>>>> diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig
>>>>> index 1850fe7f9360..80bbddcc2504 100644
>>>>> --- a/drivers/media/platform/atmel/Kconfig
>>>>> +++ b/drivers/media/platform/atmel/Kconfig
>>>>> @@ -21,3 +21,16 @@ config VIDEO_ATMEL_ISI
>>>>> help
>>>>> This module makes the ATMEL Image Sensor Interface available
>>>>> as a v4l2 device.
>>>>> +
>>>>> +config VIDEO_MICROCHIP_CSI2DC
>>>>> + tristate "Microchip CSI2 Demux Controller"
>>>>> + depends on VIDEO_V4L2 && COMMON_CLK && OF
>>>>> + depends on ARCH_AT91 || COMPILE_TEST
>>>>> + select MEDIA_CONTROLLER
>>>>> + select VIDEO_V4L2_SUBDEV_API
>>>>> + select V4L2_FWNODE
>>>>> + help
>>>>> + CSI2 Demux Controller driver. CSI2DC is a helper chip
>>>>> + that converts IDI interface byte stream to a parallel pixel stream.
>>>>> + It supports various RAW formats as input.
>>>>> + Performs clock domain crossing between hardware blocks.
>>>>> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
>>>>> index 2dba38994a70..8af7c5b534c3 100644
>>>>> --- a/drivers/media/platform/atmel/Makefile
>>>>> +++ b/drivers/media/platform/atmel/Makefile
>>>>> @@ -3,3 +3,4 @@ atmel-isc-objs = atmel-sama5d2-isc.o atmel-isc-base.o
>>>>>
>>>>> obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>>>>> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
>>>>> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o
>>>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c
>>>>> new file mode 100644
>>>>> index 000000000000..97d07a80bbf7
>>>>> --- /dev/null
>>>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c
>>>>> @@ -0,0 +1,700 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Microchip CSI2 Demux Controller (CSI2DC) driver
>>>>> + *
>>>>> + * Copyright (C) 2018-2020 Microchip Technology, Inc.
>>>>> + *
>>>>> + * Author: Eugen Hristev <[email protected]>
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#include <linux/clk.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/of_graph.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/pm_runtime.h>
>>>>> +#include <linux/videodev2.h>
>>>>> +
>>>>> +#include <media/v4l2-device.h>
>>>>> +#include <media/v4l2-fwnode.h>
>>>>> +#include <media/v4l2-subdev.h>
>>>>> +#include <media/videobuf2-dma-contig.h>
>>>>> +
>>>>> +/* Global configuration register */
>>>>> +#define CSI2DC_GCFG 0x0
>>>>> +
>>>>> +/* MIPI sensor pixel clock is free running */
>>>>> +#define CSI2DC_GCFG_MIPIFRN BIT(0)
>>>>> +/* Output waveform inter-line minimum delay */
>>>>> +#define CSI2DC_GCFG_HLC(v) ((v) << 4)
>>>>> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4)
>>>>> +
>>>>> +/* Global control register */
>>>>> +#define CSI2DC_GCTLR 0x04
>>>>> +#define CSI2DC_GCTLR_SWRST BIT(0)
>>>>> +
>>>>> +/* Global status register */
>>>>> +#define CSI2DC_GS 0x08
>>>>> +
>>>>> +/* SSP interrupt status register */
>>>>> +#define CSI2DC_SSPIS 0x28
>>>>> +/* Pipe update register */
>>>>> +#define CSI2DC_PU 0xC0
>>>>> +/* Video pipe attributes update */
>>>>> +#define CSI2DC_PU_VP BIT(0)
>>>>> +
>>>>> +/* Pipe update status register */
>>>>> +#define CSI2DC_PUS 0xC4
>>>>> +
>>>>> +/* Video pipeline enable register */
>>>>> +#define CSI2DC_VPE 0xF8
>>>>> +#define CSI2DC_VPE_ENABLE BIT(0)
>>>>> +
>>>>> +/* Video pipeline configuration register */
>>>>> +#define CSI2DC_VPCFG 0xFC
>>>>> +/* Data type */
>>>>> +#define CSI2DC_VPCFG_DT(v) ((v) << 0)
>>>>> +#define CSI2DC_VPCFG_DT_MASK GENMASK(5, 0)
>>>>> +/* Virtual channel identifier */
>>>>> +#define CSI2DC_VPCFG_VC(v) ((v) << 6)
>>>>> +#define CSI2DC_VPCFG_VC_MASK GENMASK(7, 6)
>>>>> +/* Decompression enable */
>>>>> +#define CSI2DC_VPCFG_DE BIT(8)
>>>>> +/* Decoder mode */
>>>>> +#define CSI2DC_VPCFG_DM(v) ((v) << 9)
>>>>> +#define CSI2DC_VPCFG_DM_DECODER8TO12 0
>>>>> +/* Decoder predictor 2 selection */
>>>>> +#define CSI2DC_VPCFG_DP2 BIT(12)
>>>>> +/* Recommended memory storage */
>>>>> +#define CSI2DC_VPCFG_RMS BIT(13)
>>>>> +/* Post adjustment */
>>>>> +#define CSI2DC_VPCFG_PA BIT(14)
>>>>> +
>>>>> +/* Video pipeline column register */
>>>>> +#define CSI2DC_VPCOL 0x100
>>>>> +/* Column number */
>>>>> +#define CSI2DC_VPCOL_COL(v) ((v) << 0)
>>>>> +#define CSI2DC_VPCOL_COL_MASK GENMASK(15, 0)
>>>>> +
>>>>> +/* Video pipeline row register */
>>>>> +#define CSI2DC_VPROW 0x104
>>>>> +/* Row number */
>>>>> +#define CSI2DC_VPROW_ROW(v) ((v) << 0)
>>>>> +#define CSI2DC_VPROW_ROW_MASK GENMASK(15, 0)
>>>>> +
>>>>> +/* Version register */
>>>>> +#define CSI2DC_VERSION 0x1FC
>>>>> +
>>>>> +/* register read/write helpers */
>>>>> +#define csi2dc_readl(st, reg) readl_relaxed((st)->base + (reg))
>>>>> +#define csi2dc_writel(st, reg, val) writel_relaxed((val), (st)->base + (reg))
>>>>> +
>>>>> +/* supported RAW data types */
>>>>> +#define CSI2DC_DT_RAW6 0x28
>>>>> +#define CSI2DC_DT_RAW7 0x29
>>>>> +#define CSI2DC_DT_RAW8 0x2A
>>>>> +#define CSI2DC_DT_RAW10 0x2B
>>>>> +#define CSI2DC_DT_RAW12 0x2C
>>>>> +#define CSI2DC_DT_RAW14 0x2D
>>>>> +
>>>>> +struct csi2dc_format {
>>>>> + u32 mbus_code;
>>>>> + u32 dt;
>>>>> +};
>>>>> +
>>>>> +static struct csi2dc_format csi2dc_formats_list[] = {
>>>>
>>>> const
>>>
>>> Elements from this array are being selected in a dynamic list, and
>>> pointed from a non-const array of selected working formats (at subdevice
>>> completion time). If I make this const , I will get compiler warnings
>>> when assigning to the non-const array.
>>> What solution do you suggest for this ?
>>
>> You'll need to make the other instances const, too. They do not need to be
>> modified I presume?
>
> That's the problem I am trying to explain. I have a dynamic (initially
> empty) array that is filled with elements from this array which yes, is
> constant, but if I qualify it as const, all const elements 'cannot be
> assigned to a non-const array' and if I make the dynamic array as const,
> then well, I cannot modify it.
>
>>
>>>
>>>
>>>>
>>>>> + {
>>>>> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
>>>>> + .dt = CSI2DC_DT_RAW10,
>>>>> + },
>>>>> +};
>>>>> +
>>>>> +enum mipi_csi_pads {
>>>>> + CSI2DC_PAD_SINK = 0,
>>>>> + CSI2DC_PAD_SOURCE = 1,
>>>>> + CSI2DC_PADS_NUM = 2,
>>>>> +};
>>>>> +
>>>>> +struct csi2dc_device {
>>>>> + void __iomem *base;
>>>>> + struct v4l2_subdev csi2dc_sd;
>>>>> + struct device *dev;
>>>>> + struct v4l2_device v4l2_dev;
>>>>
>>>> This device is a bridge; the V4L2 device should be registered by the master
>>>> driver, i.e. the one handling DMA and registering the video nodes. Please
>>>> drop this.
Hello Sakari,
Taking this v4l2_dev out, means that I am no longer able to create and
add a notifier to this 'csi2dc' device. This means that I can no longer
connect the subdevice to this device.
In this situation, how can I connect the subdevice in the pipeline to
the csi2dc node ? The subdevice is the 'csi2host' hardware block.
The pipeline looks like this:
Sensor -> (mipi csi2) -> csi2host -> (IDI interface) -> csi2dc ->
(parallel interface) -> image sensor controller (master driver)
The master driver is also written to cope with this pipeline
Sensor -> (parallel interface) -> image sensor controller (master driver)
The master driver awaits for the subdevice to have its 'completion'
ready, which in the first pipeline is the subdev that csi2dc is
registering, in the second case it's the sensor itself.
Could you please help me understand how to change this such that both
pipelines work ?
Thanks again for your review,
Eugen
>>>>
>>>>> + struct clk *pclk;
>>>>> + struct clk *scck;
>>>>> +
>>>>> + bool video_pipe;
>>>>> +
>>>>> + u32 num_fmts;
>>>>> + struct csi2dc_format **formats;
>>>>> +
>>>>> + struct csi2dc_format *cur_fmt;
>>>>> + struct csi2dc_format *try_fmt;
>>>>> +
>>>>> + struct media_pad pads[CSI2DC_PADS_NUM];
>>>>> +
>>>>> + bool clk_gated;
>>>>> + u32 inter_line_delay;
>>>>> + u32 vc;
>>>>> +
>>>>> + struct v4l2_async_subdev *asd;
>>>>> + struct v4l2_async_notifier notifier;
>>>>> +
>>>>> + struct v4l2_subdev *input_sd;
>>>>> + bool completed;
>>>>> +};
>>>>> +
>>>>> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc)
>>>>> +{
>>>>> + u32 vp;
>>>>> +
>>>>> + vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;
>>>>> + vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;
>>>>> + vp &= ~CSI2DC_VPCFG_DE;
>>>>> + vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);
>>>>> + vp &= ~CSI2DC_VPCFG_DP2;
>>>>> + vp &= ~CSI2DC_VPCFG_RMS;
>>>>> + vp |= CSI2DC_VPCFG_PA;
>>>>> +
>>>>> + csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);
>>>>> + csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);
>>>>> + csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);
>>>>> +}
>>>>> +
>>>>> +static inline struct csi2dc_device *
>>>>> +csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)
>>>>> +{
>>>>> + return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_pad_config *cfg,
>>>>> + struct v4l2_subdev_mbus_code_enum *code)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (code->index >= csi2dc->num_fmts)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + code->code = csi2dc->formats[code->index]->mbus_code;
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_try_fmt(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_pad_config *cfg,
>>>>> + struct v4l2_subdev_format *req_fmt)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> + struct csi2dc_format *fmt;
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + for (fmt = csi2dc->formats[0]; fmt; fmt++)
>>>>> + if (req_fmt->format.code == fmt->mbus_code)
>>>>> + csi2dc->try_fmt = fmt;
>>>>> +
>>>>> + /* in case we could not find the desired format, default to something */
>>>>> + if (!csi2dc->try_fmt ||
>>>>> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) {
>>>>> + csi2dc->try_fmt = csi2dc->formats[0];
>>>>> + req_fmt->format.code = csi2dc->formats[0]->mbus_code;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_pad_config *cfg,
>>>>> + struct v4l2_subdev_format *req_fmt)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> + int ret;
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + csi2dc_try_fmt(csi2dc_sd, cfg, req_fmt);
>>>>> +
>>>>> + if (csi2dc->try_fmt->mbus_code != req_fmt->format.code) {
>>>>> + dev_dbg(csi2dc->dev, "CSI2DC unsupported format 0x%x\n",
>>>>> + req_fmt->format.code);
>>>>
>>>> Please adjust the format into a supported one instead.
>>>
>>> I thought that only the 'try_fmt' calls should adapt to something that
>>> works. It is the same case for set_fmt ? This function cannot return
>>> invalid format ?
>>
>> Correct. set_fmt should always return with a valid format.
>>
>> Perhaps merging csi2dc_try_fmt() to this function would help? It's not used
>> elsewhere. It seems you're doing some redundant checks here, too.
>
> But this would mean that this driver will not have a pure 'try_fmt' and
> what will happen on try attempts from the master driver ?
>
>>
>>>>
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
>>>>> + if (ret) {
>>>>> + dev_err(csi2dc->dev, "input subdev failed %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + csi2dc->cur_fmt = csi2dc->try_fmt;
>>>>> + /* update video pipe */
>>>>> + csi2dc_vp_update(csi2dc);
>>>>> +
>>>>> + dev_dbg(csi2dc->dev, "CSI2DC new format: 0x%x\n", req_fmt->format.code);
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_formats_init(struct csi2dc_device *csi2dc)
>>>>> +{
>>>>> + int i, j;
>>>>> + struct csi2dc_format *fmt = &csi2dc_formats_list[0];
>>>>> +
>>>>> + csi2dc->num_fmts = 1;
>>>>> +
>>>>> + csi2dc->formats = devm_kcalloc(csi2dc->dev, csi2dc->num_fmts,
>>>>> + sizeof(*csi2dc->formats), GFP_KERNEL);
>>>>> +
>>>>> + for (i = 0, j = 0; i < ARRAY_SIZE(csi2dc_formats_list); i++, fmt++)
>>>>> + if (fmt->mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10)
>>>>> + csi2dc->formats[j++] = fmt;
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_s_power(struct v4l2_subdev *csi2dc_sd, int on)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> + int ret = 0;
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + if (on)
>>>>> + ret = clk_prepare_enable(csi2dc->scck);
>>>>> + else
>>>>> + clk_disable_unprepare(csi2dc->scck);
>>>>> + if (ret)
>>>>> + dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret);
>>>>> +
>>>>> + /* if powering up, deassert reset line */
>>>>> + if (on)
>>>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);
>>>>> +
>>>>> + ret = v4l2_subdev_call(csi2dc->input_sd, core, s_power, on);
>>>>> +
>>>>> + /* if powering down, assert reset line */
>>>>> + if (!on)
>>>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST);
>>>>
>>>> These go to the runtime resume and suspend callbacks.
>>>
>>> Okay I will update
>
> Another question is what happens when CONFIG_PM is not used, thus we do
> not have runtime resume and suspend callbacks ?
>
>>>
>>>>
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_g_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_frame_interval *interval)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, video, g_frame_interval,
>>>>> + interval);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_s_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_frame_interval *interval)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_frame_interval,
>>>>> + interval);
>>>>
>>>> If your driver is used with a MC-enabled master driver, you can drop all
>>>> the v4l2_subdev_call() macros apart from the ones related to s_stream
>>>> operation.
>>>
>>> And the set frame interval for example, will it be sent to the sensor
>>> automatically ?
>>> I noticed that such operations on the master driver were not propagated
>>> to the underlying subdevice. How does the master device 'know' how to
>>> call the input_sd ? this input_sd is not referenced anywhere in the
>>> master driver.
>>
>> That's correct. In MC-centric drivers it's the responsibility of the user
>> space to configure the entire pipeline.
>>
>> As your device is a CSI-2 to parallel (?) converter, it may well be part of
>> a complex pipeline that the master driver could not know about.
>>
>> I remember there was an existing stm parallel receiver that was tightly
>> coupled by a CSI-2 to parallel IP block where the V4L2-centric model was
>> used. But virtually all devices that are not just simple sensor + receiver
>> with DMA are MC-centric.
>
> Without these calls to the subdevice, the subdevice (csi2host and
> sensor) are not aware of anything configured on the master driver. How
> are such settings sent to the sensor ?
>
> for example v4l2-ctl --list-formats-ext
>
>
> # v4l2-ctl -d /dev/video0 --list-formats-ext
> ioctl: VIDIOC_ENUM_FMT
> Type: Video Capture
>
> [0]: 'AR12' (16-bit ARGB 4-4-4-4)
> Size: Discrete 3280x2464
> Size: Discrete 1920x1080
> Size: Discrete 1640x1232
> [1]: 'AR15' (16-bit ARGB 1-5-5-5)
> Size: Discrete 3280x2464
> Size: Discrete 1920x1080
> Size: Discrete 1640x1232
> [2]: 'RGBP' (16-bit RGB 5-6-5)
> Size: Discrete 3280x2464
> Size: Discrete 1920x1080
> Size: Discrete 1640x1232
>
> All this information is obtained because of the enum_frame_size callback
> to the subdevice
>
> If I remove these,
>
> # v4l2-ctl -d /dev/video0 --list-formats-ext
> ioctl: VIDIOC_ENUM_FMT
> Type: Video Capture
>
> [0]: 'AR12' (16-bit ARGB 4-4-4-4)
> [1]: 'AR15' (16-bit ARGB 1-5-5-5)
> [2]: 'RGBP' (16-bit RGB 5-6-5)
> [3]: 'AR24' (32-bit BGRA 8-8-8-8)
> [4]: 'XR24' (32-bit BGRX 8-8-8-8)
> [5]: 'YU12' (Planar YUV 4:2:0)
> [6]: 'UYVY' (UYVY 4:2:2)
> [7]: 'VYUY' (VYUY 4:2:2)
> [8]: 'YUYV' (YUYV 4:2:2)
> [9]: '422P' (Planar YUV 4:2:2)
> [10]: 'GREY' (8-bit Greyscale)
> [11]: 'Y10 ' (10-bit Greyscale)
> [12]: 'Y16 ' (16-bit Greyscale)
> [13]: 'RG10' (10-bit Bayer RGRG/GBGB)
>
>
> All the frame information is no longer available.
>
> So how can I obtain this information if I remove these callbacks that
> you say they should be removed ?
>
>>
>>>
>>>>
>>>> If not, then you can drop the HAS_DEVNODE flag from the subdev flags. But
>>>> I'd try to avoid this route if possible.
>>>>
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_enum_frame_size(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_pad_config *cfg,
>>>>> + struct v4l2_subdev_frame_size_enum *fse)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_size, cfg,
>>>>> + fse);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_enum_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_pad_config *cfg,
>>>>> + struct v4l2_subdev_frame_interval_enum *fie)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_interval, cfg,
>>>>> + fie);
>>>>> +}
>>>>> +
>>>>> +static const struct v4l2_subdev_core_ops csi2dc_core_ops = {
>>>>> + .s_power = csi2dc_s_power,
>>>>
>>>> Please use runtime PM instead.
>
> What happens if we do not have runtime PM ? (CONFIG_PM=n)
>>>>
>>>>> +};
>>>>> +
>>>>> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {
>>>>> + .enum_mbus_code = csi2dc_enum_mbus_code,
>>>>> + .set_fmt = csi2dc_set_fmt,
>>>>> + .enum_frame_size = csi2dc_enum_frame_size,
>>>>> + .enum_frame_interval = csi2dc_enum_frame_interval,
>>>>> +};
>>>>> +
>>>>> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = {
>>>>> + .s_stream = csi2dc_s_stream,
>>>>> + .g_frame_interval = csi2dc_g_frame_interval,
>>>>> + .s_frame_interval = csi2dc_s_frame_interval,
>>>>> +};
>>>>> +
>>>>> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = {
>>>>> + .core = &csi2dc_core_ops,
>>>>> + .pad = &csi2dc_pad_ops,
>>>>> + .video = &csi2dc_video_ops,
>>>>> +};
>>>>> +
>>>>> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,
>>>>> + struct v4l2_subdev *subdev,
>>>>> + struct v4l2_async_subdev *asd)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = container_of(notifier->v4l2_dev,
>>>>> + struct csi2dc_device, v4l2_dev);
>>>>> + csi2dc->input_sd = subdev;
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_async_complete(struct v4l2_async_notifier *notifier)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc =
>>>>> + container_of(notifier->v4l2_dev, struct csi2dc_device, v4l2_dev);
>>>>
>>>> Over 80 characters per line. Please check the rest of the driver, too.
>>>>
>>>>> + int ret;
>>>>> +
>>>>> + ret = v4l2_device_register_subdev_nodes(&csi2dc->v4l2_dev);
>>>>> + if (ret < 0) {
>>>>> + v4l2_err(&csi2dc->v4l2_dev, "failed to register subdev nodes\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + csi2dc_writel(csi2dc, CSI2DC_GCFG,
>>>>> + (CSI2DC_GCFG_HLC(csi2dc->inter_line_delay) &
>>>>> + CSI2DC_GCFG_HLC_MASK) |
>>>>> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));
>>>>> +
>>>>> + csi2dc_writel(csi2dc, CSI2DC_VPCOL,
>>>>> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK);
>>>>> + csi2dc_writel(csi2dc, CSI2DC_VPROW,
>>>>> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK);
>>>>> +
>>>>> + csi2dc->completed = true;
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = {
>>>>> + .bound = csi2dc_async_bound,
>>>>> + .complete = csi2dc_async_complete,
>>>>> +};
>>>>> +
>>>>> +static void csi2dc_cleanup_notifier(struct csi2dc_device *csi2dc)
>>>>> +{
>>>>> + v4l2_async_notifier_unregister(&csi2dc->notifier);
>>>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,
>>>>> + struct device_node *input_parent)
>>>>> +{
>>>>> + int ret;
>>>>> +
>>>>> + v4l2_async_notifier_init(&csi2dc->notifier);
>>>>> +
>>>>> + csi2dc->asd = kzalloc(sizeof(*csi2dc->asd), GFP_KERNEL);
>>>>> + if (!csi2dc->asd)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + csi2dc->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
>>>>> + csi2dc->asd->match.fwnode = of_fwnode_handle(input_parent);
>>>>> +
>>>>> + ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd);
>>>>> + if (ret) {
>>>>> + dev_err(csi2dc->dev, "failed to add async notifier.\n");
>>>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
>>>>> + goto csi2dc_prepare_notifier_err;
>>>>> + }
>>>>> +
>>>>> + csi2dc->notifier.ops = &csi2dc_async_ops;
>>>>> +
>>>>> + ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev,
>>>>> + &csi2dc->notifier);
>>>>> +
>>>>> + if (ret) {
>>>>> + dev_err(csi2dc->dev, "fail to register async notifier.\n");
>>>>> + goto csi2dc_prepare_notifier_err;
>>>>> + }
>>>>> +
>>>>> +csi2dc_prepare_notifier_err:
>>>>> + of_node_put(input_parent);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc,
>>>>> + struct device_node *of_node)
>>>>> +{
>>>>> + struct device_node *input_node, *sink_node, *input_parent;
>>>>> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, sink_endpoint = { 0 };
>>>>> + int ret;
>>>>> +
>>>>> + input_node = of_graph_get_next_endpoint(of_node, NULL);
>>>>> +
>>>>> + if (!input_node) {
>>>>> + dev_err(csi2dc->dev, "missing port node at %s, input node is mandatory.\n",
>>>>> + of_node->full_name);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node),
>>>>> + &input_endpoint);
>>>>> +
>>>>> + if (ret) {
>>>>> + dev_err(csi2dc->dev, "endpoint not defined at %s\n",
>>>>> + of_node->full_name);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + input_parent = of_graph_get_remote_port_parent(input_node);
>>>>> + if (!input_parent) {
>>>>> + dev_err(csi2dc->dev, "could not get input node's parent node.\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + sink_node = of_graph_get_next_endpoint(of_node, input_node);
>>>>> +
>>>>> + if (sink_node)
>>>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(sink_node),
>>>>> + &sink_endpoint);
>>>>> +
>>>>> + if (!sink_node || ret) {
>>>>> + dev_info(csi2dc->dev, "missing sink node at %s, data pipe available only.\n",
>>>>> + of_node->full_name);
>>>>> + } else {
>>>>> + csi2dc->video_pipe = true;
>>>>> + csi2dc->vc = sink_endpoint.base.id;
>>>>> +
>>>>> + dev_dbg(csi2dc->dev, "block %s %d.%d->%d.%d video pipe VC %d\n",
>>>>> + of_node->full_name, input_endpoint.base.port,
>>>>> + input_endpoint.base.id, sink_endpoint.base.port,
>>>>> + sink_endpoint.base.id, csi2dc->vc);
>>>>> + }
>>>>> +
>>>>> + csi2dc->clk_gated = of_property_read_bool(of_node,
>>>>> + "microchip,clk-gated");
>>>>> +
>>>>> + dev_dbg(csi2dc->dev, "%s clock\n",
>>>>> + csi2dc->clk_gated ? "Gated" : "Free running");
>>>>> +
>>>>> + ret = of_property_read_u32(of_node, "microchip,inter-line-delay",
>>>>> + &csi2dc->inter_line_delay);
>>>>> +
>>>>> + if (ret || csi2dc->inter_line_delay > 16 ||
>>>>> + csi2dc->inter_line_delay == 0) {
>>>>> + dev_dbg(csi2dc->dev, "assuming inter line delay = 16 clocks");
>>>>> + csi2dc->inter_line_delay = 16;
>>>>> + ret = 0;
>>>>> + }
>>>>> + /* hardware automatically adds 1 */
>>>>> + csi2dc->inter_line_delay--;
>>>>> +
>>>>> + /* prepare async notifier for subdevice completion */
>>>>> +
>>>>> + of_node_put(sink_node);
>>>>> + of_node_put(input_node);
>>>>> +
>>>>> + return csi2dc_prepare_notifier(csi2dc, input_parent);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_probe(struct platform_device *pdev)
>>>>> +{
>>>>> + struct device *dev = &pdev->dev;
>>>>> + struct csi2dc_device *csi2dc;
>>>>> + struct resource *res = NULL;
>>>>> + int ret = 0;
>>>>> +
>>>>> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);
>>>>> + if (!csi2dc)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + csi2dc->dev = dev;
>>>>> +
>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> + if (!res)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + csi2dc->base = devm_ioremap_resource(dev, res);
>>>>> +
>>>>> + if (IS_ERR(csi2dc->base)) {
>>>>> + dev_err(dev, "base address not set\n");
>>>>> + return PTR_ERR(csi2dc->base);
>>>>> + }
>>>>> +
>>>>> + csi2dc->pclk = devm_clk_get(dev, "pclk");
>>>>> + if (IS_ERR(csi2dc->pclk)) {
>>>>> + ret = PTR_ERR(csi2dc->pclk);
>>>>> + dev_err(dev, "failed to get pclk: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + ret = clk_prepare_enable(csi2dc->pclk);
>>>>> + if (ret) {
>>>>> + dev_err(dev, "failed to enable pclk: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + csi2dc->scck = devm_clk_get(dev, "scck");
>>>>> + if (IS_ERR(csi2dc->scck)) {
>>>>> + ret = PTR_ERR(csi2dc->scck);
>>>>> + dev_err(dev, "failed to get scck: %d\n", ret);
>>>>> + goto csi2dc_clk_fail;
>>>>> + }
>>>>> +
>>>>> + ret = v4l2_device_register(dev, &csi2dc->v4l2_dev);
>>>>> + if (ret) {
>>>>> + dev_err(dev, "unable to register v4l2 device.\n");
>>>>> + goto csi2dc_clk_fail;
>>>>> + }
>>>>> +
>>>>> + v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);
>>>>> +
>>>>> + csi2dc->csi2dc_sd.owner = THIS_MODULE;
>>>>> + csi2dc->csi2dc_sd.dev = dev;
>>>>> + snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),
>>>>> + "CSI2DC.0");
>>>>> +
>>>>> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>>>> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>>>>> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>>>>> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
>>>>> +
>>>>> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, CSI2DC_PADS_NUM,
>>>>> + csi2dc->pads);
>>>>> + if (ret < 0) {
>>>>> + dev_err(dev, "media entity init failed\n");
>>>>> + goto csi2dc_probe_entity_err;
>>>>> + }
>>>>> +
>>>>> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev);
>>>>> +
>>>>> + platform_set_drvdata(pdev, &csi2dc->csi2dc_sd);
>>>>> +
>>>>> + ret = csi2dc_of_parse(csi2dc, dev->of_node);
>>>>> + if (ret)
>>>>> + goto csi2dc_probe_entity_err;
>>>>> +
>>>>> + ret = csi2dc_formats_init(csi2dc);
>>>>> + if (ret)
>>>>> + goto csi2dc_probe_error;
>>>>> +
>>>>> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);
>>>>> + if (ret)
>>>>> + goto csi2dc_probe_error;
>>>>> +
>>>>> + pr_info("Microchip CSI2DC version %x\n",
>>>>> + csi2dc_readl(csi2dc, CSI2DC_VERSION));
>>>>
>>>> dev_info(), please. Or just dev_dbg().
>>>>
>>>>> +
>>>>> + pm_runtime_set_active(dev);
>>>>> + pm_runtime_enable(dev);
>>>>> + pm_request_idle(dev);
>>>>> +
>>>>> + return 0;
>>>>> +
>>>>> +csi2dc_probe_error:
>>>>> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);
>>>>> + csi2dc_cleanup_notifier(csi2dc);
>>>>> +
>>>>> +csi2dc_probe_entity_err:
>>>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>>>>> + v4l2_device_unregister(&csi2dc->v4l2_dev);
>>>>> +csi2dc_clk_fail:
>>>>> + clk_disable_unprepare(csi2dc->pclk);
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_remove(struct platform_device *pdev)
>>>>> +{
>>>>> + struct v4l2_subdev *csi2dc_sd = platform_get_drvdata(pdev);
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + pm_runtime_disable(&pdev->dev);
>>>>> + csi2dc_cleanup_notifier(csi2dc);
>>>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>>>>> + v4l2_device_unregister(&csi2dc->v4l2_dev);
>>>>> + clk_disable_unprepare(csi2dc->pclk);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)
>>>>> +{
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev)
>>>>> +{
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {
>>>>> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)
>>>>> +};
>>>>> +
>>>>> +static const struct of_device_id csi2dc_of_match[] = {
>>>>> + { .compatible = "microchip,sama7g5-csi2dc" },
>>>>> + { }
>>>>> +};
>>>>> +
>>>>> +MODULE_DEVICE_TABLE(of, csi2dc_of_match);
>>>>> +
>>>>> +static struct platform_driver csi2dc_driver = {
>>>>> + .probe = csi2dc_probe,
>>>>> + .remove = csi2dc_remove,
>>>>> + .driver = {
>>>>> + .name = "microchip-csi2dc",
>>>>> + .pm = &csi2dc_dev_pm_ops,
>>>>> + .of_match_table = of_match_ptr(csi2dc_of_match),
>>>>> + },
>>>>> +};
>>>>> +
>>>>> +module_platform_driver(csi2dc_driver);
>>>>> +
>>>>> +MODULE_AUTHOR("Eugen Hristev <[email protected]>");
>>>>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>> +MODULE_SUPPORTED_DEVICE("video");
>>>>
>>>> Hmm. This line needs to be more specific. I'd just drop it.
>>>>
>>>> --
>>>> Kind regards,
>>>>
>>>> Sakari Ailus
>>>>
>>>
>>
>> --
>> Regards,
>>
>> Sakari Ailus
>>
>
On 12.10.2020 16:04, Jacopo Mondi wrote:
> Hello,
> just my 2 cents, as I've noticed this patch skimming through the
> list
>
> On Mon, Oct 12, 2020 at 07:19:43AM +0000, [email protected] wrote:
>> On 11.10.2020 00:17, Laurent Pinchart wrote:
>>> Hi Eugen,
>>>
>>> Thank you for the patch.
>>
>> Hi,
>>
>> Thank you for your review,
>>
>>>
>>> On Wed, Aug 26, 2020 at 09:51:40AM +0300, Eugen Hristev wrote:
>>>> Add bindings documentation for Microchip CSI2 Demultiplexer controller.
>>>>
>>>> CSI2DC is a demultiplexer from Synopsys IDI interface specification to
>>>> parallel interface connection or direct memory access.
>>>>
>>>> Signed-off-by: Eugen Hristev <[email protected]>
>>>> ---
>>>> Changes in v3:
>>>> - Removed some text from description, as it was explained in the schema
>>>> - fixed other things as per Rob's review
>>>> - moved some text inside the schema, like the clock description
>>>>
>>>> Changes in v2:
>>>> - fixed warnings reported by dt_binding_check
>>>>
>>>> .../bindings/media/microchip,csi2dc.yaml | 174 ++++++++++++++++++
>>>> 1 file changed, 174 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml b/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
>>>> new file mode 100644
>>>> index 000000000000..b4c1b8800a3b
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
>>>> @@ -0,0 +1,174 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/media/microchip,csi2dc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Microchip CSI2 Demux Controller (CSI2DC)
>>>> +
>>>> +maintainers:
>>>> + - Eugen Hristev <[email protected]>
>>>> +
>>>> +description:
>>>> + CSI2DC - Camera Serial Interface 2 Demux Controller
>>>> +
>>>> + CSI2DC is a hardware block that receives incoming data from an IDI interface
>>>> + and filters packets based on their data type and virtual channel identifier,
>>>> + then converts the byte stream into a cross clock domain to a pixel stream
>>>> + to a parallel interface that can be read by a sensor controller.
>>>> +
>>>> + CSI2DC provides two pipes, one video pipe and one data pipe. Video pipe
>>>> + is connected to a sensor controller and the data pipe is accessible
>>>> + as a DMA slave port to a DMA controller.
>>>> +
>>>> + CSI2DC supports a single 'port' node as a source pad with Synopsys 32-bit
>>>> + IDI interface. The connected endpoint must be a IDI interface compatible
>>>> + device (like Synopsys CSI2HOST) , that can provide 32-bit IDI interface
>>>> + connection as sink pad.
>>>> + For media entity and endpoints please refer to the bindings defined in
>>>> + Documentation/devicetree/bindings/media/video-interfaces.txt.
>>>> + For Synopsys IDI interface please refer to
>>>> + Documentation/devicetree/bindings/media/snps,dw-csi-plat.txt
>>>> +
>>>> + CSI2DC supports one 'port' node as sink pad with parallel interface. This is
>>>> + called video pipe.
>>>> + This port has an 'endpoint' can then be used as a source pad for another
>>>> + controller (next in pipeline).
>>>> + Please refer to the bindings defined in
>>>> + Documentation/devicetree/bindings/media/video-interfaces.txt.
>>>> +
>>>> + CSI2DC also supports direct access to the data through AHB, via DMA channel,
>>>> + called data pipe.
>>>> + Because of this, the sink 'port' child node (second) is not mandatory.
>>>> + If the sink 'port' child node is missing, only data pipe is available.
>>>> +
>>>> +properties:
>>>> + compatible:
>>>> + const: microchip,sama7g5-csi2dc
>>>> +
>>>> + reg:
>>>> + maxItems: 1
>>>> +
>>>> + clocks:
>>>> + maxItems: 2
>>>> +
>>>> + clock-names:
>>>> + description:
>>>> + CSI2DC must have two clocks to function correctly. One clock is the
>>>> + peripheral clock for the inside functionality of the hardware block.
>>>> + This is named 'pclk'. The second clock must be the cross domain clock,
>>>> + in which CSI2DC will perform clock crossing. This clock must be fed
>>>> + by the next controller in pipeline, which usually is a sensor controller.
>>>> + Normally this clock should be given by this sensor controller who
>>>> + is also a clock source. This clock is named 'scck', sensor controller clock.
>>>> + items:
>>>> + - const: pclk
>>>> + - const: scck
>>>> +
>>>> + microchip,clk-gated:
>>>> + type: boolean
>>>> + description:
>>>> + If present, indicates that the clock is gated.
>>>> + Otherwise, the clock is free-running.
>>>
>>> I don't think this belongs to the DT bindings, it should instead be
>>> queried from the source subdev at runtime.
>>
>> If this should be queried, do you know what is the v4l2 mechanism to
>> query such information ?
>> The subdevice is connected through a port interface to this device, so
>> it was natural for me to fully describe the interface in the devicetree
>> port description
>>
Hi,
Thanks for helping,
>
> Is this property describing the CSI-2 clock continuous, non-continuous
> mode configuration, or did I mis-interpreted it ?
I think so. This is a setting inside the csi2dc regarding clock. If we
can obtain it from pads operations, then it's good, but the question is,
if the devices can provide this or not ?
>
> We added support for retrieving run-time configuration of the media
> bus with the get_mbus_config pad operations recently. Among the
> configuration flags for MBUS_CSI2_DPHY there are inded CONTINUOUS and
> NON_CONTINUOUS clock flags.
>
>>>
>>>> +
>>>> + microchip,inter-line-delay:
>>>> + allOf:
>>>> + - $ref: /schemas/types.yaml#/definitions/uint32
>>>> + - minimum: 1
>>>> + - maximum: 16
>>>> + default: 16
>>>> + description:
>>>> + Indicates how many clock cycles should be introduced between each line.
>>>
>>> This also sounds like a configuration parameter. How does one compute
>>> the right value for this ?
>>
>> I think this is a delay that can be added inside the hardware block,
>> depending on the interface speed and bandwidth. I will try to understand
>> more details from the hardware design and come back with a more detailed
>> answer.
>>
Regarding this, I will remove it. Our design team advised to have a
hardcoded value for this product.
>>>
>>>> +
>>>> + port@0:
>>>> + type: object
>>>> + description:
>>>> + Input port node, single endpoint describing the input pad.
>>>> +
>>>> + properties:
>>>> + reg:
>>>> + const: 0
>>>> +
>>>> + endpoint:
>>>> + type: object
>>>> +
>>>> + properties:
>>>> + remote-endpoint: true
>>>> +
>>>> + required:
>>>> + - remote-endpoint
>>>> +
>>>> + additionalProperties: false
>>>> +
>>>> + additionalProperties: false
>>>> +
>>>> + port@1:
>>>> + type: object
>>>> + description:
>>>> + Output port node, single endpoint, describing the output pad.
>>>> +
>>>> + properties:
>>>> + '#address-cells':
>>>> + const: 1
>>>> +
>>>> + '#size-cells':
>>>> + const: 0
>>>> +
>>>> + reg:
>>>> + const: 1
>>>> +
>>>> + patternProperties:
>>>> + "^endpoint@[0-3]$":
>>>> + type: object
>>>> +
>>>> + properties:
>>>> + reg:
>>>> + enum: [0, 1, 2, 3]
>>>> + description: virtual channel for the endpoint
>>>
>>> The virtual channel used by the source is also something that needs to
>>> be queried from the source at runtime, it doesn't belong to this
>>> binding.
>>
>> The same question as for the gated clock configuration. How can we use
>> v4l2 subdevice API to obtain such information from the subdevice? And if
>> the subdevice does not offer such information ?
>
> I think the subdev driver should be instrumented to report it instead of
> hard-coding the information in DT which should be otherwise updated
> depending on which sensor is connected to the board. Does it make
> sense to you ?
It does, but then, it won't work unless connected to instrumented
subdevices. Which is not really something I would do, since it would
completely limit the usability.
Do you have any example on how to get the virtual id from the subdev ?
Thanks again,
Eugen
>
> Cheers
> j
>
>>
>> Thanks again,
>>
>> Eugen
>>>
>>>> +
>>>> + remote-endpoint: true
>>>> +
>>>> + required:
>>>> + - remote-endpoint
>>>> + - reg
>>>> +
>>>> + additionalProperties: false
>>>> +
>>>> + additionalProperties: false
>>>> +
>>>> +required:
>>>> + - compatible
>>>> + - reg
>>>> + - clocks
>>>> + - clock-names
>>>> + - port@0
>>>> +
>>>> +examples:
>>>> + - |
>>>> + csi2dc@e1404000 {
>>>> + compatible = "microchip,sama7g5-csi2dc";
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> + reg = <0xe1404000 0x500>;
>>>> + clocks = <&pclk>, <&scck>;
>>>> + clock-names = "pclk", "scck";
>>>> +
>>>> + port@0 {
>>>> + reg = <0>; /* must be 0, first child port */
>>>> + csi2dc_in: endpoint { /* input from IDI interface */
>>>> + remote-endpoint = <&csi2host_out>;
>>>> + };
>>>> + };
>>>> +
>>>> + port@1 {
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> + reg = <1>; /* must be 1, second child port */
>>>> + csi2dc_out: endpoint@2 {
>>>> + reg = <2>; /* virtual channel identifier */
>>>> + remote-endpoint = <&xisc_in>; /* output to sensor controller */
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> +...
>>>
>>> --
>>> Regards,
>>>
>>> Laurent Pinchart
>>>
>>
Hello Eugen,
On Fri, Oct 16, 2020 at 12:30:59PM +0000, [email protected] wrote:
> On 12.10.2020 16:04, Jacopo Mondi wrote:
> > Hello,
> > just my 2 cents, as I've noticed this patch skimming through the
> > list
> >
> > On Mon, Oct 12, 2020 at 07:19:43AM +0000, [email protected] wrote:
[snip]
>
> Hi,
>
> Thanks for helping,
> >
> > Is this property describing the CSI-2 clock continuous, non-continuous
> > mode configuration, or did I mis-interpreted it ?
>
> I think so. This is a setting inside the csi2dc regarding clock. If we
> can obtain it from pads operations, then it's good, but the question is,
> if the devices can provide this or not ?
The transmitter can provide this information, as it knows which clock
mode it is going to use (it's not clear from the CSI-2 spec which side
is in charge of the selection, but it seems natural to me that it's up
to the transmitter side). Whether they implement the right operation
to do so, well... see below
> >
> > We added support for retrieving run-time configuration of the media
> > bus with the get_mbus_config pad operations recently. Among the
> > configuration flags for MBUS_CSI2_DPHY there are inded CONTINUOUS and
> > NON_CONTINUOUS clock flags.
> >
> >>>
> >>>> +
> >>>> + microchip,inter-line-delay:
> >>>> + allOf:
> >>>> + - $ref: /schemas/types.yaml#/definitions/uint32
> >>>> + - minimum: 1
> >>>> + - maximum: 16
> >>>> + default: 16
> >>>> + description:
> >>>> + Indicates how many clock cycles should be introduced between each line.
> >>>
> >>> This also sounds like a configuration parameter. How does one compute
> >>> the right value for this ?
> >>
> >> I think this is a delay that can be added inside the hardware block,
> >> depending on the interface speed and bandwidth. I will try to understand
> >> more details from the hardware design and come back with a more detailed
> >> answer.
> >>
>
> Regarding this, I will remove it. Our design team advised to have a
> hardcoded value for this product.
>
> >>>
> >>>> +
> >>>> +
> >>>> + properties:
> >>>> + reg:
> >>>> + enum: [0, 1, 2, 3]
> >>>> + description: virtual channel for the endpoint
> >>>
> >>> The virtual channel used by the source is also something that needs to
> >>> be queried from the source at runtime, it doesn't belong to this
> >>> binding.
> >>
> >> The same question as for the gated clock configuration. How can we use
> >> v4l2 subdevice API to obtain such information from the subdevice? And if
> >> the subdevice does not offer such information ?
> >
> > I think the subdev driver should be instrumented to report it instead of
> > hard-coding the information in DT which should be otherwise updated
> > depending on which sensor is connected to the board. Does it make
> > sense to you ?
>
> It does, but then, it won't work unless connected to instrumented
> subdevices. Which is not really something I would do, since it would
> completely limit the usability.
> Do you have any example on how to get the virtual id from the subdev ?
As examples of CSI-2 transmitters implementing get_mbus_config() we
have tc358743 and adv748x reporting the number of active data lanes.
Reporting the virtual channel in use is a matter of using one of the
following flags:
/* CSI-2 Virtual Channel identifiers. */
#define V4L2_MBUS_CSI2_CHANNEL_0 BIT(4)
#define V4L2_MBUS_CSI2_CHANNEL_1 BIT(5)
#define V4L2_MBUS_CSI2_CHANNEL_2 BIT(6)
#define V4L2_MBUS_CSI2_CHANNEL_3 BIT(7)
As an example of a receiver driver rcar-csi2 uses the operation to
dynamically negotiate the number of data lanes. To handle virtual
channel you would need to inspect which of the above flags have been
set by the subdevice.
On deciding if this better handled by using a subdev operation or a DT
property, I understand the subdev driver needs to implement
get_mbus_config, but hardcoding it in DT I fear is a no-go for
mainline and honestly it doesn't sound much more advantageous, as it
needs to be adapted according to the connected sensor anyway, doesn't
it ? But I'm happy to defer this call to the maintainers, I hope I
just have provided some useful references.
Cheers
j
>
> Thanks again,
>
> Eugen
> >
> > Cheers
> > j
> >
> >>
> >> Thanks again,
> >>
> >> Eugen
> >>>
> >>>> +
> >>>> + remote-endpoint: true
> >>>> +
> >>>> + required:
> >>>> + - remote-endpoint
> >>>> + - reg
> >>>> +
> >>>> + additionalProperties: false
> >>>> +
> >>>> + additionalProperties: false
> >>>> +
> >>>> +required:
> >>>> + - compatible
> >>>> + - reg
> >>>> + - clocks
> >>>> + - clock-names
> >>>> + - port@0
> >>>> +
> >>>> +examples:
> >>>> + - |
> >>>> + csi2dc@e1404000 {
> >>>> + compatible = "microchip,sama7g5-csi2dc";
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> + reg = <0xe1404000 0x500>;
> >>>> + clocks = <&pclk>, <&scck>;
> >>>> + clock-names = "pclk", "scck";
> >>>> +
> >>>> + port@0 {
> >>>> + reg = <0>; /* must be 0, first child port */
> >>>> + csi2dc_in: endpoint { /* input from IDI interface */
> >>>> + remote-endpoint = <&csi2host_out>;
> >>>> + };
> >>>> + };
> >>>> +
> >>>> + port@1 {
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> + reg = <1>; /* must be 1, second child port */
> >>>> + csi2dc_out: endpoint@2 {
> >>>> + reg = <2>; /* virtual channel identifier */
> >>>> + remote-endpoint = <&xisc_in>; /* output to sensor controller */
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> +
> >>>> +...
> >>>
> >>> --
> >>> Regards,
> >>>
> >>> Laurent Pinchart
> >>>
> >>
>