| Message ID | 20211022075247.518880-4-eugen.hristev@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: atmel: atmel-isc: implement media controller | expand |
Hi Eugen, On Fri, Oct 22, 2021 at 10:52:29AM +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 this revision: > - addressed comments by Jacopo and Laurent as in this thread: > https://www.spinics.net/lists/linux-media/msg181044.html > > Previous change log : > Changes in v5: > - only in bindings > > Changes in v4: > - now using get_mbus_config ops to get data from the subdevice, like the > virtual channel id, and the clock type. > - now having possibility to select any of the RAW10 data modes > - at completion time, select which formats are also available in the subdevice, > and move to the dynamic list accordingly > - changed the pipeline integration, do not advertise subdev ready at probe time. > wait until completion is done, and then start a workqueue that will register > this device as a subdevice for the next element in pipeline. > - moved the s_power code into a different function called now csi2dc_power > that is called with CONFIG_PM functions. This is also called at completion, > to have the device ready in case CONFIG_PM is not selected on the platform. > - merged try_fmt into set_fmt > - driver cleanup, wrapped lines over 80 characters > > Changes in v2: > - moved driver to platform/atmel > - fixed minor things as per Sakari's review > - still some things from v2 review are not yet addressed, to be followed up > > > drivers/media/platform/atmel/Kconfig | 15 + > drivers/media/platform/atmel/Makefile | 1 + > .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++ > 3 files changed, 716 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 dda2f27da317..f83bee373d82 100644 > --- a/drivers/media/platform/atmel/Kconfig > +++ b/drivers/media/platform/atmel/Kconfig > @@ -40,3 +40,18 @@ 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. > + > + To compile this driver as a module, choose M here: the > + module will be called microchip-csi2dc. > diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile > index 46d264ab7948..39f0a7eba702 100644 > --- a/drivers/media/platform/atmel/Makefile > +++ b/drivers/media/platform/atmel/Makefile > @@ -6,3 +6,4 @@ obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o > obj-$(CONFIG_VIDEO_ATMEL_ISC_BASE) += atmel-isc-base.o > obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o > obj-$(CONFIG_VIDEO_ATMEL_XISC) += atmel-xisc.o > +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o > diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c > new file mode 100644 > index 000000000000..277b86988eee > --- /dev/null > +++ b/drivers/media/platform/atmel/microchip-csi2dc.c > @@ -0,0 +1,700 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Microchip CSI2 Demux Controller (CSI2DC) driver > + * > + * Copyright (C) 2018 Microchip Technology, Inc. > + * > + * Author: Eugen Hristev <eugen.hristev@microchip.com> > + * > + */ > + > +#include <linux/clk.h> > +#include <linux/module.h> > +#include <linux/of.h> Isn't linux/mod_devicetable.h enough ? > +#include <linux/of_graph.h> You should probably move to use the fwnode_graph framwork instead of of_graph. This driver depends on OF so it shouldn't be an issue but I defer this to maintainers > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/videodev2.h> > + > +#include <media/v4l2-device.h> Do you need this include ? > +#include <media/v4l2-fwnode.h> > +#include <media/v4l2-subdev.h> > +#include <media/videobuf2-dma-contig.h> Is this one needed as well ? > + > +/* Global configuration register */ > +#define CSI2DC_GCFG 0x0 > + > +/* MIPI sensor pixel clock is free running */ > +#define CSI2DC_GCFG_MIPIFRN BIT(0) > +/* Output waveform inter-line minimum delay */ > +#define CSI2DC_GCFG_HLC(v) ((v) << 4) > +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4) > +/* SAMA7G5 requires a HLC delay of 15 */ > +#define SAMA7G5_HLC (15) > + > +/* Global control register */ > +#define CSI2DC_GCTLR 0x04 > +#define CSI2DC_GCTLR_SWRST BIT(0) > + > +/* Global status register */ > +#define CSI2DC_GS 0x08 > + > +/* SSP interrupt status register */ > +#define CSI2DC_SSPIS 0x28 > +/* Pipe update register */ > +#define CSI2DC_PU 0xC0 What about using lowercase for hex values (I know there's not strict rule, so keep what you like the most, but most drivers use lowercase > +/* 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 - CSI2DC format type struct > + * @mbus_code: Media bus code for the format > + * @dt: Data type constant for this format > + */ > +struct csi2dc_format { > + u32 mbus_code; > + u32 dt; > +}; > + > +static const struct csi2dc_format csi2dc_formats[] = { > + { > + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, > + .dt = CSI2DC_DT_RAW10, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, > + .dt = CSI2DC_DT_RAW10, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, > + .dt = CSI2DC_DT_RAW10, > + }, { > + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, > + .dt = CSI2DC_DT_RAW10, > + }, > +}; How unfortunate we don't have this in the core... > + > +enum mipi_csi_pads { > + CSI2DC_PAD_SINK = 0, > + CSI2DC_PAD_SOURCE = 1, > + CSI2DC_PADS_NUM = 2, > +}; > + > +/* > + * struct csi2dc_device - CSI2DC device driver data/config struct > + * @base: Register map base address > + * @csi2dc_sd: v4l2 subdevice for the csi2dc device > + * This is the subdevice that the csi2dc device itself > + * registers in v4l2 subsystem > + * @dev: struct device for this csi2dc device > + * @pclk: Peripheral clock reference > + * Input clock that clocks the hardware block internal > + * logic > + * @scck: Sensor Controller clock reference > + * Input clock that is used to generate the pixel clock > + * @format: Current saved format used in g/s fmt > + * @cur_fmt: Current state format > + * @try_fmt: Try format that is being tried > + * @pads: Media entity pads for the csi2dc subdevice > + * @clk_gated: Whether the clock is gated or free running > + * @video_pipe: Whether video pipeline is configured > + * @vc: Current set virtual channel > + * @asd: Async subdevice for async bound of the underlying subdev > + * @notifier: Async notifier that is used to bound the underlying > + * subdevice to the csi2dc subdevice > + * @input_sd: Reference to the underlying subdevice bound to the > + * csi2dc subdevice > + * @remote_pad: Pad number of the underlying subdevice that is linked > + * to the csi2dc subdevice sink pad. > + */ > +struct csi2dc_device { > + void __iomem *base; > + struct v4l2_subdev csi2dc_sd; > + struct device *dev; > + struct clk *pclk; > + struct clk *scck; > + > + struct v4l2_mbus_framefmt format; > + > + const struct csi2dc_format *cur_fmt; > + const struct csi2dc_format *try_fmt; > + > + struct media_pad pads[CSI2DC_PADS_NUM]; > + > + bool clk_gated; > + bool video_pipe; > + u32 vc; > + > + struct v4l2_async_subdev *asd; > + struct v4l2_async_notifier notifier; > + > + struct v4l2_subdev *input_sd; > + > + u32 remote_pad; > +}; > + > +static void csi2dc_vp_update(struct csi2dc_device *csi2dc) Could you move this below closer to the only caller ? > +{ > + u32 vp; > + > + if (!csi2dc->cur_fmt) { You should probably initialize this to a default format > + dev_err(csi2dc->dev, "format must be configured first\n"); > + return; > + } > + > + if (!csi2dc->video_pipe) { This is only called internally by the driver at s_stream() time, can this happen ? Or rather won't you have a streamon call also when the data pipe only is available ? In that case you would error out here > + dev_err(csi2dc->dev, "video pipeline unavailable\n"); > + return; > + } > + > + 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_state *sd_state, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + if (code->index >= ARRAY_SIZE(csi2dc_formats)) > + return -EINVAL; > + > + code->code = csi2dc_formats[code->index].mbus_code; > + > + return 0; > +} > + > +static int csi2dc_get_fmt(struct v4l2_subdev *csi2dc_sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_format *format) > +{ > + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); > + > + format->format = csi2dc->format; > + > + return 0; You should support try formats by storing the format in the file handle state in s_fmt and return it in case which == TRY Grep for v4l2_subdev_get_try_format() for usage examples > +} > + > +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_format *req_fmt) > +{ > + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); > + const struct csi2dc_format *fmt; > + int i; unsigned > + > + for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { > + fmt = &csi2dc_formats[i]; > + if (req_fmt->format.code == fmt->mbus_code) > + csi2dc->try_fmt = fmt; Shouldn't you break ? > + fmt++; > + } And make this a simpler for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { if (req_fmt->format.code == csi2dc_formats[i].mbus_code) break; } if (i == ARRAY_SIZE(csi2dc_formats) i = 0; > + > + /* in case we could not find the desired format, default to something */ > + if (!csi2dc->try_fmt || > + req_fmt->format.code != csi2dc->try_fmt->mbus_code) { > + csi2dc->try_fmt = &csi2dc_formats[0]; > + > + dev_dbg(csi2dc->dev, > + "CSI2DC unsupported format 0x%x, defaulting to 0x%x\n", > + req_fmt->format.code, csi2dc_formats[0].mbus_code); > + > + req_fmt->format.code = csi2dc_formats[0].mbus_code; > + } > + > + req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB; > + req_fmt->format.field = V4L2_FIELD_NONE; > + > + /* save the format for later requests */ You should support TRY formats > + csi2dc->format = req_fmt->format; > + > + /* if we are just trying, we are done */ > + if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) > + return 0; > + > + csi2dc->cur_fmt = csi2dc->try_fmt; csi2dc->cur_fmt = &csi2dc_format[i]; So you can drop the try_fmt from the driver structure as it seems to be used as a temporary variable here only. > + > + dev_dbg(csi2dc->dev, "new format set: 0x%x\n", req_fmt->format.code); > + > + return 0; > +} > + > +static int csi2dc_power(struct csi2dc_device *csi2dc, int on) > +{ > + int ret = 0; > + > + if (on) { > + ret = clk_prepare_enable(csi2dc->pclk); > + if (ret) { > + dev_err(csi2dc->dev, "failed to enable pclk: %d\n", ret); > + return ret; > + } > + > + ret = clk_prepare_enable(csi2dc->scck); > + if (ret) > + dev_err(csi2dc->dev, > + "failed to enable scck: %d\n", ret); Shouldn't you bail out here ? > + > + /* if powering up, deassert reset line */ > + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST); > + } else { > + clk_disable_unprepare(csi2dc->scck); > + > + /* if powering down, assert reset line */ > + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST); Isn't reverse order of activation better ? csi2dc_writel(..) clk_disable_unprepare(..) clk_disable_unprepare(..) > + > + clk_disable_unprepare(csi2dc->pclk); > + } > + > + 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); > + int ret; > + > + if (enable) { > + ret = pm_runtime_resume_and_get(csi2dc->dev); > + if (ret < 0) > + return ret; > + csi2dc_vp_update(csi2dc); > + } else { > + pm_runtime_put_sync(csi2dc->dev); > + } > + > + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable); Should the remote subdev be started before and stopped after ? > +} > + > +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = { > + .enum_mbus_code = csi2dc_enum_mbus_code, > + .set_fmt = csi2dc_set_fmt, > + .get_fmt = csi2dc_get_fmt, > +}; > + > +static const struct v4l2_subdev_video_ops csi2dc_video_ops = { > + .s_stream = csi2dc_s_stream, > +}; > + > +static const struct v4l2_subdev_ops csi2dc_subdev_ops = { > + .pad = &csi2dc_pad_ops, > + .video = &csi2dc_video_ops, > +}; > + > +static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > +{ > + struct v4l2_mbus_config mbus_config = { 0 }; > + int ret; > + > + ret = v4l2_subdev_call(csi2dc->input_sd, pad, get_mbus_config, > + csi2dc->remote_pad, &mbus_config); > + if (ret == -ENOIOCTLCMD) { > + dev_dbg(csi2dc->dev, > + "no remote mbus configuration available\n"); > + goto csi2dc_get_mbus_config_defaults; > + } > + > + if (ret) { > + dev_err(csi2dc->dev, > + "failed to get remote mbus configuration\n"); > + goto csi2dc_get_mbus_config_defaults; > + } > + > + if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) > + csi2dc->vc = 0; > + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) > + csi2dc->vc = 1; > + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) > + csi2dc->vc = 2; > + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) > + csi2dc->vc = 3; > + > + dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); > + > + csi2dc->clk_gated = mbus_config.flags & > + V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK; This should come from the default clock-noncontinuous property in the endpoint. It is available in the mbus_configuration only to support subdevs that can change it at runtime, and if that's the case it's ok, but I think it should be in the endpoint. Speaking of remote subdevs, is there any driver available for IDI transmitters ? > + > + dev_dbg(csi2dc->dev, "%s clock\n", > + csi2dc->clk_gated ? "gated" : "free running"); > + > + return 0; > + > +csi2dc_get_mbus_config_defaults: > + csi2dc->vc = 0; /* Virtual ID 0 by default */ > + csi2dc->clk_gated = false; /* Free running clock by default */ > + > + return 0; > +} > + > +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *subdev, > + struct v4l2_async_subdev *asd) > +{ > + struct csi2dc_device *csi2dc = container_of(notifier, > + struct csi2dc_device, notifier); > + int pad; > + int ret; > + > + csi2dc->input_sd = subdev; > + > + pad = media_entity_get_fwnode_pad(&subdev->entity, You can use 'ret' > + asd->match.fwnode, > + MEDIA_PAD_FL_SOURCE); > + if (pad < 0) { > + dev_err(csi2dc->dev, "Failed to find pad for %s\n", > + subdev->name); > + return pad; > + } > + > + csi2dc->remote_pad = pad; > + > + csi2dc_get_mbus_config(csi2dc); Ideally, as this is not fatal, you could move this at s_stream time to fetch the most up-to-date configuration > + > + ret = media_create_pad_link(&csi2dc->input_sd->entity, > + csi2dc->remote_pad, > + &csi2dc->csi2dc_sd.entity, 0, > + MEDIA_LNK_FL_ENABLED); > + if (ret < 0) { if (ret) > + dev_err(csi2dc->dev, > + "Failed to create pad link: %s to %s\n", > + csi2dc->input_sd->entity.name, > + csi2dc->csi2dc_sd.entity.name); > + return ret; > + } > + > + dev_dbg(csi2dc->dev, "link with %s pad: %d\n", > + csi2dc->input_sd->name, csi2dc->remote_pad); > + > + ret = pm_runtime_resume_and_get(csi2dc->dev); > + if (ret < 0) > + return ret; > + > + csi2dc_writel(csi2dc, CSI2DC_GCFG, > + (SAMA7G5_HLC & CSI2DC_GCFG_HLC_MASK) | > + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN)); > + > + csi2dc_writel(csi2dc, CSI2DC_VPCOL, > + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK); > + csi2dc_writel(csi2dc, CSI2DC_VPROW, > + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK); > + > + pm_runtime_put_sync(csi2dc->dev); I would really move access to the HW to s_stream time if possible > + > + return ret; > +} > + > +static const struct v4l2_async_notifier_operations csi2dc_async_ops = { > + .bound = csi2dc_async_bound, > +}; > + > +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_node) > +{ > + int ret = 0; > + > + v4l2_async_notifier_init(&csi2dc->notifier); > + > + csi2dc->asd = v4l2_async_notifier_add_fwnode_remote_subdev do you need asd in the driver structure ? > + (&csi2dc->notifier, of_fwnode_handle(input_node), > + struct v4l2_async_subdev); > + > + of_node_put(input_node); > + > + if (IS_ERR(csi2dc->asd)) { > + ret = PTR_ERR(csi2dc->asd); > + dev_err(csi2dc->dev, > + "failed to add async notifier for node %pOF: %d\n", > + input_node, ret); > + v4l2_async_notifier_cleanup(&csi2dc->notifier); > + return ret; > + } > + > + csi2dc->notifier.ops = &csi2dc_async_ops; > + > + ret = v4l2_async_subdev_notifier_register(&csi2dc->csi2dc_sd, > + &csi2dc->notifier); > + > + if (ret) { > + dev_err(csi2dc->dev, "fail to register async notifier: %d\n", > + ret); > + v4l2_async_notifier_cleanup(&csi2dc->notifier); > + } > + > + return ret; > +} > + > +static int csi2dc_of_parse(struct csi2dc_device *csi2dc, > + struct device_node *of_node) > +{ > + struct device_node *input_node, *output_node; > + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, > + output_endpoint = { 0 }; > + int ret; > + > + output_endpoint.bus_type = V4L2_MBUS_PARALLEL; > + > + input_node = of_graph_get_next_endpoint(of_node, NULL); > + > + if (!input_node) { > + dev_err(csi2dc->dev, > + "missing port node at %pOF, input node is mandatory.\n", > + of_node); > + return -EINVAL; > + } > + > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node), > + &input_endpoint); > + > + if (ret) { of_node_put(input_node); > + dev_err(csi2dc->dev, "endpoint not defined at %pOF\n", of_node); > + return ret; > + } > + > + output_node = of_graph_get_next_endpoint(of_node, input_node); > + > + if (output_node) > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(output_node), > + &output_endpoint); of_node_put(output_node); > + > + if (!output_node || ret) { > + dev_info(csi2dc->dev, > + "missing output node at %pOF, data pipe available only.\n", > + of_node); > + } else { > + csi2dc->video_pipe = true; > + > + dev_dbg(csi2dc->dev, "block %pOF %d.%d->%d.%d video pipeline\n", > + of_node, input_endpoint.base.port, > + input_endpoint.base.id, output_endpoint.base.port, > + output_endpoint.base.id); > + } > + > + of_node_put(output_node); Drop this if you put it above > + of_node_put(input_node); Should you put input_node before passing it to the function ? > + > + /* prepare async notifier for subdevice completion */ > + return csi2dc_prepare_notifier(csi2dc, input_node); > +} > + > +static int csi2dc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct csi2dc_device *csi2dc; > + struct resource *res = NULL; > + int ret = 0; > + u32 ver; > + > + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL); > + if (!csi2dc) > + return -ENOMEM; > + > + csi2dc->dev = dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + csi2dc->base = devm_ioremap_resource(dev, res); Should devm_platform_ioremap_resource(pdev, 0) be used here ? > + 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; > + } > + > + 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); > + return ret; > + } > + > + 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"); > + > + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev); Not used it seems > + > + platform_set_drvdata(pdev, csi2dc); > + > + ret = csi2dc_of_parse(csi2dc, dev->of_node); > + if (ret) > + goto csi2dc_probe_cleanup_entity; > + > + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > + if (csi2dc->video_pipe) > + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; > + > + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, > + csi2dc->video_pipe ? CSI2DC_PADS_NUM : 1, > + csi2dc->pads); > + if (ret < 0) { > + dev_err(dev, "media entity init failed\n"); > + goto csi2dc_probe_cleanup_entity; Should you also clean up the notifier in the error path ? > + } > + > + /* turn power on to validate capabilities */ > + ret = csi2dc_power(csi2dc, true); > + if (ret < 0) > + goto csi2dc_probe_cleanup_entity; > + > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + ver = csi2dc_readl(csi2dc, CSI2DC_VERSION); > + pm_request_idle(dev); > + > + /* > + * we must register the subdev after PM runtime has been requested, > + * otherwise we might bound immediately and request pm_runtime_resume > + * before runtime_enable. > + */ > + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd); > + if (ret) { > + dev_err(csi2dc->dev, "failed to register the subdevice\n"); > + goto csi2dc_probe_cleanup_entity; > + } > + > + dev_info(dev, "Microchip CSI2DC version %x\n", ver); > + > + return 0; > + > +csi2dc_probe_cleanup_entity: > + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); > + > + return ret; > +} > + > +static int csi2dc_remove(struct platform_device *pdev) > +{ > + struct csi2dc_device *csi2dc = platform_get_drvdata(pdev); > + > + pm_runtime_disable(&pdev->dev); > + > + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd); > + csi2dc_cleanup_notifier(csi2dc); > + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); > + > + return 0; > +} > + > +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev) > +{ > + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); > + > + return csi2dc_power(csi2dc, false); > +} > + > +static int __maybe_unused csi2dc_runtime_resume(struct device *dev) > +{ > + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); > + > + return csi2dc_power(csi2dc, true); > +} > + > +static const struct dev_pm_ops csi2dc_dev_pm_ops = { > + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL) > +}; > + > +static const struct of_device_id csi2dc_of_match[] = { > + { .compatible = "microchip,sama7g5-csi2dc" }, > + { } > +}; > + > +MODULE_DEVICE_TABLE(of, csi2dc_of_match); > + > +static struct platform_driver csi2dc_driver = { > + .probe = csi2dc_probe, > + .remove = csi2dc_remove, > + .driver = { > + .name = "microchip-csi2dc", > + .pm = &csi2dc_dev_pm_ops, > + .of_match_table = of_match_ptr(csi2dc_of_match), > + }, > +}; > + > +module_platform_driver(csi2dc_driver); > + > +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>"); > +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.25.1 >
On 11/2/21 7:22 PM, Jacopo Mondi wrote: > Hi Eugen, > Hi, Thank you for your review. I will try to understand everything you said and come up with a new version. I still have some inline questions though, about some things which are still unclear. > On Fri, Oct 22, 2021 at 10:52:29AM +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 this revision: >> - addressed comments by Jacopo and Laurent as in this thread: >> https://www.spinics.net/lists/linux-media/msg181044.html >> >> Previous change log : >> Changes in v5: >> - only in bindings >> >> Changes in v4: >> - now using get_mbus_config ops to get data from the subdevice, like the >> virtual channel id, and the clock type. >> - now having possibility to select any of the RAW10 data modes >> - at completion time, select which formats are also available in the subdevice, >> and move to the dynamic list accordingly >> - changed the pipeline integration, do not advertise subdev ready at probe time. >> wait until completion is done, and then start a workqueue that will register >> this device as a subdevice for the next element in pipeline. >> - moved the s_power code into a different function called now csi2dc_power >> that is called with CONFIG_PM functions. This is also called at completion, >> to have the device ready in case CONFIG_PM is not selected on the platform. >> - merged try_fmt into set_fmt >> - driver cleanup, wrapped lines over 80 characters >> >> Changes in v2: >> - moved driver to platform/atmel >> - fixed minor things as per Sakari's review >> - still some things from v2 review are not yet addressed, to be followed up >> >> >> drivers/media/platform/atmel/Kconfig | 15 + >> drivers/media/platform/atmel/Makefile | 1 + >> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++ >> 3 files changed, 716 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 dda2f27da317..f83bee373d82 100644 >> --- a/drivers/media/platform/atmel/Kconfig >> +++ b/drivers/media/platform/atmel/Kconfig >> @@ -40,3 +40,18 @@ 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. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called microchip-csi2dc. >> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile >> index 46d264ab7948..39f0a7eba702 100644 >> --- a/drivers/media/platform/atmel/Makefile >> +++ b/drivers/media/platform/atmel/Makefile >> @@ -6,3 +6,4 @@ obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o >> obj-$(CONFIG_VIDEO_ATMEL_ISC_BASE) += atmel-isc-base.o >> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o >> obj-$(CONFIG_VIDEO_ATMEL_XISC) += atmel-xisc.o >> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o >> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c >> new file mode 100644 >> index 000000000000..277b86988eee >> --- /dev/null >> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c >> @@ -0,0 +1,700 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Microchip CSI2 Demux Controller (CSI2DC) driver >> + * >> + * Copyright (C) 2018 Microchip Technology, Inc. >> + * >> + * Author: Eugen Hristev <eugen.hristev@microchip.com> >> + * >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/module.h> >> +#include <linux/of.h> > > Isn't linux/mod_devicetable.h enough ? > >> +#include <linux/of_graph.h> > > You should probably move to use the fwnode_graph framwork instead of > of_graph. This driver depends on OF so it shouldn't be an issue but I > defer this to maintainers > >> +#include <linux/platform_device.h> >> +#include <linux/pm_runtime.h> >> +#include <linux/videodev2.h> >> + >> +#include <media/v4l2-device.h> > > Do you need this include ? > >> +#include <media/v4l2-fwnode.h> >> +#include <media/v4l2-subdev.h> >> +#include <media/videobuf2-dma-contig.h> > > Is this one needed as well ? > >> + >> +/* Global configuration register */ >> +#define CSI2DC_GCFG 0x0 >> + >> +/* MIPI sensor pixel clock is free running */ >> +#define CSI2DC_GCFG_MIPIFRN BIT(0) >> +/* Output waveform inter-line minimum delay */ >> +#define CSI2DC_GCFG_HLC(v) ((v) << 4) >> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4) >> +/* SAMA7G5 requires a HLC delay of 15 */ >> +#define SAMA7G5_HLC (15) >> + >> +/* Global control register */ >> +#define CSI2DC_GCTLR 0x04 >> +#define CSI2DC_GCTLR_SWRST BIT(0) >> + >> +/* Global status register */ >> +#define CSI2DC_GS 0x08 >> + >> +/* SSP interrupt status register */ >> +#define CSI2DC_SSPIS 0x28 >> +/* Pipe update register */ >> +#define CSI2DC_PU 0xC0 > > What about using lowercase for hex values (I know there's not strict > rule, so keep what you like the most, but most drivers use lowercase > >> +/* 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 - CSI2DC format type struct >> + * @mbus_code: Media bus code for the format >> + * @dt: Data type constant for this format >> + */ >> +struct csi2dc_format { >> + u32 mbus_code; >> + u32 dt; >> +}; >> + >> +static const struct csi2dc_format csi2dc_formats[] = { >> + { >> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, >> + .dt = CSI2DC_DT_RAW10, >> + }, { >> + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, >> + .dt = CSI2DC_DT_RAW10, >> + }, { >> + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, >> + .dt = CSI2DC_DT_RAW10, >> + }, { >> + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, >> + .dt = CSI2DC_DT_RAW10, >> + }, >> +}; > > How unfortunate we don't have this in the core... > >> + >> +enum mipi_csi_pads { >> + CSI2DC_PAD_SINK = 0, >> + CSI2DC_PAD_SOURCE = 1, >> + CSI2DC_PADS_NUM = 2, >> +}; >> + >> +/* >> + * struct csi2dc_device - CSI2DC device driver data/config struct >> + * @base: Register map base address >> + * @csi2dc_sd: v4l2 subdevice for the csi2dc device >> + * This is the subdevice that the csi2dc device itself >> + * registers in v4l2 subsystem >> + * @dev: struct device for this csi2dc device >> + * @pclk: Peripheral clock reference >> + * Input clock that clocks the hardware block internal >> + * logic >> + * @scck: Sensor Controller clock reference >> + * Input clock that is used to generate the pixel clock >> + * @format: Current saved format used in g/s fmt >> + * @cur_fmt: Current state format >> + * @try_fmt: Try format that is being tried >> + * @pads: Media entity pads for the csi2dc subdevice >> + * @clk_gated: Whether the clock is gated or free running >> + * @video_pipe: Whether video pipeline is configured >> + * @vc: Current set virtual channel >> + * @asd: Async subdevice for async bound of the underlying subdev >> + * @notifier: Async notifier that is used to bound the underlying >> + * subdevice to the csi2dc subdevice >> + * @input_sd: Reference to the underlying subdevice bound to the >> + * csi2dc subdevice >> + * @remote_pad: Pad number of the underlying subdevice that is linked >> + * to the csi2dc subdevice sink pad. >> + */ >> +struct csi2dc_device { >> + void __iomem *base; >> + struct v4l2_subdev csi2dc_sd; >> + struct device *dev; >> + struct clk *pclk; >> + struct clk *scck; >> + >> + struct v4l2_mbus_framefmt format; >> + >> + const struct csi2dc_format *cur_fmt; >> + const struct csi2dc_format *try_fmt; >> + >> + struct media_pad pads[CSI2DC_PADS_NUM]; >> + >> + bool clk_gated; >> + bool video_pipe; >> + u32 vc; >> + >> + struct v4l2_async_subdev *asd; >> + struct v4l2_async_notifier notifier; >> + >> + struct v4l2_subdev *input_sd; >> + >> + u32 remote_pad; >> +}; >> + >> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc) > > Could you move this below closer to the only caller ? > >> +{ >> + u32 vp; >> + >> + if (!csi2dc->cur_fmt) { > > You should probably initialize this to a default format > >> + dev_err(csi2dc->dev, "format must be configured first\n"); >> + return; >> + } >> + >> + if (!csi2dc->video_pipe) { > > This is only called internally by the driver at s_stream() time, can > this happen ? Or rather won't you have a streamon call also when the > data pipe only is available ? In that case you would error out here This can happen if the source node is not found in the DT. In that case, there is no video pipe available, thus this whole function should be a no-op (csi2dc_vp_update) . If no vp, there is no vp update. Normall without a vp, there should be a dp (data pipe), but I haven't tested this and it's not supported in the driver. It doesn't make any sense to configure the vp registers below if the vp is not available. However, I shouldn't return an error since the vp is not mandatory for the function of the hardware. Just that the dp will be implemented at a later time (.... or never). > >> + dev_err(csi2dc->dev, "video pipeline unavailable\n"); >> + return; >> + } >> + >> + 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_state *sd_state, >> + struct v4l2_subdev_mbus_code_enum *code) >> +{ >> + if (code->index >= ARRAY_SIZE(csi2dc_formats)) >> + return -EINVAL; >> + >> + code->code = csi2dc_formats[code->index].mbus_code; >> + >> + return 0; >> +} >> + >> +static int csi2dc_get_fmt(struct v4l2_subdev *csi2dc_sd, >> + struct v4l2_subdev_state *sd_state, >> + struct v4l2_subdev_format *format) >> +{ >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); >> + >> + format->format = csi2dc->format; >> + >> + return 0; > > You should support try formats by storing the format in the file > handle state in s_fmt and return it in case which == TRY > > Grep for v4l2_subdev_get_try_format() for usage examples I did that initially, but could not find any utility for it in my tests. So I removed it. I will try to bring it back, but I have no idea how to test that the try format works fine or not > >> +} >> + >> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd, >> + struct v4l2_subdev_state *sd_state, >> + struct v4l2_subdev_format *req_fmt) >> +{ >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); >> + const struct csi2dc_format *fmt; >> + int i; > > unsigned > >> + >> + for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { >> + fmt = &csi2dc_formats[i]; >> + if (req_fmt->format.code == fmt->mbus_code) >> + csi2dc->try_fmt = fmt; > > Shouldn't you break ? > >> + fmt++; >> + } > > And make this a simpler > > for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { > if (req_fmt->format.code == csi2dc_formats[i].mbus_code) > break; > } > > if (i == ARRAY_SIZE(csi2dc_formats) > i = 0; > >> + >> + /* in case we could not find the desired format, default to something */ >> + if (!csi2dc->try_fmt || >> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) { >> + csi2dc->try_fmt = &csi2dc_formats[0]; >> + >> + dev_dbg(csi2dc->dev, >> + "CSI2DC unsupported format 0x%x, defaulting to 0x%x\n", >> + req_fmt->format.code, csi2dc_formats[0].mbus_code); >> + >> + req_fmt->format.code = csi2dc_formats[0].mbus_code; >> + } >> + >> + req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB; >> + req_fmt->format.field = V4L2_FIELD_NONE; >> + >> + /* save the format for later requests */ > > You should support TRY formats > >> + csi2dc->format = req_fmt->format; >> + >> + /* if we are just trying, we are done */ >> + if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) >> + return 0; >> + >> + csi2dc->cur_fmt = csi2dc->try_fmt; > > csi2dc->cur_fmt = &csi2dc_format[i]; > > So you can drop the try_fmt from the driver structure as it seems to > be used as a temporary variable here only. > >> + >> + dev_dbg(csi2dc->dev, "new format set: 0x%x\n", req_fmt->format.code); >> + >> + return 0; >> +} >> + >> +static int csi2dc_power(struct csi2dc_device *csi2dc, int on) >> +{ >> + int ret = 0; >> + >> + if (on) { >> + ret = clk_prepare_enable(csi2dc->pclk); >> + if (ret) { >> + dev_err(csi2dc->dev, "failed to enable pclk: %d\n", ret); >> + return ret; >> + } >> + >> + ret = clk_prepare_enable(csi2dc->scck); >> + if (ret) >> + dev_err(csi2dc->dev, >> + "failed to enable scck: %d\n", ret); > > Shouldn't you bail out here ? Initially this clock was not mandatory, but you are right, I should bail out. > >> + >> + /* if powering up, deassert reset line */ >> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST); >> + } else { >> + clk_disable_unprepare(csi2dc->scck); >> + >> + /* if powering down, assert reset line */ >> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST); > > Isn't reverse order of activation better ? > > csi2dc_writel(..) > clk_disable_unprepare(..) > clk_disable_unprepare(..) >> + >> + clk_disable_unprepare(csi2dc->pclk); >> + } >> + >> + 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); >> + int ret; >> + >> + if (enable) { >> + ret = pm_runtime_resume_and_get(csi2dc->dev); >> + if (ret < 0) >> + return ret; >> + csi2dc_vp_update(csi2dc); >> + } else { >> + pm_runtime_put_sync(csi2dc->dev); >> + } >> + >> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable); > > Should the remote subdev be started before and stopped after ? Do you mean s_power ? > >> +} >> + >> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = { >> + .enum_mbus_code = csi2dc_enum_mbus_code, >> + .set_fmt = csi2dc_set_fmt, >> + .get_fmt = csi2dc_get_fmt, >> +}; >> + >> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = { >> + .s_stream = csi2dc_s_stream, >> +}; >> + >> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = { >> + .pad = &csi2dc_pad_ops, >> + .video = &csi2dc_video_ops, >> +}; >> + >> +static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) >> +{ >> + struct v4l2_mbus_config mbus_config = { 0 }; >> + int ret; >> + >> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, get_mbus_config, >> + csi2dc->remote_pad, &mbus_config); >> + if (ret == -ENOIOCTLCMD) { >> + dev_dbg(csi2dc->dev, >> + "no remote mbus configuration available\n"); >> + goto csi2dc_get_mbus_config_defaults; >> + } >> + >> + if (ret) { >> + dev_err(csi2dc->dev, >> + "failed to get remote mbus configuration\n"); >> + goto csi2dc_get_mbus_config_defaults; >> + } >> + >> + if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) >> + csi2dc->vc = 0; >> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) >> + csi2dc->vc = 1; >> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) >> + csi2dc->vc = 2; >> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) >> + csi2dc->vc = 3; >> + >> + dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); >> + >> + csi2dc->clk_gated = mbus_config.flags & >> + V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK; > > This should come from the default clock-noncontinuous property in the > endpoint. It is available in the mbus_configuration only to support > subdevs that can change it at runtime, and if that's the case it's ok, > but I think it should be in the endpoint. I do not completely understand your point. In a previous version which you reviewed, I had this as a DT property (so in the endpoint ?), and now I converted it to a get_mbus_config flag get operation. It's not the right way to do it ? How should it be specified in the endpoint ? > > Speaking of remote subdevs, is there any driver available for IDI > transmitters ? I am not aware of any at the moment > >> + >> + dev_dbg(csi2dc->dev, "%s clock\n", >> + csi2dc->clk_gated ? "gated" : "free running"); >> + >> + return 0; >> + >> +csi2dc_get_mbus_config_defaults: >> + csi2dc->vc = 0; /* Virtual ID 0 by default */ >> + csi2dc->clk_gated = false; /* Free running clock by default */ >> + >> + return 0; >> +} >> + >> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier, >> + struct v4l2_subdev *subdev, >> + struct v4l2_async_subdev *asd) >> +{ >> + struct csi2dc_device *csi2dc = container_of(notifier, >> + struct csi2dc_device, notifier); >> + int pad; >> + int ret; >> + >> + csi2dc->input_sd = subdev; >> + >> + pad = media_entity_get_fwnode_pad(&subdev->entity, > > You can use 'ret' I can, but it is weird to do remote_pad = ret ... May bring confusion, don't you agree ? > >> + asd->match.fwnode, >> + MEDIA_PAD_FL_SOURCE); >> + if (pad < 0) { >> + dev_err(csi2dc->dev, "Failed to find pad for %s\n", >> + subdev->name); >> + return pad; >> + } >> + >> + csi2dc->remote_pad = pad; >> + >> + csi2dc_get_mbus_config(csi2dc); > > Ideally, as this is not fatal, you could move this at s_stream time to > fetch the most up-to-date configuration > >> + >> + ret = media_create_pad_link(&csi2dc->input_sd->entity, >> + csi2dc->remote_pad, >> + &csi2dc->csi2dc_sd.entity, 0, >> + MEDIA_LNK_FL_ENABLED); >> + if (ret < 0) { > > if (ret) > >> + dev_err(csi2dc->dev, >> + "Failed to create pad link: %s to %s\n", >> + csi2dc->input_sd->entity.name, >> + csi2dc->csi2dc_sd.entity.name); >> + return ret; >> + } >> + >> + dev_dbg(csi2dc->dev, "link with %s pad: %d\n", >> + csi2dc->input_sd->name, csi2dc->remote_pad); >> + >> + ret = pm_runtime_resume_and_get(csi2dc->dev); >> + if (ret < 0) >> + return ret; >> + >> + csi2dc_writel(csi2dc, CSI2DC_GCFG, >> + (SAMA7G5_HLC & CSI2DC_GCFG_HLC_MASK) | >> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN)); >> + >> + csi2dc_writel(csi2dc, CSI2DC_VPCOL, >> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK); >> + csi2dc_writel(csi2dc, CSI2DC_VPROW, >> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK); >> + >> + pm_runtime_put_sync(csi2dc->dev); > > I would really move access to the HW to s_stream time if possible > >> + >> + return ret; >> +} >> + >> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = { >> + .bound = csi2dc_async_bound, >> +}; >> + >> +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_node) >> +{ >> + int ret = 0; >> + >> + v4l2_async_notifier_init(&csi2dc->notifier); >> + >> + csi2dc->asd = v4l2_async_notifier_add_fwnode_remote_subdev > > do you need asd in the driver structure ? > >> + (&csi2dc->notifier, of_fwnode_handle(input_node), >> + struct v4l2_async_subdev); >> + >> + of_node_put(input_node); >> + >> + if (IS_ERR(csi2dc->asd)) { >> + ret = PTR_ERR(csi2dc->asd); >> + dev_err(csi2dc->dev, >> + "failed to add async notifier for node %pOF: %d\n", >> + input_node, ret); >> + v4l2_async_notifier_cleanup(&csi2dc->notifier); >> + return ret; >> + } >> + >> + csi2dc->notifier.ops = &csi2dc_async_ops; >> + >> + ret = v4l2_async_subdev_notifier_register(&csi2dc->csi2dc_sd, >> + &csi2dc->notifier); >> + >> + if (ret) { >> + dev_err(csi2dc->dev, "fail to register async notifier: %d\n", >> + ret); >> + v4l2_async_notifier_cleanup(&csi2dc->notifier); >> + } >> + >> + return ret; >> +} >> + >> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc, >> + struct device_node *of_node) >> +{ >> + struct device_node *input_node, *output_node; >> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, >> + output_endpoint = { 0 }; >> + int ret; >> + >> + output_endpoint.bus_type = V4L2_MBUS_PARALLEL; >> + >> + input_node = of_graph_get_next_endpoint(of_node, NULL); >> + >> + if (!input_node) { >> + dev_err(csi2dc->dev, >> + "missing port node at %pOF, input node is mandatory.\n", >> + of_node); >> + return -EINVAL; >> + } >> + >> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node), >> + &input_endpoint); >> + >> + if (ret) { > of_node_put(input_node); > >> + dev_err(csi2dc->dev, "endpoint not defined at %pOF\n", of_node); >> + return ret; >> + } >> + >> + output_node = of_graph_get_next_endpoint(of_node, input_node); >> + >> + if (output_node) >> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(output_node), >> + &output_endpoint); > > of_node_put(output_node); >> + >> + if (!output_node || ret) { >> + dev_info(csi2dc->dev, >> + "missing output node at %pOF, data pipe available only.\n", >> + of_node); >> + } else { >> + csi2dc->video_pipe = true; >> + >> + dev_dbg(csi2dc->dev, "block %pOF %d.%d->%d.%d video pipeline\n", >> + of_node, input_endpoint.base.port, >> + input_endpoint.base.id, output_endpoint.base.port, >> + output_endpoint.base.id); >> + } >> + >> + of_node_put(output_node); > > Drop this if you put it above > >> + of_node_put(input_node); > > Should you put input_node before passing it to the function ? >> + >> + /* prepare async notifier for subdevice completion */ >> + return csi2dc_prepare_notifier(csi2dc, input_node); >> +} >> + >> +static int csi2dc_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct csi2dc_device *csi2dc; >> + struct resource *res = NULL; >> + int ret = 0; >> + u32 ver; >> + >> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL); >> + if (!csi2dc) >> + return -ENOMEM; >> + >> + csi2dc->dev = dev; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + >> + csi2dc->base = devm_ioremap_resource(dev, res); > > Should devm_platform_ioremap_resource(pdev, 0) be used here ? > >> + 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; >> + } >> + >> + 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); >> + return ret; >> + } >> + >> + 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"); >> + >> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; >> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev); > > Not used it seems > >> + >> + platform_set_drvdata(pdev, csi2dc); >> + >> + ret = csi2dc_of_parse(csi2dc, dev->of_node); >> + if (ret) >> + goto csi2dc_probe_cleanup_entity; >> + >> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK; >> + if (csi2dc->video_pipe) >> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; >> + >> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, >> + csi2dc->video_pipe ? CSI2DC_PADS_NUM : 1, >> + csi2dc->pads); >> + if (ret < 0) { >> + dev_err(dev, "media entity init failed\n"); >> + goto csi2dc_probe_cleanup_entity; > > Should you also clean up the notifier in the error path ? > >> + } >> + >> + /* turn power on to validate capabilities */ >> + ret = csi2dc_power(csi2dc, true); >> + if (ret < 0) >> + goto csi2dc_probe_cleanup_entity; >> + >> + pm_runtime_set_active(dev); >> + pm_runtime_enable(dev); >> + ver = csi2dc_readl(csi2dc, CSI2DC_VERSION); >> + pm_request_idle(dev); >> + >> + /* >> + * we must register the subdev after PM runtime has been requested, >> + * otherwise we might bound immediately and request pm_runtime_resume >> + * before runtime_enable. >> + */ >> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd); >> + if (ret) { >> + dev_err(csi2dc->dev, "failed to register the subdevice\n"); >> + goto csi2dc_probe_cleanup_entity; >> + } >> + >> + dev_info(dev, "Microchip CSI2DC version %x\n", ver); >> + >> + return 0; >> + >> +csi2dc_probe_cleanup_entity: >> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); >> + >> + return ret; >> +} >> + >> +static int csi2dc_remove(struct platform_device *pdev) >> +{ >> + struct csi2dc_device *csi2dc = platform_get_drvdata(pdev); >> + >> + pm_runtime_disable(&pdev->dev); >> + >> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd); >> + csi2dc_cleanup_notifier(csi2dc); >> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); >> + >> + return 0; >> +} >> + >> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev) >> +{ >> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); >> + >> + return csi2dc_power(csi2dc, false); >> +} >> + >> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev) >> +{ >> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); >> + >> + return csi2dc_power(csi2dc, true); >> +} >> + >> +static const struct dev_pm_ops csi2dc_dev_pm_ops = { >> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL) >> +}; >> + >> +static const struct of_device_id csi2dc_of_match[] = { >> + { .compatible = "microchip,sama7g5-csi2dc" }, >> + { } >> +}; >> + >> +MODULE_DEVICE_TABLE(of, csi2dc_of_match); >> + >> +static struct platform_driver csi2dc_driver = { >> + .probe = csi2dc_probe, >> + .remove = csi2dc_remove, >> + .driver = { >> + .name = "microchip-csi2dc", >> + .pm = &csi2dc_dev_pm_ops, >> + .of_match_table = of_match_ptr(csi2dc_of_match), >> + }, >> +}; >> + >> +module_platform_driver(csi2dc_driver); >> + >> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>"); >> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 2.25.1 >>
Hi Eugen On Wed, Nov 03, 2021 at 08:31:36AM +0000, Eugen.Hristev@microchip.com wrote: > On 11/2/21 7:22 PM, Jacopo Mondi wrote: > > Hi Eugen, > > > > Hi, > > Thank you for your review. I will try to understand everything you said > and come up with a new version. I still have some inline questions > though, about some things which are still unclear. > > > On Fri, Oct 22, 2021 at 10:52:29AM +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 this revision: > >> - addressed comments by Jacopo and Laurent as in this thread: > >> https://www.spinics.net/lists/linux-media/msg181044.html > >> > >> Previous change log : > >> Changes in v5: > >> - only in bindings > >> > >> Changes in v4: > >> - now using get_mbus_config ops to get data from the subdevice, like the > >> virtual channel id, and the clock type. > >> - now having possibility to select any of the RAW10 data modes > >> - at completion time, select which formats are also available in the subdevice, > >> and move to the dynamic list accordingly > >> - changed the pipeline integration, do not advertise subdev ready at probe time. > >> wait until completion is done, and then start a workqueue that will register > >> this device as a subdevice for the next element in pipeline. > >> - moved the s_power code into a different function called now csi2dc_power > >> that is called with CONFIG_PM functions. This is also called at completion, > >> to have the device ready in case CONFIG_PM is not selected on the platform. > >> - merged try_fmt into set_fmt > >> - driver cleanup, wrapped lines over 80 characters > >> > >> Changes in v2: > >> - moved driver to platform/atmel > >> - fixed minor things as per Sakari's review > >> - still some things from v2 review are not yet addressed, to be followed up > >> > >> > >> drivers/media/platform/atmel/Kconfig | 15 + > >> drivers/media/platform/atmel/Makefile | 1 + > >> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++ > >> 3 files changed, 716 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 dda2f27da317..f83bee373d82 100644 > >> --- a/drivers/media/platform/atmel/Kconfig > >> +++ b/drivers/media/platform/atmel/Kconfig > >> @@ -40,3 +40,18 @@ 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. > >> + > >> + To compile this driver as a module, choose M here: the > >> + module will be called microchip-csi2dc. > >> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile > >> index 46d264ab7948..39f0a7eba702 100644 > >> --- a/drivers/media/platform/atmel/Makefile > >> +++ b/drivers/media/platform/atmel/Makefile > >> @@ -6,3 +6,4 @@ obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o > >> obj-$(CONFIG_VIDEO_ATMEL_ISC_BASE) += atmel-isc-base.o > >> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o > >> obj-$(CONFIG_VIDEO_ATMEL_XISC) += atmel-xisc.o > >> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o > >> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c > >> new file mode 100644 > >> index 000000000000..277b86988eee > >> --- /dev/null > >> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c > >> @@ -0,0 +1,700 @@ > >> +// SPDX-License-Identifier: GPL-2.0-only > >> +/* > >> + * Microchip CSI2 Demux Controller (CSI2DC) driver > >> + * > >> + * Copyright (C) 2018 Microchip Technology, Inc. > >> + * > >> + * Author: Eugen Hristev <eugen.hristev@microchip.com> > >> + * > >> + */ > >> + > >> +#include <linux/clk.h> > >> +#include <linux/module.h> > >> +#include <linux/of.h> > > > > Isn't linux/mod_devicetable.h enough ? > > > >> +#include <linux/of_graph.h> > > > > You should probably move to use the fwnode_graph framwork instead of > > of_graph. This driver depends on OF so it shouldn't be an issue but I > > defer this to maintainers > > > >> +#include <linux/platform_device.h> > >> +#include <linux/pm_runtime.h> > >> +#include <linux/videodev2.h> > >> + > >> +#include <media/v4l2-device.h> > > > > Do you need this include ? > > > >> +#include <media/v4l2-fwnode.h> > >> +#include <media/v4l2-subdev.h> > >> +#include <media/videobuf2-dma-contig.h> > > > > Is this one needed as well ? > > > >> + > >> +/* Global configuration register */ > >> +#define CSI2DC_GCFG 0x0 > >> + > >> +/* MIPI sensor pixel clock is free running */ > >> +#define CSI2DC_GCFG_MIPIFRN BIT(0) > >> +/* Output waveform inter-line minimum delay */ > >> +#define CSI2DC_GCFG_HLC(v) ((v) << 4) > >> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4) > >> +/* SAMA7G5 requires a HLC delay of 15 */ > >> +#define SAMA7G5_HLC (15) > >> + > >> +/* Global control register */ > >> +#define CSI2DC_GCTLR 0x04 > >> +#define CSI2DC_GCTLR_SWRST BIT(0) > >> + > >> +/* Global status register */ > >> +#define CSI2DC_GS 0x08 > >> + > >> +/* SSP interrupt status register */ > >> +#define CSI2DC_SSPIS 0x28 > >> +/* Pipe update register */ > >> +#define CSI2DC_PU 0xC0 > > > > What about using lowercase for hex values (I know there's not strict > > rule, so keep what you like the most, but most drivers use lowercase > > > >> +/* 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 - CSI2DC format type struct > >> + * @mbus_code: Media bus code for the format > >> + * @dt: Data type constant for this format > >> + */ > >> +struct csi2dc_format { > >> + u32 mbus_code; > >> + u32 dt; > >> +}; > >> + > >> +static const struct csi2dc_format csi2dc_formats[] = { > >> + { > >> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, > >> + .dt = CSI2DC_DT_RAW10, > >> + }, { > >> + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, > >> + .dt = CSI2DC_DT_RAW10, > >> + }, { > >> + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, > >> + .dt = CSI2DC_DT_RAW10, > >> + }, { > >> + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, > >> + .dt = CSI2DC_DT_RAW10, > >> + }, > >> +}; > > > > How unfortunate we don't have this in the core... > > > >> + > >> +enum mipi_csi_pads { > >> + CSI2DC_PAD_SINK = 0, > >> + CSI2DC_PAD_SOURCE = 1, > >> + CSI2DC_PADS_NUM = 2, > >> +}; > >> + > >> +/* > >> + * struct csi2dc_device - CSI2DC device driver data/config struct > >> + * @base: Register map base address > >> + * @csi2dc_sd: v4l2 subdevice for the csi2dc device > >> + * This is the subdevice that the csi2dc device itself > >> + * registers in v4l2 subsystem > >> + * @dev: struct device for this csi2dc device > >> + * @pclk: Peripheral clock reference > >> + * Input clock that clocks the hardware block internal > >> + * logic > >> + * @scck: Sensor Controller clock reference > >> + * Input clock that is used to generate the pixel clock > >> + * @format: Current saved format used in g/s fmt > >> + * @cur_fmt: Current state format > >> + * @try_fmt: Try format that is being tried > >> + * @pads: Media entity pads for the csi2dc subdevice > >> + * @clk_gated: Whether the clock is gated or free running > >> + * @video_pipe: Whether video pipeline is configured > >> + * @vc: Current set virtual channel > >> + * @asd: Async subdevice for async bound of the underlying subdev > >> + * @notifier: Async notifier that is used to bound the underlying > >> + * subdevice to the csi2dc subdevice > >> + * @input_sd: Reference to the underlying subdevice bound to the > >> + * csi2dc subdevice > >> + * @remote_pad: Pad number of the underlying subdevice that is linked > >> + * to the csi2dc subdevice sink pad. > >> + */ > >> +struct csi2dc_device { > >> + void __iomem *base; > >> + struct v4l2_subdev csi2dc_sd; > >> + struct device *dev; > >> + struct clk *pclk; > >> + struct clk *scck; > >> + > >> + struct v4l2_mbus_framefmt format; > >> + > >> + const struct csi2dc_format *cur_fmt; > >> + const struct csi2dc_format *try_fmt; > >> + > >> + struct media_pad pads[CSI2DC_PADS_NUM]; > >> + > >> + bool clk_gated; > >> + bool video_pipe; > >> + u32 vc; > >> + > >> + struct v4l2_async_subdev *asd; > >> + struct v4l2_async_notifier notifier; > >> + > >> + struct v4l2_subdev *input_sd; > >> + > >> + u32 remote_pad; > >> +}; > >> + > >> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc) > > > > Could you move this below closer to the only caller ? > > > >> +{ > >> + u32 vp; > >> + > >> + if (!csi2dc->cur_fmt) { > > > > You should probably initialize this to a default format > > > >> + dev_err(csi2dc->dev, "format must be configured first\n"); > >> + return; > >> + } > >> + > >> + if (!csi2dc->video_pipe) { > > > > This is only called internally by the driver at s_stream() time, can > > this happen ? Or rather won't you have a streamon call also when the > > data pipe only is available ? In that case you would error out here > > This can happen if the source node is not found in the DT. In that case, > there is no video pipe available, thus this whole function should be a > no-op (csi2dc_vp_update) . If no vp, there is no vp update. > Normall without a vp, there should be a dp (data pipe), but I haven't > tested this and it's not supported in the driver. > It doesn't make any sense to configure the vp registers below if the vp > is not available. > However, I shouldn't return an error since the vp is not mandatory for > the function of the hardware. Just that the dp will be implemented at a > later time (.... or never). > I see, so the data pipe is still work in progress... I understand bindings should allow for that to be later accepted so you cannot mandate port@1 to be there all the times, but should the driver instead refuse to operate if no port@1 is provided ? Out of curiosity what will the interaction model with the driver be in data pipe mode ? There will be a video device but there won't be an link to it, how is stream start/stop controlled ? > > > >> + dev_err(csi2dc->dev, "video pipeline unavailable\n"); > >> + return; > >> + } > >> + > >> + 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_state *sd_state, > >> + struct v4l2_subdev_mbus_code_enum *code) > >> +{ > >> + if (code->index >= ARRAY_SIZE(csi2dc_formats)) > >> + return -EINVAL; > >> + > >> + code->code = csi2dc_formats[code->index].mbus_code; > >> + > >> + return 0; > >> +} > >> + > >> +static int csi2dc_get_fmt(struct v4l2_subdev *csi2dc_sd, > >> + struct v4l2_subdev_state *sd_state, > >> + struct v4l2_subdev_format *format) > >> +{ > >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); > >> + > >> + format->format = csi2dc->format; > >> + > >> + return 0; > > > > You should support try formats by storing the format in the file > > handle state in s_fmt and return it in case which == TRY > > > > Grep for v4l2_subdev_get_try_format() for usage examples > > I did that initially, but could not find any utility for it in my tests. I understand.. Have you run v4l2-compliance on this sudev ? Doesn't it test try formats support ? > So I removed it. > I will try to bring it back, but I have no idea how to test that the try > format works fine or not > I think also v4l2-ctl supports try formats. --try-subdev-fmt pad=<pad>,width=<w>,height=<h>,code=<code>,field=<f>,colorspace=<c>, I'm not sure if it allows you to read it back, but it's easy to hack it out just to verify it. > > > >> +} > >> + > >> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd, > >> + struct v4l2_subdev_state *sd_state, > >> + struct v4l2_subdev_format *req_fmt) > >> +{ > >> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); > >> + const struct csi2dc_format *fmt; > >> + int i; > > > > unsigned > > > >> + > >> + for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { > >> + fmt = &csi2dc_formats[i]; > >> + if (req_fmt->format.code == fmt->mbus_code) > >> + csi2dc->try_fmt = fmt; > > > > Shouldn't you break ? > > > >> + fmt++; > >> + } > > > > And make this a simpler > > > > for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { > > if (req_fmt->format.code == csi2dc_formats[i].mbus_code) > > break; > > } > > > > if (i == ARRAY_SIZE(csi2dc_formats) > > i = 0; > > > >> + > >> + /* in case we could not find the desired format, default to something */ > >> + if (!csi2dc->try_fmt || > >> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) { > >> + csi2dc->try_fmt = &csi2dc_formats[0]; > >> + > >> + dev_dbg(csi2dc->dev, > >> + "CSI2DC unsupported format 0x%x, defaulting to 0x%x\n", > >> + req_fmt->format.code, csi2dc_formats[0].mbus_code); > >> + > >> + req_fmt->format.code = csi2dc_formats[0].mbus_code; > >> + } > >> + > >> + req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB; > >> + req_fmt->format.field = V4L2_FIELD_NONE; > >> + > >> + /* save the format for later requests */ > > > > You should support TRY formats > > > >> + csi2dc->format = req_fmt->format; > >> + > >> + /* if we are just trying, we are done */ > >> + if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) > >> + return 0; > >> + > >> + csi2dc->cur_fmt = csi2dc->try_fmt; > > > > csi2dc->cur_fmt = &csi2dc_format[i]; > > > > So you can drop the try_fmt from the driver structure as it seems to > > be used as a temporary variable here only. > > > >> + > >> + dev_dbg(csi2dc->dev, "new format set: 0x%x\n", req_fmt->format.code); > >> + > >> + return 0; > >> +} > >> + > >> +static int csi2dc_power(struct csi2dc_device *csi2dc, int on) > >> +{ > >> + int ret = 0; > >> + > >> + if (on) { > >> + ret = clk_prepare_enable(csi2dc->pclk); > >> + if (ret) { > >> + dev_err(csi2dc->dev, "failed to enable pclk: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + ret = clk_prepare_enable(csi2dc->scck); > >> + if (ret) > >> + dev_err(csi2dc->dev, > >> + "failed to enable scck: %d\n", ret); > > > > Shouldn't you bail out here ? > > Initially this clock was not mandatory, but you are right, I should bail > out. > > > > >> + > >> + /* if powering up, deassert reset line */ > >> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST); > >> + } else { > >> + clk_disable_unprepare(csi2dc->scck); > >> + > >> + /* if powering down, assert reset line */ > >> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST); > > > > Isn't reverse order of activation better ? > > > > csi2dc_writel(..) > > clk_disable_unprepare(..) > > clk_disable_unprepare(..) > >> + > >> + clk_disable_unprepare(csi2dc->pclk); > >> + } > >> + > >> + 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); > >> + int ret; > >> + > >> + if (enable) { > >> + ret = pm_runtime_resume_and_get(csi2dc->dev); > >> + if (ret < 0) > >> + return ret; > >> + csi2dc_vp_update(csi2dc); > >> + } else { > >> + pm_runtime_put_sync(csi2dc->dev); > >> + } > >> + > >> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable); > > > > Should the remote subdev be started before and stopped after ? > > Do you mean s_power ? > Nope, I meant the operation order. if (enable) { v4l2_subdev_call(...); pm_runtime_resume_and_get(); csi2dc_vp_update(); } else { pm_runtime_put_sync(); v4l2_subdev_call(); } However, my comment is clearly wrong, it should have been the contrary. Sorry about this It's right to configure the interface and power if up -before- starting the remote subdev, but I would stop it before powering it down. Like in: if (enable) { pm_runtime_resume_and_get(); csi2dc_vp_update(); v4l2_subdev_call(1); } else { v4l2_subdev_call(0); pm_runtime_put_sync(); } What do you think ? Stopping the remote before powering the interface down ensures no data is being put on the bus while the interace is powered off. > > > >> +} > >> + > >> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = { > >> + .enum_mbus_code = csi2dc_enum_mbus_code, > >> + .set_fmt = csi2dc_set_fmt, > >> + .get_fmt = csi2dc_get_fmt, > >> +}; > >> + > >> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = { > >> + .s_stream = csi2dc_s_stream, > >> +}; > >> + > >> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = { > >> + .pad = &csi2dc_pad_ops, > >> + .video = &csi2dc_video_ops, > >> +}; > >> + > >> +static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > >> +{ > >> + struct v4l2_mbus_config mbus_config = { 0 }; > >> + int ret; > >> + > >> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, get_mbus_config, > >> + csi2dc->remote_pad, &mbus_config); > >> + if (ret == -ENOIOCTLCMD) { > >> + dev_dbg(csi2dc->dev, > >> + "no remote mbus configuration available\n"); > >> + goto csi2dc_get_mbus_config_defaults; > >> + } > >> + > >> + if (ret) { > >> + dev_err(csi2dc->dev, > >> + "failed to get remote mbus configuration\n"); > >> + goto csi2dc_get_mbus_config_defaults; > >> + } > >> + > >> + if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) > >> + csi2dc->vc = 0; > >> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) > >> + csi2dc->vc = 1; > >> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) > >> + csi2dc->vc = 2; > >> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) > >> + csi2dc->vc = 3; > >> + > >> + dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); > >> + > >> + csi2dc->clk_gated = mbus_config.flags & > >> + V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK; > > > > This should come from the default clock-noncontinuous property in the > > endpoint. It is available in the mbus_configuration only to support > > subdevs that can change it at runtime, and if that's the case it's ok, > > but I think it should be in the endpoint. > > I do not completely understand your point. In a previous version which > you reviewed, I had this as a DT property (so in the endpoint ?), and > now I converted it to a get_mbus_config flag get operation. I re-read the conversation and I think you're right. > It's not the right way to do it ? > How should it be specified in the endpoint ? > There's a default property 'clock-noncontinuous' which gets parsed by v4l2_fwnode_endpoint_parse() and reported to drivers through the V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK and V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flags. In your case, as you fetch it at run time it's not required if not to initialize a default according to the property presence in DT. Up to you, what you have here it's fine! > > > > Speaking of remote subdevs, is there any driver available for IDI > > transmitters ? > > I am not aware of any at the moment > > > >> + > >> + dev_dbg(csi2dc->dev, "%s clock\n", > >> + csi2dc->clk_gated ? "gated" : "free running"); > >> + > >> + return 0; > >> + > >> +csi2dc_get_mbus_config_defaults: > >> + csi2dc->vc = 0; /* Virtual ID 0 by default */ > >> + csi2dc->clk_gated = false; /* Free running clock by default */ > >> + > >> + return 0; > >> +} > >> + > >> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier, > >> + struct v4l2_subdev *subdev, > >> + struct v4l2_async_subdev *asd) > >> +{ > >> + struct csi2dc_device *csi2dc = container_of(notifier, > >> + struct csi2dc_device, notifier); > >> + int pad; > >> + int ret; > >> + > >> + csi2dc->input_sd = subdev; > >> + > >> + pad = media_entity_get_fwnode_pad(&subdev->entity, > > > > You can use 'ret' > > I can, but it is weird to do remote_pad = ret ... > > May bring confusion, don't you agree ? > Up to you, it's a small detail :) Thanks! j > > > >> + asd->match.fwnode, > >> + MEDIA_PAD_FL_SOURCE); > >> + if (pad < 0) { > >> + dev_err(csi2dc->dev, "Failed to find pad for %s\n", > >> + subdev->name); > >> + return pad; > >> + } > >> + > >> + csi2dc->remote_pad = pad; > >> + > >> + csi2dc_get_mbus_config(csi2dc); > > > > Ideally, as this is not fatal, you could move this at s_stream time to > > fetch the most up-to-date configuration > > > >> + > >> + ret = media_create_pad_link(&csi2dc->input_sd->entity, > >> + csi2dc->remote_pad, > >> + &csi2dc->csi2dc_sd.entity, 0, > >> + MEDIA_LNK_FL_ENABLED); > >> + if (ret < 0) { > > > > if (ret) > > > >> + dev_err(csi2dc->dev, > >> + "Failed to create pad link: %s to %s\n", > >> + csi2dc->input_sd->entity.name, > >> + csi2dc->csi2dc_sd.entity.name); > >> + return ret; > >> + } > >> + > >> + dev_dbg(csi2dc->dev, "link with %s pad: %d\n", > >> + csi2dc->input_sd->name, csi2dc->remote_pad); > >> + > >> + ret = pm_runtime_resume_and_get(csi2dc->dev); > >> + if (ret < 0) > >> + return ret; > >> + > >> + csi2dc_writel(csi2dc, CSI2DC_GCFG, > >> + (SAMA7G5_HLC & CSI2DC_GCFG_HLC_MASK) | > >> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN)); > >> + > >> + csi2dc_writel(csi2dc, CSI2DC_VPCOL, > >> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK); > >> + csi2dc_writel(csi2dc, CSI2DC_VPROW, > >> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK); > >> + > >> + pm_runtime_put_sync(csi2dc->dev); > > > > I would really move access to the HW to s_stream time if possible > > > >> + > >> + return ret; > >> +} > >> + > >> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = { > >> + .bound = csi2dc_async_bound, > >> +}; > >> + > >> +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_node) > >> +{ > >> + int ret = 0; > >> + > >> + v4l2_async_notifier_init(&csi2dc->notifier); > >> + > >> + csi2dc->asd = v4l2_async_notifier_add_fwnode_remote_subdev > > > > do you need asd in the driver structure ? > > > >> + (&csi2dc->notifier, of_fwnode_handle(input_node), > >> + struct v4l2_async_subdev); > >> + > >> + of_node_put(input_node); > >> + > >> + if (IS_ERR(csi2dc->asd)) { > >> + ret = PTR_ERR(csi2dc->asd); > >> + dev_err(csi2dc->dev, > >> + "failed to add async notifier for node %pOF: %d\n", > >> + input_node, ret); > >> + v4l2_async_notifier_cleanup(&csi2dc->notifier); > >> + return ret; > >> + } > >> + > >> + csi2dc->notifier.ops = &csi2dc_async_ops; > >> + > >> + ret = v4l2_async_subdev_notifier_register(&csi2dc->csi2dc_sd, > >> + &csi2dc->notifier); > >> + > >> + if (ret) { > >> + dev_err(csi2dc->dev, "fail to register async notifier: %d\n", > >> + ret); > >> + v4l2_async_notifier_cleanup(&csi2dc->notifier); > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc, > >> + struct device_node *of_node) > >> +{ > >> + struct device_node *input_node, *output_node; > >> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, > >> + output_endpoint = { 0 }; > >> + int ret; > >> + > >> + output_endpoint.bus_type = V4L2_MBUS_PARALLEL; > >> + > >> + input_node = of_graph_get_next_endpoint(of_node, NULL); > >> + > >> + if (!input_node) { > >> + dev_err(csi2dc->dev, > >> + "missing port node at %pOF, input node is mandatory.\n", > >> + of_node); > >> + return -EINVAL; > >> + } > >> + > >> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node), > >> + &input_endpoint); > >> + > >> + if (ret) { > > of_node_put(input_node); > > > >> + dev_err(csi2dc->dev, "endpoint not defined at %pOF\n", of_node); > >> + return ret; > >> + } > >> + > >> + output_node = of_graph_get_next_endpoint(of_node, input_node); > >> + > >> + if (output_node) > >> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(output_node), > >> + &output_endpoint); > > > > of_node_put(output_node); > >> + > >> + if (!output_node || ret) { > >> + dev_info(csi2dc->dev, > >> + "missing output node at %pOF, data pipe available only.\n", > >> + of_node); > >> + } else { > >> + csi2dc->video_pipe = true; > >> + > >> + dev_dbg(csi2dc->dev, "block %pOF %d.%d->%d.%d video pipeline\n", > >> + of_node, input_endpoint.base.port, > >> + input_endpoint.base.id, output_endpoint.base.port, > >> + output_endpoint.base.id); > >> + } > >> + > >> + of_node_put(output_node); > > > > Drop this if you put it above > > > >> + of_node_put(input_node); > > > > Should you put input_node before passing it to the function ? > >> + > >> + /* prepare async notifier for subdevice completion */ > >> + return csi2dc_prepare_notifier(csi2dc, input_node); > >> +} > >> + > >> +static int csi2dc_probe(struct platform_device *pdev) > >> +{ > >> + struct device *dev = &pdev->dev; > >> + struct csi2dc_device *csi2dc; > >> + struct resource *res = NULL; > >> + int ret = 0; > >> + u32 ver; > >> + > >> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL); > >> + if (!csi2dc) > >> + return -ENOMEM; > >> + > >> + csi2dc->dev = dev; > >> + > >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> + > >> + csi2dc->base = devm_ioremap_resource(dev, res); > > > > Should devm_platform_ioremap_resource(pdev, 0) be used here ? > > > >> + 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; > >> + } > >> + > >> + 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); > >> + return ret; > >> + } > >> + > >> + 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"); > >> + > >> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > >> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > >> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev); > > > > Not used it seems > > > >> + > >> + platform_set_drvdata(pdev, csi2dc); > >> + > >> + ret = csi2dc_of_parse(csi2dc, dev->of_node); > >> + if (ret) > >> + goto csi2dc_probe_cleanup_entity; > >> + > >> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > >> + if (csi2dc->video_pipe) > >> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; > >> + > >> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, > >> + csi2dc->video_pipe ? CSI2DC_PADS_NUM : 1, > >> + csi2dc->pads); > >> + if (ret < 0) { > >> + dev_err(dev, "media entity init failed\n"); > >> + goto csi2dc_probe_cleanup_entity; > > > > Should you also clean up the notifier in the error path ? > > > >> + } > >> + > >> + /* turn power on to validate capabilities */ > >> + ret = csi2dc_power(csi2dc, true); > >> + if (ret < 0) > >> + goto csi2dc_probe_cleanup_entity; > >> + > >> + pm_runtime_set_active(dev); > >> + pm_runtime_enable(dev); > >> + ver = csi2dc_readl(csi2dc, CSI2DC_VERSION); > >> + pm_request_idle(dev); > >> + > >> + /* > >> + * we must register the subdev after PM runtime has been requested, > >> + * otherwise we might bound immediately and request pm_runtime_resume > >> + * before runtime_enable. > >> + */ > >> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd); > >> + if (ret) { > >> + dev_err(csi2dc->dev, "failed to register the subdevice\n"); > >> + goto csi2dc_probe_cleanup_entity; > >> + } > >> + > >> + dev_info(dev, "Microchip CSI2DC version %x\n", ver); > >> + > >> + return 0; > >> + > >> +csi2dc_probe_cleanup_entity: > >> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); > >> + > >> + return ret; > >> +} > >> + > >> +static int csi2dc_remove(struct platform_device *pdev) > >> +{ > >> + struct csi2dc_device *csi2dc = platform_get_drvdata(pdev); > >> + > >> + pm_runtime_disable(&pdev->dev); > >> + > >> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd); > >> + csi2dc_cleanup_notifier(csi2dc); > >> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); > >> + > >> + return 0; > >> +} > >> + > >> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev) > >> +{ > >> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); > >> + > >> + return csi2dc_power(csi2dc, false); > >> +} > >> + > >> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev) > >> +{ > >> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); > >> + > >> + return csi2dc_power(csi2dc, true); > >> +} > >> + > >> +static const struct dev_pm_ops csi2dc_dev_pm_ops = { > >> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL) > >> +}; > >> + > >> +static const struct of_device_id csi2dc_of_match[] = { > >> + { .compatible = "microchip,sama7g5-csi2dc" }, > >> + { } > >> +}; > >> + > >> +MODULE_DEVICE_TABLE(of, csi2dc_of_match); > >> + > >> +static struct platform_driver csi2dc_driver = { > >> + .probe = csi2dc_probe, > >> + .remove = csi2dc_remove, > >> + .driver = { > >> + .name = "microchip-csi2dc", > >> + .pm = &csi2dc_dev_pm_ops, > >> + .of_match_table = of_match_ptr(csi2dc_of_match), > >> + }, > >> +}; > >> + > >> +module_platform_driver(csi2dc_driver); > >> + > >> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>"); > >> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver"); > >> +MODULE_LICENSE("GPL v2"); > >> -- > >> 2.25.1 > >> >
On 11/3/21 11:28 AM, Jacopo Mondi wrote: > Hi Eugen > > On Wed, Nov 03, 2021 at 08:31:36AM +0000, Eugen.Hristev@microchip.com wrote: >> On 11/2/21 7:22 PM, Jacopo Mondi wrote: >>> Hi Eugen, >>> >> >> Hi, >> >> Thank you for your review. I will try to understand everything you said >> and come up with a new version. I still have some inline questions >> though, about some things which are still unclear. >> >>> On Fri, Oct 22, 2021 at 10:52:29AM +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 this revision: >>>> - addressed comments by Jacopo and Laurent as in this thread: >>>> https://www.spinics.net/lists/linux-media/msg181044.html >>>> >>>> Previous change log : >>>> Changes in v5: >>>> - only in bindings >>>> >>>> Changes in v4: >>>> - now using get_mbus_config ops to get data from the subdevice, like the >>>> virtual channel id, and the clock type. >>>> - now having possibility to select any of the RAW10 data modes >>>> - at completion time, select which formats are also available in the subdevice, >>>> and move to the dynamic list accordingly >>>> - changed the pipeline integration, do not advertise subdev ready at probe time. >>>> wait until completion is done, and then start a workqueue that will register >>>> this device as a subdevice for the next element in pipeline. >>>> - moved the s_power code into a different function called now csi2dc_power >>>> that is called with CONFIG_PM functions. This is also called at completion, >>>> to have the device ready in case CONFIG_PM is not selected on the platform. >>>> - merged try_fmt into set_fmt >>>> - driver cleanup, wrapped lines over 80 characters >>>> >>>> Changes in v2: >>>> - moved driver to platform/atmel >>>> - fixed minor things as per Sakari's review >>>> - still some things from v2 review are not yet addressed, to be followed up >>>> >>>> >>>> drivers/media/platform/atmel/Kconfig | 15 + >>>> drivers/media/platform/atmel/Makefile | 1 + >>>> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++ >>>> 3 files changed, 716 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 dda2f27da317..f83bee373d82 100644 >>>> --- a/drivers/media/platform/atmel/Kconfig >>>> +++ b/drivers/media/platform/atmel/Kconfig >>>> @@ -40,3 +40,18 @@ 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. >>>> + >>>> + To compile this driver as a module, choose M here: the >>>> + module will be called microchip-csi2dc. >>>> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile >>>> index 46d264ab7948..39f0a7eba702 100644 >>>> --- a/drivers/media/platform/atmel/Makefile >>>> +++ b/drivers/media/platform/atmel/Makefile >>>> @@ -6,3 +6,4 @@ obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o >>>> obj-$(CONFIG_VIDEO_ATMEL_ISC_BASE) += atmel-isc-base.o >>>> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o >>>> obj-$(CONFIG_VIDEO_ATMEL_XISC) += atmel-xisc.o >>>> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o >>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c >>>> new file mode 100644 >>>> index 000000000000..277b86988eee >>>> --- /dev/null >>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c >>>> @@ -0,0 +1,700 @@ >>>> +// SPDX-License-Identifier: GPL-2.0-only >>>> +/* >>>> + * Microchip CSI2 Demux Controller (CSI2DC) driver >>>> + * >>>> + * Copyright (C) 2018 Microchip Technology, Inc. >>>> + * >>>> + * Author: Eugen Hristev <eugen.hristev@microchip.com> >>>> + * >>>> + */ >>>> + >>>> +#include <linux/clk.h> >>>> +#include <linux/module.h> >>>> +#include <linux/of.h> >>> >>> Isn't linux/mod_devicetable.h enough ? >>> >>>> +#include <linux/of_graph.h> >>> >>> You should probably move to use the fwnode_graph framwork instead of >>> of_graph. This driver depends on OF so it shouldn't be an issue but I >>> defer this to maintainers >>> >>>> +#include <linux/platform_device.h> >>>> +#include <linux/pm_runtime.h> >>>> +#include <linux/videodev2.h> >>>> + >>>> +#include <media/v4l2-device.h> >>> >>> Do you need this include ? >>> >>>> +#include <media/v4l2-fwnode.h> >>>> +#include <media/v4l2-subdev.h> >>>> +#include <media/videobuf2-dma-contig.h> >>> >>> Is this one needed as well ? >>> >>>> + >>>> +/* Global configuration register */ >>>> +#define CSI2DC_GCFG 0x0 >>>> + >>>> +/* MIPI sensor pixel clock is free running */ >>>> +#define CSI2DC_GCFG_MIPIFRN BIT(0) >>>> +/* Output waveform inter-line minimum delay */ >>>> +#define CSI2DC_GCFG_HLC(v) ((v) << 4) >>>> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4) >>>> +/* SAMA7G5 requires a HLC delay of 15 */ >>>> +#define SAMA7G5_HLC (15) >>>> + >>>> +/* Global control register */ >>>> +#define CSI2DC_GCTLR 0x04 >>>> +#define CSI2DC_GCTLR_SWRST BIT(0) >>>> + >>>> +/* Global status register */ >>>> +#define CSI2DC_GS 0x08 >>>> + >>>> +/* SSP interrupt status register */ >>>> +#define CSI2DC_SSPIS 0x28 >>>> +/* Pipe update register */ >>>> +#define CSI2DC_PU 0xC0 >>> >>> What about using lowercase for hex values (I know there's not strict >>> rule, so keep what you like the most, but most drivers use lowercase >>> >>>> +/* 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 - CSI2DC format type struct >>>> + * @mbus_code: Media bus code for the format >>>> + * @dt: Data type constant for this format >>>> + */ >>>> +struct csi2dc_format { >>>> + u32 mbus_code; >>>> + u32 dt; >>>> +}; >>>> + >>>> +static const struct csi2dc_format csi2dc_formats[] = { >>>> + { >>>> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, >>>> + .dt = CSI2DC_DT_RAW10, >>>> + }, { >>>> + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, >>>> + .dt = CSI2DC_DT_RAW10, >>>> + }, { >>>> + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, >>>> + .dt = CSI2DC_DT_RAW10, >>>> + }, { >>>> + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, >>>> + .dt = CSI2DC_DT_RAW10, >>>> + }, >>>> +}; >>> >>> How unfortunate we don't have this in the core... >>> >>>> + >>>> +enum mipi_csi_pads { >>>> + CSI2DC_PAD_SINK = 0, >>>> + CSI2DC_PAD_SOURCE = 1, >>>> + CSI2DC_PADS_NUM = 2, >>>> +}; >>>> + >>>> +/* >>>> + * struct csi2dc_device - CSI2DC device driver data/config struct >>>> + * @base: Register map base address >>>> + * @csi2dc_sd: v4l2 subdevice for the csi2dc device >>>> + * This is the subdevice that the csi2dc device itself >>>> + * registers in v4l2 subsystem >>>> + * @dev: struct device for this csi2dc device >>>> + * @pclk: Peripheral clock reference >>>> + * Input clock that clocks the hardware block internal >>>> + * logic >>>> + * @scck: Sensor Controller clock reference >>>> + * Input clock that is used to generate the pixel clock >>>> + * @format: Current saved format used in g/s fmt >>>> + * @cur_fmt: Current state format >>>> + * @try_fmt: Try format that is being tried >>>> + * @pads: Media entity pads for the csi2dc subdevice >>>> + * @clk_gated: Whether the clock is gated or free running >>>> + * @video_pipe: Whether video pipeline is configured >>>> + * @vc: Current set virtual channel >>>> + * @asd: Async subdevice for async bound of the underlying subdev >>>> + * @notifier: Async notifier that is used to bound the underlying >>>> + * subdevice to the csi2dc subdevice >>>> + * @input_sd: Reference to the underlying subdevice bound to the >>>> + * csi2dc subdevice >>>> + * @remote_pad: Pad number of the underlying subdevice that is linked >>>> + * to the csi2dc subdevice sink pad. >>>> + */ >>>> +struct csi2dc_device { >>>> + void __iomem *base; >>>> + struct v4l2_subdev csi2dc_sd; >>>> + struct device *dev; >>>> + struct clk *pclk; >>>> + struct clk *scck; >>>> + >>>> + struct v4l2_mbus_framefmt format; >>>> + >>>> + const struct csi2dc_format *cur_fmt; >>>> + const struct csi2dc_format *try_fmt; >>>> + >>>> + struct media_pad pads[CSI2DC_PADS_NUM]; >>>> + >>>> + bool clk_gated; >>>> + bool video_pipe; >>>> + u32 vc; >>>> + >>>> + struct v4l2_async_subdev *asd; >>>> + struct v4l2_async_notifier notifier; >>>> + >>>> + struct v4l2_subdev *input_sd; >>>> + >>>> + u32 remote_pad; >>>> +}; >>>> + >>>> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc) >>> >>> Could you move this below closer to the only caller ? >>> >>>> +{ >>>> + u32 vp; >>>> + >>>> + if (!csi2dc->cur_fmt) { >>> >>> You should probably initialize this to a default format >>> >>>> + dev_err(csi2dc->dev, "format must be configured first\n"); >>>> + return; >>>> + } >>>> + >>>> + if (!csi2dc->video_pipe) { >>> >>> This is only called internally by the driver at s_stream() time, can >>> this happen ? Or rather won't you have a streamon call also when the >>> data pipe only is available ? In that case you would error out here >> >> This can happen if the source node is not found in the DT. In that case, >> there is no video pipe available, thus this whole function should be a >> no-op (csi2dc_vp_update) . If no vp, there is no vp update. >> Normall without a vp, there should be a dp (data pipe), but I haven't >> tested this and it's not supported in the driver. >> It doesn't make any sense to configure the vp registers below if the vp >> is not available. >> However, I shouldn't return an error since the vp is not mandatory for >> the function of the hardware. Just that the dp will be implemented at a >> later time (.... or never). >> > > I see, so the data pipe is still work in progress... I understand > bindings should allow for that to be later accepted so you cannot > mandate port@1 to be there all the times, but should the driver > instead refuse to operate if no port@1 is provided ? > It will not refuse to operate, but won't operate anything basically. And if I make it refuse to operate, it will violate the bindings, since port@1 is not mandatory > Out of curiosity what will the interaction model with the driver be in > data pipe mode ? There will be a video device but there won't be an > link to it, how is stream start/stop controlled ? My idea is that on data pipe, the csi2dc should register it's own video device and act basically as a dma engine entity that will use an external DMA controller to just write data to DRAM. It is useful if we do not want to convert the MIPI/IDI packets into pixels, but rather dump the to DRAM and have a piece of software further use them. However, it's not a clear use case about this functionality at this moment. One possibility is to be able to obtain stream meta-data from the MIPI / IDI interface, like for example synchronization data, to synchronize with an audio capture device. > >>> >>>> + dev_err(csi2dc->dev, "video pipeline unavailable\n"); >>>> + return; >>>> + } >>>> + >>>> + 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_state *sd_state, >>>> + struct v4l2_subdev_mbus_code_enum *code) >>>> +{ >>>> + if (code->index >= ARRAY_SIZE(csi2dc_formats)) >>>> + return -EINVAL; >>>> + >>>> + code->code = csi2dc_formats[code->index].mbus_code; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int csi2dc_get_fmt(struct v4l2_subdev *csi2dc_sd, >>>> + struct v4l2_subdev_state *sd_state, >>>> + struct v4l2_subdev_format *format) >>>> +{ >>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); >>>> + >>>> + format->format = csi2dc->format; >>>> + >>>> + return 0; >>> >>> You should support try formats by storing the format in the file >>> handle state in s_fmt and return it in case which == TRY >>> >>> Grep for v4l2_subdev_get_try_format() for usage examples >> >> I did that initially, but could not find any utility for it in my tests. > > I understand.. Have you run v4l2-compliance on this sudev ? Doesn't it > test try formats support ? > >> So I removed it. >> I will try to bring it back, but I have no idea how to test that the try >> format works fine or not >> > > I think also v4l2-ctl supports try formats. > > --try-subdev-fmt pad=<pad>,width=<w>,height=<h>,code=<code>,field=<f>,colorspace=<c>, > > I'm not sure if it allows you to read it back, but it's easy to hack > it out just to verify it. > Ok, I will try it >>> >>>> +} >>>> + >>>> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd, >>>> + struct v4l2_subdev_state *sd_state, >>>> + struct v4l2_subdev_format *req_fmt) >>>> +{ >>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); >>>> + const struct csi2dc_format *fmt; >>>> + int i; >>> >>> unsigned >>> >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { >>>> + fmt = &csi2dc_formats[i]; >>>> + if (req_fmt->format.code == fmt->mbus_code) >>>> + csi2dc->try_fmt = fmt; >>> >>> Shouldn't you break ? >>> >>>> + fmt++; >>>> + } >>> >>> And make this a simpler >>> >>> for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { >>> if (req_fmt->format.code == csi2dc_formats[i].mbus_code) >>> break; >>> } >>> >>> if (i == ARRAY_SIZE(csi2dc_formats) >>> i = 0; >>> >>>> + >>>> + /* in case we could not find the desired format, default to something */ >>>> + if (!csi2dc->try_fmt || >>>> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) { >>>> + csi2dc->try_fmt = &csi2dc_formats[0]; >>>> + >>>> + dev_dbg(csi2dc->dev, >>>> + "CSI2DC unsupported format 0x%x, defaulting to 0x%x\n", >>>> + req_fmt->format.code, csi2dc_formats[0].mbus_code); >>>> + >>>> + req_fmt->format.code = csi2dc_formats[0].mbus_code; >>>> + } >>>> + >>>> + req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB; >>>> + req_fmt->format.field = V4L2_FIELD_NONE; >>>> + >>>> + /* save the format for later requests */ >>> >>> You should support TRY formats >>> >>>> + csi2dc->format = req_fmt->format; >>>> + >>>> + /* if we are just trying, we are done */ >>>> + if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) >>>> + return 0; >>>> + >>>> + csi2dc->cur_fmt = csi2dc->try_fmt; >>> >>> csi2dc->cur_fmt = &csi2dc_format[i]; >>> >>> So you can drop the try_fmt from the driver structure as it seems to >>> be used as a temporary variable here only. >>> >>>> + >>>> + dev_dbg(csi2dc->dev, "new format set: 0x%x\n", req_fmt->format.code); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int csi2dc_power(struct csi2dc_device *csi2dc, int on) >>>> +{ >>>> + int ret = 0; >>>> + >>>> + if (on) { >>>> + ret = clk_prepare_enable(csi2dc->pclk); >>>> + if (ret) { >>>> + dev_err(csi2dc->dev, "failed to enable pclk: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ret = clk_prepare_enable(csi2dc->scck); >>>> + if (ret) >>>> + dev_err(csi2dc->dev, >>>> + "failed to enable scck: %d\n", ret); >>> >>> Shouldn't you bail out here ? >> >> Initially this clock was not mandatory, but you are right, I should bail >> out. >> >>> >>>> + >>>> + /* if powering up, deassert reset line */ >>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST); >>>> + } else { >>>> + clk_disable_unprepare(csi2dc->scck); >>>> + >>>> + /* if powering down, assert reset line */ >>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST); >>> >>> Isn't reverse order of activation better ? >>> >>> csi2dc_writel(..) >>> clk_disable_unprepare(..) >>> clk_disable_unprepare(..) >>>> + >>>> + clk_disable_unprepare(csi2dc->pclk); >>>> + } >>>> + >>>> + 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); >>>> + int ret; >>>> + >>>> + if (enable) { >>>> + ret = pm_runtime_resume_and_get(csi2dc->dev); >>>> + if (ret < 0) >>>> + return ret; >>>> + csi2dc_vp_update(csi2dc); >>>> + } else { >>>> + pm_runtime_put_sync(csi2dc->dev); >>>> + } >>>> + >>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable); >>> >>> Should the remote subdev be started before and stopped after ? >> >> Do you mean s_power ? >> > > Nope, I meant the operation order. > > if (enable) { > v4l2_subdev_call(...); > > pm_runtime_resume_and_get(); > csi2dc_vp_update(); > } else { > pm_runtime_put_sync(); > > v4l2_subdev_call(); > } > > However, my comment is clearly wrong, it should have been the > contrary. Sorry about this > > It's right to configure the interface and power if up -before- > starting the remote subdev, but I would stop it before powering it > down. Like in: > > if (enable) { > pm_runtime_resume_and_get(); > csi2dc_vp_update(); > > v4l2_subdev_call(1); > } else { > v4l2_subdev_call(0); > > pm_runtime_put_sync(); > } > > What do you think ? Stopping the remote before powering the interface > down ensures no data is being put on the bus while the interace is > powered off. > In this case it might not work like this. If I start the sensor streaming before the vp update , then, there will be frames received on video pipe update, which means that the csi2dc will not detect the phy STOP state reached, and it will fail initialization. I believe the csi2dc has to be started before the sensor's first frame such that the interface is prepared for receiving. The PHY must be in stop state for a certain amount of time. I will try to see if it works the other way around, to double check to make sure. But if it doesn't , then the VP must be configured while the whole pipeline is in stop state. >>> >>>> +} >>>> + >>>> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = { >>>> + .enum_mbus_code = csi2dc_enum_mbus_code, >>>> + .set_fmt = csi2dc_set_fmt, >>>> + .get_fmt = csi2dc_get_fmt, >>>> +}; >>>> + >>>> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = { >>>> + .s_stream = csi2dc_s_stream, >>>> +}; >>>> + >>>> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = { >>>> + .pad = &csi2dc_pad_ops, >>>> + .video = &csi2dc_video_ops, >>>> +}; >>>> + >>>> +static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) >>>> +{ >>>> + struct v4l2_mbus_config mbus_config = { 0 }; >>>> + int ret; >>>> + >>>> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, get_mbus_config, >>>> + csi2dc->remote_pad, &mbus_config); >>>> + if (ret == -ENOIOCTLCMD) { >>>> + dev_dbg(csi2dc->dev, >>>> + "no remote mbus configuration available\n"); >>>> + goto csi2dc_get_mbus_config_defaults; >>>> + } >>>> + >>>> + if (ret) { >>>> + dev_err(csi2dc->dev, >>>> + "failed to get remote mbus configuration\n"); >>>> + goto csi2dc_get_mbus_config_defaults; >>>> + } >>>> + >>>> + if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) >>>> + csi2dc->vc = 0; >>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) >>>> + csi2dc->vc = 1; >>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) >>>> + csi2dc->vc = 2; >>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) >>>> + csi2dc->vc = 3; >>>> + >>>> + dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); >>>> + >>>> + csi2dc->clk_gated = mbus_config.flags & >>>> + V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK; >>> >>> This should come from the default clock-noncontinuous property in the >>> endpoint. It is available in the mbus_configuration only to support >>> subdevs that can change it at runtime, and if that's the case it's ok, >>> but I think it should be in the endpoint. >> >> I do not completely understand your point. In a previous version which >> you reviewed, I had this as a DT property (so in the endpoint ?), and >> now I converted it to a get_mbus_config flag get operation. > > I re-read the conversation and I think you're right. > >> It's not the right way to do it ? >> How should it be specified in the endpoint ? >> > > There's a default property 'clock-noncontinuous' which gets parsed by > v4l2_fwnode_endpoint_parse() and reported to drivers through the > V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK and V4L2_MBUS_CSI2_CONTINUOUS_CLOCK > flags. In your case, as you fetch it at run time it's not required if > not to initialize a default according to the property presence in DT. > > Up to you, what you have here it's fine! > Speaking of this, I have a parallel sensor that does not implement get_mbus_config . I tried to implement this in the sensor driver, to see if the csi2dc receives correct information from the sensor subdev (to see if it's parallel or serial). And it works fine. I sent a patch for this, but Sakari says that the csi2dc can obtain all this information from DT, and I guess he is right. What would be the correct way to do it then ? Could you comment on this patch please : https://lore.kernel.org/linux-media/YYJDcIiBXo%2FXlKCX@paasikivi.fi.intel.com/T/#m990b903ed6d56c3c34232c54c251490ad33e92c9 Thanks again for reviewing ! >>> >>> Speaking of remote subdevs, is there any driver available for IDI >>> transmitters ? >> >> I am not aware of any at the moment >>> >>>> + >>>> + dev_dbg(csi2dc->dev, "%s clock\n", >>>> + csi2dc->clk_gated ? "gated" : "free running"); >>>> + >>>> + return 0; >>>> + >>>> +csi2dc_get_mbus_config_defaults: >>>> + csi2dc->vc = 0; /* Virtual ID 0 by default */ >>>> + csi2dc->clk_gated = false; /* Free running clock by default */ >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier, >>>> + struct v4l2_subdev *subdev, >>>> + struct v4l2_async_subdev *asd) >>>> +{ >>>> + struct csi2dc_device *csi2dc = container_of(notifier, >>>> + struct csi2dc_device, notifier); >>>> + int pad; >>>> + int ret; >>>> + >>>> + csi2dc->input_sd = subdev; >>>> + >>>> + pad = media_entity_get_fwnode_pad(&subdev->entity, >>> >>> You can use 'ret' >> >> I can, but it is weird to do remote_pad = ret ... >> >> May bring confusion, don't you agree ? >> > > Up to you, it's a small detail :) > > Thanks! > j > >>> >>>> + asd->match.fwnode, >>>> + MEDIA_PAD_FL_SOURCE); >>>> + if (pad < 0) { >>>> + dev_err(csi2dc->dev, "Failed to find pad for %s\n", >>>> + subdev->name); >>>> + return pad; >>>> + } >>>> + >>>> + csi2dc->remote_pad = pad; >>>> + >>>> + csi2dc_get_mbus_config(csi2dc); >>> >>> Ideally, as this is not fatal, you could move this at s_stream time to >>> fetch the most up-to-date configuration >>> >>>> + >>>> + ret = media_create_pad_link(&csi2dc->input_sd->entity, >>>> + csi2dc->remote_pad, >>>> + &csi2dc->csi2dc_sd.entity, 0, >>>> + MEDIA_LNK_FL_ENABLED); >>>> + if (ret < 0) { >>> >>> if (ret) >>> >>>> + dev_err(csi2dc->dev, >>>> + "Failed to create pad link: %s to %s\n", >>>> + csi2dc->input_sd->entity.name, >>>> + csi2dc->csi2dc_sd.entity.name); >>>> + return ret; >>>> + } >>>> + >>>> + dev_dbg(csi2dc->dev, "link with %s pad: %d\n", >>>> + csi2dc->input_sd->name, csi2dc->remote_pad); >>>> + >>>> + ret = pm_runtime_resume_and_get(csi2dc->dev); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + csi2dc_writel(csi2dc, CSI2DC_GCFG, >>>> + (SAMA7G5_HLC & CSI2DC_GCFG_HLC_MASK) | >>>> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN)); >>>> + >>>> + csi2dc_writel(csi2dc, CSI2DC_VPCOL, >>>> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK); >>>> + csi2dc_writel(csi2dc, CSI2DC_VPROW, >>>> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK); >>>> + >>>> + pm_runtime_put_sync(csi2dc->dev); >>> >>> I would really move access to the HW to s_stream time if possible >>> >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = { >>>> + .bound = csi2dc_async_bound, >>>> +}; >>>> + >>>> +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_node) >>>> +{ >>>> + int ret = 0; >>>> + >>>> + v4l2_async_notifier_init(&csi2dc->notifier); >>>> + >>>> + csi2dc->asd = v4l2_async_notifier_add_fwnode_remote_subdev >>> >>> do you need asd in the driver structure ? >>> >>>> + (&csi2dc->notifier, of_fwnode_handle(input_node), >>>> + struct v4l2_async_subdev); >>>> + >>>> + of_node_put(input_node); >>>> + >>>> + if (IS_ERR(csi2dc->asd)) { >>>> + ret = PTR_ERR(csi2dc->asd); >>>> + dev_err(csi2dc->dev, >>>> + "failed to add async notifier for node %pOF: %d\n", >>>> + input_node, ret); >>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier); >>>> + return ret; >>>> + } >>>> + >>>> + csi2dc->notifier.ops = &csi2dc_async_ops; >>>> + >>>> + ret = v4l2_async_subdev_notifier_register(&csi2dc->csi2dc_sd, >>>> + &csi2dc->notifier); >>>> + >>>> + if (ret) { >>>> + dev_err(csi2dc->dev, "fail to register async notifier: %d\n", >>>> + ret); >>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier); >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc, >>>> + struct device_node *of_node) >>>> +{ >>>> + struct device_node *input_node, *output_node; >>>> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, >>>> + output_endpoint = { 0 }; >>>> + int ret; >>>> + >>>> + output_endpoint.bus_type = V4L2_MBUS_PARALLEL; >>>> + >>>> + input_node = of_graph_get_next_endpoint(of_node, NULL); >>>> + >>>> + if (!input_node) { >>>> + dev_err(csi2dc->dev, >>>> + "missing port node at %pOF, input node is mandatory.\n", >>>> + of_node); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node), >>>> + &input_endpoint); >>>> + >>>> + if (ret) { >>> of_node_put(input_node); >>> >>>> + dev_err(csi2dc->dev, "endpoint not defined at %pOF\n", of_node); >>>> + return ret; >>>> + } >>>> + >>>> + output_node = of_graph_get_next_endpoint(of_node, input_node); >>>> + >>>> + if (output_node) >>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(output_node), >>>> + &output_endpoint); >>> >>> of_node_put(output_node); >>>> + >>>> + if (!output_node || ret) { >>>> + dev_info(csi2dc->dev, >>>> + "missing output node at %pOF, data pipe available only.\n", >>>> + of_node); >>>> + } else { >>>> + csi2dc->video_pipe = true; >>>> + >>>> + dev_dbg(csi2dc->dev, "block %pOF %d.%d->%d.%d video pipeline\n", >>>> + of_node, input_endpoint.base.port, >>>> + input_endpoint.base.id, output_endpoint.base.port, >>>> + output_endpoint.base.id); >>>> + } >>>> + >>>> + of_node_put(output_node); >>> >>> Drop this if you put it above >>> >>>> + of_node_put(input_node); >>> >>> Should you put input_node before passing it to the function ? >>>> + >>>> + /* prepare async notifier for subdevice completion */ >>>> + return csi2dc_prepare_notifier(csi2dc, input_node); >>>> +} >>>> + >>>> +static int csi2dc_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device *dev = &pdev->dev; >>>> + struct csi2dc_device *csi2dc; >>>> + struct resource *res = NULL; >>>> + int ret = 0; >>>> + u32 ver; >>>> + >>>> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL); >>>> + if (!csi2dc) >>>> + return -ENOMEM; >>>> + >>>> + csi2dc->dev = dev; >>>> + >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> + >>>> + csi2dc->base = devm_ioremap_resource(dev, res); >>> >>> Should devm_platform_ioremap_resource(pdev, 0) be used here ? >>> >>>> + 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; >>>> + } >>>> + >>>> + 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); >>>> + return ret; >>>> + } >>>> + >>>> + 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"); >>>> + >>>> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >>>> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; >>>> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev); >>> >>> Not used it seems >>> >>>> + >>>> + platform_set_drvdata(pdev, csi2dc); >>>> + >>>> + ret = csi2dc_of_parse(csi2dc, dev->of_node); >>>> + if (ret) >>>> + goto csi2dc_probe_cleanup_entity; >>>> + >>>> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK; >>>> + if (csi2dc->video_pipe) >>>> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; >>>> + >>>> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, >>>> + csi2dc->video_pipe ? CSI2DC_PADS_NUM : 1, >>>> + csi2dc->pads); >>>> + if (ret < 0) { >>>> + dev_err(dev, "media entity init failed\n"); >>>> + goto csi2dc_probe_cleanup_entity; >>> >>> Should you also clean up the notifier in the error path ? >>> >>>> + } >>>> + >>>> + /* turn power on to validate capabilities */ >>>> + ret = csi2dc_power(csi2dc, true); >>>> + if (ret < 0) >>>> + goto csi2dc_probe_cleanup_entity; >>>> + >>>> + pm_runtime_set_active(dev); >>>> + pm_runtime_enable(dev); >>>> + ver = csi2dc_readl(csi2dc, CSI2DC_VERSION); >>>> + pm_request_idle(dev); >>>> + >>>> + /* >>>> + * we must register the subdev after PM runtime has been requested, >>>> + * otherwise we might bound immediately and request pm_runtime_resume >>>> + * before runtime_enable. >>>> + */ >>>> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd); >>>> + if (ret) { >>>> + dev_err(csi2dc->dev, "failed to register the subdevice\n"); >>>> + goto csi2dc_probe_cleanup_entity; >>>> + } >>>> + >>>> + dev_info(dev, "Microchip CSI2DC version %x\n", ver); >>>> + >>>> + return 0; >>>> + >>>> +csi2dc_probe_cleanup_entity: >>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int csi2dc_remove(struct platform_device *pdev) >>>> +{ >>>> + struct csi2dc_device *csi2dc = platform_get_drvdata(pdev); >>>> + >>>> + pm_runtime_disable(&pdev->dev); >>>> + >>>> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd); >>>> + csi2dc_cleanup_notifier(csi2dc); >>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev) >>>> +{ >>>> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); >>>> + >>>> + return csi2dc_power(csi2dc, false); >>>> +} >>>> + >>>> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev) >>>> +{ >>>> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); >>>> + >>>> + return csi2dc_power(csi2dc, true); >>>> +} >>>> + >>>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = { >>>> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL) >>>> +}; >>>> + >>>> +static const struct of_device_id csi2dc_of_match[] = { >>>> + { .compatible = "microchip,sama7g5-csi2dc" }, >>>> + { } >>>> +}; >>>> + >>>> +MODULE_DEVICE_TABLE(of, csi2dc_of_match); >>>> + >>>> +static struct platform_driver csi2dc_driver = { >>>> + .probe = csi2dc_probe, >>>> + .remove = csi2dc_remove, >>>> + .driver = { >>>> + .name = "microchip-csi2dc", >>>> + .pm = &csi2dc_dev_pm_ops, >>>> + .of_match_table = of_match_ptr(csi2dc_of_match), >>>> + }, >>>> +}; >>>> + >>>> +module_platform_driver(csi2dc_driver); >>>> + >>>> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>"); >>>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver"); >>>> +MODULE_LICENSE("GPL v2"); >>>> -- >>>> 2.25.1 >>>> >>
Hi Eugen On Wed, Nov 03, 2021 at 09:59:35AM +0000, Eugen.Hristev@microchip.com wrote: > On 11/3/21 11:28 AM, Jacopo Mondi wrote: > > Hi Eugen > > > > On Wed, Nov 03, 2021 at 08:31:36AM +0000, Eugen.Hristev@microchip.com wrote: > >> On 11/2/21 7:22 PM, Jacopo Mondi wrote: > >>> Hi Eugen, > >>> > >> > >> Hi, > >> > >> Thank you for your review. I will try to understand everything you said > >> and come up with a new version. I still have some inline questions > >> though, about some things which are still unclear. > >> > >>> On Fri, Oct 22, 2021 at 10:52:29AM +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 this revision: > >>>> - addressed comments by Jacopo and Laurent as in this thread: > >>>> https://www.spinics.net/lists/linux-media/msg181044.html > >>>> > >>>> Previous change log : > >>>> Changes in v5: > >>>> - only in bindings > >>>> > >>>> Changes in v4: > >>>> - now using get_mbus_config ops to get data from the subdevice, like the > >>>> virtual channel id, and the clock type. > >>>> - now having possibility to select any of the RAW10 data modes > >>>> - at completion time, select which formats are also available in the subdevice, > >>>> and move to the dynamic list accordingly > >>>> - changed the pipeline integration, do not advertise subdev ready at probe time. > >>>> wait until completion is done, and then start a workqueue that will register > >>>> this device as a subdevice for the next element in pipeline. > >>>> - moved the s_power code into a different function called now csi2dc_power > >>>> that is called with CONFIG_PM functions. This is also called at completion, > >>>> to have the device ready in case CONFIG_PM is not selected on the platform. > >>>> - merged try_fmt into set_fmt > >>>> - driver cleanup, wrapped lines over 80 characters > >>>> > >>>> Changes in v2: > >>>> - moved driver to platform/atmel > >>>> - fixed minor things as per Sakari's review > >>>> - still some things from v2 review are not yet addressed, to be followed up > >>>> > >>>> > >>>> drivers/media/platform/atmel/Kconfig | 15 + > >>>> drivers/media/platform/atmel/Makefile | 1 + > >>>> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++ > >>>> 3 files changed, 716 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 dda2f27da317..f83bee373d82 100644 > >>>> --- a/drivers/media/platform/atmel/Kconfig > >>>> +++ b/drivers/media/platform/atmel/Kconfig > >>>> @@ -40,3 +40,18 @@ 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. > >>>> + > >>>> + To compile this driver as a module, choose M here: the > >>>> + module will be called microchip-csi2dc. > >>>> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile > >>>> index 46d264ab7948..39f0a7eba702 100644 > >>>> --- a/drivers/media/platform/atmel/Makefile > >>>> +++ b/drivers/media/platform/atmel/Makefile > >>>> @@ -6,3 +6,4 @@ obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o > >>>> obj-$(CONFIG_VIDEO_ATMEL_ISC_BASE) += atmel-isc-base.o > >>>> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o > >>>> obj-$(CONFIG_VIDEO_ATMEL_XISC) += atmel-xisc.o > >>>> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o > >>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c > >>>> new file mode 100644 > >>>> index 000000000000..277b86988eee > >>>> --- /dev/null > >>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c > >>>> @@ -0,0 +1,700 @@ > >>>> +// SPDX-License-Identifier: GPL-2.0-only > >>>> +/* > >>>> + * Microchip CSI2 Demux Controller (CSI2DC) driver > >>>> + * > >>>> + * Copyright (C) 2018 Microchip Technology, Inc. > >>>> + * > >>>> + * Author: Eugen Hristev <eugen.hristev@microchip.com> > >>>> + * > >>>> + */ > >>>> + > >>>> +#include <linux/clk.h> > >>>> +#include <linux/module.h> > >>>> +#include <linux/of.h> > >>> > >>> Isn't linux/mod_devicetable.h enough ? > >>> > >>>> +#include <linux/of_graph.h> > >>> > >>> You should probably move to use the fwnode_graph framwork instead of > >>> of_graph. This driver depends on OF so it shouldn't be an issue but I > >>> defer this to maintainers > >>> > >>>> +#include <linux/platform_device.h> > >>>> +#include <linux/pm_runtime.h> > >>>> +#include <linux/videodev2.h> > >>>> + > >>>> +#include <media/v4l2-device.h> > >>> > >>> Do you need this include ? > >>> > >>>> +#include <media/v4l2-fwnode.h> > >>>> +#include <media/v4l2-subdev.h> > >>>> +#include <media/videobuf2-dma-contig.h> > >>> > >>> Is this one needed as well ? > >>> > >>>> + > >>>> +/* Global configuration register */ > >>>> +#define CSI2DC_GCFG 0x0 > >>>> + > >>>> +/* MIPI sensor pixel clock is free running */ > >>>> +#define CSI2DC_GCFG_MIPIFRN BIT(0) > >>>> +/* Output waveform inter-line minimum delay */ > >>>> +#define CSI2DC_GCFG_HLC(v) ((v) << 4) > >>>> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4) > >>>> +/* SAMA7G5 requires a HLC delay of 15 */ > >>>> +#define SAMA7G5_HLC (15) > >>>> + > >>>> +/* Global control register */ > >>>> +#define CSI2DC_GCTLR 0x04 > >>>> +#define CSI2DC_GCTLR_SWRST BIT(0) > >>>> + > >>>> +/* Global status register */ > >>>> +#define CSI2DC_GS 0x08 > >>>> + > >>>> +/* SSP interrupt status register */ > >>>> +#define CSI2DC_SSPIS 0x28 > >>>> +/* Pipe update register */ > >>>> +#define CSI2DC_PU 0xC0 > >>> > >>> What about using lowercase for hex values (I know there's not strict > >>> rule, so keep what you like the most, but most drivers use lowercase > >>> > >>>> +/* 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 - CSI2DC format type struct > >>>> + * @mbus_code: Media bus code for the format > >>>> + * @dt: Data type constant for this format > >>>> + */ > >>>> +struct csi2dc_format { > >>>> + u32 mbus_code; > >>>> + u32 dt; > >>>> +}; > >>>> + > >>>> +static const struct csi2dc_format csi2dc_formats[] = { > >>>> + { > >>>> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, > >>>> + .dt = CSI2DC_DT_RAW10, > >>>> + }, { > >>>> + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, > >>>> + .dt = CSI2DC_DT_RAW10, > >>>> + }, { > >>>> + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, > >>>> + .dt = CSI2DC_DT_RAW10, > >>>> + }, { > >>>> + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, > >>>> + .dt = CSI2DC_DT_RAW10, > >>>> + }, > >>>> +}; > >>> > >>> How unfortunate we don't have this in the core... > >>> > >>>> + > >>>> +enum mipi_csi_pads { > >>>> + CSI2DC_PAD_SINK = 0, > >>>> + CSI2DC_PAD_SOURCE = 1, > >>>> + CSI2DC_PADS_NUM = 2, > >>>> +}; > >>>> + > >>>> +/* > >>>> + * struct csi2dc_device - CSI2DC device driver data/config struct > >>>> + * @base: Register map base address > >>>> + * @csi2dc_sd: v4l2 subdevice for the csi2dc device > >>>> + * This is the subdevice that the csi2dc device itself > >>>> + * registers in v4l2 subsystem > >>>> + * @dev: struct device for this csi2dc device > >>>> + * @pclk: Peripheral clock reference > >>>> + * Input clock that clocks the hardware block internal > >>>> + * logic > >>>> + * @scck: Sensor Controller clock reference > >>>> + * Input clock that is used to generate the pixel clock > >>>> + * @format: Current saved format used in g/s fmt > >>>> + * @cur_fmt: Current state format > >>>> + * @try_fmt: Try format that is being tried > >>>> + * @pads: Media entity pads for the csi2dc subdevice > >>>> + * @clk_gated: Whether the clock is gated or free running > >>>> + * @video_pipe: Whether video pipeline is configured > >>>> + * @vc: Current set virtual channel > >>>> + * @asd: Async subdevice for async bound of the underlying subdev > >>>> + * @notifier: Async notifier that is used to bound the underlying > >>>> + * subdevice to the csi2dc subdevice > >>>> + * @input_sd: Reference to the underlying subdevice bound to the > >>>> + * csi2dc subdevice > >>>> + * @remote_pad: Pad number of the underlying subdevice that is linked > >>>> + * to the csi2dc subdevice sink pad. > >>>> + */ > >>>> +struct csi2dc_device { > >>>> + void __iomem *base; > >>>> + struct v4l2_subdev csi2dc_sd; > >>>> + struct device *dev; > >>>> + struct clk *pclk; > >>>> + struct clk *scck; > >>>> + > >>>> + struct v4l2_mbus_framefmt format; > >>>> + > >>>> + const struct csi2dc_format *cur_fmt; > >>>> + const struct csi2dc_format *try_fmt; > >>>> + > >>>> + struct media_pad pads[CSI2DC_PADS_NUM]; > >>>> + > >>>> + bool clk_gated; > >>>> + bool video_pipe; > >>>> + u32 vc; > >>>> + > >>>> + struct v4l2_async_subdev *asd; > >>>> + struct v4l2_async_notifier notifier; > >>>> + > >>>> + struct v4l2_subdev *input_sd; > >>>> + > >>>> + u32 remote_pad; > >>>> +}; > >>>> + > >>>> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc) > >>> > >>> Could you move this below closer to the only caller ? > >>> > >>>> +{ > >>>> + u32 vp; > >>>> + > >>>> + if (!csi2dc->cur_fmt) { > >>> > >>> You should probably initialize this to a default format > >>> > >>>> + dev_err(csi2dc->dev, "format must be configured first\n"); > >>>> + return; > >>>> + } > >>>> + > >>>> + if (!csi2dc->video_pipe) { > >>> > >>> This is only called internally by the driver at s_stream() time, can > >>> this happen ? Or rather won't you have a streamon call also when the > >>> data pipe only is available ? In that case you would error out here > >> > >> This can happen if the source node is not found in the DT. In that case, > >> there is no video pipe available, thus this whole function should be a > >> no-op (csi2dc_vp_update) . If no vp, there is no vp update. > >> Normall without a vp, there should be a dp (data pipe), but I haven't > >> tested this and it's not supported in the driver. > >> It doesn't make any sense to configure the vp registers below if the vp > >> is not available. > >> However, I shouldn't return an error since the vp is not mandatory for > >> the function of the hardware. Just that the dp will be implemented at a > >> later time (.... or never). > >> > > > > I see, so the data pipe is still work in progress... I understand > > bindings should allow for that to be later accepted so you cannot > > mandate port@1 to be there all the times, but should the driver > > instead refuse to operate if no port@1 is provided ? > > > > It will not refuse to operate, but won't operate anything basically. > And if I make it refuse to operate, it will violate the bindings, since > port@1 is not mandatory I don't think that's a problem. I would say the it would also be fine to have port@1 mandatory as if it becomes optional in future once the data-pipe is implemented, old DTS will still be compatible. However I also think it would be fine for the driver to not support all the options listed in the bindings. What's importante is that anything that gets into the bindings does not break retro-compatibility in case it gets extended. To make a counter example, if your bindings define port@1 as optional but you later make it mandatory for some reason, old dts without port@1 won't work anymore and that's a problem. In this case if you make it mandatory and later demote it to be optional, old DTS will all have port@1 and will work in video-pipe mode while new ones can optionally swtich to data-pipe by not providing port@1. Anyway, not a big deal, I think what you have here is fine but I won't bother too much about checking for if (!csi2dc->video_pipe) in the driver as if I understood you correctly the driver cannot currently be operated in data-pipe mode. > > > Out of curiosity what will the interaction model with the driver be in > > data pipe mode ? There will be a video device but there won't be an > > link to it, how is stream start/stop controlled ? > > My idea is that on data pipe, the csi2dc should register it's own video > device and act basically as a dma engine entity that will use an > external DMA controller to just write data to DRAM. > > It is useful if we do not want to convert the MIPI/IDI packets into > pixels, but rather dump the to DRAM and have a piece of software further > use them. > > However, it's not a clear use case about this functionality at this > moment. One possibility is to be able to obtain stream meta-data from > the MIPI / IDI interface, like for example synchronization data, to > synchronize with an audio capture device. > > > > >>> > >>>> + dev_err(csi2dc->dev, "video pipeline unavailable\n"); > >>>> + return; > >>>> + } > >>>> + > >>>> + 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_state *sd_state, > >>>> + struct v4l2_subdev_mbus_code_enum *code) > >>>> +{ > >>>> + if (code->index >= ARRAY_SIZE(csi2dc_formats)) > >>>> + return -EINVAL; > >>>> + > >>>> + code->code = csi2dc_formats[code->index].mbus_code; > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int csi2dc_get_fmt(struct v4l2_subdev *csi2dc_sd, > >>>> + struct v4l2_subdev_state *sd_state, > >>>> + struct v4l2_subdev_format *format) > >>>> +{ > >>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); > >>>> + > >>>> + format->format = csi2dc->format; > >>>> + > >>>> + return 0; > >>> > >>> You should support try formats by storing the format in the file > >>> handle state in s_fmt and return it in case which == TRY > >>> > >>> Grep for v4l2_subdev_get_try_format() for usage examples > >> > >> I did that initially, but could not find any utility for it in my tests. > > > > I understand.. Have you run v4l2-compliance on this sudev ? Doesn't it > > test try formats support ? > > > >> So I removed it. > >> I will try to bring it back, but I have no idea how to test that the try > >> format works fine or not > >> > > > > I think also v4l2-ctl supports try formats. > > > > --try-subdev-fmt pad=<pad>,width=<w>,height=<h>,code=<code>,field=<f>,colorspace=<c>, > > > > I'm not sure if it allows you to read it back, but it's easy to hack > > it out just to verify it. > > > > Ok, I will try it > > >>> > >>>> +} > >>>> + > >>>> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd, > >>>> + struct v4l2_subdev_state *sd_state, > >>>> + struct v4l2_subdev_format *req_fmt) > >>>> +{ > >>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); > >>>> + const struct csi2dc_format *fmt; > >>>> + int i; > >>> > >>> unsigned > >>> > >>>> + > >>>> + for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { > >>>> + fmt = &csi2dc_formats[i]; > >>>> + if (req_fmt->format.code == fmt->mbus_code) > >>>> + csi2dc->try_fmt = fmt; > >>> > >>> Shouldn't you break ? > >>> > >>>> + fmt++; > >>>> + } > >>> > >>> And make this a simpler > >>> > >>> for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { > >>> if (req_fmt->format.code == csi2dc_formats[i].mbus_code) > >>> break; > >>> } > >>> > >>> if (i == ARRAY_SIZE(csi2dc_formats) > >>> i = 0; > >>> > >>>> + > >>>> + /* in case we could not find the desired format, default to something */ > >>>> + if (!csi2dc->try_fmt || > >>>> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) { > >>>> + csi2dc->try_fmt = &csi2dc_formats[0]; > >>>> + > >>>> + dev_dbg(csi2dc->dev, > >>>> + "CSI2DC unsupported format 0x%x, defaulting to 0x%x\n", > >>>> + req_fmt->format.code, csi2dc_formats[0].mbus_code); > >>>> + > >>>> + req_fmt->format.code = csi2dc_formats[0].mbus_code; > >>>> + } > >>>> + > >>>> + req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB; > >>>> + req_fmt->format.field = V4L2_FIELD_NONE; > >>>> + > >>>> + /* save the format for later requests */ > >>> > >>> You should support TRY formats > >>> > >>>> + csi2dc->format = req_fmt->format; > >>>> + > >>>> + /* if we are just trying, we are done */ > >>>> + if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) > >>>> + return 0; > >>>> + > >>>> + csi2dc->cur_fmt = csi2dc->try_fmt; > >>> > >>> csi2dc->cur_fmt = &csi2dc_format[i]; > >>> > >>> So you can drop the try_fmt from the driver structure as it seems to > >>> be used as a temporary variable here only. > >>> > >>>> + > >>>> + dev_dbg(csi2dc->dev, "new format set: 0x%x\n", req_fmt->format.code); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int csi2dc_power(struct csi2dc_device *csi2dc, int on) > >>>> +{ > >>>> + int ret = 0; > >>>> + > >>>> + if (on) { > >>>> + ret = clk_prepare_enable(csi2dc->pclk); > >>>> + if (ret) { > >>>> + dev_err(csi2dc->dev, "failed to enable pclk: %d\n", ret); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + ret = clk_prepare_enable(csi2dc->scck); > >>>> + if (ret) > >>>> + dev_err(csi2dc->dev, > >>>> + "failed to enable scck: %d\n", ret); > >>> > >>> Shouldn't you bail out here ? > >> > >> Initially this clock was not mandatory, but you are right, I should bail > >> out. > >> > >>> > >>>> + > >>>> + /* if powering up, deassert reset line */ > >>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST); > >>>> + } else { > >>>> + clk_disable_unprepare(csi2dc->scck); > >>>> + > >>>> + /* if powering down, assert reset line */ > >>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST); > >>> > >>> Isn't reverse order of activation better ? > >>> > >>> csi2dc_writel(..) > >>> clk_disable_unprepare(..) > >>> clk_disable_unprepare(..) > >>>> + > >>>> + clk_disable_unprepare(csi2dc->pclk); > >>>> + } > >>>> + > >>>> + 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); > >>>> + int ret; > >>>> + > >>>> + if (enable) { > >>>> + ret = pm_runtime_resume_and_get(csi2dc->dev); > >>>> + if (ret < 0) > >>>> + return ret; > >>>> + csi2dc_vp_update(csi2dc); > >>>> + } else { > >>>> + pm_runtime_put_sync(csi2dc->dev); > >>>> + } > >>>> + > >>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable); > >>> > >>> Should the remote subdev be started before and stopped after ? > >> > >> Do you mean s_power ? > >> > > > > Nope, I meant the operation order. > > > > if (enable) { > > v4l2_subdev_call(...); > > > > pm_runtime_resume_and_get(); > > csi2dc_vp_update(); > > } else { > > pm_runtime_put_sync(); > > > > v4l2_subdev_call(); > > } > > > > However, my comment is clearly wrong, it should have been the > > contrary. Sorry about this > > > > It's right to configure the interface and power if up -before- > > starting the remote subdev, but I would stop it before powering it > > down. Like in: > > > > if (enable) { > > pm_runtime_resume_and_get(); > > csi2dc_vp_update(); > > > > v4l2_subdev_call(1); > > } else { > > v4l2_subdev_call(0); > > > > pm_runtime_put_sync(); > > } > > > > What do you think ? Stopping the remote before powering the interface > > down ensures no data is being put on the bus while the interace is > > powered off. > > > > In this case it might not work like this. If I start the sensor > streaming before the vp update , then, there will be frames received on > video pipe update, which means that the csi2dc will not detect the phy > STOP state reached, and it will fail initialization. > I believe the csi2dc has to be started before the sensor's first frame > such that the interface is prepared for receiving. The PHY must be in > stop state for a certain amount of time. > I will try to see if it works the other way around, to double check to > make sure. > But if it doesn't , then the VP must be configured while the whole > pipeline is in stop state. > I agree, as I've said my first comment was wrong and the sensor has to be started after the csi2dc. However in my last comment I was suggesting to stop it before the csi2dc to avoid having data being sent on the bus while the csi2dc gets powered down. > >>> > >>>> +} > >>>> + > >>>> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = { > >>>> + .enum_mbus_code = csi2dc_enum_mbus_code, > >>>> + .set_fmt = csi2dc_set_fmt, > >>>> + .get_fmt = csi2dc_get_fmt, > >>>> +}; > >>>> + > >>>> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = { > >>>> + .s_stream = csi2dc_s_stream, > >>>> +}; > >>>> + > >>>> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = { > >>>> + .pad = &csi2dc_pad_ops, > >>>> + .video = &csi2dc_video_ops, > >>>> +}; > >>>> + > >>>> +static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > >>>> +{ > >>>> + struct v4l2_mbus_config mbus_config = { 0 }; > >>>> + int ret; > >>>> + > >>>> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, get_mbus_config, > >>>> + csi2dc->remote_pad, &mbus_config); > >>>> + if (ret == -ENOIOCTLCMD) { > >>>> + dev_dbg(csi2dc->dev, > >>>> + "no remote mbus configuration available\n"); > >>>> + goto csi2dc_get_mbus_config_defaults; > >>>> + } > >>>> + > >>>> + if (ret) { > >>>> + dev_err(csi2dc->dev, > >>>> + "failed to get remote mbus configuration\n"); > >>>> + goto csi2dc_get_mbus_config_defaults; > >>>> + } > >>>> + > >>>> + if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) > >>>> + csi2dc->vc = 0; > >>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) > >>>> + csi2dc->vc = 1; > >>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) > >>>> + csi2dc->vc = 2; > >>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) > >>>> + csi2dc->vc = 3; > >>>> + > >>>> + dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); > >>>> + > >>>> + csi2dc->clk_gated = mbus_config.flags & > >>>> + V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK; > >>> > >>> This should come from the default clock-noncontinuous property in the > >>> endpoint. It is available in the mbus_configuration only to support > >>> subdevs that can change it at runtime, and if that's the case it's ok, > >>> but I think it should be in the endpoint. > >> > >> I do not completely understand your point. In a previous version which > >> you reviewed, I had this as a DT property (so in the endpoint ?), and > >> now I converted it to a get_mbus_config flag get operation. > > > > I re-read the conversation and I think you're right. > > > >> It's not the right way to do it ? > >> How should it be specified in the endpoint ? > >> > > > > There's a default property 'clock-noncontinuous' which gets parsed by > > v4l2_fwnode_endpoint_parse() and reported to drivers through the > > V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK and V4L2_MBUS_CSI2_CONTINUOUS_CLOCK > > flags. In your case, as you fetch it at run time it's not required if > > not to initialize a default according to the property presence in DT. > > > > Up to you, what you have here it's fine! > > > > Speaking of this, I have a parallel sensor that does not implement > get_mbus_config . > > I tried to implement this in the sensor driver, to see if the csi2dc > receives correct information from the sensor subdev (to see if it's > parallel or serial). And it works fine. > I sent a patch for this, but Sakari says that the csi2dc can obtain all > this information from DT, and I guess he is right. What would be the > correct way to do it then ? Good question, and probably Sakari is in a better position than me to set a direction. My take is that get_mbus_config should be used to retrieve from the remote subdevice configuration parameters that can change at runtime, or to express properties that cannot be defined in bindings as they're not strictly media bus configuration parameters (like the CSI-2 VC, as you're doing here). To give you another example, I used get_mbus_config to retrieve at run-time the number of CSI-2 data lanes currently in use, as I have a transmitter that if configured to output low resolution data needs to reduce the number of data lanes to stay in the D-PHY defined clock frequency lower bound (I think that's the use case that lead to resurect get_mbus_config() as a pad operation, as in the past there was a g_mbus_config() video operation that got deprecated along with the soc_camera framework). The noncontinuous clock option cannot be changed at runtime afaict and it's indeed an mbus configuration parameter for which a standard DT-property is defined so it makes sense to have it in DTS. The clear win is that your receiver driver can operate with sensor drivers not instrumented with get_mbus_config() as it will get the info from DTS. In your case you require get_mbus_config() for VC handling, something everyone hopes is temporary as long as the in-development multiplexed stream support has not landed, but if the operation is not implemented you fallback to a default which is indeed the best option for VC, but for clock-continuous you could avoid that by using the standard DTS mechanism and retain compatibility with subdevices that do not implement get_mbus_config. I know in v3 you have been suggested to use get_mbus_config and I'm sorry if it drove you off-road, but I think the best option here is: - get the clock continuous property from DTS - as you need get_mbus_config() for VC, inspect if the remote reports an option for clock continuous and use it if that's the case - In case the subdev does not implement get_mbus_config() or does not report clock information use the value parsed from DTS instead of an arbitrary default This should allow you to operate with sensor drivers not instrumented with get_mbus_config() (as long as they send data on the VC you use as a defaul fallback :) > > Could you comment on this patch please : > > https://lore.kernel.org/linux-media/YYJDcIiBXo%2FXlKCX@paasikivi.fi.intel.com/T/#m990b903ed6d56c3c34232c54c251490ad33e92c9 > Afaict all the mbus configuration for a parallel bus can be fully described in DTS, and for the above reasons of being compatible with most subdev drivers I would fetch it from there. > > Thanks again for reviewing ! > You're welcome! > >>> > >>> Speaking of remote subdevs, is there any driver available for IDI > >>> transmitters ? > >> > >> I am not aware of any at the moment > >>> > >>>> + > >>>> + dev_dbg(csi2dc->dev, "%s clock\n", > >>>> + csi2dc->clk_gated ? "gated" : "free running"); > >>>> + > >>>> + return 0; > >>>> + > >>>> +csi2dc_get_mbus_config_defaults: > >>>> + csi2dc->vc = 0; /* Virtual ID 0 by default */ > >>>> + csi2dc->clk_gated = false; /* Free running clock by default */ > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier, > >>>> + struct v4l2_subdev *subdev, > >>>> + struct v4l2_async_subdev *asd) > >>>> +{ > >>>> + struct csi2dc_device *csi2dc = container_of(notifier, > >>>> + struct csi2dc_device, notifier); > >>>> + int pad; > >>>> + int ret; > >>>> + > >>>> + csi2dc->input_sd = subdev; > >>>> + > >>>> + pad = media_entity_get_fwnode_pad(&subdev->entity, > >>> > >>> You can use 'ret' > >> > >> I can, but it is weird to do remote_pad = ret ... > >> > >> May bring confusion, don't you agree ? > >> > > > > Up to you, it's a small detail :) > > > > Thanks! > > j > > > >>> > >>>> + asd->match.fwnode, > >>>> + MEDIA_PAD_FL_SOURCE); > >>>> + if (pad < 0) { > >>>> + dev_err(csi2dc->dev, "Failed to find pad for %s\n", > >>>> + subdev->name); > >>>> + return pad; > >>>> + } > >>>> + > >>>> + csi2dc->remote_pad = pad; > >>>> + > >>>> + csi2dc_get_mbus_config(csi2dc); > >>> > >>> Ideally, as this is not fatal, you could move this at s_stream time to > >>> fetch the most up-to-date configuration > >>> > >>>> + > >>>> + ret = media_create_pad_link(&csi2dc->input_sd->entity, > >>>> + csi2dc->remote_pad, > >>>> + &csi2dc->csi2dc_sd.entity, 0, > >>>> + MEDIA_LNK_FL_ENABLED); > >>>> + if (ret < 0) { > >>> > >>> if (ret) > >>> > >>>> + dev_err(csi2dc->dev, > >>>> + "Failed to create pad link: %s to %s\n", > >>>> + csi2dc->input_sd->entity.name, > >>>> + csi2dc->csi2dc_sd.entity.name); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + dev_dbg(csi2dc->dev, "link with %s pad: %d\n", > >>>> + csi2dc->input_sd->name, csi2dc->remote_pad); > >>>> + > >>>> + ret = pm_runtime_resume_and_get(csi2dc->dev); > >>>> + if (ret < 0) > >>>> + return ret; > >>>> + > >>>> + csi2dc_writel(csi2dc, CSI2DC_GCFG, > >>>> + (SAMA7G5_HLC & CSI2DC_GCFG_HLC_MASK) | > >>>> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN)); > >>>> + > >>>> + csi2dc_writel(csi2dc, CSI2DC_VPCOL, > >>>> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK); > >>>> + csi2dc_writel(csi2dc, CSI2DC_VPROW, > >>>> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK); > >>>> + > >>>> + pm_runtime_put_sync(csi2dc->dev); > >>> > >>> I would really move access to the HW to s_stream time if possible > >>> > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = { > >>>> + .bound = csi2dc_async_bound, > >>>> +}; > >>>> + > >>>> +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_node) > >>>> +{ > >>>> + int ret = 0; > >>>> + > >>>> + v4l2_async_notifier_init(&csi2dc->notifier); > >>>> + > >>>> + csi2dc->asd = v4l2_async_notifier_add_fwnode_remote_subdev > >>> > >>> do you need asd in the driver structure ? > >>> > >>>> + (&csi2dc->notifier, of_fwnode_handle(input_node), > >>>> + struct v4l2_async_subdev); > >>>> + > >>>> + of_node_put(input_node); > >>>> + > >>>> + if (IS_ERR(csi2dc->asd)) { > >>>> + ret = PTR_ERR(csi2dc->asd); > >>>> + dev_err(csi2dc->dev, > >>>> + "failed to add async notifier for node %pOF: %d\n", > >>>> + input_node, ret); > >>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + csi2dc->notifier.ops = &csi2dc_async_ops; > >>>> + > >>>> + ret = v4l2_async_subdev_notifier_register(&csi2dc->csi2dc_sd, > >>>> + &csi2dc->notifier); > >>>> + > >>>> + if (ret) { > >>>> + dev_err(csi2dc->dev, "fail to register async notifier: %d\n", > >>>> + ret); > >>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier); > >>>> + } > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc, > >>>> + struct device_node *of_node) > >>>> +{ > >>>> + struct device_node *input_node, *output_node; > >>>> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, > >>>> + output_endpoint = { 0 }; > >>>> + int ret; > >>>> + > >>>> + output_endpoint.bus_type = V4L2_MBUS_PARALLEL; > >>>> + > >>>> + input_node = of_graph_get_next_endpoint(of_node, NULL); > >>>> + > >>>> + if (!input_node) { > >>>> + dev_err(csi2dc->dev, > >>>> + "missing port node at %pOF, input node is mandatory.\n", > >>>> + of_node); > >>>> + return -EINVAL; > >>>> + } > >>>> + > >>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node), > >>>> + &input_endpoint); > >>>> + > >>>> + if (ret) { > >>> of_node_put(input_node); > >>> > >>>> + dev_err(csi2dc->dev, "endpoint not defined at %pOF\n", of_node); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + output_node = of_graph_get_next_endpoint(of_node, input_node); > >>>> + > >>>> + if (output_node) > >>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(output_node), > >>>> + &output_endpoint); > >>> > >>> of_node_put(output_node); > >>>> + > >>>> + if (!output_node || ret) { > >>>> + dev_info(csi2dc->dev, > >>>> + "missing output node at %pOF, data pipe available only.\n", > >>>> + of_node); > >>>> + } else { > >>>> + csi2dc->video_pipe = true; > >>>> + > >>>> + dev_dbg(csi2dc->dev, "block %pOF %d.%d->%d.%d video pipeline\n", > >>>> + of_node, input_endpoint.base.port, > >>>> + input_endpoint.base.id, output_endpoint.base.port, > >>>> + output_endpoint.base.id); > >>>> + } > >>>> + > >>>> + of_node_put(output_node); > >>> > >>> Drop this if you put it above > >>> > >>>> + of_node_put(input_node); > >>> > >>> Should you put input_node before passing it to the function ? > >>>> + > >>>> + /* prepare async notifier for subdevice completion */ > >>>> + return csi2dc_prepare_notifier(csi2dc, input_node); > >>>> +} > >>>> + > >>>> +static int csi2dc_probe(struct platform_device *pdev) > >>>> +{ > >>>> + struct device *dev = &pdev->dev; > >>>> + struct csi2dc_device *csi2dc; > >>>> + struct resource *res = NULL; > >>>> + int ret = 0; > >>>> + u32 ver; > >>>> + > >>>> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL); > >>>> + if (!csi2dc) > >>>> + return -ENOMEM; > >>>> + > >>>> + csi2dc->dev = dev; > >>>> + > >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>>> + > >>>> + csi2dc->base = devm_ioremap_resource(dev, res); > >>> > >>> Should devm_platform_ioremap_resource(pdev, 0) be used here ? > >>> > >>>> + 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; > >>>> + } > >>>> + > >>>> + 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); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + 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"); > >>>> + > >>>> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > >>>> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > >>>> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev); > >>> > >>> Not used it seems > >>> > >>>> + > >>>> + platform_set_drvdata(pdev, csi2dc); > >>>> + > >>>> + ret = csi2dc_of_parse(csi2dc, dev->of_node); > >>>> + if (ret) > >>>> + goto csi2dc_probe_cleanup_entity; > >>>> + > >>>> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > >>>> + if (csi2dc->video_pipe) > >>>> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; > >>>> + > >>>> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, > >>>> + csi2dc->video_pipe ? CSI2DC_PADS_NUM : 1, > >>>> + csi2dc->pads); > >>>> + if (ret < 0) { > >>>> + dev_err(dev, "media entity init failed\n"); > >>>> + goto csi2dc_probe_cleanup_entity; > >>> > >>> Should you also clean up the notifier in the error path ? > >>> > >>>> + } > >>>> + > >>>> + /* turn power on to validate capabilities */ > >>>> + ret = csi2dc_power(csi2dc, true); > >>>> + if (ret < 0) > >>>> + goto csi2dc_probe_cleanup_entity; > >>>> + > >>>> + pm_runtime_set_active(dev); > >>>> + pm_runtime_enable(dev); > >>>> + ver = csi2dc_readl(csi2dc, CSI2DC_VERSION); > >>>> + pm_request_idle(dev); > >>>> + > >>>> + /* > >>>> + * we must register the subdev after PM runtime has been requested, > >>>> + * otherwise we might bound immediately and request pm_runtime_resume > >>>> + * before runtime_enable. > >>>> + */ > >>>> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd); > >>>> + if (ret) { > >>>> + dev_err(csi2dc->dev, "failed to register the subdevice\n"); > >>>> + goto csi2dc_probe_cleanup_entity; > >>>> + } > >>>> + > >>>> + dev_info(dev, "Microchip CSI2DC version %x\n", ver); > >>>> + > >>>> + return 0; > >>>> + > >>>> +csi2dc_probe_cleanup_entity: > >>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static int csi2dc_remove(struct platform_device *pdev) > >>>> +{ > >>>> + struct csi2dc_device *csi2dc = platform_get_drvdata(pdev); > >>>> + > >>>> + pm_runtime_disable(&pdev->dev); > >>>> + > >>>> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd); > >>>> + csi2dc_cleanup_notifier(csi2dc); > >>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev) > >>>> +{ > >>>> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); > >>>> + > >>>> + return csi2dc_power(csi2dc, false); > >>>> +} > >>>> + > >>>> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev) > >>>> +{ > >>>> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); > >>>> + > >>>> + return csi2dc_power(csi2dc, true); > >>>> +} > >>>> + > >>>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = { > >>>> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL) > >>>> +}; > >>>> + > >>>> +static const struct of_device_id csi2dc_of_match[] = { > >>>> + { .compatible = "microchip,sama7g5-csi2dc" }, > >>>> + { } > >>>> +}; > >>>> + > >>>> +MODULE_DEVICE_TABLE(of, csi2dc_of_match); > >>>> + > >>>> +static struct platform_driver csi2dc_driver = { > >>>> + .probe = csi2dc_probe, > >>>> + .remove = csi2dc_remove, > >>>> + .driver = { > >>>> + .name = "microchip-csi2dc", > >>>> + .pm = &csi2dc_dev_pm_ops, > >>>> + .of_match_table = of_match_ptr(csi2dc_of_match), > >>>> + }, > >>>> +}; > >>>> + > >>>> +module_platform_driver(csi2dc_driver); > >>>> + > >>>> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>"); > >>>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver"); > >>>> +MODULE_LICENSE("GPL v2"); > >>>> -- > >>>> 2.25.1 > >>>> > >> >
On 11/3/21 12:59 PM, Jacopo Mondi wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Eugen > > On Wed, Nov 03, 2021 at 09:59:35AM +0000, Eugen.Hristev@microchip.com wrote: >> On 11/3/21 11:28 AM, Jacopo Mondi wrote: >>> Hi Eugen >>> >>> On Wed, Nov 03, 2021 at 08:31:36AM +0000, Eugen.Hristev@microchip.com wrote: >>>> On 11/2/21 7:22 PM, Jacopo Mondi wrote: >>>>> Hi Eugen, >>>>> >>>> >>>> Hi, >>>> >>>> Thank you for your review. I will try to understand everything you said >>>> and come up with a new version. I still have some inline questions >>>> though, about some things which are still unclear. >>>> >>>>> On Fri, Oct 22, 2021 at 10:52:29AM +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 this revision: >>>>>> - addressed comments by Jacopo and Laurent as in this thread: >>>>>> https://www.spinics.net/lists/linux-media/msg181044.html >>>>>> >>>>>> Previous change log : >>>>>> Changes in v5: >>>>>> - only in bindings >>>>>> >>>>>> Changes in v4: >>>>>> - now using get_mbus_config ops to get data from the subdevice, like the >>>>>> virtual channel id, and the clock type. >>>>>> - now having possibility to select any of the RAW10 data modes >>>>>> - at completion time, select which formats are also available in the subdevice, >>>>>> and move to the dynamic list accordingly >>>>>> - changed the pipeline integration, do not advertise subdev ready at probe time. >>>>>> wait until completion is done, and then start a workqueue that will register >>>>>> this device as a subdevice for the next element in pipeline. >>>>>> - moved the s_power code into a different function called now csi2dc_power >>>>>> that is called with CONFIG_PM functions. This is also called at completion, >>>>>> to have the device ready in case CONFIG_PM is not selected on the platform. >>>>>> - merged try_fmt into set_fmt >>>>>> - driver cleanup, wrapped lines over 80 characters >>>>>> >>>>>> Changes in v2: >>>>>> - moved driver to platform/atmel >>>>>> - fixed minor things as per Sakari's review >>>>>> - still some things from v2 review are not yet addressed, to be followed up >>>>>> >>>>>> >>>>>> drivers/media/platform/atmel/Kconfig | 15 + >>>>>> drivers/media/platform/atmel/Makefile | 1 + >>>>>> .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++ >>>>>> 3 files changed, 716 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 dda2f27da317..f83bee373d82 100644 >>>>>> --- a/drivers/media/platform/atmel/Kconfig >>>>>> +++ b/drivers/media/platform/atmel/Kconfig >>>>>> @@ -40,3 +40,18 @@ 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. >>>>>> + >>>>>> + To compile this driver as a module, choose M here: the >>>>>> + module will be called microchip-csi2dc. >>>>>> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile >>>>>> index 46d264ab7948..39f0a7eba702 100644 >>>>>> --- a/drivers/media/platform/atmel/Makefile >>>>>> +++ b/drivers/media/platform/atmel/Makefile >>>>>> @@ -6,3 +6,4 @@ obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o >>>>>> obj-$(CONFIG_VIDEO_ATMEL_ISC_BASE) += atmel-isc-base.o >>>>>> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o >>>>>> obj-$(CONFIG_VIDEO_ATMEL_XISC) += atmel-xisc.o >>>>>> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o >>>>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c >>>>>> new file mode 100644 >>>>>> index 000000000000..277b86988eee >>>>>> --- /dev/null >>>>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c >>>>>> @@ -0,0 +1,700 @@ >>>>>> +// SPDX-License-Identifier: GPL-2.0-only >>>>>> +/* >>>>>> + * Microchip CSI2 Demux Controller (CSI2DC) driver >>>>>> + * >>>>>> + * Copyright (C) 2018 Microchip Technology, Inc. >>>>>> + * >>>>>> + * Author: Eugen Hristev <eugen.hristev@microchip.com> >>>>>> + * >>>>>> + */ >>>>>> + >>>>>> +#include <linux/clk.h> >>>>>> +#include <linux/module.h> >>>>>> +#include <linux/of.h> >>>>> >>>>> Isn't linux/mod_devicetable.h enough ? >>>>> >>>>>> +#include <linux/of_graph.h> >>>>> >>>>> You should probably move to use the fwnode_graph framwork instead of >>>>> of_graph. This driver depends on OF so it shouldn't be an issue but I >>>>> defer this to maintainers >>>>> >>>>>> +#include <linux/platform_device.h> >>>>>> +#include <linux/pm_runtime.h> >>>>>> +#include <linux/videodev2.h> >>>>>> + >>>>>> +#include <media/v4l2-device.h> >>>>> >>>>> Do you need this include ? >>>>> >>>>>> +#include <media/v4l2-fwnode.h> >>>>>> +#include <media/v4l2-subdev.h> >>>>>> +#include <media/videobuf2-dma-contig.h> >>>>> >>>>> Is this one needed as well ? >>>>> >>>>>> + >>>>>> +/* Global configuration register */ >>>>>> +#define CSI2DC_GCFG 0x0 >>>>>> + >>>>>> +/* MIPI sensor pixel clock is free running */ >>>>>> +#define CSI2DC_GCFG_MIPIFRN BIT(0) >>>>>> +/* Output waveform inter-line minimum delay */ >>>>>> +#define CSI2DC_GCFG_HLC(v) ((v) << 4) >>>>>> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4) >>>>>> +/* SAMA7G5 requires a HLC delay of 15 */ >>>>>> +#define SAMA7G5_HLC (15) >>>>>> + >>>>>> +/* Global control register */ >>>>>> +#define CSI2DC_GCTLR 0x04 >>>>>> +#define CSI2DC_GCTLR_SWRST BIT(0) >>>>>> + >>>>>> +/* Global status register */ >>>>>> +#define CSI2DC_GS 0x08 >>>>>> + >>>>>> +/* SSP interrupt status register */ >>>>>> +#define CSI2DC_SSPIS 0x28 >>>>>> +/* Pipe update register */ >>>>>> +#define CSI2DC_PU 0xC0 >>>>> >>>>> What about using lowercase for hex values (I know there's not strict >>>>> rule, so keep what you like the most, but most drivers use lowercase >>>>> >>>>>> +/* 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 - CSI2DC format type struct >>>>>> + * @mbus_code: Media bus code for the format >>>>>> + * @dt: Data type constant for this format >>>>>> + */ >>>>>> +struct csi2dc_format { >>>>>> + u32 mbus_code; >>>>>> + u32 dt; >>>>>> +}; >>>>>> + >>>>>> +static const struct csi2dc_format csi2dc_formats[] = { >>>>>> + { >>>>>> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, >>>>>> + .dt = CSI2DC_DT_RAW10, >>>>>> + }, { >>>>>> + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, >>>>>> + .dt = CSI2DC_DT_RAW10, >>>>>> + }, { >>>>>> + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, >>>>>> + .dt = CSI2DC_DT_RAW10, >>>>>> + }, { >>>>>> + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, >>>>>> + .dt = CSI2DC_DT_RAW10, >>>>>> + }, >>>>>> +}; >>>>> >>>>> How unfortunate we don't have this in the core... >>>>> >>>>>> + >>>>>> +enum mipi_csi_pads { >>>>>> + CSI2DC_PAD_SINK = 0, >>>>>> + CSI2DC_PAD_SOURCE = 1, >>>>>> + CSI2DC_PADS_NUM = 2, >>>>>> +}; >>>>>> + >>>>>> +/* >>>>>> + * struct csi2dc_device - CSI2DC device driver data/config struct >>>>>> + * @base: Register map base address >>>>>> + * @csi2dc_sd: v4l2 subdevice for the csi2dc device >>>>>> + * This is the subdevice that the csi2dc device itself >>>>>> + * registers in v4l2 subsystem >>>>>> + * @dev: struct device for this csi2dc device >>>>>> + * @pclk: Peripheral clock reference >>>>>> + * Input clock that clocks the hardware block internal >>>>>> + * logic >>>>>> + * @scck: Sensor Controller clock reference >>>>>> + * Input clock that is used to generate the pixel clock >>>>>> + * @format: Current saved format used in g/s fmt >>>>>> + * @cur_fmt: Current state format >>>>>> + * @try_fmt: Try format that is being tried >>>>>> + * @pads: Media entity pads for the csi2dc subdevice >>>>>> + * @clk_gated: Whether the clock is gated or free running >>>>>> + * @video_pipe: Whether video pipeline is configured >>>>>> + * @vc: Current set virtual channel >>>>>> + * @asd: Async subdevice for async bound of the underlying subdev >>>>>> + * @notifier: Async notifier that is used to bound the underlying >>>>>> + * subdevice to the csi2dc subdevice >>>>>> + * @input_sd: Reference to the underlying subdevice bound to the >>>>>> + * csi2dc subdevice >>>>>> + * @remote_pad: Pad number of the underlying subdevice that is linked >>>>>> + * to the csi2dc subdevice sink pad. >>>>>> + */ >>>>>> +struct csi2dc_device { >>>>>> + void __iomem *base; >>>>>> + struct v4l2_subdev csi2dc_sd; >>>>>> + struct device *dev; >>>>>> + struct clk *pclk; >>>>>> + struct clk *scck; >>>>>> + >>>>>> + struct v4l2_mbus_framefmt format; >>>>>> + >>>>>> + const struct csi2dc_format *cur_fmt; >>>>>> + const struct csi2dc_format *try_fmt; >>>>>> + >>>>>> + struct media_pad pads[CSI2DC_PADS_NUM]; >>>>>> + >>>>>> + bool clk_gated; >>>>>> + bool video_pipe; >>>>>> + u32 vc; >>>>>> + >>>>>> + struct v4l2_async_subdev *asd; >>>>>> + struct v4l2_async_notifier notifier; >>>>>> + >>>>>> + struct v4l2_subdev *input_sd; >>>>>> + >>>>>> + u32 remote_pad; >>>>>> +}; >>>>>> + >>>>>> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc) >>>>> >>>>> Could you move this below closer to the only caller ? >>>>> >>>>>> +{ >>>>>> + u32 vp; >>>>>> + >>>>>> + if (!csi2dc->cur_fmt) { >>>>> >>>>> You should probably initialize this to a default format >>>>> >>>>>> + dev_err(csi2dc->dev, "format must be configured first\n"); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + if (!csi2dc->video_pipe) { >>>>> >>>>> This is only called internally by the driver at s_stream() time, can >>>>> this happen ? Or rather won't you have a streamon call also when the >>>>> data pipe only is available ? In that case you would error out here >>>> >>>> This can happen if the source node is not found in the DT. In that case, >>>> there is no video pipe available, thus this whole function should be a >>>> no-op (csi2dc_vp_update) . If no vp, there is no vp update. >>>> Normall without a vp, there should be a dp (data pipe), but I haven't >>>> tested this and it's not supported in the driver. >>>> It doesn't make any sense to configure the vp registers below if the vp >>>> is not available. >>>> However, I shouldn't return an error since the vp is not mandatory for >>>> the function of the hardware. Just that the dp will be implemented at a >>>> later time (.... or never). >>>> >>> >>> I see, so the data pipe is still work in progress... I understand >>> bindings should allow for that to be later accepted so you cannot >>> mandate port@1 to be there all the times, but should the driver >>> instead refuse to operate if no port@1 is provided ? >>> >> >> It will not refuse to operate, but won't operate anything basically. >> And if I make it refuse to operate, it will violate the bindings, since >> port@1 is not mandatory > > I don't think that's a problem. I would say the it would also be fine > to have port@1 mandatory as if it becomes optional in future once the > data-pipe is implemented, old DTS will still be compatible. > > However I also think it would be fine for the driver to not support > all the options listed in the bindings. What's importante is that > anything that gets into the bindings does not break > retro-compatibility in case it gets extended. > > To make a counter example, if your bindings define port@1 as optional > but you later make it mandatory for some reason, old dts without > port@1 won't work anymore and that's a problem. In this case if you > make it mandatory and later demote it to be optional, old DTS will all > have port@1 and will work in video-pipe mode while new ones can > optionally swtich to data-pipe by not providing port@1. > > Anyway, not a big deal, I think what you have here is fine but I won't > bother too much about checking for if (!csi2dc->video_pipe) in the > driver as if I understood you correctly the driver cannot currently > be operated in data-pipe mode. > >> >>> Out of curiosity what will the interaction model with the driver be in >>> data pipe mode ? There will be a video device but there won't be an >>> link to it, how is stream start/stop controlled ? >> >> My idea is that on data pipe, the csi2dc should register it's own video >> device and act basically as a dma engine entity that will use an >> external DMA controller to just write data to DRAM. >> >> It is useful if we do not want to convert the MIPI/IDI packets into >> pixels, but rather dump the to DRAM and have a piece of software further >> use them. >> >> However, it's not a clear use case about this functionality at this >> moment. One possibility is to be able to obtain stream meta-data from >> the MIPI / IDI interface, like for example synchronization data, to >> synchronize with an audio capture device. >> >>> >>>>> >>>>>> + dev_err(csi2dc->dev, "video pipeline unavailable\n"); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + 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_state *sd_state, >>>>>> + struct v4l2_subdev_mbus_code_enum *code) >>>>>> +{ >>>>>> + if (code->index >= ARRAY_SIZE(csi2dc_formats)) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + code->code = csi2dc_formats[code->index].mbus_code; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int csi2dc_get_fmt(struct v4l2_subdev *csi2dc_sd, >>>>>> + struct v4l2_subdev_state *sd_state, >>>>>> + struct v4l2_subdev_format *format) >>>>>> +{ >>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); >>>>>> + >>>>>> + format->format = csi2dc->format; >>>>>> + >>>>>> + return 0; >>>>> >>>>> You should support try formats by storing the format in the file >>>>> handle state in s_fmt and return it in case which == TRY >>>>> >>>>> Grep for v4l2_subdev_get_try_format() for usage examples >>>> >>>> I did that initially, but could not find any utility for it in my tests. >>> >>> I understand.. Have you run v4l2-compliance on this sudev ? Doesn't it >>> test try formats support ? Hi Jacopo, Coming back to this, I tried to run v4l2-compliance, but, Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) It looks like it;s not attempted. I have another subdev and for that one it works, it tries. Maybe this subdev is configured in a different way (bridge) ? Or why is it that v4l2-compliance does not try to use try formats for it ? (that's one reason why I did not find issues for formats, as it looks g/s fmt is not tested either ) Thanks! Eugen >>> >>>> So I removed it. >>>> I will try to bring it back, but I have no idea how to test that the try >>>> format works fine or not >>>> >>> >>> I think also v4l2-ctl supports try formats. >>> >>> --try-subdev-fmt pad=<pad>,width=<w>,height=<h>,code=<code>,field=<f>,colorspace=<c>, >>> >>> I'm not sure if it allows you to read it back, but it's easy to hack >>> it out just to verify it. >>> >> >> Ok, I will try it >> >>>>> >>>>>> +} >>>>>> + >>>>>> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd, >>>>>> + struct v4l2_subdev_state *sd_state, >>>>>> + struct v4l2_subdev_format *req_fmt) >>>>>> +{ >>>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); >>>>>> + const struct csi2dc_format *fmt; >>>>>> + int i; >>>>> >>>>> unsigned >>>>> >>>>>> + >>>>>> + for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { >>>>>> + fmt = &csi2dc_formats[i]; >>>>>> + if (req_fmt->format.code == fmt->mbus_code) >>>>>> + csi2dc->try_fmt = fmt; >>>>> >>>>> Shouldn't you break ? >>>>> >>>>>> + fmt++; >>>>>> + } >>>>> >>>>> And make this a simpler >>>>> >>>>> for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { >>>>> if (req_fmt->format.code == csi2dc_formats[i].mbus_code) >>>>> break; >>>>> } >>>>> >>>>> if (i == ARRAY_SIZE(csi2dc_formats) >>>>> i = 0; >>>>> >>>>>> + >>>>>> + /* in case we could not find the desired format, default to something */ >>>>>> + if (!csi2dc->try_fmt || >>>>>> + req_fmt->format.code != csi2dc->try_fmt->mbus_code) { >>>>>> + csi2dc->try_fmt = &csi2dc_formats[0]; >>>>>> + >>>>>> + dev_dbg(csi2dc->dev, >>>>>> + "CSI2DC unsupported format 0x%x, defaulting to 0x%x\n", >>>>>> + req_fmt->format.code, csi2dc_formats[0].mbus_code); >>>>>> + >>>>>> + req_fmt->format.code = csi2dc_formats[0].mbus_code; >>>>>> + } >>>>>> + >>>>>> + req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB; >>>>>> + req_fmt->format.field = V4L2_FIELD_NONE; >>>>>> + >>>>>> + /* save the format for later requests */ >>>>> >>>>> You should support TRY formats >>>>> >>>>>> + csi2dc->format = req_fmt->format; >>>>>> + >>>>>> + /* if we are just trying, we are done */ >>>>>> + if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) >>>>>> + return 0; >>>>>> + >>>>>> + csi2dc->cur_fmt = csi2dc->try_fmt; >>>>> >>>>> csi2dc->cur_fmt = &csi2dc_format[i]; >>>>> >>>>> So you can drop the try_fmt from the driver structure as it seems to >>>>> be used as a temporary variable here only. >>>>> >>>>>> + >>>>>> + dev_dbg(csi2dc->dev, "new format set: 0x%x\n", req_fmt->format.code); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int csi2dc_power(struct csi2dc_device *csi2dc, int on) >>>>>> +{ >>>>>> + int ret = 0; >>>>>> + >>>>>> + if (on) { >>>>>> + ret = clk_prepare_enable(csi2dc->pclk); >>>>>> + if (ret) { >>>>>> + dev_err(csi2dc->dev, "failed to enable pclk: %d\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + ret = clk_prepare_enable(csi2dc->scck); >>>>>> + if (ret) >>>>>> + dev_err(csi2dc->dev, >>>>>> + "failed to enable scck: %d\n", ret); >>>>> >>>>> Shouldn't you bail out here ? >>>> >>>> Initially this clock was not mandatory, but you are right, I should bail >>>> out. >>>> >>>>> >>>>>> + >>>>>> + /* if powering up, deassert reset line */ >>>>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST); >>>>>> + } else { >>>>>> + clk_disable_unprepare(csi2dc->scck); >>>>>> + >>>>>> + /* if powering down, assert reset line */ >>>>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST); >>>>> >>>>> Isn't reverse order of activation better ? >>>>> >>>>> csi2dc_writel(..) >>>>> clk_disable_unprepare(..) >>>>> clk_disable_unprepare(..) >>>>>> + >>>>>> + clk_disable_unprepare(csi2dc->pclk); >>>>>> + } >>>>>> + >>>>>> + 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); >>>>>> + int ret; >>>>>> + >>>>>> + if (enable) { >>>>>> + ret = pm_runtime_resume_and_get(csi2dc->dev); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + csi2dc_vp_update(csi2dc); >>>>>> + } else { >>>>>> + pm_runtime_put_sync(csi2dc->dev); >>>>>> + } >>>>>> + >>>>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable); >>>>> >>>>> Should the remote subdev be started before and stopped after ? >>>> >>>> Do you mean s_power ? >>>> >>> >>> Nope, I meant the operation order. >>> >>> if (enable) { >>> v4l2_subdev_call(...); >>> >>> pm_runtime_resume_and_get(); >>> csi2dc_vp_update(); >>> } else { >>> pm_runtime_put_sync(); >>> >>> v4l2_subdev_call(); >>> } >>> >>> However, my comment is clearly wrong, it should have been the >>> contrary. Sorry about this >>> >>> It's right to configure the interface and power if up -before- >>> starting the remote subdev, but I would stop it before powering it >>> down. Like in: >>> >>> if (enable) { >>> pm_runtime_resume_and_get(); >>> csi2dc_vp_update(); >>> >>> v4l2_subdev_call(1); >>> } else { >>> v4l2_subdev_call(0); >>> >>> pm_runtime_put_sync(); >>> } >>> >>> What do you think ? Stopping the remote before powering the interface >>> down ensures no data is being put on the bus while the interace is >>> powered off. >>> >> >> In this case it might not work like this. If I start the sensor >> streaming before the vp update , then, there will be frames received on >> video pipe update, which means that the csi2dc will not detect the phy >> STOP state reached, and it will fail initialization. >> I believe the csi2dc has to be started before the sensor's first frame >> such that the interface is prepared for receiving. The PHY must be in >> stop state for a certain amount of time. >> I will try to see if it works the other way around, to double check to >> make sure. >> But if it doesn't , then the VP must be configured while the whole >> pipeline is in stop state. >> > > I agree, as I've said my first comment was wrong and the sensor has to > be started after the csi2dc. However in my last comment I was > suggesting to stop it before the csi2dc to avoid having data being > sent on the bus while the csi2dc gets powered down. > >>>>> >>>>>> +} >>>>>> + >>>>>> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = { >>>>>> + .enum_mbus_code = csi2dc_enum_mbus_code, >>>>>> + .set_fmt = csi2dc_set_fmt, >>>>>> + .get_fmt = csi2dc_get_fmt, >>>>>> +}; >>>>>> + >>>>>> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = { >>>>>> + .s_stream = csi2dc_s_stream, >>>>>> +}; >>>>>> + >>>>>> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = { >>>>>> + .pad = &csi2dc_pad_ops, >>>>>> + .video = &csi2dc_video_ops, >>>>>> +}; >>>>>> + >>>>>> +static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) >>>>>> +{ >>>>>> + struct v4l2_mbus_config mbus_config = { 0 }; >>>>>> + int ret; >>>>>> + >>>>>> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, get_mbus_config, >>>>>> + csi2dc->remote_pad, &mbus_config); >>>>>> + if (ret == -ENOIOCTLCMD) { >>>>>> + dev_dbg(csi2dc->dev, >>>>>> + "no remote mbus configuration available\n"); >>>>>> + goto csi2dc_get_mbus_config_defaults; >>>>>> + } >>>>>> + >>>>>> + if (ret) { >>>>>> + dev_err(csi2dc->dev, >>>>>> + "failed to get remote mbus configuration\n"); >>>>>> + goto csi2dc_get_mbus_config_defaults; >>>>>> + } >>>>>> + >>>>>> + if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) >>>>>> + csi2dc->vc = 0; >>>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) >>>>>> + csi2dc->vc = 1; >>>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) >>>>>> + csi2dc->vc = 2; >>>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) >>>>>> + csi2dc->vc = 3; >>>>>> + >>>>>> + dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); >>>>>> + >>>>>> + csi2dc->clk_gated = mbus_config.flags & >>>>>> + V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK; >>>>> >>>>> This should come from the default clock-noncontinuous property in the >>>>> endpoint. It is available in the mbus_configuration only to support >>>>> subdevs that can change it at runtime, and if that's the case it's ok, >>>>> but I think it should be in the endpoint. >>>> >>>> I do not completely understand your point. In a previous version which >>>> you reviewed, I had this as a DT property (so in the endpoint ?), and >>>> now I converted it to a get_mbus_config flag get operation. >>> >>> I re-read the conversation and I think you're right. >>> >>>> It's not the right way to do it ? >>>> How should it be specified in the endpoint ? >>>> >>> >>> There's a default property 'clock-noncontinuous' which gets parsed by >>> v4l2_fwnode_endpoint_parse() and reported to drivers through the >>> V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK and V4L2_MBUS_CSI2_CONTINUOUS_CLOCK >>> flags. In your case, as you fetch it at run time it's not required if >>> not to initialize a default according to the property presence in DT. >>> >>> Up to you, what you have here it's fine! >>> >> >> Speaking of this, I have a parallel sensor that does not implement >> get_mbus_config . >> >> I tried to implement this in the sensor driver, to see if the csi2dc >> receives correct information from the sensor subdev (to see if it's >> parallel or serial). And it works fine. >> I sent a patch for this, but Sakari says that the csi2dc can obtain all >> this information from DT, and I guess he is right. What would be the >> correct way to do it then ? > > Good question, and probably Sakari is in a better position than me to > set a direction. > > My take is that get_mbus_config should be used to retrieve from the remote > subdevice configuration parameters that can change at runtime, or to > express properties that cannot be defined in bindings as they're not > strictly media bus configuration parameters (like the CSI-2 VC, as > you're doing here). To give you another example, I used > get_mbus_config to retrieve at run-time the number of CSI-2 data lanes > currently in use, as I have a transmitter that if configured to output > low resolution data needs to reduce the number of data lanes to stay > in the D-PHY defined clock frequency lower bound (I think that's the > use case that lead to resurect get_mbus_config() as a pad operation, > as in the past there was a g_mbus_config() video operation that got > deprecated along with the soc_camera framework). > > The noncontinuous clock option cannot be changed at runtime afaict and > it's indeed an mbus configuration parameter for which a standard > DT-property is defined so it makes sense to have it in DTS. The clear > win is that your receiver driver can operate with sensor drivers not > instrumented with get_mbus_config() as it will get the info from DTS. > > In your case you require get_mbus_config() for VC handling, something > everyone hopes is temporary as long as the in-development multiplexed > stream support has not landed, but if the operation is not implemented > you fallback to a default which is indeed the best option for VC, but > for clock-continuous you could avoid that by using the standard DTS > mechanism and retain compatibility with subdevices that do not > implement get_mbus_config. > > I know in v3 you have been suggested to use get_mbus_config and I'm > sorry if it drove you off-road, but I think the best option here is: > > - get the clock continuous property from DTS > - as you need get_mbus_config() for VC, inspect if the remote > reports an option for clock continuous and use it if that's the case > - In case the subdev does not implement get_mbus_config() or does not > report clock information use the value parsed from DTS instead of an > arbitrary default > > This should allow you to operate with sensor drivers not instrumented > with get_mbus_config() (as long as they send data on the VC you use as > a defaul fallback :) > >> >> Could you comment on this patch please : >> >> https://lore.kernel.org/linux-media/YYJDcIiBXo%2FXlKCX@paasikivi.fi.intel.com/T/#m990b903ed6d56c3c34232c54c251490ad33e92c9 >> > > Afaict all the mbus configuration for a parallel bus can be fully > described in DTS, and for the above reasons of being compatible with > most subdev drivers I would fetch it from there. > >> >> Thanks again for reviewing ! >> > > You're welcome! > >>>>> >>>>> Speaking of remote subdevs, is there any driver available for IDI >>>>> transmitters ? >>>> >>>> I am not aware of any at the moment >>>>> >>>>>> + >>>>>> + dev_dbg(csi2dc->dev, "%s clock\n", >>>>>> + csi2dc->clk_gated ? "gated" : "free running"); >>>>>> + >>>>>> + return 0; >>>>>> + >>>>>> +csi2dc_get_mbus_config_defaults: >>>>>> + csi2dc->vc = 0; /* Virtual ID 0 by default */ >>>>>> + csi2dc->clk_gated = false; /* Free running clock by default */ >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier, >>>>>> + struct v4l2_subdev *subdev, >>>>>> + struct v4l2_async_subdev *asd) >>>>>> +{ >>>>>> + struct csi2dc_device *csi2dc = container_of(notifier, >>>>>> + struct csi2dc_device, notifier); >>>>>> + int pad; >>>>>> + int ret; >>>>>> + >>>>>> + csi2dc->input_sd = subdev; >>>>>> + >>>>>> + pad = media_entity_get_fwnode_pad(&subdev->entity, >>>>> >>>>> You can use 'ret' >>>> >>>> I can, but it is weird to do remote_pad = ret ... >>>> >>>> May bring confusion, don't you agree ? >>>> >>> >>> Up to you, it's a small detail :) >>> >>> Thanks! >>> j >>> >>>>> >>>>>> + asd->match.fwnode, >>>>>> + MEDIA_PAD_FL_SOURCE); >>>>>> + if (pad < 0) { >>>>>> + dev_err(csi2dc->dev, "Failed to find pad for %s\n", >>>>>> + subdev->name); >>>>>> + return pad; >>>>>> + } >>>>>> + >>>>>> + csi2dc->remote_pad = pad; >>>>>> + >>>>>> + csi2dc_get_mbus_config(csi2dc); >>>>> >>>>> Ideally, as this is not fatal, you could move this at s_stream time to >>>>> fetch the most up-to-date configuration >>>>> >>>>>> + >>>>>> + ret = media_create_pad_link(&csi2dc->input_sd->entity, >>>>>> + csi2dc->remote_pad, >>>>>> + &csi2dc->csi2dc_sd.entity, 0, >>>>>> + MEDIA_LNK_FL_ENABLED); >>>>>> + if (ret < 0) { >>>>> >>>>> if (ret) >>>>> >>>>>> + dev_err(csi2dc->dev, >>>>>> + "Failed to create pad link: %s to %s\n", >>>>>> + csi2dc->input_sd->entity.name, >>>>>> + csi2dc->csi2dc_sd.entity.name); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + dev_dbg(csi2dc->dev, "link with %s pad: %d\n", >>>>>> + csi2dc->input_sd->name, csi2dc->remote_pad); >>>>>> + >>>>>> + ret = pm_runtime_resume_and_get(csi2dc->dev); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + csi2dc_writel(csi2dc, CSI2DC_GCFG, >>>>>> + (SAMA7G5_HLC & CSI2DC_GCFG_HLC_MASK) | >>>>>> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN)); >>>>>> + >>>>>> + csi2dc_writel(csi2dc, CSI2DC_VPCOL, >>>>>> + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK); >>>>>> + csi2dc_writel(csi2dc, CSI2DC_VPROW, >>>>>> + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK); >>>>>> + >>>>>> + pm_runtime_put_sync(csi2dc->dev); >>>>> >>>>> I would really move access to the HW to s_stream time if possible >>>>> >>>>>> + >>>>>> + return ret; >>>>>> +} >>>>>> + >>>>>> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = { >>>>>> + .bound = csi2dc_async_bound, >>>>>> +}; >>>>>> + >>>>>> +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_node) >>>>>> +{ >>>>>> + int ret = 0; >>>>>> + >>>>>> + v4l2_async_notifier_init(&csi2dc->notifier); >>>>>> + >>>>>> + csi2dc->asd = v4l2_async_notifier_add_fwnode_remote_subdev >>>>> >>>>> do you need asd in the driver structure ? >>>>> >>>>>> + (&csi2dc->notifier, of_fwnode_handle(input_node), >>>>>> + struct v4l2_async_subdev); >>>>>> + >>>>>> + of_node_put(input_node); >>>>>> + >>>>>> + if (IS_ERR(csi2dc->asd)) { >>>>>> + ret = PTR_ERR(csi2dc->asd); >>>>>> + dev_err(csi2dc->dev, >>>>>> + "failed to add async notifier for node %pOF: %d\n", >>>>>> + input_node, ret); >>>>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + csi2dc->notifier.ops = &csi2dc_async_ops; >>>>>> + >>>>>> + ret = v4l2_async_subdev_notifier_register(&csi2dc->csi2dc_sd, >>>>>> + &csi2dc->notifier); >>>>>> + >>>>>> + if (ret) { >>>>>> + dev_err(csi2dc->dev, "fail to register async notifier: %d\n", >>>>>> + ret); >>>>>> + v4l2_async_notifier_cleanup(&csi2dc->notifier); >>>>>> + } >>>>>> + >>>>>> + return ret; >>>>>> +} >>>>>> + >>>>>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc, >>>>>> + struct device_node *of_node) >>>>>> +{ >>>>>> + struct device_node *input_node, *output_node; >>>>>> + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, >>>>>> + output_endpoint = { 0 }; >>>>>> + int ret; >>>>>> + >>>>>> + output_endpoint.bus_type = V4L2_MBUS_PARALLEL; >>>>>> + >>>>>> + input_node = of_graph_get_next_endpoint(of_node, NULL); >>>>>> + >>>>>> + if (!input_node) { >>>>>> + dev_err(csi2dc->dev, >>>>>> + "missing port node at %pOF, input node is mandatory.\n", >>>>>> + of_node); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node), >>>>>> + &input_endpoint); >>>>>> + >>>>>> + if (ret) { >>>>> of_node_put(input_node); >>>>> >>>>>> + dev_err(csi2dc->dev, "endpoint not defined at %pOF\n", of_node); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + output_node = of_graph_get_next_endpoint(of_node, input_node); >>>>>> + >>>>>> + if (output_node) >>>>>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(output_node), >>>>>> + &output_endpoint); >>>>> >>>>> of_node_put(output_node); >>>>>> + >>>>>> + if (!output_node || ret) { >>>>>> + dev_info(csi2dc->dev, >>>>>> + "missing output node at %pOF, data pipe available only.\n", >>>>>> + of_node); >>>>>> + } else { >>>>>> + csi2dc->video_pipe = true; >>>>>> + >>>>>> + dev_dbg(csi2dc->dev, "block %pOF %d.%d->%d.%d video pipeline\n", >>>>>> + of_node, input_endpoint.base.port, >>>>>> + input_endpoint.base.id, output_endpoint.base.port, >>>>>> + output_endpoint.base.id); >>>>>> + } >>>>>> + >>>>>> + of_node_put(output_node); >>>>> >>>>> Drop this if you put it above >>>>> >>>>>> + of_node_put(input_node); >>>>> >>>>> Should you put input_node before passing it to the function ? >>>>>> + >>>>>> + /* prepare async notifier for subdevice completion */ >>>>>> + return csi2dc_prepare_notifier(csi2dc, input_node); >>>>>> +} >>>>>> + >>>>>> +static int csi2dc_probe(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct device *dev = &pdev->dev; >>>>>> + struct csi2dc_device *csi2dc; >>>>>> + struct resource *res = NULL; >>>>>> + int ret = 0; >>>>>> + u32 ver; >>>>>> + >>>>>> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL); >>>>>> + if (!csi2dc) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + csi2dc->dev = dev; >>>>>> + >>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>> + >>>>>> + csi2dc->base = devm_ioremap_resource(dev, res); >>>>> >>>>> Should devm_platform_ioremap_resource(pdev, 0) be used here ? >>>>> >>>>>> + 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; >>>>>> + } >>>>>> + >>>>>> + 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); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + 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"); >>>>>> + >>>>>> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >>>>>> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; >>>>>> + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev); >>>>> >>>>> Not used it seems >>>>> >>>>>> + >>>>>> + platform_set_drvdata(pdev, csi2dc); >>>>>> + >>>>>> + ret = csi2dc_of_parse(csi2dc, dev->of_node); >>>>>> + if (ret) >>>>>> + goto csi2dc_probe_cleanup_entity; >>>>>> + >>>>>> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK; >>>>>> + if (csi2dc->video_pipe) >>>>>> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; >>>>>> + >>>>>> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, >>>>>> + csi2dc->video_pipe ? CSI2DC_PADS_NUM : 1, >>>>>> + csi2dc->pads); >>>>>> + if (ret < 0) { >>>>>> + dev_err(dev, "media entity init failed\n"); >>>>>> + goto csi2dc_probe_cleanup_entity; >>>>> >>>>> Should you also clean up the notifier in the error path ? >>>>> >>>>>> + } >>>>>> + >>>>>> + /* turn power on to validate capabilities */ >>>>>> + ret = csi2dc_power(csi2dc, true); >>>>>> + if (ret < 0) >>>>>> + goto csi2dc_probe_cleanup_entity; >>>>>> + >>>>>> + pm_runtime_set_active(dev); >>>>>> + pm_runtime_enable(dev); >>>>>> + ver = csi2dc_readl(csi2dc, CSI2DC_VERSION); >>>>>> + pm_request_idle(dev); >>>>>> + >>>>>> + /* >>>>>> + * we must register the subdev after PM runtime has been requested, >>>>>> + * otherwise we might bound immediately and request pm_runtime_resume >>>>>> + * before runtime_enable. >>>>>> + */ >>>>>> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd); >>>>>> + if (ret) { >>>>>> + dev_err(csi2dc->dev, "failed to register the subdevice\n"); >>>>>> + goto csi2dc_probe_cleanup_entity; >>>>>> + } >>>>>> + >>>>>> + dev_info(dev, "Microchip CSI2DC version %x\n", ver); >>>>>> + >>>>>> + return 0; >>>>>> + >>>>>> +csi2dc_probe_cleanup_entity: >>>>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); >>>>>> + >>>>>> + return ret; >>>>>> +} >>>>>> + >>>>>> +static int csi2dc_remove(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct csi2dc_device *csi2dc = platform_get_drvdata(pdev); >>>>>> + >>>>>> + pm_runtime_disable(&pdev->dev); >>>>>> + >>>>>> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd); >>>>>> + csi2dc_cleanup_notifier(csi2dc); >>>>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev) >>>>>> +{ >>>>>> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); >>>>>> + >>>>>> + return csi2dc_power(csi2dc, false); >>>>>> +} >>>>>> + >>>>>> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev) >>>>>> +{ >>>>>> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); >>>>>> + >>>>>> + return csi2dc_power(csi2dc, true); >>>>>> +} >>>>>> + >>>>>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = { >>>>>> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL) >>>>>> +}; >>>>>> + >>>>>> +static const struct of_device_id csi2dc_of_match[] = { >>>>>> + { .compatible = "microchip,sama7g5-csi2dc" }, >>>>>> + { } >>>>>> +}; >>>>>> + >>>>>> +MODULE_DEVICE_TABLE(of, csi2dc_of_match); >>>>>> + >>>>>> +static struct platform_driver csi2dc_driver = { >>>>>> + .probe = csi2dc_probe, >>>>>> + .remove = csi2dc_remove, >>>>>> + .driver = { >>>>>> + .name = "microchip-csi2dc", >>>>>> + .pm = &csi2dc_dev_pm_ops, >>>>>> + .of_match_table = of_match_ptr(csi2dc_of_match), >>>>>> + }, >>>>>> +}; >>>>>> + >>>>>> +module_platform_driver(csi2dc_driver); >>>>>> + >>>>>> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>"); >>>>>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver"); >>>>>> +MODULE_LICENSE("GPL v2"); >>>>>> -- >>>>>> 2.25.1 >>>>>> >>>> >>
diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig index dda2f27da317..f83bee373d82 100644 --- a/drivers/media/platform/atmel/Kconfig +++ b/drivers/media/platform/atmel/Kconfig @@ -40,3 +40,18 @@ 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. + + To compile this driver as a module, choose M here: the + module will be called microchip-csi2dc. diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile index 46d264ab7948..39f0a7eba702 100644 --- a/drivers/media/platform/atmel/Makefile +++ b/drivers/media/platform/atmel/Makefile @@ -6,3 +6,4 @@ obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o obj-$(CONFIG_VIDEO_ATMEL_ISC_BASE) += atmel-isc-base.o obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o obj-$(CONFIG_VIDEO_ATMEL_XISC) += atmel-xisc.o +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c new file mode 100644 index 000000000000..277b86988eee --- /dev/null +++ b/drivers/media/platform/atmel/microchip-csi2dc.c @@ -0,0 +1,700 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Microchip CSI2 Demux Controller (CSI2DC) driver + * + * Copyright (C) 2018 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) +/* SAMA7G5 requires a HLC delay of 15 */ +#define SAMA7G5_HLC (15) + +/* Global control register */ +#define CSI2DC_GCTLR 0x04 +#define CSI2DC_GCTLR_SWRST BIT(0) + +/* Global status register */ +#define CSI2DC_GS 0x08 + +/* SSP interrupt status register */ +#define CSI2DC_SSPIS 0x28 +/* Pipe update register */ +#define CSI2DC_PU 0xC0 +/* Video pipe attributes update */ +#define CSI2DC_PU_VP BIT(0) + +/* Pipe update status register */ +#define CSI2DC_PUS 0xC4 + +/* Video pipeline enable register */ +#define CSI2DC_VPE 0xF8 +#define CSI2DC_VPE_ENABLE BIT(0) + +/* Video pipeline configuration register */ +#define CSI2DC_VPCFG 0xFC +/* Data type */ +#define CSI2DC_VPCFG_DT(v) ((v) << 0) +#define CSI2DC_VPCFG_DT_MASK GENMASK(5, 0) +/* Virtual channel identifier */ +#define CSI2DC_VPCFG_VC(v) ((v) << 6) +#define CSI2DC_VPCFG_VC_MASK GENMASK(7, 6) +/* Decompression enable */ +#define CSI2DC_VPCFG_DE BIT(8) +/* Decoder mode */ +#define CSI2DC_VPCFG_DM(v) ((v) << 9) +#define CSI2DC_VPCFG_DM_DECODER8TO12 0 +/* Decoder predictor 2 selection */ +#define CSI2DC_VPCFG_DP2 BIT(12) +/* Recommended memory storage */ +#define CSI2DC_VPCFG_RMS BIT(13) +/* Post adjustment */ +#define CSI2DC_VPCFG_PA BIT(14) + +/* Video pipeline column register */ +#define CSI2DC_VPCOL 0x100 +/* Column number */ +#define CSI2DC_VPCOL_COL(v) ((v) << 0) +#define CSI2DC_VPCOL_COL_MASK GENMASK(15, 0) + +/* Video pipeline row register */ +#define CSI2DC_VPROW 0x104 +/* Row number */ +#define CSI2DC_VPROW_ROW(v) ((v) << 0) +#define CSI2DC_VPROW_ROW_MASK GENMASK(15, 0) + +/* Version register */ +#define CSI2DC_VERSION 0x1FC + +/* register read/write helpers */ +#define csi2dc_readl(st, reg) readl_relaxed((st)->base + (reg)) +#define csi2dc_writel(st, reg, val) writel_relaxed((val), \ + (st)->base + (reg)) + +/* supported RAW data types */ +#define CSI2DC_DT_RAW6 0x28 +#define CSI2DC_DT_RAW7 0x29 +#define CSI2DC_DT_RAW8 0x2A +#define CSI2DC_DT_RAW10 0x2B +#define CSI2DC_DT_RAW12 0x2C +#define CSI2DC_DT_RAW14 0x2D + +/* + * struct csi2dc_format - CSI2DC format type struct + * @mbus_code: Media bus code for the format + * @dt: Data type constant for this format + */ +struct csi2dc_format { + u32 mbus_code; + u32 dt; +}; + +static const struct csi2dc_format csi2dc_formats[] = { + { + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, + .dt = CSI2DC_DT_RAW10, + }, { + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, + .dt = CSI2DC_DT_RAW10, + }, { + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, + .dt = CSI2DC_DT_RAW10, + }, { + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, + .dt = CSI2DC_DT_RAW10, + }, +}; + +enum mipi_csi_pads { + CSI2DC_PAD_SINK = 0, + CSI2DC_PAD_SOURCE = 1, + CSI2DC_PADS_NUM = 2, +}; + +/* + * struct csi2dc_device - CSI2DC device driver data/config struct + * @base: Register map base address + * @csi2dc_sd: v4l2 subdevice for the csi2dc device + * This is the subdevice that the csi2dc device itself + * registers in v4l2 subsystem + * @dev: struct device for this csi2dc device + * @pclk: Peripheral clock reference + * Input clock that clocks the hardware block internal + * logic + * @scck: Sensor Controller clock reference + * Input clock that is used to generate the pixel clock + * @format: Current saved format used in g/s fmt + * @cur_fmt: Current state format + * @try_fmt: Try format that is being tried + * @pads: Media entity pads for the csi2dc subdevice + * @clk_gated: Whether the clock is gated or free running + * @video_pipe: Whether video pipeline is configured + * @vc: Current set virtual channel + * @asd: Async subdevice for async bound of the underlying subdev + * @notifier: Async notifier that is used to bound the underlying + * subdevice to the csi2dc subdevice + * @input_sd: Reference to the underlying subdevice bound to the + * csi2dc subdevice + * @remote_pad: Pad number of the underlying subdevice that is linked + * to the csi2dc subdevice sink pad. + */ +struct csi2dc_device { + void __iomem *base; + struct v4l2_subdev csi2dc_sd; + struct device *dev; + struct clk *pclk; + struct clk *scck; + + struct v4l2_mbus_framefmt format; + + const struct csi2dc_format *cur_fmt; + const struct csi2dc_format *try_fmt; + + struct media_pad pads[CSI2DC_PADS_NUM]; + + bool clk_gated; + bool video_pipe; + u32 vc; + + struct v4l2_async_subdev *asd; + struct v4l2_async_notifier notifier; + + struct v4l2_subdev *input_sd; + + u32 remote_pad; +}; + +static void csi2dc_vp_update(struct csi2dc_device *csi2dc) +{ + u32 vp; + + if (!csi2dc->cur_fmt) { + dev_err(csi2dc->dev, "format must be configured first\n"); + return; + } + + if (!csi2dc->video_pipe) { + dev_err(csi2dc->dev, "video pipeline unavailable\n"); + return; + } + + 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_state *sd_state, + struct v4l2_subdev_mbus_code_enum *code) +{ + if (code->index >= ARRAY_SIZE(csi2dc_formats)) + return -EINVAL; + + code->code = csi2dc_formats[code->index].mbus_code; + + return 0; +} + +static int csi2dc_get_fmt(struct v4l2_subdev *csi2dc_sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_format *format) +{ + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); + + format->format = csi2dc->format; + + return 0; +} + +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_format *req_fmt) +{ + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd); + const struct csi2dc_format *fmt; + int i; + + for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) { + fmt = &csi2dc_formats[i]; + if (req_fmt->format.code == fmt->mbus_code) + csi2dc->try_fmt = fmt; + fmt++; + } + + /* in case we could not find the desired format, default to something */ + if (!csi2dc->try_fmt || + req_fmt->format.code != csi2dc->try_fmt->mbus_code) { + csi2dc->try_fmt = &csi2dc_formats[0]; + + dev_dbg(csi2dc->dev, + "CSI2DC unsupported format 0x%x, defaulting to 0x%x\n", + req_fmt->format.code, csi2dc_formats[0].mbus_code); + + req_fmt->format.code = csi2dc_formats[0].mbus_code; + } + + req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB; + req_fmt->format.field = V4L2_FIELD_NONE; + + /* save the format for later requests */ + csi2dc->format = req_fmt->format; + + /* if we are just trying, we are done */ + if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) + return 0; + + csi2dc->cur_fmt = csi2dc->try_fmt; + + dev_dbg(csi2dc->dev, "new format set: 0x%x\n", req_fmt->format.code); + + return 0; +} + +static int csi2dc_power(struct csi2dc_device *csi2dc, int on) +{ + int ret = 0; + + if (on) { + ret = clk_prepare_enable(csi2dc->pclk); + if (ret) { + dev_err(csi2dc->dev, "failed to enable pclk: %d\n", ret); + return ret; + } + + ret = clk_prepare_enable(csi2dc->scck); + if (ret) + dev_err(csi2dc->dev, + "failed to enable scck: %d\n", ret); + + /* if powering up, deassert reset line */ + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST); + } else { + clk_disable_unprepare(csi2dc->scck); + + /* if powering down, assert reset line */ + csi2dc_writel(csi2dc, CSI2DC_GCTLR, !CSI2DC_GCTLR_SWRST); + + clk_disable_unprepare(csi2dc->pclk); + } + + 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); + int ret; + + if (enable) { + ret = pm_runtime_resume_and_get(csi2dc->dev); + if (ret < 0) + return ret; + csi2dc_vp_update(csi2dc); + } else { + pm_runtime_put_sync(csi2dc->dev); + } + + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream, enable); +} + +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = { + .enum_mbus_code = csi2dc_enum_mbus_code, + .set_fmt = csi2dc_set_fmt, + .get_fmt = csi2dc_get_fmt, +}; + +static const struct v4l2_subdev_video_ops csi2dc_video_ops = { + .s_stream = csi2dc_s_stream, +}; + +static const struct v4l2_subdev_ops csi2dc_subdev_ops = { + .pad = &csi2dc_pad_ops, + .video = &csi2dc_video_ops, +}; + +static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) +{ + struct v4l2_mbus_config mbus_config = { 0 }; + int ret; + + ret = v4l2_subdev_call(csi2dc->input_sd, pad, get_mbus_config, + csi2dc->remote_pad, &mbus_config); + if (ret == -ENOIOCTLCMD) { + dev_dbg(csi2dc->dev, + "no remote mbus configuration available\n"); + goto csi2dc_get_mbus_config_defaults; + } + + if (ret) { + dev_err(csi2dc->dev, + "failed to get remote mbus configuration\n"); + goto csi2dc_get_mbus_config_defaults; + } + + if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) + csi2dc->vc = 0; + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) + csi2dc->vc = 1; + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) + csi2dc->vc = 2; + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) + csi2dc->vc = 3; + + dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); + + csi2dc->clk_gated = mbus_config.flags & + V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK; + + dev_dbg(csi2dc->dev, "%s clock\n", + csi2dc->clk_gated ? "gated" : "free running"); + + return 0; + +csi2dc_get_mbus_config_defaults: + csi2dc->vc = 0; /* Virtual ID 0 by default */ + csi2dc->clk_gated = false; /* Free running clock by default */ + + return 0; +} + +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier, + struct v4l2_subdev *subdev, + struct v4l2_async_subdev *asd) +{ + struct csi2dc_device *csi2dc = container_of(notifier, + struct csi2dc_device, notifier); + int pad; + int ret; + + csi2dc->input_sd = subdev; + + pad = media_entity_get_fwnode_pad(&subdev->entity, + asd->match.fwnode, + MEDIA_PAD_FL_SOURCE); + if (pad < 0) { + dev_err(csi2dc->dev, "Failed to find pad for %s\n", + subdev->name); + return pad; + } + + csi2dc->remote_pad = pad; + + csi2dc_get_mbus_config(csi2dc); + + ret = media_create_pad_link(&csi2dc->input_sd->entity, + csi2dc->remote_pad, + &csi2dc->csi2dc_sd.entity, 0, + MEDIA_LNK_FL_ENABLED); + if (ret < 0) { + dev_err(csi2dc->dev, + "Failed to create pad link: %s to %s\n", + csi2dc->input_sd->entity.name, + csi2dc->csi2dc_sd.entity.name); + return ret; + } + + dev_dbg(csi2dc->dev, "link with %s pad: %d\n", + csi2dc->input_sd->name, csi2dc->remote_pad); + + ret = pm_runtime_resume_and_get(csi2dc->dev); + if (ret < 0) + return ret; + + csi2dc_writel(csi2dc, CSI2DC_GCFG, + (SAMA7G5_HLC & CSI2DC_GCFG_HLC_MASK) | + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN)); + + csi2dc_writel(csi2dc, CSI2DC_VPCOL, + CSI2DC_VPCOL_COL(0xFFF) & CSI2DC_VPCOL_COL_MASK); + csi2dc_writel(csi2dc, CSI2DC_VPROW, + CSI2DC_VPROW_ROW(0xFFF) & CSI2DC_VPROW_ROW_MASK); + + pm_runtime_put_sync(csi2dc->dev); + + return ret; +} + +static const struct v4l2_async_notifier_operations csi2dc_async_ops = { + .bound = csi2dc_async_bound, +}; + +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_node) +{ + int ret = 0; + + v4l2_async_notifier_init(&csi2dc->notifier); + + csi2dc->asd = v4l2_async_notifier_add_fwnode_remote_subdev + (&csi2dc->notifier, of_fwnode_handle(input_node), + struct v4l2_async_subdev); + + of_node_put(input_node); + + if (IS_ERR(csi2dc->asd)) { + ret = PTR_ERR(csi2dc->asd); + dev_err(csi2dc->dev, + "failed to add async notifier for node %pOF: %d\n", + input_node, ret); + v4l2_async_notifier_cleanup(&csi2dc->notifier); + return ret; + } + + csi2dc->notifier.ops = &csi2dc_async_ops; + + ret = v4l2_async_subdev_notifier_register(&csi2dc->csi2dc_sd, + &csi2dc->notifier); + + if (ret) { + dev_err(csi2dc->dev, "fail to register async notifier: %d\n", + ret); + v4l2_async_notifier_cleanup(&csi2dc->notifier); + } + + return ret; +} + +static int csi2dc_of_parse(struct csi2dc_device *csi2dc, + struct device_node *of_node) +{ + struct device_node *input_node, *output_node; + struct v4l2_fwnode_endpoint input_endpoint = { 0 }, + output_endpoint = { 0 }; + int ret; + + output_endpoint.bus_type = V4L2_MBUS_PARALLEL; + + input_node = of_graph_get_next_endpoint(of_node, NULL); + + if (!input_node) { + dev_err(csi2dc->dev, + "missing port node at %pOF, input node is mandatory.\n", + of_node); + return -EINVAL; + } + + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(input_node), + &input_endpoint); + + if (ret) { + dev_err(csi2dc->dev, "endpoint not defined at %pOF\n", of_node); + return ret; + } + + output_node = of_graph_get_next_endpoint(of_node, input_node); + + if (output_node) + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(output_node), + &output_endpoint); + + if (!output_node || ret) { + dev_info(csi2dc->dev, + "missing output node at %pOF, data pipe available only.\n", + of_node); + } else { + csi2dc->video_pipe = true; + + dev_dbg(csi2dc->dev, "block %pOF %d.%d->%d.%d video pipeline\n", + of_node, input_endpoint.base.port, + input_endpoint.base.id, output_endpoint.base.port, + output_endpoint.base.id); + } + + of_node_put(output_node); + of_node_put(input_node); + + /* prepare async notifier for subdevice completion */ + return csi2dc_prepare_notifier(csi2dc, input_node); +} + +static int csi2dc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct csi2dc_device *csi2dc; + struct resource *res = NULL; + int ret = 0; + u32 ver; + + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL); + if (!csi2dc) + return -ENOMEM; + + csi2dc->dev = dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + + 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; + } + + 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); + return ret; + } + + 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"); + + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; + v4l2_set_subdevdata(&csi2dc->csi2dc_sd, pdev); + + platform_set_drvdata(pdev, csi2dc); + + ret = csi2dc_of_parse(csi2dc, dev->of_node); + if (ret) + goto csi2dc_probe_cleanup_entity; + + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK; + if (csi2dc->video_pipe) + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; + + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity, + csi2dc->video_pipe ? CSI2DC_PADS_NUM : 1, + csi2dc->pads); + if (ret < 0) { + dev_err(dev, "media entity init failed\n"); + goto csi2dc_probe_cleanup_entity; + } + + /* turn power on to validate capabilities */ + ret = csi2dc_power(csi2dc, true); + if (ret < 0) + goto csi2dc_probe_cleanup_entity; + + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + ver = csi2dc_readl(csi2dc, CSI2DC_VERSION); + pm_request_idle(dev); + + /* + * we must register the subdev after PM runtime has been requested, + * otherwise we might bound immediately and request pm_runtime_resume + * before runtime_enable. + */ + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd); + if (ret) { + dev_err(csi2dc->dev, "failed to register the subdevice\n"); + goto csi2dc_probe_cleanup_entity; + } + + dev_info(dev, "Microchip CSI2DC version %x\n", ver); + + return 0; + +csi2dc_probe_cleanup_entity: + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); + + return ret; +} + +static int csi2dc_remove(struct platform_device *pdev) +{ + struct csi2dc_device *csi2dc = platform_get_drvdata(pdev); + + pm_runtime_disable(&pdev->dev); + + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd); + csi2dc_cleanup_notifier(csi2dc); + media_entity_cleanup(&csi2dc->csi2dc_sd.entity); + + return 0; +} + +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev) +{ + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); + + return csi2dc_power(csi2dc, false); +} + +static int __maybe_unused csi2dc_runtime_resume(struct device *dev) +{ + struct csi2dc_device *csi2dc = dev_get_drvdata(dev); + + return csi2dc_power(csi2dc, true); +} + +static const struct dev_pm_ops csi2dc_dev_pm_ops = { + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL) +}; + +static const struct of_device_id csi2dc_of_match[] = { + { .compatible = "microchip,sama7g5-csi2dc" }, + { } +}; + +MODULE_DEVICE_TABLE(of, csi2dc_of_match); + +static struct platform_driver csi2dc_driver = { + .probe = csi2dc_probe, + .remove = csi2dc_remove, + .driver = { + .name = "microchip-csi2dc", + .pm = &csi2dc_dev_pm_ops, + .of_match_table = of_match_ptr(csi2dc_of_match), + }, +}; + +module_platform_driver(csi2dc_driver); + +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>"); +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver"); +MODULE_LICENSE("GPL v2");
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 this revision: - addressed comments by Jacopo and Laurent as in this thread: https://www.spinics.net/lists/linux-media/msg181044.html Previous change log : Changes in v5: - only in bindings Changes in v4: - now using get_mbus_config ops to get data from the subdevice, like the virtual channel id, and the clock type. - now having possibility to select any of the RAW10 data modes - at completion time, select which formats are also available in the subdevice, and move to the dynamic list accordingly - changed the pipeline integration, do not advertise subdev ready at probe time. wait until completion is done, and then start a workqueue that will register this device as a subdevice for the next element in pipeline. - moved the s_power code into a different function called now csi2dc_power that is called with CONFIG_PM functions. This is also called at completion, to have the device ready in case CONFIG_PM is not selected on the platform. - merged try_fmt into set_fmt - driver cleanup, wrapped lines over 80 characters Changes in v2: - moved driver to platform/atmel - fixed minor things as per Sakari's review - still some things from v2 review are not yet addressed, to be followed up drivers/media/platform/atmel/Kconfig | 15 + drivers/media/platform/atmel/Makefile | 1 + .../media/platform/atmel/microchip-csi2dc.c | 700 ++++++++++++++++++ 3 files changed, 716 insertions(+) create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.c