Message-ID: <170748150921.721346.13310424783139362841@ping.linuxembedded.co.uk>
Date: Fri, 09 Feb 2024 12:25:09 +0000
From: Kieran Bingham <kieran.bingham@...asonboard.com>
To: Bhavin Sharma <bhavin.sharma@...iconsignals.io>, Hans Verkuil <hverkuil@...all.nl>, mchehab@...nel.org
Cc: Lars-Peter Clausen <lars@...afoo.de>, linux-media@...r.kernel.org <linux-media@...r.kernel.org>, linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] media: adv7180: Fix cppcheck warnings

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@...iconsignals.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

Powered by blists - more mailing lists