| Message ID | 20200718135201.191881-1-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] dmaengine: xilinx: dpdma: remove comparison of unsigned expression | expand |
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);
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
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);
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(-)