Hi Laurent, On Wed, May 31, 2023 at 05:42:58PM +0300, Laurent Pinchart wrote: > Hi Tommaso, > > On Wed, May 31, 2023 at 04:19:03PM +0200, Tommaso Merciai wrote: > > On Wed, May 31, 2023 at 02:33:31PM +0300, Laurent Pinchart wrote: > > > On Wed, May 31, 2023 at 12:13:39PM +0200, Tommaso Merciai wrote: > > > > On Mon, May 29, 2023 at 10:40:18AM +0300, Laurent Pinchart wrote: > > > > > On Fri, May 26, 2023 at 07:39:44PM +0200, Tommaso Merciai wrote: > > > > > > The Alvium camera is shipped with sensor + isp in the same housing. > > > > > > > > > > What ISP is that ? > > > > > > > > The isp comes from Allied Vision, is called Alvium and is part of the camera. > > > > In real is a vision processor, this offers more functionality then an isp can do. > > > > It is not sold separately but is an integrated part of the camera itself > > > > which also takes care of the sensor abstraction. > > > > > > > > > > The camera can be equipped with one out of various sensor and abstract > > > > > > the user from this. Camera is connected via MIPI CSI-2. > > > > > > > > > > > > Most of the sensor's features are supported, with the main exception > > > > > > being fw update. > > > > > > > > > > I assume you mean "camera module features" here, as firmware update > > > > > isn't a camera sensor feature. > > > > > > > > Yep. > > > > > > > > > > The driver provides all mandatory, optional and recommended V4L2 controls > > > > > > for maximum compatibility with libcamera > > > > > > > > > > > > References: > > > > > > - https://www.alliedvision.com/en/products/embedded-vision-solutions > > > > > > > > > > I'll do a partial review only as the patch is large, and will try to > > > > > review the rest in v2. > > > > > > > > No problem. > > > > > > > > > > Signed-off-by: Tommaso Merciai <tomm.merciai@xxxxxxxxx> > > > > > > --- > > > > > > drivers/media/i2c/Kconfig | 9 + > > > > > > drivers/media/i2c/Makefile | 1 + > > > > > > drivers/media/i2c/alvium.c | 3547 ++++++++++++++++++++++++++++++++++++ > > > > > > drivers/media/i2c/alvium.h | 500 +++++ > > > > > > 4 files changed, 4057 insertions(+) > > > > > > create mode 100644 drivers/media/i2c/alvium.c > > > > > > create mode 100644 drivers/media/i2c/alvium.h > > [snip] > > > > > > > diff --git a/drivers/media/i2c/alvium.c b/drivers/media/i2c/alvium.c > > > > > > new file mode 100644 > > > > > > index 000000000000..e77fb6bda64b > > > > > > --- /dev/null > > > > > > +++ b/drivers/media/i2c/alvium.c > > > > > > @@ -0,0 +1,3547 @@ > > [snip] > > > > > > > +static int alvium_set_streamon_delay(struct alvium_dev *alvium) > > > > > > +{ > > > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > > > + int ret; > > > > > > + u64 streamon_delay; > > > > > > + > > > > > > + if (!alvium->bcrm_addr) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + /* > > > > > > + * CSI2 receivers sometimes need some delay before are ready to > > > > > > + * get data. This register set delay before camera start > > > > > > + * capturing frames in us. > > > > > > > > > > If the CSI-2 receiver needs a delay, it should be handled by the CSI-2 > > > > > receiver driver, not here. > > > > > > > > My bad, bad description/bad name. > > > > > > > > This regs is an optional delay that alvium introduces between switching > > > > from lp to hs mode. > > > > Some mipi receivers require this delay on the sender side. > > > > > > > > I plan to switch to the following name: alliedvision,lp2hs-delay-us > > > > and fix the comment with the above comment^ > > > > > > > > This is more clear, what do you think about? > > > > > > It's a bit clearer, but still not fully :-) Does this delay correspond > > > to one of the MIPI D-PHY timing parameters ? If so, which one ? > > > > Thanks to Martin Hecht I'm able to provide you a clearer description: > > > > Alvium normally delivers a continous clock even when it is not streaming. > > That's an interesting design choice. > > > The purpose of this reg is force a DPhy reset for the period described by > > the us on the reg before it starts streaming. To be clear, with that value > > bigger than 0 the Alvium forces a dphy-reset on all lanes for that period. > > That means all lanes go up into low power state. > > This may help a csi2 rx ip to reset if that IP can't deal with a continous clock. > > No CSI-2 receiver can possibly handle this :-) If the camera power can't > be turned on only after the CSI-2 receiver is started (as obviously the > camera can't output a HS clock when it's not powered on), then the > receiver wouldn't be able to synchronize. > > Thanks for the explanation. Could you record this in the DT bindings ? Ofc, Thanks! Regards, Tommaso > > > > > > > + */ > > > > > > + streamon_delay = (u64)alvium->streamon_delay; > > > > > > + > > > > > > + ret = alvium_i2c_write_reg_hshake(alvium, > > > > > > + alvium->bcrm_addr + > > > > > > + REG_BCRM_STREAM_ON_DELAY_32RW, > > > > > > + 4, streamon_delay); > > > > > > + if (ret) { > > > > > > + dev_err(dev, "Fail to set streamon delay reg\n"); > > > > > > + return ret; > > > > > > + } > > > > > > + > > > > > > + return 0; > > > > > > +} > > [snip] > > -- > Regards, > > Laurent Pinchart
