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

From: Alexander Shiyan
Date: Mon Mar 11 2013 - 08:30:46 EST


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

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

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

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

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

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

> this looks ok to me.
>
> So, besides above minor comments,
> Acked-by: Dong Aisheng <dong.aisheng@xxxxxxxxxx>
>
> 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‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ 0¶ìh®å’i