Message-ID: <ZHR5qup6412nLgzz@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
Date:   Mon, 29 May 2023 12:08:42 +0200
From:   Tommaso Merciai <tomm.merciai@...il.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     jacopo.mondi@...asonboard.com, laurent.pinchart@...asonboard.com,
        martin.hecht@...et.eu, linuxfancy@...glegroups.com,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Marco Felsch <m.felsch@...gutronix.de>,
        Gerald Loacker <gerald.loacker@...fvision.net>,
        Nicholas Roth <nicholas@...hemail.net>,
        Shawn Tu <shawnx.tu@...el.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH v2 2/2] media: i2c: Add support for alvium camera

Hi Christophe,
Thanks for the review.

On Fri, May 26, 2023 at 08:39:44PM +0200, Christophe JAILLET wrote:
> Le 26/05/2023 à 19:39, Tommaso Merciai a écrit :
> > 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 sensor's 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>
> > ---
> 
> Hi,
> 
> a few nit below, should it help.
> 
> 
> > +static int alvium_setup_mipi_fmt(struct alvium_dev *alvium)
> > +{
> > +	int sz = 0;
> > +	int fmt = 0;
> 
> No need to init.
> If the loop index was just 'i', the code below would be slighly less
> verbose.
> 
> > +	int avail_fmt_cnt = 0;
> > +
> > +	alvium->alvium_csi2_fmt = NULL;
> > +
> > +	/* calculate fmt array size */
> > +	for (fmt = 0; fmt < ALVIUM_NUM_SUPP_MIPI_DATA_FMT; fmt++) {
> > +		if (alvium->is_mipi_fmt_avail[alvium_csi2_fmts[fmt].fmt_av_bit]) {
> > +			if (!alvium_csi2_fmts[fmt].is_raw) {
> > +				sz++;
> > +			} else if (alvium_csi2_fmts[fmt].is_raw &&
> > +			      alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit]) {
> 
> It is makes sense, this if/else looks equivalent to:
> 
> 			if (!alvium_csi2_fmts[fmt].is_raw ||
> 			    alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit]) {
> 				sz++;
> 
> The same simplification could also be applied in the for loop below.

I Don't agree on this.
This is not a semplification.
This change the logic of the statement.

Also initialization of sz and avail_fmt_cnt is needed.
Maybe I can include fmt variable initialization into the for loop:

for (int fmt = 0; fmt < ALVIUM_NUM_SUPP_MIPI_DATA_FMT; fmt++) {

But from my perspective this seems clear.

> 
> > +				sz++;
> > +			}
> > +		}
> > +	}
> > +
> > +	/* init alvium_csi2_fmt array */
> > +	alvium->alvium_csi2_fmt_n = sz;
> > +	alvium->alvium_csi2_fmt = kmalloc((
> > +						     sizeof(struct alvium_pixfmt) * sz),
> > +						     GFP_KERNEL);
> 
> kmalloc_array()?
> Also some unneeded ( and )

Thanks for this tip.

> 
> > +
> > +	/* Create the alvium_csi2 fmt array from formats available */
> > +	for (fmt = 0; fmt < ALVIUM_NUM_SUPP_MIPI_DATA_FMT; fmt++) {
> > +		if (alvium->is_mipi_fmt_avail[alvium_csi2_fmts[fmt].fmt_av_bit]) {
> > +			if (!alvium_csi2_fmts[fmt].is_raw) {
> > +				alvium->alvium_csi2_fmt[avail_fmt_cnt] =
> > +					alvium_csi2_fmts[fmt];
> > +				avail_fmt_cnt++;
> > +			} else if (alvium_csi2_fmts[fmt].is_raw &&
> > +			      alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit]) {
> > +				alvium->alvium_csi2_fmt[avail_fmt_cnt] =
> > +					alvium_csi2_fmts[fmt];
> > +				avail_fmt_cnt++;
> > +			}
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> 
> [...]
> 
> > +struct alvium_mode {
> > +	struct v4l2_rect crop;
> > +	struct v4l2_mbus_framefmt fmt;
> > +	u32 width;
> > +	u32 height;
> > +
> 
> Useless NL.

Right, thanks.

> 
> > +};
> > +
> > +struct alvium_pixfmt {
> > +	u8 id;
> > +	u32 code;
> > +	u32 colorspace;
> > +	u8 fmt_av_bit;
> > +	u8 bay_av_bit;
> > +	u64 mipi_fmt_regval;
> > +	u64 bay_fmt_regval;
> > +	u8 is_raw;
> 
> If moved a few lines above, there would be less holes in the struct.

?

> 
> > +};
> > +
> 
> [...]
> 
> > +struct alvium_dev {
> > +	struct i2c_client *i2c_client;
> > +	struct v4l2_subdev sd;
> > +	struct v4l2_fwnode_endpoint ep;
> > +	struct media_pad pad;
> > +
> > +	struct mutex lock;
> > +
> > +	struct gpio_desc *reset_gpio;
> > +	struct gpio_desc *pwdn_gpio;
> > +
> > +	u16 bcrm_addr;
> > +	alvium_bcrm_vers_t bcrm_v;
> > +	alvium_fw_vers_t fw_v;
> > +
> > +	alvium_avail_feat_t avail_ft;
> > +	u8 is_mipi_fmt_avail[ALVIUM_NUM_SUPP_MIPI_DATA_BIT];
> > +	u8 is_bay_avail[ALVIUM_NUM_BAY_AV_BIT];
> > +
> > +	u32 min_csi_clk;
> > +	u32 max_csi_clk;
> > +	u32 img_min_width;
> > +	u32 img_max_width;
> > +	u32 img_inc_width;
> > +	u32 img_min_height;
> > +	u32 img_max_height;
> > +	u32 img_inc_height;
> > +	u32 min_offx;
> > +	u32 max_offx;
> > +	u32 inc_offx;
> > +	u32 min_offy;
> > +	u32 max_offy;
> > +	u32 inc_offy;
> > +	u64 min_gain;
> > +	u64 max_gain;
> > +	u64 inc_gain;
> > +	u64 min_exp;
> > +	u64 max_exp;
> > +	u64 inc_exp;
> > +	u64 min_rbalance;
> > +	u64 max_rbalance;
> > +	u64 inc_rbalance;
> > +	u64 min_bbalance;
> > +	u64 max_bbalance;
> > +	u64 inc_bbalance;
> > +	s32 min_hue;
> > +	s32 max_hue;
> > +	s32 inc_hue;
> > +	u32 min_contrast;
> > +	u32 max_contrast;
> > +	u32 inc_contrast;
> > +	u32 min_sat;
> > +	u32 max_sat;
> > +	u32 inc_sat;
> > +	s32 min_black_lvl;
> > +	s32 max_black_lvl;
> > +	s32 inc_black_lvl;
> > +	u64 min_gamma;
> > +	u64 max_gamma;
> > +	u64 inc_gamma;
> > +	s32 min_sharp;
> > +	s32 max_sharp;
> > +	s32 inc_sharp;
> > +
> > +	u32 streamon_delay;
> > +
> > +	struct alvium_mode mode;
> > +	struct v4l2_fract frame_interval;
> > +	u64 min_fr;
> > +	u64 max_fr;
> > +	u64 fr;
> > +
> > +	u8 h_sup_csi_lanes;
> > +	struct clk *xclk;
> > +	u32 xclk_freq;
> > +	u32 csi_clk;
> > +	u64 link_freq;
> > +
> > +	struct alvium_ctrls ctrls;
> > +
> > +	u8 bcrm_mode;
> > +	u8 hshake_bit;
> 
> What is the need of keeping this value in the struct?
> Its usage seems to be only local to some function (read from HW, then used)
> 
> Should it be kept, does it make sense to have it a u8:1 and maybe some !! in
> the code, to pack it with the bitfield just a few lines below.

I don't agree on this.
Those variable are not used only locally.
Also v4l2 ctrls use those variables.
We need to keep into the priv struct of the driver.

> 
> 
> > +
> > +	struct alvium_pixfmt *alvium_csi2_fmt;
> > +	u8 alvium_csi2_fmt_n;
> > +	struct v4l2_mbus_framefmt fmt;
> > +
> > +	u8 streaming:1;
> > +	u8 apply_fiv:1;
> > +
> > +	bool upside_down;
> 
> This looks only written. Is it useles or here for future use?
> Can these fields be all u8:1, or bool:1 ?

Rotation is not used.
I will drop this in v3. Thanks!

Regards,
Tommaso

> 
> CJ
> 
> > +};
> > +
> > +static inline struct alvium_dev *sd_to_alvium(struct v4l2_subdev *sd)
> > +{
> > +	return container_of(sd, struct alvium_dev, sd);
> > +}
> > +
> > +static inline struct alvium_dev *i2c_to_alvium(struct i2c_client *client)
> > +{
> > +	return sd_to_alvium(i2c_get_clientdata(client));
> > +}
> > +
> > +static inline bool alvium_is_csi2(const struct alvium_dev *alvium)
> > +{
> > +	return alvium->ep.bus_type == V4L2_MBUS_CSI2_DPHY;
> > +}
> > +
> > +static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> > +{
> > +	return &container_of(ctrl->handler, struct alvium_dev,
> > +					  ctrls.handler)->sd;
> > +}
> > +#endif /* ALVIUM_H_ */
> 

Powered by blists - more mailing lists