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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3221 bytes --]

On Tue, Oct 01, 2024 at 02:45:20PM +0200, Andi Shyti wrote:
> Hi Conor,
> 
> On Mon, Sep 30, 2024 at 02:38:27PM GMT, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > 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.

I think the fixes tag is accurate, because it only happened to work on
the limited set of devices I and others tried. This patch came about cos
I got reports of it being broken in 6.6

> 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@vger.kernel.org> # v6.0+
> 
> (This is specified in the
> Documentation/process/stable-kernel-rules.rst and I'm starting to
> enforce it here).

Yah, some maintainers want to add the tags themselves, so got into a
(bad?) habit of leaving them out. I can add it if there's a v2.

> 
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> ...
> 
> > +	/*
> > +	 * 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?

I suppose not, do you want a v2 if that's the only change?

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

I don't agree that it is extra code, I think it is clearer like this as
I intentionally wrote it this way.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]