On 11/3/21 11:28 AM, Jacopo Mondi wrote: > Hi Eugen > > On Wed, Nov 03, 2021 at 08:31:36AM +0000, Eugen.Hristev@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx> >>>> --- >>>> 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@xxxxxxxxxxxxx> >>>> + * >>>> + */ >>>> + >>>> +#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@xxxxxxxxxxxxxxxxxxxxxx/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@xxxxxxxxxxxxx>"); >>>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver"); >>>> +MODULE_LICENSE("GPL v2"); >>>> -- >>>> 2.25.1 >>>> >>
