Messages in this thread | | | From | Andy Shevchenko <> | Date | Sat, 3 Mar 2018 17:44:44 +0200 | Subject | Re: [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips |
| |
On Sat, Mar 3, 2018 at 5:37 PM, Jonathan Cameron <jic23@kernel.org> wrote: > On Wed, 28 Feb 2018 17:06:09 +0200 >> On Wed, Feb 28, 2018 at 2:15 AM, Pierre Bourdon <delroth@google.com> wrote:
Better to address even minors before submission.
>> > + 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.
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.
>> int ret;
-- With Best Regards, Andy Shevchenko
| |