On 18.11.2020 11:11, Jacopo Mondi wrote: > Hello Eugen, > > On Tue, Nov 17, 2020 at 05:24:30PM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote: >> On 17.11.2020 14:09, Laurent Pinchart wrote: >>> Hello everybody, >>> >>> On Tue, Nov 17, 2020 at 12:59:02PM +0100, Jacopo Mondi wrote: >>>> On Thu, Nov 12, 2020 at 03:34:36PM +0200, Eugen Hristev wrote: >>>>> Microchip CSI2DC (CSI2 Demultiplexer Controller) is a misc bridge device >>>>> that converts a byte stream in IDI Synopsys format (coming from a CSI2HOST) >>>>> to a pixel stream that can be captured by a sensor controller. >>>>> >>>>> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> >>>>> --- >>>>> 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. >>>> >>>> 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. >>> >>> Sub-notifiers were invited for this purpose, so I think they're the >>> right tool. >>> >>>>> 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. >>> >>> 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. >>> >>>> 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. >>> >>> 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 ? > > 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. > > It's really (conceptually) not different than doing the same using > media-ctl and v4l2-ctl. > >> 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 ? > > Media links on a media graph then need to be enabled opportunely > depending on the desired use case, the platform topology etc > >> 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 ? 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'. > > 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 ? > >> >> 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) 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 > > 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 > >> >>> >>>> Some more comments on the implementation below >>>> >>>>> 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 >>>>> >>>>> diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig >>>>> index 99b51213f871..147e1c14129b 100644 >>>>> --- a/drivers/media/platform/atmel/Kconfig >>>>> +++ b/drivers/media/platform/atmel/Kconfig >>>>> @@ -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. >>>> >>>> Is the last phrase necessary ? Also missing the usual >>>> >>>> To compile this driver as a module, choose M here: the >>>> module will be called ... >>>> >>>>> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile >>>>> index c5c01556c653..8e80af500bf5 100644 >>>>> --- a/drivers/media/platform/atmel/Makefile >>>>> +++ b/drivers/media/platform/atmel/Makefile >>>>> @@ -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 >>>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c >>>>> new file mode 100644 >>>>> index 000000000000..fda1d5882dbb >>>>> --- /dev/null >>>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c >>>>> @@ -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-later >>>> >>>>> +/* >>>>> + * Microchip CSI2 Demux Controller (CSI2DC) driver >>>>> + * >>>>> + * Copyright (C) 2018-2020 Microchip Technology, Inc. >>>>> + * >>>>> + * Author: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> >>>>> + * >>>>> + */ >>>>> + >>>>> +#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; >>>>> + >>>>> + 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 >>>> >>>>> + >>>>> + if (!num_fmts) >>>>> + return -ENXIO; >>>>> + >>>>> + csi2dc->num_fmts = num_fmts; >>>>> + >>>>> + return 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 ? >>>> >>>>> + >>>>> + if (csi2dc->powered_on == on) >>>>> + return 0; >>>> >>>> Same question >>>> >>>>> + >>>>> + if (on) >>>>> + ret = clk_prepare_enable(csi2dc->scck); >>>>> + else >>>>> + clk_disable_unprepare(csi2dc->scck); >>>>> + if (ret) >>>>> + dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret); >>>>> + >>>>> + /* if powering up, deassert reset line */ >>>>> + if (on) >>>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST); >>>>> + >>>>> + /* 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 >>>> } >>>>> + >>>>> + 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); >>>>> + 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. >>>> >>>>> + } >>>>> + >>>>> + 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, >>>>> + 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. >>>> >>>>> + >>>>> + 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) >>>>> +{ >>>>> + 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. >>>> >>>>> + >>>>> + ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd); >>>>> + if (ret) { >>>>> + dev_err(csi2dc->dev, "failed to add async notifier.\n"); >>>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier); >>>>> + goto csi2dc_prepare_notifier_err; >>>>> + } >>>>> + >>>>> + csi2dc->notifier.ops = &csi2dc_async_ops; >>>>> + >>>>> + ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev, >>>>> + &csi2dc->notifier); >>>>> + >>>>> + if (ret) { >>>>> + dev_err(csi2dc->dev, "fail to register async notifier.\n"); >>>> >>>> Shouldn't you cleanup the notifier if registration fails ? >>>> >>>>> + goto csi2dc_prepare_notifier_err; >>>>> + } >>>>> + >>>>> +csi2dc_prepare_notifier_err: >>>>> + of_node_put(input_parent); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc, >>>>> + struct device_node *of_node) >>>>> +{ >>>>> + struct device_node *input_node, *sink_node, *input_parent; >>>>> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, >>>>> + sink_endpoint = { 0 }; >>>>> + int ret; >>>>> + >>>>> + input_node = of_graph_get_next_endpoint(of_node, NULL); >>>>> + >>>>> + if (!input_node) { >>>>> + dev_err(csi2dc->dev, >>>>> + "missing port node at %s, input node is mandatory.\n", >>>> >>>> Please use %pOF to print the node name >>>> Also, input_node should be put in the error path >>>> >>>>> + 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 ?) >>>> >>>>> + >>>>> + 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) >>>> >>>>> + 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 >>>> >>>>> + >>>>> + 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. >>>> >>>>> + 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 >>>> >>>>> + >>>>> + 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); >>>>> +} >>>>> + >>>>> +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 ? >>>> >>>>> + >>>>> + 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 ? >>>> >>>>> +} >>>>> + >>>>> +static int csi2dc_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct device *dev = &pdev->dev; >>>>> + struct csi2dc_device *csi2dc; >>>>> + struct resource *res = NULL; >>>>> + int ret = 0; >>>>> + >>>>> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL); >>>>> + if (!csi2dc) >>>>> + return -ENOMEM; >>>>> + >>>>> + csi2dc->dev = dev; >>>>> + >>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> + if (!res) >>>>> + return -EINVAL; >>>> >>>> devm_ioremap_resource() checks for the validity of 'res' so you can >>>> drop the previous two lines >>>> >>>>> + >>>>> + csi2dc->base = devm_ioremap_resource(dev, res); >>>>> + >>>> >>>> Is the empy line intentional, sometimes you have one, sometimes you >>>> don't ? >>>> >>>>> + 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 >>>> >>>>> + } >>>>> + >>>>> + 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 ? >>>> >>>>> + >>>>> + csi2dc->scck = devm_clk_get(dev, "scck"); >>>>> + if (IS_ERR(csi2dc->scck)) { >>>>> + ret = PTR_ERR(csi2dc->scck); >>>>> + dev_err(dev, "failed to get scck: %d\n", ret); >>>>> + goto csi2dc_clk_fail; >>>>> + } >>>>> + >>>>> + ret = v4l2_device_register(dev, &csi2dc->v4l2_dev); >>>>> + if (ret) { >>>>> + dev_err(dev, "unable to register v4l2 device.\n"); >>>>> + goto csi2dc_clk_fail; >>>>> + } >>>>> + >>>>> + v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops); >>>>> + >>>>> + csi2dc->csi2dc_sd.owner = THIS_MODULE; >>>>> + csi2dc->csi2dc_sd.dev = dev; >>>>> + snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name), >>>>> + "CSI2DC.0"); >>>>> + >>>>> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >>>>> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; >>>>> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK; >>>>> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; >>>> >>>> Isn't the source pad presence conditional to the presence of >>>> port@1 ? >>>> >>>>> + >>>>> + 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), >>>>> + }, >>>>> +}; >>>>> + >>>>> +module_platform_driver(csi2dc_driver); >>>>> + >>>>> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>"); >>>>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver"); >>>>> +MODULE_LICENSE("GPL v2"); >>>>> +MODULE_SUPPORTED_DEVICE("video"); >>>> >>>> For my education, what's MODULE_SUPPORTED_DEVICE for ? >>> >>> It's "not yet implemented" since 2005, I think it can be dropped :-) >>> >>> -- >>> Regards, >>> >>> Laurent Pinchart >>> >>
