On Tuesday, August 13, 2013 5:14 AM, Jonathan Cameron wrote: > On 07/24/13 06:36, Jingoo Han wrote: [.....] > Sorry it has taken me so long to get to this. Was obviously going > to be 'almost' as tedious to review as it would have been to put > it together :) > > You are brave taking this one as there are a lot of dragons > hiding in this code. > > In a few cases I'd have been tempted to strtobool warts and all. > > Two choices in adt7316 are too short. > Unsigned short in ad5933 should be u16 I think (to avoid > any issues). > If we are going for minimum length variables, the isl29018 use of > an unsigned long for a value that at most equals 16 seems excessive. > (I'm not sure we are aiming for minimal though..) > Also in that driver, you fill an int with a kstrtouint. Thank you for your comments. :-) I agree with your opinions. I will fix and send v2 patch soon. Best regards, Jingoo Han > ADE7753 plays some evil games in using a signed longs to get the > variables from userspace, not checking their range at all then writing > them to a mixture of signed and unsigned 16 bit registers. > Honestly I'd be tempted to leave that driver alone or trying and persuade > Lars-Peter to take a look at it. The datasheet is out there if you are > feeling brave. > > From a quick look, the ade7754, ade7758, ade7759, ade7854 play the same game. > > > --- > > drivers/staging/iio/accel/sca3000_core.c | 8 ++--- > > drivers/staging/iio/accel/sca3000_ring.c | 4 +-- > > drivers/staging/iio/addac/adt7316.c | 38 ++++++++++---------- > > drivers/staging/iio/frequency/ad9832.c | 4 +-- > > drivers/staging/iio/frequency/ad9834.c | 4 +-- > > drivers/staging/iio/gyro/adis16260_core.c | 4 +-- > > drivers/staging/iio/impedance-analyzer/ad5933.c | 12 +++---- > > drivers/staging/iio/light/isl29018.c | 12 +++---- > > drivers/staging/iio/light/tsl2583.c | 24 ++++++------- > > drivers/staging/iio/meter/ade7753.c | 12 +++---- > > drivers/staging/iio/meter/ade7754.c | 12 +++---- > > drivers/staging/iio/meter/ade7758_core.c | 12 +++---- > > drivers/staging/iio/meter/ade7759.c | 12 +++---- > > drivers/staging/iio/meter/ade7854.c | 16 ++++----- > > drivers/staging/iio/resolver/ad2s1210.c | 20 +++++------ > > drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 23 +++++------- > > .../staging/iio/trigger/iio-trig-periodic-rtc.c | 4 +-- > > 17 files changed, 108 insertions(+), 113 deletions(-) > > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
