On Sat, Sep 20, 2008 at 10:18:41AM -0700, David Brownell wrote: > > None that I know about - generally other drivers look up their private > > data in some kind of array and assign to file->private_data in their > > open() method - in much the same way that watchdog and misc drivers do. > > Yet i_private gets set up *somewhere* else that field wouldn't exist. > Look around; you'll see it gets used. (My quick grep started in the > USB tree, where it turns out both usbfs and gadgetfs use it.) Both of which are filesystems which have more control over the lifetime of the inode. Device drivers don't have such luxuries. > > The simplest solution is as the watchdog drivers are doing. > > I'll disagree. It requires extra state even in this simple case; > state of a generically undesirable flavor (global). And likewise, > it requires lookup mechanisms ... which if you look around, tend > to be rather error prone, locking often gets goofed up there. > > Familiar and simple != simplest, != best. It will often become > cargo cult programming. Well, are you going to manufacture a patch to update all the watchdog drivers to use your new i_private method, and get that merged into Wim's tree now, so that then the omap watchdog drivers can satisfy your apparant objection (which Wim _has_ taken as an objection against them going in)? > > And anyway, the point of these patches is not to fix issues like this. > > It's to get what's in mainline updated to what's in the OMAP tree so > > stuff can move forwards. So, let's not go down rabbit warrens trying > > to find obscure new issues which lots of other code already "suffers" > > from. > > Right, my original comment pointed out one thing that was clearly > wrong (extra/unused struct field) and one odd thing (the global). > > You said "not that odd, here's why"; I said "hmm, well OK but it's > still got problem X, which isn't for fixing in this patchset". The "well OK" didn't come over at all - neither I nor Wim seem to have received that point. > > We're into the third day on this one driver. If every OMAP driver takes > > this long ... > > That seems like a strange way to account things. It presumes > that the only time review comments should be accepted is within > a day of the patch getting posted. Regardless of whether the > reviewer has time at that point. You define accounting for things in real time as "strange" - lol. Your following sentences don't follow either. My point is that we currently have a BIG problem, and that is the OMAP fork being so far out of line with mainline, it isn't funny. It's causing lots of pain for everyone here. Folk are screaming for mainline to be buildable for OMAP. There are two approaches to achieve that: take each driver, polish it for weeks on end until it's nice and shiney, and then submit it upstream. Eventually, given enough man hours, you'll get to the point where you've pushed everything upstream, but in the mean time, new work has been queued so you need to start at the beginning again. You've got a job for life constantly polishing code. The other approach is to decide that we have what we have, and that in the interests of efficiently reducing divergence, merging the upstream changes with the downstream changes and pushing the result upstream ASAP. Once merged, further improvements and cleanups can be made by pushing them separately upstream along with any other bug fixes. Given the amount of divergence, the only approach which gives realistic progress is the second one. If you think the first approach is the way to go, then please join in with Tony and myself reviewing the _entire_ OMAP tree, polishing every patch, and pushing it upstream. And I mean _everything_. Not just the USB stuff. Encourage everyone else to do the same - because it will take an army of individuals to make any forward progress. > If you *really* wanted to avoid wasting time you wouldn't have > replied to my previous post, which agreed that one point I raised > was a non-issue for this particular patch series. Wim said: "Will add patches 1 to 3 when everyone is OK with them. I still saw some comments from David." So, would you like to clearly tell Wim what your comments mean as far as merging this patch series? It seems I'm not the only one who's confused as to the intention and meaning of your comments. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
