by Eugen Hristev

[permalink] [raw]

Subject: Re: [PATCH 11/21] media: atmel: atmel-isc-base: implement mbus_code support in enumfmt

On 11/8/21 4:08 PM, Eugen Hristev - M18282 wrote:
> On 11/8/21 1:20 PM, Jacopo Mondi wrote:
>> Hi Eugen
>>
>> On Fri, Nov 05, 2021 at 11:03:25AM +0000, [email protected] wrote:
>>> On 11/5/21 11:51 AM, Jacopo Mondi wrote:
>>>> Hi Eugen
>>>>
>>>> On Fri, Nov 05, 2021 at 10:25:59AM +0100, Jacopo Mondi wrote:
>>>>> Hi Eugen,
>>>>>
>>>>> On Fri, Oct 22, 2021 at 10:52:37AM +0300, Eugen Hristev wrote:
>>>>>> If enumfmt is called with an mbus_code, the enumfmt handler should only
>>>>>> return the formats that are supported for this mbus_code.
>>>>>> To make it more easy to understand the formats, changed the report order
>>>>>> to report first the native formats, and after that the formats that the ISC
>>>>>> can convert to.
>>>>>>
>>>>>> Signed-off-by: Eugen Hristev <[email protected]>
>>>>>
>>>>> Reviewed-by: Jacopo Mondi <[email protected]>
>>>>>
>>>>
>>>> Too soon! Sorry...
>>>>
>>>>> Thanks
>>>>> j
>>>>>
>>>>>> ---
>>>>>> drivers/media/platform/atmel/atmel-isc-base.c | 51 ++++++++++++++++---
>>>>>> 1 file changed, 43 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>> index 2dd2511c7be1..1f7fbe5e4d79 100644
>>>>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>> @@ -499,21 +499,56 @@ static int isc_enum_fmt_vid_cap(struct file *file, void *priv,
>>>>>> u32 index = f->index;
>>>>>> u32 i, supported_index;
>>>>>>
>>>>>> - if (index < isc->controller_formats_size) {
>>>>>> - f->pixelformat = isc->controller_formats[index].fourcc;
>>>>>> - return 0;
>>>>>> + supported_index = 0;
>>>>>> +
>>>
>>> Hi Jacopo,
>>>
>>> This for loop below first iterates through the formats that were
>>> identified as directly supported by the subdevice.
>>> As we are able in the ISC to just dump what the subdevice can stream, we
>>> advertise all these formats here.
>>> (if the userspace requires one specific mbus code, we only advertise the
>>> corresponding format)
>>>
>>>>>> + for (i = 0; i < isc->formats_list_size; i++) {
>>>>>> + if (!isc->formats_list[i].sd_support)
>>>>
>>>>
>>>>>> + continue;
>>>>>> + /*
>>>>>> + * If specific mbus_code is requested, provide only
>>>>>> + * supported formats with this mbus code
>>>>>> + */
>>>>>> + if (f->mbus_code && f->mbus_code !=
>>>>>> + isc->formats_list[i].mbus_code)
>>>>>> + continue;
>>>>>> + if (supported_index == index) {
>>>>>> + f->pixelformat = isc->formats_list[i].fourcc;
>>>>>> + return 0;
>>>>>> + }
>>>>>> + supported_index++;
>>>>>> }
>>>>>>
>>>>>> - index -= isc->controller_formats_size;
>>>>>> + /*
>>>>>> + * If the sensor does not support this mbus_code whatsoever,
>>>>>> + * there is no reason to advertise any of our output formats
>>>>>> + */
>>>>>> + if (supported_index == 0)
>>>>>> + return -EINVAL;
>>>>
>>>> Shouldn't you also return -EINVAL if index > supported_index ?
>>>
>>> No. If we could not find any format that the sensor can use
>>> (supported_index == 0), and that our ISC can also use, then we don't
>>> support anything, thus, return -EINVAL regardless of the index.
>>>
>>>>
>>>> In that case, I'm not gettng what the rest of the function is for ?
>>>>
>>>
>>> This is the case when we identified one supported format for both the
>>> sensor and the ISC (case where supported_index found earlier is >= 1)
>>>
>>>>>> +
>>>>>> + /*
>>>>>> + * If the sensor uses a format that is not raw, then we cannot
>>>>>> + * convert it to any of the formats that we usually can with a
>>>>>> + * RAW sensor. Thus, do not advertise them.
>>>>>> + */
>>>>>> + if (!isc->config.sd_format ||
>>>>>> + !ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code))
>>>>>> + return -EINVAL;
>>>>>>
>>>
>>> Next, if the current format from the subdev is not raw, we are done.
>>
>> Ok, I think here it's were I disconnect.
>>
>> I don't think you should care about the -current- format on the
>> subdev, as once you move to MC the ISC formats should be enumerated
>> regardless of the how the remote subdev is configured. Rather, you
>> should consider if IS_RAW(f->mbus_code) and from there enumerate the
>> output formats you can generate from raw bayer (in addition to the
>> ones that can be produced by dumping the sensor produced formats)
>
> Actually , in some words, this is what I am doing.
> I am following Laurent's advice:
> enumerate the formats supported for a given mbus code.
>
> In consequence, if the mbus code given is a raw mbus code , I can
> advertise all my ISC formats, and if the mbus code is not raw, I don't .
>
> So I am doing what you are saying, namely three cases:
>
> If there is no mbus code given as a parameter to this function, I
> advertise all my formats, raw, non-raw, etc.
>
> If there is raw mbus code given, I advertise this specific raw mbus code
> if the sensor supports it, and the ISC supports it, and in addition, all
> the formats ISC can convert from raw to.
>
> If the mbus code given is not raw, then, I can only advertise this
> specific non-raw format, since there is nothing more I can do with it.
> It wouldn't make much sense to still advertise my rgb,yuv formats, since
> they cannot be used at all, and my later try_validate_formats will bail out
>
>
>>
>>> But, if it's raw, we can use it to convert to a plethora of other
>>> formats. So we have to enumerate them below :
>>>
>>> With the previous checks, I am making sure that the ISC can really
>>> convert to these formats, otherwise they are badly reported
>>>
>>> Hope this makes things more clear, but please ask if something looks strange
>>>
>>
>> I think our disconnection comes from the way the supported formats are
>> defined in ISC and I think their definition could be reworkd to
>> simplify the format selection logic.
>>
>> The driver defines three lists of formats:
>>
>> - isc->controller_formats
>>
>> static const struct isc_format sama7g5_controller_formats[] = {
>> {
>> .fourcc = V4L2_PIX_FMT_ARGB444,
>> },
>> {
>> .fourcc = V4L2_PIX_FMT_ARGB555,
>> },
>> ...
>>
>> };
>>
>> These are listed with the output fourcc only, and if I get
>> try_configure_pipeline() right, they can be generated from any Bayer
>> RAW format. Is this right ?
>>
>> - isc->formats_list
>>
>> static struct isc_format sama7g5_formats_list[] = {
>> {
>> .fourcc = V4L2_PIX_FMT_SBGGR8,
>> .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
>> .pfe_cfg0_bps = ISC_PFE_CFG0_BPS_EIGHT,
>> .cfa_baycfg = ISC_BAY_CFG_BGBG,
>> },
>> {
>> .fourcc = V4L2_PIX_FMT_SGBRG8,
>> .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
>> .pfe_cfg0_bps = ISC_PFE_CFG0_BPS_EIGHT,
>> .cfa_baycfg = ISC_BAY_CFG_GBGB,
>> },
>>
>> ...
>>
>> };
>>
>> These are formats the ISC can output by dumping the sensor output,
>> hence they require a specific format to be output from the sensor
>>
>> - isc->user_formats
>>
>> static int isc_formats_init() {
>>
>> ...
>>
>> fmt = &isc->formats_list[0];
>> for (i = 0, j = 0; i < list_size; i++) {
>> if (fmt->sd_support)
>> isc->user_formats[j++] = fmt;
>> fmt++;
>> }
>>
>> }
>>
>> this list is obtained at runtime by restricting the formats_list to
>> what the sensor can actually output. I think these, if you move to
>> MC should be removed but I'm not 100% sure it is possible with the
>> current implementation of set_fmt().
>>
>> Do you think controller_formats and formats_list should be unified ?
>>
>> If my understanding that all controller_formats can be generated from
>> RAW Bayer formats you could model struct isc_format as
>>
>> struct isc_format {
>> u32 fourcc;
>> bool processed;
>> u32 mbus_codes
>> u32 cfa_baycfg;
>> u32 pfe_cfg0_bps;
>> };
>>
>> and have in example:
>>
>> {
>> .fourcc = V4L2_PIX_FMT_ARGB444,
>> .processed = true,
>> .mbus_codes = nullptr,
>> ....
>> },
>> {
>> .fourcc = V4L2_PIX_FMT_SBGGR8,
>> .processed = false,
>> .mbus_codes = { MEDIA_BUS_FMT_SBGGR8_1X8 }
>> .pfe_cfg0_bps = ISC_PFE_CFG0_BPS_EIGHT,
>> .cfa_baycfg = ISC_BAY_CFG_BGBG,
>> },
>>
>> and when enumerating and configuring formats check that
>>
>> if (isc_format[i].processed &&
>> (f->mbus_code && IS_RAW(f->mbus)code))
>>
>> or
>>
>> if (!isc_format[i].processed &&
>> (f->mbus_code == isc_format[i].mbus_code
>>
>> if you have other restrictions as in example V4L2_PIX_FMT_YUV420
>> requiring a packed YUV format, you can implement more complex
>> validations to match processed formats, like you do in try_validate_formats()
>>
>> Also, and a bit unrelated to enum_fmt, if I got this right
>> at format configuration time you match the ISC format with
>> the sensor format. I think this should be moved to .link_validate() op time.
>>
>> The MC core calls .link_validate when streaming is started on each
>> entity part of a pipeline, and there you could make sure that the ISC output format can be produced using
>> the sensor format (and sizes). This will greatly simplify set_fmt() as
>> there you will have just to make sure the supplied format is in the
>> list of the ISC supported ones and that's it. If userspace fails to
>> configure the pipeline correctly (in example by setting a non RAW
>> Bayer sensor on the format and requesting a RAW Bayer format from ISC,
>> you will fail at s_stream() time by returning an EPIPE.

Hi Jacopo,

I tried to look over the link_validate call.
It looks like this is only called by media_pipeline_start, which most
drivers use in start_streaming() .
However, I need the format validation to be done before that, at
streamon() time, because after streamon() , the vb2 queue will be filled
with buffers, and I need to know exactly which format I will be using.
So, do you think it's fine to keep the format validation at streamon()
time instead of calling media_pipeline_start in start_streaming ?

Currently I am not calling media_pipeline_start at all.
Do you think it's required?

Or maybe I am missing something and it should be done in a different way ?

Thanks !

>>
>> Hope all of this makes a bit of sense :)
>
> About this last part you are telling me: I have to tell you what am I
> doing right now: with this patch series, including this patch, I am
> adding support for mc handling in this driver, but! the driver is still
> completely compatible with 'the old way' like setting fmt-video for
> /dev/video0 and everything is propagated down the pipeline.
>
> I am doing the conversion to mc-only type of driver in multiple steps.
> This series adds the 'new way' while having the 'old way' still there.
> At the moment I am working on another patch on top of this series that
> will basically remove most of my format propagation logic from
> /dev/video0 to the subdevice, and after this patch that is on the way,
> the 'old way' is gone. The sensor will *have to* be configured through
> userspace because ISC will never call set_fmt on it at all.
>
> So the purpose of the patch you are reviewing now is to have the mbus
> code parameter in the enum_fmt_vid_cap in the way the current driver
> handles things.
>
> So if you agree, I will let the other patch speak for itself and rework
> the logic completely .
> I am currently trying to do it at streamon() time like Laurent told me,
> but I can try to have it at validate link as well, to see how it works.
>
> I will add that patch to the series when it's ready and I have a v2 of
> this series as well. So in baby steps, I am working towards the goal. I
> am not sure if this means that you could agree to this patch as-is, or I
> have to integrate it completely into a bigger patch that will also fix
> everything up including the format logic.
>
> Thanks again for your review and ideas
>
> Eugen
>>
>>>>>> + /*
>>>>>> + * Iterate again through the formats that we can convert to.
>>>>>> + * However, to avoid duplicates, skip the formats that
>>>>>> + * the sensor already supports directly
>>>>>> + */
>>>>>> + index -= supported_index;
>>>>>> supported_index = 0;
>>>>>>
>>>>>> - for (i = 0; i < isc->formats_list_size; i++) {
>>>>>> - if (!ISC_IS_FORMAT_RAW(isc->formats_list[i].mbus_code) ||
>>>>>> - !isc->formats_list[i].sd_support)
>>>>>> + for (i = 0; i < isc->controller_formats_size; i++) {
>>>>>> + /* if this format is already supported by sensor, skip it */
>>>>>> + if (find_format_by_fourcc(isc, isc->controller_formats[i].fourcc))
>>>>>> continue;
>>>>>> if (supported_index == index) {
>>>>>> - f->pixelformat = isc->formats_list[i].fourcc;
>>>>>> + f->pixelformat =
>>>>>> + isc->controller_formats[i].fourcc;
>>>>>> return 0;
>>>>>> }
>>>>>> supported_index++;
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>>>
>

2021-11-11 10:05:15

by Eugen Hristev

[permalink] [raw]

Subject: Re: [PATCH 03/21] media: atmel: introduce microchip csi2dc driver

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