Re: [PATCH 1/2] media: i2c: Add driver for the Analog Devices ADDI9036 ToF front-end

[Sakari Ailus]

Hi Bogdan,

On Fri, Sep 11, 2020 at 01:32:39PM +0000, Togorean, Bogdan wrote:
> Hi Laurent,
> 
> > Hi Bogdan,
> > 
> > Thank you for the patch.
> 
> Thank you for review!
> 
> > 
> > On Thu, Sep 10, 2020 at 07:24:06PM +0300, Bogdan Togorean wrote:
> > > The ADDI9036 is a complete, 45 MHz, front-end solution for charge
> > > coupled device (CCD) time of flight (TOF) imaging applications.
> > >
> > > It has 2-lane MIPI CSI-2 RAW12 data output and i2c control interface.
> > >
> > > The programming of calibration and firmware is performed by userspace
> > > SDK through CID_USER class IOCTLs.
> > 
> > I'm afraid that's not acceptable. The firmware should be loaded using
> > the Linux kernel firmware API. As for calibration, is it static data ?
> > How is it generated ?
> 
> Calibration data is static data, generated at production and stored on and EEPROM
> memory on camera board. The camera board consists mainly of ADDI9036 processor
> + CCD image sensor + EEPROM.
> 
> The firmware is a set of (address, data) tuples that are written via I2C.
> We want this driver to be a generic one and support all implementations
> of ADDI9036 ToF processor. Now the problem is that some implementations 
> store firmware + calibration on external EEPROM and some store just calibration 
> data on EEPROM and firmware is loaded from a file stored in SDK. With the help 
> of the 2 custom controls we move the camera model detection, firmware
> and calibration fetching from driver to user space SDK.

Can you figure out by reading EEPROM which one of these is the case?
Alternatively you could add a custom DT property IMO.

The calibration data could be loaded using request_firmware(), too. I'd use
a specific directory for just this purpose.

> 
> Also an important point is that the ToF processor has 3 operating modes 
> (near, medium and far). At each mode change a new set of firmware + 
> calibration data is loaded into ADDI9036 chip. If we move the firmware loading
> in driver we would still need a custom enumeration control to select the working mode.

I don't see there being an issue having such a control.

-- 
Kind regards,

Sakari Ailus