| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <849280cfb709f14f6407925c750547ee4bfa8fff.camel@analog.com>
Date: Mon, 9 Mar 2020 11:51:12 +0000
From: "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To: "jic23@...nel.org" <jic23@...nel.org>
CC: "Hennerich, Michael" <Michael.Hennerich@...log.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"lars@...afoo.de" <lars@...afoo.de>
Subject: Re: [PATCH v8 7/8] iio: adc: ad9467: add support AD9467 ADC
On Sat, 2020-03-07 at 15:05 +0000, Jonathan Cameron wrote:
> [External]
>
> On Fri, 6 Mar 2020 13:00:59 +0200
> Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
>
> > From: Michael Hennerich <michael.hennerich@...log.com>
> >
> > The AD9467 is a 16-bit, monolithic, IF sampling analog-to-digital converter
> > (ADC). It is optimized for high performanceover wide bandwidths and ease of
> > use. The product operates at a 250 MSPS conversion rate and is designed for
> > wireless receivers, instrumentation, and test equipment that require a high
> > dynamic range. The ADC requires 1.8 V and 3.3 V power supplies and a low
> > voltage differential input clock for full performance operation. No
> > external reference or driver components are required for many applications.
> > Data outputs are LVDS compatible (ANSI-644 compatible) and include the
> > means to reduce the overall current needed for short trace distances.
> >
> > Since the chip can operate at such high sample-rates (much higher than
> > classical interfaces), it requires that a DMA controller be used to
> > interface directly to the chip and push data into memory.
> > Typically, the AXI ADC IP core is used to interface with it.
> >
> > Link:
> > https://www.analog.com/media/en/technical-documentation/data-sheets/AD9467.pdf
> >
> > Signed-off-by: Lars-Peter Clausen <lars@...afoo.de>
> > Signed-off-by: Michael Hennerich <michael.hennerich@...log.com>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
>
> A few minor things but otherwise looks good to me..
>
> > ---
> > drivers/iio/adc/Kconfig | 15 ++
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ad9467.c | 432 +++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 448 insertions(+)
> > create mode 100644 drivers/iio/adc/ad9467.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 445070abf376..a0796510f9d4 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -246,6 +246,21 @@ config AD799X
> > To compile this driver as a module, choose M here: the module will be
> > called ad799x.
> >
> ...
> > +static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> > +{
> > + unsigned char buf[3];
> > + int ret;
> > +
> > + buf[0] = 0x80 | (reg >> 8);
> > + buf[1] = reg & 0xFF;
> > +
> > + ret = spi_write_then_read(spi, &buf[0], 2, &buf[2], 1);
>
> Why not split buf into send part and receive? Might make it slightly
> more readable for no actual cost..
sure
>
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + return buf[2];
> > +}
> ...
>
> > +static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + const struct adi_axi_adc_chip_info *info = conv->chip_info;
> > + struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> > + unsigned long r_clk;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + return ad9467_set_scale(conv, val, val2);
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + if (!st->clk)
> > + return -ENODEV;
> > +
> > + if (chan->extend_name)
>
> This is a very 'odd' test. Why?
left-over from the original driver; and it slipped when i adapted from that;
no idea why it was added; and git history is not helping either;
will drop;
>
> > + return -ENODEV;
> > +
> > + r_clk = clk_round_rate(st->clk, val);
> > + if (r_clk < 0 || r_clk > info->max_rate) {
> > + dev_warn(&st->spi->dev,
> > + "Error setting ADC sample rate %ld", r_clk);
> > + return -EINVAL;
> > + }
> > +
> > + return clk_set_rate(st->clk, r_clk);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> ...
> > +static int ad9467_probe(struct spi_device *spi)
> > +{
> > + const struct of_device_id *oid;
> > + struct adi_axi_adc_conv *conv;
> > + struct ad9467_state *st;
> > + unsigned int id;
> > + int ret;
> > +
> > + if (!spi->dev.of_node) {
> > + dev_err(&spi->dev, "DT node is null\n");
> > + return -ENODEV;
>
> Silly question for you. Can this happen? We can only probe this
> if it is in DT and hence there must be a node to get here I think.
good point;
will drop;
looks like something i added inertially
>
> > + }
> > +
> > + oid = of_match_node(ad9467_of_match, spi->dev.of_node);
> > + if (!oid)
> > + return -ENODEV;
>
> You only ever want the data field so you can get that directly.
> of_device_get_match_data
ack
>
> > +
> > + conv = devm_adi_axi_adc_conv_register(&spi->dev, sizeof(*st));
> > + if (IS_ERR(conv))
> > + return PTR_ERR(conv);
> > +
> > + st = adi_axi_adc_conv_priv(conv);
> > + st->spi = spi;
> > +
> > + st->clk = devm_clk_get(&spi->dev, "adc-clk");
> > + if (IS_ERR(st->clk))
> > + return PTR_ERR(st->clk);
> > +
> > + ret = clk_prepare_enable(st->clk);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(&spi->dev, ad9467_clk_disable, st);
> > + if (ret)
> > + return ret;
> > +
> > + st->pwrdown_gpio = devm_gpiod_get_optional(&spi->dev, "powerdown",
> > + GPIOD_OUT_LOW);
> > + if (IS_ERR(st->pwrdown_gpio))
> > + return PTR_ERR(st->pwrdown_gpio);
> > +
> > + st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
> > + GPIOD_OUT_LOW);
> > + if (IS_ERR(st->reset_gpio))
> > + return PTR_ERR(st->reset_gpio);
> > +
> > + if (st->reset_gpio) {
> > + udelay(1);
> > + ret = gpiod_direction_output(st->reset_gpio, 1);
> > + mdelay(10);
> > + }
> > +
> > + spi_set_drvdata(spi, st);
> > +
> > + id = (unsigned int)oid->data;
> > + conv->chip_info = &ad9467_chip_info_tbl[id];
> > +
> > + id = ad9467_spi_read(spi, AN877_ADC_REG_CHIP_ID);
> > + if (id != conv->chip_info->id) {
> > + dev_err(&spi->dev, "Unrecognized CHIP_ID 0x%X\n", id);
> > + return -ENODEV;
> > + }
> > +
> > + conv->reg_access = ad9467_reg_access;
> > + conv->write_raw = ad9467_write_raw;
> > + conv->read_raw = ad9467_read_raw;
> > + conv->preenable_setup = ad9467_preenable_setup;
> > +
> > + return ad9467_setup(st, id);
> > +}
> > +
> > +static struct spi_driver ad9467_driver = {
> > + .driver = {
> > + .name = "ad9467",
> > + .of_match_table = ad9467_of_match,
> > + },
> > + .probe = ad9467_probe,
> > +};
> > +module_spi_driver(ad9467_driver);
> > +
> > +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@...log.com>");
> > +MODULE_DESCRIPTION("Analog Devices AD9467 ADC driver");
> > +MODULE_LICENSE("GPL v2");
Powered by blists - more mailing lists