On Fri, 2016-07-15 at 09:29 +0300, Laurent Pinchart wrote:
> Hi Dave,
>
> Thank you for the patch.
>
> On Thursday 14 Jul 2016 15:00:09 Dave Jiang wrote:
> > Adding a new callback that will provide the error result for a
> > transaction.
> > The result is allocated on the stack and the callback should create
> > a copy
> > if it wishes to retain the information after exiting.
> >
> > Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > ---
> > include/linux/dmaengine.h | 35 ++++++++++++++++++++++++++++++++-
> > --
>
> Missing change for Documentation/dmaengine/*
Yes I realized that after the patches went out. I'll update.
>
> > 1 file changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index b601f23..ab8c498 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -441,6 +441,21 @@ typedef bool (*dma_filter_fn)(struct dma_chan
> > *chan,
> > void *filter_param);
> >
> > typedef void (*dma_async_tx_callback)(void *dma_async_param);
> >
> > +enum dma_trans_result {
> > + DMA_TRANS_NOERROR = 0,
> > + DMA_TRANS_READ_FAILED,
> > + DMA_TRANS_WRITE_FAILED,
> > + DMA_TRANS_ABORTED,
> > +};
> > +
> > +struct dmaengine_result {
> > + enum dma_trans_result result;
> > + u32 residue;
> > +};
> > +
> > +typedef void (*dma_async_tx_callback_result)(void
> > *dma_async_param,
> > + struct
> > dmaengine_result *result);
>
> How about making the result const ? Is there any reason to allow the
> callback
> to modify the result ?
I'll make the change.
>
> > +
> > struct dmaengine_unmap_data {
> > u8 map_cnt;
> > u8 to_cnt;
> > @@ -478,6 +493,7 @@ struct dma_async_tx_descriptor {
> > dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor
> > *tx);
> > int (*desc_free)(struct dma_async_tx_descriptor *tx);
> > dma_async_tx_callback callback;
> > + dma_async_tx_callback_result callback_result;
>
> So we now have two APIs. On the DMA engine drivers side, the
> transition path
> is quite clear. Drivers should be ported to the new API to report
> results. The
> results will be ignored by older client drivers, and newer client
> drivers that
> interact with older DMA engine drivers will get dummy results that
> always
> report success. There's no regression compared to the current API as
> client
> drivers have no way to detect failures today.
>
> On the client drivers side however, what's your plan to transition to
> the new
> API ? We don't want to keep two APIs forever, so we should get all
> client
> drivers converted in the not too distant future. There needs to be
> an
> incentive for that to happen, unless you plan to do the work
> yourself. The
> latter option would certainly be appreciated :-)
Honestly I would prefer the driver owners cared enough to move over to
the new API. Can we mark the old callback as deprecated in
documentation? We could add a check and print out a warning saying it's
deprecated?
>
> > void *callback_param;
> > struct dmaengine_unmap_data *unmap;
> > #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH
> > @@ -1374,6 +1390,7 @@ static inline int dmaengine_desc_free(struct
> > dma_async_tx_descriptor *desc)
> >
> > struct dma_desc_callback {
> > dma_async_tx_callback callback;
> > + dma_async_tx_callback_result callback_result;
> > void *callback_param;
> > };
> >
> > @@ -1382,14 +1399,26 @@ dmaengine_desc_get_callback(struct
> > dma_async_tx_descriptor *tx, struct dma_desc_callback *cb)
> > {
> > cb->callback = tx->callback;
> > + cb->callback_result = tx->callback_result;
> > cb->callback_param = tx->callback_param;
> > }
> >
> > static inline void
> > -dmaengine_desc_callback_invoke(struct dma_desc_callback *cb, void
> > *result)
> > -{
> > - if (cb->callback)
> > +dmaengine_desc_callback_invoke(struct dma_desc_callback *cb,
> > + struct dmaengine_result *result)
> > +{
> > + struct dmaengine_result dummy_result = {
> > + .result = DMA_TRANS_NOERROR,
> > + .residue = 0
> > + };
> > +
> > + if (cb->callback_result) {
> > + if (!result)
> > + result = &dummy_result;
> > + cb->callback_result(cb->callback_param, result);
> > + } else if (cb->callback) {
> > cb->callback(cb->callback_param);
> > + }
> > }
> >
> > /* --- DMA device --- */
> ��.n��������+%������w��{.n��������)�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
