[v4,3/5] media: platform: mediatek: isp_30: add mediatek ISP3.0 sensor interface

From: Louis Kuo <louis.kuo@mediatek.com>

This will add the mediatek ISP3.0 seninf (sensor interface) driver found
on several Mediatek SoCs such as the mt8365.

Then seninf module has 4 physical CSI-2 inputs. Depending on the soc they
may not be all connected.

Signed-off-by: Louis Kuo <louis.kuo@mediatek.com>
Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com>
Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Co-developed-by: Julien Stephan <jstephan@baylibre.com>
Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 MAINTAINERS                                   |    1 +
 drivers/media/platform/mediatek/Kconfig       |    1 +
 drivers/media/platform/mediatek/Makefile      |    1 +
 drivers/media/platform/mediatek/isp/Kconfig   |    2 +
 drivers/media/platform/mediatek/isp/Makefile  |    3 +
 .../platform/mediatek/isp/isp_30/Kconfig      |   16 +
 .../platform/mediatek/isp/isp_30/Makefile     |    3 +
 .../mediatek/isp/isp_30/seninf/Makefile       |    5 +
 .../mediatek/isp/isp_30/seninf/mtk_seninf.c   | 1488 +++++++++++++++++
 .../isp/isp_30/seninf/mtk_seninf_reg.h        |  112 ++
 10 files changed, 1632 insertions(+)
 create mode 100644 drivers/media/platform/mediatek/isp/Kconfig
 create mode 100644 drivers/media/platform/mediatek/isp/Makefile
 create mode 100644 drivers/media/platform/mediatek/isp/isp_30/Kconfig
 create mode 100644 drivers/media/platform/mediatek/isp/isp_30/Makefile
 create mode 100644 drivers/media/platform/mediatek/isp/isp_30/seninf/Makefile
 create mode 100644 drivers/media/platform/mediatek/isp/isp_30/seninf/mtk_seninf.c
 create mode 100644 drivers/media/platform/mediatek/isp/isp_30/seninf/mtk_seninf_reg.h

Hi, Julien:

On Wed, 2024-01-10 at 15:14 +0100, Julien Stephan wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  From: Louis Kuo <louis.kuo@mediatek.com>
> 
> This will add the mediatek ISP3.0 seninf (sensor interface) driver
> found
> on several Mediatek SoCs such as the mt8365.
> 
> Then seninf module has 4 physical CSI-2 inputs. Depending on the soc
> they
> may not be all connected.
> 
> Signed-off-by: Louis Kuo <louis.kuo@mediatek.com>
> Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com>
> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Co-developed-by: Julien Stephan <jstephan@baylibre.com>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> ---

[snip]

> +
> +static const struct mtk_seninf_conf seninf_8365_conf = {
> +	.model = "mtk-camsys-3.0",
> +	.nb_inputs = 4,
> +	.nb_muxes = 6,
> +	.nb_outputs = 4,
> +};

I think you should directly define these value as symbols because now
only support one SoC.

#define MODEL     "mtk-camsys-3.0"
#define INPUT_NR  4
#define MUTEX_NR  6
#define OUTPUT_NR 4

Because we don't know which SoC would be upstream later, maybe the next
SoC would be

static const struct mtk_seninf_conf seninf_83xx_conf = {
	.model = "mtk-camsys-3.0",
	.nb_inputs = 4,
	.nb_muxes = 6,
	.nb_outputs = 4,
	.support_xxx = true;
};

then model, nb_inputs, nb_muxes, and nb_outputs has no difference, so
it's not necessary to define them as variable. So define them as
constant now, and when next SoC upstream, then we know which one would
be variable.

Regards,
CK

Il 10/06/24 16:39, Julien Stephan ha scritto:
> Le jeu. 11 janv. 2024 à 13:04, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> a écrit :
>>
>> Il 10/01/24 15:14, Julien Stephan ha scritto:
>>> From: Louis Kuo <louis.kuo@mediatek.com>
>>>
>>> This will add the mediatek ISP3.0 seninf (sensor interface) driver found
>>> on several Mediatek SoCs such as the mt8365.
>>>
>>> Then seninf module has 4 physical CSI-2 inputs. Depending on the soc they
>>> may not be all connected.
>>>
>>> Signed-off-by: Louis Kuo <louis.kuo@mediatek.com>
>>> Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com>
>>> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
>>> Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Co-developed-by: Julien Stephan <jstephan@baylibre.com>
>>> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
>>> ---
>>>    MAINTAINERS                                   |    1 +
>>>    drivers/media/platform/mediatek/Kconfig       |    1 +
>>>    drivers/media/platform/mediatek/Makefile      |    1 +
>>>    drivers/media/platform/mediatek/isp/Kconfig   |    2 +
>>>    drivers/media/platform/mediatek/isp/Makefile  |    3 +
>>>    .../platform/mediatek/isp/isp_30/Kconfig      |   16 +
>>>    .../platform/mediatek/isp/isp_30/Makefile     |    3 +
>>>    .../mediatek/isp/isp_30/seninf/Makefile       |    5 +
>>>    .../mediatek/isp/isp_30/seninf/mtk_seninf.c   | 1488 +++++++++++++++++
>>>    .../isp/isp_30/seninf/mtk_seninf_reg.h        |  112 ++
>>>    10 files changed, 1632 insertions(+)
>>>    create mode 100644 drivers/media/platform/mediatek/isp/Kconfig
>>>    create mode 100644 drivers/media/platform/mediatek/isp/Makefile
>>>    create mode 100644 drivers/media/platform/mediatek/isp/isp_30/Kconfig
>>>    create mode 100644 drivers/media/platform/mediatek/isp/isp_30/Makefile
>>>    create mode 100644 drivers/media/platform/mediatek/isp/isp_30/seninf/Makefile
>>>    create mode 100644 drivers/media/platform/mediatek/isp/isp_30/seninf/mtk_seninf.c
>>>    create mode 100644 drivers/media/platform/mediatek/isp/isp_30/seninf/mtk_seninf_reg.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 3ea2158864e1..52d200d5e36c 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -13618,6 +13618,7 @@ M:    Andy Hsieh <andy.hsieh@mediatek.com>
>>>    S:  Supported
>>>    F:  Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
>>>    F:  Documentation/devicetree/bindings/media/mediatek,mt8365-seninf.yaml
>>> +F:   drivers/media/platform/mediatek/isp/isp_30/seninf/*
>>>
>>>    MEDIATEK SMI DRIVER
>>>    M:  Yong Wu <yong.wu@mediatek.com>
>>> diff --git a/drivers/media/platform/mediatek/Kconfig b/drivers/media/platform/mediatek/Kconfig
>>> index 84104e2cd024..4e0a5a43f35e 100644
>>> --- a/drivers/media/platform/mediatek/Kconfig
>>> +++ b/drivers/media/platform/mediatek/Kconfig
>>> @@ -7,3 +7,4 @@ source "drivers/media/platform/mediatek/mdp/Kconfig"
>>>    source "drivers/media/platform/mediatek/vcodec/Kconfig"
>>>    source "drivers/media/platform/mediatek/vpu/Kconfig"
>>>    source "drivers/media/platform/mediatek/mdp3/Kconfig"
>>> +source "drivers/media/platform/mediatek/isp/Kconfig"
>>> diff --git a/drivers/media/platform/mediatek/Makefile b/drivers/media/platform/mediatek/Makefile
>>> index 38e6ba917fe5..695f05f525a6 100644
>>> --- a/drivers/media/platform/mediatek/Makefile
>>> +++ b/drivers/media/platform/mediatek/Makefile
>>> @@ -4,3 +4,4 @@ obj-y += mdp/
>>>    obj-y += vcodec/
>>>    obj-y += vpu/
>>>    obj-y += mdp3/
>>> +obj-y += isp/
>>> diff --git a/drivers/media/platform/mediatek/isp/Kconfig b/drivers/media/platform/mediatek/isp/Kconfig
>>> new file mode 100644
>>> index 000000000000..708b9a6660d2
>>> --- /dev/null
>>> +++ b/drivers/media/platform/mediatek/isp/Kconfig
>>> @@ -0,0 +1,2 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +source "drivers/media/platform/mediatek/isp/isp_30/Kconfig"
>>> diff --git a/drivers/media/platform/mediatek/isp/Makefile b/drivers/media/platform/mediatek/isp/Makefile
>>> new file mode 100644
>>> index 000000000000..a81ab33d0dd3
>>> --- /dev/null
>>> +++ b/drivers/media/platform/mediatek/isp/Makefile
>>> @@ -0,0 +1,3 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +obj-y += isp_30/
>>> diff --git a/drivers/media/platform/mediatek/isp/isp_30/Kconfig b/drivers/media/platform/mediatek/isp/isp_30/Kconfig
>>> new file mode 100644
>>> index 000000000000..9791312589fb
>>> --- /dev/null
>>> +++ b/drivers/media/platform/mediatek/isp/isp_30/Kconfig
>>> @@ -0,0 +1,16 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +config MTK_SENINF30
>>> +     tristate "MediaTek ISP3.0 SENINF driver"
>>> +     depends on VIDEO_V4L2_SUBDEV_API
>>> +     depends on MEDIA_CAMERA_SUPPORT
>>> +     depends on ARCH_MEDIATEK || COMPILE_TEST
>>> +     depends on OF
>>> +     select V4L2_FWNODE
>>> +     default n
>>> +     help
>>> +       This driver provides a MIPI CSI-2 receiver interface to connect
>>> +       an external camera module with MediaTek ISP3.0. It is able to handle
>>> +       multiple cameras at the same time.
>>> +
>>> +       To compile this driver as a module, choose M here: the
>>> +       module will be called mtk-seninf.
>>> diff --git a/drivers/media/platform/mediatek/isp/isp_30/Makefile b/drivers/media/platform/mediatek/isp/isp_30/Makefile
>>> new file mode 100644
>>> index 000000000000..ac3142de4739
>>> --- /dev/null
>>> +++ b/drivers/media/platform/mediatek/isp/isp_30/Makefile
>>> @@ -0,0 +1,3 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +
>>> +obj-$(CONFIG_MTK_SENINF30) += seninf/
>>> diff --git a/drivers/media/platform/mediatek/isp/isp_30/seninf/Makefile b/drivers/media/platform/mediatek/isp/isp_30/seninf/Makefile
>>> new file mode 100644
>>> index 000000000000..f28480d6d6c3
>>> --- /dev/null
>>> +++ b/drivers/media/platform/mediatek/isp/isp_30/seninf/Makefile
>>> @@ -0,0 +1,5 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +
>>> +mtk-seninf-objs += mtk_seninf.o
>>> +
>>> +obj-$(CONFIG_MTK_SENINF30) += mtk-seninf.o
>>> diff --git a/drivers/media/platform/mediatek/isp/isp_30/seninf/mtk_seninf.c b/drivers/media/platform/mediatek/isp/isp_30/seninf/mtk_seninf.c
>>> new file mode 100644
>>> index 000000000000..67b2c697d9ca
>>> --- /dev/null
>>> +++ b/drivers/media/platform/mediatek/isp/isp_30/seninf/mtk_seninf.c
>>> @@ -0,0 +1,1488 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2022 MediaTek Inc.
>>> + */
>>> +#include <linux/bitfield.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_graph.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/phy/phy.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/videodev2.h>
>>> +#include <media/media-device.h>
>>> +#include <media/media-entity.h>
>>> +#include <media/v4l2-async.h>
>>> +#include <media/v4l2-common.h>
>>> +#include <media/v4l2-ctrls.h>
>>> +#include <media/v4l2-dev.h>
>>> +#include <media/v4l2-device.h>
>>> +#include <media/v4l2-event.h>
>>> +#include <media/v4l2-fwnode.h>
>>> +#include <media/v4l2-mc.h>
>>> +#include <media/v4l2-subdev.h>
>>> +
>>> +#include "mtk_seninf_reg.h"
>>> +
>>> +#define SENINF_TIMESTAMP_STEP                0x67
>>> +#define SENINF_SETTLE_DELAY          0x15
>>> +#define SENINF_HS_TRAIL_PARAMETER    0x8
>>> +
>>> +#define SENINF_MAX_NUM_INPUTS                4
>>> +#define SENINF_MAX_NUM_OUTPUTS               6
>>> +#define SENINF_MAX_NUM_MUXES         6
>>> +#define SENINF_MAX_NUM_PADS          (SENINF_MAX_NUM_INPUTS + \
>>> +                                      SENINF_MAX_NUM_OUTPUTS)
>>> +
>>> +#define SENINF_DEFAULT_BUS_FMT               MEDIA_BUS_FMT_SGRBG10_1X10
>>> +#define SENINF_DEFAULT_WIDTH         1920
>>> +#define SENINF_DEFAULT_HEIGHT                1080
>>> +
>>> +#define SENINF_PAD_10BIT             0
>>> +
>>> +#define SENINF_TEST_MODEL            0
>>> +#define SENINF_NORMAL_MODEL          1
>>> +#define SENINF_ALL_ERR_IRQ_EN                0x7f
>>> +#define SENINF_IRQ_CLR_SEL           0x80000000
>>> +
>>> +#define SENINF_MIPI_SENSOR           0x8
>>> +
>>> +#define MTK_CSI_MAX_LANES 4
>>> +
>>> +/* Port number in the device tree. */
>>> +enum mtk_seninf_port {
>>> +     CSI_PORT_0 = 0, /* 4D1C or 2D1C */
>>> +     CSI_PORT_1,     /* 4D1C */
>>> +     CSI_PORT_2,     /* 4D1C */
>>> +     CSI_PORT_0B,    /* 2D1C */
>>> +};
>>> +
>>> +enum mtk_seninf_id {
>>> +     SENINF_1 = 0,
>>> +     SENINF_2 = 1,
>>> +     SENINF_3 = 2,
>>> +     SENINF_5 = 4,
>>> +};
>>> +
>>> +static const u32 port_to_seninf_id[] = {
>>> +     [CSI_PORT_0] = SENINF_1,
>>> +     [CSI_PORT_1] = SENINF_3,
>>> +     [CSI_PORT_2] = SENINF_5,
>>> +     [CSI_PORT_0B] = SENINF_2,
>>> +};
>>> +
>>> +enum mtk_seninf_phy_mode {
>>> +     SENINF_PHY_MODE_NONE,
>>> +     SENINF_PHY_MODE_4D1C,
>>> +     SENINF_PHY_MODE_2D1C,
>>> +};
>>> +
>>> +enum mtk_seninf_format_flag {
>>> +     MTK_SENINF_FORMAT_BAYER = BIT(0),
>>> +     MTK_SENINF_FORMAT_DPCM = BIT(1),
>>> +     MTK_SENINF_FORMAT_JPEG = BIT(2),
>>> +     MTK_SENINF_FORMAT_INPUT_ONLY = BIT(3),
>>> +};
>>> +
>>> +/**
>>> + * struct mtk_seninf_conf - Model-specific SENINF parameters
>>> + * @model: Model description
>>> + * @nb_inputs: Number of SENINF inputs
>>> + * @nb_muxes: Number of SENINF MUX (FIFO) instances
>>> + * @nb_outputs: Number of outputs (to CAM and CAMSV instances)
>>> + */
>>> +struct mtk_seninf_conf {
>>> +     const char *model;
>>> +     u8 nb_inputs;
>>> +     u8 nb_muxes;
>>> +     u8 nb_outputs;
>>> +};
>>> +
>>> +/**
>>> + * struct mtk_seninf_format_info - Information about media bus formats
>>> + * @code: V4L2 media bus code
>>> + * @flags: Flags describing the format, as a combination of MTK_SENINF_FORMAT_*
>>> + */
>>> +struct mtk_seninf_format_info {
>>> +     u32 code;
>>> +     u32 flags;
>>> +};
>>> +
>>> +/**
>>> + * struct mtk_seninf_input - SENINF input block
>>> + * @pad: DT port and media entity pad number
>>> + * @seninf_id: SENINF hardware instance ID
>>> + * @base: Memory mapped I/O based address
>>> + * @seninf: Back pointer to the mtk_seninf
>>> + * @phy: PHY connected to the input
>>> + * @phy_mode: PHY operation mode (NONE when the input is not connected)
>>> + * @bus: CSI-2 bus configuration from DT
>>> + * @source_sd: Source subdev connected to the input
>>> + */
>>> +struct mtk_seninf_input {
>>> +     enum mtk_seninf_port pad;
>>> +     enum mtk_seninf_id seninf_id;
>>> +     void __iomem *base;
>>> +     struct mtk_seninf *seninf;
>>> +
>>> +     struct phy *phy;
>>> +     enum mtk_seninf_phy_mode phy_mode;
>>> +
>>> +     struct v4l2_mbus_config_mipi_csi2 bus;
>>> +
>>> +     struct v4l2_subdev *source_sd;
>>> +};
>>> +
>>> +/**
>>> + * struct mtk_seninf_mux - SENINF MUX channel
>>> + * @pad: DT port and media entity pad number
>>> + * @mux_id: MUX hardware instance ID
>>> + * @base: Memory mapped I/O based address
>>> + * @seninf: Back pointer to the mtk_seninf
>>> + */
>>> +struct mtk_seninf_mux {
>>> +     unsigned int pad;
>>> +     unsigned int mux_id;
>>> +     void __iomem *base;
>>> +     struct mtk_seninf *seninf;
>>> +};
>>> +
>>> +/**
>>> + * struct mtk_seninf - Top-level SENINF device
>>> + * @dev: The (platform) device
>>> + * @phy: PHYs at the SENINF inputs
>>> + * @num_clks: Number of clocks in the clks array
>>> + * @clks: Clocks
>>> + * @base: Memory mapped I/O base address
>>> + * @media_dev: Media controller device
>>> + * @v4l2_dev: V4L2 device
>>> + * @subdev: V4L2 subdevice
>>> + * @pads: Media entity pads
>>> + * @notifier: V4L2 async notifier for source subdevs
>>> + * @ctrl_handler: V4L2 controls handler
>>> + * @source_format: Active format on the source pad
>>> + * @inputs: Array of SENINF inputs
>>> + * @muxes: Array of MUXes
>>> + * @conf: Model-specific SENINF parameters
>>> + * @is_testmode: Whether or not the test pattern generator is enabled
>>> + */
>>> +struct mtk_seninf {
>>> +     struct device *dev;
>>> +     struct phy *phy[5];
>>> +     unsigned int num_clks;
>>> +     struct clk_bulk_data *clks;
>>> +     void __iomem *base;
>>> +
>>> +     struct media_device media_dev;
>>> +     struct v4l2_device v4l2_dev;
>>> +     struct v4l2_subdev subdev;
>>> +     struct media_pad pads[SENINF_MAX_NUM_PADS];
>>> +     struct v4l2_async_notifier notifier;
>>> +     struct v4l2_ctrl_handler ctrl_handler;
>>> +
>>> +     struct mtk_seninf_input inputs[SENINF_MAX_NUM_INPUTS];
>>> +     struct mtk_seninf_mux muxes[SENINF_MAX_NUM_MUXES];
>>> +
>>> +     const struct mtk_seninf_conf *conf;
>>> +
>>> +     bool is_testmode;
>>> +};
>>> +
>>> +inline struct mtk_seninf *sd_to_mtk_seninf(struct v4l2_subdev *sd)
>>> +{
>>> +     return container_of(sd, struct mtk_seninf, subdev);
>>> +}
>>> +
>>> +static inline bool mtk_seninf_pad_is_sink(struct mtk_seninf *priv,
>>> +                                       unsigned int pad)
>>> +{
>>> +     return pad < priv->conf->nb_inputs;
>>> +}
>>> +
>>> +static inline bool mtk_seninf_pad_is_source(struct mtk_seninf *priv,
>>> +                                         unsigned int pad)
>>> +{
>>> +     return !mtk_seninf_pad_is_sink(priv, pad);
>>> +}
>>> +
>>> +/* -----------------------------------------------------------------------------
>>> + * Formats
>>> + */
>>> +
>>> +static const struct mtk_seninf_format_info mtk_seninf_formats[] = {
>>> +     {
>>> +             .code = MEDIA_BUS_FMT_SBGGR8_1X8,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SGBRG8_1X8,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SGRBG8_1X8,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SRGGB8_1X8,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SGRBG10_1X10,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SRGGB10_1X10,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SBGGR10_1X10,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SGBRG10_1X10,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SBGGR12_1X12,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SGBRG12_1X12,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SGRBG12_1X12,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SRGGB12_1X12,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SBGGR14_1X14,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SGBRG14_1X14,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SGRBG14_1X14,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SRGGB14_1X14,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SBGGR16_1X16,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SGBRG16_1X16,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SGRBG16_1X16,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SRGGB16_1X16,
>>> +             .flags = MTK_SENINF_FORMAT_BAYER,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_UYVY8_1X16,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_VYUY8_1X16,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_YUYV8_1X16,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_YVYU8_1X16,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_JPEG_1X8,
>>> +             .flags = MTK_SENINF_FORMAT_JPEG,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_S5C_UYVY_JPEG_1X8,
>>> +             .flags = MTK_SENINF_FORMAT_JPEG,
>>> +     },
>>> +     /* Keep the input-only formats last. */
>>
>> Your comment doesn't make me understand why input-only formats shall be
>> placed last - and makes me think that having two arrays (one for both
>> and one for input only) would be easier and less error prone, other than
>> making you able to drop the MTK_SENINF_FORMAT_INPUT_ONLY flag entirely.
>>
>>> +     {
>>> +             .code = MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8,
>>> +             .flags = MTK_SENINF_FORMAT_DPCM | MTK_SENINF_FORMAT_INPUT_ONLY,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8,
>>> +             .flags = MTK_SENINF_FORMAT_DPCM | MTK_SENINF_FORMAT_INPUT_ONLY,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8,
>>> +             .flags = MTK_SENINF_FORMAT_DPCM | MTK_SENINF_FORMAT_INPUT_ONLY,
>>> +     }, {
>>> +             .code = MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8,
>>> +             .flags = MTK_SENINF_FORMAT_DPCM | MTK_SENINF_FORMAT_INPUT_ONLY,
>>> +     }
>>> +};
>>> +
>>> +static const struct mtk_seninf_format_info *mtk_seninf_format_info(u32 code)
>>> +{
>>> +     unsigned int i;
>>> +
>>> +     for (i = 0; i < ARRAY_SIZE(mtk_seninf_formats); ++i) {
>>> +             if (mtk_seninf_formats[i].code == code)
>>> +                     return &mtk_seninf_formats[i];
>>> +     }
>>> +
>>> +     return NULL;
>>> +}
>>> +
>>
>> ..snip..
>>
>>> +
>>> +static void mtk_seninf_input_setup_csi2(struct mtk_seninf_input *input,
>>> +                                     struct v4l2_subdev_state *state)
>>> +{
>>> +     const struct mtk_seninf_format_info *fmtinfo;
>>> +     const struct v4l2_mbus_framefmt *format;
>>> +     unsigned int num_data_lanes = input->bus.num_data_lanes;
>>> +     unsigned int val = 0;
>>> +
>>> +     format = v4l2_subdev_state_get_stream_format(state, input->pad, 0);
>>> +     fmtinfo = mtk_seninf_format_info(format->code);
>>> +
>>> +     /* Configure timestamp */
>>> +     writel(SENINF_TIMESTAMP_STEP, input->base + SENINF_TG1_TM_STP);
>>> +
>>> +     /* HQ */
>>> +     writel(0x0, input->base + SENINF_TG1_PH_CNT);
>>
>> Zero means:
>>    - Sensor master clock: ISP_CLK
>>    - Sensor clock polarity: Rising edge
>>    - Sensor reset deasserted
>>    - Sensor powered up
>>    - Pixel clock inversion disabled
>>    - Sensor master clock polarity disabled
>>    - Phase counter disabled
>>
>>> +     writel(0x10001, input->base + SENINF_TG1_SEN_CK);
>>
>> Unroll this one... this is the TG1 sensor clock divider.
>>
>> CLKFL GENMASK(5, 0)
>> CLKRS GENMASK(13, 8)
>> CLKCNT GENMASK(21,16)
>>
>> Like this, I don't get what you're trying to set, because you're using a fixed
>> sensor clock rate, meaning that only a handful of camera sensors will be usable.
>>
>> Is this 8Mhz? 16? 24? what? :-)
>>
>> Two hints:
>>    - sensor_clk = clk_get_rate(isp_clk) / (tg1_sen_ck_clkcnt + 1);
>>    - int mtk_seninf_set_sensor_clk(u8 rate_mhz);
>>
>> Please :-)
> 
> Hi Angelo,
> 
> I think I get your point about not hardcoding the sensor rate, but I
> am not sure how to use
> a mtk_seninf_set_sensor_clk(u8 rate_mhz); function.
> 
> Where would it be called? How is it exposed to the user?
> 

As for where: setup, streaming start, resolution change (which may be covered
by streaming start anyway, as a change should be calling stop->start anyway).

And for the how is it exposed to the user - well, depends what you mean for user,
but it's all standard V4L2 API :-)

Last but not least, I can give you another hint....

struct media_entity *entity = (something_here);
struct media_pad *mpad;
struct v4l2_subdev *cam_subdev;
struct v4l2_ctrl *ctl;
s64 link_frequency, pixel_clock;

if (entity->pads[0].flags & MEDIA_PAD_FL_SINK)
    return -E_NOT_A_CAMERA_SENSOR_WE_IGNORE_THIS_ONE;

pad = media_pad_remote_pad_first(&entity->pads[0]);
if (!pad)
   return -ENOENT;

if (!is_media_entity_v4l2_subdev(pad->entity))
   return -ENOENT;

if (pad->entity->function != MEDIA_ENT_F_CAM_SENSOR)
   return -ENOENT;

cam_subdev = media_entity_to_v4l2_subdev(pad->entity);
ctl = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);

/* multiplier is usually bits per pixel, divider is usually num of lanes */
link_frequency = v4l2_get_link_freq(cam_subdev->ctrl_handler, multiplier, divider);
pixel_clock = v4l2_ctrl_g_ctrl_int64(ctl);


....now you know what the sensor wants, set the seninf sensor clock accordingly.

Cheers
Angelo

> Cheers
> Julien
> 
>>
>>> +
>>> +     /* First Enable Sensor interface and select pad (0x1a04_0200) */
>>> +     mtk_seninf_input_update(input, SENINF_CTRL, SENINF_EN, 1);
>>> +     mtk_seninf_input_update(input, SENINF_CTRL, PAD2CAM_DATA_SEL, SENINF_PAD_10BIT);
>>> +     mtk_seninf_input_update(input, SENINF_CTRL, SENINF_SRC_SEL, 0);
>>> +     mtk_seninf_input_update(input, SENINF_CTRL_EXT, SENINF_CSI2_IP_EN, 1);
>>> +     mtk_seninf_input_update(input, SENINF_CTRL_EXT, SENINF_NCSI2_IP_EN, 0);
>>> +
>>> +     /* DPCM Enable */
>>> +     if (fmtinfo->flags & MTK_SENINF_FORMAT_DPCM)
>>> +             val = SENINF_CSI2_DPCM_DI_2A_DPCM_EN;
>>> +     else
>>> +             val = SENINF_CSI2_DPCM_DI_30_DPCM_EN;
>>> +     writel(val, input->base + SENINF_CSI2_DPCM);
>>> +
>>> +     /* Settle delay */
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_LNRD_TIMING,
>>> +                             DATA_SETTLE_PARAMETER, SENINF_SETTLE_DELAY);
>>> +
>>> +     /* HQ */
>>> +     writel(0x10, input->base + SENINF_CSI2_LNRC_FSM);
>>
>> As far as I know, SENINF_CSI2_LNRC_FSM is a read-only register: this write will do
>> exactly nothing...
>>
>>> +
>>> +     /* CSI2 control */
>>> +     val = readl(input->base + SENINF_CSI2_CTL)
>>> +         | (FIELD_PREP(SENINF_CSI2_CTL_ED_SEL, DATA_HEADER_ORDER_DI_WCL_WCH)
>>> +         | SENINF_CSI2_CTL_CLOCK_LANE_EN | (BIT(num_data_lanes) - 1));
>>> +     writel(val, input->base + SENINF_CSI2_CTL);
>>> +
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_RESYNC_MERGE_CTL,
>>> +                             BYPASS_LANE_RESYNC, 0);
>>
>> 93 columns: fits in one line (not only this one!).
>>
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_RESYNC_MERGE_CTL, CDPHY_SEL, 0);
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_RESYNC_MERGE_CTL,
>>> +                             CPHY_LANE_RESYNC_CNT, 3);
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_MODE, CSR_CSI2_MODE, 0);
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_MODE, CSR_CSI2_HEADER_LEN, 0);
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_DPHY_SYNC, SYNC_SEQ_MASK_0, 0xff00);
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_DPHY_SYNC, SYNC_SEQ_PAT_0, 0x001d);
>>> +
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_CTL, CLOCK_HS_OPTION, 0);
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_CTL, HSRX_DET_EN, 0);
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_CTL, HS_TRAIL_EN, 1);
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_HS_TRAIL, HS_TRAIL_PARAMETER,
>>> +                             SENINF_HS_TRAIL_PARAMETER);
>>> +
>>> +     /* Set debug port to output packet number */
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_DGB_SEL, DEBUG_EN, 1);
>>> +     mtk_seninf_input_update(input, SENINF_CSI2_DGB_SEL, DEBUG_SEL, 0x1a);
>>> +
>>> +     /* HQ */
>>> +     writel(0xfffffffe, input->base + SENINF_CSI2_SPARE0);
>>
>> I have no idea what this SPARE0 does, but I think that this is something that you
>> want to get from platform_data, as I guess this would be different on various SoCs?
>>
>>> +
>>> +     /* Enable CSI2 IRQ mask */
>>> +     /* Turn on all interrupt */
>>> +     writel(0xffffffff, input->base + SENINF_CSI2_INT_EN);
>>> +     /* Write clear CSI2 IRQ */
>>> +     writel(0xffffffff, input->base + SENINF_CSI2_INT_STATUS);
>>> +     /* Enable CSI2 Extend IRQ mask */
>>
>> You missed:
>>          writel(0xffffffff, input->base + SENINF_CSI2_INT_EN_EXT);
>>
>> P.S.: #define SENINF_CSI2_INT_EN_EXT 0x0b10
>>
>>
>>> +     /* Turn on all interrupt */
>>
>> /* Reset the CSI2 to commit changes */ <-- makes more sense, doesn't it?
>>
>>> +     mtk_seninf_input_update(input, SENINF_CTRL, CSI2_SW_RST, 1);
>>> +     udelay(1);
>>> +     mtk_seninf_input_update(input, SENINF_CTRL, CSI2_SW_RST, 0);
>>> +}
>>> +
>>> +static void mtk_seninf_mux_setup(struct mtk_seninf_mux *mux,
>>> +                              struct mtk_seninf_input *input,
>>> +                              struct v4l2_subdev_state *state)
>>> +{
>>> +     const struct mtk_seninf_format_info *fmtinfo;
>>> +     const struct v4l2_mbus_framefmt *format;
>>> +     unsigned int pix_sel_ext;
>>> +     unsigned int pix_sel;
>>> +     unsigned int hs_pol = 0;
>>> +     unsigned int vs_pol = 0;
>>> +     unsigned int val;
>>> +     u32 rst_mask;
>>> +
>>> +     format = v4l2_subdev_state_get_stream_format(state, input->pad, 0);
>>> +     fmtinfo = mtk_seninf_format_info(format->code);
>>> +
>>> +     /* Enable mux */
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_MUX_EN, 1);
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_SRC_SEL, SENINF_MIPI_SENSOR);
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL_EXT, SENINF_SRC_SEL_EXT, SENINF_NORMAL_MODEL);
>>> +
>>> +     pix_sel_ext = 0;
>>> +     pix_sel = 1;
>>
>>
>>          pixels_per_cycle = 1;
>>          bus_width = pixels_per_cycle >> 1;
>>
>> because:  0 == 1pix/cyc, 1 == 2pix/cyc, 2 == 4pix/cyc, 3 == 8pix... etc
>> ...but the width of this register depends on the SoC, so you also want to set
>> constraints to the bus width on a per-soc basis (platform data again, or at
>> least leave a comment here).
>>
>>          mtk_seninf_mux_update(....  PIX_SEL_EXT, bus_width);
>>          mtk_seninf_mux_update(....  PIX_SEL, bus_width);
>>
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL_EXT, SENINF_PIX_SEL_EXT, pix_sel_ext);
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_PIX_SEL, pix_sel);
>>> +
>>> +     if (fmtinfo->flags & MTK_SENINF_FORMAT_JPEG) {
>>> +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FULL_WR_EN, 0);
>>> +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FLUSH_EN,
>>> +                                   FIFO_FLUSH_EN_JPEG_2_PIXEL_MODE);
>>> +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_PUSH_EN,
>>> +                             FIFO_PUSH_EN_JPEG_2_PIXEL_MODE);
>>> +     } else {
>>> +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FULL_WR_EN, 2);
>>> +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FLUSH_EN,
>>> +                                  FIFO_FLUSH_EN_NORMAL_MODE);
>>> +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_PUSH_EN,
>>> +                             FIFO_PUSH_EN_NORMAL_MODE);
>>> +     }
>>> +
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_HSYNC_POL, hs_pol);
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_VSYNC_POL, vs_pol);
>>> +
>>> +     val = mtk_seninf_mux_read(mux, SENINF_MUX_CTRL);
>>> +     rst_mask = SENINF_MUX_CTRL_SENINF_IRQ_SW_RST | SENINF_MUX_CTRL_SENINF_MUX_SW_RST;
>>> +
>>> +     mtk_seninf_mux_write(mux, SENINF_MUX_CTRL, val | rst_mask);
>>
>> Are you sure that you don't need any wait between assertion and deassertion of RST?
>> Looks strange, but I don't really know then.
>>
>>> +     mtk_seninf_mux_write(mux, SENINF_MUX_CTRL, val & ~rst_mask);
>>> +
>>> +     /* HQ */
>>> +     mtk_seninf_mux_write(mux, SENINF_MUX_SPARE, 0xc2000);
>>
>> val = SENINF_FIFO_FULL_SEL;
>>
>> /* SPARE field meaning is unknown */
>> val |= 0xc0000;
>>
>>          mtk_seninf_mux_write(mux, SENINF_MUX_SPARE, val);
>>
>>> +}
>>> +
>>> +static void mtk_seninf_top_mux_setup(struct mtk_seninf *priv,
>>> +                                  enum mtk_seninf_id seninf_id,
>>> +                                  struct mtk_seninf_mux *mux)
>>> +{
>>> +     unsigned int val;
>>> +
>>> +     /*
>>> +      * Use the top mux (from SENINF input to MUX) to configure routing, and
>>> +      * hardcode a 1:1 mapping from the MUX instances to the SENINF outputs.
>>> +      */
>>> +     val = readl(priv->base + SENINF_TOP_MUX_CTRL)
>>> +             & ~(0xf << (mux->mux_id * 4));
>>> +     val |= (seninf_id & 0xf) << (mux->mux_id * 4);
>>> +     writel(val, priv->base + SENINF_TOP_MUX_CTRL);
>>> +
>>> +     writel(0x76541010, priv->base + SENINF_TOP_CAM_MUX_CTRL);
>>
>> Each four bits of TOP_CAM_MUX_CTRL selects between seninf1 to seninf8 muxes, and
>> TOP_MUX_CTRL is laid out in the very same way.
>>
>> This means that if you're calculating a value for TOP_MUX_CTRL, you can do exactly
>> the same for TOP_CAM_MUX_CTRL.
>>
>>> +}
>>> +
>>> +static void seninf_enable_test_pattern(struct mtk_seninf *priv,
>>> +                                    struct v4l2_subdev_state *state)
>>> +{
>>> +     struct mtk_seninf_input *input = &priv->inputs[CSI_PORT_0];
>>> +     struct mtk_seninf_mux *mux = &priv->muxes[0];
>>> +     const struct mtk_seninf_format_info *fmtinfo;
>>> +     const struct v4l2_mbus_framefmt *format;
>>> +     unsigned int val;
>>> +     unsigned int pix_sel_ext;
>>> +     unsigned int pix_sel;
>>> +     unsigned int hs_pol = 0;
>>> +     unsigned int vs_pol = 0;
>>> +     unsigned int seninf = 0;
>>> +     unsigned int tm_size = 0;
>>> +     unsigned int mux_id = mux->mux_id;
>>> +
>>> +     format = v4l2_subdev_state_get_stream_format(state, priv->conf->nb_inputs, 0);
>>> +     fmtinfo = mtk_seninf_format_info(format->code);
>>> +
>>> +     mtk_seninf_update(priv, SENINF_TOP_CTRL, MUX_LP_MODE, 0);
>>> +
>>> +     mtk_seninf_update(priv, SENINF_TOP_CTRL, SENINF_PCLK_EN, 1);
>>> +     mtk_seninf_update(priv, SENINF_TOP_CTRL, SENINF2_PCLK_EN, 1);
>>> +
>>> +     mtk_seninf_input_update(input, SENINF_CTRL, SENINF_EN, 1);
>>> +     mtk_seninf_input_update(input, SENINF_CTRL, SENINF_SRC_SEL, 1);
>>> +     mtk_seninf_input_update(input, SENINF_CTRL_EXT,
>>> +                             SENINF_TESTMDL_IP_EN, 1);
>>> +
>>> +     mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_EN, 1);
>>> +     mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_PAT, 0xc);
>>> +     mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_VSYNC, 4);
>>> +     mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_DUMMYPXL, 0x28);
>>> +
>>> +     if (fmtinfo->flags & MTK_SENINF_FORMAT_BAYER)
>>> +             mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_FMT, 0x0);
>>> +     else
>>> +             mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_FMT, 0x1);
>>> +
>>> +     tm_size = FIELD_PREP(SENINF_TG1_TM_SIZE_TM_LINE, format->height + 8);
>>> +     switch (format->code) {
>>> +     case MEDIA_BUS_FMT_UYVY8_1X16:
>>> +     case MEDIA_BUS_FMT_VYUY8_1X16:
>>> +     case MEDIA_BUS_FMT_YUYV8_1X16:
>>> +     case MEDIA_BUS_FMT_YVYU8_1X16:
>>> +             tm_size |= FIELD_PREP(SENINF_TG1_TM_SIZE_TM_PXL, format->width * 2);
>>> +             break;
>>> +     default:
>>> +             tm_size |= FIELD_PREP(SENINF_TG1_TM_SIZE_TM_PXL, format->width);
>>> +             break;
>>> +     }
>>> +     writel(tm_size, input->base + SENINF_TG1_TM_SIZE);
>>> +
>>> +     writel(TEST_MODEL_CLK_DIVIDED_CNT, input->base + SENINF_TG1_TM_CLK);
>>> +     writel(TIME_STAMP_DIVIDER, input->base + SENINF_TG1_TM_STP);
>>> +
>>> +     /* Set top mux */
>>> +     val = (readl(priv->base + SENINF_TOP_MUX_CTRL) & (~(0xf << (mux_id * 4)))) |
>>> +             ((seninf & 0xf) << (mux_id * 4));
>>> +     writel(val, priv->base + SENINF_TOP_MUX_CTRL);
>>
>> This is duplicated, and it is the same that you have in mtk_seninf_top_mux_setup()
>>
>>> +
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_MUX_EN, 1);
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL_EXT,
>>> +                           SENINF_SRC_SEL_EXT, SENINF_TEST_MODEL);
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_SRC_SEL, 1);
>>> +
>>> +     pix_sel_ext = 0;
>>> +     pix_sel = 1;
>>> +
>>
>> This is in mtk_seninf_mux_setup(), but if you apply my suggestion, it won't be in
>> there anymore, so you'll call a function here to set the right value :-)
>>
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL_EXT,
>>> +                           SENINF_PIX_SEL_EXT, pix_sel_ext);
>>> +
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_PIX_SEL, pix_sel);
>>> +
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_PUSH_EN, 0x1f);
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FLUSH_EN, 0x1b);
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FULL_WR_EN, 2);
>>> +
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_HSYNC_POL, hs_pol);
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_VSYNC_POL, vs_pol);
>>> +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_HSYNC_MASK, 1);
>>> +
>>> +     mtk_seninf_mux_write(mux, SENINF_MUX_INTEN,
>>> +                          SENINF_IRQ_CLR_SEL | SENINF_ALL_ERR_IRQ_EN);
>>> +
>>> +     mtk_seninf_mux_write(mux, SENINF_MUX_CTRL,
>>> +                          mtk_seninf_mux_read(mux, SENINF_MUX_CTRL) |
>>> +                          SENINF_MUX_CTRL_SENINF_IRQ_SW_RST |
>>> +                          SENINF_MUX_CTRL_SENINF_MUX_SW_RST);
>>> +     udelay(1);
>>> +     mtk_seninf_mux_write(mux, SENINF_MUX_CTRL,
>>> +                          mtk_seninf_mux_read(mux, SENINF_MUX_CTRL) &
>>> +                          ~(SENINF_MUX_CTRL_SENINF_IRQ_SW_RST |
>>> +                            SENINF_MUX_CTRL_SENINF_MUX_SW_RST));
>>> +
>>> +     //check this
>>> +     writel(0x76540010, priv->base + SENINF_TOP_CAM_MUX_CTRL);
>>> +
>>> +     dev_dbg(priv->dev, "%s: OK\n", __func__);
>>> +}
>>> +
>>
>> Cheers,
>> Angelo
>>

Le mer. 12 juin 2024 à 10:06, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> a écrit :
>
> Il 10/06/24 16:39, Julien Stephan ha scritto:
[...]
> >>
> >>> +     writel(0x10001, input->base + SENINF_TG1_SEN_CK);
> >>
> >> Unroll this one... this is the TG1 sensor clock divider.
> >>
> >> CLKFL GENMASK(5, 0)
> >> CLKRS GENMASK(13, 8)
> >> CLKCNT GENMASK(21,16)
> >>
> >> Like this, I don't get what you're trying to set, because you're using a fixed
> >> sensor clock rate, meaning that only a handful of camera sensors will be usable.
> >>
> >> Is this 8Mhz? 16? 24? what? :-)
> >>
> >> Two hints:
> >>    - sensor_clk = clk_get_rate(isp_clk) / (tg1_sen_ck_clkcnt + 1);
> >>    - int mtk_seninf_set_sensor_clk(u8 rate_mhz);
> >>
> >> Please :-)
> >
> > Hi Angelo,
> >
> > I think I get your point about not hardcoding the sensor rate, but I
> > am not sure how to use
> > a mtk_seninf_set_sensor_clk(u8 rate_mhz); function.
> >
> > Where would it be called? How is it exposed to the user?
> >
>
> As for where: setup, streaming start, resolution change (which may be covered
> by streaming start anyway, as a change should be calling stop->start anyway).
>
> And for the how is it exposed to the user - well, depends what you mean for user,
> but it's all standard V4L2 API :-)
>
> Last but not least, I can give you another hint....
>
> struct media_entity *entity = (something_here);
> struct media_pad *mpad;
> struct v4l2_subdev *cam_subdev;
> struct v4l2_ctrl *ctl;
> s64 link_frequency, pixel_clock;
>
> if (entity->pads[0].flags & MEDIA_PAD_FL_SINK)
>     return -E_NOT_A_CAMERA_SENSOR_WE_IGNORE_THIS_ONE;
>
> pad = media_pad_remote_pad_first(&entity->pads[0]);
> if (!pad)
>    return -ENOENT;
>
> if (!is_media_entity_v4l2_subdev(pad->entity))
>    return -ENOENT;
>
> if (pad->entity->function != MEDIA_ENT_F_CAM_SENSOR)
>    return -ENOENT;
>

Hi Angelo,

Thank you for the detailed explanation :)
However, I can't make it work because in my case, seninf is connected
to an external ISP
so pad->entity->function == MEDIA_ENT_F_PROC_VIDEO_ISP.

How can I get the pad corresponding to the sensor?


> cam_subdev = media_entity_to_v4l2_subdev(pad->entity);
> ctl = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
>

Is this mandatory to implement V4L2_CID_PIXEL_RATE ?
Should I return an error if not found?

> /* multiplier is usually bits per pixel, divider is usually num of lanes */
> link_frequency = v4l2_get_link_freq(cam_subdev->ctrl_handler, multiplier, divider);
> pixel_clock = v4l2_ctrl_g_ctrl_int64(ctl);
>

How to know the sensor clock given link_frequency and pixel_clock?
Can you point me to drivers doing something similar?

Cheers,
Julien
>
> ....now you know what the sensor wants, set the seninf sensor clock accordingly.
>
> Cheers
> Angelo
>
[...]

Cheers
Julien

On Fri, Jun 14, 2024 at 12:38:15PM +0200, Julien Stephan wrote:
> Le mer. 12 juin 2024 à 10:06, AngeloGioacchino Del Regno a écrit :
> >
> > Il 10/06/24 16:39, Julien Stephan ha scritto:
> [...]
> > >>
> > >>> +     writel(0x10001, input->base + SENINF_TG1_SEN_CK);
> > >>
> > >> Unroll this one... this is the TG1 sensor clock divider.
> > >>
> > >> CLKFL GENMASK(5, 0)
> > >> CLKRS GENMASK(13, 8)
> > >> CLKCNT GENMASK(21,16)
> > >>
> > >> Like this, I don't get what you're trying to set, because you're using a fixed
> > >> sensor clock rate, meaning that only a handful of camera sensors will be usable.
> > >>
> > >> Is this 8Mhz? 16? 24? what? :-)
> > >>
> > >> Two hints:
> > >>    - sensor_clk = clk_get_rate(isp_clk) / (tg1_sen_ck_clkcnt + 1);
> > >>    - int mtk_seninf_set_sensor_clk(u8 rate_mhz);
> > >>
> > >> Please :-)
> > >
> > > Hi Angelo,
> > >
> > > I think I get your point about not hardcoding the sensor rate, but I
> > > am not sure how to use
> > > a mtk_seninf_set_sensor_clk(u8 rate_mhz); function.
> > >
> > > Where would it be called? How is it exposed to the user?
> > >
> >
> > As for where: setup, streaming start, resolution change (which may be covered
> > by streaming start anyway, as a change should be calling stop->start anyway).
> >
> > And for the how is it exposed to the user - well, depends what you mean for user,
> > but it's all standard V4L2 API :-)
> >
> > Last but not least, I can give you another hint....
> >
> > struct media_entity *entity = (something_here);
> > struct media_pad *mpad;
> > struct v4l2_subdev *cam_subdev;
> > struct v4l2_ctrl *ctl;
> > s64 link_frequency, pixel_clock;
> >
> > if (entity->pads[0].flags & MEDIA_PAD_FL_SINK)
> >     return -E_NOT_A_CAMERA_SENSOR_WE_IGNORE_THIS_ONE;
> >
> > pad = media_pad_remote_pad_first(&entity->pads[0]);
> > if (!pad)
> >    return -ENOENT;
> >
> > if (!is_media_entity_v4l2_subdev(pad->entity))
> >    return -ENOENT;
> >
> > if (pad->entity->function != MEDIA_ENT_F_CAM_SENSOR)
> >    return -ENOENT;
> >
> 
> Hi Angelo,
> 
> Thank you for the detailed explanation :)
> However, I can't make it work because in my case, seninf is connected
> to an external ISP
> so pad->entity->function == MEDIA_ENT_F_PROC_VIDEO_ISP.
> 
> How can I get the pad corresponding to the sensor?

You don't have to. You can drop that check, and get the link frequency
of the source subdev with v4l2_get_link_freq(), whatever it is.

> > cam_subdev = media_entity_to_v4l2_subdev(pad->entity);
> > ctl = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
> 
> Is this mandatory to implement V4L2_CID_PIXEL_RATE ?
> Should I return an error if not found?

Does SENINF need to know both the pixel rate and link frequency ?
V4L2_CID_PIXEL_RATE is very ill-defined, at the moment it only makes
sense as a value relative to the sensor pixel array, and doesn't really
apply further down in the pipeline. What information do you need to
program the SENINF ?

> > /* multiplier is usually bits per pixel, divider is usually num of lanes */
> > link_frequency = v4l2_get_link_freq(cam_subdev->ctrl_handler, multiplier, divider);
> > pixel_clock = v4l2_ctrl_g_ctrl_int64(ctl);
> 
> How to know the sensor clock given link_frequency and pixel_clock?
> Can you point me to drivers doing something similar?
> 
> >
> > ....now you know what the sensor wants, set the seninf sensor clock accordingly.
> >
> > Cheers
> > Angelo
> >
> [...]

Le ven. 14 juin 2024 à 14:34, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> a écrit :
>
> On Fri, Jun 14, 2024 at 12:38:15PM +0200, Julien Stephan wrote:
> > Le mer. 12 juin 2024 à 10:06, AngeloGioacchino Del Regno a écrit :
> > >
> > > Il 10/06/24 16:39, Julien Stephan ha scritto:
> > [...]
> > > >>
> > > >>> +     writel(0x10001, input->base + SENINF_TG1_SEN_CK);
> > > >>
> > > >> Unroll this one... this is the TG1 sensor clock divider.
> > > >>
> > > >> CLKFL GENMASK(5, 0)
> > > >> CLKRS GENMASK(13, 8)
> > > >> CLKCNT GENMASK(21,16)
> > > >>
> > > >> Like this, I don't get what you're trying to set, because you're using a fixed
> > > >> sensor clock rate, meaning that only a handful of camera sensors will be usable.
> > > >>
> > > >> Is this 8Mhz? 16? 24? what? :-)
> > > >>
> > > >> Two hints:
> > > >>    - sensor_clk = clk_get_rate(isp_clk) / (tg1_sen_ck_clkcnt + 1);
> > > >>    - int mtk_seninf_set_sensor_clk(u8 rate_mhz);
> > > >>
> > > >> Please :-)
> > > >
> > > > Hi Angelo,
> > > >
> > > > I think I get your point about not hardcoding the sensor rate, but I
> > > > am not sure how to use
> > > > a mtk_seninf_set_sensor_clk(u8 rate_mhz); function.
> > > >
> > > > Where would it be called? How is it exposed to the user?
> > > >
> > >
> > > As for where: setup, streaming start, resolution change (which may be covered
> > > by streaming start anyway, as a change should be calling stop->start anyway).
> > >
> > > And for the how is it exposed to the user - well, depends what you mean for user,
> > > but it's all standard V4L2 API :-)
> > >
> > > Last but not least, I can give you another hint....
> > >
> > > struct media_entity *entity = (something_here);
> > > struct media_pad *mpad;
> > > struct v4l2_subdev *cam_subdev;
> > > struct v4l2_ctrl *ctl;
> > > s64 link_frequency, pixel_clock;
> > >
> > > if (entity->pads[0].flags & MEDIA_PAD_FL_SINK)
> > >     return -E_NOT_A_CAMERA_SENSOR_WE_IGNORE_THIS_ONE;
> > >
> > > pad = media_pad_remote_pad_first(&entity->pads[0]);
> > > if (!pad)
> > >    return -ENOENT;
> > >
> > > if (!is_media_entity_v4l2_subdev(pad->entity))
> > >    return -ENOENT;
> > >
> > > if (pad->entity->function != MEDIA_ENT_F_CAM_SENSOR)
> > >    return -ENOENT;
> > >
> >
> > Hi Angelo,
> >
> > Thank you for the detailed explanation :)
> > However, I can't make it work because in my case, seninf is connected
> > to an external ISP
> > so pad->entity->function == MEDIA_ENT_F_PROC_VIDEO_ISP.
> >
> > How can I get the pad corresponding to the sensor?
>
> You don't have to. You can drop that check, and get the link frequency
> of the source subdev with v4l2_get_link_freq(), whatever it is.
>
> > > cam_subdev = media_entity_to_v4l2_subdev(pad->entity);
> > > ctl = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
> >
> > Is this mandatory to implement V4L2_CID_PIXEL_RATE ?
> > Should I return an error if not found?
>
> Does SENINF need to know both the pixel rate and link frequency ?
> V4L2_CID_PIXEL_RATE is very ill-defined, at the moment it only makes
> sense as a value relative to the sensor pixel array, and doesn't really
> apply further down in the pipeline. What information do you need to
> program the SENINF ?
>

Hi Laurent,

I need to know the clock divider for the sensor

Cheers
Julien

> > > /* multiplier is usually bits per pixel, divider is usually num of lanes */
> > > link_frequency = v4l2_get_link_freq(cam_subdev->ctrl_handler, multiplier, divider);
> > > pixel_clock = v4l2_ctrl_g_ctrl_int64(ctl);
> >
> > How to know the sensor clock given link_frequency and pixel_clock?
> > Can you point me to drivers doing something similar?
> >
> > >
> > > ....now you know what the sensor wants, set the seninf sensor clock accordingly.
> > >
> > > Cheers
> > > Angelo
> > >
> > [...]
>
> --
> Regards,
>
> Laurent Pinchart

Hi Julien,

On Fri, Jun 14, 2024 at 04:14:52PM +0200, Julien Stephan wrote:
> Le ven. 14 juin 2024 à 14:34, Laurent Pinchart a écrit :
> > On Fri, Jun 14, 2024 at 12:38:15PM +0200, Julien Stephan wrote:
> > > Le mer. 12 juin 2024 à 10:06, AngeloGioacchino Del Regno a écrit :
> > > >
> > > > Il 10/06/24 16:39, Julien Stephan ha scritto:
> > > [...]
> > > > >>
> > > > >>> +     writel(0x10001, input->base + SENINF_TG1_SEN_CK);
> > > > >>
> > > > >> Unroll this one... this is the TG1 sensor clock divider.
> > > > >>
> > > > >> CLKFL GENMASK(5, 0)
> > > > >> CLKRS GENMASK(13, 8)
> > > > >> CLKCNT GENMASK(21,16)
> > > > >>
> > > > >> Like this, I don't get what you're trying to set, because you're using a fixed
> > > > >> sensor clock rate, meaning that only a handful of camera sensors will be usable.
> > > > >>
> > > > >> Is this 8Mhz? 16? 24? what? :-)
> > > > >>
> > > > >> Two hints:
> > > > >>    - sensor_clk = clk_get_rate(isp_clk) / (tg1_sen_ck_clkcnt + 1);
> > > > >>    - int mtk_seninf_set_sensor_clk(u8 rate_mhz);
> > > > >>
> > > > >> Please :-)
> > > > >
> > > > > Hi Angelo,
> > > > >
> > > > > I think I get your point about not hardcoding the sensor rate, but I
> > > > > am not sure how to use
> > > > > a mtk_seninf_set_sensor_clk(u8 rate_mhz); function.
> > > > >
> > > > > Where would it be called? How is it exposed to the user?
> > > > >
> > > >
> > > > As for where: setup, streaming start, resolution change (which may be covered
> > > > by streaming start anyway, as a change should be calling stop->start anyway).
> > > >
> > > > And for the how is it exposed to the user - well, depends what you mean for user,
> > > > but it's all standard V4L2 API :-)
> > > >
> > > > Last but not least, I can give you another hint....
> > > >
> > > > struct media_entity *entity = (something_here);
> > > > struct media_pad *mpad;
> > > > struct v4l2_subdev *cam_subdev;
> > > > struct v4l2_ctrl *ctl;
> > > > s64 link_frequency, pixel_clock;
> > > >
> > > > if (entity->pads[0].flags & MEDIA_PAD_FL_SINK)
> > > >     return -E_NOT_A_CAMERA_SENSOR_WE_IGNORE_THIS_ONE;
> > > >
> > > > pad = media_pad_remote_pad_first(&entity->pads[0]);
> > > > if (!pad)
> > > >    return -ENOENT;
> > > >
> > > > if (!is_media_entity_v4l2_subdev(pad->entity))
> > > >    return -ENOENT;
> > > >
> > > > if (pad->entity->function != MEDIA_ENT_F_CAM_SENSOR)
> > > >    return -ENOENT;
> > > >
> > >
> > > Hi Angelo,
> > >
> > > Thank you for the detailed explanation :)
> > > However, I can't make it work because in my case, seninf is connected
> > > to an external ISP
> > > so pad->entity->function == MEDIA_ENT_F_PROC_VIDEO_ISP.
> > >
> > > How can I get the pad corresponding to the sensor?
> >
> > You don't have to. You can drop that check, and get the link frequency
> > of the source subdev with v4l2_get_link_freq(), whatever it is.
> >
> > > > cam_subdev = media_entity_to_v4l2_subdev(pad->entity);
> > > > ctl = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > >
> > > Is this mandatory to implement V4L2_CID_PIXEL_RATE ?
> > > Should I return an error if not found?
> >
> > Does SENINF need to know both the pixel rate and link frequency ?
> > V4L2_CID_PIXEL_RATE is very ill-defined, at the moment it only makes
> > sense as a value relative to the sensor pixel array, and doesn't really
> > apply further down in the pipeline. What information do you need to
> > program the SENINF ?
> 
> Hi Laurent,
> 
> I need to know the clock divider for the sensor

Could you provide some details on how the SENINF uses that divisor ?
What does it control, and what are the constraints ?

> > > > /* multiplier is usually bits per pixel, divider is usually num of lanes */
> > > > link_frequency = v4l2_get_link_freq(cam_subdev->ctrl_handler, multiplier, divider);
> > > > pixel_clock = v4l2_ctrl_g_ctrl_int64(ctl);
> > >
> > > How to know the sensor clock given link_frequency and pixel_clock?
> > > Can you point me to drivers doing something similar?
> > >
> > > >
> > > > ....now you know what the sensor wants, set the seninf sensor clock accordingly.
> > > >
> > > > Cheers
> > > > Angelo
> > > >
> > > [...]

Le ven. 14 juin 2024 à 16:43, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> a écrit :
>
> Hi Julien,
>
> On Fri, Jun 14, 2024 at 04:14:52PM +0200, Julien Stephan wrote:
> > Le ven. 14 juin 2024 à 14:34, Laurent Pinchart a écrit :
> > > On Fri, Jun 14, 2024 at 12:38:15PM +0200, Julien Stephan wrote:
> > > > Le mer. 12 juin 2024 à 10:06, AngeloGioacchino Del Regno a écrit :
> > > > >
> > > > > Il 10/06/24 16:39, Julien Stephan ha scritto:
> > > > [...]
> > > > > >>
> > > > > >>> +     writel(0x10001, input->base + SENINF_TG1_SEN_CK);
> > > > > >>
> > > > > >> Unroll this one... this is the TG1 sensor clock divider.
> > > > > >>
> > > > > >> CLKFL GENMASK(5, 0)
> > > > > >> CLKRS GENMASK(13, 8)
> > > > > >> CLKCNT GENMASK(21,16)
> > > > > >>
> > > > > >> Like this, I don't get what you're trying to set, because you're using a fixed
> > > > > >> sensor clock rate, meaning that only a handful of camera sensors will be usable.
> > > > > >>
> > > > > >> Is this 8Mhz? 16? 24? what? :-)
> > > > > >>
> > > > > >> Two hints:
> > > > > >>    - sensor_clk = clk_get_rate(isp_clk) / (tg1_sen_ck_clkcnt + 1);
> > > > > >>    - int mtk_seninf_set_sensor_clk(u8 rate_mhz);
> > > > > >>
> > > > > >> Please :-)
> > > > > >
> > > > > > Hi Angelo,
> > > > > >
> > > > > > I think I get your point about not hardcoding the sensor rate, but I
> > > > > > am not sure how to use
> > > > > > a mtk_seninf_set_sensor_clk(u8 rate_mhz); function.
> > > > > >
> > > > > > Where would it be called? How is it exposed to the user?
> > > > > >
> > > > >
> > > > > As for where: setup, streaming start, resolution change (which may be covered
> > > > > by streaming start anyway, as a change should be calling stop->start anyway).
> > > > >
> > > > > And for the how is it exposed to the user - well, depends what you mean for user,
> > > > > but it's all standard V4L2 API :-)
> > > > >
> > > > > Last but not least, I can give you another hint....
> > > > >
> > > > > struct media_entity *entity = (something_here);
> > > > > struct media_pad *mpad;
> > > > > struct v4l2_subdev *cam_subdev;
> > > > > struct v4l2_ctrl *ctl;
> > > > > s64 link_frequency, pixel_clock;
> > > > >
> > > > > if (entity->pads[0].flags & MEDIA_PAD_FL_SINK)
> > > > >     return -E_NOT_A_CAMERA_SENSOR_WE_IGNORE_THIS_ONE;
> > > > >
> > > > > pad = media_pad_remote_pad_first(&entity->pads[0]);
> > > > > if (!pad)
> > > > >    return -ENOENT;
> > > > >
> > > > > if (!is_media_entity_v4l2_subdev(pad->entity))
> > > > >    return -ENOENT;
> > > > >
> > > > > if (pad->entity->function != MEDIA_ENT_F_CAM_SENSOR)
> > > > >    return -ENOENT;
> > > > >
> > > >
> > > > Hi Angelo,
> > > >
> > > > Thank you for the detailed explanation :)
> > > > However, I can't make it work because in my case, seninf is connected
> > > > to an external ISP
> > > > so pad->entity->function == MEDIA_ENT_F_PROC_VIDEO_ISP.
> > > >
> > > > How can I get the pad corresponding to the sensor?
> > >
> > > You don't have to. You can drop that check, and get the link frequency
> > > of the source subdev with v4l2_get_link_freq(), whatever it is.
> > >
> > > > > cam_subdev = media_entity_to_v4l2_subdev(pad->entity);
> > > > > ctl = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > > >
> > > > Is this mandatory to implement V4L2_CID_PIXEL_RATE ?
> > > > Should I return an error if not found?
> > >
> > > Does SENINF need to know both the pixel rate and link frequency ?
> > > V4L2_CID_PIXEL_RATE is very ill-defined, at the moment it only makes
> > > sense as a value relative to the sensor pixel array, and doesn't really
> > > apply further down in the pipeline. What information do you need to
> > > program the SENINF ?
> >
> > Hi Laurent,
> >
> > I need to know the clock divider for the sensor
>
> Could you provide some details on how the SENINF uses that divisor ?
> What does it control, and what are the constraints ?
>

According to the datasheet,  SENINF_TG1_SEN_CK[21:16] :CLKCNT : Sensor
master clock will be ISP_clock/(CLKCNT+1) where CLKCNT >= 1

> > > > > /* multiplier is usually bits per pixel, divider is usually num of lanes */
> > > > > link_frequency = v4l2_get_link_freq(cam_subdev->ctrl_handler, multiplier, divider);
> > > > > pixel_clock = v4l2_ctrl_g_ctrl_int64(ctl);
> > > >
> > > > How to know the sensor clock given link_frequency and pixel_clock?
> > > > Can you point me to drivers doing something similar?
> > > >
> > > > >
> > > > > ....now you know what the sensor wants, set the seninf sensor clock accordingly.
> > > > >
> > > > > Cheers
> > > > > Angelo
> > > > >
> > > > [...]
>
> --
> Regards,
>
> Laurent Pinchart

On Fri, Jun 14, 2024 at 04:54:47PM +0200, Julien Stephan wrote:
> Le ven. 14 juin 2024 à 16:43, Laurent Pinchart a écrit :
> > On Fri, Jun 14, 2024 at 04:14:52PM +0200, Julien Stephan wrote:
> > > Le ven. 14 juin 2024 à 14:34, Laurent Pinchart a écrit :
> > > > On Fri, Jun 14, 2024 at 12:38:15PM +0200, Julien Stephan wrote:
> > > > > Le mer. 12 juin 2024 à 10:06, AngeloGioacchino Del Regno a écrit :
> > > > > >
> > > > > > Il 10/06/24 16:39, Julien Stephan ha scritto:
> > > > > [...]
> > > > > > >>
> > > > > > >>> +     writel(0x10001, input->base + SENINF_TG1_SEN_CK);
> > > > > > >>
> > > > > > >> Unroll this one... this is the TG1 sensor clock divider.
> > > > > > >>
> > > > > > >> CLKFL GENMASK(5, 0)
> > > > > > >> CLKRS GENMASK(13, 8)
> > > > > > >> CLKCNT GENMASK(21,16)
> > > > > > >>
> > > > > > >> Like this, I don't get what you're trying to set, because you're using a fixed
> > > > > > >> sensor clock rate, meaning that only a handful of camera sensors will be usable.
> > > > > > >>
> > > > > > >> Is this 8Mhz? 16? 24? what? :-)
> > > > > > >>
> > > > > > >> Two hints:
> > > > > > >>    - sensor_clk = clk_get_rate(isp_clk) / (tg1_sen_ck_clkcnt + 1);
> > > > > > >>    - int mtk_seninf_set_sensor_clk(u8 rate_mhz);
> > > > > > >>
> > > > > > >> Please :-)
> > > > > > >
> > > > > > > Hi Angelo,
> > > > > > >
> > > > > > > I think I get your point about not hardcoding the sensor rate, but I
> > > > > > > am not sure how to use
> > > > > > > a mtk_seninf_set_sensor_clk(u8 rate_mhz); function.
> > > > > > >
> > > > > > > Where would it be called? How is it exposed to the user?
> > > > > > >
> > > > > >
> > > > > > As for where: setup, streaming start, resolution change (which may be covered
> > > > > > by streaming start anyway, as a change should be calling stop->start anyway).
> > > > > >
> > > > > > And for the how is it exposed to the user - well, depends what you mean for user,
> > > > > > but it's all standard V4L2 API :-)
> > > > > >
> > > > > > Last but not least, I can give you another hint....
> > > > > >
> > > > > > struct media_entity *entity = (something_here);
> > > > > > struct media_pad *mpad;
> > > > > > struct v4l2_subdev *cam_subdev;
> > > > > > struct v4l2_ctrl *ctl;
> > > > > > s64 link_frequency, pixel_clock;
> > > > > >
> > > > > > if (entity->pads[0].flags & MEDIA_PAD_FL_SINK)
> > > > > >     return -E_NOT_A_CAMERA_SENSOR_WE_IGNORE_THIS_ONE;
> > > > > >
> > > > > > pad = media_pad_remote_pad_first(&entity->pads[0]);
> > > > > > if (!pad)
> > > > > >    return -ENOENT;
> > > > > >
> > > > > > if (!is_media_entity_v4l2_subdev(pad->entity))
> > > > > >    return -ENOENT;
> > > > > >
> > > > > > if (pad->entity->function != MEDIA_ENT_F_CAM_SENSOR)
> > > > > >    return -ENOENT;
> > > > > >
> > > > >
> > > > > Hi Angelo,
> > > > >
> > > > > Thank you for the detailed explanation :)
> > > > > However, I can't make it work because in my case, seninf is connected
> > > > > to an external ISP
> > > > > so pad->entity->function == MEDIA_ENT_F_PROC_VIDEO_ISP.
> > > > >
> > > > > How can I get the pad corresponding to the sensor?
> > > >
> > > > You don't have to. You can drop that check, and get the link frequency
> > > > of the source subdev with v4l2_get_link_freq(), whatever it is.
> > > >
> > > > > > cam_subdev = media_entity_to_v4l2_subdev(pad->entity);
> > > > > > ctl = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > > > >
> > > > > Is this mandatory to implement V4L2_CID_PIXEL_RATE ?
> > > > > Should I return an error if not found?
> > > >
> > > > Does SENINF need to know both the pixel rate and link frequency ?
> > > > V4L2_CID_PIXEL_RATE is very ill-defined, at the moment it only makes
> > > > sense as a value relative to the sensor pixel array, and doesn't really
> > > > apply further down in the pipeline. What information do you need to
> > > > program the SENINF ?
> > >
> > > Hi Laurent,
> > >
> > > I need to know the clock divider for the sensor
> >
> > Could you provide some details on how the SENINF uses that divisor ?
> > What does it control, and what are the constraints ?
> 
> According to the datasheet,  SENINF_TG1_SEN_CK[21:16] :CLKCNT : Sensor
> master clock will be ISP_clock/(CLKCNT+1) where CLKCNT >= 1

I'll need more information. My guess so far is that there's a FIFO
somewhere in the SENINF, with the pixel bus clocked by the CSI-2 clock
before the FIFO, and by the "Sensor master clock" after the FIFO. Is
that right ? If so, the simplest approach would be to use the link
frequency to compute the pixel clock before the FIFO, and make sure that
the sensor master clock will be larger than or equal to that.

A better approach from a power consumption point of view would be to
consider horizontal blanking. The FIFO can fill faster than it gets
emptied during the active portion of the line and then drain during
blanking. This allows for a slower clock on the output side. You will
need to pick an output clock frequency that

- on average is larger than the number of active pixels per line divided
  by the line duration ; and

- ensures the FIFO never overflows during the active portion of the line,
  for cases where the line length is larger than the FIFO size.

> > > > > > /* multiplier is usually bits per pixel, divider is usually num of lanes */
> > > > > > link_frequency = v4l2_get_link_freq(cam_subdev->ctrl_handler, multiplier, divider);
> > > > > > pixel_clock = v4l2_ctrl_g_ctrl_int64(ctl);
> > > > >
> > > > > How to know the sensor clock given link_frequency and pixel_clock?
> > > > > Can you point me to drivers doing something similar?
> > > > >
> > > > > >
> > > > > > ....now you know what the sensor wants, set the seninf sensor clock accordingly.
> > > > > >
> > > > > > Cheers
> > > > > > Angelo
> > > > > >
> > > > > [...]

Le sam. 15 juin 2024 à 02:47, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> a écrit :
>
> On Fri, Jun 14, 2024 at 04:54:47PM +0200, Julien Stephan wrote:
> > Le ven. 14 juin 2024 à 16:43, Laurent Pinchart a écrit :
> > > On Fri, Jun 14, 2024 at 04:14:52PM +0200, Julien Stephan wrote:
> > > > Le ven. 14 juin 2024 à 14:34, Laurent Pinchart a écrit :
> > > > > On Fri, Jun 14, 2024 at 12:38:15PM +0200, Julien Stephan wrote:
> > > > > > Le mer. 12 juin 2024 à 10:06, AngeloGioacchino Del Regno a écrit :
> > > > > > >
> > > > > > > Il 10/06/24 16:39, Julien Stephan ha scritto:
> > > > > > [...]
> > > > > > > >>
> > > > > > > >>> +     writel(0x10001, input->base + SENINF_TG1_SEN_CK);
> > > > > > > >>
> > > > > > > >> Unroll this one... this is the TG1 sensor clock divider.
> > > > > > > >>
> > > > > > > >> CLKFL GENMASK(5, 0)
> > > > > > > >> CLKRS GENMASK(13, 8)
> > > > > > > >> CLKCNT GENMASK(21,16)
> > > > > > > >>
> > > > > > > >> Like this, I don't get what you're trying to set, because you're using a fixed
> > > > > > > >> sensor clock rate, meaning that only a handful of camera sensors will be usable.
> > > > > > > >>
> > > > > > > >> Is this 8Mhz? 16? 24? what? :-)
> > > > > > > >>
> > > > > > > >> Two hints:
> > > > > > > >>    - sensor_clk = clk_get_rate(isp_clk) / (tg1_sen_ck_clkcnt + 1);
> > > > > > > >>    - int mtk_seninf_set_sensor_clk(u8 rate_mhz);
> > > > > > > >>
> > > > > > > >> Please :-)
> > > > > > > >
> > > > > > > > Hi Angelo,
> > > > > > > >
> > > > > > > > I think I get your point about not hardcoding the sensor rate, but I
> > > > > > > > am not sure how to use
> > > > > > > > a mtk_seninf_set_sensor_clk(u8 rate_mhz); function.
> > > > > > > >
> > > > > > > > Where would it be called? How is it exposed to the user?
> > > > > > > >
> > > > > > >
> > > > > > > As for where: setup, streaming start, resolution change (which may be covered
> > > > > > > by streaming start anyway, as a change should be calling stop->start anyway).
> > > > > > >
> > > > > > > And for the how is it exposed to the user - well, depends what you mean for user,
> > > > > > > but it's all standard V4L2 API :-)
> > > > > > >
> > > > > > > Last but not least, I can give you another hint....
> > > > > > >
> > > > > > > struct media_entity *entity = (something_here);
> > > > > > > struct media_pad *mpad;
> > > > > > > struct v4l2_subdev *cam_subdev;
> > > > > > > struct v4l2_ctrl *ctl;
> > > > > > > s64 link_frequency, pixel_clock;
> > > > > > >
> > > > > > > if (entity->pads[0].flags & MEDIA_PAD_FL_SINK)
> > > > > > >     return -E_NOT_A_CAMERA_SENSOR_WE_IGNORE_THIS_ONE;
> > > > > > >
> > > > > > > pad = media_pad_remote_pad_first(&entity->pads[0]);
> > > > > > > if (!pad)
> > > > > > >    return -ENOENT;
> > > > > > >
> > > > > > > if (!is_media_entity_v4l2_subdev(pad->entity))
> > > > > > >    return -ENOENT;
> > > > > > >
> > > > > > > if (pad->entity->function != MEDIA_ENT_F_CAM_SENSOR)
> > > > > > >    return -ENOENT;
> > > > > > >
> > > > > >
> > > > > > Hi Angelo,
> > > > > >
> > > > > > Thank you for the detailed explanation :)
> > > > > > However, I can't make it work because in my case, seninf is connected
> > > > > > to an external ISP
> > > > > > so pad->entity->function == MEDIA_ENT_F_PROC_VIDEO_ISP.
> > > > > >
> > > > > > How can I get the pad corresponding to the sensor?
> > > > >
> > > > > You don't have to. You can drop that check, and get the link frequency
> > > > > of the source subdev with v4l2_get_link_freq(), whatever it is.
> > > > >
> > > > > > > cam_subdev = media_entity_to_v4l2_subdev(pad->entity);
> > > > > > > ctl = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > > > > >
> > > > > > Is this mandatory to implement V4L2_CID_PIXEL_RATE ?
> > > > > > Should I return an error if not found?
> > > > >
> > > > > Does SENINF need to know both the pixel rate and link frequency ?
> > > > > V4L2_CID_PIXEL_RATE is very ill-defined, at the moment it only makes
> > > > > sense as a value relative to the sensor pixel array, and doesn't really
> > > > > apply further down in the pipeline. What information do you need to
> > > > > program the SENINF ?
> > > >
> > > > Hi Laurent,
> > > >
> > > > I need to know the clock divider for the sensor
> > >
> > > Could you provide some details on how the SENINF uses that divisor ?
> > > What does it control, and what are the constraints ?
> >
> > According to the datasheet,  SENINF_TG1_SEN_CK[21:16] :CLKCNT : Sensor
> > master clock will be ISP_clock/(CLKCNT+1) where CLKCNT >= 1
>
> I'll need more information. My guess so far is that there's a FIFO
> somewhere in the SENINF, with the pixel bus clocked by the CSI-2 clock
> before the FIFO, and by the "Sensor master clock" after the FIFO. Is
> that right ? If so, the simplest approach would be to use the link
> frequency to compute the pixel clock before the FIFO, and make sure that
> the sensor master clock will be larger than or equal to that.
>
> A better approach from a power consumption point of view would be to
> consider horizontal blanking. The FIFO can fill faster than it gets
> emptied during the active portion of the line and then drain during
> blanking. This allows for a slower clock on the output side. You will
> need to pick an output clock frequency that
>
> - on average is larger than the number of active pixels per line divided
>   by the line duration ; and
>
> - ensures the FIFO never overflows during the active portion of the line,
>   for cases where the line length is larger than the FIFO size.
>

Hi Laurent and Angelo,

Thank you for your help on this. I just sent a v5 with such a
function. But it seems like changing
this register has no effect. I asked some feedback from mediatek's app
engineer but they are OoO
until 16/7. I am waiting for them.

Cheers
Julien
> > > > > > > /* multiplier is usually bits per pixel, divider is usually num of lanes */
> > > > > > > link_frequency = v4l2_get_link_freq(cam_subdev->ctrl_handler, multiplier, divider);
> > > > > > > pixel_clock = v4l2_ctrl_g_ctrl_int64(ctl);
> > > > > >
> > > > > > How to know the sensor clock given link_frequency and pixel_clock?
> > > > > > Can you point me to drivers doing something similar?
> > > > > >
> > > > > > >
> > > > > > > ....now you know what the sensor wants, set the seninf sensor clock accordingly.
> > > > > > >
> > > > > > > Cheers
> > > > > > > Angelo
> > > > > > >
> > > > > > [...]
>
> --
> Regards,
>
> Laurent Pinchart