On 12.10.2020 10:15, Eugen Hristev - M18282 wrote:
> On 09.10.2020 17:58, Sakari Ailus wrote:
>
>> Hi Eugen,
>>
>> My apologies for the late reply.
>>
>
> Hi,
>
> Thank you for replying,
>
>> On Mon, Sep 07, 2020 at 09:16:57AM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:
>>> On 31.08.2020 11:50, Sakari Ailus wrote:
>>>
>>>> Hi Eugen,
>>>>
>>>> Thanks for the update.
>>>
>>> Hi Sakari,
>>>
>>> Thanks for reviewing, but, could you please help me understand your
>>> review below ?
>>>
>>>>
>>>> On Wed, Aug 26, 2020 at 09:51:41AM +0300, Eugen Hristev wrote:
>>>>> Microchip CSI2DC (CSI2 Demultiplexer Controller) is a misc bridge device
>>>>> that converts a byte stream in IDI Synopsys format (coming from a CSI2HOST)
>>>>> to a pixel stream that can be captured by a sensor controller.
>>>>>
>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
>>>>> ---
>>>>> Changes in v2:
>>>>> - moved driver to platform/atmel
>>>>> - fixed minor things as per Sakari's review
>>>>> - still some things from v2 review are not yet addressed, to be followed up
>>>>>
>>>>> drivers/media/platform/atmel/Kconfig | 13 +
>>>>> drivers/media/platform/atmel/Makefile | 1 +
>>>>> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++
>>>>> 3 files changed, 714 insertions(+)
>>>>> create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.c
>>>>>
>>>>> diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig
>>>>> index 1850fe7f9360..80bbddcc2504 100644
>>>>> --- a/drivers/media/platform/atmel/Kconfig
>>>>> +++ b/drivers/media/platform/atmel/Kconfig
>>>>> @@ -21,3 +21,16 @@ config VIDEO_ATMEL_ISI
>>>>> help
>>>>> This module makes the ATMEL Image Sensor Interface available
>>>>> as a v4l2 device.
>>>>> +
>>>>> +config VIDEO_MICROCHIP_CSI2DC
>>>>> + tristate "Microchip CSI2 Demux Controller"
>>>>> + depends on VIDEO_V4L2 && COMMON_CLK && OF
>>>>> + depends on ARCH_AT91 || COMPILE_TEST
>>>>> + select MEDIA_CONTROLLER
>>>>> + select VIDEO_V4L2_SUBDEV_API
>>>>> + select V4L2_FWNODE
>>>>> + help
>>>>> + CSI2 Demux Controller driver. CSI2DC is a helper chip
>>>>> + that converts IDI interface byte stream to a parallel pixel stream.
>>>>> + It supports various RAW formats as input.
>>>>> + Performs clock domain crossing between hardware blocks.
>>>>> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
>>>>> index 2dba38994a70..8af7c5b534c3 100644
>>>>> --- a/drivers/media/platform/atmel/Makefile
>>>>> +++ b/drivers/media/platform/atmel/Makefile
>>>>> @@ -3,3 +3,4 @@ atmel-isc-objs = atmel-sama5d2-isc.o atmel-isc-base.o
>>>>>
>>>>> obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>>>>> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
>>>>> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o
>>>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c
>>>>> new file mode 100644
>>>>> index 000000000000..97d07a80bbf7
>>>>> --- /dev/null
>>>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c
>>>>> @@ -0,0 +1,700 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Microchip CSI2 Demux Controller (CSI2DC) driver
>>>>> + *
>>>>> + * Copyright (C) 2018-2020 Microchip Technology, Inc.
>>>>> + *
>>>>> + * Author: Eugen Hristev <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)
>>>>> +
>>>>> +/* Global control register */
>>>>> +#define CSI2DC_GCTLR 0x04
>>>>> +#define CSI2DC_GCTLR_SWRST BIT(0)
>>>>> +
>>>>> +/* Global status register */
>>>>> +#define CSI2DC_GS 0x08
>>>>> +
>>>>> +/* SSP interrupt status register */
>>>>> +#define CSI2DC_SSPIS 0x28
>>>>> +/* Pipe update register */
>>>>> +#define CSI2DC_PU 0xC0
>>>>> +/* Video pipe attributes update */
>>>>> +#define CSI2DC_PU_VP BIT(0)
>>>>> +
>>>>> +/* Pipe update status register */
>>>>> +#define CSI2DC_PUS 0xC4
>>>>> +
>>>>> +/* Video pipeline enable register */
>>>>> +#define CSI2DC_VPE 0xF8
>>>>> +#define CSI2DC_VPE_ENABLE BIT(0)
>>>>> +
>>>>> +/* Video pipeline configuration register */
>>>>> +#define CSI2DC_VPCFG 0xFC
>>>>> +/* Data type */
>>>>> +#define CSI2DC_VPCFG_DT(v) ((v) << 0)
>>>>> +#define CSI2DC_VPCFG_DT_MASK GENMASK(5, 0)
>>>>> +/* Virtual channel identifier */
>>>>> +#define CSI2DC_VPCFG_VC(v) ((v) << 6)
>>>>> +#define CSI2DC_VPCFG_VC_MASK GENMASK(7, 6)
>>>>> +/* Decompression enable */
>>>>> +#define CSI2DC_VPCFG_DE BIT(8)
>>>>> +/* Decoder mode */
>>>>> +#define CSI2DC_VPCFG_DM(v) ((v) << 9)
>>>>> +#define CSI2DC_VPCFG_DM_DECODER8TO12 0
>>>>> +/* Decoder predictor 2 selection */
>>>>> +#define CSI2DC_VPCFG_DP2 BIT(12)
>>>>> +/* Recommended memory storage */
>>>>> +#define CSI2DC_VPCFG_RMS BIT(13)
>>>>> +/* Post adjustment */
>>>>> +#define CSI2DC_VPCFG_PA BIT(14)
>>>>> +
>>>>> +/* Video pipeline column register */
>>>>> +#define CSI2DC_VPCOL 0x100
>>>>> +/* Column number */
>>>>> +#define CSI2DC_VPCOL_COL(v) ((v) << 0)
>>>>> +#define CSI2DC_VPCOL_COL_MASK GENMASK(15, 0)
>>>>> +
>>>>> +/* Video pipeline row register */
>>>>> +#define CSI2DC_VPROW 0x104
>>>>> +/* Row number */
>>>>> +#define CSI2DC_VPROW_ROW(v) ((v) << 0)
>>>>> +#define CSI2DC_VPROW_ROW_MASK GENMASK(15, 0)
>>>>> +
>>>>> +/* Version register */
>>>>> +#define CSI2DC_VERSION 0x1FC
>>>>> +
>>>>> +/* register read/write helpers */
>>>>> +#define csi2dc_readl(st, reg) readl_relaxed((st)->base + (reg))
>>>>> +#define csi2dc_writel(st, reg, val) writel_relaxed((val), (st)->base + (reg))
>>>>> +
>>>>> +/* supported RAW data types */
>>>>> +#define CSI2DC_DT_RAW6 0x28
>>>>> +#define CSI2DC_DT_RAW7 0x29
>>>>> +#define CSI2DC_DT_RAW8 0x2A
>>>>> +#define CSI2DC_DT_RAW10 0x2B
>>>>> +#define CSI2DC_DT_RAW12 0x2C
>>>>> +#define CSI2DC_DT_RAW14 0x2D
>>>>> +
>>>>> +struct csi2dc_format {
>>>>> + u32 mbus_code;
>>>>> + u32 dt;
>>>>> +};
>>>>> +
>>>>> +static struct csi2dc_format csi2dc_formats_list[] = {
>>>>
>>>> const
>>>
>>> Elements from this array are being selected in a dynamic list, and
>>> pointed from a non-const array of selected working formats (at subdevice
>>> completion time). If I make this const , I will get compiler warnings
>>> when assigning to the non-const array.
>>> What solution do you suggest for this ?
>>
>> You'll need to make the other instances const, too. They do not need to be
>> modified I presume?
>
> That's the problem I am trying to explain. I have a dynamic (initially
> empty) array that is filled with elements from this array which yes, is
> constant, but if I qualify it as const, all const elements 'cannot be
> assigned to a non-const array' and if I make the dynamic array as const,
> then well, I cannot modify it.
>
>>
>>>
>>>
>>>>
>>>>> + {
>>>>> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
>>>>> + .dt = CSI2DC_DT_RAW10,
>>>>> + },
>>>>> +};
>>>>> +
>>>>> +enum mipi_csi_pads {
>>>>> + CSI2DC_PAD_SINK = 0,
>>>>> + CSI2DC_PAD_SOURCE = 1,
>>>>> + CSI2DC_PADS_NUM = 2,
>>>>> +};
>>>>> +
>>>>> +struct csi2dc_device {
>>>>> + void __iomem *base;
>>>>> + struct v4l2_subdev csi2dc_sd;
>>>>> + struct device *dev;
>>>>> + struct v4l2_device v4l2_dev;
>>>>
>>>> This device is a bridge; the V4L2 device should be registered by the master
>>>> driver, i.e. the one handling DMA and registering the video nodes. Please
>>>> drop this.
Hello Sakari,
Taking this v4l2_dev out, means that I am no longer able to create and
add a notifier to this 'csi2dc' device. This means that I can no longer
connect the subdevice to this device.
In this situation, how can I connect the subdevice in the pipeline to
the csi2dc node ? The subdevice is the 'csi2host' hardware block.
The pipeline looks like this:
Sensor -> (mipi csi2) -> csi2host -> (IDI interface) -> csi2dc ->
(parallel interface) -> image sensor controller (master driver)
The master driver is also written to cope with this pipeline
Sensor -> (parallel interface) -> image sensor controller (master driver)
The master driver awaits for the subdevice to have its 'completion'
ready, which in the first pipeline is the subdev that csi2dc is
registering, in the second case it's the sensor itself.
Could you please help me understand how to change this such that both
pipelines work ?
Thanks again for your review,
Eugen
>>>>
>>>>> + struct clk *pclk;
>>>>> + struct clk *scck;
>>>>> +
>>>>> + bool video_pipe;
>>>>> +
>>>>> + u32 num_fmts;
>>>>> + struct csi2dc_format **formats;
>>>>> +
>>>>> + struct csi2dc_format *cur_fmt;
>>>>> + struct csi2dc_format *try_fmt;
>>>>> +
>>>>> + struct media_pad pads[CSI2DC_PADS_NUM];
>>>>> +
>>>>> + bool clk_gated;
>>>>> + u32 inter_line_delay;
>>>>> + u32 vc;
>>>>> +
>>>>> + struct v4l2_async_subdev *asd;
>>>>> + struct v4l2_async_notifier notifier;
>>>>> +
>>>>> + struct v4l2_subdev *input_sd;
>>>>> + bool completed;
>>>>> +};
>>>>> +
>>>>> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc)
>>>>> +{
>>>>> + u32 vp;
>>>>> +
>>>>> + vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;
>>>>> + vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;
>>>>> + vp &= ~CSI2DC_VPCFG_DE;
>>>>> + vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);
>>>>> + vp &= ~CSI2DC_VPCFG_DP2;
>>>>> + vp &= ~CSI2DC_VPCFG_RMS;
>>>>> + vp |= CSI2DC_VPCFG_PA;
>>>>> +
>>>>> + csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);
>>>>> + csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);
>>>>> + csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);
>>>>> +}
>>>>> +
>>>>> +static inline struct csi2dc_device *
>>>>> +csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)
>>>>> +{
>>>>> + return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_pad_config *cfg,
>>>>> + struct v4l2_subdev_mbus_code_enum *code)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (code->index >= csi2dc->num_fmts)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + code->code = csi2dc->formats[code->index]->mbus_code;
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_try_fmt(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_pad_config *cfg,
>>>>> + struct v4l2_subdev_format *req_fmt)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> + struct csi2dc_format *fmt;
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + for (fmt = csi2dc->formats[0]; fmt; fmt++)
>>>>> + if (req_fmt->format.code == fmt->mbus_code)
>>>>> + csi2dc->try_fmt = fmt;
>>>>> +
>>>>> + /* in case we could not find the desired format, default to something */
>>>>> + if (!csi2dc->try_fmt ||
>>>>> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) {
>>>>> + csi2dc->try_fmt = csi2dc->formats[0];
>>>>> + req_fmt->format.code = csi2dc->formats[0]->mbus_code;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_pad_config *cfg,
>>>>> + struct v4l2_subdev_format *req_fmt)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> + int ret;
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + csi2dc_try_fmt(csi2dc_sd, cfg, req_fmt);
>>>>> +
>>>>> + if (csi2dc->try_fmt->mbus_code != req_fmt->format.code) {
>>>>> + dev_dbg(csi2dc->dev, "CSI2DC unsupported format 0x%x\n",
>>>>> + req_fmt->format.code);
>>>>
>>>> Please adjust the format into a supported one instead.
>>>
>>> I thought that only the 'try_fmt' calls should adapt to something that
>>> works. It is the same case for set_fmt ? This function cannot return
>>> invalid format ?
>>
>> Correct. set_fmt should always return with a valid format.
>>
>> Perhaps merging csi2dc_try_fmt() to this function would help? It's not used
>> elsewhere. It seems you're doing some redundant checks here, too.
>
> But this would mean that this driver will not have a pure 'try_fmt' and
> what will happen on try attempts from the master driver ?
>
>>
>>>>
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);
>>>>> + if (ret) {
>>>>> + dev_err(csi2dc->dev, "input subdev failed %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + csi2dc->cur_fmt = csi2dc->try_fmt;
>>>>> + /* update video pipe */
>>>>> + csi2dc_vp_update(csi2dc);
>>>>> +
>>>>> + dev_dbg(csi2dc->dev, "CSI2DC new format: 0x%x\n", req_fmt->format.code);
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_formats_init(struct csi2dc_device *csi2dc)
>>>>> +{
>>>>> + int i, j;
>>>>> + struct csi2dc_format *fmt = &csi2dc_formats_list[0];
>>>>> +
>>>>> + csi2dc->num_fmts = 1;
>>>>> +
>>>>> + csi2dc->formats = devm_kcalloc(csi2dc->dev, csi2dc->num_fmts,
>>>>> + sizeof(*csi2dc->formats), GFP_KERNEL);
>>>>> +
>>>>> + for (i = 0, j = 0; i < ARRAY_SIZE(csi2dc_formats_list); i++, fmt++)
>>>>> + if (fmt->mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10)
>>>>> + csi2dc->formats[j++] = fmt;
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_s_power(struct v4l2_subdev *csi2dc_sd, int on)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> + int ret = 0;
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + if (on)
>>>>> + ret = clk_prepare_enable(csi2dc->scck);
>>>>> + else
>>>>> + clk_disable_unprepare(csi2dc->scck);
>>>>> + if (ret)
>>>>> + dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret);
>>>>> +
>>>>> + /* if powering up, deassert reset line */
>>>>> + if (on)
>>>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);
>>>>> +
>>>>> + ret = v4l2_subdev_call(csi2dc->input_sd, core, s_power, on);
>>>>> +
>>>>> + /* if powering down, assert reset line */
>>>>> + if (!on)
>>>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST);
>>>>
>>>> These go to the runtime resume and suspend callbacks.
>>>
>>> Okay I will update
>
> Another question is what happens when CONFIG_PM is not used, thus we do
> not have runtime resume and suspend callbacks ?
>
>>>
>>>>
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_g_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_frame_interval *interval)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, video, g_frame_interval,
>>>>> + interval);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_s_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_frame_interval *interval)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_frame_interval,
>>>>> + interval);
>>>>
>>>> If your driver is used with a MC-enabled master driver, you can drop all
>>>> the v4l2_subdev_call() macros apart from the ones related to s_stream
>>>> operation.
>>>
>>> And the set frame interval for example, will it be sent to the sensor
>>> automatically ?
>>> I noticed that such operations on the master driver were not propagated
>>> to the underlying subdevice. How does the master device 'know' how to
>>> call the input_sd ? this input_sd is not referenced anywhere in the
>>> master driver.
>>
>> That's correct. In MC-centric drivers it's the responsibility of the user
>> space to configure the entire pipeline.
>>
>> As your device is a CSI-2 to parallel (?) converter, it may well be part of
>> a complex pipeline that the master driver could not know about.
>>
>> I remember there was an existing stm parallel receiver that was tightly
>> coupled by a CSI-2 to parallel IP block where the V4L2-centric model was
>> used. But virtually all devices that are not just simple sensor + receiver
>> with DMA are MC-centric.
>
> Without these calls to the subdevice, the subdevice (csi2host and
> sensor) are not aware of anything configured on the master driver. How
> are such settings sent to the sensor ?
>
> for example v4l2-ctl --list-formats-ext
>
>
> # v4l2-ctl -d /dev/video0 --list-formats-ext
> ioctl: VIDIOC_ENUM_FMT
> Type: Video Capture
>
> [0]: 'AR12' (16-bit ARGB 4-4-4-4)
> Size: Discrete 3280x2464
> Size: Discrete 1920x1080
> Size: Discrete 1640x1232
> [1]: 'AR15' (16-bit ARGB 1-5-5-5)
> Size: Discrete 3280x2464
> Size: Discrete 1920x1080
> Size: Discrete 1640x1232
> [2]: 'RGBP' (16-bit RGB 5-6-5)
> Size: Discrete 3280x2464
> Size: Discrete 1920x1080
> Size: Discrete 1640x1232
>
> All this information is obtained because of the enum_frame_size callback
> to the subdevice
>
> If I remove these,
>
> # v4l2-ctl -d /dev/video0 --list-formats-ext
> ioctl: VIDIOC_ENUM_FMT
> Type: Video Capture
>
> [0]: 'AR12' (16-bit ARGB 4-4-4-4)
> [1]: 'AR15' (16-bit ARGB 1-5-5-5)
> [2]: 'RGBP' (16-bit RGB 5-6-5)
> [3]: 'AR24' (32-bit BGRA 8-8-8-8)
> [4]: 'XR24' (32-bit BGRX 8-8-8-8)
> [5]: 'YU12' (Planar YUV 4:2:0)
> [6]: 'UYVY' (UYVY 4:2:2)
> [7]: 'VYUY' (VYUY 4:2:2)
> [8]: 'YUYV' (YUYV 4:2:2)
> [9]: '422P' (Planar YUV 4:2:2)
> [10]: 'GREY' (8-bit Greyscale)
> [11]: 'Y10 ' (10-bit Greyscale)
> [12]: 'Y16 ' (16-bit Greyscale)
> [13]: 'RG10' (10-bit Bayer RGRG/GBGB)
>
>
> All the frame information is no longer available.
>
> So how can I obtain this information if I remove these callbacks that
> you say they should be removed ?
>
>>
>>>
>>>>
>>>> If not, then you can drop the HAS_DEVNODE flag from the subdev flags. But
>>>> I'd try to avoid this route if possible.
>>>>
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_enum_frame_size(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_pad_config *cfg,
>>>>> + struct v4l2_subdev_frame_size_enum *fse)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_size, cfg,
>>>>> + fse);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_enum_frame_interval(struct v4l2_subdev *csi2dc_sd,
>>>>> + struct v4l2_subdev_pad_config *cfg,
>>>>> + struct v4l2_subdev_frame_interval_enum *fie)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + if (!csi2dc->completed) {
>>>>> + dev_dbg((csi2dc)->dev, "subdev not registered yet\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_interval, cfg,
>>>>> + fie);
>>>>> +}
>>>>> +
>>>>> +static const struct v4l2_subdev_core_ops csi2dc_core_ops = {
>>>>> + .s_power = csi2dc_s_power,
>>>>
>>>> Please use runtime PM instead.
>
> What happens if we do not have runtime PM ? (CONFIG_PM=n)
>>>>
>>>>> +};
>>>>> +
>>>>> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {
>>>>> + .enum_mbus_code = csi2dc_enum_mbus_code,
>>>>> + .set_fmt = csi2dc_set_fmt,
>>>>> + .enum_frame_size = csi2dc_enum_frame_size,
>>>>> + .enum_frame_interval = csi2dc_enum_frame_interval,
>>>>> +};
>>>>> +
>>>>> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = {
>>>>> + .s_stream = csi2dc_s_stream,
>>>>> + .g_frame_interval = csi2dc_g_frame_interval,
>>>>> + .s_frame_interval = csi2dc_s_frame_interval,
>>>>> +};
>>>>> +
>>>>> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = {
>>>>> + .core = &csi2dc_core_ops,
>>>>> + .pad = &csi2dc_pad_ops,
>>>>> + .video = &csi2dc_video_ops,
>>>>> +};
>>>>> +
>>>>> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,
>>>>> + struct v4l2_subdev *subdev,
>>>>> + struct v4l2_async_subdev *asd)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc = container_of(notifier->v4l2_dev,
>>>>> + struct csi2dc_device, v4l2_dev);
>>>>> + csi2dc->input_sd = subdev;
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_async_complete(struct v4l2_async_notifier *notifier)
>>>>> +{
>>>>> + struct csi2dc_device *csi2dc =
>>>>> + container_of(notifier->v4l2_dev, struct csi2dc_device, v4l2_dev);
>>>>
>>>> Over 80 characters per line. Please check the rest of the driver, too.
>>>>
>>>>> + int ret;
>>>>> +
>>>>> + ret = v4l2_device_register_subdev_nodes(&csi2dc->v4l2_dev);
>>>>> + if (ret < 0) {
>>>>> + v4l2_err(&csi2dc->v4l2_dev, "failed to register subdev nodes\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + csi2dc_writel(csi2dc, CSI2DC_GCFG,
>>>>> + (CSI2DC_GCFG_HLC(csi2dc->inter_line_delay) &
>>>>> + CSI2DC_GCFG_HLC_MASK) |
>>>>> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));
>>>>> +
>>>>> + csi2dc_writel(csi2dc, CSI2DC_VPCOL,
>>>>> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK);
>>>>> + csi2dc_writel(csi2dc, CSI2DC_VPROW,
>>>>> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK);
>>>>> +
>>>>> + csi2dc->completed = true;
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = {
>>>>> + .bound = csi2dc_async_bound,
>>>>> + .complete = csi2dc_async_complete,
>>>>> +};
>>>>> +
>>>>> +static void csi2dc_cleanup_notifier(struct csi2dc_device *csi2dc)
>>>>> +{
>>>>> + v4l2_async_notifier_unregister(&csi2dc->notifier);
>>>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,
>>>>> + struct device_node *input_parent)
>>>>> +{
>>>>> + int ret;
>>>>> +
>>>>> + v4l2_async_notifier_init(&csi2dc->notifier);
>>>>> +
>>>>> + csi2dc->asd = kzalloc(sizeof(*csi2dc->asd), GFP_KERNEL);
>>>>> + if (!csi2dc->asd)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + csi2dc->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
>>>>> + csi2dc->asd->match.fwnode = of_fwnode_handle(input_parent);
>>>>> +
>>>>> + ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd);
>>>>> + if (ret) {
>>>>> + dev_err(csi2dc->dev, "failed to add async notifier.\n");
>>>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier);
>>>>> + goto csi2dc_prepare_notifier_err;
>>>>> + }
>>>>> +
>>>>> + csi2dc->notifier.ops = &csi2dc_async_ops;
>>>>> +
>>>>> + ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev,
>>>>> + &csi2dc->notifier);
>>>>> +
>>>>> + if (ret) {
>>>>> + dev_err(csi2dc->dev, "fail to register async notifier.\n");
>>>>> + goto csi2dc_prepare_notifier_err;
>>>>> + }
>>>>> +
>>>>> +csi2dc_prepare_notifier_err:
>>>>> + of_node_put(input_parent);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc,
>>>>> + struct device_node *of_node)
>>>>> +{
>>>>> + struct device_node *input_node, *sink_node, *input_parent;
>>>>> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, sink_endpoint = { 0 };
>>>>> + int ret;
>>>>> +
>>>>> + input_node = of_graph_get_next_endpoint(of_node, NULL);
>>>>> +
>>>>> + if (!input_node) {
>>>>> + dev_err(csi2dc->dev, "missing port node at %s, input node is mandatory.\n",
>>>>> + of_node->full_name);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node),
>>>>> + &input_endpoint);
>>>>> +
>>>>> + if (ret) {
>>>>> + dev_err(csi2dc->dev, "endpoint not defined at %s\n",
>>>>> + of_node->full_name);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + input_parent = of_graph_get_remote_port_parent(input_node);
>>>>> + if (!input_parent) {
>>>>> + dev_err(csi2dc->dev, "could not get input node's parent node.\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + sink_node = of_graph_get_next_endpoint(of_node, input_node);
>>>>> +
>>>>> + if (sink_node)
>>>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(sink_node),
>>>>> + &sink_endpoint);
>>>>> +
>>>>> + if (!sink_node || ret) {
>>>>> + dev_info(csi2dc->dev, "missing sink node at %s, data pipe available only.\n",
>>>>> + of_node->full_name);
>>>>> + } else {
>>>>> + csi2dc->video_pipe = true;
>>>>> + csi2dc->vc = sink_endpoint.base.id;
>>>>> +
>>>>> + dev_dbg(csi2dc->dev, "block %s %d.%d->%d.%d video pipe VC %d\n",
>>>>> + of_node->full_name, input_endpoint.base.port,
>>>>> + input_endpoint.base.id, sink_endpoint.base.port,
>>>>> + sink_endpoint.base.id, csi2dc->vc);
>>>>> + }
>>>>> +
>>>>> + csi2dc->clk_gated = of_property_read_bool(of_node,
>>>>> + "microchip,clk-gated");
>>>>> +
>>>>> + dev_dbg(csi2dc->dev, "%s clock\n",
>>>>> + csi2dc->clk_gated ? "Gated" : "Free running");
>>>>> +
>>>>> + ret = of_property_read_u32(of_node, "microchip,inter-line-delay",
>>>>> + &csi2dc->inter_line_delay);
>>>>> +
>>>>> + if (ret || csi2dc->inter_line_delay > 16 ||
>>>>> + csi2dc->inter_line_delay == 0) {
>>>>> + dev_dbg(csi2dc->dev, "assuming inter line delay = 16 clocks");
>>>>> + csi2dc->inter_line_delay = 16;
>>>>> + ret = 0;
>>>>> + }
>>>>> + /* hardware automatically adds 1 */
>>>>> + csi2dc->inter_line_delay--;
>>>>> +
>>>>> + /* prepare async notifier for subdevice completion */
>>>>> +
>>>>> + of_node_put(sink_node);
>>>>> + of_node_put(input_node);
>>>>> +
>>>>> + return csi2dc_prepare_notifier(csi2dc, input_parent);
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_probe(struct platform_device *pdev)
>>>>> +{
>>>>> + struct device *dev = &pdev->dev;
>>>>> + struct csi2dc_device *csi2dc;
>>>>> + struct resource *res = NULL;
>>>>> + int ret = 0;
>>>>> +
>>>>> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);
>>>>> + if (!csi2dc)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + csi2dc->dev = dev;
>>>>> +
>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> + if (!res)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + csi2dc->base = devm_ioremap_resource(dev, res);
>>>>> +
>>>>> + if (IS_ERR(csi2dc->base)) {
>>>>> + dev_err(dev, "base address not set\n");
>>>>> + return PTR_ERR(csi2dc->base);
>>>>> + }
>>>>> +
>>>>> + csi2dc->pclk = devm_clk_get(dev, "pclk");
>>>>> + if (IS_ERR(csi2dc->pclk)) {
>>>>> + ret = PTR_ERR(csi2dc->pclk);
>>>>> + dev_err(dev, "failed to get pclk: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + ret = clk_prepare_enable(csi2dc->pclk);
>>>>> + if (ret) {
>>>>> + dev_err(dev, "failed to enable pclk: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + csi2dc->scck = devm_clk_get(dev, "scck");
>>>>> + if (IS_ERR(csi2dc->scck)) {
>>>>> + ret = PTR_ERR(csi2dc->scck);
>>>>> + dev_err(dev, "failed to get scck: %d\n", ret);
>>>>> + goto csi2dc_clk_fail;
>>>>> + }
>>>>> +
>>>>> + ret = v4l2_device_register(dev, &csi2dc->v4l2_dev);
>>>>> + if (ret) {
>>>>> + dev_err(dev, "unable to register v4l2 device.\n");
>>>>> + goto csi2dc_clk_fail;
>>>>> + }
>>>>> +
>>>>> + v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);
>>>>> +
>>>>> + csi2dc->csi2dc_sd.owner = THIS_MODULE;
>>>>> + csi2dc->csi2dc_sd.dev = dev;
>>>>> + snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),
>>>>> + "CSI2DC.0");
>>>>> +
>>>>> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>>>> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>>>>> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>>>>> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
>>>>> +
>>>>> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, CSI2DC_PADS_NUM,
>>>>> + csi2dc->pads);
>>>>> + if (ret < 0) {
>>>>> + dev_err(dev, "media entity init failed\n");
>>>>> + goto csi2dc_probe_entity_err;
>>>>> + }
>>>>> +
>>>>> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev);
>>>>> +
>>>>> + platform_set_drvdata(pdev, &csi2dc->csi2dc_sd);
>>>>> +
>>>>> + ret = csi2dc_of_parse(csi2dc, dev->of_node);
>>>>> + if (ret)
>>>>> + goto csi2dc_probe_entity_err;
>>>>> +
>>>>> + ret = csi2dc_formats_init(csi2dc);
>>>>> + if (ret)
>>>>> + goto csi2dc_probe_error;
>>>>> +
>>>>> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);
>>>>> + if (ret)
>>>>> + goto csi2dc_probe_error;
>>>>> +
>>>>> + pr_info("Microchip CSI2DC version %x\n",
>>>>> + csi2dc_readl(csi2dc, CSI2DC_VERSION));
>>>>
>>>> dev_info(), please. Or just dev_dbg().
>>>>
>>>>> +
>>>>> + pm_runtime_set_active(dev);
>>>>> + pm_runtime_enable(dev);
>>>>> + pm_request_idle(dev);
>>>>> +
>>>>> + return 0;
>>>>> +
>>>>> +csi2dc_probe_error:
>>>>> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);
>>>>> + csi2dc_cleanup_notifier(csi2dc);
>>>>> +
>>>>> +csi2dc_probe_entity_err:
>>>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>>>>> + v4l2_device_unregister(&csi2dc->v4l2_dev);
>>>>> +csi2dc_clk_fail:
>>>>> + clk_disable_unprepare(csi2dc->pclk);
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int csi2dc_remove(struct platform_device *pdev)
>>>>> +{
>>>>> + struct v4l2_subdev *csi2dc_sd = platform_get_drvdata(pdev);
>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>>> +
>>>>> + pm_runtime_disable(&pdev->dev);
>>>>> + csi2dc_cleanup_notifier(csi2dc);
>>>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>>>>> + v4l2_device_unregister(&csi2dc->v4l2_dev);
>>>>> + clk_disable_unprepare(csi2dc->pclk);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)
>>>>> +{
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev)
>>>>> +{
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {
>>>>> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)
>>>>> +};
>>>>> +
>>>>> +static const struct of_device_id csi2dc_of_match[] = {
>>>>> + { .compatible = "microchip,sama7g5-csi2dc" },
>>>>> + { }
>>>>> +};
>>>>> +
>>>>> +MODULE_DEVICE_TABLE(of, csi2dc_of_match);
>>>>> +
>>>>> +static struct platform_driver csi2dc_driver = {
>>>>> + .probe = csi2dc_probe,
>>>>> + .remove = csi2dc_remove,
>>>>> + .driver = {
>>>>> + .name = "microchip-csi2dc",
>>>>> + .pm = &csi2dc_dev_pm_ops,
>>>>> + .of_match_table = of_match_ptr(csi2dc_of_match),
>>>>> + },
>>>>> +};
>>>>> +
>>>>> +module_platform_driver(csi2dc_driver);
>>>>> +
>>>>> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>");
>>>>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>> +MODULE_SUPPORTED_DEVICE("video");
>>>>
>>>> Hmm. This line needs to be more specific. I'd just drop it.
>>>>
>>>> --
>>>> Kind regards,
>>>>
>>>> Sakari Ailus
>>>>
>>>
>>
>> --
>> Regards,
>>
>> Sakari Ailus
>>
>
