Hi Conor,
On Tue, Oct 01, 2024 at 02:02:18PM GMT, Conor Dooley wrote:
> 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.
I started adding them already from a few releases and this is the
first time I am writing it.
I won't cry if someone doesn't add it :-)
> > > 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?
No need.
> >
> > ...
> >
> > > + /*
> > > + * 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.
Yes, I understood the reason. Mine was not a binding comment.
Thanks,
Andi
> > The rest looks good. I just need to know if Wolfram has some more
> > observations here.
> >
> > Thanks,
> > Andi
