Date: Mon, 8 Jun 2015 14:03:51 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>, Olof Johansson <olof@lixom.net>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will.deacon@arm.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] genalloc: add support of multiple gen_pools per device
Message-Id: <20150608140351.86918d9178f243d60cdd10c0@linux-foundation.org>
In-Reply-To: <5570E0C2.5030105@mentor.com>
References: <1433418952-31749-1-git-send-email-vladimir_zapolskiy@mentor.com>
	<20150604153548.81ffa1de23a7a1954bcd4aad@linux-foundation.org>
	<5570E0C2.5030105@mentor.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3167
Lines: 86

On Fri, 5 Jun 2015 02:35:30 +0300 Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote:

> 
> > If there's a pattern, it is "subsytem-identifier_operation-on-it", so
> > 
> > 	devm_gen_pool_named_create
> > 	dev_gen_pool_named_get
> > 	of_named_gen_pool_get
> > 
> > ie: it's big-endian.  The name starts out with the most significant
> > thing (subsystem identification) and fields in order of decreasing
> > significance.
> > 
> > Anyway, please have a think about it ;)
> 
> What would be your opinion on the following naming proposal:
> 
>   devm_gen_pool_create()  --> devm_gen_pool_create(),
>                               devm_gen_pool_create_named()
> 
> No changes, and "named" flavour of a new gen_pool create interface goes
> to the suffix position.
> 
>   dev_get_gen_pool()      --> dev_gen_pool_get(),
>                               dev_gen_pool_get_named()
> 
> And the last one
> 
>   of_get_named_gen_pool() --> of_gen_pool_get()
> 
> Here "named" reminds of need to provide a phandle name, not gen_pool
> name as above, to avoid confusion I would propose to drop it.
> 
> If it is okay from your point of view, I'll send another non-functional
> patch against linux-next to rename existing interfaces.

Sounds good.

> >> +/**
> >> + * dev_get_gen_pool_named - Obtain the gen_pool (if any) for a device
> >> + * @dev: device to retrieve the gen_pool from
> >> + * @name: name of a gen_pool, addresses a particular gen_pool from device
> >> + *
> >> + * Returns the gen_pool for the device if one is present, or NULL.
> >> + */
> >> +struct gen_pool *dev_get_gen_pool_named(struct device *dev, const char *name)
> >> +{
> >> +	struct gen_pool **p = devres_find(dev, devm_gen_pool_release,
> >> +					  dev_gen_pool_match, (void *)name);
> >> +
> >> +	if (!p)
> >> +		return NULL;
> >> +	return *p;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dev_get_gen_pool_named);
> > 
> > But we didn't do anything to prevent duplicated names.
> 
> Like with MTD OF partitions used as a template technically it is
> possible to register several gen_pools under the same name, when
> requested the first found one is returned to a client, correctness of
> one to one mapping is offloaded to the register party, for instance if
> of_get_named_gen_pool() is supposed to be used, then DTS description is
> expected to be consistent.

Well, if we go this way then your "first found one" becomes part of
the dev_get_gen_pool_named() interface definition and should be
documented and maintained.  Most-recently-registered or
least-recently-registered?

But either way, does the interface make sense?  Or it is an error
condition?  If it's an error condition then we should check for it
rather than silently ignoring it.

> Is it good enough or better to add a name uniqueness check? In my
> opinion the latter case may require to change error return value of
> devm_gen_pool_create() from NULL to ERR_PTR().

It sounds that way.


--
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/