From: Jonathan C. <co...@lw...> - 2010-04-10 00:28:09
|
On Sat, 10 Apr 2010 01:32:36 +0200 Florian Tobias Schandinat <Flo...@gm...> wrote: > Please correct me if I am wrong but the remaining 6 patches concerning > suspend&resume look like a real big FIXME. So at the end it is expected > to work only on VX855 and needs something called OFW? OFW = OpenFirmware. You could say BIOS instead. It's pretty normal to expect the BIOS to put things into a semi-rational state at resume time. > It doesn't seem to make much sense to review each of them because the > following patches might or might not correct some of the issues of the > other. It is really a pain to have 6 patches trying to add a single > feature. Is there any way to fix this mess. (I assume you didn't merge > them due to authorship issues?) It's true that one needs to look at the end product. I only looked at the S/R code recently, and tried to fix some of the biggest issues that would keep it out of the mainline. > I think it might be better to drop those for now and wait for viafb to > be in a better shape before adding this feature. The mode setting should > be in a pretty good shape just 1 or 2 kernel versions ahead so that the > dependency on OFW can be dropped I think. > > Sorry but I really think this is not in a shape where merging it is an > option. I think it would be better to skip those suspend/resume patches > for the next merge window. Well, if we want to keep s/r out of tree, we can do that. It will complicate the merge of the other stuff, since it's got hooks into the GPIO and camera code too. But, like everything else I've posted so far, it's not the work that I personally set out to do. I can push that work on others :) That said, the suspend/resume support in this patch set makes suspend work on one chipset, and probably comes pretty close on the others without breaking anything there. I don't see the harm in merging it; it makes the code better than it is now. I would rather not have to separate it out from the rest. But I'll not fight over this one; if there's real opposition then we can force OLPC to continue to carry it out of tree. jon |
From: Jonathan C. <co...@lw...> - 2010-04-10 00:55:11
|
On Sat, 10 Apr 2010 02:42:52 +0200 Florian Tobias Schandinat <Flo...@gm...> wrote: > > That said, machine_is_olpc() is properly defined for all > > configurations, so the #ifdefs can (and should) come out. > > I'm not sure I get you right here. If you talk about removing the > defines and only letting the machine check that is something that I > would accept now. Yes, that is what I mean; the ifdefs don't need to be there. Had I thought that through I would have removed them before posting the patch. jon |
From: Jonathan C. <co...@lw...> - 2010-04-18 17:34:40
|
[Getting back to the older stuff...] On Fri, 09 Apr 2010 22:34:16 +0200 Florian Tobias Schandinat <Flo...@gm...> wrote: > >> Just a minor nit: > >> Could we change the default so that if someone adds support for a new > >> IGP (and misses this function) we default to either the newest or > >> preferably to none? I've just seen too much poorly maintained code in > >> this driver and defaulting to the oldest is hence a bad idea. > >> Otherwise it's fine. > > > > That would require making an exhaustive list of older chipset types. > > It could probably be inferred through inspection of the code, but I > > worry about making assumptions in this area... > > Such list already exists. gfx_chip_name = pdi->driver_data in hw.c (and > only there) so what is needed is the list viafb_pci_table in viafbdev.c > (relatively at the end) of all chips: I've spent a bit of time looking at this. What's really needed is a better way of abstracting the chip types so that we can maybe get rid of all those switch statements throughout the driver. For the purposes of getting this work in, I'm not quite prepared to make that change, though I could certainly consider doing it in the future. In the absence of that, the only course of action which makes sense is to simply fail the initialization if an unknown chip type shows up there. That's easy, and I can do it. But, given that this was a "minor nit," can we leave it as-is for now? There's a *lot* of things to clean up in this driver, I'd like to make it better a step at a time rather than trying to do the whole thing at once. Thanks, jon |
From: Jonathan C. <co...@lw...> - 2010-04-18 17:39:39
|
On Fri, 09 Apr 2010 23:27:30 +0200 Florian Tobias Schandinat <Flo...@gm...> wrote: > What will happen if someone with no VX855 will try to enter 1200x900 > mode? Probably the world won't be destroyed but I have a really ugly > feeling that the driver is not well prepared for this situation. OK, I'm certainly exposing my relative lack of understanding of framebuffer issues (I'm here to add a camera driver, remember? :), but...is this really something that older chipsets can't handle? Or is it just that nobody has had that size of panel before? > Haven't > tested yet as I'm waiting for the official forward port but I think it > might be better to reschedule this patch for later or add some PLL > values that are supposed to work (using/executing the formulas in the > VIA open source X driver) If it's really not acceptable, then it will stay out, but I would rather get it merged. I will leave it in the series for now; it can come out later if need be. Thanks, jon |