[PATCH 1/3] dmaengine: xilinx: dpdma: remove comparison of unsigned expression

[PATCH 1/3] dmaengine: xilinx: dpdma: remove comparison of unsigned expression

[Vinod Koul]

xilinx_dpdma_config() channel id is unsigned int and compares with
ZYNQMP_DPDMA_VIDEO0 which is zero, so remove this comparison

drivers/dma/xilinx/xilinx_dpdma.c:1073:15: warning: comparison of
unsigned expression in ‘>= 0’ is always true [-Wtype-limits] if
	(chan->id >= ZYNQMP_DPDMA_VIDEO0 && chan->id <= ZYNQMP_DPDMA_VIDEO2)

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/dma/xilinx/xilinx_dpdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
index af88a6762ef4..8e602378f2dc 100644
--- a/drivers/dma/xilinx/xilinx_dpdma.c
+++ b/drivers/dma/xilinx/xilinx_dpdma.c
@@ -1070,7 +1070,7 @@ static int xilinx_dpdma_config(struct dma_chan *dchan,
 	 * Abuse the slave_id to indicate that the channel is part of a video
 	 * group.
 	 */
-	if (chan->id >= ZYNQMP_DPDMA_VIDEO0 && chan->id <= ZYNQMP_DPDMA_VIDEO2)
+	if (chan->id <= ZYNQMP_DPDMA_VIDEO2)
 		chan->video_group = config->slave_id != 0;
 
 	spin_unlock_irqrestore(&chan->lock, flags);
-- 
2.26.2

[PATCH 2/3] dmaengine: xilinx: dpdma: add missing kernel doc

[Vinod Koul]

xilinx_dpdma_sw_desc_set_dma_addrs() documentation is missing describing
'xdev', so add it

drivers/dma/xilinx/xilinx_dpdma.c:313: warning: Function parameter or
member 'xdev' not described in 'xilinx_dpdma_sw_desc_set_dma_addrs'

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/dma/xilinx/xilinx_dpdma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
index 8e602378f2dc..430f3714f6a3 100644
--- a/drivers/dma/xilinx/xilinx_dpdma.c
+++ b/drivers/dma/xilinx/xilinx_dpdma.c
@@ -295,6 +295,7 @@ static inline void dpdma_set(void __iomem *base, u32 offset, u32 set)
 
 /**
  * xilinx_dpdma_sw_desc_set_dma_addrs - Set DMA addresses in the descriptor
+ * @xdev: DPDMA device
  * @sw_desc: The software descriptor in which to set DMA addresses
  * @prev: The previous descriptor
  * @dma_addr: array of dma addresses
-- 
2.26.2

Re: [PATCH 2/3] dmaengine: xilinx: dpdma: add missing kernel doc

[Laurent Pinchart]

Hi Vinod,

Thank you for the patch.

On Sat, Jul 18, 2020 at 07:22:00PM +0530, Vinod Koul wrote:
> xilinx_dpdma_sw_desc_set_dma_addrs() documentation is missing describing
> 'xdev', so add it
> 
> drivers/dma/xilinx/xilinx_dpdma.c:313: warning: Function parameter or
> member 'xdev' not described in 'xilinx_dpdma_sw_desc_set_dma_addrs'
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/dma/xilinx/xilinx_dpdma.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
> index 8e602378f2dc..430f3714f6a3 100644
> --- a/drivers/dma/xilinx/xilinx_dpdma.c
> +++ b/drivers/dma/xilinx/xilinx_dpdma.c
> @@ -295,6 +295,7 @@ static inline void dpdma_set(void __iomem *base, u32 offset, u32 set)
>  
>  /**
>   * xilinx_dpdma_sw_desc_set_dma_addrs - Set DMA addresses in the descriptor
> + * @xdev: DPDMA device
>   * @sw_desc: The software descriptor in which to set DMA addresses
>   * @prev: The previous descriptor
>   * @dma_addr: array of dma addresses

-- 
Regards,

Laurent Pinchart

[PATCH 3/3] dmaengine: xilinx: dpdma: fix kernel doc format

[Vinod Koul]

xilinx_dpdma_chan structure documents 'desc' members, but that leads
to warnings, so split that up and describe members

drivers/dma/xilinx/xilinx_dpdma.c:241: warning: Function parameter or
member 'desc' not described in 'xilinx_dpdma_chan'

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/dma/xilinx/xilinx_dpdma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
index 430f3714f6a3..d94c75a842f8 100644
--- a/drivers/dma/xilinx/xilinx_dpdma.c
+++ b/drivers/dma/xilinx/xilinx_dpdma.c
@@ -214,8 +214,8 @@ struct xilinx_dpdma_tx_desc {
  * @lock: lock to access struct xilinx_dpdma_chan
  * @desc_pool: descriptor allocation pool
  * @err_task: error IRQ bottom half handler
- * @desc.pending: Descriptor schedule to the hardware, pending execution
- * @desc.active: Descriptor being executed by the hardware
+ * @desc: pending: Descriptor schedule to the hardware, pending execution
+ *        active: Descriptor being executed by the hardware
  * @xdev: DPDMA device
  */
 struct xilinx_dpdma_chan {
-- 
2.26.2

Re: [PATCH 3/3] dmaengine: xilinx: dpdma: fix kernel doc format

[Laurent Pinchart]

Hi Vinod,

Thank you for the patch.

On Sat, Jul 18, 2020 at 07:22:01PM +0530, Vinod Koul wrote:
> xilinx_dpdma_chan structure documents 'desc' members, but that leads
> to warnings, so split that up and describe members
> 
> drivers/dma/xilinx/xilinx_dpdma.c:241: warning: Function parameter or
> member 'desc' not described in 'xilinx_dpdma_chan'
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/dma/xilinx/xilinx_dpdma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
> index 430f3714f6a3..d94c75a842f8 100644
> --- a/drivers/dma/xilinx/xilinx_dpdma.c
> +++ b/drivers/dma/xilinx/xilinx_dpdma.c
> @@ -214,8 +214,8 @@ struct xilinx_dpdma_tx_desc {
>   * @lock: lock to access struct xilinx_dpdma_chan
>   * @desc_pool: descriptor allocation pool
>   * @err_task: error IRQ bottom half handler
> - * @desc.pending: Descriptor schedule to the hardware, pending execution
> - * @desc.active: Descriptor being executed by the hardware
> + * @desc: pending: Descriptor schedule to the hardware, pending execution
> + *        active: Descriptor being executed by the hardware

According to
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#nested-structs-unions,
the existing syntax is supposed to be valid. Where does the above
warning come from ?

>   * @xdev: DPDMA device
>   */
>  struct xilinx_dpdma_chan {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] dmaengine: xilinx: dpdma: fix kernel doc format

[Vinod Koul]

On 22-07-20, 16:11, Laurent Pinchart wrote:
> Hi Vinod,
> 
> Thank you for the patch.
> 
> On Sat, Jul 18, 2020 at 07:22:01PM +0530, Vinod Koul wrote:
> > xilinx_dpdma_chan structure documents 'desc' members, but that leads
> > to warnings, so split that up and describe members
> > 
> > drivers/dma/xilinx/xilinx_dpdma.c:241: warning: Function parameter or
> > member 'desc' not described in 'xilinx_dpdma_chan'
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  drivers/dma/xilinx/xilinx_dpdma.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
> > index 430f3714f6a3..d94c75a842f8 100644
> > --- a/drivers/dma/xilinx/xilinx_dpdma.c
> > +++ b/drivers/dma/xilinx/xilinx_dpdma.c
> > @@ -214,8 +214,8 @@ struct xilinx_dpdma_tx_desc {
> >   * @lock: lock to access struct xilinx_dpdma_chan
> >   * @desc_pool: descriptor allocation pool
> >   * @err_task: error IRQ bottom half handler
> > - * @desc.pending: Descriptor schedule to the hardware, pending execution
> > - * @desc.active: Descriptor being executed by the hardware
> > + * @desc: pending: Descriptor schedule to the hardware, pending execution
> > + *        active: Descriptor being executed by the hardware
> 
> According to
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#nested-structs-unions,
> the existing syntax is supposed to be valid. Where does the above
> warning come from ?

W=1 build again..

> 
> >   * @xdev: DPDMA device
> >   */
> >  struct xilinx_dpdma_chan {
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
~Vinod

Re: [PATCH 3/3] dmaengine: xilinx: dpdma: fix kernel doc format

[Laurent Pinchart]

Hi Vinod,

On Wed, Jul 22, 2020 at 07:56:08PM +0530, Vinod Koul wrote:
> On 22-07-20, 16:11, Laurent Pinchart wrote:
> > On Sat, Jul 18, 2020 at 07:22:01PM +0530, Vinod Koul wrote:
> > > xilinx_dpdma_chan structure documents 'desc' members, but that leads
> > > to warnings, so split that up and describe members
> > > 
> > > drivers/dma/xilinx/xilinx_dpdma.c:241: warning: Function parameter or
> > > member 'desc' not described in 'xilinx_dpdma_chan'
> > > 
> > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > ---
> > >  drivers/dma/xilinx/xilinx_dpdma.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
> > > index 430f3714f6a3..d94c75a842f8 100644
> > > --- a/drivers/dma/xilinx/xilinx_dpdma.c
> > > +++ b/drivers/dma/xilinx/xilinx_dpdma.c
> > > @@ -214,8 +214,8 @@ struct xilinx_dpdma_tx_desc {
> > >   * @lock: lock to access struct xilinx_dpdma_chan
> > >   * @desc_pool: descriptor allocation pool
> > >   * @err_task: error IRQ bottom half handler
> > > - * @desc.pending: Descriptor schedule to the hardware, pending execution
> > > - * @desc.active: Descriptor being executed by the hardware
> > > + * @desc: pending: Descriptor schedule to the hardware, pending execution
> > > + *        active: Descriptor being executed by the hardware
> > 
> > According to
> > https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#nested-structs-unions,
> > the existing syntax is supposed to be valid. Where does the above
> > warning come from ?
> 
> W=1 build again..

I get the same when plumbing the source file into the kerneldoc build.
The generated documentation however contains the description of both
desc.pending and desc.active. If you want to fix the warning, I think
you should instead add a line to document @desc, but without removing
the existing @desc.pending and @desc.active lines.

> > >   * @xdev: DPDMA device
> > >   */
> > >  struct xilinx_dpdma_chan {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] dmaengine: xilinx: dpdma: fix kernel doc format

[Vinod Koul]

On 22-07-20, 17:51, Laurent Pinchart wrote:

> > W=1 build again..
> 
> I get the same when plumbing the source file into the kerneldoc build.
> The generated documentation however contains the description of both
> desc.pending and desc.active. If you want to fix the warning, I think
> you should instead add a line to document @desc, but without removing
> the existing @desc.pending and @desc.active lines.

I would like to see clean build with W=1, helps to find other issues :)
So do feel free to send the patch to fix this, i will swap them..

-- 
~Vinod

Re: [PATCH 3/3] dmaengine: xilinx: dpdma: fix kernel doc format

[Laurent Pinchart]

On Wed, Jul 22, 2020 at 08:24:35PM +0530, Vinod Koul wrote:
> On 22-07-20, 17:51, Laurent Pinchart wrote:
> 
> > > W=1 build again..
> > 
> > I get the same when plumbing the source file into the kerneldoc build.
> > The generated documentation however contains the description of both
> > desc.pending and desc.active. If you want to fix the warning, I think
> > you should instead add a line to document @desc, but without removing
> > the existing @desc.pending and @desc.active lines.
> 
> I would like to see clean build with W=1, helps to find other issues :)
> So do feel free to send the patch to fix this, i will swap them..

Done, see "[PATCH] dmaengine: xilinx: dpdma: Fix kerneldoc warning".

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] dmaengine: xilinx: dpdma: remove comparison of unsigned expression

[Laurent Pinchart]

Hi Vinod,

Thank you for the patch.

On Sat, Jul 18, 2020 at 07:21:59PM +0530, Vinod Koul wrote:
> xilinx_dpdma_config() channel id is unsigned int and compares with
> ZYNQMP_DPDMA_VIDEO0 which is zero, so remove this comparison
> 
> drivers/dma/xilinx/xilinx_dpdma.c:1073:15: warning: comparison of
> unsigned expression in ‘>= 0’ is always true [-Wtype-limits] if
> 	(chan->id >= ZYNQMP_DPDMA_VIDEO0 && chan->id <= ZYNQMP_DPDMA_VIDEO2)

I didn't see that warning, how do you produce it ?

> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/dma/xilinx/xilinx_dpdma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
> index af88a6762ef4..8e602378f2dc 100644
> --- a/drivers/dma/xilinx/xilinx_dpdma.c
> +++ b/drivers/dma/xilinx/xilinx_dpdma.c
> @@ -1070,7 +1070,7 @@ static int xilinx_dpdma_config(struct dma_chan *dchan,
>  	 * Abuse the slave_id to indicate that the channel is part of a video
>  	 * group.
>  	 */
> -	if (chan->id >= ZYNQMP_DPDMA_VIDEO0 && chan->id <= ZYNQMP_DPDMA_VIDEO2)
> +	if (chan->id <= ZYNQMP_DPDMA_VIDEO2)

While this doesn't affect the behaviour, I'm worried about the risk of
introducing bugs in the future if the ZYNQMP_DPDMA_VIDEO0 becomes
non-zero. On the other hand, that's part of the DT ABI, so it shouldn't
change. How about

	switch (chan->id) {
	case ZYNQMP_DPDMA_VIDEO0:
	case ZYNQMP_DPDMA_VIDEO1:
	case ZYNQMP_DPDMA_VIDEO2:
  		chan->video_group = config->slave_id != 0;
		break;
	}

instead ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		chan->video_group = config->slave_id != 0;
>  
>  	spin_unlock_irqrestore(&chan->lock, flags);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] dmaengine: xilinx: dpdma: remove comparison of unsigned expression

[Vinod Koul]

On 22-07-20, 15:44, Laurent Pinchart wrote:
> Hi Vinod,
> 
> Thank you for the patch.
> 
> On Sat, Jul 18, 2020 at 07:21:59PM +0530, Vinod Koul wrote:
> > xilinx_dpdma_config() channel id is unsigned int and compares with
> > ZYNQMP_DPDMA_VIDEO0 which is zero, so remove this comparison
> > 
> > drivers/dma/xilinx/xilinx_dpdma.c:1073:15: warning: comparison of
> > unsigned expression in ‘>= 0’ is always true [-Wtype-limits] if
> > 	(chan->id >= ZYNQMP_DPDMA_VIDEO0 && chan->id <= ZYNQMP_DPDMA_VIDEO2)
> 
> I didn't see that warning, how do you produce it ?

I see it with gcc and W=1.

> 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  drivers/dma/xilinx/xilinx_dpdma.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
> > index af88a6762ef4..8e602378f2dc 100644
> > --- a/drivers/dma/xilinx/xilinx_dpdma.c
> > +++ b/drivers/dma/xilinx/xilinx_dpdma.c
> > @@ -1070,7 +1070,7 @@ static int xilinx_dpdma_config(struct dma_chan *dchan,
> >  	 * Abuse the slave_id to indicate that the channel is part of a video
> >  	 * group.
> >  	 */
> > -	if (chan->id >= ZYNQMP_DPDMA_VIDEO0 && chan->id <= ZYNQMP_DPDMA_VIDEO2)
> > +	if (chan->id <= ZYNQMP_DPDMA_VIDEO2)
> 
> While this doesn't affect the behaviour, I'm worried about the risk of
> introducing bugs in the future if the ZYNQMP_DPDMA_VIDEO0 becomes
> non-zero. On the other hand, that's part of the DT ABI, so it shouldn't
> change. How about
> 
> 	switch (chan->id) {
> 	case ZYNQMP_DPDMA_VIDEO0:
> 	case ZYNQMP_DPDMA_VIDEO1:
> 	case ZYNQMP_DPDMA_VIDEO2:
>   		chan->video_group = config->slave_id != 0;
> 		break;
> 	}
> 
> instead ?
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks

> 
> >  		chan->video_group = config->slave_id != 0;
> >  
> >  	spin_unlock_irqrestore(&chan->lock, flags);
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
~Vinod