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

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

On Tue, Jun 24, 2025 at 02:47:10PM -0400, Frank Li wrote:
> On Tue, Jun 24, 2025 at 01:47:01AM +0300, Laurent Pinchart wrote:
> > On Mon, Jun 23, 2025 at 03:17:38PM -0400, Frank Li wrote:
> > > From: Anil Kumar Mamidala <anil.mamidala@xilinx.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@xilinx.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Stefan Hladnik <stefan.hladnik@gmail.com>
> > > Signed-off-by: Florian Rebaudo <frebaudo@witekio.com>
> > > Signed-off-by: Frank Li <Frank.Li@nxp.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.
> 
> It is quite common when work with firmwares. Generally, it need version
> information at header.
> 
> The driver need check firmware's API version, if miss match or incompatible,
> just return and report error.
> 
> we can't assume firmware always align driver code because many user just
> update kernel without update rootfs or firmware package.

Sure, but that's not the point. The point is that there are multiple
out-of-tree ap1302 driver versions, developed or adapted by different
SoC vendors. Those variants use firmware files produced by those SoC
vendors, and they not standard. We need to standardize on a firmware
format to upstream a driver, and that standardization needs to involve
the device manufacturer.

> > 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.
> 
> who is onsemi contact windows.
> 
> > > - 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