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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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

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

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

-- 
Kind regards,

Sakari Ailus