* [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference
@ 2018-03-19 14:07 Nishka Dasgupta
2018-03-19 14:12 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Nishka Dasgupta @ 2018-03-19 14:07 UTC (permalink / raw)
To: gregkh, outreachy-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference
2018-03-19 14:07 [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference Nishka Dasgupta
@ 2018-03-19 14:12 ` Greg KH
2018-03-19 15:24 ` Nishka Dasgupta
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2018-03-19 14:12 UTC (permalink / raw)
To: Nishka Dasgupta; +Cc: outreachy-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference
2018-03-19 14:12 ` Greg KH
@ 2018-03-19 15:24 ` Nishka Dasgupta
2018-03-19 15:30 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Nishka Dasgupta @ 2018-03-19 15:24 UTC (permalink / raw)
To: Greg KH; +Cc: outreachy-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference
2018-03-19 15:24 ` Nishka Dasgupta
@ 2018-03-19 15:30 ` Greg KH
2018-03-19 15:34 ` Nishka Dasgupta
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2018-03-19 15:30 UTC (permalink / raw)
To: Nishka Dasgupta; +Cc: outreachy-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference
2018-03-19 15:30 ` Greg KH
@ 2018-03-19 15:34 ` Nishka Dasgupta
2018-03-19 15:47 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Nishka Dasgupta @ 2018-03-19 15:34 UTC (permalink / raw)
To: Greg KH; +Cc: outreachy-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference
2018-03-19 15:34 ` Nishka Dasgupta
@ 2018-03-19 15:47 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2018-03-19 15:47 UTC (permalink / raw)
To: Nishka Dasgupta; +Cc: outreachy-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-19 15:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-19 14:07 [PATCH] staging:vc04_services:bcm2835-camera:controls.c: Fix multiple line dereference Nishka Dasgupta
2018-03-19 14:12 ` Greg KH
2018-03-19 15:24 ` Nishka Dasgupta
2018-03-19 15:30 ` Greg KH
2018-03-19 15:34 ` Nishka Dasgupta
2018-03-19 15:47 ` Greg KH
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.