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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241125102250.GO19381@pendragon.ideasonboard.com>
Date: Mon, 25 Nov 2024 12:22:50 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: CK Hu (胡俊光) <ck.hu@...iatek.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"paul.elder@...asonboard.com" <paul.elder@...asonboard.com>,
	"mchehab@...nel.org" <mchehab@...nel.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	"robh@...nel.org" <robh@...nel.org>,
	Andy Hsieh (謝智皓) <Andy.Hsieh@...iatek.com>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	Julien Stephan <jstephan@...libre.com>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"fsylvestre@...libre.com" <fsylvestre@...libre.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v7 4/5] media: platform: mediatek: isp: add mediatek
 ISP3.0 camsv

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@...libre.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@...libre.com>
> > > > Signed-off-by: Florian Sylvestre <fsylvestre@...libre.com>
> > > > [Paul Elder fix irq locking]
> > > > Signed-off-by: Paul Elder <paul.elder@...asonboard.com>
> > > > Co-developed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > > Co-developed-by: Julien Stephan <jstephan@...libre.com>
> > > > Signed-off-by: Julien Stephan <jstephan@...libre.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