Re: [PATCH v2 2/2] media: i2c: Add support for alvium camera

[Christophe JAILLET]

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@xxxxxxxxx>
> ---

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.

> +				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 )

> +
> +	/* 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.

> +};
> +
> +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.


> +
> +	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 ?

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_ */