Hi, On Tue, Oct 9, 2018 at 12:45 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > Quoting Doug Anderson (2018-10-09 10:48:55) > > > > Ah, you're suggesting separating the platform_get_irq() and the > > request_irq() so that we call platform_get_irq() as the first thing in > > the function but don't do the request_irq() until later. Yeah, that > > could be done and I guess if you feel strongly about it I wouldn't > > object to the change, but I don't feel it buys us a lot and I kind of > > like keeping the two next to each other. Specifically why I don't > > think it buys us a lot: > > > > 1. You still need the "dev_err" print, right? platform_get_irq() > > doesn't automatically print errors for you I think. > > I usually leave it out. Who cares? Maybe we should throw a dev_err() > into platform_get_irq() on failure so we can keep drivers cleaner and > reduce a bunch of "can't find my IRQ" messages throughout the kernel. Yeah, all the boilerplate code is annoying. ...of course by hoisting it up then you get a whole bunch of people that have "optional" IRQs suddenly getting error messages spewed which is also no good. IMO the convention of Linux drivers I've always reviewed is to print errors like this, so unless that changes my vote is to follow convention. > > 2. You now need a local variable "irq". By putting the > > platform_get_irq() before the memory allocation you now can't store it > > directly in mas->irq. We could try using "ret" as a temporary > > variable but that seems worse in this case since it'd be a bit > > fragile. > > > > 3. You don't get rid of any error labels / error handling so we don't > > really save any code > > > > When I tried this my diffstat says 8 lines added and 7 removed, so a > > net increase in LOC FWIW. I'm relying in gmail so my patch will be > > whitespace-damaged (sigh), but you can find a clean one at: > > > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e0325d618e209c22379e3a4269c14627b19243a8%5E%21/#F0 > > > > ...the basic idea is this though: > > > > Thanks! Here's an updated patch that I haven't compile tested in any way > that hoists up the IO mapping part too, which shows that the 'se' local > variable is almost entirely useless. Yeah, I'd be all for getting rid of "se". I'm still not really seeing the benefit of hoisting all the rest of the stuff up. Do you feel strongly about it? In any case I think we've both said that all of our comments so far are just nits and could be addressed in a followup patch. Unless Mark Brown wants these nits fixed ahead of time or has other changes he'd like, I don't think we're expecting another spin of this patch from Alok or Dilip, right? We'd just expect them to post some follow-up patches after Mark lands it? -Doug
