diff mbox series

[v2] media: adv7180: Fix cppcheck warnings

Message ID 20240102142729.1743421-1-bhavin.sharma@siliconsignals.io (mailing list archive)
State Rejected
Delegated to: Hans Verkuil
Headers
Series [v2] media: adv7180: Fix cppcheck warnings |

Commit Message

Bhavin Sharma Jan. 2, 2024, 2:27 p.m. UTC 
  WARNING: Missing a blank line after declarations

Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>
---
 drivers/media/i2c/adv7180.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

Comments

Hans Verkuil Feb. 5, 2024, 8:36 a.m. UTC | #1 
Hi Bhavin,

On 02/01/2024 15:27, Bhavin Sharma wrote:
> WARNING: Missing a blank line after declarations
> 
> Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>
> ---
>  drivers/media/i2c/adv7180.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 54134473186b..0023a546b3c9 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -335,8 +335,9 @@ static u32 adv7180_status_to_v4l2(u8 status1)
>  static int __adv7180_status(struct adv7180_state *state, u32 *status,
>  			    v4l2_std_id *std)
>  {
> -	int status1 = adv7180_read(state, ADV7180_REG_STATUS1);
> +	int status1;
>  
> +	status1 = adv7180_read(state, ADV7180_REG_STATUS1);
>  	if (status1 < 0)
>  		return status1;
>  
> @@ -356,7 +357,9 @@ static inline struct adv7180_state *to_state(struct v4l2_subdev *sd)
>  static int adv7180_querystd(struct v4l2_subdev *sd, v4l2_std_id *std)
>  {
>  	struct adv7180_state *state = to_state(sd);
> -	int err = mutex_lock_interruptible(&state->mutex);
> +	int err;
> +
> +	err = mutex_lock_interruptible(&state->mutex);

The problem here is the missing empty line, not that 'int err = <something>;' part.
So just add the empty line and don't split up the variable assignment.

>  	if (err)
>  		return err;
>  
> @@ -388,8 +391,9 @@ static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
>  			     u32 output, u32 config)
>  {
>  	struct adv7180_state *state = to_state(sd);
> -	int ret = mutex_lock_interruptible(&state->mutex);
> +	int ret;
>  
> +	ret = mutex_lock_interruptible(&state->mutex);
>  	if (ret)
>  		return ret;
>  
> @@ -399,7 +403,6 @@ static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
>  	}
>  
>  	ret = state->chip_info->select_input(state, input);
> -

Why remove this empty line? It has nothing to do with what you are trying
to fix.

>  	if (ret == 0)
>  		state->input = input;
>  out:
> @@ -410,7 +413,9 @@ static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
>  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);
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&state->mutex);
>  	if (ret)
>  		return ret;
>  
> @@ -436,8 +441,9 @@ static int adv7180_program_std(struct adv7180_state *state)
>  static int adv7180_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
>  {
>  	struct adv7180_state *state = to_state(sd);
> -	int ret = mutex_lock_interruptible(&state->mutex);
> +	int ret;
>  
> +	ret = mutex_lock_interruptible(&state->mutex);
>  	if (ret)
>  		return ret;
>  
> @@ -466,8 +472,9 @@ static int adv7180_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm)
>  static int adv7180_g_frame_interval(struct v4l2_subdev *sd,
>  				    struct v4l2_subdev_frame_interval *fi)
>  {
> -	struct adv7180_state *state = to_state(sd);
> +	struct adv7180_state *state;
>  
> +	state = to_state(sd);

And I am sure this never produced a cppcheck warning since there is an
empty line. If cppcheck DOES produce a warning on this, then it is a
useless application.

>  	if (state->curr_norm & V4L2_STD_525_60) {
>  		fi->interval.numerator = 1001;
>  		fi->interval.denominator = 30000;
> @@ -828,8 +835,9 @@ static int adv7180_get_mbus_config(struct v4l2_subdev *sd,
>  				   unsigned int pad,
>  				   struct v4l2_mbus_config *cfg)
>  {
> -	struct adv7180_state *state = to_state(sd);
> +	struct adv7180_state *state;
>  
> +	state = to_state(sd);
>  	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
>  		cfg->type = V4L2_MBUS_CSI2_DPHY;
>  		cfg->bus.mipi_csi2.num_data_lanes = 1;
> @@ -857,8 +865,9 @@ static int adv7180_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
>  
>  static int adv7180_g_pixelaspect(struct v4l2_subdev *sd, struct v4l2_fract *aspect)
>  {
> -	struct adv7180_state *state = to_state(sd);
> +	struct adv7180_state *state;
>  
> +	state = to_state(sd);
>  	if (state->curr_norm & V4L2_STD_525_60) {
>  		aspect->numerator = 11;
>  		aspect->denominator = 10;

Honestly, none of these changes are worth the effort, so I just reject this.

Regards,

	Hans
Bhavin Sharma Feb. 6, 2024, 5:05 a.m. UTC | #2 
Hi Hans,

> Hi Bhavin,

> On 02/01/2024 15:27, Bhavin Sharma wrote:
>> WARNING: Missing a blank line after declarations
>>
>> Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>
>> ---
>>  drivers/media/i2c/adv7180.c | 27 ++++++++++++++++++---------
>>  1 file changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
>> index 54134473186b..0023a546b3c9 100644
>> --- a/drivers/media/i2c/adv7180.c
>> +++ b/drivers/media/i2c/adv7180.c
>> @@ -335,8 +335,9 @@ static u32 adv7180_status_to_v4l2(u8 status1)
>>  static int __adv7180_status(struct adv7180_state *state, u32 *status,
>>                           v4l2_std_id *std)
>>  {
>> -     int status1 = adv7180_read(state, ADV7180_REG_STATUS1);
>> +     int status1;
>>
>> +     status1 = adv7180_read(state, ADV7180_REG_STATUS1);
>>       if (status1 < 0)
>>               return status1;
>>
>> @@ -356,7 +357,9 @@ static inline struct adv7180_state *to_state(struct v4l2_subdev *sd)
>>  static int adv7180_querystd(struct v4l2_subdev *sd, v4l2_std_id *std)
>>  {
>>       struct adv7180_state *state = to_state(sd);
>> -     int err = mutex_lock_interruptible(&state->mutex);
>> +     int err;
>> +
>> +     err = mutex_lock_interruptible(&state->mutex);

> The problem here is the missing empty line, not that 'int err = <something>;' part.
> So just add the empty line and don't split up the variable assignment.

Yes, the error is of missing empty line and I only resolved that particular error in the first version
of this patch.

But I was recommended to keep the conditional statement close to the line it is associated with
and to make changes in the code wherever similar format is followed. 

So I followed the advise of Kieran Bingham and made changes accordingly. 

Below is the link of the full discussion : https://lore.kernel.org/lkml/MAZPR01MB695752E4ADB0110443EA695CF2432@MAZPR01MB6957.INDPRD01.PROD.OUTLOOK.COM/T/

>>       if (err)
>>               return err;
>>
>> @@ -388,8 +391,9 @@ static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
>>                            u32 output, u32 config)
>>  {
>>       struct adv7180_state *state = to_state(sd);
>> -     int ret = mutex_lock_interruptible(&state->mutex);
>> +     int ret;
>>
>> +     ret = mutex_lock_interruptible(&state->mutex);
>>       if (ret)
>>               return ret;
>>
>> @@ -399,7 +403,6 @@ static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
>>       }
>>
>>       ret = state->chip_info->select_input(state, input);
>> -

> Why remove this empty line? It has nothing to do with what you are trying
> to fix.

>>       if (ret == 0)
>>               state->input = input;
>>  out:
>> @@ -410,7 +413,9 @@ static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
>>  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);
>> +     int ret;
>> +
>> +     ret = mutex_lock_interruptible(&state->mutex);
>>       if (ret)
>>               return ret;
>>
>> @@ -436,8 +441,9 @@ static int adv7180_program_std(struct adv7180_state *state)
>>  static int adv7180_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
>>  {
>>       struct adv7180_state *state = to_state(sd);
>> -     int ret = mutex_lock_interruptible(&state->mutex);
>> +     int ret;
>>
>> +     ret = mutex_lock_interruptible(&state->mutex);
>>       if (ret)
>>               return ret;
>>
>> @@ -466,8 +472,9 @@ static int adv7180_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm)
>>  static int adv7180_g_frame_interval(struct v4l2_subdev *sd,
>>                                   struct v4l2_subdev_frame_interval *fi)
>>  {
>> -     struct adv7180_state *state = to_state(sd);
>> +     struct adv7180_state *state;
>>
>> +     state = to_state(sd);

> And I am sure this never produced a cppcheck warning since there is an
> empty line. If cppcheck DOES produce a warning on this, then it is a
> useless application.

>>       if (state->curr_norm & V4L2_STD_525_60) {
>>               fi->interval.numerator = 1001;
>>               fi->interval.denominator = 30000;
>> @@ -828,8 +835,9 @@ static int adv7180_get_mbus_config(struct v4l2_subdev *sd,
>>                                  unsigned int pad,
>>                                  struct v4l2_mbus_config *cfg)
>>  {
>> -     struct adv7180_state *state = to_state(sd);
>> +     struct adv7180_state *state;
>>
>> +     state = to_state(sd);
>>       if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
>>               cfg->type = V4L2_MBUS_CSI2_DPHY;
>>               cfg->bus.mipi_csi2.num_data_lanes = 1;
>> @@ -857,8 +865,9 @@ static int adv7180_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
>>
>>  static int adv7180_g_pixelaspect(struct v4l2_subdev *sd, struct v4l2_fract *aspect)
>>  {
>> -     struct adv7180_state *state = to_state(sd);
>> +     struct adv7180_state *state;
>>
>> +     state = to_state(sd);
>>       if (state->curr_norm & V4L2_STD_525_60) {
>>               aspect->numerator = 11;
>>               aspect->denominator = 10;

> Honestly, none of these changes are worth the effort, so I just reject this.

Kindly give your suggestions.

Regards,
Bhavin Sharma
Hans Verkuil Feb. 6, 2024, 7:38 a.m. UTC | #3 
On 06/02/2024 06:05, Bhavin Sharma wrote:
> Hi Hans,
> 
>> Hi Bhavin,
> 
>> On 02/01/2024 15:27, Bhavin Sharma wrote:
>>> WARNING: Missing a blank line after declarations
>>>
>>> Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>
>>> ---
>>>   drivers/media/i2c/adv7180.c | 27 ++++++++++++++++++---------
>>>   1 file changed, 18 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
>>> index 54134473186b..0023a546b3c9 100644
>>> --- a/drivers/media/i2c/adv7180.c
>>> +++ b/drivers/media/i2c/adv7180.c
>>> @@ -335,8 +335,9 @@ static u32 adv7180_status_to_v4l2(u8 status1)
>>>   static int __adv7180_status(struct adv7180_state *state, u32 *status,
>>>                            v4l2_std_id *std)
>>>   {
>>> -     int status1 = adv7180_read(state, ADV7180_REG_STATUS1);
>>> +     int status1;
>>>
>>> +     status1 = adv7180_read(state, ADV7180_REG_STATUS1);
>>>        if (status1 < 0)
>>>                return status1;
>>>
>>> @@ -356,7 +357,9 @@ static inline struct adv7180_state *to_state(struct v4l2_subdev *sd)
>>>   static int adv7180_querystd(struct v4l2_subdev *sd, v4l2_std_id *std)
>>>   {
>>>        struct adv7180_state *state = to_state(sd);
>>> -     int err = mutex_lock_interruptible(&state->mutex);
>>> +     int err;
>>> +
>>> +     err = mutex_lock_interruptible(&state->mutex);
> 
>> The problem here is the missing empty line, not that 'int err = <something>;' part.
>> So just add the empty line and don't split up the variable assignment.
> 
> Yes, the error is of missing empty line and I only resolved that particular error in the first version
> of this patch.
> 
> But I was recommended to keep the conditional statement close to the line it is associated with
> and to make changes in the code wherever similar format is followed. 
> 
> So I followed the advise of Kieran Bingham and made changes accordingly. 
> 
> Below is the link of the full discussion : https://lore.kernel.org/lkml/MAZPR01MB695752E4ADB0110443EA695CF2432@MAZPR01MB6957.INDPRD01.PROD.OUTLOOK.COM/T/

Kieran said this:

>> @@ -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.
> 
> 
>>         int err = mutex_lock_interruptible(&state->mutex);
>> +
>>         if (err)
>>                 return err;
>>  

which I interpret as saying that he doesn't like adding the extra empty line.

> 
>>>        if (err)
>>>                return err;
>>>
>>> @@ -388,8 +391,9 @@ static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
>>>                             u32 output, u32 config)
>>>   {
>>>        struct adv7180_state *state = to_state(sd);
>>> -     int ret = mutex_lock_interruptible(&state->mutex);
>>> +     int ret;
>>>
>>> +     ret = mutex_lock_interruptible(&state->mutex);

I don't believe he meant doing this.

In any case, none of this is worth the effort, just leave this driver as-is.

Regards,

	Hans

>>>        if (ret)
>>>                return ret;
>>>
>>> @@ -399,7 +403,6 @@ static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
>>>        }
>>>
>>>        ret = state->chip_info->select_input(state, input);
>>> -
> 
>> Why remove this empty line? It has nothing to do with what you are trying
>> to fix.
> 
>>>        if (ret == 0)
>>>                state->input = input;
>>>   out:
>>> @@ -410,7 +413,9 @@ static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
>>>   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);
>>> +     int ret;
>>> +
>>> +     ret = mutex_lock_interruptible(&state->mutex);
>>>        if (ret)
>>>                return ret;
>>>
>>> @@ -436,8 +441,9 @@ static int adv7180_program_std(struct adv7180_state *state)
>>>   static int adv7180_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
>>>   {
>>>        struct adv7180_state *state = to_state(sd);
>>> -     int ret = mutex_lock_interruptible(&state->mutex);
>>> +     int ret;
>>>
>>> +     ret = mutex_lock_interruptible(&state->mutex);
>>>        if (ret)
>>>                return ret;
>>>
>>> @@ -466,8 +472,9 @@ static int adv7180_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm)
>>>   static int adv7180_g_frame_interval(struct v4l2_subdev *sd,
>>>                                    struct v4l2_subdev_frame_interval *fi)
>>>   {
>>> -     struct adv7180_state *state = to_state(sd);
>>> +     struct adv7180_state *state;
>>>
>>> +     state = to_state(sd);
> 
>> And I am sure this never produced a cppcheck warning since there is an
>> empty line. If cppcheck DOES produce a warning on this, then it is a
>> useless application.
> 
>>>        if (state->curr_norm & V4L2_STD_525_60) {
>>>                fi->interval.numerator = 1001;
>>>                fi->interval.denominator = 30000;
>>> @@ -828,8 +835,9 @@ static int adv7180_get_mbus_config(struct v4l2_subdev *sd,
>>>                                   unsigned int pad,
>>>                                   struct v4l2_mbus_config *cfg)
>>>   {
>>> -     struct adv7180_state *state = to_state(sd);
>>> +     struct adv7180_state *state;
>>>
>>> +     state = to_state(sd);
>>>        if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
>>>                cfg->type = V4L2_MBUS_CSI2_DPHY;
>>>                cfg->bus.mipi_csi2.num_data_lanes = 1;
>>> @@ -857,8 +865,9 @@ static int adv7180_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
>>>
>>>   static int adv7180_g_pixelaspect(struct v4l2_subdev *sd, struct v4l2_fract *aspect)
>>>   {
>>> -     struct adv7180_state *state = to_state(sd);
>>> +     struct adv7180_state *state;
>>>
>>> +     state = to_state(sd);
>>>        if (state->curr_norm & V4L2_STD_525_60) {
>>>                aspect->numerator = 11;
>>>                aspect->denominator = 10;
> 
>> Honestly, none of these changes are worth the effort, so I just reject this.
> 
> Kindly give your suggestions.
> 
> Regards,
> Bhavin Sharma
Bhavin Sharma Feb. 9, 2024, 9:11 a.m. UTC | #4 
Hi Hans,

> On 06/02/2024 06:05, Bhavin Sharma wrote:
>> Hi Hans,
>>
> >> Hi Bhavin,
>>
>>> On 02/01/2024 15:27, Bhavin Sharma wrote:
>>>> WARNING: Missing a blank line after declarations
>>>>
>>>> Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>
>>>> ---
>>>>   drivers/media/i2c/adv7180.c | 27 ++++++++++++++++++---------
>>>>   1 file changed, 18 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
>>>> index 54134473186b..0023a546b3c9 100644
>>>> --- a/drivers/media/i2c/adv7180.c
>>>> +++ b/drivers/media/i2c/adv7180.c
>>>> @@ -335,8 +335,9 @@ static u32 adv7180_status_to_v4l2(u8 status1)
>>>>   static int __adv7180_status(struct adv7180_state *state, u32 *status,
>>>>                            v4l2_std_id *std)
>>>>   {
>>>> -     int status1 = adv7180_read(state, ADV7180_REG_STATUS1);
>>>> +     int status1;
>>>>
>>>> +     status1 = adv7180_read(state, ADV7180_REG_STATUS1);
>>>>        if (status1 < 0)
>>>>                return status1;
>>>>
>>>> @@ -356,7 +357,9 @@ static inline struct adv7180_state *to_state(struct v4l2_subdev *sd)
>>>>   static int adv7180_querystd(struct v4l2_subdev *sd, v4l2_std_id *std)
>>>>   {
>>>>        struct adv7180_state *state = to_state(sd);
>>>> -     int err = mutex_lock_interruptible(&state->mutex);
>>>> +     int err;
>>>> +
>>>> +     err = mutex_lock_interruptible(&state->mutex);
>>
>>> The problem here is the missing empty line, not that 'int err = <something>;' part.
>>> So just add the empty line and don't split up the variable assignment.
>>
>> Yes, the error is of missing empty line and I only resolved that particular error in the first version
>> of this patch.
>>
>> But I was recommended to keep the conditional statement close to the line it is associated with
>> and to make changes in the code wherever similar format is followed.
>
>> So I followed the advise of Kieran Bingham and made changes accordingly.
>>
>> Below is the link of the full discussion : https://lore.kernel.org/lkml/MAZPR01MB695752E4ADB0110443EA695CF2432@MAZPR01MB6957.INDPRD01.PROD.OUTLOOK.COM/T/

> Kieran said this:

>>> @@ -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.
>>
>>
>>>         int err = mutex_lock_interruptible(&state->mutex);
>>> +
>>>         if (err)
>>>                 return err;
>>>

> which I interpret as saying that he doesn't like adding the extra empty line.

>>
>>>>        if (err)
>>>>                return err;
>>>>
>>>> @@ -388,8 +391,9 @@ static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
>>>>                             u32 output, u32 config)
>>>>   {
>>>>        struct adv7180_state *state = to_state(sd);
>>>> -     int ret = mutex_lock_interruptible(&state->mutex);
>>>> +     int ret;
>>>>
>>>> +     ret = mutex_lock_interruptible(&state->mutex);

> I don't believe he meant doing this.

> In any case, none of this is worth the effort, just leave this driver as-is.

I appreciate your comments. 
My intention is to make linux kernel source as per kernel code style. In this approach I found these warnings "missing a blank line after declarations"  and made changes accordingly. 
Also, there should be blank line after declaration of a variable, correct me here if I am wrong.
As per the suggestions of Kieran Bingham, he recommended to keep the if(err) hugging the line it's associated. So to adopt this change I made changes accordingly.

Regards,
Bhavin Sharma
diff mbox series

Patch

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 54134473186b..0023a546b3c9 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -335,8 +335,9 @@  static u32 adv7180_status_to_v4l2(u8 status1)
 static int __adv7180_status(struct adv7180_state *state, u32 *status,
 			    v4l2_std_id *std)
 {
-	int status1 = adv7180_read(state, ADV7180_REG_STATUS1);
+	int status1;
 
+	status1 = adv7180_read(state, ADV7180_REG_STATUS1);
 	if (status1 < 0)
 		return status1;
 
@@ -356,7 +357,9 @@  static inline struct adv7180_state *to_state(struct v4l2_subdev *sd)
 static int adv7180_querystd(struct v4l2_subdev *sd, v4l2_std_id *std)
 {
 	struct adv7180_state *state = to_state(sd);
-	int err = mutex_lock_interruptible(&state->mutex);
+	int err;
+
+	err = mutex_lock_interruptible(&state->mutex);
 	if (err)
 		return err;
 
@@ -388,8 +391,9 @@  static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
 			     u32 output, u32 config)
 {
 	struct adv7180_state *state = to_state(sd);
-	int ret = mutex_lock_interruptible(&state->mutex);
+	int ret;
 
+	ret = mutex_lock_interruptible(&state->mutex);
 	if (ret)
 		return ret;
 
@@ -399,7 +403,6 @@  static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
 	}
 
 	ret = state->chip_info->select_input(state, input);
-
 	if (ret == 0)
 		state->input = input;
 out:
@@ -410,7 +413,9 @@  static int adv7180_s_routing(struct v4l2_subdev *sd, u32 input,
 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);
+	int ret;
+
+	ret = mutex_lock_interruptible(&state->mutex);
 	if (ret)
 		return ret;
 
@@ -436,8 +441,9 @@  static int adv7180_program_std(struct adv7180_state *state)
 static int adv7180_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 {
 	struct adv7180_state *state = to_state(sd);
-	int ret = mutex_lock_interruptible(&state->mutex);
+	int ret;
 
+	ret = mutex_lock_interruptible(&state->mutex);
 	if (ret)
 		return ret;
 
@@ -466,8 +472,9 @@  static int adv7180_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm)
 static int adv7180_g_frame_interval(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_frame_interval *fi)
 {
-	struct adv7180_state *state = to_state(sd);
+	struct adv7180_state *state;
 
+	state = to_state(sd);
 	if (state->curr_norm & V4L2_STD_525_60) {
 		fi->interval.numerator = 1001;
 		fi->interval.denominator = 30000;
@@ -828,8 +835,9 @@  static int adv7180_get_mbus_config(struct v4l2_subdev *sd,
 				   unsigned int pad,
 				   struct v4l2_mbus_config *cfg)
 {
-	struct adv7180_state *state = to_state(sd);
+	struct adv7180_state *state;
 
+	state = to_state(sd);
 	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
 		cfg->type = V4L2_MBUS_CSI2_DPHY;
 		cfg->bus.mipi_csi2.num_data_lanes = 1;
@@ -857,8 +865,9 @@  static int adv7180_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
 
 static int adv7180_g_pixelaspect(struct v4l2_subdev *sd, struct v4l2_fract *aspect)
 {
-	struct adv7180_state *state = to_state(sd);
+	struct adv7180_state *state;
 
+	state = to_state(sd);
 	if (state->curr_norm & V4L2_STD_525_60) {
 		aspect->numerator = 11;
 		aspect->denominator = 10;