linux-kernel - Re: [PATCH] drivers: phy: add generic PHY framework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1348678677.15175.18.camel@joe-AO722>
Date:	Wed, 26 Sep 2012 09:57:57 -0700
From:	Joe Perches <joe@...ches.com>
To:	Kishon Vijay Abraham I <kishon@...com>
Cc:	akpm@...ux-foundation.org, davem@...emloft.net,
	santosh.shilimkar@...com, grant.likely@...retlab.ca, balbi@...com,
	arnd@...db.de, gregkh@...uxfoundation.org,
	linux-omap@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org, robherring2@...il.com,
	richard.zhao@...escale.com, B29397@...escale.com,
	alexander.shishkin@...ux.intel.com, mkl@...gutronix.de,
	marex@...x.de, p.paneri@...sung.com,
	devicetree-discuss@...ts.ozlabs.org,
	broonie@...nsource.wolfsonmicro.com
Subject: Re: [PATCH] drivers: phy: add generic PHY framework

On Wed, 2012-09-26 at 20:31 +0530, Kishon Vijay Abraham I wrote:
> The PHY framework provides a set of API's for the PHY drivers to
> create/destroy a PHY and API's 

Just some trivial notes.

> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
[]
> @@ -0,0 +1,445 @@
[]
> +static void devm_phy_release(struct device *dev, void *res)
> +{
> +	struct phy *phy = *(struct phy **)res;

That's a bit twisted isn't it?
Perhaps
	struct phy *phy = res;

[]

> +static int devm_phy_match(struct device *dev, void *res, void *match_data)
> +{
> +	return res == match_data;

static bool devm_phy_match(etc...)

[]

> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle, u8 index)
> +{
> +	struct phy *phy = NULL, **ptr;
> +	struct device_node *node;
> +
> +	if (!dev->of_node) {
> +		dev_dbg(dev, "device does not have a device node entry\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	node = of_parse_phandle(dev->of_node, phandle, index);
> +	if (!node) {
> +		dev_dbg(dev, "failed to get %s phandle in %s node\n", phandle,
> +			dev->of_node->full_name);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);

Is this the right size?

Because ptr is **, perhaps sizeof(struct phy) is clearer.

> +	if (!ptr) {
> +		dev_dbg(dev, "failed to allocate memory for devres\n");

alloc failures generally don't need specific OOM messages
as the general alloc OOM dumps stack.

> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	mutex_lock(&phy_list_mutex);
> +
> +	phy = of_phy_lookup(dev, node);
> +	if (IS_ERR(phy) || !phy->dev.class ||
> +			!try_module_get(phy->dev.class->owner)) {

I think it's tasteful coding style to align like:

	if (IS_ERR(phy) || !phy->dev.class ||
	    !try_module_get(phy->dev.class->owner)) {
[]

> +struct phy *phy_create(struct device *dev, struct phy_descriptor *desc)
> +{
> +	int ret;
> +	struct phy *phy;
> +	struct phy_bind *phy_bind;
> +	const char *devname = NULL;
> +
> +	if (!dev || !desc) {
> +		dev_err(dev, "no descriptor/device provided for PHY\n");
> +		ret = -EINVAL;
> +		goto err0;
> +	}
> +
> +	if (!desc->ops) {
> +		dev_err(dev, "no PHY ops provided\n");
> +		ret = -EINVAL;
> +		goto err0;
> +	}
> +
> +	phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> +	if (!phy) {
> +		dev_err(dev, "no memory for PHY\n");

No OOM message required...

[]

> +static int __init phy_core_init(void)
> +{
> +	phy_class = class_create(THIS_MODULE, "phy");
> +	if (IS_ERR(phy_class)) {
> +		pr_err("failed to create phy class --> %ld\n",
> +			PTR_ERR(phy_class));

You might add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
before any include so that pr_<levels> are prefixed.

cheers, Joe

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