Hi Johan, On Thu, Jun 14, 2018 at 4:55 PM Johan Hovold <jo...@kernel.org> wrote: > > On Thu, Jun 14, 2018 at 04:06:18PM +0200, Ricardo Ribalda Delgado wrote: > > Hi Johan, > > On Thu, Jun 14, 2018 at 3:34 PM Johan Hovold <jo...@kernel.org> wrote: > > > > And there are more issues with the series which are less apparent than > > > the rx (and partial tx) regression. > > > > Any hints about this? What else should I change on the series? > > There are implementation issues and there's the more fundamental > question about whether your approach to this is the right one. > > Like Rob, I'm not sure we want to have the device topology depend on a > kernel config symbol (serdev and your ttydev driver). We may need to > explore Rob's sibling-device idea further.
>From my point of view, if the user enables serdev, then everything has to be a serdev, because serdev does not provide the same functionality as a core tty device I had to implement, serdev-ttydev.c. Which is nothing more than a wrapper. It is very hacky, but allows replacing the core tty device with another serdev. > > I also want to make sure that this can be used for discoverable buses > (e.g. the USB CEC device the I've used as an example before). > I have tried your patch: https://github.com/ribalda/linux/commit/5cb30b4ce6477132a23492c674d8b3dc81ecff86 the only issue is that the serdev device sometimes explotes (OOPS) when the usb is unplugged :S. And that might be quite tricy to solve > As for the current implementation there are both larger and smaller > issues, like for example: > > - the fact that your sysfs and lookup interface does not use any > locking whatsoever and thus is susceptible to races I thought that sysfs access where serialised. If that is not the case yes, we need a lock. > > - your ttyport driver currently breaks the sysfs interface for all > serial (core) devices by ignoring the attribute groups Yep, you are right, I screwed up that one :). > > - the ttyport driver is arguably a hack with layering issues (which > admittedly may be hard to avoid given the retrofitting of serdev into > the tty layer) > > Again, I suggest you submit a subset of your series (aim at 10 patches > or so) as an RFC which can be used as a basis for further discussion. No > point in discussing every implementation detail if the underlying > approach needs to be revised. Will do. Give me some time to give it a hand of paint. Thanks for time reviewing my little moster > > > > It's legacy as in old, and to be used for one-off hacks and such. But > > > sure, that is also what this series aims at even if that doesn't mean > > > you *have to* copy the interface. > > > > It is not only one-off hack. It is the ONLY way to use i2c devices > > that are not enumerated. > > > > The same way as today we do not have any way of using serdev on non > > enumerated devices. > > You're missing the point: none of that means you have to copy the > interface. > > Johan -- Ricardo Ribalda
