* [PATCH v3] staging: media: omap4iss: Fix lines ending with open parenthesis
@ 2019-10-14 5:53 Jamal Shareef
2019-10-14 6:10 ` [Outreachy kernel] " Julia Lawall
0 siblings, 1 reply; 4+ messages in thread
From: Jamal Shareef @ 2019-10-14 5:53 UTC (permalink / raw)
To: boqun.feng, andrea.parri
Cc: jamal.k.shareef, laurent.pinchart, mchehab, gregkh, outreachy-kernel
Fix lines ending with open parenthesis. Issue
found by checkpatch.
Signed-off-by: Jamal Shareef <jamal.k.shareef@gmail.com>
---
Changes in v3:
- Remove created local variables and revert
back to original function calls because intent
is to dynamically poll the value.
- Format code for better readability. This
introduces new warning for "alignment should
match open parenthesis" but will be disregarded.
Changes in v2:
- Include similar changes in iss.c file.
- Create local variables for iss_reg_read
function calls for better code formatting.
---
drivers/staging/media/omap4iss/iss.c | 58 +++++++++++++----------
drivers/staging/media/omap4iss/iss_csi2.c | 12 ++---
2 files changed, 39 insertions(+), 31 deletions(-)
diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index 1a966cb2f3a6..9112eb89b7c0 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,9 @@ 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 +596,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);
if (timeout) {
dev_err(iss->dev, "ISP5 reset timeout\n");
return -ETIMEDOUT;
@@ -964,7 +966,8 @@ iss_register_subdev_group(struct iss_device *iss,
}
subdev = v4l2_i2c_new_subdev_board(&iss->v4l2_dev, adapter,
- board_info->board_info, NULL);
+ board_info->board_info,
+ NULL);
if (!subdev) {
dev_err(iss->dev, "Unable to register subdev %s\n",
board_info->board_info->type);
@@ -1108,33 +1111,38 @@ 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;
diff --git a/drivers/staging/media/omap4iss/iss_csi2.c b/drivers/staging/media/omap4iss/iss_csi2.c
index a6dc2d2b1228..cd705df04f6c 100644
--- a/drivers/staging/media/omap4iss/iss_csi2.c
+++ b/drivers/staging/media/omap4iss/iss_csi2.c
@@ -495,9 +495,9 @@ int omap4iss_csi2_reset(struct iss_csi2_device *csi2)
iss_reg_set(csi2->iss, csi2->regs1, CSI2_SYSCONFIG,
CSI2_SYSCONFIG_SOFT_RESET);
- timeout = iss_poll_condition_timeout(
- iss_reg_read(csi2->iss, csi2->regs1, CSI2_SYSSTATUS) &
- CSI2_SYSSTATUS_RESET_DONE, 500, 100, 200);
+ timeout = iss_poll_condition_timeout(iss_reg_read(csi2->iss,
+ csi2->regs1, CSI2_SYSSTATUS) &
+ CSI2_SYSSTATUS_RESET_DONE, 500, 100, 200);
if (timeout) {
dev_err(csi2->iss->dev, "CSI2: Soft reset timeout!\n");
return -EBUSY;
@@ -506,9 +506,9 @@ int omap4iss_csi2_reset(struct iss_csi2_device *csi2)
iss_reg_set(csi2->iss, csi2->regs1, CSI2_COMPLEXIO_CFG,
CSI2_COMPLEXIO_CFG_RESET_CTRL);
- timeout = iss_poll_condition_timeout(
- iss_reg_read(csi2->iss, csi2->phy->phy_regs, REGISTER1) &
- REGISTER1_RESET_DONE_CTRLCLK, 10000, 100, 500);
+ timeout = iss_poll_condition_timeout(iss_reg_read(csi2->iss,
+ csi2->phy->phy_regs, REGISTER1) &
+ REGISTER1_RESET_DONE_CTRLCLK, 10000, 100, 500);
if (timeout) {
dev_err(csi2->iss->dev, "CSI2: CSI2_96M_FCLK reset timeout!\n");
return -EBUSY;
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH v3] staging: media: omap4iss: Fix lines ending with open parenthesis
2019-10-14 5:53 [PATCH v3] staging: media: omap4iss: Fix lines ending with open parenthesis Jamal Shareef
@ 2019-10-14 6:10 ` Julia Lawall
2019-10-14 22:21 ` Laurent Pinchart
0 siblings, 1 reply; 4+ messages in thread
From: Julia Lawall @ 2019-10-14 6:10 UTC (permalink / raw)
To: Jamal Shareef
Cc: boqun.feng, andrea.parri, laurent.pinchart, mchehab, gregkh,
outreachy-kernel, Joe Perches
On Sun, 13 Oct 2019, Jamal Shareef wrote:
> Fix lines ending with open parenthesis. Issue
> found by checkpatch.
>
> Signed-off-by: Jamal Shareef <jamal.k.shareef@gmail.com>
> ---
> Changes in v3:
> - Remove created local variables and revert
> back to original function calls because intent
> is to dynamically poll the value.
> - Format code for better readability. This
> introduces new warning for "alignment should
> match open parenthesis" but will be disregarded.
>
> Changes in v2:
> - Include similar changes in iss.c file.
> - Create local variables for iss_reg_read
> function calls for better code formatting.
> ---
> drivers/staging/media/omap4iss/iss.c | 58 +++++++++++++----------
> drivers/staging/media/omap4iss/iss_csi2.c | 12 ++---
> 2 files changed, 39 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index 1a966cb2f3a6..9112eb89b7c0 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);
There is indeed something unpleasant about something that looks like a
function call, but it actually a while loop. Perhaps the word "poll" is
supposed to be sufficiently suggestive of loopness.
In the proposed code, it is very hard to see what argument goes with what.
Perhaps an improvement for the original code would be to leave the first
argument on a new line as it is, but move the ISS_HL_SYSCONFIG_SOFTRESET
over 2 spaces so that it is lined up with what it is anded with, and then
put the 1000 etc on a line below, lines up with the !, so that it is clear
that they are not part of the polling condition.
julia
> if (timeout) {
> dev_err(iss->dev, "ISS reset timeout\n");
> return -ETIMEDOUT;
> @@ -583,9 +584,9 @@ 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 +596,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);
> if (timeout) {
> dev_err(iss->dev, "ISP5 reset timeout\n");
> return -ETIMEDOUT;
> @@ -964,7 +966,8 @@ iss_register_subdev_group(struct iss_device *iss,
> }
>
> subdev = v4l2_i2c_new_subdev_board(&iss->v4l2_dev, adapter,
> - board_info->board_info, NULL);
> + board_info->board_info,
> + NULL);
> if (!subdev) {
> dev_err(iss->dev, "Unable to register subdev %s\n",
> board_info->board_info->type);
> @@ -1108,33 +1111,38 @@ 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;
>
> diff --git a/drivers/staging/media/omap4iss/iss_csi2.c b/drivers/staging/media/omap4iss/iss_csi2.c
> index a6dc2d2b1228..cd705df04f6c 100644
> --- a/drivers/staging/media/omap4iss/iss_csi2.c
> +++ b/drivers/staging/media/omap4iss/iss_csi2.c
> @@ -495,9 +495,9 @@ int omap4iss_csi2_reset(struct iss_csi2_device *csi2)
> iss_reg_set(csi2->iss, csi2->regs1, CSI2_SYSCONFIG,
> CSI2_SYSCONFIG_SOFT_RESET);
>
> - timeout = iss_poll_condition_timeout(
> - iss_reg_read(csi2->iss, csi2->regs1, CSI2_SYSSTATUS) &
> - CSI2_SYSSTATUS_RESET_DONE, 500, 100, 200);
> + timeout = iss_poll_condition_timeout(iss_reg_read(csi2->iss,
> + csi2->regs1, CSI2_SYSSTATUS) &
> + CSI2_SYSSTATUS_RESET_DONE, 500, 100, 200);
> if (timeout) {
> dev_err(csi2->iss->dev, "CSI2: Soft reset timeout!\n");
> return -EBUSY;
> @@ -506,9 +506,9 @@ int omap4iss_csi2_reset(struct iss_csi2_device *csi2)
> iss_reg_set(csi2->iss, csi2->regs1, CSI2_COMPLEXIO_CFG,
> CSI2_COMPLEXIO_CFG_RESET_CTRL);
>
> - timeout = iss_poll_condition_timeout(
> - iss_reg_read(csi2->iss, csi2->phy->phy_regs, REGISTER1) &
> - REGISTER1_RESET_DONE_CTRLCLK, 10000, 100, 500);
> + timeout = iss_poll_condition_timeout(iss_reg_read(csi2->iss,
> + csi2->phy->phy_regs, REGISTER1) &
> + REGISTER1_RESET_DONE_CTRLCLK, 10000, 100, 500);
> if (timeout) {
> dev_err(csi2->iss->dev, "CSI2: CSI2_96M_FCLK reset timeout!\n");
> return -EBUSY;
> --
> 2.17.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20191014055308.GA1069%40jamal-XPS-13-9343.
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH v3] staging: media: omap4iss: Fix lines ending with open parenthesis
2019-10-14 6:10 ` [Outreachy kernel] " Julia Lawall
@ 2019-10-14 22:21 ` Laurent Pinchart
2019-10-15 23:07 ` Jamal Shareef
0 siblings, 1 reply; 4+ messages in thread
From: Laurent Pinchart @ 2019-10-14 22:21 UTC (permalink / raw)
To: Julia Lawall
Cc: Jamal Shareef, boqun.feng, andrea.parri, mchehab, gregkh,
outreachy-kernel, Joe Perches
Hello Jamal and Julia,
On Mon, Oct 14, 2019 at 08:10:30AM +0200, Julia Lawall wrote:
> On Sun, 13 Oct 2019, Jamal Shareef wrote:
> > Fix lines ending with open parenthesis. Issue
> > found by checkpatch.
> >
> > Signed-off-by: Jamal Shareef <jamal.k.shareef@gmail.com>
> > ---
> > Changes in v3:
> > - Remove created local variables and revert
> > back to original function calls because intent
> > is to dynamically poll the value.
> > - Format code for better readability. This
> > introduces new warning for "alignment should
> > match open parenthesis" but will be disregarded.
> >
> > Changes in v2:
> > - Include similar changes in iss.c file.
> > - Create local variables for iss_reg_read
> > function calls for better code formatting.
> > ---
> > drivers/staging/media/omap4iss/iss.c | 58 +++++++++++++----------
> > drivers/staging/media/omap4iss/iss_csi2.c | 12 ++---
> > 2 files changed, 39 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> > index 1a966cb2f3a6..9112eb89b7c0 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);
>
> There is indeed something unpleasant about something that looks like a
> function call, but it actually a while loop. Perhaps the word "poll" is
> supposed to be sufficiently suggestive of loopness.
It's similar to the wait_event_* family of macros from
include/linux/wait.h. Not ideal, I agree, but C has its limitations :-)
> In the proposed code, it is very hard to see what argument goes with what.
> Perhaps an improvement for the original code would be to leave the first
> argument on a new line as it is, but move the ISS_HL_SYSCONFIG_SOFTRESET
> over 2 spaces so that it is lined up with what it is anded with, and then
> put the 1000 etc on a line below, lines up with the !, so that it is clear
> that they are not part of the polling condition.
We could also define helper functions to perform the checks. Something
along the line of
static bool iss_reset_done(struct iss_device *iss)
{
return !(iss_reg_read(iss, OMAP4_ISS_MEM_TOP, ISS_HL_SYSCONFIG) &
ISS_HL_SYSCONFIG_SOFTRESET);
}
Then the code in iss_reset() could be written as
timeout = iss_poll_condition_timeout(iss_reset_done(iss),
1000, 10, 100);
> > if (timeout) {
> > dev_err(iss->dev, "ISS reset timeout\n");
> > return -ETIMEDOUT;
> > @@ -583,9 +584,9 @@ 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 +596,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);
> > if (timeout) {
> > dev_err(iss->dev, "ISP5 reset timeout\n");
> > return -ETIMEDOUT;
> > @@ -964,7 +966,8 @@ iss_register_subdev_group(struct iss_device *iss,
> > }
> >
> > subdev = v4l2_i2c_new_subdev_board(&iss->v4l2_dev, adapter,
> > - board_info->board_info, NULL);
> > + board_info->board_info,
> > + NULL);
> > if (!subdev) {
> > dev_err(iss->dev, "Unable to register subdev %s\n",
> > board_info->board_info->type);
> > @@ -1108,33 +1111,38 @@ 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;
> >
> > diff --git a/drivers/staging/media/omap4iss/iss_csi2.c b/drivers/staging/media/omap4iss/iss_csi2.c
> > index a6dc2d2b1228..cd705df04f6c 100644
> > --- a/drivers/staging/media/omap4iss/iss_csi2.c
> > +++ b/drivers/staging/media/omap4iss/iss_csi2.c
> > @@ -495,9 +495,9 @@ int omap4iss_csi2_reset(struct iss_csi2_device *csi2)
> > iss_reg_set(csi2->iss, csi2->regs1, CSI2_SYSCONFIG,
> > CSI2_SYSCONFIG_SOFT_RESET);
> >
> > - timeout = iss_poll_condition_timeout(
> > - iss_reg_read(csi2->iss, csi2->regs1, CSI2_SYSSTATUS) &
> > - CSI2_SYSSTATUS_RESET_DONE, 500, 100, 200);
> > + timeout = iss_poll_condition_timeout(iss_reg_read(csi2->iss,
> > + csi2->regs1, CSI2_SYSSTATUS) &
> > + CSI2_SYSSTATUS_RESET_DONE, 500, 100, 200);
> > if (timeout) {
> > dev_err(csi2->iss->dev, "CSI2: Soft reset timeout!\n");
> > return -EBUSY;
> > @@ -506,9 +506,9 @@ int omap4iss_csi2_reset(struct iss_csi2_device *csi2)
> > iss_reg_set(csi2->iss, csi2->regs1, CSI2_COMPLEXIO_CFG,
> > CSI2_COMPLEXIO_CFG_RESET_CTRL);
> >
> > - timeout = iss_poll_condition_timeout(
> > - iss_reg_read(csi2->iss, csi2->phy->phy_regs, REGISTER1) &
> > - REGISTER1_RESET_DONE_CTRLCLK, 10000, 100, 500);
> > + timeout = iss_poll_condition_timeout(iss_reg_read(csi2->iss,
> > + csi2->phy->phy_regs, REGISTER1) &
> > + REGISTER1_RESET_DONE_CTRLCLK, 10000, 100, 500);
> > if (timeout) {
> > dev_err(csi2->iss->dev, "CSI2: CSI2_96M_FCLK reset timeout!\n");
> > return -EBUSY;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH v3] staging: media: omap4iss: Fix lines ending with open parenthesis
2019-10-14 22:21 ` Laurent Pinchart
@ 2019-10-15 23:07 ` Jamal Shareef
0 siblings, 0 replies; 4+ messages in thread
From: Jamal Shareef @ 2019-10-15 23:07 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Julia Lawall, boqun.feng, andrea.parri, mchehab, gregkh,
outreachy-kernel, Joe Perches
On Tue, Oct 15, 2019 at 01:21:37AM +0300, Laurent Pinchart wrote:
> Hello Jamal and Julia,
>
> On Mon, Oct 14, 2019 at 08:10:30AM +0200, Julia Lawall wrote:
> > On Sun, 13 Oct 2019, Jamal Shareef wrote:
> > > Fix lines ending with open parenthesis. Issue
> > > found by checkpatch.
> > >
> > > Signed-off-by: Jamal Shareef <jamal.k.shareef@gmail.com>
> > > ---
> > > Changes in v3:
> > > - Remove created local variables and revert
> > > back to original function calls because intent
> > > is to dynamically poll the value.
> > > - Format code for better readability. This
> > > introduces new warning for "alignment should
> > > match open parenthesis" but will be disregarded.
> > >
> > > Changes in v2:
> > > - Include similar changes in iss.c file.
> > > - Create local variables for iss_reg_read
> > > function calls for better code formatting.
> > > ---
> > > drivers/staging/media/omap4iss/iss.c | 58 +++++++++++++----------
> > > drivers/staging/media/omap4iss/iss_csi2.c | 12 ++---
> > > 2 files changed, 39 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> > > index 1a966cb2f3a6..9112eb89b7c0 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);
> >
> > There is indeed something unpleasant about something that looks like a
> > function call, but it actually a while loop. Perhaps the word "poll" is
> > supposed to be sufficiently suggestive of loopness.
>
> It's similar to the wait_event_* family of macros from
> include/linux/wait.h. Not ideal, I agree, but C has its limitations :-)
>
> > In the proposed code, it is very hard to see what argument goes with what.
> > Perhaps an improvement for the original code would be to leave the first
> > argument on a new line as it is, but move the ISS_HL_SYSCONFIG_SOFTRESET
> > over 2 spaces so that it is lined up with what it is anded with, and then
> > put the 1000 etc on a line below, lines up with the !, so that it is clear
> > that they are not part of the polling condition.
>
> We could also define helper functions to perform the checks. Something
> along the line of
>
> static bool iss_reset_done(struct iss_device *iss)
> {
> return !(iss_reg_read(iss, OMAP4_ISS_MEM_TOP, ISS_HL_SYSCONFIG) &
> ISS_HL_SYSCONFIG_SOFTRESET);
> }
>
> Then the code in iss_reset() could be written as
>
> timeout = iss_poll_condition_timeout(iss_reset_done(iss),
> 1000, 10, 100);
>
This would simplify things a lot. We would have to pass in arguments
(or have multiple helper functions) for the memory resource and offset
because the function will be called differently in the two cases.
Another function also may be necessary for the idle check which
does not have the "!" operator.
I am open to either way of tidying this up - helper function
or adjusting the spacing a bit.
Would this modification to the helper function and specific calls
in the proceeding functions work?
****
static bool iss_reset_done(struct iss_device *iss, enum iss_mem_resources res,
u32 offset)
{
return !(iss_reg_read(iss, res, offset) &
ISS_HL_SYSCONFIG_SOFTRESET);
}
{
...
enum iss_mem_resources res = OMAP4_ISS_MEM_TOP;
u32 offset = ISS_HL_SYSCONFIG;
timeout = iss_poll_condition_timeout(!iss_reset_done(iss, res, offset),
1000, 10, 100);
...
****
There is some repetitive code here, and I did yet not account for the
different SOFTRESET or STANDBY functions. It might not be worth the
effort.
Jamal
> > > if (timeout) {
> > > dev_err(iss->dev, "ISS reset timeout\n");
> > > return -ETIMEDOUT;
> > > @@ -583,9 +584,9 @@ 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 +596,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);
> > > if (timeout) {
> > > dev_err(iss->dev, "ISP5 reset timeout\n");
> > > return -ETIMEDOUT;
> > > @@ -964,7 +966,8 @@ iss_register_subdev_group(struct iss_device *iss,
> > > }
> > >
> > > subdev = v4l2_i2c_new_subdev_board(&iss->v4l2_dev, adapter,
> > > - board_info->board_info, NULL);
> > > + board_info->board_info,
> > > + NULL);
> > > if (!subdev) {
> > > dev_err(iss->dev, "Unable to register subdev %s\n",
> > > board_info->board_info->type);
> > > @@ -1108,33 +1111,38 @@ 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;
> > >
> > > diff --git a/drivers/staging/media/omap4iss/iss_csi2.c b/drivers/staging/media/omap4iss/iss_csi2.c
> > > index a6dc2d2b1228..cd705df04f6c 100644
> > > --- a/drivers/staging/media/omap4iss/iss_csi2.c
> > > +++ b/drivers/staging/media/omap4iss/iss_csi2.c
> > > @@ -495,9 +495,9 @@ int omap4iss_csi2_reset(struct iss_csi2_device *csi2)
> > > iss_reg_set(csi2->iss, csi2->regs1, CSI2_SYSCONFIG,
> > > CSI2_SYSCONFIG_SOFT_RESET);
> > >
> > > - timeout = iss_poll_condition_timeout(
> > > - iss_reg_read(csi2->iss, csi2->regs1, CSI2_SYSSTATUS) &
> > > - CSI2_SYSSTATUS_RESET_DONE, 500, 100, 200);
> > > + timeout = iss_poll_condition_timeout(iss_reg_read(csi2->iss,
> > > + csi2->regs1, CSI2_SYSSTATUS) &
> > > + CSI2_SYSSTATUS_RESET_DONE, 500, 100, 200);
> > > if (timeout) {
> > > dev_err(csi2->iss->dev, "CSI2: Soft reset timeout!\n");
> > > return -EBUSY;
> > > @@ -506,9 +506,9 @@ int omap4iss_csi2_reset(struct iss_csi2_device *csi2)
> > > iss_reg_set(csi2->iss, csi2->regs1, CSI2_COMPLEXIO_CFG,
> > > CSI2_COMPLEXIO_CFG_RESET_CTRL);
> > >
> > > - timeout = iss_poll_condition_timeout(
> > > - iss_reg_read(csi2->iss, csi2->phy->phy_regs, REGISTER1) &
> > > - REGISTER1_RESET_DONE_CTRLCLK, 10000, 100, 500);
> > > + timeout = iss_poll_condition_timeout(iss_reg_read(csi2->iss,
> > > + csi2->phy->phy_regs, REGISTER1) &
> > > + REGISTER1_RESET_DONE_CTRLCLK, 10000, 100, 500);
> > > if (timeout) {
> > > dev_err(csi2->iss->dev, "CSI2: CSI2_96M_FCLK reset timeout!\n");
> > > return -EBUSY;
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-15 23:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-14 5:53 [PATCH v3] staging: media: omap4iss: Fix lines ending with open parenthesis Jamal Shareef
2019-10-14 6:10 ` [Outreachy kernel] " Julia Lawall
2019-10-14 22:21 ` Laurent Pinchart
2019-10-15 23:07 ` Jamal Shareef
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.