| Message ID | 20230630100321.1951138-1-jstephan@baylibre.com |
|---|---|
| Headers | show |
| Series | Add Mediatek ISP3.0 | expand |
Hi, Julien: Julien Stephan <jstephan@baylibre.com> 於 2023年6月30日 週五 下午6:05寫道： > > 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> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 | 1561 +++++++++++++++++ > .../isp/isp_30/seninf/mtk_seninf_reg.h | 99 ++ I think it's not necessary to create a folder isp_30 inside isp and you could just place all file in isp folder because there are only few files. After adding more and more files then we consider to create folder to separate them. Regards, Chun-Kuang. > 10 files changed, 1692 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: Julien Stephan <jstephan@baylibre.com> 於 2023年6月30日 週五 下午6:05寫道： > > 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> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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, > +}; > + > +static const struct of_device_id mtk_seninf_of_match[] = { > + { > + .compatible = "mediatek,mt8365-seninf", > + .data = &seninf_8365_conf, Now only support one SoC, so it's necessary to select the SoC data and you could directly place the data in the code. After support other SoC, so we could know what should be placed in struct mtg_seninf_conf (Now we have no any information). Regards, Chun-Kuang. > + }, > + { > + }, > +}; > +MODULE_DEVICE_TABLE(of, mtk_seninf_of_match);
Hi, Julien: Chun-Kuang Hu <chunkuang.hu@kernel.org> 於 2023年7月2日 週日 上午7:35寫道： > > Hi, Julien: > > Julien Stephan <jstephan@baylibre.com> 於 2023年6月30日 週五 下午6:05寫道： > > > > 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> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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, > > +}; > > + > > +static const struct of_device_id mtk_seninf_of_match[] = { > > + { > > + .compatible = "mediatek,mt8365-seninf", > > + .data = &seninf_8365_conf, > > Now only support one SoC, so it's necessary to select the SoC data and > you could directly place the data in the code. After support other Typo. Now only support one SoC, so it's not necessary to select the SoC data and you could directly place the data in the code. Regards, Chun-Kuang. > SoC, so we could know what should be placed in struct mtg_seninf_conf > (Now we have no any information). > > Regards, > Chun-Kuang. > > > + }, > > + { > > + }, > > +}; > > +MODULE_DEVICE_TABLE(of, mtk_seninf_of_match);
Hi, Julien: Julien Stephan <jstephan@baylibre.com> 於 2023年6月30日 週五 下午6:06寫道： > > From: Phi-bang Nguyen <pnguyen@baylibre.com> > > This driver provides a path to bypass the SoC ISP so that image data > coming from the SENINF can go directly into memory without any image > processing. This allows the use of an external ISP. > > Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com> > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > --- [snip] > + > +static irqreturn_t isp_irq_camsv30(int irq, void *data) > +{ > + struct mtk_cam_dev *cam_dev = (struct mtk_cam_dev *)data; > + struct mtk_cam_dev_buffer *buf; > + unsigned long flags = 0; > + unsigned int irq_status; > + > + spin_lock_irqsave(&cam_dev->irqlock, flags); > + > + irq_status = mtk_camsv30_read(cam_dev, CAMSV_INT_STATUS); > + > + if (irq_status & INT_ST_MASK_CAMSV_ERR) { > + dev_err(cam_dev->dev, "irq error 0x%x\n", > + (unsigned int)(irq_status & INT_ST_MASK_CAMSV_ERR)); > + } > + > + /* De-queue frame */ > + if (irq_status & CAMSV_IRQ_PASS1_DON) { > + cam_dev->sequence++; > + > + if (!cam_dev->is_dummy_used) { > + buf = list_first_entry_or_null(&cam_dev->buf_list, > + struct mtk_cam_dev_buffer, > + list); > + if (buf) { > + buf->v4l2_buf.sequence = cam_dev->sequence; > + buf->v4l2_buf.vb2_buf.timestamp = ktime_get_ns(); > + vb2_buffer_done(&buf->v4l2_buf.vb2_buf, > + VB2_BUF_STATE_DONE); > + list_del(&buf->list); > + } > + } > + > + if (list_empty(&cam_dev->buf_list)) { > + (*cam_dev->hw_functions->mtk_cam_update_buffers_add) > + (cam_dev, &cam_dev->dummy); It's not necessary to use function pointer. Call mtk_camsv30_update_buffers_add() directly. Regards, Chun-Kuang. > + cam_dev->is_dummy_used = true; > + } else { > + buf = list_first_entry_or_null(&cam_dev->buf_list, > + struct mtk_cam_dev_buffer, > + list); > + (*cam_dev->hw_functions->mtk_cam_update_buffers_add) > + (cam_dev, buf); > + cam_dev->is_dummy_used = false; > + } > + } > + > + spin_unlock_irqrestore(&cam_dev->irqlock, flags); > + > + return IRQ_HANDLED; > +} > +
Il 30/06/23 12:01, 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> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 | 1561 +++++++++++++++++ > .../isp/isp_30/seninf/mtk_seninf_reg.h | 99 ++ > 10 files changed, 1692 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 af273a477139..9fda342ad331 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13356,6 +13356,7 @@ M: Julien Stephan <jstephan@baylibre.com> > M: Andy Hsieh <andy.hsieh@mediatek.com> > S: Supported > 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..831f9bee5fdd > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_30/Kconfig > @@ -0,0 +1,16 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +config MTK_SENINF MTK_SENINF_V30 or MTK_ISP30_SENINF instead? > + 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. ..snip.. > 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..93a13a87d967 > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_30/seninf/mtk_seninf.c s/isp_30/isp30/g and s/mtk_seninf.c/mtk-seninf.c/g ...but then, I'm not sure that having a `isp30` folder inside of `isp` would be actually beneficial for the driver structure. You're the judge. > @@ -0,0 +1,1561 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2022 MediaTek Inc. > + */ > + > +#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" If seninf for ISP3 is entirely different from ISP2 and ISP1 seninf, I'd rename that header to "mtk_isp3_seninf_reg.h" instead. > + > +#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 > + > +enum PIXEL_MODE { Lowercase please. > + ONE_PIXEL_MODE = 0x0, > + TWO_PIXEL_MODE = 0x1, > + FOUR_PIXEL_MODE = 0x2, > +}; > + > +/* 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, Why is there no SENINF_4? That deserves a comment in the code. > +}; > + > +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, Is the port-to-seninf path really static? Is there any way we can for example configure CSI_PORT_0 to use SENINF_3 instead? > +}; > + > +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; > +}; > + ...snip... > + > +static const struct mtk_seninf_format_info mtk_seninf_formats[] = { > + { > + .code = MEDIA_BUS_FMT_SBGGR8_1X8, > + .flags = MTK_SENINF_FORMAT_BAYER, Each entry fits in one line. { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .flags = MTK_SENINF_FORMAT_BAYER }, > + }, { > + .code = MEDIA_BUS_FMT_SGBRG8_1X8, > + .flags = MTK_SENINF_FORMAT_BAYER, ..snip.. > + > +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; > +} > + > +/* ----------------------------------------------------------------------------- > + * I/O Accessors > + */ > +static u32 mtk_seninf_read(struct mtk_seninf *priv, u32 reg) > +{ > + return readl(priv->base + reg); I don't think that we need this kind of accessor. It's one line, performing either just a readl or just a writel, so you can simply use readl/writel directly. > +} > + > +static void mtk_seninf_write(struct mtk_seninf *priv, u32 reg, u32 value) > +{ > + writel(value, priv->base + reg); > +} > + > +static void __mtk_seninf_update(struct mtk_seninf *priv, u32 reg, > + u32 mask, u32 value) > +{ > + u32 val = mtk_seninf_read(priv, reg); > + > + mtk_seninf_write(priv, reg, (val & ~mask) | (value & mask)); > +} > + > +#define mtk_seninf_update(priv, reg, field, val) \ > + __mtk_seninf_update(priv, reg, reg##_##field, \ > + FIELD_PREP(reg##_##field, val)) > + > +static u32 mtk_seninf_input_read(struct mtk_seninf_input *input, u32 reg) > +{ > + return readl(input->base + reg); > +} > + > +static void mtk_seninf_input_write(struct mtk_seninf_input *input, u32 reg, > + u32 value) > +{ same here. > + writel(value, input->base + reg); > +} > + > +static void __mtk_seninf_input_update(struct mtk_seninf_input *input, u32 reg, > + u32 mask, u32 value) > +{ > + u32 val = mtk_seninf_input_read(input, reg); > + > + mtk_seninf_input_write(input, reg, (val & ~mask) | (value & mask)); > +} > + > +#define mtk_seninf_input_update(input, reg, field, val) \ > + __mtk_seninf_input_update(input, reg, reg##_##field, \ > + FIELD_PREP(reg##_##field, val)) > + > +static u32 mtk_seninf_mux_read(struct mtk_seninf_mux *mux, u32 reg) > +{ > + return readl(mux->base + reg); ...and here > +} > + > +static void mtk_seninf_mux_write(struct mtk_seninf_mux *mux, u32 reg, > + u32 value) > +{ > + writel(value, mux->base + reg); > +} > + > +static void __mtk_seninf_mux_update(struct mtk_seninf_mux *mux, u32 reg, > + u32 mask, u32 value) > +{ > + u32 val = mtk_seninf_mux_read(mux, reg); > + > + mtk_seninf_mux_write(mux, reg, (val & ~mask) | (value & mask)); > +} > + > +#define mtk_seninf_mux_update(mux, reg, field, val) \ > + __mtk_seninf_mux_update(mux, reg, reg##_##field, \ > + FIELD_PREP(reg##_##field, val)) > + ..snip.. > +static void mtk_seninf_input_setup_csi2_rx(struct mtk_seninf_input *input) > +{ > + unsigned int lanes[4] = { }; I don't like that `4` magic number. #define MTK_CSI_MAX_LANES 4 or #define MTK_CSIHW_MAX_LANES 4 > + unsigned int i; > + > + /* > + * Configure data lane muxing. In 2D1C mode, lanes 0 to 2 correspond to > + * CSIx[AB]_L{0,1,2}, and in 4D1C lanes 0 to 5 correspond to > + * CSIxA_L{0,1,2}, CSIxB_L{0,1,2}. > + * > + * The clock lane must be skipped when calculating the index of the > + * physical data lane. For instance, in 4D1C mode, the sensor clock > + * lane is typically connected to lane 2 (CSIxA_L2), and the sensor > + * data lanes 0-3 to lanes 1 (CSIxA_L1), 3 (CSIxB_L0), 0 (CSIxA_L0) and > + * 4 (CSIxB_L1). The when skipping the clock lane, the data lane > + * indices become 1, 2, 0 and 3. > + */ > + for (i = 0; i < input->bus.num_data_lanes; ++i) { > + lanes[i] = input->bus.data_lanes[i]; > + if (lanes[i] > input->bus.clock_lane) > + lanes[i]--; > + } > + > + mtk_seninf_input_update(input, MIPI_RX_CON24_CSI0, > + CSI0_BIST_LN0_MUX, lanes[0]); > + mtk_seninf_input_update(input, MIPI_RX_CON24_CSI0, > + CSI0_BIST_LN1_MUX, lanes[1]); 89 columns is fine, so all of those do fit in one line. > + mtk_seninf_input_update(input, MIPI_RX_CON24_CSI0, > + CSI0_BIST_LN2_MUX, lanes[2]); > + mtk_seninf_input_update(input, MIPI_RX_CON24_CSI0, > + CSI0_BIST_LN3_MUX, lanes[3]); > +} > + > +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 dpcm; > + unsigned int data_lane_num = input->bus.num_data_lanes; `unsigned int num_data_lanes` would be more consistent. > + unsigned int data_header_order = 1; Is this a constant?! If so, why is it not a definition? Also, what does `1` mean? > + unsigned int val = 0; > + > + format = v4l2_subdev_state_get_stream_format(state, input->pad, 0); > + fmtinfo = mtk_seninf_format_info(format->code); > + > + /* Configure timestamp */ > + mtk_seninf_input_write(input, SENINF_TG1_TM_STP, SENINF_TIMESTAMP_STEP); > + > + /* HQ */ > + mtk_seninf_input_write(input, SENINF_TG1_PH_CNT, 0x0); > + mtk_seninf_input_write(input, SENINF_TG1_SEN_CK, 0x10001); > + > + /* 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 */ > + dpcm = fmtinfo->flags & MTK_SENINF_FORMAT_DPCM ? 0x2a : 0; What does 0x2a mean? > + val = 1 << ((dpcm == 0x2a) ? 15 : ((dpcm & 0xF) + 7)); This is BIT(something); > + mtk_seninf_input_write(input, SENINF_CSI2_DPCM, val); > + > + /* Settle delay */ > + mtk_seninf_input_update(input, SENINF_CSI2_LNRD_TIMING, > + DATA_SETTLE_PARAMETER, SENINF_SETTLE_DELAY); > + > + /* HQ */ > + mtk_seninf_input_write(input, SENINF_CSI2_LNRC_FSM, 0x10); > + > + /* CSI2 control */ > + val = mtk_seninf_input_read(input, SENINF_CSI2_CTL) > + | (data_header_order << 16) | 0x10 | ((1 << data_lane_num) - 1); What is 0x10? Also, (BIT(data_lane_num) - 1). > + mtk_seninf_input_write(input, SENINF_CSI2_CTL, val); > + > + mtk_seninf_input_update(input, SENINF_CSI2_RESYNC_MERGE_CTL, > + BYPASS_LANE_RESYNC, 0); > + 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 */ > + mtk_seninf_input_write(input, SENINF_CSI2_SPARE0, 0xfffffffe); > + > + /* Enable CSI2 IRQ mask */ > + /* Turn on all interrupt */ > + mtk_seninf_input_write(input, SENINF_CSI2_INT_EN, 0xffffffff); > + /* Write clear CSI2 IRQ */ > + mtk_seninf_input_write(input, SENINF_CSI2_INT_STATUS, 0xffffffff); > + /* Enable CSI2 Extend IRQ mask */ > + /* Turn on all interrupt */ > + mtk_seninf_input_update(input, SENINF_CTRL, CSI2_SW_RST, 1); > + udelay(1); > + mtk_seninf_input_update(input, SENINF_CTRL, CSI2_SW_RST, 0); Is there any way to check if the CSI port did reset, or is it *guaranteed* to get out of reset in a microsecond after deasserting SW_RST? > +} > + > +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 pixel_mode = TWO_PIXEL_MODE; > + unsigned int val; > + > + 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); Fits in one line. > + mtk_seninf_mux_update(mux, SENINF_MUX_CTRL_EXT, > + SENINF_SRC_SEL_EXT, SENINF_NORMAL_MODEL); This one too, it's 98 cols, still acceptable. > + > + switch (pixel_mode) { You're forcing TWO_PIXEL_MODE but then you have a switch here: I get the point, as in I get that you expect this to be extended to have that read from somewhere, but only then will this switch make sense. So, you could either remove this switch or actually introduce some logic that will make `pixel_mode` variable; I'd be happy if you go for the second option. > + case 1: /* 2 Pixel */ Besides, `case 1` is from enum pixel_mode, so please use the definitions instead of magic numbers. > + pix_sel_ext = 0; > + pix_sel = 1; > + break; > + case 2: /* 4 Pixel */ > + pix_sel_ext = 1; > + pix_sel = 0; > + break; > + default: /* 1 Pixel */ > + pix_sel_ext = 0; > + pix_sel = 0; > + break; > + } > + > + mtk_seninf_mux_update(mux, SENINF_MUX_CTRL_EXT, > + SENINF_PIX_SEL_EXT, pix_sel_ext); one line. > + mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_PIX_SEL, pix_sel); > + > + if (!(fmtinfo->flags & MTK_SENINF_FORMAT_JPEG)) { Logically, inverting this branch to if (fmtinfo->flags & MTK_SENINF_FORMAT_JPEG) { ... } else { ... } would be better imo. > + mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FULL_WR_EN, 2); > + mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FLUSH_EN, 0x1b); > + mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_PUSH_EN, 0x1f); Also, can you please add definitions for those FIFO_xxxx_EN values? > + } else { > + mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FULL_WR_EN, 0); > + mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FLUSH_EN, 0x18); > + mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_PUSH_EN, 0x1e); > + } > + > + 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; writel(mux->base + SENINF_MUX_CTRL, val | rst_mask); writel(mux->base + SENINFMUX_CTRL, val & ~rst_mask); that's better, right? :-) > + mtk_seninf_mux_write(mux, SENINF_MUX_CTRL, val | > + SENINF_MUX_CTRL_SENINF_IRQ_SW_RST | > + SENINF_MUX_CTRL_SENINF_MUX_SW_RST); > + mtk_seninf_mux_write(mux, SENINF_MUX_CTRL, val & > + ~(SENINF_MUX_CTRL_SENINF_IRQ_SW_RST | > + SENINF_MUX_CTRL_SENINF_MUX_SW_RST)); > + > + /* HQ */ > + mtk_seninf_mux_write(mux, SENINF_MUX_SPARE, 0xc2000); Again, what does 0xc2000 mean?! > +} > + > +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 = mtk_seninf_read(priv, SENINF_TOP_MUX_CTRL) > + & ~(0xf << (mux->mux_id * 4)); > + val |= (seninf_id & 0xf) << (mux->mux_id * 4); > + mtk_seninf_write(priv, SENINF_TOP_MUX_CTRL, val); > + > + mtk_seninf_write(priv, SENINF_TOP_CAM_MUX_CTRL, 0x76541010); > +} > + > +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 pixel_mode = TWO_PIXEL_MODE; > + 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 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); > + #define SENINF_TG1_TM_HEIGHT GENMASK(31, 16) #define SENINF_TG1_TM_WIDTH GENMASK(15, 0) size = FIELD_PREP(SENINF_TG1_TM_HEIGHT, (format->height + 8)); switch ... { case something: size |= FIELD_PREP(SENINF_TG1_TM_WIDTH, format->width * 2); break; case default: size |= FIELD_PREP(SENINF_TG1_TM_WIDTH, format->width); break; } writel(the_base + the_reg, size); :-) :-) :-) :-) > + 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: > + mtk_seninf_input_write(input, SENINF_TG1_TM_SIZE, > + (format->height + 8) << 16 | > + format->width * 2); > + break; > + default: > + mtk_seninf_input_write(input, SENINF_TG1_TM_SIZE, > + (format->height + 8) << 16 | > + format->width); > + break; > + } > + > + mtk_seninf_input_write(input, SENINF_TG1_TM_CLK, 0x8); Magic numbers again... > + mtk_seninf_input_write(input, SENINF_TG1_TM_STP, 0x1); > + > + /* Set top mux */ > + val = (mtk_seninf_read(priv, SENINF_TOP_MUX_CTRL) & (~(0xf << (mux_id * 4)))) | > + ((seninf & 0xf) << (mux_id * 4)); > + mtk_seninf_write(priv, SENINF_TOP_MUX_CTRL, val); > + > + 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); > + > + switch (pixel_mode) { > + case 1: > + pix_sel_ext = 0; > + pix_sel = 1; > + break; > + case 2: > + pix_sel_ext = 1; > + pix_sel = 0; > + break; > + default: > + pix_sel_ext = 0; > + pix_sel = 0; > + break; > + } > + > + 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 Please, check this :-) :-) > + mtk_seninf_write(priv, SENINF_TOP_CAM_MUX_CTRL, 0x76540010); > + > + dev_dbg(priv->dev, "%s: OK\n", __func__); > +} > + > +static void mtk_seninf_start(struct mtk_seninf *priv, > + struct v4l2_subdev_state *state, > + struct mtk_seninf_input *input, > + struct mtk_seninf_mux *mux) > +{ > + phy_power_on(input->phy); > + > + mtk_seninf_input_setup_csi2_rx(input); > + mtk_seninf_input_setup_csi2(input, state); > + > + mtk_seninf_mux_setup(mux, input, state); > + mtk_seninf_top_mux_setup(priv, input->seninf_id, mux); > +} > + > +static void mtk_seninf_stop(struct mtk_seninf *priv, > + struct mtk_seninf_input *input) > +{ > + unsigned int val; > + > + /* Disable CSI2(2.5G) first */ > + val = mtk_seninf_input_read(input, SENINF_CSI2_CTL); > + val &= ~(SENINF_CSI2_CTL_CLOCK_LANE_EN | > + SENINF_CSI2_CTL_DATA_LANE3_EN | > + SENINF_CSI2_CTL_DATA_LANE2_EN | > + SENINF_CSI2_CTL_DATA_LANE1_EN | > + SENINF_CSI2_CTL_DATA_LANE0_EN); > + mtk_seninf_input_write(input, SENINF_CSI2_CTL, val); > + > + if (!priv->is_testmode) > + phy_power_off(input->phy); > +} > + ..snip.. > +/* ----------------------------------------------------------------------------- > + * V4L2 Subdev Operations > + */ > +static int seninf_s_stream(struct v4l2_subdev *sd, unsigned int source_pad, > + int on) > +{ > + struct mtk_seninf *priv = sd_to_mtk_seninf(sd); > + struct v4l2_subdev_state *state; > + struct mtk_seninf_input *input; > + struct mtk_seninf_mux *mux; > + struct v4l2_subdev *source; > + u32 sink_pad; > + int ret; > + > + /* Stream control can only operate on source pads. */ > + if (source_pad < priv->conf->nb_inputs || > + source_pad >= priv->conf->nb_inputs + priv->conf->nb_outputs) > + return -EINVAL; > + > + /* > + * Locate the SENINF input and MUX for the source pad. > + * > + * Hardcode a 1:1 mapping of MUX instances to SENINF > + * outputs to match the TOP_CAM_MUX configuration in > + * mtk_seninf_top_mux_setup(). > + */ > + state = v4l2_subdev_lock_and_get_active_state(&priv->subdev); > + if (!state) > + return -EPIPE; > + > + ret = v4l2_subdev_routing_find_opposite_end(&state->routing, source_pad, 0, &sink_pad, > + NULL); > + if (ret) { > + dev_dbg(priv->dev, "No sink pad routed to source pad %u\n", > + source_pad); one line please... > + goto unlock; > + } > + ..snip.. > + > +static int seninf_set_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *fmt) > +{ > + struct mtk_seninf *priv = sd_to_mtk_seninf(sd); > + const struct mtk_seninf_format_info *fmtinfo; > + struct v4l2_mbus_framefmt *format; > + > + /* > + * TODO (?): We should disallow setting formats on the source pad > + * completely, as the SENINF can't perform any processing. This would > + * however break usage of the test pattern generator, as there would be > + * no way to configure formats at all when no active input is selected. > + */ It's either a TODO or not... but whatever, I imagine that the difficulty here is that `priv->is_testmode` may become `true` after .set_fmt(), right? > + > + /* > + * Default to the first format if the requested media bus code isn't > + * supported. > + */ > + fmtinfo = mtk_seninf_format_info(fmt->format.code); > + if (!fmtinfo) { > + fmtinfo = &mtk_seninf_formats[0]; > + fmt->format.code = fmtinfo->code; > + } > + > + /* Interlaced formats are not supported yet. */ > + fmt->format.field = V4L2_FIELD_NONE; > + > + /* Store the format. */ > + Drop extra blank line please. > + format = v4l2_subdev_state_get_stream_format(state, fmt->pad, fmt->stream); > + if (!format) > + return -EINVAL; > + > + *format = fmt->format; > + > + if (mtk_seninf_pad_is_source(priv, fmt->pad)) > + return 0; > + > + /* Propagate the format to the corresponding source pad. */ > + format = v4l2_subdev_state_get_opposite_stream_format(state, fmt->pad, > + fmt->stream); > + if (!format) > + return -EINVAL; > + > + *format = fmt->format; > + > + return 0; > +} > + ..snip.. > + > +static int mtk_seninf_fwnode_parse(struct device *dev, > + unsigned int id) > + > +{ > + static const char * const phy_names[] = { "csi0", "csi1", "csi2", "csi0b"}; > + Please remove the extra blank line. > + struct mtk_seninf *priv = dev_get_drvdata(dev); > + struct mtk_seninf_async_subdev *s_asd; > + unsigned int port; Move this before `ret`: makes it a little more readable. > + struct mtk_seninf_input *input; > + struct fwnode_handle *ep, *fwnode; > + struct v4l2_fwnode_endpoint vep = { > + .bus_type = V4L2_MBUS_CSI2_DPHY, > + }; > + struct v4l2_async_subdev *asd; > + int ret; static const char * const phy_names[] = { "csi0", "csi1", "csi2", "csi0b" }; struct mtk_seninf *priv = dev_get_drvdata(dev); struct mtk_seninf_async_subdev *s_asd; struct fwnode_handle *ep, *fwnode; struct mtk_seninf_input *input; struct v4l2_async_subdev *asd; struct v4l2_fwnode_endpoint vep = { .bus_type = V4L2_MBUS_CSI2_DPHY, }; unsigned int port; int ret; > + > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), id, 0, 0); > + if (!ep) > + return 0; Are you sure that this should really be `return 0`? > + > + fwnode = fwnode_graph_get_remote_endpoint(ep); > + ret = v4l2_fwnode_endpoint_parse(ep, &vep); > + if (ret) { > + dev_err(dev, "Failed to parse %pOF\n", to_of_node(fwnode)); > + ret = -EINVAL; > + goto out; > + } > + > + asd = v4l2_async_nf_add_fwnode(&priv->notifier, > + fwnode, struct v4l2_async_subdev); > + if (IS_ERR(asd)) { > + ret = PTR_ERR(asd); > + goto out; > + } > + s_asd = container_of(asd, struct mtk_seninf_async_subdev, asd); > + > + port = vep.base.port; > + s_asd->port = port; > + > + if (mtk_seninf_pad_is_source(priv, port)) > + return 0; hmmm... no fwnode_handle_put()!? > + > + if (vep.bus_type != V4L2_MBUS_CSI2_DPHY) { > + dev_err(dev, "Only CSI2 bus type is currently supported\n"); > + ret = -EINVAL; > + goto out; > + } > + > + input = &priv->inputs[port]; > + > + input->pad = port; > + input->seninf_id = port_to_seninf_id[port]; > + input->base = priv->base + 0x1000 * input->seninf_id; > + input->seninf = priv; > + > + input->bus = vep.bus.mipi_csi2; > + > + input->phy = devm_phy_get(dev, phy_names[port]); > + if (IS_ERR(input->phy)) { > + dev_err(dev, "failed to get phy:%ld\n", PTR_ERR(input->phy)); > + ret = PTR_ERR(input->phy); > + goto out; > + } > + input->phy_mode = SENINF_PHY_MODE_4D1C; > + > + s_asd->input = input; > + > + ret = 0; Honestly, I don't like that `ret = 0`, but I admit that this is the cleanest option that we can have here, so it's fine. > +out: > + fwnode_handle_put(ep); > + fwnode_handle_put(fwnode); > + return ret; > +} > + > +static int mtk_seninf_notifier_bound(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *sd, > + struct v4l2_async_subdev *asd) > +{ > + struct mtk_seninf *priv = container_of(notifier, struct mtk_seninf, notifier); > + struct mtk_seninf_async_subdev *s_asd = > + container_of(asd, struct mtk_seninf_async_subdev, asd); > + struct device_link *link; > + int ret; > + > + dev_dbg(priv->dev, "%s bound to SENINF port %u\n", sd->entity.name, > + s_asd->port); one line. > + > + if (mtk_seninf_pad_is_sink(priv, s_asd->port)) { > + struct mtk_seninf_input *input = s_asd->input; > + > + input->source_sd = sd; > + > + link = device_link_add(priv->dev, sd->dev, > + DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS); > + if (!link) { > + dev_err(priv->dev, > + "Failed to create device link from source %s\n", sd->name); > + return -EINVAL; > + } > + > + ret = v4l2_create_fwnode_links_to_pad(sd, &priv->pads[input->pad], > + MEDIA_LNK_FL_IMMUTABLE | > + MEDIA_LNK_FL_ENABLED); > + } else { > + link = device_link_add(sd->dev, priv->dev, > + DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS); > + if (!link) { > + dev_err(priv->dev, > + "Failed to create device link to output %s\n", sd->name); > + return -EINVAL; > + } > + > + ret = v4l2_create_fwnode_links_to_pad(&priv->subdev, > + &sd->entity.pads[0], > + MEDIA_LNK_FL_IMMUTABLE | > + MEDIA_LNK_FL_ENABLED); > + } > + if (ret) > + dev_err(priv->dev, "Failed to create links between SENINF port %u and %s (%d)\n", > + s_asd->port, sd->entity.name, ret); if (ret) { dev_err(...) return ret; } return 0; > + > + return ret; > +} > + > +static int mtk_seninf_notifier_complete(struct v4l2_async_notifier *notifier) > +{ > + struct mtk_seninf *priv = container_of(notifier, struct mtk_seninf, notifier); > + int ret; > + > + ret = v4l2_device_register_subdev_nodes(&priv->v4l2_dev); > + if (ret) > + dev_err(priv->dev, "Failed to register subdev nodes: %d\n", ret); if (ret) { dev_err.... return ret; } return 0; > + > + return ret; > +} > + > +static const struct v4l2_async_notifier_operations mtk_seninf_async_ops = { > + .bound = mtk_seninf_notifier_bound, > + .complete = mtk_seninf_notifier_complete, > +}; > + > +/* ----------------------------------------------------------------------------- > + * Probe & Remove > + */ I don't think that we need such comments, btw. > + > +static int mtk_seninf_media_init(struct mtk_seninf *priv) > +{ > + struct media_pad *pads = priv->pads; > + const struct mtk_seninf_conf *conf = priv->conf; > + struct device *dev = priv->dev; > + struct media_device *media_dev = &priv->media_dev; > + u8 num_pads = conf->nb_outputs + conf->nb_inputs; Some reordering would be good for readability here. > + unsigned int i; > + int ret; > + > + media_dev->dev = dev; > + strscpy(media_dev->model, conf->model, sizeof(media_dev->model)); > + snprintf(media_dev->bus_info, sizeof(media_dev->bus_info), > + "platform:%s", dev_name(dev)); > + media_dev->hw_revision = 0; > + media_device_init(media_dev); > + > + ret = media_entity_pads_init(&priv->subdev.entity, num_pads, pads); > + if (ret) > + goto err_clean_media; > + > + for (i = 0; i < conf->nb_inputs; i++) > + pads[i].flags = MEDIA_PAD_FL_SINK; > + for (i = conf->nb_inputs; i < num_pads; i++) > + pads[i].flags = MEDIA_PAD_FL_SOURCE; > + > + return 0; > +err_clean_media: > + media_device_cleanup(media_dev); > + > + return ret; > +} > + > +static int mtk_seninf_v4l2_async_register(struct mtk_seninf *priv) > +{ > + struct device *dev = priv->dev; > + const struct mtk_seninf_conf *conf = priv->conf; > + int ret; > + unsigned int i; const struct mtk_seninf_conf *conf = priv->conf; struct device *dev = priv->dev; unsigned int i; int ret; > + > + v4l2_async_nf_init(&priv->notifier); > + > + for (i = 0; i < conf->nb_inputs + conf->nb_outputs; ++i) { > + ret = mtk_seninf_fwnode_parse(dev, i); > + > + if (ret) { > + dev_err(dev, "Failed to parse endpoint at port %d, err: %d\n", i, ret); > + goto err_clean_notififer; > + } > + } > + > + priv->notifier.ops = &mtk_seninf_async_ops; > + ret = v4l2_async_nf_register(&priv->v4l2_dev, &priv->notifier); > + if (ret) > + dev_err(dev, "Failed to register async notifier: %d\n", ret); So you don't care about this failure?! :-) > + return 0; > + > +err_clean_notififer: > + v4l2_async_nf_cleanup(&priv->notifier); > + > + return ret; > +} > + > +static int mtk_seninf_v4l2_register(struct mtk_seninf *priv) > +{ > + struct v4l2_subdev *sd = &priv->subdev; > + struct device *dev = priv->dev; > + int ret; > + > + /* Initialize media device & pads. */ > + ret = mtk_seninf_media_init(priv); > + if (ret) > + return ret; > + > + /* Initialize & register v4l2 device. */ > + priv->v4l2_dev.mdev = &priv->media_dev; > + > + ret = v4l2_device_register(dev, &priv->v4l2_dev); > + if (ret) { > + dev_err(dev, "Failed to register V4L2 device: %d\n", ret); You can still use dev_err_probe, even if you're not returning: this would make sure that the log format is consistent for at least all probe functions :-) > + goto err_clean_media; > + } > + > + /* Initialize & register subdev. */ > + v4l2_subdev_init(sd, &seninf_subdev_ops); > + sd->dev = dev; > + sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > + sd->entity.ops = &seninf_media_ops; > + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS > + | V4L2_SUBDEV_FL_STREAMS; > + strscpy(sd->name, dev_name(dev), V4L2_SUBDEV_NAME_SIZE); > + ret = seninf_initialize_controls(priv); > + if (ret) { > + dev_err(dev, "Failed to initialize controls: %d\n", ret); > + goto err_unreg_v4l2; > + } > + v4l2_set_subdevdata(sd, priv); > + > + ret = v4l2_subdev_init_finalize(sd); > + if (ret) > + goto err_free_handler; > + > + ret = v4l2_device_register_subdev(&priv->v4l2_dev, sd); > + if (ret) { > + dev_err(dev, "Failed to register subdev: %d\n", ret); > + goto err_cleanup_subdev; > + } > + > + /* Set up async device */ > + ret = mtk_seninf_v4l2_async_register(priv); > + if (ret) { > + dev_err(dev, "Failed to register v4l2 async notifier: %d\n", ret); > + goto err_unreg_subdev; > + } > + > + /* Register media device */ > + ret = media_device_register(&priv->media_dev); > + if (ret) { > + dev_err(dev, "failed to register media device: %d\n", ret); > + goto err_unreg_notifier; > + } > + > + return 0; > + > +err_unreg_notifier: > + v4l2_async_nf_unregister(&priv->notifier); > +err_unreg_subdev: > + v4l2_device_unregister_subdev(sd); > +err_cleanup_subdev: > + v4l2_subdev_cleanup(sd); > +err_free_handler: > + v4l2_ctrl_handler_free(&priv->ctrl_handler); > +err_unreg_v4l2: > + v4l2_device_unregister(&priv->v4l2_dev); > +err_clean_media: > + media_entity_cleanup(&sd->entity); > + media_device_cleanup(&priv->media_dev); > + > + return ret; > +} > + > +static int seninf_probe(struct platform_device *pdev) > +{ > + /* List of clocks required by seninf. */ > + static const char * const clk_names[] = { > + "camsys", "top_mux" > + }; Fits in one line; drop extra blank line. > + > + struct mtk_seninf *priv; > + struct device *dev = &pdev->dev; reordering: dev before priv > + unsigned int i; > + int ret; > + > + priv = devm_kzalloc(dev, sizeof(struct mtk_seninf), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->conf = of_device_get_match_data(dev); > + > + dev_set_drvdata(dev, priv); > + priv->dev = dev; > + > + priv->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(priv->base)) > + return PTR_ERR(priv->base); > + > + priv->num_clks = ARRAY_SIZE(clk_names); > + priv->clks = devm_kcalloc(dev, priv->num_clks, > + sizeof(*priv->clks), GFP_KERNEL); > + if (!priv->clks) > + return -ENOMEM; > + > + for (i = 0; i < priv->num_clks; ++i) > + priv->clks[i].id = clk_names[i]; > + > + ret = devm_clk_bulk_get(dev, priv->num_clks, priv->clks); > + if (ret) { > + dev_err(dev, "failed to get seninf clock:%d\n", ret); > + return ret; > + } > + > + for (i = 0; i < priv->conf->nb_muxes; ++i) { > + struct mtk_seninf_mux *mux = &priv->muxes[i]; > + > + mux->pad = priv->conf->nb_inputs + i; > + mux->mux_id = i; > + mux->base = priv->base + 0x1000 * i; > + mux->seninf = priv; > + } > + > + ret = mtk_seninf_v4l2_register(priv); if (ret) return ret; pm_runtime_enable(dev); (P.S.: Why not devm_pm_runtime_enable()?) return 0; } > + if (!ret) > + pm_runtime_enable(dev); > + > + return ret; > +} > + > +static int seninf_pm_suspend(struct device *dev) > +{ > + struct mtk_seninf *priv = dev_get_drvdata(dev); > + > + dev_dbg(dev, "seninf runtime suspend\n"); > + clk_bulk_disable_unprepare(priv->num_clks, priv->clks); > + > + return 0; > +} > + > +static int seninf_pm_resume(struct device *dev) > +{ > + struct mtk_seninf *priv = dev_get_drvdata(dev); > + int ret; > + > + dev_dbg(dev, "seninf runtime resume\n"); > + ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks); > + if (ret) { > + dev_err(dev, "failed to enable clock:%d\n", ret); > + return ret; > + } > + > + mtk_seninf_csi2_setup_phy(priv); > + > + return 0; > +} > + > +static const struct dev_pm_ops runtime_pm_ops = { > + SET_RUNTIME_PM_OPS(seninf_pm_suspend, seninf_pm_resume, NULL) > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) Fits in one line. > +}; > + > +static int seninf_remove(struct platform_device *pdev) > +{ > + struct mtk_seninf *priv = dev_get_drvdata(&pdev->dev); > + > + media_device_unregister(&priv->media_dev); > + media_device_cleanup(&priv->media_dev); > + v4l2_async_nf_unregister(&priv->notifier); > + v4l2_async_nf_cleanup(&priv->notifier); > + v4l2_device_unregister_subdev(&priv->subdev); > + v4l2_subdev_cleanup(&priv->subdev); > + v4l2_ctrl_handler_free(&priv->ctrl_handler); > + media_entity_cleanup(&priv->subdev.entity); > + v4l2_device_unregister(&priv->v4l2_dev); > + > + pm_runtime_disable(priv->dev); > + > + return 0; > +} > + > +static const struct mtk_seninf_conf seninf_8365_conf = { > + .model = "mtk-camsys-3.0", > + .nb_inputs = 4, > + .nb_muxes = 6, > + .nb_outputs = 4, > +}; > + > +static const struct of_device_id mtk_seninf_of_match[] = { > + { > + .compatible = "mediatek,mt8365-seninf", > + .data = &seninf_8365_conf, This fits in one line: { .compatible = "mediatek,mt8365-seninf", .data = &seninf_8365_conf } > + }, > + { Please always end of_device_id lists with { /* sentinel */ } > + }, > +}; > +MODULE_DEVICE_TABLE(of, mtk_seninf_of_match); > + > +static struct platform_driver seninf_pdrv = { > + .driver = { > + .name = "mtk-seninf", Either tabs or one space please. > + .pm = &runtime_pm_ops, > + .of_match_table = mtk_seninf_of_match, > + }, > + .probe = seninf_probe, > + .remove = seninf_remove, > +}; > + > +module_platform_driver(seninf_pdrv); > + > +MODULE_DESCRIPTION("MTK sensor interface driver"); > +MODULE_AUTHOR("Louis Kuo <louis.kuo@mediatek.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/media/platform/mediatek/isp/isp_30/seninf/mtk_seninf_reg.h b/drivers/media/platform/mediatek/isp/isp_30/seninf/mtk_seninf_reg.h > new file mode 100644 > index 000000000000..b6af8567fa20 > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_30/seninf/mtk_seninf_reg.h > @@ -0,0 +1,99 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2022 MediaTek Inc. > + */ > + > +#ifndef __SENINF_REG_H__ > +#define __SENINF_REG_H__ > + > +#include <linux/bits.h> > + > +#define SENINF_TOP_CTRL 0x0000 > +#define SENINF_TOP_CTRL_MUX_LP_MODE BIT(31) ..snip.. > +#define SENINF_MUX_CTRL_SENINF_MUX_EN BIT(31) > +#define SENINF_MUX_INTEN 0x0d04 > +#define SENINF_MUX_SPARE 0x0d2c > +#define SENINF_MUX_CTRL_EXT 0x0d3c > +#define SENINF_MUX_CTRL_EXT_SENINF_SRC_SEL_EXT GENMASK(1, 0) > +#define SENINF_MUX_CTRL_EXT_SENINF_PIX_SEL_EXT BIT(4) #endif /* __SENINF_REG_H__ */ > +#endif Regards, Angelo
On Sun, Jul 02, 2023 at 05:20:25PM +0800, Chun-Kuang Hu wrote: > Chun-Kuang Hu <chunkuang.hu@kernel.org> 於 2023年7月2日 週日 上午7:35寫道： > > Julien Stephan <jstephan@baylibre.com> 於 2023年6月30日 週五 下午6:05寫道： > > > > > > 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> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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, > > > +}; > > > + > > > +static const struct of_device_id mtk_seninf_of_match[] = { > > > + { > > > + .compatible = "mediatek,mt8365-seninf", > > > + .data = &seninf_8365_conf, > > > > Now only support one SoC, so it's necessary to select the SoC data and > > you could directly place the data in the code. After support other > > Typo. > > Now only support one SoC, so it's not necessary to select the SoC data > and you could directly place the data in the code. I think Julien has visibility on other SoCs that could use the same driver, and has designed the mtk_seninf_conf structure accordingly. I'll let Julien confirm (or tell I'm wrong). > > SoC, so we could know what should be placed in struct mtg_seninf_conf > > (Now we have no any information). > > > > > + }, > > > + { > > > + }, > > > +}; > > > +MODULE_DEVICE_TABLE(of, mtk_seninf_of_match);
Il 30/06/23 12:01, Julien Stephan ha scritto: > From: Phi-bang Nguyen <pnguyen@baylibre.com> > > This driver provides a path to bypass the SoC ISP so that image data > coming from the SENINF can go directly into memory without any image > processing. This allows the use of an external ISP. > > Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com> > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > --- > MAINTAINERS | 1 + > .../platform/mediatek/isp/isp_30/Kconfig | 19 + > .../platform/mediatek/isp/isp_30/Makefile | 1 + > .../mediatek/isp/isp_30/camsv/Makefile | 7 + > .../mediatek/isp/isp_30/camsv/mtk_camsv.c | 334 ++++++++ > .../mediatek/isp/isp_30/camsv/mtk_camsv.h | 200 +++++ > .../isp/isp_30/camsv/mtk_camsv30_hw.c | 474 +++++++++++ > .../isp/isp_30/camsv/mtk_camsv30_regs.h | 59 ++ > .../isp/isp_30/camsv/mtk_camsv_video.c | 781 ++++++++++++++++++ > 9 files changed, 1876 insertions(+) > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/Makefile > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.c > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_hw.c > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_regs.h > create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv_video.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 60b2bceca584..cebb8fb62b45 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13357,6 +13357,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/camsv/* > F: drivers/media/platform/mediatek/isp/isp_30/seninf/* > > MEDIATEK SMI DRIVER > diff --git a/drivers/media/platform/mediatek/isp/isp_30/Kconfig b/drivers/media/platform/mediatek/isp/isp_30/Kconfig > index 831f9bee5fdd..04d4a2af80bc 100644 > --- a/drivers/media/platform/mediatek/isp/isp_30/Kconfig > +++ b/drivers/media/platform/mediatek/isp/isp_30/Kconfig > @@ -14,3 +14,22 @@ config MTK_SENINF > > To compile this driver as a module, choose M here: the > module will be called mtk-seninf. > + > +config MTK_CAMSV30 > + tristate "MediaTek ISP3.0 CAMSV driver" > + depends on VIDEO_V4L2_SUBDEV_API > + depends on ARCH_MEDIATEK || COMPILE_TEST > + depends on OF > + depends on PM > + select VIDEOBUF2_VMALLOC > + select VIDEOBUF2_DMA_CONTIG > + select MTK_SENINF > + select PHY_MTK_MIPI_CSI_0_5 > + default n > + help > + This driver provides a path to bypass the SoC ISP so that > + image data come from the SENINF can go directly into memory > + without any image processing. > + > + To compile this driver as a module, choose M here: the > + module will be called mtk-camsv30. > diff --git a/drivers/media/platform/mediatek/isp/isp_30/Makefile b/drivers/media/platform/mediatek/isp/isp_30/Makefile > index 2e7d731d42b7..c0034ef31702 100644 > --- a/drivers/media/platform/mediatek/isp/isp_30/Makefile > +++ b/drivers/media/platform/mediatek/isp/isp_30/Makefile > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0 > > obj-$(CONFIG_MTK_SENINF) += seninf/ > +obj-$(CONFIG_MTK_CAMSV30) += camsv/ > diff --git a/drivers/media/platform/mediatek/isp/isp_30/camsv/Makefile b/drivers/media/platform/mediatek/isp/isp_30/camsv/Makefile > new file mode 100644 > index 000000000000..fffbc6e7cb78 > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_30/camsv/Makefile > @@ -0,0 +1,7 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +mtk-camsv30-objs += mtk_camsv.o > +mtk-camsv30-objs += mtk_camsv30_hw.o > +mtk-camsv30-objs += mtk_camsv_video.o > + > +obj-$(CONFIG_MTK_CAMSV30) += mtk-camsv30.o > diff --git a/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.c b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.c > new file mode 100644 > index 000000000000..03d234f54e1c > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.c > @@ -0,0 +1,334 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2020 BayLibre > + * Copyright (c) 2022 MediaTek Inc. > + */ > + > +#include <media/v4l2-async.h> > +#include <media/v4l2-device.h> > + > +#include "mtk_camsv.h" > + > +static inline struct mtk_cam_dev *to_mtk_cam_dev(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct mtk_cam_dev, subdev); > +} > + > +static const u32 mtk_cam_mbus_formats[] = { > + MEDIA_BUS_FMT_SBGGR8_1X8, > + MEDIA_BUS_FMT_SGBRG8_1X8, > + MEDIA_BUS_FMT_SGRBG8_1X8, > + MEDIA_BUS_FMT_SRGGB8_1X8, > + MEDIA_BUS_FMT_SBGGR10_1X10, > + MEDIA_BUS_FMT_SGBRG10_1X10, > + MEDIA_BUS_FMT_SGRBG10_1X10, > + MEDIA_BUS_FMT_SRGGB10_1X10, > + MEDIA_BUS_FMT_SBGGR12_1X12, > + MEDIA_BUS_FMT_SGBRG12_1X12, > + MEDIA_BUS_FMT_SGRBG12_1X12, > + MEDIA_BUS_FMT_SRGGB12_1X12, > + MEDIA_BUS_FMT_UYVY8_1X16, > + MEDIA_BUS_FMT_VYUY8_1X16, > + MEDIA_BUS_FMT_YUYV8_1X16, > + MEDIA_BUS_FMT_YVYU8_1X16, > +}; > + > +/* ----------------------------------------------------------------------------- > + * V4L2 Subdev Operations > + */ > + > +static int mtk_cam_cio_stream_on(struct mtk_cam_dev *cam) > +{ > + struct device *dev = cam->dev; > + struct v4l2_subdev *seninf; > + int ret; > + > + if (!cam->seninf) { > + cam->seninf = media_pad_remote_pad_first(&cam->subdev_pads[MTK_CAM_CIO_PAD_SENINF]); > + if (!cam->seninf) { > + dev_err(dev, "%s: No SENINF connected\n", __func__); > + return -ENOLINK; > + } > + } > + > + seninf = media_entity_to_v4l2_subdev(cam->seninf->entity); > + > + /* Seninf must stream on first */ > + ret = v4l2_subdev_call(seninf, pad, enable_streams, NULL, cam->seninf->index, 0); > + if (ret) { > + dev_err(dev, "failed to stream on %s:%d\n", > + seninf->entity.name, ret); > + return ret; > + } > + > + cam->streaming = true; > + > + return 0; > +} > + > +static int mtk_cam_cio_stream_off(struct mtk_cam_dev *cam) > +{ > + struct device *dev = cam->dev; You're using `dev` only once, might as well drop it and just reference to cam->dev directly for that dev_err() call... > + struct v4l2_subdev *seninf; > + int ret; > + > + if (cam->seninf) { if (cam->seninf) { struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(...); > + seninf = media_entity_to_v4l2_subdev(cam->seninf->entity); > + > + ret = v4l2_subdev_call(seninf, pad, disable_streams, NULL, > + cam->seninf->index, 0); > + if (ret) { > + dev_err(dev, "failed to stream off %s:%d\n", > + seninf->entity.name, ret); > + return ret; > + } > + } > + > + cam->streaming = false; > + > + return 0; > +} > + ..snip.. > +static int mtk_cam_v4l2_register(struct mtk_cam_dev *cam) > +{ > + struct device *dev = cam->dev; > + int ret; > + > + /* Initialize subdev pads */ > + ret = media_entity_pads_init(&cam->subdev.entity, > + ARRAY_SIZE(cam->subdev_pads), > + cam->subdev_pads); > + if (ret) { > + dev_err(dev, "failed to initialize media pads:%d\n", ret); > + return ret; > + } > + > + cam->subdev_pads[MTK_CAM_CIO_PAD_SENINF].flags = MEDIA_PAD_FL_SINK; > + cam->subdev_pads[MTK_CAM_CIO_PAD_VIDEO].flags = MEDIA_PAD_FL_SOURCE; > + > + /* Initialize subdev */ > + v4l2_subdev_init(&cam->subdev, &mtk_cam_subdev_ops); > + > + cam->subdev.dev = dev; > + cam->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; > + cam->subdev.entity.ops = &mtk_cam_media_entity_ops; > + cam->subdev.internal_ops = &mtk_cam_internal_ops; > + cam->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > + strscpy(cam->subdev.name, dev_name(dev), V4L2_SUBDEV_NAME_SIZE); > + v4l2_set_subdevdata(&cam->subdev, cam); > + > + mtk_cam_init_cfg(&cam->subdev, NULL); mtk_cam_init_cfg() returns an integer, and this is probably because you expect it to get logic to fail: in that case, check the result of that call here, otherwise, make that function void. > + > + ret = v4l2_async_register_subdev(&cam->subdev); > + if (ret) { > + dev_err(dev, "failed to initialize subdev:%d\n", ret); > + goto fail_clean_media_entiy; You don't need a goto here, as this the only fail path that requires cleaning up the entity: just do the same here and return ret *here*. > + } > + > + return 0; > + > +fail_clean_media_entiy: > + media_entity_cleanup(&cam->subdev.entity); > + > + return ret; > +} > + > +static void mtk_cam_v4l2_unregister(struct mtk_cam_dev *cam) > +{ > + mtk_cam_video_unregister(&cam->vdev); > + > + media_entity_cleanup(&cam->subdev.entity); > + v4l2_async_unregister_subdev(&cam->subdev); > +} > + > +int mtk_cam_dev_init(struct mtk_cam_dev *cam_dev) > +{ > + int ret; > + > + mutex_init(&cam_dev->op_lock); > + > + /* v4l2 sub-device registration */ > + ret = mtk_cam_v4l2_register(cam_dev); > + if (ret) { > + mutex_destroy(&cam_dev->op_lock); > + return ret; > + } > + > + return ret; return 0; > +} > + > +void mtk_cam_dev_cleanup(struct mtk_cam_dev *cam) > +{ > + mtk_cam_v4l2_unregister(cam); > + mutex_destroy(&cam->op_lock); > +} > diff --git a/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.h b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.h > new file mode 100644 > index 000000000000..91e30fe006a8 > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.h > @@ -0,0 +1,200 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2020 BayLibre > + * Copyright (c) 2022 MediaTek Inc. > + */ > + > +#ifndef __MTK_CAMSV_H__ > +#define __MTK_CAMSV_H__ > + > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/errno.h> > +#include <linux/kernel.h> > +#include <linux/of_graph.h> > +#include <linux/pm_runtime.h> > +#include <linux/videodev2.h> > +#include <media/media-entity.h> > +#include <media/v4l2-subdev.h> > +#include <media/videobuf2-core.h> > +#include <media/videobuf2-dma-contig.h> > +#include <media/videobuf2-v4l2.h> > +#include <soc/mediatek/smi.h> > + > +#define IMG_MAX_WIDTH 5376 > +#define IMG_MAX_HEIGHT 4032 > +#define IMG_DEF_WIDTH 1920 > +#define IMG_DEF_HEIGHT 1080 > +#define IMG_MIN_WIDTH 80 > +#define IMG_MIN_HEIGHT 60 > + > +#define MTK_CAM_CIO_PAD_SENINF 0 > +#define MTK_CAM_CIO_PAD_VIDEO 1 > +#define MTK_CAM_CIO_NUM_PADS 2 > + > +struct mtk_cam_format_info { > + u32 code; > + u32 fourcc; > + bool packed; > + unsigned int bpp; Can we get any format that uses a billion bits per pixel? :-P u8 bpp; > +}; > + > +struct mtk_cam_dev_buffer { > + struct vb2_v4l2_buffer v4l2_buf; > + struct list_head list; > + dma_addr_t daddr; > + dma_addr_t fhaddr; > +}; > + > +struct mtk_cam_sparams { > + unsigned int w_factor; > + unsigned int module_en_pak; > + unsigned int fmt_sel; > + unsigned int pak; > + unsigned int imgo_stride; If this relates to registers somehow, that must be a fixed type; otherwise, if not, I believe you can reduce the size of some of those vars in here. > +}; > + > +/* For kerneldoc, you start with `/**` > + * struct mtk_cam_vdev_desc - MTK camera device descriptor > + * Remove extra blank line. > + * @name: name of the node > + * @cap: supported V4L2 capabilities > + * @buf_type: supported V4L2 buffer type > + * @link_flags: default media link flags > + * @def_width: the default format width > + * @def_height: the default format height > + * @num_fmts: the number of supported node formats > + * @max_buf_count: maximum VB2 buffer count > + * @ioctl_ops: mapped to v4l2_ioctl_ops > + * @fmts: supported format > + * @frmsizes: supported V4L2 frame size number > + * same > + */ > +struct mtk_cam_vdev_desc { > + const char *name; > + u32 cap; > + u32 buf_type; > + u32 link_flags; > + u32 def_width; > + u32 def_height; > + u8 num_fmts; > + u8 max_buf_count; > + const struct v4l2_ioctl_ops *ioctl_ops; > + const u32 *fmts; > + const struct v4l2_frmsizeenum *frmsizes; > +}; > + > +/* /** > + * struct mtk_cam_video_device - MediaTek video device structure > + * and again... please fix all instances of that. > + * @desc: The node description of video device > + * @vdev_pad: The media pad graph object of video device > + * @vdev: The video device instance > + * @vbq: A videobuf queue of video device > + * @vdev_lock: Serializes vb2 queue and video device operations > + * @format: The V4L2 format of video device > + * @fmtinfo: Information about the current format > + */ > +struct mtk_cam_video_device { > + const struct mtk_cam_vdev_desc *desc; > + > + struct media_pad vdev_pad; > + struct video_device vdev; > + struct vb2_queue vbq; > + > + /* Serializes vb2 queue and video device operations */ > + struct mutex vdev_lock; > + > + struct v4l2_pix_format_mplane format; > + const struct mtk_cam_format_info *fmtinfo; > +}; > + > +/* > + * struct mtk_cam_dev - MediaTek camera device structure. > + * > + * @dev: Pointer to device. > + * @regs: Base address of CAMSV. > + * @regs_img0: Base address of CAMSV IMG0. > + * @regs_tg: Base address of CAMSV TG. > + * @num_clks: Number of clocks. > + * @clks: The clocks. > + * @irq: Irq fired when buffer is ready. > + * @conf: soc specific driver data. > + * @pipeline: Media pipeline information. > + * @subdev: The V4L2 sub-device instance. > + * @subdev_pads: Media pads of this sub-device. > + * @formats: Media bus format for all pads. > + * @vdev: The video device node. > + * @seninf: Pointer to the seninf pad. > + * @streaming: Indicate the overall streaming status is on or off. > + * @stream_count: Number of streaming video nodes. > + * @sequence: Buffer sequence number. > + * @op_lock: Serializes driver's VB2 callback operations. > + * @irqlock: Protects the buffer list. > + * @buf_list: List head for the buffer list. > + * @hw_functions: Hardware specific functions. > + * @dummy: Dummy buffer used when user buffer is not available. > + * @dummy_size : Size of the dummy buffer. > + * @is_dummy_used: True if dummy buffer is currently used. > + */ > +struct mtk_cam_dev { > + struct device *dev; > + void __iomem *regs; > + void __iomem *regs_img0; > + void __iomem *regs_tg; > + > + unsigned int num_clks; > + struct clk_bulk_data *clks; > + unsigned int irq; > + const struct mtk_cam_conf *conf; > + > + struct media_pipeline pipeline; > + struct v4l2_subdev subdev; > + struct media_pad subdev_pads[MTK_CAM_CIO_NUM_PADS]; > + struct v4l2_mbus_framefmt formats[MTK_CAM_CIO_NUM_PADS]; > + struct mtk_cam_video_device vdev; > + struct media_pad *seninf; > + unsigned int streaming; > + unsigned int stream_count; > + unsigned int sequence; > + > + struct mutex op_lock; > + spinlock_t irqlock; > + > + struct list_head buf_list; > + > + struct mtk_cam_hw_functions *hw_functions; > + > + struct mtk_cam_dev_buffer dummy; > + unsigned int dummy_size; > + bool is_dummy_used; > +}; > + > +struct mtk_cam_conf { > + unsigned int tg_sen_mode; > + unsigned int module_en; > + unsigned int pak; > + unsigned int dma_special_fun; > + unsigned int imgo_con; > + unsigned int imgo_con2; > + unsigned int imgo_con3; u32? > + bool enableFH; bool frm_hdr_en; > +}; > + > +struct mtk_cam_hw_functions { > + void (*mtk_cam_setup)(struct mtk_cam_dev *cam_dev, u32 width, > + u32 height, u32 bpl, u32 mbus_fmt); > + void (*mtk_cam_update_buffers_add)(struct mtk_cam_dev *cam_dev, > + struct mtk_cam_dev_buffer *buf); > + void (*mtk_cam_cmos_vf_hw_enable)(struct mtk_cam_dev *cam_dev, > + bool pak_en); > + void (*mtk_cam_cmos_vf_hw_disable)(struct mtk_cam_dev *cam_dev, > + bool pak_en); > +}; > + > +int mtk_cam_dev_init(struct mtk_cam_dev *cam_dev); > +void mtk_cam_dev_cleanup(struct mtk_cam_dev *cam_dev); > +int mtk_cam_video_register(struct mtk_cam_dev *cam_dev); > +void mtk_cam_video_unregister(struct mtk_cam_video_device *vdev); > + > +#endif /* __MTK_CAMSV_H__ */ > diff --git a/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_hw.c b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_hw.c > new file mode 100644 > index 000000000000..39fc3ef7b4e7 > --- /dev/null > +++ b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_hw.c > @@ -0,0 +1,474 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2020 BayLibre > + * Copyright (c) 2022 MediaTek Inc. > + */ > + > +#include <linux/device.h> > +#include <linux/interrupt.h> > +#include <linux/iommu.h> > +#include <linux/ktime.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > + > +#include "mtk_camsv.h" > +#include "mtk_camsv30_regs.h" > + > +#define MTK_CAMSV30_AUTOSUSPEND_DELAY_MS 100 > + > +static const struct mtk_cam_conf camsv30_conf = { > + .tg_sen_mode = 0x00010002U, /* TIME_STP_EN = 1. DBL_DATA_BUS = 1 */ > + .module_en = 0x40000001U, /* enable double buffer and TG */ > + .dma_special_fun = 0x61000000U, /* enable RDMA insterlace */ > + .imgo_con = 0x80000080U, /* DMA FIFO depth and burst */ > + .imgo_con2 = 0x00020002U, /* DMA priority */ > + .imgo_con3 = 0x00020002U, /* DMA pre-priority */ > + .enableFH = false, /* Frame Header disabled */ > +}; > + > +static void fmt_to_sparams(u32 mbus_fmt, struct mtk_cam_sparams *sparams) > +{ > + switch (mbus_fmt) { > + /* SBGGR values coming from isp5.0 configuration. > + * not tested on isp2.0 > + */ > + case MEDIA_BUS_FMT_SBGGR12_1X12: > + case MEDIA_BUS_FMT_SGBRG12_1X12: > + case MEDIA_BUS_FMT_SGRBG12_1X12: > + case MEDIA_BUS_FMT_SRGGB12_1X12: > + sparams->w_factor = 1; > + sparams->module_en_pak = 0x4; > + sparams->fmt_sel = 0x2; > + sparams->pak = 0x5; > + sparams->imgo_stride = 0x000B0000; > + break; > + case MEDIA_BUS_FMT_SBGGR10_1X10: > + case MEDIA_BUS_FMT_SGBRG10_1X10: > + case MEDIA_BUS_FMT_SGRBG10_1X10: > + case MEDIA_BUS_FMT_SRGGB10_1X10: > + sparams->w_factor = 1; > + sparams->module_en_pak = 0x4; > + sparams->fmt_sel = 0x1; > + sparams->pak = 0x6; > + sparams->imgo_stride = 0x000B0000; > + break; > + case MEDIA_BUS_FMT_SBGGR8_1X8: > + case MEDIA_BUS_FMT_SGBRG8_1X8: > + case MEDIA_BUS_FMT_SGRBG8_1X8: > + case MEDIA_BUS_FMT_SRGGB8_1X8: > + sparams->w_factor = 1; > + sparams->module_en_pak = 0x4; > + sparams->fmt_sel = 0x0; > + sparams->pak = 0x7; > + sparams->imgo_stride = 0x000B0000; > + break; > + case MEDIA_BUS_FMT_UYVY8_1X16: > + case MEDIA_BUS_FMT_VYUY8_1X16: > + case MEDIA_BUS_FMT_YUYV8_1X16: > + case MEDIA_BUS_FMT_YVYU8_1X16: > + sparams->w_factor = 2; > + sparams->module_en_pak = 0x8; > + sparams->fmt_sel = 0x1000003; > + sparams->pak = 0x0; > + sparams->imgo_stride = 0x00090000; > + break; > + default: > + break; > + } > +} > + > +static u32 mtk_camsv30_read(struct mtk_cam_dev *priv, u32 reg) > +{ > + return readl(priv->regs + reg); Drop those one-liners please, it's just not worth the noise imo. > +} > + > +static void mtk_camsv30_write(struct mtk_cam_dev *priv, u32 reg, u32 value) > +{ > + writel(value, priv->regs + reg); > +} > + > +static void mtk_camsv30_img0_write(struct mtk_cam_dev *priv, u32 reg, u32 value) > +{ > + writel(value, priv->regs_img0 + reg); > +} > + > +static u32 mtk_camsv30_tg_read(struct mtk_cam_dev *priv, u32 reg) > +{ > + return readl(priv->regs_tg + reg); > +} > + > +static void mtk_camsv30_tg_write(struct mtk_cam_dev *priv, u32 reg, u32 value) > +{ > + writel(value, priv->regs_tg + reg); > +} > + ..snip.. > + > +static void mtk_camsv30_setup(struct mtk_cam_dev *cam_dev, u32 w, u32 h, > + u32 bpl, u32 mbus_fmt) > +{ > + const struct mtk_cam_conf *conf = cam_dev->conf; > + int poll_num = 1000; > + u32 int_en = INT_ST_MASK_CAMSV; > + struct mtk_cam_sparams sparams; > + > + fmt_to_sparams(mbus_fmt, &sparams); > + > + spin_lock(&cam_dev->irqlock); > + > + if (pm_runtime_get_sync(cam_dev->dev) < 0) { > + dev_err(cam_dev->dev, "failed to get pm_runtime\n"); > + spin_unlock(&cam_dev->irqlock); > + return; > + } > + > + mtk_camsv30_tg_write(cam_dev, CAMSV_TG_SEN_MODE, conf->tg_sen_mode); > + > + mtk_camsv30_tg_write(cam_dev, > + CAMSV_TG_SEN_GRAB_PXL, (w * sparams.w_factor) << 16U); > + > + mtk_camsv30_tg_write(cam_dev, CAMSV_TG_SEN_GRAB_LIN, h << 16U); > + > + /* YUV_U2S_DIS: disable YUV sensor unsigned to signed */ > + mtk_camsv30_tg_write(cam_dev, CAMSV_TG_PATH_CFG, 0x1000U); > + > + /* Reset cam */ > + mtk_camsv30_write(cam_dev, CAMSV_SW_CTL, CAMSV_SW_RST); > + mtk_camsv30_write(cam_dev, CAMSV_SW_CTL, 0x0U); > + mtk_camsv30_write(cam_dev, CAMSV_SW_CTL, CAMSV_IMGO_RST_TRIG); > + > + while (mtk_camsv30_read(cam_dev, CAMSV_SW_CTL) != > + (CAMSV_IMGO_RST_TRIG | CAMSV_IMGO_RST_ST) && > + poll_num++ < 1000) Decide: this is either # poll_num = 1000; poll_num-- > 0; or # poll_num = 0; poll_num++ < 1000. Besides, however we put it, it's anyway wrong, as you can use common (atomic or not) helpers for polling registers, which is what you should do here :-) > + ; > + > + mtk_camsv30_write(cam_dev, CAMSV_SW_CTL, 0x0U); > + > + mtk_camsv30_write(cam_dev, CAMSV_INT_EN, int_en); > + > + mtk_camsv30_write(cam_dev, CAMSV_MODULE_EN, > + conf->module_en | sparams.module_en_pak); > + mtk_camsv30_write(cam_dev, CAMSV_FMT_SEL, sparams.fmt_sel); > + mtk_camsv30_write(cam_dev, CAMSV_PAK, sparams.pak); > + > + mtk_camsv30_img0_write(cam_dev, CAMSV_IMGO_SV_XSIZE, bpl - 1U); > + mtk_camsv30_img0_write(cam_dev, CAMSV_IMGO_SV_YSIZE, h - 1U); > + > + mtk_camsv30_img0_write(cam_dev, CAMSV_IMGO_SV_STRIDE, sparams.imgo_stride | bpl); > + > + mtk_camsv30_img0_write(cam_dev, CAMSV_IMGO_SV_CON, conf->imgo_con); > + mtk_camsv30_img0_write(cam_dev, CAMSV_IMGO_SV_CON2, conf->imgo_con2); > + > + /* CMOS_EN first */ > + mtk_camsv30_tg_write(cam_dev, CAMSV_TG_SEN_MODE, > + mtk_camsv30_tg_read(cam_dev, CAMSV_TG_SEN_MODE) | 0x1U); #define CAMSV_TG_SEN_MODE_CMOS_EN BIT(0) (did I get that right?) > + > + /* finally, CAMSV_MODULE_EN : IMGO_EN */ > + mtk_camsv30_write(cam_dev, CAMSV_MODULE_EN, > + mtk_camsv30_read(cam_dev, CAMSV_MODULE_EN) | 0x00000010U); #define CAMSV_MODULE_IMGO_EN BIT(4) > + > + pm_runtime_put_autosuspend(cam_dev->dev); > + spin_unlock(&cam_dev->irqlock); > +} > + ..snip.. > + > +static int mtk_camsv30_probe(struct platform_device *pdev) > +{ > + static const char * const clk_names[] = { > + "camsys_cam_cgpdn", > + "camsys_camtg_cgpdn", > + "camsys_camsv" "CG" stands for "Clock Gating" while "PDN" stands for "Power DowN", and it also looks obvious for those clocks to be in the camsys clock controller anyway... and even if not, we definitely don't care (from this driver's perspective) about which clock controller they're located at. In any case, we have those names in the clock controller drivers (so, names are accessible from the clock framework anyway), and each clock controller driver has the subsystem name (so, the camsys clk controller advertises being camsys already), which means that we can shorten the clk_names to... static const char * const clk_names[] = { "cam", "camtg", "camsv" }; ...also eventually leading to a more readable devicetree, as the clock-names will be just one line. > + }; > + > + struct mtk_cam_dev *cam_dev; > + struct device *dev = &pdev->dev; > + int ret; > + int i; > + > + if (!iommu_present(&platform_bus_type)) > + return -EPROBE_DEFER; > + > + cam_dev = devm_kzalloc(dev, sizeof(*cam_dev), GFP_KERNEL); > + if (!cam_dev) > + return -ENOMEM; > + > + cam_dev->conf = of_device_get_match_data(dev); > + if (!cam_dev->conf) > + return -ENODEV; > + > + cam_dev->dev = dev; > + dev_set_drvdata(dev, cam_dev); > + > + cam_dev->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(cam_dev->regs)) { > + dev_err(dev, "failed to map register base\n"); return dev_err_probe( .... ) > + return PTR_ERR(cam_dev->regs); > + } > + > + cam_dev->regs_img0 = devm_platform_ioremap_resource(pdev, 1); > + > + if (IS_ERR(cam_dev->regs_img0)) { > + dev_err(dev, "failed to map img0 register base\n"); > + return PTR_ERR(cam_dev->regs_img0); dev_err_probe() > + } > + > + cam_dev->regs_tg = devm_platform_ioremap_resource(pdev, 2); > + if (IS_ERR(cam_dev->regs_tg)) { > + dev_err(dev, "failed to map TG register base\n"); > + return PTR_ERR(cam_dev->regs_tg); again > + } > + > + cam_dev->num_clks = ARRAY_SIZE(clk_names); > + cam_dev->clks = devm_kcalloc(dev, cam_dev->num_clks, > + sizeof(*cam_dev->clks), GFP_KERNEL); > + if (!cam_dev->clks) > + return -ENOMEM; > + > + for (i = 0; i < cam_dev->num_clks; ++i) > + cam_dev->clks[i].id = clk_names[i]; > + > + ret = devm_clk_bulk_get(dev, cam_dev->num_clks, cam_dev->clks); > + if (ret) { > + dev_err(dev, "failed to get clocks: %i\n", ret); > + return ret; and again > + } > + > + cam_dev->irq = platform_get_irq(pdev, 0); > + ret = devm_request_irq(dev, cam_dev->irq, > + isp_irq_camsv30, 0, > + dev_name(dev), cam_dev); > + if (ret != 0) { > + dev_err(dev, "failed to request irq=%d\n", cam_dev->irq); > + return -ENODEV; and again. > + } > + > + cam_dev->hw_functions = &mtk_camsv30_hw_functions; > + > + spin_lock_init(&cam_dev->irqlock); > + > + /* initialise runtime power management */ > + pm_runtime_set_autosuspend_delay(dev, MTK_CAMSV30_AUTOSUSPEND_DELAY_MS); > + pm_runtime_use_autosuspend(dev); > + pm_runtime_set_suspended(dev); > + pm_runtime_enable(dev); > + //pm_runtime_get_sync(dev); Oops! development leftovers around! :-) > + > + /* Initialize the v4l2 common part */ > + return mtk_cam_dev_init(cam_dev); > +} > + > +static int mtk_camsv30_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct mtk_cam_dev *cam_dev = dev_get_drvdata(dev); > + > + mtk_cam_dev_cleanup(cam_dev); > + pm_runtime_put_autosuspend(dev); > + pm_runtime_disable(dev); > + > + return 0; > +} > + > +static const struct dev_pm_ops mtk_camsv30_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) > + SET_RUNTIME_PM_OPS(mtk_camsv30_runtime_suspend, > + mtk_camsv30_runtime_resume, NULL) > +}; > + > +static const struct of_device_id mtk_camsv30_of_ids[] = { > + { > + .compatible = "mediatek,mt8365-camsv", > + .data = &camsv30_conf, Fits in one line: { .compatible =..... data... }, > + }, > + {} and { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, mtk_camsv30_of_ids); > + > +static struct platform_driver mtk_camsv30_driver = { > + .probe = mtk_camsv30_probe, > + .remove = mtk_camsv30_remove, > + .driver = { > + .name = "mtk-camsv-isp30", > + .of_match_table = mtk_camsv30_of_ids, > + .pm = &mtk_camsv30_pm_ops, > + } > +}; > + > +module_platform_driver(mtk_camsv30_driver); > + > +MODULE_DESCRIPTION("MediaTek CAMSV ISP3.0 driver"); > +MODULE_AUTHOR("Florian Sylvestre <fsylvestre@baylibre.com>"); > +MODULE_LICENSE("GPL"); ...there might be something else, but it's a long review already; let's see what comes as v3. Regards, Angelo
Il 03/07/23 14:18, Laurent Pinchart ha scritto: > On Sun, Jul 02, 2023 at 05:20:25PM +0800, Chun-Kuang Hu wrote: >> Chun-Kuang Hu <chunkuang.hu@kernel.org> 於 2023年7月2日 週日 上午7:35寫道： >>> Julien Stephan <jstephan@baylibre.com> 於 2023年6月30日 週五 下午6:05寫道： >>>> >>>> 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> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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, >>>> +}; >>>> + >>>> +static const struct of_device_id mtk_seninf_of_match[] = { >>>> + { >>>> + .compatible = "mediatek,mt8365-seninf", >>>> + .data = &seninf_8365_conf, >>> >>> Now only support one SoC, so it's necessary to select the SoC data and >>> you could directly place the data in the code. After support other >> >> Typo. >> >> Now only support one SoC, so it's not necessary to select the SoC data >> and you could directly place the data in the code. > > I think Julien has visibility on other SoCs that could use the same > driver, and has designed the mtk_seninf_conf structure accordingly. I'll > let Julien confirm (or tell I'm wrong). > You're not wrong. Cheers, Angelo >>> SoC, so we could know what should be placed in struct mtg_seninf_conf >>> (Now we have no any information). >>> >>>> + }, >>>> + { >>>> + }, >>>> +}; >>>> +MODULE_DEVICE_TABLE(of, mtk_seninf_of_match); >
On Mon, Jul 03, 2023 at 02:21:17PM +0200, AngeloGioacchino Del Regno wrote: ..snip.. > > + > > + mtk_cam_init_cfg(&cam->subdev, NULL); > > mtk_cam_init_cfg() returns an integer, and this is probably because you expect > it to get logic to fail: in that case, check the result of that call here, > otherwise, make that function void. > Hi Angelo, mtk_cam_init_cfg returns an int because it should match v4l2_subdev_pad_ops->init_cfg prototype.. mtk_cam_init does not fail, always return 0, so I though it would be ok to discard the return value here. What do you think? > > + ..snip.. > > + unsigned int bpp; > > Can we get any format that uses a billion bits per pixel? :-P > > u8 bpp; > I am preparing the driver for futur upgrades.. in a billion years ;) Cheers Julien
On Thu, Aug 03, 2023 at 11:53:35AM +0200, Julien Stephan wrote: > On Mon, Jul 03, 2023 at 02:21:17PM +0200, AngeloGioacchino Del Regno wrote: > ..snip.. > > > + > > > + mtk_cam_init_cfg(&cam->subdev, NULL); > > > > mtk_cam_init_cfg() returns an integer, and this is probably because you expect > > it to get logic to fail: in that case, check the result of that call here, > > otherwise, make that function void. > > > > Hi Angelo, > > mtk_cam_init_cfg returns an int because it should match v4l2_subdev_pad_ops->init_cfg prototype.. > > mtk_cam_init does not fail, always return 0, so I though it would be ok > to discard the return value here. What do you think? Fine with me. > > > + > > ..snip.. > > > > + unsigned int bpp; > > > > Can we get any format that uses a billion bits per pixel? :-P > > > > u8 bpp; > > I am preparing the driver for futur upgrades.. in a billion years ;)
Il 03/08/23 11:53, Julien Stephan ha scritto: > On Mon, Jul 03, 2023 at 02:21:17PM +0200, AngeloGioacchino Del Regno wrote: > ..snip.. >>> + >>> + mtk_cam_init_cfg(&cam->subdev, NULL); >> >> mtk_cam_init_cfg() returns an integer, and this is probably because you expect >> it to get logic to fail: in that case, check the result of that call here, >> otherwise, make that function void. >> > > Hi Angelo, > > mtk_cam_init_cfg returns an int because it should match v4l2_subdev_pad_ops->init_cfg prototype.. > > mtk_cam_init does not fail, always return 0, so I though it would be ok > to discard the return value here. What do you think? > Right, I was thinking to make a void __mtk_cam_init_cfg(), but as an afterthought it's just better to leave it as it is, let's go with your solution. Cheers, Angelo >>> + > > ..snip.. > >>> + unsigned int bpp; >> >> Can we get any format that uses a billion bits per pixel? :-P >> >> u8 bpp; >> > > I am preparing the driver for futur upgrades.. in a billion years ;) > > Cheers > Julien
On Mon, Jul 03, 2023 at 01:02:02PM +0200, AngeloGioacchino Del Regno wrote: > Il 30/06/23 12:01, Julien Stephan ha scritto: ..snip.. > > + > > +static const struct mtk_seninf_format_info mtk_seninf_formats[] = { > > + { > > + .code = MEDIA_BUS_FMT_SBGGR8_1X8, > > + .flags = MTK_SENINF_FORMAT_BAYER, > > Each entry fits in one line. > > { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .flags = MTK_SENINF_FORMAT_BAYER }, > Hi Angelo, Actually not all entries fit in one line. The last 4 ones don't: { .code = MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, .flags = MTK_SENINF_FORMAT_DPCM | MTK_SENINF_FORMAT_INPUT_ONLY }, which is 115 chars.. so what is the best? put all in one line except the last 4 one? or keep them all as is? > > + }, { ..snip.. > > + udelay(1); > > + mtk_seninf_input_update(input, SENINF_CTRL, CSI2_SW_RST, 0); > > Is there any way to check if the CSI port did reset, or is it *guaranteed* to get > out of reset in a microsecond after deasserting SW_RST? > I will double check this > > +} > > + ..snip.. > > + > > + 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; > > writel(mux->base + SENINF_MUX_CTRL, val | rst_mask); > writel(mux->base + SENINFMUX_CTRL, val & ~rst_mask); > > that's better, right? :-) > right :) Cheers Julien > > + mtk_seninf_mux_write(mux, SENINF_MUX_CTRL, val | > > + SENINF_MUX_CTRL_SENINF_IRQ_SW_RST | > > + SENINF_MUX_CTRL_SENINF_MUX_SW_RST);
On Fri, Aug 04, 2023 at 04:50:05PM +0200, Julien Stephan wrote: > On Mon, Jul 03, 2023 at 01:02:02PM +0200, AngeloGioacchino Del Regno wrote: > > Il 30/06/23 12:01, Julien Stephan ha scritto: > ..snip.. > > > + > > > +static const struct mtk_seninf_format_info mtk_seninf_formats[] = { > > > + { > > > + .code = MEDIA_BUS_FMT_SBGGR8_1X8, > > > + .flags = MTK_SENINF_FORMAT_BAYER, > > > > Each entry fits in one line. > > > > { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .flags = MTK_SENINF_FORMAT_BAYER }, > > > > Hi Angelo, > > Actually not all entries fit in one line. The last 4 ones don't: > > { .code = MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, .flags = MTK_SENINF_FORMAT_DPCM | MTK_SENINF_FORMAT_INPUT_ONLY }, > which is 115 chars.. > > so what is the best? put all in one line except the last 4 one? or keep > them all as is? I'd keep them all as is. > > > + }, { > ..snip.. > > > + udelay(1); > > > + mtk_seninf_input_update(input, SENINF_CTRL, CSI2_SW_RST, 0); > > > > Is there any way to check if the CSI port did reset, or is it *guaranteed* to get > > out of reset in a microsecond after deasserting SW_RST? > > I will double check this > > > > +} > > > + > ..snip.. > > > + > > > + 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; > > > > writel(mux->base + SENINF_MUX_CTRL, val | rst_mask); > > writel(mux->base + SENINFMUX_CTRL, val & ~rst_mask); > > > > that's better, right? :-) With mtk_seninf_mux_write() instead of writel(), yes :-) > right :) > > > > + mtk_seninf_mux_write(mux, SENINF_MUX_CTRL, val | > > > + SENINF_MUX_CTRL_SENINF_IRQ_SW_RST | > > > + SENINF_MUX_CTRL_SENINF_MUX_SW_RST);
This series add the support of the Mediatek ISP3.0 found on some Mediatek SoCs such as the mt8365. The driver is divided into 2 parts: * SENINF: the sensor interface * CAMSV: this driver provides a path to bypass the SoC ISP so that image data coming from the SENINF can go directly into memory without any image processing. This allows the use of an external ISP or camera sensor directly. The SENINF driver is based on previous work done by Louis Kuo available as an RFC here: https://lore.kernel.org/all/20200708104023.3225-1-louis.kuo@mediatek.com/ This series depends on the following series: [1] for the phy, [2] for power management support Changes in v2: - renamed clock `cam_seninf` to `camsys` - renamed clock `top_mux_seninf` to `top_mux` - moved phy properties from port nodes to top level - remove patternProperties - specify power management dependency in the cover letter description to fix missing include in dt-binding example - change '$ref' properties on some endpoint nodes from '$ref: video-interfaces.yaml#' to '$ref: /schemas/graph.yaml#/$defs/endpoint-base' where applicable Best Julien Stephan [1] : https://lore.kernel.org/all/20230620121928.1231745-1-jstephan@baylibre.com/ [2] : https://lore.kernel.org/lkml/20230627131040.3418538-1-msp@baylibre.com/ Louis Kuo (2): dt-bindings: media: add mediatek ISP3.0 sensor interface media: platform: mediatek: isp_30: add mediatek ISP3.0 sensor interface Phi-bang Nguyen (2): dt-bindings: media: add mediatek ISP3.0 camsv media: platform: mediatek: isp_30: add mediatek ISP3.0 camsv .../bindings/media/mediatek,mt8365-camsv.yaml | 113 ++ .../media/mediatek,mt8365-seninf.yaml | 295 ++++ MAINTAINERS | 10 + 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 | 35 + .../platform/mediatek/isp/isp_30/Makefile | 4 + .../mediatek/isp/isp_30/camsv/Makefile | 7 + .../mediatek/isp/isp_30/camsv/mtk_camsv.c | 334 ++++ .../mediatek/isp/isp_30/camsv/mtk_camsv.h | 200 +++ .../isp/isp_30/camsv/mtk_camsv30_hw.c | 474 +++++ .../isp/isp_30/camsv/mtk_camsv30_regs.h | 59 + .../isp/isp_30/camsv/mtk_camsv_video.c | 781 +++++++++ .../mediatek/isp/isp_30/seninf/Makefile | 5 + .../mediatek/isp/isp_30/seninf/mtk_seninf.c | 1561 +++++++++++++++++ .../isp/isp_30/seninf/mtk_seninf_reg.h | 99 ++ 18 files changed, 3984 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8365-seninf.yaml 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/camsv/Makefile create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.c create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.h create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_hw.c create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_regs.h create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv_video.c 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 -- 2.41.0