Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor

[Greg Kroah-Hartman]

On Mon, Aug 28, 2017 at 09:31:30AM -0700, Andrey Smirnov wrote:
> +static int rave_sp_debugfs_create(struct rave_sp *sp)
> +{
> +	struct dentry *file;
> +
> +	sp->debugfs = debugfs_create_dir("rave", NULL);
> +	if (!sp->debugfs)
> +		return -ENOMEM;

Why do you care about the return value of the debugfs function?  Hint,
you don't...

You should never care if debugfs failed, was built in, or succeeded,
your code should just do the same thing always.  Any value returned from
a debugfs call can just be passed to another one, regardless of the
value returned from the first one.

So here, just save it, and don't check it.

> +	file = debugfs_create_file("i2c_device_status", 0444,
> +				   sp->debugfs, sp,
> +				   &rave_sp_i2c_device_status);
> +	if (!file)
> +		goto error;

Nope, you don't care, just make the call.

> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, part_number_firmware);
> +	if (!file)
> +		goto error;

Same for all of these.

> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, part_number_bootloader);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_rev_deb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_rev_rmb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_mod_deb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_mod_rmb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, silicon_devrev);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, silicon_devid);
> +	if (!file)
> +		goto error;
> +
> +	return 0;

And your function can not fail, no need for this to return anything.

> +error:
> +	debugfs_remove_recursive(sp->debugfs);
> +	return -ENOMEM;
> +}
> +
> +static void rave_sp_degugfs_release(struct device *dev, void *res)
> +{
> +	struct rave_sp *sp = *(struct rave_sp **)res;
> +
> +	debugfs_remove_recursive(sp->debugfs);
> +}
> +
> +static int devm_rave_sp_debugfs_create(struct rave_sp *sp)
> +{
> +	struct rave_sp **rcsp;
> +	struct device *dev = &sp->serdev->dev;
> +	int ret;
> +
> +	rcsp = devres_alloc(rave_sp_degugfs_release, sizeof(*rcsp), GFP_KERNEL);
> +	if (!rcsp)
> +		return -ENOMEM;
> +
> +	ret = rave_sp_debugfs_create(sp);
> +	if (!ret) {
> +		*rcsp = sp;
> +		devres_add(dev, rcsp);
> +	} else {
> +		devres_free(rcsp);
> +	}

You should not care what debugfs is doing, if it is working or not.  So
no need to check here either.

debugfs was written to make it dirt-simple to use, I don't know why
people keep trying to put some error handling around it :)

thanks,

greg k-h