lkml.org 
[lkml]   [2017]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 2/2] staging: iio: ad7606: move out of staging
From
Date
On 12/30/2016 09:16 PM, Jonathan Cameron wrote:
> On 11/12/16 02:47, Eva Rachel Retuya wrote:
>> Move the ad7606 driver from staging/iio/adc to iio/adc. Also, update the
>> corresponding Makefile and Kconfig associated with the change.
>>
>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> Personally (and this is much debated ;) I prefer for the odd case
> of staging graduations, that git isn't run with the -M parameter so we
> have the whole driver to comment on.
>
> Anyhow, just casting my eyes over the code so here are some notes:
> 1. The whole computing scale available values on the fly seems rather over the
> top as they can be easily precomputed and stored in a static const array.
> There are only 2 of them!
>
> 2. There are some single line comments still using multiline syntax (now
> I'm really nitpicking ;)
>
> 3. A slight oddity has crept in with the cleanup of attributes. We now
> prevent the appearance of the _scale_available / oversampling_ratio_available
> attributes if the gpios are attached, but not _scale or _oversampling.
> (oops). If we are going to do this dance, then we should also have an
> alternative set of iio_chan_spec array to reflect this.
>
> 4. I'd drop the 'More devices added in future' comment. It's now the future
> and they haven't been yet ;)
>

I actually have a patch adding more devices, that just waits for the driver
to be moved out of staging. But still the comment can be removed it is not
really meaningful.

> 5. We can write oversampling ratio, but queries to the write_get_fmt will
> return an error for that. Probably want to fix that unless I'm missing something.
>
> 6. There are some ancient references to 'ring' buffers in the comments and
> function naming. We switched over from my hand rolled ring buffer to the
> kfifo buffer we now use ages ago. Would be nice to clear those out.
>
> None of these are really blockers to it moving out of staging
> (except the bugs we introduced in the cleanup), but might be nice
> to do one last series pinning those down then get a final review done to
> see if we missed anything else.
>
> Been a long time since I looked at this one in any depth and it's certainly
> heading in the right direction!

\
 
 \ /
  Last update: 2017-01-02 19:34    [W:0.224 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site