Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1549354

Re: [PATCH v3 2/2] staging: iio: ad7606: move out of staging

Show all headers | View raw

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!

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

Re: [PATCH v3 2/2] staging: iio: ad7606: move out of staging Jonathan Cameron <jic23@kernel.org> - 2016-12-30 21:20 +0100
  Re: [PATCH v3 2/2] staging: iio: ad7606: move out of staging Lars-Peter Clausen <lars@metafoo.de> - 2017-01-02 19:40 +0100

csiph-web