Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 31 May 2023 17:12:00 +0200
From:   Tommaso Merciai <tomm.merciai@gmail.com>
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     jacopo.mondi@ideasonboard.com, martin.hecht@avnet.eu,
        linuxfancy@googlegroups.com,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Marco Felsch <m.felsch@pengutronix.de>,
        Gerald Loacker <gerald.loacker@wolfvision.net>,
        Nicholas Roth <nicholas@rothemail.net>,
        Shawn Tu <shawnx.tu@intel.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Benjamin Mugnier <benjamin.mugnier@foss.st.com>,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org
Subject: Re: [PATCH v2 2/2] media: i2c: Add support for alvium camera
Message-ID: <ZHdjwAnAlgb6c2dN@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
References: <20230526173955.797226-1-tomm.merciai@gmail.com>
 <20230526173955.797226-3-tomm.merciai@gmail.com>
 <20230529074018.GD25984@pendragon.ideasonboard.com>
 <ZHcd09f5wOKjQdHX@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
 <20230531113331.GC27043@pendragon.ideasonboard.com>
 <ZHdXV4SPOW0dxtcG@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
 <20230531144258.GB24749@pendragon.ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230531144258.GB24749@pendragon.ideasonboard.com>
Precedence: bulk

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@gmail.com>
> > > > > > ---
> > > > > >  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