On Fri, Aug 26, 2016 at 10:37 AM, Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx> wrote: > comments from a quick read below Thanks, very good comments! >> +#define MPU3050_X_OFFS_USRH 0x0C > > should probably end in _H to match the comment above Fixed. >> +/* >> + * Fullscale precision is (for finest precision) +/- 250 deg/s, so the full >> + * scale is actually 500 deg/s. All 16 bits are then used to cover this scale, >> + * so 2^15 positive numbers and 2^15 negative numbers. > > I don't get comment regarding 16 bits; does it mean -32768 is not used? My confusion. Albeit it is not even documented, values are of course in two's complement, so -32768 to 32767 with one zero. 2^15 values, not 2^15+1. Fixing this everywhere. >> + msleep(50 + 1000/mpu3050_get_freq(mpu3050)); > > maybe space around / OK >> + struct mpu3050 *mpu3050 = iio_priv(indio_dev); >> + int ret = -EINVAL; > > I'd rather not init ret and directly > return -EINVAL instead of break OK fixed this, and found a bug. >> + case IIO_ANGL_VEL: >> + /* >> + * Convert to the corresponding full scale in >> + * radians. All 16 bits are used with sign to >> + * span the available scale. >> + */ >> + *val = mpu3050_fs_precision[mpu3050->fullscale]; >> + *val2 = S16_MAX + 1; /* 2^15 + 1 (also the zero) */ > > really 2^15 + 1 No I guess what I should be doing is a fraction like 2/(2^16) so we cover the range properly. Like so: *val = mpu3050_fs_precision[mpu3050->fullscale] * 2; *val2 = U16_MAX; >> + /* >> + * We support +/-250, +/-500, +/-1000 and +/2000 deg/s >> + * which means we need to round to the closest radians >> + * which will be roughly +/-4.3, +/-8.7, +/-17.5, +/-35 >> + * rad/s. The scale is then for the 16 bits used to cover >> + * it 1/(2^15+1) of that. > > 2^15 + 1? Fixing this too. >> + ret = regmap_bulk_read(mpu3050->map, >> + MPU3050_FIFO_R, >> + &fifo_values[offset], >> + toread); >> + >> + dev_dbg(mpu3050->dev, >> + "%04x %04x %04x %04x %04x\n", >> + fifo_values[0], > > should probably use be16_to_cpu() on the values read for debug output No here I really want to see the raw values from the FIFO in their native endianness. >> + dev_err(mpu3050->dev, "Cannot enable regulators\n"); > > most messages start lowercase in this driver OK >> + dev_err(mpu3050->dev, "error putting MPU-3050 to sleep\n"); > > necessary to mention the part name (MPU-3050) in the message? No. >> + dev_err(mpu3050->dev, "error disable MPU-3050 regulators\n"); > > disabling OK >> + ret = -ENXIO; > > ENODEV seems to be more popular in IIO OK fixed everywhere. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
