On Sun, Sep 29, 2024 at 10:46:37PM -0500, Bjorn Andersson wrote:
> > @@ -602,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
> > peripheral.clk_div = itr->clk_div;
> > peripheral.set_config = 1;
> > peripheral.multi_msg = false;
> > + peripheral.shared_se = gi2c->se.shared_geni_se;
> >
> > for (i = 0; i < num; i++) {
> > gi2c->cur = &msgs[i];
> > @@ -612,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
> > if (i < num - 1)
> > peripheral.stretch = 1;
> >
> > + peripheral.first_msg = (i == 0);
> > + peripheral.last_msg = (i == num - 1);
>
> There are multiple error paths in this loop, which would result in us
> never issuing the unlock TRE - effectively blocking other subsystems
> from accessing the serial engine until we perform our next access
> (assuming that APSS issuing a lock TRE when APSS already has the channel
> locked isn't a problem?)
>
Hi Bjorn,
I saw the words "error paths" and "unlock" and I thought there was maybe
something we could do here with static analysis. But I don't know what TRE
or APSS mean.
The one thing I do see is that this uses "one err" style error handling where
there is one err label and it calls dmaengine_terminate_sync(gi2c->rx_c)
regardless of whether or not geni_i2c_gpi() was called or failed/succeeded.
regards,
dan carpenter
