netdev - Re: [PATCH 2/2 v3] net/smsc911x: Add regulator support

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: <CAJaTeTrzFwxsn22U-NHQ8iCiaUM+mj8PyicavR-DN0HgE08ZEQ@mail.gmail.com>
Date:	Thu, 27 Oct 2011 15:21:47 +0200
From:	Mike Frysinger <vapier@...too.org>
To:	Linus Walleij <linus.walleij@...ricsson.com>
Cc:	netdev@...r.kernel.org,
	Steve Glendinning <steve.glendinning@...c.com>,
	Mathieu Poirer <mathieu.poirier@...aro.org>,
	Robert Marklund <robert.marklund@...ricsson.com>,
	Paul Mundt <lethal@...ux-sh.org>, linux-sh@...r.kernel.org,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Tony Lindgren <tony@...mide.com>, linux-omap@...r.kernel.org,
	uclinux-dist-devel@...ckfin.uclinux.org,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH 2/2 v3] net/smsc911x: Add regulator support

On Thu, Oct 27, 2011 at 14:48, Linus Walleij wrote:
> Platforms that use regulators and the smsc911x and have no defined
> regulator for the smsc911x and claim complete regulator
> constraints with no dummy regulators will need to provide it, for
> example using a fixed voltage regulator. It appears that this may
> affect (apart from Ux500 Snowball) possibly these archs/machines
> that from some grep:s appear to define both CONFIG_SMSC911X and
> CONFIG_REGULATOR:
>
> - ARM Freescale mx3 and OMAP 2 plus, Raumfeld machines
> - Blackfin
> - Super-H

no Blackfin board in the tree uses regulators by default.  we do list
regulator resources with some boards so people can rebuild the
development system to support addon daughter cards.

my gut reaction: smsc911x is working just fine without regulator
support for many people, so why do we suddenly need to make it a
requirement ?  this is a fairly small amount of code, so adding a
smsc911x Kconfig symbol to control the regulator support seems like
overkill.  only other option would be to change the patch to not make
missing regulators non-fatal.  so i'd probably lean towards the latter
(and it sounds like you changed this with earlier versions).

> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c
>
> +#define SMSC911X_NUM_SUPPLIES 2

this gets used once (to define the array), so i wonder if it shouldn't
just be inlined

> +static int smsc911x_enable_disable_resources(struct platform_device *pdev,
> +                                            bool enable)
> +{
> ...
> +       if (enable) {
> +               ret = regulator_bulk_enable(ARRAY_SIZE(pdata->supplies),
> +                               pdata->supplies);
> +               if (ret)
> +                       netdev_err(ndev, "failed to enable regulators %d\n",
> +                                       ret);
> ...
> static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
> ...
> +       retval = smsc911x_request_free_resources(pdev, true);
> +       if (retval) {
> +               pr_err("Could request regulators needed aborting\n");

you warn twice here, and the grammar in the later error is broken, and
uses pr_err() instead of netdev_err().  i would simply drop the latter
pr_err().

> +static int smsc911x_request_free_resources(struct platform_device *pdev,
> +               bool request)
> +{
> ...
> +       if (request) {
> +               ret = regulator_bulk_get(&pdev->dev,
> +                               ARRAY_SIZE(pdata->supplies),
> +                               pdata->supplies);
> +               if (ret)
> +                       netdev_err(ndev, "couldn't get regulators %d\n",
> +                                       ret);
> static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
> ...
> +       retval = smsc911x_enable_disable_resources(pdev, true);
> +       if (retval) {
> +               pr_err("Could enable regulators needed aborting\n");

same exact issues with the request/free helper

> +       retval = smsc911x_request_free_resources(pdev, false);
> +       /* ignore not all have regulators */

old comment ?
-mike
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html