[PATCH 0/5] patchset for clean-ups in comedi

[PATCH 0/5] patchset for clean-ups in comedi

[Kaaira Gupta]

THis patchset contains changes that reformat the code in a function to
make it more readbale, put conditions in one `if` statement, add lines
after a struct, add braces in a macro to prevent precedence issues, and
converts signed to unsigned int in driver comedi.

Kaaira Gupta (5):
  staging: comedi: ni_mio_common.c: reformat code
  staging: comedi: check condition in one statement
  staging: comedi: drivers: add line after struct
  staging: comedi: add braces to prevent precedence
  staging: comedi: drivers: use unsigned int

 drivers/staging/comedi/drivers/das08.c         | 1 +
 drivers/staging/comedi/drivers/das1800.c       | 2 +-
 drivers/staging/comedi/drivers/ni_mio_common.c | 8 ++++----
 drivers/staging/comedi/drivers/usbdux.c        | 4 +---
 4 files changed, 7 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH 1/5] staging: comedi: ni_mio_common.c: reformat code

[Kaaira Gupta]

reformat the arguments given to ni_stc_writew() to increase code
readability in ni_mio_common.c

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 drivers/staging/comedi/drivers/ni_mio_common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index b72a40a79930..06ed4b1dd875 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -2445,9 +2445,9 @@ static int ni_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
 #endif
 
 	if (cmd->start_src == TRIG_NOW) {
-		ni_stc_writew(dev, NISTC_AI_CMD2_START1_PULSE |
-				   devpriv->ai_cmd2,
-			      NISTC_AI_CMD2_REG);
+		ni_stc_writew(dev, devpriv->ai_cmd2 |
+				NISTC_AI_CMD2_START1_PULSE,
+				NISTC_AI_CMD2_REG);
 		s->async->inttrig = NULL;
 	} else if (cmd->start_src == TRIG_EXT) {
 		s->async->inttrig = NULL;
-- 
2.17.1

[PATCH 2/5] staging: comedi: check condition in one statement

[Kaaira Gupta]

check two conditions in one `if` statement rather than two since both
return same thing in file usbdux.c

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 drivers/staging/comedi/drivers/usbdux.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
index 0350f303d557..8eff7f140560 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -617,9 +617,7 @@ static int receive_dux_commands(struct comedi_device *dev, unsigned int command)
 		ret = usb_bulk_msg(usb, usb_rcvbulkpipe(usb, 8),
 				   devpriv->insn_buf, SIZEINSNBUF,
 				   &nrec, BULK_TIMEOUT);
-		if (ret < 0)
-			return ret;
-		if (le16_to_cpu(devpriv->insn_buf[0]) == command)
+		if (ret < 0 || le16_to_cpu(devpriv->insn_buf[0]) == command)
 			return ret;
 	}
 	/* command not received */
-- 
2.17.1

[PATCH 3/5] staging: comedi: drivers: add line after struct

[Kaaira Gupta]

Add a blank line after struct declaration

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 drivers/staging/comedi/drivers/das08.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/comedi/drivers/das08.c b/drivers/staging/comedi/drivers/das08.c
index 65e5f2e6c122..e6c1ff0e5beb 100644
--- a/drivers/staging/comedi/drivers/das08.c
+++ b/drivers/staging/comedi/drivers/das08.c
@@ -141,6 +141,7 @@ static const struct comedi_lrange *const das08_ai_lranges[] = {
 static const int das08_pgh_ai_gainlist[] = {
 	8, 0, 10, 2, 12, 4, 14, 6, 1, 3, 5, 7
 };
+
 static const int das08_pgl_ai_gainlist[] = { 8, 0, 2, 4, 6, 1, 3, 5, 7 };
 static const int das08_pgm_ai_gainlist[] = { 8, 0, 10, 12, 14, 9, 11, 13, 15 };
 
-- 
2.17.1

[PATCH 4/5] staging: comedi: add braces to prevent precedence

[Kaaira Gupta]

Add braces around 'a' in comedi/drivers/das1800.c to prevent precedence
isuues.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
 drivers/staging/comedi/drivers/das1800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/das1800.c b/drivers/staging/comedi/drivers/das1800.c
index f16aa7e9f4f3..be4b33c8e93d 100644
--- a/drivers/staging/comedi/drivers/das1800.c
+++ b/drivers/staging/comedi/drivers/das1800.c
@@ -91,7 +91,7 @@
 #define DAS1800_SELECT          0x2
 #define   ADC                     0x0
 #define   QRAM                    0x1
-#define   DAC(a)                  (0x2 + a)
+#define   DAC(a)                  (0x2 + (a))
 #define DAS1800_DIGITAL         0x3
 #define DAS1800_CONTROL_A       0x4
 #define   FFEN                    0x1
-- 
2.17.1

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

[Kaaira Gupta]

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;
 	static const int timeout = 10000;
 	unsigned long flags;
 	int retval = 0;
-- 
2.17.1

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

[Julia Lawall]

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.

julia

>  	static const int timeout = 10000;
>  	unsigned long flags;
>  	int retval = 0;
> --
> 2.17.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200311184327.25338-6-kgupta%40es.iitr.ac.in.
>

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

[Kaaira Gupta]

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?

> 
> julia
> 
> >  	static const int timeout = 10000;
> >  	unsigned long flags;
> >  	int retval = 0;
> > --
> > 2.17.1
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200311184327.25338-6-kgupta%40es.iitr.ac.in.
> >

Re: [Outreachy kernel] [PATCH 2/5] staging: comedi: check condition in one statement

[Stefano Brivio]

On Thu, 12 Mar 2020 00:13:24 +0530
Kaaira Gupta <kgupta@es.iitr.ac.in> wrote:

> check two conditions in one `if` statement rather than two since both
> return same thing in file usbdux.c
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  drivers/staging/comedi/drivers/usbdux.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
> index 0350f303d557..8eff7f140560 100644
> --- a/drivers/staging/comedi/drivers/usbdux.c
> +++ b/drivers/staging/comedi/drivers/usbdux.c
> @@ -617,9 +617,7 @@ static int receive_dux_commands(struct comedi_device *dev, unsigned int command)
>  		ret = usb_bulk_msg(usb, usb_rcvbulkpipe(usb, 8),
>  				   devpriv->insn_buf, SIZEINSNBUF,
>  				   &nrec, BULK_TIMEOUT);
> -		if (ret < 0)
> -			return ret;
> -		if (le16_to_cpu(devpriv->insn_buf[0]) == command)
> +		if (ret < 0 || le16_to_cpu(devpriv->insn_buf[0]) == command)
>  			return ret;

I personally would prefer the original version in this case: the reason
why this function returns early in the two cases is fairly different:
first time it's an error, second time is because the same command is
already in the buffer. So keeping two 'return' statements makes it
easier to understand, in my opinion.

-- 
Stefano

Re: [Outreachy kernel] [PATCH 1/5] staging: comedi: ni_mio_common.c: reformat code

[Stefano Brivio]

On Thu, 12 Mar 2020 00:13:23 +0530
Kaaira Gupta <kgupta@es.iitr.ac.in> wrote:

> reformat the arguments given to ni_stc_writew() to increase code
> readability in ni_mio_common.c
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  drivers/staging/comedi/drivers/ni_mio_common.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index b72a40a79930..06ed4b1dd875 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -2445,9 +2445,9 @@ static int ni_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
>  #endif
>  
>  	if (cmd->start_src == TRIG_NOW) {
> -		ni_stc_writew(dev, NISTC_AI_CMD2_START1_PULSE |
> -				   devpriv->ai_cmd2,
> -			      NISTC_AI_CMD2_REG);
> +		ni_stc_writew(dev, devpriv->ai_cmd2 |
> +				NISTC_AI_CMD2_START1_PULSE,
> +				NISTC_AI_CMD2_REG);

This becomes confusing: you're aligning NISTC_AI_CMD2_REG with
NISTC_AI_CMD2_START1_PULSE, but it's two different arguments, and
breaking the alignment between devpriv->ai_cmd2 and
NISTC_AI_CMD2_START1_PULSE, which should instead be preserved as
they are the two operands.

Did you find this with an automated tool? What was the reported
issue? At a quick visual check, I don't see any.

-- 
Stefano

Re: [PATCH 1/5] staging: comedi: ni_mio_common.c: reformat code

[Greg Kroah-Hartman]

On Thu, Mar 12, 2020 at 12:13:23AM +0530, Kaaira Gupta wrote:
> reformat the arguments given to ni_stc_writew() to increase code
> readability in ni_mio_common.c
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  drivers/staging/comedi/drivers/ni_mio_common.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index b72a40a79930..06ed4b1dd875 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -2445,9 +2445,9 @@ static int ni_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
>  #endif
>  
>  	if (cmd->start_src == TRIG_NOW) {
> -		ni_stc_writew(dev, NISTC_AI_CMD2_START1_PULSE |
> -				   devpriv->ai_cmd2,
> -			      NISTC_AI_CMD2_REG);
> +		ni_stc_writew(dev, devpriv->ai_cmd2 |
> +				NISTC_AI_CMD2_START1_PULSE,
> +				NISTC_AI_CMD2_REG);

No, the original is correct.  What caused you to want to rewrite this?

thanks,

greg k-h

Re: [PATCH 2/5] staging: comedi: check condition in one statement

[Greg Kroah-Hartman]

On Thu, Mar 12, 2020 at 12:13:24AM +0530, Kaaira Gupta wrote:
> check two conditions in one `if` statement rather than two since both
> return same thing in file usbdux.c
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  drivers/staging/comedi/drivers/usbdux.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
> index 0350f303d557..8eff7f140560 100644
> --- a/drivers/staging/comedi/drivers/usbdux.c
> +++ b/drivers/staging/comedi/drivers/usbdux.c
> @@ -617,9 +617,7 @@ static int receive_dux_commands(struct comedi_device *dev, unsigned int command)
>  		ret = usb_bulk_msg(usb, usb_rcvbulkpipe(usb, 8),
>  				   devpriv->insn_buf, SIZEINSNBUF,
>  				   &nrec, BULK_TIMEOUT);
> -		if (ret < 0)
> -			return ret;
> -		if (le16_to_cpu(devpriv->insn_buf[0]) == command)
> +		if (ret < 0 || le16_to_cpu(devpriv->insn_buf[0]) == command)
>  			return ret;

The original is better, please leave it as-is.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Thu, Mar 12, 2020 at 12:13:27AM +0530, 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;

Why is this needed?

Re: [PATCH 4/5] staging: comedi: add braces to prevent precedence

[Greg Kroah-Hartman]

On Thu, Mar 12, 2020 at 12:13:26AM +0530, Kaaira Gupta wrote:
> Add braces around 'a' in comedi/drivers/das1800.c to prevent precedence
> isuues.
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  drivers/staging/comedi/drivers/das1800.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/comedi/drivers/das1800.c b/drivers/staging/comedi/drivers/das1800.c
> index f16aa7e9f4f3..be4b33c8e93d 100644
> --- a/drivers/staging/comedi/drivers/das1800.c
> +++ b/drivers/staging/comedi/drivers/das1800.c
> @@ -91,7 +91,7 @@
>  #define DAS1800_SELECT          0x2
>  #define   ADC                     0x0
>  #define   QRAM                    0x1
> -#define   DAC(a)                  (0x2 + a)
> +#define   DAC(a)                  (0x2 + (a))

There are no precedence issues with the use of this macro.

But, to be "safe" it is fine to do, just use a better reason in the
changelog comments please.

thanks,

greg k-h

Re: [PATCH 3/5] staging: comedi: drivers: add line after struct

[Greg Kroah-Hartman]

On Thu, Mar 12, 2020 at 12:13:25AM +0530, Kaaira Gupta wrote:
> Add a blank line after struct declaration
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
>  drivers/staging/comedi/drivers/das08.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/comedi/drivers/das08.c b/drivers/staging/comedi/drivers/das08.c
> index 65e5f2e6c122..e6c1ff0e5beb 100644
> --- a/drivers/staging/comedi/drivers/das08.c
> +++ b/drivers/staging/comedi/drivers/das08.c
> @@ -141,6 +141,7 @@ static const struct comedi_lrange *const das08_ai_lranges[] = {
>  static const int das08_pgh_ai_gainlist[] = {
>  	8, 0, 10, 2, 12, 4, 14, 6, 1, 3, 5, 7
>  };
> +

You said _what_ you did in the changelog (and subject), but not _WHY_
you did it.

thanks,

greg k-h

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

[Kaaira Gupta]

On Thu, Mar 12, 2020 at 09:17:14AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 12, 2020 at 12:13:27AM +0530, 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;
> 
> Why is this needed?

Since it's always positive and I though that it's maximum value might get
greater than 2^31, hence I changed it.

> 

Re: [PATCH 4/5] staging: comedi: add braces to prevent precedence

[Kaaira Gupta]

On Thu, Mar 12, 2020 at 09:18:24AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 12, 2020 at 12:13:26AM +0530, Kaaira Gupta wrote:
> > Add braces around 'a' in comedi/drivers/das1800.c to prevent precedence
> > isuues.
> > 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> >  drivers/staging/comedi/drivers/das1800.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/comedi/drivers/das1800.c b/drivers/staging/comedi/drivers/das1800.c
> > index f16aa7e9f4f3..be4b33c8e93d 100644
> > --- a/drivers/staging/comedi/drivers/das1800.c
> > +++ b/drivers/staging/comedi/drivers/das1800.c
> > @@ -91,7 +91,7 @@
> >  #define DAS1800_SELECT          0x2
> >  #define   ADC                     0x0
> >  #define   QRAM                    0x1
> > -#define   DAC(a)                  (0x2 + a)
> > +#define   DAC(a)                  (0x2 + (a))
> 
> There are no precedence issues with the use of this macro.
> 
> But, to be "safe" it is fine to do, just use a better reason in the
> changelog comments please.

Yes, sure.

> 
> thanks,
> 
> greg k-h

Re: [PATCH 3/5] staging: comedi: drivers: add line after struct

[Kaaira Gupta]

On Thu, Mar 12, 2020 at 09:18:46AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 12, 2020 at 12:13:25AM +0530, Kaaira Gupta wrote:
> > Add a blank line after struct declaration
> > 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> >  drivers/staging/comedi/drivers/das08.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/staging/comedi/drivers/das08.c b/drivers/staging/comedi/drivers/das08.c
> > index 65e5f2e6c122..e6c1ff0e5beb 100644
> > --- a/drivers/staging/comedi/drivers/das08.c
> > +++ b/drivers/staging/comedi/drivers/das08.c
> > @@ -141,6 +141,7 @@ static const struct comedi_lrange *const das08_ai_lranges[] = {
> >  static const int das08_pgh_ai_gainlist[] = {
> >  	8, 0, 10, 2, 12, 4, 14, 6, 1, 3, 5, 7
> >  };
> > +
> 
> You said _what_ you did in the changelog (and subject), but not _WHY_
> you did it.

I'll correct it. Thanks!

> 
> thanks,
> 
> greg k-h

Re: [PATCH 2/5] staging: comedi: check condition in one statement

[Kaaira Gupta]

On Thu, Mar 12, 2020 at 09:17:02AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 12, 2020 at 12:13:24AM +0530, Kaaira Gupta wrote:
> > check two conditions in one `if` statement rather than two since both
> > return same thing in file usbdux.c
> > 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> >  drivers/staging/comedi/drivers/usbdux.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
> > index 0350f303d557..8eff7f140560 100644
> > --- a/drivers/staging/comedi/drivers/usbdux.c
> > +++ b/drivers/staging/comedi/drivers/usbdux.c
> > @@ -617,9 +617,7 @@ static int receive_dux_commands(struct comedi_device *dev, unsigned int command)
> >  		ret = usb_bulk_msg(usb, usb_rcvbulkpipe(usb, 8),
> >  				   devpriv->insn_buf, SIZEINSNBUF,
> >  				   &nrec, BULK_TIMEOUT);
> > -		if (ret < 0)
> > -			return ret;
> > -		if (le16_to_cpu(devpriv->insn_buf[0]) == command)
> > +		if (ret < 0 || le16_to_cpu(devpriv->insn_buf[0]) == command)
> >  			return ret;
> 
> The original is better, please leave it as-is.

OKay! 
Thanks!

> 
> thanks,
> 
> greg k-h

Re: [PATCH 1/5] staging: comedi: ni_mio_common.c: reformat code

[Kaaira Gupta]

On Thu, Mar 12, 2020 at 09:16:45AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 12, 2020 at 12:13:23AM +0530, Kaaira Gupta wrote:
> > reformat the arguments given to ni_stc_writew() to increase code
> > readability in ni_mio_common.c
> > 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > ---
> >  drivers/staging/comedi/drivers/ni_mio_common.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> > index b72a40a79930..06ed4b1dd875 100644
> > --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> > +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> > @@ -2445,9 +2445,9 @@ static int ni_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
> >  #endif
> >  
> >  	if (cmd->start_src == TRIG_NOW) {
> > -		ni_stc_writew(dev, NISTC_AI_CMD2_START1_PULSE |
> > -				   devpriv->ai_cmd2,
> > -			      NISTC_AI_CMD2_REG);
> > +		ni_stc_writew(dev, devpriv->ai_cmd2 |
> > +				NISTC_AI_CMD2_START1_PULSE,
> > +				NISTC_AI_CMD2_REG);
> 
> No, the original is correct.  What caused you to want to rewrite this?

It is more readable and gives the same results, hence changed it.

> 
> thanks,
> 
> greg k-h

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

[Greg Kroah-Hartman]

On Thu, Mar 12, 2020 at 08:22:31PM +0530, Kaaira Gupta wrote:
> On Thu, Mar 12, 2020 at 09:17:14AM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Mar 12, 2020 at 12:13:27AM +0530, 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;
> > 
> > Why is this needed?
> 
> Since it's always positive and I though that it's maximum value might get
> greater than 2^31, hence I changed it.

Then say so in the changelog text.

But first verify that this really can get that big, otherwise just leave
it alone.

thanks,

greg k-h

Re: [PATCH 1/5] staging: comedi: ni_mio_common.c: reformat code

[Greg Kroah-Hartman]

On Thu, Mar 12, 2020 at 08:26:03PM +0530, Kaaira Gupta wrote:
> On Thu, Mar 12, 2020 at 09:16:45AM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Mar 12, 2020 at 12:13:23AM +0530, Kaaira Gupta wrote:
> > > reformat the arguments given to ni_stc_writew() to increase code
> > > readability in ni_mio_common.c
> > > 
> > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> > > ---
> > >  drivers/staging/comedi/drivers/ni_mio_common.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> > > index b72a40a79930..06ed4b1dd875 100644
> > > --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> > > +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> > > @@ -2445,9 +2445,9 @@ static int ni_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
> > >  #endif
> > >  
> > >  	if (cmd->start_src == TRIG_NOW) {
> > > -		ni_stc_writew(dev, NISTC_AI_CMD2_START1_PULSE |
> > > -				   devpriv->ai_cmd2,
> > > -			      NISTC_AI_CMD2_REG);
> > > +		ni_stc_writew(dev, devpriv->ai_cmd2 |
> > > +				NISTC_AI_CMD2_START1_PULSE,
> > > +				NISTC_AI_CMD2_REG);
> > 
> > No, the original is correct.  What caused you to want to rewrite this?
> 
> It is more readable and gives the same results, hence changed it.

It is not more readable, you just lost some context information here,
making the code harder to read and understand easily :(

Whitespace matters, otherwise we wouldn't be doing all of these cleanups :)

thanks,

greg k-h

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

[Stefano Brivio]

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