Re: [PATCH v4 2/2] i2c: qcom-target: Add driver for Qualcomm I2C target controller

[Andi Shyti]

Hi Viken,

...

> +static void qcom_i2c_target_write_requested(struct qcom_i2c_target *target)
> +{
> +	u8 val = 0;
> +
> +	if (!test_and_set_bit(WRITE_IN_PROGRESS, &target->status)) {
> +		dev_dbg(target->dev, "Write phase started\n");
> +		i2c_slave_event(target->slave, I2C_SLAVE_WRITE_REQUESTED, &val);

It's the second time in two days that I'm reviewing this:
slave_event can fail and when it fails we need to nack the next
write requests (check slave-interface.rst).

> +	}
> +}

...

> +static int qcom_i2c_target_reg_slave(struct i2c_client *slave)

Please, use the inclusive name:

master -> adapter
slave  -> target

> +{
> +	struct qcom_i2c_target *target = i2c_get_adapdata(slave->adapter);
> +
> +	if (target->slave)
> +		return -EBUSY;

...

> +static int qcom_i2c_target_icc_init(struct qcom_i2c_target *target)
> +{
> +	int ret;
> +
> +	target->icc_path = devm_of_icc_get(target->dev, NULL);
> +	if (IS_ERR(target->icc_path))
> +		return dev_err_probe(target->dev, PTR_ERR(target->icc_path),
> +				     "failed to get ICC path\n");
> +
> +	/*
> +	 * The controller only needs interconnect bandwidth while it is
> +	 * powered. The vote is placed here and across resume with
> +	 * icc_set_bw(), and dropped with icc_set_bw(path, 0, 0) on suspend
> +	 * and remove, so the vote and unvote are always symmetric.
> +	 */
> +	ret = icc_set_bw(target->icc_path, APPS_PROC_TO_I2C_TARGET_VOTE,
> +			 APPS_PROC_TO_I2C_TARGET_VOTE);

TARGET_VOTE is in bytes per second, while icc_set_bw expects
kilobytes per second.

Thanks,
Andi

> +	if (ret)
> +		return dev_err_probe(target->dev, ret, "icc_set_bw failed\n");
> +
> +	return 0;
> +}