Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMAresources

[Sekhar Nori]

On Tuesday 17 September 2013 11:08 AM, Joel Fernandes wrote:
> On 09/17/2013 12:08 AM, Sekhar Nori wrote:
> [..]
>>>> I still cannot find any users of edma in the device tree sources either
>>>> in linux-next or linus/master. Why cannot this wait until v3.13?
>>>
>>> I understand this affects only DT users of EDMA. But I get so many private
>>> reports of breakage due to this patch not being there that I think it will save
>>> everyone a lot of pain, specially folks creating integration trees to have this
>>> patch available by default.
>>
>> Well, I do agree that the current DT support for EDMA is incomplete
>> without this patch even if there are no in-kernel users of it. I will
>> try sending this for the next -rc if we get to the final version in time
>> and after that its upto the upstreams to take it.
> 
> Ok, except for the one minor nit below my last scissor patch is good to go.
> 
>>>>> +
>>>>> +			ctlr = EDMA_CTLR(dma_spec.args[0]);
>>>>> +			clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
>>>>> +				  edma_cc[ctlr]->edma_unused);
>>>>
>>>> We don't support the second controller when using DT and the controller
>>>> number is not really encoded in the argument to edma phandle. So just
>>>> simplify this to:
>>>>
>>>> 	clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), 	
>>>> 		  edma_cc[0]->edma_unused);
>>>
>>> I think let's not make that assumption just incase in the future we support more
>>> than one EDMA controller for DT-based boot. Is that ok?
>>
>> We don't write future proof code in that _hope_ that it will get used
>> someday.  In fact this will confuse the reader into wondering if support
>> for second channel controller is present or not as the code will send
> 
> Nope I don't agree with this at all.. EDMA CC ctrl will not be hardcoded. We
> need to write future-proof code to make sure sudden regressions don't pop up
> when say a second EDMA CC is introduced.. further edma_cc[ctrl] pattern is used

I am not sure why you call this a regression. There is no support for
second channel controller. If someone wants to add that support there is
a lot to worry about than just these two lines of code. So, no, there
wont be "sudden regressions that pop up" like you say. Look at the probe
routine. There is no way the second CC is going to get probed.

> all through out the code so what you're asking to do doesn't make much sense in
> this context. There's no reason to break out of this pattern. It actually will
> confuse the reader more.

Thats because that code is also used for existing platform data based
approach. If you are adding code paths which just get exercised with DT,
no point referring to the second channel controller since that does not
exist.

> 
> Second controller can be present in future. I don't want to come back to change
> the code when we introduce more than 1 CC which is possible in the future.

You *will* have to come back and change the code to support the second
channel controller. That is true with or without this patch. So lets
stop arguing like the second channel controller support is only blocked
because of these two lines of code.

> 
>> mixed messages. In short, we aim for consistency with situation today,
>> not tomorrow.
> 
> What you're asking to do infact breaks consistency with the rest of the code.

Well, ideally we support second CC even with DT to be consistent all
around. Since that has not happened, I don't want the code to pretend
that it it supports the second channel controller with DT that too only
in parts of code.

> 
>>
>> Besides, I can bet that when second CC support does get added, it is
>> very unlikely that the CC number is get encoded into channel number when
>> using DT.
> 
> Even if it is not encoded, the data structure for edma_cc is an array and what
> you're asking is to hardcode the controller number to 0 always. No way I'm going
> to hard code controller number to a single value.

You are missing the point. Look at the code. You are extracting
controller number from dma_spec.args[0] which is directly the value
present in DT. The binding today does not specify that the value be
encoded with controller number. So I will have not have code that
interprets it that way.

The code follows from bindings, not the other way around.

> 
> Different topic but anyway why wouldn't ctrl number be encoded in the channel?
> That's clean, and saves variables and extra structures. Better use of the
> integer bitmap making up the Ctrl and channel number of small ranges.

Lets have this discussion when you update the bindings to support the
second controller.

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/