2013-03-11 09:55:53

by Dong Aisheng

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

On Thu, Feb 28, 2013 at 06:57:14PM +0400, Alexander Shiyan wrote:
> This patch allow using syscon driver from the platform data, i.e.
> possibility using driver on systems without oftree support.
> For search syscon device from the client drivers,
> "syscon_regmap_lookup_by_pdevname" function was added.
>
> Signed-off-by: Alexander Shiyan <[email protected]>
> ---
> drivers/mfd/Kconfig | 1 -
> drivers/mfd/syscon.c | 77 +++++++++++++++++++++++++++-------------------
> include/linux/mfd/syscon.h | 1 +
> 3 files changed, 47 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 671f5b1..8fdd87e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1070,7 +1070,6 @@ config MFD_STA2X11
>
> config MFD_SYSCON
> bool "System Controller Register R/W Based on Regmap"
> - depends on OF
> select REGMAP_MMIO
> help
> Select this option to enable accessing system control registers
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 674af14..e5b6be1 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -29,7 +29,7 @@ struct syscon {
> struct regmap *regmap;
> };
>
> -static int syscon_match(struct device *dev, void *data)
> +static int syscon_match_node(struct device *dev, void *data)
> {
> struct device_node *dn = data;
>
> @@ -42,7 +42,7 @@ struct regmap *syscon_node_to_regmap(struct device_node *np)
> struct device *dev;
>
> dev = driver_find_device(&syscon_driver.driver, NULL, np,
> - syscon_match);
> + syscon_match_node);
> if (!dev)
> return ERR_PTR(-EPROBE_DEFER);
>
> @@ -68,6 +68,34 @@ struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
> }
> EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
>
> +static int syscon_match_pdevname(struct device *dev, void *data)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + const struct platform_device_id *id = platform_get_device_id(pdev);
> +
> + if (id)
> + if (!strcmp(id->name, (const char *)data))
> + return 1;
> +
> + return !strcmp(dev_name(dev), (const char *)data);
> +}
> +
> +struct regmap *syscon_regmap_lookup_by_pdevname(const char *s)
> +{
> + struct device *dev;
> + struct syscon *syscon;
> +
> + dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> + syscon_match_pdevname);
> + if (!dev)
> + return ERR_PTR(-ENODEV);

Should it be ERR_PTR(-EPROBE_DEFER)?

> +
> + syscon = dev_get_drvdata(dev);
> +
> + return syscon->regmap;
> +}
> +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname);
> +
> struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
> const char *property)
> {
> @@ -99,30 +127,24 @@ static struct regmap_config syscon_regmap_config = {
> static int syscon_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - struct device_node *np = dev->of_node;
> struct syscon *syscon;
> - struct resource res;
> - int ret;
> -
> - if (!np)
> - return -ENOENT;
> + struct resource *res;
>
> - syscon = devm_kzalloc(dev, sizeof(struct syscon),
> - GFP_KERNEL);
> + syscon = devm_kzalloc(dev, sizeof(struct syscon), GFP_KERNEL);
> if (!syscon)
> return -ENOMEM;
>
> - syscon->base = of_iomap(np, 0);
> - if (!syscon->base)
> - return -EADDRNOTAVAIL;
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENOENT;
>
> - ret = of_address_to_resource(np, 0, &res);
> - if (ret)
> - return ret;
> + syscon->base = devm_ioremap(dev, res->start, resource_size(res));
> + if (!syscon->base)
> + return -ENOMEM;
>
> - syscon_regmap_config.max_register = res.end - res.start - 3;
> + syscon_regmap_config.max_register = res->end - res->start - 3;
> syscon->regmap = devm_regmap_init_mmio(dev, syscon->base,
> - &syscon_regmap_config);
> + &syscon_regmap_config);

Unneeded change to me

> if (IS_ERR(syscon->regmap)) {
> dev_err(dev, "regmap init failed\n");
> return PTR_ERR(syscon->regmap);
> @@ -130,22 +152,15 @@ static int syscon_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, syscon);
>
> - dev_info(dev, "syscon regmap start 0x%x end 0x%x registered\n",
> - res.start, res.end);
> + dev_info(dev, "regmap 0x%x-0x%x registered\n", res->start, res->end);

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

>
> return 0;
> }
>
> -static int syscon_remove(struct platform_device *pdev)
> -{
> - struct syscon *syscon;
> -
> - syscon = platform_get_drvdata(pdev);
> - iounmap(syscon->base);
> - platform_set_drvdata(pdev, NULL);
> -
> - return 0;
> -}
> +static const struct platform_device_id syscon_ids[] = {
> + { "syscon", },
> + { }
> +};
>
> static struct platform_driver syscon_driver = {
> .driver = {
> @@ -154,7 +169,7 @@ static struct platform_driver syscon_driver = {
> .of_match_table = of_syscon_match,
> },
> .probe = syscon_probe,
> - .remove = syscon_remove,

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

So, besides above minor comments,
Acked-by: Dong Aisheng <[email protected]>

Regards
Dong Aisheng

> + .id_table = syscon_ids,
> };
>
> static int __init syscon_init(void)
> diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
> index 6aeb6b8..5c9ee6e 100644
> --- a/include/linux/mfd/syscon.h
> +++ b/include/linux/mfd/syscon.h
> @@ -17,6 +17,7 @@
>
> extern struct regmap *syscon_node_to_regmap(struct device_node *np);
> extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s);
> +extern struct regmap *syscon_regmap_lookup_by_pdevname(const char *s);
> extern struct regmap *syscon_regmap_lookup_by_phandle(
> struct device_node *np,
> const char *property);
> --
> 1.7.12.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>


2013-03-11 10:36:22

by Dong Aisheng

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

On Mon, Mar 11, 2013 at 02:16:44PM +0400, Alexander Shiyan wrote:
> > On Thu, Feb 28, 2013 at 06:57:14PM +0400, Alexander Shiyan wrote:
> > > This patch allow using syscon driver from the platform data, i.e.
> > > possibility using driver on systems without oftree support.
> > > For search syscon device from the client drivers,
> > > "syscon_regmap_lookup_by_pdevname" function was added.
> > >
> > > Signed-off-by: Alexander Shiyan <[email protected]>
> > > ---
> > > drivers/mfd/Kconfig | 1 -
> > > drivers/mfd/syscon.c | 77 +++++++++++++++++++++++++++-------------------
> > > include/linux/mfd/syscon.h | 1 +
> > > 3 files changed, 47 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 671f5b1..8fdd87e 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1070,7 +1070,6 @@ config MFD_STA2X11
> > >
> > > config MFD_SYSCON
> > > bool "System Controller Register R/W Based on Regmap"
> > > - depends on OF
> > > select REGMAP_MMIO
> > > help
> > > Select this option to enable accessing system control registers
> > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > > index 674af14..e5b6be1 100644
> > > --- a/drivers/mfd/syscon.c
> > > +++ b/drivers/mfd/syscon.c
> > > @@ -29,7 +29,7 @@ struct syscon {
> > > struct regmap *regmap;
> > > };
> > >
> > > -static int syscon_match(struct device *dev, void *data)
> > > +static int syscon_match_node(struct device *dev, void *data)
> > > {
> > > struct device_node *dn = data;
> > >
> > > @@ -42,7 +42,7 @@ struct regmap *syscon_node_to_regmap(struct device_node *np)
> > > struct device *dev;
> > >
> > > dev = driver_find_device(&syscon_driver.driver, NULL, np,
> > > - syscon_match);
> > > + syscon_match_node);
> > > if (!dev)
> > > return ERR_PTR(-EPROBE_DEFER);
> > >
> > > @@ -68,6 +68,34 @@ struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
> > > }
> > > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
> > >
> > > +static int syscon_match_pdevname(struct device *dev, void *data)
> > > +{
> > > + struct platform_device *pdev = to_platform_device(dev);
> > > + const struct platform_device_id *id = platform_get_device_id(pdev);
> > > +
> > > + if (id)
> > > + if (!strcmp(id->name, (const char *)data))
> > > + return 1;
> > > +
> > > + return !strcmp(dev_name(dev), (const char *)data);
> > > +}
> > > +
> > > +struct regmap *syscon_regmap_lookup_by_pdevname(const char *s)
> > > +{
> > > + struct device *dev;
> > > + struct syscon *syscon;
> > > +
> > > + dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> > > + syscon_match_pdevname);
> > > + if (!dev)
> > > + return ERR_PTR(-ENODEV);
> >
> > Should it be ERR_PTR(-EPROBE_DEFER)?
>
> I have no idea what better using here. Think that is not so important
> since we may have only one possible error code here.

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

>
> > > +
> > > + syscon = dev_get_drvdata(dev);
> > > +
> > > + return syscon->regmap;
> > > +}
> > > +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname);
> > > +
> > > struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
> > > const char *property)
> > > {
> > > @@ -99,30 +127,24 @@ static struct regmap_config syscon_regmap_config = {
> > > static int syscon_probe(struct platform_device *pdev)
> > > {
> > > struct device *dev = &pdev->dev;
> > > - struct device_node *np = dev->of_node;
> > > struct syscon *syscon;
> > > - struct resource res;
> > > - int ret;
> > > -
> > > - if (!np)
> > > - return -ENOENT;
> > > + struct resource *res;
> > >
> > > - syscon = devm_kzalloc(dev, sizeof(struct syscon),
> > > - GFP_KERNEL);
> > > + syscon = devm_kzalloc(dev, sizeof(struct syscon), GFP_KERNEL);
> > > if (!syscon)
> > > return -ENOMEM;
> > >
> > > - syscon->base = of_iomap(np, 0);
> > > - if (!syscon->base)
> > > - return -EADDRNOTAVAIL;
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + if (!res)
> > > + return -ENOENT;
> > >
> > > - ret = of_address_to_resource(np, 0, &res);
> > > - if (ret)
> > > - return ret;
> > > + syscon->base = devm_ioremap(dev, res->start, resource_size(res));
> > > + if (!syscon->base)
> > > + return -ENOMEM;
> > >
> > > - syscon_regmap_config.max_register = res.end - res.start - 3;
> > > + syscon_regmap_config.max_register = res->end - res->start - 3;
> > > syscon->regmap = devm_regmap_init_mmio(dev, syscon->base,
> > > - &syscon_regmap_config);
> > > + &syscon_regmap_config);
> >
> > Unneeded change to me
> >
> > > if (IS_ERR(syscon->regmap)) {
> > > dev_err(dev, "regmap init failed\n");
> > > return PTR_ERR(syscon->regmap);
> > > @@ -130,22 +152,15 @@ static int syscon_probe(struct platform_device *pdev)
> > >
> > > platform_set_drvdata(pdev, syscon);
> > >
> > > - dev_info(dev, "syscon regmap start 0x%x end 0x%x registered\n",
> > > - res.start, res.end);
> > > + dev_info(dev, "regmap 0x%x-0x%x registered\n", res->start, res->end);
> >
> > Doesn't with a 'syscon' prefix look better?
>
> Prefix is added by dev_info, so no need to print it twice.

Reasonable to me.

Regards
Dong Aisheng

> > >
> > > return 0;
> > > }
> > >
> > > -static int syscon_remove(struct platform_device *pdev)
> > > -{
> > > - struct syscon *syscon;
> > > -
> > > - syscon = platform_get_drvdata(pdev);
> > > - iounmap(syscon->base);
> > > - platform_set_drvdata(pdev, NULL);
> > > -
> > > - return 0;
> > > -}
> > > +static const struct platform_device_id syscon_ids[] = {
> > > + { "syscon", },
> > > + { }
> > > +};
> > >
> > > static struct platform_driver syscon_driver = {
> > > .driver = {
> > > @@ -154,7 +169,7 @@ static struct platform_driver syscon_driver = {
> > > .of_match_table = of_syscon_match,
> > > },
> > > .probe = syscon_probe,
> > > - .remove = syscon_remove,
> >
> > So you reply on the managed resource to address the resource leak issue
> > raised by Dmitry on doing manually unbind here, right?
>
> Yes. We are killing "remove"-entry since all our resources are "managed" now.
>
> > this looks ok to me.
> >
> > So, besides above minor comments,
> > Acked-by: Dong Aisheng <[email protected]>
> >
> > Regards
> > Dong Aisheng
> >
> > > + .id_table = syscon_ids,
> > > };
> > >
> > > static int __init syscon_init(void)
> > > diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
> > > index 6aeb6b8..5c9ee6e 100644
> > > --- a/include/linux/mfd/syscon.h
> > > +++ b/include/linux/mfd/syscon.h
> > > @@ -17,6 +17,7 @@
> > >
> > > extern struct regmap *syscon_node_to_regmap(struct device_node *np);
> > > extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s);
> > > +extern struct regmap *syscon_regmap_lookup_by_pdevname(const char *s);
> > > extern struct regmap *syscon_regmap_lookup_by_phandle(
> > > struct device_node *np,
> > > const char *property);
> > > --
> > > 1.7.12.4
>
> ---

2013-03-11 12:30:43

by Alexander Shiyan

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

> On Thu, Feb 28, 2013 at 06:57:14PM +0400, Alexander Shiyan wrote:
> > This patch allow using syscon driver from the platform data, i.e.
> > possibility using driver on systems without oftree support.
> > For search syscon device from the client drivers,
> > "syscon_regmap_lookup_by_pdevname" function was added.
> >
> > Signed-off-by: Alexander Shiyan <[email protected]>
> > ---
> > drivers/mfd/Kconfig | 1 -
> > drivers/mfd/syscon.c | 77 +++++++++++++++++++++++++++-------------------
> > include/linux/mfd/syscon.h | 1 +
> > 3 files changed, 47 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 671f5b1..8fdd87e 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1070,7 +1070,6 @@ config MFD_STA2X11
> >
> > config MFD_SYSCON
> > bool "System Controller Register R/W Based on Regmap"
> > - depends on OF
> > select REGMAP_MMIO
> > help
> > Select this option to enable accessing system control registers
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index 674af14..e5b6be1 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -29,7 +29,7 @@ struct syscon {
> > struct regmap *regmap;
> > };
> >
> > -static int syscon_match(struct device *dev, void *data)
> > +static int syscon_match_node(struct device *dev, void *data)
> > {
> > struct device_node *dn = data;
> >
> > @@ -42,7 +42,7 @@ struct regmap *syscon_node_to_regmap(struct device_node *np)
> > struct device *dev;
> >
> > dev = driver_find_device(&syscon_driver.driver, NULL, np,
> > - syscon_match);
> > + syscon_match_node);
> > if (!dev)
> > return ERR_PTR(-EPROBE_DEFER);
> >
> > @@ -68,6 +68,34 @@ struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
> > }
> > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
> >
> > +static int syscon_match_pdevname(struct device *dev, void *data)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + const struct platform_device_id *id = platform_get_device_id(pdev);
> > +
> > + if (id)
> > + if (!strcmp(id->name, (const char *)data))
> > + return 1;
> > +
> > + return !strcmp(dev_name(dev), (const char *)data);
> > +}
> > +
> > +struct regmap *syscon_regmap_lookup_by_pdevname(const char *s)
> > +{
> > + struct device *dev;
> > + struct syscon *syscon;
> > +
> > + dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> > + syscon_match_pdevname);
> > + if (!dev)
> > + return ERR_PTR(-ENODEV);
>
> Should it be ERR_PTR(-EPROBE_DEFER)?

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

> > +
> > + syscon = dev_get_drvdata(dev);
> > +
> > + return syscon->regmap;
> > +}
> > +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname);
> > +
> > struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
> > const char *property)
> > {
> > @@ -99,30 +127,24 @@ static struct regmap_config syscon_regmap_config = {
> > static int syscon_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > - struct device_node *np = dev->of_node;
> > struct syscon *syscon;
> > - struct resource res;
> > - int ret;
> > -
> > - if (!np)
> > - return -ENOENT;
> > + struct resource *res;
> >
> > - syscon = devm_kzalloc(dev, sizeof(struct syscon),
> > - GFP_KERNEL);
> > + syscon = devm_kzalloc(dev, sizeof(struct syscon), GFP_KERNEL);
> > if (!syscon)
> > return -ENOMEM;
> >
> > - syscon->base = of_iomap(np, 0);
> > - if (!syscon->base)
> > - return -EADDRNOTAVAIL;
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -ENOENT;
> >
> > - ret = of_address_to_resource(np, 0, &res);
> > - if (ret)
> > - return ret;
> > + syscon->base = devm_ioremap(dev, res->start, resource_size(res));
> > + if (!syscon->base)
> > + return -ENOMEM;
> >
> > - syscon_regmap_config.max_register = res.end - res.start - 3;
> > + syscon_regmap_config.max_register = res->end - res->start - 3;
> > syscon->regmap = devm_regmap_init_mmio(dev, syscon->base,
> > - &syscon_regmap_config);
> > + &syscon_regmap_config);
>
> Unneeded change to me
>
> > if (IS_ERR(syscon->regmap)) {
> > dev_err(dev, "regmap init failed\n");
> > return PTR_ERR(syscon->regmap);
> > @@ -130,22 +152,15 @@ static int syscon_probe(struct platform_device *pdev)
> >
> > platform_set_drvdata(pdev, syscon);
> >
> > - dev_info(dev, "syscon regmap start 0x%x end 0x%x registered\n",
> > - res.start, res.end);
> > + dev_info(dev, "regmap 0x%x-0x%x registered\n", res->start, res->end);
>
> Doesn't with a 'syscon' prefix look better?

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

> >
> > return 0;
> > }
> >
> > -static int syscon_remove(struct platform_device *pdev)
> > -{
> > - struct syscon *syscon;
> > -
> > - syscon = platform_get_drvdata(pdev);
> > - iounmap(syscon->base);
> > - platform_set_drvdata(pdev, NULL);
> > -
> > - return 0;
> > -}
> > +static const struct platform_device_id syscon_ids[] = {
> > + { "syscon", },
> > + { }
> > +};
> >
> > static struct platform_driver syscon_driver = {
> > .driver = {
> > @@ -154,7 +169,7 @@ static struct platform_driver syscon_driver = {
> > .of_match_table = of_syscon_match,
> > },
> > .probe = syscon_probe,
> > - .remove = syscon_remove,
>
> So you reply on the managed resource to address the resource leak issue
> raised by Dmitry on doing manually unbind here, right?

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

> this looks ok to me.
>
> So, besides above minor comments,
> Acked-by: Dong Aisheng <[email protected]>
>
> Regards
> Dong Aisheng
>
> > + .id_table = syscon_ids,
> > };
> >
> > static int __init syscon_init(void)
> > diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
> > index 6aeb6b8..5c9ee6e 100644
> > --- a/include/linux/mfd/syscon.h
> > +++ b/include/linux/mfd/syscon.h
> > @@ -17,6 +17,7 @@
> >
> > extern struct regmap *syscon_node_to_regmap(struct device_node *np);
> > extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s);
> > +extern struct regmap *syscon_regmap_lookup_by_pdevname(const char *s);
> > extern struct regmap *syscon_regmap_lookup_by_phandle(
> > struct device_node *np,
> > const char *property);
> > --
> > 1.7.12.4

---
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2013-03-11 13:39:13

by Arnd Bergmann

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

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

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

Arnd

2013-03-11 16:17:29

by Alexander Shiyan

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

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

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

---
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?