Re: [PATCH] media: adv7180: Fix cppcheck warnings and errors

[Kieran Bingham]

Quoting Bhavin Sharma (2023-12-29 13:37:14)
> Thanks for the reply,�Kieran
> 
> >> WARNING: Missing a blank line after declarations
> >> ERROR: else should follow close brace '}'
> >> 
> >> Signed-off-by: Bhavin Sharma <bhavin.sharma@xxxxxxxxxxxxxxxxx>
> >> 
> >> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> >> index 54134473186b..91756116eff7 100644
> >> --- a/drivers/media/i2c/adv7180.c
> >> +++ b/drivers/media/i2c/adv7180.c
> >> @@ -357,6 +357,7 @@ static int adv7180_querystd(struct v4l2_subdev *sd, v4l2_std_id *std)
> >>� {
> >>�������� struct adv7180_state *state = to_state(sd);
> 
> >>Personally, I would keep the if (err) hugging the line it's associated
> with.
> 
> If we follow the code base pattern for this diver, we are getting same online space in conditional if statements.
> So, we need to make changes there also.

If there are multiple places in a file for the same fixup, then indeed -
make them all in a single patch as a single cleanup.


> >>�������� int err = mutex_lock_interruptible(&state->mutex);
> >> +
> >>�������� if (err)
> >>���������������� return err;
> >>� 
> >> @@ -411,6 +412,7 @@ static int adv7180_g_input_status(struct v4l2_subdev *sd, u32 *status)
> >>� {
> >>�������� struct adv7180_state *state = to_state(sd);
> >>�������� int ret = mutex_lock_interruptible(&state->mutex);
> >> +
> >>�������� if (ret)
> >>���������������� return ret;
> >>� 
> >> @@ -1046,8 +1048,7 @@ static int adv7182_init(struct adv7180_state *state)
> >>���������������������������������������������� ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> >>���������������������������������������������� 0x17);
> >>������������������������ }
> >> -�������������� }
> >> -�������������� else
> >> +�������������� } else
> 
> >>I think kernel code style requires an else clause following a multiline
> scope to also have its scope enclosed in braces even if it's a single
> statement.
> 
> On many places in driver there is single statement after else without closing�
> So, we have to make changes in those places also.
> 
> So, better I should make changes in all places and make version V2 patch.

Yes, but you should probably tackle both cleanups as two patches
covering the whole file for each cleanup.

--
Kieran


> 
> Please give your suggestions.
> 
> --
> Bhavin Sharma
> 
> >>������������������������ adv7180_write(state,
> >>�������������������������������������� ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> >>�������������������������������������� 0x07);
> >> -- 
> >> 2.25.1
> >>