Message-ID: <48539E1A.90601@keyaccess.nl>
Date: Sat, 14 Jun 2008 12:31:54 +0200
From: Rene Herman <rene.herman@keyaccess.nl>
User-Agent: Thunderbird 2.0.0.14 (X11/20080421)
MIME-Version: 1.0
To: Bjorn Helgaas <bjorn.helgaas@hp.com>
CC: Len Brown <lenb@kernel.org>, linux-acpi@vger.kernel.org,
       linux-kernel@vger.kernel.org, Adam Belay <ambx1@neo.rr.com>,
       Adam M Belay <abelay@mit.edu>, Li Shaohua <shaohua.li@intel.com>,
       Matthieu Castet <castet.matthieu@free.fr>,
       Thomas Renninger <trenn@suse.de>, Jaroslav Kysela <perex@perex.cz>,
       Andrew Morton <akpm@linux-foundation.org>, Takashi Iwai <tiwai@suse.de>
Subject: Re: [patch 18/18] PNP: convert resource options to single linked
 list
References: <20080604220933.168145536@ldl.fc.hp.com> <20080604221130.337606204@ldl.fc.hp.com>
In-Reply-To: <20080604221130.337606204@ldl.fc.hp.com>
Content-Type: text/plain; charset=ISO-8859-15; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1705
Lines: 63

On 05-06-08 00:09, Bjorn Helgaas wrote:

> ISAPNP, PNPBIOS, and ACPI describe the "possible resource settings" of

[ ... ]

Acked-by: Rene Herman <rene.herman@gmail.com>

Only minor comment:

> +static inline unsigned int pnp_independent_option(void)
> +{
> +	return 0;
> +}

I think this is a somewhat unintuitive name (the function doesn't return 
an option) and now that the pnp_dependent_option() one has been renamed 
to pnp_new_dependent_set() even the symmetry doesn't survive.

pnp_independent_option_flags? pnp_independent_flags? Or better yet, just 
literal 0? That last one unless you have some as of yet unpublished plan 
for the abstraction ofcourse but this function seems to obscure more 
than it helps any at the moment.

Even if you agree, obviously also fine as follow-up patch.

Only trivial:

> +static int pnp_assign_resources(struct pnp_dev *dev, int set)
> {

[ ... ]

> -fail:
> -	pnp_clean_resource_table(dev);
>  	mutex_unlock(&pnp_res_mutex);
> -	dbg_pnp_show_resources(dev, "after pnp_assign_resources (failed)");
> -	return 0;
> +	if (ret) {
> +		dev_dbg(&dev->dev, "pnp_assign_resources failed (%d)\n", ret);
> +		pnp_clean_resource_table(dev);
> +	} else
> +		dbg_pnp_show_resources(dev, "pnp_assign_resources succeeded");
> +	return ret;
>  }

if (ret < 0) would agree with the rest.

>  int pnp_auto_config_dev(struct pnp_dev *dev)
>  {
> -	struct pnp_option *dep;
> -	int i = 1;
> +	int i, ret = 0;

int ret; will do;

Rene.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/