Re: [PATCH 4/4] iio: st_sensors: support open drain mode

[Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

On 24/03/16 13:18, Linus Walleij wrote:
> Some types of ST Sensors can be connected to the same IRQ line
> as other peripherals using open drain. Add a device tree binding
> and a sensor data property to flip the right bit in the interrupt
> control register to enable open drain mode on the INT line.
> 
> If the line is set to be open drain, also tag on IRQF_SHARED
> to the IRQ flags when requesting the interrupt, as the whole
> point of using open drain interrupt lines is to share them with
> more than one peripheral (wire-or).
> 
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Giuseppe Barba <giuseppe.barba-qxv4g6HH51o@public.gmane.org>
> Cc: Denis Ciocca <denis.ciocca-qxv4g6HH51o@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Few comments inline but looks fine to me.  Ideally again an
ack from Denis would be good. Mostly this one is stalled behind the earlier
ones.

Jonathan
> ---
> ChangeLog v2->v3:
> - Rebase on top of the patches fixing the other issues (handling
>   IRQ status check and channel reading bug).
> ChangeLog v1->v2:
> - Rebased to fit the new patch order.
> ---
>  Documentation/devicetree/bindings/iio/st-sensors.txt |  3 +++
>  drivers/iio/accel/st_accel_core.c                    |  8 ++++++++
>  drivers/iio/common/st_sensors/st_sensors_core.c      | 20 ++++++++++++++++++++
>  drivers/iio/common/st_sensors/st_sensors_trigger.c   | 13 +++++++++++++
>  drivers/iio/pressure/st_pressure_core.c              |  8 ++++++++
>  include/linux/iio/common/st_sensors.h                |  6 ++++++
>  include/linux/platform_data/st_sensors_pdata.h       |  2 ++
>  7 files changed, 60 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/st-sensors.txt b/Documentation/devicetree/bindings/iio/st-sensors.txt
> index d4b87cc1e446..accb5681fbbb 100644
> --- a/Documentation/devicetree/bindings/iio/st-sensors.txt
> +++ b/Documentation/devicetree/bindings/iio/st-sensors.txt
> @@ -16,6 +16,9 @@ Optional properties:
>  - st,drdy-int-pin: the pin on the package that will be used to signal
>    "data ready" (valid values: 1 or 2). This property is not configurable
>    on all sensors.
> +- st,int-pin-open-drain: the interrupt/data ready line will be configured
> +  as open drain, which is useful if several sensors share the same
> +  interrupt line.
Really feels like this one ought to be more generic.. Mind you so does
drdy-int-pin!

Hmm. Looks like most open drain controls are chip specific - other than
your pinctrl ones and that doesn't really map to here I guess.


>  
>  Sensors may also have applicable pin control settings, those use the
>  standard bindings from pinctrl/pinctrl-bindings.txt.
> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> index 4df4c278af23..f06f4329db5b 100644
> --- a/drivers/iio/accel/st_accel_core.c
> +++ b/drivers/iio/accel/st_accel_core.c
> @@ -96,6 +96,8 @@
>  #define ST_ACCEL_2_DRDY_IRQ_INT2_MASK		0x10
>  #define ST_ACCEL_2_IHL_IRQ_ADDR			0x22
>  #define ST_ACCEL_2_IHL_IRQ_MASK			0x80
> +#define ST_ACCEL_2_OD_IRQ_ADDR			0x22
> +#define ST_ACCEL_2_OD_IRQ_MASK			0x40
>  #define ST_ACCEL_2_MULTIREAD_BIT		true
>  
>  /* CUSTOM VALUES FOR SENSOR 3 */
> @@ -177,6 +179,8 @@
>  #define ST_ACCEL_5_DRDY_IRQ_INT2_MASK		0x20
>  #define ST_ACCEL_5_IHL_IRQ_ADDR			0x22
>  #define ST_ACCEL_5_IHL_IRQ_MASK			0x80
> +#define ST_ACCEL_5_OD_IRQ_ADDR			0x22
> +#define ST_ACCEL_5_OD_IRQ_MASK			0x40
>  #define ST_ACCEL_5_IG1_EN_ADDR			0x21
>  #define ST_ACCEL_5_IG1_EN_MASK			0x08
>  #define ST_ACCEL_5_MULTIREAD_BIT		false
> @@ -368,6 +372,8 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
>  			.mask_int2 = ST_ACCEL_2_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_ACCEL_2_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_ACCEL_2_IHL_IRQ_MASK,
> +			.addr_od = ST_ACCEL_2_OD_IRQ_ADDR,
> +			.mask_od = ST_ACCEL_2_OD_IRQ_MASK,
>  			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_ACCEL_2_MULTIREAD_BIT,
> @@ -557,6 +563,8 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
>  			.mask_int2 = ST_ACCEL_5_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_ACCEL_5_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_ACCEL_5_IHL_IRQ_MASK,
> +			.addr_od = ST_ACCEL_5_OD_IRQ_ADDR,
> +			.mask_od = ST_ACCEL_5_OD_IRQ_MASK,
>  			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_ACCEL_5_MULTIREAD_BIT,
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index f5a2d445d0c0..2e03c1220d1f 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -301,6 +301,14 @@ static int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev,
>  		return -EINVAL;
>  	}
>  
> +	if (pdata->open_drain) {
> +		if (!sdata->sensor_settings->drdy_irq.addr_od)
> +			dev_err(&indio_dev->dev,
> +				"open drain requested but unsupported.\n");
> +		else
> +			sdata->int_pin_open_drain = true;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -321,6 +329,8 @@ static struct st_sensors_platform_data *st_sensors_of_probe(struct device *dev,
>  	else
>  		pdata->drdy_int_pin = defdata ? defdata->drdy_int_pin : 0;
>  
> +	pdata->open_drain = of_property_read_bool(np, "st,int-pin-open-drain");
> +
>  	return pdata;
>  }
>  #else
> @@ -374,6 +384,16 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
>  			return err;
>  	}
>  
> +	if (sdata->int_pin_open_drain) {
> +		dev_info(&indio_dev->dev,
> +			 "set interrupt line to open drain mode\n");
Hmm. Curriously I have a board with an lis3l02dq on it where the interrupt
is shared, but doesn't explicitly support open drain. In that case it
is done with some external circuitry.

Anyhow - I guess when I actually add support for that I'll just add a sanity
check in here that addr_od and mask_od are specified :)


> +		err = st_sensors_write_data_with_mask(indio_dev,
> +				sdata->sensor_settings->drdy_irq.addr_od,
> +				sdata->sensor_settings->drdy_irq.mask_od, 1);
> +		if (err < 0)
> +			return err;
> +	}
> +
>  	err = st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
>  
>  	return err;
> diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> index 6a8c98327945..da72279fcf99 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> @@ -64,6 +64,19 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  			"rising edge\n", irq_trig);
>  		irq_trig = IRQF_TRIGGER_RISING;
>  	}
> +
> +	/*
> +	 * If the interrupt pin is Open Drain, by definition this
> +	 * means that the interrupt line may be shared with other
> +	 * peripherals. But to do this we also need to have a status
> +	 * register and mask to figure out if this sensor was firing
> +	 * the IRQ or not, so we can tell the interrupt handle that
> +	 * it was "our" interrupt.
> +	 */
> +	if (sdata->int_pin_open_drain &&
> +	    sdata->sensor_settings->drdy_irq.addr_stat_drdy)
> +		irq_trig |= IRQF_SHARED;
> +
>  	err = request_threaded_irq(irq,
>  			iio_trigger_generic_data_rdy_poll,
>  			NULL,
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index 1cd37eaa4a57..9e9b72a8f18f 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -64,6 +64,8 @@
>  #define ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK	0x20
>  #define ST_PRESS_LPS331AP_IHL_IRQ_ADDR		0x22
>  #define ST_PRESS_LPS331AP_IHL_IRQ_MASK		0x80
> +#define ST_PRESS_LPS331AP_OD_IRQ_ADDR		0x22
> +#define ST_PRESS_LPS331AP_OD_IRQ_MASK		0x40
>  #define ST_PRESS_LPS331AP_MULTIREAD_BIT		true
>  #define ST_PRESS_LPS331AP_TEMP_OFFSET		42500
>  
> @@ -104,6 +106,8 @@
>  #define ST_PRESS_LPS25H_DRDY_IRQ_INT2_MASK	0x10
>  #define ST_PRESS_LPS25H_IHL_IRQ_ADDR		0x22
>  #define ST_PRESS_LPS25H_IHL_IRQ_MASK		0x80
> +#define ST_PRESS_LPS25H_OD_IRQ_ADDR		0x22
> +#define ST_PRESS_LPS25H_OD_IRQ_MASK		0x40
>  #define ST_PRESS_LPS25H_MULTIREAD_BIT		true
>  #define ST_PRESS_LPS25H_TEMP_OFFSET		42500
>  #define ST_PRESS_LPS25H_OUT_XL_ADDR		0x28
> @@ -226,6 +230,8 @@ static const struct st_sensor_settings st_press_sensors_settings[] = {
>  			.mask_int2 = ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_PRESS_LPS331AP_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_PRESS_LPS331AP_IHL_IRQ_MASK,
> +			.addr_od = ST_PRESS_LPS331AP_OD_IRQ_ADDR,
> +			.mask_od = ST_PRESS_LPS331AP_OD_IRQ_MASK,
>  			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_PRESS_LPS331AP_MULTIREAD_BIT,
> @@ -313,6 +319,8 @@ static const struct st_sensor_settings st_press_sensors_settings[] = {
>  			.mask_int2 = ST_PRESS_LPS25H_DRDY_IRQ_INT2_MASK,
>  			.addr_ihl = ST_PRESS_LPS25H_IHL_IRQ_ADDR,
>  			.mask_ihl = ST_PRESS_LPS25H_IHL_IRQ_MASK,
> +			.addr_od = ST_PRESS_LPS25H_OD_IRQ_ADDR,
> +			.mask_od = ST_PRESS_LPS25H_OD_IRQ_MASK,
>  			.addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
>  		},
>  		.multi_read_bit = ST_PRESS_LPS25H_MULTIREAD_BIT,
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> index d8da075bfda0..d029ffac0d69 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -122,6 +122,8 @@ struct st_sensor_bdu {
>   * @mask_int2: mask to enable/disable IRQ on INT2 pin.
>   * @addr_ihl: address to enable/disable active low on the INT lines.
>   * @mask_ihl: mask to enable/disable active low on the INT lines.
> + * @addr_od: address to enable/disable Open Drain on the INT lines.
> + * @mask_od: mask to enable/disable Open Drain on the INT lines.
>   * @addr_stat_drdy: address to read status of DRDY (data ready) interrupt
>   * struct ig1 - represents the Interrupt Generator 1 of sensors.
>   * @en_addr: address of the enable ig1 register.
> @@ -133,6 +135,8 @@ struct st_sensor_data_ready_irq {
>  	u8 mask_int2;
>  	u8 addr_ihl;
>  	u8 mask_ihl;
> +	u8 addr_od;
> +	u8 mask_od;
>  	u8 addr_stat_drdy;
>  	struct {
>  		u8 en_addr;
> @@ -215,6 +219,7 @@ struct st_sensor_settings {
>   * @odr: Output data rate of the sensor [Hz].
>   * num_data_channels: Number of data channels used in buffer.
>   * @drdy_int_pin: Redirect DRDY on pin 1 (1) or pin 2 (2).
> + * @int_pin_open_drain: Set the interrupt/DRDY to open drain.
>   * @get_irq_data_ready: Function to get the IRQ used for data ready signal.
>   * @tf: Transfer function structure used by I/O operations.
>   * @tb: Transfer buffers and mutex used by I/O operations.
> @@ -236,6 +241,7 @@ struct st_sensor_data {
>  	unsigned int num_data_channels;
>  
>  	u8 drdy_int_pin;
> +	bool int_pin_open_drain;
>  
>  	unsigned int (*get_irq_data_ready) (struct iio_dev *indio_dev);
>  
> diff --git a/include/linux/platform_data/st_sensors_pdata.h b/include/linux/platform_data/st_sensors_pdata.h
> index 753839187ba0..79b0e4cdb814 100644
> --- a/include/linux/platform_data/st_sensors_pdata.h
> +++ b/include/linux/platform_data/st_sensors_pdata.h
> @@ -16,9 +16,11 @@
>   * @drdy_int_pin: Redirect DRDY on pin 1 (1) or pin 2 (2).
>   *	Available only for accelerometer and pressure sensors.
>   *	Accelerometer DRDY on LSM330 available only on pin 1 (see datasheet).
> + * @open_drain: set the interrupt line to be open drain if possible.
>   */
>  struct st_sensors_platform_data {
>  	u8 drdy_int_pin;
> +	bool open_drain;
>  };
>  
>  #endif /* ST_SENSORS_PDATA_H */
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html