Re: [PATCH v2] Staging: vc04_services: Cleanup in ctrl_set_bitrate()

[Madhumitha Prabakaran]

On 04/20  :51, Greg KH wrote:
> On Sat, Apr 20, 2019 at 11:56:44AM -0500, Madhumitha Prabakaran wrote:
> > Remove unnecessary variable and replace return type.
> > In addition to that align the function parameters with parentheses to
> > maintain Linux coding style.
> > 
> > Issue suggested by Coccinelle.
> > 
> > Signed-off-by: Madhumitha Prabakaran <madhumithabiw@xxxxxxxxx>
> > 
> > ---
> > Changes in v2
> > 
> > 1) Fix my own error of removing function call from the variable.
> > 2) Align the function parameters with parentheses.
> > 
> > ---
> > ---
> >  .../staging/vc04_services/bcm2835-camera/controls.c    | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > index e39ab5fae724..71eb349c3138 100644
> > --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > @@ -607,18 +607,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;
> >  }
> 
> Why does this function even return anything at all if it can not fail?

I looked at the function vchiq_mmal_port_parameter_set, which calls
another function for setting the port parameters, which inturns sends an
synchronous message. If it is able to send the message successfully, it
returns 0. It can also fail and can return any other value. Also the
function ctrl_set_bitrate is not evaluated anywhere for true or false,
so it is safe to return the value we get from
vchiq_mmal_port_parameter_set instead of returning 0 always.


> 
> Why not return the value that vchiq_mmal_port_parameter_set() returned?
> 

I will change the code to return the value from
vchiq_mmal_port_parameter_set directly.

> thanks,
> 
> greg k-h