[v3,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.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
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

On 07.09.2020 12:16, Eugen Hristev - M18282 wrote:
> On 31.08.2020 11:50, Sakari Ailus wrote:

> 

>> Hi Eugen,

>>

>> Thanks for the update.

> 

> Hi Sakari,

> 

> Thanks for reviewing, but, could you please help me understand your

> review below ?

> 


Gentle ping

Thanks

>>

>> On Wed, Aug 26, 2020 at 09:51:41AM +0300, Eugen Hristev wrote:

>>> Microchip CSI2DC (CSI2 Demultiplexer Controller) is a misc bridge device

>>> that converts a byte stream in IDI Synopsys format (coming from a CSI2HOST)

>>> to a pixel stream that can be captured by a sensor controller.

>>>

>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>

>>> ---

>>> 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@microchip.com>

>>> + *

>>> + */

>>> +

>>> +#include <linux/clk.h>

>>> +#include <linux/module.h>

>>> +#include <linux/of.h>

>>> +#include <linux/of_graph.h>

>>> +#include <linux/platform_device.h>

>>> +#include <linux/pm_runtime.h>

>>> +#include <linux/videodev2.h>

>>> +

>>> +#include <media/v4l2-device.h>

>>> +#include <media/v4l2-fwnode.h>

>>> +#include <media/v4l2-subdev.h>

>>> +#include <media/videobuf2-dma-contig.h>

>>> +

>>> +/* Global configuration register */

>>> +#define CSI2DC_GCFG                  0x0

>>> +

>>> +/* MIPI sensor pixel clock is free running */

>>> +#define CSI2DC_GCFG_MIPIFRN          BIT(0)

>>> +/* Output waveform inter-line minimum delay */

>>> +#define CSI2DC_GCFG_HLC(v)           ((v) << 4)

>>> +#define CSI2DC_GCFG_HLC_MASK         GENMASK(7, 4)

>>> +

>>> +/* Global control register */

>>> +#define CSI2DC_GCTLR                 0x04

>>> +#define CSI2DC_GCTLR_SWRST           BIT(0)

>>> +

>>> +/* Global status register */

>>> +#define CSI2DC_GS                    0x08

>>> +

>>> +/* SSP interrupt status register */

>>> +#define CSI2DC_SSPIS                 0x28

>>> +/* Pipe update register */

>>> +#define CSI2DC_PU                    0xC0

>>> +/* Video pipe attributes update */

>>> +#define CSI2DC_PU_VP                 BIT(0)

>>> +

>>> +/* Pipe update status register */

>>> +#define CSI2DC_PUS                   0xC4

>>> +

>>> +/* Video pipeline enable register */

>>> +#define CSI2DC_VPE                   0xF8

>>> +#define CSI2DC_VPE_ENABLE            BIT(0)

>>> +

>>> +/* Video pipeline configuration register */

>>> +#define CSI2DC_VPCFG                 0xFC

>>> +/* Data type */

>>> +#define CSI2DC_VPCFG_DT(v)           ((v) << 0)

>>> +#define CSI2DC_VPCFG_DT_MASK         GENMASK(5, 0)

>>> +/* Virtual channel identifier */

>>> +#define CSI2DC_VPCFG_VC(v)           ((v) << 6)

>>> +#define CSI2DC_VPCFG_VC_MASK         GENMASK(7, 6)

>>> +/* Decompression enable */

>>> +#define CSI2DC_VPCFG_DE                      BIT(8)

>>> +/* Decoder mode */

>>> +#define CSI2DC_VPCFG_DM(v)           ((v) << 9)

>>> +#define CSI2DC_VPCFG_DM_DECODER8TO12 0

>>> +/* Decoder predictor 2 selection */

>>> +#define CSI2DC_VPCFG_DP2             BIT(12)

>>> +/* Recommended memory storage */

>>> +#define CSI2DC_VPCFG_RMS             BIT(13)

>>> +/* Post adjustment */

>>> +#define CSI2DC_VPCFG_PA                      BIT(14)

>>> +

>>> +/* Video pipeline column register */

>>> +#define CSI2DC_VPCOL                 0x100

>>> +/* Column number */

>>> +#define CSI2DC_VPCOL_COL(v)          ((v) << 0)

>>> +#define CSI2DC_VPCOL_COL_MASK                GENMASK(15, 0)

>>> +

>>> +/* Video pipeline row register */

>>> +#define CSI2DC_VPROW                 0x104

>>> +/* Row number */

>>> +#define CSI2DC_VPROW_ROW(v)          ((v) << 0)

>>> +#define CSI2DC_VPROW_ROW_MASK                GENMASK(15, 0)

>>> +

>>> +/* Version register */

>>> +#define CSI2DC_VERSION                       0x1FC

>>> +

>>> +/* register read/write helpers */

>>> +#define csi2dc_readl(st, reg)                readl_relaxed((st)->base + (reg))

>>> +#define csi2dc_writel(st, reg, val)  writel_relaxed((val), (st)->base + (reg))

>>> +

>>> +/* supported RAW data types */

>>> +#define CSI2DC_DT_RAW6                       0x28

>>> +#define CSI2DC_DT_RAW7                       0x29

>>> +#define CSI2DC_DT_RAW8                       0x2A

>>> +#define CSI2DC_DT_RAW10                      0x2B

>>> +#define CSI2DC_DT_RAW12                      0x2C

>>> +#define CSI2DC_DT_RAW14                      0x2D

>>> +

>>> +struct csi2dc_format {

>>> +     u32                             mbus_code;

>>> +     u32                             dt;

>>> +};

>>> +

>>> +static struct csi2dc_format csi2dc_formats_list[] = {

>>

>> const

> 

> Elements from this array are being selected in a dynamic list, and

> pointed from a non-const array of selected working formats (at subdevice

> completion time). If I make this const , I will get compiler warnings

> when assigning to the non-const array.

> What solution do you suggest for this ?

> 

> 

>>

>>> +     {

>>> +             .mbus_code =            MEDIA_BUS_FMT_SRGGB10_1X10,

>>> +             .dt =                   CSI2DC_DT_RAW10,

>>> +     },

>>> +};

>>> +

>>> +enum mipi_csi_pads {

>>> +     CSI2DC_PAD_SINK                 = 0,

>>> +     CSI2DC_PAD_SOURCE               = 1,

>>> +     CSI2DC_PADS_NUM                 = 2,

>>> +};

>>> +

>>> +struct csi2dc_device {

>>> +     void __iomem                    *base;

>>> +     struct v4l2_subdev              csi2dc_sd;

>>> +     struct device                   *dev;

>>> +     struct v4l2_device              v4l2_dev;

>>

>> This device is a bridge; the V4L2 device should be registered by the master

>> driver, i.e. the one handling DMA and registering the video nodes. Please

>> drop this.

>>

>>> +     struct clk                      *pclk;

>>> +     struct clk                      *scck;

>>> +

>>> +     bool                            video_pipe;

>>> +

>>> +     u32                             num_fmts;

>>> +     struct csi2dc_format            **formats;

>>> +

>>> +     struct csi2dc_format            *cur_fmt;

>>> +     struct csi2dc_format            *try_fmt;

>>> +

>>> +     struct media_pad                pads[CSI2DC_PADS_NUM];

>>> +

>>> +     bool                            clk_gated;

>>> +     u32                             inter_line_delay;

>>> +     u32                             vc;

>>> +

>>> +     struct v4l2_async_subdev        *asd;

>>> +     struct v4l2_async_notifier      notifier;

>>> +

>>> +     struct v4l2_subdev              *input_sd;

>>> +     bool                            completed;

>>> +};

>>> +

>>> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc)

>>> +{

>>> +     u32 vp;

>>> +

>>> +     vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;

>>> +     vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;

>>> +     vp &= ~CSI2DC_VPCFG_DE;

>>> +     vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);

>>> +     vp &= ~CSI2DC_VPCFG_DP2;

>>> +     vp &= ~CSI2DC_VPCFG_RMS;

>>> +     vp |= CSI2DC_VPCFG_PA;

>>> +

>>> +     csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);

>>> +     csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);

>>> +     csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);

>>> +}

>>> +

>>> +static inline struct csi2dc_device *

>>> +csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)

>>> +{

>>> +     return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);

>>> +}

>>> +

>>> +static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,

>>> +                              struct v4l2_subdev_pad_config *cfg,

>>> +                              struct v4l2_subdev_mbus_code_enum *code)

>>> +{

>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>> +

>>> +     if (code->index >= csi2dc->num_fmts)

>>> +             return -EINVAL;

>>> +

>>> +     code->code = csi2dc->formats[code->index]->mbus_code;

>>> +     return 0;

>>> +}

>>> +

>>> +static int csi2dc_try_fmt(struct v4l2_subdev *csi2dc_sd,

>>> +                       struct v4l2_subdev_pad_config *cfg,

>>> +                       struct v4l2_subdev_format *req_fmt)

>>> +{

>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>> +     struct csi2dc_format *fmt;

>>> +

>>> +     if (!csi2dc->completed) {

>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>> +             return 0;

>>> +     }

>>> +

>>> +     for (fmt = csi2dc->formats[0]; fmt; fmt++)

>>> +             if (req_fmt->format.code == fmt->mbus_code)

>>> +                     csi2dc->try_fmt = fmt;

>>> +

>>> +     /* in case we could not find the desired format, default to something */

>>> +     if (!csi2dc->try_fmt  ||

>>> +         req_fmt->format.code != csi2dc->try_fmt->mbus_code) {

>>> +             csi2dc->try_fmt = csi2dc->formats[0];

>>> +             req_fmt->format.code = csi2dc->formats[0]->mbus_code;

>>> +     }

>>> +

>>> +     return v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);

>>> +}

>>> +

>>> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,

>>> +                       struct v4l2_subdev_pad_config *cfg,

>>> +                       struct v4l2_subdev_format *req_fmt)

>>> +{

>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>> +     int ret;

>>> +

>>> +     if (!csi2dc->completed) {

>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>> +             return 0;

>>> +     }

>>> +

>>> +     csi2dc_try_fmt(csi2dc_sd, cfg, req_fmt);

>>> +

>>> +     if (csi2dc->try_fmt->mbus_code != req_fmt->format.code) {

>>> +             dev_dbg(csi2dc->dev, "CSI2DC unsupported format 0x%x\n",

>>> +                     req_fmt->format.code);

>>

>> Please adjust the format into a supported one instead.

> 

> I thought that only the 'try_fmt' calls should adapt to something that

> works. It is the same case for set_fmt ? This function cannot return

> invalid format ?

>>

>>> +             return -EINVAL;

>>> +     }

>>> +

>>> +     ret = v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);

>>> +     if (ret) {

>>> +             dev_err(csi2dc->dev, "input subdev failed %d\n", ret);

>>> +             return ret;

>>> +     }

>>> +

>>> +     csi2dc->cur_fmt = csi2dc->try_fmt;

>>> +     /* update video pipe */

>>> +     csi2dc_vp_update(csi2dc);

>>> +

>>> +     dev_dbg(csi2dc->dev, "CSI2DC new format: 0x%x\n", req_fmt->format.code);

>>> +     return 0;

>>> +}

>>> +

>>> +static int csi2dc_formats_init(struct csi2dc_device *csi2dc)

>>> +{

>>> +     int i, j;

>>> +     struct csi2dc_format *fmt = &csi2dc_formats_list[0];

>>> +

>>> +     csi2dc->num_fmts = 1;

>>> +

>>> +     csi2dc->formats = devm_kcalloc(csi2dc->dev, csi2dc->num_fmts,

>>> +                                    sizeof(*csi2dc->formats), GFP_KERNEL);

>>> +

>>> +     for (i = 0, j = 0; i < ARRAY_SIZE(csi2dc_formats_list); i++, fmt++)

>>> +             if (fmt->mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10)

>>> +                     csi2dc->formats[j++] = fmt;

>>> +     return 0;

>>> +}

>>> +

>>> +static int csi2dc_s_power(struct v4l2_subdev *csi2dc_sd, int on)

>>> +{

>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>> +     int ret = 0;

>>> +

>>> +     if (!csi2dc->completed) {

>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>> +             return 0;

>>> +     }

>>> +

>>> +     if (on)

>>> +             ret = clk_prepare_enable(csi2dc->scck);

>>> +     else

>>> +             clk_disable_unprepare(csi2dc->scck);

>>> +     if (ret)

>>> +             dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret);

>>> +

>>> +     /* if powering up, deassert reset line */

>>> +     if (on)

>>> +             csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);

>>> +

>>> +     ret = v4l2_subdev_call(csi2dc->input_sd, core, s_power, on);

>>> +

>>> +     /* if powering down, assert reset line */

>>> +     if (!on)

>>> +             csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST);

>>

>> These go to the runtime resume and suspend callbacks.

> 

> Okay I will update

> 

>>

>>> +

>>> +     return ret;

>>> +}

>>> +

>>> +static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)

>>> +{

>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>> +

>>> +     if (!csi2dc->completed) {

>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>> +             return 0;

>>> +     }

>>> +

>>> +     return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable);

>>> +}

>>> +

>>> +static int csi2dc_g_frame_interval(struct v4l2_subdev *csi2dc_sd,

>>> +                                struct v4l2_subdev_frame_interval *interval)

>>> +{

>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>> +

>>> +     if (!csi2dc->completed) {

>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>> +             return 0;

>>> +     }

>>> +

>>> +     return v4l2_subdev_call(csi2dc->input_sd, video, g_frame_interval,

>>> +                             interval);

>>> +}

>>> +

>>> +static int csi2dc_s_frame_interval(struct v4l2_subdev *csi2dc_sd,

>>> +                                struct v4l2_subdev_frame_interval *interval)

>>> +{

>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>> +

>>> +     if (!csi2dc->completed) {

>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>> +             return 0;

>>> +     }

>>> +

>>> +     return v4l2_subdev_call(csi2dc->input_sd, video, s_frame_interval,

>>> +                             interval);

>>

>> If your driver is used with a MC-enabled master driver, you can drop all

>> the v4l2_subdev_call() macros apart from the ones related to s_stream

>> operation.

> 

> And the set frame interval for example, will it be sent to the sensor

> automatically ?

> I noticed that such operations on the master driver were not propagated

> to the underlying subdevice. How does the master device 'know' how to

> call the input_sd ? this input_sd is not referenced anywhere in the

> master driver.

> 

>>

>> If not, then you can drop the HAS_DEVNODE flag from the subdev flags. But

>> I'd try to avoid this route if possible.

>>

>>> +}

>>> +

>>> +static int csi2dc_enum_frame_size(struct v4l2_subdev *csi2dc_sd,

>>> +                               struct v4l2_subdev_pad_config *cfg,

>>> +                               struct v4l2_subdev_frame_size_enum *fse)

>>> +{

>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>> +

>>> +     if (!csi2dc->completed) {

>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>> +             return 0;

>>> +     }

>>> +

>>> +     return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_size, cfg,

>>> +                             fse);

>>> +}

>>> +

>>> +static int csi2dc_enum_frame_interval(struct v4l2_subdev *csi2dc_sd,

>>> +                                   struct v4l2_subdev_pad_config *cfg,

>>> +                                   struct v4l2_subdev_frame_interval_enum *fie)

>>> +{

>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>> +

>>> +     if (!csi2dc->completed) {

>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>> +             return 0;

>>> +     }

>>> +

>>> +     return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_interval, cfg,

>>> +                             fie);

>>> +}

>>> +

>>> +static const struct v4l2_subdev_core_ops csi2dc_core_ops = {

>>> +     .s_power = csi2dc_s_power,

>>

>> Please use runtime PM instead.

>>

>>> +};

>>> +

>>> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {

>>> +     .enum_mbus_code = csi2dc_enum_mbus_code,

>>> +     .set_fmt = csi2dc_set_fmt,

>>> +     .enum_frame_size = csi2dc_enum_frame_size,

>>> +     .enum_frame_interval = csi2dc_enum_frame_interval,

>>> +};

>>> +

>>> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = {

>>> +     .s_stream = csi2dc_s_stream,

>>> +     .g_frame_interval = csi2dc_g_frame_interval,

>>> +     .s_frame_interval = csi2dc_s_frame_interval,

>>> +};

>>> +

>>> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = {

>>> +     .core = &csi2dc_core_ops,

>>> +     .pad = &csi2dc_pad_ops,

>>> +     .video = &csi2dc_video_ops,

>>> +};

>>> +

>>> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,

>>> +                           struct v4l2_subdev *subdev,

>>> +                           struct v4l2_async_subdev *asd)

>>> +{

>>> +     struct csi2dc_device *csi2dc = container_of(notifier->v4l2_dev,

>>> +                                     struct csi2dc_device, v4l2_dev);

>>> +     csi2dc->input_sd = subdev;

>>> +

>>> +     return 0;

>>> +}

>>> +

>>> +static int csi2dc_async_complete(struct v4l2_async_notifier *notifier)

>>> +{

>>> +     struct csi2dc_device *csi2dc =

>>> +             container_of(notifier->v4l2_dev, struct csi2dc_device, v4l2_dev);

>>

>> Over 80 characters per line. Please check the rest of the driver, too.

>>

>>> +     int ret;

>>> +

>>> +     ret = v4l2_device_register_subdev_nodes(&csi2dc->v4l2_dev);

>>> +     if (ret < 0) {

>>> +             v4l2_err(&csi2dc->v4l2_dev, "failed to register subdev nodes\n");

>>> +             return ret;

>>> +     }

>>> +

>>> +     csi2dc_writel(csi2dc, CSI2DC_GCFG,

>>> +                   (CSI2DC_GCFG_HLC(csi2dc->inter_line_delay) &

>>> +                   CSI2DC_GCFG_HLC_MASK) |

>>> +                   (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));

>>> +

>>> +     csi2dc_writel(csi2dc, CSI2DC_VPCOL,

>>> +                   CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK);

>>> +     csi2dc_writel(csi2dc, CSI2DC_VPROW,

>>> +                   CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK);

>>> +

>>> +     csi2dc->completed = true;

>>> +

>>> +     return ret;

>>> +}

>>> +

>>> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = {

>>> +     .bound = csi2dc_async_bound,

>>> +     .complete = csi2dc_async_complete,

>>> +};

>>> +

>>> +static void csi2dc_cleanup_notifier(struct csi2dc_device *csi2dc)

>>> +{

>>> +     v4l2_async_notifier_unregister(&csi2dc->notifier);

>>> +     v4l2_async_notifier_cleanup(&csi2dc->notifier);

>>> +}

>>> +

>>> +static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,

>>> +                                struct device_node *input_parent)

>>> +{

>>> +     int ret;

>>> +

>>> +     v4l2_async_notifier_init(&csi2dc->notifier);

>>> +

>>> +     csi2dc->asd = kzalloc(sizeof(*csi2dc->asd), GFP_KERNEL);

>>> +     if (!csi2dc->asd)

>>> +             return -ENOMEM;

>>> +

>>> +     csi2dc->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;

>>> +     csi2dc->asd->match.fwnode = of_fwnode_handle(input_parent);

>>> +

>>> +     ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd);

>>> +     if (ret) {

>>> +             dev_err(csi2dc->dev, "failed to add async notifier.\n");

>>> +             v4l2_async_notifier_cleanup(&csi2dc->notifier);

>>> +             goto csi2dc_prepare_notifier_err;

>>> +     }

>>> +

>>> +     csi2dc->notifier.ops = &csi2dc_async_ops;

>>> +

>>> +     ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev,

>>> +                                        &csi2dc->notifier);

>>> +

>>> +     if (ret) {

>>> +             dev_err(csi2dc->dev, "fail to register async notifier.\n");

>>> +             goto csi2dc_prepare_notifier_err;

>>> +     }

>>> +

>>> +csi2dc_prepare_notifier_err:

>>> +     of_node_put(input_parent);

>>> +

>>> +     return ret;

>>> +}

>>> +

>>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc,

>>> +                        struct device_node *of_node)

>>> +{

>>> +     struct device_node *input_node, *sink_node, *input_parent;

>>> +     struct v4l2_fwnode_endpoint input_endpoint = { 0 }, sink_endpoint = { 0 };

>>> +     int ret;

>>> +

>>> +     input_node = of_graph_get_next_endpoint(of_node, NULL);

>>> +

>>> +     if (!input_node) {

>>> +             dev_err(csi2dc->dev, "missing port node at %s, input node is mandatory.\n",

>>> +                     of_node->full_name);

>>> +             return -EINVAL;

>>> +     }

>>> +

>>> +     ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node),

>>> +                                      &input_endpoint);

>>> +

>>> +     if (ret) {

>>> +             dev_err(csi2dc->dev, "endpoint not defined at %s\n",

>>> +                     of_node->full_name);

>>> +             return ret;

>>> +     }

>>> +

>>> +     input_parent = of_graph_get_remote_port_parent(input_node);

>>> +     if (!input_parent) {

>>> +             dev_err(csi2dc->dev, "could not get input node's parent node.\n");

>>> +             return -EINVAL;

>>> +     }

>>> +

>>> +     sink_node = of_graph_get_next_endpoint(of_node, input_node);

>>> +

>>> +     if (sink_node)

>>> +             ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(sink_node),

>>> +                                              &sink_endpoint);

>>> +

>>> +     if (!sink_node || ret) {

>>> +             dev_info(csi2dc->dev, "missing sink node at %s, data pipe available only.\n",

>>> +                      of_node->full_name);

>>> +     } else {

>>> +             csi2dc->video_pipe = true;

>>> +             csi2dc->vc = sink_endpoint.base.id;

>>> +

>>> +             dev_dbg(csi2dc->dev, "block %s %d.%d->%d.%d video pipe VC %d\n",

>>> +                     of_node->full_name, input_endpoint.base.port,

>>> +                     input_endpoint.base.id, sink_endpoint.base.port,

>>> +                     sink_endpoint.base.id, csi2dc->vc);

>>> +     }

>>> +

>>> +     csi2dc->clk_gated = of_property_read_bool(of_node,

>>> +                                               "microchip,clk-gated");

>>> +

>>> +     dev_dbg(csi2dc->dev, "%s clock\n",

>>> +             csi2dc->clk_gated ? "Gated" : "Free running");

>>> +

>>> +     ret = of_property_read_u32(of_node, "microchip,inter-line-delay",

>>> +                                &csi2dc->inter_line_delay);

>>> +

>>> +     if (ret || csi2dc->inter_line_delay > 16 ||

>>> +         csi2dc->inter_line_delay == 0) {

>>> +             dev_dbg(csi2dc->dev, "assuming inter line delay = 16 clocks");

>>> +             csi2dc->inter_line_delay = 16;

>>> +             ret = 0;

>>> +     }

>>> +     /* hardware automatically adds 1 */

>>> +     csi2dc->inter_line_delay--;

>>> +

>>> +     /* prepare async notifier for subdevice completion */

>>> +

>>> +     of_node_put(sink_node);

>>> +     of_node_put(input_node);

>>> +

>>> +     return csi2dc_prepare_notifier(csi2dc, input_parent);

>>> +}

>>> +

>>> +static int csi2dc_probe(struct platform_device *pdev)

>>> +{

>>> +     struct device *dev = &pdev->dev;

>>> +     struct csi2dc_device *csi2dc;

>>> +     struct resource *res = NULL;

>>> +     int ret = 0;

>>> +

>>> +     csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);

>>> +     if (!csi2dc)

>>> +             return -ENOMEM;

>>> +

>>> +     csi2dc->dev = dev;

>>> +

>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

>>> +     if (!res)

>>> +             return -EINVAL;

>>> +

>>> +     csi2dc->base = devm_ioremap_resource(dev, res);

>>> +

>>> +     if (IS_ERR(csi2dc->base)) {

>>> +             dev_err(dev, "base address not set\n");

>>> +             return PTR_ERR(csi2dc->base);

>>> +     }

>>> +

>>> +     csi2dc->pclk = devm_clk_get(dev, "pclk");

>>> +     if (IS_ERR(csi2dc->pclk)) {

>>> +             ret = PTR_ERR(csi2dc->pclk);

>>> +             dev_err(dev, "failed to get pclk: %d\n", ret);

>>> +             return ret;

>>> +     }

>>> +

>>> +     ret = clk_prepare_enable(csi2dc->pclk);

>>> +     if (ret) {

>>> +             dev_err(dev, "failed to enable pclk: %d\n", ret);

>>> +             return ret;

>>> +     }

>>> +

>>> +     csi2dc->scck = devm_clk_get(dev, "scck");

>>> +     if (IS_ERR(csi2dc->scck)) {

>>> +             ret = PTR_ERR(csi2dc->scck);

>>> +             dev_err(dev, "failed to get scck: %d\n", ret);

>>> +             goto csi2dc_clk_fail;

>>> +     }

>>> +

>>> +     ret = v4l2_device_register(dev, &csi2dc->v4l2_dev);

>>> +     if (ret) {

>>> +             dev_err(dev, "unable to register v4l2 device.\n");

>>> +             goto csi2dc_clk_fail;

>>> +     }

>>> +

>>> +     v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);

>>> +

>>> +     csi2dc->csi2dc_sd.owner = THIS_MODULE;

>>> +     csi2dc->csi2dc_sd.dev = dev;

>>> +     snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),

>>> +              "CSI2DC.0");

>>> +

>>> +     csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;

>>> +     csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;

>>> +     csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;

>>> +     csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;

>>> +

>>> +     ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, CSI2DC_PADS_NUM,

>>> +                                  csi2dc->pads);

>>> +     if (ret < 0) {

>>> +             dev_err(dev, "media entity init failed\n");

>>> +             goto csi2dc_probe_entity_err;

>>> +     }

>>> +

>>> +     v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev);

>>> +

>>> +     platform_set_drvdata(pdev, &csi2dc->csi2dc_sd);

>>> +

>>> +     ret = csi2dc_of_parse(csi2dc, dev->of_node);

>>> +     if (ret)

>>> +             goto csi2dc_probe_entity_err;

>>> +

>>> +     ret = csi2dc_formats_init(csi2dc);

>>> +     if (ret)

>>> +             goto csi2dc_probe_error;

>>> +

>>> +     ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);

>>> +     if (ret)

>>> +             goto csi2dc_probe_error;

>>> +

>>> +     pr_info("Microchip CSI2DC version %x\n",

>>> +             csi2dc_readl(csi2dc, CSI2DC_VERSION));

>>

>> dev_info(), please. Or just dev_dbg().

>>

>>> +

>>> +     pm_runtime_set_active(dev);

>>> +     pm_runtime_enable(dev);

>>> +     pm_request_idle(dev);

>>> +

>>> +     return 0;

>>> +

>>> +csi2dc_probe_error:

>>> +     v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);

>>> +     csi2dc_cleanup_notifier(csi2dc);

>>> +

>>> +csi2dc_probe_entity_err:

>>> +     media_entity_cleanup(&csi2dc->csi2dc_sd.entity);

>>> +     v4l2_device_unregister(&csi2dc->v4l2_dev);

>>> +csi2dc_clk_fail:

>>> +     clk_disable_unprepare(csi2dc->pclk);

>>> +     return ret;

>>> +}

>>> +

>>> +static int csi2dc_remove(struct platform_device *pdev)

>>> +{

>>> +     struct v4l2_subdev *csi2dc_sd = platform_get_drvdata(pdev);

>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>> +

>>> +     pm_runtime_disable(&pdev->dev);

>>> +     csi2dc_cleanup_notifier(csi2dc);

>>> +     media_entity_cleanup(&csi2dc->csi2dc_sd.entity);

>>> +     v4l2_device_unregister(&csi2dc->v4l2_dev);

>>> +     clk_disable_unprepare(csi2dc->pclk);

>>> +

>>> +     return 0;

>>> +}

>>> +

>>> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)

>>> +{

>>> +     return 0;

>>> +}

>>> +

>>> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev)

>>> +{

>>> +     return 0;

>>> +}

>>> +

>>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {

>>> +     SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)

>>> +};

>>> +

>>> +static const struct of_device_id csi2dc_of_match[] = {

>>> +     { .compatible = "microchip,sama7g5-csi2dc" },

>>> +     { }

>>> +};

>>> +

>>> +MODULE_DEVICE_TABLE(of, csi2dc_of_match);

>>> +

>>> +static struct platform_driver csi2dc_driver = {

>>> +     .probe  = csi2dc_probe,

>>> +     .remove = csi2dc_remove,

>>> +     .driver = {

>>> +             .name           = "microchip-csi2dc",

>>> +             .pm             = &csi2dc_dev_pm_ops,

>>> +             .of_match_table = of_match_ptr(csi2dc_of_match),

>>> +     },

>>> +};

>>> +

>>> +module_platform_driver(csi2dc_driver);

>>> +

>>> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");

>>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver");

>>> +MODULE_LICENSE("GPL v2");

>>> +MODULE_SUPPORTED_DEVICE("video");

>>

>> Hmm. This line needs to be more specific. I'd just drop it.

>>

>> --

>> Kind regards,

>>

>> Sakari Ailus

>>

>

Hi Eugen,

My apologies for the late reply.

On Mon, Sep 07, 2020 at 09:16:57AM +0000, Eugen.Hristev@microchip.com 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@microchip.com>

> >> ---

> >> 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@microchip.com>

> >> + *

> >> + */

> >> +

> >> +#include <linux/clk.h>

> >> +#include <linux/module.h>

> >> +#include <linux/of.h>

> >> +#include <linux/of_graph.h>

> >> +#include <linux/platform_device.h>

> >> +#include <linux/pm_runtime.h>

> >> +#include <linux/videodev2.h>

> >> +

> >> +#include <media/v4l2-device.h>

> >> +#include <media/v4l2-fwnode.h>

> >> +#include <media/v4l2-subdev.h>

> >> +#include <media/videobuf2-dma-contig.h>

> >> +

> >> +/* Global configuration register */

> >> +#define CSI2DC_GCFG                  0x0

> >> +

> >> +/* MIPI sensor pixel clock is free running */

> >> +#define CSI2DC_GCFG_MIPIFRN          BIT(0)

> >> +/* Output waveform inter-line minimum delay */

> >> +#define CSI2DC_GCFG_HLC(v)           ((v) << 4)

> >> +#define CSI2DC_GCFG_HLC_MASK         GENMASK(7, 4)

> >> +

> >> +/* Global control register */

> >> +#define CSI2DC_GCTLR                 0x04

> >> +#define CSI2DC_GCTLR_SWRST           BIT(0)

> >> +

> >> +/* Global status register */

> >> +#define CSI2DC_GS                    0x08

> >> +

> >> +/* SSP interrupt status register */

> >> +#define CSI2DC_SSPIS                 0x28

> >> +/* Pipe update register */

> >> +#define CSI2DC_PU                    0xC0

> >> +/* Video pipe attributes update */

> >> +#define CSI2DC_PU_VP                 BIT(0)

> >> +

> >> +/* Pipe update status register */

> >> +#define CSI2DC_PUS                   0xC4

> >> +

> >> +/* Video pipeline enable register */

> >> +#define CSI2DC_VPE                   0xF8

> >> +#define CSI2DC_VPE_ENABLE            BIT(0)

> >> +

> >> +/* Video pipeline configuration register */

> >> +#define CSI2DC_VPCFG                 0xFC

> >> +/* Data type */

> >> +#define CSI2DC_VPCFG_DT(v)           ((v) << 0)

> >> +#define CSI2DC_VPCFG_DT_MASK         GENMASK(5, 0)

> >> +/* Virtual channel identifier */

> >> +#define CSI2DC_VPCFG_VC(v)           ((v) << 6)

> >> +#define CSI2DC_VPCFG_VC_MASK         GENMASK(7, 6)

> >> +/* Decompression enable */

> >> +#define CSI2DC_VPCFG_DE                      BIT(8)

> >> +/* Decoder mode */

> >> +#define CSI2DC_VPCFG_DM(v)           ((v) << 9)

> >> +#define CSI2DC_VPCFG_DM_DECODER8TO12 0

> >> +/* Decoder predictor 2 selection */

> >> +#define CSI2DC_VPCFG_DP2             BIT(12)

> >> +/* Recommended memory storage */

> >> +#define CSI2DC_VPCFG_RMS             BIT(13)

> >> +/* Post adjustment */

> >> +#define CSI2DC_VPCFG_PA                      BIT(14)

> >> +

> >> +/* Video pipeline column register */

> >> +#define CSI2DC_VPCOL                 0x100

> >> +/* Column number */

> >> +#define CSI2DC_VPCOL_COL(v)          ((v) << 0)

> >> +#define CSI2DC_VPCOL_COL_MASK                GENMASK(15, 0)

> >> +

> >> +/* Video pipeline row register */

> >> +#define CSI2DC_VPROW                 0x104

> >> +/* Row number */

> >> +#define CSI2DC_VPROW_ROW(v)          ((v) << 0)

> >> +#define CSI2DC_VPROW_ROW_MASK                GENMASK(15, 0)

> >> +

> >> +/* Version register */

> >> +#define CSI2DC_VERSION                       0x1FC

> >> +

> >> +/* register read/write helpers */

> >> +#define csi2dc_readl(st, reg)                readl_relaxed((st)->base + (reg))

> >> +#define csi2dc_writel(st, reg, val)  writel_relaxed((val), (st)->base + (reg))

> >> +

> >> +/* supported RAW data types */

> >> +#define CSI2DC_DT_RAW6                       0x28

> >> +#define CSI2DC_DT_RAW7                       0x29

> >> +#define CSI2DC_DT_RAW8                       0x2A

> >> +#define CSI2DC_DT_RAW10                      0x2B

> >> +#define CSI2DC_DT_RAW12                      0x2C

> >> +#define CSI2DC_DT_RAW14                      0x2D

> >> +

> >> +struct csi2dc_format {

> >> +     u32                             mbus_code;

> >> +     u32                             dt;

> >> +};

> >> +

> >> +static struct csi2dc_format csi2dc_formats_list[] = {

> > 

> > const

> 

> Elements from this array are being selected in a dynamic list, and 

> pointed from a non-const array of selected working formats (at subdevice 

> completion time). If I make this const , I will get compiler warnings 

> when assigning to the non-const array.

> What solution do you suggest for this ?


You'll need to make the other instances const, too. They do not need to be
modified I presume?

> 

> 

> > 

> >> +     {

> >> +             .mbus_code =            MEDIA_BUS_FMT_SRGGB10_1X10,

> >> +             .dt =                   CSI2DC_DT_RAW10,

> >> +     },

> >> +};

> >> +

> >> +enum mipi_csi_pads {

> >> +     CSI2DC_PAD_SINK                 = 0,

> >> +     CSI2DC_PAD_SOURCE               = 1,

> >> +     CSI2DC_PADS_NUM                 = 2,

> >> +};

> >> +

> >> +struct csi2dc_device {

> >> +     void __iomem                    *base;

> >> +     struct v4l2_subdev              csi2dc_sd;

> >> +     struct device                   *dev;

> >> +     struct v4l2_device              v4l2_dev;

> > 

> > This device is a bridge; the V4L2 device should be registered by the master

> > driver, i.e. the one handling DMA and registering the video nodes. Please

> > drop this.

> > 

> >> +     struct clk                      *pclk;

> >> +     struct clk                      *scck;

> >> +

> >> +     bool                            video_pipe;

> >> +

> >> +     u32                             num_fmts;

> >> +     struct csi2dc_format            **formats;

> >> +

> >> +     struct csi2dc_format            *cur_fmt;

> >> +     struct csi2dc_format            *try_fmt;

> >> +

> >> +     struct media_pad                pads[CSI2DC_PADS_NUM];

> >> +

> >> +     bool                            clk_gated;

> >> +     u32                             inter_line_delay;

> >> +     u32                             vc;

> >> +

> >> +     struct v4l2_async_subdev        *asd;

> >> +     struct v4l2_async_notifier      notifier;

> >> +

> >> +     struct v4l2_subdev              *input_sd;

> >> +     bool                            completed;

> >> +};

> >> +

> >> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc)

> >> +{

> >> +     u32 vp;

> >> +

> >> +     vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;

> >> +     vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;

> >> +     vp &= ~CSI2DC_VPCFG_DE;

> >> +     vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);

> >> +     vp &= ~CSI2DC_VPCFG_DP2;

> >> +     vp &= ~CSI2DC_VPCFG_RMS;

> >> +     vp |= CSI2DC_VPCFG_PA;

> >> +

> >> +     csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);

> >> +     csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);

> >> +     csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);

> >> +}

> >> +

> >> +static inline struct csi2dc_device *

> >> +csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)

> >> +{

> >> +     return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);

> >> +}

> >> +

> >> +static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,

> >> +                              struct v4l2_subdev_pad_config *cfg,

> >> +                              struct v4l2_subdev_mbus_code_enum *code)

> >> +{

> >> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

> >> +

> >> +     if (code->index >= csi2dc->num_fmts)

> >> +             return -EINVAL;

> >> +

> >> +     code->code = csi2dc->formats[code->index]->mbus_code;

> >> +     return 0;

> >> +}

> >> +

> >> +static int csi2dc_try_fmt(struct v4l2_subdev *csi2dc_sd,

> >> +                       struct v4l2_subdev_pad_config *cfg,

> >> +                       struct v4l2_subdev_format *req_fmt)

> >> +{

> >> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

> >> +     struct csi2dc_format *fmt;

> >> +

> >> +     if (!csi2dc->completed) {

> >> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

> >> +             return 0;

> >> +     }

> >> +

> >> +     for (fmt = csi2dc->formats[0]; fmt; fmt++)

> >> +             if (req_fmt->format.code == fmt->mbus_code)

> >> +                     csi2dc->try_fmt = fmt;

> >> +

> >> +     /* in case we could not find the desired format, default to something */

> >> +     if (!csi2dc->try_fmt  ||

> >> +         req_fmt->format.code != csi2dc->try_fmt->mbus_code) {

> >> +             csi2dc->try_fmt = csi2dc->formats[0];

> >> +             req_fmt->format.code = csi2dc->formats[0]->mbus_code;

> >> +     }

> >> +

> >> +     return v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);

> >> +}

> >> +

> >> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,

> >> +                       struct v4l2_subdev_pad_config *cfg,

> >> +                       struct v4l2_subdev_format *req_fmt)

> >> +{

> >> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

> >> +     int ret;

> >> +

> >> +     if (!csi2dc->completed) {

> >> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

> >> +             return 0;

> >> +     }

> >> +

> >> +     csi2dc_try_fmt(csi2dc_sd, cfg, req_fmt);

> >> +

> >> +     if (csi2dc->try_fmt->mbus_code != req_fmt->format.code) {

> >> +             dev_dbg(csi2dc->dev, "CSI2DC unsupported format 0x%x\n",

> >> +                     req_fmt->format.code);

> > 

> > Please adjust the format into a supported one instead.

> 

> I thought that only the 'try_fmt' calls should adapt to something that 

> works. It is the same case for set_fmt ? This function cannot return 

> invalid format ?


Correct. set_fmt should always return with a valid format.

Perhaps merging csi2dc_try_fmt() to this function would help? It's not used
elsewhere. It seems you're doing some redundant checks here, too.

> > 

> >> +             return -EINVAL;

> >> +     }

> >> +

> >> +     ret = v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);

> >> +     if (ret) {

> >> +             dev_err(csi2dc->dev, "input subdev failed %d\n", ret);

> >> +             return ret;

> >> +     }

> >> +

> >> +     csi2dc->cur_fmt = csi2dc->try_fmt;

> >> +     /* update video pipe */

> >> +     csi2dc_vp_update(csi2dc);

> >> +

> >> +     dev_dbg(csi2dc->dev, "CSI2DC new format: 0x%x\n", req_fmt->format.code);

> >> +     return 0;

> >> +}

> >> +

> >> +static int csi2dc_formats_init(struct csi2dc_device *csi2dc)

> >> +{

> >> +     int i, j;

> >> +     struct csi2dc_format *fmt = &csi2dc_formats_list[0];

> >> +

> >> +     csi2dc->num_fmts = 1;

> >> +

> >> +     csi2dc->formats = devm_kcalloc(csi2dc->dev, csi2dc->num_fmts,

> >> +                                    sizeof(*csi2dc->formats), GFP_KERNEL);

> >> +

> >> +     for (i = 0, j = 0; i < ARRAY_SIZE(csi2dc_formats_list); i++, fmt++)

> >> +             if (fmt->mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10)

> >> +                     csi2dc->formats[j++] = fmt;

> >> +     return 0;

> >> +}

> >> +

> >> +static int csi2dc_s_power(struct v4l2_subdev *csi2dc_sd, int on)

> >> +{

> >> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

> >> +     int ret = 0;

> >> +

> >> +     if (!csi2dc->completed) {

> >> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

> >> +             return 0;

> >> +     }

> >> +

> >> +     if (on)

> >> +             ret = clk_prepare_enable(csi2dc->scck);

> >> +     else

> >> +             clk_disable_unprepare(csi2dc->scck);

> >> +     if (ret)

> >> +             dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret);

> >> +

> >> +     /* if powering up, deassert reset line */

> >> +     if (on)

> >> +             csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);

> >> +

> >> +     ret = v4l2_subdev_call(csi2dc->input_sd, core, s_power, on);

> >> +

> >> +     /* if powering down, assert reset line */

> >> +     if (!on)

> >> +             csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST);

> > 

> > These go to the runtime resume and suspend callbacks.

> 

> Okay I will update

> 

> > 

> >> +

> >> +     return ret;

> >> +}

> >> +

> >> +static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)

> >> +{

> >> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

> >> +

> >> +     if (!csi2dc->completed) {

> >> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

> >> +             return 0;

> >> +     }

> >> +

> >> +     return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable);

> >> +}

> >> +

> >> +static int csi2dc_g_frame_interval(struct v4l2_subdev *csi2dc_sd,

> >> +                                struct v4l2_subdev_frame_interval *interval)

> >> +{

> >> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

> >> +

> >> +     if (!csi2dc->completed) {

> >> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

> >> +             return 0;

> >> +     }

> >> +

> >> +     return v4l2_subdev_call(csi2dc->input_sd, video, g_frame_interval,

> >> +                             interval);

> >> +}

> >> +

> >> +static int csi2dc_s_frame_interval(struct v4l2_subdev *csi2dc_sd,

> >> +                                struct v4l2_subdev_frame_interval *interval)

> >> +{

> >> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

> >> +

> >> +     if (!csi2dc->completed) {

> >> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

> >> +             return 0;

> >> +     }

> >> +

> >> +     return v4l2_subdev_call(csi2dc->input_sd, video, s_frame_interval,

> >> +                             interval);

> > 

> > If your driver is used with a MC-enabled master driver, you can drop all

> > the v4l2_subdev_call() macros apart from the ones related to s_stream

> > operation.

> 

> And the set frame interval for example, will it be sent to the sensor 

> automatically ?

> I noticed that such operations on the master driver were not propagated 

> to the underlying subdevice. How does the master device 'know' how to 

> call the input_sd ? this input_sd is not referenced anywhere in the 

> master driver.


That's correct. In MC-centric drivers it's the responsibility of the user
space to configure the entire pipeline.

As your device is a CSI-2 to parallel (?) converter, it may well be part of
a complex pipeline that the master driver could not know about.

I remember there was an existing stm parallel receiver that was tightly
coupled by a CSI-2 to parallel IP block where the V4L2-centric model was
used. But virtually all devices that are not just simple sensor + receiver
with DMA are MC-centric.

> 

> > 

> > If not, then you can drop the HAS_DEVNODE flag from the subdev flags. But

> > I'd try to avoid this route if possible.

> > 

> >> +}

> >> +

> >> +static int csi2dc_enum_frame_size(struct v4l2_subdev *csi2dc_sd,

> >> +                               struct v4l2_subdev_pad_config *cfg,

> >> +                               struct v4l2_subdev_frame_size_enum *fse)

> >> +{

> >> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

> >> +

> >> +     if (!csi2dc->completed) {

> >> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

> >> +             return 0;

> >> +     }

> >> +

> >> +     return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_size, cfg,

> >> +                             fse);

> >> +}

> >> +

> >> +static int csi2dc_enum_frame_interval(struct v4l2_subdev *csi2dc_sd,

> >> +                                   struct v4l2_subdev_pad_config *cfg,

> >> +                                   struct v4l2_subdev_frame_interval_enum *fie)

> >> +{

> >> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

> >> +

> >> +     if (!csi2dc->completed) {

> >> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

> >> +             return 0;

> >> +     }

> >> +

> >> +     return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_interval, cfg,

> >> +                             fie);

> >> +}

> >> +

> >> +static const struct v4l2_subdev_core_ops csi2dc_core_ops = {

> >> +     .s_power = csi2dc_s_power,

> > 

> > Please use runtime PM instead.

> > 

> >> +};

> >> +

> >> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {

> >> +     .enum_mbus_code = csi2dc_enum_mbus_code,

> >> +     .set_fmt = csi2dc_set_fmt,

> >> +     .enum_frame_size = csi2dc_enum_frame_size,

> >> +     .enum_frame_interval = csi2dc_enum_frame_interval,

> >> +};

> >> +

> >> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = {

> >> +     .s_stream = csi2dc_s_stream,

> >> +     .g_frame_interval = csi2dc_g_frame_interval,

> >> +     .s_frame_interval = csi2dc_s_frame_interval,

> >> +};

> >> +

> >> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = {

> >> +     .core = &csi2dc_core_ops,

> >> +     .pad = &csi2dc_pad_ops,

> >> +     .video = &csi2dc_video_ops,

> >> +};

> >> +

> >> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,

> >> +                           struct v4l2_subdev *subdev,

> >> +                           struct v4l2_async_subdev *asd)

> >> +{

> >> +     struct csi2dc_device *csi2dc = container_of(notifier->v4l2_dev,

> >> +                                     struct csi2dc_device, v4l2_dev);

> >> +     csi2dc->input_sd = subdev;

> >> +

> >> +     return 0;

> >> +}

> >> +

> >> +static int csi2dc_async_complete(struct v4l2_async_notifier *notifier)

> >> +{

> >> +     struct csi2dc_device *csi2dc =

> >> +             container_of(notifier->v4l2_dev, struct csi2dc_device, v4l2_dev);

> > 

> > Over 80 characters per line. Please check the rest of the driver, too.

> > 

> >> +     int ret;

> >> +

> >> +     ret = v4l2_device_register_subdev_nodes(&csi2dc->v4l2_dev);

> >> +     if (ret < 0) {

> >> +             v4l2_err(&csi2dc->v4l2_dev, "failed to register subdev nodes\n");

> >> +             return ret;

> >> +     }

> >> +

> >> +     csi2dc_writel(csi2dc, CSI2DC_GCFG,

> >> +                   (CSI2DC_GCFG_HLC(csi2dc->inter_line_delay) &

> >> +                   CSI2DC_GCFG_HLC_MASK) |

> >> +                   (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));

> >> +

> >> +     csi2dc_writel(csi2dc, CSI2DC_VPCOL,

> >> +                   CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK);

> >> +     csi2dc_writel(csi2dc, CSI2DC_VPROW,

> >> +                   CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK);

> >> +

> >> +     csi2dc->completed = true;

> >> +

> >> +     return ret;

> >> +}

> >> +

> >> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = {

> >> +     .bound = csi2dc_async_bound,

> >> +     .complete = csi2dc_async_complete,

> >> +};

> >> +

> >> +static void csi2dc_cleanup_notifier(struct csi2dc_device *csi2dc)

> >> +{

> >> +     v4l2_async_notifier_unregister(&csi2dc->notifier);

> >> +     v4l2_async_notifier_cleanup(&csi2dc->notifier);

> >> +}

> >> +

> >> +static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,

> >> +                                struct device_node *input_parent)

> >> +{

> >> +     int ret;

> >> +

> >> +     v4l2_async_notifier_init(&csi2dc->notifier);

> >> +

> >> +     csi2dc->asd = kzalloc(sizeof(*csi2dc->asd), GFP_KERNEL);

> >> +     if (!csi2dc->asd)

> >> +             return -ENOMEM;

> >> +

> >> +     csi2dc->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;

> >> +     csi2dc->asd->match.fwnode = of_fwnode_handle(input_parent);

> >> +

> >> +     ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd);

> >> +     if (ret) {

> >> +             dev_err(csi2dc->dev, "failed to add async notifier.\n");

> >> +             v4l2_async_notifier_cleanup(&csi2dc->notifier);

> >> +             goto csi2dc_prepare_notifier_err;

> >> +     }

> >> +

> >> +     csi2dc->notifier.ops = &csi2dc_async_ops;

> >> +

> >> +     ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev,

> >> +                                        &csi2dc->notifier);

> >> +

> >> +     if (ret) {

> >> +             dev_err(csi2dc->dev, "fail to register async notifier.\n");

> >> +             goto csi2dc_prepare_notifier_err;

> >> +     }

> >> +

> >> +csi2dc_prepare_notifier_err:

> >> +     of_node_put(input_parent);

> >> +

> >> +     return ret;

> >> +}

> >> +

> >> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc,

> >> +                        struct device_node *of_node)

> >> +{

> >> +     struct device_node *input_node, *sink_node, *input_parent;

> >> +     struct v4l2_fwnode_endpoint input_endpoint = { 0 }, sink_endpoint = { 0 };

> >> +     int ret;

> >> +

> >> +     input_node = of_graph_get_next_endpoint(of_node, NULL);

> >> +

> >> +     if (!input_node) {

> >> +             dev_err(csi2dc->dev, "missing port node at %s, input node is mandatory.\n",

> >> +                     of_node->full_name);

> >> +             return -EINVAL;

> >> +     }

> >> +

> >> +     ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node),

> >> +                                      &input_endpoint);

> >> +

> >> +     if (ret) {

> >> +             dev_err(csi2dc->dev, "endpoint not defined at %s\n",

> >> +                     of_node->full_name);

> >> +             return ret;

> >> +     }

> >> +

> >> +     input_parent = of_graph_get_remote_port_parent(input_node);

> >> +     if (!input_parent) {

> >> +             dev_err(csi2dc->dev, "could not get input node's parent node.\n");

> >> +             return -EINVAL;

> >> +     }

> >> +

> >> +     sink_node = of_graph_get_next_endpoint(of_node, input_node);

> >> +

> >> +     if (sink_node)

> >> +             ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(sink_node),

> >> +                                              &sink_endpoint);

> >> +

> >> +     if (!sink_node || ret) {

> >> +             dev_info(csi2dc->dev, "missing sink node at %s, data pipe available only.\n",

> >> +                      of_node->full_name);

> >> +     } else {

> >> +             csi2dc->video_pipe = true;

> >> +             csi2dc->vc = sink_endpoint.base.id;

> >> +

> >> +             dev_dbg(csi2dc->dev, "block %s %d.%d->%d.%d video pipe VC %d\n",

> >> +                     of_node->full_name, input_endpoint.base.port,

> >> +                     input_endpoint.base.id, sink_endpoint.base.port,

> >> +                     sink_endpoint.base.id, csi2dc->vc);

> >> +     }

> >> +

> >> +     csi2dc->clk_gated = of_property_read_bool(of_node,

> >> +                                               "microchip,clk-gated");

> >> +

> >> +     dev_dbg(csi2dc->dev, "%s clock\n",

> >> +             csi2dc->clk_gated ? "Gated" : "Free running");

> >> +

> >> +     ret = of_property_read_u32(of_node, "microchip,inter-line-delay",

> >> +                                &csi2dc->inter_line_delay);

> >> +

> >> +     if (ret || csi2dc->inter_line_delay > 16 ||

> >> +         csi2dc->inter_line_delay == 0) {

> >> +             dev_dbg(csi2dc->dev, "assuming inter line delay = 16 clocks");

> >> +             csi2dc->inter_line_delay = 16;

> >> +             ret = 0;

> >> +     }

> >> +     /* hardware automatically adds 1 */

> >> +     csi2dc->inter_line_delay--;

> >> +

> >> +     /* prepare async notifier for subdevice completion */

> >> +

> >> +     of_node_put(sink_node);

> >> +     of_node_put(input_node);

> >> +

> >> +     return csi2dc_prepare_notifier(csi2dc, input_parent);

> >> +}

> >> +

> >> +static int csi2dc_probe(struct platform_device *pdev)

> >> +{

> >> +     struct device *dev = &pdev->dev;

> >> +     struct csi2dc_device *csi2dc;

> >> +     struct resource *res = NULL;

> >> +     int ret = 0;

> >> +

> >> +     csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);

> >> +     if (!csi2dc)

> >> +             return -ENOMEM;

> >> +

> >> +     csi2dc->dev = dev;

> >> +

> >> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

> >> +     if (!res)

> >> +             return -EINVAL;

> >> +

> >> +     csi2dc->base = devm_ioremap_resource(dev, res);

> >> +

> >> +     if (IS_ERR(csi2dc->base)) {

> >> +             dev_err(dev, "base address not set\n");

> >> +             return PTR_ERR(csi2dc->base);

> >> +     }

> >> +

> >> +     csi2dc->pclk = devm_clk_get(dev, "pclk");

> >> +     if (IS_ERR(csi2dc->pclk)) {

> >> +             ret = PTR_ERR(csi2dc->pclk);

> >> +             dev_err(dev, "failed to get pclk: %d\n", ret);

> >> +             return ret;

> >> +     }

> >> +

> >> +     ret = clk_prepare_enable(csi2dc->pclk);

> >> +     if (ret) {

> >> +             dev_err(dev, "failed to enable pclk: %d\n", ret);

> >> +             return ret;

> >> +     }

> >> +

> >> +     csi2dc->scck = devm_clk_get(dev, "scck");

> >> +     if (IS_ERR(csi2dc->scck)) {

> >> +             ret = PTR_ERR(csi2dc->scck);

> >> +             dev_err(dev, "failed to get scck: %d\n", ret);

> >> +             goto csi2dc_clk_fail;

> >> +     }

> >> +

> >> +     ret = v4l2_device_register(dev, &csi2dc->v4l2_dev);

> >> +     if (ret) {

> >> +             dev_err(dev, "unable to register v4l2 device.\n");

> >> +             goto csi2dc_clk_fail;

> >> +     }

> >> +

> >> +     v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);

> >> +

> >> +     csi2dc->csi2dc_sd.owner = THIS_MODULE;

> >> +     csi2dc->csi2dc_sd.dev = dev;

> >> +     snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),

> >> +              "CSI2DC.0");

> >> +

> >> +     csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;

> >> +     csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;

> >> +     csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;

> >> +     csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;

> >> +

> >> +     ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, CSI2DC_PADS_NUM,

> >> +                                  csi2dc->pads);

> >> +     if (ret < 0) {

> >> +             dev_err(dev, "media entity init failed\n");

> >> +             goto csi2dc_probe_entity_err;

> >> +     }

> >> +

> >> +     v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev);

> >> +

> >> +     platform_set_drvdata(pdev, &csi2dc->csi2dc_sd);

> >> +

> >> +     ret = csi2dc_of_parse(csi2dc, dev->of_node);

> >> +     if (ret)

> >> +             goto csi2dc_probe_entity_err;

> >> +

> >> +     ret = csi2dc_formats_init(csi2dc);

> >> +     if (ret)

> >> +             goto csi2dc_probe_error;

> >> +

> >> +     ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);

> >> +     if (ret)

> >> +             goto csi2dc_probe_error;

> >> +

> >> +     pr_info("Microchip CSI2DC version %x\n",

> >> +             csi2dc_readl(csi2dc, CSI2DC_VERSION));

> > 

> > dev_info(), please. Or just dev_dbg().

> > 

> >> +

> >> +     pm_runtime_set_active(dev);

> >> +     pm_runtime_enable(dev);

> >> +     pm_request_idle(dev);

> >> +

> >> +     return 0;

> >> +

> >> +csi2dc_probe_error:

> >> +     v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);

> >> +     csi2dc_cleanup_notifier(csi2dc);

> >> +

> >> +csi2dc_probe_entity_err:

> >> +     media_entity_cleanup(&csi2dc->csi2dc_sd.entity);

> >> +     v4l2_device_unregister(&csi2dc->v4l2_dev);

> >> +csi2dc_clk_fail:

> >> +     clk_disable_unprepare(csi2dc->pclk);

> >> +     return ret;

> >> +}

> >> +

> >> +static int csi2dc_remove(struct platform_device *pdev)

> >> +{

> >> +     struct v4l2_subdev *csi2dc_sd = platform_get_drvdata(pdev);

> >> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

> >> +

> >> +     pm_runtime_disable(&pdev->dev);

> >> +     csi2dc_cleanup_notifier(csi2dc);

> >> +     media_entity_cleanup(&csi2dc->csi2dc_sd.entity);

> >> +     v4l2_device_unregister(&csi2dc->v4l2_dev);

> >> +     clk_disable_unprepare(csi2dc->pclk);

> >> +

> >> +     return 0;

> >> +}

> >> +

> >> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)

> >> +{

> >> +     return 0;

> >> +}

> >> +

> >> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev)

> >> +{

> >> +     return 0;

> >> +}

> >> +

> >> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {

> >> +     SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)

> >> +};

> >> +

> >> +static const struct of_device_id csi2dc_of_match[] = {

> >> +     { .compatible = "microchip,sama7g5-csi2dc" },

> >> +     { }

> >> +};

> >> +

> >> +MODULE_DEVICE_TABLE(of, csi2dc_of_match);

> >> +

> >> +static struct platform_driver csi2dc_driver = {

> >> +     .probe  = csi2dc_probe,

> >> +     .remove = csi2dc_remove,

> >> +     .driver = {

> >> +             .name           = "microchip-csi2dc",

> >> +             .pm             = &csi2dc_dev_pm_ops,

> >> +             .of_match_table = of_match_ptr(csi2dc_of_match),

> >> +     },

> >> +};

> >> +

> >> +module_platform_driver(csi2dc_driver);

> >> +

> >> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");

> >> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver");

> >> +MODULE_LICENSE("GPL v2");

> >> +MODULE_SUPPORTED_DEVICE("video");

> > 

> > Hmm. This line needs to be more specific. I'd just drop it.

> > 

> > --

> > Kind regards,

> > 

> > Sakari Ailus

> > 

> 


-- 
Regards,

Sakari Ailus

On 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@microchip.com 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@microchip.com>

>>>> ---

>>>> 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@microchip.com>

>>>> + *

>>>> + */

>>>> +

>>>> +#include <linux/clk.h>

>>>> +#include <linux/module.h>

>>>> +#include <linux/of.h>

>>>> +#include <linux/of_graph.h>

>>>> +#include <linux/platform_device.h>

>>>> +#include <linux/pm_runtime.h>

>>>> +#include <linux/videodev2.h>

>>>> +

>>>> +#include <media/v4l2-device.h>

>>>> +#include <media/v4l2-fwnode.h>

>>>> +#include <media/v4l2-subdev.h>

>>>> +#include <media/videobuf2-dma-contig.h>

>>>> +

>>>> +/* Global configuration register */

>>>> +#define CSI2DC_GCFG                  0x0

>>>> +

>>>> +/* MIPI sensor pixel clock is free running */

>>>> +#define CSI2DC_GCFG_MIPIFRN          BIT(0)

>>>> +/* Output waveform inter-line minimum delay */

>>>> +#define CSI2DC_GCFG_HLC(v)           ((v) << 4)

>>>> +#define CSI2DC_GCFG_HLC_MASK         GENMASK(7, 4)

>>>> +

>>>> +/* Global control register */

>>>> +#define CSI2DC_GCTLR                 0x04

>>>> +#define CSI2DC_GCTLR_SWRST           BIT(0)

>>>> +

>>>> +/* Global status register */

>>>> +#define CSI2DC_GS                    0x08

>>>> +

>>>> +/* SSP interrupt status register */

>>>> +#define CSI2DC_SSPIS                 0x28

>>>> +/* Pipe update register */

>>>> +#define CSI2DC_PU                    0xC0

>>>> +/* Video pipe attributes update */

>>>> +#define CSI2DC_PU_VP                 BIT(0)

>>>> +

>>>> +/* Pipe update status register */

>>>> +#define CSI2DC_PUS                   0xC4

>>>> +

>>>> +/* Video pipeline enable register */

>>>> +#define CSI2DC_VPE                   0xF8

>>>> +#define CSI2DC_VPE_ENABLE            BIT(0)

>>>> +

>>>> +/* Video pipeline configuration register */

>>>> +#define CSI2DC_VPCFG                 0xFC

>>>> +/* Data type */

>>>> +#define CSI2DC_VPCFG_DT(v)           ((v) << 0)

>>>> +#define CSI2DC_VPCFG_DT_MASK         GENMASK(5, 0)

>>>> +/* Virtual channel identifier */

>>>> +#define CSI2DC_VPCFG_VC(v)           ((v) << 6)

>>>> +#define CSI2DC_VPCFG_VC_MASK         GENMASK(7, 6)

>>>> +/* Decompression enable */

>>>> +#define CSI2DC_VPCFG_DE                      BIT(8)

>>>> +/* Decoder mode */

>>>> +#define CSI2DC_VPCFG_DM(v)           ((v) << 9)

>>>> +#define CSI2DC_VPCFG_DM_DECODER8TO12 0

>>>> +/* Decoder predictor 2 selection */

>>>> +#define CSI2DC_VPCFG_DP2             BIT(12)

>>>> +/* Recommended memory storage */

>>>> +#define CSI2DC_VPCFG_RMS             BIT(13)

>>>> +/* Post adjustment */

>>>> +#define CSI2DC_VPCFG_PA                      BIT(14)

>>>> +

>>>> +/* Video pipeline column register */

>>>> +#define CSI2DC_VPCOL                 0x100

>>>> +/* Column number */

>>>> +#define CSI2DC_VPCOL_COL(v)          ((v) << 0)

>>>> +#define CSI2DC_VPCOL_COL_MASK                GENMASK(15, 0)

>>>> +

>>>> +/* Video pipeline row register */

>>>> +#define CSI2DC_VPROW                 0x104

>>>> +/* Row number */

>>>> +#define CSI2DC_VPROW_ROW(v)          ((v) << 0)

>>>> +#define CSI2DC_VPROW_ROW_MASK                GENMASK(15, 0)

>>>> +

>>>> +/* Version register */

>>>> +#define CSI2DC_VERSION                       0x1FC

>>>> +

>>>> +/* register read/write helpers */

>>>> +#define csi2dc_readl(st, reg)                readl_relaxed((st)->base + (reg))

>>>> +#define csi2dc_writel(st, reg, val)  writel_relaxed((val), (st)->base + (reg))

>>>> +

>>>> +/* supported RAW data types */

>>>> +#define CSI2DC_DT_RAW6                       0x28

>>>> +#define CSI2DC_DT_RAW7                       0x29

>>>> +#define CSI2DC_DT_RAW8                       0x2A

>>>> +#define CSI2DC_DT_RAW10                      0x2B

>>>> +#define CSI2DC_DT_RAW12                      0x2C

>>>> +#define CSI2DC_DT_RAW14                      0x2D

>>>> +

>>>> +struct csi2dc_format {

>>>> +     u32                             mbus_code;

>>>> +     u32                             dt;

>>>> +};

>>>> +

>>>> +static struct csi2dc_format csi2dc_formats_list[] = {

>>>

>>> const

>>

>> Elements from this array are being selected in a dynamic list, and

>> pointed from a non-const array of selected working formats (at subdevice

>> completion time). If I make this const , I will get compiler warnings

>> when assigning to the non-const array.

>> What solution do you suggest for this ?

> 

> You'll need to make the other instances const, too. They do not need to be

> modified I presume?


That's the problem I am trying to explain. I have a dynamic (initially 
empty) array that is filled with elements from this array  which yes, is 
constant, but if I qualify it as const, all const elements 'cannot be 
assigned to a non-const array' and if I make the dynamic array as const, 
then well, I cannot modify it.

> 

>>

>>

>>>

>>>> +     {

>>>> +             .mbus_code =            MEDIA_BUS_FMT_SRGGB10_1X10,

>>>> +             .dt =                   CSI2DC_DT_RAW10,

>>>> +     },

>>>> +};

>>>> +

>>>> +enum mipi_csi_pads {

>>>> +     CSI2DC_PAD_SINK                 = 0,

>>>> +     CSI2DC_PAD_SOURCE               = 1,

>>>> +     CSI2DC_PADS_NUM                 = 2,

>>>> +};

>>>> +

>>>> +struct csi2dc_device {

>>>> +     void __iomem                    *base;

>>>> +     struct v4l2_subdev              csi2dc_sd;

>>>> +     struct device                   *dev;

>>>> +     struct v4l2_device              v4l2_dev;

>>>

>>> This device is a bridge; the V4L2 device should be registered by the master

>>> driver, i.e. the one handling DMA and registering the video nodes. Please

>>> drop this.

>>>

>>>> +     struct clk                      *pclk;

>>>> +     struct clk                      *scck;

>>>> +

>>>> +     bool                            video_pipe;

>>>> +

>>>> +     u32                             num_fmts;

>>>> +     struct csi2dc_format            **formats;

>>>> +

>>>> +     struct csi2dc_format            *cur_fmt;

>>>> +     struct csi2dc_format            *try_fmt;

>>>> +

>>>> +     struct media_pad                pads[CSI2DC_PADS_NUM];

>>>> +

>>>> +     bool                            clk_gated;

>>>> +     u32                             inter_line_delay;

>>>> +     u32                             vc;

>>>> +

>>>> +     struct v4l2_async_subdev        *asd;

>>>> +     struct v4l2_async_notifier      notifier;

>>>> +

>>>> +     struct v4l2_subdev              *input_sd;

>>>> +     bool                            completed;

>>>> +};

>>>> +

>>>> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc)

>>>> +{

>>>> +     u32 vp;

>>>> +

>>>> +     vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;

>>>> +     vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;

>>>> +     vp &= ~CSI2DC_VPCFG_DE;

>>>> +     vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);

>>>> +     vp &= ~CSI2DC_VPCFG_DP2;

>>>> +     vp &= ~CSI2DC_VPCFG_RMS;

>>>> +     vp |= CSI2DC_VPCFG_PA;

>>>> +

>>>> +     csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);

>>>> +     csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);

>>>> +     csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);

>>>> +}

>>>> +

>>>> +static inline struct csi2dc_device *

>>>> +csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)

>>>> +{

>>>> +     return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);

>>>> +}

>>>> +

>>>> +static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,

>>>> +                              struct v4l2_subdev_pad_config *cfg,

>>>> +                              struct v4l2_subdev_mbus_code_enum *code)

>>>> +{

>>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>>> +

>>>> +     if (code->index >= csi2dc->num_fmts)

>>>> +             return -EINVAL;

>>>> +

>>>> +     code->code = csi2dc->formats[code->index]->mbus_code;

>>>> +     return 0;

>>>> +}

>>>> +

>>>> +static int csi2dc_try_fmt(struct v4l2_subdev *csi2dc_sd,

>>>> +                       struct v4l2_subdev_pad_config *cfg,

>>>> +                       struct v4l2_subdev_format *req_fmt)

>>>> +{

>>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>>> +     struct csi2dc_format *fmt;

>>>> +

>>>> +     if (!csi2dc->completed) {

>>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>>> +             return 0;

>>>> +     }

>>>> +

>>>> +     for (fmt = csi2dc->formats[0]; fmt; fmt++)

>>>> +             if (req_fmt->format.code == fmt->mbus_code)

>>>> +                     csi2dc->try_fmt = fmt;

>>>> +

>>>> +     /* in case we could not find the desired format, default to something */

>>>> +     if (!csi2dc->try_fmt  ||

>>>> +         req_fmt->format.code != csi2dc->try_fmt->mbus_code) {

>>>> +             csi2dc->try_fmt = csi2dc->formats[0];

>>>> +             req_fmt->format.code = csi2dc->formats[0]->mbus_code;

>>>> +     }

>>>> +

>>>> +     return v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);

>>>> +}

>>>> +

>>>> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,

>>>> +                       struct v4l2_subdev_pad_config *cfg,

>>>> +                       struct v4l2_subdev_format *req_fmt)

>>>> +{

>>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>>> +     int ret;

>>>> +

>>>> +     if (!csi2dc->completed) {

>>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>>> +             return 0;

>>>> +     }

>>>> +

>>>> +     csi2dc_try_fmt(csi2dc_sd, cfg, req_fmt);

>>>> +

>>>> +     if (csi2dc->try_fmt->mbus_code != req_fmt->format.code) {

>>>> +             dev_dbg(csi2dc->dev, "CSI2DC unsupported format 0x%x\n",

>>>> +                     req_fmt->format.code);

>>>

>>> Please adjust the format into a supported one instead.

>>

>> I thought that only the 'try_fmt' calls should adapt to something that

>> works. It is the same case for set_fmt ? This function cannot return

>> invalid format ?

> 

> Correct. set_fmt should always return with a valid format.

> 

> Perhaps merging csi2dc_try_fmt() to this function would help? It's not used

> elsewhere. It seems you're doing some redundant checks here, too.


But this would mean that this driver will not have a pure 'try_fmt' and 
what will happen on try attempts from the master driver ?

> 

>>>

>>>> +             return -EINVAL;

>>>> +     }

>>>> +

>>>> +     ret = v4l2_subdev_call(csi2dc->input_sd, pad, set_fmt, cfg, req_fmt);

>>>> +     if (ret) {

>>>> +             dev_err(csi2dc->dev, "input subdev failed %d\n", ret);

>>>> +             return ret;

>>>> +     }

>>>> +

>>>> +     csi2dc->cur_fmt = csi2dc->try_fmt;

>>>> +     /* update video pipe */

>>>> +     csi2dc_vp_update(csi2dc);

>>>> +

>>>> +     dev_dbg(csi2dc->dev, "CSI2DC new format: 0x%x\n", req_fmt->format.code);

>>>> +     return 0;

>>>> +}

>>>> +

>>>> +static int csi2dc_formats_init(struct csi2dc_device *csi2dc)

>>>> +{

>>>> +     int i, j;

>>>> +     struct csi2dc_format *fmt = &csi2dc_formats_list[0];

>>>> +

>>>> +     csi2dc->num_fmts = 1;

>>>> +

>>>> +     csi2dc->formats = devm_kcalloc(csi2dc->dev, csi2dc->num_fmts,

>>>> +                                    sizeof(*csi2dc->formats), GFP_KERNEL);

>>>> +

>>>> +     for (i = 0, j = 0; i < ARRAY_SIZE(csi2dc_formats_list); i++, fmt++)

>>>> +             if (fmt->mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10)

>>>> +                     csi2dc->formats[j++] = fmt;

>>>> +     return 0;

>>>> +}

>>>> +

>>>> +static int csi2dc_s_power(struct v4l2_subdev *csi2dc_sd, int on)

>>>> +{

>>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>>> +     int ret = 0;

>>>> +

>>>> +     if (!csi2dc->completed) {

>>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>>> +             return 0;

>>>> +     }

>>>> +

>>>> +     if (on)

>>>> +             ret = clk_prepare_enable(csi2dc->scck);

>>>> +     else

>>>> +             clk_disable_unprepare(csi2dc->scck);

>>>> +     if (ret)

>>>> +             dev_err(csi2dc->dev, "failed to enable scck: %d\n", ret);

>>>> +

>>>> +     /* if powering up, deassert reset line */

>>>> +     if (on)

>>>> +             csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);

>>>> +

>>>> +     ret = v4l2_subdev_call(csi2dc->input_sd, core, s_power, on);

>>>> +

>>>> +     /* if powering down, assert reset line */

>>>> +     if (!on)

>>>> +             csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST);

>>>

>>> These go to the runtime resume and suspend callbacks.

>>

>> Okay I will update


Another question is what happens when CONFIG_PM is not used, thus we do 
not have runtime resume and suspend callbacks ?

>>

>>>

>>>> +

>>>> +     return ret;

>>>> +}

>>>> +

>>>> +static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)

>>>> +{

>>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>>> +

>>>> +     if (!csi2dc->completed) {

>>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>>> +             return 0;

>>>> +     }

>>>> +

>>>> +     return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable);

>>>> +}

>>>> +

>>>> +static int csi2dc_g_frame_interval(struct v4l2_subdev *csi2dc_sd,

>>>> +                                struct v4l2_subdev_frame_interval *interval)

>>>> +{

>>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>>> +

>>>> +     if (!csi2dc->completed) {

>>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>>> +             return 0;

>>>> +     }

>>>> +

>>>> +     return v4l2_subdev_call(csi2dc->input_sd, video, g_frame_interval,

>>>> +                             interval);

>>>> +}

>>>> +

>>>> +static int csi2dc_s_frame_interval(struct v4l2_subdev *csi2dc_sd,

>>>> +                                struct v4l2_subdev_frame_interval *interval)

>>>> +{

>>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>>> +

>>>> +     if (!csi2dc->completed) {

>>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>>> +             return 0;

>>>> +     }

>>>> +

>>>> +     return v4l2_subdev_call(csi2dc->input_sd, video, s_frame_interval,

>>>> +                             interval);

>>>

>>> If your driver is used with a MC-enabled master driver, you can drop all

>>> the v4l2_subdev_call() macros apart from the ones related to s_stream

>>> operation.

>>

>> And the set frame interval for example, will it be sent to the sensor

>> automatically ?

>> I noticed that such operations on the master driver were not propagated

>> to the underlying subdevice. How does the master device 'know' how to

>> call the input_sd ? this input_sd is not referenced anywhere in the

>> master driver.

> 

> That's correct. In MC-centric drivers it's the responsibility of the user

> space to configure the entire pipeline.

> 

> As your device is a CSI-2 to parallel (?) converter, it may well be part of

> a complex pipeline that the master driver could not know about.

> 

> I remember there was an existing stm parallel receiver that was tightly

> coupled by a CSI-2 to parallel IP block where the V4L2-centric model was

> used. But virtually all devices that are not just simple sensor + receiver

> with DMA are MC-centric.


Without these calls to the subdevice, the subdevice (csi2host and 
sensor) are not aware of anything configured on the master driver. How 
are such settings sent to the sensor ?

for example v4l2-ctl --list-formats-ext


# v4l2-ctl -d /dev/video0 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
         Type: Video Capture

         [0]: 'AR12' (16-bit ARGB 4-4-4-4)
                 Size: Discrete 3280x2464
                 Size: Discrete 1920x1080
                 Size: Discrete 1640x1232
         [1]: 'AR15' (16-bit ARGB 1-5-5-5)
                 Size: Discrete 3280x2464
                 Size: Discrete 1920x1080
                 Size: Discrete 1640x1232
         [2]: 'RGBP' (16-bit RGB 5-6-5)
                 Size: Discrete 3280x2464
                 Size: Discrete 1920x1080
                 Size: Discrete 1640x1232

All this information is obtained because of the enum_frame_size callback 
to the subdevice

If I remove these,

# v4l2-ctl -d /dev/video0 --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
         Type: Video Capture

         [0]: 'AR12' (16-bit ARGB 4-4-4-4)
         [1]: 'AR15' (16-bit ARGB 1-5-5-5)
         [2]: 'RGBP' (16-bit RGB 5-6-5)
         [3]: 'AR24' (32-bit BGRA 8-8-8-8)
         [4]: 'XR24' (32-bit BGRX 8-8-8-8)
         [5]: 'YU12' (Planar YUV 4:2:0)
         [6]: 'UYVY' (UYVY 4:2:2)
         [7]: 'VYUY' (VYUY 4:2:2)
         [8]: 'YUYV' (YUYV 4:2:2)
         [9]: '422P' (Planar YUV 4:2:2)
         [10]: 'GREY' (8-bit Greyscale)
         [11]: 'Y10 ' (10-bit Greyscale)
         [12]: 'Y16 ' (16-bit Greyscale)
         [13]: 'RG10' (10-bit Bayer RGRG/GBGB)


All the frame information is no longer available.

So how can I obtain this information if I remove these callbacks that 
you say they should be removed ?

> 

>>

>>>

>>> If not, then you can drop the HAS_DEVNODE flag from the subdev flags. But

>>> I'd try to avoid this route if possible.

>>>

>>>> +}

>>>> +

>>>> +static int csi2dc_enum_frame_size(struct v4l2_subdev *csi2dc_sd,

>>>> +                               struct v4l2_subdev_pad_config *cfg,

>>>> +                               struct v4l2_subdev_frame_size_enum *fse)

>>>> +{

>>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>>> +

>>>> +     if (!csi2dc->completed) {

>>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>>> +             return 0;

>>>> +     }

>>>> +

>>>> +     return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_size, cfg,

>>>> +                             fse);

>>>> +}

>>>> +

>>>> +static int csi2dc_enum_frame_interval(struct v4l2_subdev *csi2dc_sd,

>>>> +                                   struct v4l2_subdev_pad_config *cfg,

>>>> +                                   struct v4l2_subdev_frame_interval_enum *fie)

>>>> +{

>>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>>> +

>>>> +     if (!csi2dc->completed) {

>>>> +             dev_dbg((csi2dc)->dev, "subdev not registered yet\n");

>>>> +             return 0;

>>>> +     }

>>>> +

>>>> +     return v4l2_subdev_call(csi2dc->input_sd, pad, enum_frame_interval, cfg,

>>>> +                             fie);

>>>> +}

>>>> +

>>>> +static const struct v4l2_subdev_core_ops csi2dc_core_ops = {

>>>> +     .s_power = csi2dc_s_power,

>>>

>>> Please use runtime PM instead.


What happens if we do not have runtime PM ? (CONFIG_PM=n)
>>>

>>>> +};

>>>> +

>>>> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {

>>>> +     .enum_mbus_code = csi2dc_enum_mbus_code,

>>>> +     .set_fmt = csi2dc_set_fmt,

>>>> +     .enum_frame_size = csi2dc_enum_frame_size,

>>>> +     .enum_frame_interval = csi2dc_enum_frame_interval,

>>>> +};

>>>> +

>>>> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = {

>>>> +     .s_stream = csi2dc_s_stream,

>>>> +     .g_frame_interval = csi2dc_g_frame_interval,

>>>> +     .s_frame_interval = csi2dc_s_frame_interval,

>>>> +};

>>>> +

>>>> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = {

>>>> +     .core = &csi2dc_core_ops,

>>>> +     .pad = &csi2dc_pad_ops,

>>>> +     .video = &csi2dc_video_ops,

>>>> +};

>>>> +

>>>> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,

>>>> +                           struct v4l2_subdev *subdev,

>>>> +                           struct v4l2_async_subdev *asd)

>>>> +{

>>>> +     struct csi2dc_device *csi2dc = container_of(notifier->v4l2_dev,

>>>> +                                     struct csi2dc_device, v4l2_dev);

>>>> +     csi2dc->input_sd = subdev;

>>>> +

>>>> +     return 0;

>>>> +}

>>>> +

>>>> +static int csi2dc_async_complete(struct v4l2_async_notifier *notifier)

>>>> +{

>>>> +     struct csi2dc_device *csi2dc =

>>>> +             container_of(notifier->v4l2_dev, struct csi2dc_device, v4l2_dev);

>>>

>>> Over 80 characters per line. Please check the rest of the driver, too.

>>>

>>>> +     int ret;

>>>> +

>>>> +     ret = v4l2_device_register_subdev_nodes(&csi2dc->v4l2_dev);

>>>> +     if (ret < 0) {

>>>> +             v4l2_err(&csi2dc->v4l2_dev, "failed to register subdev nodes\n");

>>>> +             return ret;

>>>> +     }

>>>> +

>>>> +     csi2dc_writel(csi2dc, CSI2DC_GCFG,

>>>> +                   (CSI2DC_GCFG_HLC(csi2dc->inter_line_delay) &

>>>> +                   CSI2DC_GCFG_HLC_MASK) |

>>>> +                   (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));

>>>> +

>>>> +     csi2dc_writel(csi2dc, CSI2DC_VPCOL,

>>>> +                   CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK);

>>>> +     csi2dc_writel(csi2dc, CSI2DC_VPROW,

>>>> +                   CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK);

>>>> +

>>>> +     csi2dc->completed = true;

>>>> +

>>>> +     return ret;

>>>> +}

>>>> +

>>>> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = {

>>>> +     .bound = csi2dc_async_bound,

>>>> +     .complete = csi2dc_async_complete,

>>>> +};

>>>> +

>>>> +static void csi2dc_cleanup_notifier(struct csi2dc_device *csi2dc)

>>>> +{

>>>> +     v4l2_async_notifier_unregister(&csi2dc->notifier);

>>>> +     v4l2_async_notifier_cleanup(&csi2dc->notifier);

>>>> +}

>>>> +

>>>> +static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,

>>>> +                                struct device_node *input_parent)

>>>> +{

>>>> +     int ret;

>>>> +

>>>> +     v4l2_async_notifier_init(&csi2dc->notifier);

>>>> +

>>>> +     csi2dc->asd = kzalloc(sizeof(*csi2dc->asd), GFP_KERNEL);

>>>> +     if (!csi2dc->asd)

>>>> +             return -ENOMEM;

>>>> +

>>>> +     csi2dc->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;

>>>> +     csi2dc->asd->match.fwnode = of_fwnode_handle(input_parent);

>>>> +

>>>> +     ret = v4l2_async_notifier_add_subdev(&csi2dc->notifier, csi2dc->asd);

>>>> +     if (ret) {

>>>> +             dev_err(csi2dc->dev, "failed to add async notifier.\n");

>>>> +             v4l2_async_notifier_cleanup(&csi2dc->notifier);

>>>> +             goto csi2dc_prepare_notifier_err;

>>>> +     }

>>>> +

>>>> +     csi2dc->notifier.ops = &csi2dc_async_ops;

>>>> +

>>>> +     ret = v4l2_async_notifier_register(&csi2dc->v4l2_dev,

>>>> +                                        &csi2dc->notifier);

>>>> +

>>>> +     if (ret) {

>>>> +             dev_err(csi2dc->dev, "fail to register async notifier.\n");

>>>> +             goto csi2dc_prepare_notifier_err;

>>>> +     }

>>>> +

>>>> +csi2dc_prepare_notifier_err:

>>>> +     of_node_put(input_parent);

>>>> +

>>>> +     return ret;

>>>> +}

>>>> +

>>>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc,

>>>> +                        struct device_node *of_node)

>>>> +{

>>>> +     struct device_node *input_node, *sink_node, *input_parent;

>>>> +     struct v4l2_fwnode_endpoint input_endpoint = { 0 }, sink_endpoint = { 0 };

>>>> +     int ret;

>>>> +

>>>> +     input_node = of_graph_get_next_endpoint(of_node, NULL);

>>>> +

>>>> +     if (!input_node) {

>>>> +             dev_err(csi2dc->dev, "missing port node at %s, input node is mandatory.\n",

>>>> +                     of_node->full_name);

>>>> +             return -EINVAL;

>>>> +     }

>>>> +

>>>> +     ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node),

>>>> +                                      &input_endpoint);

>>>> +

>>>> +     if (ret) {

>>>> +             dev_err(csi2dc->dev, "endpoint not defined at %s\n",

>>>> +                     of_node->full_name);

>>>> +             return ret;

>>>> +     }

>>>> +

>>>> +     input_parent = of_graph_get_remote_port_parent(input_node);

>>>> +     if (!input_parent) {

>>>> +             dev_err(csi2dc->dev, "could not get input node's parent node.\n");

>>>> +             return -EINVAL;

>>>> +     }

>>>> +

>>>> +     sink_node = of_graph_get_next_endpoint(of_node, input_node);

>>>> +

>>>> +     if (sink_node)

>>>> +             ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(sink_node),

>>>> +                                              &sink_endpoint);

>>>> +

>>>> +     if (!sink_node || ret) {

>>>> +             dev_info(csi2dc->dev, "missing sink node at %s, data pipe available only.\n",

>>>> +                      of_node->full_name);

>>>> +     } else {

>>>> +             csi2dc->video_pipe = true;

>>>> +             csi2dc->vc = sink_endpoint.base.id;

>>>> +

>>>> +             dev_dbg(csi2dc->dev, "block %s %d.%d->%d.%d video pipe VC %d\n",

>>>> +                     of_node->full_name, input_endpoint.base.port,

>>>> +                     input_endpoint.base.id, sink_endpoint.base.port,

>>>> +                     sink_endpoint.base.id, csi2dc->vc);

>>>> +     }

>>>> +

>>>> +     csi2dc->clk_gated = of_property_read_bool(of_node,

>>>> +                                               "microchip,clk-gated");

>>>> +

>>>> +     dev_dbg(csi2dc->dev, "%s clock\n",

>>>> +             csi2dc->clk_gated ? "Gated" : "Free running");

>>>> +

>>>> +     ret = of_property_read_u32(of_node, "microchip,inter-line-delay",

>>>> +                                &csi2dc->inter_line_delay);

>>>> +

>>>> +     if (ret || csi2dc->inter_line_delay > 16 ||

>>>> +         csi2dc->inter_line_delay == 0) {

>>>> +             dev_dbg(csi2dc->dev, "assuming inter line delay = 16 clocks");

>>>> +             csi2dc->inter_line_delay = 16;

>>>> +             ret = 0;

>>>> +     }

>>>> +     /* hardware automatically adds 1 */

>>>> +     csi2dc->inter_line_delay--;

>>>> +

>>>> +     /* prepare async notifier for subdevice completion */

>>>> +

>>>> +     of_node_put(sink_node);

>>>> +     of_node_put(input_node);

>>>> +

>>>> +     return csi2dc_prepare_notifier(csi2dc, input_parent);

>>>> +}

>>>> +

>>>> +static int csi2dc_probe(struct platform_device *pdev)

>>>> +{

>>>> +     struct device *dev = &pdev->dev;

>>>> +     struct csi2dc_device *csi2dc;

>>>> +     struct resource *res = NULL;

>>>> +     int ret = 0;

>>>> +

>>>> +     csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);

>>>> +     if (!csi2dc)

>>>> +             return -ENOMEM;

>>>> +

>>>> +     csi2dc->dev = dev;

>>>> +

>>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

>>>> +     if (!res)

>>>> +             return -EINVAL;

>>>> +

>>>> +     csi2dc->base = devm_ioremap_resource(dev, res);

>>>> +

>>>> +     if (IS_ERR(csi2dc->base)) {

>>>> +             dev_err(dev, "base address not set\n");

>>>> +             return PTR_ERR(csi2dc->base);

>>>> +     }

>>>> +

>>>> +     csi2dc->pclk = devm_clk_get(dev, "pclk");

>>>> +     if (IS_ERR(csi2dc->pclk)) {

>>>> +             ret = PTR_ERR(csi2dc->pclk);

>>>> +             dev_err(dev, "failed to get pclk: %d\n", ret);

>>>> +             return ret;

>>>> +     }

>>>> +

>>>> +     ret = clk_prepare_enable(csi2dc->pclk);

>>>> +     if (ret) {

>>>> +             dev_err(dev, "failed to enable pclk: %d\n", ret);

>>>> +             return ret;

>>>> +     }

>>>> +

>>>> +     csi2dc->scck = devm_clk_get(dev, "scck");

>>>> +     if (IS_ERR(csi2dc->scck)) {

>>>> +             ret = PTR_ERR(csi2dc->scck);

>>>> +             dev_err(dev, "failed to get scck: %d\n", ret);

>>>> +             goto csi2dc_clk_fail;

>>>> +     }

>>>> +

>>>> +     ret = v4l2_device_register(dev, &csi2dc->v4l2_dev);

>>>> +     if (ret) {

>>>> +             dev_err(dev, "unable to register v4l2 device.\n");

>>>> +             goto csi2dc_clk_fail;

>>>> +     }

>>>> +

>>>> +     v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);

>>>> +

>>>> +     csi2dc->csi2dc_sd.owner = THIS_MODULE;

>>>> +     csi2dc->csi2dc_sd.dev = dev;

>>>> +     snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),

>>>> +              "CSI2DC.0");

>>>> +

>>>> +     csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;

>>>> +     csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;

>>>> +     csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;

>>>> +     csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;

>>>> +

>>>> +     ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, CSI2DC_PADS_NUM,

>>>> +                                  csi2dc->pads);

>>>> +     if (ret < 0) {

>>>> +             dev_err(dev, "media entity init failed\n");

>>>> +             goto csi2dc_probe_entity_err;

>>>> +     }

>>>> +

>>>> +     v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev);

>>>> +

>>>> +     platform_set_drvdata(pdev, &csi2dc->csi2dc_sd);

>>>> +

>>>> +     ret = csi2dc_of_parse(csi2dc, dev->of_node);

>>>> +     if (ret)

>>>> +             goto csi2dc_probe_entity_err;

>>>> +

>>>> +     ret = csi2dc_formats_init(csi2dc);

>>>> +     if (ret)

>>>> +             goto csi2dc_probe_error;

>>>> +

>>>> +     ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);

>>>> +     if (ret)

>>>> +             goto csi2dc_probe_error;

>>>> +

>>>> +     pr_info("Microchip CSI2DC version %x\n",

>>>> +             csi2dc_readl(csi2dc, CSI2DC_VERSION));

>>>

>>> dev_info(), please. Or just dev_dbg().

>>>

>>>> +

>>>> +     pm_runtime_set_active(dev);

>>>> +     pm_runtime_enable(dev);

>>>> +     pm_request_idle(dev);

>>>> +

>>>> +     return 0;

>>>> +

>>>> +csi2dc_probe_error:

>>>> +     v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);

>>>> +     csi2dc_cleanup_notifier(csi2dc);

>>>> +

>>>> +csi2dc_probe_entity_err:

>>>> +     media_entity_cleanup(&csi2dc->csi2dc_sd.entity);

>>>> +     v4l2_device_unregister(&csi2dc->v4l2_dev);

>>>> +csi2dc_clk_fail:

>>>> +     clk_disable_unprepare(csi2dc->pclk);

>>>> +     return ret;

>>>> +}

>>>> +

>>>> +static int csi2dc_remove(struct platform_device *pdev)

>>>> +{

>>>> +     struct v4l2_subdev *csi2dc_sd = platform_get_drvdata(pdev);

>>>> +     struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);

>>>> +

>>>> +     pm_runtime_disable(&pdev->dev);

>>>> +     csi2dc_cleanup_notifier(csi2dc);

>>>> +     media_entity_cleanup(&csi2dc->csi2dc_sd.entity);

>>>> +     v4l2_device_unregister(&csi2dc->v4l2_dev);

>>>> +     clk_disable_unprepare(csi2dc->pclk);

>>>> +

>>>> +     return 0;

>>>> +}

>>>> +

>>>> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)

>>>> +{

>>>> +     return 0;

>>>> +}

>>>> +

>>>> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev)

>>>> +{

>>>> +     return 0;

>>>> +}

>>>> +

>>>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {

>>>> +     SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)

>>>> +};

>>>> +

>>>> +static const struct of_device_id csi2dc_of_match[] = {

>>>> +     { .compatible = "microchip,sama7g5-csi2dc" },

>>>> +     { }

>>>> +};

>>>> +

>>>> +MODULE_DEVICE_TABLE(of, csi2dc_of_match);

>>>> +

>>>> +static struct platform_driver csi2dc_driver = {

>>>> +     .probe  = csi2dc_probe,

>>>> +     .remove = csi2dc_remove,

>>>> +     .driver = {

>>>> +             .name           = "microchip-csi2dc",

>>>> +             .pm             = &csi2dc_dev_pm_ops,

>>>> +             .of_match_table = of_match_ptr(csi2dc_of_match),

>>>> +     },

>>>> +};

>>>> +

>>>> +module_platform_driver(csi2dc_driver);

>>>> +

>>>> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");

>>>> +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

>