On Sun, 2020-03-22 at 12:45 +0200, Andy Shevchenko wrote: > +Cc Kees (see below about allocation size checks) > > On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru > <alexandru.Ardelean@xxxxxxxxxx> wrote: > > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote: > > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean > > > <alexandru.ardelean@xxxxxxxxxx> wrote: > > ... > > > > > 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; > > Use like [1] ... > ... > > [1]: https://... > > Or maybe introduce is as a tag DocLink:, for example? > Or Datasheet: ? > > ... > > > > > +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; > > In both cases it's pointer arithmetic, right? Even look at the example > of netdev you gave below, they haven't done these (redundant) checks. > The outcome that crashes if any will be more distinct. > > > > > + 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); > > } > > -------------------------------------------------------- > > Good point. > > > > > +} > > ... > > > > > +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? > > It seems it lacks of this kind of allocation size checks... Perhaps add one? > Kees, what do you think? i borrowed this allocation logic from IIO core [iio_device_alloc()]; i may be stupid, but i still don't understand how to use overflow.h or what to get from it; the checks in there seem to be a bit too much for what's needed here; or maybe there is something else in some newer linux-tree? or maybe an example of how it's used? > > > > > + cl = kzalloc(alloc_size, GFP_KERNEL); > > > > + if (!cl) > > > > + return ERR_PTR(-ENOMEM); > > ... > > > > > +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; > > I think it makes code dirty (too much protective programming). We have > a lot places where we can shoot our feet, but at least not hiding the > issue is a benefit in my opinion. > > ... > > > > > > > +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 > > There is a rationale behind this. If there is a weird case of adding > entry behind the terminator, you will see it immediately at compile > time (consider automatic rebase). > > > > > +}; > > > > > > ... > > > > > > > +/* 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. > > Ditto. > > > > > +}; > > ... > > > > > + if (!dev->of_node) { > > > > + dev_err(dev, "DT node is null\n"); > > > > + return ERR_PTR(-ENODEV); > > > > + } > > I guess this check is redundant since following OF calls will fail anyway. > > > > > + > > > > + 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 > > Even better. But see above note. > > > > > + if (!id) > > > > + return ERR_PTR(-ENODEV);
