[PATCH v3] mfd: syscon: Add non-DT support

[PATCH v3] 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.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/mfd/Kconfig  |  1 -
 drivers/mfd/syscon.c | 76 +++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 57 insertions(+), 20 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 4a7ed5a..3c0abcb 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -28,7 +28,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;
 
@@ -41,7 +41,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);
 
@@ -51,19 +51,41 @@ struct regmap *syscon_node_to_regmap(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
 
+static int syscon_match_id(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)
+		return !strcmp(id->name, (const char *)data);
+
+	return 0;
+}
+
 struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
 {
 	struct device_node *syscon_np;
 	struct regmap *regmap;
+	struct syscon *syscon;
+	struct device *dev;
 
 	syscon_np = of_find_compatible_node(NULL, NULL, s);
-	if (!syscon_np)
+	if (syscon_np) {
+		regmap = syscon_node_to_regmap(syscon_np);
+		of_node_put(syscon_np);
+
+		return regmap;
+	}
+
+	/* Fallback to search by id_entry.name string */
+	dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
+				 syscon_match_id);
+	if (!dev)
 		return ERR_PTR(-ENODEV);
 
-	regmap = syscon_node_to_regmap(syscon_np);
-	of_node_put(syscon_np);
+	syscon = dev_get_drvdata(dev);
 
-	return regmap;
+	return syscon->regmap;
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
 
@@ -100,37 +122,53 @@ 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) {
+			iounmap(syscon->base);
+			return ret;
+		}
+	} else {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!res)
+			return -ENOENT;
+
+		if (!devm_request_mem_region(dev, 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)) {
 		dev_err(dev, "regmap init failed\n");
+		iounmap(syscon->base);
 		return PTR_ERR(syscon->regmap);
 	}
 
 	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;
 }
-- 
1.7.12.4

Re: [PATCH v3] mfd: syscon: Add non-DT support

[Arnd Bergmann]

On Monday 18 February 2013, Alexander Shiyan wrote:
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 4a7ed5a..3c0abcb 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c

Hi Alexander,

>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
>  {
>  	struct device_node *syscon_np;
>  	struct regmap *regmap;
> +	struct syscon *syscon;
> +	struct device *dev;
>  
>  	syscon_np = of_find_compatible_node(NULL, NULL, s);
> -	if (!syscon_np)
> +	if (syscon_np) {
> +		regmap = syscon_node_to_regmap(syscon_np);
> +		of_node_put(syscon_np);
> +
> +		return regmap;
> +	}
> +
> +	/* Fallback to search by id_entry.name string */
> +	dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> +				 syscon_match_id);
> +	if (!dev)
>  		return ERR_PTR(-ENODEV);
>  
> -	regmap = syscon_node_to_regmap(syscon_np);
> -	of_node_put(syscon_np);
> +	syscon = dev_get_drvdata(dev);
>  
> -	return regmap;
> +	return syscon->regmap;
>  }
>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);

Since you are not actually comparing the "compatible" property here,
I would suggest adding another function here, something like
syscon_regmap_lookup_by_pdevname() that you can use in device
drivers that are not DT-enabled. I would also recommend enclosing
that function in #ifdef CONFIG_ATAGS.

Which code do you have in mind that would call this anyway?
The changeset description does not really explain what ATAG
boot support in syscon is good for.

> +	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) {
> +			iounmap(syscon->base);
> +			return ret;
> +		}
> +	} else {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!res)
> +			return -ENOENT;
> +
> +		if (!devm_request_mem_region(dev, res->start,
> +					     resource_size(res),
> +					     dev_name(&pdev->dev)))
> +			return -EBUSY;
> +
> +		syscon->base = ioremap(res->start, resource_size(res));
> +		if (!syscon->base)
> +			return -EADDRNOTAVAIL;
> +	}

These two code paths look equivalent. Why not always use the second
one? Also, you might want to convert this to devm_ioremap_resource()
to simplify the code in the process.

	Arnd

Re: [PATCH v3] mfd: syscon: Add non-DT support

[Dong Aisheng]

On 19 February 2013 00:02, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 18 February 2013, Alexander Shiyan wrote:
>> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
>> index 4a7ed5a..3c0abcb 100644
>> --- a/drivers/mfd/syscon.c
>> +++ b/drivers/mfd/syscon.c
>
> Hi Alexander,
>
>>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
>>  {
>>       struct device_node *syscon_np;
>>       struct regmap *regmap;
>> +     struct syscon *syscon;
>> +     struct device *dev;
>>
>>       syscon_np = of_find_compatible_node(NULL, NULL, s);
>> -     if (!syscon_np)
>> +     if (syscon_np) {
>> +             regmap = syscon_node_to_regmap(syscon_np);
>> +             of_node_put(syscon_np);
>> +
>> +             return regmap;
>> +     }
>> +
>> +     /* Fallback to search by id_entry.name string */
>> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
>> +                              syscon_match_id);
>> +     if (!dev)
>>               return ERR_PTR(-ENODEV);
>>
>> -     regmap = syscon_node_to_regmap(syscon_np);
>> -     of_node_put(syscon_np);
>> +     syscon = dev_get_drvdata(dev);
>>
>> -     return regmap;
>> +     return syscon->regmap;
>>  }
>>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
>
> Since you are not actually comparing the "compatible" property here,
> I would suggest adding another function here,

Yes, i also think like that.

> something like
> syscon_regmap_lookup_by_pdevname() that you can use in device
> drivers that are not DT-enabled.

IMHO i would like syscon_dev_to_regmap, then we do not need to
care in case pdevname changes.
See:
http://www.gossamer-threads.com/lists/linux/kernel/1675210#1675210
What do you think?

> I would also recommend enclosing
> that function in #ifdef CONFIG_ATAGS.
>
> Which code do you have in mind that would call this anyway?
> The changeset description does not really explain what ATAG
> boot support in syscon is good for.
>
>> +     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) {
>> +                     iounmap(syscon->base);
>> +                     return ret;
>> +             }
>> +     } else {
>> +             res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +             if (!res)
>> +                     return -ENOENT;
>> +
>> +             if (!devm_request_mem_region(dev, res->start,
>> +                                          resource_size(res),
>> +                                          dev_name(&pdev->dev)))
>> +                     return -EBUSY;
>> +
>> +             syscon->base = ioremap(res->start, resource_size(res));
>> +             if (!syscon->base)
>> +                     return -EADDRNOTAVAIL;
>> +     }
>
> These two code paths look equivalent. Why not always use the second
> one? Also, you might want to convert this to devm_ioremap_resource()
> to simplify the code in the process.
>

These two code paths have a slight difference.
The path1 does not request the mem region, the main reason of that is we meet
some devices register ranges used as syscon maybe overlapped with
other exist drivers.
e.g imx6q.dtsi
The gpr is a few registers contained in iomuxc.
gpr: iomuxc-gpr@020e0000 {
        compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
        reg = <0x020e0000 0x38>;
};

iomuxc: iomuxc@020e0000 {
        compatible = "fsl,imx6q-iomuxc";
        reg = <0x020e0000 0x4000>;
...
};

The iomuxc already has a pinctrl driver, so there are conflicts if both
request the same mem region.

So i wonder maybe we also do not need request mem region in non-dt case,
then we can only  use second code path.

Regards
Dong Aisheng

Re[2]: [PATCH v3] 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: 4645 bytes --]

Hello.

Strange, but I not received an original answer from Arnd, have only this mail.

...
> >> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> >> index 4a7ed5a..3c0abcb 100644
> >> --- a/drivers/mfd/syscon.c
> >> +++ b/drivers/mfd/syscon.c
> >
> > Hi Alexander,
> >
> >>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
> >>  {
> >>       struct device_node *syscon_np;
> >>       struct regmap *regmap;
> >> +     struct syscon *syscon;
> >> +     struct device *dev;
> >>
> >>       syscon_np = of_find_compatible_node(NULL, NULL, s);
> >> -     if (!syscon_np)
> >> +     if (syscon_np) {
> >> +             regmap = syscon_node_to_regmap(syscon_np);
> >> +             of_node_put(syscon_np);
> >> +
> >> +             return regmap;
> >> +     }
> >> +
> >> +     /* Fallback to search by id_entry.name string */
> >> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> >> +                              syscon_match_id);
> >> +     if (!dev)
> >>               return ERR_PTR(-ENODEV);
> >>
> >> -     regmap = syscon_node_to_regmap(syscon_np);
> >> -     of_node_put(syscon_np);
> >> +     syscon = dev_get_drvdata(dev);
> >>
> >> -     return regmap;
> >> +     return syscon->regmap;
> >>  }
> >>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
> >
> > Since you are not actually comparing the "compatible" property here,
> > I would suggest adding another function here,
> 
> Yes, i also think like that.

In this case we should provide two paths for drivers which can work as with DT
and without DT. In my case we can use platform_device_id.name field with
"compatible" string. My way in this case is transparency for driver which is
using "syscon".

> 
> > something like
> > syscon_regmap_lookup_by_pdevname() that you can use in device
> > drivers that are not DT-enabled.
> 
> IMHO i would like syscon_dev_to_regmap, then we do not need to
> care in case pdevname changes.
> See:
> http://www.gossamer-threads.com/lists/linux/kernel/1675210#1675210
> What do you think?

For me, I still do not understand how we get syscon_dev from driver.

> > I would also recommend enclosing
> > that function in #ifdef CONFIG_ATAGS.
> >
> > Which code do you have in mind that would call this anyway?
> > The changeset description does not really explain what ATAG
> > boot support in syscon is good for.
> >
> >> +     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) {
> >> +                     iounmap(syscon->base);
> >> +                     return ret;
> >> +             }
> >> +     } else {
> >> +             res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +             if (!res)
> >> +                     return -ENOENT;
> >> +
> >> +             if (!devm_request_mem_region(dev, res->start,
> >> +                                          resource_size(res),
> >> +                                          dev_name(&pdev->dev)))
> >> +                     return -EBUSY;
> >> +
> >> +             syscon->base = ioremap(res->start, resource_size(res));
> >> +             if (!syscon->base)
> >> +                     return -EADDRNOTAVAIL;
> >> +     }
> >
> > These two code paths look equivalent. Why not always use the second
> > one? Also, you might want to convert this to devm_ioremap_resource()
> > to simplify the code in the process.
> >
> 
> These two code paths have a slight difference.
> The path1 does not request the mem region, the main reason of that is we meet
> some devices register ranges used as syscon maybe overlapped with
> other exist drivers.
> e.g imx6q.dtsi
> The gpr is a few registers contained in iomuxc.
> gpr: iomuxc-gpr@020e0000 {
>         compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
>         reg = <0x020e0000 0x38>;
> };
> 
> iomuxc: iomuxc@020e0000 {
>         compatible = "fsl,imx6q-iomuxc";
>         reg = <0x020e0000 0x4000>;
> ...
> };
> 
> The iomuxc already has a pinctrl driver, so there are conflicts if both
> request the same mem region.
> 
> So i wonder maybe we also do not need request mem region in non-dt case,
> then we can only  use second code path.

So, you suggest completely remove CONFIG_OF-dependent code from "probe"?
I am not a guru in DT, is it will work correct?

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

[Dong Aisheng]

On 19 February 2013 15:03, Alexander Shiyan <shc_work@mail.ru> wrote:
> Hello.
>
> Strange, but I not received an original answer from Arnd, have only this mail.
>
> ...
>> >> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
>> >> index 4a7ed5a..3c0abcb 100644
>> >> --- a/drivers/mfd/syscon.c
>> >> +++ b/drivers/mfd/syscon.c
>> >
>> > Hi Alexander,
>> >
>> >>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
>> >>  {
>> >>       struct device_node *syscon_np;
>> >>       struct regmap *regmap;
>> >> +     struct syscon *syscon;
>> >> +     struct device *dev;
>> >>
>> >>       syscon_np = of_find_compatible_node(NULL, NULL, s);
>> >> -     if (!syscon_np)
>> >> +     if (syscon_np) {
>> >> +             regmap = syscon_node_to_regmap(syscon_np);
>> >> +             of_node_put(syscon_np);
>> >> +
>> >> +             return regmap;
>> >> +     }
>> >> +
>> >> +     /* Fallback to search by id_entry.name string */
>> >> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
>> >> +                              syscon_match_id);
>> >> +     if (!dev)
>> >>               return ERR_PTR(-ENODEV);
>> >>
>> >> -     regmap = syscon_node_to_regmap(syscon_np);
>> >> -     of_node_put(syscon_np);
>> >> +     syscon = dev_get_drvdata(dev);
>> >>
>> >> -     return regmap;
>> >> +     return syscon->regmap;
>> >>  }
>> >>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
>> >
>> > Since you are not actually comparing the "compatible" property here,
>> > I would suggest adding another function here,
>>
>> Yes, i also think like that.
>
> In this case we should provide two paths for drivers which can work as with DT
> and without DT.

Yes.

> In my case we can use platform_device_id.name field with
> "compatible" string. My way in this case is transparency for driver which is
> using "syscon".
>

Yes, but it also brings misleading and mass.
And i wonder even the API can cover the two type of matches, the
caller still can't use
the only one name for two cases since the name is different.
So it looks to me not make too much sense to provide only one API.

>>
>> > something like
>> > syscon_regmap_lookup_by_pdevname() that you can use in device
>> > drivers that are not DT-enabled.
>>
>> IMHO i would like syscon_dev_to_regmap, then we do not need to
>> care in case pdevname changes.
>> See:
>> http://www.gossamer-threads.com/lists/linux/kernel/1675210#1675210
>> What do you think?
>
> For me, I still do not understand how we get syscon_dev from driver.
>

Can't via platform_data?

>> > I would also recommend enclosing
>> > that function in #ifdef CONFIG_ATAGS.
>> >
>> > Which code do you have in mind that would call this anyway?
>> > The changeset description does not really explain what ATAG
>> > boot support in syscon is good for.
>> >
>> >> +     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) {
>> >> +                     iounmap(syscon->base);
>> >> +                     return ret;
>> >> +             }
>> >> +     } else {
>> >> +             res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> +             if (!res)
>> >> +                     return -ENOENT;
>> >> +
>> >> +             if (!devm_request_mem_region(dev, res->start,
>> >> +                                          resource_size(res),
>> >> +                                          dev_name(&pdev->dev)))
>> >> +                     return -EBUSY;
>> >> +
>> >> +             syscon->base = ioremap(res->start, resource_size(res));
>> >> +             if (!syscon->base)
>> >> +                     return -EADDRNOTAVAIL;
>> >> +     }
>> >
>> > These two code paths look equivalent. Why not always use the second
>> > one? Also, you might want to convert this to devm_ioremap_resource()
>> > to simplify the code in the process.
>> >
>>
>> These two code paths have a slight difference.
>> The path1 does not request the mem region, the main reason of that is we meet
>> some devices register ranges used as syscon maybe overlapped with
>> other exist drivers.
>> e.g imx6q.dtsi
>> The gpr is a few registers contained in iomuxc.
>> gpr: iomuxc-gpr@020e0000 {
>>         compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
>>         reg = <0x020e0000 0x38>;
>> };
>>
>> iomuxc: iomuxc@020e0000 {
>>         compatible = "fsl,imx6q-iomuxc";
>>         reg = <0x020e0000 0x4000>;
>> ...
>> };
>>
>> The iomuxc already has a pinctrl driver, so there are conflicts if both
>> request the same mem region.
>>
>> So i wonder maybe we also do not need request mem region in non-dt case,
>> then we can only  use second code path.
>
> So, you suggest completely remove CONFIG_OF-dependent code from "probe"?
> I am not a guru in DT, is it will work correct?
>

I think yes.
The core will create resource for the populated devices.
See: of_platform_populate

Regards
Dong Aisheng

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

[Dong Aisheng]

On 19 February 2013 15:55, Dong Aisheng <dong.aisheng@linaro.org> wrote:
> On 19 February 2013 15:03, Alexander Shiyan <shc_work@mail.ru> wrote:
>> Hello.
>>
>> Strange, but I not received an original answer from Arnd, have only this mail.
>>
>> ...
>>> >> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
>>> >> index 4a7ed5a..3c0abcb 100644
>>> >> --- a/drivers/mfd/syscon.c
>>> >> +++ b/drivers/mfd/syscon.c
>>> >
>>> > Hi Alexander,
>>> >
>>> >>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
>>> >>  {
>>> >>       struct device_node *syscon_np;
>>> >>       struct regmap *regmap;
>>> >> +     struct syscon *syscon;
>>> >> +     struct device *dev;
>>> >>
>>> >>       syscon_np = of_find_compatible_node(NULL, NULL, s);
>>> >> -     if (!syscon_np)
>>> >> +     if (syscon_np) {
>>> >> +             regmap = syscon_node_to_regmap(syscon_np);
>>> >> +             of_node_put(syscon_np);
>>> >> +
>>> >> +             return regmap;
>>> >> +     }
>>> >> +
>>> >> +     /* Fallback to search by id_entry.name string */
>>> >> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
>>> >> +                              syscon_match_id);
>>> >> +     if (!dev)
>>> >>               return ERR_PTR(-ENODEV);
>>> >>
>>> >> -     regmap = syscon_node_to_regmap(syscon_np);
>>> >> -     of_node_put(syscon_np);
>>> >> +     syscon = dev_get_drvdata(dev);
>>> >>
>>> >> -     return regmap;
>>> >> +     return syscon->regmap;
>>> >>  }
>>> >>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
>>> >
>>> > Since you are not actually comparing the "compatible" property here,
>>> > I would suggest adding another function here,
>>>
>>> Yes, i also think like that.
>>
>> In this case we should provide two paths for drivers which can work as with DT
>> and without DT.
>
> Yes.
>
>> In my case we can use platform_device_id.name field with
>> "compatible" string. My way in this case is transparency for driver which is
>> using "syscon".
>>
>
> Yes, but it also brings misleading and mass.
> And i wonder even the API can cover the two type of matches, the
> caller still can't use
> the only one name for two cases since the name is different.
> So it looks to me not make too much sense to provide only one API.
>
>>>
>>> > something like
>>> > syscon_regmap_lookup_by_pdevname() that you can use in device
>>> > drivers that are not DT-enabled.
>>>
>>> IMHO i would like syscon_dev_to_regmap, then we do not need to
>>> care in case pdevname changes.
>>> See:
>>> http://www.gossamer-threads.com/lists/linux/kernel/1675210#1675210
>>> What do you think?
>>
>> For me, I still do not understand how we get syscon_dev from driver.
>>
>
> Can't via platform_data?
>

Hmm...
I think one real problem is that for those dynamically created platform device,
we can not easily get its pointer in board file.
Then we can not pass the syscon device pointer via platfrom_data...

Regards
Dong Aisheng

Re[4]: [PATCH v3] 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: 2275 bytes --]

...
> >> >>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
> >> >>  {
> >> >>       struct device_node *syscon_np;
> >> >>       struct regmap *regmap;
> >> >> +     struct syscon *syscon;
> >> >> +     struct device *dev;
> >> >>
> >> >>       syscon_np = of_find_compatible_node(NULL, NULL, s);
> >> >> -     if (!syscon_np)
> >> >> +     if (syscon_np) {
> >> >> +             regmap = syscon_node_to_regmap(syscon_np);
> >> >> +             of_node_put(syscon_np);
> >> >> +
> >> >> +             return regmap;
> >> >> +     }
> >> >> +
> >> >> +     /* Fallback to search by id_entry.name string */
> >> >> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> >> >> +                              syscon_match_id);
> >> >> +     if (!dev)
> >> >>               return ERR_PTR(-ENODEV);
> >> >>
> >> >> -     regmap = syscon_node_to_regmap(syscon_np);
> >> >> -     of_node_put(syscon_np);
> >> >> +     syscon = dev_get_drvdata(dev);
> >> >>
> >> >> -     return regmap;
> >> >> +     return syscon->regmap;
> >> >>  }
> >> >>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
> >> >
> >> > Since you are not actually comparing the "compatible" property here,
> >> > I would suggest adding another function here,
> >>
> >> Yes, i also think like that.
> >
> > In this case we should provide two paths for drivers which can work as with DT
> > and without DT.
> 
> Yes.

I still think the universal procedure is better for the driver.

> > In my case we can use platform_device_id.name field with
> > "compatible" string. My way in this case is transparency for driver which is
> > using "syscon".
> >
> 
> Yes, but it also brings misleading and mass.
> And i wonder even the API can cover the two type of matches, the
> caller still can't use
> the only one name for two cases since the name is different.
> So it looks to me not make too much sense to provide only one API.

The previous version of the patch keep conformity to the name of
procedure ("compatible" field in platform_data)...

So, now I'm totally confused what we do with the search function.

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

[Dong Aisheng]

On 19 February 2013 16:56, Alexander Shiyan <shc_work@mail.ru> wrote:
> ...
>> >> >>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
>> >> >>  {
>> >> >>       struct device_node *syscon_np;
>> >> >>       struct regmap *regmap;
>> >> >> +     struct syscon *syscon;
>> >> >> +     struct device *dev;
>> >> >>
>> >> >>       syscon_np = of_find_compatible_node(NULL, NULL, s);
>> >> >> -     if (!syscon_np)
>> >> >> +     if (syscon_np) {
>> >> >> +             regmap = syscon_node_to_regmap(syscon_np);
>> >> >> +             of_node_put(syscon_np);
>> >> >> +
>> >> >> +             return regmap;
>> >> >> +     }
>> >> >> +
>> >> >> +     /* Fallback to search by id_entry.name string */
>> >> >> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
>> >> >> +                              syscon_match_id);
>> >> >> +     if (!dev)
>> >> >>               return ERR_PTR(-ENODEV);
>> >> >>
>> >> >> -     regmap = syscon_node_to_regmap(syscon_np);
>> >> >> -     of_node_put(syscon_np);
>> >> >> +     syscon = dev_get_drvdata(dev);
>> >> >>
>> >> >> -     return regmap;
>> >> >> +     return syscon->regmap;
>> >> >>  }
>> >> >>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
>> >> >
>> >> > Since you are not actually comparing the "compatible" property here,
>> >> > I would suggest adding another function here,
>> >>
>> >> Yes, i also think like that.
>> >
>> > In this case we should provide two paths for drivers which can work as with DT
>> > and without DT.
>>
>> Yes.
>
> I still think the universal procedure is better for the driver.
>

Why?
I did not see your reply on my other comments on the problems of using universal
procedure?
Please let me know if you think they're not issues.

>> > In my case we can use platform_device_id.name field with
>> > "compatible" string. My way in this case is transparency for driver which is
>> > using "syscon".
>> >
>>
>> Yes, but it also brings misleading and mass.
>> And i wonder even the API can cover the two type of matches, the
>> caller still can't use
>> the only one name for two cases since the name is different.
>> So it looks to me not make too much sense to provide only one API.
>
> The previous version of the patch keep conformity to the name of
> procedure ("compatible" field in platform_data)...
>
> So, now I'm totally confused what we do with the search function.
>

You can do as you currently do but with a different API for non-dt.

Regards
Dong Aisheng

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

[Arnd Bergmann]

On Tuesday 19 February 2013, Alexander Shiyan wrote:
> Hello.
> 
> Strange, but I not received an original answer from Arnd, have only this mail.

I got a reject notice from mail.ru, which apparently didn't like the
SMTP server of my German ISP. I already tried to resend my mail to you,
and now try to send it out through the gmail server. Please let me know
if you get it.

> > >> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > >> index 4a7ed5a..3c0abcb 100644
> > >> --- a/drivers/mfd/syscon.c
> > >> +++ b/drivers/mfd/syscon.c
> > >
> > > Hi Alexander,
> > >
> > >>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
> > >>  {
> > >>       struct device_node *syscon_np;
> > >>       struct regmap *regmap;
> > >> +     struct syscon *syscon;
> > >> +     struct device *dev;
> > >>
> > >>       syscon_np = of_find_compatible_node(NULL, NULL, s);
> > >> -     if (!syscon_np)
> > >> +     if (syscon_np) {
> > >> +             regmap = syscon_node_to_regmap(syscon_np);
> > >> +             of_node_put(syscon_np);
> > >> +
> > >> +             return regmap;
> > >> +     }
> > >> +
> > >> +     /* Fallback to search by id_entry.name string */
> > >> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> > >> +                              syscon_match_id);
> > >> +     if (!dev)
> > >>               return ERR_PTR(-ENODEV);
> > >>
> > >> -     regmap = syscon_node_to_regmap(syscon_np);
> > >> -     of_node_put(syscon_np);
> > >> +     syscon = dev_get_drvdata(dev);
> > >>
> > >> -     return regmap;
> > >> +     return syscon->regmap;
> > >>  }
> > >>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
> > >
> > > Since you are not actually comparing the "compatible" property here,
> > > I would suggest adding another function here,
> > 
> > Yes, i also think like that.
> 
> In this case we should provide two paths for drivers which can work as with DT
> and without DT. In my case we can use platform_device_id.name field with
> "compatible" string. My way in this case is transparency for driver which is
> using "syscon".

But drivers should use the _by_phandle variant normally, which does
not work without DT, so you already have separate code paths.

	Arnd

Re[6]: [PATCH v3] 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: 2471 bytes --]

...
> >> >> >>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
> >> >> >>  {
> >> >> >>       struct device_node *syscon_np;
> >> >> >>       struct regmap *regmap;
> >> >> >> +     struct syscon *syscon;
> >> >> >> +     struct device *dev;
> >> >> >>
> >> >> >>       syscon_np = of_find_compatible_node(NULL, NULL, s);
> >> >> >> -     if (!syscon_np)
> >> >> >> +     if (syscon_np) {
> >> >> >> +             regmap = syscon_node_to_regmap(syscon_np);
> >> >> >> +             of_node_put(syscon_np);
> >> >> >> +
> >> >> >> +             return regmap;
> >> >> >> +     }
> >> >> >> +
> >> >> >> +     /* Fallback to search by id_entry.name string */
> >> >> >> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> >> >> >> +                              syscon_match_id);
> >> >> >> +     if (!dev)
> >> >> >>               return ERR_PTR(-ENODEV);
> >> >> >>
> >> >> >> -     regmap = syscon_node_to_regmap(syscon_np);
> >> >> >> -     of_node_put(syscon_np);
> >> >> >> +     syscon = dev_get_drvdata(dev);
> >> >> >>
> >> >> >> -     return regmap;
> >> >> >> +     return syscon->regmap;
> >> >> >>  }
> >> >> >>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
> >> >> >
> >> >> > Since you are not actually comparing the "compatible" property here,
> >> >> > I would suggest adding another function here,
> >> >>
> >> >> Yes, i also think like that.
> >> >
> >> > In this case we should provide two paths for drivers which can work as with DT
> >> > and without DT.
> >>
> >> Yes.
> >
> > I still think the universal procedure is better for the driver.
> >
> 
> Why?
> I did not see your reply on my other comments on the problems of using universal
> procedure?
> Please let me know if you think they're not issues.

Yes, I do not see a problem here.
I will try to show the code.

In the driver:
struct regmap *r;
r = syscon_regmap_lookup_by_compatible("my_super_syscon");

For DT case:
syscon@123456 {
  compatible = "my_super_syscon", "syscon";
  ...
};

For non-DT case:
struct platform_device_id id = { .name = "my_super_syscon", };
struct platform_device syscon_pdev = {                                           
  .name = "syscon",
  .id_entry = &id,
  ...
};
platform_device_register(&syscon_pdev);

Do I understand what you mean?
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[6]: [PATCH v3] mfd: syscon: Add non-DT support

[Dong Aisheng]

On 19 February 2013 18:54, Alexander Shiyan <shc_work@mail.ru> wrote:
> ...
>> >> >> >>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
>> >> >> >>  {
>> >> >> >>       struct device_node *syscon_np;
>> >> >> >>       struct regmap *regmap;
>> >> >> >> +     struct syscon *syscon;
>> >> >> >> +     struct device *dev;
>> >> >> >>
>> >> >> >>       syscon_np = of_find_compatible_node(NULL, NULL, s);
>> >> >> >> -     if (!syscon_np)
>> >> >> >> +     if (syscon_np) {
>> >> >> >> +             regmap = syscon_node_to_regmap(syscon_np);
>> >> >> >> +             of_node_put(syscon_np);
>> >> >> >> +
>> >> >> >> +             return regmap;
>> >> >> >> +     }
>> >> >> >> +
>> >> >> >> +     /* Fallback to search by id_entry.name string */
>> >> >> >> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
>> >> >> >> +                              syscon_match_id);
>> >> >> >> +     if (!dev)
>> >> >> >>               return ERR_PTR(-ENODEV);
>> >> >> >>
>> >> >> >> -     regmap = syscon_node_to_regmap(syscon_np);
>> >> >> >> -     of_node_put(syscon_np);
>> >> >> >> +     syscon = dev_get_drvdata(dev);
>> >> >> >>
>> >> >> >> -     return regmap;
>> >> >> >> +     return syscon->regmap;
>> >> >> >>  }
>> >> >> >>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
>> >> >> >
>> >> >> > Since you are not actually comparing the "compatible" property here,
>> >> >> > I would suggest adding another function here,
>> >> >>
>> >> >> Yes, i also think like that.
>> >> >
>> >> > In this case we should provide two paths for drivers which can work as with DT
>> >> > and without DT.
>> >>
>> >> Yes.
>> >
>> > I still think the universal procedure is better for the driver.
>> >
>>
>> Why?
>> I did not see your reply on my other comments on the problems of using universal
>> procedure?
>> Please let me know if you think they're not issues.
>
> Yes, I do not see a problem here.
> I will try to show the code.
>
> In the driver:
> struct regmap *r;
> r = syscon_regmap_lookup_by_compatible("my_super_syscon");
>
> For DT case:
> syscon@123456 {
>   compatible = "my_super_syscon", "syscon";
>   ...
> };
>
> For non-DT case:
> struct platform_device_id id = { .name = "my_super_syscon", };
> struct platform_device syscon_pdev = {
>   .name = "syscon",
>   .id_entry = &id,

This is really strange to me and i've never seen such using.
Usually the id_table is provided by the driver and the match process then will
set the correct id_entry for the platform device once it matches.
Please see the platform_bus match process: drivers/base/platform.c

>   ...
> };
> platform_device_register(&syscon_pdev);
>
> Do I understand what you mean?
>

My understanding for non-dt case is something like:
In client driver:
struct regmap *r;
r = syscon_regmap_lookup_by_pdevname("my_super_syscon");

In board code:
struct platform_device syscon_pdev = {
.name = "my_super_syscon",
...
};
 platform_device_register(&syscon_pdev);

In syscon driver:
static struct platform_device_id syscon_device_ids[] = {
        {
                .name = "my_super_syscon",
        }, {
                /* sentinel */
        }
};
MODULE_DEVICE_TABLE(platform, syscon_device_ids);

static struct platform_driver syscon_driver = {
        .driver = {
                .name = "syscon",
                .owner = THIS_MODULE,
                .of_match_table = of_syscon_match,
        },
        .id_table       = syscon_device_ids,
        .probe          = syscon_probe,
        .remove         = syscon_remove,
};

One problem is that every user needs to add their syscon compatible device
support(platform_device_id) in syscon driver first before they can use it.
But it looks to me just like how other driver generally does.

Arnd,
Do you think this is an issue?

Regards
Dong Aisheng

Re[8]: [PATCH v3] 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: 4523 bytes --]

> > ...
> >> >> >> >>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
> >> >> >> >>  {
> >> >> >> >>       struct device_node *syscon_np;
> >> >> >> >>       struct regmap *regmap;
> >> >> >> >> +     struct syscon *syscon;
> >> >> >> >> +     struct device *dev;
> >> >> >> >>
> >> >> >> >>       syscon_np = of_find_compatible_node(NULL, NULL, s);
> >> >> >> >> -     if (!syscon_np)
> >> >> >> >> +     if (syscon_np) {
> >> >> >> >> +             regmap = syscon_node_to_regmap(syscon_np);
> >> >> >> >> +             of_node_put(syscon_np);
> >> >> >> >> +
> >> >> >> >> +             return regmap;
> >> >> >> >> +     }
> >> >> >> >> +
> >> >> >> >> +     /* Fallback to search by id_entry.name string */
> >> >> >> >> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> >> >> >> >> +                              syscon_match_id);
> >> >> >> >> +     if (!dev)
> >> >> >> >>               return ERR_PTR(-ENODEV);
> >> >> >> >>
> >> >> >> >> -     regmap = syscon_node_to_regmap(syscon_np);
> >> >> >> >> -     of_node_put(syscon_np);
> >> >> >> >> +     syscon = dev_get_drvdata(dev);
> >> >> >> >>
> >> >> >> >> -     return regmap;
> >> >> >> >> +     return syscon->regmap;
> >> >> >> >>  }
> >> >> >> >>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
> >> >> >> >
> >> >> >> > Since you are not actually comparing the "compatible" property here,
> >> >> >> > I would suggest adding another function here,
> >> >> >>
> >> >> >> Yes, i also think like that.
> >> >> >
> >> >> > In this case we should provide two paths for drivers which can work as with DT
> >> >> > and without DT.
> >> >>
> >> >> Yes.
> >> >
> >> > I still think the universal procedure is better for the driver.
> >> >
> >>
> >> Why?
> >> I did not see your reply on my other comments on the problems of using universal
> >> procedure?
> >> Please let me know if you think they're not issues.
> >
> > Yes, I do not see a problem here.
> > I will try to show the code.
> >
> > In the driver:
> > struct regmap *r;
> > r = syscon_regmap_lookup_by_compatible("my_super_syscon");
> >
> > For DT case:
> > syscon@123456 {
> >   compatible = "my_super_syscon", "syscon";
> >   ...
> > };
> >
> > For non-DT case:
> > struct platform_device_id id = { .name = "my_super_syscon", };
> > struct platform_device syscon_pdev = {
> >   .name = "syscon",
> >   .id_entry = &id,
> 
> This is really strange to me and i've never seen such using.
> Usually the id_table is provided by the driver and the match process then will
> set the correct id_entry for the platform device once it matches.
> Please see the platform_bus match process: drivers/base/platform.c

Yes, this is non-standard usage. Currently we do not have platform_device_id
entries for a "syscon" driver, so this field is untouched for a platform device
and we can use it. At least this works, but, of course, more experts should
fix me on this question if I think incorrect.

> 
> >   ...
> > };
> > platform_device_register(&syscon_pdev);
> >
> > Do I understand what you mean?
> >
> 
> My understanding for non-dt case is something like:
> In client driver:
> struct regmap *r;
> r = syscon_regmap_lookup_by_pdevname("my_super_syscon");
> 
> In board code:
> struct platform_device syscon_pdev = {
> .name = "my_super_syscon",
> ...
> };
>  platform_device_register(&syscon_pdev);
> 
> In syscon driver:
> static struct platform_device_id syscon_device_ids[] = {
>         {
>                 .name = "my_super_syscon",
>         }, {
>                 /* sentinel */
>         }
> };
> MODULE_DEVICE_TABLE(platform, syscon_device_ids);
> 
> static struct platform_driver syscon_driver = {
>         .driver = {
>                 .name = "syscon",
>                 .owner = THIS_MODULE,
>                 .of_match_table = of_syscon_match,
>         },
>         .id_table       = syscon_device_ids,
>         .probe          = syscon_probe,
>         .remove         = syscon_remove,
> };
> 
> One problem is that every user needs to add their syscon compatible device
> support(platform_device_id) in syscon driver first before they can use it.
> But it looks to me just like how other driver generally does.

I agree, this is a more proper way, but in this case we should create a big table
here with records for each device...

> Arnd,
> Do you think this is an issue?

---
ÿôèº{.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[8]: [PATCH v3] mfd: syscon: Add non-DT support

[Dong Aisheng]

On 20 February 2013 13:41, Alexander Shiyan <shc_work@mail.ru> wrote:
>> > ...
>> >> >> >> >>  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
>> >> >> >> >>  {
>> >> >> >> >>       struct device_node *syscon_np;
>> >> >> >> >>       struct regmap *regmap;
>> >> >> >> >> +     struct syscon *syscon;
>> >> >> >> >> +     struct device *dev;
>> >> >> >> >>
>> >> >> >> >>       syscon_np = of_find_compatible_node(NULL, NULL, s);
>> >> >> >> >> -     if (!syscon_np)
>> >> >> >> >> +     if (syscon_np) {
>> >> >> >> >> +             regmap = syscon_node_to_regmap(syscon_np);
>> >> >> >> >> +             of_node_put(syscon_np);
>> >> >> >> >> +
>> >> >> >> >> +             return regmap;
>> >> >> >> >> +     }
>> >> >> >> >> +
>> >> >> >> >> +     /* Fallback to search by id_entry.name string */
>> >> >> >> >> +     dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
>> >> >> >> >> +                              syscon_match_id);
>> >> >> >> >> +     if (!dev)
>> >> >> >> >>               return ERR_PTR(-ENODEV);
>> >> >> >> >>
>> >> >> >> >> -     regmap = syscon_node_to_regmap(syscon_np);
>> >> >> >> >> -     of_node_put(syscon_np);
>> >> >> >> >> +     syscon = dev_get_drvdata(dev);
>> >> >> >> >>
>> >> >> >> >> -     return regmap;
>> >> >> >> >> +     return syscon->regmap;
>> >> >> >> >>  }
>> >> >> >> >>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
>> >> >> >> >
>> >> >> >> > Since you are not actually comparing the "compatible" property here,
>> >> >> >> > I would suggest adding another function here,
>> >> >> >>
>> >> >> >> Yes, i also think like that.
>> >> >> >
>> >> >> > In this case we should provide two paths for drivers which can work as with DT
>> >> >> > and without DT.
>> >> >>
>> >> >> Yes.
>> >> >
>> >> > I still think the universal procedure is better for the driver.
>> >> >
>> >>
>> >> Why?
>> >> I did not see your reply on my other comments on the problems of using universal
>> >> procedure?
>> >> Please let me know if you think they're not issues.
>> >
>> > Yes, I do not see a problem here.
>> > I will try to show the code.
>> >
>> > In the driver:
>> > struct regmap *r;
>> > r = syscon_regmap_lookup_by_compatible("my_super_syscon");
>> >
>> > For DT case:
>> > syscon@123456 {
>> >   compatible = "my_super_syscon", "syscon";
>> >   ...
>> > };
>> >
>> > For non-DT case:
>> > struct platform_device_id id = { .name = "my_super_syscon", };
>> > struct platform_device syscon_pdev = {
>> >   .name = "syscon",
>> >   .id_entry = &id,
>>
>> This is really strange to me and i've never seen such using.
>> Usually the id_table is provided by the driver and the match process then will
>> set the correct id_entry for the platform device once it matches.
>> Please see the platform_bus match process: drivers/base/platform.c
>
> Yes, this is non-standard usage. Currently we do not have platform_device_id
> entries for a "syscon" driver, so this field is untouched for a platform device
> and we can use it. At least this works, but, of course, more experts should
> fix me on this question if I think incorrect.
>

Hmm, i'm afraid most people may not accept this.
The platform_device_id are not desgined for such using, i guess.

>>
>> >   ...
>> > };
>> > platform_device_register(&syscon_pdev);
>> >
>> > Do I understand what you mean?
>> >
>>
>> My understanding for non-dt case is something like:
>> In client driver:
>> struct regmap *r;
>> r = syscon_regmap_lookup_by_pdevname("my_super_syscon");
>>
>> In board code:
>> struct platform_device syscon_pdev = {
>> .name = "my_super_syscon",
>> ...
>> };
>>  platform_device_register(&syscon_pdev);
>>
>> In syscon driver:
>> static struct platform_device_id syscon_device_ids[] = {
>>         {
>>                 .name = "my_super_syscon",
>>         }, {
>>                 /* sentinel */
>>         }
>> };
>> MODULE_DEVICE_TABLE(platform, syscon_device_ids);
>>
>> static struct platform_driver syscon_driver = {
>>         .driver = {
>>                 .name = "syscon",
>>                 .owner = THIS_MODULE,
>>                 .of_match_table = of_syscon_match,
>>         },
>>         .id_table       = syscon_device_ids,
>>         .probe          = syscon_probe,
>>         .remove         = syscon_remove,
>> };
>>
>> One problem is that every user needs to add their syscon compatible device
>> support(platform_device_id) in syscon driver first before they can use it.
>> But it looks to me just like how other driver generally does.
>
> I agree, this is a more proper way, but in this case we should create a big table
> here with records for each device...
>

Only non-dt needs add it which may not be so many and you're the first one.

Regards
Dong Aisheng

Re: Re[8]: [PATCH v3] mfd: syscon: Add non-DT support

[Arnd Bergmann]

On Wednesday 20 February 2013, Alexander Shiyan wrote:
> On 20 February 2013 13:41, Alexander Shiyan <shc_work@mail.ru> wrote:
> > 
> > One problem is that every user needs to add their syscon compatible device
> > support(platform_device_id) in syscon driver first before they can use it.
> > But it looks to me just like how other driver generally does.
> 
> I agree, this is a more proper way, but in this case we should create a big table
> here with records for each device...
> 
> > Arnd,
> > Do you think this is an issue?

I would first like to get an answer to the question I asked in my first mail,
which is what the use case of non-DT support in this driver is. If this
is used only by a new platform that has to use DT anyway, or by an existing
platform that is easy enough to convert, we probably shouldn't do all this
at all.

	Arnd

Re: Re[8]: [PATCH v3] mfd: syscon: Add non-DT support

[Dong Aisheng]

On 20 February 2013 18:06, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 20 February 2013, Alexander Shiyan wrote:
>> On 20 February 2013 13:41, Alexander Shiyan <shc_work@mail.ru> wrote:
>> >
>> > One problem is that every user needs to add their syscon compatible device
>> > support(platform_device_id) in syscon driver first before they can use it.
>> > But it looks to me just like how other driver generally does.
>>
>> I agree, this is a more proper way, but in this case we should create a big table
>> here with records for each device...
>>
>> > Arnd,
>> > Do you think this is an issue?
>
> I would first like to get an answer to the question I asked in my first mail,
> which is what the use case of non-DT support in this driver is. If this
> is used only by a new platform that has to use DT anyway, or by an existing
> platform that is easy enough to convert, we probably shouldn't do all this
> at all.
>

If the platform can convert to dt, then we do not have such issue.
The question is do we allow the existing non-dt platforms to use it
before converting?

Regards
Dong Aisheng

Re: Re[8]: [PATCH v3] mfd: syscon: Add non-DT support

[Arnd Bergmann]

On Wednesday 20 February 2013, Dong Aisheng wrote:
> On 20 February 2013 18:06, Arnd Bergmann <arnd@arndb.de> wrote:
> > I would first like to get an answer to the question I asked in my first mail,
> > which is what the use case of non-DT support in this driver is. If this
> > is used only by a new platform that has to use DT anyway, or by an existing
> > platform that is easy enough to convert, we probably shouldn't do all this
> > at all.
> >
> 
> If the platform can convert to dt, then we do not have such issue.
> The question is do we allow the existing non-dt platforms to use it
> before converting?

I think the answer to that is "it depends". It's basically a question of
how much work it would be to convert the platforms that need it over to
DT, and how much of the interface it actually needs. E.g. if there is
only one in-tree platform that needs to use syscon but can't easily be
moved over to DT, but that platform can only have a single syscon device,
then we don't need any of the matching support but could simply return
the first regmap area we have in the list.

Of course, if the platform in question is out of tree, I would argue
that the whatever patches are needed by that platform should also
remain out of tree.

	Arnd

Re: Re[8]: [PATCH v3] mfd: syscon: Add non-DT support

[Dong Aisheng]

On 20 February 2013 19:14, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 20 February 2013, Dong Aisheng wrote:
>> On 20 February 2013 18:06, Arnd Bergmann <arnd@arndb.de> wrote:
>> > I would first like to get an answer to the question I asked in my first mail,
>> > which is what the use case of non-DT support in this driver is. If this
>> > is used only by a new platform that has to use DT anyway, or by an existing
>> > platform that is easy enough to convert, we probably shouldn't do all this
>> > at all.
>> >
>>
>> If the platform can convert to dt, then we do not have such issue.
>> The question is do we allow the existing non-dt platforms to use it
>> before converting?
>
> I think the answer to that is "it depends". It's basically a question of
> how much work it would be to convert the platforms that need it over to
> DT, and how much of the interface it actually needs. E.g. if there is
> only one in-tree platform that needs to use syscon but can't easily be
> moved over to DT, but that platform can only have a single syscon device,
> then we don't need any of the matching support but could simply return
> the first regmap area we have in the list.
>
> Of course, if the platform in question is out of tree, I would argue
> that the whatever patches are needed by that platform should also
> remain out of tree.
>

Basically i agree with your point.
Alexander seems to be the first non-dt user of syscon driver.
He may answer whether they could choose to convert to dt first.
But one question i wonder is that it may be hard to know how many poteintial
non-dt platforms may use syscon.

Regards
Dong Aisheng

Re: Re[8]: [PATCH v3] mfd: syscon: Add non-DT support

[Arnd Bergmann]

On Wednesday 20 February 2013, Dong Aisheng wrote:
> But one question i wonder is that it may be hard to know how many poteintial
> non-dt platforms may use syscon.

Yes, good point. Of course we want as many platforms as possible to
use syscon, just like we want them to use DT, because both things
are generally useful concepts, but they are not necessarily tied
together.

	Arnd

Re[10]: [PATCH v3] 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: 2264 bytes --]

> On 20 February 2013 19:14, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 20 February 2013, Dong Aisheng wrote:
> >> On 20 February 2013 18:06, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > I would first like to get an answer to the question I asked in my first mail,
> >> > which is what the use case of non-DT support in this driver is. If this
> >> > is used only by a new platform that has to use DT anyway, or by an existing
> >> > platform that is easy enough to convert, we probably shouldn't do all this
> >> > at all.
> >> >
> >>
> >> If the platform can convert to dt, then we do not have such issue.
> >> The question is do we allow the existing non-dt platforms to use it
> >> before converting?
> >
> > I think the answer to that is "it depends". It's basically a question of
> > how much work it would be to convert the platforms that need it over to
> > DT, and how much of the interface it actually needs. E.g. if there is
> > only one in-tree platform that needs to use syscon but can't easily be
> > moved over to DT, but that platform can only have a single syscon device,
> > then we don't need any of the matching support but could simply return
> > the first regmap area we have in the list.
> >
> > Of course, if the platform in question is out of tree, I would argue
> > that the whatever patches are needed by that platform should also
> > remain out of tree.
> >
> 
> Basically i agree with your point.
> Alexander seems to be the first non-dt user of syscon driver.
> He may answer whether they could choose to convert to dt first.
> But one question i wonder is that it may be hard to know how many poteintial
> non-dt platforms may use syscon.

OK. I can convert platform to DT, no so easy, but possible.
But I will use syscon as way to using DT (and MULTIPLATFORM in the future),
this mean that I should completely drop ATAG support from this platform
(since I cannot use syscon device without DT support, but several platform devices
need to use system-wide registers).
Arnd, if its OK for you, I will use this way. (I talking about CLPS711X, you know it :) ).

---
ÿôèº{.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[10]: [PATCH v3] mfd: syscon: Add non-DT support

[Arnd Bergmann]

On Wednesday 20 February 2013, Alexander Shiyan wrote:
> OK. I can convert platform to DT, no so easy, but possible.
> But I will use syscon as way to using DT (and MULTIPLATFORM in the future),
> this mean that I should completely drop ATAG support from this platform
> (since I cannot use syscon device without DT support, but several platform devices
> need to use system-wide registers).
> Arnd, if its OK for you, I will use this way. (I talking about CLPS711X, you know it :) ).

Ah, I should have realized that this is about clps711x when the patch
is coming from you ;-)

This is an existing platform, and I would not require you to move it to
devicetree just for supporting syscon, although it may be a good idea
to make that move in the long run.

For clps711x, we know that there is only one syscon register set, so you
could do a very simple patch like below. Basically we don't need to
treat the absence of DT information as an error, and a call to
syscon_regmap_lookup_by_compatible or syscon_regmap_lookup_by_phandle
will always return the syscon device that was registered first, or
-EPROBE_DEFER for any error.

	Arnd


diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 3f10591..d3c06c6 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -59,9 +59,6 @@ struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
 	struct regmap *regmap;
 
 	syscon_np = of_find_compatible_node(NULL, NULL, s);
-	if (!syscon_np)
-		return ERR_PTR(-ENODEV);
-
 	regmap = syscon_node_to_regmap(syscon_np);
 	of_node_put(syscon_np);
 
@@ -76,9 +73,6 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
 	struct regmap *regmap;
 
 	syscon_np = of_parse_phandle(np, property, 0);
-	if (!syscon_np)
-		return ERR_PTR(-ENODEV);
-
 	regmap = syscon_node_to_regmap(syscon_np);
 	of_node_put(syscon_np);
 
@@ -100,26 +94,22 @@ 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;
+	struct resource *res;
 	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)
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
 		return -EADDRNOTAVAIL;
 
-	ret = of_address_to_resource(np, 0, &res);
-	if (ret)
-		return ret;
+	syscon->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!syscon->base)
+		return -ENOMEM;
 
 	syscon_regmap_config.max_register = res.end - res.start - 3;
 	syscon->regmap = devm_regmap_init_mmio(dev, syscon->base,

Re[12]: [PATCH v3] 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: 1864 bytes --]

> > OK. I can convert platform to DT, no so easy, but possible.
> > But I will use syscon as way to using DT (and MULTIPLATFORM in the future),
> > this mean that I should completely drop ATAG support from this platform
> > (since I cannot use syscon device without DT support, but several platform devices
> > need to use system-wide registers).
> > Arnd, if its OK for you, I will use this way. (I talking about CLPS711X, you know it :) ).
> 
> Ah, I should have realized that this is about clps711x when the patch
> is coming from you ;-)
> 
> This is an existing platform, and I would not require you to move it to
> devicetree just for supporting syscon, although it may be a good idea
> to make that move in the long run.
> 
> For clps711x, we know that there is only one syscon register set, so you
> could do a very simple patch like below. Basically we don't need to

No. Target have a three SYSCON registers and two SYSFLG. All these registers
can be combined into three syscon devices.
Only these registers will be handled via syscon device, so it is not only one.
Or you mean about handle all register via syscon? It is not it.

> treat the absence of DT information as an error, and a call to
> syscon_regmap_lookup_by_compatible or syscon_regmap_lookup_by_phandle
> will always return the syscon device that was registered first, or
> -EPROBE_DEFER for any error.

The initial idea is search desired syscon device from drivers only by one function
(i.e. search syscon device by compatible string or by specific alias) and no depend
on DT or non-DT. I.e. define syscon device always at machine start (even if we run
machine from DTS), because device should be always present in system.

---
ÿôèº{.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[12]: [PATCH v3] mfd: syscon: Add non-DT support

[Arnd Bergmann]

On Wednesday 20 February 2013, Alexander Shiyan wrote:
> No. Target have a three SYSCON registers and two SYSFLG. All these registers
> can be combined into three syscon devices.
> Only these registers will be handled via syscon device, so it is not only one.
> Or you mean about handle all register via syscon? It is not it.

Yes, I was expecting that you would list all three pages in the resource
for the syscon device, basically making all of the core clps711x
registers available this way.

> > treat the absence of DT information as an error, and a call to
> > syscon_regmap_lookup_by_compatible or syscon_regmap_lookup_by_phandle
> > will always return the syscon device that was registered first, or
> > -EPROBE_DEFER for any error.
> 
> The initial idea is search desired syscon device from drivers only by one function
> (i.e. search syscon device by compatible string or by specific alias) and no depend
> on DT or non-DT. I.e. define syscon device always at machine start (even if we run
> machine from DTS), because device should be always present in system.

I don't understand yet what the advantage for clps711x is over just a single
register area that would get registered at boot time and replace all the
clps_readl/clps_writel calls.

	Arnd

Re[14]: [PATCH v3] 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: 1936 bytes --]

> On Wednesday 20 February 2013, Alexander Shiyan wrote:
> > No. Target have a three SYSCON registers and two SYSFLG. All these registers
> > can be combined into three syscon devices.
> > Only these registers will be handled via syscon device, so it is not only one.
> > Or you mean about handle all register via syscon? It is not it.
> 
> Yes, I was expecting that you would list all three pages in the resource
> for the syscon device, basically making all of the core clps711x
> registers available this way.

All other will be passed as resource to drivers, as for other drivers.
And this change replaces clps_read/write.

> > > treat the absence of DT information as an error, and a call to
> > > syscon_regmap_lookup_by_compatible or syscon_regmap_lookup_by_phandle
> > > will always return the syscon device that was registered first, or
> > > -EPROBE_DEFER for any error.
> > 
> > The initial idea is search desired syscon device from drivers only by one function
> > (i.e. search syscon device by compatible string or by specific alias) and no depend
> > on DT or non-DT. I.e. define syscon device always at machine start (even if we run
> > machine from DTS), because device should be always present in system.
> 
> I don't understand yet what the advantage for clps711x is over just a single
> register area that would get registered at boot time and replace all the
> clps_readl/clps_writel calls.

This cause a serious perfomance impact. Only SYSCON and SYSFLG is used
in several places and should be protected by spinlocks. Other registers
can be used without locks. And, as say before, clps_read/write will be replaced with
read/write when registers will passed as resource. First example of this change I
sent to you before (patchset for serial driver).

---
ÿôèº{.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[14]: [PATCH v3] mfd: syscon: Add non-DT support

[Arnd Bergmann]

On Wednesday 20 February 2013, Alexander Shiyan wrote:
> > On Wednesday 20 February 2013, Alexander Shiyan wrote:
> > > No. Target have a three SYSCON registers and two SYSFLG. All these registers
> > > can be combined into three syscon devices.
> > > Only these registers will be handled via syscon device, so it is not only one.
> > > Or you mean about handle all register via syscon? It is not it.
> > 
> > Yes, I was expecting that you would list all three pages in the resource
> > for the syscon device, basically making all of the core clps711x
> > registers available this way.
> 
> All other will be passed as resource to drivers, as for other drivers.
> And this change replaces clps_read/write.

Ok, I see.

> > > > treat the absence of DT information as an error, and a call to
> > > > syscon_regmap_lookup_by_compatible or syscon_regmap_lookup_by_phandle
> > > > will always return the syscon device that was registered first, or
> > > > -EPROBE_DEFER for any error.
> > > 
> > > The initial idea is search desired syscon device from drivers only by one function
> > > (i.e. search syscon device by compatible string or by specific alias) and no depend
> > > on DT or non-DT. I.e. define syscon device always at machine start (even if we run
> > > machine from DTS), because device should be always present in system.
> > 
> > I don't understand yet what the advantage for clps711x is over just a single
> > register area that would get registered at boot time and replace all the
> > clps_readl/clps_writel calls.
> 
> This cause a serious perfomance impact. Only SYSCON and SYSFLG is used
> in several places and should be protected by spinlocks. Other registers
> can be used without locks. And, as say before, clps_read/write will be replaced with
> read/write when registers will passed as resource. First example of this change I
> sent to you before (patchset for serial driver).

Yes, that makes sense. I have no fundamental objections then. I'll wait
for the next version of your patch and then comment on any details I still
find sticking out.

	Arnd

Re[16]: [PATCH v3] 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: 1859 bytes --]

...
> > > > > treat the absence of DT information as an error, and a call to
> > > > > syscon_regmap_lookup_by_compatible or syscon_regmap_lookup_by_phandle
> > > > > will always return the syscon device that was registered first, or
> > > > > -EPROBE_DEFER for any error.
> > > > 
> > > > The initial idea is search desired syscon device from drivers only by one function
> > > > (i.e. search syscon device by compatible string or by specific alias) and no depend
> > > > on DT or non-DT. I.e. define syscon device always at machine start (even if we run
> > > > machine from DTS), because device should be always present in system.
> > > 
> > > I don't understand yet what the advantage for clps711x is over just a single
> > > register area that would get registered at boot time and replace all the
> > > clps_readl/clps_writel calls.
> > 
> > This cause a serious perfomance impact. Only SYSCON and SYSFLG is used
> > in several places and should be protected by spinlocks. Other registers
> > can be used without locks. And, as say before, clps_read/write will be replaced with
> > read/write when registers will passed as resource. First example of this change I
> > sent to you before (patchset for serial driver).
> 
> Yes, that makes sense. I have no fundamental objections then. I'll wait
> for the next version of your patch and then comment on any details I still
> find sticking out.

OK. Next version will be sent in few minutes.
I am leave search by id_entry and extend it with variant from first patch version.
This will allow to search syscon device with id_entry record in the driver, and
without last one by specifying generic name with id (for example "syscon.0").
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¥