> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Montag, 6. April 2020 18:20
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: linux-iio <linux-iio@xxxxxxxxxxxxxxx>; devicetree
> <devicetree@xxxxxxxxxxxxxxx>; Jonathan Cameron <jic23@xxxxxxxxxx>;
> Hartmut Knaack <knaack.h@xxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>; Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx>; Rob
> Herring <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>;
> Ardelean, Alexandru <alexandru.Ardelean@xxxxxxxxxx>; Hennerich,
> Michael <Michael.Hennerich@xxxxxxxxxx>
> Subject: Re: [PATCH v4 5/6] iio: imu: Add support for adis16475
>
> On Mon, Apr 6, 2020 at 6:10 PM Nuno Sá <nuno.sa@xxxxxxxxxx> wrote:
> >
> > Support ADIS16475 and similar IMU devices. These devices are
> > a precision, miniature MEMS inertial measurement unit (IMU) that
> > includes a triaxial gyroscope and a triaxial accelerometer. Each
> > inertial sensor combines with signal conditioning that optimizes
> > dynamic performance.
> >
> > The driver adds support for the following devices:
> > * adis16470, adis16475, adis16477, adis16465, adis16467, adis16500,
> > adis16505, adis16507.
>
> ...
>
> > + int i = ARRAY_SIZE(adis16475_3db_freqs);
> > + int ret;
>
> > + while (--i)
> > + if (adis16475_3db_freqs[i] >= filter)
> > + break;
>
> Nit: perhaps {} to add.
>
Actually thought about that and removed them in the last minute. Will add them again...
> ...
>
> > +enum adis16475_variant {
> > + ADIS16470,
> > + ADIS16475_1,
> > + ADIS16475_2,
> > + ADIS16475_3,
> > + ADIS16477_1,
> > + ADIS16477_2,
> > + ADIS16477_3,
> > + ADIS16465_1,
> > + ADIS16465_2,
> > + ADIS16465_3,
> > + ADIS16467_1,
> > + ADIS16467_2,
> > + ADIS16467_3,
> > + ADIS16500,
> > + ADIS16505_1,
> > + ADIS16505_2,
> > + ADIS16505_3,
> > + ADIS16507_1,
> > + ADIS16507_2,
> > + ADIS16507_3,
>
> > +
>
> Extra blank line.
>
ack
> > +};
>
> ...
>
> > +static void adis16475_burst32_check(struct adis16475 *st)
> > +{
> > + int ret;
> > + struct adis *adis = &st->adis;
> > +
> > + if (!st->info->has_burst32)
> > + return;
> > +
> > + if (st->lsb_flag && !st->burst32) {
> > + const u16 en = ADIS16500_BURST32(1);
> > +
> > + ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> > + ADIS16500_BURST32_MASK, en);
> > + if (ret)
> > + return;
> > +
> > + st->burst32 = true;
>
> + Blank line.
>
Can add it. For some reason I like to save blank lines when the next lines are comments...
> > + /*
> > + * In 32bit mode we need extra 2 bytes for all gyro
>
> 32-bit
>
> > + * and accel channels.
> > + */
> > + adis->burst_extra_len = 6 * sizeof(u16);
> > + adis->xfer[1].len += 6 * sizeof(u16);
> > + dev_dbg(&adis->spi->dev, "Enable burst32 mode, xfer:%d",
> > + adis->xfer[1].len);
> > +
> > + } else if (!st->lsb_flag && st->burst32) {
> > + const u16 en = ADIS16500_BURST32(0);
> > +
> > + ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> > + ADIS16500_BURST32_MASK, en);
> > + if (ret)
> > + return;
> > +
> > + st->burst32 = false;
>
> + Blank line
>
> > + /* Remove the extra bits */
> > + adis->burst_extra_len = 0;
> > + adis->xfer[1].len -= 6 * sizeof(u16);
> > + dev_dbg(&adis->spi->dev, "Disable burst32 mode, xfer:%d\n",
> > + adis->xfer[1].len);
> > + }
> > +}
>
> ...
>
> > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > + indio_dev->masklength) {
>
> One line?
>
It goes beyond 80 col limit... I know I could initialize these to some local const but...
> > + if (st->lsb_flag && !st->info->has_burst32) {
> > + u16 val = 0;
>
> > + const u32 reg = ADIS16475_REG_X_GYRO_L +
> > + (bit * 4);
>
> Redundant parentheses.
>
Definitely...
> > + adis_read_reg_16(adis, reg, &val);
> > + data[i++] = cpu_to_be16(val);
> > + } else {
> > + /* lower not used */
> > + data[i++] = 0;
> > + }
> > + }
> > + break;
> > + }
> > + }
>
> ...
>
> > + if (sync->sync_mode == ADIS16475_SYNC_SCALED) {
> > + u16 up_scale;
> > + u32 scaled_out_freq = 0;
> > + /*
> > + * If we are in scaled mode, we must have an up_scale.
> > + * In scaled mode the allowable input clock range is
> > + * 1 Hz to 128 Hz, and the allowable output range is
> > + * 1900 to 2100 Hz. Hence, a scale must be given to
> > + * get the allowable output.
> > + */
> > + device_property_read_u32(dev, "adi,scaled-output-hz",
> > + &scaled_out_freq);
> > +
> > + if (scaled_out_freq < 1900 || scaled_out_freq > 2100) {
> > + dev_err(dev,
> > + "Invalid value:%u for adi,scaled-output-hz",
> > + scaled_out_freq);
>
> When there is no property or property has a value 0 this message can't
> tell the difference.
> Perhaps you have to check return code from device_property_read_u32()
> call.
>
Well, I think we don't really need to. If the sync mode is scaled, then this property is mandatory
(and this is stated in the bindings). So, I don't really care if the property is not there or if it's just
a wrong value. We should fail either way and I'm not sure an extra if with some other message will
give us that extra value...
> > + return -EINVAL;
> > + }
>
> > + }
>
> ...
>
> > + /*
> > + * It is possible to configure the data ready polarity. Furthermore, we
> > + * need to update the adis struct if we want data ready as active low.
> > + */
> > + irq_type = irqd_get_trigger_type(desc);
> > + if (irq_type == IRQF_TRIGGER_RISING) {
> > + polarity = 1;
> > + } else if (irq_type == IRQF_TRIGGER_FALLING) {
> > + polarity = 0;
> > + st->adis.irq_flag = IRQF_TRIGGER_FALLING;
> > + } else {
> > + dev_err(&spi->dev, "Invalid interrupt type 0x%x specified\n",
> > + irq_type);
> > + return -EINVAL;
> > + }
>
> Here is the problem. You got type, but you compare it to flags. It's
> not correct.
> Although values are the same, the meaning is different.
>
Hmm, thanks! Honestly, this was copy paste from adis16480 and I never realized this. I will
use IRQ_TYPE_EDGE_RISING and IRQ_TYPE_EDGE_FALLING. I guess we need a patch changing this
in adis16480...
Thanks,
- Nuno Sá
> --
> With Best Regards,
> Andy Shevchenko
