On Tue, 2018-07-24 at 10:21 -0500, Aaron Sierra wrote: > I will concede that your consistency argument has merit. Yes. Keep the driver consistent. > However, how > do you propose to change the ioread8() call that's already present? It has been problem of review, I could tell lack of review. The first occurrence of ioreadXX() appeared in your patch. > I > would typically shy away from submitting possible "churn" except for > your insistence: I think you didn't get. No need to create a standalone patch for that. Just append this to the (future) fix we are discussing. > > > I've been under the impression that ioreadX() are the > > > preferred accessors > > > > Why do we need it? Why it's preferred here? > > It's not *needed*, but it is already there and is not harmful. Again, the patch makes the driver to use inconsistent APIs. That's not good. It might create an illusion that the ioreadXX() API is mandatory to use when it's not. Second reader's question would be "what the heck readXX() are doing in the same driver for the same code?". > Sure, but that's not really a mandate to avoid using them when dealing > with MMIO-only devices. This quote from the very next bullet covers my > stance: > > It's also, perhaps, a bit syntactically easier to work with, so some > people might prefer that for that reason. See above question: why on the earth you have added that one _without_ changing the rest? This is my very question as reviewer and as a reader of the code. > I prefer the ioread/iowrite accessors for two reasons related to > syntax: > > * clearer naming of ioread[8,16,32,64] over read[b,w,l,q] > * more "natural" argument ordering of iowriteX over writeY > > Regardless, I am willing to acquiesce to your demand. Please review > the > patch that I included inline above. > Send a patch to fix all occurrences and we would waste more time on this kind of discussions. Really, I have got no argument why _in this very driver_ one occurrence of ioreadXX() is better then keeping everything consistent. If you have such, you should have created a series of the patches back then, i.e. a) converting to ioreadXX(), b) introducing better IRQ handling (or in vise versa order), but instead we have inconsistent use of I/O accessors for no reason by my opinion. -- Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
