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.
