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

[PATCH v6 1/2] 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 61aea63..674af14 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 v6 2/2] 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       | 77 +++++++++++++++++++++++++++-------------------
 include/linux/mfd/syscon.h |  1 +
 3 files changed, 47 insertions(+), 32 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 674af14..e5b6be1 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,22 +152,15 @@ 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 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 const struct platform_device_id syscon_ids[] = {
+	{ "syscon", },
+	{ }
+};
 
 static struct platform_driver syscon_driver = {
 	.driver = {
@@ -154,7 +169,7 @@ static struct platform_driver syscon_driver = {
 		.of_match_table = of_syscon_match,
 	},
 	.probe		= syscon_probe,
-	.remove		= syscon_remove,
+	.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 v6 1/2] mfd: syscon: Removed unneeded field "dev" from private driver structure

[Alexander Shiyan]

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

> 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 61aea63..674af14 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

ping

---
ÿôèº{.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 v6 2/2] mfd: syscon: Add non-DT support

[Dong Aisheng]

On Thu, Feb 28, 2013 at 06:57:14PM +0400, Alexander Shiyan wrote:
> 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       | 77 +++++++++++++++++++++++++++-------------------
>  include/linux/mfd/syscon.h |  1 +
>  3 files changed, 47 insertions(+), 32 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 674af14..e5b6be1 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);

Should it be ERR_PTR(-EPROBE_DEFER)?

> +
> +	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);

Unneeded change to me

>  	if (IS_ERR(syscon->regmap)) {
>  		dev_err(dev, "regmap init failed\n");
>  		return PTR_ERR(syscon->regmap);
> @@ -130,22 +152,15 @@ 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);

Doesn't with a 'syscon' prefix look better?

>  
>  	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 const struct platform_device_id syscon_ids[] = {
> +	{ "syscon", },
> +	{ }
> +};
>  
>  static struct platform_driver syscon_driver = {
>  	.driver = {
> @@ -154,7 +169,7 @@ static struct platform_driver syscon_driver = {
>  		.of_match_table = of_syscon_match,
>  	},
>  	.probe		= syscon_probe,
> -	.remove		= syscon_remove,

So you reply on the managed resource to address the resource leak issue
raised by Dmitry on doing manually unbind here, right?
this looks ok to me.

So, besides above minor comments,
Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

Regards
Dong Aisheng

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

Re: [PATCH v6 1/2] mfd: syscon: Removed unneeded field "dev" from private driver structure

[Dong Aisheng]

On Thu, Feb 28, 2013 at 06:57:13PM +0400, Alexander Shiyan wrote:
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

Regards
Dong Aisheng

Re: [PATCH v6 2/2] mfd: syscon: Add non-DT support

[Alexander Shiyan]

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

> On Thu, Feb 28, 2013 at 06:57:14PM +0400, Alexander Shiyan wrote:
> > 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       | 77 +++++++++++++++++++++++++++-------------------
> >  include/linux/mfd/syscon.h |  1 +
> >  3 files changed, 47 insertions(+), 32 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 674af14..e5b6be1 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);
> 
> Should it be ERR_PTR(-EPROBE_DEFER)?

I have no idea what better using here. Think that is not so important
since we may have only one possible error code here.

> > +
> > +	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);
> 
> Unneeded change to me
> 
> >  	if (IS_ERR(syscon->regmap)) {
> >  		dev_err(dev, "regmap init failed\n");
> >  		return PTR_ERR(syscon->regmap);
> > @@ -130,22 +152,15 @@ 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);
> 
> Doesn't with a 'syscon' prefix look better?

Prefix is added by dev_info, so no need to print it twice.

> >  
> >  	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 const struct platform_device_id syscon_ids[] = {
> > +	{ "syscon", },
> > +	{ }
> > +};
> >  
> >  static struct platform_driver syscon_driver = {
> >  	.driver = {
> > @@ -154,7 +169,7 @@ static struct platform_driver syscon_driver = {
> >  		.of_match_table = of_syscon_match,
> >  	},
> >  	.probe		= syscon_probe,
> > -	.remove		= syscon_remove,
> 
> So you reply on the managed resource to address the resource leak issue
> raised by Dmitry on doing manually unbind here, right?

Yes. We are killing "remove"-entry since all our resources are "managed" now.

> this looks ok to me.
> 
> So, besides above minor comments,
> Acked-by: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Regards
> Dong Aisheng
> 
> > +	.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

---
ÿôèº{.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 v6 2/2] mfd: syscon: Add non-DT support

[Dong Aisheng]

On Mon, Mar 11, 2013 at 02:16:44PM +0400, Alexander Shiyan wrote:
> > On Thu, Feb 28, 2013 at 06:57:14PM +0400, Alexander Shiyan wrote:
> > > 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       | 77 +++++++++++++++++++++++++++-------------------
> > >  include/linux/mfd/syscon.h |  1 +
> > >  3 files changed, 47 insertions(+), 32 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 674af14..e5b6be1 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);
> > 
> > Should it be ERR_PTR(-EPROBE_DEFER)?
> 
> I have no idea what better using here. Think that is not so important
> since we may have only one possible error code here.

I'm not quite understand your meaning.
Since the syscon device may be still not registered,
so it may be better to return a EPROBE_DERFER which is the same
as dt version.

> 
> > > +
> > > +	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);
> > 
> > Unneeded change to me
> > 
> > >  	if (IS_ERR(syscon->regmap)) {
> > >  		dev_err(dev, "regmap init failed\n");
> > >  		return PTR_ERR(syscon->regmap);
> > > @@ -130,22 +152,15 @@ 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);
> > 
> > Doesn't with a 'syscon' prefix look better?
> 
> Prefix is added by dev_info, so no need to print it twice.

Reasonable to me.

Regards
Dong Aisheng

> > >  
> > >  	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 const struct platform_device_id syscon_ids[] = {
> > > +	{ "syscon", },
> > > +	{ }
> > > +};
> > >  
> > >  static struct platform_driver syscon_driver = {
> > >  	.driver = {
> > > @@ -154,7 +169,7 @@ static struct platform_driver syscon_driver = {
> > >  		.of_match_table = of_syscon_match,
> > >  	},
> > >  	.probe		= syscon_probe,
> > > -	.remove		= syscon_remove,
> > 
> > So you reply on the managed resource to address the resource leak issue
> > raised by Dmitry on doing manually unbind here, right?
> 
> Yes. We are killing "remove"-entry since all our resources are "managed" now.
> 
> > this looks ok to me.
> > 
> > So, besides above minor comments,
> > Acked-by: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > Regards
> > Dong Aisheng
> > 
> > > +	.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 v6 2/2] mfd: syscon: Add non-DT support

[Arnd Bergmann]

On Monday 11 March 2013, Dong Aisheng wrote:
> > > > + dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> > > > +                          syscon_match_pdevname);
> > > > + if (!dev)
> > > > +         return ERR_PTR(-ENODEV);
> > > 
> > > Should it be ERR_PTR(-EPROBE_DEFER)?
> > 
> > I have no idea what better using here. Think that is not so important
> > since we may have only one possible error code here.
> 
> I'm not quite understand your meaning.
> Since the syscon device may be still not registered,
> so it may be better to return a EPROBE_DERFER which is the same
> as dt version.

I'm guessing that Alexander has not encountered deferred probing yet.
Alexander, the point here is that returning -EPROBE_DEFER has the
advantage that a probe() callback calling this function can
simply return that error code to the driver core. If the driver
core sees -EPROBE_DEFER, it will retry the same probe function
later, after all other device probe functions have been called
and at least one of them was successful. This way you can load the
syscon driver after loading a driver depending on it and everything
will still work.

	Arnd

Re[2]: [PATCH v6 2/2] mfd: syscon: Add non-DT support

[Alexander Shiyan]

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

> On Monday 11 March 2013, Dong Aisheng wrote:
> > > > > + dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> > > > > +                          syscon_match_pdevname);
> > > > > + if (!dev)
> > > > > +         return ERR_PTR(-ENODEV);
> > > > 
> > > > Should it be ERR_PTR(-EPROBE_DEFER)?
> > > 
> > > I have no idea what better using here. Think that is not so important
> > > since we may have only one possible error code here.
> > 
> > I'm not quite understand your meaning.
> > Since the syscon device may be still not registered,
> > so it may be better to return a EPROBE_DERFER which is the same
> > as dt version.
> 
> I'm guessing that Alexander has not encountered deferred probing yet.
> Alexander, the point here is that returning -EPROBE_DEFER has the
> advantage that a probe() callback calling this function can
> simply return that error code to the driver core. If the driver
> core sees -EPROBE_DEFER, it will retry the same probe function
> later, after all other device probe functions have been called
> and at least one of them was successful. This way you can load the
> syscon driver after loading a driver depending on it and everything
> will still work.

You are right Arnd. I usually try to control the correct order of loading
drivers. Using EPROBE_DEFER in this procedure is more correct.
Probably time for v7... I hope it will finally be the last.

---
ÿôèº{.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¥