Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used

[David Howells <dhowells@redhat.com>]

Justin P. Mattock <justinmattock@gmail.com> wrote:

>  	if (!ACPI_FAILURE(status)) {
> -		int ret;
> 
> -		ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
> +		fn = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
>  				"firmware_node");
> -		ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
> +		pn = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
>  				"physical_node");
> +	if (fn) {

That new if-statement still needs indenting one more tab stop.  It's indented
the same as the previous if-statement, but is actually in the body of that
previous if-statement.

The body of the second if-statement should be indented one tab beyond the if,
and else/else-if statements and the final closing brace should be indented
level with the if:

	if (...) {
		body;
	} else if (...) {
		body;
	} else {
		body;
	}

so that they line up vertically.

> +		dev_warn(dev, "dev:%p Failed to create firmware_node: %d\n",
> +			acpi_dev, fn);

The "dev:%p " seems like it ought to be superfluous if you're using
dev_warn(), and certainly, returning the pointer isn't really useful, I
suspect.

However, at this point you have two device struct pointers: dev and
&acip_dev->dev, so printing them both is may be good.  Perhaps something like:

+		dev_warn(&acpi_dev->dev,
+			 "Failed to create firmware_node link to %s %s: %d\n",
+			 dev_driver_string(dev), dev_name(dev), fn);

David