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@xxxxxxxxxx> wrote: > > From: Michael Hennerich <michael.hennerich@xxxxxxxxxx> > > > > 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(®istered_clients_lock); > > + > > + get_device(dev); > > + cl->dev = dev; > > cl->dev = get_device(dev); sure > > > + 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? 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(®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? 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(®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; sure > ? > > > + 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); > > +} > >
