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

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

Hi Tommaso,

On Mon, Oct 30, 2023 at 02:29:36PM +0100, Tommaso Merciai wrote:
> On Mon, Oct 30, 2023 at 02:37:03PM +0200, Laurent Pinchart wrote:
> > On Mon, Oct 30, 2023 at 01:26:58PM +0100, Tommaso Merciai wrote:
> > > On Thu, Oct 26, 2023 at 01:18:21PM +0000, Sakari Ailus wrote:
> > > > Hi Tommaso,
> > > > 
> > > > Thanks for the update.
> > > > 
> > > > There's still quite a bit to do in this driver. Feel free to ask further
> > > > questions regarding the comments.
> > > > 
> > > > On Fri, Oct 20, 2023 at 04:13:51PM +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
> > > > > 
> > > > > Changes since v5:
> > > > >  - Used tab instead of space in .h as suggested by SAilus
> > > > >  - Added support for new CCI API from HDeGoede as suggested by SAilus
> > > > >  - Fixed alvium_write/alvium_read, functions now using the new CCI api, suggested by LPinchart
> > > > >  - Fixed alvium_get_feat_inq func as suggested by SAilus
> > > > >  - Fixed indentation/var-order/includes-order as suggested by SAilus
> > > > >  - Fixed alvium_csi2_fmts with MIPI_CSI2_DT_ defines as suggested by SAilus
> > > > >  - Fixed alvium_is_alive as suggested by SAilus
> > > > >  - Fixed alvium_code_to_pixfmt funct as suggested by SAilus
> > > > >  - Fixed alvium_get_dt_data function, now use only fwnode as suggested by SAilus
> > > > >  - Fixed autosuspend into the probe, is disable as default as suggested by SAilus
> > > > >  - Fixed alvium_get_dt_data function, assigned bus type before parsing the ep
> > > > >    as suggested by SAilus
> > > > >  - Fixed alvium_power_off, removed wrong print as suggested by SAilus
> > > > > 
> > > > > Changes since v6:
> > > > >  - Fixed .h indentation
> > > > >  - Fixed function params indentation
> > > > >  - Added int *err params for alvium_read/alvium_write as suggested by LPinchart
> > > > >  - Removed dbg print from the driver, driver is now using dbg/err prints that comes from
> > > > >    new cci API as suggested by LPinchart. This, fits SAilus suggestion on common pattern function.
> > > > >  - Fixed alvium_write_hshake, now use read_poll_timeout as suggested by LPinchart
> > > > >  - Removed useless includes
> > > > >  - Added maintainers file entries
> > > > > 
> > > > > Changes since v7:
> > > > >  - Fix company legal entity from Inc. to GmbH
> > > > >  - Fix warnings given from HVerkuil build-scripts in alvium_get_bcrm_vers,
> > > > >    alvium_get_fw_version and probe functions using __le16/__le32. Fixed also
> > > > >    probe function warning alvium-csi2.c:2665 alvium_probe() warn: missing error code? 'ret'
> > > > > 
> > > > > Changes since v8:
> > > > >  - Fixed alvium_i2c_driver struct, use probe istead of probe_new
> > > > >  - Fixed Kconfig description taking as reference new mt9m114 driver
> > > > >  - Fixed Kconfig just select V4L2_CCI_I2C taking as reference new mt9m114 driver
> > > > > 
> > > > > Changes since v9:
> > > > >  - Fixed Y8_1X8 mipi_fmt_regval
> > > > >  - Removed alliedvision,lp2hs-delay-us property we set now a default safe value as discussed with SAilus
> > > > >  - Added dft property for ctrls initialization, we first read dft values from the camera and set this into ctrls
> > > > >  - Fixed indentation as suggested by SAilus
> > > > >  - Fixed bit field definitions alignment into .h as suggested by SAilus
> > > > >  - Fixed Heartbeat reg from R -> RW
> > > > >  - Fixed adjusting values in format/crop changes as suggested by SAilus
> > > > >  - Removed unnecessary brcm_addr checks as suggested by SAilus
> > > > >  - Merged poweron/poweroff functions as suggested by SAilus
> > > > >  - Added poweroff path during probe as suggested by SAilus
> > > > >  - Fixed module license type as suggested by SAilus
> > > > >  - Removed unnecessary MODULE_DEVICE_TABLE as suggested by SAilus
> > > > >  - Fixed pm support in s_ctrl and s_stream functions
> > > > >  - Removed unnecessary local variables  as suggested by SAilus
> > > > >  - Added ret values checks as suggested by SAilus
> > > > > 
> > > > >  MAINTAINERS                     |    9 +
> > > > >  drivers/media/i2c/Kconfig       |   10 +
> > > > >  drivers/media/i2c/Makefile      |    1 +
> > > > >  drivers/media/i2c/alvium-csi2.c | 2666 +++++++++++++++++++++++++++++++
> > > > >  drivers/media/i2c/alvium-csi2.h |  489 ++++++
> > > > >  5 files changed, 3175 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..2c40804655cd
> > > > > --- /dev/null
> > > > > +++ b/drivers/media/i2c/alvium-csi2.c
> > > > > @@ -0,0 +1,2666 @@
> > 
> > [snip]
> > 
> > > > > +static int alvium_get_host_supp_csi_lanes(struct alvium_dev *alvium)
> > > > > +{
> > > > > +	u64 val;
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	alvium_read(alvium, REG_BCRM_CSI2_LANE_COUNT_RW, &val, &ret);
> > > > 
> > > > Missing error checking before the use of the value. The same pattern
> > > > remains prevalent throughout the driver.
> > > > 
> > > > I think it'd be easier if you didn't use a temporary variable for reading,
> > > > but instead had a register width specific access function. You could even
> > > > introduce a helper macro to read this information as I suggested in an
> > > > earlier review.
> > > 
> > > oks.
> > > We are moving to use the following macros:
> > > 
> > > #define alvium_read_check(alvium, reg, value) \
> > > { \
> > > 	int ret = alvium_read(alvium, reg, value, NULL); \
> > > 	if (ret) \
> > > 		return ret; \
> > > }
> > 
> > Please don't. Embedding a return in a macro is very confusing for the
> > reader, and very very frowned upon in the kernel.
> 
> I'm a bit confused :)
> Sorry.
> 
> Plan is to replace a common pattern.
> First I switch to alvium_read(alvium, reg, value, err); implementation.
> Then I switch to this macro that is not really safe :)
> 
> Do you have some hint? :)
> Thanks in advance.
> 
> Maybe I haven't catch completely your comments.

In this specific case, the right pattern is either

	u64 val;
	int ret;

	ret = alvium_read(alvium, REG_BCRM_CSI2_LANE_COUNT_RW, &val, NULL);
	if (ret)
		return ret;

	alvium->h_sup_csi_lanes = val;

	return 0;

or

	u64 val;
	int ret = 0;

	alvium_read(alvium, REG_BCRM_CSI2_LANE_COUNT_RW, &val, &ret);
	if (ret)
		return ret;

	alvium->h_sup_csi_lanes = val;

	return 0;

I personally prefer the former.

If the function was *writing* multiple registers, the right pattern
would be

	int ret = 0;

	alvium_write(alvium, REG_BCRM_REG_1, foo, &ret);
	alvium_write(alvium, REG_BCRM_REG_2, bar, &ret);
	alvium_write(alvium, REG_BCRM_REG_3, baz, &ret);

	if (ret)
		return ret;

	...

	return 0;

If you have to read multiple registers, you can also do

	u64 mul, div;
	int ret = 0;

	alvium_read(alvium, REG_BCRM_MULTIPLER, &mul, &ret);
	alvium_read(alvium, REG_BCRM_DIVIDER, &div, &ret);

	if (ret)
		return ret;

	alvium->value = mul / div;

	return 0;

I hope this is clearer.

> > > > > +	alvium->h_sup_csi_lanes = val;
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > 
> > [snip]

-- 
Regards,

Laurent Pinchart