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

[Tommaso Merciai]

Hi Sakari,
Thanks for your comments.

On Mon, Oct 30, 2023 at 10:43:47PM +0000, Sakari Ailus wrote:
> Hi Tommaso,
> 
> On Mon, Oct 30, 2023 at 01:26:58PM +0100, Tommaso Merciai wrote:
> 
> ...
> 
> > > > +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; \
> > }
> > 
> 
> You could do something like (entirely untested):
> 
> #define ALVIUM_DECLARE_READ(sign, bits) \
> 	static int
> 	alvium_read_ ## sign ## bits(struct alvium_dev *alvium, u32 reg, \
> 				     sign ## bits *val, int *err) \
> 	{ \
> 		u64 val64; \
> 		int ret; \
> 			\
> 		if (err && *err < 0) \
> 			return *err; \
> 			\
> 		alvium_read(alvium, reg, &val64, &ret); \
> 		if (ret < 0) { \
> 			if (err) \
> 				*err = ret; \
> 			return ret; \
> 		}	\
> 			\
> 		*val = val64; \
> 			\
> 		return 0; \
> 	}
> 
> ALVIUM_DECLARE_READ(u, 32);
> 
> And then, e.g. instead of (and failing to check ret):
> 
> 	u64 val;
> 
> 	alvium_read(alvium, REG_BCRM_CONTRAST_VALUE_RW, &val, &ret);
> 	alvium->dft_contrast = val;
> 
> you'd have a single call:
> 
> 	alvium_read_u32(alvium, REG_BCRM_CONTRAST_VALUE_RW,
> 		        &alvium->dft_contrast, &ret);
> 
> And so on.
> 
> You can drop sign if you don't need signed reads but some of the struct
> fields you're writing something appear to be signed.
> 
> It'd be good to check the register size matches with the size of *val, too.
> Maybe something like:
> 
> WARN_ON((CCI_REG ## bits(0) && CCI_REG_WIDTH_MASK) >> CCI_REG_WIDTH_SHIFT
> 	!= sizeof(sign ## bits));

Laurent suggest me also a good way.
I switched to the Laurent suggested implementation in v11.
I think now is clear. Thanks to both again.
Let me know what do you think about :)

> 
> > > > +static int alvium_get_csi_clk_params(struct alvium_dev *alvium)
> > > > +{
> > > > +	u64 val;
> > > > +	int ret = 0;
> > > > +
> > > > +	alvium_read(alvium, REG_BCRM_CSI2_CLOCK_MIN_R, &val, &ret);
> > > > +	alvium->min_csi_clk = val;
> > > > +
> > > > +	alvium_read(alvium, REG_BCRM_CSI2_CLOCK_MAX_R, &val, &ret);
> > > > +	alvium->max_csi_clk = val;
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int alvium_set_csi_clk(struct alvium_dev *alvium)
> > > > +{
> > > > +	struct device *dev = &alvium->i2c_client->dev;
> > > > +	u64 csi_clk;
> > > > +	int ret;
> > > > +
> > > > +	csi_clk = (u32)alvium->ep.link_frequencies[0];
> > > 
> > > Why casting to u32? Shouldn't csi_clk be u32 instead?
> > 
> > Ok we fix this in v11.
> > Change to use u64 for calculation because type of ep.link_frequencies[0]
> > Plan is to clamp csi_clk between min/max instead of returning error.
> 
> I think I would keep it as-is: this isn't V4L2 UAPI.
> 
> > 
> > > 
> > > > +
> > > > +	if (csi_clk < alvium->min_csi_clk || csi_clk > alvium->max_csi_clk)
> > > > +		return -EINVAL;
> > > > +
> > > > +	ret = alvium_write_hshake(alvium, REG_BCRM_CSI2_CLOCK_RW, csi_clk);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Fail to set csi lanes reg\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	alvium->link_freq = alvium->ep.link_frequencies[0];
> > > > +
> > > > +	return 0;
> > > > +}
> 
> ...
> 
> > > > +			goto out;
> > > > +
> > > > +		ret = alvium_set_mode(alvium, state);
> > > > +		if (ret)
> > > > +			goto out;
> > > > +
> > > > +		fmt = v4l2_subdev_get_pad_format(sd, state, 0);
> > > > +		ret = alvium_set_framefmt(alvium, fmt);
> > > > +		if (ret)
> > > > +			goto out;
> > > > +
> > > > +		ret = alvium_set_stream_mipi(alvium, enable);
> > > > +		if (ret)
> > > > +			goto out;
> > > > +
> > > > +	} else {
> > > > +		alvium_set_stream_mipi(alvium, enable);
> > > > +		pm_runtime_mark_last_busy(&client->dev);
> > > > +		pm_runtime_put_autosuspend(&client->dev);
> > > 
> > > pm_runtime_put() here, too.
> > 
> > Here is not needed we already have pm_runtime_put_autosuspend.
> > I'm missing something?
> 
> Ah, I missed that while reviewing. Please ignore that comment then.

No problem, update in v11.

> 
> > 
> > > 
> > > > +	}
> > > > +
> > > > +	alvium->streaming = !!enable;
> > > > +	v4l2_subdev_unlock_state(state);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +out:
> > > > +	v4l2_subdev_unlock_state(state);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +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;
> > > 
> > > Init_cfg() is expected to be configuration independent (as much as
> > > possible). Therefore you should use defaults here, not current mode.
> > 
> > Defaults alvium mode already used here.
> 
> Ah, indeed. Please ignore.

No problem.

Thanks & Regards,
Tommaso

> 
> -- 
> Kind regards,
> 
> Sakari Ailus