[PATCH v5 1/3] mfd: syscon: Removed support for unloading

[PATCH v5 1/3] mfd: syscon: Removed support for unloading

[Alexander Shiyan]

The driver can be used in various subsystems and therefore should not
be unloaded when it is defined in the kernel configuration, so remove
support for unloading it.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/mfd/syscon.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 61aea63..55d7915 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -139,17 +139,6 @@ static int syscon_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int syscon_remove(struct platform_device *pdev)
-{
-	struct syscon *syscon;
-
-	syscon = platform_get_drvdata(pdev);
-	iounmap(syscon->base);
-	platform_set_drvdata(pdev, NULL);
-
-	return 0;
-}
-
 static struct platform_driver syscon_driver = {
 	.driver = {
 		.name = "syscon",
@@ -157,7 +146,6 @@ static struct platform_driver syscon_driver = {
 		.of_match_table = of_syscon_match,
 	},
 	.probe		= syscon_probe,
-	.remove		= syscon_remove,
 };
 
 static int __init syscon_init(void)
@@ -166,12 +154,6 @@ static int __init syscon_init(void)
 }
 postcore_initcall(syscon_init);
 
-static void __exit syscon_exit(void)
-{
-	platform_driver_unregister(&syscon_driver);
-}
-module_exit(syscon_exit);
-
 MODULE_AUTHOR("Dong Aisheng <dong.aisheng@linaro.org>");
 MODULE_DESCRIPTION("System Control driver");
 MODULE_LICENSE("GPL v2");
-- 
1.7.12.4

[PATCH v5 2/3] mfd: syscon: Removed unneeded field "dev" from private driver structure

[Alexander Shiyan]

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/mfd/syscon.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 55d7915..2c59ce6 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -25,17 +25,15 @@
 static struct platform_driver syscon_driver;
 
 struct syscon {
-	struct device *dev;
 	void __iomem *base;
 	struct regmap *regmap;
 };
 
 static int syscon_match(struct device *dev, void *data)
 {
-	struct syscon *syscon = dev_get_drvdata(dev);
 	struct device_node *dn = data;
 
-	return (syscon->dev->of_node == dn) ? 1 : 0;
+	return (dev->of_node == dn) ? 1 : 0;
 }
 
 struct regmap *syscon_node_to_regmap(struct device_node *np)
@@ -130,7 +128,6 @@ static int syscon_probe(struct platform_device *pdev)
 		return PTR_ERR(syscon->regmap);
 	}
 
-	syscon->dev = dev;
 	platform_set_drvdata(pdev, syscon);
 
 	dev_info(dev, "syscon regmap start 0x%x end 0x%x registered\n",
-- 
1.7.12.4

[PATCH v5 3/3] mfd: syscon: Add non-DT support

[Alexander Shiyan]

This patch allow using syscon driver from the platform data, i.e.
possibility using driver on systems without oftree support.
For search syscon device from the client drivers,
"syscon_regmap_lookup_by_pdevname" function was added.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/mfd/Kconfig        |  1 -
 drivers/mfd/syscon.c       | 67 ++++++++++++++++++++++++++++++++--------------
 include/linux/mfd/syscon.h |  1 +
 3 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 671f5b1..8fdd87e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1070,7 +1070,6 @@ config MFD_STA2X11
 
 config MFD_SYSCON
 	bool "System Controller Register R/W Based on Regmap"
-	depends on OF
 	select REGMAP_MMIO
 	help
 	  Select this option to enable accessing system control registers
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 2c59ce6..ead27d6 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -29,7 +29,7 @@ struct syscon {
 	struct regmap *regmap;
 };
 
-static int syscon_match(struct device *dev, void *data)
+static int syscon_match_node(struct device *dev, void *data)
 {
 	struct device_node *dn = data;
 
@@ -42,7 +42,7 @@ struct regmap *syscon_node_to_regmap(struct device_node *np)
 	struct device *dev;
 
 	dev = driver_find_device(&syscon_driver.driver, NULL, np,
-				 syscon_match);
+				 syscon_match_node);
 	if (!dev)
 		return ERR_PTR(-EPROBE_DEFER);
 
@@ -68,6 +68,34 @@ struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
 
+static int syscon_match_pdevname(struct device *dev, void *data)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	const struct platform_device_id *id = platform_get_device_id(pdev);
+
+	if (id)
+		if (!strcmp(id->name, (const char *)data))
+			return 1;
+
+	return !strcmp(dev_name(dev), (const char *)data);
+}
+
+struct regmap *syscon_regmap_lookup_by_pdevname(const char *s)
+{
+	struct device *dev;
+	struct syscon *syscon;
+
+	dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
+				 syscon_match_pdevname);
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	syscon = dev_get_drvdata(dev);
+
+	return syscon->regmap;
+}
+EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname);
+
 struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
 					const char *property)
 {
@@ -99,30 +127,24 @@ static struct regmap_config syscon_regmap_config = {
 static int syscon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
 	struct syscon *syscon;
-	struct resource res;
-	int ret;
-
-	if (!np)
-		return -ENOENT;
+	struct resource *res;
 
-	syscon = devm_kzalloc(dev, sizeof(struct syscon),
-			    GFP_KERNEL);
+	syscon = devm_kzalloc(dev, sizeof(struct syscon), GFP_KERNEL);
 	if (!syscon)
 		return -ENOMEM;
 
-	syscon->base = of_iomap(np, 0);
-	if (!syscon->base)
-		return -EADDRNOTAVAIL;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENOENT;
 
-	ret = of_address_to_resource(np, 0, &res);
-	if (ret)
-		return ret;
+	syscon->base = devm_ioremap(dev, res->start, resource_size(res));
+	if (!syscon->base)
+		return -ENOMEM;
 
-	syscon_regmap_config.max_register = res.end - res.start - 3;
+	syscon_regmap_config.max_register = res->end - res->start - 3;
 	syscon->regmap = devm_regmap_init_mmio(dev, syscon->base,
-					&syscon_regmap_config);
+					       &syscon_regmap_config);
 	if (IS_ERR(syscon->regmap)) {
 		dev_err(dev, "regmap init failed\n");
 		return PTR_ERR(syscon->regmap);
@@ -130,12 +152,16 @@ static int syscon_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, syscon);
 
-	dev_info(dev, "syscon regmap start 0x%x end 0x%x registered\n",
-		res.start, res.end);
+	dev_info(dev, "regmap 0x%x-0x%x registered\n", res->start, res->end);
 
 	return 0;
 }
 
+static const struct platform_device_id syscon_ids[] = {
+	{ "syscon", },
+	{ }
+};
+
 static struct platform_driver syscon_driver = {
 	.driver = {
 		.name = "syscon",
@@ -143,6 +169,7 @@ static struct platform_driver syscon_driver = {
 		.of_match_table = of_syscon_match,
 	},
 	.probe		= syscon_probe,
+	.id_table	= syscon_ids,
 };
 
 static int __init syscon_init(void)
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index 6aeb6b8..5c9ee6e 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -17,6 +17,7 @@
 
 extern struct regmap *syscon_node_to_regmap(struct device_node *np);
 extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s);
+extern struct regmap *syscon_regmap_lookup_by_pdevname(const char *s);
 extern struct regmap *syscon_regmap_lookup_by_phandle(
 					struct device_node *np,
 					const char *property);
-- 
1.7.12.4

Re: [PATCH v5 1/3] mfd: syscon: Removed support for unloading

[Stephen Warren]

On 02/22/2013 10:15 PM, Alexander Shiyan wrote:
> The driver can be used in various subsystems and therefore should not
> be unloaded when it is defined in the kernel configuration, so remove
> support for unloading it.

Why not fix the clients to module_get() at the appropriate times; then
you could still allow unloading, couldn't you?

Re[2]: [PATCH v5 1/3] mfd: syscon: Removed support for unloading

[Alexander Shiyan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 742 bytes --]

> On 02/22/2013 10:15 PM, Alexander Shiyan wrote:
> > The driver can be used in various subsystems and therefore should not
> > be unloaded when it is defined in the kernel configuration, so remove
> > support for unloading it.
> 
> Why not fix the clients to module_get() at the appropriate times; then
> you could still allow unloading, couldn't you?

I has explain this before. Driver defined as "bool" and loaded via postcore_initcall.
Once loaded it should not be unloaded, in other case it have not way to be loaded back.
I am correctly understand your question?
Thanks.

---
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v5 1/3] mfd: syscon: Removed support for unloading

[Arnd Bergmann]

On Saturday 23 February 2013, Alexander Shiyan wrote:
> The driver can be used in various subsystems and therefore should not
> be unloaded when it is defined in the kernel configuration, so remove
> support for unloading it.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

I think the description is still wrong here, as mentioned before.

	Arnd

Re: [PATCH v5 1/3] mfd: syscon: Removed support for unloading

[Stephen Warren]

On 02/22/2013 10:28 PM, Alexander Shiyan wrote:
>> On 02/22/2013 10:15 PM, Alexander Shiyan wrote:
>>> The driver can be used in various subsystems and therefore should not
>>> be unloaded when it is defined in the kernel configuration, so remove
>>> support for unloading it.
>>
>> Why not fix the clients to module_get() at the appropriate times; then
>> you could still allow unloading, couldn't you?
> 
> I has explain this before.

If multiple people have asked this, perhaps it'd be a good idea to
include the answer in the commit description.

> Driver defined as "bool" and loaded via postcore_initcall.

Being defined as a "bool" sounds like a reasonable reason that no
remove() is required.

Being loaded via postcore_initcall() (a) should have absolutely no
influence over whether a remove() is required (an actual module could
just as well include a postcore_initcall function, which would get
executed at module load time), and (b) is wrong; initcall ordering
shouldn't be used to influence driver probe ordering.

Re: [PATCH v5 1/3] mfd: syscon: Removed support for unloading

[Dmitry Torokhov]

On Sat, Feb 23, 2013 at 04:51:19PM -0700, Stephen Warren wrote:
> On 02/22/2013 10:28 PM, Alexander Shiyan wrote:
> >> On 02/22/2013 10:15 PM, Alexander Shiyan wrote:
> >>> The driver can be used in various subsystems and therefore should not
> >>> be unloaded when it is defined in the kernel configuration, so remove
> >>> support for unloading it.
> >>
> >> Why not fix the clients to module_get() at the appropriate times; then
> >> you could still allow unloading, couldn't you?
> > 
> > I has explain this before.
> 
> If multiple people have asked this, perhaps it'd be a good idea to
> include the answer in the commit description.
> 
> > Driver defined as "bool" and loaded via postcore_initcall.
> 
> Being defined as a "bool" sounds like a reasonable reason that no
> remove() is required.

No, it is not - I can still unbind the device from driver via sysfs. Now
what will happen is resources are leaked and won't be available when
trying to bin (via sysfs) again.

This patch is broken.

Thanks.

-- 
Dmitry

Re[2]: [PATCH v5 1/3] mfd: syscon: Removed support for unloading

[Alexander Shiyan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 2025 bytes --]

> On Sat, Feb 23, 2013 at 04:51:19PM -0700, Stephen Warren wrote:
> > On 02/22/2013 10:28 PM, Alexander Shiyan wrote:
> > >> On 02/22/2013 10:15 PM, Alexander Shiyan wrote:
> > >>> The driver can be used in various subsystems and therefore should not
> > >>> be unloaded when it is defined in the kernel configuration, so remove
> > >>> support for unloading it.
> > >>
> > >> Why not fix the clients to module_get() at the appropriate times; then
> > >> you could still allow unloading, couldn't you?
> > > 
> > > I has explain this before.
> > 
> > If multiple people have asked this, perhaps it'd be a good idea to
> > include the answer in the commit description.
> > 
> > > Driver defined as "bool" and loaded via postcore_initcall.
> > 
> > Being defined as a "bool" sounds like a reasonable reason that no
> > remove() is required.
> 
> No, it is not - I can still unbind the device from driver via sysfs. Now
> what will happen is resources are leaked and won't be available when
> trying to bin (via sysfs) again.
> 
> This patch is broken.

I will try to resolve the dispute.
Initially, the first part of the patch (this) was an attempt to prevent the unloading
(unregister) of the driver (not the device), ie remove module_exit call.
The patch also removes the code to remove the device. Inclusion in this patch
of the code was a mistake. Because of this, people are confused about the terms,
including me.
Code, relating to the removal of the device should be moved to the third patch.
Since all moved to using the managed resources, it should not have any questions.

About uloading (unregister) driver: As far i understand "__exit" section is
completely discarded when kernel compiled without module support.
But where is this call (module_exit) is placing in case "bool" driver and
kernel is compiled with module support? Fixme please.

Thanks.

---
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v5 1/3] mfd: syscon: Removed support for unloading

[Dmitry Torokhov]

On Mon, Feb 25, 2013 at 10:53:42AM +0400, Alexander Shiyan wrote:
> > On Sat, Feb 23, 2013 at 04:51:19PM -0700, Stephen Warren wrote:
> > > On 02/22/2013 10:28 PM, Alexander Shiyan wrote:
> > > >> On 02/22/2013 10:15 PM, Alexander Shiyan wrote:
> > > >>> The driver can be used in various subsystems and therefore should not
> > > >>> be unloaded when it is defined in the kernel configuration, so remove
> > > >>> support for unloading it.
> > > >>
> > > >> Why not fix the clients to module_get() at the appropriate times; then
> > > >> you could still allow unloading, couldn't you?
> > > > 
> > > > I has explain this before.
> > > 
> > > If multiple people have asked this, perhaps it'd be a good idea to
> > > include the answer in the commit description.
> > > 
> > > > Driver defined as "bool" and loaded via postcore_initcall.
> > > 
> > > Being defined as a "bool" sounds like a reasonable reason that no
> > > remove() is required.
> > 
> > No, it is not - I can still unbind the device from driver via sysfs. Now
> > what will happen is resources are leaked and won't be available when
> > trying to bin (via sysfs) again.
> > 
> > This patch is broken.
> 
> I will try to resolve the dispute.
> Initially, the first part of the patch (this) was an attempt to prevent the unloading
> (unregister) of the driver (not the device), ie remove module_exit call.
> The patch also removes the code to remove the device. Inclusion in this patch
> of the code was a mistake. Because of this, people are confused about the terms,
> including me.
> Code, relating to the removal of the device should be moved to the third patch.
> Since all moved to using the managed resources, it should not have any questions.
> 
> About uloading (unregister) driver: As far i understand "__exit" section is
> completely discarded when kernel compiled without module support.
> But where is this call (module_exit) is placing in case "bool" driver and
> kernel is compiled with module support? Fixme please.

In both cases __exit code goes to .exit.text section which is either
discarded (code is compiled into the kernel) or kept (code is built as a
module).

Still, because the device can forcibly be unbound from the driver, all
these changes are pretty useless for the stated purpose. All in all, it
seems to me that this code should not be a driver at all, but rather
rolled into regmap infrastructure and be part of it's initialization
process.

Thanks.

-- 
Dmitry