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.
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;
> }
> + dev_set_name(&dev->dev, "iio:device%d", dev->id);
> + INIT_LIST_HEAD(&dev->buffer_list);
>
> return dev;
> }
