Re: [PATCH v7 4/5] media: platform: mediatek: isp: add mediatek ISP3.0 camsv

On Mon, 2024-11-25 at 12:22 +0200, Laurent Pinchart wrote:

External email : Please do not click links or open attachments until you have verified the sender or the content.


On Mon, Nov 25, 2024 at 09:56:54AM +0000, CK Hu (胡俊光) wrote:

quoted

On Mon, 2024-11-25 at 11:39 +0200, Laurent Pinchart wrote:

quoted

On Mon, Nov 25, 2024 at 06:59:59AM +0000, CK Hu (胡俊光) wrote:

quoted

On Thu, 2024-11-21 at 09:53 +0100, Julien Stephan wrote:

quoted

From: Phi-bang Nguyen <redacted>

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 <redacted>
Signed-off-by: Florian Sylvestre <redacted>
[Paul Elder fix irq locking]
Signed-off-by: Paul Elder <paul.elder@ideasonboard.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]

quoted

+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 */
+       .imgo_con = 0x80000080U, /* DMA FIFO depth and burst */
+       .imgo_con2 = 0x00020002U, /* DMA priority */

Now support only one SoC, so it's not necessary make these SoC variable.
They could be constant symbol now.

This I would keep as a mtk_cam_conf structure instance, as I think it
makes it clear what we consider to be model-specific without hindering
readability. I don't have a very strong opinion though.

If this is a configuration table, I would like it to be

{
.time_stp_en = true;
.dbl_data_bus = 1;
.double_buffer_en = true;
.tg = 0x4;
...
}

I like that too, it's more readable than raw register values.

quoted

If next SoC has only one parameter is different, we duplicate all
other parameter?

That's what we usually do when the amount of parameters is not too
large. When it becomes larger, we sometimes split the configuration data
in multiple chunks. For instance,

static const char * const family_a_clks[] = {
        "core",
        "io",
};

static sont char * const family_b_clks[] = {
        "main",
        "ram",
        "bus",
};

static const foo_dev_info soc_1_info = {
        .has_time_machine = false,
        .clks = family_a_clks,
        .num_clks = ARRAY_SIZE(family_a_clks),
};

static const foo_dev_info soc_2_info = {
        .has_time_machine = false,
        .clks = family_b_clks,
        .num_clks = ARRAY_SIZE(family_b_clks),
};

static const foo_dev_info soc_3_info = {
        .has_time_machine = true,
        .clks = family_b_clks,
        .num_clks = ARRAY_SIZE(family_b_clks),
};

There's no clear rule, it's on a case-by-case basis.

OK. That's fine for me.

Regards,
CK

quoted

quoted

quoted

quoted

+};
+

--
Regards,

Laurent Pinchart