Re: [Outreachy kernel] [PATCH 5/5] staging: comedi: drivers: use unsigned int

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 12 Mar 2020 03:41:22 +0530
Kaaira Gupta <kgupta@es.iitr.ac.in> wrote:

> On Wed, Mar 11, 2020 at 09:51:10PM +0100, Julia Lawall wrote:
> > 
> > 
> > On Thu, 12 Mar 2020, Kaaira Gupta wrote:
> >   
> > > Change i's type to unsigned instead of signed. It's always positive.
> > > Reported by coccinelle.
> > >
> > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > > ---
> > >  drivers/staging/comedi/drivers/ni_mio_common.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> > > index 06ed4b1dd875..034cf8dafaf0 100644
> > > --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> > > +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> > > @@ -888,7 +888,7 @@ static void ni_sync_ai_dma(struct comedi_device *dev)
> > >  static int ni_ai_drain_dma(struct comedi_device *dev)
> > >  {
> > >  	struct ni_private *devpriv = dev->private;
> > > -	int i;
> > > +	unsigned int i;  
> > 
> > I'm not sure this is useful.  The value is not going to b lare enough to
> > require an unsigned.  And loop indices often have type int.  
> 
> Sorry, I didn't know that. I'll take care to see the largest value as
> well before making such changes. Thanks!
> 
> Should I resend the patchset by removing this patch or leave it as it
> is?

I think you should remove this patch, in the three cases where you have
that 'const int timeout = 10000' it's never needed to switch to an
unsigned value.

If you were to write that code from scratch, I would recommend that the
"10000" should be a #define, and all that register polling should be
consistent, I don't see any reason behind those differences, and the
udelay(5) feels like an abuse: Documentation/timers/timers-howto.rst is
a quick, nice read.

However, I'm quite sure you don't have that specific hardware to test
any change like that, so it's probably better to leave it alone :)

-- 
Stefano