Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

31.01.2019 15:06, Thierry Reding ÐÐÑÐÑ:
> On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
>> 30.01.2019 19:01, Sowjanya Komatineni ÐÐÑÐÑ:
> [...]
>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> [...]
>>> +		return -EIO;
>>> +	}
>>> +
>>> +	dma_desc->callback = tegra_i2c_dma_complete;
>>> +	dma_desc->callback_param = i2c_dev;
>>> +	dmaengine_submit(dma_desc);
>>> +	dma_async_issue_pending(chan);
>>> +	return 0;
>>> +}
>>> +
>>> +static int tegra_i2c_init_dma_param(struct tegra_i2c_dev *i2c_dev,
>>> +				    bool dma_to_memory)
>>> +{
>>> +	struct dma_chan *dma_chan;
>>> +	u32 *dma_buf;
>>> +	dma_addr_t dma_phys;
>>> +	int ret;
>>> +	const char *chan_name = dma_to_memory ? "rx" : "tx";
>>
>> What about to move out chan_name into the function arguments?
> 
> That opens up the possibility of passing dma_to_memory = true and
> chan_name as "tx" and create an inconsistency.
> 
>>> @@ -884,6 +1187,8 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
>>>  
>>>  	i2c_dev->is_multimaster_mode = of_property_read_bool(np,
>>>  			"multi-master");
>>> +
>>> +	i2c_dev->has_dma = of_property_read_bool(np, "dmas");
>>
>> Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.
>>
>> Hence there is a need to check for DMA driver presence in the code:
>>
>> 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
>> 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");
> 
> Do we even need the ->has_dma at all? We can just go ahead and request
> the channels at probe time and respond accordingly. If there's no dmas
> property in DT, dma_request_slave_channel_reason() returns an error so
> we can just deal with it at that time.
> 
> So if we get -EPROBE_DEFER we can propagate that, for any other errors
> we can simply fallback to PIO. Or perhaps we want to restrict fallback
> to PIO for -ENODEV?
> 
> I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
> The purpose of these subsystems it to abstract all of that away.
> Otherwise we could just as well use custom APIs, if we're tying together
> drivers in this way anyway.

DMA API doesn't fully abstract the dependencies between drivers, hence I disagree.

>> Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
>> driver built-in when I2C driver is built-in:
> 
> I don't think there's a requirement for that. The only dependency we
> really have here is the one on the DMA engine API. Since dmaengine.h
> already provides dummy implementations, there's really no need for
> us to have the dependency. If the DMA engine API is completely disabled,
> a call to dma_request_slave_channel_reason() will return -ENODEV and we
> should just deal with that the same way we would if there was no "dmas"
> property present.

In my opinion it is much better to avoid I2C driver probe failing with -EPROBE_DEFER if we could. It's just one line in code and one in Kconfig.. really. 

Good point about handling -ENODEV of dma_request_slave_channel_reason(), +1 for it.