[PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference

[PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference

[Nishka Dasgupta]

Fix multiple line dereference. Issue found with checkpatch.

Signed-off-by: Nishka Dasgupta <nishka.dasgupta_ug18@ashoka.edu.in>
---
 drivers/staging/vc04_services/bcm2835-camera/controls.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c
index cff7b1e..bae7f2e 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
@@ -1194,6 +1194,8 @@ int set_framerate_params(struct bm2835_mmal_dev *dev)
 {
 	struct mmal_parameter_fps_range fps_range;
 	int ret;
+	int mcc = MMAL_COMPONENT_CAMERA;
+	int mcpp = MMAL_CAMERA_PORT_PREVIEW;
 
 	if ((dev->exposure_mode_active != MMAL_PARAM_EXPOSUREMODE_OFF) &&
 	    (dev->exp_auto_priority)) {
@@ -1220,8 +1222,7 @@ int set_framerate_params(struct bm2835_mmal_dev *dev)
 		 fps_range.fps_high.den);
 
 	ret = vchiq_mmal_port_parameter_set(dev->instance,
-					    &dev->component[MMAL_COMPONENT_CAMERA]->
-					    output[MMAL_CAMERA_PORT_PREVIEW],
+					    &dev->component[mcc]->output[mcpp],
 					    MMAL_PARAMETER_FPS_RANGE,
 					    &fps_range, sizeof(fps_range));
 	ret += vchiq_mmal_port_parameter_set(dev->instance,
-- 
2.7.4

Re: [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference

[Greg KH]

On Mon, Mar 19, 2018 at 02:07:42PM +0000, Nishka Dasgupta wrote:
> Fix multiple line dereference. Issue found with checkpatch.

What is a "multiple line dereference"?

> 
> Signed-off-by: Nishka Dasgupta <nishka.dasgupta_ug18@ashoka.edu.in>
> ---
>  drivers/staging/vc04_services/bcm2835-camera/controls.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> index cff7b1e..bae7f2e 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
> +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> @@ -1194,6 +1194,8 @@ int set_framerate_params(struct bm2835_mmal_dev *dev)
>  {
>  	struct mmal_parameter_fps_range fps_range;
>  	int ret;
> +	int mcc = MMAL_COMPONENT_CAMERA;
> +	int mcpp = MMAL_CAMERA_PORT_PREVIEW;
>  
>  	if ((dev->exposure_mode_active != MMAL_PARAM_EXPOSUREMODE_OFF) &&
>  	    (dev->exp_auto_priority)) {
> @@ -1220,8 +1222,7 @@ int set_framerate_params(struct bm2835_mmal_dev *dev)
>  		 fps_range.fps_high.den);
>  
>  	ret = vchiq_mmal_port_parameter_set(dev->instance,
> -					    &dev->component[MMAL_COMPONENT_CAMERA]->
> -					    output[MMAL_CAMERA_PORT_PREVIEW],
> +					    &dev->component[mcc]->output[mcpp],

I don't see what the benifit of this change is here, a shorter line?
It's really not worth it, don't you think?

thanks,

greg k-h

Re: [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference

[Nishka Dasgupta]

The reference (in this and other patches of the same type) is of the
form dev->component[something]->output[something else]. Because of the
line limit, this tended to be split over multiple lines even though
it's technically (as far as I can tell) a single argument. Such
splitting may have the effect of making the code harder to follow
(since we now have to mentally assemble the same value from multiple
lines of code).
By condensing the parts within the square brackets to 3 or 4
characters, I could fit the entire variable in a single line without
violating the 80 character limit.

Thanking you,
Nishka Dasgupta
Nishka Dasgupta
Undergraduate Class of 2018, Ashoka University


On Mon, Mar 19, 2018 at 7:42 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Mar 19, 2018 at 02:07:42PM +0000, Nishka Dasgupta wrote:
>> Fix multiple line dereference. Issue found with checkpatch.
>
> What is a "multiple line dereference"?
>
>>
>> Signed-off-by: Nishka Dasgupta <nishka.dasgupta_ug18@ashoka.edu.in>
>> ---
>>  drivers/staging/vc04_services/bcm2835-camera/controls.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c
>> index cff7b1e..bae7f2e 100644
>> --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
>> +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
>> @@ -1194,6 +1194,8 @@ int set_framerate_params(struct bm2835_mmal_dev *dev)
>>  {
>>       struct mmal_parameter_fps_range fps_range;
>>       int ret;
>> +     int mcc = MMAL_COMPONENT_CAMERA;
>> +     int mcpp = MMAL_CAMERA_PORT_PREVIEW;
>>
>>       if ((dev->exposure_mode_active != MMAL_PARAM_EXPOSUREMODE_OFF) &&
>>           (dev->exp_auto_priority)) {
>> @@ -1220,8 +1222,7 @@ int set_framerate_params(struct bm2835_mmal_dev *dev)
>>                fps_range.fps_high.den);
>>
>>       ret = vchiq_mmal_port_parameter_set(dev->instance,
>> -                                         &dev->component[MMAL_COMPONENT_CAMERA]->
>> -                                         output[MMAL_CAMERA_PORT_PREVIEW],
>> +                                         &dev->component[mcc]->output[mcpp],
>
> I don't see what the benifit of this change is here, a shorter line?
> It's really not worth it, don't you think?
>
> thanks,
>
> greg k-h

Re: [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference

[Greg KH]

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Mon, Mar 19, 2018 at 08:54:11PM +0530, Nishka Dasgupta wrote:
> The reference (in this and other patches of the same type) is of the
> form dev->component[something]->output[something else]. Because of the
> line limit, this tended to be split over multiple lines even though
> it's technically (as far as I can tell) a single argument. Such
> splitting may have the effect of making the code harder to follow
> (since we now have to mentally assemble the same value from multiple
> lines of code).
> By condensing the parts within the square brackets to 3 or 4
> characters, I could fit the entire variable in a single line without
> violating the 80 character limit.

Yeah, but you "waste" two new variables for this, and force the reader
to have to go backwards to try to understand what exactly the array
values are, and be confused as to why they never change.

If you don't like the statement, it can be all put on one line, and
ignore the 80 column rule here.  Or better yet, just indent things a bit
to keep it all on one line, or use a temp variable for the pointer
value.

As it is, I don't like this change, sorry.

greg k-h

Re: [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference

[Nishka Dasgupta]

On Mon, Mar 19, 2018 at 9:00 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?

Will remember not to top post in future emails.

> A: No.
> Q: Should I include quotations after my reply?
>
> http://daringfireball.net/2007/07/on_top
>
> On Mon, Mar 19, 2018 at 08:54:11PM +0530, Nishka Dasgupta wrote:
>> The reference (in this and other patches of the same type) is of the
>> form dev->component[something]->output[something else]. Because of the
>> line limit, this tended to be split over multiple lines even though
>> it's technically (as far as I can tell) a single argument. Such
>> splitting may have the effect of making the code harder to follow
>> (since we now have to mentally assemble the same value from multiple
>> lines of code).
>> By condensing the parts within the square brackets to 3 or 4
>> characters, I could fit the entire variable in a single line without
>> violating the 80 character limit.
>
> Yeah, but you "waste" two new variables for this, and force the reader
> to have to go backwards to try to understand what exactly the array
> values are, and be confused as to why they never change.
>
> If you don't like the statement, it can be all put on one line, and
> ignore the 80 column rule here.  Or better yet, just indent things a bit
> to keep it all on one line, or use a temp variable for the pointer
> value.

Is it advisable to ignore the 80-column rule for a change like this?
Because this is something that comes up in at least 10 more places in
another file in the same driver, so I was wondering whether to ignore
all of them, or break the 80-column limit for every such line. Which
should I do?

> As it is, I don't like this change, sorry.
>
> greg k-h

Thanking you,
Nishka Dasgupta

Re: [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference

[Greg KH]

On Mon, Mar 19, 2018 at 09:04:59PM +0530, Nishka Dasgupta wrote:
> On Mon, Mar 19, 2018 at 9:00 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > A: Because it messes up the order in which people normally read text.
> > Q: Why is top-posting such a bad thing?
> > A: Top-posting.
> > Q: What is the most annoying thing in e-mail?
> 
> Will remember not to top post in future emails.
> 
> > A: No.
> > Q: Should I include quotations after my reply?
> >
> > http://daringfireball.net/2007/07/on_top
> >
> > On Mon, Mar 19, 2018 at 08:54:11PM +0530, Nishka Dasgupta wrote:
> >> The reference (in this and other patches of the same type) is of the
> >> form dev->component[something]->output[something else]. Because of the
> >> line limit, this tended to be split over multiple lines even though
> >> it's technically (as far as I can tell) a single argument. Such
> >> splitting may have the effect of making the code harder to follow
> >> (since we now have to mentally assemble the same value from multiple
> >> lines of code).
> >> By condensing the parts within the square brackets to 3 or 4
> >> characters, I could fit the entire variable in a single line without
> >> violating the 80 character limit.
> >
> > Yeah, but you "waste" two new variables for this, and force the reader
> > to have to go backwards to try to understand what exactly the array
> > values are, and be confused as to why they never change.
> >
> > If you don't like the statement, it can be all put on one line, and
> > ignore the 80 column rule here.  Or better yet, just indent things a bit
> > to keep it all on one line, or use a temp variable for the pointer
> > value.
> 
> Is it advisable to ignore the 80-column rule for a change like this?
> Because this is something that comes up in at least 10 more places in
> another file in the same driver, so I was wondering whether to ignore
> all of them, or break the 80-column limit for every such line. Which
> should I do?

That is up to you, use your best judgement here.

good luck!

greg k-h