linux-kernel - Re: [PATCH 1/5] phy: add support for USB cluster on the Armada 375 SoC

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]
Date:	Fri, 23 May 2014 23:50:54 +0200
From:	Gregory CLEMENT <gregory.clement@...e-electrons.com>
To:	Kishon Vijay Abraham I <kishon@...com>,
	Mathias Nyman <mathias.nyman@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jason Cooper <jason@...edaemon.net>,
	Andrew Lunn <andrew@...n.ch>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
CC:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	linux-arm-kernel@...ts.infradead.org,
	Lior Amsalem <alior@...vell.com>,
	Tawfik Bayouk <tawfik@...vell.com>,
	Nadav Haklai <nadavh@...vell.com>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH 1/5] phy: add support for USB cluster on the Armada 375
 SoC

On 23/05/2014 11:20, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Friday 16 May 2014 09:52 PM, Gregory CLEMENT wrote:
>> The Armada 375 SoC comes with an USB2 host and device controller and
>> an USB3 controller. The USB cluster control register allows to manage
>> common features of both USB controllers.
>>
>> This commit adds a driver integrated in the generic PHY framework to
>> control this USB cluster feature.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@...e-electrons.com>
>> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
>> ---
>>  drivers/phy/Kconfig                              |   6 +
>>  drivers/phy/Makefile                             |   1 +
>>  drivers/phy/phy-armada375-usb2.c                 | 140 +++++++++++++++++++++++
>>  include/dt-bindings/phy/armada-375-usb-cluster.h |  18 +++
[...]

>> +static struct phy_ops armada375_usb_phy_ops = {
>> +	.init = armada375_usb_phy_init,
>> +	.owner		= THIS_MODULE,
> 
> nitpick: unnecessary tab.

OK

>> +};
>> +
>> +/*
>> + * Only one controller can use this PHY. We shouldn't have the case
>> + * when two controllers want to use this PHY. But if this case occurs
>> + * then we provide a phy to the first one and return an error for the
>> + * next one. This error has also to be an error returned by
>> + * devm_phy_optional_get() so different from ENODEV for USB2. In the
>> + * USB3 case it still optional and we use ENODEV.
>> + */
>> +static struct phy *armada375_usb_phy_xlate(struct device *dev,
>> +					struct of_phandle_args *args)
>> +{
>> +	if (WARN_ON(usb_cluster_phy.phy_provided)) {
>> +		dev_err(dev, "This PHY has already been provided!\n");
>> +		dev_err(dev, "Check your device tree, only one controller can use it\n.");
>> +		if (args->args[0] == PHY_USB2)
>> +			return ERR_PTR(-EBUSY);
>> +		else
>> +			return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	if (args->args[0] == PHY_USB2)
>> +		usb_cluster_phy.use_usb3 = false;
>> +	else if (args->args[0] == PHY_USB3)
>> +		usb_cluster_phy.use_usb3 = true;
>> +	else {
>> +		dev_err(dev, "Invalid PHY mode\n");
>> +		return ERR_PTR(-ENODEV);
>> +	}
> 
> how will this behave if there is a phy_put and then a phy_get?

Badly I think :(

I have to think about a better solution then.

>> +
>> +	usb_cluster_phy.phy_provided = true;
>> +
>> +	return usb_cluster_phy.phy;
>> +}
>> +
>> +static int armada375_usb_phy_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct phy *phy;
>> +	struct phy_provider *phy_provider;
>> +	void __iomem *usb_cluster_base;
>> +	struct resource *res;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	usb_cluster_base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (!usb_cluster_base)
>> +		return -ENOMEM;
>> +
>> +	phy = devm_phy_create(dev, &armada375_usb_phy_ops, NULL);
>> +	if (IS_ERR(phy)) {
>> +		dev_err(dev, "failed to create PHY \n");
>> +		return PTR_ERR(phy);
>> +	}
>> +
>> +	usb_cluster_phy.phy = phy;
>> +	usb_cluster_phy.reg = usb_cluster_base;
>> +	phy_set_drvdata(phy, &usb_cluster_phy);
>> +
>> +	phy_provider = devm_of_phy_provider_register(&pdev->dev,
>> +						     armada375_usb_phy_xlate);
>> +	if (IS_ERR(phy_provider))
>> +		return PTR_ERR(phy_provider);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id of_usb_cluster_table[] = {
>> +	{ .compatible = "marvell,armada-375-usb-cluster", },
>> +	{ /* end of list */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, of_usb_cluster_table);
>> +
>> +static struct platform_driver armada375_usb_phy_driver = {
>> +	.probe	= armada375_usb_phy_probe,
>> +	.driver = {
>> +		.of_match_table	= of_usb_cluster_table,
>> +		.name  = "armada-375-usb-cluster",
>> +		.owner = THIS_MODULE,
>> +	}
>> +};
>> +module_platform_driver(armada375_usb_phy_driver);
>> +
>> +MODULE_DESCRIPTION("Armada 375 USB cluster driver");
>> +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@...e-electrons.com>");
>> +MODULE_LICENSE("GPL");
> 
> GPL v2?

See the header, I chose "GNU General Public License version 2 or later."
so "GPL" match it.


Thanks for your review, once I will have found a proper solution for the
phy_put/phy_get scenario, I will sent a new version with all the other issues
fixed.


Gregory

> 
> Thanks
> Kishon
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
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/