lkml.org 
[lkml]   [2019]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH V7 3/5] i2c: tegra: Add DMA Support
    From
    Date
    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.

    \
     
     \ /
      Last update: 2019-01-31 15:09    [W:2.419 / U:0.108 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site