[PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Aline Santana Cordeiro]

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.
Both issues detected by checkpatch.pl.

Signed-off-by: Aline Santana Cordeiro <alinesantanacordeiro@gmail.com>
---

Changes since v1:
 - Keep the * 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..e2b36fa 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

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Matthew Wilcox]

On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote:
> -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);

No, this does not match coding style.  Probably best to break the
80-column guideline in this instance.  Best would be to have a function
and/or struct name that isn't so ridiculously long, but that would
require some in-depth thinking.

> -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);
> +

Good.

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[ascordeiro]

Em qui, 2021-04-15 às 18:14 +0100, Matthew Wilcox escreveu:
> On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro
> wrote:
> > -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);
> 
> No, this does not match coding style.  Probably best to break the
> 80-column guideline in this instance.  Best would be to have a
> function
> and/or struct name that isn't so ridiculously long, but that would
> require some in-depth thinking.
> 

I left the type of function and its name with the parameters in
different lines, following up some examples of other files, such as
atomisp_acc.c.

But I didn't pay attention and left the pointer with the function name
instead of left it with the type of the function in v1, so Hans
suggested it to a v2, as I did.

What should I do in this case?

Thank you in advance,
Aline

> > -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);
> > +
> 
> Good.
> 

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Sakari Ailus]

On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote:
> On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote:
> > -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);
> 
> No, this does not match coding style.  Probably best to break the
> 80-column guideline in this instance.  Best would be to have a function

Having the return type on the previous line is perfectly fine. There should
be a space before the asterisk though.

-- 
Sakari Ailus

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Matthew Wilcox]

On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote:
> On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote:
> > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote:
> > > -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);
> > 
> > No, this does not match coding style.  Probably best to break the
> > 80-column guideline in this instance.  Best would be to have a function
> 
> Having the return type on the previous line is perfectly fine. There should
> be a space before the asterisk though.

No, it's not.  Linus has ranted about that before.

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Matthew Wilcox]

On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote:
> On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote:
> > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote:
> > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote:
> > > > -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);
> > > 
> > > No, this does not match coding style.  Probably best to break the
> > > 80-column guideline in this instance.  Best would be to have a function
> > 
> > Having the return type on the previous line is perfectly fine. There should
> > be a space before the asterisk though.
> 
> No, it's not.  Linus has ranted about that before.

Found it.  https://lore.kernel.org/lkml/1054519757.161606@palladium.transmeta.com/

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Sakari Ailus]

On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote:
> On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote:
> > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote:
> > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote:
> > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote:
> > > > > -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);
> > > > 
> > > > No, this does not match coding style.  Probably best to break the
> > > > 80-column guideline in this instance.  Best would be to have a function
> > > 
> > > Having the return type on the previous line is perfectly fine. There should
> > > be a space before the asterisk though.
> > 
> > No, it's not.  Linus has ranted about that before.
> 
> Found it.  https://lore.kernel.org/lkml/1054519757.161606@palladium.transmeta.com/

Two decades ago, really?

This is simply one of the practical means how you split long function
declarations and avoid overly long lines. Not my favourite though, but
still better than those long lines.

My personal preference would be to wrap at the opening parenthesis and
indent by just a tab, but I know many people who disagree with that...

-- 
Sakari Ailus

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Dan Carpenter]

On Fri, Apr 16, 2021 at 12:21:58AM +0300, Sakari Ailus wrote:
> On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote:
> > On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote:
> > > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote:
> > > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote:
> > > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote:
> > > > > > -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);
> > > > > 
> > > > > No, this does not match coding style.  Probably best to break the
> > > > > 80-column guideline in this instance.  Best would be to have a function
> > > > 
> > > > Having the return type on the previous line is perfectly fine. There should
> > > > be a space before the asterisk though.
> > > 
> > > No, it's not.  Linus has ranted about that before.
> > 
> > Found it.  https://lore.kernel.org/lkml/1054519757.161606@palladium.transmeta.com/
> 
> Two decades ago, really?
> 
> This is simply one of the practical means how you split long function
> declarations and avoid overly long lines. Not my favourite though, but
> still better than those long lines.

I've always thought we allow either style, but it has to be done
consistently within the file.  I was pretty sure that was policy but
it's another thing that goes back decades so I don't have a reference.
It shouldn't be about breaking up long lines.

> 
> My personal preference would be to wrap at the opening parenthesis and
> indent by just a tab, but I know many people who disagree with that...

If you're running into the 80 character limit, then it's fine to use
two tabs.  I think we have been rejecting patches that push align the
parameters but push past the 80 character limit.  Using one tab is
confusing because it makes the decalarations line up with the code.

regards,
dan carpenter

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Sakari Ailus]

Hi Dan,

On Fri, Apr 16, 2021 at 08:49:41AM +0300, Dan Carpenter wrote:
> On Fri, Apr 16, 2021 at 12:21:58AM +0300, Sakari Ailus wrote:
> > On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote:
> > > On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote:
> > > > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote:
> > > > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote:
> > > > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote:
> > > > > > > -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);
> > > > > > 
> > > > > > No, this does not match coding style.  Probably best to break the
> > > > > > 80-column guideline in this instance.  Best would be to have a function
> > > > > 
> > > > > Having the return type on the previous line is perfectly fine. There should
> > > > > be a space before the asterisk though.
> > > > 
> > > > No, it's not.  Linus has ranted about that before.
> > > 
> > > Found it.  https://lore.kernel.org/lkml/1054519757.161606@palladium.transmeta.com/
> > 
> > Two decades ago, really?
> > 
> > This is simply one of the practical means how you split long function
> > declarations and avoid overly long lines. Not my favourite though, but
> > still better than those long lines.
> 
> I've always thought we allow either style, but it has to be done
> consistently within the file.  I was pretty sure that was policy but
> it's another thing that goes back decades so I don't have a reference.
> It shouldn't be about breaking up long lines.
> 
> > 
> > My personal preference would be to wrap at the opening parenthesis and
> > indent by just a tab, but I know many people who disagree with that...
> 
> If you're running into the 80 character limit, then it's fine to use
> two tabs.  I think we have been rejecting patches that push align the
> parameters but push past the 80 character limit.  Using one tab is
> confusing because it makes the decalarations line up with the code.

Interesting. Do you have an example of this? I've thought checkpatch.pl
gave a warning if the line ended with an opening parenthesis no matter
what.

-- 
Kind regards,

Sakari Ailus

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Julia Lawall]

> > If you're running into the 80 character limit, then it's fine to use
> > two tabs.  I think we have been rejecting patches that push align the
> > parameters but push past the 80 character limit.  Using one tab is
> > confusing because it makes the decalarations line up with the code.
>
> Interesting. Do you have an example of this? I've thought checkpatch.pl
> gave a warning if the line ended with an opening parenthesis no matter
> what.

Checkpatch is a perl script that searches for patterns that often indicate
code that is hard to understand.  It is not a precise definition of what
is allowed in the Linux kernel.  Humans have to amke compromises.

julia

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Sakari Ailus]

On Fri, Apr 16, 2021 at 10:46:54AM +0200, Julia Lawall wrote:
> > > If you're running into the 80 character limit, then it's fine to use
> > > two tabs.  I think we have been rejecting patches that push align the
> > > parameters but push past the 80 character limit.  Using one tab is
> > > confusing because it makes the decalarations line up with the code.
> >
> > Interesting. Do you have an example of this? I've thought checkpatch.pl
> > gave a warning if the line ended with an opening parenthesis no matter
> > what.
> 
> Checkpatch is a perl script that searches for patterns that often indicate
> code that is hard to understand.  It is not a precise definition of what
> is allowed in the Linux kernel.  Humans have to amke compromises.

Yeah... but I'd think if this is a preferred style then the warning could
be omitted. It might not be that difficult.

-- 
Sakari Ailus

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Julia Lawall]

On Fri, 16 Apr 2021, Sakari Ailus wrote:

> On Fri, Apr 16, 2021 at 10:46:54AM +0200, Julia Lawall wrote:
> > > > If you're running into the 80 character limit, then it's fine to use
> > > > two tabs.  I think we have been rejecting patches that push align the
> > > > parameters but push past the 80 character limit.  Using one tab is
> > > > confusing because it makes the decalarations line up with the code.
> > >
> > > Interesting. Do you have an example of this? I've thought checkpatch.pl
> > > gave a warning if the line ended with an opening parenthesis no matter
> > > what.
> >
> > Checkpatch is a perl script that searches for patterns that often indicate
> > code that is hard to understand.  It is not a precise definition of what
> > is allowed in the Linux kernel.  Humans have to amke compromises.
>
> Yeah... but I'd think if this is a preferred style then the warning could
> be omitted. It might not be that difficult.

No idea.  It involves looking at two successive lines, which may make it
more complicated.  Probably the biggest problem would be knowing whether
the line being looked at represents a function header.  Maybe that could
be detected by the fact that there is normally no space at the beginning
of the line?

julia

Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Dan Carpenter]

On Fri, Apr 16, 2021 at 11:37:28AM +0300, Sakari Ailus wrote:
> Hi Dan,
> 
> On Fri, Apr 16, 2021 at 08:49:41AM +0300, Dan Carpenter wrote:
> > On Fri, Apr 16, 2021 at 12:21:58AM +0300, Sakari Ailus wrote:
> > > On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote:
> > > > On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote:
> > > > > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote:
> > > > > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote:
> > > > > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote:
> > > > > > > > -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);
> > > > > > > 
> > > > > > > No, this does not match coding style.  Probably best to break the
> > > > > > > 80-column guideline in this instance.  Best would be to have a function
> > > > > > 
> > > > > > Having the return type on the previous line is perfectly fine. There should
> > > > > > be a space before the asterisk though.
> > > > > 
> > > > > No, it's not.  Linus has ranted about that before.
> > > > 
> > > > Found it.  https://lore.kernel.org/lkml/1054519757.161606@palladium.transmeta.com/
> > > 
> > > Two decades ago, really?
> > > 
> > > This is simply one of the practical means how you split long function
> > > declarations and avoid overly long lines. Not my favourite though, but
> > > still better than those long lines.
> > 
> > I've always thought we allow either style, but it has to be done
> > consistently within the file.  I was pretty sure that was policy but
> > it's another thing that goes back decades so I don't have a reference.
> > It shouldn't be about breaking up long lines.
> > 
> > > 
> > > My personal preference would be to wrap at the opening parenthesis and
> > > indent by just a tab, but I know many people who disagree with that...
> > 
> > If you're running into the 80 character limit, then it's fine to use
> > two tabs.  I think we have been rejecting patches that push align the
> > parameters but push past the 80 character limit.  Using one tab is
> > confusing because it makes the decalarations line up with the code.
> 
> Interesting. Do you have an example of this? I've thought checkpatch.pl
> gave a warning if the line ended with an opening parenthesis no matter
> what.

The prefered style is still aligning with the parentheses but if you
have to choose between a warning about going over the limit or a warning
about aligning then probably it's fine to not align.

I can't find an example, but I'm pretty sure we've been rejecting
patches that align the parameters but now go over the 80/100 char limit.

regards,
dan carpenter

Re: [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line

[Hans Verkuil]

On 15/04/2021 19:08, 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.
> Both issues detected by checkpatch.pl.
> 
> Signed-off-by: Aline Santana Cordeiro <alinesantanacordeiro@gmail.com>
> ---
> 
> Changes since v1:
>  - Keep the * 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..e2b36fa 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*

Keep the space before the *!

Regards,

	Hans

> +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);
>