Re: [PATCH v1] i2c: microchip-core: actually use repeated sends

[Andi Shyti]

Hi Conor,

On Mon, Sep 30, 2024 at 02:38:27PM GMT, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> 
> At present, where repeated sends are intended to be used, the
> i2c-microchip-core driver sends a stop followed by a start. Lots of i2c
> devices must not malfunction in the face of this behaviour, because the
> driver has operated like this for years! Try to keep track of whether or
> not a repeated send is required, and suppress sending a stop in these
> cases.
> 
> Fixes: 64a6f1c4987e ("i2c: add support for microchip fpga i2c controllers")

I don't think the Fixes tag is needed here if everything worked
until now, unless you got some other device that requires this
change and you need to explain it.

If this is more an improvement (because it has worked), then we
shouldn't add the Fixes tag.

In any case, when patches are going to stable, we need to Cc
stable too.

Cc: <stable@xxxxxxxxxxxxxxx> # v6.0+

(This is specified in the
Documentation/process/stable-kernel-rules.rst and I'm starting to
enforce it here).

> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>

...

> +	/*
> +	 * If there's been an error, the isr needs to return control
> +	 * to the "main" part of the driver, so as not to keep sending
> +	 * messages once it completes and clears the SI bit.
> +	 */
> +	if (idev->msg_err) {
> +		complete(&idev->msg_complete);
> +		return;
> +	}
> +
> +	this_msg = (idev->msg_queue)++;

do we need parenthesis here?

...

> +	/*
> +	 * The isr controls the flow of a transfer, this info needs to be saved
> +	 * to a location that it can access the queue information from.
> +	 */
> +	idev->restart_needed = false;
> +	idev->msg_queue = msgs;
> +	idev->total_num = num;
> +	idev->current_num = 0;
> +
> +	/*
> +	 * But the first entry to the isr is triggered by the start in this
> +	 * function, so the first message needs to be "dequeued".
> +	 */
> +	idev->addr = i2c_8bit_addr_from_msg(this_msg);
> +	idev->msg_len = this_msg->len;
> +	idev->buf = this_msg->buf;
> +	idev->msg_err = 0;
> +
> +	if (idev->total_num > 1) {
> +		struct i2c_msg *next_msg = msgs + 1;
> +
> +		idev->restart_needed = next_msg->flags & I2C_M_RD;
> +	}
> +
> +	idev->current_num++;
> +	idev->msg_queue++;

Can we initialize only once? This part is just adding extra code.

The rest looks good. I just need to know if Wolfram has some more
observations here.

Thanks,
Andi