Re: [PATCH 2/6] iio: imu: adis16480: Add support for configurable drdy indicator

[Popa, Stefan Serban]

On Mi, 2019-02-20 at 10:29 +0000, Jonathan Cameron wrote:
> [External]
> 
> 
> On Tue, 19 Feb 2019 19:12:14 +0200
> Stefan Popa <stefan.popa@xxxxxxxxxx> wrote:
> 
> > 
> > The FNCTIO_CTRL register provides configuration control for each I/O
> > pin
> > (DIO1, DIO2, DIO3 and DIO4).
> > 
> > This patch adds the option to configure each DIOx pin as data ready
> > indicator with positive or negative polarity by reading the
> > 'interrupts'
> > and 'interrupt-names' properties from the devicetree. The
> > 'interrupt-names' property is optional, if it is not specified, then
> > the
> > factory default DIO2 data ready signal is used.
> > 
> > Signed-off-by: Stefan Popa <stefan.popa@xxxxxxxxxx>
> Other than follow on from the previous patch change of the default, this
> looks fine to me.
> 
> One question inline.
> 
> Jonathan
Hi Jonathan,

Thank you for the review!
I will fall back on the 'wrong' default in the previous patch.
Answer inline.

-Stefan

> > 
> > ---
> > Âdrivers/iio/imu/adis16480.c | 76
> > +++++++++++++++++++++++++++++++++++++++++++++
> > Â1 file changed, 76 insertions(+)
> > 
> > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > index d222188..38ba0c1 100644
> > --- a/drivers/iio/imu/adis16480.c
> > +++ b/drivers/iio/imu/adis16480.c
> > @@ -10,6 +10,7 @@
> > Â */
> > 
> > Â#include <linux/bitfield.h>
> > +#include <linux/of_irq.h>
> > Â#include <linux/interrupt.h>
> > Â#include <linux/delay.h>
> > Â#include <linux/mutex.h>
> > @@ -109,6 +110,10 @@
> > Â#define
> > ADIS16480_FIR_COEF_D(x)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂADIS16480_FIR_COEF(0x0B,
> > (x))
> > 
> > Â/* ADIS16480_REG_FNCTIO_CTRL */
> > +#define ADIS16480_DRDY_SEL_MSKÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂGENMASK(1, 0)
> > +#define
> > ADIS16480_DRDY_SEL(x)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂFIELD_PREP(ADIS16480_DRDY_SEL_MSK,
> > x)
> > +#define ADIS16480_DRDY_POL_MSKÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂBIT(2)
> > +#define
> > ADIS16480_DRDY_POL(x)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂFIELD_PREP(ADIS16480_DRDY_POL_MSK,
> > x)
> > Â#define ADIS16480_DRDY_EN_MSKÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂBIT(3)
> > Â#define ADIS16480_DRDY_EN(x)ÂÂÂÂÂÂÂÂÂFIELD_PREP(ADIS16480_DRDY_EN_MSK,
> > x)
> > 
> > @@ -121,12 +126,26 @@ struct adis16480_chip_info {
> > ÂÂÂÂÂÂunsigned int accel_max_scale;
> > Â};
> > 
> > +enum adis16480_int_pin {
> > +ÂÂÂÂÂADIS16480_PIN_DIO1,
> > +ÂÂÂÂÂADIS16480_PIN_DIO2,
> > +ÂÂÂÂÂADIS16480_PIN_DIO3,
> > +ÂÂÂÂÂADIS16480_PIN_DIO4
> > +};
> > +
> > Âstruct adis16480 {
> > ÂÂÂÂÂÂconst struct adis16480_chip_info *chip_info;
> > 
> > ÂÂÂÂÂÂstruct adis adis;
> > Â};
> > 
> > +static const char * const adis16480_int_pin_names[4] = {
> > +ÂÂÂÂÂ[ADIS16480_PIN_DIO1] = "DIO1",
> > +ÂÂÂÂÂ[ADIS16480_PIN_DIO2] = "DIO2",
> > +ÂÂÂÂÂ[ADIS16480_PIN_DIO3] = "DIO3",
> > +ÂÂÂÂÂ[ADIS16480_PIN_DIO4] = "DIO4",
> > +};
> > +
> > Â#ifdef CONFIG_DEBUG_FS
> > 
> > Âstatic ssize_t adis16480_show_firmware_revision(struct file *file,
> > @@ -840,6 +859,59 @@ static const struct adis_data adis16480_data = {
> > ÂÂÂÂÂÂ.enable_irq = adis16480_enable_irq,
> > Â};
> > 
> > +static int adis16480_config_irq_pin(struct device_node *of_node,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct adis16480 *st)
> > +{
> > +ÂÂÂÂÂstruct irq_data *desc;
> > +ÂÂÂÂÂenum adis16480_int_pin pin;
> > +ÂÂÂÂÂunsigned int irq_type;
> > +ÂÂÂÂÂuint16_t val;
> > +ÂÂÂÂÂint i, irq = 0;
> > +
> > +ÂÂÂÂÂdesc = irq_get_irq_data(st->adis.spi->irq);
> > +ÂÂÂÂÂif (!desc) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂdev_err(&st->adis.spi->dev, "Could not find IRQ %d\n",
> > irq);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EINVAL;
> > +ÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂ/* Disable data ready */
> > +ÂÂÂÂÂval = ADIS16480_DRDY_EN(0);
> Does it default to on after reset?
> That's a little unusual and nasty.ÂÂIf not, why are you disabling here?
Yes, the default after reset is on.Â
> > 
> > +
> > +ÂÂÂÂÂ/*
> > +ÂÂÂÂÂÂ* Get the interrupt from the devicetre by reading the
> > +ÂÂÂÂÂÂ* interrupt-names property. If it is not specified, use
> > +ÂÂÂÂÂÂ* the default interrupt on DIO2 pin.
> > +ÂÂÂÂÂÂ*/
> > +ÂÂÂÂÂpin = ADIS16480_PIN_DIO2;
> > +ÂÂÂÂÂfor (i = 0; i < ARRAY_SIZE(adis16480_int_pin_names); i++) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂirq = of_irq_get_byname(of_node,
> > adis16480_int_pin_names[i]);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂif (irq > 0) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpin = i;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > +ÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂval |= ADIS16480_DRDY_SEL(pin);
> > +
> > +ÂÂÂÂÂ/*
> > +ÂÂÂÂÂÂ* Get the interrupt line behaviour. The data ready polarity can
> > be
> > +ÂÂÂÂÂÂ* configured as positive or negative, corresponding to
> > +ÂÂÂÂÂÂ* IRQF_TRIGGER_RISING or IRQF_TRIGGER_FALLING respectively.
> > +ÂÂÂÂÂÂ*/
> > +ÂÂÂÂÂirq_type = irqd_get_trigger_type(desc);
> > +ÂÂÂÂÂif (irq_type == IRQF_TRIGGER_RISING) { /* Default */
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂval |= ADIS16480_DRDY_POL(1);
> > +ÂÂÂÂÂ} else if (irq_type == IRQF_TRIGGER_FALLING) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂval |= ADIS16480_DRDY_POL(0);
> > +ÂÂÂÂÂ} else {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂdev_err(&st->adis.spi->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"Invalid interrupt type 0x%x specified\n",
> > irq_type);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EINVAL;
> > +ÂÂÂÂÂ}
> > +ÂÂÂÂÂ/* Write the data ready configuration to the FNCTIO_CTRL register
> > */
> > +ÂÂÂÂÂreturn adis_write_reg_16(&st->adis, ADIS16480_REG_FNCTIO_CTRL,
> > val);
> > +}
> > +
> > Âstatic int adis16480_probe(struct spi_device *spi)
> > Â{
> > ÂÂÂÂÂÂconst struct spi_device_id *id = spi_get_device_id(spi);
> > @@ -867,6 +939,10 @@ static int adis16480_probe(struct spi_device *spi)
> > ÂÂÂÂÂÂif (ret)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ret;
> > 
> > +ÂÂÂÂÂret =ÂÂadis16480_config_irq_pin(spi->dev.of_node, st);
> > +ÂÂÂÂÂif (ret)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ret;
> > +
> > ÂÂÂÂÂÂret = adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
> > ÂÂÂÂÂÂif (ret)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ret;