Patchwork [1/4] Power: Reset: Driver to turn QNAP board power off.

login
register
mail settings
Submitter Anton Vorontsov
Date Jan. 6, 2013, 9:55 p.m.
Message ID <20130106215509.GD26928@lizard.sbx05280.losalca.wayport.net>
Download mbox | patch
Permalink /patch/209835/
State New
Headers show

Comments

Anton Vorontsov - Jan. 6, 2013, 9:55 p.m.
On Fri, Dec 28, 2012 at 01:25:09PM +0100, Andrew Lunn wrote:
> From: Andrew Lunn <andrew.lunn@ruag.com>
> 
> The QNAP NAS boxes have a microcontroller attached to the SoCs second
> serial port. By sending it a simple command, it will turn the power
> for the board off. This driver registers a function for pm_power_off
> to send such a command.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
[...]
> +MODULE_LICENSE("GPLv2+");

That doesn't make sense, and probably will result into a T (tainted) flag.
The kernel is under GPLv2 license, no '+'. Anyone who touches the code
assumes that it is "GPLv2 only". So, any changes on top of your code
automatically turns it into "GPLv2 only". IANAL, but that's how I believe
things work.

Anyway, fixed this warning:

  CC      drivers/power/reset/qnap-poweroff.o
drivers/power/reset/qnap-poweroff.c: In function ‘qnap_power_off_probe’:
drivers/power/reset/qnap-poweroff.c:80:3: warning: passing argument 1 of ‘lookup_symbol_name’ makes integer from pointer without a cast [enabled by default]
In file included from drivers/power/reset/qnap-poweroff.c:21:0:
include/linux/kallsyms.h:45:5: note: expected ‘long unsigned int’ but argument is of type ‘void (*)(void)’

... and have made a few cosmetic changes (see below). But I didn't change
the license string, you probably should send a patch for it.
Anton Vorontsov - Jan. 20, 2013, 2:05 a.m.
On Tue, Jan 08, 2013 at 07:15:26PM +0100, Andrew Lunn wrote:
> GPLv2+ is not a valid license string. Replace it with one that is.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---

Applied, thanks a lot!

>  drivers/power/reset/qnap-poweroff.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/reset/qnap-poweroff.c b/drivers/power/reset/qnap-poweroff.c
> index ca0b476..8af772b 100644
> --- a/drivers/power/reset/qnap-poweroff.c
> +++ b/drivers/power/reset/qnap-poweroff.c
> @@ -121,4 +121,4 @@ module_platform_driver(qnap_power_off_driver);
>  
>  MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch>");
>  MODULE_DESCRIPTION("QNAP Power off driver");
> -MODULE_LICENSE("GPLv2+");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.7.10.4
Uwe Kleine-König - Jan. 20, 2013, 8:13 p.m.
On Tue, Jan 08, 2013 at 07:15:26PM +0100, Andrew Lunn wrote:
> GPLv2+ is not a valid license string. Replace it with one that is.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/power/reset/qnap-poweroff.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/reset/qnap-poweroff.c b/drivers/power/reset/qnap-poweroff.c
> index ca0b476..8af772b 100644
> --- a/drivers/power/reset/qnap-poweroff.c
> +++ b/drivers/power/reset/qnap-poweroff.c
> @@ -121,4 +121,4 @@ module_platform_driver(qnap_power_off_driver);
>  
>  MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch>");
>  MODULE_DESCRIPTION("QNAP Power off driver");
> -MODULE_LICENSE("GPLv2+");
> +MODULE_LICENSE("GPL v2");
This change is wrong.

According to include/linux/module.h "GPL v2" means exactly that: version
2. As the file specifies v2 or later in the header you have to use "GPL"
which means v2 or later.

Best regards
Uwe
Anton Vorontsov - Jan. 20, 2013, 8:47 p.m.
On Sun, Jan 20, 2013 at 09:13:36PM +0100, Uwe Kleine-König wrote:
> On Tue, Jan 08, 2013 at 07:15:26PM +0100, Andrew Lunn wrote:
> > GPLv2+ is not a valid license string. Replace it with one that is.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  drivers/power/reset/qnap-poweroff.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/power/reset/qnap-poweroff.c b/drivers/power/reset/qnap-poweroff.c
> > index ca0b476..8af772b 100644
> > --- a/drivers/power/reset/qnap-poweroff.c
> > +++ b/drivers/power/reset/qnap-poweroff.c
> > @@ -121,4 +121,4 @@ module_platform_driver(qnap_power_off_driver);
> >  
> >  MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch>");
> >  MODULE_DESCRIPTION("QNAP Power off driver");
> > -MODULE_LICENSE("GPLv2+");
> > +MODULE_LICENSE("GPL v2");
> This change is wrong.
> 
> According to include/linux/module.h "GPL v2" means exactly that: version
> 2. As the file specifies v2 or later in the header you have to use "GPL"
> which means v2 or later.

Does it even make sense to have the two separate things ("GPL v2" and
"GPL")?

Suppose there is a global change that modifies a bunch of drivers, some of
them are GPLv2+. Now, the author of the global change is submitting it
under "GPL v2 only" license, which, by definition, turns any GPLv2+ code
into "GPL v2 only", right?

So, changing from GPLv2+ to "GPL v2 only" is OK, but not the other way
around.

IANAL, tho.

Anton

p.s. Yes, in this particular driver it also makes sense to remove "or
later" words from the header, just to be consistent.
Uwe Kleine-König - Jan. 21, 2013, 8:11 a.m.
Hello Anton,

On Sun, Jan 20, 2013 at 12:47:29PM -0800, Anton Vorontsov wrote:
> On Sun, Jan 20, 2013 at 09:13:36PM +0100, Uwe Kleine-König wrote:
> > On Tue, Jan 08, 2013 at 07:15:26PM +0100, Andrew Lunn wrote:
> > > GPLv2+ is not a valid license string. Replace it with one that is.
> > > 
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > > ---
> > >  drivers/power/reset/qnap-poweroff.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/power/reset/qnap-poweroff.c b/drivers/power/reset/qnap-poweroff.c
> > > index ca0b476..8af772b 100644
> > > --- a/drivers/power/reset/qnap-poweroff.c
> > > +++ b/drivers/power/reset/qnap-poweroff.c
> > > @@ -121,4 +121,4 @@ module_platform_driver(qnap_power_off_driver);
> > >  
> > >  MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch>");
> > >  MODULE_DESCRIPTION("QNAP Power off driver");
> > > -MODULE_LICENSE("GPLv2+");
> > > +MODULE_LICENSE("GPL v2");
> > This change is wrong.
> > 
> > According to include/linux/module.h "GPL v2" means exactly that: version
> > 2. As the file specifies v2 or later in the header you have to use "GPL"
> > which means v2 or later.
> 
> Does it even make sense to have the two separate things ("GPL v2" and
> "GPL")?
Yeah. If you had another OS project that uses GPL-4 you can just copy
over a GPL-2+ driver to it, not an GPL-2 driver. So assuming the kernel
will stay at GPL-2 forever it doesn't make any difference for the
kernel. But other projects might benefit. (And if in the future someone
might want to change the kernel to GPL-4, she only needs to contact the
GPL-2 authors and can legally change GPL-2+ to GPL-4+.)

> Suppose there is a global change that modifies a bunch of drivers, some of
> them are GPLv2+. Now, the author of the global change is submitting it
> under "GPL v2 only" license, which, by definition, turns any GPLv2+ code
> into "GPL v2 only", right?
See http://yarchive.net/comp/linux/dual_license_bsd_gpl.html for Linus'
POV. It's an old mail, but I think it still applies.

> So, changing from GPLv2+ to "GPL v2 only" is OK, but not the other way
> around.
OK as in (probably) legal. OK as in fair is questionable.

> IANAL, tho.
ditto.

Best regards
Uwe

Patch

diff --git a/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt b/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt
index 0951952..9a599d2 100644
--- a/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt
+++ b/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt
@@ -11,4 +11,3 @@  Required Properties:
 
 - reg: Address and length of the register set for UART1
 - clocks: tclk clock
-
diff --git a/drivers/power/reset/qnap-poweroff.c b/drivers/power/reset/qnap-poweroff.c
index ca0b476..cecb317 100644
--- a/drivers/power/reset/qnap-poweroff.c
+++ b/drivers/power/reset/qnap-poweroff.c
@@ -28,8 +28,7 @@ 
 static void __iomem *base;
 static unsigned long tclk;
 
-static void
-qnap_power_off(void)
+static void qnap_power_off(void)
 {
 	/* 19200 baud divisor */
 	const unsigned divisor = ((tclk + (8 * 19200)) / (16 * 19200));
@@ -49,28 +48,25 @@  qnap_power_off(void)
 	writel('A', UART1_REG(TX));
 }
 
-static int
-qnap_power_off_probe(struct platform_device *pdev)
+static int qnap_power_off_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	struct clk *clk;
 	char symname[KSYM_NAME_LEN];
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (res == NULL) {
+	if (!res) {
 		dev_err(&pdev->dev, "Missing resource");
 		return -EINVAL;
 	}
 
-	base = devm_ioremap(&pdev->dev, res->start,
-			    resource_size(res));
-	if (base == NULL) {
+	base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!base) {
 		dev_err(&pdev->dev, "Unable to map resource");
 		return -EINVAL;
 	}
 
-	/* We need to know tclk in order to calculate the UART
-	   divisor */
+	/* We need to know tclk in order to calculate the UART divisor */
 	clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {
 		dev_err(&pdev->dev, "Clk missing");
@@ -80,8 +76,8 @@  qnap_power_off_probe(struct platform_device *pdev)
 	tclk = clk_get_rate(clk);
 
 	/* Check that nothing else has already setup a handler */
-	if (pm_power_off != NULL) {
-		lookup_symbol_name(pm_power_off, symname);
+	if (pm_power_off) {
+		lookup_symbol_name((ulong)pm_power_off, symname);
 		dev_err(&pdev->dev,
 			"pm_power_off already claimed %p %s",
 			pm_power_off, symname);
@@ -92,11 +88,9 @@  qnap_power_off_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int
-qnap_power_off_remove(struct platform_device *pdev)
+static int qnap_power_off_remove(struct platform_device *pdev)
 {
 	pm_power_off = NULL;
-
 	return 0;
 }
 
@@ -104,7 +98,6 @@  static const struct of_device_id qnap_power_off_of_match_table[] = {
 	{ .compatible = "qnap,power-off", },
 	{}
 };
-
 MODULE_DEVICE_TABLE(of, qnap_power_off_of_match_table);
 
 static struct platform_driver qnap_power_off_driver = {
@@ -116,7 +109,6 @@  static struct platform_driver qnap_power_off_driver = {
 		.of_match_table = of_match_ptr(qnap_power_off_of_match_table),
 	},
 };
-
 module_platform_driver(qnap_power_off_driver);
 
 MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch>");