Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
@ 2021-04-15 17:08 Aline Santana Cordeiro
  2021-04-15 17:14 ` [Outreachy kernel] " Matthew Wilcox
  2021-04-20 12:41 ` Hans Verkuil
  0 siblings, 2 replies; 14+ messages in thread
From: Aline Santana Cordeiro @ 2021-04-15 17:08 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman
  Cc: linux-media, linux-staging, linux-kernel, outreachy-kernel

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-15 17:08 [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line Aline Santana Cordeiro
@ 2021-04-15 17:14 ` Matthew Wilcox
  2021-04-15 18:09   ` ascordeiro
  2021-04-15 19:49   ` Sakari Ailus
  2021-04-20 12:41 ` Hans Verkuil
  1 sibling, 2 replies; 14+ messages in thread
From: Matthew Wilcox @ 2021-04-15 17:14 UTC (permalink / raw)
  To: Aline Santana Cordeiro
  Cc: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
	linux-media, linux-staging, linux-kernel, outreachy-kernel

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.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-15 17:14 ` [Outreachy kernel] " Matthew Wilcox
@ 2021-04-15 18:09   ` ascordeiro
  2021-04-15 19:49   ` Sakari Ailus
  1 sibling, 0 replies; 14+ messages in thread
From: ascordeiro @ 2021-04-15 18:09 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
	linux-media, linux-staging, linux-kernel, outreachy-kernel

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.
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-15 17:14 ` [Outreachy kernel] " Matthew Wilcox
  2021-04-15 18:09   ` ascordeiro
@ 2021-04-15 19:49   ` Sakari Ailus
  2021-04-15 19:57     ` Matthew Wilcox
  1 sibling, 1 reply; 14+ messages in thread
From: Sakari Ailus @ 2021-04-15 19:49 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Aline Santana Cordeiro, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel,
	outreachy-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-15 19:49   ` Sakari Ailus
@ 2021-04-15 19:57     ` Matthew Wilcox
  2021-04-15 19:59       ` Matthew Wilcox
  0 siblings, 1 reply; 14+ messages in thread
From: Matthew Wilcox @ 2021-04-15 19:57 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Aline Santana Cordeiro, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel,
	outreachy-kernel

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-15 19:57     ` Matthew Wilcox
@ 2021-04-15 19:59       ` Matthew Wilcox
  2021-04-15 21:21         ` Sakari Ailus
  0 siblings, 1 reply; 14+ messages in thread
From: Matthew Wilcox @ 2021-04-15 19:59 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Aline Santana Cordeiro, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel,
	outreachy-kernel

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/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-15 19:59       ` Matthew Wilcox
@ 2021-04-15 21:21         ` Sakari Ailus
  2021-04-16  5:49           ` Dan Carpenter
  0 siblings, 1 reply; 14+ messages in thread
From: Sakari Ailus @ 2021-04-15 21:21 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Aline Santana Cordeiro, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel,
	outreachy-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-15 21:21         ` Sakari Ailus
@ 2021-04-16  5:49           ` Dan Carpenter
  2021-04-16  8:37             ` Sakari Ailus
  0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2021-04-16  5:49 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Matthew Wilcox, Aline Santana Cordeiro, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel,
	outreachy-kernel

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-16  5:49           ` Dan Carpenter
@ 2021-04-16  8:37             ` Sakari Ailus
  2021-04-16  8:46               ` Julia Lawall
  2021-04-16  9:13               ` Dan Carpenter
  0 siblings, 2 replies; 14+ messages in thread
From: Sakari Ailus @ 2021-04-16  8:37 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Matthew Wilcox, Aline Santana Cordeiro, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel,
	outreachy-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-16  8:37             ` Sakari Ailus
@ 2021-04-16  8:46               ` Julia Lawall
  2021-04-16  8:54                 ` Sakari Ailus
  2021-04-16  9:13               ` Dan Carpenter
  1 sibling, 1 reply; 14+ messages in thread
From: Julia Lawall @ 2021-04-16  8:46 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Dan Carpenter, Matthew Wilcox, Aline Santana Cordeiro,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
	linux-staging, linux-kernel, outreachy-kernel

> > 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-16  8:46               ` Julia Lawall
@ 2021-04-16  8:54                 ` Sakari Ailus
  2021-04-16  9:03                   ` Julia Lawall
  0 siblings, 1 reply; 14+ messages in thread
From: Sakari Ailus @ 2021-04-16  8:54 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Dan Carpenter, Matthew Wilcox, Aline Santana Cordeiro,
	Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
	linux-staging, linux-kernel, outreachy-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-16  8:54                 ` Sakari Ailus
@ 2021-04-16  9:03                   ` Julia Lawall
  0 siblings, 0 replies; 14+ messages in thread
From: Julia Lawall @ 2021-04-16  9:03 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Julia Lawall, Dan Carpenter, Matthew Wilcox,
	Aline Santana Cordeiro, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel,
	outreachy-kernel



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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-16  8:37             ` Sakari Ailus
  2021-04-16  8:46               ` Julia Lawall
@ 2021-04-16  9:13               ` Dan Carpenter
  1 sibling, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2021-04-16  9:13 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Matthew Wilcox, Aline Santana Cordeiro, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel,
	outreachy-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
  2021-04-15 17:08 [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line Aline Santana Cordeiro
  2021-04-15 17:14 ` [Outreachy kernel] " Matthew Wilcox
@ 2021-04-20 12:41 ` Hans Verkuil
  1 sibling, 0 replies; 14+ messages in thread
From: Hans Verkuil @ 2021-04-20 12:41 UTC (permalink / raw)
  To: Aline Santana Cordeiro, Mauro Carvalho Chehab, Sakari Ailus,
	Greg Kroah-Hartman
  Cc: linux-media, linux-staging, linux-kernel, outreachy-kernel

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-15 17:08 [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line Aline Santana Cordeiro
2021-04-15 17:14 ` [Outreachy kernel] " Matthew Wilcox
2021-04-15 18:09   ` ascordeiro
2021-04-15 19:49   ` Sakari Ailus
2021-04-15 19:57     ` Matthew Wilcox
2021-04-15 19:59       ` Matthew Wilcox
2021-04-15 21:21         ` Sakari Ailus
2021-04-16  5:49           ` Dan Carpenter
2021-04-16  8:37             ` Sakari Ailus
2021-04-16  8:46               ` Julia Lawall
2021-04-16  8:54                 ` Sakari Ailus
2021-04-16  9:03                   ` Julia Lawall
2021-04-16  9:13               ` Dan Carpenter
2021-04-20 12:41 ` Hans Verkuil

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git