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