Re: [Outreachy kernel] [PATCH v3] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
From: Aline Santana Cordeiro
Date: Fri Apr 23 2021 - 22:11:42 EST
Em sex, 2021-04-23 às 11:21 +0200, Hans Verkuil escreveu:
> On 21/04/2021 16:21, Aline Santana Cordeiro wrote:
> > Em qua, 2021-04-21 às 15:56 +0200, Julia Lawall escreveu:
> > >
> > >
> > > On Wed, 21 Apr 2021, Aline Santana Cordeiro wrote:
> > >
> > > > Em qua, 2021-04-21 às 15:08 +0200, Julia Lawall escreveu:
> > > > >
> > > > >
> > > > > On Wed, 21 Apr 2021, Aline Santana Cordeiro wrote:
> > > > >
> > > > > > Change line break to avoid an open parenthesis at the end
> > > > > > of
> > > > > > the
> > > > > > line.
> > > > > > It consequently removed spaces at the start of the
> > > > > > subsequent
> > > > > > line.
> > > > >
> > > > > The message is hard to understand. There are a lot of
> > > > > singular
> > > > > nouns, but
> > > > > actually there are two changes. Which change is being
> > > > > described
> > > > > by
> > > > > the
> > > > > above message? What does "It" refer to?
> > > > >
> > > > > julia
> > > >
> > > > Checkpatch indicated two problems with this function
> > > > declaration:
> > > > 1) The line ending with an open parenthesis, and
> > > > 2) The following line - with the function parameters - has
> > > > spaces
> > > > in
> > > > its identation.
> > > >
> > > > When I changed the line break to put the function name and its
> > > > parameter in the following line, both checkpath checks were
> > > > eliminated.
> > > >
> > > > So, the main change was the line break and, also, the line
> > > > break
> > > > (it)
> > > > removed the space in the following line.
> > > >
> > > > Is it better to change the message and explain only about the
> > > > line
> > > > break?
> > >
> > > The message should explain about the whole patch. So if you
> > > change
> > > two
> > > things, it should be clear that what you are saying covers both
> > > of
> > > them.
> >
> > Ok, I can do that. In the commit message I described just one issue
> > because it is only one patch, I didn't want it to look like I was
> > changing different issues in just one patch.
> >
> > >
> > > But it seems that Matthew doesn't think that the line break is a
> > > good
> > > idea
> > > anyway.
> >
> > Yes, I'm sending this email to Matthew too, because I don't know
> > exactly how to proceed as Hans asked me to made some corrections
> > too.
> > I've made these changes because checkpatch has indicated and with
> > this
> > line break, checkpatch does not indicate any check or warning
> > anymore.
> > But I can undo that too, I just don't know what I'm supposed to do
> > with
> > so many opposite opinions.
>
> As one of the media maintainers I can say that in this case the
> preference
> would be to split it up in two lines. It's one of those areas where
> different maintainers have different opinions.
>
> Just keep in mind that this is all nitpicking and normally we
> probably
> wouldn't bother with this at all, but it is a good exercise to learn
> about patches and contributing :-)
>
> Regards,
>
> Hans
I really appreciate all the feedbacks I've received :)
Indeed we can learn a lot about all the contributing process.
Thank you,
Aline
>
> >
> >
> > Thank you all,
> > Aline
> > >
> > > julia
> > >
> > > >
> > > > Thank you,
> > > > Aline
> > > > >
> > > > >
> > > > > > Both issues detected by checkpatch.pl.
> > > > > >
> > > > > > Signed-off-by: Aline Santana Cordeiro <
> > > > > > alinesantanacordeiro@xxxxxxxxx>
> > > > > > ---
> > > > > >
> > > > > > Changes since v2:
> > > > > > - Insert a space between the function type and pointer
> > > > > >
> > > > > > Changes since v1:
> > > > > > - Keep the pointer with the function return type
> > > > > > instead of left it with the function name
> > > > > >
> > > > > > drivers/staging/media/atomisp/pci/atomisp_cmd.h | 10
> > > > > > +++++----
> > > > > > -
> > > > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git
> > > > > > a/drivers/staging/media/atomisp/pci/atomisp_cmd.h
> > > > > > b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
> > > > > > index 1c0d464..639eca3 100644
> > > > > > --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.h
> > > > > > +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
> > > > > > @@ -75,8 +75,8 @@ void atomisp_wdt(struct timer_list *t);
> > > > > > void atomisp_setup_flash(struct atomisp_sub_device *asd);
> > > > > > irqreturn_t atomisp_isr(int irq, void *dev);
> > > > > > irqreturn_t atomisp_isr_thread(int irq, void *isp_ptr);
> > > > > > -const struct atomisp_format_bridge
> > > > > > *get_atomisp_format_bridge_from_mbus(
> > > > > > - u32 mbus_code);
> > > > > > +const struct atomisp_format_bridge *
> > > > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code);
> > > > > > bool atomisp_is_mbuscode_raw(uint32_t code);
> > > > > > int atomisp_get_frame_pgnr(struct atomisp_device *isp,
> > > > > > const struct ia_css_frame
> > > > > > *frame,
> > > > > > u32
> > > > > > *p_pgnr);
> > > > > > @@ -381,9 +381,9 @@ enum mipi_port_id
> > > > > > __get_mipi_port(struct
> > > > > > atomisp_device *isp,
> > > > > >
> > > > > > bool atomisp_is_vf_pipe(struct atomisp_video_pipe *pipe);
> > > > > >
> > > > > > -void atomisp_apply_css_parameters(
> > > > > > - struct atomisp_sub_device *asd,
> > > > > > - struct atomisp_css_params *css_param);
> > > > > > +void atomisp_apply_css_parameters(struct
> > > > > > atomisp_sub_device
> > > > > > *asd,
> > > > > > + struct atomisp_css_params
> > > > > > *css_param);
> > > > > > +
> > > > > > void atomisp_free_css_parameters(struct atomisp_css_params
> > > > > > *css_param);
> > > > > >
> > > > > > void atomisp_handle_parameter_and_buffer(struct
> > > > > > atomisp_video_pipe
> > > > > > *pipe);
> > > > > > --
> > > > > > 2.7.4
> > > > > >
> > > > > > --
> > > > > > 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@xxxxxxxxxxxxxxxx.
> > > > > > To view this discussion on the web visit
> > > > > > https://groups.google.com/d/msgid/outreachy-kernel/20210421123718.GA4597%40focaruja
> > > > > > .
> > > > > >
> > > >
> > > >
> > > > --
> > > > 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@xxxxxxxxxxxxxxxx.
> > > > To view this discussion on the web visit
> > > > https://groups.google.com/d/msgid/outreachy-kernel/7aeac7041a6f6d7b3d8563f0d0bf0a4d31f379b0.camel%40gmail.com
> > > > .
> >
> >
>