| Message ID | 1527754134-164985-5-git-send-email-tien.fong.chee@intel.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add DMA driver for DMA330 controller | expand |
Hi, On 31 May 2018 at 02:08, <tien.fong.chee@intel.com> wrote: > From: Tien Fong Chee <tien.fong.chee@intel.com> > > Update the dma_memcpy description on return argument for DMA330 driver. > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com> > --- > include/dma.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/dma.h b/include/dma.h > index 0a0c9dd..b825c1e 100644 > --- a/include/dma.h > +++ b/include/dma.h > @@ -79,8 +79,8 @@ int dma_get_device(u32 transfer_type, struct udevice **devp); > * @dst - destination pointer > * @src - souce pointer > * @len - data length to be copied > - * @return - on successful transfer returns no of bytes > - transferred and on failure return error code. > + * @return - on successful transfer returns no of bytes or zero(for DMA330) > + * transferred and on failure return error code. This is a public API so you cannot change it just for one device. You can change the API for everyone if you like. But why would it want to return 0? > */ > int dma_memcpy(struct udevice *dev, void *dst, void *src, size_t len); > > -- > 2.2.0 > Regards, Simon
On Fri, 2018-06-01 at 08:25 -0600, Simon Glass wrote: > Hi, > > On 31 May 2018 at 02:08, <tien.fong.chee@intel.com> wrote: > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > Update the dma_memcpy description on return argument for DMA330 > > driver. > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com> > > --- > > include/dma.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/dma.h b/include/dma.h > > index 0a0c9dd..b825c1e 100644 > > --- a/include/dma.h > > +++ b/include/dma.h > > @@ -79,8 +79,8 @@ int dma_get_device(u32 transfer_type, struct > > udevice **devp); > > * @dst - destination pointer > > * @src - souce pointer > > * @len - data length to be copied > > - * @return - on successful transfer returns no of bytes > > - transferred and on failure return error code. > > + * @return - on successful transfer returns no of bytes or > > zero(for DMA330) > > + * transferred and on failure return error code. > This is a public API so you cannot change it just for one device. You > can change the API for everyone if you like. > > But why would it want to return 0? > Because we only able to check the DMA tranferring status, full transfer or failed. I can return the len argument user set if full tranfer is finished. > > > > */ > > int dma_memcpy(struct udevice *dev, void *dst, void *src, size_t > > len); > > > > -- > > 2.2.0 > > > Regards, > Simon
Hi, On 3 June 2018 at 23:14, Chee, Tien Fong <tien.fong.chee@intel.com> wrote: > On Fri, 2018-06-01 at 08:25 -0600, Simon Glass wrote: >> Hi, >> >> On 31 May 2018 at 02:08, <tien.fong.chee@intel.com> wrote: >> > >> > From: Tien Fong Chee <tien.fong.chee@intel.com> >> > >> > Update the dma_memcpy description on return argument for DMA330 >> > driver. >> > >> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com> >> > --- >> > include/dma.h | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/include/dma.h b/include/dma.h >> > index 0a0c9dd..b825c1e 100644 >> > --- a/include/dma.h >> > +++ b/include/dma.h >> > @@ -79,8 +79,8 @@ int dma_get_device(u32 transfer_type, struct >> > udevice **devp); >> > * @dst - destination pointer >> > * @src - souce pointer >> > * @len - data length to be copied >> > - * @return - on successful transfer returns no of bytes >> > - transferred and on failure return error code. >> > + * @return - on successful transfer returns no of bytes or >> > zero(for DMA330) >> > + * transferred and on failure return error code. >> This is a public API so you cannot change it just for one device. You >> can change the API for everyone if you like. >> >> But why would it want to return 0? >> > Because we only able to check the DMA tranferring status, full transfer > or failed. I can return the len argument user set if full tranfer is > finished. OK. My concern is that you have a comment saying that the function does something different for one device versus others. This is not the place for such a comment. Here you can just document that it can return two possible meanings. You should add comments explaining what 0 means too (e.g. completed, but length unknown?). For something in progress, you should use -EINPROGRESS / -EAGAIN Regards, Simon
On Fri, 2018-06-08 at 13:59 -0800, Simon Glass wrote: > Hi, > > On 3 June 2018 at 23:14, Chee, Tien Fong <tien.fong.chee@intel.com> > wrote: > > > > On Fri, 2018-06-01 at 08:25 -0600, Simon Glass wrote: > > > > > > Hi, > > > > > > On 31 May 2018 at 02:08, <tien.fong.chee@intel.com> wrote: > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > > > > > Update the dma_memcpy description on return argument for DMA330 > > > > driver. > > > > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com> > > > > --- > > > > include/dma.h | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/dma.h b/include/dma.h > > > > index 0a0c9dd..b825c1e 100644 > > > > --- a/include/dma.h > > > > +++ b/include/dma.h > > > > @@ -79,8 +79,8 @@ int dma_get_device(u32 transfer_type, struct > > > > udevice **devp); > > > > * @dst - destination pointer > > > > * @src - souce pointer > > > > * @len - data length to be copied > > > > - * @return - on successful transfer returns no of bytes > > > > - transferred and on failure return error code. > > > > + * @return - on successful transfer returns no of bytes or > > > > zero(for DMA330) > > > > + * transferred and on failure return error code. > > > This is a public API so you cannot change it just for one device. > > > You > > > can change the API for everyone if you like. > > > > > > But why would it want to return 0? > > > > > Because we only able to check the DMA tranferring status, full > > transfer > > or failed. I can return the len argument user set if full tranfer > > is > > finished. > OK. My concern is that you have a comment saying that the function > does something different for one device versus others. This is not > the > place for such a comment. Here you can just document that it can > return two possible meanings. You should add comments explaining what > 0 means too (e.g. completed, but length unknown?). > > For something in progress, you should use -EINPROGRESS / -EAGAIN > Okay. > Regards, > Simon
diff --git a/include/dma.h b/include/dma.h index 0a0c9dd..b825c1e 100644 --- a/include/dma.h +++ b/include/dma.h @@ -79,8 +79,8 @@ int dma_get_device(u32 transfer_type, struct udevice **devp); * @dst - destination pointer * @src - souce pointer * @len - data length to be copied - * @return - on successful transfer returns no of bytes - transferred and on failure return error code. + * @return - on successful transfer returns no of bytes or zero(for DMA330) + * transferred and on failure return error code. */ int dma_memcpy(struct udevice *dev, void *dst, void *src, size_t len);