Re: [PATCH V13 05/10] mmc: cqhci: support for command queue enabled host

[Adrian Hunter]

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.