Re: [PATCH v4 1/3] iio: adc: ti-ads112c14: add DRDY interrupt support

[Jonathan Cameron]

> Add handling for the DRDY interrupt to wait for data ready events rather
> than polling (only when it is wired up).
> 
> Signed-off-by: David Lechner (TI) <dlechner@xxxxxxxxxxxx>

I'm messing with "b4 review tui" to see if it works well for me and
given it has nice sashiko integration, we have comments from there
inline.  To me they all seem fine.

One request - I think we should add a little more detail
to the dt-binding about interrupt types that make sense.
That can be a follow up though if this series is otherwise ready
to go.

>
> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> index 60eab4852ba2..63b819339788 100644
> --- a/drivers/iio/adc/ti-ads112c14.c
> +++ b/drivers/iio/adc/ti-ads112c14.c
> @@ -10,6 +10,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/cleanup.h>
> +#include <linux/completion.h>
>  #include <linux/crc8.h>
>  #include <linux/delay.h>
>  #include <linux/dev_printk.h>
> @@ -19,6 +20,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/interrupt.h>
>  #include <linux/math64.h>
>  #include <linux/minmax.h>
>  #include <linux/module.h>
> @@ -117,9 +119,15 @@
>  #define   ADS112C14_GPIO_CFG_GPIO2_CFG			GENMASK(5, 4)
>  #define   ADS112C14_GPIO_CFG_GPIO1_CFG			GENMASK(3, 2)
>  #define   ADS112C14_GPIO_CFG_GPIO0_CFG			GENMASK(1, 0)
> +#define     ADS112C14_GPIO_CFG_GPIO_CFG_DISABLED	  0
> +#define     ADS112C14_GPIO_CFG_GPIO_CFG_INPUT		  1
> +#define     ADS112C14_GPIO_CFG_GPIO_CFG_OUTPUT_PUSH_PULL  2
> +#define     ADS112C14_GPIO_CFG_GPIO_CFG_OUTPUT_OPEN_DRAIN 3
>  
>  #define ADS112C14_REG_GPIO_DATA_OUTPUT			0x0C
>  #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC		BIT(7)
> +#define     ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC_DAT_OUT  0
> +#define     ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC_DRDY	  1
>  #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO2_SRC		BIT(6)
>  #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO3_DAT_OUT	BIT(3)
>  #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO2_DAT_OUT	BIT(2)
> @@ -251,6 +259,8 @@ struct ads112c14_data {
>  	struct regmap *regmap;
>  	/* Synchronizes access to register value fields. */
>  	struct mutex lock;
> +	int drdy_irq;
> +	struct completion drdy_completion;
>  	bool i2c_crc_enabled;
>  	u32 avdd_uV;
>  	u32 ext_ref_uV;
> @@ -265,6 +275,16 @@ struct ads112c14_data {
>  						 ARRAY_SIZE(ads112c14_sys_mon_channels));
>  };

>  static bool ads112c14_writeable_reg(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
> @@ -581,12 +601,45 @@ static int ads112c14_prepare_sys_mon_channel(struct ads112c14_data *data,
>  	return 0;
>  }
>  
> +static int ads112c14_wait_for_conversion_irq(struct ads112c14_data *data)
> +{
> +	unsigned long remaining;
> +	int ret;
> +
> +	reinit_completion(&data->drdy_completion);
> +
> +	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
> +			   ADS112C14_CONVERSION_CTRL_START);

 | sashiko.dev <sashiko@xxxxxxxxxxx>:
 |
 | [Severity: Medium]
 | Could this allow a stale DRDY interrupt from a previous timed-out conversion
 | to falsely complete the wait?
 |
 | Since regmap_write() performs a slow I2C transaction that can sleep, if a
 | previous conversion had timed out, the device might complete it and assert
 |  the DRDY interrupt exactly during this regmap_write(). The interrupt handler
 | would then call complete(), causing the subsequent
 | wait_for_completion_timeout() to return immediately and the driver to read
 | stale data.
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-1-1e51a6e20a69@xxxxxxxxxxxx

For this I don't think we care.  It is unlikely to happen and the chance
of getting stale data in that corner case probably doesn't bother us.

> +	if (ret)
> +		return ret;
> +
> +	remaining = wait_for_completion_timeout(&data->drdy_completion,
> +						msecs_to_jiffies(100));
> +
> +	return remaining ? 0 : -ETIMEDOUT;
> +}
> +
> +static int ads112c14_wait_for_conversion_poll(struct ads112c14_data *data)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
> +			   ADS112C14_CONVERSION_CTRL_START);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_read_poll_timeout(data->regmap,
> +					ADS112C14_REG_STATUS_MSB, reg_val,
> +					FIELD_GET(ADS112C14_STATUS_MSB_DRDY, reg_val),
> +					1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
> +}
> +
>  static int ads112c14_single_conversion(struct ads112c14_data *data,
>  				       const struct iio_chan_spec *chan,
>  				       u8 *buf, bool for_scan)
>  {
>  	struct i2c_client *client = to_i2c_client(regmap_get_device(data->regmap));
> -	u32 reg_val;
>  	int ret;
>  
>  	guard(mutex)(&data->lock);
> @@ -601,15 +654,10 @@ static int ads112c14_single_conversion(struct ads112c14_data *data,
>  			return ret;
>  	}
>  
> -	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
> -			   ADS112C14_CONVERSION_CTRL_START);
> -	if (ret)
> -		return ret;
> -
> -	ret = regmap_read_poll_timeout(data->regmap,
> -				       ADS112C14_REG_STATUS_MSB, reg_val,
> -				       FIELD_GET(ADS112C14_STATUS_MSB_DRDY, reg_val),
> -				       1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
> +	if (data->drdy_irq)
> +		ret = ads112c14_wait_for_conversion_irq(data);
> +	else
> +		ret = ads112c14_wait_for_conversion_poll(data);
>  	if (ret)
>  		return ret;
>  
> @@ -1391,6 +1439,38 @@ static int ads112c14_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	if (device_property_present(dev, "interrupt-names")) {
> +		data->drdy_irq = fwnode_irq_get_byname(dev_fwnode(dev), "drdy");
> +		if (data->drdy_irq < 0)
> +			return dev_err_probe(dev, data->drdy_irq,
> +					     "failed to get drdy interrupt\n");
> +
> +		/*
> +		 * REVISIT: would probably need to implement a pin controller in
> +		 * order to support open drain option here.
> +		 */
> +		ret = regmap_update_bits(data->regmap, ADS112C14_REG_GPIO_CFG,
> +					 ADS112C14_GPIO_CFG_GPIO3_CFG,
> +					 FIELD_PREP(ADS112C14_GPIO_CFG_GPIO3_CFG,
> +						    ADS112C14_GPIO_CFG_GPIO_CFG_OUTPUT_PUSH_PULL));

 | sashiko.dev <sashiko@xxxxxxxxxxx>:
 |
 | [Severity: High]
 | Does this unconditionally force GPIO3 into an output even on board designs
 | that use the dedicated DRDY pin instead?
 |
 | For package variants (like the TSSOP) with a dedicated DRDY pin, a board
 | designer might route the dedicated pin to the interrupt controller and use
 | GPIO3 for another circuit. By forcing GPIO3 to a push-pull output here,
 | the driver will drive voltage into whatever is connected to it, which could
 | cause an electrical short if it is tied to GND/VCC or another output.
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-1-1e51a6e20a69@xxxxxxxxxxxx

This seems to be a datasheet misread. Kind of reasonable given
the datasheet talks about Data Ready (!DRDY) Pin. But that section goes
on to explain.

"GPIO3 can be configured as a dedicated DRDY output pin (GPIO3_CFG[1:0] = 10b or 11b, GPIO3_SRC = 1b)"

> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(data->regmap, ADS112C14_REG_GPIO_DATA_OUTPUT,
> +					 ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC,
> +					 FIELD_PREP(ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC,
> +						    ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC_DRDY));
> +		if (ret)
> +			return ret;
> +
> +		init_completion(&data->drdy_completion);
> +
> +		ret = devm_request_irq(dev, data->drdy_irq, ads112c14_drdy_irq_handler,
> +				       0, dev_name(dev), indio_dev);

 | sashiko.dev <sashiko@xxxxxxxxxxx>:
 |
 | [Severity: High]
 | Could this lead to an interrupt storm on systems with level-triggered
 | interrupts, or fail completely on nested IRQ controllers?
 |
 | The IRQ is requested without a thread function and without IRQF_ONESHOT or
 | explicit edge-trigger flags. Since ads112c14_drdy_irq_handler() is a hardirq
 | handler, it acknowledges the OS interrupt but cannot perform a sleeping I2C
 | transaction to clear the physical line state on the device.

IRQF_ONESHOT in a non threaded interrupt handler would be pointless.

 |
 | On level-triggered systems, this will cause the IRQ controller to immediately
 | re-trigger the interrupt, starving the CPU. Additionally, if the interrupt
 | is routed through an I2C/SPI GPIO expander (nested IRQ controller),
 | devm_request_irq() will fail with -EINVAL because a threaded handler is
 | strictly required.
 |
 | Would it be better to use a threaded IRQ with IRQF_ONESHOT, or strictly
 | require edge-triggering?
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-1-1e51a6e20a69@xxxxxxxxxxxx

Personally I don't consider it the drivers job to handle broken firmware or
indeed the more complex cases of expanders that rely on threaded interrupts
only.  I do note that the dt file doesn't express that it expects
an edge interrupt though. Maybe that could be improved?  Not to mention
providing interrupts in one of the examples (that would at least
have hinted about this constraint).

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>