Re: [PATCH v5 3/3] media: i2c: Add support for alvium camera

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

On Tue, Jun 20, 2023 at 06:54:43AM +0200, Tommaso Merciai wrote:
> On Mon, Jun 19, 2023 at 05:24:58PM +0300, Laurent Pinchart wrote:
> > On Mon, Jun 19, 2023 at 10:50:10AM +0000, Sakari Ailus wrote:
> > > On Fri, Jun 09, 2023 at 03:09:41PM +0200, Tommaso Merciai wrote:
> > > > On Fri, Jun 09, 2023 at 09:17:42AM +0000, Sakari Ailus wrote:
> > > > > On Thu, Jun 08, 2023 at 10:31:16AM +0200, Tommaso Merciai wrote:
> > > > > > The Alvium camera is shipped with sensor + isp in the same housing.
> > > > > > 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 camera module features are supported, with the main exception
> > > > > > being fw update.
> > > > > > 
> > > > > > 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
> > > > > > 
> > > > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > > > > > ---
> > > > > > Changes since v2:
> > > > > >  - Removed gpios/clock handling as suggested by LPinchart
> > > > > >  - Added vcc-ext-in supply support as suggested by LPinchart
> > > > > >  - Fixed alvium_setup_mipi_fmt funct as suggested by CJAILLET
> > > > > >  - Removed upside_down/hshake_bit priv data as suggested by CJAILLET
> > > > > >  - Fixed commit body as suggested by LPinchart
> > > > > >  - Mv alvium_set_streamon_delay to yalvium_set_lp2hs_delay
> > > > > >  - Fixed comment on lp2hs prop as suggested by LPinchart
> > > > > >  - Added pm resume/suspend functs as suggested by LPinchart
> > > > > >  - Dropped alvium_link_setup/alvium_s_power as suggested by LPinchart
> > > > > >  - Fixed regs defines as suggested by LPinchart
> > > > > >  - Fixed typedef as suggested by LPinchart
> > > > > >  - Dropped bcrm_v/fw_v from priv data as suggested by LPinchart
> > > > > >  - Now driver use the subdev active state to store the active format and crop
> > > > > >    as suggested by LPinchart
> > > > > >  - Dropped alvium_is_csi2/i2c_to_alvium as suggested by LPinchart
> > > > > > 
> > > > > > Changes since v3:
> > > > > >  - Fixed warnings Reported-by: kernel test robot <lkp@intel.com>
> > > > > > 
> > > > > > Changes since v4:
> > > > > >  - Removed print into alvium_get_dt_data for alliedvision,lp2hs-delay-us as
> > > > > >    suggested by CDooley
> > > > > > 
> > > > > >  drivers/media/i2c/Kconfig       |   10 +
> > > > > >  drivers/media/i2c/Makefile      |    1 +
> > > > > >  drivers/media/i2c/alvium-csi2.c | 3479 +++++++++++++++++++++++++++++++
> > > > > >  drivers/media/i2c/alvium-csi2.h |  485 +++++
> > > > > >  4 files changed, 3975 insertions(+)
> > > > > >  create mode 100644 drivers/media/i2c/alvium-csi2.c
> > > > > >  create mode 100644 drivers/media/i2c/alvium-csi2.h
> > 
> > [snip]
> > 
> > > > > > diff --git a/drivers/media/i2c/alvium-csi2.c b/drivers/media/i2c/alvium-csi2.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..52c9263075cf
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/media/i2c/alvium-csi2.c
> > > > > > @@ -0,0 +1,3479 @@
> > 
> > [snip]
> > 
> > > > > > +static int alvium_get_img_width_params(struct alvium_dev *alvium)
> > > > > > +{
> > > > > > +	struct device *dev = &alvium->i2c_client->dev;
> > > > > > +	int ret;
> > > > > > +	u64 val;
> > > > > > +
> > > > > > +	if (!alvium->bcrm_addr)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	ret = alvium_read(alvium,
> > > > > > +			  REG_BCRM_IMG_WIDTH_MIN_R,
> > > > > > +			  &val);
> > > > > > +	if (ret) {
> > > > > > +		dev_err(dev, "Fail to read img min width reg\n");
> > > > > > +		return ret;
> > > > > > +	}
> > > > > 
> > > > > Could you add a macro that assigns the value to the variable (or a struct
> > > > > field in this case) when the read is successful? Add the print if you think
> > > > > you need it.
> > > > 
> > > > I don't get this comment.
> > > > Can you explain me better your plan please.
> > > 
> > > You have exactly the same pattern repeated over and over in a number of
> > > functions. I'd like you to add a macro (or a function) that takes what
> > > varies as arguments, and call that function here. It would reduce a lot of
> > > the repeated lines code here.
> > > 
> > > ...
> > 
> > The best option is to print an error message in alvium_read() and drop
> > all error messages from the callers.
> 
> What about don't print anything? We already have prints that comes from
> CCI API if some errors occurs. Laurent suggest me this into some
> previous comments. Let me know.

We need to print something somewhere as silent failures are bad. The
messages printed by the CCI helpers are good enough, so no need to print
anything specific in the alvium driver.

-- 
Regards,

Laurent Pinchart