Re: [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 3 Mar 2018 17:44:44 +0200
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:

> On Sat, Mar 3, 2018 at 5:37 PM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > On Wed, 28 Feb 2018 17:06:09 +0200  
> >> On Wed, Feb 28, 2018 at 2:15 AM, Pierre Bourdon <delroth@xxxxxxxxxx> wrote:  
> 
> Better to address even minors before submission.

Absolutely.

> 
> >> > +       if (itime <= 0 || itime > 255)  
> >>
> >> Just side note: Suprisingly how many in_range() implementations we
> >> have in kernel...  
> > I guess one of those things that is so simple it's not worth having
> > one true in_range to rule them all ;)  
> 
> We have already several implementations of the macro.
> 
> >> > +static int bh1730_adjust_gain(struct bh1730_data *bh1730)
> >> > +{
> >> > +       int visible, ir, highest, gain, ret, i;  
> >>
> >> int visible, ir, highest, gain;
> >> unsigned int i;  
> >
> > Is there a strong reason for this one that I'm missing?
> > (beyond personal taste!)  
> 
> First of all, I'm far from being fan of mixing int ret into other
> variable definitions.
> 
> unsigned int i OTOH shows explicitly that we have counter which is not
> supposed to be negative.
Given it's specifically indexing over an enum (which can have any definition
it likes) I wouldn't normally care, but fair enough.

> 
> int i in most of the cases will work, so, it's a minor. I'm not
> insisting, though having counter variable on separate line is also a
> good thing.
> 
> In general, having different things in one line is a bad idea for my opinion.
Agreed.

> 
> >> int ret;  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux