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

From: Tommaso Merciai
Date: Wed Nov 22 2023 - 03:36:37 EST

Next message: Santhosh Kumar K: "[PATCH] dt-bindings: iio: adc: ti,am3359-adc: Allow dmas property to be optional"
Previous message: Ryan Roberts: "Re: [PATCH v2 12/14] arm64/mm: Wire up PTE_CONT for user mappings"
In reply to: Sakari Ailus: "Re: [PATCH v13 3/3] media: i2c: Add support for alvium camera"
Next in thread: Sakari Ailus: "Re: [PATCH v13 3/3] media: i2c: Add support for alvium camera"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Sakari,
Thanks for your comments.

On Wed, Nov 22, 2023 at 07:39:40AM +0000, Sakari Ailus wrote:
> Hi Tommaso,
>
> On Tue, Nov 21, 2023 at 12:08:57PM +0100, Tommaso Merciai wrote:
> > Hi Sakari,
> > Thanks for your comments.
> >
> > On Thu, Nov 16, 2023 at 10:41:17AM +0000, Sakari Ailus wrote:
> > > Hi Tommaso,
> > >
> > > On Thu, Nov 16, 2023 at 09:05:51AM +0100, Tommaso Merciai wrote:
> > > > Hi Sakari,
> > > > Thanks for your review!
> > > > Some comments on my side :)
> > >
> > > Thanks. Please also see my comment on releasing alvium->ep.
> > >
> > > >
> > > > On Thu, Nov 09, 2023 at 09:33:32AM +0000, Sakari Ailus wrote:
> > > > > Hi Tommaso,
> > > > >
> > > > > Reviewed again. There are quite a few matters remaining I've commented on
> > > > > previously --- please address all comments before posting a new version of
> > > > > the set. There are a few new findings, too, some related to spots that have
> > > > > been now cleaned up a bit.
> > > > >
> > > > > That being said, the remaining issues are fairly localised. The big picture
> > > > > starts to look better already.
> > > > >
> > > > > On Mon, Nov 06, 2023 at 09:20:58AM +0100, 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@xxxxxxxxx>
> > > > > > ---
> > > > > > 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@xxxxxxxxx>
> > > > > >
> > > > > > 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
> > > > > >
> > > > > > Changes since v10:
> > > > > > - Fixed alignment as suggested by SAilus
> > > > > > - Fixed alvium_read pattern over the driver as suggested by LPinchart
> > > > > > - Fixed alvium_set_csi_clk
> > > > > > - Fixed counters types of alvium_setup_mipi_fmt as suggested by SAilus
> > > > > > - Fixed alvium_set_frame_rate now don't use local var as suggested by SAilus
> > > > > > - Added pm_runtime_put into alvium_s_stream as suggested by SAilus
> > > > > > - Fixed alvium_g_volatile_ctrl as suggested by SAilus
> > > > > >
> > > > > > Changes since v11:
> > > > > > - Fixed kmalloc_array alignment in alvium_setup_mipi_fmt as suggested by CJAILLET
> > > > > > - Fixed alvium_s_frame_interval: return ret instead of -EIO as suggested by CJAILLET
> > > > > > - Fixed alvium_power_on: useless init ret var as suggested by CJAILLET
> > > > > > - Fixed missing space in alvium_power_on function as suggested by CJAILLET
> > > > > > - Fixed probe function print, from now driver use dev_err_probe as suggested by CJAILLET
> > > > > > - Add missing alvium_subdev_cleanup into alvium_remove function as suggested by CJAILLET
> > > > > >
> > > > > > Changes since v12:
> > > > > > - Fixed alvium_remove function as suggested by CJAILLET/SAilus
> > > > > >
> > > > > > MAINTAINERS | 9 +
> > > > > > drivers/media/i2c/Kconfig | 10 +
> > > > > > drivers/media/i2c/Makefile | 1 +
> > > > > > drivers/media/i2c/alvium-csi2.c | 2637 +++++++++++++++++++++++++++++++
> > > > > > drivers/media/i2c/alvium-csi2.h | 488 ++++++
> > > > > > 5 files changed, 3145 insertions(+)
> > > > > > create mode 100644 drivers/media/i2c/alvium-csi2.c
> > > > > > create mode 100644 drivers/media/i2c/alvium-csi2.h
> > > > > >
> > > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > > index f3e6dbbbbccb..98d322880c96 100644
> > > > > > --- a/MAINTAINERS
> > > > > > +++ b/MAINTAINERS
> > > > > > @@ -709,6 +709,15 @@ S: Maintained
> > > > > > F: Documentation/devicetree/bindings/media/allegro,al5e.yaml
> > > > > > F: drivers/media/platform/allegro-dvt/
> > > > > >
> > > > > > +ALLIED VISION ALVIUM CAMERA DRIVER
> > > > > > +M: Tommaso Merciai <tomm.merciai@xxxxxxxxx>
> > > > > > +M: Martin Hecht <martin.hecht@xxxxxxxx>
> > > > > > +L: linux-media@xxxxxxxxxxxxxxx
> > > > > > +S: Maintained
> > > > > > +F: Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
> > > > > > +F: drivers/media/i2c/alvium-csi2.c
> > > > > > +F: drivers/media/i2c/alvium-csi2.h
> > > > > > +
> > > > > > ALLWINNER A10 CSI DRIVER
> > > > > > M: Maxime Ripard <mripard@xxxxxxxxxx>
> > > > > > L: linux-media@xxxxxxxxxxxxxxx
> > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > > index d182c3514fb5..8525cfde26ba 100644
> > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > @@ -41,6 +41,16 @@ config VIDEO_APTINA_PLL
> > > > > > config VIDEO_CCS_PLL
> > > > > > tristate
> > > > > >
> > > > > > +config VIDEO_ALVIUM_CSI2
> > > > > > + tristate "Allied Vision ALVIUM MIPI CSI-2 camera support"
> > > > > > + select V4L2_CCI_I2C
> > > > > > + help
> > > > > > + This is a Video4Linux2 sensor-level driver for the Allied Vision
> > > > > > + ALVIUM camera connected via MIPI CSI-2 interface.
> > > > > > +
> > > > > > + To compile this driver as a module, choose M here: the
> > > > > > + module will be called alvium-csi2.
> > > > > > +
> > > > > > config VIDEO_AR0521
> > > > > > tristate "ON Semiconductor AR0521 sensor support"
> > > > > > help
> > > > > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > > > > > index f5010f80a21f..d75aa7f74315 100644
> > > > > > --- a/drivers/media/i2c/Makefile
> > > > > > +++ b/drivers/media/i2c/Makefile
> > > > > > @@ -17,6 +17,7 @@ obj-$(CONFIG_VIDEO_ADV7604) += adv7604.o
> > > > > > obj-$(CONFIG_VIDEO_ADV7842) += adv7842.o
> > > > > > obj-$(CONFIG_VIDEO_AK7375) += ak7375.o
> > > > > > obj-$(CONFIG_VIDEO_AK881X) += ak881x.o
> > > > > > +obj-$(CONFIG_VIDEO_ALVIUM_CSI2) += alvium-csi2.o
> > > > > > obj-$(CONFIG_VIDEO_APTINA_PLL) += aptina-pll.o
> > > > > > obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
> > > > > > obj-$(CONFIG_VIDEO_BT819) += bt819.o
> > > > > > diff --git a/drivers/media/i2c/alvium-csi2.c b/drivers/media/i2c/alvium-csi2.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..b089673d6ef4
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/media/i2c/alvium-csi2.c
> > > > > > @@ -0,0 +1,2637 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * Allied Vision Technologies GmbH Alvium camera driver
> > > > > > + *
> > > > > > + * Copyright (C) 2023 Tommaso Merciai
> > > > > > + * Copyright (C) 2023 Martin Hecht
> > > > > > + * Copyright (C) 2023 Avnet EMG GmbH
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/i2c.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/pm_runtime.h>
> > > > > > +#include <linux/regmap.h>
> > > > > > +#include <linux/regulator/consumer.h>
> > > > > > +#include <media/mipi-csi2.h>
> > > > > > +#include <media/v4l2-async.h>
> > > > > > +#include <media/v4l2-ctrls.h>
> > > > > > +#include <media/v4l2-device.h>
> > > > > > +#include <media/v4l2-event.h>
> > > > > > +#include <media/v4l2-fwnode.h>
> > > > > > +#include <media/v4l2-subdev.h>
> > > > > > +
> > > > > > +#include "alvium-csi2.h"
> > > > > > +
> > > > > > +static const struct v4l2_mbus_framefmt alvium_csi2_default_fmt = {
> > > > > > + .code = MEDIA_BUS_FMT_UYVY8_1X16,
> > > > > > + .width = 640,
> > > > > > + .height = 480,
> > > > > > + .colorspace = V4L2_COLORSPACE_SRGB,
> > > > > > + .ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB),
> > > > > > + .quantization = V4L2_QUANTIZATION_FULL_RANGE,
> > > > > > + .xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SRGB),
> > > > > > + .field = V4L2_FIELD_NONE,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct alvium_pixfmt alvium_csi2_fmts[] = {
> > > > > > + {
> > > > > > + /* UYVY8_2X8 */
> > > > > > + .id = ALVIUM_FMT_UYVY8_2X8,
> > > > > > + .code = MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > > + .colorspace = V4L2_COLORSPACE_SRGB,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_YUV422_8,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_NONE,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_YUV422_8B,
> > > > > > + .bay_fmt_regval = -1,
> > > > > > + .is_raw = 0,
> > > > > > + }, {
> > > > > > + /* UYVY8_1X16 */
> > > > > > + .id = ALVIUM_FMT_UYVY8_1X16,
> > > > > > + .code = MEDIA_BUS_FMT_UYVY8_1X16,
> > > > > > + .colorspace = V4L2_COLORSPACE_SRGB,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_YUV422_8,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_NONE,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_YUV422_8B,
> > > > > > + .bay_fmt_regval = -1,
> > > > > > + .is_raw = 0,
> > > > > > + }, {
> > > > > > + /* YUYV8_1X16 */
> > > > > > + .id = ALVIUM_FMT_YUYV8_1X16,
> > > > > > + .code = MEDIA_BUS_FMT_YUYV8_1X16,
> > > > > > + .colorspace = V4L2_COLORSPACE_SRGB,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_YUV422_8,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_NONE,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_YUV422_8B,
> > > > > > + .bay_fmt_regval = -1,
> > > > > > + .is_raw = 0,
> > > > > > + }, {
> > > > > > + /* YUYV8_2X8 */
> > > > > > + .id = ALVIUM_FMT_YUYV8_2X8,
> > > > > > + .code = MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > > + .colorspace = V4L2_COLORSPACE_SRGB,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_YUV422_8,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_NONE,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_YUV422_8B,
> > > > > > + .bay_fmt_regval = -1,
> > > > > > + .is_raw = 0,
> > > > > > + }, {
> > > > > > + /* YUYV10_1X20 */
> > > > > > + .id = ALVIUM_FMT_YUYV10_1X20,
> > > > > > + .code = MEDIA_BUS_FMT_YUYV10_1X20,
> > > > > > + .colorspace = V4L2_COLORSPACE_SRGB,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_YUV422_10,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_NONE,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_YUV422_10B,
> > > > > > + .bay_fmt_regval = -1,
> > > > > > + .is_raw = 0,
> > > > > > + }, {
> > > > > > + /* RGB888_1X24 */
> > > > > > + .id = ALVIUM_FMT_RGB888_1X24,
> > > > > > + .code = MEDIA_BUS_FMT_RGB888_1X24,
> > > > > > + .colorspace = V4L2_COLORSPACE_SRGB,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RGB888,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_NONE,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RGB888,
> > > > > > + .bay_fmt_regval = -1,
> > > > > > + .is_raw = 0,
> > > > > > + }, {
> > > > > > + /* RBG888_1X24 */
> > > > > > + .id = ALVIUM_FMT_RBG888_1X24,
> > > > > > + .code = MEDIA_BUS_FMT_RBG888_1X24,
> > > > > > + .colorspace = V4L2_COLORSPACE_SRGB,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RGB888,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_NONE,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RGB888,
> > > > > > + .bay_fmt_regval = -1,
> > > > > > + .is_raw = 0,
> > > > > > + }, {
> > > > > > + /* BGR888_1X24 */
> > > > > > + .id = ALVIUM_FMT_BGR888_1X24,
> > > > > > + .code = MEDIA_BUS_FMT_BGR888_1X24,
> > > > > > + .colorspace = V4L2_COLORSPACE_SRGB,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RGB888,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_NONE,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RGB888,
> > > > > > + .bay_fmt_regval = -1,
> > > > > > + .is_raw = 0,
> > > > > > + }, {
> > > > > > + /* RGB888_3X8 */
> > > > > > + .id = ALVIUM_FMT_RGB888_3X8,
> > > > > > + .code = MEDIA_BUS_FMT_RGB888_3X8,
> > > > > > + .colorspace = V4L2_COLORSPACE_SRGB,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RGB888,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_NONE,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RGB888,
> > > > > > + .bay_fmt_regval = -1,
> > > > > > + .is_raw = 0,
> > > > > > + }, {
> > > > > > + /* Y8_1X8 */
> > > > > > + .id = ALVIUM_FMT_Y8_1X8,
> > > > > > + .code = MEDIA_BUS_FMT_Y8_1X8,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW8,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_MONO,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW8,
> > > > > > + .bay_fmt_regval = 0x00,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SGRBG8_1X8 */
> > > > > > + .id = ALVIUM_FMT_SGRBG8_1X8,
> > > > > > + .code = MEDIA_BUS_FMT_SGRBG8_1X8,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW8,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_GR,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW8,
> > > > > > + .bay_fmt_regval = 0x01,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SRGGB8_1X8 */
> > > > > > + .id = ALVIUM_FMT_SRGGB8_1X8,
> > > > > > + .code = MEDIA_BUS_FMT_SRGGB8_1X8,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW8,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_RG,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW8,
> > > > > > + .bay_fmt_regval = 0x02,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SGBRG8_1X8 */
> > > > > > + .id = ALVIUM_FMT_SGBRG8_1X8,
> > > > > > + .code = MEDIA_BUS_FMT_SGBRG8_1X8,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW8,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_GB,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW8,
> > > > > > + .bay_fmt_regval = 0x03,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SBGGR8_1X8 */
> > > > > > + .id = ALVIUM_FMT_SBGGR8_1X8,
> > > > > > + .code = MEDIA_BUS_FMT_SBGGR8_1X8,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW8,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_BG,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW8,
> > > > > > + .bay_fmt_regval = 0x04,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* Y10_1X10 */
> > > > > > + .id = ALVIUM_FMT_Y10_1X10,
> > > > > > + .code = MEDIA_BUS_FMT_Y10_1X10,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW10,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_MONO,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW10,
> > > > > > + .bay_fmt_regval = 0x00,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SGRBG10_1X10 */
> > > > > > + .id = ALVIUM_FMT_SGRBG10_1X10,
> > > > > > + .code = MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW10,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_GR,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW10,
> > > > > > + .bay_fmt_regval = 0x01,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SRGGB10_1X10 */
> > > > > > + .id = ALVIUM_FMT_SRGGB10_1X10,
> > > > > > + .code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW10,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_RG,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW10,
> > > > > > + .bay_fmt_regval = 0x02,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SGBRG10_1X10 */
> > > > > > + .id = ALVIUM_FMT_SGBRG10_1X10,
> > > > > > + .code = MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW10,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_GB,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW10,
> > > > > > + .bay_fmt_regval = 0x03,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SBGGR10_1X10 */
> > > > > > + .id = ALVIUM_FMT_SBGGR10_1X10,
> > > > > > + .code = MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW10,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_BG,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW10,
> > > > > > + .bay_fmt_regval = 0x04,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* Y12_1X12 */
> > > > > > + .id = ALVIUM_FMT_Y12_1X12,
> > > > > > + .code = MEDIA_BUS_FMT_Y12_1X12,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW12,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_MONO,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW12,
> > > > > > + .bay_fmt_regval = 0x00,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SGRBG12_1X12 */
> > > > > > + .id = ALVIUM_FMT_SGRBG12_1X12,
> > > > > > + .code = MEDIA_BUS_FMT_SGRBG12_1X12,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW12,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_GR,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW12,
> > > > > > + .bay_fmt_regval = 0x01,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SRGGB12_1X12 */
> > > > > > + .id = ALVIUM_FMT_SRGGB12_1X12,
> > > > > > + .code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW12,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_RG,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW12,
> > > > > > + .bay_fmt_regval = 0x02,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SGBRG12_1X12 */
> > > > > > + .id = ALVIUM_FMT_SGBRG12_1X12,
> > > > > > + .code = MEDIA_BUS_FMT_SGBRG12_1X12,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW12,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_GB,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW12,
> > > > > > + .bay_fmt_regval = 0x03,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SBGGR12_1X12 */
> > > > > > + .id = ALVIUM_FMT_SBGGR12_1X12,
> > > > > > + .code = MEDIA_BUS_FMT_SBGGR12_1X12,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW12,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_BG,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW12,
> > > > > > + .bay_fmt_regval = 0x04,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SBGGR14_1X14 */
> > > > > > + .id = ALVIUM_FMT_SBGGR14_1X14,
> > > > > > + .code = MEDIA_BUS_FMT_SBGGR14_1X14,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW14,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_GR,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW14,
> > > > > > + .bay_fmt_regval = 0x01,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SGBRG14_1X14 */
> > > > > > + .id = ALVIUM_FMT_SGBRG14_1X14,
> > > > > > + .code = MEDIA_BUS_FMT_SGBRG14_1X14,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW14,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_RG,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW14,
> > > > > > + .bay_fmt_regval = 0x02,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SRGGB14_1X14 */
> > > > > > + .id = ALVIUM_FMT_SRGGB14_1X14,
> > > > > > + .code = MEDIA_BUS_FMT_SRGGB14_1X14,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW14,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_GB,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW14,
> > > > > > + .bay_fmt_regval = 0x03,
> > > > > > + .is_raw = 1,
> > > > > > + }, {
> > > > > > + /* SGRBG14_1X14 */
> > > > > > + .id = ALVIUM_FMT_SGRBG14_1X14,
> > > > > > + .code = MEDIA_BUS_FMT_SGRBG14_1X14,
> > > > > > + .colorspace = V4L2_COLORSPACE_RAW,
> > > > > > + .fmt_av_bit = ALVIUM_BIT_RAW14,
> > > > > > + .bay_av_bit = ALVIUM_BIT_BAY_BG,
> > > > > > + .mipi_fmt_regval = MIPI_CSI2_DT_RAW14,
> > > > > > + .bay_fmt_regval = 0x04,
> > > > > > + .is_raw = 1,
> > > > > > + },
> > > > > > + { /* sentinel */ }
> > > > > > +};
> > > > > > +
> > > > > > +static int alvium_read(struct alvium_dev *alvium, u32 reg, u64 *val, int *err)
> > > > > > +{
> > > > > > + if (reg & REG_BCRM_V4L2) {
> > > > > > + reg &= ~REG_BCRM_V4L2;
> > > > > > + reg += alvium->bcrm_addr;
> > > > > > + }
> > > > > > +
> > > > > > + return cci_read(alvium->regmap, reg, val, err);
> > > > > > +}
> > > > > > +
> > > > > > +static int alvium_write(struct alvium_dev *alvium, u32 reg, u64 val, int *err)
> > > > > > +{
> > > > > > + if (reg & REG_BCRM_V4L2) {
> > > > > > + reg &= ~REG_BCRM_V4L2;
> > > > > > + reg += alvium->bcrm_addr;
> > > > > > + }
> > > > > > +
> > > > > > + return cci_write(alvium->regmap, reg, val, err);
> > > > > > +}
> > > > > > +
> > > > > > +static int alvium_write_hshake(struct alvium_dev *alvium, u32 reg, u64 val)
> > > > > > +{
> > > > > > + struct device *dev = &alvium->i2c_client->dev;
> > > > > > + u64 hshake_bit;
> > > > > > + int ret = 0;
> > > > > > +
> > > > > > + /* reset handshake bit and write alvium reg */
> > > > > > + alvium_write(alvium, REG_BCRM_WRITE_HANDSHAKE_RW, 0, &ret);
> > > > > > + alvium_write(alvium, reg, val, &ret);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "Fail to write reg\n");
> > > > > > + return ret;
> > > > > > + }
> > > > > > +
> > > > > > + /* poll handshake bit since bit0 = 1 */
> > > > > > + read_poll_timeout(alvium_read, hshake_bit,
> > > > > > + ((hshake_bit & BCRM_HANDSHAKE_W_DONE_EN_BIT) == 1),
> > > > > > + 15000, 45000, true,
> > > > > > + alvium, REG_BCRM_WRITE_HANDSHAKE_RW, &hshake_bit, &ret);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "poll bit[0] = 1, hshake reg fail\n");
> > > > > > + return ret;
> > > > > > + }
> > > > > > +
> > > > > > + /* reset handshake bit, write 0 to bit0 */
> > > > > > + alvium_write(alvium, REG_BCRM_WRITE_HANDSHAKE_RW, 0, &ret);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "Fail to reset hshake reg\n");
> > > > > > + return ret;
> > > > > > + }
> > > > > > +
> > > > > > + /* poll handshake bit since bit0 = 0 */
> > > > > > + read_poll_timeout(alvium_read, hshake_bit,
> > > > > > + ((hshake_bit & BCRM_HANDSHAKE_W_DONE_EN_BIT) == 0),
> > > > > > + 15000, 45000, true,
> > > > > > + alvium, REG_BCRM_WRITE_HANDSHAKE_RW, &hshake_bit, &ret);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "poll bit[0] = 0, hshake reg fail\n");
> > > > > > + return ret;
> > > > > > + }
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int alvium_get_bcrm_vers(struct alvium_dev *alvium)
> > > > > > +{
> > > > > > + struct device *dev = &alvium->i2c_client->dev;
> > > > > > + struct alvium_bcrm_vers *v;
> > > > > > + u64 val;
> > > > > > + int ret;
> > > > > > +
> > > > > > + ret = alvium_read(alvium, REG_BCRM_VERSION_R, &val, NULL);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > +
> > > > > > + v = (struct alvium_bcrm_vers *) &val;
> > > > >
> > > > > No space before "&" in type cast, please. The same elsewhere.
> > > > >
> > > > > As you cast a single value to a struct, I think the struct field values
> > > > > will be swapped on BE systems. You'll need to convert each value
> > > > > separately. Same for struct alvium_fw_vers below.
> > > >
> > > > What about:
> > > >
> > > > v->minor = le16_to_cpu(v->minor);
> > > > v->major = le16_to_cpu(v->major);
> > > >
> > > > here. I posted this solution in some previous v :)
> > >
> > > You shouldn't assign it to a field marked little endian. Instead, use
> > > le16_to_cpu() when you access the data below.
> > >
> > > If you want to access the struct in the driver without using the conversion
> > > macros, you should read the data one field at a time (and use u16 instead
> > > of __le16 type for the fields).
> >
> > It's fine with your suggestion thanks.
> > I'm moving to use the following to prints those values:
> >
> > v = (struct alvium_bcrm_vers *)&val;
> >
> > dev_info(dev, "bcrm version: %u.%u\n",
> > le16_to_cpu(v->minor), le16_to_cpu(v->major));
> >
> > thanks for the hint.
>
> Sorry, I forgot alvium_read(), via cci_read(), already does endianness
> conversion here, from big endian to CPU endianness. Is there a need to do
> further conversion here? Noting that le16_to_cpu() does nothing on little
> endian systems, is it necessary here?
>
> The options here are either changing the struct fields to CPU endianness
> and reading them individually or accessing the register as a single 32-bit
> value.
>
> I'd do the former, it's easier to access them that way in the driver.
>
> The same applies to BCRM version below.
>
> struct alvium_bcrm_vers {
> u16 minor;
> u16 major;
> };

Understood, thanks.

Then to resume just compared to v13 I just need to
revert the alvium_bcrm_vers/alvium_fw_vers struct
to:

struct alvium_bcrm_vers {
u16 minor;
u16 major;
};

struct alvium_fw_vers {
u8 special;
u8 major;
u16 minor;
u16 patch;
};

>
> >
> >
> > >
> > > >
> > > > >
> > > > > > + dev_info(dev, "bcrm version: %u.%u\n", v->minor, v->major);
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int alvium_get_fw_version(struct alvium_dev *alvium)
> > > > > > +{
> > > > > > + struct device *dev = &alvium->i2c_client->dev;
> > > > > > + struct alvium_fw_vers *fw_v;
> > > > > > + u64 val;
> > > > > > + int ret;
> > > > > > +
> > > > > > + ret = alvium_read(alvium, REG_BCRM_DEVICE_FIRMWARE_VERSION_R, &val, NULL);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > +
> > > > > > + fw_v = (struct alvium_fw_vers *) &val;
> > > > > > + dev_info(dev, "fw version: %u.%u.%u.%u\n", fw_v->special, fw_v->major,
> > > > > > + fw_v->minor, fw_v->patch);
> > > > >
> > > > > Ditto.
> > > >
> > > > Same here:
> > > >
> > > > I think we just need:
> > > >
> > > > fw_v->minor = le16_to_cpu(fw_v->minor);
> > > > fw_v->patch = le32_to_cpu(fw_v->patch);
> > >
> > > Same as earlier.
> >
> > Same here:
> >
> > fw_v = (struct alvium_fw_vers *)&val;
> >
> > dev_info(dev, "fw version: %u.%u.%u.%u\n",
> > fw_v->special, fw_v->major,
> > le16_to_cpu(fw_v->minor), le32_to_cpu(fw_v->patch));
> ...
>
> > > > > > + if ((!alvium_csi2_fmts[fmt].is_raw) ||
> > > > > > + (alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit]))
> > > > > > + sz++;
> > > > > > + }
> > > > > > +
> > > > > > + /* init alvium_csi2_fmt array */
> > > > > > + alvium->alvium_csi2_fmt_n = sz;
> > > > > > + alvium->alvium_csi2_fmt = kmalloc_array(sz, sizeof(struct alvium_pixfmt),
> > > > >
> > > > > Wrap after "=".
> > > > >
> > > > > sizeof(*alvium->alvium_csi2_fmt)
> > > > >
> > > > > > + GFP_KERNEL);
> > > >
> > > > alvium->alvium_csi2_fmt =
> > > > kmalloc_array(sz, sizeof(struct alvium_pixfmt), GFP_KERNEL);
> > >
> > > One tab stop is enough here, meaning the line will be less than 80
> > > characters.
> > >
> > > > ?
> > > >
> > > > >
> > > > > Where is this released?
> > > >
> > > > You are completely right actually is missing.
> > > > I think I need to add kfree into the probe function:
> > > >
> > > > ret = alvium_setup_mipi_fmt(alvium);
> > > > if (ret) {
> > > > dev_err_probe(dev, ret, "setup_mipi_fmt fail\n");
> > > > kfree(alvium->alvium_csi2_fmt);
> > >
> > > Don't do this here but in error handling, under an appropriate label. Note
> > > that kfree(NULL) is a nop.
> > >
> > > I think it'd be less error-prone to do this under whatever label that you
> > > use next, rather than putting it to alvium_subdev_cleanup().
> > >
> > > Ideally alvium_setup_mipi_fmt() would release it by itself in error case.
> > >
> > > > goto err_powerdown;
> > > > }
> > > >
> > > > and at the end of the alvium_subdev_cleanup function
> >
> > Some comments on alvium_setup_mipi_fmt:
> >
> > alvium_setup_mipi_fmt can't fail right now don't need to check the return
> > value my plan is to add the following pointer check:
> >
> > alvium->alvium_csi2_fmt =
> > kmalloc_array(sz, sizeof(struct alvium_pixfmt), GFP_KERNEL);
> > if (!alvium->alvium_csi2_fmt)
> > return -ENOMEM;
> >
> > Then the call will be:
> >
> > ret = alvium_setup_mipi_fmt(alvium);
> > if (ret) {
> > dev_err_probe(dev, ret, "setup_mipi_fmt fail\n");
> > goto err_powerdown;
> > }
> >
> > For freeing this what about adding kfree into err_pm label of probe
> > function. This is the clean way that jump in my mind right now.
> >
> > err_subdev:
> > alvium_subdev_cleanup(alvium);
> > err_pm:
> > pm_runtime_disable(dev);
> > pm_runtime_put_noidle(dev);
> > kfree(alvium->alvium_csi2_fmt);
> > err_powerdown:
> > alvium_set_power(alvium, false);
> >
> > return ret;
> >
> >
> > I think also kfree(alvium->alvium_csi2_fmt);
> > at the end of the alvium_subdev_cleanup is the only way that we have no?
>
> You can't put it there as you're already freeing it under err_pm label
> above.

Damn, yep. Thanks.

>
> Otherwise this seems good to me.

Perfect.

>
> >
> > Here I plan to free also to clean the alvium->ep as you suggested.
> >
> > static void alvium_subdev_cleanup(struct alvium_dev *alvium)
> > {
> > v4l2_fwnode_endpoint_free(&alvium->ep);
> > v4l2_subdev_cleanup(&alvium->sd);
> > media_entity_cleanup(&alvium->sd.entity);
> > v4l2_ctrl_handler_free(&alvium->ctrls.handler);
> > kfree(alvium->alvium_csi2_fmt);
> > }
> >
> > Right now I haven't find a way to put an err_free label as you
> > suggested. :'(
> >
> > What do you think?
> >
> > > >
> > > > static void alvium_subdev_cleanup(struct alvium_dev *alvium)
> > > > {
> > > > v4l2_subdev_cleanup(&alvium->sd);
> > > > media_entity_cleanup(&alvium->sd.entity);
> > > > v4l2_ctrl_handler_free(&alvium->ctrls.handler);
> > > > kfree(alvium->alvium_csi2_fmt);
> > > > }
> > > > What do you think?
> > > >
>
> ...
>
> > > > > > +static int alvium_init_cfg(struct v4l2_subdev *sd,
> > > > > > + struct v4l2_subdev_state *state)
> > > > > > +{
> > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > + struct alvium_mode *mode = &alvium->mode;
> > > > > > + struct v4l2_subdev_format sd_fmt = {
> > > > > > + .which = V4L2_SUBDEV_FORMAT_TRY,
> > > > > > + .format = alvium_csi2_default_fmt,
> > > > > > + };
> > > > > > + struct v4l2_subdev_crop sd_crop = {
> > > > > > + .which = V4L2_SUBDEV_FORMAT_TRY,
> > > > > > + .rect = {
> > > > > > + .left = mode->crop.left,
> > > > > > + .top = mode->crop.top,
> > > > > > + .width = mode->crop.width,
> > > > > > + .height = mode->crop.height,
> > > > > > + },
> > > > > > + };
> > > > > > +
> > > > > > + *v4l2_subdev_get_pad_crop(sd, state, 0) = sd_crop.rect;
> > > > > > + *v4l2_subdev_get_pad_format(sd, state, 0) = sd_fmt.format;
> > > > >
> > > > > Shouldn't the format have same width and height as crop? What about the
> > > > > mbus code?
> >
> > Can you clarify to me this comment pls :)
>
> The purpose of the init_cfg operation is to initialise the sub-device
> state, including format and crop rectangle (if applicable). The width and
> height fields of the format are not initialised above, leaving them zeros.

Mmmmm.
Why zeros?

.format = alvium_csi2_default_fmt

where:

static const struct v4l2_mbus_framefmt alvium_csi2_default_fmt = {
.code = MEDIA_BUS_FMT_UYVY8_1X16,
.width = 640,
.height = 480,
.colorspace = V4L2_COLORSPACE_SRGB,
.ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB),
.quantization = V4L2_QUANTIZATION_FULL_RANGE,
.xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SRGB),
.field = V4L2_FIELD_NONE,
};

> That doesn't seem to be a valid configuration, given that the crop
> rectangle is initiliased with different values.

Why different values?
crop is initialized into subdev_init

static int alvium_subdev_init(struct alvium_dev *alvium)
{
struct i2c_client *client = alvium->i2c_client;
struct device *dev = &alvium->i2c_client->dev;
struct v4l2_subdev *sd = &alvium->sd;
int ret;

/* Setup initial frame interval*/
alvium->frame_interval.numerator = 1;
alvium->frame_interval.denominator = ALVIUM_DEFAULT_FR_HZ;
alvium->fr = ALVIUM_DEFAULT_FR_HZ;

/* Setup the initial mode */
alvium->mode.fmt = alvium_csi2_default_fmt;
alvium->mode.width = alvium_csi2_default_fmt.width;
alvium->mode.height = alvium_csi2_default_fmt.height;
alvium->mode.crop.left = alvium->min_offx;
alvium->mode.crop.top = alvium->min_offy;
alvium->mode.crop.width = alvium_csi2_default_fmt.width;
alvium->mode.crop.height = alvium_csi2_default_fmt.height;
...

Then:

static int alvium_init_cfg(struct v4l2_subdev *sd,
struct v4l2_subdev_state *state)
{
struct alvium_dev *alvium = sd_to_alvium(sd);
struct alvium_mode *mode = &alvium->mode;
struct v4l2_subdev_format sd_fmt = {
.which = V4L2_SUBDEV_FORMAT_TRY,
.format = alvium_csi2_default_fmt,
};
struct v4l2_subdev_crop sd_crop = {
.which = V4L2_SUBDEV_FORMAT_TRY,
.rect = {
.left = mode->crop.left,
.top = mode->crop.top,
.width = mode->crop.width,
.height = mode->crop.height,
},
};
...

Seems that crop and format are using the same init values
or I'm wrong?
Mmm.. maybe I'm missing something?

Let me know

>
> >
> > > > >
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int alvium_set_fmt(struct v4l2_subdev *sd,
> > > > > > + struct v4l2_subdev_state *sd_state,
> > > > > > + struct v4l2_subdev_format *format)
> > > > > > +{
> > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > + const struct alvium_pixfmt *alvium_csi2_fmt;
> > > > > > + struct v4l2_mbus_framefmt *fmt;
> > > > > > + struct v4l2_rect *crop;
> > > > > > +
> > > > > > + fmt = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > > > > + crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > > > > +
> > > > > > + fmt->width = clamp(format->format.width, alvium->img_min_width,
> > > > > > + alvium->img_max_width);
> > > > > > + fmt->height = clamp(format->format.height, alvium->img_min_height,
> > > > > > + alvium->img_max_height);
> > > > > > +
> > > > > > + /* Adjust left and top to prevent roll over sensor area */
> > > > > > + crop->left = clamp((u32)crop->left, (u32)0,
> > > > > > + (alvium->img_max_width - fmt->width));
> > > > > > + crop->top = clamp((u32)crop->top, (u32)0,
> > > > > > + (alvium->img_max_height - fmt->height));
> > > > > > +
> > > > > > + /* Set also the crop width and height when set a new fmt */
> > > > > > + crop->width = fmt->width;
> > > > > > + crop->height = fmt->height;
> > > > > > +
> > > > > > + alvium_csi2_fmt = alvium_code_to_pixfmt(alvium, format->format.code);
> > > > > > + fmt->code = alvium_csi2_fmt->code;
> > > > > > +
> > > > > > + *fmt = format->format;
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int alvium_set_selection(struct v4l2_subdev *sd,
> > > > > > + struct v4l2_subdev_state *sd_state,
> > > > > > + struct v4l2_subdev_selection *sel)
> > > > > > +{
> > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > + struct v4l2_mbus_framefmt *fmt;
> > > > > > + struct v4l2_rect *crop;
> > > > > > +
> > > > > > + crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > > > > + fmt = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > > > > +
> > > > > > + /*
> > > > > > + * Alvium can only shift the origin of the img
> > > > > > + * then we accept only value with the same value of the actual fmt
> > > > > > + */
> > > > > > + if (sel->r.width != fmt->width)
> > > > > > + sel->r.width = fmt->width;
> > > > > > +
> > > > > > + if (sel->r.height != fmt->height)
> > > > > > + sel->r.height = fmt->height;
> > > > > > +
> > > > > > + if (sel->target != V4L2_SEL_TGT_CROP)
> > > > > > + return -EINVAL;
> > > > >
> > > > > This should be the first thing to test.
> > > >
> > > > Oks
> > > >
> > > > >
> > > > > > +
> > > > > > + /* alvium don't accept negative crop left/top */
> > > > > > + crop->left = clamp((u32)max(0, sel->r.left), alvium->min_offx,
> > > > > > + alvium->img_max_width - sel->r.width);
> > > > > > + crop->top = clamp((u32)max(0, sel->r.top), alvium->min_offy,
> > > > > > + alvium->img_max_height - sel->r.height);
> > > > > > +
> > > > > > + sel->r = *crop;
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int alvium_get_selection(struct v4l2_subdev *sd,
> > > > > > + struct v4l2_subdev_state *sd_state,
> > > > > > + struct v4l2_subdev_selection *sel)
> > > > > > +{
> > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > +
> > > > > > + switch (sel->target) {
> > > > > > + /* Current cropping area */
> > > > > > + case V4L2_SEL_TGT_CROP:
> > > > > > + sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > > > > + break;
> > > > > > + /* Cropping bounds */
> > > > > > + case V4L2_SEL_TGT_NATIVE_SIZE:
> > > > > > + sel->r.top = 0;
> > > > > > + sel->r.left = 0;
> > > > > > + sel->r.width = alvium->img_max_width;
> > > > > > + sel->r.height = alvium->img_max_height;
> > > > > > + break;
> > > > > > + /* Default cropping area */
> > > > > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > > > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > > > > + sel->r.top = alvium->min_offy;
> > > > > > + sel->r.left = alvium->min_offx;
> > > > > > + sel->r.width = alvium->img_max_width;
> > > > > > + sel->r.height = alvium->img_max_height;
> > > > > > + break;
> > > > > > + default:
> > > > > > + return -EINVAL;
> > > > > > + }
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int alvium_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > +{
> > > > > > + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > + int val;
> > > > > > +
> > > > > > + switch (ctrl->id) {
> > > > > > + case V4L2_CID_GAIN:
> > > > > > + val = alvium_get_gain(alvium);
> > > > > > + if (val < 0)
> > > > > > + return val;
> > > > > > + alvium->ctrls.gain->val = val;
> > > > > > + break;
> > > > > > + case V4L2_CID_EXPOSURE:
> > > > > > + val = alvium_get_exposure(alvium);
> > > > > > + if (val < 0)
> > > > > > + return val;
> > > > > > + alvium->ctrls.exposure->val = val;
> > > > > > + break;
> > > > > > + }
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int alvium_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > +{
> > > > > > + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > + struct i2c_client *client = v4l2_get_subdevdata(&alvium->sd);
> > > > > > + int ret;
> > > > > > +
> > > > > > + /*
> > > > > > + * Applying V4L2 control value only happens
> > > > > > + * when power is up for streaming
> > > > > > + */
> > > > > > + if (!pm_runtime_get_if_in_use(&client->dev))
> > > > > > + return 0;
> > > > > > +
> > > > > > + switch (ctrl->id) {
> > > > > > + case V4L2_CID_AUTOGAIN:
> > > > > > + ret = alvium_set_ctrl_gain(alvium, ctrl->val);
> > > > >
> > > > > ret = alvium_set_autogain(alvium, ctrl->val);
> > > > >
> > > >
> > > > Pls check [1]
> > > >
> > > > > Where do you set the manual gain value? What about the manual exposure
> > > > > value? Both appear to be missing here.
> > > > >
> > > > > How have you tested this?
> > > > >
> > > > > > + break;
> > > > > > + case V4L2_CID_EXPOSURE_AUTO:
> > > > > > + ret = alvium_set_ctrl_exposure(alvium, ctrl->val);
> > > > >
> > > > > ret = alvium_set_autoexposure(alvium, ctrl->val);
> > > > >
> > > > > You're still missing grabbing the manual controls when the corresponding
> > > > > automatic control is enabled. I've commented on the same matter previously.
> > > >
> > > > Same comment in [1]
> > > >
> > > > >
> > > > > > + break;
> > > > > > + case V4L2_CID_AUTO_WHITE_BALANCE:
> > > > > > + ret = alvium_set_ctrl_white_balance(alvium, ctrl->val);
> > > > > > + break;
> > > > > > + case V4L2_CID_HUE:
> > > > > > + ret = alvium_set_ctrl_hue(alvium, ctrl->val);
> > > > > > + break;
> > > > > > + case V4L2_CID_CONTRAST:
> > > > > > + ret = alvium_set_ctrl_contrast(alvium, ctrl->val);
> > > > > > + break;
> > > > > > + case V4L2_CID_SATURATION:
> > > > > > + ret = alvium_set_ctrl_saturation(alvium, ctrl->val);
> > > > > > + break;
> > > > > > + case V4L2_CID_GAMMA:
> > > > > > + ret = alvium_set_ctrl_gamma(alvium, ctrl->val);
> > > > > > + break;
> > > > > > + case V4L2_CID_SHARPNESS:
> > > > > > + ret = alvium_set_ctrl_sharpness(alvium, ctrl->val);
> > > > > > + break;
> > > > > > + case V4L2_CID_HFLIP:
> > > > > > + ret = alvium_set_ctrl_hflip(alvium, ctrl->val);
> > > > > > + break;
> > > > > > + case V4L2_CID_VFLIP:
> > > > > > + ret = alvium_set_ctrl_vflip(alvium, ctrl->val);
> > > > > > + break;
> > > > > > + default:
> > > > > > + ret = -EINVAL;
> > > > > > + break;
> > > > > > + }
> > > > > > +
> > > > > > + pm_runtime_put(&client->dev);
> > > > > > +
> > > > > > + return ret;
> > > > > > +}

Here I plan to switch to:

static int alvium_s_ctrl(struct v4l2_ctrl *ctrl)
{
struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
struct alvium_dev *alvium = sd_to_alvium(sd);
struct i2c_client *client = v4l2_get_subdevdata(&alvium->sd);
int ret;

/*
* Applying V4L2 control value only happens
* when power is up for streaming
*/
if (!pm_runtime_get_if_in_use(&client->dev))
return 0;

switch (ctrl->id) {
case V4L2_CID_GAIN:
ret = alvium_set_ctrl_gain(alvium, ctrl->val);
break;
case V4L2_CID_AUTOGAIN:
ret = alvium_set_ctrl_auto_gain(alvium, ctrl->val);
break;
case V4L2_CID_EXPOSURE:
ret = alvium_set_ctrl_exposure(alvium, ctrl->val);
break;
case V4L2_CID_EXPOSURE_AUTO:
ret = alvium_set_ctrl_auto_exposure(alvium, ctrl->val);
break;
case V4L2_CID_RED_BALANCE:
ret = alvium_set_ctrl_red_balance_ratio(alvium, ctrl->val);
break;
case V4L2_CID_BLUE_BALANCE:
ret = alvium_set_ctrl_blue_balance_ratio(alvium, ctrl->val);
break;
case V4L2_CID_AUTO_WHITE_BALANCE:
ret = alvium_set_ctrl_awb(alvium, ctrl->val);
break;
case V4L2_CID_HUE:
ret = alvium_set_ctrl_hue(alvium, ctrl->val);
break;
case V4L2_CID_CONTRAST:
ret = alvium_set_ctrl_contrast(alvium, ctrl->val);
break;
case V4L2_CID_SATURATION:
ret = alvium_set_ctrl_saturation(alvium, ctrl->val);
break;
case V4L2_CID_GAMMA:
ret = alvium_set_ctrl_gamma(alvium, ctrl->val);
break;
case V4L2_CID_SHARPNESS:
ret = alvium_set_ctrl_sharpness(alvium, ctrl->val);
break;
case V4L2_CID_HFLIP:
ret = alvium_set_ctrl_hflip(alvium, ctrl->val);
break;
case V4L2_CID_VFLIP:
ret = alvium_set_ctrl_vflip(alvium, ctrl->val);
break;
default:
ret = -EINVAL;
break;
}

pm_runtime_put(&client->dev);

return ret;
}

Like you suggested.
I think is more clean/understandable.
Thanks for your comment.

> > > > > > +
> > > > > > +static const struct v4l2_ctrl_ops alvium_ctrl_ops = {
> > > > > > + .g_volatile_ctrl = alvium_g_volatile_ctrl,
> > > > > > + .s_ctrl = alvium_s_ctrl,
> > > > > > +};
> > > > > > +
> > > > > > +static int alvium_ctrl_init(struct alvium_dev *alvium)
> > > > > > +{
> > > > > > + const struct v4l2_ctrl_ops *ops = &alvium_ctrl_ops;
> > > > > > + struct alvium_ctrls *ctrls = &alvium->ctrls;
> > > > > > + struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> > > > > > + struct v4l2_fwnode_device_properties props;
> > > > > > + int ret;
> > > > > > +
> > > > > > + v4l2_ctrl_handler_init(hdl, 32);
> > > > > > +
> > > > > > + /* Pixel rate is fixed */
> > > > > > + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_PIXEL_RATE, 0,
> > > > > > + ALVIUM_DEFAULT_PIXEL_RATE_MHZ, 1,
> > > > > > + ALVIUM_DEFAULT_PIXEL_RATE_MHZ);
> > > > > > +
> > > > > > + /* Link freq is fixed */
> > > > > > + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops,
> > > > > > + V4L2_CID_LINK_FREQ,
> > > > > > + 0, 0, &alvium->link_freq);
> > > > > > +
> > > > > > + /* Auto/manual white balance */
> > > > > > + ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_AUTO_WHITE_BALANCE,
> > > > > > + 0, 1, 1,
> > > > > > + alvium->avail_ft.auto_whiteb ? 1 : 0);
> > > > > > +
> > > > > > + ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_BLUE_BALANCE,
> > > > > > + alvium->min_bbalance,
> > > > > > + alvium->max_bbalance,
> > > > > > + alvium->inc_bbalance,
> > > > > > + alvium->dft_bbalance);
> > > > > > + ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_RED_BALANCE,
> > > > > > + alvium->min_rbalance,
> > > > > > + alvium->max_rbalance,
> > > > > > + alvium->inc_rbalance,
> > > > > > + alvium->dft_rbalance);
> > > > > > +
> > > > > > + /* Auto/manual exposure */
> > > > > > + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> > > > > > + V4L2_CID_EXPOSURE_AUTO,
> > > > > > + V4L2_EXPOSURE_MANUAL, 0,
> > > > > > + alvium->avail_ft.auto_exp ?
> > > > > > + V4L2_EXPOSURE_AUTO : V4L2_EXPOSURE_MANUAL);
> > > > > > +
> > > > > > + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_EXPOSURE,
> > > > > > + alvium->min_exp,
> > > > > > + alvium->max_exp,
> > > > > > + alvium->inc_exp,
> > > > > > + alvium->dft_exp);
> > > > > > +
> > > > > > + /* Auto/manual gain */
> > > > > > + ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_AUTOGAIN,
> > > > > > + 0, 1, 1,
> > > > > > + alvium->avail_ft.auto_gain ? 1 : 0);
> > > > > > +
> > > > > > + if (alvium->avail_ft.gain)
> > > > > > + ctrls->gain = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_GAIN,
> > > > > > + alvium->min_gain,
> > > > > > + alvium->max_gain,
> > > > > > + alvium->inc_gain,
> > > > > > + alvium->dft_gain);
> > > > > > +
> > > > > > + if (alvium->avail_ft.sat)
> > > > > > + ctrls->saturation = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_SATURATION,
> > > > > > + alvium->min_sat,
> > > > > > + alvium->max_sat,
> > > > > > + alvium->inc_sat,
> > > > > > + alvium->dft_sat);
> > > > > > +
> > > > > > + if (alvium->avail_ft.hue)
> > > > > > + ctrls->hue = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_HUE,
> > > > > > + alvium->min_hue,
> > > > > > + alvium->max_hue,
> > > > > > + alvium->inc_hue,
> > > > > > + alvium->dft_hue);
> > > > > > +
> > > > > > + if (alvium->avail_ft.contrast)
> > > > > > + ctrls->contrast = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_CONTRAST,
> > > > > > + alvium->min_contrast,
> > > > > > + alvium->max_contrast,
> > > > > > + alvium->inc_contrast,
> > > > > > + alvium->dft_contrast);
> > > > > > +
> > > > > > + if (alvium->avail_ft.gamma)
> > > > > > + ctrls->gamma = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_GAMMA,
> > > > > > + alvium->min_gamma,
> > > > > > + alvium->max_gamma,
> > > > > > + alvium->inc_gamma,
> > > > > > + alvium->dft_gamma);
> > > > > > +
> > > > > > + if (alvium->avail_ft.sharp)
> > > > > > + ctrls->sharpness = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_SHARPNESS,
> > > > > > + alvium->min_sharp,
> > > > > > + alvium->max_sharp,
> > > > > > + alvium->inc_sharp,
> > > > > > + alvium->dft_sharp);
> > > > > > +
> > > > > > + if (alvium->avail_ft.rev_x)
> > > > > > + ctrls->hflip = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_HFLIP,
> > > > >
> > > > > Fits on previous line.
> > > > >
> > > > > > + 0, 1, 1, 0);
> > > > > > +
> > > > > > + if (alvium->avail_ft.rev_y)
> > > > > > + ctrls->vflip = v4l2_ctrl_new_std(hdl, ops,
> > > > > > + V4L2_CID_VFLIP,
> > > > >
> > > > > Ditto.
> > > > >
> > > > > > + 0, 1, 1, 0);
> > > > > > +
> > > >
> > > > Oks
> > > > > > + if (hdl->error) {
> > > > > > + ret = hdl->error;
> > > > > > + goto free_ctrls;
> > > > > > + }
> > > > > > +
> > > > > > + ret = v4l2_fwnode_device_parse(&alvium->i2c_client->dev, &props);
> > > > > > + if (ret)
> > > > > > + goto free_ctrls;
> > > > > > +
> > > > > > + ret = v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> > > > > > + if (ret)
> > > > > > + goto free_ctrls;
> > > > > > +
> > > > > > + ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > > > + ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > > > + ctrls->gain->flags |= V4L2_CTRL_FLAG_VOLATILE;
> > > > > > + ctrls->exposure->flags |= V4L2_CTRL_FLAG_VOLATILE;
> > > > > > +
> > > > > > + v4l2_ctrl_auto_cluster(3, &ctrls->auto_wb, 0, false);
> > > > > > + v4l2_ctrl_auto_cluster(2, &ctrls->auto_gain, 0, true);
> > > > > > + v4l2_ctrl_auto_cluster(2, &ctrls->auto_exp, 1, true);
> > > > > > +
> > > > > > + alvium->sd.ctrl_handler = hdl;
> > > > > > + return 0;
> > > > > > +
> > > > > > +free_ctrls:
> > > > > > + v4l2_ctrl_handler_free(hdl);
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct v4l2_subdev_core_ops alvium_core_ops = {
> > > > > > + .log_status = v4l2_ctrl_subdev_log_status,
> > > > > > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > > > > > + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct v4l2_subdev_video_ops alvium_video_ops = {
> > > > > > + .g_frame_interval = alvium_g_frame_interval,
> > > > > > + .s_frame_interval = alvium_s_frame_interval,
> > > > > > + .s_stream = alvium_s_stream,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct v4l2_subdev_pad_ops alvium_pad_ops = {
> > > > > > + .init_cfg = alvium_init_cfg,
> > > > > > + .enum_mbus_code = alvium_enum_mbus_code,
> > > > > > + .get_fmt = v4l2_subdev_get_fmt,
> > > > > > + .set_fmt = alvium_set_fmt,
> > > > > > + .get_selection = alvium_get_selection,
> > > > > > + .set_selection = alvium_set_selection,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct v4l2_subdev_ops alvium_subdev_ops = {
> > > > > > + .core = &alvium_core_ops,
> > > > > > + .pad = &alvium_pad_ops,
> > > > > > + .video = &alvium_video_ops,
> > > > > > +};
> > > > > > +
> > > > > > +static int alvium_subdev_init(struct alvium_dev *alvium)
> > > > > > +{
> > > > > > + struct i2c_client *client = alvium->i2c_client;
> > > > > > + struct device *dev = &alvium->i2c_client->dev;
> > > > > > + struct v4l2_subdev *sd = &alvium->sd;
> > > > > > + int ret;
> > > > > > +
> > > > > > + /* Setup initial frame interval*/
> > > > > > + alvium->frame_interval.numerator = 1;
> > > > > > + alvium->frame_interval.denominator = ALVIUM_DEFAULT_FR_HZ;
> > > > > > + alvium->fr = ALVIUM_DEFAULT_FR_HZ;
> > > > > > +
> > > > > > + /* Setup the initial mode */
> > > > > > + alvium->mode.fmt = alvium_csi2_default_fmt;
> > > > > > + alvium->mode.width = alvium_csi2_default_fmt.width;
> > > > > > + alvium->mode.height = alvium_csi2_default_fmt.height;
> > > > > > + alvium->mode.crop.left = alvium->min_offx;
> > > > > > + alvium->mode.crop.top = alvium->min_offy;
> > > > > > + alvium->mode.crop.width = alvium_csi2_default_fmt.width;
> > > > > > + alvium->mode.crop.height = alvium_csi2_default_fmt.height;
> > > > > > +
> > > > > > + /* init alvium sd */
> > > > > > + v4l2_i2c_subdev_init(sd, client, &alvium_subdev_ops);
> > > > > > +
> > > > > > + sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > > > > + alvium->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > > > > + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > > > > > +
> > > > > > + ret = media_entity_pads_init(&sd->entity, 1, &alvium->pad);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "Could not register media entity\n");
> > > > > > + return ret;
> > > > > > + }
> > > > > > +
> > > > > > + ret = alvium_ctrl_init(alvium);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "Control initialization error %d\n", ret);
> > > > > > + goto entity_cleanup;
> > > > > > + }
> > > > > > +
> > > > > > + alvium->sd.state_lock = alvium->ctrls.handler.lock;
> > > > > > +
> > > > > > + ret = v4l2_subdev_init_finalize(sd);
> > > > > > + if (ret < 0) {
> > > > > > + dev_err(dev, "subdev initialization error %d\n", ret);
> > > > > > + goto err_ctrls;
> > > > > > + }
> > > > > > +
> > > > > > + return 0;
> > > > > > +
> > > > > > +err_ctrls:
> > > > > > + v4l2_ctrl_handler_free(&alvium->ctrls.handler);
> > > > > > +entity_cleanup:
> > > > > > + media_entity_cleanup(&alvium->sd.entity);
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static void alvium_subdev_cleanup(struct alvium_dev *alvium)
> > > > > > +{
> > > > > > + v4l2_subdev_cleanup(&alvium->sd);
> > > > > > + media_entity_cleanup(&alvium->sd.entity);
> > > > > > + v4l2_ctrl_handler_free(&alvium->ctrls.handler);
> > > > > > +}
> > > > > > +
> > > > > > +static int alvium_get_dt_data(struct alvium_dev *alvium)
> > > > > > +{
> > > > > > + struct device *dev = &alvium->i2c_client->dev;
> > > > > > + struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > > > > + struct fwnode_handle *endpoint;
> > > > > > +
> > > > > > + if (!fwnode)
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > + /* Only CSI2 is supported for now: */
> > > > > > + alvium->ep.bus_type = V4L2_MBUS_CSI2_DPHY;
> > > > > > +
> > > > > > + endpoint = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> > > > > > + if (!endpoint) {
> > > > > > + dev_err(dev, "endpoint node not found\n");
> > > > > > + return -EINVAL;
> > > > > > + }
> > > > > > +
> > > > > > + if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &alvium->ep)) {
> > > > > > + dev_err(dev, "could not parse endpoint\n");
> > > > > > + goto error_out;
> > > > > > + }
> > > > > > +
> > > > > > + if (!alvium->ep.nr_of_link_frequencies) {
> > > > > > + dev_err(dev, "no link frequencies defined");
> > > > > > + goto error_out;
> > > > > > + }
> > > > > > +
> > > > > > + return 0;
> > > > > > +
> > > > > > +error_out:
> > > > > > + v4l2_fwnode_endpoint_free(&alvium->ep);
> > >
> > > You're missing a call to release this in successful case.
> >
> > As written before what about freeing this into?
> >
> > static void alvium_subdev_cleanup(struct alvium_dev *alvium)
> > {
> > v4l2_fwnode_endpoint_free(&alvium->ep);
> > v4l2_subdev_cleanup(&alvium->sd);
> > media_entity_cleanup(&alvium->sd.entity);
> > v4l2_ctrl_handler_free(&alvium->ctrls.handler);
> > kfree(alvium->alvium_csi2_fmt);
> > }
>
> I think putting v4l2_fwnode_endpoint_free() there should be fine, as long
> as alvium_subdev_cleanup() is called late enough in probe error handling to
> qualify.

Perfect, thanks.
I saw also others drivers that are using this way.

>
> --
> Regards,
>
> Sakari Ailus

Thanks & regards,
Tommaso

Next message: Santhosh Kumar K: "[PATCH] dt-bindings: iio: adc: ti,am3359-adc: Allow dmas property to be optional"
Previous message: Ryan Roberts: "Re: [PATCH v2 12/14] arm64/mm: Wire up PTE_CONT for user mappings"
In reply to: Sakari Ailus: "Re: [PATCH v13 3/3] media: i2c: Add support for alvium camera"
Next in thread: Sakari Ailus: "Re: [PATCH v13 3/3] media: i2c: Add support for alvium camera"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]