On 09/11/17 14:26, Linus Walleij wrote: > On Wed, Nov 8, 2017 at 3:14 PM, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote: >> On 08/11/17 11:22, Linus Walleij wrote: >>> On Fri, Nov 3, 2017 at 2:20 PM, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote: > >>> (...) > >>>> +EXPORT_SYMBOL(cqhci_resume); >>> >>> Why would the CQE case require special suspend/resume >>> functionality? >> >> Seems like a very strange question. > > Please realize that patch review is partly about education. > > Educating me or anyone about your patch set involves > being humbe and not seeing your peers as lesser. > > Making your reviewer feel stupid by saying the ask > "strange" or outright stupid questions is not helping > your cause. Please try to forgive me for being rude, it is just frustration. > >> Obviously CQHCI has to be configured >> after suspend. > > Yeah. I think I misunderstood is such that: > >> Also please don't confuse CQE and CQHCI. CQHCI is an implementation of a >> CQE. We currently do not expect to have another implementation, but it is >> not impossible. > > OK now you educated me, see it's not that hard without > using belitteling language. > >>> This seems two much like on the side CQE-silo engineering, >>> just use the device .[runtime]_suspend/resume callbacks like >>> everyone else, make it possible for the host to figure out >>> if it is in CQE mode or not (I guess it should know already >>> since cqhci .enable() has been called?) and handle it >>> from there. >> >> That is how it works! The host controller has to decide how to handle >> suspend / resume. > > OK. > >> cqhci_suspend() / cqhci_resume() are helper functions that the host >> controller can use, but doesn't have to. > > OK. > >>> Why would CQE hosts need special accessors and the rest >>> of the host not need it? >> >> Special accessors can be used to fix up registers that don't work exactly >> the way the standard specified. > > Yeah this is fine as it is for CQHCI, I didn't get that > part :) > >>> ->enable and ->disable() for just CQE seem reasonable. >>> But that leaves just two new ops. >>> >>> So why not just put .cqe_enable() and .cqe_disable() >>> ops into mmc_host_ops as optional and be done with it? >> >> Ok so you are not understanding this at all. > > No I did not get it. But I do now (I think). > >> As a CQE implementation, CQHCI interfaces with the upper layers through the >> CQE ops etc. >> >> But CQHCI also has to work with any host controller driver, so it needs an >> interface for that, which is what cqhci_host_ops is for. All the ops serve >> useful purposes. > (...) >> The whole point is to prove a library that can work with any host controller >> driver. That means it must provide functions and callbacks. > > OK > >>> I think the above approach to put any CQE-specific callbacks >>> directly into the struct mmc_host_ops is way more viable. >> >> Nothing to do with CQE. This is CQHCI. Please try to get the difference. > > I am trying, please try to think about your language. I strongly disapprove of being rude but sadly it seems to get results.
