[PATCH 1/5] iio: xilinx-xadc: Fix ADC-B powerdown

[PATCH 1/5] iio: xilinx-xadc: Fix ADC-B powerdown

[Lars-Peter Clausen]

The check for shutting down the second ADC is inverted. This causes it to
be powered down when it should be enabled. As a result channels that are
supposed to be handled by the second ADC return invalid conversion results.

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

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 2d6505a66511..4fcf1729341f 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -722,13 +722,14 @@ static int xadc_power_adc_b(struct xadc *xadc, unsigned int seq_mode)
 {
 	uint16_t val;
 
+	/* Powerdown the ADC-B when it is not needed. */
 	switch (seq_mode) {
 	case XADC_CONF1_SEQ_SIMULTANEOUS:
 	case XADC_CONF1_SEQ_INDEPENDENT:
-		val = XADC_CONF2_PD_ADC_B;
+		val = 0;
 		break;
 	default:
-		val = 0;
+		val = XADC_CONF2_PD_ADC_B;
 		break;
 	}
 
-- 
2.20.1

[PATCH 2/5] iio: xilinx-xadc: Fix clearing interrupt when enabling trigger

[Lars-Peter Clausen]

When enabling the trigger and unmasking the end-of-sequence (EOS) interrupt
the EOS interrupt should be cleared from the status register. Otherwise it
is possible that it was still set from a previous capture. If that is the
case the interrupt would fire immediately even though no conversion has
been done yet and stale data is being read from the device.

The old code only clears the interrupt if the interrupt was previously
unmasked. Which does not make much sense since the interrupt is always
masked at this point and in addition masking the interrupt does not clear
the interrupt from the status register. So the clearing needs to be done
unconditionally.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/adc/xilinx-xadc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 4fcf1729341f..04a2a609ced4 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -674,7 +674,7 @@ static int xadc_trigger_set_state(struct iio_trigger *trigger, bool state)
 
 	spin_lock_irqsave(&xadc->lock, flags);
 	xadc_read_reg(xadc, XADC_AXI_REG_IPIER, &val);
-	xadc_write_reg(xadc, XADC_AXI_REG_IPISR, val & XADC_AXI_INT_EOS);
+	xadc_write_reg(xadc, XADC_AXI_REG_IPISR, XADC_AXI_INT_EOS);
 	if (state)
 		val |= XADC_AXI_INT_EOS;
 	else
-- 
2.20.1

[PATCH 3/5] iio: xilinx-xadc: Fix sequencer configuration for aux channels in simultaneous mode

[Lars-Peter Clausen]

The XADC has two internal ADCs. Depending on the mode it is operating in
either one or both of them are used. The device manual calls this
continuous (one ADC) and simultaneous (both ADCs) mode.

The meaning of the sequencing register for the aux channels changes
depending on the mode.

In continuous mode each bit corresponds to one of the 16 aux channels. And
the single ADC will convert them one by one in order.

In simultaneous mode the aux channels are split into two groups the first 8
channels are assigned to the first ADC and the other 8 channels to the
second ADC. The upper 8 bits of the sequencing register are unused and the
lower 8 bits control both ADCs. This means a bit needs to be set if either
the corresponding channel from the first group or the second group (or
both) are set.

Currently the driver does not have the special handling required for
simultaneous mode. Add it.

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

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 04a2a609ced4..4acababda4d5 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -798,6 +798,16 @@ static int xadc_preenable(struct iio_dev *indio_dev)
 	if (ret)
 		goto err;
 
+	/*
+	 * In simultaneous mode the upper and lower aux channels are samples at
+	 * the same time. In this mode the upper 8 bits in the sequencer
+	 * register are don't care and the lower 8 bits control two channels
+	 * each. As such we must set the bit if either the channel in the lower
+	 * group or the upper group is enabled.
+	 */
+	if (seq_mode == XADC_CONF1_SEQ_SIMULTANEOUS)
+		scan_mask = ((scan_mask >> 8) | scan_mask) & 0xff0000;
+
 	ret = xadc_write_adc_reg(xadc, XADC_REG_SEQ(1), scan_mask >> 16);
 	if (ret)
 		goto err;
-- 
2.20.1

[PATCH 4/5] iio: xilinx-xadc: Make sure not exceed maximum samplerate

[Lars-Peter Clausen]

The XADC supports a samplerate of up to 1MSPS. Unfortunately the hardware
does not have a FIFO, which means it generates an interrupt for each
conversion sequence. At one 1MSPS this creates an interrupt storm that
causes the system to soft-lock.

For this reason the driver limits the maximum samplerate to 150kSPS.
Currently this check is only done when setting a new samplerate. But it is
also possible that the initial samplerate configured in the FPGA bitstream
exceeds the limit.

In this case when starting to capture data without first changing the
samplerate the system can overload.

To prevent this check the currently configured samplerate in the probe
function and reduce it to the maximum if necessary.

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

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 4acababda4d5..c724b8788007 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -102,6 +102,16 @@ static const unsigned int XADC_ZYNQ_UNMASK_TIMEOUT = 500;
 
 #define XADC_FLAGS_BUFFERED BIT(0)
 
+/*
+ * The XADC hardware supports a samplerate of up to 1MSPS. Unfortunately it does
+ * not have a hardware FIFO. Which means an interrupt is generated for each
+ * conversion sequence. At 1MSPS sample rate the CPU in ZYNQ7000 is completely
+ * overloaded by the interrupts that it soft-lockups. For this reason the driver
+ * limits the maximum samplerate 150kSPS. At this rate the CPU is fairly busy,
+ * but still responsive.
+ */
+#define XADC_MAX_SAMPLERATE 150000
+
 static void xadc_write_reg(struct xadc *xadc, unsigned int reg,
 	uint32_t val)
 {
@@ -834,11 +844,27 @@ static const struct iio_buffer_setup_ops xadc_buffer_ops = {
 	.postdisable = &xadc_postdisable,
 };
 
+static int xadc_read_samplerate(struct xadc *xadc)
+{
+	unsigned int div;
+	uint16_t val16;
+	int ret;
+
+	ret = xadc_read_adc_reg(xadc, XADC_REG_CONF2, &val16);
+	if (ret)
+		return ret;
+
+	div = (val16 & XADC_CONF2_DIV_MASK) >> XADC_CONF2_DIV_OFFSET;
+	if (div < 2)
+		div = 2;
+
+	return xadc_get_dclk_rate(xadc) / div / 26;
+}
+
 static int xadc_read_raw(struct iio_dev *indio_dev,
 	struct iio_chan_spec const *chan, int *val, int *val2, long info)
 {
 	struct xadc *xadc = iio_priv(indio_dev);
-	unsigned int div;
 	uint16_t val16;
 	int ret;
 
@@ -891,41 +917,31 @@ static int xadc_read_raw(struct iio_dev *indio_dev,
 		*val = -((273150 << 12) / 503975);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		ret = xadc_read_adc_reg(xadc, XADC_REG_CONF2, &val16);
-		if (ret)
+		ret = xadc_read_samplerate(xadc);
+		if (ret < 0)
 			return ret;
 
-		div = (val16 & XADC_CONF2_DIV_MASK) >> XADC_CONF2_DIV_OFFSET;
-		if (div < 2)
-			div = 2;
-
-		*val = xadc_get_dclk_rate(xadc) / div / 26;
-
+		*val = ret;
 		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
 }
 
-static int xadc_write_raw(struct iio_dev *indio_dev,
-	struct iio_chan_spec const *chan, int val, int val2, long info)
+static int xadc_write_samplerate(struct xadc *xadc, int val)
 {
-	struct xadc *xadc = iio_priv(indio_dev);
 	unsigned long clk_rate = xadc_get_dclk_rate(xadc);
 	unsigned int div;
 
 	if (!clk_rate)
 		return -EINVAL;
 
-	if (info != IIO_CHAN_INFO_SAMP_FREQ)
-		return -EINVAL;
-
 	if (val <= 0)
 		return -EINVAL;
 
 	/* Max. 150 kSPS */
-	if (val > 150000)
-		val = 150000;
+	if (val > XADC_MAX_SAMPLERATE)
+		val = XADC_MAX_SAMPLERATE;
 
 	val *= 26;
 
@@ -938,7 +954,7 @@ static int xadc_write_raw(struct iio_dev *indio_dev,
 	 * limit.
 	 */
 	div = clk_rate / val;
-	if (clk_rate / div / 26 > 150000)
+	if (clk_rate / div / 26 > XADC_MAX_SAMPLERATE)
 		div++;
 	if (div < 2)
 		div = 2;
@@ -949,6 +965,17 @@ static int xadc_write_raw(struct iio_dev *indio_dev,
 		div << XADC_CONF2_DIV_OFFSET);
 }
 
+static int xadc_write_raw(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, int val, int val2, long info)
+{
+	struct xadc *xadc = iio_priv(indio_dev);
+
+	if (info != IIO_CHAN_INFO_SAMP_FREQ)
+		return -EINVAL;
+
+	return xadc_write_samplerate(xadc, val);
+}
+
 static const struct iio_event_spec xadc_temp_events[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -1234,6 +1261,21 @@ static int xadc_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_free_samplerate_trigger;
 
+	/*
+	 * Make sure not to exceed the maximum samplerate since otherwise the
+	 * resulting interrupt storm will soft-lock the system.
+	 */
+	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
+		ret = xadc_read_samplerate(xadc);
+		if (ret < 0)
+			goto err_free_samplerate_trigger;
+		if (ret > XADC_MAX_SAMPLERATE) {
+			ret = xadc_write_samplerate(xadc, XADC_MAX_SAMPLERATE);
+			if (ret < 0)
+				goto err_free_samplerate_trigger;
+		}
+	}
+
 	ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
 			dev_name(&pdev->dev), indio_dev);
 	if (ret)
-- 
2.20.1

[PATCH 5/5] iio: xilinx-xadc: Fix typo

[Lars-Peter Clausen]

Fix a typo. 'at the a time' -> 'at a time'.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/adc/xilinx-xadc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index c724b8788007..d7fecab9252e 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -663,7 +663,7 @@ static int xadc_trigger_set_state(struct iio_trigger *trigger, bool state)
 	mutex_lock(&xadc->mutex);
 
 	if (state) {
-		/* Only one of the two triggers can be active at the a time. */
+		/* Only one of the two triggers can be active at a time. */
 		if (xadc->trigger != NULL) {
 			ret = -EBUSY;
 			goto err_out;
-- 
2.20.1

Re: [PATCH 1/5] iio: xilinx-xadc: Fix ADC-B powerdown

[Jonathan Cameron]

On Fri,  3 Apr 2020 15:27:13 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> The check for shutting down the second ADC is inverted. This causes it to
> be powered down when it should be enabled. As a result channels that are
> supposed to be handled by the second ADC return invalid conversion results.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Fixes tag?  Definitely sounds like something we should be backporting!

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 2d6505a66511..4fcf1729341f 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -722,13 +722,14 @@ static int xadc_power_adc_b(struct xadc *xadc, unsigned int seq_mode)
>  {
>  	uint16_t val;
>  
> +	/* Powerdown the ADC-B when it is not needed. */
>  	switch (seq_mode) {
>  	case XADC_CONF1_SEQ_SIMULTANEOUS:
>  	case XADC_CONF1_SEQ_INDEPENDENT:
> -		val = XADC_CONF2_PD_ADC_B;
> +		val = 0;
>  		break;
>  	default:
> -		val = 0;
> +		val = XADC_CONF2_PD_ADC_B;
>  		break;
>  	}
>  

Re: [PATCH 2/5] iio: xilinx-xadc: Fix clearing interrupt when enabling trigger

[Jonathan Cameron]

On Fri,  3 Apr 2020 15:27:14 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> When enabling the trigger and unmasking the end-of-sequence (EOS) interrupt
> the EOS interrupt should be cleared from the status register. Otherwise it
> is possible that it was still set from a previous capture. If that is the
> case the interrupt would fire immediately even though no conversion has
> been done yet and stale data is being read from the device.
> 
> The old code only clears the interrupt if the interrupt was previously
> unmasked. Which does not make much sense since the interrupt is always
> masked at this point and in addition masking the interrupt does not clear
> the interrupt from the status register. So the clearing needs to be done
> unconditionally.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Looks good but were is my fixes tag?  :) I fear we have a theme
here!

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 4fcf1729341f..04a2a609ced4 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -674,7 +674,7 @@ static int xadc_trigger_set_state(struct iio_trigger *trigger, bool state)
>  
>  	spin_lock_irqsave(&xadc->lock, flags);
>  	xadc_read_reg(xadc, XADC_AXI_REG_IPIER, &val);
> -	xadc_write_reg(xadc, XADC_AXI_REG_IPISR, val & XADC_AXI_INT_EOS);
> +	xadc_write_reg(xadc, XADC_AXI_REG_IPISR, XADC_AXI_INT_EOS);
>  	if (state)
>  		val |= XADC_AXI_INT_EOS;
>  	else

Re: [PATCH 1/5] iio: xilinx-xadc: Fix ADC-B powerdown

[Lars-Peter Clausen]

On 4/5/20 2:10 PM, Jonathan Cameron wrote:
> On Fri,  3 Apr 2020 15:27:13 +0200
> Lars-Peter Clausen <lars@metafoo.de> wrote:
>
>> The check for shutting down the second ADC is inverted. This causes it to
>> be powered down when it should be enabled. As a result channels that are
>> supposed to be handled by the second ADC return invalid conversion results.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Fixes tag?  Definitely sounds like something we should be backporting!

Fixes: bdc8cda1d010 ("iio:adc: Add Xilinx XADC driver")

>
> Jonathan
>
>> ---
>>   drivers/iio/adc/xilinx-xadc-core.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
>> index 2d6505a66511..4fcf1729341f 100644
>> --- a/drivers/iio/adc/xilinx-xadc-core.c
>> +++ b/drivers/iio/adc/xilinx-xadc-core.c
>> @@ -722,13 +722,14 @@ static int xadc_power_adc_b(struct xadc *xadc, unsigned int seq_mode)
>>   {
>>   	uint16_t val;
>>   
>> +	/* Powerdown the ADC-B when it is not needed. */
>>   	switch (seq_mode) {
>>   	case XADC_CONF1_SEQ_SIMULTANEOUS:
>>   	case XADC_CONF1_SEQ_INDEPENDENT:
>> -		val = XADC_CONF2_PD_ADC_B;
>> +		val = 0;
>>   		break;
>>   	default:
>> -		val = 0;
>> +		val = XADC_CONF2_PD_ADC_B;
>>   		break;
>>   	}
>>   

Re: [PATCH 3/5] iio: xilinx-xadc: Fix sequencer configuration for aux channels in simultaneous mode

[Jonathan Cameron]

On Fri,  3 Apr 2020 15:27:15 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> The XADC has two internal ADCs. Depending on the mode it is operating in
> either one or both of them are used. The device manual calls this
> continuous (one ADC) and simultaneous (both ADCs) mode.
> 
> The meaning of the sequencing register for the aux channels changes
> depending on the mode.
> 
> In continuous mode each bit corresponds to one of the 16 aux channels. And
> the single ADC will convert them one by one in order.
> 
> In simultaneous mode the aux channels are split into two groups the first 8
> channels are assigned to the first ADC and the other 8 channels to the
> second ADC. The upper 8 bits of the sequencing register are unused and the
> lower 8 bits control both ADCs. This means a bit needs to be set if either
> the corresponding channel from the first group or the second group (or
> both) are set.
> 
> Currently the driver does not have the special handling required for
> simultaneous mode. Add it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Looks fine, but sounds like a fix, so when was the problem introduced?

Thanks,

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 04a2a609ced4..4acababda4d5 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -798,6 +798,16 @@ static int xadc_preenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto err;
>  
> +	/*
> +	 * In simultaneous mode the upper and lower aux channels are samples at
> +	 * the same time. In this mode the upper 8 bits in the sequencer
> +	 * register are don't care and the lower 8 bits control two channels
> +	 * each. As such we must set the bit if either the channel in the lower
> +	 * group or the upper group is enabled.
> +	 */
> +	if (seq_mode == XADC_CONF1_SEQ_SIMULTANEOUS)
> +		scan_mask = ((scan_mask >> 8) | scan_mask) & 0xff0000;
> +
>  	ret = xadc_write_adc_reg(xadc, XADC_REG_SEQ(1), scan_mask >> 16);
>  	if (ret)
>  		goto err;

Re: [PATCH 4/5] iio: xilinx-xadc: Make sure not exceed maximum samplerate

[Jonathan Cameron]

On Fri,  3 Apr 2020 15:27:16 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> The XADC supports a samplerate of up to 1MSPS. Unfortunately the hardware
> does not have a FIFO, which means it generates an interrupt for each
> conversion sequence. At one 1MSPS this creates an interrupt storm that
> causes the system to soft-lock.
> 
> For this reason the driver limits the maximum samplerate to 150kSPS.
> Currently this check is only done when setting a new samplerate. But it is
> also possible that the initial samplerate configured in the FPGA bitstream
> exceeds the limit.
> 
> In this case when starting to capture data without first changing the
> samplerate the system can overload.
> 
> To prevent this check the currently configured samplerate in the probe
> function and reduce it to the maximum if necessary.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Looks sensible to me.  This one is a bit more marginal as a 'fix' or
a workaround for silly fpga configurations.  Still probably best to
just give it a fixes tag and merge it with the reset of the series.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 78 +++++++++++++++++++++++-------
>  1 file changed, 60 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 4acababda4d5..c724b8788007 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -102,6 +102,16 @@ static const unsigned int XADC_ZYNQ_UNMASK_TIMEOUT = 500;
>  
>  #define XADC_FLAGS_BUFFERED BIT(0)
>  
> +/*
> + * The XADC hardware supports a samplerate of up to 1MSPS. Unfortunately it does
> + * not have a hardware FIFO. Which means an interrupt is generated for each
> + * conversion sequence. At 1MSPS sample rate the CPU in ZYNQ7000 is completely
> + * overloaded by the interrupts that it soft-lockups. For this reason the driver
> + * limits the maximum samplerate 150kSPS. At this rate the CPU is fairly busy,
> + * but still responsive.
> + */
> +#define XADC_MAX_SAMPLERATE 150000
> +
>  static void xadc_write_reg(struct xadc *xadc, unsigned int reg,
>  	uint32_t val)
>  {
> @@ -834,11 +844,27 @@ static const struct iio_buffer_setup_ops xadc_buffer_ops = {
>  	.postdisable = &xadc_postdisable,
>  };
>  
> +static int xadc_read_samplerate(struct xadc *xadc)
> +{
> +	unsigned int div;
> +	uint16_t val16;
> +	int ret;
> +
> +	ret = xadc_read_adc_reg(xadc, XADC_REG_CONF2, &val16);
> +	if (ret)
> +		return ret;
> +
> +	div = (val16 & XADC_CONF2_DIV_MASK) >> XADC_CONF2_DIV_OFFSET;
> +	if (div < 2)
> +		div = 2;
> +
> +	return xadc_get_dclk_rate(xadc) / div / 26;
> +}
> +
>  static int xadc_read_raw(struct iio_dev *indio_dev,
>  	struct iio_chan_spec const *chan, int *val, int *val2, long info)
>  {
>  	struct xadc *xadc = iio_priv(indio_dev);
> -	unsigned int div;
>  	uint16_t val16;
>  	int ret;
>  
> @@ -891,41 +917,31 @@ static int xadc_read_raw(struct iio_dev *indio_dev,
>  		*val = -((273150 << 12) / 503975);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		ret = xadc_read_adc_reg(xadc, XADC_REG_CONF2, &val16);
> -		if (ret)
> +		ret = xadc_read_samplerate(xadc);
> +		if (ret < 0)
>  			return ret;
>  
> -		div = (val16 & XADC_CONF2_DIV_MASK) >> XADC_CONF2_DIV_OFFSET;
> -		if (div < 2)
> -			div = 2;
> -
> -		*val = xadc_get_dclk_rate(xadc) / div / 26;
> -
> +		*val = ret;
>  		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
>  	}
>  }
>  
> -static int xadc_write_raw(struct iio_dev *indio_dev,
> -	struct iio_chan_spec const *chan, int val, int val2, long info)
> +static int xadc_write_samplerate(struct xadc *xadc, int val)
>  {
> -	struct xadc *xadc = iio_priv(indio_dev);
>  	unsigned long clk_rate = xadc_get_dclk_rate(xadc);
>  	unsigned int div;
>  
>  	if (!clk_rate)
>  		return -EINVAL;
>  
> -	if (info != IIO_CHAN_INFO_SAMP_FREQ)
> -		return -EINVAL;
> -
>  	if (val <= 0)
>  		return -EINVAL;
>  
>  	/* Max. 150 kSPS */
> -	if (val > 150000)
> -		val = 150000;
> +	if (val > XADC_MAX_SAMPLERATE)
> +		val = XADC_MAX_SAMPLERATE;
>  
>  	val *= 26;
>  
> @@ -938,7 +954,7 @@ static int xadc_write_raw(struct iio_dev *indio_dev,
>  	 * limit.
>  	 */
>  	div = clk_rate / val;
> -	if (clk_rate / div / 26 > 150000)
> +	if (clk_rate / div / 26 > XADC_MAX_SAMPLERATE)
>  		div++;
>  	if (div < 2)
>  		div = 2;
> @@ -949,6 +965,17 @@ static int xadc_write_raw(struct iio_dev *indio_dev,
>  		div << XADC_CONF2_DIV_OFFSET);
>  }
>  
> +static int xadc_write_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, int val, int val2, long info)
> +{
> +	struct xadc *xadc = iio_priv(indio_dev);
> +
> +	if (info != IIO_CHAN_INFO_SAMP_FREQ)
> +		return -EINVAL;
> +
> +	return xadc_write_samplerate(xadc, val);
> +}
> +
>  static const struct iio_event_spec xadc_temp_events[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> @@ -1234,6 +1261,21 @@ static int xadc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_free_samplerate_trigger;
>  
> +	/*
> +	 * Make sure not to exceed the maximum samplerate since otherwise the
> +	 * resulting interrupt storm will soft-lock the system.
> +	 */
> +	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
> +		ret = xadc_read_samplerate(xadc);
> +		if (ret < 0)
> +			goto err_free_samplerate_trigger;
> +		if (ret > XADC_MAX_SAMPLERATE) {
> +			ret = xadc_write_samplerate(xadc, XADC_MAX_SAMPLERATE);
> +			if (ret < 0)
> +				goto err_free_samplerate_trigger;
> +		}
> +	}
> +
>  	ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
>  			dev_name(&pdev->dev), indio_dev);
>  	if (ret)

Re: [PATCH 5/5] iio: xilinx-xadc: Fix typo

[Jonathan Cameron]

On Fri,  3 Apr 2020 15:27:17 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> Fix a typo. 'at the a time' -> 'at a time'.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Independent of the rest of the series so I applied this one via the slow route.
Mainly so we can forget about it if the others are going via the fixes route.

Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index c724b8788007..d7fecab9252e 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -663,7 +663,7 @@ static int xadc_trigger_set_state(struct iio_trigger *trigger, bool state)
>  	mutex_lock(&xadc->mutex);
>  
>  	if (state) {
> -		/* Only one of the two triggers can be active at the a time. */
> +		/* Only one of the two triggers can be active at a time. */
>  		if (xadc->trigger != NULL) {
>  			ret = -EBUSY;
>  			goto err_out;

Re: [PATCH 1/5] iio: xilinx-xadc: Fix ADC-B powerdown

[Jonathan Cameron]

On Sun, 5 Apr 2020 14:13:32 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 4/5/20 2:10 PM, Jonathan Cameron wrote:
> > On Fri,  3 Apr 2020 15:27:13 +0200
> > Lars-Peter Clausen <lars@metafoo.de> wrote:
> >  
> >> The check for shutting down the second ADC is inverted. This causes it to
> >> be powered down when it should be enabled. As a result channels that are
> >> supposed to be handled by the second ADC return invalid conversion results.
> >>
> >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>  
> > Fixes tag?  Definitely sounds like something we should be backporting!  
> 
> Fixes: bdc8cda1d010 ("iio:adc: Add Xilinx XADC driver")
For all of them? (just checking)

Jonathan

> 
> >
> > Jonathan
> >  
> >> ---
> >>   drivers/iio/adc/xilinx-xadc-core.c | 5 +++--
> >>   1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> >> index 2d6505a66511..4fcf1729341f 100644
> >> --- a/drivers/iio/adc/xilinx-xadc-core.c
> >> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> >> @@ -722,13 +722,14 @@ static int xadc_power_adc_b(struct xadc *xadc, unsigned int seq_mode)
> >>   {
> >>   	uint16_t val;
> >>   
> >> +	/* Powerdown the ADC-B when it is not needed. */
> >>   	switch (seq_mode) {
> >>   	case XADC_CONF1_SEQ_SIMULTANEOUS:
> >>   	case XADC_CONF1_SEQ_INDEPENDENT:
> >> -		val = XADC_CONF2_PD_ADC_B;
> >> +		val = 0;
> >>   		break;
> >>   	default:
> >> -		val = 0;
> >> +		val = XADC_CONF2_PD_ADC_B;
> >>   		break;
> >>   	}
> >>     
> 
> 

Re: [PATCH 1/5] iio: xilinx-xadc: Fix ADC-B powerdown

[Lars-Peter Clausen]

On 4/5/20 2:26 PM, Jonathan Cameron wrote:
> On Sun, 5 Apr 2020 14:13:32 +0200
> Lars-Peter Clausen <lars@metafoo.de> wrote:
>
>> On 4/5/20 2:10 PM, Jonathan Cameron wrote:
>>> On Fri,  3 Apr 2020 15:27:13 +0200
>>> Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>   
>>>> The check for shutting down the second ADC is inverted. This causes it to
>>>> be powered down when it should be enabled. As a result channels that are
>>>> supposed to be handled by the second ADC return invalid conversion results.
>>>>
>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>> Fixes tag?  Definitely sounds like something we should be backporting!
>> Fixes: bdc8cda1d010 ("iio:adc: Add Xilinx XADC driver")
> For all of them? (just checking)
Yes, took 6 years for somebody to notice :)

Re: [PATCH 1/5] iio: xilinx-xadc: Fix ADC-B powerdown

[Jonathan Cameron]

On Sun, 5 Apr 2020 14:29:00 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 4/5/20 2:26 PM, Jonathan Cameron wrote:
> > On Sun, 5 Apr 2020 14:13:32 +0200
> > Lars-Peter Clausen <lars@metafoo.de> wrote:
> >  
> >> On 4/5/20 2:10 PM, Jonathan Cameron wrote:  
> >>> On Fri,  3 Apr 2020 15:27:13 +0200
> >>> Lars-Peter Clausen <lars@metafoo.de> wrote:
> >>>     
> >>>> The check for shutting down the second ADC is inverted. This causes it to
> >>>> be powered down when it should be enabled. As a result channels that are
> >>>> supposed to be handled by the second ADC return invalid conversion results.
> >>>>
> >>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>  
> >>> Fixes tag?  Definitely sounds like something we should be backporting!  
> >> Fixes: bdc8cda1d010 ("iio:adc: Add Xilinx XADC driver")  
> > For all of them? (just checking)  
> Yes, took 6 years for somebody to notice :)
We've had ones that took longer :)

Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan