[PATCH] mfd: syscon: Added support for using platform driver resources

[PATCH] mfd: syscon: Added support for using platform driver resources

[Alexander Shiyan]

This patch adds support usage platform driver resources, i.e.
possibility works without oftree support. Additionally patch
removes CONFIG_OF dependency and adds helper for accessing
regmap by searching device by its name.

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

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 47ad4e2..389f5e2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1065,7 +1065,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 3f10591..27d0a08 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -29,7 +29,27 @@ struct syscon {
 	struct regmap *regmap;
 };
 
-static int syscon_match(struct device *dev, void *data)
+static int syscon_match_name(struct device *dev, void *data)
+{
+	return !strcmp(dev_name(dev), (const char *)data);
+}
+
+struct regmap *syscon_regmap_lookup_by_name(const char *name)
+{
+	struct syscon *syscon;
+	struct device *dev;
+
+	dev = driver_find_device(&syscon_driver.driver, NULL, (void *)name,
+				 syscon_match_name);
+	if (!dev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	syscon = dev_get_drvdata(dev);
+
+	return syscon->regmap;
+}
+
+static int syscon_match_of(struct device *dev, void *data)
 {
 	struct syscon *syscon = dev_get_drvdata(dev);
 	struct device_node *dn = data;
@@ -43,7 +63,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_of);
 	if (!dev)
 		return ERR_PTR(-EPROBE_DEFER);
 
@@ -102,26 +122,38 @@ 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;
+	struct resource *res, res_of;
 	int ret;
 
-	if (!np)
-		return -ENOENT;
-
 	syscon = devm_kzalloc(dev, sizeof(struct syscon),
 			    GFP_KERNEL);
 	if (!syscon)
 		return -ENOMEM;
 
-	syscon->base = of_iomap(np, 0);
-	if (!syscon->base)
-		return -EADDRNOTAVAIL;
-
-	ret = of_address_to_resource(np, 0, &res);
-	if (ret)
-		return ret;
+	if (IS_ENABLED(CONFIG_OF) && np) {
+		syscon->base = of_iomap(np, 0);
+		if (!syscon->base)
+			return -EADDRNOTAVAIL;
+
+		res = &res_of;
+		ret = of_address_to_resource(np, 0, res);
+		if (ret)
+			return ret;
+	} else {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!res)
+			return -ENXIO;
+
+		if (!request_mem_region(res->start, resource_size(res),
+					dev_name(&pdev->dev)))
+			return -EBUSY;
+
+		syscon->base = ioremap(res->start, resource_size(res));
+		if (!syscon->base)
+			return -EADDRNOTAVAIL;
+	}
 
-	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);
 	if (IS_ERR(syscon->regmap)) {
@@ -133,7 +165,7 @@ 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);
+		res->start, res->end);
 
 	return 0;
 }
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index 6aeb6b8..e14f7fd 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -15,6 +15,7 @@
 #ifndef __LINUX_MFD_SYSCON_H__
 #define __LINUX_MFD_SYSCON_H__
 
+extern struct regmap *syscon_regmap_lookup_by_name(const char *name);
 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_phandle(
-- 
1.7.12.4

Re: [PATCH] mfd: syscon: Added support for using platform driver resources

[Dong Aisheng]

Hi Alexander,

Thanks for the patch adding non-dt support. :-)

On Mon, Feb 04, 2013 at 07:00:40PM +0400, Alexander Shiyan wrote:
> This patch adds support usage platform driver resources, i.e.
> possibility works without oftree support. Additionally patch
> removes CONFIG_OF dependency and adds helper for accessing
> regmap by searching device by its name.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  drivers/mfd/Kconfig        |  1 -
>  drivers/mfd/syscon.c       | 62 +++++++++++++++++++++++++++++++++++-----------
>  include/linux/mfd/syscon.h |  1 +
>  3 files changed, 48 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 47ad4e2..389f5e2 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1065,7 +1065,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 3f10591..27d0a08 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -29,7 +29,27 @@ struct syscon {
>  	struct regmap *regmap;
>  };
>  
> -static int syscon_match(struct device *dev, void *data)
> +static int syscon_match_name(struct device *dev, void *data)
> +{
> +	return !strcmp(dev_name(dev), (const char *)data);
> +}
> +
> +struct regmap *syscon_regmap_lookup_by_name(const char *name)
> +{
> +	struct syscon *syscon;
> +	struct device *dev;
> +
> +	dev = driver_find_device(&syscon_driver.driver, NULL, (void *)name,
> +				 syscon_match_name);
> +	if (!dev)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	syscon = dev_get_drvdata(dev);
> +
> +	return syscon->regmap;
> +}
> +

How about syscon_dev_to_regmap(struct device *dev) as the exist dt version
syscon_node_to_regmap since it's not affected by the name change of devices?

> +static int syscon_match_of(struct device *dev, void *data)
>  {
>  	struct syscon *syscon = dev_get_drvdata(dev);
>  	struct device_node *dn = data;
> @@ -43,7 +63,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_of);
>  	if (!dev)
>  		return ERR_PTR(-EPROBE_DEFER);
>  
> @@ -102,26 +122,38 @@ 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;
> +	struct resource *res, res_of;
>  	int ret;
>  
> -	if (!np)
> -		return -ENOENT;
> -
>  	syscon = devm_kzalloc(dev, sizeof(struct syscon),
>  			    GFP_KERNEL);
>  	if (!syscon)
>  		return -ENOMEM;
>  
> -	syscon->base = of_iomap(np, 0);
> -	if (!syscon->base)
> -		return -EADDRNOTAVAIL;
> -
> -	ret = of_address_to_resource(np, 0, &res);
> -	if (ret)
> -		return ret;
> +	if (IS_ENABLED(CONFIG_OF) && np) {
> +		syscon->base = of_iomap(np, 0);
> +		if (!syscon->base)
> +			return -EADDRNOTAVAIL;
> +
> +		res = &res_of;
> +		ret = of_address_to_resource(np, 0, res);
> +		if (ret)
> +			return ret;
> +	} else {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!res)
> +			return -ENXIO;
> +
> +		if (!request_mem_region(res->start, resource_size(res),
> +					dev_name(&pdev->dev)))
> +			return -EBUSY;
> +
> +		syscon->base = ioremap(res->start, resource_size(res));
> +		if (!syscon->base)
> +			return -EADDRNOTAVAIL;

devm_request_and_ioremap?

Regards
Dong Aisheng

> +	}
>  
> -	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);
>  	if (IS_ERR(syscon->regmap)) {
> @@ -133,7 +165,7 @@ 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);
> +		res->start, res->end);
>  
>  	return 0;
>  }
> diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
> index 6aeb6b8..e14f7fd 100644
> --- a/include/linux/mfd/syscon.h
> +++ b/include/linux/mfd/syscon.h
> @@ -15,6 +15,7 @@
>  #ifndef __LINUX_MFD_SYSCON_H__
>  #define __LINUX_MFD_SYSCON_H__
>  
> +extern struct regmap *syscon_regmap_lookup_by_name(const char *name);
>  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_phandle(
> -- 
> 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[2]: [PATCH] mfd: syscon: Added support for using platform driver resources

[Alexander Shiyan]

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

Hello.
...
> Thanks for the patch adding non-dt support. :-)
> 
> On Mon, Feb 04, 2013 at 07:00:40PM +0400, Alexander Shiyan wrote:
> > This patch adds support usage platform driver resources, i.e.
> > possibility works without oftree support. Additionally patch
> > removes CONFIG_OF dependency and adds helper for accessing
> > regmap by searching device by its name.
...
> > +static int syscon_match_name(struct device *dev, void *data)
> > +{
> > +	return !strcmp(dev_name(dev), (const char *)data);
> > +}
> > +
> > +struct regmap *syscon_regmap_lookup_by_name(const char *name)
> > +{
> > +	struct syscon *syscon;
> > +	struct device *dev;
> > +
> > +	dev = driver_find_device(&syscon_driver.driver, NULL, (void *)name,
> > +				 syscon_match_name);
> > +	if (!dev)
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +
> > +	syscon = dev_get_drvdata(dev);
> > +
> > +	return syscon->regmap;
> > +}
> > +
> 
> How about syscon_dev_to_regmap(struct device *dev) as the exist dt version
> syscon_node_to_regmap since it's not affected by the name change of devices?

I am not completely understand what you mean. In my version which doing
search regmap by name, we can call this function with desired device name,
then use regmap:
struct regmap *r = syscon_regmap_lookup_by_name("syscon.1");

You suggest use "struct device" as parameter? I do not know what we should
use as parameter to the function in this case, since we can get "struct device"
only when register this device, i.e. in board support code, not from anywhere,
for example from another driver.
Fixme please.

...
> > +	if (IS_ENABLED(CONFIG_OF) && np) {
> > +		syscon->base = of_iomap(np, 0);
> > +		if (!syscon->base)
> > +			return -EADDRNOTAVAIL;
> > +
> > +		res = &res_of;
> > +		ret = of_address_to_resource(np, 0, res);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +		if (!res)
> > +			return -ENXIO;
> > +
> > +		if (!request_mem_region(res->start, resource_size(res),
> > +					dev_name(&pdev->dev)))
> > +			return -EBUSY;
> > +
> > +		syscon->base = ioremap(res->start, resource_size(res));
> > +		if (!syscon->base)
> > +			return -EADDRNOTAVAIL;
> 
> devm_request_and_ioremap?

We call of_iomap for DT-version,  for removal procedure - iounmap.
Will iounmap work properly with devm_-version, I'm not sure.
May be better to completely remove ".remove" (and module_exit) feature for driver?
It is loaded at startup always once compiled, and should always be in the system.

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: Re[2]: [PATCH] mfd: syscon: Added support for using platform driver resources

[Dong Aisheng]

Hi Alexander,

On 7 February 2013 16:41, Alexander Shiyan <shc_work@mail.ru> wrote:
> Hello.
> ...
>> Thanks for the patch adding non-dt support. :-)
>>
>> On Mon, Feb 04, 2013 at 07:00:40PM +0400, Alexander Shiyan wrote:
>> > This patch adds support usage platform driver resources, i.e.
>> > possibility works without oftree support. Additionally patch
>> > removes CONFIG_OF dependency and adds helper for accessing
>> > regmap by searching device by its name.
> ...
>> > +static int syscon_match_name(struct device *dev, void *data)
>> > +{
>> > +   return !strcmp(dev_name(dev), (const char *)data);
>> > +}
>> > +
>> > +struct regmap *syscon_regmap_lookup_by_name(const char *name)
>> > +{
>> > +   struct syscon *syscon;
>> > +   struct device *dev;
>> > +
>> > +   dev = driver_find_device(&syscon_driver.driver, NULL, (void *)name,
>> > +                            syscon_match_name);
>> > +   if (!dev)
>> > +           return ERR_PTR(-EPROBE_DEFER);
>> > +
>> > +   syscon = dev_get_drvdata(dev);
>> > +
>> > +   return syscon->regmap;
>> > +}
>> > +
>>
>> How about syscon_dev_to_regmap(struct device *dev) as the exist dt version
>> syscon_node_to_regmap since it's not affected by the name change of devices?
>
> I am not completely understand what you mean. In my version which doing
> search regmap by name, we can call this function with desired device name,
> then use regmap:
> struct regmap *r = syscon_regmap_lookup_by_name("syscon.1");
>

My concern is that this API may be used by other client drivers, if we hardcode
the device name in those drivers, once the device name is changed,
all those drivers need change too.
For dt case, we use device node to find regmap, so does not care about the name.

> You suggest use "struct device" as parameter?

A device pointer.

> I do not know what we should
> use as parameter to the function in this case, since we can get "struct device"
> only when register this device,

If we have a platform_device, then we have its device pointer, right?

> i.e. in board support code, not from anywhere,
> for example from another driver.
> Fixme please.
>

My understanding is that in board support code, we can have the
platform device instance
of that syscon compatible device, then we can use it as the platform
data parameter for those devices driver who wants to use it.
Then in client driver, it can call:
syscon_dev_to_regmap(syscon_compatible_dev)
to find the regmap.
Just like dt working way, the device node usually uses a phandle pointing to
the syscon compatible node which it wants to use.

> ...
>> > +   if (IS_ENABLED(CONFIG_OF) && np) {
>> > +           syscon->base = of_iomap(np, 0);
>> > +           if (!syscon->base)
>> > +                   return -EADDRNOTAVAIL;
>> > +
>> > +           res = &res_of;
>> > +           ret = of_address_to_resource(np, 0, res);
>> > +           if (ret)
>> > +                   return ret;
>> > +   } else {
>> > +           res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > +           if (!res)
>> > +                   return -ENXIO;
>> > +
>> > +           if (!request_mem_region(res->start, resource_size(res),
>> > +                                   dev_name(&pdev->dev)))
>> > +                   return -EBUSY;
>> > +
>> > +           syscon->base = ioremap(res->start, resource_size(res));
>> > +           if (!syscon->base)
>> > +                   return -EADDRNOTAVAIL;
>>
>> devm_request_and_ioremap?
>
> We call of_iomap for DT-version,  for removal procedure - iounmap.
> Will iounmap work properly with devm_-version, I'm not sure.

You're right, i'm afraid not.
If that, i wonder the ioremap resource may be freed twice for driver removal.
So, i'm okay with your current way.
But one issue is that then we need take care of the resource free by ourselves
for probe failed cases. e.g. ioremap and request_mem_region.
You may need add them.

> May be better to completely remove ".remove" (and module_exit) feature for driver?
> It is loaded at startup always once compiled, and should always be in the system.
>

It supports to be a module, so it may be better to keep ".remove".

Regards
Dong Aisheng

Re[4]: [PATCH] mfd: syscon: Added support for using platform driver resources

[Alexander Shiyan]

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

Hello.

> > ...
> >> Thanks for the patch adding non-dt support. :-)
> >>
> >> On Mon, Feb 04, 2013 at 07:00:40PM +0400, Alexander Shiyan wrote:
> >> > This patch adds support usage platform driver resources, i.e.
> >> > possibility works without oftree support. Additionally patch
> >> > removes CONFIG_OF dependency and adds helper for accessing
> >> > regmap by searching device by its name.
> > ...
> >> > +static int syscon_match_name(struct device *dev, void *data)
> >> > +{
> >> > +   return !strcmp(dev_name(dev), (const char *)data);
> >> > +}
> >> > +
> >> > +struct regmap *syscon_regmap_lookup_by_name(const char *name)
> >> > +{
> >> > +   struct syscon *syscon;
> >> > +   struct device *dev;
> >> > +
> >> > +   dev = driver_find_device(&syscon_driver.driver, NULL, (void *)name,
> >> > +                            syscon_match_name);
> >> > +   if (!dev)
> >> > +           return ERR_PTR(-EPROBE_DEFER);
> >> > +
> >> > +   syscon = dev_get_drvdata(dev);
> >> > +
> >> > +   return syscon->regmap;
> >> > +}
> >> > +
> >>
> >> How about syscon_dev_to_regmap(struct device *dev) as the exist dt version
> >> syscon_node_to_regmap since it's not affected by the name change of devices?
> >
> > I am not completely understand what you mean. In my version which doing
> > search regmap by name, we can call this function with desired device name,
> > then use regmap:
> > struct regmap *r = syscon_regmap_lookup_by_name("syscon.1");
> >
> 
> My concern is that this API may be used by other client drivers, if we hardcode
> the device name in those drivers, once the device name is changed,
> all those drivers need change too.
> For dt case, we use device node to find regmap, so does not care about the name.
> 
> > You suggest use "struct device" as parameter?
> 
> A device pointer.
> 
> > I do not know what we should
> > use as parameter to the function in this case, since we can get "struct device"
> > only when register this device,
> 
> If we have a platform_device, then we have its device pointer, right?
> 
> > i.e. in board support code, not from anywhere,
> > for example from another driver.
> > Fixme please.
> >
> 
> My understanding is that in board support code, we can have the
> platform device instance
> of that syscon compatible device, then we can use it as the platform
> data parameter for those devices driver who wants to use it.
> Then in client driver, it can call:
> syscon_dev_to_regmap(syscon_compatible_dev)
> to find the regmap.
> Just like dt working way, the device node usually uses a phandle pointing to
> the syscon compatible node which it wants to use.

This is not as easy as it seems.
All devices that will use "syscon" driver, in this case should have a platform_data
record. I think that it can create problems with using these drivers as modules.
Alternatively, we can create additional (virtual) "compatible" text property in syscon
private data structure and use it to find the proper device. What is your opinion?

> > ...
> >> > +   if (IS_ENABLED(CONFIG_OF) && np) {
> >> > +           syscon->base = of_iomap(np, 0);
> >> > +           if (!syscon->base)
> >> > +                   return -EADDRNOTAVAIL;
> >> > +
> >> > +           res = &res_of;
> >> > +           ret = of_address_to_resource(np, 0, res);
> >> > +           if (ret)
> >> > +                   return ret;
> >> > +   } else {
> >> > +           res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> > +           if (!res)
> >> > +                   return -ENXIO;
> >> > +
> >> > +           if (!request_mem_region(res->start, resource_size(res),
> >> > +                                   dev_name(&pdev->dev)))
> >> > +                   return -EBUSY;
> >> > +
> >> > +           syscon->base = ioremap(res->start, resource_size(res));
> >> > +           if (!syscon->base)
> >> > +                   return -EADDRNOTAVAIL;
> >>
> >> devm_request_and_ioremap?
> >
> > We call of_iomap for DT-version,  for removal procedure - iounmap.
> > Will iounmap work properly with devm_-version, I'm not sure.
> 
> You're right, i'm afraid not.
> If that, i wonder the ioremap resource may be freed twice for driver removal.
> So, i'm okay with your current way.
> But one issue is that then we need take care of the resource free by ourselves
> for probe failed cases. e.g. ioremap and request_mem_region.
> You may need add them.

Yes, thanks. I forget about it.

> > May be better to completely remove ".remove" (and module_exit) feature for driver?
> > It is loaded at startup always once compiled, and should always be in the system.
> It supports to be a module, so it may be better to keep ".remove".

As far I can see, no. Kernel symbol defined as bool, so driver cannot be compiled
as module:
config MFD_SYSCON
        bool "System Controller Register R/W Based on Regmap"

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: Re[4]: [PATCH] mfd: syscon: Added support for using platform driver resources

[Dong Aisheng]

Hi Alexander,

On 7 February 2013 23:52, Alexander Shiyan <shc_work@mail.ru> wrote:
> Hello.
>
>> > ...
>> >> Thanks for the patch adding non-dt support. :-)
>> >>
>> >> On Mon, Feb 04, 2013 at 07:00:40PM +0400, Alexander Shiyan wrote:
>> >> > This patch adds support usage platform driver resources, i.e.
>> >> > possibility works without oftree support. Additionally patch
>> >> > removes CONFIG_OF dependency and adds helper for accessing
>> >> > regmap by searching device by its name.
>> > ...
>> >> > +static int syscon_match_name(struct device *dev, void *data)
>> >> > +{
>> >> > +   return !strcmp(dev_name(dev), (const char *)data);
>> >> > +}
>> >> > +
>> >> > +struct regmap *syscon_regmap_lookup_by_name(const char *name)
>> >> > +{
>> >> > +   struct syscon *syscon;
>> >> > +   struct device *dev;
>> >> > +
>> >> > +   dev = driver_find_device(&syscon_driver.driver, NULL, (void *)name,
>> >> > +                            syscon_match_name);
>> >> > +   if (!dev)
>> >> > +           return ERR_PTR(-EPROBE_DEFER);
>> >> > +
>> >> > +   syscon = dev_get_drvdata(dev);
>> >> > +
>> >> > +   return syscon->regmap;
>> >> > +}
>> >> > +
>> >>
>> >> How about syscon_dev_to_regmap(struct device *dev) as the exist dt version
>> >> syscon_node_to_regmap since it's not affected by the name change of devices?
>> >
>> > I am not completely understand what you mean. In my version which doing
>> > search regmap by name, we can call this function with desired device name,
>> > then use regmap:
>> > struct regmap *r = syscon_regmap_lookup_by_name("syscon.1");
>> >
>>
>> My concern is that this API may be used by other client drivers, if we hardcode
>> the device name in those drivers, once the device name is changed,
>> all those drivers need change too.
>> For dt case, we use device node to find regmap, so does not care about the name.
>>
>> > You suggest use "struct device" as parameter?
>>
>> A device pointer.
>>
>> > I do not know what we should
>> > use as parameter to the function in this case, since we can get "struct device"
>> > only when register this device,
>>
>> If we have a platform_device, then we have its device pointer, right?
>>
>> > i.e. in board support code, not from anywhere,
>> > for example from another driver.
>> > Fixme please.
>> >
>>
>> My understanding is that in board support code, we can have the
>> platform device instance
>> of that syscon compatible device, then we can use it as the platform
>> data parameter for those devices driver who wants to use it.
>> Then in client driver, it can call:
>> syscon_dev_to_regmap(syscon_compatible_dev)
>> to find the regmap.
>> Just like dt working way, the device node usually uses a phandle pointing to
>> the syscon compatible node which it wants to use.
>
> This is not as easy as it seems.
> All devices that will use "syscon" driver, in this case should have a platform_data
> record.

Yes, that's the same way as the dt version does.

> I think that it can create problems with using these drivers as modules.

What problems do you think?

> Alternatively, we can create additional (virtual) "compatible" text property in syscon
> private data structure and use it to find the proper device. What is your opinion?
>

Hmm, i can't see why we need that.
IMO, for non-dt, we can just use platform_device_id to match devices.

>> > ...
>> >> > +   if (IS_ENABLED(CONFIG_OF) && np) {
>> >> > +           syscon->base = of_iomap(np, 0);
>> >> > +           if (!syscon->base)
>> >> > +                   return -EADDRNOTAVAIL;
>> >> > +
>> >> > +           res = &res_of;
>> >> > +           ret = of_address_to_resource(np, 0, res);
>> >> > +           if (ret)
>> >> > +                   return ret;
>> >> > +   } else {
>> >> > +           res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> > +           if (!res)
>> >> > +                   return -ENXIO;
>> >> > +
>> >> > +           if (!request_mem_region(res->start, resource_size(res),
>> >> > +                                   dev_name(&pdev->dev)))
>> >> > +                   return -EBUSY;
>> >> > +
>> >> > +           syscon->base = ioremap(res->start, resource_size(res));
>> >> > +           if (!syscon->base)
>> >> > +                   return -EADDRNOTAVAIL;
>> >>
>> >> devm_request_and_ioremap?
>> >
>> > We call of_iomap for DT-version,  for removal procedure - iounmap.
>> > Will iounmap work properly with devm_-version, I'm not sure.
>>
>> You're right, i'm afraid not.
>> If that, i wonder the ioremap resource may be freed twice for driver removal.
>> So, i'm okay with your current way.
>> But one issue is that then we need take care of the resource free by ourselves
>> for probe failed cases. e.g. ioremap and request_mem_region.
>> You may need add them.
>
> Yes, thanks. I forget about it.
>
>> > May be better to completely remove ".remove" (and module_exit) feature for driver?
>> > It is loaded at startup always once compiled, and should always be in the system.
>> It supports to be a module, so it may be better to keep ".remove".
>
> As far I can see, no. Kernel symbol defined as bool, so driver cannot be compiled
> as module:
> config MFD_SYSCON
>         bool "System Controller Register R/W Based on Regmap"
>

I see.
Thanks

Regards
Dong Aisheng

> Thanks!
>
> ---

Re[6]: [PATCH] mfd: syscon: Added support for using platform driver resources

[Alexander Shiyan]

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

Hello.

...
> >> >> Thanks for the patch adding non-dt support. :-)
> >> >>
> >> >> On Mon, Feb 04, 2013 at 07:00:40PM +0400, Alexander Shiyan wrote:
> >> >> > This patch adds support usage platform driver resources, i.e.
> >> >> > possibility works without oftree support. Additionally patch
> >> >> > removes CONFIG_OF dependency and adds helper for accessing
> >> >> > regmap by searching device by its name.
> >> > ...
> >> >> > +static int syscon_match_name(struct device *dev, void *data)
> >> >> > +{
> >> >> > +   return !strcmp(dev_name(dev), (const char *)data);
> >> >> > +}
> >> >> > +
> >> >> > +struct regmap *syscon_regmap_lookup_by_name(const char *name)
> >> >> > +{
> >> >> > +   struct syscon *syscon;
> >> >> > +   struct device *dev;
> >> >> > +
> >> >> > +   dev = driver_find_device(&syscon_driver.driver, NULL, (void *)name,
> >> >> > +                            syscon_match_name);
> >> >> > +   if (!dev)
> >> >> > +           return ERR_PTR(-EPROBE_DEFER);
> >> >> > +
> >> >> > +   syscon = dev_get_drvdata(dev);
> >> >> > +
> >> >> > +   return syscon->regmap;
> >> >> > +}
> >> >> > +
> >> >>
> >> >> How about syscon_dev_to_regmap(struct device *dev) as the exist dt version
> >> >> syscon_node_to_regmap since it's not affected by the name change of devices?
> >> >
> >> > I am not completely understand what you mean. In my version which doing
> >> > search regmap by name, we can call this function with desired device name,
> >> > then use regmap:
> >> > struct regmap *r = syscon_regmap_lookup_by_name("syscon.1");
> >> >
> >>
> >> My concern is that this API may be used by other client drivers, if we hardcode
> >> the device name in those drivers, once the device name is changed,
> >> all those drivers need change too.
> >> For dt case, we use device node to find regmap, so does not care about the name.
> >>
> >> > You suggest use "struct device" as parameter?
> >>
> >> A device pointer.
> >>
> >> > I do not know what we should
> >> > use as parameter to the function in this case, since we can get "struct device"
> >> > only when register this device,
> >>
> >> If we have a platform_device, then we have its device pointer, right?
> >>
> >> > i.e. in board support code, not from anywhere,
> >> > for example from another driver.
> >> > Fixme please.
> >> >
> >> My understanding is that in board support code, we can have the
> >> platform device instance
> >> of that syscon compatible device, then we can use it as the platform
> >> data parameter for those devices driver who wants to use it.
> >> Then in client driver, it can call:
> >> syscon_dev_to_regmap(syscon_compatible_dev)
> >> to find the regmap.
> >> Just like dt working way, the device node usually uses a phandle pointing to
> >> the syscon compatible node which it wants to use.
> >
> > This is not as easy as it seems.
> > All devices that will use "syscon" driver, in this case should have a platform_data
> > record.
> 
> Yes, that's the same way as the dt version does.
> 
> > I think that it can create problems with using these drivers as modules.
> 
> What problems do you think?

I've changed my opinion ;)

> > Alternatively, we can create additional (virtual) "compatible" text property in syscon
> > private data structure and use it to find the proper device. What is your opinion?
> >
> 
> Hmm, i can't see why we need that.
> IMO, for non-dt, we can just use platform_device_id to match devices.

Unfortunately platform_device_id.name field have a length limitation. But, of course,
this is a other theme for discussion. So, I'll send new version of patch.
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¥