RE: [PATCH v4 5/6] iio: imu: Add support for adis16475

From: "Sa, Nuno" <Nuno.Sa@analog.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-iio <linux-iio@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	Jonathan Cameron <jic23@kernel.org>,
	Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	"Ardelean, Alexandru" <alexandru.Ardelean@analog.com>,
	"Hennerich, Michael" <Michael.Hennerich@analog.com>
Subject: RE: [PATCH v4 5/6] iio: imu: Add support for adis16475
Date: Tue, 7 Apr 2020 07:26:30 +0000	[thread overview]
Message-ID: <BN6PR03MB33476ECFB3F8454CC6C67F0E99C30@BN6PR03MB3347.namprd03.prod.outlook.com> (raw)
In-Reply-To: <CAHp75VeK8K0WHK3gDwyT3ZAaMaVdfZ5xVG94JY4O8fd2U0HEHg@mail.gmail.com>

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Montag, 6. April 2020 18:20
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen
> <lars@metafoo.de>; Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>;
> Ardelean, Alexandru <alexandru.Ardelean@analog.com>; Hennerich,
> Michael <Michael.Hennerich@analog.com>
> 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@analog.com> 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