lkml.org 
[lkml]   [2023]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@gmail.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_ */
>

\
 
 \ /
  Last update: 2023-05-29 12:09    [W:0.228 / U:0.476 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site