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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx>
Thanks
>
> > chan->video_group = config->slave_id != 0;
> >
> > spin_unlock_irqrestore(&chan->lock, flags);
>
> --
> Regards,
>
> Laurent Pinchart
--
~Vinod
