[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231030133739.GL12144@pendragon.ideasonboard.com>
Date: Mon, 30 Oct 2023 15:37:39 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Tommaso Merciai <tomm.merciai@...il.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, martin.hecht@...et.eu,
michael.roeder@...et.eu, mhecht73@...il.com,
linuxfancy@...glegroups.com,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Hans de Goede <hdegoede@...hat.com>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
Marco Felsch <m.felsch@...gutronix.de>,
Gerald Loacker <gerald.loacker@...fvision.net>,
Andy Shevchenko <andy.shevchenko@...il.com>,
Daniel Scally <djrscally@...il.com>,
Shawn Tu <shawnx.tu@...el.com>,
Linus Walleij <linus.walleij@...aro.org>,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH v10 3/3] media: i2c: Add support for alvium camera
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@...il.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@...el.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
Powered by blists - more mailing lists