From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Osipenko Subject: Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support Date: Thu, 31 Jan 2019 17:06:18 +0300 Message-ID: <0e832fa6-f143-7b24-2685-b88a55e77e51@gmail.com> References: <1548864096-20974-1-git-send-email-skomatineni@nvidia.com> <1548864096-20974-3-git-send-email-skomatineni@nvidia.com> <1f10cb76-59a1-93c5-ae03-ccc0cd8db1a3@gmail.com> <20190131120640.GF23438@ulmo> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20190131120640.GF23438@ulmo> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Thierry Reding , Sowjanya Komatineni Cc: jonathanh@nvidia.com, mkarthik@nvidia.com, smohammed@nvidia.com, talho@nvidia.com, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org List-Id: linux-tegra@vger.kernel.org 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.