Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751975AbdHaQpI (ORCPT <rfc822;w@1wt.eu>);
        Thu, 31 Aug 2017 12:45:08 -0400
Received: from mail.linuxfoundation.org ([140.211.169.12]:46794 "EHLO
        mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751811AbdHaQpH (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 31 Aug 2017 12:45:07 -0400
Date: Thu, 31 Aug 2017 18:45:11 +0200
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: linux-kernel@vger.kernel.org, cphealy@gmail.com,
        Lucas Stach <l.stach@pengutronix.de>,
        Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
        Lee Jones <lee.jones@linaro.org>, Pavel Machek <pavel@ucw.cz>
Subject: Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory
 Processor
Message-ID: <20170831164511.GA7247@kroah.com>
References: <20170828163131.24815-1-andrew.smirnov@gmail.com>
 <20170828163131.24815-2-andrew.smirnov@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170828163131.24815-2-andrew.smirnov@gmail.com>
User-Agent: Mutt/1.8.3 (2017-05-23)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2711
Lines: 106

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