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

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

Hi Tommaso,

A few more comments below...

On Fri, Nov 24, 2023 at 10:30:07AM +0100, Tommaso Merciai wrote:

...

> +static int alvium_get_bcrm_vers(struct alvium_dev *alvium)
> +{
> +	struct device *dev = &alvium->i2c_client->dev;
> +	struct alvium_bcrm_vers *v;
> +	u64 val;
> +	int ret;
> +
> +	ret = alvium_read(alvium, REG_BCRM_VERSION_R, &val, NULL);
> +	if (ret)
> +		return ret;
> +
> +	v = (struct alvium_bcrm_vers *)&val;

You're still reading the entire struct using a single read. :-( This won't
work on a BE machine as while the struct fields are in the same memory
locations, the respective data in a single 64-bit value is not.

> +
> +	dev_info(dev, "bcrm version: %u.%u\n", v->minor, v->major);
> +
> +	return 0;
> +}
> +
> +static int alvium_get_fw_version(struct alvium_dev *alvium)
> +{
> +	struct device *dev = &alvium->i2c_client->dev;
> +	struct alvium_fw_vers *fw_v;
> +	u64 val;
> +	int ret;
> +
> +	ret = alvium_read(alvium, REG_BCRM_DEVICE_FIRMWARE_VERSION_R, &val, NULL);
> +	if (ret)
> +		return ret;
> +
> +	fw_v = (struct alvium_fw_vers *)&val;

Same here.

> +
> +	dev_info(dev, "fw version: %u.%u.%u.%u\n", fw_v->special, fw_v->major,
> +		 fw_v->minor, fw_v->patch);
> +
> +	return 0;
> +}
> +
> +static int alvium_get_bcrm_addr(struct alvium_dev *alvium)
> +{
> +	u64 val;
> +	int ret;
> +
> +	ret = alvium_read(alvium, REG_BCRM_REG_ADDR_R, &val, NULL);
> +	if (ret)
> +		return ret;
> +
> +	alvium->bcrm_addr = val;
> +
> +	return 0;
> +}

...

> +static int alvium_setup_mipi_fmt(struct alvium_dev *alvium)
> +{
> +	unsigned int avail_fmt_cnt = 0;
> +	unsigned int fmt = 0;
> +	size_t sz = 0;
> +
> +	alvium->alvium_csi2_fmt = NULL;

This seems to be unnnecessary: the field is assigned below without using it
(obviously).

> +
> +	/* 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])
> +			continue;
> +
> +		if ((!alvium_csi2_fmts[fmt].is_raw) ||
> +		    (alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit]))
> +			sz++;
> +	}
> +
> +	/* init alvium_csi2_fmt array */
> +	alvium->alvium_csi2_fmt_n = sz;
> +	alvium->alvium_csi2_fmt =
> +		kmalloc_array(sz, sizeof(struct alvium_pixfmt), GFP_KERNEL);
> +	if (!alvium->alvium_csi2_fmt)
> +		return -ENOMEM;
> +
> +	/* 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])
> +			continue;
> +
> +		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;
> +}

...

> +static const struct alvium_pixfmt *
> +alvium_code_to_pixfmt(struct alvium_dev *alvium, u32 code)
> +{
> +	const struct alvium_pixfmt *formats = alvium->alvium_csi2_fmt;

I'd use alvium->alvium_csi2_fmt and not add a local variable. Up to you.

> +	unsigned int i;
> +
> +	for (i = 0; formats[i].code; ++i)
> +		if (formats[i].code == code)
> +			return &formats[i];
> +
> +	return &formats[0];
> +}
> +
> +static int alvium_set_mode(struct alvium_dev *alvium,
> +			   struct v4l2_subdev_state *state)
> +{
> +	struct v4l2_mbus_framefmt *fmt;
> +	struct v4l2_rect *crop;
> +	int ret;
> +
> +	crop = v4l2_subdev_state_get_crop(state, 0);
> +	fmt = v4l2_subdev_state_get_format(state, 0);
> +
> +	v4l_bound_align_image(&fmt->width, alvium->img_min_width,
> +			      alvium->img_max_width, 0,
> +			      &fmt->height, alvium->img_min_height,
> +			      alvium->img_max_height, 0, 0);
> +
> +	/* alvium don't accept negative crop left/top */
> +	crop->left = clamp((u32)max(0, crop->left), alvium->min_offx,
> +			   (u32)(alvium->img_max_width - fmt->width));
> +	crop->top = clamp((u32)max(0, crop->top), alvium->min_offy,
> +			  (u32)(alvium->img_max_height - fmt->height));
> +
> +	ret = alvium_set_img_width(alvium, fmt->width);
> +	if (ret)
> +		return ret;
> +
> +	ret = alvium_set_img_height(alvium, fmt->height);
> +	if (ret)
> +		return ret;
> +
> +	ret = alvium_set_img_offx(alvium, crop->left);
> +	if (ret)
> +		return ret;
> +
> +	ret = alvium_set_img_offy(alvium, crop->top);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

-- 
Kind regards,

Sakari Ailus