Re: [PATCH v3 1/3] media: i2c: cvs: Add driver of Intel Computer Vision Sensing Controller(CVS)

[Mehdi Djait <mehdi.djait@linux.intel.com>]

Hi Miguel,

Thank you for the patch!

A couple of comments below.

On Thu, May 21, 2026 at 03:23:57PM -0700, Miguel Vadillo wrote:

[..]

> +
> +/* I2C transport helpers */
> +
> +/**
> + * cvs_read_i2c - Issue a read-type command and fetch device response
> + * @ctx: CVS device context
> + * @cmd_id: Command identifier (big endian)
> + * @resp: Destination buffer for response payload
> + * @size: Size of payload to read into @resp (without prefix)
> + *
> + * Sends @cmd_id and reads back the response in a single I2C transaction.
> + * When the device prepends a 4-byte protocol prefix, the combined
> + * prefix+payload is read into a temporary buffer and only the payload is
> + * copied to @resp, avoiding any dependency on the layout of the caller's
> + * buffer.
> + *
> + * Return: 0 on success or negative errno.
> + */
> +static int cvs_read_i2c(struct icvs *ctx, __be16 cmd_id, void *resp,
> +			size_t size)
> +{
> +	size_t prefix_size = ctx->prefix ? sizeof(u32) : 0;
> +	size_t read_size = size + prefix_size;
> +	struct i2c_client *i2c = ctx->i2c_client;
> +	u8 *buf;

use this instead:
u8 *buf __free(kfree) = NULL;

and then remove the two calls to kfree() below.

> +	int cnt;
> +
> +	if (!resp || !size)
> +		return -EINVAL;
> +
> +	cnt = i2c_master_send(i2c, (const char *)&cmd_id, sizeof(cmd_id));
> +	if (cnt != sizeof(cmd_id))
> +		return cnt < 0 ? cnt : -EIO;
> +
> +	buf = kmalloc(read_size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	cnt = i2c_master_recv(i2c, buf, read_size);
> +	if (cnt != read_size) {
> +		dev_dbg(cvs_dev(ctx), "recv cmd 0x%04x short read (%d/%zu)\n",
> +			be16_to_cpu(cmd_id), cnt, read_size);
> +		kfree(buf);
> +		return cnt < 0 ? cnt : -EIO;
> +	}
> +
> +	memcpy(resp, buf + prefix_size, size);
> +	kfree(buf);
> +
> +	return 0;
> +}

[..]

> +/**
> + * cvs_send - Common command submission path
> + * @ctx: CVS device context
> + * @cmd: Command buffer (icvs_cmd) with cmd_id and param populated
> + * @len: Buffer length
> + *
> + * Dispatches a set of supported commands:
> + * - ICVS_SET_DEV_HOST_ID,
> + * - ICVS_HOST_SENSOR_OWNER,
> + * - ICVS_HOST_SET_MIPI_CONFIG
> + * - ICVS_FW_LOADER_*
> + *
> + * For I2C based commands it sets big-endian cmd ids, writes to the device
> + * and waits (via delayed work) for completion or timeout.
> + * GPIO based ownership toggles are handled locally.
> + *
> + * Caller must hold ctx->lock when invoking this function and check for i2c
> + * bus availability.
> + *
> + * Return: 0 on success, negative errno, -EINVAL for unsupported command
> + * or status from device in ctx->wq_resp.
> + */
> +int cvs_send(struct icvs *ctx, struct icvs_cmd *cmd, size_t len)
> +{
> +	int ret, status = 0;
> +
> +	dev_dbg(cvs_dev(ctx), "send cmd = 0x%04x", be16_to_cpu(cmd->cmd_id));

From where I see this function being called, it needs to hold the
ctx->lock. It is also mentioned in the documentation of function

Let's add a lockdep_assert_held() to make sure of it at the beginning of
cvs_send()

> +
> +	reinit_completion(&ctx->cmd_completion);
> +
> +	switch (be16_to_cpu(cmd->cmd_id)) {
> +	case ICVS_SET_DEV_HOST_ID:
> +		cmd->cmd_id = cpu_to_be16(ICVS_SET_DEV_HOST_ID);
> +		ret = cvs_write_i2c(ctx, cmd, len);
> +		if (ret < 0)
> +			break;
> +
> +		ret = cvs_schedule_and_wait(ctx, FW_READY_DELAY_MS,
> +					    CMD_TIMEOUT);
> +		if (ret < 0)
> +			break;
> +
> +		status = ctx->wq_resp.resp.state &
> +			  ICVS_DEV_STATE_ERROR ? -EINVAL : 0;
> +		break;
> +	case ICVS_HOST_SENSOR_OWNER:
> +		gpiod_set_value_cansleep(ctx->req, cmd->param.param);
> +		fsleep(FW_READY_DELAY_MS * USEC_PER_MSEC);
> +		ret = gpiod_get_value_cansleep(ctx->resp);
> +		status = cmd->param.param == ret ? 0 : -EINVAL;
> +		ret = 0; /* success */
> +		break;
> +	case ICVS_HOST_SET_MIPI_CONFIG:
> +		cmd->cmd_id = cpu_to_be16(ICVS_HOST_SET_MIPI_CONFIG);
> +		ret = cvs_config_mipi(ctx, cmd, len);
> +		if (ret < 0)
> +			break;
> +
> +		ret = cvs_schedule_and_wait(ctx, FW_READY_DELAY_MS,
> +					    CMD_TIMEOUT);
> +		status = (ctx->wq_resp.resp.state &
> +			  ICVS_DEV_STATE_ERROR) ? -EINVAL : 0;
> +		break;
> +	case ICVS_FW_LOADER_START:
> +		cmd->cmd_id = cpu_to_be16(ICVS_FW_LOADER_START);
> +		ret = cvs_write_i2c(ctx, cmd, len);
> +		if (ret < 0)
> +			break;
> +
> +		ret = cvs_wait_wake_or_sleep(ctx, CMD_TIMEOUT,
> +					     FW_READY_DELAY_MS);
> +		if (ret)
> +			break;
> +
> +		ret = cvs_schedule_and_wait(ctx, FW_READY_DELAY_MS,
> +					    CMD_TIMEOUT);
> +		status = (ctx->wq_resp.resp.state &
> +			  ICVS_DEV_STATE_DOWNLOAD) ? 0 : -EINVAL;
> +		break;
> +	case ICVS_FW_LOADER_DATA:
> +		/* Quirk for older protocols */
> +		if (ctx->caps.protocol_version_major >= 2 &&
> +		    ctx->caps.protocol_version_minor >= 2) {
> +			cmd->cmd_id = cpu_to_be16(ICVS_FW_LOADER_DATA);
> +			ret = cvs_write_i2c(ctx, cmd, len);
> +		} else {
> +			ret = cvs_write_i2c(ctx, &cmd->param,
> +					    len - sizeof(cmd->cmd_id));
> +		}
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = cvs_wait_wake_or_sleep(ctx, FW_READY_DELAY_MS,
> +					     FW_READY_DELAY_MS);
> +		if (ret)
> +			break;
> +
> +		ret = cvs_schedule_and_wait(ctx, FW_READY_DELAY_MS,
> +					    CMD_TIMEOUT);
> +		status = ctx->wq_resp.resp.state &
> +			  ICVS_DEV_STATE_ERROR ? -EINVAL : 0;
> +		break;
> +	case ICVS_FW_LOADER_END:
> +		cmd->cmd_id = cpu_to_be16(ICVS_FW_LOADER_END);
> +		ret = cvs_write_i2c(ctx, cmd, len);
> +		if (ret < 0)
> +			break;
> +
> +		ret = cvs_wait_wake_or_sleep(ctx, CMD_TIMEOUT,
> +					     FW_READY_DELAY_MS);
> +		if (ret)
> +			break;
> +
> +		ret = cvs_schedule_and_wait(ctx, FW_READY_DELAY_MS,
> +					    CMD_TIMEOUT);
> +		status = !(ctx->wq_resp.resp.state &
> +			   ICVS_DEV_STATE_DOWNLOAD) ? 0 : -EINVAL;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return ctx->wq_resp.status = status;
> +}

[..]

> + * cvs_core_remove - Shared remove logic
> + * @dev: Device
> + */
> +static void cvs_core_remove(struct device *dev)
> +{
> +	struct icvs *ctx = dev_get_drvdata(dev);
> +
> +	cvs_csi_remove(ctx);

Shouldn't we cancel ctx work first ? And then do the CSI2 v4l2 cleanup ?

> +	cancel_delayed_work_sync(&ctx->work);
> +
> +	if (ctx->ipu_link)
> +		device_link_del(ctx->ipu_link);
> +
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +
> +	cvs_reset(ctx);
> +}
> +
> +/**
> + * cvs_remove - I2C driver remove
> + * @client: I2C client
> + */
> +static void cvs_remove(struct i2c_client *client)
> +{
> +	cvs_core_remove(&client->dev);

[..]

--
Kind Regards
Mehdi Djait