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 ? 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@xxxxxxxxxxxxx>"); > >> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver"); > >> +MODULE_LICENSE("GPL v2"); > >> -- > >> 2.25.1 > >> >
