All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hans Verkuil <hverkuil@xs4all.nl>
To: Beatriz Martins de Carvalho <martinsdecarvalhobeatriz@gmail.com>,
	laurent.pinchart@ideasonboard.com, mchehab@kernel.org,
	gregkh@linuxfoundation.org, linux-media@vger.kernel.org,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	outreachy-kernel@googlegroups.com
Subject: Re: [PATCH 1/2] staging: media: omap4iss: Ending line with argument
Date: Wed, 7 Apr 2021 15:46:12 +0200	[thread overview]
Message-ID: <e1b25826-1359-24dd-47ad-41fbd3a406b9@xs4all.nl> (raw)
In-Reply-To: <441d27060ff6477d0ad418f41e194b96373c1f7f.1617287509.git.martinsdecarvalhobeatriz@gmail.com>

Hi Beatriz,

I'm now reviewing staging/media patches instead of Greg KH. See Vaishali's
email from today: "Sending patches for the drivers/staging/media".

On 01/04/2021 17:07, Beatriz Martins de Carvalho wrote:
> Remove checkpatch check "CHECK: Lines should not end with a '('" with
> argument present in next line and reorganize characters so the lines
> are under 100 columns
> 
> Signed-off-by: Beatriz Martins de Carvalho <martinsdecarvalhobeatriz@gmail.com>
> ---
>  drivers/staging/media/omap4iss/iss.c | 46 +++++++++++++---------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index dae9073e7d3c..e8f724dbf810 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -559,9 +559,10 @@ static int iss_reset(struct iss_device *iss)
>  	iss_reg_set(iss, OMAP4_ISS_MEM_TOP, ISS_HL_SYSCONFIG,
>  		    ISS_HL_SYSCONFIG_SOFTRESET);
>  
> -	timeout = iss_poll_condition_timeout(
> -		!(iss_reg_read(iss, OMAP4_ISS_MEM_TOP, ISS_HL_SYSCONFIG) &
> -		ISS_HL_SYSCONFIG_SOFTRESET), 1000, 10, 100);
> +	timeout = iss_poll_condition_timeout(!(iss_reg_read(iss,
> +							    OMAP4_ISS_MEM_TOP, ISS_HL_SYSCONFIG)
> +							    & ISS_HL_SYSCONFIG_SOFTRESET),
> +							    1000, 10, 100);
>  	if (timeout) {
>  		dev_err(iss->dev, "ISS reset timeout\n");
>  		return -ETIMEDOUT;
> @@ -583,9 +584,10 @@ static int iss_isp_reset(struct iss_device *iss)
>  
>  	iss_reg_set(iss, OMAP4_ISS_MEM_ISP_SYS1, ISP5_CTRL, ISP5_CTRL_MSTANDBY);
>  
> -	timeout = iss_poll_condition_timeout(
> -		iss_reg_read(iss, OMAP4_ISS_MEM_ISP_SYS1, ISP5_CTRL) &
> -		ISP5_CTRL_MSTANDBY_WAIT, 1000000, 1000, 1500);
> +	timeout = iss_poll_condition_timeout(iss_reg_read(iss,
> +							  OMAP4_ISS_MEM_ISP_SYS1, ISP5_CTRL)
> +							  & ISP5_CTRL_MSTANDBY_WAIT, 1000000,
> +							  1000, 1500);
>  	if (timeout) {
>  		dev_err(iss->dev, "ISP5 standby timeout\n");
>  		return -ETIMEDOUT;
> @@ -595,9 +597,10 @@ static int iss_isp_reset(struct iss_device *iss)
>  	iss_reg_set(iss, OMAP4_ISS_MEM_ISP_SYS1, ISP5_SYSCONFIG,
>  		    ISP5_SYSCONFIG_SOFTRESET);
>  
> -	timeout = iss_poll_condition_timeout(
> -		!(iss_reg_read(iss, OMAP4_ISS_MEM_ISP_SYS1, ISP5_SYSCONFIG) &
> -		ISP5_SYSCONFIG_SOFTRESET), 1000000, 1000, 1500);
> +	timeout = iss_poll_condition_timeout(!(iss_reg_read(iss, OMAP4_ISS_MEM_ISP_SYS1,
> +							    ISP5_SYSCONFIG) &
> +							    ISP5_SYSCONFIG_SOFTRESET), 1000000,
> +							    1000, 1500);

As several other people already commented, these changes do not improve readability.
Just leave this code alone, it's good enough. Even splitting up the condition into
a separate function would degrade readability since that would make it harder to
discover the exact condition that will be polled for.

Not everything that checkpatch.pl flags is necessarily bad code :-)

>  	if (timeout) {
>  		dev_err(iss->dev, "ISP5 reset timeout\n");
>  		return -ETIMEDOUT;
> @@ -1104,33 +1107,28 @@ static int iss_create_links(struct iss_device *iss)
>  	}
>  
>  	/* Connect the submodules. */
> -	ret = media_create_pad_link(
> -			&iss->csi2a.subdev.entity, CSI2_PAD_SOURCE,
> -			&iss->ipipeif.subdev.entity, IPIPEIF_PAD_SINK, 0);
> +	ret = media_create_pad_link(&iss->csi2a.subdev.entity, CSI2_PAD_SOURCE,
> +				    &iss->ipipeif.subdev.entity, IPIPEIF_PAD_SINK, 0);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = media_create_pad_link(
> -			&iss->csi2b.subdev.entity, CSI2_PAD_SOURCE,
> -			&iss->ipipeif.subdev.entity, IPIPEIF_PAD_SINK, 0);
> +	ret = media_create_pad_link(&iss->csi2b.subdev.entity, CSI2_PAD_SOURCE,
> +				    &iss->ipipeif.subdev.entity, IPIPEIF_PAD_SINK, 0);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = media_create_pad_link(
> -			&iss->ipipeif.subdev.entity, IPIPEIF_PAD_SOURCE_VP,
> -			&iss->resizer.subdev.entity, RESIZER_PAD_SINK, 0);
> +	ret = media_create_pad_link(&iss->ipipeif.subdev.entity, IPIPEIF_PAD_SOURCE_VP,
> +				    &iss->resizer.subdev.entity, RESIZER_PAD_SINK, 0);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = media_create_pad_link(
> -			&iss->ipipeif.subdev.entity, IPIPEIF_PAD_SOURCE_VP,
> -			&iss->ipipe.subdev.entity, IPIPE_PAD_SINK, 0);
> +	ret = media_create_pad_link(&iss->ipipeif.subdev.entity, IPIPEIF_PAD_SOURCE_VP,
> +				    &iss->ipipe.subdev.entity, IPIPE_PAD_SINK, 0);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = media_create_pad_link(
> -			&iss->ipipe.subdev.entity, IPIPE_PAD_SOURCE_VP,
> -			&iss->resizer.subdev.entity, RESIZER_PAD_SINK, 0);
> +	ret = media_create_pad_link(&iss->ipipe.subdev.entity, IPIPE_PAD_SOURCE_VP,
> +				    &iss->resizer.subdev.entity, RESIZER_PAD_SINK, 0);
>  	if (ret < 0)
>  		return ret;
>  
> 

These, however, are readability improvements, so I'm happy with that.

Regards,

	Hans

  parent reply	other threads:[~2021-04-07 13:47 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-01 15:07 [PATCH 0/2] staging: media: omap4iss: Patchsets cleans up in iss.c Beatriz Martins de Carvalho
2021-04-01 15:07 ` [PATCH 1/2] staging: media: omap4iss: Ending line with argument Beatriz Martins de Carvalho
2021-04-01 15:28   ` [Outreachy kernel] " Matthew Wilcox
2021-04-07 10:00     ` Beatriz Martins de Carvalho
2021-04-07 10:15       ` Julia Lawall
2021-04-07 10:15         ` Julia Lawall
2021-04-07 13:46   ` Hans Verkuil [this message]
2021-04-07 14:19     ` Beatriz Martins de Carvalho
2021-04-01 15:07 ` [PATCH 2/2] staging: media: omap4iss: align arguments with open parenthesis Beatriz Martins de Carvalho
2021-04-01 15:29   ` [Outreachy kernel] " Matthew Wilcox
2021-04-07 15:23   ` Hans Verkuil
2021-04-01 15:25 ` [PATCH 0/2] staging: media: omap4iss: Patchsets cleans up in iss.c Greg KH

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e1b25826-1359-24dd-47ad-41fbd3a406b9@xs4all.nl \
    --to=hverkuil@xs4all.nl \
    --cc=gregkh@linuxfoundation.org \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=martinsdecarvalhobeatriz@gmail.com \
    --cc=mchehab@kernel.org \
    --cc=outreachy-kernel@googlegroups.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.