diff mbox series

[v2] media: adv7180: Fix cppcheck warnings

Message ID 20240102142729.1743421-1-bhavin.sharma@siliconsignals.io
State New
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
Kieran Bingham Feb. 9, 2024, 12:25 p.m. UTC | #5 
Quoting Bhavin Sharma (2024-02-09 09:11:22)
> 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/adv7180c
> >>>> 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.

Yes, I stated keep the if (err) hugging the line that sets err:


  >         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;


To me the if (err) is directly associated with the
mutex_lock_interruptible(). That's the error it's checking, so they
should stay together.

Which you can do by using the following if it's clearer:

	struct adv7180_state *state = to_state(sd);

	int err = mutex_lock_interruptible(&state->mutex);
	if (err)
		return err;

That may not even remove the checkpatch warning though.

As Hans says, there's little reward here, except for learning how to get
patches into the kernel and bumping your kernel stats.

If you're a junior trying to learn how to get into kernel development, I
think that's reasonable to some degree. (Which was my assumption when I
responded, and on that note, It seems that your replies are coming
through really badly formatted to me, which I assume is your mail client
needing some checking through).


I see your updated patch now makes unrelated changes which confuse
matters. This was partially at my request, but I think it was
mis-understood, so I'm sorry for not being more clear and direct:


      >>>        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)


And I agree with Hans, now you have a commit title that states:

 WARNING: Missing a blank line after declarations

And you are making changes which bear no direct relation ship to that.
I suggested if you are making one change through out it should be in
it's own full patch, but that patch must be able to stand out right on
it's own. So the commit message but clearly say what the patch does and
it should do only one thing.



If you're really trying to work through the whole kernel with cleanups,
then I think there's a kernel janitors project you should post to. But
I'm not sure if that's still a thing. Otherwise, this is indeed taking
rare and valuable review time away from more substantial topics.

--
Regards

Kieran
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;