On Wed, 25 May 2011, Mauro Carvalho Chehab wrote:

> Em 23-05-2011 16:04, Hans Petter Selasky escreveu:
> > On Monday 23 May 2011 20:22:02 Guennadi Liakhovetski wrote:
> >> Please, inline patches. Otherwise, this is what one gets, when replying.
> >>
> >> On Mon, 23 May 2011, Hans Petter Selasky wrote:
> >>> --HPS
> >>
> >> In any case, just throwing in my 2 cents - no idea how not using the
> >> return value of WARN() makes code more readable. On the contrary, using it
> >> is a standard practice. This patch doesn't seem like an improvement to me.
> > 
> > There is no strong reason for the WARN() part, you may ignore that, but the 
> > ret = 0, part is still valid. Should I generate a new patch or can you 
> > handle 
> > this?
> Em 23-05-2011 08:07, Hans Petter Selasky escreveu:
> > --HPS
> > 
> > 
> > dvb-usb-0005.patch
> > 
> > 
> > From 94b88b92763f9309018ba04c200a8842ce1ff0ed Mon Sep 17 00:00:00 2001
> > From: Hans Petter Selasky <hsela...@c2i.net>
> > Date: Mon, 23 May 2011 13:07:08 +0200
> > Subject: [PATCH] Make code more readable by not using the return value of 
> > the WARN() macro. Set ret variable in an undefined case.
> > 
> > Signed-off-by: Hans Petter Selasky <hsela...@c2i.net>
> > ---
> >  drivers/media/video/sr030pc30.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/media/video/sr030pc30.c 
> > b/drivers/media/video/sr030pc30.c
> > index c901721..6cc64c9 100644
> > --- a/drivers/media/video/sr030pc30.c
> > +++ b/drivers/media/video/sr030pc30.c
> > @@ -726,8 +726,10 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, 
> > int on)
> >     const struct sr030pc30_platform_data *pdata = info->pdata;
> >     int ret;
> >  
> > -   if (WARN(pdata == NULL, "No platform data!\n"))
> > +   if (pdata == NULL) {
> > +           WARN(1, "No platform data!\n");
> >             return -ENOMEM;
> > +   }
> >  
> >     /*
> >      * Put sensor into power sleep mode before switching off
> > @@ -746,6 +748,7 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, 
> > int on)
> >     if (on) {
> >             ret = sr030pc30_base_config(sd);
> >     } else {
> > +           ret = 0;
> >             info->curr_win = NULL;
> >             info->curr_fmt = NULL;
> >     }
> > -- 1.7.1.1
> 
> IMHO, both hunks make sense, as, on the first hunk, it is returning an error 
> condition.
> Yet, -ENOMEM seems to be the wrong return code. -EINVAL is probably more 
> appropriate.

Sorry, Mauro, don't understand. AFAICS, the first hunk makes _no_ 
functional difference, it only obfuscates a perfectly valid use of the 
WARN() macro.

Thanks
Guennadi

> 
> However, the patch is badly described. It is not about making the code 
> cleaner, but
> about avoiding to run s_power if no platform data is found, and to avoid 
> having
> ret undefined. Eventually, it should be broken into two different patches, as 
> they
> fix different things.
> 
> Please, when sending us patches, provide a proper description with "what" 
> information
> at the first line, and why and how at the patch descriptions. Please, also 
> avoid to
> have any line bigger than 74 characters, otherwise they'll look weird when 
> seeing the
> patch history.
> 
> Thanks,
> Mauro.
> information a
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to