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@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.
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@xxxxxxxxxxxxxxx> # 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@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?
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:
signature.asc
Description: PGP signature
