[PATCH 0/2] Enhance code readability

[PATCH 0/2] Enhance code readability

[Narcisa Ana Maria Vasile]

Refactor controls.c for readability

Narcisa Ana Maria Vasile (2):
  staging: vc04_services: bcm2835-camera: Remove unnecessary else
    instruction
  staging: vc04_services: bcm2835-camera: Remove unnecessary variable

 .../staging/vc04_services/bcm2835-camera/controls.c   | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH 1/2] staging: vc04_services: bcm2835-camera: Remove unnecessary else instruction

[Narcisa Ana Maria Vasile]

Initialize the variable shutter_speed to avoid unnecessary else branch

Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
---
 drivers/staging/vc04_services/bcm2835-camera/controls.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c
index 9c0667e..dcd5bec 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
@@ -798,7 +798,7 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
 			       const struct bm2835_mmal_v4l2_ctrl *mmal_ctrl)
 {
 	int ret = 0;
-	int shutter_speed;
+	int shutter_speed = 0;
 	struct vchiq_mmal_port *control;
 
 	v4l2_dbg(0, bcm2835_v4l2_debug, &dev->v4l2_dev,
@@ -815,8 +815,6 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
 
 		if (dev->exposure_mode_user == MMAL_PARAM_EXPOSUREMODE_OFF)
 			shutter_speed = dev->manual_shutter_speed;
-		else
-			shutter_speed = 0;
 
 		v4l2_dbg(0, bcm2835_v4l2_debug, &dev->v4l2_dev,
 			 "%s: scene mode none: shut_speed %d, exp_mode %d, metering %d\n",
@@ -843,7 +841,7 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
 		/* Set up scene mode */
 		int i;
 		const struct v4l2_mmal_scene_config *scene = NULL;
-		int shutter_speed;
+		int shutter_speed = 0;
 		enum mmal_parameter_exposuremode exposure_mode;
 		enum mmal_parameter_exposuremeteringmode metering_mode;
 
@@ -864,8 +862,7 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
 
 		if (scene->exposure_mode == MMAL_PARAM_EXPOSUREMODE_OFF)
 			shutter_speed = dev->manual_shutter_speed;
-		else
-			shutter_speed = 0;
+		
 		exposure_mode = scene->exposure_mode;
 		metering_mode = scene->metering_mode;
 
-- 
1.9.1

[PATCH 2/2] staging: vc04_services: bcm2835-camera: Remove unnecessary variable

[Narcisa Ana Maria Vasile]

Instead of using the ret variable, return 0 directly

Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
---
 drivers/staging/vc04_services/bcm2835-camera/controls.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c
index dcd5bec..e25f1c9 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
@@ -609,18 +609,16 @@ static int ctrl_set_bitrate(struct bm2835_mmal_dev *dev,
 			    struct v4l2_ctrl *ctrl,
 			    const struct bm2835_mmal_v4l2_ctrl *mmal_ctrl)
 {
-	int ret;
 	struct vchiq_mmal_port *encoder_out;
 
 	dev->capture.encode_bitrate = ctrl->val;
 
 	encoder_out = &dev->component[MMAL_COMPONENT_VIDEO_ENCODE]->output[0];
 
-	ret = vchiq_mmal_port_parameter_set(dev->instance, encoder_out,
-					    mmal_ctrl->mmal_id,
-					    &ctrl->val, sizeof(ctrl->val));
-	ret = 0;
-	return ret;
+	vchiq_mmal_port_parameter_set(dev->instance, encoder_out,
+				      mmal_ctrl->mmal_id,
+				      &ctrl->val, sizeof(ctrl->val));
+	return 0;
 }
 
 static int ctrl_set_bitrate_mode(struct bm2835_mmal_dev *dev,
@@ -862,7 +860,7 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
 
 		if (scene->exposure_mode == MMAL_PARAM_EXPOSUREMODE_OFF)
 			shutter_speed = dev->manual_shutter_speed;
-		
+
 		exposure_mode = scene->exposure_mode;
 		metering_mode = scene->metering_mode;
 
-- 
1.9.1

Re: [Outreachy kernel] [PATCH 2/2] staging: vc04_services: bcm2835-camera: Remove unnecessary variable

[Julia Lawall]

On Wed, 1 Mar 2017, Narcisa Ana Maria Vasile wrote:

> Instead of using the ret variable, return 0 directly
>
> Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
> ---
>  drivers/staging/vc04_services/bcm2835-camera/controls.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> index dcd5bec..e25f1c9 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
> +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> @@ -609,18 +609,16 @@ static int ctrl_set_bitrate(struct bm2835_mmal_dev *dev,
>  			    struct v4l2_ctrl *ctrl,
>  			    const struct bm2835_mmal_v4l2_ctrl *mmal_ctrl)
>  {
> -	int ret;
>  	struct vchiq_mmal_port *encoder_out;
>
>  	dev->capture.encode_bitrate = ctrl->val;
>
>  	encoder_out = &dev->component[MMAL_COMPONENT_VIDEO_ENCODE]->output[0];
>
> -	ret = vchiq_mmal_port_parameter_set(dev->instance, encoder_out,
> -					    mmal_ctrl->mmal_id,
> -					    &ctrl->val, sizeof(ctrl->val));
> -	ret = 0;
> -	return ret;
> +	vchiq_mmal_port_parameter_set(dev->instance, encoder_out,
> +				      mmal_ctrl->mmal_id,
> +				      &ctrl->val, sizeof(ctrl->val));
> +	return 0;
>  }
>
>  static int ctrl_set_bitrate_mode(struct bm2835_mmal_dev *dev,
> @@ -862,7 +860,7 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
>
>  		if (scene->exposure_mode == MMAL_PARAM_EXPOSUREMODE_OFF)
>  			shutter_speed = dev->manual_shutter_speed;
> -
> +

Something went wrong here.  It's fine to fi the whitespace issue, but it
should be a separate patch.

julia

>  		exposure_mode = scene->exposure_mode;
>  		metering_mode = scene->metering_mode;
>
> --
> 1.9.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 post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/c49c187ff219c61323b25f01ab3f051950ae507a.1488371940.git.narcisaanamaria12%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH 1/2] staging: vc04_services: bcm2835-camera: Remove unnecessary else instruction

[Julia Lawall]

On Wed, 1 Mar 2017, Narcisa Ana Maria Vasile wrote:

> Initialize the variable shutter_speed to avoid unnecessary else branch

Is the else really bad?  With it, one can see the two possible values at
once.

julia

>
> Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
> ---
>  drivers/staging/vc04_services/bcm2835-camera/controls.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> index 9c0667e..dcd5bec 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
> +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> @@ -798,7 +798,7 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
>  			       const struct bm2835_mmal_v4l2_ctrl *mmal_ctrl)
>  {
>  	int ret = 0;
> -	int shutter_speed;
> +	int shutter_speed = 0;
>  	struct vchiq_mmal_port *control;
>
>  	v4l2_dbg(0, bcm2835_v4l2_debug, &dev->v4l2_dev,
> @@ -815,8 +815,6 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
>
>  		if (dev->exposure_mode_user == MMAL_PARAM_EXPOSUREMODE_OFF)
>  			shutter_speed = dev->manual_shutter_speed;
> -		else
> -			shutter_speed = 0;
>
>  		v4l2_dbg(0, bcm2835_v4l2_debug, &dev->v4l2_dev,
>  			 "%s: scene mode none: shut_speed %d, exp_mode %d, metering %d\n",
> @@ -843,7 +841,7 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
>  		/* Set up scene mode */
>  		int i;
>  		const struct v4l2_mmal_scene_config *scene = NULL;
> -		int shutter_speed;
> +		int shutter_speed = 0;
>  		enum mmal_parameter_exposuremode exposure_mode;
>  		enum mmal_parameter_exposuremeteringmode metering_mode;
>
> @@ -864,8 +862,7 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
>
>  		if (scene->exposure_mode == MMAL_PARAM_EXPOSUREMODE_OFF)
>  			shutter_speed = dev->manual_shutter_speed;
> -		else
> -			shutter_speed = 0;
> +
>  		exposure_mode = scene->exposure_mode;
>  		metering_mode = scene->metering_mode;
>
> --
> 1.9.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 post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/ff2d51e7327a03defd26b7d3561eb2c154a2fa0c.1488371940.git.narcisaanamaria12%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH 2/2] staging: vc04_services: bcm2835-camera: Remove unnecessary variable

[Narcisa Ana Maria Vasile]

On Wed, Mar 01, 2017 at 02:02:39PM +0100, Julia Lawall wrote:
> 
> 
> On Wed, 1 Mar 2017, Narcisa Ana Maria Vasile wrote:
> 
> > Instead of using the ret variable, return 0 directly
> >
> > Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
> > ---
> >  drivers/staging/vc04_services/bcm2835-camera/controls.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > index dcd5bec..e25f1c9 100644
> > --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > @@ -609,18 +609,16 @@ static int ctrl_set_bitrate(struct bm2835_mmal_dev *dev,
> >  			    struct v4l2_ctrl *ctrl,
> >  			    const struct bm2835_mmal_v4l2_ctrl *mmal_ctrl)
> >  {
> > -	int ret;
> >  	struct vchiq_mmal_port *encoder_out;
> >
> >  	dev->capture.encode_bitrate = ctrl->val;
> >
> >  	encoder_out = &dev->component[MMAL_COMPONENT_VIDEO_ENCODE]->output[0];
> >
> > -	ret = vchiq_mmal_port_parameter_set(dev->instance, encoder_out,
> > -					    mmal_ctrl->mmal_id,
> > -					    &ctrl->val, sizeof(ctrl->val));
> > -	ret = 0;
> > -	return ret;
> > +	vchiq_mmal_port_parameter_set(dev->instance, encoder_out,
> > +				      mmal_ctrl->mmal_id,
> > +				      &ctrl->val, sizeof(ctrl->val));
> > +	return 0;
> >  }
> >
> >  static int ctrl_set_bitrate_mode(struct bm2835_mmal_dev *dev,
> > @@ -862,7 +860,7 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
> >
> >  		if (scene->exposure_mode == MMAL_PARAM_EXPOSUREMODE_OFF)
> >  			shutter_speed = dev->manual_shutter_speed;
> > -
> > +
> 
> Something went wrong here.  It's fine to fi the whitespace issue, but it
> should be a separate patch.
> 
> julia
>
  So, should I send a separate patch which solves the problem as in
  PATCH 2/2 (removes the ret variable and returns directly) or is this
  an unnecessary change? I'm not sure I understand what the whitespace
  issue is. 

  Narcisa
> >  		exposure_mode = scene->exposure_mode;
> >  		metering_mode = scene->metering_mode;
> >
> > --
> > 1.9.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 post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/c49c187ff219c61323b25f01ab3f051950ae507a.1488371940.git.narcisaanamaria12%40gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
> >

Re: [Outreachy kernel] [PATCH 2/2] staging: vc04_services: bcm2835-camera: Remove unnecessary variable

[Julia Lawall]

On Wed, 1 Mar 2017, Narcisa Ana Maria Vasile wrote:

> On Wed, Mar 01, 2017 at 02:02:39PM +0100, Julia Lawall wrote:
> >
> >
> > On Wed, 1 Mar 2017, Narcisa Ana Maria Vasile wrote:
> >
> > > Instead of using the ret variable, return 0 directly
> > >
> > > Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
> > > ---
> > >  drivers/staging/vc04_services/bcm2835-camera/controls.c | 12 +++++-------
> > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > > index dcd5bec..e25f1c9 100644
> > > --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > > +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > > @@ -609,18 +609,16 @@ static int ctrl_set_bitrate(struct bm2835_mmal_dev *dev,
> > >  			    struct v4l2_ctrl *ctrl,
> > >  			    const struct bm2835_mmal_v4l2_ctrl *mmal_ctrl)
> > >  {
> > > -	int ret;
> > >  	struct vchiq_mmal_port *encoder_out;
> > >
> > >  	dev->capture.encode_bitrate = ctrl->val;
> > >
> > >  	encoder_out = &dev->component[MMAL_COMPONENT_VIDEO_ENCODE]->output[0];
> > >
> > > -	ret = vchiq_mmal_port_parameter_set(dev->instance, encoder_out,
> > > -					    mmal_ctrl->mmal_id,
> > > -					    &ctrl->val, sizeof(ctrl->val));
> > > -	ret = 0;
> > > -	return ret;
> > > +	vchiq_mmal_port_parameter_set(dev->instance, encoder_out,
> > > +				      mmal_ctrl->mmal_id,
> > > +				      &ctrl->val, sizeof(ctrl->val));
> > > +	return 0;
> > >  }
> > >
> > >  static int ctrl_set_bitrate_mode(struct bm2835_mmal_dev *dev,
> > > @@ -862,7 +860,7 @@ static int ctrl_set_scene_mode(struct bm2835_mmal_dev *dev,
> > >
> > >  		if (scene->exposure_mode == MMAL_PARAM_EXPOSUREMODE_OFF)
> > >  			shutter_speed = dev->manual_shutter_speed;
> > > -
> > > +
> >
> > Something went wrong here.  It's fine to fi the whitespace issue, but it
> > should be a separate patch.
> >
> > julia
> >
>   So, should I send a separate patch which solves the problem as in
>   PATCH 2/2 (removes the ret variable and returns directly) or is this
>   an unnecessary change? I'm not sure I understand what the whitespace
>   issue is.

What you did with ret seems fine.  But if you look above there is a - with
what looks like an empty line after it, and then a + with what looks like
an empty line after it.  Actually, the - used to have some tabs after it;
they aren't there now so I guess some mailer removed them, but I think
they were there in the message that I commented on.

Probably you removed the unnecessary whitespace at some point, then forgot
about it and modified the file in another way.

julia

>
>   Narcisa
> > >  		exposure_mode = scene->exposure_mode;
> > >  		metering_mode = scene->metering_mode;
> > >
> > > --
> > > 1.9.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 post to this group, send email to outreachy-kernel@googlegroups.com.
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/c49c187ff219c61323b25f01ab3f051950ae507a.1488371940.git.narcisaanamaria12%40gmail.com.
> > > For more options, visit https://groups.google.com/d/optout.
> > >
>
> --
> 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 post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170301155906.GA20400%40nati-X550JK.
> For more options, visit https://groups.google.com/d/optout.
>