Re: [PATCH] staging: comedi: Improved readability of function comedi_nsamples_left.

Re: [PATCH] staging: comedi: Improved readability of function comedi_nsamples_left.

[Greg Kroah-Hartman]

On Sat, Jun 09, 2018 at 04:23:21PM +0200, Chris Opperman wrote:
> Signed-off-by: Chris Opperman <eklikeroomys@gmail.com>
> ---
>  drivers/staging/comedi/drivers.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)

I can not take patches without any changelog text at all :(

[PATCH] staging: comedi: Improved readability of function comedi_nsamples_left.

[Chris Opperman]

Signed-off-by: Chris Opperman <eklikeroomys@gmail.com>
---
 drivers/staging/comedi/drivers.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 9d73347..3207ae2 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -468,26 +468,25 @@ EXPORT_SYMBOL_GPL(comedi_nscans_left);
  * Returns the number of samples remaining to complete the command, or the
  * specified expected number of samples (@nsamples), whichever is fewer.
  */
-unsigned int comedi_nsamples_left(struct comedi_subdevice *s,
-				  unsigned int nsamples)
+u32 comedi_nsamples_left(struct comedi_subdevice *s, u32 nsamples)
 {
 	struct comedi_async *async = s->async;
 	struct comedi_cmd *cmd = &async->cmd;
+	u32 scans_left;
+	u64 samples_left;
 
-	if (cmd->stop_src == TRIG_COUNT) {
-		unsigned int scans_left = __comedi_nscans_left(s, cmd->stop_arg);
-		unsigned int scan_pos =
-		    comedi_bytes_to_samples(s, async->scan_progress);
-		unsigned long long samples_left = 0;
-
-		if (scans_left) {
-			samples_left = ((unsigned long long)scans_left *
-					cmd->scan_end_arg) - scan_pos;
-		}
+	if (cmd->stop_src != TRIG_COUNT)
+		return nsamples;
 
-		if (samples_left < nsamples)
-			nsamples = samples_left;
-	}
+	scans_left = __comedi_nscans_left(s, cmd->stop_arg);
+	if (!scans_left)
+		return 0;
+
+	samples_left = ((u64)scans_left * cmd->scan_end_arg) -
+		comedi_bytes_to_samples(s, async->scan_progress);
+
+	if (samples_left < nsamples)
+		return samples_left;
 	return nsamples;
 }
 EXPORT_SYMBOL_GPL(comedi_nsamples_left);
-- 
2.1.4

Re: [PATCH] staging: comedi: Improved readability of function comedi_nsamples_left.

[chris]

Hi Greg,
I've added changelog text to the patch below. Appreciate your 
feedback!
Regards,
Chris Opperman

>8----------------------------------------------------------------------8<

Improved the readability of comedi_nsamples_left:
a) Reduced nesting by using more return calls.
b) Separated variable declarations from code.
c) Using kernel integer types.

Signed-off-by: Chris Opperman <eklikeroomys@gmail.com>
---
 drivers/staging/comedi/drivers.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 9d73347..3207ae2 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -468,26 +468,25 @@ EXPORT_SYMBOL_GPL(comedi_nscans_left);
  * Returns the number of samples remaining to complete the command, or the
  * specified expected number of samples (@nsamples), whichever is fewer.
  */
-unsigned int comedi_nsamples_left(struct comedi_subdevice *s,
-				  unsigned int nsamples)
+u32 comedi_nsamples_left(struct comedi_subdevice *s, u32 nsamples)
 {
 	struct comedi_async *async = s->async;
 	struct comedi_cmd *cmd = &async->cmd;
+	u32 scans_left;
+	u64 samples_left;
 
-	if (cmd->stop_src == TRIG_COUNT) {
-		unsigned int scans_left = __comedi_nscans_left(s, cmd->stop_arg);
-		unsigned int scan_pos =
-		    comedi_bytes_to_samples(s, async->scan_progress);
-		unsigned long long samples_left = 0;
-
-		if (scans_left) {
-			samples_left = ((unsigned long long)scans_left *
-					cmd->scan_end_arg) - scan_pos;
-		}
+	if (cmd->stop_src != TRIG_COUNT)
+		return nsamples;
 
-		if (samples_left < nsamples)
-			nsamples = samples_left;
-	}
+	scans_left = __comedi_nscans_left(s, cmd->stop_arg);
+	if (!scans_left)
+		return 0;
+
+	samples_left = ((u64)scans_left * cmd->scan_end_arg) -
+		comedi_bytes_to_samples(s, async->scan_progress);
+
+	if (samples_left < nsamples)
+		return samples_left;
 	return nsamples;
 }
 EXPORT_SYMBOL_GPL(comedi_nsamples_left);
-- 
2.1.4

Re: [PATCH] staging: comedi: Improved readability of function comedi_nsamples_left.

[Dan Carpenter]

You haven't been sending the v2 and v3 patches in the right way.

Take a look through the email archive and see how this is normally done.
Also google it:
https://www.google.com/search?q=how+to+send+a+v2+patch

On Sun, Jun 10, 2018 at 12:30:01PM +0200, chris wrote:
> Hi Greg,
> I've added changelog text to the patch below. Appreciate your 
> feedback!
> Regards,
> Chris Opperman
> 
> >8----------------------------------------------------------------------8<
> 
> Improved the readability of comedi_nsamples_left:
> a) Reduced nesting by using more return calls.

nit: return isn't a function so it can't be called.

> b) Separated variable declarations from code.

It was separate in the original code too.

> c) Using kernel integer types.

The original types were fine/better.

The rules are that we don't like uint32_t and the reason for not liking
it is that we looked through the kernel code and standardized on what
was most common...

Generally u32 and friends mean that the hardware or network protocol
specifies the number of bits.  That's especially true for s32.  If you
see code using s32 when it's not tied to a hardware spec then that's a
sign that the code is written by an insane person and it's a tricky
situation to deal with.

Some other reasons to use u32 is because it's shorter to type than
unsigned int and because of line lengths...  I've sometimes done that.
But it's perhaps lazy.

> 
> Signed-off-by: Chris Opperman <eklikeroomys@gmail.com>
> ---
>  drivers/staging/comedi/drivers.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
> index 9d73347..3207ae2 100644
> --- a/drivers/staging/comedi/drivers.c
> +++ b/drivers/staging/comedi/drivers.c
> @@ -468,26 +468,25 @@ EXPORT_SYMBOL_GPL(comedi_nscans_left);
>   * Returns the number of samples remaining to complete the command, or the
>   * specified expected number of samples (@nsamples), whichever is fewer.
>   */
> -unsigned int comedi_nsamples_left(struct comedi_subdevice *s,
> -				  unsigned int nsamples)
> +u32 comedi_nsamples_left(struct comedi_subdevice *s, u32 nsamples)
>  {
>  	struct comedi_async *async = s->async;
>  	struct comedi_cmd *cmd = &async->cmd;
> +	u32 scans_left;

I would make scans_left an unsigned long long so that you can remove
the cast when you do the multiply.

> +	u64 samples_left;

Nothing wrong with unsigned long long.

regards,
dan carpenter

Re: [PATCH] staging: comedi: Improved readability of function comedi_nsamples_left.

[Chris Opperman]

Hi Dan,
Thank you for the feedback, I'll update V4 of the patch accordingly
and resend. I'll soon get a hang of the workflow! :)
Best Regards,
Chris Opperman