Re: [PATCH] staging: greybus: Alignment should match open parenthesis

Re: [PATCH] staging: greybus: Alignment should match open parenthesis

[Nandha Kumar Singaram]

greybus-dev@lists.linaro.org, linux-staging@lists.linux.dev,
linux-kernel@vger.kernel.org, kumaran.4353@gmail.com
Bcc: 
Subject: Re: [PATCH] staging: greybus: Alignment should match open parenthesis
Reply-To: 
In-Reply-To: <2023102131-slacker-lily-28ff@gregkh>

On Sat, Oct 21, 2023 at 11:48:32AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Oct 19, 2023 at 02:39:49PM -0700, Nandha Kumar Singaram wrote:
> > Adhere to linux coding style. Reported by checkpatch.pl:
> > CHECK: Alignment should match open parenthesis
> > 
> > Signed-off-by: Nandha Kumar Singaram <nandhakumar.singaram@gmail.com>
> > ---
> >  drivers/staging/greybus/camera.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c
> > index cdbb42cd413b..405c8e78aa72 100644
> > --- a/drivers/staging/greybus/camera.c
> > +++ b/drivers/staging/greybus/camera.c
> > @@ -220,7 +220,7 @@ static int gb_camera_operation_sync_flags(struct gb_connection *connection,
> >  }
> >  
> >  static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> > -		struct gb_camera_configure_streams_response *resp)
> > +				      struct gb_camera_configure_streams_response *resp)
> >  {
> >  	unsigned int max_pkt_size = 0;
> >  	unsigned int i;
> > @@ -267,8 +267,7 @@ static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> >   * Validate the stream configuration response verifying padding is correctly
> >   * set and the returned number of streams is supported
> >   */
> > -static const int gb_camera_configure_streams_validate_response(
> > -		struct gb_camera *gcam,
> > +static const int gb_camera_configure_streams_validate_response(struct gb_camera *gcam,
> 
> Why change this line?
>
did these Code change to fix CHECK: Lines should not end with a '(' reported by
checkpatch.pl

> It's fine, but then:
> 
> >  		struct gb_camera_configure_streams_response *resp,
> >  		unsigned int nstreams)
> 
> These lines are not fixed up?
> 
> thanks,
> 
> greg k-h

Fixing these lines results in another checkpatch check(CHECK: line length 
of 113 exceeds 100 columns) so didn't added any change

Thanks,
Nandha Kumar

Re: [PATCH] staging: greybus: Alignment should match open parenthesis

[Greg Kroah-Hartman]

On Sat, Oct 21, 2023 at 06:35:27AM -0700, Nandha Kumar Singaram wrote:
> greybus-dev@lists.linaro.org, linux-staging@lists.linux.dev,
> linux-kernel@vger.kernel.org, kumaran.4353@gmail.com
> Bcc: 
> Subject: Re: [PATCH] staging: greybus: Alignment should match open parenthesis
> Reply-To: 
> In-Reply-To: <2023102131-slacker-lily-28ff@gregkh>
> 
> On Sat, Oct 21, 2023 at 11:48:32AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Oct 19, 2023 at 02:39:49PM -0700, Nandha Kumar Singaram wrote:
> > > Adhere to linux coding style. Reported by checkpatch.pl:
> > > CHECK: Alignment should match open parenthesis
> > > 
> > > Signed-off-by: Nandha Kumar Singaram <nandhakumar.singaram@gmail.com>
> > > ---
> > >  drivers/staging/greybus/camera.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c
> > > index cdbb42cd413b..405c8e78aa72 100644
> > > --- a/drivers/staging/greybus/camera.c
> > > +++ b/drivers/staging/greybus/camera.c
> > > @@ -220,7 +220,7 @@ static int gb_camera_operation_sync_flags(struct gb_connection *connection,
> > >  }
> > >  
> > >  static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> > > -		struct gb_camera_configure_streams_response *resp)
> > > +				      struct gb_camera_configure_streams_response *resp)
> > >  {
> > >  	unsigned int max_pkt_size = 0;
> > >  	unsigned int i;
> > > @@ -267,8 +267,7 @@ static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> > >   * Validate the stream configuration response verifying padding is correctly
> > >   * set and the returned number of streams is supported
> > >   */
> > > -static const int gb_camera_configure_streams_validate_response(
> > > -		struct gb_camera *gcam,
> > > +static const int gb_camera_configure_streams_validate_response(struct gb_camera *gcam,
> > 
> > Why change this line?
> >
> did these Code change to fix CHECK: Lines should not end with a '(' reported by
> checkpatch.pl

That's great, but you didn't say that in your changelog text, so it
looks wrong here :(

thanks,

greg k-h

Re: [PATCH] staging: greybus: Alignment should match open parenthesis

[Nandha Kumar Singaram]

On Sat, Oct 21, 2023 at 04:45:46PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Oct 21, 2023 at 06:35:27AM -0700, Nandha Kumar Singaram wrote:
> > greybus-dev@lists.linaro.org, linux-staging@lists.linux.dev,
> > linux-kernel@vger.kernel.org, kumaran.4353@gmail.com
> > Bcc: 
> > Subject: Re: [PATCH] staging: greybus: Alignment should match open parenthesis
> > Reply-To: 
> > In-Reply-To: <2023102131-slacker-lily-28ff@gregkh>
> > 
> > On Sat, Oct 21, 2023 at 11:48:32AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Oct 19, 2023 at 02:39:49PM -0700, Nandha Kumar Singaram wrote:
> > > > Adhere to linux coding style. Reported by checkpatch.pl:
> > > > CHECK: Alignment should match open parenthesis
> > > > 
> > > > Signed-off-by: Nandha Kumar Singaram <nandhakumar.singaram@gmail.com>
> > > > ---
> > > >  drivers/staging/greybus/camera.c | 14 +++++++-------
> > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c
> > > > index cdbb42cd413b..405c8e78aa72 100644
> > > > --- a/drivers/staging/greybus/camera.c
> > > > +++ b/drivers/staging/greybus/camera.c
> > > > @@ -220,7 +220,7 @@ static int gb_camera_operation_sync_flags(struct gb_connection *connection,
> > > >  }
> > > >  
> > > >  static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> > > > -		struct gb_camera_configure_streams_response *resp)
> > > > +				      struct gb_camera_configure_streams_response *resp)
> > > >  {
> > > >  	unsigned int max_pkt_size = 0;
> > > >  	unsigned int i;
> > > > @@ -267,8 +267,7 @@ static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> > > >   * Validate the stream configuration response verifying padding is correctly
> > > >   * set and the returned number of streams is supported
> > > >   */
> > > > -static const int gb_camera_configure_streams_validate_response(
> > > > -		struct gb_camera *gcam,
> > > > +static const int gb_camera_configure_streams_validate_response(struct gb_camera *gcam,
> > > 
> > > Why change this line?
> > >
> > did these Code change to fix CHECK: Lines should not end with a '(' reported by
> > checkpatch.pl
> 
> That's great, but you didn't say that in your changelog text, so it
> looks wrong here :(
> 
> thanks,
> 
> greg k-h

Sorry for missing that, will add and send the updated patch.

Regards,
Nandha Kumar

Re: [PATCH] staging: greybus: Alignment should match open parenthesis

[Greg Kroah-Hartman]

On Sat, Oct 21, 2023 at 11:06:34AM -0700, Nandha Kumar Singaram wrote:
> On Sat, Oct 21, 2023 at 04:45:46PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Oct 21, 2023 at 06:35:27AM -0700, Nandha Kumar Singaram wrote:
> > > greybus-dev@lists.linaro.org, linux-staging@lists.linux.dev,
> > > linux-kernel@vger.kernel.org, kumaran.4353@gmail.com
> > > Bcc: 
> > > Subject: Re: [PATCH] staging: greybus: Alignment should match open parenthesis
> > > Reply-To: 
> > > In-Reply-To: <2023102131-slacker-lily-28ff@gregkh>
> > > 
> > > On Sat, Oct 21, 2023 at 11:48:32AM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Oct 19, 2023 at 02:39:49PM -0700, Nandha Kumar Singaram wrote:
> > > > > Adhere to linux coding style. Reported by checkpatch.pl:
> > > > > CHECK: Alignment should match open parenthesis
> > > > > 
> > > > > Signed-off-by: Nandha Kumar Singaram <nandhakumar.singaram@gmail.com>
> > > > > ---
> > > > >  drivers/staging/greybus/camera.c | 14 +++++++-------
> > > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c
> > > > > index cdbb42cd413b..405c8e78aa72 100644
> > > > > --- a/drivers/staging/greybus/camera.c
> > > > > +++ b/drivers/staging/greybus/camera.c
> > > > > @@ -220,7 +220,7 @@ static int gb_camera_operation_sync_flags(struct gb_connection *connection,
> > > > >  }
> > > > >  
> > > > >  static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> > > > > -		struct gb_camera_configure_streams_response *resp)
> > > > > +				      struct gb_camera_configure_streams_response *resp)
> > > > >  {
> > > > >  	unsigned int max_pkt_size = 0;
> > > > >  	unsigned int i;
> > > > > @@ -267,8 +267,7 @@ static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> > > > >   * Validate the stream configuration response verifying padding is correctly
> > > > >   * set and the returned number of streams is supported
> > > > >   */
> > > > > -static const int gb_camera_configure_streams_validate_response(
> > > > > -		struct gb_camera *gcam,
> > > > > +static const int gb_camera_configure_streams_validate_response(struct gb_camera *gcam,
> > > > 
> > > > Why change this line?
> > > >
> > > did these Code change to fix CHECK: Lines should not end with a '(' reported by
> > > checkpatch.pl
> > 
> > That's great, but you didn't say that in your changelog text, so it
> > looks wrong here :(
> > 
> > thanks,
> > 
> > greg k-h
> 
> Sorry for missing that, will add and send the updated patcha

Careful, remember to only do one logical thing per patch.

good luck!

greg k-h

[PATCH] staging: greybus: Alignment should match open parenthesis

[Nandha Kumar Singaram]

Adhere to linux coding style. Reported by checkpatch.pl:
CHECK: Alignment should match open parenthesis

Signed-off-by: Nandha Kumar Singaram <nandhakumar.singaram@gmail.com>
---
 drivers/staging/greybus/camera.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c
index cdbb42cd413b..405c8e78aa72 100644
--- a/drivers/staging/greybus/camera.c
+++ b/drivers/staging/greybus/camera.c
@@ -220,7 +220,7 @@ static int gb_camera_operation_sync_flags(struct gb_connection *connection,
 }
 
 static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
-		struct gb_camera_configure_streams_response *resp)
+				      struct gb_camera_configure_streams_response *resp)
 {
 	unsigned int max_pkt_size = 0;
 	unsigned int i;
@@ -267,8 +267,7 @@ static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
  * Validate the stream configuration response verifying padding is correctly
  * set and the returned number of streams is supported
  */
-static const int gb_camera_configure_streams_validate_response(
-		struct gb_camera *gcam,
+static const int gb_camera_configure_streams_validate_response(struct gb_camera *gcam,
 		struct gb_camera_configure_streams_response *resp,
 		unsigned int nstreams)
 {
@@ -378,8 +377,8 @@ struct ap_csi_config_request {
 #define GB_CAMERA_CSI_CLK_FREQ_MARGIN		150000000U
 
 static int gb_camera_setup_data_connection(struct gb_camera *gcam,
-		struct gb_camera_configure_streams_response *resp,
-		struct gb_camera_csi_params *csi_params)
+					   struct gb_camera_configure_streams_response *resp,
+					   struct gb_camera_csi_params *csi_params)
 {
 	struct ap_csi_config_request csi_cfg;
 	struct gb_connection *conn;
@@ -783,8 +782,9 @@ static ssize_t gb_camera_op_capabilities(void *priv, char *data, size_t len)
 }
 
 static int gb_camera_op_configure_streams(void *priv, unsigned int *nstreams,
-		unsigned int *flags, struct gb_camera_stream *streams,
-		struct gb_camera_csi_params *csi_params)
+					  unsigned int *flags,
+					  struct gb_camera_stream *streams,
+					  struct gb_camera_csi_params *csi_params)
 {
 	struct gb_camera *gcam = priv;
 	struct gb_camera_stream_config *gb_streams;
-- 
2.25.1

Re: [PATCH] staging: greybus: Alignment should match open parenthesis

[Alex Elder]

On 10/19/23 4:39 PM, Nandha Kumar Singaram wrote:
> Adhere to linux coding style. Reported by checkpatch.pl:
> CHECK: Alignment should match open parenthesis

Sometimes checkpatch.pl warns about things that are not
really that important.  One class of this type of issue
is white space errors.

Yes, consistency is good, but the kernel simply doesn't
have universally consistent conventions, and I doubt
it ever will.  There might be times where a source file
consistently follows a white space convention that
differs from what checkpatch wants.  Suggesting a
wholesale change to that file to "fix" that typically
wouldn't be welcome.

Unfortunately without some experience it's hard to know
which checkpatch warnings can be safely ignored.  I would
place white space warnings at a lower priority for fixing
than some others.  For example, this is also a pretty
trivial warning:
   CHECK: Macro argument 'gcam' may be better as '(gcam)' to avoid 
precedence issues
And it is most likely not a problem in this case, but fixing
this type of warning is probably more constructive than
just adjusting white space.

I have no objection to your patch, and it's a fine way to
get some experience with the patch process, but I don't
think this particular change is necessary.

					-Alex

> Signed-off-by: Nandha Kumar Singaram <nandhakumar.singaram@gmail.com>
> ---
>   drivers/staging/greybus/camera.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c
> index cdbb42cd413b..405c8e78aa72 100644
> --- a/drivers/staging/greybus/camera.c
> +++ b/drivers/staging/greybus/camera.c
> @@ -220,7 +220,7 @@ static int gb_camera_operation_sync_flags(struct gb_connection *connection,
>   }
>   
>   static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> -		struct gb_camera_configure_streams_response *resp)
> +				      struct gb_camera_configure_streams_response *resp)
>   {
>   	unsigned int max_pkt_size = 0;
>   	unsigned int i;
> @@ -267,8 +267,7 @@ static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
>    * Validate the stream configuration response verifying padding is correctly
>    * set and the returned number of streams is supported
>    */
> -static const int gb_camera_configure_streams_validate_response(
> -		struct gb_camera *gcam,
> +static const int gb_camera_configure_streams_validate_response(struct gb_camera *gcam,
>   		struct gb_camera_configure_streams_response *resp,
>   		unsigned int nstreams)
>   {
> @@ -378,8 +377,8 @@ struct ap_csi_config_request {
>   #define GB_CAMERA_CSI_CLK_FREQ_MARGIN		150000000U
>   
>   static int gb_camera_setup_data_connection(struct gb_camera *gcam,
> -		struct gb_camera_configure_streams_response *resp,
> -		struct gb_camera_csi_params *csi_params)
> +					   struct gb_camera_configure_streams_response *resp,
> +					   struct gb_camera_csi_params *csi_params)
>   {
>   	struct ap_csi_config_request csi_cfg;
>   	struct gb_connection *conn;
> @@ -783,8 +782,9 @@ static ssize_t gb_camera_op_capabilities(void *priv, char *data, size_t len)
>   }
>   
>   static int gb_camera_op_configure_streams(void *priv, unsigned int *nstreams,
> -		unsigned int *flags, struct gb_camera_stream *streams,
> -		struct gb_camera_csi_params *csi_params)
> +					  unsigned int *flags,
> +					  struct gb_camera_stream *streams,
> +					  struct gb_camera_csi_params *csi_params)
>   {
>   	struct gb_camera *gcam = priv;
>   	struct gb_camera_stream_config *gb_streams;

Re: [PATCH] staging: greybus: Alignment should match open parenthesis

[Nandha Kumar Singaram]

On Thu, Oct 19, 2023 at 07:15:37PM -0500, Alex Elder wrote:
> On 10/19/23 4:39 PM, Nandha Kumar Singaram wrote:
> > Adhere to linux coding style. Reported by checkpatch.pl:
> > CHECK: Alignment should match open parenthesis
> 
> Sometimes checkpatch.pl warns about things that are not
> really that important.  One class of this type of issue
> is white space errors.
> 
> Yes, consistency is good, but the kernel simply doesn't
> have universally consistent conventions, and I doubt
> it ever will.  There might be times where a source file
> consistently follows a white space convention that
> differs from what checkpatch wants.  Suggesting a
> wholesale change to that file to "fix" that typically
> wouldn't be welcome.
> 
> Unfortunately without some experience it's hard to know
> which checkpatch warnings can be safely ignored.  I would
> place white space warnings at a lower priority for fixing
> than some others.  For example, this is also a pretty
> trivial warning:
>   CHECK: Macro argument 'gcam' may be better as '(gcam)' to avoid precedence
> issues
> And it is most likely not a problem in this case, but fixing
> this type of warning is probably more constructive than
> just adjusting white space.
> 
> I have no objection to your patch, and it's a fine way to
> get some experience with the patch process, but I don't
> think this particular change is necessary.
> 
> 					-Alex
> 
> > Signed-off-by: Nandha Kumar Singaram <nandhakumar.singaram@gmail.com>
> > ---
> >   drivers/staging/greybus/camera.c | 14 +++++++-------
> >   1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c
> > index cdbb42cd413b..405c8e78aa72 100644
> > --- a/drivers/staging/greybus/camera.c
> > +++ b/drivers/staging/greybus/camera.c
> > @@ -220,7 +220,7 @@ static int gb_camera_operation_sync_flags(struct gb_connection *connection,
> >   }
> >   static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> > -		struct gb_camera_configure_streams_response *resp)
> > +				      struct gb_camera_configure_streams_response *resp)
> >   {
> >   	unsigned int max_pkt_size = 0;
> >   	unsigned int i;
> > @@ -267,8 +267,7 @@ static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> >    * Validate the stream configuration response verifying padding is correctly
> >    * set and the returned number of streams is supported
> >    */
> > -static const int gb_camera_configure_streams_validate_response(
> > -		struct gb_camera *gcam,
> > +static const int gb_camera_configure_streams_validate_response(struct gb_camera *gcam,
> >   		struct gb_camera_configure_streams_response *resp,
> >   		unsigned int nstreams)
> >   {
> > @@ -378,8 +377,8 @@ struct ap_csi_config_request {
> >   #define GB_CAMERA_CSI_CLK_FREQ_MARGIN		150000000U
> >   static int gb_camera_setup_data_connection(struct gb_camera *gcam,
> > -		struct gb_camera_configure_streams_response *resp,
> > -		struct gb_camera_csi_params *csi_params)
> > +					   struct gb_camera_configure_streams_response *resp,
> > +					   struct gb_camera_csi_params *csi_params)
> >   {
> >   	struct ap_csi_config_request csi_cfg;
> >   	struct gb_connection *conn;
> > @@ -783,8 +782,9 @@ static ssize_t gb_camera_op_capabilities(void *priv, char *data, size_t len)
> >   }
> >   static int gb_camera_op_configure_streams(void *priv, unsigned int *nstreams,
> > -		unsigned int *flags, struct gb_camera_stream *streams,
> > -		struct gb_camera_csi_params *csi_params)
> > +					  unsigned int *flags,
> > +					  struct gb_camera_stream *streams,
> > +					  struct gb_camera_csi_params *csi_params)
> >   {
> >   	struct gb_camera *gcam = priv;
> >   	struct gb_camera_stream_config *gb_streams;
> 

Thanks Alex for the review and feedback.

Regards,
Nandha Kumar

Re: [PATCH] staging: greybus: Alignment should match open parenthesis

[Greg Kroah-Hartman]

On Thu, Oct 19, 2023 at 02:39:49PM -0700, Nandha Kumar Singaram wrote:
> Adhere to linux coding style. Reported by checkpatch.pl:
> CHECK: Alignment should match open parenthesis
> 
> Signed-off-by: Nandha Kumar Singaram <nandhakumar.singaram@gmail.com>
> ---
>  drivers/staging/greybus/camera.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c
> index cdbb42cd413b..405c8e78aa72 100644
> --- a/drivers/staging/greybus/camera.c
> +++ b/drivers/staging/greybus/camera.c
> @@ -220,7 +220,7 @@ static int gb_camera_operation_sync_flags(struct gb_connection *connection,
>  }
>  
>  static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
> -		struct gb_camera_configure_streams_response *resp)
> +				      struct gb_camera_configure_streams_response *resp)
>  {
>  	unsigned int max_pkt_size = 0;
>  	unsigned int i;
> @@ -267,8 +267,7 @@ static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
>   * Validate the stream configuration response verifying padding is correctly
>   * set and the returned number of streams is supported
>   */
> -static const int gb_camera_configure_streams_validate_response(
> -		struct gb_camera *gcam,
> +static const int gb_camera_configure_streams_validate_response(struct gb_camera *gcam,

Why change this line?

It's fine, but then:

>  		struct gb_camera_configure_streams_response *resp,
>  		unsigned int nstreams)

These lines are not fixed up?

thanks,

greg k-h