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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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