linux-kernel - Re: [PATCH v3 2/2] media: i2c: Add ON Semiconductor AP1302 ISP driver

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: <20250623224701.GE15951@pendragon.ideasonboard.com>
Date: Tue, 24 Jun 2025 01:47:01 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Frank Li <Frank.Li@....com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Kumar M <anil.mamidala@...inx.com>, linux-media@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	imx@...ts.linux.dev, "Guoniu.zhou" <guoniu.zhou@....com>,
	Stefan Hladnik <stefan.hladnik@...il.com>,
	Florian Rebaudo <frebaudo@...ekio.com>
Subject: Re: [PATCH v3 2/2] media: i2c: Add ON Semiconductor AP1302 ISP driver

Hi Frank,

Thank you for the patch.

On Mon, Jun 23, 2025 at 03:17:38PM -0400, Frank Li wrote:
> From: Anil Kumar Mamidala <anil.mamidala@...inx.com>
> 
> The AP1302 is a standalone ISP for ON Semiconductor sensors.
> AP1302 ISP supports single and dual sensor inputs. The driver
> code supports AR1335, AR0144 and AR0330 sensors with single and
> dual mode by loading the corresponding firmware.
> 
> Signed-off-by: Anil Kumar Mamidala <anil.mamidala@...inx.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Signed-off-by: Stefan Hladnik <stefan.hladnik@...il.com>
> Signed-off-by: Florian Rebaudo <frebaudo@...ekio.com>
> Signed-off-by: Frank Li <Frank.Li@....com>
> ---
> Change in v3:
> - add extra empty line between difference register define
> - add bits.h
> - use GEN_MASK and align regiser bit define from 31 to 0.
> - add ap1302_sensor_supply
> - add enable gpio
> - update firmware header format

One of the main issues with this driver is that we need to standardize
the header format. The standardized format will need to be approved by
onsemi as we will need to provide not just a driver, but also a
toolchain that will produce firmwares in the right format. Furthermore,
some time ago the AP1302 firmware was extended with the ability to
dynamically compute PLL parameters IIRC. This needs to be taken into
account.

I want to resuscitate this driver and get it merged. There's more work
to do, in collaboration with onsemi, and I haven't had time to tackle
it. If you want to propose a proper design for firmware handling I would
be happy to participate in the discussion.

> - update raw sensor supply delay time
> - use gpiod_set_value_cansleep() insteand gpiod_set_value()
> - update use latest v4l2 api
> - use ctrl_to_sd() helper function
> - add ap1302_g_volatile_ctrl()
> - remove ap1302_get_fmt()
> - use guard for mutex.
> - use dev_err_probe
> - use devm_add_action_or_reset to simple error handle at probe.
> - use read_poll_timeout() simple dma idle polling.
> 
> previous upstream:
> https://lore.kernel.org/linux-media/1631091372-16191-1-git-send-email-anil.mamidala@xilinx.com/
> ---
>  MAINTAINERS                |    1 +
>  drivers/media/i2c/Kconfig  |    9 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/ap1302.c | 2838 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 2849 insertions(+)

[snip]

-- 
Regards,

Laurent Pinchart