From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Alexandru Ardelean <alexandru.ardelean@analog.com>
Cc: linux-iio <linux-iio@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
devicetree <devicetree@vger.kernel.org>,
Jonathan Cameron <jic23@kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Laszlo.Nagy@analog.com, Andrei.Grozav@analog.com,
Michael Hennerich <Michael.Hennerich@analog.com>,
Istvan.Csomortani@analog.com, Adrian.Costina@analog.com,
Dragos.Bogdan@analog.com, Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core
Date: Sat, 21 Mar 2020 23:38:18 +0200 [thread overview]
Message-ID: <CAHp75VecnornqckmG_WgN-V9A1VSQfRT85TxFzwHgaLw9dAHeA@mail.gmail.com> (raw)
In-Reply-To: <20200321085315.11030-6-alexandru.ardelean@analog.com>
On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
<alexandru.ardelean@analog.com> wrote:
>
> From: Michael Hennerich <michael.hennerich@analog.com>
>
> This change adds support for the Analog Devices Generic AXI ADC IP core.
> The IP core is used for interfacing with analog-to-digital (ADC) converters
> that require either a high-speed serial interface (JESD204B/C) or a source
> synchronous parallel interface (LVDS/CMOS).
>
> Usually, some other interface type (i.e SPI) is used as a control interface
> for the actual ADC, while the IP core (controlled via this driver), will
> interface to the data-lines of the ADC and handle the streaming of data
> into memory via DMA.
>
> Because of this, the AXI ADC driver needs the other SPI-ADC driver to
> register with it. The SPI-ADC needs to be register via the SPI framework,
> while the AXI ADC registers as a platform driver. The two cannot be ordered
> in a hierarchy as both drivers have their own registers, and trying to
> organize this [in a hierarchy becomes] problematic when trying to map
> memory/registers.
>
> There are some modes where the AXI ADC can operate as standalone ADC, but
> those will be implemented at a later point in time.
>
> Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
Is it tag or simple link? I would suggest not to use Link: if it's not a tag.
...
> +static struct adi_axi_adc_client *conv_to_client(struct adi_axi_adc_conv *conv)
> +{
> + if (!conv)
> + return NULL;
This is so unusual. Why do you need it?
> + return container_of(conv, struct adi_axi_adc_client, conv);
> +}
> +
> +void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
> +{
> + struct adi_axi_adc_client *cl = conv_to_client(conv);
> +
> + if (!cl)
> + return NULL;
So about this.
> +
> + return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client), IIO_ALIGN);
This all looks a bit confusing. Is it invention of offsetof() ?
> +}
...
> +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device *dev,
> + int sizeof_priv)
> +{
> + struct adi_axi_adc_client *cl;
> + size_t alloc_size;
> +
> + alloc_size = sizeof(struct adi_axi_adc_client);
> + if (sizeof_priv) {
> + alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> + alloc_size += sizeof_priv;
> + }
> + alloc_size += IIO_ALIGN - 1;
Have you looked at linux/overflow.h?
> + cl = kzalloc(alloc_size, GFP_KERNEL);
> + if (!cl)
> + return ERR_PTR(-ENOMEM);
> +
> + mutex_lock(®istered_clients_lock);
> +
> + get_device(dev);
> + cl->dev = dev;
cl->dev = get_device(dev);
> + list_add_tail(&cl->entry, ®istered_clients);
> +
> + mutex_unlock(®istered_clients_lock);
> +
> + return &cl->conv;
> +}
> +
> +static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
> +{
> + struct adi_axi_adc_client *cl = conv_to_client(conv);
> +
> + if (!cl)
> + return;
When is this possible?
> +
> + mutex_lock(®istered_clients_lock);
> +
> + list_del(&cl->entry);
> + put_device(cl->dev);
> +
> + mutex_unlock(®istered_clients_lock);
> +
> + kfree(cl);
> +}
...
> +static ssize_t in_voltage_scale_available_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + for (i = 0; i < conv->chip_info->num_scales; i++) {
> + const unsigned int *s = conv->chip_info->scale_table[i];
> +
> + len += scnprintf(buf + len, PAGE_SIZE - len,
> + "%u.%06u ", s[0], s[1]);
> + }
> + buf[len - 1] = '\n';
Is num_scales guaranteed to be great than 0 whe we call this?
> +
> + return len;
> +}
...
> +static struct attribute *adi_axi_adc_attributes[] = {
> + ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
> + NULL,
Terminators good w/o comma.
> +};
...
> +/* Match table for of_platform binding */
> +static const struct of_device_id adi_axi_adc_of_match[] = {
> + { .compatible = "adi,axi-adc-10.0.a", .data = &adi_axi_adc_10_0_a_info },
> + { /* end of list */ },
Ditto.
> +};
...
> +struct adi_axi_adc_client *adi_axi_adc_attach_client(struct device *dev)
> +{
> + const struct of_device_id *id;
> + struct adi_axi_adc_client *cl;
> + struct device_node *cln;
> +
> + if (!dev->of_node) {
> + dev_err(dev, "DT node is null\n");
> + return ERR_PTR(-ENODEV);
> + }
> +
> + id = of_match_node(adi_axi_adc_of_match, dev->of_node);
You may use this from struct driver and move the table after this function.
> + if (!id)
> + return ERR_PTR(-ENODEV);
> +
> + cln = of_parse_phandle(dev->of_node, "adi,adc-dev", 0);
> + if (!cln) {
> + dev_err(dev, "No 'adi,adc-dev' node defined\n");
> + return ERR_PTR(-ENODEV);
> + }
> +
> + mutex_lock(®istered_clients_lock);
> +
> + list_for_each_entry(cl, ®istered_clients, entry) {
> + if (!cl->dev)
> + continue;
> + if (cl->dev->of_node == cln) {
So, why not to be consistent with above, i.e.
if (of_node != cln)
continue;
?
> + if (!try_module_get(dev->driver->owner)) {
> + mutex_unlock(®istered_clients_lock);
> + return ERR_PTR(-ENODEV);
> + }
> + get_device(dev);
> + cl->info = id->data;
> + mutex_unlock(®istered_clients_lock);
> + return cl;
> + }
> + }
> +
> + mutex_unlock(®istered_clients_lock);
> +
> + return ERR_PTR(-EPROBE_DEFER);
> +}
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2020-03-21 21:38 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-21 8:53 [PATCH v11 0/8] iio: adi-axi-adc,ad9647: Add support for AD9467 ADC Alexandru Ardelean
2020-03-21 8:53 ` [PATCH v11 1/8] include: fpga: adi-axi-common.h: fixup whitespace tab -> space Alexandru Ardelean
2020-03-21 8:53 ` [PATCH v11 2/8] include: fpga: adi-axi-common.h: add version helper macros Alexandru Ardelean
2020-03-21 17:21 ` Jonathan Cameron
2020-03-21 8:53 ` [PATCH v11 3/8] iio: buffer-dmaengine: use %zu specifier for sprintf(align) Alexandru Ardelean
2020-03-21 17:22 ` Jonathan Cameron
2020-03-21 8:53 ` [PATCH v11 4/8] iio: buffer-dmaengine: add dev-managed calls for buffer alloc Alexandru Ardelean
2020-03-21 17:22 ` Jonathan Cameron
2020-03-21 8:53 ` [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core Alexandru Ardelean
2020-03-21 17:15 ` Jonathan Cameron
2020-03-21 21:38 ` Andy Shevchenko [this message]
2020-03-22 9:35 ` Ardelean, Alexandru
2020-03-22 10:45 ` Andy Shevchenko
2020-03-22 16:11 ` Ardelean, Alexandru
2020-03-22 16:16 ` Kees Cook
2020-03-22 16:31 ` Ardelean, Alexandru
2020-03-22 16:44 ` Ardelean, Alexandru
2020-03-22 16:53 ` Jonathan Cameron
2020-03-22 17:40 ` Ardelean, Alexandru
2020-03-22 18:26 ` Jonathan Cameron
2020-03-24 7:10 ` Ardelean, Alexandru
2020-03-22 15:20 ` Jonathan Cameron
2020-03-21 8:53 ` [PATCH v11 6/8] dt-bindings: iio: adc: add bindings doc for AXI ADC driver Alexandru Ardelean
2020-03-21 17:23 ` Jonathan Cameron
2020-03-21 8:53 ` [PATCH v11 7/8] iio: adc: ad9467: add support AD9467 ADC Alexandru Ardelean
2020-03-21 17:23 ` Jonathan Cameron
2020-03-21 8:53 ` [PATCH v11 8/8] dt-bindings: iio: adc: add bindings doc for " Alexandru Ardelean
2020-03-21 17:24 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHp75VecnornqckmG_WgN-V9A1VSQfRT85TxFzwHgaLw9dAHeA@mail.gmail.com \
--to=andy.shevchenko@gmail.com \
--cc=Adrian.Costina@analog.com \
--cc=Andrei.Grozav@analog.com \
--cc=Dragos.Bogdan@analog.com \
--cc=Istvan.Csomortani@analog.com \
--cc=Laszlo.Nagy@analog.com \
--cc=Michael.Hennerich@analog.com \
--cc=alexandru.ardelean@analog.com \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).