| Message ID | 20210415170819.GA17534@focaruja (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line | expand |
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.
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. >
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.
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.
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/
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...
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
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.
> > 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
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.
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
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
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); >
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);
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(-)