[PATCH 1/2] iio: xilinx-xadc: Remove `irq` field from state struct

[PATCH 1/2] iio: xilinx-xadc: Remove `irq` field from state struct

[Lars-Peter Clausen]

Since commit 2a9685d1a3b7 ("iio: adc: xilinx: use more devres helpers and
remove remove()") the `irq` field from XADC driver state struct is only
used in the `probe()` function.

Use the local `irq` variable throughout the `probe()` function and remove
the now unused field from the state struct.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/adc/xilinx-xadc-core.c | 5 ++---
 drivers/iio/adc/xilinx-xadc.h      | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 6914c1900ed0..dd1c92b2811f 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1332,7 +1332,6 @@ static int xadc_probe(struct platform_device *pdev)
 
 	xadc = iio_priv(indio_dev);
 	xadc->ops = id->data;
-	xadc->irq = irq;
 	init_completion(&xadc->completion);
 	mutex_init(&xadc->mutex);
 	spin_lock_init(&xadc->lock);
@@ -1397,7 +1396,7 @@ static int xadc_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = devm_request_irq(dev, xadc->irq, xadc->ops->interrupt_handler, 0,
+	ret = devm_request_irq(dev, irq, xadc->ops->interrupt_handler, 0,
 			       dev_name(dev), indio_dev);
 	if (ret)
 		return ret;
@@ -1407,7 +1406,7 @@ static int xadc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
+	ret = xadc->ops->setup(pdev, indio_dev, irq);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iio/adc/xilinx-xadc.h b/drivers/iio/adc/xilinx-xadc.h
index 8b80195725e9..7d78ce698967 100644
--- a/drivers/iio/adc/xilinx-xadc.h
+++ b/drivers/iio/adc/xilinx-xadc.h
@@ -67,7 +67,6 @@ struct xadc {
 	spinlock_t lock;
 
 	struct completion completion;
-	int irq;
 };
 
 enum xadc_type {
-- 
2.20.1

[PATCH 2/2] iio: xilinx-xadc: Make IRQ optional

[Lars-Peter Clausen]

In some setups the IRQ signal of the XADC might not be wired to the host
system. The driver currently requires that an interrupt is specified. Make
the interrupt optional so the driver can be used in such setups where the
interrupt is not connected.

Since both buffered capture as well as events depend on the interrupt being
connected both are not available when the interrupt is not connected.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/adc/xilinx-xadc-core.c | 39 +++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index dd1c92b2811f..ffefb01a3893 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1182,7 +1182,7 @@ static const struct of_device_id xadc_of_match_table[] = {
 MODULE_DEVICE_TABLE(of, xadc_of_match_table);
 
 static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
-	unsigned int *conf)
+	unsigned int *conf, int irq)
 {
 	struct device *dev = indio_dev->dev.parent;
 	struct xadc *xadc = iio_priv(indio_dev);
@@ -1195,6 +1195,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 	u32 ext_mux_chan;
 	u32 reg;
 	int ret;
+	int i;
 
 	*conf = 0;
 
@@ -1273,6 +1274,14 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 	}
 	of_node_put(chan_node);
 
+	/* No IRQ => no events */
+	if (irq <= 0) {
+		for (i = 0; i < num_channels; i++) {
+			channels[i].event_spec = NULL;
+			channels[i].num_event_specs = 0;
+		}
+	}
+
 	indio_dev->num_channels = num_channels;
 	indio_dev->channels = devm_krealloc(dev, channels,
 					    sizeof(*channels) * num_channels,
@@ -1322,9 +1331,9 @@ static int xadc_probe(struct platform_device *pdev)
 	if (!id)
 		return -EINVAL;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0)
-		return -ENXIO;
+	irq = platform_get_irq_optional(pdev, 0);
+	if (irq < 0 && irq != -ENXIO)
+		return irq;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*xadc));
 	if (!indio_dev)
@@ -1345,11 +1354,11 @@ static int xadc_probe(struct platform_device *pdev)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &xadc_info;
 
-	ret = xadc_parse_dt(indio_dev, dev->of_node, &conf0);
+	ret = xadc_parse_dt(indio_dev, dev->of_node, &conf0, irq);
 	if (ret)
 		return ret;
 
-	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
+	if (irq > 0 && (xadc->ops->flags & XADC_FLAGS_BUFFERED)) {
 		ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
 						      &iio_pollfunc_store_time,
 						      &xadc_trigger_handler,
@@ -1396,15 +1405,17 @@ static int xadc_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = devm_request_irq(dev, irq, xadc->ops->interrupt_handler, 0,
-			       dev_name(dev), indio_dev);
-	if (ret)
-		return ret;
+	if (irq > 0) {
+		ret = devm_request_irq(dev, irq, xadc->ops->interrupt_handler,
+				       0, dev_name(dev), indio_dev);
+		if (ret)
+			return ret;
 
-	ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
-				       &xadc->zynq_unmask_work);
-	if (ret)
-		return ret;
+		ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
+					       &xadc->zynq_unmask_work);
+		if (ret)
+			return ret;
+	}
 
 	ret = xadc->ops->setup(pdev, indio_dev, irq);
 	if (ret)
-- 
2.20.1

Re: [PATCH 2/2] iio: xilinx-xadc: Make IRQ optional

[Jonathan Cameron]

On Sun, 25 Apr 2021 12:26:38 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> In some setups the IRQ signal of the XADC might not be wired to the host
> system. The driver currently requires that an interrupt is specified. Make
> the interrupt optional so the driver can be used in such setups where the
> interrupt is not connected.
> 
> Since both buffered capture as well as events depend on the interrupt being
> connected both are not available when the interrupt is not connected.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Code looks fine to me but raises a question.  As there is no validate_trigger
callback in this driver, nothing stops it being associated with for example
an hrtimer trigger.  I don't think that will work so perhaps we should add the
callback?  If it does work, then the presence or not of an irq should not
change whether we register the buffer.

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 39 +++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index dd1c92b2811f..ffefb01a3893 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -1182,7 +1182,7 @@ static const struct of_device_id xadc_of_match_table[] = {
>  MODULE_DEVICE_TABLE(of, xadc_of_match_table);
>  
>  static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
> -	unsigned int *conf)
> +	unsigned int *conf, int irq)
>  {
>  	struct device *dev = indio_dev->dev.parent;
>  	struct xadc *xadc = iio_priv(indio_dev);
> @@ -1195,6 +1195,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
>  	u32 ext_mux_chan;
>  	u32 reg;
>  	int ret;
> +	int i;
>  
>  	*conf = 0;
>  
> @@ -1273,6 +1274,14 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
>  	}
>  	of_node_put(chan_node);
>  
> +	/* No IRQ => no events */
> +	if (irq <= 0) {
> +		for (i = 0; i < num_channels; i++) {
> +			channels[i].event_spec = NULL;
> +			channels[i].num_event_specs = 0;
> +		}
> +	}
> +
>  	indio_dev->num_channels = num_channels;
>  	indio_dev->channels = devm_krealloc(dev, channels,
>  					    sizeof(*channels) * num_channels,
> @@ -1322,9 +1331,9 @@ static int xadc_probe(struct platform_device *pdev)
>  	if (!id)
>  		return -EINVAL;
>  
> -	irq = platform_get_irq(pdev, 0);
> -	if (irq <= 0)
> -		return -ENXIO;
> +	irq = platform_get_irq_optional(pdev, 0);
> +	if (irq < 0 && irq != -ENXIO)
> +		return irq;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*xadc));
>  	if (!indio_dev)
> @@ -1345,11 +1354,11 @@ static int xadc_probe(struct platform_device *pdev)
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &xadc_info;
>  
> -	ret = xadc_parse_dt(indio_dev, dev->of_node, &conf0);
> +	ret = xadc_parse_dt(indio_dev, dev->of_node, &conf0, irq);
>  	if (ret)
>  		return ret;
>  
> -	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
> +	if (irq > 0 && (xadc->ops->flags & XADC_FLAGS_BUFFERED)) {
>  		ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>  						      &iio_pollfunc_store_time,
>  						      &xadc_trigger_handler,
> @@ -1396,15 +1405,17 @@ static int xadc_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	ret = devm_request_irq(dev, irq, xadc->ops->interrupt_handler, 0,
> -			       dev_name(dev), indio_dev);
> -	if (ret)
> -		return ret;
> +	if (irq > 0) {
> +		ret = devm_request_irq(dev, irq, xadc->ops->interrupt_handler,
> +				       0, dev_name(dev), indio_dev);
> +		if (ret)
> +			return ret;
>  
> -	ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
> -				       &xadc->zynq_unmask_work);
> -	if (ret)
> -		return ret;
> +		ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
> +					       &xadc->zynq_unmask_work);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = xadc->ops->setup(pdev, indio_dev, irq);
>  	if (ret)

Re: [PATCH 2/2] iio: xilinx-xadc: Make IRQ optional

[Lars-Peter Clausen]

On 4/25/21 5:25 PM, Jonathan Cameron wrote:
> On Sun, 25 Apr 2021 12:26:38 +0200
> Lars-Peter Clausen <lars@metafoo.de> wrote:
>
>> In some setups the IRQ signal of the XADC might not be wired to the host
>> system. The driver currently requires that an interrupt is specified. Make
>> the interrupt optional so the driver can be used in such setups where the
>> interrupt is not connected.
>>
>> Since both buffered capture as well as events depend on the interrupt being
>> connected both are not available when the interrupt is not connected.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Code looks fine to me but raises a question.  As there is no validate_trigger
> callback in this driver, nothing stops it being associated with for example
> an hrtimer trigger.  I don't think that will work so perhaps we should add the
> callback?  If it does work, then the presence or not of an irq should not
> change whether we register the buffer.

This is a good point. But the driver can actually work with a hrtimer 
trigger. You'd get aliasing problems and all that stuff, but if you only 
care about the instantaneous values it will work.

Without an IRQ we should only mask out the events and not register the 
triggers. Registering the buffer should be fine.

- Lars