On 22/04/16 14:56, Shardar Mohammed wrote:
> Thanks for the review, please check my comments inline in [Shardar].
>
>> On 22/04/16 13:44, Shardar Shariff Md wrote:
>>> Initialize default channel slave_id(req_sel) to invalid id (i.e max
>>> supported slave id + 1) to avoid overwriting of slave_id during
>>> tegra_dma_slave_config() with client data if slave_id is not
>>> initialized through DT
>>>
>>> Signed-off-by: Shardar Shariff Md <smohammed@xxxxxxxxxx>
>>>
>>> ---
>>> - Instead of initializing the slave id to -1 define macros for
>>> max slave id and invalid slave id and do the checks accordingly.
>>> ---
>>> drivers/dma/tegra20-apb-dma.c | 13 +++++++++++--
>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/dma/tegra20-apb-dma.c
>>> b/drivers/dma/tegra20-apb-dma.c index 3871f29..2957e26 100644
>>> --- a/drivers/dma/tegra20-apb-dma.c
>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>> @@ -114,6 +114,9 @@
>>> /* Channel base address offset from APBDMA base address */
>>> #define TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET 0x1000
>>>
>>> +#define TEGRA_APBDMA_SLAVE_ID_MAX 31
>>
>> Sorry, thinking about this some more, given that the ID is programmed into a
>> field in the CSR register could we just define as mask here for the CSR field ...
>>
>> #define TEGRA_APBDMA_CSR_REQ_SEL_MASK 0x1f
>>
>> Note this should be defined with the other CSR register field definitions.
> [Shardar] Will take care of it. One doubt do you mean below.
> Will define this with other CSR register fields.
> #define TEGRA_APBDMA_CSR_REQ_SEL_MASK 0x1f
>
> #define TEGRA_APBDMA_SLAVE_ID_MAX TEGRA_APBDMA_CSR_REQ_SEL_MASK
We don't need the MAX definition as well.
>>
>>> +#define TEGRA_APBDMA_SLAVE_ID_INVALID
>> (TEGRA_APBDMA_SLAVE_ID_MAX + 1)
>>>
>>> struct tegra_dma;
>>>
>>> /*
>>> @@ -353,7 +356,7 @@ static int tegra_dma_slave_config(struct dma_chan
>> *dc,
>>> }
>>>
>>> memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
>>> - if (!tdc->slave_id)
>>> + if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID)
>>> tdc->slave_id = sconfig->slave_id;
>>
>> I believe I mentioned before that here we should ...
>>
>> if (tdc->slave_id != TEGRA_APBDMA_SLAVE_ID_INVALID)
>> return -EBUSY;
>
> [Shardar] By the time this function tegra_dma_slave_config is called, slave_id here is already initialized through DT(provided option) through function tegra_dma_of_xlate().
> And filling up the slave id here from sconfig structure that is provided by dma client driver is used here if it dma client is not initialized through DT.
> Please correct me if am wrong.
Yes you are right. So on further inspection this allows the user to set
the slave ID if it is not set yet. So I think we need ...
if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID) {
if (sconfig->slave_id > TEGRA_APBDMA_CSR_REQ_SEL_MASK)
return -EINVAL;
else
tdc->slave_id = sconfig->slave_id;
}
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
