dt-bindings: media: atmel: csi2dc: add bindings for microchip csi2dc
Subsystems (4)
PATCH v5 1/3 dt-bindings: media: atmel: csi2dc: add bindings for microchip csi2dc
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.
---
Changes in v5:
- modified bindings as per Rob Herring review
Changes in v4:
- Removed property for inter-line-delay and for clock continuous/non-continuous
- Removed virtual channel by reg for second endpoint
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 | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/microchip,csi2dc.yaml
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +119 -0
@@ -0,0 +1,119 @@ +# 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 + + ports: + type: object + description: + List of ports + + properties: + port@0: + type: object + description: + Input port node, single endpoint describing the input pad. + port@1: + type: object + description: + Output port node, single endpoint, describing the output pad. + +additionalProperties: false + +required: + - compatible + - reg + - clocks + - clock-names + - ports + +examples: + - | + csi2dc@e1404000 { + compatible = "microchip,sama7g5-csi2dc"; + reg = <0xe1404000 0x500>; + clocks = <&pclk>, <&scck>; + clock-names = "pclk", "scck"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; /* must be 0, first child port */ + csi2dc_in: endpoint { /* input from IDI interface */ + remote-endpoint = <&csi2host_out>; + }; + }; + + port@1 { + reg = <1>; /* must be 1, second child port */ + csi2dc_out: endpoint { + remote-endpoint = <&xisc_in>; /* output to sensor controller */ + }; + }; + }; + }; + +...
Signature · 1 line
2.25.1
PATCH v5 2/3 media: atmel: introduce microchip csi2dc driver
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.
---
Hello,
There still are some open questions regarding the last reviews on the ML.
Regarding the format list which is not const, I cannot have it const because I reference elements from this list into a dynamic list which is non-const.
Regarding the presence of the v4l2_dev, without this I cannot add a notifier for this device to have it async completed , when the underlying subdevice finishes probing.
Regarding the callbacks for set frame interval, set frame size, etc, I cannot remove them because losing functionality to the underlying subdevice, as explained in :
https://lkml.org/lkml/2020/10/12/427
Changes in v5:
- only in bindings
Changes in v4:
- now using get_mbus_config ops to get data from the subdevice, like the virtual channel id, and the clock type.
- now having possibility to select any of the RAW10 data modes
- at completion time, select which formats are also available in the subdevice, and move to the dynamic list accordingly
- changed the pipeline integration, do not advertise subdev ready at probe time. wait until completion is done, and then start a workqueue that will register this device as a subdevice for the next element in pipeline.
- moved the s_power code into a different function called now csi2dc_power that is called with CONFIG_PM functions. This is also called at completion, to have the device ready in case CONFIG_PM is not selected on the platform.
- merged try_fmt into set_fmt
- driver cleanup, wrapped lines over 80 characters
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 | 785 ++++++++++++++++++ 3 files changed, 799 insertions(+) create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.c
drivers/media/platform/atmel/Kconfig +13 -0
@@ -32,3 +32,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.
drivers/media/platform/atmel/Makefile +1 -0
@@ -5,3 +5,4 @@ atmel-xisc-objs = atmel-sama7g5-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_ATMEL_XISC) += atmel-xisc.o +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o
drivers/media/platform/atmel/microchip-csi2dc.c new file +785 -0
785 lines not on this page · raw message
Signature · 1 line
2.25.1
Re: [PATCH v5 2/3] media: atmel: introduce microchip csi2dc driver
Hi Eugen,
Quoted 8 lines · Eugen Hristev
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]>---
Hello,
Quoted 9 lines
There still are some open questions regarding the last reviews on the ML.
Regarding the format list which is not const, I cannot have it const because I reference elements from this list into a dynamic list which is non-const.
Regarding the presence of the
v4l2_dev, without this I cannot add a notifier for this device to have it async completed , when the underlying subdevice finishes probing.
I see you have a discussion with Sakari on-going on this, and I have the very same comment he had.
Should a sub-notifier be used and this driver should not register any v4l2_dev but integrate in the media graph of the receiver ?
For reference, rcar-csi2 uses sub-notifiers and register in the rcar-vin media graph, it doesn't need to set up a v4l2_dev.
rcar-vin (the DMA engine driver) that register video nodes and the v4l2-dev also registers the device nodes for all the connected subdevices.
Quoted 5 lines
Regarding the callbacks for set frame interval, set frame size, etc, I cannot remove them because losing functionality to the underlying subdevice, as explained in :
https://lkml.org/lkml/2020/10/12/427
Using the frame sizes as in the example you reported, if this device has limitations on the supported sizes, (a min and a max most probably) that's the only thing that should be exposed from this driver subdevice. The sensor subdevice instead will report the available discrete sizes and userspace configures the sensor's source pad and the bridge's sink pad with the same sizes to properly set up the capture pipeline. video0 should not report the sizes exposed by the sensor nor any information that depends on what's connected to it.
I know what your next question is: what about existing application that only know about video0 and want to setup everything from there. I understand vendors not wanting to wrap gstreamer or what is used in the wild in custom scripts that sets up the pipeline opportunely before streaming, and I'm pretty sure you know what is the solution I would propose for this :)
I've just gone through the same path with RPi's Unicam, that does the same thing as your one does: it's an MC driver, but wants to control the sensor through the v4l2-subdev kAPI to support legacy use cases not covered by libcamera. The fact is that we're stuck in this limbo where MC pipeline tries to decouple components one from each other and delegate their configuration to userspace, but at the same time vendors want to be able to use the simple plain video node centric interface because of their existing user base, so I understand your discomfort, but the 'simple plain' approach is what's preventing Linux platforms to be in-par with proprietary and custom solutions for any use case that is not a simple frame capture, so I think it's fair for mainline to push for this evolution to happen. Just my2c of course.
Some more comments on the implementation below
Quoted 40 lines
Changes in v5:
- only in bindingsChanges in v4:
- now usingget_mbus_configops to get data from the subdevice, like the virtual channel id, and the clock type.
- now having possibility to select any of the RAW10 data modes
- at completion time, select which formats are also available in the subdevice, and move to the dynamic list accordingly
- changed the pipeline integration, do not advertise subdev ready at probe time. wait until completion is done, and then start a workqueue that will register this device as a subdevice for the next element in pipeline.
- moved thes_powercode into a different function called nowcsi2dc_powerthat is called withCONFIG_PMfunctions. This is also called at completion, to have the device ready in caseCONFIG_PMis not selected on the platform.
- mergedtry_fmtintoset_fmt
- driver cleanup, wrapped lines over 80 charactersChanges 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 updrivers/media/platform/atmel/Kconfig | 13 + drivers/media/platform/atmel/Makefile | 1 + .../media/platform/atmel/microchip-csi2dc.c | 785 ++++++++++++++++++ 3 files changed, 799 insertions(+) create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.cdrivers/media/platform/atmel/Kconfig +3
-0@@ -32,3 +32,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"
drivers/media/platform/atmel/Kconfig config VIDEO_ATMEL_ISI · line 47
Quoted 10 lines
drivers/media/platform/atmel/Kconfig +10
-0+ 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.
Is the last phrase necessary ? Also missing the usual
To compile this driver as a module, choose M here: the module will be called ...
Quoted 15 lines
drivers/media/platform/atmel/Makefile +1
-0@@ -5,3 +5,4 @@ atmel-xisc-objs = atmel-sama7g5-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_ATMEL_XISC) += atmel-xisc.o +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.odrivers/media/platform/atmel/microchip-csi2dc.c new file +0
-0
drivers/media/platform/atmel/microchip-csi2dc.c line 1
Quoted 1 line
drivers/media/platform/atmel/microchip-csi2dc.c new file +1
-0@@ -0,0 +1,785 @@ +// SPDX-License-Identifier: GPL-2.0
Nit: I see GPL-2.0 listed as deprecated by spdx.org Should you use one of:
GPL-2.0-only
GPL-2.0-or-laterQuoted 265 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +265
-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) +/* SAMA7G5 requires a HLC delay of 15 */ +#define SAMA7G5_HLC (15) + +/* 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 , + }, { + .mbus_code =MEDIA_BUS_FMT_SBGGR10_1X10 , + .dt =CSI2DC_DT_RAW10 , + }, { + .mbus_code =MEDIA_BUS_FMT_SGRBG10_1X10 , + .dt =CSI2DC_DT_RAW10 , + }, { + .mbus_code =MEDIA_BUS_FMT_SGBRG10_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 vc; + +struct v4l2_async_subdev *asd; +struct v4l2_async_notifier notifier; + +struct v4l2_subdev *input_sd; + +u32 remote_pad; + +bool completed; +bool powered_on; + +struct work_struct workq; +}; + +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_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); +struct csi2dc_format *fmt; +int ret, i; + +if (!csi2dc->completed) { +dev_dbg ((csi2dc)->dev,"subdev not registered yet\n" ); +return 0 ; + } + +for (fmt = csi2dc->formats[0 ], i =0 ; i < csi2dc->num_fmts; fmt++, i++) +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 ]; + +dev_dbg (csi2dc->dev, +"CSI2DC unsupported format 0x%x, defaulting to 0x%x\n" , + req_fmt->format.code, csi2dc->formats[0 ]->mbus_code); + + req_fmt->format.code = csi2dc->formats[0 ]->mbus_code; + } + +/* if we are just trying, we are done */ +if (req_fmt->which ==V4L2_SUBDEV_FORMAT_TRY ) +return 0 ; + + 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) +{ +struct csi2dc_format *fmt; +struct v4l2_subdev *subdev = csi2dc->input_sd; +unsigned int num_fmts, i; + +struct v4l2_subdev_mbus_code_enum mbus_code = { + .which =V4L2_SUBDEV_FORMAT_ACTIVE , + .index =0 , + }; + + num_fmts =0 ; + + csi2dc->formats =devm_kcalloc (csi2dc->dev, +ARRAY_SIZE (csi2dc_formats_list), +sizeof (*csi2dc->formats),GFP_KERNEL ); +if (!csi2dc->formats) +return -ENOMEM ; +
drivers/media/platform/atmel/microchip-csi2dc.c line 276
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+while (!v4l2_subdev_call (subdev, pad, enum_mbus_code,NULL , + &mbus_code)) { + mbus_code.index++; +for (fmt = &csi2dc_formats_list[0 ], i =0 ; + i <ARRAY_SIZE (csi2dc_formats_list); i++, fmt++) +if (fmt->mbus_code == mbus_code.code) { + csi2dc->formats[num_fmts] = fmt; + num_fmts++; + } + }
For the reasons explained above and by Sakari, I think this should be dropped
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-0+ +if (!num_fmts) +return -ENXIO ; + + csi2dc->num_fmts = num_fmts; + +return 0 ;
drivers/media/platform/atmel/microchip-csi2dc.c line 293
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+} + +static int csi2dc_power (struct csi2dc_device *csi2dc,int on) +{ +int ret =0 ; + +if (!csi2dc->completed) { +dev_dbg ((csi2dc)->dev,"subdev not registered yet\n" ); +return 0 ; + }
Can this happen ?
drivers/media/platform/atmel/microchip-csi2dc.c line 296
Quoted 3 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +3
-0+ +if (csi2dc->powered_on == on) +return 0 ;
Same question
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-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);
drivers/media/platform/atmel/microchip-csi2dc.c line 313
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ +/* if powering up, deassert reset line */ +if (on) +csi2dc_writel (csi2dc,CSI2DC_GCTLR ,CSI2DC_GCTLR_SWRST ); + +/* if powering down, assert reset line */ +if (!on) +csi2dc_writel (csi2dc,CSI2DC_GCTLR , !CSI2DC_GCTLR_SWRST ); +if (!ret) + csi2dc->powered_on = on;
Wouldn't this be better coded as
if (on) {
clk_prepare_enable
csi2dc_writel(SWRST)
powered_on = true
} else {
clk_disable_unrepare
csi2dc_writel(!SWRST)
powered_on = false
}Quoted 99 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +99
-0+ +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_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 = { + .pad = &csi2dc_pad_ops, + .video = &csi2dc_video_ops, +}; + +static int csi2dc_get_mbus_config (struct csi2dc_device *csi2dc) +{ +struct v4l2_mbus_config mbus_config = {0 }; +int ret; + + ret =v4l2_subdev_call (csi2dc->input_sd, pad, get_mbus_config, + csi2dc->remote_pad, &mbus_config);
drivers/media/platform/atmel/microchip-csi2dc.c line 422
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+if (ret == -ENOIOCTLCMD ) { +dev_dbg (csi2dc->dev, +"no remote mbus configuration available\n" ); + goto csi2dc_get_mbus_config_defaults; + } + +if (ret) { +dev_err (csi2dc->dev, +"failed to get remote mbus configuration\n" ); + goto csi2dc_get_mbus_config_defaults;
This should probably be an error reported to the caller. But this function return value is not checked, so it's probably fine.
Quoted 40 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +40
-0+ } + +if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_0 ) + csi2dc->vc =0 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_1 ) + csi2dc->vc =1 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_2 ) + csi2dc->vc =2 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_3 ) + csi2dc->vc =3 ; + +dev_dbg (csi2dc->dev,"subdev sending on channel %d\n" , csi2dc->vc); + + csi2dc->clk_gated = mbus_config.flags & +V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK ; + +dev_dbg (csi2dc->dev,"%s clock\n" , + csi2dc->clk_gated ?"gated" :"free running" ); + +return 0 ; + +csi2dc_get_mbus_config_defaults: + csi2dc->vc =0 ;/* Virtual ID 0 by default */ + csi2dc->clk_gated = false;/* Free running clock by default */ + +return 0 ; +} + +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); +int pad; + + csi2dc->input_sd = subdev; + + pad = media_entity_get_fwnode_pad(&subdev->entity, + asd->match.fwnode,
drivers/media/platform/atmel/microchip-csi2dc.c line 472
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ MEDIA_PAD_FL_SOURCE); +if (pad <0 ) { +dev_err (csi2dc->dev,"Failed to find pad for %s\n" , + subdev->name); +return pad; + } + + csi2dc->remote_pad = pad; + +csi2dc_get_mbus_config (csi2dc);
The closer to s_stream you call get_mbus_config, the less the chances of receiving a stale configuration, if this can happen.
Quoted 53 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +53
-0+ +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 =csi2dc_formats_init (csi2dc); +if (ret) +return 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->completed = true; + +csi2dc_writel (csi2dc,CSI2DC_GCFG , + (SAMA7G5_HLC &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 ); + +/* once we are completed, we can register ourselves in the pipeline */ +schedule_work (&csi2dc->workq); + +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) +{
drivers/media/platform/atmel/microchip-csi2dc.c line 535
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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);
If you use v4l2_async_notifier_add_fwnode_subdev() the framework does this for you.
Quoted 5 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +5
-0+ + 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);
drivers/media/platform/atmel/microchip-csi2dc.c line 550
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ 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" );
Shouldn't you cleanup the notifier if registration fails ?
Quoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +12
-0+ 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) +{
drivers/media/platform/atmel/microchip-csi2dc.c line 572
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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",
Please use %pOF to print the node name
Also, input_node should be put in the error path
drivers/media/platform/atmel/microchip-csi2dc.c line 578
Quoted 6 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +6
-0+ of_node->full_name); +return -EINVAL ; + } + + ret =v4l2_fwnode_endpoint_parse (of_fwnode_handle (input_node), + &input_endpoint);
Please set input_endpoint.bus_type if you know what kind of bus you're dealing with (not having the bindings of the IDI transmitter I'm not sure. Is this V4L2_MBUS_CSI2_DPHY ?)
drivers/media/platform/atmel/microchip-csi2dc.c line 587
Quoted 9 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +9
-0+ +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) {
Missing of_node_put(input_node)
drivers/media/platform/atmel/microchip-csi2dc.c line 597
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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);
Same, set sink_endpoint.bus_type if you know it's MBUS_PARALLEL
drivers/media/platform/atmel/microchip-csi2dc.c line 599
Quoted 2 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +2
-0+ +if (!sink_node || ret) {
If parsing fail you don't return an error but continue registering the notifier and potentially end up registering the async subdev for this entity even if the endpoint parsing failed.
drivers/media/platform/atmel/microchip-csi2dc.c line 604
Quoted 5 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +5
-0+dev_info (csi2dc->dev, +"missing sink node at %s, data pipe available only.\n" , + of_node->full_name); + }else { + csi2dc->video_pipe = true;
video_pipe is unused it seems
Quoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +12
-0+ +dev_dbg (csi2dc->dev,"block %s %d.%d->%d.%d video pipeline\n" , + of_node->full_name, input_endpoint.base.port, + input_endpoint.base.id, sink_endpoint.base.port, + sink_endpoint.base.id); + } + +of_node_put (sink_node); +of_node_put (input_node); + +/* prepare async notifier for subdevice completion */ +return csi2dc_prepare_notifier (csi2dc, input_parent);
drivers/media/platform/atmel/microchip-csi2dc.c line 626
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+} + +static void csi2dc_workq_handler (struct work_struct *workq) +{ +struct csi2dc_device *csi2dc =container_of (workq, + struct csi2dc_device, workq); +int ret; + +if (v4l2_async_register_subdev (&csi2dc->csi2dc_sd)) +dev_dbg (csi2dc->dev,"failed to register the subdevice\n" );
That's peculiar, why do you have to schedule this to a workqueue instead of doing this in the complete() callback ?
drivers/media/platform/atmel/microchip-csi2dc.c line 630
Quoted 4 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +4
-0+ + ret =csi2dc_power (csi2dc, true); +if (ret <0 ) +v4l2_err (&csi2dc->v4l2_dev,"failed to power on\n" );
Should the device be powered on at s_stream time ? Possibly with a pm_runtime_get_sync() call ? I see you register pm_runtime operations, but never call get_sync() or _put().
What if CONFIG_PM not selected ? Should this driver select it ? Do you have use cases where that's not possible ?
Quoted 8 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +8
-0+} + +static int csi2dc_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct csi2dc_device *csi2dc; +struct resource *res =NULL ; +int ret =0 ;
drivers/media/platform/atmel/microchip-csi2dc.c line 648
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-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 ;
devm_ioremap_resource() checks for the validity of 'res' so you can drop the previous two lines
drivers/media/platform/atmel/microchip-csi2dc.c line 651
Quoted 3 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +3
-0+ + csi2dc->base =devm_ioremap_resource (dev, res); +
Is the empy line intentional, sometimes you have one, sometimes you don't ?
drivers/media/platform/atmel/microchip-csi2dc.c line 661
Quoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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;
Just return PTR_ERR instead of assigning to ret
drivers/media/platform/atmel/microchip-csi2dc.c line 668
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-0+ } + + ret =clk_prepare_enable (csi2dc->pclk); +if (ret) { +dev_err (dev,"failed to enable pclk: %d\n" , ret); +return ret; + }
Has this clock to be enabled here or can it be done in the pm_runtime callback ?
Quoted 15 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +15
-0+ + 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);
drivers/media/platform/atmel/microchip-csi2dc.c line 693
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ + 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 ;
Isn't the source pad presence conditional to the presence of port@1 ?
Quoted 82 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +82
-0+ + 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); + +INIT_WORK (&csi2dc->workq, csi2dc_workq_handler); + + ret =csi2dc_of_parse (csi2dc, dev->of_node); +if (ret) + goto csi2dc_probe_entity_err; + +dev_info (dev,"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_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); + +v4l2_async_unregister_subdev (&csi2dc->csi2dc_sd); +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) +{ +struct csi2dc_device *csi2dc =dev_get_drvdata (dev); + +return csi2dc_power (csi2dc, false); +} + +static int __maybe_unused csi2dc_runtime_resume (struct device *dev) +{ +struct csi2dc_device *csi2dc =dev_get_drvdata (dev); + +return csi2dc_power (csi2dc, true); +} + +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,
drivers/media/platform/atmel/microchip-csi2dc.c line 785
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ .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" );
For my education, what's MODULE_SUPPORTED_DEVICE for ?
Thanks
j
Quoted 3 lines
--
2.25.1
Re: [PATCH v5 2/3] media: atmel: introduce microchip csi2dc driver
Hello everybody,
Quoted 21 lines · Jacopo Mondi
Quoted 8 lines · Eugen Hristev
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]>---
Hello,Quoted 8 lines
There still are some open questions regarding the last reviews on the ML.
Regarding the format list which is not const, I cannot have it const because I reference elements from this list into a dynamic list which is non-const.
Regarding the presence of the
v4l2_dev, without this I cannot add a notifier for this device to have it async completed , when the underlying subdevice finishes probing.I see you have a discussion with Sakari on-going on this, and I have the very same comment he had.
Quoted 8 lines
Should a sub-notifier be used and this driver should not register any
v4l2_devbut integrate in the media graph of the receiver ?For reference, rcar-csi2 uses sub-notifiers and register in the rcar-vin media graph, it doesn't need to set up a
v4l2_dev.rcar-vin (the DMA engine driver) that register video nodes and the v4l2-dev also registers the device nodes for all the connected subdevices.
Sub-notifiers were invited for this purpose, so I think they're the right tool.
Quoted 5 lines
Quoted 4 lines
Regarding the callbacks for set frame interval, set frame size, etc, I cannot remove them because losing functionality to the underlying subdevice, as explained in :
https://lkml.org/lkml/2020/10/12/427
Quoted 8 lines
Using the frame sizes as in the example you reported, if this device has limitations on the supported sizes, (a min and a max most probably) that's the only thing that should be exposed from this driver subdevice. The sensor subdevice instead will report the available discrete sizes and userspace configures the sensor's source pad and the bridge's sink pad with the same sizes to properly set up the capture pipeline. video0 should not report the sizes exposed by the sensor nor any information that depends on what's connected to it.
Agreed. Subdev drivers should remain simple and cover only the subdev they manage. Bundling it together is the job of userspace. In the legacy model it uses to be the job of the top-level V4L2 driver (the one implementing v4l2_device), but that's legacy, especially for embedded camera use cases. In any case, the top-level logic must not be spread across subdevices.
Quoted 7 lines
I know what your next question is: what about existing application that only know about video0 and want to setup everything from there. I understand vendors not wanting to wrap gstreamer or what is used in the wild in custom scripts that sets up the pipeline opportunely before streaming, and I'm pretty sure you know what is the solution I would propose for this :)
Quoted 12 lines
I've just gone through the same path with RPi's Unicam, that does the same thing as your one does: it's an MC driver, but wants to control the sensor through the v4l2-subdev kAPI to support legacy use cases not covered by libcamera. The fact is that we're stuck in this limbo where MC pipeline tries to decouple components one from each other and delegate their configuration to userspace, but at the same time vendors want to be able to use the simple plain video node centric interface because of their existing user base, so I understand your discomfort, but the 'simple plain' approach is what's preventing Linux platforms to be in-par with proprietary and custom solutions for any use case that is not a simple frame capture, so I think it's fair for mainline to push for this evolution to happen. Just my2c of course.
Compatibility with the userspace of downstream kernels is important, but that's a downstream kernel issue. In mainline we want to go the MC way, and downstream can then, if desired, implement the configuration of the pipeline in the top-level V4L2 driver in the kernel to support the existing userbase. For new drivers I wouldn't recommend that, for existing downstream drivers it may be required.
Quoted 950 lines
Some more comments on the implementation below
Quoted 40 lines
Changes in v5:
- only in bindingsChanges in v4:
- now usingget_mbus_configops to get data from the subdevice, like the virtual channel id, and the clock type.
- now having possibility to select any of the RAW10 data modes
- at completion time, select which formats are also available in the subdevice, and move to the dynamic list accordingly
- changed the pipeline integration, do not advertise subdev ready at probe time. wait until completion is done, and then start a workqueue that will register this device as a subdevice for the next element in pipeline.
- moved thes_powercode into a different function called nowcsi2dc_powerthat is called withCONFIG_PMfunctions. This is also called at completion, to have the device ready in caseCONFIG_PMis not selected on the platform.
- mergedtry_fmtintoset_fmt
- driver cleanup, wrapped lines over 80 charactersChanges 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 updrivers/media/platform/atmel/Kconfig | 13 + drivers/media/platform/atmel/Makefile | 1 + .../media/platform/atmel/microchip-csi2dc.c | 785 ++++++++++++++++++ 3 files changed, 799 insertions(+) create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.cdrivers/media/platform/atmel/Kconfig +3
-0@@ -32,3 +32,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"drivers/media/platform/atmel/Kconfig config VIDEO_ATMEL_ISI · line 47
Quoted 10 lines
drivers/media/platform/atmel/Kconfig +10
-0+ 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.Is the last phrase necessary ? Also missing the usual
To compile this driver as a module, choose M here: the module will be called ...
Quoted 15 lines
drivers/media/platform/atmel/Makefile +1
-0@@ -5,3 +5,4 @@ atmel-xisc-objs = atmel-sama7g5-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_ATMEL_XISC) += atmel-xisc.o +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.odrivers/media/platform/atmel/microchip-csi2dc.c new file +0
-0drivers/media/platform/atmel/microchip-csi2dc.c line 1
Quoted 1 line
drivers/media/platform/atmel/microchip-csi2dc.c new file +1
-0@@ -0,0 +1,785 @@ +// SPDX-License-Identifier: GPL-2.0 Nit: I see GPL-2.0 listed as deprecated by spdx.org Should you use one of:
GPL-2.0-only GPL-2.0-or-laterQuoted 265 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +265
-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) +/* SAMA7G5 requires a HLC delay of 15 */ +#define SAMA7G5_HLC (15) + +/* 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 , + }, { + .mbus_code =MEDIA_BUS_FMT_SBGGR10_1X10 , + .dt =CSI2DC_DT_RAW10 , + }, { + .mbus_code =MEDIA_BUS_FMT_SGRBG10_1X10 , + .dt =CSI2DC_DT_RAW10 , + }, { + .mbus_code =MEDIA_BUS_FMT_SGBRG10_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 vc; + +struct v4l2_async_subdev *asd; +struct v4l2_async_notifier notifier; + +struct v4l2_subdev *input_sd; + +u32 remote_pad; + +bool completed; +bool powered_on; + +struct work_struct workq; +}; + +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_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); +struct csi2dc_format *fmt; +int ret, i; + +if (!csi2dc->completed) { +dev_dbg ((csi2dc)->dev,"subdev not registered yet\n" ); +return 0 ; + } + +for (fmt = csi2dc->formats[0 ], i =0 ; i < csi2dc->num_fmts; fmt++, i++) +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 ]; + +dev_dbg (csi2dc->dev, +"CSI2DC unsupported format 0x%x, defaulting to 0x%x\n" , + req_fmt->format.code, csi2dc->formats[0 ]->mbus_code); + + req_fmt->format.code = csi2dc->formats[0 ]->mbus_code; + } + +/* if we are just trying, we are done */ +if (req_fmt->which ==V4L2_SUBDEV_FORMAT_TRY ) +return 0 ; + + 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) +{ +struct csi2dc_format *fmt; +struct v4l2_subdev *subdev = csi2dc->input_sd; +unsigned int num_fmts, i; + +struct v4l2_subdev_mbus_code_enum mbus_code = { + .which =V4L2_SUBDEV_FORMAT_ACTIVE , + .index =0 , + }; + + num_fmts =0 ; + + csi2dc->formats =devm_kcalloc (csi2dc->dev, +ARRAY_SIZE (csi2dc_formats_list), +sizeof (*csi2dc->formats),GFP_KERNEL ); +if (!csi2dc->formats) +return -ENOMEM ; +drivers/media/platform/atmel/microchip-csi2dc.c line 276
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+while (!v4l2_subdev_call (subdev, pad, enum_mbus_code,NULL , + &mbus_code)) { + mbus_code.index++; +for (fmt = &csi2dc_formats_list[0 ], i =0 ; + i <ARRAY_SIZE (csi2dc_formats_list); i++, fmt++) +if (fmt->mbus_code == mbus_code.code) { + csi2dc->formats[num_fmts] = fmt; + num_fmts++; + } + }For the reasons explained above and by Sakari, I think this should be dropped
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-0+ +if (!num_fmts) +return -ENXIO ; + + csi2dc->num_fmts = num_fmts; + +return 0 ;drivers/media/platform/atmel/microchip-csi2dc.c line 293
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+} + +static int csi2dc_power (struct csi2dc_device *csi2dc,int on) +{ +int ret =0 ; + +if (!csi2dc->completed) { +dev_dbg ((csi2dc)->dev,"subdev not registered yet\n" ); +return 0 ; + }Can this happen ?
drivers/media/platform/atmel/microchip-csi2dc.c line 296
Quoted 3 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +3
-0+ +if (csi2dc->powered_on == on) +return 0 ;Same question
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-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);drivers/media/platform/atmel/microchip-csi2dc.c line 313
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ +/* if powering up, deassert reset line */ +if (on) +csi2dc_writel (csi2dc,CSI2DC_GCTLR ,CSI2DC_GCTLR_SWRST ); + +/* if powering down, assert reset line */ +if (!on) +csi2dc_writel (csi2dc,CSI2DC_GCTLR , !CSI2DC_GCTLR_SWRST ); +if (!ret) + csi2dc->powered_on = on;Wouldn't this be better coded as
if (on) { clk_prepare_enable csi2dc_writel(SWRST) powered_on = true } else { clk_disable_unrepare csi2dc_writel(!SWRST) powered_on = false }Quoted 99 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +99
-0+ +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_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 = { + .pad = &csi2dc_pad_ops, + .video = &csi2dc_video_ops, +}; + +static int csi2dc_get_mbus_config (struct csi2dc_device *csi2dc) +{ +struct v4l2_mbus_config mbus_config = {0 }; +int ret; + + ret =v4l2_subdev_call (csi2dc->input_sd, pad, get_mbus_config, + csi2dc->remote_pad, &mbus_config);drivers/media/platform/atmel/microchip-csi2dc.c line 422
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+if (ret == -ENOIOCTLCMD ) { +dev_dbg (csi2dc->dev, +"no remote mbus configuration available\n" ); + goto csi2dc_get_mbus_config_defaults; + } + +if (ret) { +dev_err (csi2dc->dev, +"failed to get remote mbus configuration\n" ); + goto csi2dc_get_mbus_config_defaults;This should probably be an error reported to the caller. But this function return value is not checked, so it's probably fine.
Quoted 40 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +40
-0+ } + +if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_0 ) + csi2dc->vc =0 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_1 ) + csi2dc->vc =1 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_2 ) + csi2dc->vc =2 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_3 ) + csi2dc->vc =3 ; + +dev_dbg (csi2dc->dev,"subdev sending on channel %d\n" , csi2dc->vc); + + csi2dc->clk_gated = mbus_config.flags & +V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK ; + +dev_dbg (csi2dc->dev,"%s clock\n" , + csi2dc->clk_gated ?"gated" :"free running" ); + +return 0 ; + +csi2dc_get_mbus_config_defaults: + csi2dc->vc =0 ;/* Virtual ID 0 by default */ + csi2dc->clk_gated = false;/* Free running clock by default */ + +return 0 ; +} + +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); +int pad; + + csi2dc->input_sd = subdev; + + pad = media_entity_get_fwnode_pad(&subdev->entity, + asd->match.fwnode,drivers/media/platform/atmel/microchip-csi2dc.c line 472
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ MEDIA_PAD_FL_SOURCE); +if (pad <0 ) { +dev_err (csi2dc->dev,"Failed to find pad for %s\n" , + subdev->name); +return pad; + } + + csi2dc->remote_pad = pad; + +csi2dc_get_mbus_config (csi2dc);The closer to
s_streamyou callget_mbus_config, the less the chances of receiving a stale configuration, if this can happen.Quoted 53 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +53
-0+ +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 =csi2dc_formats_init (csi2dc); +if (ret) +return 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->completed = true; + +csi2dc_writel (csi2dc,CSI2DC_GCFG , + (SAMA7G5_HLC &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 ); + +/* once we are completed, we can register ourselves in the pipeline */ +schedule_work (&csi2dc->workq); + +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) +{drivers/media/platform/atmel/microchip-csi2dc.c line 535
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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);If you use
v4l2_async_notifier_add_fwnode_subdev()the framework does this for you.Quoted 5 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +5
-0+ + 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);drivers/media/platform/atmel/microchip-csi2dc.c line 550
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ 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" );Shouldn't you cleanup the notifier if registration fails ?
Quoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +12
-0+ 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) +{drivers/media/platform/atmel/microchip-csi2dc.c line 572
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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",Please use %pOF to print the node name
Also,input_nodeshould be put in the error pathdrivers/media/platform/atmel/microchip-csi2dc.c line 578
Quoted 6 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +6
-0+ of_node->full_name); +return -EINVAL ; + } + + ret =v4l2_fwnode_endpoint_parse (of_fwnode_handle (input_node), + &input_endpoint);Please set input_endpoint.bus_type if you know what kind of bus you're dealing with (not having the bindings of the IDI transmitter I'm not sure. Is this
V4L2_MBUS_CSI2_DPHY?)drivers/media/platform/atmel/microchip-csi2dc.c line 587
Quoted 9 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +9
-0+ +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) {Missing
of_node_put(input_node)drivers/media/platform/atmel/microchip-csi2dc.c line 597
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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);Same, set sink_endpoint.bus_type if you know it's
MBUS_PARALLELdrivers/media/platform/atmel/microchip-csi2dc.c line 599
Quoted 2 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +2
-0+ +if (!sink_node || ret) {If parsing fail you don't return an error but continue registering the notifier and potentially end up registering the async subdev for this entity even if the endpoint parsing failed.
drivers/media/platform/atmel/microchip-csi2dc.c line 604
Quoted 5 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +5
-0+dev_info (csi2dc->dev, +"missing sink node at %s, data pipe available only.\n" , + of_node->full_name); + }else { + csi2dc->video_pipe = true;
video_pipeis unused it seemsQuoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +12
-0+ +dev_dbg (csi2dc->dev,"block %s %d.%d->%d.%d video pipeline\n" , + of_node->full_name, input_endpoint.base.port, + input_endpoint.base.id, sink_endpoint.base.port, + sink_endpoint.base.id); + } + +of_node_put (sink_node); +of_node_put (input_node); + +/* prepare async notifier for subdevice completion */ +return csi2dc_prepare_notifier (csi2dc, input_parent);drivers/media/platform/atmel/microchip-csi2dc.c line 626
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+} + +static void csi2dc_workq_handler (struct work_struct *workq) +{ +struct csi2dc_device *csi2dc =container_of (workq, + struct csi2dc_device, workq); +int ret; + +if (v4l2_async_register_subdev (&csi2dc->csi2dc_sd)) +dev_dbg (csi2dc->dev,"failed to register the subdevice\n" );That's peculiar, why do you have to schedule this to a workqueue instead of doing this in the
complete()callback ?drivers/media/platform/atmel/microchip-csi2dc.c line 630
Quoted 4 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +4
-0+ + ret =csi2dc_power (csi2dc, true); +if (ret <0 ) +v4l2_err (&csi2dc->v4l2_dev,"failed to power on\n" );Should the device be powered on at
s_streamtime ? Possibly with apm_runtime_get_sync()call ? I see you registerpm_runtimeoperations, but never callget_sync()or_put().What if
CONFIG_PMnot selected ? Should this driver select it ? Do you have use cases where that's not possible ?Quoted 8 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +8
-0+} + +static int csi2dc_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct csi2dc_device *csi2dc; +struct resource *res =NULL ; +int ret =0 ;drivers/media/platform/atmel/microchip-csi2dc.c line 648
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-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 ;
devm_ioremap_resource()checks for the validity of 'res' so you can drop the previous two linesdrivers/media/platform/atmel/microchip-csi2dc.c line 651
Quoted 3 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +3
-0+ + csi2dc->base =devm_ioremap_resource (dev, res); +Is the empy line intentional, sometimes you have one, sometimes you don't ?
drivers/media/platform/atmel/microchip-csi2dc.c line 661
Quoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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;Just return
PTR_ERRinstead of assigning to retdrivers/media/platform/atmel/microchip-csi2dc.c line 668
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-0+ } + + ret =clk_prepare_enable (csi2dc->pclk); +if (ret) { +dev_err (dev,"failed to enable pclk: %d\n" , ret); +return ret; + }Has this clock to be enabled here or can it be done in the
pm_runtimecallback ?Quoted 15 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +15
-0+ + 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);drivers/media/platform/atmel/microchip-csi2dc.c line 693
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ + 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 ;Isn't the source pad presence conditional to the presence of port@1 ?
Quoted 86 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +86
-0+ + 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); + +INIT_WORK (&csi2dc->workq, csi2dc_workq_handler); + + ret =csi2dc_of_parse (csi2dc, dev->of_node); +if (ret) + goto csi2dc_probe_entity_err; + +dev_info (dev,"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_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); + +v4l2_async_unregister_subdev (&csi2dc->csi2dc_sd); +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) +{ +struct csi2dc_device *csi2dc =dev_get_drvdata (dev); + +return csi2dc_power (csi2dc, false); +} + +static int __maybe_unused csi2dc_runtime_resume (struct device *dev) +{ +struct csi2dc_device *csi2dc =dev_get_drvdata (dev); + +return csi2dc_power (csi2dc, true); +} + +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), + }, +}; +
Quoted 8 lines
drivers/media/platform/atmel/microchip-csi2dc.c line 785
Quoted 6 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +6
-0+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" );For my education, what's
MODULE_SUPPORTED_DEVICEfor ?
It's "not yet implemented" since 2005, I think it can be dropped :-)
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v5 2/3] media: atmel: introduce microchip csi2dc driver
Quoted 77 lines · Laurent Pinchart
Hello everybody,
Quoted 21 lines · Jacopo Mondi
Quoted 8 lines · Eugen Hristev
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]>---
Hello,Quoted 8 lines
There still are some open questions regarding the last reviews on the ML.
Regarding the format list which is not const, I cannot have it const because I reference elements from this list into a dynamic list which is non-const.
Regarding the presence of the
v4l2_dev, without this I cannot add a notifier for this device to have it async completed , when the underlying subdevice finishes probing.I see you have a discussion with Sakari on-going on this, and I have the very same comment he had.
Quoted 8 lines
Should a sub-notifier be used and this driver should not register any
v4l2_devbut integrate in the media graph of the receiver ?For reference, rcar-csi2 uses sub-notifiers and register in the rcar-vin media graph, it doesn't need to set up a
v4l2_dev.rcar-vin (the DMA engine driver) that register video nodes and the v4l2-dev also registers the device nodes for all the connected subdevices.
Sub-notifiers were invited for this purpose, so I think they're the right tool.
Quoted 5 lines
Quoted 4 lines
Regarding the callbacks for set frame interval, set frame size, etc, I cannot remove them because losing functionality to the underlying subdevice, as explained in :
https://lkml.org/lkml/2020/10/12/427Quoted 8 lines
Using the frame sizes as in the example you reported, if this device has limitations on the supported sizes, (a min and a max most probably) that's the only thing that should be exposed from this driver subdevice. The sensor subdevice instead will report the available discrete sizes and userspace configures the sensor's source pad and the bridge's sink pad with the same sizes to properly set up the capture pipeline. video0 should not report the sizes exposed by the sensor nor any information that depends on what's connected to it.
Agreed. Subdev drivers should remain simple and cover only the subdev they manage. Bundling it together is the job of userspace. In the legacy model it uses to be the job of the top-level V4L2 driver (the one implementing
v4l2_device), but that's legacy, especially for embedded camera use cases. In any case, the top-level logic must not be spread across subdevices.Quoted 7 lines
I know what your next question is: what about existing application that only know about video0 and want to setup everything from there. I understand vendors not wanting to wrap gstreamer or what is used in the wild in custom scripts that sets up the pipeline opportunely before streaming, and I'm pretty sure you know what is the solution I would propose for this :)
Quoted 12 lines
I've just gone through the same path with RPi's Unicam, that does the same thing as your one does: it's an MC driver, but wants to control the sensor through the v4l2-subdev kAPI to support legacy use cases not covered by libcamera. The fact is that we're stuck in this limbo where MC pipeline tries to decouple components one from each other and delegate their configuration to userspace, but at the same time vendors want to be able to use the simple plain video node centric interface because of their existing user base, so I understand your discomfort, but the 'simple plain' approach is what's preventing Linux platforms to be in-par with proprietary and custom solutions for any use case that is not a simple frame capture, so I think it's fair for mainline to push for this evolution to happen. Just my2c of course.
Quoted 6 lines
Compatibility with the userspace of downstream kernels is important, but that's a downstream kernel issue. In mainline we want to go the MC way, and downstream can then, if desired, implement the configuration of the pipeline in the top-level V4L2 driver in the kernel to support the existing userbase. For new drivers I wouldn't recommend that, for existing downstream drivers it may be required.
Hello,
First of all thank you for the review and explanations.
I am still confused about how does the userspace know which elements are in the pipeline and how to connect them ?
And if this is the case, what is the purpose of the endpoints in the DT ?
At this moment, I just use the /dev/video0, because the top v4l2 driver configures the subdevice, and this subdevice configures it's own subdevice and so on, until the sensor itself.
My top v4l2 driver will not 'complete' unless a subdevice registers itself , and if this subdevice does not provide any information regarding its formats, the probe of the top v4l2 driver will fail, because it will not find compatibility between it's supported input formats and what the subdevice provides.
So what to do in this case ? Libcamera will configure this whole pipeline for me , if the drivers just probe ( and start/stop streaming by selecting the respective registers) ? or how to do this whole pipeline configuration ?
Quoted 967 lines
Quoted 951 lines
Some more comments on the implementation below
Quoted 40 lines
Changes in v5:
- only in bindingsChanges in v4:
- now usingget_mbus_configops to get data from the subdevice, like the virtual channel id, and the clock type.
- now having possibility to select any of the RAW10 data modes
- at completion time, select which formats are also available in the subdevice, and move to the dynamic list accordingly
- changed the pipeline integration, do not advertise subdev ready at probe time. wait until completion is done, and then start a workqueue that will register this device as a subdevice for the next element in pipeline.
- moved thes_powercode into a different function called nowcsi2dc_powerthat is called withCONFIG_PMfunctions. This is also called at completion, to have the device ready in caseCONFIG_PMis not selected on the platform.
- mergedtry_fmtintoset_fmt
- driver cleanup, wrapped lines over 80 charactersChanges 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 updrivers/media/platform/atmel/Kconfig | 13 + drivers/media/platform/atmel/Makefile | 1 + .../media/platform/atmel/microchip-csi2dc.c | 785 ++++++++++++++++++ 3 files changed, 799 insertions(+)create mode 100644
drivers/media/platform/atmel/microchip-csi2dc.cdrivers/media/platform/atmel/Kconfig +3
-0@@ -32,3 +32,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"drivers/media/platform/atmel/Kconfig config VIDEO_ATMEL_ISI · line 47
Quoted 10 lines
drivers/media/platform/atmel/Kconfig +10
-0+ 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.Is the last phrase necessary ? Also missing the usual
To compile this driver as a module, choose M here: the module will be called ...
Quoted 15 lines
drivers/media/platform/atmel/Makefile +1
-0@@ -5,3 +5,4 @@ atmel-xisc-objs = atmel-sama7g5-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_ATMEL_XISC) += atmel-xisc.o +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.odrivers/media/platform/atmel/microchip-csi2dc.c new file +0
-0drivers/media/platform/atmel/microchip-csi2dc.c line 1
Quoted 1 line
drivers/media/platform/atmel/microchip-csi2dc.c new file +1
-0@@ -0,0 +1,785 @@ +// SPDX-License-Identifier: GPL-2.0 Nit: I see GPL-2.0 listed as deprecated by spdx.org Should you use one of:
GPL-2.0-only GPL-2.0-or-laterQuoted 265 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +265
-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) +/* SAMA7G5 requires a HLC delay of 15 */ +#define SAMA7G5_HLC (15) + +/* 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 , + }, { + .mbus_code =MEDIA_BUS_FMT_SBGGR10_1X10 , + .dt =CSI2DC_DT_RAW10 , + }, { + .mbus_code =MEDIA_BUS_FMT_SGRBG10_1X10 , + .dt =CSI2DC_DT_RAW10 , + }, { + .mbus_code =MEDIA_BUS_FMT_SGBRG10_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 vc; + +struct v4l2_async_subdev *asd; +struct v4l2_async_notifier notifier; + +struct v4l2_subdev *input_sd; + +u32 remote_pad; + +bool completed; +bool powered_on; + +struct work_struct workq; +}; + +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_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); +struct csi2dc_format *fmt; +int ret, i; + +if (!csi2dc->completed) { +dev_dbg ((csi2dc)->dev,"subdev not registered yet\n" ); +return 0 ; + } + +for (fmt = csi2dc->formats[0 ], i =0 ; i < csi2dc->num_fmts; fmt++, i++) +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 ]; + +dev_dbg (csi2dc->dev, +"CSI2DC unsupported format 0x%x, defaulting to 0x%x\n" , + req_fmt->format.code, csi2dc->formats[0 ]->mbus_code); + + req_fmt->format.code = csi2dc->formats[0 ]->mbus_code; + } + +/* if we are just trying, we are done */ +if (req_fmt->which ==V4L2_SUBDEV_FORMAT_TRY ) +return 0 ; + + 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) +{ +struct csi2dc_format *fmt; +struct v4l2_subdev *subdev = csi2dc->input_sd; +unsigned int num_fmts, i; + +struct v4l2_subdev_mbus_code_enum mbus_code = { + .which =V4L2_SUBDEV_FORMAT_ACTIVE , + .index =0 , + }; + + num_fmts =0 ; + + csi2dc->formats =devm_kcalloc (csi2dc->dev, +ARRAY_SIZE (csi2dc_formats_list), +sizeof (*csi2dc->formats),GFP_KERNEL ); +if (!csi2dc->formats) +return -ENOMEM ; +drivers/media/platform/atmel/microchip-csi2dc.c line 276
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+while (!v4l2_subdev_call (subdev, pad, enum_mbus_code,NULL , + &mbus_code)) { + mbus_code.index++; +for (fmt = &csi2dc_formats_list[0 ], i =0 ; + i <ARRAY_SIZE (csi2dc_formats_list); i++, fmt++) +if (fmt->mbus_code == mbus_code.code) { + csi2dc->formats[num_fmts] = fmt; + num_fmts++; + } + }For the reasons explained above and by Sakari, I think this should be dropped
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-0+ +if (!num_fmts) +return -ENXIO ; + + csi2dc->num_fmts = num_fmts; + +return 0 ;drivers/media/platform/atmel/microchip-csi2dc.c line 293
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+} + +static int csi2dc_power (struct csi2dc_device *csi2dc,int on) +{ +int ret =0 ; + +if (!csi2dc->completed) { +dev_dbg ((csi2dc)->dev,"subdev not registered yet\n" ); +return 0 ; + }Can this happen ?
drivers/media/platform/atmel/microchip-csi2dc.c line 296
Quoted 3 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +3
-0+ +if (csi2dc->powered_on == on) +return 0 ;Same question
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-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);drivers/media/platform/atmel/microchip-csi2dc.c line 313
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ +/* if powering up, deassert reset line */ +if (on) +csi2dc_writel (csi2dc,CSI2DC_GCTLR ,CSI2DC_GCTLR_SWRST ); + +/* if powering down, assert reset line */ +if (!on) +csi2dc_writel (csi2dc,CSI2DC_GCTLR , !CSI2DC_GCTLR_SWRST ); +if (!ret) + csi2dc->powered_on = on;Wouldn't this be better coded as
if (on) { clk_prepare_enable csi2dc_writel(SWRST) powered_on = true } else { clk_disable_unrepare csi2dc_writel(!SWRST) powered_on = false }Quoted 99 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +99
-0+ +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_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 = { + .pad = &csi2dc_pad_ops, + .video = &csi2dc_video_ops, +}; + +static int csi2dc_get_mbus_config (struct csi2dc_device *csi2dc) +{ +struct v4l2_mbus_config mbus_config = {0 }; +int ret; + + ret =v4l2_subdev_call (csi2dc->input_sd, pad, get_mbus_config, + csi2dc->remote_pad, &mbus_config);drivers/media/platform/atmel/microchip-csi2dc.c line 422
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+if (ret == -ENOIOCTLCMD ) { +dev_dbg (csi2dc->dev, +"no remote mbus configuration available\n" ); + goto csi2dc_get_mbus_config_defaults; + } + +if (ret) { +dev_err (csi2dc->dev, +"failed to get remote mbus configuration\n" ); + goto csi2dc_get_mbus_config_defaults;This should probably be an error reported to the caller. But this function return value is not checked, so it's probably fine.
Quoted 40 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +40
-0+ } + +if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_0 ) + csi2dc->vc =0 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_1 ) + csi2dc->vc =1 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_2 ) + csi2dc->vc =2 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_3 ) + csi2dc->vc =3 ; + +dev_dbg (csi2dc->dev,"subdev sending on channel %d\n" , csi2dc->vc); + + csi2dc->clk_gated = mbus_config.flags & +V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK ; + +dev_dbg (csi2dc->dev,"%s clock\n" , + csi2dc->clk_gated ?"gated" :"free running" ); + +return 0 ; + +csi2dc_get_mbus_config_defaults: + csi2dc->vc =0 ;/* Virtual ID 0 by default */ + csi2dc->clk_gated = false;/* Free running clock by default */ + +return 0 ; +} + +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); +int pad; + + csi2dc->input_sd = subdev; + + pad = media_entity_get_fwnode_pad(&subdev->entity, + asd->match.fwnode,drivers/media/platform/atmel/microchip-csi2dc.c line 472
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ MEDIA_PAD_FL_SOURCE); +if (pad <0 ) { +dev_err (csi2dc->dev,"Failed to find pad for %s\n" , + subdev->name); +return pad; + } + + csi2dc->remote_pad = pad; + +csi2dc_get_mbus_config (csi2dc);The closer to
s_streamyou callget_mbus_config, the less the chances of receiving a stale configuration, if this can happen.Quoted 53 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +53
-0+ +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 =csi2dc_formats_init (csi2dc); +if (ret) +return 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->completed = true; + +csi2dc_writel (csi2dc,CSI2DC_GCFG , + (SAMA7G5_HLC &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 ); + +/* once we are completed, we can register ourselves in the pipeline */ +schedule_work (&csi2dc->workq); + +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) +{drivers/media/platform/atmel/microchip-csi2dc.c line 535
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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);If you use
v4l2_async_notifier_add_fwnode_subdev()the framework does this for you.Quoted 5 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +5
-0+ + 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);drivers/media/platform/atmel/microchip-csi2dc.c line 550
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ 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" );Shouldn't you cleanup the notifier if registration fails ?
Quoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +12
-0+ 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) +{drivers/media/platform/atmel/microchip-csi2dc.c line 572
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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",Please use %pOF to print the node name
Also,input_nodeshould be put in the error pathdrivers/media/platform/atmel/microchip-csi2dc.c line 578
Quoted 6 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +6
-0+ of_node->full_name); +return -EINVAL ; + } + + ret =v4l2_fwnode_endpoint_parse (of_fwnode_handle (input_node), + &input_endpoint);Please set input_endpoint.bus_type if you know what kind of bus you're dealing with (not having the bindings of the IDI transmitter I'm not sure. Is this
V4L2_MBUS_CSI2_DPHY?)drivers/media/platform/atmel/microchip-csi2dc.c line 587
Quoted 9 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +9
-0+ +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) {Missing
of_node_put(input_node)drivers/media/platform/atmel/microchip-csi2dc.c line 597
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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);Same, set sink_endpoint.bus_type if you know it's
MBUS_PARALLELdrivers/media/platform/atmel/microchip-csi2dc.c line 599
Quoted 2 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +2
-0+ +if (!sink_node || ret) {If parsing fail you don't return an error but continue registering the notifier and potentially end up registering the async subdev for this entity even if the endpoint parsing failed.
drivers/media/platform/atmel/microchip-csi2dc.c line 604
Quoted 5 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +5
-0+dev_info (csi2dc->dev, +"missing sink node at %s, data pipe available only.\n" , + of_node->full_name); + }else { + csi2dc->video_pipe = true;
video_pipeis unused it seemsQuoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +12
-0+ +dev_dbg (csi2dc->dev,"block %s %d.%d->%d.%d video pipeline\n" , + of_node->full_name, input_endpoint.base.port, + input_endpoint.base.id, sink_endpoint.base.port, + sink_endpoint.base.id); + } + +of_node_put (sink_node); +of_node_put (input_node); + +/* prepare async notifier for subdevice completion */ +return csi2dc_prepare_notifier (csi2dc, input_parent);drivers/media/platform/atmel/microchip-csi2dc.c line 626
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+} + +static void csi2dc_workq_handler (struct work_struct *workq) +{ +struct csi2dc_device *csi2dc =container_of (workq, + struct csi2dc_device, workq); +int ret; + +if (v4l2_async_register_subdev (&csi2dc->csi2dc_sd)) +dev_dbg (csi2dc->dev,"failed to register the subdevice\n" );That's peculiar, why do you have to schedule this to a workqueue instead of doing this in the
complete()callback ?drivers/media/platform/atmel/microchip-csi2dc.c line 630
Quoted 4 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +4
-0+ + ret =csi2dc_power (csi2dc, true); +if (ret <0 ) +v4l2_err (&csi2dc->v4l2_dev,"failed to power on\n" );Should the device be powered on at
s_streamtime ? Possibly with apm_runtime_get_sync()call ? I see you registerpm_runtimeoperations, but never callget_sync()or_put().What if
CONFIG_PMnot selected ? Should this driver select it ? Do you have use cases where that's not possible ?Quoted 8 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +8
-0+} + +static int csi2dc_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct csi2dc_device *csi2dc; +struct resource *res =NULL ; +int ret =0 ;drivers/media/platform/atmel/microchip-csi2dc.c line 648
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-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 ;
devm_ioremap_resource()checks for the validity of 'res' so you can drop the previous two linesdrivers/media/platform/atmel/microchip-csi2dc.c line 651
Quoted 3 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +3
-0+ + csi2dc->base =devm_ioremap_resource (dev, res); +Is the empy line intentional, sometimes you have one, sometimes you don't ?
drivers/media/platform/atmel/microchip-csi2dc.c line 661
Quoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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;Just return
PTR_ERRinstead of assigning to retdrivers/media/platform/atmel/microchip-csi2dc.c line 668
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-0+ } + + ret =clk_prepare_enable (csi2dc->pclk); +if (ret) { +dev_err (dev,"failed to enable pclk: %d\n" , ret); +return ret; + }Has this clock to be enabled here or can it be done in the
pm_runtimecallback ?Quoted 15 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +15
-0+ + 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);drivers/media/platform/atmel/microchip-csi2dc.c line 693
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ + 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 ;Isn't the source pad presence conditional to the presence of port@1 ?
Quoted 86 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +86
-0+ + 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); + +INIT_WORK (&csi2dc->workq, csi2dc_workq_handler); + + ret =csi2dc_of_parse (csi2dc, dev->of_node); +if (ret) + goto csi2dc_probe_entity_err; + +dev_info (dev,"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_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); + +v4l2_async_unregister_subdev (&csi2dc->csi2dc_sd); +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) +{ +struct csi2dc_device *csi2dc =dev_get_drvdata (dev); + +return csi2dc_power (csi2dc, false); +} + +static int __maybe_unused csi2dc_runtime_resume (struct device *dev) +{ +struct csi2dc_device *csi2dc =dev_get_drvdata (dev); + +return csi2dc_power (csi2dc, true); +} + +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), + }, +}; +Quoted 8 lines
drivers/media/platform/atmel/microchip-csi2dc.c line 785
Quoted 6 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +6
-0+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" );For my education, what's
MODULE_SUPPORTED_DEVICEfor ?It's "not yet implemented" since 2005, I think it can be dropped :-)
--
Regards,Laurent Pinchart
Re: [PATCH v5 2/3] media: atmel: introduce microchip csi2dc driver
Hello Eugen,
Quoted 85 lines · On Tue, Nov 17, 2020 at 05:24:30PM +0000, [email protected] wrote:
Quoted 77 lines · Laurent Pinchart
Hello everybody,
Quoted 21 lines · Jacopo Mondi
Quoted 8 lines · Eugen Hristev
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]>---
Hello,Quoted 8 lines
There still are some open questions regarding the last reviews on the ML.
Regarding the format list which is not const, I cannot have it const because I reference elements from this list into a dynamic list which is non-const.
Regarding the presence of the
v4l2_dev, without this I cannot add a notifier for this device to have it async completed , when the underlying subdevice finishes probing.I see you have a discussion with Sakari on-going on this, and I have the very same comment he had.
Quoted 8 lines
Should a sub-notifier be used and this driver should not register any
v4l2_devbut integrate in the media graph of the receiver ?For reference, rcar-csi2 uses sub-notifiers and register in the rcar-vin media graph, it doesn't need to set up a
v4l2_dev.rcar-vin (the DMA engine driver) that register video nodes and the v4l2-dev also registers the device nodes for all the connected subdevices.
Sub-notifiers were invited for this purpose, so I think they're the right tool.
Quoted 5 lines
Quoted 4 lines
Regarding the callbacks for set frame interval, set frame size, etc, I cannot remove them because losing functionality to the underlying subdevice, as explained in :
https://lkml.org/lkml/2020/10/12/427Quoted 8 lines
Using the frame sizes as in the example you reported, if this device has limitations on the supported sizes, (a min and a max most probably) that's the only thing that should be exposed from this driver subdevice. The sensor subdevice instead will report the available discrete sizes and userspace configures the sensor's source pad and the bridge's sink pad with the same sizes to properly set up the capture pipeline. video0 should not report the sizes exposed by the sensor nor any information that depends on what's connected to it.
Agreed. Subdev drivers should remain simple and cover only the subdev they manage. Bundling it together is the job of userspace. In the legacy model it uses to be the job of the top-level V4L2 driver (the one implementing
v4l2_device), but that's legacy, especially for embedded camera use cases. In any case, the top-level logic must not be spread across subdevices.Quoted 7 lines
I know what your next question is: what about existing application that only know about video0 and want to setup everything from there. I understand vendors not wanting to wrap gstreamer or what is used in the wild in custom scripts that sets up the pipeline opportunely before streaming, and I'm pretty sure you know what is the solution I would propose for this :)
Quoted 12 lines
I've just gone through the same path with RPi's Unicam, that does the same thing as your one does: it's an MC driver, but wants to control the sensor through the v4l2-subdev kAPI to support legacy use cases not covered by libcamera. The fact is that we're stuck in this limbo where MC pipeline tries to decouple components one from each other and delegate their configuration to userspace, but at the same time vendors want to be able to use the simple plain video node centric interface because of their existing user base, so I understand your discomfort, but the 'simple plain' approach is what's preventing Linux platforms to be in-par with proprietary and custom solutions for any use case that is not a simple frame capture, so I think it's fair for mainline to push for this evolution to happen. Just my2c of course.
Quoted 6 lines
Compatibility with the userspace of downstream kernels is important, but that's a downstream kernel issue. In mainline we want to go the MC way, and downstream can then, if desired, implement the configuration of the pipeline in the top-level V4L2 driver in the kernel to support the existing userbase. For new drivers I wouldn't recommend that, for existing downstream drivers it may be required.
Quoted 6 lines
Hello,
First of all thank you for the review and explanations.
I am still confused about how does the userspace know which elements are in the pipeline and how to connect them ?
Using the media-controller uAPI to inspect, explore and configure the media graph (it's mostly about enabling and disabling media links), and using the v4l2 subdev uAPI to configure formats, sizes and such on each subdevice device node associated with a media entity. Finally, using the v4l2 uAPI to stream from the top-level driver that expose a video device node.
It's really (conceptually) not different than doing the same using media-ctl and v4l2-ctl.
Quoted 2 lines
And if this is the case, what is the purpose of the endpoints in the DT ?
DTS describe the hardware. Usually a driver parses the firmware graph to create and model the media graph and the available connections between media entities but there's no requirement to have a 1-to-1 match (as far I'm aware).
Media links on a media graph then need to be enabled opportunely depending on the desired use case, the platform topology etc
Quoted 9 lines
At this moment, I just use the
/dev/video0, because the top v4l2 driver configures the subdevice, and this subdevice configures it's own subdevice and so on, until the sensor itself.My top v4l2 driver will not 'complete' unless a subdevice registers itself , and if this subdevice does not provide any information regarding its formats, the probe of the top v4l2 driver will fail, because it will not find compatibility between it's supported input formats and what the subdevice provides.
ouch, I see.. Which driver is that ?
If that's an MC driver, and that seems the case, it should not bother validating the subdevice formats, but only care about listing what its capabilities are.
Quoted 6 lines
So what to do in this case ? Libcamera will configure this whole pipeline for me , if the drivers just probe ( and start/stop streaming by selecting the respective registers) ? or how to do this whole pipeline configuration ?
Well, libcamera offers a framework to enable specialized code (called "pipeline handler") to operate under an high-level API towards application and implement image tuning algorithms on top of that common API.
The code to configure the pipeline is device specific (unless your pipeline is very simple). So it's not that libcamera just magically knows how to operate on every platforms it runs on, it needs platform specific code to do so. What you get in exchange is an high level API to offer to application developers, a framework to implement 3A and tuning algorithms, integration with a growing set of other userspace frameworks (android, gstreamer, pipewire and a legacy v4l2-compat layer)
We have a lengthy pipeline handler writers guide that might help better explain this:
https://git.linuxtv.org/libcamera.git/tree/Documentation/guides/pipeline-handler.rst
(you might want to compile libcamera to be able to render it in a browser)
Hope this helps to clarify things a bit
Quoted 969 lines
Quoted 968 lines
Quoted 952 lines
Some more comments on the implementation below
Quoted 40 lines
Changes in v5:
- only in bindingsChanges in v4:
- now usingget_mbus_configops to get data from the subdevice, like the virtual channel id, and the clock type.
- now having possibility to select any of the RAW10 data modes
- at completion time, select which formats are also available in the subdevice, and move to the dynamic list accordingly
- changed the pipeline integration, do not advertise subdev ready at probe time. wait until completion is done, and then start a workqueue that will register this device as a subdevice for the next element in pipeline.
- moved thes_powercode into a different function called nowcsi2dc_powerthat is called withCONFIG_PMfunctions. This is also called at completion, to have the device ready in caseCONFIG_PMis not selected on the platform.
- mergedtry_fmtintoset_fmt
- driver cleanup, wrapped lines over 80 charactersChanges 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 updrivers/media/platform/atmel/Kconfig | 13 + drivers/media/platform/atmel/Makefile | 1 + .../media/platform/atmel/microchip-csi2dc.c | 785 ++++++++++++++++++ 3 files changed, 799 insertions(+)create mode 100644
drivers/media/platform/atmel/microchip-csi2dc.cdrivers/media/platform/atmel/Kconfig +3
-0@@ -32,3 +32,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"drivers/media/platform/atmel/Kconfig config VIDEO_ATMEL_ISI · line 47
Quoted 10 lines
drivers/media/platform/atmel/Kconfig +10
-0+ 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.Is the last phrase necessary ? Also missing the usual
To compile this driver as a module, choose M here: the module will be called ...
Quoted 15 lines
drivers/media/platform/atmel/Makefile +1
-0@@ -5,3 +5,4 @@ atmel-xisc-objs = atmel-sama7g5-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_ATMEL_XISC) += atmel-xisc.o +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.odrivers/media/platform/atmel/microchip-csi2dc.c new file +0
-0drivers/media/platform/atmel/microchip-csi2dc.c line 1
Quoted 1 line
drivers/media/platform/atmel/microchip-csi2dc.c new file +1
-0@@ -0,0 +1,785 @@ +// SPDX-License-Identifier: GPL-2.0 Nit: I see GPL-2.0 listed as deprecated by spdx.org Should you use one of:
GPL-2.0-only GPL-2.0-or-laterQuoted 265 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +265
-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) +/* SAMA7G5 requires a HLC delay of 15 */ +#define SAMA7G5_HLC (15) + +/* 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 , + }, { + .mbus_code =MEDIA_BUS_FMT_SBGGR10_1X10 , + .dt =CSI2DC_DT_RAW10 , + }, { + .mbus_code =MEDIA_BUS_FMT_SGRBG10_1X10 , + .dt =CSI2DC_DT_RAW10 , + }, { + .mbus_code =MEDIA_BUS_FMT_SGBRG10_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 vc; + +struct v4l2_async_subdev *asd; +struct v4l2_async_notifier notifier; + +struct v4l2_subdev *input_sd; + +u32 remote_pad; + +bool completed; +bool powered_on; + +struct work_struct workq; +}; + +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_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); +struct csi2dc_format *fmt; +int ret, i; + +if (!csi2dc->completed) { +dev_dbg ((csi2dc)->dev,"subdev not registered yet\n" ); +return 0 ; + } + +for (fmt = csi2dc->formats[0 ], i =0 ; i < csi2dc->num_fmts; fmt++, i++) +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 ]; + +dev_dbg (csi2dc->dev, +"CSI2DC unsupported format 0x%x, defaulting to 0x%x\n" , + req_fmt->format.code, csi2dc->formats[0 ]->mbus_code); + + req_fmt->format.code = csi2dc->formats[0 ]->mbus_code; + } + +/* if we are just trying, we are done */ +if (req_fmt->which ==V4L2_SUBDEV_FORMAT_TRY ) +return 0 ; + + 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) +{ +struct csi2dc_format *fmt; +struct v4l2_subdev *subdev = csi2dc->input_sd; +unsigned int num_fmts, i; + +struct v4l2_subdev_mbus_code_enum mbus_code = { + .which =V4L2_SUBDEV_FORMAT_ACTIVE , + .index =0 , + }; + + num_fmts =0 ; + + csi2dc->formats =devm_kcalloc (csi2dc->dev, +ARRAY_SIZE (csi2dc_formats_list), +sizeof (*csi2dc->formats),GFP_KERNEL ); +if (!csi2dc->formats) +return -ENOMEM ; +drivers/media/platform/atmel/microchip-csi2dc.c line 276
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+while (!v4l2_subdev_call (subdev, pad, enum_mbus_code,NULL , + &mbus_code)) { + mbus_code.index++; +for (fmt = &csi2dc_formats_list[0 ], i =0 ; + i <ARRAY_SIZE (csi2dc_formats_list); i++, fmt++) +if (fmt->mbus_code == mbus_code.code) { + csi2dc->formats[num_fmts] = fmt; + num_fmts++; + } + }For the reasons explained above and by Sakari, I think this should be dropped
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-0+ +if (!num_fmts) +return -ENXIO ; + + csi2dc->num_fmts = num_fmts; + +return 0 ;drivers/media/platform/atmel/microchip-csi2dc.c line 293
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+} + +static int csi2dc_power (struct csi2dc_device *csi2dc,int on) +{ +int ret =0 ; + +if (!csi2dc->completed) { +dev_dbg ((csi2dc)->dev,"subdev not registered yet\n" ); +return 0 ; + }Can this happen ?
drivers/media/platform/atmel/microchip-csi2dc.c line 296
Quoted 3 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +3
-0+ +if (csi2dc->powered_on == on) +return 0 ;Same question
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-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);drivers/media/platform/atmel/microchip-csi2dc.c line 313
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ +/* if powering up, deassert reset line */ +if (on) +csi2dc_writel (csi2dc,CSI2DC_GCTLR ,CSI2DC_GCTLR_SWRST ); + +/* if powering down, assert reset line */ +if (!on) +csi2dc_writel (csi2dc,CSI2DC_GCTLR , !CSI2DC_GCTLR_SWRST ); +if (!ret) + csi2dc->powered_on = on;Wouldn't this be better coded as
if (on) { clk_prepare_enable csi2dc_writel(SWRST) powered_on = true } else { clk_disable_unrepare csi2dc_writel(!SWRST) powered_on = false }Quoted 99 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +99
-0+ +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_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 = { + .pad = &csi2dc_pad_ops, + .video = &csi2dc_video_ops, +}; + +static int csi2dc_get_mbus_config (struct csi2dc_device *csi2dc) +{ +struct v4l2_mbus_config mbus_config = {0 }; +int ret; + + ret =v4l2_subdev_call (csi2dc->input_sd, pad, get_mbus_config, + csi2dc->remote_pad, &mbus_config);drivers/media/platform/atmel/microchip-csi2dc.c line 422
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+if (ret == -ENOIOCTLCMD ) { +dev_dbg (csi2dc->dev, +"no remote mbus configuration available\n" ); + goto csi2dc_get_mbus_config_defaults; + } + +if (ret) { +dev_err (csi2dc->dev, +"failed to get remote mbus configuration\n" ); + goto csi2dc_get_mbus_config_defaults;This should probably be an error reported to the caller. But this function return value is not checked, so it's probably fine.
Quoted 40 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +40
-0+ } + +if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_0 ) + csi2dc->vc =0 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_1 ) + csi2dc->vc =1 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_2 ) + csi2dc->vc =2 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_3 ) + csi2dc->vc =3 ; + +dev_dbg (csi2dc->dev,"subdev sending on channel %d\n" , csi2dc->vc); + + csi2dc->clk_gated = mbus_config.flags & +V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK ; + +dev_dbg (csi2dc->dev,"%s clock\n" , + csi2dc->clk_gated ?"gated" :"free running" ); + +return 0 ; + +csi2dc_get_mbus_config_defaults: + csi2dc->vc =0 ;/* Virtual ID 0 by default */ + csi2dc->clk_gated = false;/* Free running clock by default */ + +return 0 ; +} + +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); +int pad; + + csi2dc->input_sd = subdev; + + pad = media_entity_get_fwnode_pad(&subdev->entity, + asd->match.fwnode,drivers/media/platform/atmel/microchip-csi2dc.c line 472
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ MEDIA_PAD_FL_SOURCE); +if (pad <0 ) { +dev_err (csi2dc->dev,"Failed to find pad for %s\n" , + subdev->name); +return pad; + } + + csi2dc->remote_pad = pad; + +csi2dc_get_mbus_config (csi2dc);The closer to
s_streamyou callget_mbus_config, the less the chances of receiving a stale configuration, if this can happen.Quoted 53 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +53
-0+ +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 =csi2dc_formats_init (csi2dc); +if (ret) +return 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->completed = true; + +csi2dc_writel (csi2dc,CSI2DC_GCFG , + (SAMA7G5_HLC &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 ); + +/* once we are completed, we can register ourselves in the pipeline */ +schedule_work (&csi2dc->workq); + +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) +{drivers/media/platform/atmel/microchip-csi2dc.c line 535
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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);If you use
v4l2_async_notifier_add_fwnode_subdev()the framework does this for you.Quoted 5 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +5
-0+ + 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);drivers/media/platform/atmel/microchip-csi2dc.c line 550
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ 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" );Shouldn't you cleanup the notifier if registration fails ?
Quoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +12
-0+ 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) +{drivers/media/platform/atmel/microchip-csi2dc.c line 572
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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",Please use %pOF to print the node name
Also,input_nodeshould be put in the error pathdrivers/media/platform/atmel/microchip-csi2dc.c line 578
Quoted 6 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +6
-0+ of_node->full_name); +return -EINVAL ; + } + + ret =v4l2_fwnode_endpoint_parse (of_fwnode_handle (input_node), + &input_endpoint);Please set input_endpoint.bus_type if you know what kind of bus you're dealing with (not having the bindings of the IDI transmitter I'm not sure. Is this
V4L2_MBUS_CSI2_DPHY?)drivers/media/platform/atmel/microchip-csi2dc.c line 587
Quoted 9 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +9
-0+ +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) {Missing
of_node_put(input_node)drivers/media/platform/atmel/microchip-csi2dc.c line 597
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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);Same, set sink_endpoint.bus_type if you know it's
MBUS_PARALLELdrivers/media/platform/atmel/microchip-csi2dc.c line 599
Quoted 2 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +2
-0+ +if (!sink_node || ret) {If parsing fail you don't return an error but continue registering the notifier and potentially end up registering the async subdev for this entity even if the endpoint parsing failed.
drivers/media/platform/atmel/microchip-csi2dc.c line 604
Quoted 5 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +5
-0+dev_info (csi2dc->dev, +"missing sink node at %s, data pipe available only.\n" , + of_node->full_name); + }else { + csi2dc->video_pipe = true;
video_pipeis unused it seemsQuoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +12
-0+ +dev_dbg (csi2dc->dev,"block %s %d.%d->%d.%d video pipeline\n" , + of_node->full_name, input_endpoint.base.port, + input_endpoint.base.id, sink_endpoint.base.port, + sink_endpoint.base.id); + } + +of_node_put (sink_node); +of_node_put (input_node); + +/* prepare async notifier for subdevice completion */ +return csi2dc_prepare_notifier (csi2dc, input_parent);drivers/media/platform/atmel/microchip-csi2dc.c line 626
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+} + +static void csi2dc_workq_handler (struct work_struct *workq) +{ +struct csi2dc_device *csi2dc =container_of (workq, + struct csi2dc_device, workq); +int ret; + +if (v4l2_async_register_subdev (&csi2dc->csi2dc_sd)) +dev_dbg (csi2dc->dev,"failed to register the subdevice\n" );That's peculiar, why do you have to schedule this to a workqueue instead of doing this in the
complete()callback ?drivers/media/platform/atmel/microchip-csi2dc.c line 630
Quoted 4 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +4
-0+ + ret =csi2dc_power (csi2dc, true); +if (ret <0 ) +v4l2_err (&csi2dc->v4l2_dev,"failed to power on\n" );Should the device be powered on at
s_streamtime ? Possibly with apm_runtime_get_sync()call ? I see you registerpm_runtimeoperations, but never callget_sync()or_put().What if
CONFIG_PMnot selected ? Should this driver select it ? Do you have use cases where that's not possible ?Quoted 8 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +8
-0+} + +static int csi2dc_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct csi2dc_device *csi2dc; +struct resource *res =NULL ; +int ret =0 ;drivers/media/platform/atmel/microchip-csi2dc.c line 648
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-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 ;
devm_ioremap_resource()checks for the validity of 'res' so you can drop the previous two linesdrivers/media/platform/atmel/microchip-csi2dc.c line 651
Quoted 3 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +3
-0+ + csi2dc->base =devm_ioremap_resource (dev, res); +Is the empy line intentional, sometimes you have one, sometimes you don't ?
drivers/media/platform/atmel/microchip-csi2dc.c line 661
Quoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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;Just return
PTR_ERRinstead of assigning to retdrivers/media/platform/atmel/microchip-csi2dc.c line 668
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-0+ } + + ret =clk_prepare_enable (csi2dc->pclk); +if (ret) { +dev_err (dev,"failed to enable pclk: %d\n" , ret); +return ret; + }Has this clock to be enabled here or can it be done in the
pm_runtimecallback ?Quoted 15 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +15
-0+ + 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);drivers/media/platform/atmel/microchip-csi2dc.c line 693
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ + 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 ;Isn't the source pad presence conditional to the presence of port@1 ?
Quoted 86 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +86
-0+ + 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); + +INIT_WORK (&csi2dc->workq, csi2dc_workq_handler); + + ret =csi2dc_of_parse (csi2dc, dev->of_node); +if (ret) + goto csi2dc_probe_entity_err; + +dev_info (dev,"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_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); + +v4l2_async_unregister_subdev (&csi2dc->csi2dc_sd); +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) +{ +struct csi2dc_device *csi2dc =dev_get_drvdata (dev); + +return csi2dc_power (csi2dc, false); +} + +static int __maybe_unused csi2dc_runtime_resume (struct device *dev) +{ +struct csi2dc_device *csi2dc =dev_get_drvdata (dev); + +return csi2dc_power (csi2dc, true); +} + +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), + }, +}; +Quoted 8 lines
drivers/media/platform/atmel/microchip-csi2dc.c line 785
Quoted 6 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +6
-0+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" );For my education, what's
MODULE_SUPPORTED_DEVICEfor ?It's "not yet implemented" since 2005, I think it can be dropped :-)
--
Regards,Laurent Pinchart
Re: [PATCH v5 2/3] media: atmel: introduce microchip csi2dc driver
Quoted 95 lines · Jacopo Mondi
Hello Eugen,
Quoted 85 lines · On Tue, Nov 17, 2020 at 05:24:30PM +0000, [email protected] wrote:
Quoted 77 lines · Laurent Pinchart
Hello everybody,
Quoted 21 lines · Jacopo Mondi
Quoted 8 lines · Eugen Hristev
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]>---
Hello,Quoted 8 lines
There still are some open questions regarding the last reviews on the ML.
Regarding the format list which is not const, I cannot have it const because I reference elements from this list into a dynamic list which is non-const.
Regarding the presence of the
v4l2_dev, without this I cannot add a notifier for this device to have it async completed , when the underlying subdevice finishes probing.I see you have a discussion with Sakari on-going on this, and I have the very same comment he had.
Quoted 8 lines
Should a sub-notifier be used and this driver should not register any
v4l2_devbut integrate in the media graph of the receiver ?For reference, rcar-csi2 uses sub-notifiers and register in the rcar-vin media graph, it doesn't need to set up a
v4l2_dev.rcar-vin (the DMA engine driver) that register video nodes and the v4l2-dev also registers the device nodes for all the connected subdevices.
Sub-notifiers were invited for this purpose, so I think they're the right tool.
Quoted 5 lines
Quoted 4 lines
Regarding the callbacks for set frame interval, set frame size, etc, I cannot remove them because losing functionality to the underlying subdevice, as explained in :
https://lkml.org/lkml/2020/10/12/427Quoted 8 lines
Using the frame sizes as in the example you reported, if this device has limitations on the supported sizes, (a min and a max most probably) that's the only thing that should be exposed from this driver subdevice. The sensor subdevice instead will report the available discrete sizes and userspace configures the sensor's source pad and the bridge's sink pad with the same sizes to properly set up the capture pipeline. video0 should not report the sizes exposed by the sensor nor any information that depends on what's connected to it.
Agreed. Subdev drivers should remain simple and cover only the subdev they manage. Bundling it together is the job of userspace. In the legacy model it uses to be the job of the top-level V4L2 driver (the one implementing
v4l2_device), but that's legacy, especially for embedded camera use cases. In any case, the top-level logic must not be spread across subdevices.Quoted 7 lines
I know what your next question is: what about existing application that only know about video0 and want to setup everything from there. I understand vendors not wanting to wrap gstreamer or what is used in the wild in custom scripts that sets up the pipeline opportunely before streaming, and I'm pretty sure you know what is the solution I would propose for this :)
Quoted 12 lines
I've just gone through the same path with RPi's Unicam, that does the same thing as your one does: it's an MC driver, but wants to control the sensor through the v4l2-subdev kAPI to support legacy use cases not covered by libcamera. The fact is that we're stuck in this limbo where MC pipeline tries to decouple components one from each other and delegate their configuration to userspace, but at the same time vendors want to be able to use the simple plain video node centric interface because of their existing user base, so I understand your discomfort, but the 'simple plain' approach is what's preventing Linux platforms to be in-par with proprietary and custom solutions for any use case that is not a simple frame capture, so I think it's fair for mainline to push for this evolution to happen. Just my2c of course.
Quoted 6 lines
Compatibility with the userspace of downstream kernels is important, but that's a downstream kernel issue. In mainline we want to go the MC way, and downstream can then, if desired, implement the configuration of the pipeline in the top-level V4L2 driver in the kernel to support the existing userbase. For new drivers I wouldn't recommend that, for existing downstream drivers it may be required.
Quoted 6 lines
Hello,
First of all thank you for the review and explanations.
I am still confused about how does the userspace know which elements are in the pipeline and how to connect them ?
Quoted 6 lines
Using the media-controller uAPI to inspect, explore and configure the media graph (it's mostly about enabling and disabling media links), and using the v4l2 subdev uAPI to configure formats, sizes and such on each subdevice device node associated with a media entity. Finally, using the v4l2 uAPI to stream from the top-level driver that expose a video device node.
Okay but which application in userspace uses this userAPI ? So I could use it to test my pipeline.
Quoted 11 lines
It's really (conceptually) not different than doing the same using media-ctl and v4l2-ctl.
Quoted 2 lines
And if this is the case, what is the purpose of the endpoints in the DT ?
DTS describe the hardware. Usually a driver parses the firmware graph to create and model the media graph and the available connections between media entities but there's no requirement to have a 1-to-1 match (as far I'm aware).
So the top level v4l2 driver should parse the whole graph ?
Quoted 8 lines
Media links on a media graph then need to be enabled opportunely depending on the desired use case, the platform topology etc
Quoted 4 lines
At this moment, I just use the
/dev/video0, because the top v4l2 driver configures the subdevice, and this subdevice configures it's own subdevice and so on, until the sensor itself.
Quoted 7 lines
Quoted 5 lines
My top v4l2 driver will not 'complete' unless a subdevice registers itself , and if this subdevice does not provide any information regarding its formats, the probe of the top v4l2 driver will fail, because it will not find compatibility between it's supported input formats and what the subdevice provides.
ouch, I see.. Which driver is that ?
The atmel image sensor controller driver.
All sensor drivers that we use (for example : ov5640, ov7740, ov7670) register as subdevices, and the atmel-isc will 'complete' when the sensor is probed and registered.
To have the csi2dc module compatible with what we have on atmel-isc, it's natural to have the csi2dc register itself as a subdevice such that the atmel-isc can 'complete'.
Quoted 4 lines
If that's an MC driver, and that seems the case, it should not bother validating the subdevice formats, but only care about listing what its capabilities are.
if this atmel-isc does not offer format capabilities for the userspace via the /dev/video0, then all applications that we use (gstreamer v4l2src, ffmpeg ) fail.
What can we use then, if we cannot use these applications ?
Quoted 12 lines
Quoted 7 lines
So what to do in this case ? Libcamera will configure this whole pipeline for me , if the drivers just probe ( and start/stop streaming by selecting the respective registers) ? or how to do this whole pipeline configuration ?
Well, libcamera offers a framework to enable specialized code (called "pipeline handler") to operate under an high-level API towards application and implement image tuning algorithms on top of that common API.
Quoted 8 lines
The code to configure the pipeline is device specific (unless your pipeline is very simple). So it's not that libcamera just magically knows how to operate on every platforms it runs on, it needs platform specific code to do so. What you get in exchange is an high level API to offer to application developers, a framework to implement 3A and tuning algorithms, integration with a growing set of other userspace frameworks (android, gstreamer, pipewire and a legacy v4l2-compat layer)
I tried a buildroot with libcamera and gstreamer libcamerasrc , to see how it goes. Libcamera does not recognize the /dev/video0 device that we have. The v4l2 compatibility layer should not perform exactly this ?
# gst-launch-1.0 libcamerasrc ! fakesink Setting pipeline to PAUSED ... [0:00:28.448687200] [216] WARN IPAManager ipa_manager.cpp:147 No IPA found in '/usr/lib/libcamera' [0:00:28.448890800] [216] INFO Camera camera_manager.cpp:287 libcamera v0.0.0+54328-2020.11-rc2-dirty (2020-11-17T20:58:37+02:00) ERROR: from element /GstPipeline:pipeline0/GstLibcameraSrc:libcamerasrc0: Could not find any supported camera on this system. Additional debug info: ../src/gstreamer/gstlibcamerasrc.cpp(236): gst_libcamera_src_open (): /GstPipeline:pipeline0/GstLibcameraSrc:libcamerasrc0: libcamera::CameraMananger::cameras() is empty ERROR: pipeline doesn't want to preroll. Failed to set pipeline to PAUSED. Setting pipeline to NULL ... Freeing pipeline ...
Thanks,
Eugen
Quoted 978 lines
We have a lengthy pipeline handler writers guide that might help better explain this:
https://git.linuxtv.org/libcamera.git/tree/Documentation/guides/pipeline-handler.rst
(you might want to compile libcamera to be able to render it in a browser)Hope this helps to clarify things a bit
Quoted 969 lines
Quoted 968 lines
Quoted 952 lines
Some more comments on the implementation below
Quoted 40 lines
Changes in v5:
- only in bindingsChanges in v4:
- now usingget_mbus_configops to get data from the subdevice, like the virtual channel id, and the clock type.
- now having possibility to select any of the RAW10 data modes
- at completion time, select which formats are also available in the subdevice, and move to the dynamic list accordingly
- changed the pipeline integration, do not advertise subdev ready at probe time. wait until completion is done, and then start a workqueue that will register this device as a subdevice for the next element in pipeline.
- moved thes_powercode into a different function called nowcsi2dc_powerthat is called withCONFIG_PMfunctions. This is also called at completion, to have the device ready in caseCONFIG_PMis not selected on the platform.
- mergedtry_fmtintoset_fmt
- driver cleanup, wrapped lines over 80 charactersChanges 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 updrivers/media/platform/atmel/Kconfig | 13 + drivers/media/platform/atmel/Makefile | 1 + .../media/platform/atmel/microchip-csi2dc.c | 785 ++++++++++++++++++ 3 files changed, 799 insertions(+)create mode 100644
drivers/media/platform/atmel/microchip-csi2dc.cdrivers/media/platform/atmel/Kconfig +3
-0@@ -32,3 +32,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"drivers/media/platform/atmel/Kconfig config VIDEO_ATMEL_ISI · line 47
Quoted 10 lines
drivers/media/platform/atmel/Kconfig +10
-0+ 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.Is the last phrase necessary ? Also missing the usual
To compile this driver as a module, choose M here: the module will be called ...
Quoted 15 lines
drivers/media/platform/atmel/Makefile +1
-0@@ -5,3 +5,4 @@ atmel-xisc-objs = atmel-sama7g5-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_ATMEL_XISC) += atmel-xisc.o +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.odrivers/media/platform/atmel/microchip-csi2dc.c new file +0
-0drivers/media/platform/atmel/microchip-csi2dc.c line 1
Quoted 1 line
drivers/media/platform/atmel/microchip-csi2dc.c new file +1
-0@@ -0,0 +1,785 @@ +// SPDX-License-Identifier: GPL-2.0 Nit: I see GPL-2.0 listed as deprecated by spdx.org Should you use one of:
GPL-2.0-only GPL-2.0-or-laterQuoted 265 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +265
-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) +/* SAMA7G5 requires a HLC delay of 15 */ +#define SAMA7G5_HLC (15) + +/* 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 , + }, { + .mbus_code =MEDIA_BUS_FMT_SBGGR10_1X10 , + .dt =CSI2DC_DT_RAW10 , + }, { + .mbus_code =MEDIA_BUS_FMT_SGRBG10_1X10 , + .dt =CSI2DC_DT_RAW10 , + }, { + .mbus_code =MEDIA_BUS_FMT_SGBRG10_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 vc; + +struct v4l2_async_subdev *asd; +struct v4l2_async_notifier notifier; + +struct v4l2_subdev *input_sd; + +u32 remote_pad; + +bool completed; +bool powered_on; + +struct work_struct workq; +}; + +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_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); +struct csi2dc_format *fmt; +int ret, i; + +if (!csi2dc->completed) { +dev_dbg ((csi2dc)->dev,"subdev not registered yet\n" ); +return 0 ; + } + +for (fmt = csi2dc->formats[0 ], i =0 ; i < csi2dc->num_fmts; fmt++, i++) +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 ]; + +dev_dbg (csi2dc->dev, +"CSI2DC unsupported format 0x%x, defaulting to 0x%x\n" , + req_fmt->format.code, csi2dc->formats[0 ]->mbus_code); + + req_fmt->format.code = csi2dc->formats[0 ]->mbus_code; + } + +/* if we are just trying, we are done */ +if (req_fmt->which ==V4L2_SUBDEV_FORMAT_TRY ) +return 0 ; + + 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) +{ +struct csi2dc_format *fmt; +struct v4l2_subdev *subdev = csi2dc->input_sd; +unsigned int num_fmts, i; + +struct v4l2_subdev_mbus_code_enum mbus_code = { + .which =V4L2_SUBDEV_FORMAT_ACTIVE , + .index =0 , + }; + + num_fmts =0 ; + + csi2dc->formats =devm_kcalloc (csi2dc->dev, +ARRAY_SIZE (csi2dc_formats_list), +sizeof (*csi2dc->formats),GFP_KERNEL ); +if (!csi2dc->formats) +return -ENOMEM ; +drivers/media/platform/atmel/microchip-csi2dc.c line 276
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+while (!v4l2_subdev_call (subdev, pad, enum_mbus_code,NULL , + &mbus_code)) { + mbus_code.index++; +for (fmt = &csi2dc_formats_list[0 ], i =0 ; + i <ARRAY_SIZE (csi2dc_formats_list); i++, fmt++) +if (fmt->mbus_code == mbus_code.code) { + csi2dc->formats[num_fmts] = fmt; + num_fmts++; + } + }For the reasons explained above and by Sakari, I think this should be dropped
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-0+ +if (!num_fmts) +return -ENXIO ; + + csi2dc->num_fmts = num_fmts; + +return 0 ;drivers/media/platform/atmel/microchip-csi2dc.c line 293
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+} + +static int csi2dc_power (struct csi2dc_device *csi2dc,int on) +{ +int ret =0 ; + +if (!csi2dc->completed) { +dev_dbg ((csi2dc)->dev,"subdev not registered yet\n" ); +return 0 ; + }Can this happen ?
drivers/media/platform/atmel/microchip-csi2dc.c line 296
Quoted 3 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +3
-0+ +if (csi2dc->powered_on == on) +return 0 ;Same question
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-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);drivers/media/platform/atmel/microchip-csi2dc.c line 313
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ +/* if powering up, deassert reset line */ +if (on) +csi2dc_writel (csi2dc,CSI2DC_GCTLR ,CSI2DC_GCTLR_SWRST ); + +/* if powering down, assert reset line */ +if (!on) +csi2dc_writel (csi2dc,CSI2DC_GCTLR , !CSI2DC_GCTLR_SWRST ); +if (!ret) + csi2dc->powered_on = on;Wouldn't this be better coded as
if (on) { clk_prepare_enable csi2dc_writel(SWRST) powered_on = true } else { clk_disable_unrepare csi2dc_writel(!SWRST) powered_on = false }Quoted 99 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +99
-0+ +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_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 = { + .pad = &csi2dc_pad_ops, + .video = &csi2dc_video_ops, +}; + +static int csi2dc_get_mbus_config (struct csi2dc_device *csi2dc) +{ +struct v4l2_mbus_config mbus_config = {0 }; +int ret; + + ret =v4l2_subdev_call (csi2dc->input_sd, pad, get_mbus_config, + csi2dc->remote_pad, &mbus_config);drivers/media/platform/atmel/microchip-csi2dc.c line 422
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+if (ret == -ENOIOCTLCMD ) { +dev_dbg (csi2dc->dev, +"no remote mbus configuration available\n" ); + goto csi2dc_get_mbus_config_defaults; + } + +if (ret) { +dev_err (csi2dc->dev, +"failed to get remote mbus configuration\n" ); + goto csi2dc_get_mbus_config_defaults;This should probably be an error reported to the caller. But this function return value is not checked, so it's probably fine.
Quoted 40 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +40
-0+ } + +if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_0 ) + csi2dc->vc =0 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_1 ) + csi2dc->vc =1 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_2 ) + csi2dc->vc =2 ; +else if (mbus_config.flags &V4L2_MBUS_CSI2_CHANNEL_3 ) + csi2dc->vc =3 ; + +dev_dbg (csi2dc->dev,"subdev sending on channel %d\n" , csi2dc->vc); + + csi2dc->clk_gated = mbus_config.flags & +V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK ; + +dev_dbg (csi2dc->dev,"%s clock\n" , + csi2dc->clk_gated ?"gated" :"free running" ); + +return 0 ; + +csi2dc_get_mbus_config_defaults: + csi2dc->vc =0 ;/* Virtual ID 0 by default */ + csi2dc->clk_gated = false;/* Free running clock by default */ + +return 0 ; +} + +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); +int pad; + + csi2dc->input_sd = subdev; + + pad = media_entity_get_fwnode_pad(&subdev->entity, + asd->match.fwnode,drivers/media/platform/atmel/microchip-csi2dc.c line 472
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ MEDIA_PAD_FL_SOURCE); +if (pad <0 ) { +dev_err (csi2dc->dev,"Failed to find pad for %s\n" , + subdev->name); +return pad; + } + + csi2dc->remote_pad = pad; + +csi2dc_get_mbus_config (csi2dc);The closer to
s_streamyou callget_mbus_config, the less the chances of receiving a stale configuration, if this can happen.Quoted 53 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +53
-0+ +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 =csi2dc_formats_init (csi2dc); +if (ret) +return 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->completed = true; + +csi2dc_writel (csi2dc,CSI2DC_GCFG , + (SAMA7G5_HLC &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 ); + +/* once we are completed, we can register ourselves in the pipeline */ +schedule_work (&csi2dc->workq); + +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) +{drivers/media/platform/atmel/microchip-csi2dc.c line 535
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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);If you use
v4l2_async_notifier_add_fwnode_subdev()the framework does this for you.Quoted 5 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +5
-0+ + 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);drivers/media/platform/atmel/microchip-csi2dc.c line 550
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ 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" );Shouldn't you cleanup the notifier if registration fails ?
Quoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +12
-0+ 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) +{drivers/media/platform/atmel/microchip-csi2dc.c line 572
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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",Please use %pOF to print the node name
Also,input_nodeshould be put in the error pathdrivers/media/platform/atmel/microchip-csi2dc.c line 578
Quoted 6 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +6
-0+ of_node->full_name); +return -EINVAL ; + } + + ret =v4l2_fwnode_endpoint_parse (of_fwnode_handle (input_node), + &input_endpoint);Please set input_endpoint.bus_type if you know what kind of bus you're dealing with (not having the bindings of the IDI transmitter I'm not sure. Is this
V4L2_MBUS_CSI2_DPHY?)drivers/media/platform/atmel/microchip-csi2dc.c line 587
Quoted 9 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +9
-0+ +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) {Missing
of_node_put(input_node)drivers/media/platform/atmel/microchip-csi2dc.c line 597
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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);Same, set sink_endpoint.bus_type if you know it's
MBUS_PARALLELdrivers/media/platform/atmel/microchip-csi2dc.c line 599
Quoted 2 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +2
-0+ +if (!sink_node || ret) {If parsing fail you don't return an error but continue registering the notifier and potentially end up registering the async subdev for this entity even if the endpoint parsing failed.
drivers/media/platform/atmel/microchip-csi2dc.c line 604
Quoted 5 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +5
-0+dev_info (csi2dc->dev, +"missing sink node at %s, data pipe available only.\n" , + of_node->full_name); + }else { + csi2dc->video_pipe = true;
video_pipeis unused it seemsQuoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +12
-0+ +dev_dbg (csi2dc->dev,"block %s %d.%d->%d.%d video pipeline\n" , + of_node->full_name, input_endpoint.base.port, + input_endpoint.base.id, sink_endpoint.base.port, + sink_endpoint.base.id); + } + +of_node_put (sink_node); +of_node_put (input_node); + +/* prepare async notifier for subdevice completion */ +return csi2dc_prepare_notifier (csi2dc, input_parent);drivers/media/platform/atmel/microchip-csi2dc.c line 626
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+} + +static void csi2dc_workq_handler (struct work_struct *workq) +{ +struct csi2dc_device *csi2dc =container_of (workq, + struct csi2dc_device, workq); +int ret; + +if (v4l2_async_register_subdev (&csi2dc->csi2dc_sd)) +dev_dbg (csi2dc->dev,"failed to register the subdevice\n" );That's peculiar, why do you have to schedule this to a workqueue instead of doing this in the
complete()callback ?drivers/media/platform/atmel/microchip-csi2dc.c line 630
Quoted 4 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +4
-0+ + ret =csi2dc_power (csi2dc, true); +if (ret <0 ) +v4l2_err (&csi2dc->v4l2_dev,"failed to power on\n" );Should the device be powered on at
s_streamtime ? Possibly with apm_runtime_get_sync()call ? I see you registerpm_runtimeoperations, but never callget_sync()or_put().What if
CONFIG_PMnot selected ? Should this driver select it ? Do you have use cases where that's not possible ?Quoted 8 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +8
-0+} + +static int csi2dc_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct csi2dc_device *csi2dc; +struct resource *res =NULL ; +int ret =0 ;drivers/media/platform/atmel/microchip-csi2dc.c line 648
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-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 ;
devm_ioremap_resource()checks for the validity of 'res' so you can drop the previous two linesdrivers/media/platform/atmel/microchip-csi2dc.c line 651
Quoted 3 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +3
-0+ + csi2dc->base =devm_ioremap_resource (dev, res); +Is the empy line intentional, sometimes you have one, sometimes you don't ?
drivers/media/platform/atmel/microchip-csi2dc.c line 661
Quoted 12 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+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;Just return
PTR_ERRinstead of assigning to retdrivers/media/platform/atmel/microchip-csi2dc.c line 668
Quoted 7 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +7
-0+ } + + ret =clk_prepare_enable (csi2dc->pclk); +if (ret) { +dev_err (dev,"failed to enable pclk: %d\n" , ret); +return ret; + }Has this clock to be enabled here or can it be done in the
pm_runtimecallback ?Quoted 15 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +15
-0+ + 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);drivers/media/platform/atmel/microchip-csi2dc.c line 693
Quoted 10 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +10
-0+ + 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 ;Isn't the source pad presence conditional to the presence of port@1 ?
Quoted 86 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +86
-0+ + 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); + +INIT_WORK (&csi2dc->workq, csi2dc_workq_handler); + + ret =csi2dc_of_parse (csi2dc, dev->of_node); +if (ret) + goto csi2dc_probe_entity_err; + +dev_info (dev,"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_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); + +v4l2_async_unregister_subdev (&csi2dc->csi2dc_sd); +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) +{ +struct csi2dc_device *csi2dc =dev_get_drvdata (dev); + +return csi2dc_power (csi2dc, false); +} + +static int __maybe_unused csi2dc_runtime_resume (struct device *dev) +{ +struct csi2dc_device *csi2dc =dev_get_drvdata (dev); + +return csi2dc_power (csi2dc, true); +} + +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), + }, +}; +Quoted 8 lines
drivers/media/platform/atmel/microchip-csi2dc.c line 785
Quoted 6 lines
drivers/media/platform/atmel/microchip-csi2dc.c new file +6
-0+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" );For my education, what's
MODULE_SUPPORTED_DEVICEfor ?It's "not yet implemented" since 2005, I think it can be dropped :-)
--
Regards,Laurent Pinchart
Re: [PATCH v5 2/3] media: atmel: introduce microchip csi2dc driver
Hi Eugen,
Quoted 3 lines · On Wed, Nov 18, 2020 at 09:48:19AM +0000, [email protected] wrote:
Quoted 2 lines · Jacopo Mondi
Hello Eugen,
[snip]
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Hello,
First of all thank you for the review and explanations.
I am still confused about how does the userspace know which elements are in the pipeline and how to connect them ?
Quoted 8 lines
Quoted 6 lines
Using the media-controller uAPI to inspect, explore and configure the media graph (it's mostly about enabling and disabling media links), and using the v4l2 subdev uAPI to configure formats, sizes and such on each subdevice device node associated with a media entity. Finally, using the v4l2 uAPI to stream from the top-level driver that expose a video device node.
Okay but which application in userspace uses this userAPI ?
media-ctl which is part of v4l2-utils.
existing applications or frameworks like gstreamer have no notion of the platform they run on, they can't configure it, they only understand '/dev/video0'
Quoted 2 lines
So I could use it to test my pipeline.
The first step is to use media-ctl to manually setup the pipeline and test. Once it's setup you can usually keep using the legacy applications that only stream from /dev/video0.
Of course manually setting up the pipeline using a script does not scale, that's what the 'libcamera pipeline handler' does in facts. It implements the same logic that you would manually apply with media-ctl
Quoted 7 lines
Quoted 7 lines
It's really (conceptually) not different than doing the same using media-ctl and v4l2-ctl.
Quoted 2 lines
And if this is the case, what is the purpose of the endpoints in the DT ?
Quoted 7 lines
Quoted 4 lines
DTS describe the hardware. Usually a driver parses the firmware graph to create and model the media graph and the available connections between media entities but there's no requirement to have a 1-to-1 match (as far I'm aware).
So the top level v4l2 driver should parse the whole graph ?
Userspace should :)
Quoted 14 lines
Quoted 14 lines
Media links on a media graph then need to be enabled opportunely depending on the desired use case, the platform topology etc
Quoted 9 lines
At this moment, I just use the
/dev/video0, because the top v4l2 driver configures the subdevice, and this subdevice configures it's own subdevice and so on, until the sensor itself.My top v4l2 driver will not 'complete' unless a subdevice registers itself , and if this subdevice does not provide any information regarding its formats, the probe of the top v4l2 driver will fail, because it will not find compatibility between it's supported input formats and what the subdevice provides.
Quoted 6 lines
Quoted 1 line
ouch, I see.. Which driver is that ?
The atmel image sensor controller driver.
All sensor drivers that we use (for example : ov5640, ov7740, ov7670) register as subdevices, and the atmel-isc will 'complete' when the sensor is probed and registered.
That's ok, the complete callback is called when all the async devices registered in a notifier are matched.
Quoted 3 lines
To have the csi2dc module compatible with what we have on atmel-isc, it's natural to have the csi2dc register itself as a subdevice such that the atmel-isc can 'complete'.
the csi2d registering as subdev is all good (apart from the issue of having 2 v4l2_device, but as you've see there's a solution for that and it's called sub-notifiers).
The 'issue' here is that your top driver wants to match formats to initialize its supported format list (in isc_formats_init() from what I can see).
An MC driver should not care about that. Each piece of the pipeline has to be considered in isolation, and the format matching happens at link_validate() time for links between subdevices and usually at s_stream(1) time between sub-devices and video devices.
In example, format enumeration on the video device registered by the top v4l2 driver is conditional to the presence of the mbus_code field as you can read in:
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-enum-fmt.html?highlight=enum_fmt#c.V4L.VIDIOC_ENUM_FMT
And it should be possible by setting mbus_code = 0 to get a list of all formats supported -by the hardware- not limited to the current use case.
Userspace knows the platform and if asked for 640x480[YUV420] will go and set the right format on each piece of the pipeline, after having enabled the relevant links, using the right media bus codes and sizes
Totally random example:
[video0] s_fmt(640x480, PIXFMT_YUV422)
(pad 0)
|
v
(pad 1) sudev_s_fmt(1, 640x480, MBUS_YUYV8_1X16)
[csi2dc] --- your subdev can crop ---
(pad 0) sudev_s_fmt(0, 648x496, MBUS_YUYV8_1X16)
|
V
(pad 0) subdev_s_fmt(0, 648x496, MBUS_YUYV_1X16)
[ov5640]If your top v4l2 driver can produce RGB from a YUV stream it's perfectly legit then to s_fmt(video0, 640x480, PIXFMT_RGB565) and subdev_s_fmt(csi2dc, 1, 640x480, MBUS_YUYV8_1X16).
As you can see it all depends on what your hardware can do.
Quoted 12 lines
Quoted 6 lines
If that's an MC driver, and that seems the case, it should not bother validating the subdevice formats, but only care about listing what its capabilities are.
if this atmel-isc does not offer format capabilities for the userspace via the
/dev/video0, then all applications that we use (gstreamer v4l2src, ffmpeg ) fail.
What can we use then, if we cannot use these applications ?
I think if you setup the pipeline with media-ctl correctly (ie you don't get any -EPIPE at s_stream) you should be able to keep using legacy apps, but it very much depends on how they are implemented.
Usually setting up the pipeline using media-ctl and streaming using v4l2-ctl or yavta is the first validation test.
Quoted 21 lines
Quoted 12 lines
Quoted 7 lines
So what to do in this case ? Libcamera will configure this whole pipeline for me , if the drivers just probe ( and start/stop streaming by selecting the respective registers) ? or how to do this whole pipeline configuration ?
Well, libcamera offers a framework to enable specialized code (called "pipeline handler") to operate under an high-level API towards application and implement image tuning algorithms on top of that common API.
Quoted 8 lines
The code to configure the pipeline is device specific (unless your pipeline is very simple). So it's not that libcamera just magically knows how to operate on every platforms it runs on, it needs platform specific code to do so. What you get in exchange is an high level API to offer to application developers, a framework to implement 3A and tuning algorithms, integration with a growing set of other userspace frameworks (android, gstreamer, pipewire and a legacy v4l2-compat layer)
Quoted 3 lines
I tried a buildroot with libcamera and gstreamer libcamerasrc , to see how it goes. Libcamera does not recognize the
/dev/video0device that we have. The v4l2 compatibility layer should not perform exactly this ?
There's no pipeline handler for your platform (unless you have developed one)
Quoted 10 lines
# gst-launch-1.0 libcamerasrc ! fakesink Setting pipeline to PAUSED ... [0:00:28.448687200] [216] WARN IPAManager ipa_manager.cpp:147 No IPA found in '/usr/lib/libcamera' [0:00:28.448890800] [216] INFO Camera camera_manager.cpp:287 libcamera v0.0.0+54328-2020.11-rc2-dirty (2020-11-17T20:58:37+02:00) ERROR: from element /GstPipeline:pipeline0/GstLibcameraSrc:libcamerasrc0: Could not find any supported camera on this system.
^
And this is the expected outcome :)
As said, it's not that libcamera magically knows how to deal with all devices it is run on. We would not need libcamera at all if that was the case, any application would be self-configurable and that's it :)
Re: [PATCH v5 2/3] media: atmel: introduce microchip csi2dc driver
Quoted 127 lines · Jacopo Mondi
Hi Eugen,
Quoted 3 lines · On Wed, Nov 18, 2020 at 09:48:19AM +0000, [email protected] wrote:
Quoted 2 lines · Jacopo Mondi
Hello Eugen,
[snip]
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Hello,
First of all thank you for the review and explanations.
I am still confused about how does the userspace know which elements are in the pipeline and how to connect them ?
Quoted 8 lines
Quoted 6 lines
Using the media-controller uAPI to inspect, explore and configure the media graph (it's mostly about enabling and disabling media links), and using the v4l2 subdev uAPI to configure formats, sizes and such on each subdevice device node associated with a media entity. Finally, using the v4l2 uAPI to stream from the top-level driver that expose a video device node.
Okay but which application in userspace uses this userAPI ?
media-ctl which is part of v4l2-utils.
existing applications or frameworks like gstreamer have no notion of the platform they run on, they can't configure it, they only understand '
/dev/video0'Quoted 2 lines
So I could use it to test my pipeline.
The first step is to use media-ctl to manually setup the pipeline and test. Once it's setup you can usually keep using the legacy applications that only stream from
/dev/video0.Of course manually setting up the pipeline using a script does not scale, that's what the 'libcamera pipeline handler' does in facts. It implements the same logic that you would manually apply with media-ctl
Quoted 7 lines
Quoted 7 lines
It's really (conceptually) not different than doing the same using media-ctl and v4l2-ctl.
Quoted 2 lines
And if this is the case, what is the purpose of the endpoints in the DT ?
Quoted 7 lines
Quoted 4 lines
DTS describe the hardware. Usually a driver parses the firmware graph to create and model the media graph and the available connections between media entities but there's no requirement to have a 1-to-1 match (as far I'm aware).
So the top level v4l2 driver should parse the whole graph ?
Userspace should :)
Quoted 14 lines
Quoted 14 lines
Media links on a media graph then need to be enabled opportunely depending on the desired use case, the platform topology etc
Quoted 9 lines
At this moment, I just use the
/dev/video0, because the top v4l2 driver configures the subdevice, and this subdevice configures it's own subdevice and so on, until the sensor itself.My top v4l2 driver will not 'complete' unless a subdevice registers itself , and if this subdevice does not provide any information regarding its formats, the probe of the top v4l2 driver will fail, because it will not find compatibility between it's supported input formats and what the subdevice provides.
Quoted 6 lines
Quoted 1 line
ouch, I see.. Which driver is that ?
The atmel image sensor controller driver.
All sensor drivers that we use (for example : ov5640, ov7740, ov7670) register as subdevices, and the atmel-isc will 'complete' when the sensor is probed and registered.That's ok, the complete callback is called when all the async devices registered in a notifier are matched.
Quoted 3 lines
To have the csi2dc module compatible with what we have on atmel-isc, it's natural to have the csi2dc register itself as a subdevice such that the atmel-isc can 'complete'.
the csi2d registering as subdev is all good (apart from the issue of having 2
v4l2_device, but as you've see there's a solution for that and it's called sub-notifiers).The 'issue' here is that your top driver wants to match formats to initialize its supported format list (in
isc_formats_init()from what I can see).An MC driver should not care about that. Each piece of the pipeline has to be considered in isolation, and the format matching happens at
link_validate()time for links between subdevices and usually ats_stream(1)time between sub-devices and video devices.In example, format enumeration on the video device registered by the top v4l2 driver is conditional to the presence of the
mbus_codefield as you can read in:
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-enum-fmt.html?highlight=enum_fmt#c.V4L.VIDIOC_ENUM_FMTAnd it should be possible by setting
mbus_code= 0 to get a list of all formats supported -by the hardware- not limited to the current use case.Userspace knows the platform and if asked for 640x480[YUV420] will go and set the right format on each piece of the pipeline, after having enabled the relevant links, using the right media bus codes and sizes
Totally random example:
[video0] s_fmt(640x480, PIXFMT_YUV422) (pad 0) | v (pad 1) sudev_s_fmt(1, 640x480, MBUS_YUYV8_1X16) [csi2dc] --- your subdev can crop --- (pad 0) sudev_s_fmt(0, 648x496, MBUS_YUYV8_1X16) | V (pad 0) subdev_s_fmt(0, 648x496, MBUS_YUYV_1X16) [ov5640]
Quoted 6 lines
If your top v4l2 driver can produce RGB from a YUV stream it's perfectly legit then to
s_fmt(video0, 640x480, PIXFMT_RGB565)andsubdev_s_fmt(csi2dc, 1, 640x480, MBUS_YUYV8_1X16).As you can see it all depends on what your hardware can do.
Hi again,
Thank you very much for the explanations. It is different from what I had in mind (and how we use the subsystem right now in our drivers here ). I will take some time to understand all this, and hopefully I can come with better drivers, better pipeline handling and libcamera platform support.
I may have questions during the development, but I think I have enough information to get started.
thanks again !
Eugen
Quoted 65 lines
Quoted 12 lines
Quoted 6 lines
If that's an MC driver, and that seems the case, it should not bother validating the subdevice formats, but only care about listing what its capabilities are.
if this atmel-isc does not offer format capabilities for the userspace via the
/dev/video0, then all applications that we use (gstreamer v4l2src, ffmpeg ) fail.
What can we use then, if we cannot use these applications ?I think if you setup the pipeline with media-ctl correctly (ie you don't get any
-EPIPEats_stream) you should be able to keep using legacy apps, but it very much depends on how they are implemented.Usually setting up the pipeline using media-ctl and streaming using v4l2-ctl or yavta is the first validation test.
Quoted 21 lines
Quoted 12 lines
Quoted 7 lines
So what to do in this case ? Libcamera will configure this whole pipeline for me , if the drivers just probe ( and start/stop streaming by selecting the respective registers) ? or how to do this whole pipeline configuration ?
Well, libcamera offers a framework to enable specialized code (called "pipeline handler") to operate under an high-level API towards application and implement image tuning algorithms on top of that common API.
Quoted 8 lines
The code to configure the pipeline is device specific (unless your pipeline is very simple). So it's not that libcamera just magically knows how to operate on every platforms it runs on, it needs platform specific code to do so. What you get in exchange is an high level API to offer to application developers, a framework to implement 3A and tuning algorithms, integration with a growing set of other userspace frameworks (android, gstreamer, pipewire and a legacy v4l2-compat layer)
Quoted 3 lines
I tried a buildroot with libcamera and gstreamer libcamerasrc , to see how it goes. Libcamera does not recognize the
/dev/video0device that we have. The v4l2 compatibility layer should not perform exactly this ?There's no pipeline handler for your platform (unless you have developed one)
Quoted 10 lines
# gst-launch-1.0 libcamerasrc ! fakesink Setting pipeline to PAUSED ... [0:00:28.448687200] [216] WARN IPAManager ipa_manager.cpp:147 No IPA found in '/usr/lib/libcamera' [0:00:28.448890800] [216] INFO Camera camera_manager.cpp:287 libcamera v0.0.0+54328-2020.11-rc2-dirty (2020-11-17T20:58:37+02:00) ERROR: from element /GstPipeline:pipeline0/GstLibcameraSrc:libcamerasrc0: Could not find any supported camera on this system.^And this is the expected outcome :)
As said, it's not that libcamera magically knows how to deal with all devices it is run on. We would not need libcamera at all if that was the case, any application would be self-configurable and that's it :)
PATCH v5 3/3 MAINTAINERS: add microchip csi2dc
In reply to Eugen HristevAdd Microchip CSI2DC driver in the list.
--- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+)
MAINTAINERS +7 -0
@@ -11472,6 +11472,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]
Signature · 1 line
2.25.1
Hello Eugen,
Quoted 49 lines · Eugen Hristev
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 v5:
- modified bindings as per Rob Herring reviewChanges in v4:
- Removed property for inter-line-delay and for clock continuous/non-continuous
- Removed virtual channel by reg for second endpointChanges 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 descriptionChanges in v2:
- fixed warnings reported bydt_binding_check.../bindings/media/microchip,csi2dc.yaml | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/microchip,csi2dc.yamlDocumentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +17
-0@@ -0,0 +1,119 @@ +# 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
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml line 27
Quoted 10 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +10
-0+ 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.
Comments on a description from a non-native speaker might be mis-leading, take the following as suggestions only :)
the here mentioned port node is said to be a 'source' to which an IDI interface that provides data is connected ? Is this a s/sink/source mistake or did you mean the first port node has to be connected to a source (the IDI interface) ?
Also, maybe it's me but 'pad' is a media-controller concept, which should not be mentioned in bindings (as well as 'source' and 'sink' but they have a more generic usage)
The first port, to me, is simply an 'IDI input port'
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml line 29
Quoted 2 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +2
-0+ For media entity and endpoints please refer to the bindings defined in + Documentation/devicetree/bindings/media/video-interfaces.txt.
I would drop 'media-entity'. To be honest I would drop this phrase and mention ../video-interfaces.txt in the ports description. Up to you.
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml line 34
Quoted 5 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +5
-0+ 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.
I'm confused again, isn't the parallel interface an output ? Why is it said to be a 'sink pad' ?
Isn't port one simply a 'video output port' ?
Also the association between bindings' 'port' and their representation as media-controller constructs should in my opinion be avoided. Bindings are about describing hardware, and they could be used by non-Linux systems as well (at least, I've been told this multiple times :)
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml line 41
Quoted 7 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +7
-0+ 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.
Is my understanding correct of this 'direct access' is not represented in DTS ?
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml line 43
Quoted 2 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +2
-0+ 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.
And so the DMA channel and the output video port are mutually exclusive ? I feel like the DMA port should be described or at least represented and made exclusive with port1 presence ?
Quoted 28 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +28
-0+ +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 + + ports: + type: object + description: + List of ports
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml line 81
Quoted 10 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +10
-0+ + properties: + port@0: + type: object + description: + Input port node, single endpoint describing the input pad. + port@1: + type: object + description: + Output port node, single endpoint, describing the output pad.
That's a parallel video output, correct ? Does this port support any configurable property of the ones defined by video-interfaces.txt ?
Thanks
j
Quoted 41 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +38
-1+ +additionalProperties: false + +required: + - compatible + - reg + - clocks + - clock-names + - ports + +examples: + - | + csi2dc@e1404000 { + compatible = "microchip,sama7g5-csi2dc"; + reg = <0xe1404000 0x500>; + clocks = <&pclk>, <&scck>; + clock-names = "pclk", "scck"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; /* must be 0, first child port */ + csi2dc_in: endpoint { /* input from IDI interface */ + remote-endpoint = <&csi2host_out>; + }; + }; + + port@1 { + reg = <1>; /* must be 1, second child port */ + csi2dc_out: endpoint { + remote-endpoint = <&xisc_in>; /* output to sensor controller */ + }; + }; + }; + }; + +... --2.25.1
Hi again,
Quoted 53 lines · Eugen Hristev
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 v5:
- modified bindings as per Rob Herring reviewChanges in v4:
- Removed property for inter-line-delay and for clock continuous/non-continuous
- Removed virtual channel by reg for second endpointChanges 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 descriptionChanges in v2:
- fixed warnings reported bydt_binding_check.../bindings/media/microchip,csi2dc.yaml | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/microchip,csi2dc.yamlDocumentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +21
-0@@ -0,0 +1,119 @@ +# 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
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml line 31
Quoted 10 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +10
-0+ 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
Is it me or this file doesn't exists on the most recent media/master and on v5.10-rc4 ?
Quoted 91 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +88
-1+ + 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 + + ports: + type: object + description: + List of ports + + properties: + port@0: + type: object + description: + Input port node, single endpoint describing the input pad. + port@1: + type: object + description: + Output port node, single endpoint, describing the output pad. + +additionalProperties: false + +required: + - compatible + - reg + - clocks + - clock-names + - ports + +examples: + - | + csi2dc@e1404000 { + compatible = "microchip,sama7g5-csi2dc"; + reg = <0xe1404000 0x500>; + clocks = <&pclk>, <&scck>; + clock-names = "pclk", "scck"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; /* must be 0, first child port */ + csi2dc_in: endpoint { /* input from IDI interface */ + remote-endpoint = <&csi2host_out>; + }; + }; + + port@1 { + reg = <1>; /* must be 1, second child port */ + csi2dc_out: endpoint { + remote-endpoint = <&xisc_in>; /* output to sensor controller */ + }; + }; + }; + }; + +... --2.25.1
Quoted 61 lines · Jacopo Mondi
Hi again,
Quoted 58 lines · Eugen Hristev
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 v5:
- modified bindings as per Rob Herring reviewChanges in v4:
- Removed property for inter-line-delay and for clock continuous/non-continuous
- Removed virtual channel by reg for second endpointChanges 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 descriptionChanges in v2:
- fixed warnings reported bydt_binding_check.../bindings/media/microchip,csi2dc.yaml | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+)create mode 100644
Documentation/devicetree/bindings/media/microchip,csi2dc.yamlDocumentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +26
-0@@ -0,0 +1,119 @@ +# 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
Quoted 8 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml line 31
Quoted 5 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +5
-0+ 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.txtIs it me or this file doesn't exists on the most recent media/master and on v5.10-rc4 ?
Hi Jacopo,
Thanks for reviewing this patch.
You are right. As I said at some point during the review, actually this controller has an input the IDI interface which is not currently in kernel. There is a patch series that adds the Synopsys platform driver, but nobody worked on it anymore and I use it for testing my driver. You can see the last version that was sent here:
https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
I sent this series to understand how to improve and how to get my driver in kernel, but indeed there are holes, especially this input type of bus which is not in the subsystem either.
So how would you suggest to move forward ? Remove the reference to the synopsys binding and just leave 'IDI interface' which is undocumented ?
Eugen
Quoted 92 lines
Quoted 92 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +88
-1+ + 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 + + ports: + type: object + description: + List of ports + + properties: + port@0: + type: object + description: + Input port node, single endpoint describing the input pad. + port@1: + type: object + description: + Output port node, single endpoint, describing the output pad. + +additionalProperties: false + +required: + - compatible + - reg + - clocks + - clock-names + - ports + +examples: + - | + csi2dc@e1404000 { + compatible = "microchip,sama7g5-csi2dc"; + reg = <0xe1404000 0x500>; + clocks = <&pclk>, <&scck>; + clock-names = "pclk", "scck"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; /* must be 0, first child port */ + csi2dc_in: endpoint { /* input from IDI interface */ + remote-endpoint = <&csi2host_out>; + }; + }; + + port@1 { + reg = <1>; /* must be 1, second child port */ + csi2dc_out: endpoint { + remote-endpoint = <&xisc_in>; /* output to sensor controller */ + }; + }; + }; + }; + +... --2.25.1
Hi Eugen,
Quoted 83 lines · On Tue, Nov 17, 2020 at 11:14:18AM +0000, [email protected] wrote:
Quoted 61 lines · Jacopo Mondi
Hi again,
Quoted 58 lines · Eugen Hristev
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 v5:
- modified bindings as per Rob Herring reviewChanges in v4:
- Removed property for inter-line-delay and for clock continuous/non-continuous
- Removed virtual channel by reg for second endpointChanges 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 descriptionChanges in v2:
- fixed warnings reported bydt_binding_check.../bindings/media/microchip,csi2dc.yaml | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+)create mode 100644
Documentation/devicetree/bindings/media/microchip,csi2dc.yamlDocumentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +26
-0@@ -0,0 +1,119 @@ +# 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 interfaceQuoted 8 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml line 31
Quoted 5 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +5
-0+ 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.txtIs it me or this file doesn't exists on the most recent media/master and on v5.10-rc4 ?
Hi Jacopo,
Thanks for reviewing this patch.
You are right. As I said at some point during the review, actually this controller has an input the IDI interface which is not currently in kernel. There is a patch series that adds the Synopsys platform driver, but nobody worked on it anymore and I use it for testing my driver. You can see the last version that was sent here:https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
Quoted 6 lines
I sent this series to understand how to improve and how to get my driver in kernel, but indeed there are holes, especially this input type of bus which is not in the subsystem either.
So how would you suggest to move forward ? Remove the reference to the synopsys binding and just leave 'IDI interface' which is undocumented ?
mmm, I see... I was hoping it was a simple CSI-2 tx :)
The Synopsys proposed bindings do not tell much as they don't describe the csi2host output port. I would just mention "Synopsys IDI compatible interface" without mentioning a file that cannot be accessed :)
Thanks
j
Quoted 96 lines
Eugen
Quoted 92 lines
Quoted 92 lines
Documentation/devicetree/bindings/media/microchip,csi2dc.yaml new file +88
-1+ + 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 + + ports: + type: object + description: + List of ports + + properties: + port@0: + type: object + description: + Input port node, single endpoint describing the input pad. + port@1: + type: object + description: + Output port node, single endpoint, describing the output pad. + +additionalProperties: false + +required: + - compatible + - reg + - clocks + - clock-names + - ports + +examples: + - | + csi2dc@e1404000 { + compatible = "microchip,sama7g5-csi2dc"; + reg = <0xe1404000 0x500>; + clocks = <&pclk>, <&scck>; + clock-names = "pclk", "scck"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; /* must be 0, first child port */ + csi2dc_in: endpoint { /* input from IDI interface */ + remote-endpoint = <&csi2host_out>; + }; + }; + + port@1 { + reg = <1>; /* must be 1, second child port */ + csi2dc_out: endpoint { + remote-endpoint = <&xisc_in>; /* output to sensor controller */ + }; + }; + }; + }; + +... --2.25.1