On Fri, May 17, 2013 at 11:00:16AM +0100, Russell King - ARM Linux wrote:
> On Fri, May 17, 2013 at 11:51:51AM +0200, Wolfram Sang wrote:
> > > {
> > > switch(drv_data->action) {
> > > case MV64XXX_I2C_ACTION_SEND_RESTART:
> > > + /* We should only get here if we have further messages */
> > > + BUG_ON(drv_data->num_msgs == 0);
> > > +
> >
> > ...
> >
> > > @@ -453,16 +463,20 @@ static int
> > > mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > > {
> > > struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
> > > - int i, rc;
> > > + int rc, ret = num;
> > >
> > > - for (i = 0; i < num; i++) {
> > > - rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[i],
> > > - i == 0, i + 1 == num);
> > > - if (rc < 0)
> > > - return rc;
> > > - }
> > > + BUG_ON(drv_data->msgs != NULL);
> >
> > Can't we handle this more gracefully than to halt the kernel? Like
> > WARN_ON and resetting the controller or disabling the bus or...
>
> Well, the latter really is something which should never ever happen,
> and if it does happen it can only really be because something's
> screwed up in the locking in the I2C layer.
I'd think we should trust the layer here.
> The former is more probable, and I thought about that, but I don't
> have any good alternative solution. Given the problems I've seen,
> I don't think resetting the controller is really an option, because
> that'll likely cause the bus to lock (that's the original problem
> which I'm trying to solve in this patch.) The thing really does
> have to work according to the I2C protocol otherwise stuff goes
> irrecoverably wrong to the point of needing an entire system reset.
Fine with me for now. If somebody later has a setup where I2C slaves can
be reset (e.g. via GPIO), so a complete bus reset is possible, we might
need another solution, then.
Thanks,
Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
