| Message ID | 20200331114811.7978-1-nuno.sa@analog.com |
|---|---|
| Headers | show |
| Series | Support ADIS16475 and similar IMUs | expand |
On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > There are some ADIS devices that can configure the data ready pin > polarity. Hence, we cannot hardcode our IRQ mask as IRQF_TRIGGER_RISING > since we might want to have it as IRQF_TRIGGER_FALLING. ... > +static int adis_validate_irq_mask(struct adis *adis) > +{ > + if (!adis->irq_mask) { > + adis->irq_mask = IRQF_TRIGGER_RISING; > + return 0; > + } else if (adis->irq_mask != IRQF_TRIGGER_RISING && 'else' is redundant. > + adis->irq_mask != IRQF_TRIGGER_FALLING) { But this condition rises questions. Why i can't configure both? Why I can't configure other flags there? > + dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n", > + adis->irq_mask); > + return -EINVAL; > + } > + return 0; > +} And actually name of the function is not exactly what it does. It validates *or* initializes.
On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > This patch adds a `regmap_update_bits()` like API to the ADIS library. > It provides locked and unlocked variant. > + __val &= ~mask; > + __val |= val & mask; You can use standard one liner, i.e. __val = (__val & ~mask) | (val & mask);
On Tue, Mar 31, 2020 at 2:49 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. ... > +ANALOG DEVICES INC ADIS16475 DRIVER > +M: Nuno Sa <nuno.sa@analog.com> > +L: linux-iio@vger.kernel.org > +W: http://ez.analog.com/community/linux-device-drivers > +S: Supported > +F: drivers/iio/imu/adis16475.c > +F: Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475 Run parse-maintainers.pl to fix this. ... > +#include <asm/unaligned.h> Usually it goes after linux/* > +#include <linux/bitfield.h> > +#include <linux/bitops.h> > +#include <linux/clk.h> > +#include <linux/debugfs.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/kernel.h> What this is for? > +#include <linux/iio/buffer.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/imu/adis.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/irq.h> > +#include <linux/module.h> > +#include <linux/of_device.h> Do you really need this? Perhaps mod_devicetable.h is what you are looking for. > +#include <linux/spi/spi.h> ... > +#ifdef CONFIG_DEBUG_FS > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_serial_number_fops, > + adis16475_show_serial_number, NULL, "0x%.4llx\n"); DEBUGFS ATTRIBUTE? > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_product_id_fops, > + adis16475_show_product_id, NULL, "%llu\n"); > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_flash_count_fops, > + adis16475_show_flash_count, NULL, "%lld\n"); Ditto. > +#else > +static int adis16475_debugfs_init(struct iio_dev *indio_dev) > +{ > + return 0; > +} > +#endif ... > + /* > + * If decimation is used, then gyro and accel data will have meaningful > + * bits on the LSB registers. This info is used on the trigger handler. > + */ > + if (!dec) > + clear_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag); > + else > + set_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag); assign_bit() Also to the rest of same ... > + for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) { Why those margins? size-2 and 1 ? > + if (adis16475_3db_freqs[i] >= filter) > + break; > + } ... > +#define ADIS16475_GYRO_CHANNEL(_mod) \ > + ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \ > + ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ ## _mod, 32, \ > + 32) It's not obvious that this is macro inside macro. Can you indent better? Ditto for the rest similar ones. ... > +static int adis16475_enable_irq(struct adis *adis, bool enable) > +{ > + /* > + * There is no way to gate the data-ready signal internally inside the > + * ADIS16475. We can only control it's polarity... > + */ > + if (enable) > + enable_irq(adis->spi->irq); > + else > + disable_irq(adis->spi->irq); > + > + return 0; > +} It's seems this function is bigger than in-place calls for enable or disable IRQ. ... > + return (crc == 0); Too many parentheses. ... > + ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL, > + ADIS16500_BURST32_MASK, en); > + if (ret < 0) ret > 0 has any meaning? Maybe drop all these ' < 0' parts from the code (where it's appropriate)? > + return; ... > + buffer = (u16 *)adis->buffer; Why the casting is needed? > + crc = get_unaligned_be16(&buffer[offset + 2]); If your buffer is aligned in the structure, you may simple use be16_to_cpu(). Same for the rest of get_unaligned*() calls. Or do you have unaligned data there? > + valid = adis16475_validate_crc((u8 *)adis->buffer, crc, st->burst32); Why casting? > + if (!valid) { > + dev_err(&adis->spi->dev, "Invalid crc\n"); > + goto check_burst32; > + } ... > + /* keep sparse happy */ Perhaps buffer should be declared as __be16. > + data[i++] = (__force u16)__val; ... > + desc = irq_get_irq_data(spi->irq); > + if (!desc) { > + dev_err(&spi->dev, "Could not find IRQ %d\n", spi->irq); > + return -EINVAL; > + } Is this even possible? ... > + { .compatible = "adi,adis16507-3", > + .data = &adis16475_chip_info[ADIS16507_3] }, > + { }, Comma is not needed. ... > + st->info = of_device_get_match_data(&spi->dev); device_get_match_data() > + if (!st->info) > + return -EINVAL;
> From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Dienstag, 31. März 2020 20:16 > 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 v3 5/6] iio: imu: Add support for adis16475 > > [External] > > On Tue, Mar 31, 2020 at 2:49 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. > > ... > > > +ANALOG DEVICES INC ADIS16475 DRIVER > > +M: Nuno Sa <nuno.sa@analog.com> > > +L: linux-iio@vger.kernel.org > > +W: http://ez.analog.com/community/linux-device-drivers > > +S: Supported > > +F: drivers/iio/imu/adis16475.c > > +F: Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475 > > Run parse-maintainers.pl to fix this. > Ups. Leftover from v1. Thanks! > ... > > > +#include <asm/unaligned.h> > I thought we wanted alphabetic order... > Usually it goes after linux/* > > > +#include <linux/bitfield.h> > > +#include <linux/bitops.h> > > +#include <linux/clk.h> > > +#include <linux/debugfs.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > > +#include <linux/kernel.h> > > What this is for? > Yeps. Not really needed... > > +#include <linux/iio/buffer.h> > > +#include <linux/iio/iio.h> > > +#include <linux/iio/imu/adis.h> > > +#include <linux/iio/sysfs.h> > > +#include <linux/iio/trigger_consumer.h> > > +#include <linux/irq.h> > > +#include <linux/module.h> > > > +#include <linux/of_device.h> > > Do you really need this? Perhaps mod_devicetable.h is what you are looking > for. > Yes. For ` of_device_get_match_data ``. If changed by `device_get_match_data`, then I guess I can drop it.. > > +#include <linux/spi/spi.h> > > ... > > > +#ifdef CONFIG_DEBUG_FS > > > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_serial_number_fops, > > + adis16475_show_serial_number, NULL, "0x%.4llx\n"); > > DEBUGFS ATTRIBUTE? > > > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_product_id_fops, > > + adis16475_show_product_id, NULL, "%llu\n"); > > > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_flash_count_fops, > > + adis16475_show_flash_count, NULL, "%lld\n"); > > Ditto. > > > +#else > > +static int adis16475_debugfs_init(struct iio_dev *indio_dev) > > +{ > > + return 0; > > +} > > +#endif > > ... > > > + /* > > + * If decimation is used, then gyro and accel data will have meaningful > > + * bits on the LSB registers. This info is used on the trigger handler. > > + */ > > + if (!dec) > > + clear_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag); > > + else > > + set_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag); > > assign_bit() > Will do that... > Also to the rest of same > > ... > > > + for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) { > > Why those margins? size-2 and 1 ? > The -2 is needed since index 7 is not valid. The 1 I honestly don't remember why I did it like this. Using > 0 is the same and more "common"... > > + if (adis16475_3db_freqs[i] >= filter) > > + break; > > + } > > ... > > > +#define ADIS16475_GYRO_CHANNEL(_mod) \ > > + ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \ > > + ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ ## > _mod, 32, \ > > + 32) > > It's not obvious that this is macro inside macro. Can you indent better? > Ditto for the rest similar ones. > Honestly here I don't see any problems with indentation and it goes in conformity with other IMU drivers already in tree. So here, as long as anyone else has a problem with this, I prefer to keep it this way... > ... > > > +static int adis16475_enable_irq(struct adis *adis, bool enable) > > +{ > > + /* > > + * There is no way to gate the data-ready signal internally inside the > > + * ADIS16475. We can only control it's polarity... > > + */ > > + if (enable) > > + enable_irq(adis->spi->irq); > > + else > > + disable_irq(adis->spi->irq); > > + > > + return 0; > > +} > > It's seems this function is bigger than in-place calls for enable or > disable IRQ. > This api is callbacked from the ADIS library... > ... > > > + return (crc == 0); > > Too many parentheses. > > ... > > > + ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL, > > + ADIS16500_BURST32_MASK, en); > > + if (ret < 0) > > ret > 0 has any meaning? Maybe drop all these ' < 0' parts from the > code (where it's appropriate)? > > > + return; > > ... > > > + buffer = (u16 *)adis->buffer; > > Why the casting is needed? > > > + crc = get_unaligned_be16(&buffer[offset + 2]); > > If your buffer is aligned in the structure, you may simple use be16_to_cpu(). > Same for the rest of get_unaligned*() calls. > Or do you have unaligned data there? This is a nice point. So, honestly I made it like this to keep conformity with other drivers we have in our internal tree (in queue for upstream) and I also wondered about this. The only justification I can find to use unligned calls is to keep this independent from the ADIS lib (not sure if it makes sense) since we get the pointer from the library (allocated there). Now, if Im not missing nothing obvious we can access the buffer normally since it's being allocated with kmalloc which means we have ARCH_KMALLOC_MINALIGN (which is at least 8 if Im not mistaken). On top of this, the device sends the data as n 16 bits segments. So in theory, I guess we can ditch the overhead of the *unaligned calls if any objections? > > + valid = adis16475_validate_crc((u8 *)adis->buffer, crc, st->burst32); > > Why casting? > Not really needed... > > + if (!valid) { > > + dev_err(&adis->spi->dev, "Invalid crc\n"); > > + goto check_burst32; > > + } > > ... > > > + /* keep sparse happy */ > > Perhaps buffer should be declared as __be16. > Will do it if removing the *unaligned calls... > > + data[i++] = (__force u16)__val; > > ... > > > > + desc = irq_get_irq_data(spi->irq); > > > + if (!desc) { > > + dev_err(&spi->dev, "Could not find IRQ %d\n", spi->irq); > > + return -EINVAL; > > + } > > Is this even possible? > I guess. If someone does not include it in device tree... > ... > > > + { .compatible = "adi,adis16507-3", > > + .data = &adis16475_chip_info[ADIS16507_3] }, > > + { }, > > Comma is not needed. > Will remove it > ... > > > + st->info = of_device_get_match_data(&spi->dev); > > device_get_match_data() Will use it... > > > + if (!st->info) > > + return -EINVAL; > Thanks for the review - Nuno Sá > -- > With Best Regards, > Andy Shevchenko
> From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Dienstag, 31. März 2020 19:42 > 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 v3 3/6] iio: adis: Add adis_update_bits() APIs > > [External] > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > This patch adds a `regmap_update_bits()` like API to the ADIS library. > > It provides locked and unlocked variant. > > > + __val &= ~mask; > > + __val |= val & mask; > > You can use standard one liner, i.e. > > __val = (__val & ~mask) | (val & mask); Sure. Can do that... - Nuno Sá > -- > With Best Regards, > Andy Shevchenko
> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On > Behalf Of Andy Shevchenko > Sent: Dienstag, 31. März 2020 19:40 > 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 v3 2/6] iio: imu: adis: Add irq mask variable > > [External] > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > There are some ADIS devices that can configure the data ready pin > > polarity. Hence, we cannot hardcode our IRQ mask as > IRQF_TRIGGER_RISING > > since we might want to have it as IRQF_TRIGGER_FALLING. > > ... > > > +static int adis_validate_irq_mask(struct adis *adis) > > +{ > > + if (!adis->irq_mask) { > > + adis->irq_mask = IRQF_TRIGGER_RISING; > > + return 0; > > > + } else if (adis->irq_mask != IRQF_TRIGGER_RISING && > > 'else' is redundant. > > > + adis->irq_mask != IRQF_TRIGGER_FALLING) { > > But this condition rises questions. Why i can't configure both? > Why I can't configure other flags there? Both you can't because it is just how these type of devices work. Data is ready either on the rising edge or on the falling edge (if supported by the device)... I agree this could check if only one of the flags are set instead of directly comparing the values (invalidating other flags) but I would prefer to keep this simple for now... > > > + dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n", > > + adis->irq_mask); > > + return -EINVAL; > > + } > > > + return 0; > > +} > > And actually name of the function is not exactly what it does. It > validates *or* initializes. Well, yes. It just sets the mask to the default value to keep backward compatibility with all the other devices that don't support/use this variable... - Nuno Sá > > -- > With Best Regards, > Andy Shevchenko
On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Dienstag, 31. März 2020 20:16 > > 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 v3 5/6] iio: imu: Add support for adis16475 > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: ... > > > +#include <asm/unaligned.h> > I thought we wanted alphabetic order... Yes, but from more generic header groups to less generic. Inside each group is alphabetical. asm/ is less generic than linux/. > > Usually it goes after linux/* > > > +#include <linux/bitfield.h> > > > +#include <linux/bitops.h> > > > +#include <linux/clk.h> > > > +#include <linux/debugfs.h> > > > +#include <linux/delay.h> > > > +#include <linux/device.h> > > > > > +#include <linux/kernel.h> > > > > What this is for? > > > Yeps. Not really needed... I think you needed it for DIV_ROUND_UP or alike macros. It also has container_of... > > > +#include <linux/iio/buffer.h> > > > +#include <linux/iio/iio.h> > > > +#include <linux/iio/imu/adis.h> > > > +#include <linux/iio/sysfs.h> > > > +#include <linux/iio/trigger_consumer.h> > > > +#include <linux/irq.h> > > > +#include <linux/module.h> > > > > > +#include <linux/of_device.h> > > > > Do you really need this? Perhaps mod_devicetable.h is what you are looking > > for. > > > > Yes. For ` of_device_get_match_data ``. If changed by `device_get_match_data`, then I guess > I can drop it.. Probably change to mod_devicetable.h with property.h. > > > +#include <linux/spi/spi.h> ... > > > + for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) { > > > > Why those margins? size-2 and 1 ? > > > > The -2 is needed since index 7 is not valid. The 1 I honestly don't remember why I did it > like this. Using > 0 is the same and more "common"... More common is >= 0. That's my question basically. And if 7 is not valid why to keep it in the array at all? > > > + if (adis16475_3db_freqs[i] >= filter) > > > + break; > > > + } ... > > > +#define ADIS16475_GYRO_CHANNEL(_mod) \ > > > + ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \ > > > + ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ ## > > _mod, 32, \ > > > + 32) > > > > It's not obvious that this is macro inside macro. Can you indent better? > > Ditto for the rest similar ones. > > > > Honestly here I don't see any problems with indentation and it goes in conformity with > other IMU drivers already in tree. So here, as long as anyone else has a problem with this, I prefer > to keep it this way... I'm not a maintainer, not my call :-) ... > > > + buffer = (u16 *)adis->buffer; > > > > Why the casting is needed? > > > > > + crc = get_unaligned_be16(&buffer[offset + 2]); > > > > If your buffer is aligned in the structure, you may simple use be16_to_cpu(). > > Same for the rest of get_unaligned*() calls. > > Or do you have unaligned data there? > > This is a nice point. So, honestly I made it like this to keep conformity with other drivers we have > in our internal tree (in queue for upstream) and I also wondered about this. The only justification I can > find to use unligned calls is to keep this independent from the ADIS lib (not sure if it makes sense) since > we get the pointer from the library (allocated there). > > Now, if Im not missing nothing obvious we can access the buffer normally since it's being allocated > with kmalloc which means we have ARCH_KMALLOC_MINALIGN (which is at least 8 if Im not mistaken). > On top of this, the device sends the data as n 16 bits segments. So in theory, I guess we can ditch the > overhead of the *unaligned calls if any objections? No objections from my side at least. ... > > > + desc = irq_get_irq_data(spi->irq); > > > > > + if (!desc) { > > > + dev_err(&spi->dev, "Could not find IRQ %d\n", spi->irq); > > > + return -EINVAL; > > > + } > > > > Is this even possible? > I guess. If someone does not include it in device tree... Hmm... and this function will be called anyway?
On Wed, Apr 1, 2020 at 10:22 AM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On > > Behalf Of Andy Shevchenko > > Sent: Dienstag, 31. März 2020 19:40 > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > > > There are some ADIS devices that can configure the data ready pin > > > polarity. Hence, we cannot hardcode our IRQ mask as > > IRQF_TRIGGER_RISING > > > since we might want to have it as IRQF_TRIGGER_FALLING. ... > > > +static int adis_validate_irq_mask(struct adis *adis) > > > +{ > > > + if (!adis->irq_mask) { > > > + adis->irq_mask = IRQF_TRIGGER_RISING; > > > + return 0; > > > > > + } else if (adis->irq_mask != IRQF_TRIGGER_RISING && > > > > 'else' is redundant. > > > > > + adis->irq_mask != IRQF_TRIGGER_FALLING) { > > > > But this condition rises questions. Why i can't configure both? > > Why I can't configure other flags there? > > Both you can't because it is just how these type of devices work. Data is ready either > on the rising edge or on the falling edge (if supported by the device)... > I agree this could check if only one of the flags are set instead of directly comparing the > values (invalidating other flags) but I would prefer to keep this simple for now... > > > > > > + dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n", > > > + adis->irq_mask); > > > + return -EINVAL; > > > + } > > > > > + return 0; > > > +} > > > > And actually name of the function is not exactly what it does. It > > validates *or* initializes. > > Well, yes. It just sets the mask to the default value to keep backward compatibility > with all the other devices that don't support/use this variable... Perhaps documentation in a comment form should be added. Moreover, I realized that you added to variable and function mask suffix while it's actually flag.
> From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Mittwoch, 1. April 2020 12:27 > 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 v3 2/6] iio: imu: adis: Add irq mask variable > > On Wed, Apr 1, 2020 at 10:22 AM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> > On > > > Behalf Of Andy Shevchenko > > > Sent: Dienstag, 31. März 2020 19:40 > > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > > > > > There are some ADIS devices that can configure the data ready pin > > > > polarity. Hence, we cannot hardcode our IRQ mask as > > > IRQF_TRIGGER_RISING > > > > since we might want to have it as IRQF_TRIGGER_FALLING. > > ... > > > > > +static int adis_validate_irq_mask(struct adis *adis) > > > > +{ > > > > + if (!adis->irq_mask) { > > > > + adis->irq_mask = IRQF_TRIGGER_RISING; > > > > + return 0; > > > > > > > + } else if (adis->irq_mask != IRQF_TRIGGER_RISING && > > > > > > 'else' is redundant. > > > > > > > + adis->irq_mask != IRQF_TRIGGER_FALLING) { > > > > > > But this condition rises questions. Why i can't configure both? > > > Why I can't configure other flags there? > > > > Both you can't because it is just how these type of devices work. Data is > ready either > > on the rising edge or on the falling edge (if supported by the device)... > > I agree this could check if only one of the flags are set instead of directly > comparing the > > values (invalidating other flags) but I would prefer to keep this simple for > now... > > > > > > > > > + dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n", > > > > + adis->irq_mask); > > > > + return -EINVAL; > > > > + } > > > > > > > + return 0; > > > > +} > > > > > > And actually name of the function is not exactly what it does. It > > > validates *or* initializes. > > > > Well, yes. It just sets the mask to the default value to keep backward > compatibility > > with all the other devices that don't support/use this variable... > > Perhaps documentation in a comment form should be added. > Moreover, I realized that you added to variable and function mask > suffix while it's actually flag. Will change the suffix to flag... - Nuno Sá > -- > With Best Regards, > Andy Shevchenko
> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On > Behalf Of Andy Shevchenko > Sent: Mittwoch, 1. April 2020 12:23 > 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 v3 5/6] iio: imu: Add support for adis16475 > > On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Sent: Dienstag, 31. März 2020 20:16 > > > 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 v3 5/6] iio: imu: Add support for adis16475 > > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > ... > > > > > +#include <asm/unaligned.h> > > > I thought we wanted alphabetic order... > > Yes, but from more generic header groups to less generic. Inside each > group is alphabetical. > asm/ is less generic than linux/. > Got it... > > > Usually it goes after linux/* > > > > > +#include <linux/bitfield.h> > > > > +#include <linux/bitops.h> > > > > +#include <linux/clk.h> > > > > +#include <linux/debugfs.h> > > > > +#include <linux/delay.h> > > > > +#include <linux/device.h> > > > > > > > +#include <linux/kernel.h> > > > > > > What this is for? > > > > > Yeps. Not really needed... > > I think you needed it for DIV_ROUND_UP or alike macros. It also has > container_of... > Yes, DIV_ROUND_CLOSEST is defined there... > > > > +#include <linux/iio/buffer.h> > > > > +#include <linux/iio/iio.h> > > > > +#include <linux/iio/imu/adis.h> > > > > +#include <linux/iio/sysfs.h> > > > > +#include <linux/iio/trigger_consumer.h> > > > > +#include <linux/irq.h> > > > > +#include <linux/module.h> > > > > > > > +#include <linux/of_device.h> > > > > > > Do you really need this? Perhaps mod_devicetable.h is what you are > looking > > > for. > > > > > > > Yes. For ` of_device_get_match_data ``. If changed by > `device_get_match_data`, then I guess > > I can drop it.. > > Probably change to mod_devicetable.h with property.h. > > > > > +#include <linux/spi/spi.h> > > ... > > > > > + for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) { > > > > > > Why those margins? size-2 and 1 ? > > > > > > > The -2 is needed since index 7 is not valid. The 1 I honestly don't remember > why I did it > > like this. Using > 0 is the same and more "common"... > > More common is >= 0. That's my question basically. > And if 7 is not valid why to keep it in the array at all? Well, I can remove the 7. I honestly took it from another driver and I guess the idea is to make explicit that 7 is not supported. Since this is a 3 bit field and the datasheet does not state this directly. As for the >=0, I prefer to have either as is or >0 since we don't really need to check the index 0. If 1 fails, then we will use 0 either way... > > > > + if (adis16475_3db_freqs[i] >= filter) > > > > + break; > > > > + } > > ... > > > > > +#define ADIS16475_GYRO_CHANNEL(_mod) \ > > > > + ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \ > > > > + ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ > ## > > > _mod, 32, \ > > > > + 32) > > > > > > It's not obvious that this is macro inside macro. Can you indent better? > > > Ditto for the rest similar ones. > > > > > > > Honestly here I don't see any problems with indentation and it goes in > conformity with > > other IMU drivers already in tree. So here, as long as anyone else has a > problem with this, I prefer > > to keep it this way... > > I'm not a maintainer, not my call :-) > > ... > > > > > + buffer = (u16 *)adis->buffer; > > > > > > Why the casting is needed? > > > > > > > + crc = get_unaligned_be16(&buffer[offset + 2]); > > > > > > If your buffer is aligned in the structure, you may simple use > be16_to_cpu(). > > > Same for the rest of get_unaligned*() calls. > > > Or do you have unaligned data there? > > > > This is a nice point. So, honestly I made it like this to keep conformity with > other drivers we have > > in our internal tree (in queue for upstream) and I also wondered about this. > The only justification I can > > find to use unligned calls is to keep this independent from the ADIS lib (not > sure if it makes sense) since > > we get the pointer from the library (allocated there). > > > > Now, if Im not missing nothing obvious we can access the buffer normally > since it's being allocated > > with kmalloc which means we have ARCH_KMALLOC_MINALIGN (which is > at least 8 if Im not mistaken). > > On top of this, the device sends the data as n 16 bits segments. So in theory, > I guess we can ditch the > > overhead of the *unaligned calls if any objections? > > No objections from my side at least. > I will wait to see if someone else has anything to add and if not, I will change it to normal buffer accesses (probably using restricted types). > ... > > > > > + desc = irq_get_irq_data(spi->irq); > > > > > > > + if (!desc) { > > > > + dev_err(&spi->dev, "Could not find IRQ %d\n", spi->irq); > > > > + return -EINVAL; > > > > + } > > > > > > Is this even possible? > > > I guess. If someone does not include it in device tree... > > Hmm... and this function will be called anyway? > Yes, it is called on probe... And we should fail if no interrupt is given in device tree. It’s a required property. - Nuno Sá > -- > With Best Regards, > Andy Shevchenko
On Wed, Apr 1, 2020 at 4:27 PM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: ... > > > > > + for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) { > > > > > > > > Why those margins? size-2 and 1 ? > > > > > > > > > > The -2 is needed since index 7 is not valid. The 1 I honestly don't remember > > why I did it > > > like this. Using > 0 is the same and more "common"... > > > > More common is >= 0. That's my question basically. > > And if 7 is not valid why to keep it in the array at all? > > Well, I can remove the 7. I honestly took it from another driver and I guess the idea > is to make explicit that 7 is not supported. Since this is a 3 bit field and the datasheet > does not state this directly. > > As for the >=0, I prefer to have either as is or >0 since we don't really need to check the > index 0. If 1 fails, then we will use 0 either way... It makes sense to check to get better optimization (and increased readability). Look for this i = ARRAY_SIZE(...); while (i--) { ... } It's much better to read and understand. > > > > > + if (adis16475_3db_freqs[i] >= filter) > > > > > + break; > > > > > + }
> From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Mittwoch, 1. April 2020 16:06 > 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 v3 5/6] iio: imu: Add support for adis16475 > > On Wed, Apr 1, 2020 at 4:27 PM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com> > wrote: > > > > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> > wrote: > > ... > > > > > > > + for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) { > > > > > > > > > > Why those margins? size-2 and 1 ? > > > > > > > > > > > > > The -2 is needed since index 7 is not valid. The 1 I honestly don't > remember > > > why I did it > > > > like this. Using > 0 is the same and more "common"... > > > > > > More common is >= 0. That's my question basically. > > > And if 7 is not valid why to keep it in the array at all? > > > > Well, I can remove the 7. I honestly took it from another driver and I guess > the idea > > is to make explicit that 7 is not supported. Since this is a 3 bit field and the > datasheet > > does not state this directly. > > > > As for the >=0, I prefer to have either as is or >0 since we don't really need to > check the > > index 0. If 1 fails, then we will use 0 either way... > > It makes sense to check to get better optimization (and increased readability). > Look for this > > i = ARRAY_SIZE(...); > > while (i--) { > ... > } > > It's much better to read and understand. Well, about the readability it's a bit subjective 😊. It depends who is reading the code. Just curious, how would you get better optimization by doing >=0 instead of > 0? Either way, I don’t have any strong feeling about this so I can do as you suggest. - Nuno Sá > > > > > > + if (adis16475_3db_freqs[i] >= filter) > > > > > > + break; > > > > > > + } > > -- > With Best Regards, > Andy Shevchenko
On Wed, Apr 1, 2020 at 5:18 PM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Mittwoch, 1. April 2020 16:06 > > 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 v3 5/6] iio: imu: Add support for adis16475 > > > > On Wed, Apr 1, 2020 at 4:27 PM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > > On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com> > > wrote: > > > > > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> > > wrote: ... > > > Well, I can remove the 7. I honestly took it from another driver and I guess > > the idea > > > is to make explicit that 7 is not supported. Since this is a 3 bit field and the > > datasheet > > > does not state this directly. > > > > > > As for the >=0, I prefer to have either as is or >0 since we don't really need to > > check the > > > index 0. If 1 fails, then we will use 0 either way... > > > > It makes sense to check to get better optimization (and increased readability). > > Look for this > > > > i = ARRAY_SIZE(...); > > > > while (i--) { > > ... > > } > > > > It's much better to read and understand. > > Well, about the readability it's a bit subjective . It depends who is > reading the code. Just curious, how would you get better optimization > by doing >=0 instead of > 0? Feel the difference while (i--) vs. while (--i > 0). The latter one is a bit unusual. > Either way, I don’t have any strong feeling about this so I can do as > you suggest.
... > > > > > +#define ADIS16475_GYRO_CHANNEL(_mod) \ > > > > + ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \ > > > > + ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ ## > > > _mod, 32, \ > > > > + 32) > > > > > > It's not obvious that this is macro inside macro. Can you indent better? > > > Ditto for the rest similar ones. > > > > > > > Honestly here I don't see any problems with indentation and it goes in conformity with > > other IMU drivers already in tree. So here, as long as anyone else has a problem with this, I prefer > > to keep it this way... > > I'm not a maintainer, not my call :-) I'm lazy when it comes to things like this. Yes it could be better, but it's still fairly readable so I don't really mind. That's not to say I don't like beautiful things if you don't mind tidying it up? :) Jonathan
On Wed, 1 Apr 2020 13:27:31 +0000 "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On > > Behalf Of Andy Shevchenko > > Sent: Mittwoch, 1. April 2020 12:23 > > 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 v3 5/6] iio: imu: Add support for adis16475 > > > > On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > > > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > Sent: Dienstag, 31. März 2020 20:16 > > > > 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 v3 5/6] iio: imu: Add support for adis16475 > > > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > ... > > > > > > > +#include <asm/unaligned.h> > > > > > I thought we wanted alphabetic order... > > > > Yes, but from more generic header groups to less generic. Inside each > > group is alphabetical. > > asm/ is less generic than linux/. > > > > Got it... > > > > > Usually it goes after linux/* > > > > > > > +#include <linux/bitfield.h> > > > > > +#include <linux/bitops.h> > > > > > +#include <linux/clk.h> > > > > > +#include <linux/debugfs.h> > > > > > +#include <linux/delay.h> > > > > > +#include <linux/device.h> > > > > > > > > > +#include <linux/kernel.h> > > > > > > > > What this is for? > > > > > > > Yeps. Not really needed... > > > > I think you needed it for DIV_ROUND_UP or alike macros. It also has > > container_of... > > > > Yes, DIV_ROUND_CLOSEST is defined there... > > > > > > +#include <linux/iio/buffer.h> > > > > > +#include <linux/iio/iio.h> > > > > > +#include <linux/iio/imu/adis.h> > > > > > +#include <linux/iio/sysfs.h> > > > > > +#include <linux/iio/trigger_consumer.h> > > > > > +#include <linux/irq.h> > > > > > +#include <linux/module.h> > > > > > > > > > +#include <linux/of_device.h> > > > > > > > > Do you really need this? Perhaps mod_devicetable.h is what you are > > looking > > > > for. > > > > > > > > > > Yes. For ` of_device_get_match_data ``. If changed by > > `device_get_match_data`, then I guess > > > I can drop it.. > > > > Probably change to mod_devicetable.h with property.h. > > > > > > > +#include <linux/spi/spi.h> > > > > ... > > > > > > > + for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) { > > > > > > > > Why those margins? size-2 and 1 ? > > > > > > > > > > The -2 is needed since index 7 is not valid. The 1 I honestly don't remember > > why I did it > > > like this. Using > 0 is the same and more "common"... > > > > More common is >= 0. That's my question basically. > > And if 7 is not valid why to keep it in the array at all? > > Well, I can remove the 7. I honestly took it from another driver and I guess the idea > is to make explicit that 7 is not supported. Since this is a 3 bit field and the datasheet > does not state this directly. > > As for the >=0, I prefer to have either as is or >0 since we don't really need to check the > index 0. If 1 fails, then we will use 0 either way... > > > > > > + if (adis16475_3db_freqs[i] >= filter) > > > > > + break; > > > > > + } > > > > ... > > > > > > > +#define ADIS16475_GYRO_CHANNEL(_mod) \ > > > > > + ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \ > > > > > + ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ > > ## > > > > _mod, 32, \ > > > > > + 32) > > > > > > > > It's not obvious that this is macro inside macro. Can you indent better? > > > > Ditto for the rest similar ones. > > > > > > > > > > Honestly here I don't see any problems with indentation and it goes in > > conformity with > > > other IMU drivers already in tree. So here, as long as anyone else has a > > problem with this, I prefer > > > to keep it this way... > > > > I'm not a maintainer, not my call :-) > > > > ... > > > > > > > + buffer = (u16 *)adis->buffer; > > > > > > > > Why the casting is needed? > > > > > > > > > + crc = get_unaligned_be16(&buffer[offset + 2]); > > > > > > > > If your buffer is aligned in the structure, you may simple use > > be16_to_cpu(). > > > > Same for the rest of get_unaligned*() calls. > > > > Or do you have unaligned data there? > > > > > > This is a nice point. So, honestly I made it like this to keep conformity with > > other drivers we have > > > in our internal tree (in queue for upstream) and I also wondered about this. > > The only justification I can > > > find to use unligned calls is to keep this independent from the ADIS lib (not > > sure if it makes sense) since > > > we get the pointer from the library (allocated there). It would be very odd to get a buffer from a library dealing with this sort of device that wanted at least 8 byte aligned. I guess we could add a paranoid check in the driver, but I think we can safely assume this is fine without one. > > > > > > Now, if Im not missing nothing obvious we can access the buffer normally > > since it's being allocated > > > with kmalloc which means we have ARCH_KMALLOC_MINALIGN (which is > > at least 8 if Im not mistaken). > > > On top of this, the device sends the data as n 16 bits segments. So in theory, > > I guess we can ditch the > > > overhead of the *unaligned calls if any objections? > > > > No objections from my side at least. > > > > I will wait to see if someone else has anything to add and if not, I will change it > to normal buffer accesses (probably using restricted types). > If it's aligned, definitely prefer that to be explicit in the driver and use the relevant endian types. We have had a few cases where things are oddly padded so this may be cut and paste from one of those. Jonathan
On Tue, 31 Mar 2020 13:48:06 +0200 Nuno Sá <nuno.sa@analog.com> wrote: > This patch adds support for a managed device version of > adis_setup_buffer_and_trigger. It works exactly as the original > one but it calls all the devm_iio_* functions to setup an iio > buffer and trigger. Hence we do not need to care about cleaning those > and we do not need to support a remove() callback for every driver using > the adis library. > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> Random thought inline. Something we could use more in IIO :) > --- > Changes in v2: > * Added blank lines for readability. > > Changes in V3: > * Removed unnecessary inline; > * Free buffer resources. > > drivers/iio/imu/adis_buffer.c | 45 ++++++++++++++++++++++++++++++++++ > drivers/iio/imu/adis_trigger.c | 41 ++++++++++++++++++++++++++++--- > include/linux/iio/imu/adis.h | 17 +++++++++++++ > 3 files changed, 100 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c > index 04e5e2a0fd6b..c2211ab80d8c 100644 > --- a/drivers/iio/imu/adis_buffer.c > +++ b/drivers/iio/imu/adis_buffer.c > @@ -156,6 +156,14 @@ static irqreturn_t adis_trigger_handler(int irq, void *p) > return IRQ_HANDLED; > } > > +static void adis_buffer_cleanup(void *arg) > +{ > + struct adis *adis = arg; > + > + kfree(adis->buffer); > + kfree(adis->xfer); > +} > + > /** > * adis_setup_buffer_and_trigger() - Sets up buffer and trigger for the adis device > * @adis: The adis device. > @@ -198,6 +206,43 @@ int adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev, > } > EXPORT_SYMBOL_GPL(adis_setup_buffer_and_trigger); > > +/** > + * devm_adis_setup_buffer_and_trigger() - Sets up buffer and trigger for > + * the managed adis device > + * @adis: The adis device > + * @indio_dev: The IIO device > + * @trigger_handler: Optional trigger handler, may be NULL. > + * > + * Returns 0 on success, a negative error code otherwise. > + * > + * This function perfoms exactly the same as adis_setup_buffer_and_trigger() > + */ > +int > +devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev, > + irqreturn_t (*trigger_handler)(int, void *)) It occurred to me that there must be a lot of irq handling function pointers in the kernel and it would be odd if there wasn't a type for this... There is :) irq_handler_t https://elixir.bootlin.com/linux/latest/source/include/linux/interrupt.h#L92 Not sure why I never noticed that before. Hohum. Jonathan > +{ > + int ret; > + > + if (!trigger_handler) > + trigger_handler = adis_trigger_handler; > + > + ret = devm_iio_triggered_buffer_setup(&adis->spi->dev, indio_dev, > + &iio_pollfunc_store_time, > + trigger_handler, NULL); > + if (ret) > + return ret; > + > + if (adis->spi->irq) { > + ret = devm_adis_probe_trigger(adis, indio_dev); > + if (ret) > + return ret; > + } > + > + return devm_add_action_or_reset(&adis->spi->dev, adis_buffer_cleanup, > + adis); > +} > +EXPORT_SYMBOL_GPL(devm_adis_setup_buffer_and_trigger); > + > /** > * adis_cleanup_buffer_and_trigger() - Free buffer and trigger resources > * @adis: The adis device. > diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c > index 8b9cd02c0f9f..a36810e0b1ab 100644 > --- a/drivers/iio/imu/adis_trigger.c > +++ b/drivers/iio/imu/adis_trigger.c > @@ -27,6 +27,13 @@ static const struct iio_trigger_ops adis_trigger_ops = { > .set_trigger_state = &adis_data_rdy_trigger_set_state, > }; > > +static void adis_trigger_setup(struct adis *adis) > +{ > + adis->trig->dev.parent = &adis->spi->dev; > + adis->trig->ops = &adis_trigger_ops; > + iio_trigger_set_drvdata(adis->trig, adis); > +} > + > /** > * adis_probe_trigger() - Sets up trigger for a adis device > * @adis: The adis device > @@ -45,9 +52,7 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev) > if (adis->trig == NULL) > return -ENOMEM; > > - adis->trig->dev.parent = &adis->spi->dev; > - adis->trig->ops = &adis_trigger_ops; > - iio_trigger_set_drvdata(adis->trig, adis); > + adis_trigger_setup(adis); > > ret = request_irq(adis->spi->irq, > &iio_trigger_generic_data_rdy_poll, > @@ -73,6 +78,36 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev) > } > EXPORT_SYMBOL_GPL(adis_probe_trigger); > > +/** > + * devm_adis_probe_trigger() - Sets up trigger for a managed adis device > + * @adis: The adis device > + * @indio_dev: The IIO device > + * > + * Returns 0 on success or a negative error code > + */ > +int devm_adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev) > +{ > + int ret; > + > + adis->trig = devm_iio_trigger_alloc(&adis->spi->dev, "%s-dev%d", > + indio_dev->name, indio_dev->id); > + if (!adis->trig) > + return -ENOMEM; > + > + adis_trigger_setup(adis); > + > + ret = devm_request_irq(&adis->spi->dev, adis->spi->irq, > + &iio_trigger_generic_data_rdy_poll, > + IRQF_TRIGGER_RISING, > + indio_dev->name, > + adis->trig); > + if (ret) > + return ret; > + > + return devm_iio_trigger_register(&adis->spi->dev, adis->trig); > +} > +EXPORT_SYMBOL_GPL(devm_adis_probe_trigger); > + > /** > * adis_remove_trigger() - Remove trigger for a adis devices > * @adis: The adis device > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h > index dd8219138c2e..ac94c483bf2b 100644 > --- a/include/linux/iio/imu/adis.h > +++ b/include/linux/iio/imu/adis.h > @@ -448,11 +448,15 @@ struct adis_burst { > unsigned int extra_len; > }; > > +int > +devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev, > + irqreturn_t (*trigger_handler)(int, void *)); > int adis_setup_buffer_and_trigger(struct adis *adis, > struct iio_dev *indio_dev, irqreturn_t (*trigger_handler)(int, void *)); > void adis_cleanup_buffer_and_trigger(struct adis *adis, > struct iio_dev *indio_dev); > > +int devm_adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev); > int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev); > void adis_remove_trigger(struct adis *adis); > > @@ -461,6 +465,13 @@ int adis_update_scan_mode(struct iio_dev *indio_dev, > > #else /* CONFIG_IIO_BUFFER */ > > +static inline int > +devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev, > + irqreturn_t (*trigger_handler)(int, void *)) > +{ > + return 0; > +} > + > static inline int adis_setup_buffer_and_trigger(struct adis *adis, > struct iio_dev *indio_dev, irqreturn_t (*trigger_handler)(int, void *)) > { > @@ -472,6 +483,12 @@ static inline void adis_cleanup_buffer_and_trigger(struct adis *adis, > { > } > > +static inline int devm_adis_probe_trigger(struct adis *adis, > + struct iio_dev *indio_dev) > +{ > + return 0; > +} > + > static inline int adis_probe_trigger(struct adis *adis, > struct iio_dev *indio_dev) > {