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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Nov 25, 2024 at 09:56:54AM +0000, CK Hu (胡俊光) wrote:
> On Mon, 2024-11-25 at 11:39 +0200, Laurent Pinchart wrote:
> > On Mon, Nov 25, 2024 at 06:59:59AM +0000, CK Hu (胡俊光) wrote:
> > > On Thu, 2024-11-21 at 09:53 +0100, Julien Stephan wrote:
> > > >
> > > > 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>
> > > > [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]
> > >
> > > > +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.

> 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.

> > > > +};
> > > > +

-- 
Regards,

Laurent Pinchart