On Sat, 2020-04-25 at 16:47 +0100, Jonathan Cameron wrote:
> On Mon, 20 Apr 2020 06:17:32 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@xxxxxxxxxx> wrote:
>
> > On Sun, 2020-04-19 at 16:13 +0100, jic23@xxxxxxxxxx wrote:
> > > [External]
> > >
> > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > >
> > > Noticed whilst reviewing Alexandru's patch to the same function.
> > > If we simply flip the logic and return NULL immediately after memory
> > > allocation failure we reduce the indent of the following block and
> > > end up with more 'idiomatic' kernel code.
> > >
> >
> > I also was tempted to do it, but was tempted [a bit more] by the initial
> > change
> > that I goofed.
> >
> > A few thoughts on this [can be ignored].
> > But, since doing this change, should 'dev' be renamed to 'indio_dev'?
> > It shouldn't be a lot more code than the current change [I hope].
> > When looking through IIO core, I got a minor/slight confusion on this alloc
> > code
> > about the name 'dev' [which is of type 'struct iio_dev' vs 'struct device',
> > as
> > is more customary].
> >
> > If 'dev' was chosen to fit within any 80 col-width limit, that limit should
> > be
> > less likely to hit now.
>
> A different type of cleanup, so I think worth a separate patch
> (even though it's messing with the same block of code.)
>
> Got to keep to the rules I pester everyone else into following :)
Fair enough.
I was assuming there would be a preference to keep the patch semantic as simple
as possible.
I thought I'd point them out, since I also looked over this bit a while back.
>
> So I'll apply this as is and might get the dev->indio_dev one out
> after I've caught up with rest of email queue.
>
> Thanks,
>
> Jonathan
>
> > 1 more inline.
> >
> > Well, even with/without these changes.
> >
> > Reviewed-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > > Cc: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> > > ---
> > > drivers/iio/industrialio-core.c | 38 ++++++++++++++++-----------------
> > > 1 file changed, 19 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > > core.c
> > > index f4daf19f2a3b..96f6dacb206d 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -1504,27 +1504,27 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
> > > alloc_size += IIO_ALIGN - 1;
> > >
> > > dev = kzalloc(alloc_size, GFP_KERNEL);
> > > + if (!dev)
> > > + return NULL;
> > >
> > > - if (dev) {
> > > - dev->dev.groups = dev->groups;
> > > - dev->dev.type = &iio_device_type;
> > > - dev->dev.bus = &iio_bus_type;
> > > - device_initialize(&dev->dev);
> > > - dev_set_drvdata(&dev->dev, (void *)dev);
> > > - mutex_init(&dev->mlock);
> > > - mutex_init(&dev->info_exist_lock);
> > > - INIT_LIST_HEAD(&dev->channel_attr_list);
> > > -
> > > - dev->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL);
> > > - if (dev->id < 0) {
> > > - /* cannot use a dev_err as the name isn't available */
> > > - pr_err("failed to get device id\n");
> > > - kfree(dev);
> > > - return NULL;
> > > - }
> > > - dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > > - INIT_LIST_HEAD(&dev->buffer_list);
> > > + dev->dev.groups = dev->groups;
> > > + dev->dev.type = &iio_device_type;
> > > + dev->dev.bus = &iio_bus_type;
> > > + device_initialize(&dev->dev);
> > > + dev_set_drvdata(&dev->dev, (void *)dev);
> > > + mutex_init(&dev->mlock);
> > > + mutex_init(&dev->info_exist_lock);
> > > + INIT_LIST_HEAD(&dev->channel_attr_list);
> > > +
> > > + dev->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL);
> > > + if (dev->id < 0) {
> > > + /* cannot use a dev_err as the name isn't available */
> > > + pr_err("failed to get device id\n");
> > > + kfree(dev);
> > > + return NULL;
> >
> > would it be too much for this patch to move this right after the kzalloc()?
> > no strong opinion from my side to do it or not;
> > but it does save some init cycles, and compresses this init block a bit;
>
> It doesn't really save any cycles because the chance of failure of ID
> allocation
> is negligible... Now I'd agree with you if writing from scratch, but as a
> tidy up patch, it's good to keep things really simple.
>
>
> > > }
> > > + dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > > + INIT_LIST_HEAD(&dev->buffer_list);
> > >
> > > return dev;
> > > }
