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@xxxxxxxxx> > 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! _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
