lkml.org 
[lkml]   [2020]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core
Date
On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:
> [External]
>
> 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
>

i can send a v12 for this in a few days;

> Is it tag or simple link? I would suggest not to use Link: if it's not a tag.

simple link
any suggestions/alternatives?
i wasn't aware of conventions about this;

>
> ...
>
> > +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?

see [1]

>
> > + 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.

[1]
because 'adi_axi_adc_conv_priv()' (and implicitly conv_to_client()) gets called
from other drivers; we can't expect to be sure that conv & cl aren't NULL;

>
> > +
> > + return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client),
> > IIO_ALIGN);
>
> This all looks a bit confusing. Is it invention of offsetof() ?

umm; tbh, it's more of a copy/clone of iio_priv()

it's not un-common though;
see [and this one has more exposure]:
--------------------------------------------------------
static inline void *netdev_priv(const struct net_device *dev)
{
return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
}
--------------------------------------------------------


>
> > +}
>
> ...
>
> > +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?

i did now;
any hints where i should look closer?

>
> > + cl = kzalloc(alloc_size, GFP_KERNEL);
> > + if (!cl)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + mutex_lock(&registered_clients_lock);
> > +
> > + get_device(dev);
> > + cl->dev = dev;
>
> cl->dev = get_device(dev);

sure

>
> > + list_add_tail(&cl->entry, &registered_clients);
> > +
> > + mutex_unlock(&registered_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?

good point; it isn't;
it's a left-over from when adi_axi_adc_conv_unregister() was exported
still, i wouldn't mind leaving it [for paranoia], if there isn't a strong
opinion to remove it;

>
> > +
> > + mutex_lock(&registered_clients_lock);
> > +
> > + list_del(&cl->entry);
> > + put_device(cl->dev);
> > +
> > + mutex_unlock(&registered_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?

yes
see axi_adc_attr_is_visible()

>
> > +
> > + return len;
> > +}
>
> ...
>
> > +static struct attribute *adi_axi_adc_attributes[] = {
> > + ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
> > + NULL,
>
> Terminators good w/o comma.

i don't feel strongly pro/against
sure

>
> > +};
>
> ...
>
> > +/* 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.


right; it didn't occur to me, since i was already using
of_device_get_match_data() in ad9467

>
> > + 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(&registered_clients_lock);
> > +
> > + list_for_each_entry(cl, &registered_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;

sure

> ?
>
> > + if (!try_module_get(dev->driver->owner)) {
> > + mutex_unlock(&registered_clients_lock);
> > + return ERR_PTR(-ENODEV);
> > + }
> > + get_device(dev);
> > + cl->info = id->data;
> > + mutex_unlock(&registered_clients_lock);
> > + return cl;
> > + }
> > + }
> > +
> > + mutex_unlock(&registered_clients_lock);
> > +
> > + return ERR_PTR(-EPROBE_DEFER);
> > +}
>
>
\
 
 \ /
  Last update: 2020-03-22 10:37    [W:0.099 / U:7.996 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site