| Message ID | 1550596338-24220-3-git-send-email-stefan.popa@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: imu: adis16480: Add support for ADIS1649x family of devices | expand |
On Tue, 19 Feb 2019 19:12:14 +0200 Stefan Popa <stefan.popa@analog.com> 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@analog.com> Other than follow on from the previous patch change of the default, this looks fine to me. One question inline. Jonathan > --- > 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? > + > + /* > + * 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;
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@analog.com> 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@analog.com> > 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;
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); + + /* + * 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;
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@analog.com> --- drivers/iio/imu/adis16480.c | 76 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+)