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

[Vinod Koul <vkoul@kernel.org>]

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