On Sun, 18 Apr 2021 16:59:02 +0300 Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > On Sun, Apr 18, 2021 at 4:49 PM Andy Shevchenko > <andy.shevchenko@xxxxxxxxx> wrote: > > > > On Sun, Apr 18, 2021 at 2:07 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > Thanks for review, my answers below. > > > > > On Wed, 14 Apr 2021 22:54:53 +0300 > > > Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > > > > > > We can utilize separate drivers for accelerometer and magnetometer, > > > > so here is the glue driver to enable LSM9DS0 IMU support. > > > > > > > > The idea was suggested by Crestez Dan Leonard in [1]. The proposed change > > > > was sent as RFC due to race condition concerns, which are indeed possible. > > > > > > If you are going to mention races, good to give some flavour in here! > > > > I meant that the initial idea is racy due to different devices > > communicating to the same i2c address. > > So, any sequence of transfers are not serialized and you may end up with > > > > drv1 -> i2c > > drv2 -> i2c > > drv1 <- i2c # garbage > > > > > This driver makes me very nervous indeed. > > > > Why?! This one is race free as far as I can see. Or maybe I interpret > > this wrongly and you are talking about initial RFC? > > > > > I haven't 'found' any places > > > where the fact we'll write the same registers from each of the drivers > > > causes problems (e.g. int pin setup etc) but perhaps I'm missing something. > > > > > > Shall we say that makes me rather keener to get eyes (and thought) on this > > > patch than normal :) > > > > How should I amend the commit message to state: > > 1. First idea (RFC by the link) *is* racy AFAIU > > 2. This one *is not* racy. Great. I read it as meaning they were both potentially racey! This is less worrying. > > I re-read this and now understand better what you meant. > So, it may be that the initial proposal may work without any > amendment, but since I haven't investigated much, I should rather use > the phrase "potentially racy". In my variant it's using one regmap for > both drivers (not two), which makes the register state consistent. Am > I wrong? I think this approach is fine. I'd be more worried about the two 'sub' drivers not necessarily being happy that someone else touches state they care about. There are places where I think we write the same value to the same register twice during setup with this model, but that shouldn't matter. I'm not 100% sure that there aren't other cases though I think there aren't. So what you have is probably fine, but more eyes would make me happier ;) Lots of people care about this particular driver so hopefully we'll get them. > Do we have some places where we may write to the same register concurrently? > Only ones I can find are the setup ones where it writes the same value twice I think. So *crosses fingers* :) Given timing (missed merge window) we have masses of time to let this sit on list a while and see if anyone can spot issues neither of us have found. Jonathan
