Re: [PATCH 2/2] iio:adc: Add Xilinx XADC driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02/08/2014 03:23 PM, Jonathan Cameron wrote:
On 04/02/14 17:24, Lars-Peter Clausen wrote:
The Xilinx XADC is a ADC that can be found in the series 7 FPGAs from Xilinx.
The XADC has a DRP interface for communication. Currently two different
frontends for the DRP interface exist. One that is only available on the ZYNQ
family as a hardmacro in the SoC portion of the ZYNQ. The other one is
available
on all series 7 platforms and is a softmacro with a AXI interface. This
driver
supports both interfaces and internally has a small abstraction layer that
hides
the specifics of these interfaces from the main driver logic.

The ADC has a couple of internal channels which are used for voltage and
temperature monitoring of the FPGA as well as one primary and up to 16
channels
auxiliary channels for measuring external voltages. The external auxiliary
channels can either be directly connected each to one physical pin on the
FPGA
or they can make use of an external multiplexer which is responsible for
multiplexing the external signals onto one pair of physical pins.

The voltage and temperature monitoring channels also have an event capability
which allows to generate a interrupt when their value falls below or raises
above a set threshold.

Buffered sampling mode is supported by the driver, but only for AXI-XADC
since
the ZYNQ XADC interface does not have capabilities for supporting buffer mode
(no end-of-conversion interrupt).

Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
Hi Lars,

Hi,

Thanks for the review.


One double free bug in the error handling at the end of probe and
a few requests for some explanatory comments.  Otherwise looks great to me.

Could you describe the two triggers in the description section of the patch.


Ok.

Also, if you could insert a few references to the relevant docs from Xilinx
that
would also be great!

I can include the names of the documents which describe the XADC. The URLs seem to change on a regular basis.


Hmm.. To think I read this one first as it looked like it might be the easiest
driver in my inbox to review :)

Yea, its a beast.

[...]
+static void xadc_zynq_write_fifo(struct xadc *xadc, uint32_t *cmd,
+    unsigned int n)
+{
+    unsigned int i;
+
+    for (i = 0; i < n; i++)
+        xadc_write_reg(xadc, XADC_ZYNQ_REG_CFIFO, cmd[i]);
+}
+
+static void xadc_zynq_drain_fifo(struct xadc *xadc)
+{
+    uint32_t status, tmp;
+
+    xadc_read_reg(xadc, XADC_ZYNQ_REG_STATUS, &status);
+
+    while (!(status & XADC_ZYNQ_STATUS_DFIFOE)) {
+        xadc_read_reg(xadc, XADC_ZYNQ_REG_DFIFO, &tmp);
+        xadc_read_reg(xadc, XADC_ZYNQ_REG_STATUS, &status);
+    }
+}
+
+static void xadc_zynq_update_intmsk(struct xadc *xadc, unsigned int mask,
+    unsigned int val)
+{
+    xadc->zynq_intmask &= ~mask;
+    xadc->zynq_intmask |= val;
+
+    xadc_write_reg(xadc, XADC_ZYNQ_REG_INTMSK,
+        xadc->zynq_intmask | xadc->zynq_masked_alarm);
+}
+
Hmm. These are complex beasts. I'm not entirely sure I've understood them
correctly... Might take another look at some point.

The core that talks to the XADC is basically doing jtag do this. You submit register writes/reads to a FIFO and then a couple of moments later you can read the response from a different FIFO. I'll add a few comments explaining this.

[...]
+static void xadc_handle_event(struct iio_dev *indio_dev, unsigned int event)
+{
+    struct xadc *xadc = iio_priv(indio_dev);
+    const struct iio_chan_spec *chan;
+    unsigned int offset;
+    uint16_t val;
+    int ret;
+
+    /* Temperature threshold error, we don't handle this yet */
+    if (event == 0)
+        return;
+
+    if (event < 4)
+        offset = event;
+    else
+        offset = event + 4;
+
+    if (event < 3)
+        chan = &indio_dev->channels[event];
+    else if (event == 3)
+        chan = &indio_dev->channels[0];
+    else
+        chan = &indio_dev->channels[event - 1];
+
+    if (event != 3) {

Is there any guarantee that, by the time we get here, the value will not
have crossed back over the threshold?   Might be better just to not
separate the two and use IIO_EV_DIR_EITHER.  I'm not sure either way on this
one.

Hm, ok. I'm not to sure, but I'll give it a though.

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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux